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] Cache fee verification #2580

Open
wants to merge 2 commits into
base: staging
Choose a base branch
from

Conversation

vicsn
Copy link
Contributor

@vicsn vicsn commented Nov 28, 2024

Motivation

When processing hundreds of transmissions during block generation, transaction verification is a large bottleneck, taking up 90% of compute. Half of that is spent verifying the SNARK proofs associated to fees. We should cache that.

Some useful details:

  • Despite changing caching logic, this does not require a migration because the cache is in memory and will be cleared on a necessary upgrade restart.
  • There are 3 cases for the fee: its from a Deployment, Execution, or Fee transaction. Given that Fee transactions are only created during block generation and are only verified once, it is technically superfluous to add those to the cache. But its cleaner at negligible cost.
  • Since the PR which introduced the cache, I realize we can cache potentially way more checks, but the SNARK verification is by far the largest cost anyway, so we can stick with the existing setup.

Test Plan

  • Added cache state expectations to unit tests. Fee tests are not in there because the respective test-helper doesn't expose the rejected id.
  • Ran succesfully on deployed network

Related PRs

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.

Logic overall LGTM! I don't recall why we didn't do the fee cacheing before when we had initially introduced it.

One thing I would ask you to think about as the author of the PR is the following:

Fee's can be swapped between transactions and Executions/Deployments can be broadcasted with different Fee transitions. Does this PR cover those cases? (I think it does, but want to be doubly sure)

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