New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Feature] Introduce TRANSACTION_SPEND_LIMIT
.
#2368
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
0072b11
Add TRANSACTION_SPEND_LIMIT
d0cd f3c0ca5
Move cost to process crate
d0cd a76ad79
Add check for TRANSACTION_SPEND_LIMIT
d0cd 963d1fe
Add test
d0cd 5edfada
Move test
d0cd 7be7995
Cleanup
d0cd 1c9e77a
Addres feedback; clippy; fmt
d0cd afd30e3
Merge branch 'mainnet' into feat/transaction-spend-limit
d0cd File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,9 +15,6 @@ | |
pub(crate) mod committee; | ||
pub use committee::*; | ||
|
||
mod cost; | ||
pub use cost::*; | ||
|
||
mod macros; | ||
|
||
mod rewards; | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iamalwaysuncomfortable likely has better intuition on how to back into this number with more data.
Even a high level understanding of how many hashes, standard operations, etc. it takes to hit this limit will be valuable for developers. Also would be nice to know that we aren't setting a number too conservatively that blocks program expressivity and usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This definitely seems a little overly conservative. Thus far about 200k credit spends (which is 3 orders of magnitude higher than this) seem to be where the network has issues, but it deserves further testing with other opcodes to see where this lies.
Let me run some extra tests that try 1k & 10k and 100k spends respectively to see if we can get solid measure of where the boundary of performance issues lies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Janky analysis incoming.
I think the two main considerations for a limitation on finalize are:
1. prevent preposterous blocktimes.
The performance implications of large finalize blocks are simpler than deployments: they run serially and single threaded and only in speculate.
So if the entire expected public monthly allocation of 35_000 is spent, we get a block of up to 30 minutes and afterwards we're cool. This doesn't sound too bad.
To be conservative, a maximum runtime of 7.5 seconds for a finalize block limits how much a single actor can cause a big blocktime spike. Which would bring us to a rounded limit of around 100 credits (or 100M microcredits).
2. do not hurt expressivity too much.
Expressivity of on-chain programs is already extremely limited because of the deployment limit. So we can easily decide to lower it to:
hash.BHP [[[u8; 16u32]; 16u32]; 8u32]
or 5000 simple opcodeshash.BHP [[[u8; 16u32]; 16u32]; 8u32]
or 500 simple opcodesI vote for
10M microcredits
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention of the finalize block is not to have Ethereum style program execution, but rather to allow for applications that require some degree of peer-to-peer shared resources to be able to settle in limited fashion.
The purpose of this limit is to serve as a forcing function to move as much computation that is possible off-chain so that only requisite state is passed through to the finalize scope on-chain.
In terms of my response to the analysis above:
1. prevent preposterous block times
That sounds incredibly bad. Being able to repeatedly execute a program that can slow down block times to 30 minutes is a bug, not a feature. One of the purposes of the current limit is to prevent this exact case.
2. do not hurt expressivity too much
I am fine with 10M microcredits, but I think 100M microcredits in its current form is adequate.
As a side remark: this is a good example for the importance of setting limits now rather than later. It will be much hard to introduce limits that are well-intentioned after the fact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vicsn We need to update snarkVM pricing for these cases. Any execution that takes more than 1 second needs to be priced in accordingly to prevent this exact case.