-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Limit amount of transactions which clients verify in parallel #2970
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 👍
use tokio::task::JoinHandle; | ||
use tokio::{task::JoinHandle, time::sleep}; | ||
|
||
const VERIFICATION_CONCURRENCY_LIMIT: usize = 6; // 8 deployments of MAX_NUM_CONSTRAINTS will run out of memory. |
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.
Should this be dynamic based on your machine?
How much memory was this estimation based on?
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 was based on the Clients: 16GB of memory
requirement in the snarkOS README. Please confirm if you want me to make it a dynamic value from a cli flag.
Sidenote: given our syncing issues I understood the CPU/memory requirements need to go up anyway.
Closing in favour of: #3358 |
Motivation
Our clients can and do run out of memory, probably because they have no limit on how many transactions they verify in parallel. This PR proposes to queue them (just like the validator does in
Consensus
) and limit how much parallel verification we do.We can do a lot of clever things to increase the processing speed, like check how many constraints the incoming transactions have, await on a channel to rapidly start verifying, but the focus for now is simplicity and safety.
Test Plan
Related PRs
Implicitly assumes we have https://github.com/AleoHQ/snarkVM/pull/2271 to limit size of transactions coming through.
And this would help to minimize the overhead of transaction verification when syncing blocks with already seen transactions: https://github.com/AleoHQ/snarkVM/pull/2270