fix(mempool): fix off-by-one nonces on commit (#1753)

We assumed nonces in `commit_block` would be the last accepted, but in
reality they are the next nonces after the last accepted.
In other words, we don't need to increment nonces in `commit_block`,
they are already incremented.

Style: renamed nonces to `address_to_nonce` for readability, it's a map.

Fix: in tests mostly just bumped the nonces before running
`commit_block`.
`test_commit_block_includes_proposed_txs_subset` was
somewhat rewritten, the distribution of nonces was too arbitrary.
Now fixed the test to check 3 separate states for accounts during
commit, complete rewound, rewound 1 and rewound 2 (perhaps rewind 2 is
sufficient?).

Co-Authored-By: Gilad Chase <gilad@starkware.com>

crates/mempool/src/mempool.rs

@@ -132,20 +132,32 @@ impl Mempool {
     /// updates account balances).
     #[tracing::instrument(skip(self, args), err)]
     pub fn commit_block(&mut self, args: CommitBlockArgs) -> MempoolResult<()> {
-        let CommitBlockArgs { nonces, tx_hashes } = args;
+        let CommitBlockArgs { nonces: address_to_nonce, tx_hashes } = args;
         tracing::debug!("Committing block with {} transactions to mempool.", tx_hashes.len());
 
         // Align mempool data to committed nonces.
-        for (&address, &nonce) in &nonces {
-            let next_nonce = try_increment_nonce(nonce)?;
+        for (&address, &next_nonce) in &address_to_nonce {
+            // FIXME: Remove after first POC.
+            // If commit_block wants to decrease the stored account nonce this can mean one of two
+            // things:
+            // 1. this is a reorg, which should be handled by a dedicated TBD mechanism and not
+            //    inside commit_block
+            // 2. the stored nonce originated from add_tx, so should be treated as tentative due
+            // to possible races with the gateway; these types of nonces should be tagged somehow
+            // so that commit_block can override them. Regardless, in the first POC this cannot
+            // happen because the GW nonces are always 1.
+            if let Some(&stored_nonce) = self.account_nonces.get(&address) {
+                assert!(stored_nonce <= next_nonce, "NOT SUPPORTED YET {address:?} {next_nonce:?}.")
+            }
+
             let account_state = AccountState { address, nonce: next_nonce };
             self.align_to_account_state(account_state);
         }
         tracing::debug!("Aligned mempool to committed nonces.");
 
         // Rewind nonces of addresses that were not included in block.
         let known_addresses_not_included_in_block =
-            self.mempool_state.keys().filter(|&key| !nonces.contains_key(key));
+            self.mempool_state.keys().filter(|&key| !address_to_nonce.contains_key(key));
         for address in known_addresses_not_included_in_block {
             // Account nonce is the minimal nonce of this address: it was proposed but not included.
             let tx_reference = self

crates/mempool/src/mempool_test.rs

@@ -498,7 +498,7 @@ fn test_commit_block_includes_all_proposed_txs() {
         .build_into_mempool();
 
     // Test.
-    let nonces = [("0x0", 3), ("0x1", 2)];
+    let nonces = [("0x0", 4), ("0x1", 3)];
     let tx_hashes = [1, 4];
     commit_block(&mut mempool, nonces, tx_hashes);
 

crates/mempool/tests/flow_test.rs

@@ -86,7 +86,7 @@ fn test_add_same_nonce_tx_after_previous_not_included_in_block(mut mempool: Memp
         &[tx_nonce_3_account_nonce_3.tx, tx_nonce_4_account_nonce_3.tx.clone()],
     );
 
-    let nonces = [("0x0", 3)]; // Transaction with nonce 4 is not included in the block.
+    let nonces = [("0x0", 4)]; // Transaction with nonce 3 was included, 4 was not.
     let tx_hashes = [1];
     commit_block(&mut mempool, nonces, tx_hashes);
 
@@ -126,28 +126,33 @@ fn test_add_tx_handles_nonces_correctly(mut mempool: Mempool) {
 #[rstest]
 fn test_commit_block_includes_proposed_txs_subset(mut mempool: Mempool) {
     // Setup.
+    let tx_address_0_nonce_1 =
+        add_tx_input!(tx_hash: 1, address: "0x0", tx_nonce: 1, account_nonce: 1);
     let tx_address_0_nonce_3 =
-        add_tx_input!(tx_hash: 1, address: "0x0", tx_nonce: 3, account_nonce: 3);
-    let tx_address_0_nonce_5 =
-        add_tx_input!(tx_hash: 2, address: "0x0", tx_nonce: 5, account_nonce: 3);
-    let tx_address_0_nonce_6 =
-        add_tx_input!(tx_hash: 3, address: "0x0", tx_nonce: 6, account_nonce: 3);
-    let tx_address_1_nonce_0 =
-        add_tx_input!(tx_hash: 4, address: "0x1", tx_nonce: 0, account_nonce: 0);
-    let tx_address_1_nonce_1 =
-        add_tx_input!(tx_hash: 5, address: "0x1", tx_nonce: 1, account_nonce: 0);
+        add_tx_input!(tx_hash: 2, address: "0x0", tx_nonce: 3, account_nonce: 1);
+    let tx_address_0_nonce_4 =
+        add_tx_input!(tx_hash: 3, address: "0x0", tx_nonce: 4, account_nonce: 1);
+
     let tx_address_1_nonce_2 =
-        add_tx_input!(tx_hash: 6, address: "0x1", tx_nonce: 2, account_nonce: 0);
+        add_tx_input!(tx_hash: 4, address: "0x1", tx_nonce: 2, account_nonce: 2);
+    let tx_address_1_nonce_3 =
+        add_tx_input!(tx_hash: 5, address: "0x1", tx_nonce: 3, account_nonce: 2);
+    let tx_address_1_nonce_4 =
+        add_tx_input!(tx_hash: 6, address: "0x1", tx_nonce: 4, account_nonce: 2);
+
+    let tx_address_2_nonce_1 =
+        add_tx_input!(tx_hash: 7, address: "0x2", tx_nonce: 1, account_nonce: 1);
     let tx_address_2_nonce_2 =
-        add_tx_input!(tx_hash: 7, address: "0x2", tx_nonce: 2, account_nonce: 2);
+        add_tx_input!(tx_hash: 8, address: "0x2", tx_nonce: 2, account_nonce: 1);
 
     for input in [
-        &tx_address_0_nonce_5,
-        &tx_address_0_nonce_6,
         &tx_address_0_nonce_3,
+        &tx_address_0_nonce_4,
+        &tx_address_0_nonce_1,
+        &tx_address_1_nonce_4,
+        &tx_address_1_nonce_3,
         &tx_address_1_nonce_2,
-        &tx_address_1_nonce_1,
-        &tx_address_1_nonce_0,
+        &tx_address_2_nonce_1,
         &tx_address_2_nonce_2,
     ] {
         add_tx(&mut mempool, input);
@@ -157,23 +162,28 @@ fn test_commit_block_includes_proposed_txs_subset(mut mempool: Mempool) {
     get_txs_and_assert_expected(
         &mut mempool,
         2,
-        &[tx_address_2_nonce_2.tx.clone(), tx_address_1_nonce_0.tx],
+        &[tx_address_2_nonce_1.tx.clone(), tx_address_1_nonce_2.tx],
     );
     get_txs_and_assert_expected(
         &mut mempool,
-        2,
-        &[tx_address_1_nonce_1.tx.clone(), tx_address_0_nonce_3.tx],
+        4,
+        &[
+            tx_address_2_nonce_2.tx,
+            tx_address_1_nonce_3.tx.clone(),
+            tx_address_0_nonce_1.tx,
+            tx_address_1_nonce_4.tx.clone(),
+        ],
     );
 
-    // Not included in block: address "0x2" nonce 2, address "0x1" nonce 2.
-    let nonces = [("0x0", 3), ("0x1", 1)];
+    // Address 0x0 stays as proposed, address 0x1 rewinds nonce 4, address 0x2 rewinds completely.
+    let nonces = [("0x0", 2), ("0x1", 4)];
     let tx_hashes = [1, 4];
     commit_block(&mut mempool, nonces, tx_hashes);
 
     get_txs_and_assert_expected(
         &mut mempool,
         2,
-        &[tx_address_2_nonce_2.tx, tx_address_1_nonce_2.tx],
+        &[tx_address_2_nonce_1.tx, tx_address_1_nonce_4.tx],
     );
 }
 
@@ -192,7 +202,7 @@ fn test_commit_block_fills_nonce_gap(mut mempool: Mempool) {
 
     get_txs_and_assert_expected(&mut mempool, 2, &[tx_nonce_3_account_nonce_3.tx]);
 
-    let nonces = [("0x0", 4)];
+    let nonces = [("0x0", 5)];
     let tx_hashes = [1, 3];
     commit_block(&mut mempool, nonces, tx_hashes);
 
@@ -226,7 +236,7 @@ fn test_flow_commit_block_rewinds_queued_nonce(mut mempool: Mempool) {
     );
 
     // Test.
-    let nonces = [("0x0", 2)];
+    let nonces = [("0x0", 3)];
     let tx_hashes = [1];
     // Nonce 2 was accepted, but 3 and 4 were not, so are rewound.
     commit_block(&mut mempool, nonces, tx_hashes);
@@ -249,7 +259,7 @@ fn test_flow_commit_block_from_different_leader(mut mempool: Mempool) {
     }
 
     // Test.
-    let nonces = [("0x0", 3), ("0x1", 2)];
+    let nonces = [("0x0", 4), ("0x1", 2)];
     let tx_hashes = [
         1,  // Address 0: known hash accepted for nonce 2.
         99, // Address 0: unknown hash accepted for nonce 3.