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 a mempool for the sequencer #2341

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

rianhughes
Copy link
Contributor

This PR implements a mempool required for the sequencer.

Copy link

codecov bot commented Dec 19, 2024

Codecov Report

Attention: Patch coverage is 65.78947% with 78 lines in your changes missing coverage. Please review.

Project coverage is 74.60%. Comparing base (27b6968) to head (5286584).

Files with missing lines Patch % Lines
mempool/mempool.go 60.52% 59 Missing and 16 partials ⚠️
mempool/db_utils.go 92.10% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2341      +/-   ##
==========================================
- Coverage   74.62%   74.60%   -0.03%     
==========================================
  Files         110      112       +2     
  Lines       12024    12252     +228     
==========================================
+ Hits         8973     9140     +167     
- Misses       2354     2403      +49     
- Partials      697      709      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mempool/mempool.go Outdated Show resolved Hide resolved
mempool/mempool.go Outdated Show resolved Hide resolved
mempool/mempool.go Outdated Show resolved Hide resolved
mempool/mempool.go Outdated Show resolved Hide resolved
mempool/mempool.go Outdated Show resolved Hide resolved
Copy link
Contributor

@rodrigo-pino rodrigo-pino left a comment

Choose a reason for hiding this comment

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

Half review! Will continue in January!

mempool/mempool.go Outdated Show resolved Hide resolved
mempool/mempool.go Outdated Show resolved Hide resolved
mempool/mempool.go Outdated Show resolved Hide resolved
mempool/mempool.go Outdated Show resolved Hide resolved
@weiihann
Copy link
Contributor

From my understanding, the current implementation seems to work as such:

  • Receives new tx
  • Validates the tx
  • Puts new tx into DB directly
  • The order is FCFS

This architecture is simple and straightforward, but I'm not sure how effective it would work it would be in real life. Here are a few things we need to consider:

  1. In-memory mempool
    It's way too expensive to store and retrieve txs only through the DB. There should be an in-memory implementation of it for fast access.

  2. Order of sequencing
    The current method is FCFS, but how about other factors like fees?

  3. Transaction validation
    I'm not fully aware what the ValidatorFunc actually contains, but few things we need to check for:

  • Account balance checks
  • Nonce validation
  • Gas limit validation

There should also be a separation between pending txs and ready txs. Ready tx means tx that has higher nonce than the next immediate nonce of an account. Ready tx means the next immediate one and it's ready to be executed.

Mempool design and architecture could get complex, as it can be exploited for MEV. I'm not sure how far we want to take it at the current stage, but I'll suggest to look into existing architecture like Erigon's mempool design and go-ethereum one and see how they do certain things.

mempool/mempool.go Outdated Show resolved Hide resolved
mempool/mempool.go Outdated Show resolved Hide resolved
mempool/mempool.go Outdated Show resolved Hide resolved
mempool/mempool_test.go Outdated Show resolved Hide resolved
mempool/mempool_test.go Outdated Show resolved Hide resolved
@rianhughes rianhughes force-pushed the feature/sequencer-mempool branch 3 times, most recently from 4f1107c to 0cf2454 Compare January 8, 2025 13:46
mempool/bucket.go Outdated Show resolved Hide resolved
@rianhughes rianhughes force-pushed the feature/sequencer-mempool branch from 5badae0 to 70db337 Compare January 9, 2025 12:22
mempool/mempool.go Outdated Show resolved Hide resolved
mempool/mempool.go Outdated Show resolved Hide resolved
mempool/mempool.go Outdated Show resolved Hide resolved
Copy link
Contributor

@rodrigo-pino rodrigo-pino left a comment

Choose a reason for hiding this comment

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

Haven't checked functionality. Will do that on a second round. I am sharing now a bit of stylistic changes which I think will improve the code. Let me know what you think

mempool/mempool.go Outdated Show resolved Hide resolved
mempool/mempool.go Outdated Show resolved Hide resolved
mempool/mempool.go Outdated Show resolved Hide resolved
mempool/mempool.go Show resolved Hide resolved
mempool/mempool.go Outdated Show resolved Hide resolved
mempool/mempool.go Outdated Show resolved Hide resolved
mempool/mempool.go Outdated Show resolved Hide resolved
mempool/mempool.go Outdated Show resolved Hide resolved
mempool/mempool.go Outdated Show resolved Hide resolved
mempool/mempool.go Outdated Show resolved Hide resolved
mempool/mempool.go Outdated Show resolved Hide resolved
mempool/mempool.go Outdated Show resolved Hide resolved
mempool/mempool.go Outdated Show resolved Hide resolved
mempool/mempool.go Outdated Show resolved Hide resolved
mempool/mempool.go Show resolved Hide resolved
mempool/mempool.go Outdated Show resolved Hide resolved
mempool/mempool.go Outdated Show resolved Hide resolved
Copy link
Contributor

@rodrigo-pino rodrigo-pino left a comment

Choose a reason for hiding this comment

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

LGTM! Adding some final comments

mempool/mempool.go Outdated Show resolved Hide resolved
mempool/mempool.go Outdated Show resolved Hide resolved
mempool/mempool.go Outdated Show resolved Hide resolved
mempool/mempool.go Outdated Show resolved Hide resolved
// push multiple to non empty (push 4,5. now have 3,4,5)
for i := uint64(4); i < 6; i++ {
senderAddress := new(felt.Felt).SetUint64(i)
state.EXPECT().ContractNonce(senderAddress).Return(new(felt.Felt).SetUint64(0), nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe SetUint64(0) is not necessary. If you want to be explicit, I think returning &felt.Zero is a better option

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated it to &felt.Zero, but just out of curiosity, why do you think new(felt).SetUint64(0) is suboptimal? Just because it might get allocated on the heap?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, in this instance it was from a legibility point of view. It is easier to read that your returning zero than creating a new felt and setting it's value to zero

@rianhughes rianhughes force-pushed the feature/sequencer-mempool branch from fd3959d to 6ff70d9 Compare January 15, 2025 10:00
Copy link
Contributor

@rodrigo-pino rodrigo-pino left a comment

Choose a reason for hiding this comment

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

LGTM!

@rodrigo-pino rodrigo-pino requested a review from cicr99 January 15, 2025 11:32
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.

4 participants