test(mempool): add mempool state struct and new function

[ayeletstarkware]

This change is 

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.83%. Comparing base (7c08d83) to head (6b7b20a).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #399      +/-   ##
==========================================
- Coverage   82.89%   82.83%   -0.06%     
==========================================
  Files          37       37              
  Lines        1719     1719              
  Branches     1719     1719              
==========================================
- Hits         1425     1424       -1     
- Misses        217      218       +1     
  Partials       77       77              

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

[MohammadNassar1]

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

---

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

    tx_pool: TransactionPool,
    tx_queue: TransactionQueue,
}

Is it only used in mempool_test? Do we have scenarios to use this util in E2E or in other files?

Code quote:

struct MempoolState {
    tx_pool: TransactionPool,
    tx_queue: TransactionQueue,
}

---

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

        for tx in pool_txs {
            tx_pool.insert(tx).unwrap();
        }

Please implement from([ThinTx]) method.
It can be done in a separate PR.

Suggestion:

        let mut tx_pool = TransactionPool::from(pool_txs);

---

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

        for tx in queue_txs {
            tx_queue.insert(TransactionReference::new(&tx));
        }

The same.

Code quote:

        let mut tx_queue = TransactionQueue::default();
        for tx in queue_txs {
            tx_queue.insert(TransactionReference::new(&tx));
        }

---

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

    let n_txs = txs.len();

    let mempool_state = MempoolState::new(txs.clone(), txs);

Suggestion:

    let txs = [
        input_tip_50_address_0.clone().tx,
        input_tip_100_address_1.clone().tx,
        input_tip_10_address_2.clone().tx,
    ];
    let n_txs = txs.len();

    let mempool_state = MempoolState::new(txs.clone(), txs.to_vec());

---

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

    let mempool_state = MempoolState::new(txs.clone(), txs);
    let mut mempool: Mempool = mempool_state.into();

Suggestion:

        let mut mempool: Mempool = MempoolState::new(txs.clone(), txs).into();

[elintul]

Reviewed all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ayeletstarkware)

---

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

use crate::transaction_queue::TransactionQueue;

struct MempoolState {

Shortly document.

Code quote:

MempoolState

---

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

impl From<MempoolState> for Mempool {
    fn from(state: MempoolState) -> Mempool {

state by itself is confusing, since we use it in many other locations.

Suggestion:

mempool_state: MempoolState

---

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

impl From<MempoolState> for Mempool {
    fn from(state: MempoolState) -> Mempool {
        Mempool { tx_pool: state.tx_pool, tx_queue: state.tx_queue }

Unpack state first.

Code quote:

tx_pool: state.tx_pool, tx_queue: state.tx_queue

[ayeletstarkware]

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

---

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

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Is it only used in mempool_test? Do we have scenarios to use this util in E2E or in other files?

IDK yet. For now, it's for unit test cases.

---

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

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Please implement from([ThinTx]) method.
It can be done in a separate PR.

Done + #419

---

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

Previously, MohammadNassar1 (mohammad-starkware) wrote…

The same.

Done + #413

---

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

    let n_txs = txs.len();

    let mempool_state = MempoolState::new(txs.clone(), txs);

Done, but why is that better?

[elintul]

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

[elintul]

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

---

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

/// `MempoolState` is a utility struct for testing mempool scenarios. It allows the creation of
/// different mempool states, without following the regular mempool behavior.

Suggestion:

/// Represents the internal state of the mempool.
/// Enables customized (and potentially inconsistent) creation for unit testing.

[elintul]

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

---

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

    {
        let tx_pool = pool_txs.into_iter().collect();
        let tx_queue = queue_txs.into_iter().collect();

Add type annotations?

Code quote:

        let tx_pool = pool_txs.into_iter().collect();
        let tx_queue = queue_txs.into_iter().collect();

---

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

    // This ensures we do not exceed the number of transactions available in the mempool.
    let max_requested_txs = requested_txs.min(txs_input.len());

Maybe use MempoolState for the assertions in the end as well?

[ayeletstarkware]

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

---

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

Previously, elintul (Elin) wrote…

Maybe use MempoolState for the assertions in the end as well?

yep, it's the next step
done in #431

[elintul]

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

---

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

        txs_input[0].clone(), // tip 50
        txs_input[2].clone(), // tip 10
    ];

Suggestion:

    let add_tx_inputs = [
        add_tx_input!(tip: 50, tx_hash: 1),
        add_tx_input!(tip: 100, tx_hash: 2, sender_address: "0x1"),
        add_tx_input!(tip: 10, tx_hash: 3, sender_address: "0x2"),
    ];
    let txs_iterator = txs_input.iter().map(|input| &input.tx);
    let tx_references_iterator = txs_iter.map(TransactionReference::new);

[ayeletstarkware]

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

---

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

        txs_input[0].clone(), // tip 50
        txs_input[2].clone(), // tip 10
    ];

It doesn't work. I'm leaving a TODO and will get back to it later.

[elintul]

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

---

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

Previously, ayeletstarkware (Ayelet Zilber) wrote…

It doesn't work. I'm leaving a TODO and will get back to it later.

"It doesn't work"
Please explain why / add compilation errors (can be in Slack), so we'd understand why.
Also, note the variable names I suggested.

[MohammadNassar1]

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

[MohammadNassar1]

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

---

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

fn test_get_txs(#[case] requested_txs: usize) {
    // TODO(Ayelet): Avoid cloning the transactions in the test.
    let add_tx_input = [

Suggestion:

add_tx_inputs

[ayeletstarkware]

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

---

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

Previously, elintul (Elin) wrote…

"It doesn't work"
Please explain why / add compilation errors (can be in Slack), so we'd understand why.
Also, note the variable names I suggested.

I'm working on reducing cloning in the following PR (when MempoolState's assert_eq_mempool_state replaces assert_eq_mempool_queue) #431 (not ready yet).
Also, I fixed the names.

[elintul]

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

---

crates/mempool/src/mempool_test.rs line 155 at r7 (raw file):

        add_tx_inputs[0].tx.clone(), // tip 50
        add_tx_inputs[2].tx.clone(), // tip 10
    ];

Let's sort in-place, as discussed offline.

Code quote:

    let sorted_txs = [
        add_tx_inputs[1].tx.clone(), // tip 100
        add_tx_inputs[0].tx.clone(), // tip 50
        add_tx_inputs[2].tx.clone(), // tip 10
    ];