test(mempool): test_add_tx_fills_nonce_gap

[MohammadNassar1]

This change is 

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.87%. Comparing base (a7accbd) to head (c3cb8a5).

Additional details and impacted files

@@                           Coverage Diff                            @@
##           mohammad/mempool/fix/add-tx/input-state     #511   +/-   ##
========================================================================
  Coverage                                    81.87%   81.87%           
========================================================================
  Files                                           42       42           
  Lines                                         1826     1826           
  Branches                                      1826     1826           
========================================================================
  Hits                                          1495     1495           
  Misses                                         256      256           
  Partials                                        75       75           

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

[elintul]

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

---

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

    add_tx(&mut mempool, &tx_input_nonce_2);
    assert_eq_mempool_queue(&mempool, &[tx_input_nonce_1.tx]);
}

Make sure Ayelet haven't written this case already.

Suggestion:

fn test_add_tx_fills_hole(mut mempool: Mempool) {
    // Setup.
    let tx_input_nonce_1 = add_tx_input!(tx_hash: 1, tx_nonce: 1_u8, account_nonce: 0_u8);
    // Input that increments the account state.
    let tx_input_nonce_2 = add_tx_input!(tx_hash: 2, tx_nonce: 2_u8, account_nonce: 1_u8);
    let tx_input_nonce_3 = add_tx_input!(tx_hash: 3, tx_nonce: 3_u8, account_nonce: 0_u8);

    // Test and assert.
    
    // First, with gap.
    add_tx(&mut mempool, &tx_input_nonce_1);
    add_tx(&mut mempool, &tx_input_nonce_3);
    assert_eq_mempool_queue(&mempool, &[]);

    // Then, fill it.
    add_tx(&mut mempool, &tx_input_nonce_2);
    assert_eq_mempool_queue(&mempool, &[tx_input_nonce_1.tx]);
}

[MohammadNassar1]

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

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

Previously, elintul (Elin) wrote…

Make sure Ayelet haven't written this case already.

Done.
I don't see a similar test.
@ayeletstarkware, can you please approve?

[elintul]

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

[ayeletstarkware]

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

---

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

    add_tx(&mut mempool, &tx_input_nonce_3);
    // Queue is empty due to a nonce gap.
    assert_eq_mempool_queue(&mempool, &[]);

add a todo to use mempool state

---

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

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Done.
I don't see a similar test.
@ayeletstarkware, can you please approve?

I haven't written this case. But I think the test's name could be more informative by indicating that the hole is filled from a transaction carrying a more recent account state.

[MohammadNassar1]

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

---

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

Previously, ayeletstarkware (Ayelet Zilber) wrote…

add a todo to use mempool state

Why I need to use mempool state?

---

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

Previously, ayeletstarkware (Ayelet Zilber) wrote…

I haven't written this case. But I think the test's name could be more informative by indicating that the hole is filled from a transaction carrying a more recent account state.

Changed to test_add_tx_account_state_fills_hole.

[elintul]

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

---

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

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Changed to test_add_tx_account_state_fills_hole.

That's the only way add_tx can close a hole, so seems redundant to me. Your call.

[ayeletstarkware]

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

---

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

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Why I need to use mempool state?

assert_eq_mempool_queue will be deleted soon. we will use mempool partial state to check mempool queue instead.

[MohammadNassar1]

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

---

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

Previously, elintul (Elin) wrote…

That's the only way add_tx can close a hole, so seems redundant to me. Your call.

This scenario can close a hole:

1. AddTx(Tx(nonce=1), State(0))
2. AddTx(Tx(nonce=0), State(0))

The first tx would create a hole.
The second tx would close it.

[elintul]

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

---

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

Previously, MohammadNassar1 (mohammad-starkware) wrote…

This scenario can close a hole:

1. AddTx(Tx(nonce=1), State(0))
2. AddTx(Tx(nonce=0), State(0))

The first tx would create a hole.
The second tx would close it.

Right. 🤦

[MohammadNassar1]

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

---

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

Previously, ayeletstarkware (Ayelet Zilber) wrote…

assert_eq_mempool_queue will be deleted soon. we will use mempool partial state to check mempool queue instead.

Added

[elintul]

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

[elintul]

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

[elintul]

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

---

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

        if tx.nonce < nonce {
            return Err(MempoolError::DuplicateTransaction { tx_hash: tx.tx_hash });
        }

Remove.

Code quote:

        // Invalid input.
        // TODO(Mohammad): use `should_insert` method.
        if tx.nonce < nonce {
            return Err(MempoolError::DuplicateTransaction { tx_hash: tx.tx_hash });
        }

[MohammadNassar1]

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

---

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

Previously, elintul (Elin) wrote…

Remove.

Done.

[elintul]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @giladchase)