refactor(mempool): rearrange insert_tx before refactoring code

[ayeletstarkware]

rearrange insert_tx func before refactoring code to share code functionality

---

This change is 

[MohammadNassar1]

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

---

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

        let MempoolInput { tx, account: Account { sender_address, state: AccountState { nonce } } } =
            input;

Please add in the test a check on the queue.

---

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

        }

        self.tx_pool.remove_up_to_nonce(sender_address, nonce);

why did you move it here? after updating the queue?

Code quote:

self.tx_pool.remove_up_to_nonce(sender_address, nonce);

[elintul]

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

---

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

Previously, MohammadNassar1 (mohammad-starkware) wrote…

why did you move it here? after updating the queue?

Please explain why this is equivalent (the order doesn't matter); not sure I'm convinced.

[ayeletstarkware]

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

---

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

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Please add in the test a check on the queue.

added in #452

---

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

Previously, elintul (Elin) wrote…

Please explain why this is equivalent (the order doesn't matter); not sure I'm convinced.

I moved it to align with the commit_block code.
Why is this equivalent? Let’s break down the steps:

1. We add the new transaction to the pool.
2. We check if the new transaction has a higher account nonce than the one in the queue. If it does, we remove the queued transaction.
3. We check if there’s a transaction in the pool with the current account nonce. If there is, we insert it into the queue.
4. We remove transactions from the pool with lower nonces than the current account nonce.

In this PR, I made two changes by reordering the steps:

• Previously, step 4 (removing older nonce transactions from the pool) was the first step. I believe the order doesn’t matter because none of the other steps will interact with pool transactions that have lower nonces than the current account nonce.
• The insertion of the pool transaction was originally between steps 2 and 3. I moved it to the first line. This change is safe because step 2 doesn’t modify the pool.

[elintul]

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

---

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

Previously, ayeletstarkware (Ayelet Zilber) wrote…

I moved it to align with the commit_block code.
Why is this equivalent? Let’s break down the steps:

1. We add the new transaction to the pool.
2. We check if the new transaction has a higher account nonce than the one in the queue. If it does, we remove the queued transaction.
3. We check if there’s a transaction in the pool with the current account nonce. If there is, we insert it into the queue.
4. We remove transactions from the pool with lower nonces than the current account nonce.

In this PR, I made two changes by reordering the steps:

• Previously, step 4 (removing older nonce transactions from the pool) was the first step. I believe the order doesn’t matter because none of the other steps will interact with pool transactions that have lower nonces than the current account nonce.
• The insertion of the pool transaction was originally between steps 2 and 3. I moved it to the first line. This change is safe because step 2 doesn’t modify the pool.

Why not: maybe remove from queue, then from pool; then add to pool, maybe remove from queue.
This is a more logical story to tell.

[ayeletstarkware]

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

---

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

Previously, elintul (Elin) wrote…

Why not: maybe remove from queue, then from pool; then add to pool, maybe remove from queue.
This is a more logical story to tell.

I need the insertion to the pool to be the first or the last step since I'm extracting steps 2-4 to a function in #472. Anyway, can I change the order after extracting this logic to a function and use it in both add_tx and commit_block, so I won't have to change it in both?

[elintul]

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

---

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

Previously, ayeletstarkware (Ayelet Zilber) wrote…

I need the insertion to the pool to be the first or the last step since I'm extracting steps 2-4 to a function in #472. Anyway, can I change the order after extracting this logic to a function and use it in both add_tx and commit_block, so I won't have to change it in both?

It can't be last, since it affects what you maybe insert to the queue.
I'll reorder: maybe remove from queue, maybe remove from pool, add to pool, maybe add to queue.
When removing: you should first remove from the queue and then from pool; when inserting: you should first insert to pool, and then to queue. Perhaps we should separate removal and insertion.

[ayeletstarkware]

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

---

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

Previously, elintul (Elin) wrote…

It can't be last, since it affects what you maybe insert to the queue.
I'll reorder: maybe remove from queue, maybe remove from pool, add to pool, maybe add to queue.
When removing: you should first remove from the queue and then from pool; when inserting: you should first insert to pool, and then to queue. Perhaps we should separate removal and insertion.

Done. commit_block doesn’t insert into the pool, so this order doesn’t work if we want to share code. However, this is out of scope for this PR—let’s discuss it F2F.

[elintul]

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

---

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

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Done. commit_block doesn’t insert into the pool, so this order doesn’t work if we want to share code. However, this is out of scope for this PR—let’s discuss it F2F.

Note you haven't changed anything in the end.

[MohammadNassar1]

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

[MohammadNassar1]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @giladchase)

[elintul]

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

---

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

            // Align the queue with the committed nonces.

            // Remove transactions with lower nonces from the queue and the pool.

Please update documentation of both segments of code with the one we agreed on in #472.

Code quote:

// Remove transactions with lower nonces from the queue and the pool.

[ayeletstarkware]

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

---

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

Previously, elintul (Elin) wrote…

Please update documentation of both segments of code with the one we agreed on in #472.

Done.

[elintul]

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

---

crates/mempool/src/mempool.rs line 128 at r6 (raw file):

        // Maybe remove out-of-date transactions.
        // Note: != is actually equivalent to > here, as lower nonces are rejected in validation.

This should also appear in the new method; please be minded that you're not deleting documentation when moving code.

Code quote:

Note: != is actually equivalent to > here, as lower nonces are rejected in validation.

[ayeletstarkware]

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

---

crates/mempool/src/mempool.rs line 128 at r6 (raw file):

Previously, elintul (Elin) wrote…

This should also appear in the new method; please be minded that you're not deleting documentation when moving code.

yes it's in the align function in #472

[elintul]

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

---

crates/mempool/src/mempool.rs line 128 at r6 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

yes it's in the align function in #472

Great!

[elintul]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @giladchase)