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: recover sender only for those txs that are included #1636

Merged
merged 1 commit into from
Jan 20, 2025

Conversation

IvanBelyakoff
Copy link
Collaborator

Performance improvement for cases when due to constraints such as gas limits and counter overflows only a few transactions are included in the block.

For 5000 plain send tx:
original version:
INFO[01-15|16:03:56.498] Total time to recover sender total.ms=11457 totalCalls=13342
fixed version:
INFO[01-15|15:53:51.740] Total time to recover sender total.ms=457 totalCalls=5635

Closes #1605

NOTE: optimisation of hash calculation will be addressed in #1599

Copy link
Collaborator

@hexoscott hexoscott left a comment

Choose a reason for hiding this comment

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

A small change to moving sender recovery to before the call to attemptAddTransaction

zk/stages/stage_sequence_execute_transactions.go Outdated Show resolved Hide resolved
@IvanBelyakoff IvanBelyakoff enabled auto-merge (squash) January 17, 2025 16:53
@IvanBelyakoff IvanBelyakoff force-pushed the fix/performance_degradation_sender_recovery branch from e238ba2 to 536b1bc Compare January 20, 2025 10:22
@IvanBelyakoff IvanBelyakoff force-pushed the fix/performance_degradation_sender_recovery branch from 536b1bc to faec421 Compare January 20, 2025 10:24
@IvanBelyakoff IvanBelyakoff merged commit 90cf835 into zkevm Jan 20, 2025
14 checks passed
@IvanBelyakoff IvanBelyakoff deleted the fix/performance_degradation_sender_recovery branch January 20, 2025 11:04
@zjg555543
Copy link

Is it will still slow especially for ERC20 or ETH txs, since each batch can include 700+ transactions because it uses serial mode?

@IvanBelyakoff
Copy link
Collaborator Author

Is it will still slow especially for ERC20 or ETH txs, since each batch can include 700+ transactions because it uses serial mode?

This PR optimises sender recovery to avoid it for txs that will no be included. But I can not do that in parallel:

  1. AFAIK we can't know if tx counters will overflow in advance so we need to add transactions one by one, recovering the sender before that.
  2. Even if we could do that in parallel, that would not necessarily give any performance improvement and could actually produce the opposite effect, just like when I did some profiling with parallel hash calculation

IvanBelyakoff added a commit that referenced this pull request Feb 5, 2025
* fix: recover sender only for those txs that are included

* Moved recovering sender out of addTransaction.
Default context used for sender recovery.
IvanBelyakoff added a commit that referenced this pull request Feb 5, 2025
* fix: recover sender only for those txs that are included

* Moved recovering sender out of addTransaction.
Default context used for sender recovery.
hexoscott pushed a commit that referenced this pull request Feb 5, 2025
* fix: recover sender only for those txs that are included

* Moved recovering sender out of addTransaction.
Default context used for sender recovery.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Sequencer] performance degraded by sender recovery
4 participants