-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Bound parallel verification of transactions #2387
Conversation
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.
LGTM 👍
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.
Works like a charm
Signed-off-by: Raymond Chu <14917648+raychu86@users.noreply.github.com>
@vicsn Since the |
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.
LGTM, just two questions
synthesizer/src/vm/verify.rs
Outdated
)); | ||
} | ||
// Verify the transaction. | ||
match self.check_transaction(transaction, None, &mut rng) { |
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.
I should have noticed this earlier, but be aware: if a malicious validator inserts a deployment into their Batch Proposal with a malformed verifying key, this thread may panic, causing all transmissions to be sent back into the mempool queue for verification.
The only way I see us preventing this is to perform validation on transactions of validator Batch Proposals before signing them.
I will make sure we have a consensus test case for this to reproduce.
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.
Good point, we should wrap the call with a handle_halting
macro, like we do for other functions that have the capability of panicking.
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.
I added panic catchers in check_transaction
and updated the test - fbe604d
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.
LGTM for real now
Signed-off-by: Raymond Chu <14917648+raychu86@users.noreply.github.com>
Motivation
This PR bounds the memory usage during transactions verification by introducing
MAX_PARALLEL_DEPLOY_VERIFICATIONS
andMAX_PARALLEL_EXECUTE_VERIFICATIONS
, where we verify a limited number of transactions at once instead of all at once. Previously the unbounded verification could lead to OOM errors when a node is forced to verify many large/expensive transactions at once.Profiling memory usage of verification gives us some insights -
https://github.com/AleoHQ/snarkVM/pull/2376#discussion_r1509093306
For an upper bound of 10GB of memory dedicated to transaction verification, we set the following values to:
MAX_PARALLEL_DEPLOY_VERIFICATIONS
: 5MAX_PARALLEL_EXECUTE_VERIFICATIONS
: 1000This means we verify deployments first (in chunks of 5), then executions after (in chunks of 1000).