fix(mempool): in add_tx, delete queue transaction with lower nonce t…

[ayeletstarkware]

…hat account nonce

---

This change is 

[giladchase]

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

---

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

        // Remove transactions with lower nonce than the account nonce.
        self.tx_pool.remove_up_to_nonce(sender_address, nonce);
        if self.tx_queue.get_nonce(sender_address).is_some_and(|queued_nonce| queued_nonce != nonce)

i think, because it can't be larger? Can it be larger?

If it can, then the comment here should be fixed to reflect that.

Suggestion:

        // Remove transactions with lower nonce than the account nonce.
        self.tx_pool.remove_up_to_nonce(sender_address, nonce);
        if self.tx_queue.get_nonce(sender_address).is_some_and(|queued_nonce| queued_nonce < nonce)

---

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

        // Maybe close nonce gap.
        if self.tx_queue.get_nonce(sender_address).is_none() {

The answer to this check can be inferred from the previous lines, this adds complexity unnecessarily.

BTW I think this happened in a previous PR as well, I don't remember which, but in general when you add code to a function you should consider the whole function cohesively, sometimes the whole flow if it warrents it.

BTW, In this situation, can we forgo the removal/check entirely and just insert into the queue (if we want to know if something was there or not we can return the old value from the insert)

Code quote:

        if self.tx_queue.get_nonce(sender_address).is_none() {

[ayeletstarkware]

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

---

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

Previously, giladchase wrote…

i think, because it can't be larger? Can it be larger?

If it can, then the comment here should be fixed to reflect that.

I want to align with the commit block logic, so I'll keep the != comparison, which fits here as well, and remove the comment.

[elintul]

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

a discussion (no related file):
Not sure I understand the motivation for this PR.

---

---

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

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

I think it's better to remove from queue (which depends on the account state and is "less stable" than the pool) first, then from the pool.

[ayeletstarkware]

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

a discussion (no related file):

Previously, elintul (Elin) wrote…

Not sure I understand the motivation for this PR.

The add_tx function passes the current account nonce to insert_tx. The purpose of this PR is to remove the queued transaction if its nonce differs from the current account nonce, ensuring that a valid transaction remains in the queue.

---

---

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

Previously, giladchase wrote…

The answer to this check can be inferred from the previous lines, this adds complexity unnecessarily.

BTW I think this happened in a previous PR as well, I don't remember which, but in general when you add code to a function you should consider the whole function cohesively, sometimes the whole flow if it warrents it.

BTW, In this situation, can we forgo the removal/check entirely and just insert into the queue (if we want to know if something was there or not we can return the old value from the insert)

how can the answer to this check be inferred from the previous lines? the queue could be empty before the check/removal above.

---

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

Previously, elintul (Elin) wrote…

I think it's better to remove from queue (which depends on the account state and is "less stable" than the pool) first, then from the pool.

Done.

[elintul]

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

a discussion (no related file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

The add_tx function passes the current account nonce to insert_tx. The purpose of this PR is to remove the queued transaction if its nonce differs from the current account nonce, ensuring that a valid transaction remains in the queue.

Sounds like a bug, right? Please add a test that fails without this corretion.

---

---

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

Previously, ayeletstarkware (Ayelet Zilber) wrote…

I want to align with the commit block logic, so I'll keep the != comparison, which fits here as well, and remove the comment.

I think it can't be larger thanks to validate_input; Ayelet, can you doublecheck?

---

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

            input;

        if self.tx_queue.get_nonce(sender_address).is_some_and(|queued_nonce| queued_nonce != nonce)

Suggestion:

// Align mempool to given account state.

// First, maybe remove old nonces.
if self.tx_queue.get_nonce(sender_address).is_some_and(|queued_nonce| queued_nonce != nonce)

---

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

        self.tx_pool.remove_up_to_nonce(sender_address, nonce);

        self.tx_pool.insert(tx)?;

Suggestion:

// Then, insert new transaction.
self.tx_pool.insert(tx)?;

[elintul]

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

---

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

Previously, ayeletstarkware (Ayelet Zilber) wrote…

how can the answer to this check be inferred from the previous lines? the queue could be empty before the check/removal above.

I think I also got confused here, maybe a sign for low code readability.
There might be accounts that weren't in the queue to begin with.

[ayeletstarkware]

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

a discussion (no related file):

Previously, elintul (Elin) wrote…

Sounds like a bug, right? Please add a test that fails without this corretion.

added a test. It fails without this PR code.

---

---

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

Previously, elintul (Elin) wrote…

I think it can't be larger thanks to validate_input; Ayelet, can you doublecheck?

I will discuss this with @MohammadNassar1
added a todo.

---

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

            input;

        if self.tx_queue.get_nonce(sender_address).is_some_and(|queued_nonce| queued_nonce != nonce)

I'll add the comments in the next PR since the order is being changed.

[ayeletstarkware]

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

---

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

Previously, ayeletstarkware (Ayelet Zilber) wrote…

I will discuss this with @MohammadNassar1
added a todo.

changed to a comment.

[ayeletstarkware]

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

---

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

            input;

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

insert_tx is only used in add_tx now, as the new function is removed in #475. I suggest moving this function’s logic directly into add_tx to make the code and comment easier to understand. wdyt? @elintul

[ayeletstarkware]

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

---

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

Previously, ayeletstarkware (Ayelet Zilber) wrote…

insert_tx is only used in add_tx now, as the new function is removed in #475. I suggest moving this function’s logic directly into add_tx to make the code and comment easier to understand. wdyt? @elintul

will be easier to change after the code share

[elintul]

Reviewed 1 of 2 files at r4, 2 of 2 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 138 at r5 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

will be easier to change after the code share

Let's first merge the PRs we have open, and then revisit.

---

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

    expected_mempool_content.assert_eq_queue_content(&mempool);
}

Do we check the case where the account was not in the queue to begin with?

[ayeletstarkware]

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

---

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

Previously, elintul (Elin) wrote…

I think I also got confused here, maybe a sign for low code readability.
There might be accounts that weren't in the queue to begin with.

Is there anything you would suggest I do?

---

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

Previously, elintul (Elin) wrote…

Do we check the case where the account was not in the queue to begin with?

yes, in test_add_tx_account_state_fills_hole

[elintul]

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

---

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

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Is there anything you would suggest I do?

Please revisit a possible refactor after these PRs are merged.

---

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

Previously, ayeletstarkware (Ayelet Zilber) wrote…

yes, in test_add_tx_account_state_fills_hole

Thanks. Better to add a line to the code.

[elintul]

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

OOO

[ayeletstarkware]

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

---

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

Previously, elintul (Elin) wrote…

Please revisit a possible refactor after these PRs are merged.

Done.

[elintul]

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

[ayeletstarkware]

Dismissed @giladchase from 2 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MohammadNassar1)