feat(mempool): implement remove from nonce for account transaction index

[MohammadNassar1]

This change is 

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 15 lines in your changes missing coverage. Please review.

Project coverage is 82.72%. Comparing base (177aaf0) to head (0f5cb0b).

Files	Patch %	Lines
crates/mempool/src/transaction_pool.rs	0.00%	15 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #443      +/-   ##
==========================================
- Coverage   83.42%   82.72%   -0.70%     
==========================================
  Files          37       37              
  Lines        1780     1795      +15     
  Branches     1780     1795      +15     
==========================================
  Hits         1485     1485              
- Misses        218      233      +15     
  Partials       77       77              

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

[MohammadNassar1]

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

a discussion (no related file):
This PR implements the remove_from_nonce method for the AccountTransactionIndex. It removes all transactions with nonces lower than or equal to the specified nonce.

This method is used in mempool's commit_block method.

---

[giladchase]

Reviewable status: 0 of 1 files reviewed, 7 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @MohammadNassar1)

---

crates/mempool/src/transaction_pool.rs line 108 at r1 (raw file):

    }

    fn _remove_from_nonce(

Suggestion:

_remove_txs_up_to_nonce(

---

crates/mempool/src/transaction_pool.rs line 112 at r1 (raw file):

        address: ContractAddress,
        nonce: Nonce,
    ) -> Vec<TransactionReference> {

Second API having to pass both a contract and a nonce, which is somewhat awkward.
We'll probably have number 3 soon? Plz start thinking about an extra abstraction here by the time we get to number 3.

Code quote:

    fn get(&self, address: ContractAddress, nonce: Nonce) -> Option<&TransactionReference> {
        self.0.get(&address)?.get(&nonce)
    }

    fn _remove_from_nonce(
        &mut self,
        address: ContractAddress,
        nonce: Nonce,
    ) -> Vec<TransactionReference> {

---

crates/mempool/src/transaction_pool.rs line 115 at r1 (raw file):

        if let Some(btree_map) = self.0.get_mut(&address) {
            let nonces_to_remove: Vec<_> = btree_map.range(..=nonce).map(|(&n, _)| n).collect();
            let txs: Vec<_> = nonces_to_remove.iter().filter_map(|n| btree_map.remove(n)).collect();

no side effects in map.

Code quote:

filter_map(|n| btree_map.remove(n)).c

---

crates/mempool/src/transaction_pool.rs line 115 at r1 (raw file):

        if let Some(btree_map) = self.0.get_mut(&address) {
            let nonces_to_remove: Vec<_> = btree_map.range(..=nonce).map(|(&n, _)| n).collect();
            let txs: Vec<_> = nonces_to_remove.iter().filter_map(|n| btree_map.remove(n)).collect();

Are two allocations necessary here? (it also iterates the whole list twice, rather than once)

Code quote:

            let nonces_to_remove: Vec<_> = btree_map.range(..=nonce).map(|(&n, _)| n).collect();
            let txs: Vec<_> = nonces_to_remove.iter().filter_map(|n| btree_map.remove(n)).collect();

---

crates/mempool/src/transaction_pool.rs line 125 at r1 (raw file):

        } else {
            Vec::new()
        }

no if-let-else

Code quote:

        if let Some(btree_map) = self.0.get_mut(&address) {
            let nonces_to_remove: Vec<_> = btree_map.range(..=nonce).map(|(&n, _)| n).collect();
            let txs: Vec<_> = nonces_to_remove.iter().filter_map(|n| btree_map.remove(n)).collect();

            // Remove the entry from the HashMap if the BTreeMap is empty
            if btree_map.is_empty() {
                self.0.remove(&address);
            }

            txs
        } else {
            Vec::new()
        }

---

crates/mempool/src/transaction_pool.rs line 125 at r1 (raw file):

        } else {
            Vec::new()
        }

smells like or_default

Code quote:

        } else {
            Vec::new()
        }

[elintul]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 7 unresolved discussions (waiting on @ayeletstarkware and @MohammadNassar1)

[MohammadNassar1]

Reviewable status: 0 of 1 files reviewed, 7 unresolved discussions (waiting on @ayeletstarkware and @giladchase)

---

crates/mempool/src/transaction_pool.rs line 108 at r1 (raw file):

    }

    fn _remove_from_nonce(

Done.

---

crates/mempool/src/transaction_pool.rs line 115 at r1 (raw file):

Previously, giladchase wrote…

no side effects in map.

removed.

---

crates/mempool/src/transaction_pool.rs line 125 at r1 (raw file):

Previously, giladchase wrote…

no if-let-else

Done.

---

crates/mempool/src/transaction_pool.rs line 125 at r1 (raw file):

Previously, giladchase wrote…

smells like or_default

Done.

[elintul]

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

[MohammadNassar1]

Reviewable status: 0 of 1 files reviewed, 7 unresolved discussions (waiting on @ayeletstarkware and @giladchase)

---

crates/mempool/src/transaction_pool.rs line 115 at r1 (raw file):

Previously, giladchase wrote…

Are two allocations necessary here? (it also iterates the whole list twice, rather than once)

I think we should use separate iterators because removing elements from a BTreeMap while iterating over it is not allowed.
Error:
"cannot borrow as mutable because it is also borrowed as immutable"

[MohammadNassar1]

Reviewable status: 0 of 1 files reviewed, 7 unresolved discussions (waiting on @ayeletstarkware and @giladchase)

---

crates/mempool/src/transaction_pool.rs line 112 at r1 (raw file):

Previously, giladchase wrote…

Second API having to pass both a contract and a nonce, which is somewhat awkward.
We'll probably have number 3 soon? Plz start thinking about an extra abstraction here by the time we get to number 3.

That's a good point. The naive solution is to add a structure. I'll think about a different and more efficient solution.
I think it's unrelated to this PR, so I'll do it in a separate PR. wdyt?

[giladchase]

Reviewable status: 0 of 1 files reviewed, 7 unresolved discussions (waiting on @ayeletstarkware and @MohammadNassar1)

---

crates/mempool/src/transaction_pool.rs line 114 at r2 (raw file):

    ) -> Vec<TransactionReference> {
        match self.0.get_mut(&address) {
            Some(btree_map) => {

short-circuit with let-else pattern, less nesting

Code quote:

        match self.0.get_mut(&address) {
            Some(btree_map) => {

---

crates/mempool/src/transaction_pool.rs line 121 at r2 (raw file):

                for n in nonces_to_remove {
                    txs.push(btree_map.remove(&n).expect("Failed to remove nonce from BTreeMap"));
                }

that's possiblyk*log(n), use split_off for a log(n) solution

Code quote:

                for n in nonces_to_remove {
                    txs.push(btree_map.remove(&n).expect("Failed to remove nonce from BTreeMap"));
                }

[giladchase]

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @ayeletstarkware and @MohammadNassar1)

---

crates/mempool/src/transaction_pool.rs line 125 at r1 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Done.

There's no way to use or_default here? (there might not be, just making sure)

[MohammadNassar1]

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

---

crates/mempool/src/transaction_pool.rs line 125 at r1 (raw file):

Previously, giladchase wrote…

There's no way to use or_default here? (there might not be, just making sure)

Now I use let-else. and return if btreemap is empty.

---

crates/mempool/src/transaction_pool.rs line 114 at r2 (raw file):

Previously, giladchase wrote…

short-circuit with let-else pattern, less nesting

Done.

[elintul]

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

[MohammadNassar1]

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

---

crates/mempool/src/transaction_pool.rs line 121 at r2 (raw file):

Previously, giladchase wrote…

that's possiblyk*log(n), use split_off for a log(n) solution

Done.

[elintul]

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

---

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

    }

    fn _remove_txs_up_to_nonce(

Suggestion:

_remove_up_to_nonce

---

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

        nonce: Nonce,
    ) -> Vec<TransactionReference> {
        let Some(btree_map) = self.0.get_mut(&address) else {

Suggestion:

account_txs

---

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

        }

        txs

Your comments are repeating the code, note that good variable names make then redundant.

Suggestion:

        let txs_with_higher_or_equal_nonce = account_txs.split_off(&nonce);
        let txs_with_lower_nonce: Vec<TransactionReference> = account_txs.values().cloned().collect();

        *account_txs = txs_with_higher_or_equal_nonce;
        if account_txs.is_empty() {
            self.0.remove(&address);
        }

        txs_with_lower_nonce

[elintul]

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

---

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

        let txs_with_higher_or_equal_nonce = account_txs.split_off(&nonce);
        let txs_with_lower_nonce: Vec<TransactionReference> =
            account_txs.values().cloned().collect();

What exactly is cloned here? Making sure we don't do expensive work.

Code quote:

.cloned()

[giladchase]

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

---

crates/mempool/src/transaction_pool.rs line 121 at r2 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Done.

Nice!
I think you need to +1 the nonce though, cause split-off(k) is, somewhat unintuitively, breaking it up into [,..,k) and [k,..]

---

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

Previously, elintul (Elin) wrote…

What exactly is cloned here? Making sure we don't do expensive work.

I think he cloned the txs because he couldn't consume account_txs due to the *account_txs below.

You can avoid the clone by using a "swap" operation, which swaps account_txs with what you want and yields the ousted contents, freeing you up to take ownership of them without clone.

(but std::mem::swap is for variables, for an operation on an object you need std::mem::replace)

        let txs_with_higher_or_equal_nonce = account_txs.split_off(&nonce);

        let txs_with_lower_nonce = std::mem::replace(account_txs, txs_with_higher_or_equal_nonce);

        if account_txs.is_empty() {
            self.0.remove(&address);
        }

        txs_with_lower_nonce.into_values().collect()

OOO

[MohammadNassar1]

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

---

crates/mempool/src/transaction_pool.rs line 121 at r2 (raw file):

Previously, giladchase wrote…

Nice!
I think you need to +1 the nonce though, cause split-off(k) is, somewhat unintuitively, breaking it up into [,..,k) and [k,..]

Correct, but I think removing the lower nonces is enough, as the commit_block sends the next_nonce.

---

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

Previously, giladchase wrote…

I think he cloned the txs because he couldn't consume account_txs due to the *account_txs below.

You can avoid the clone by using a "swap" operation, which swaps account_txs with what you want and yields the ousted contents, freeing you up to take ownership of them without clone.

(but std::mem::swap is for variables, for an operation on an object you need std::mem::replace)

        let txs_with_higher_or_equal_nonce = account_txs.split_off(&nonce);

        let txs_with_lower_nonce = std::mem::replace(account_txs, txs_with_higher_or_equal_nonce);

        if account_txs.is_empty() {
            self.0.remove(&address);
        }

        txs_with_lower_nonce.into_values().collect()

Nice!
But, as TransactionReference implements Copy I can replace it with the following code, which is simpler I think:

Code snippet:

        let txs_with_lower_nonce: Vec<TransactionReference> =
            account_txs.iter().map(|(_, tx)| *tx).collect();

[elintul]

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

---

crates/mempool/src/transaction_pool.rs line 121 at r2 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Correct, but I think removing the lower nonces is enough, as the commit_block sends the next_nonce.

So I think it should send the current nonce of the changes accounts (i.e., a subset of the state diff.).

[MohammadNassar1]

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

---

crates/mempool/src/transaction_pool.rs line 121 at r2 (raw file):

Previously, elintul (Elin) wrote…

So I think it should send the current nonce of the changes accounts (i.e., a subset of the state diff.).

Note that in commit_block, I already calculate the next nonce, to modify the tx_queue.

[elintul]

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

---

crates/mempool/src/transaction_pool.rs line 119 at r7 (raw file):

        // Split the transactions at the given nonce.
        // After the split, `account_txs` will contain the transactions with lower nonces,

Isn't Gilad's suggestion a bit more elegant? It allows us to keep the names and save on documentation.

---

crates/mempool/src/transaction_pool.rs line 123 at r7 (raw file):

        let mut split_txs = account_txs.split_off(&nonce);
        // After the swap `split_txs` will contain transactions with lower nonces.
        swap(account_txs, &mut split_txs);

Fully-qualified name, please. It's not clear where it's coming from.

Code quote:

swap

[MohammadNassar1]

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

---

crates/mempool/src/transaction_pool.rs line 119 at r7 (raw file):

Previously, elintul (Elin) wrote…

Isn't Gilad's suggestion a bit more elegant? It allows us to keep the names and save on documentation.

replace doesn't solve the borrowing issue, as it doesn't take two &mut vectors.

[elintul]

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware)

[MohammadNassar1]

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

---

crates/mempool/src/transaction_pool.rs line 119 at r7 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

replace doesn't solve the borrowing issue, as it doesn't take two &mut vectors.

The code was updated.
It uses replace now.
Thanks!

[elintul]

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