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

Implement tx execution while reading from the mempool #2194

Merged
merged 10 commits into from
Dec 11, 2024

Conversation

tudor-malene
Copy link
Collaborator

@tudor-malene tudor-malene commented Dec 10, 2024

Why this change is needed

The "sequencer" functionality of building a new batch was implemented weirdly

Previously the validator and sequencer execution was identical, with the exception that the sequencer would read all transactions that fit in a batch from the mempool.
This could be used in DOS attacks.

What changes were made as part of this PR

  • move the mempool reading during the execution
  • this allows the use of the ordering structure used by go-ethereum, and implementing the account based logic
  • move some logic from the evm-facade to the batch-executor
  • better accounting of the gas used, which allows the implementation of a gas limit per batch

PR checks pre-merging

Please indicate below by ticking the checkbox that you have read and performed the required
PR checks

  • PR checks reviewed and performed

@@ -95,6 +95,8 @@ const (
L2GenesisHeight = uint64(0)
L2GenesisSeqNo = uint64(1)
L2SysContractGenesisSeqNo = uint64(2)

SyntheticTxGasLimit = uint64(10_000_000_000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we putting a gas limit here?

// Pop removes the best transaction, *not* replacing it with the next one from
// the same account. This should be used when a transaction cannot be executed
// and hence all subsequent ones should be discarded from the same account.
func (t *transactionsByPriceAndNonce) Pop() {
Copy link
Contributor

Choose a reason for hiding this comment

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

As this is copy paste, can we add additional functions with proper namings - PopToNextAccount or MoveAccount that call those; It's going to make our code that uses them clearer

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd rather keep it as it is, so we can easily keep it in sync

@tudor-malene tudor-malene merged commit 6febd69 into main Dec 11, 2024
3 checks passed
@tudor-malene tudor-malene deleted the tudor/mempool_fix branch December 11, 2024 11:54
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