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

[Perf] Greedily verify Executions before block generation #3451

Open
wants to merge 1 commit into
base: staging
Choose a base branch
from

Conversation

vicsn
Copy link
Contributor

@vicsn vicsn commented Nov 27, 2024

Motivation

When processing hundreds of transmissions during block generation, transaction verification is a large bottleneck, taking up 90% of compute. By verifying transactions ahead of time, verification will be cached in partially_verified_transactions, moving compute out of the hot path.

Some useful details:

  • The current PR does not mean to change any consensus logic. We still sign proposals without awaiting succesful verification of transmissions, just like before.
  • Solutions are not verified because there are only 4 per block, and ARC-0043: Extending the Puzzle to a Full SNARK ARCs#77 might change the game entirely.
  • Deployments are not verified, because in the honest case there won't be a lot of them, and they consume a lot of memory so would require a lot more complexity (a queue and thread steadily popping from it)

Test Plan

  • Deployed a network succesfully.

@vicsn vicsn changed the title Verify Executions before block generation [Perf] Verify Executions before block generation Nov 28, 2024
@vicsn vicsn changed the title [Perf] Verify Executions before block generation [Perf] Greedily verify Executions before block generation Nov 30, 2024
@raychu86
Copy link
Contributor

Relevant PR - #3098.

We initially did not verify transactions from other validator's proposals to prevent a block halting issue. In your case, I believe this is safe because it doesn't interfere with the consensus rules.

However, it would be interesting to see the implications of this kind of change. I worry that this may open up avenues for malicious nodes to slow down other validators by spamming them with transactions.

@vicsn
Copy link
Contributor Author

vicsn commented Dec 18, 2024

I worry that this may open up avenues for malicious nodes to slow down other validators by spamming them with transactions.

Valid concern, but note that:

  • this only impacts execution verification, which is light (takes a couple ms).
  • validators currently are only at 50% average CPU saturation. It is theoretically possible that the early tx verification eats up resources from, say, certificate deserialization. But in practice my E2E stress test shows a large speedup. Happy to share grafana links etc.
  • assuming the cache works as intended, we are not increasing compute, merely moving it (you used the term frontloading before).

Copy link
Contributor

@raychu86 raychu86 left a comment

Choose a reason for hiding this comment

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

LGTM as long as we feel confident that this shouldn't introduce any new attack vectors!

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.

2 participants