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

refactor(worker): order L1 messages by queue index #512

Merged
merged 5 commits into from
Sep 18, 2023
Merged

Conversation

Thegaram
Copy link

1. Purpose or design rationale of this PR

TransactionsByPriceAndNonce is a data structure used to order transactions by gas price and nonce. The idea is that we can process transactions from one account following their nonce order, and we can simply skip all transactions from an account if one in the middle is not executed. If all gas prices and nonces are equal, then it will order transactions based on their timestamp.

We currently reuse TransactionsByPriceAndNonce for processing L1 messages. However, L1 messages are ordered by their QueueIndex. Gas prices and nonces for L1 messages are 0, and the worker ensures that the timestamps follow the QueueIndex order so this all happens to work.

However, this is hacky and error prone for multiple reasons:

  • It is non-trivial to see that the order of processing L1 messages is correct. This was actually raised as a false positive critical finding during an audit. We added a test TestTransactionTimeSort to verify this behaviour.
  • We should ensure to never call Pop() on an L1 message set. In other words, we should only skip a single L1 message and never skip all L1 messages from the same sender account. Otherwise we might skip L1 messages unnecessarily.

This PR improves this by introducing a new interface OrderedTransactionSet. worker.commitTransactions will now receive this interface. It has two implementations: TransactionsByPriceAndNonce (used for L2 transactions) and L1MessagesByQueueIndex (used for L1 message transactions).

2. PR title

Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:

  • refactor: A code change that doesn't fix a bug, or add a feature, or improves performance

3. Deployment tag versioning

Has the version in params/version.go been updated?

  • This PR doesn't involve a new deployment, git tag, docker image tag, and it doesn't affect traces
  • Yes

4. Breaking change label

Does this PR have the breaking-change label?

  • This PR is not a breaking change
  • Yes

NazariiDenha
NazariiDenha previously approved these changes Sep 13, 2023
colinlyguo
colinlyguo previously approved these changes Sep 15, 2023
colinlyguo
colinlyguo previously approved these changes Sep 16, 2023
@Thegaram Thegaram merged commit 93a9497 into develop Sep 18, 2023
5 checks passed
@Thegaram Thegaram deleted the refactor-tx-set branch September 18, 2023 08:34
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.

None yet

3 participants