Skip to content
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

[Fix] Limit number of deployments in mempool. #3089

Merged
merged 3 commits into from
Feb 14, 2024
Merged

Conversation

vicsn
Copy link
Contributor

@vicsn vicsn commented Feb 12, 2024

A new approach to https://github.com/AleoHQ/snarkOS/pull/2999 with a focus on simplicity and readability.

At 10 deployments at most in the deployments_queue, we won't run out of memory (the network message size is at most 300 MB).

And we will never verify more than 1 deployment sequentially if there are any executions around, making sure we won't take too much compute time.

We always interweave the verification of some executions and deployments.

@vicsn vicsn requested a review from ljedrz February 12, 2024 10:19
At 10 deployments we won't run out of memory.
At 5 deployments, we won't take too much compute time.
We always interweave the verification of some executions
and deployments.
@vicsn vicsn force-pushed the limit_deployments_2 branch from fe1f0af to be8e6ce Compare February 12, 2024 10:45
Comment on lines 294 to 296
let mut deployments_queue = self.deployments_queue.lock();
// Acquire the lock on the executions queue.
let mut executions_queue = self.executions_queue.lock();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

future consideration: ideally, collections that are always accessed together could be under a single lock (for added performance), though that may require a little bit more type boilerplate

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made it anyway so Howard can decide: f93060b
Did not implement any further methods for TransactionsQueue to avoid having to add input checking for those methods and to keep it simple.

Copy link
Collaborator

@ljedrz ljedrz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@howardwu howardwu merged commit 264b2d4 into mainnet Feb 14, 2024
23 of 24 checks passed
@howardwu howardwu deleted the limit_deployments_2 branch February 14, 2024 01:30
@howardwu howardwu changed the title Limit number of deployments in mempool. [Fix] Limit number of deployments in mempool. Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants