feat(mempool): impl mempool state

[MohammadNassar1]

This change is 

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 50.00000% with 12 lines in your changes missing coverage. Please review.

Project coverage is 80.81%. Comparing base (93de0bd) to head (d43df34).

Files	Patch %	Lines
crates/mempool/src/mempool.rs	50.00%	10 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #497      +/-   ##
==========================================
- Coverage   81.21%   80.81%   -0.41%     
==========================================
  Files          42       42              
  Lines        1826     1850      +24     
  Branches     1826     1850      +24     
==========================================
+ Hits         1483     1495      +12     
- Misses        269      279      +10     
- Partials       74       76       +2     

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

[MohammadNassar1]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion

a discussion (no related file):
This PR implements the Mempool state, representing the state that the mempool is aware of during batch processing.
The Mempool state is updated in get_txs, used in add_tx and commit_block, and reset after commit_block.

---

[ayeletstarkware]

Reviewed 1 of 2 files at r1, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @elintul, @giladchase, and @MohammadNassar1)

---

crates/mempool/src/mempool.rs line 69 at r3 (raw file):

        for tx in &eligible_txs {
            self.mempool_state.entry(tx.sender_address).or_default().nonce = tx.nonce;
        }

Can you explain these loops?

Code quote:

        for tx in &eligible_txs {
            self.mempool_state.entry(tx.sender_address).and_modify(|account| {
                account.nonce = tx.nonce;
            });
        }

        for tx in &eligible_txs {
            self.mempool_state.entry(tx.sender_address).or_default().nonce = tx.nonce;
        }

---

crates/mempool/src/mempool.rs line 127 at r3 (raw file):

    fn is_duplicated_tx(&self, tx: &ThinTransaction) -> MempoolResult<()> {
        if let Some(AccountState { nonce }) = self.mempool_state.get(&tx.sender_address) {
            if *nonce >= tx.nonce {

Will this work?
if nonce >= &tx.nonce {

Code quote:

*nonce >= tx.nonce

[MohammadNassar1]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

crates/mempool/src/mempool.rs line 69 at r3 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Can you explain these loops?

My bad. I deleted the first loop.

The second one that I kept goes over the popped transactions and updates the mempool state for these addresses. I used 'or_default' so if it didn't have the address, it would generate the default 'AccountState'.

---

crates/mempool/src/mempool.rs line 127 at r3 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Will this work?
if nonce >= &tx.nonce {

Both are okay,
I used your suggestion, as using references is preferred.

[elintul]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ayeletstarkware, @giladchase, and @MohammadNassar1)

---

crates/mempool/src/mempool.rs line 25 at r4 (raw file):

    // Transactions eligible for sequencing.
    tx_queue: TransactionQueue,
    // Represents the current state of the mempool during batch processing.

Suggestion:

block creation.

---

crates/mempool/src/mempool.rs line 62 at r4 (raw file):

        // Update the mempool state with the new nonces.
        for tx in eligible_txs.iter() {

Isn't for x in iterable sugar for this?

Code quote:

.iter()

---

crates/mempool/src/mempool.rs line 120 at r4 (raw file):

    }

    fn is_duplicated_tx(&self, tx: &ThinTransaction) -> MempoolResult<()> {

Is this related to this PR?

Code quote:

is_duplicated_tx

[elintul]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ayeletstarkware, @giladchase, and @MohammadNassar1)

[MohammadNassar1]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

crates/mempool/src/mempool.rs line 25 at r4 (raw file):

    // Transactions eligible for sequencing.
    tx_queue: TransactionQueue,
    // Represents the current state of the mempool during batch processing.

Done

---

crates/mempool/src/mempool.rs line 120 at r4 (raw file):

Previously, elintul (Elin) wrote…

Is this related to this PR?

Yes, this PR adds the mempool_state and in is_duplicate_tx function it verifies that the transaction is valid using the mempool_state.

[elintul]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ayeletstarkware, @giladchase, and @MohammadNassar1)

---

crates/mempool/src/mempool.rs line 120 at r4 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Yes, this PR adds the mempool_state and in is_duplicate_tx function it verifies that the transaction is valid using the mempool_state.

Doesn't this fall into the case we discussed, where add_tx input goes back?
Should we check the Q as well?

[MohammadNassar1]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

crates/mempool/src/mempool.rs line 120 at r4 (raw file):

Previously, elintul (Elin) wrote…

Doesn't this fall into the case we discussed, where add_tx input goes back?
Should we check the Q as well?

This was added to prevent adding a transaction that was sent to the batcher using get_txs but not yet committed.

Check with whom?

[elintul]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ayeletstarkware, @giladchase, and @MohammadNassar1)

---

crates/mempool/src/mempool.rs line 120 at r4 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

This was added to prevent adding a transaction that was sent to the batcher using get_txs but not yet committed.

Check with whom?

Check the queue. Okay, I understand it's not necessarily the same case.
Let's add the other checks to this method as well.

---

crates/mempool/src/mempool.rs line 126 at r5 (raw file):

        }
        Ok(())
    }

Would that work?

Suggestion:

fn should_insert(&self, tx: &ThinTransaction) -> bool {
    self.mempool_state
        .get(&tx.sender_address)
        .map_or(true, |AccountState { nonce }| nonce < &tx.nonce)
}

[MohammadNassar1]

Reviewable status: 1 of 3 files reviewed, 5 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

crates/mempool/src/mempool.rs line 120 at r4 (raw file):

Previously, elintul (Elin) wrote…

Check the queue. Okay, I understand it's not necessarily the same case.
Let's add the other checks to this method as well.

Sorry, I didn't get it :(
Which another check?

---

crates/mempool/src/mempool.rs line 126 at r5 (raw file):

Previously, elintul (Elin) wrote…

Would that work?

Following our discussion, we decided to add another Error DuplicateNonce.
The code was updated.

[elintul]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ayeletstarkware, @giladchase, and @MohammadNassar1)

---

crates/mempool/src/mempool.rs line 120 at r4 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Sorry, I didn't get it :(
Which another check?

There's #510, maybe we should add this check to should_insert.

---

crates/mempool/src/mempool.rs line 126 at r5 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Following our discussion, we decided to add another Error DuplicateNonce.
The code was updated.

Given that we'll add more checks to this method, perhaps: validate_input?

[elintul]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ayeletstarkware and @giladchase)

[MohammadNassar1]

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

crates/mempool/src/mempool.rs line 120 at r4 (raw file):

Previously, elintul (Elin) wrote…

There's #510, maybe we should add this check to should_insert.

This is the same check :).
I added a TODO in that PR to use this method.

---

crates/mempool/src/mempool.rs line 126 at r5 (raw file):

Previously, elintul (Elin) wrote…

Given that we'll add more checks to this method, perhaps: validate_input?

Done.

Her comments were implemented

[MohammadNassar1]

Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

crates/mempool/src/mempool.rs line 127 at r3 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Both are okay,
I used your suggestion, as using references is preferred.

Done

[elintul]

Reviewed all commit messages.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware and @giladchase)

[elintul]

Reviewed 1 of 1 files at r9.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware, @giladchase, and @MohammadNassar1)

---

crates/mempool/src/mempool.rs line 120 at r4 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

This is the same check :).
I added a TODO in that PR to use this method.

Why do we need both PRs?

---

crates/mempool/src/mempool.rs line 122 at r9 (raw file):

    }

    fn validate_input(&self, tx: &ThinTransaction, account: Account) -> MempoolResult<()> {

Pass MempoolInput.

Code quote:

validate_input

[MohammadNassar1]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware and @giladchase)

---

crates/mempool/src/mempool.rs line 120 at r4 (raw file):

Previously, elintul (Elin) wrote…

Why do we need both PRs?

Talked F2F.

[elintul]

Reviewed 2 of 2 files at r10, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware and @giladchase)