feat: add tx_pool checker for mempool test

[giladchase]

• Requires adding accessors to the private tx_pools; these should not be
  used outside of our own tests.

• Didn't add a standalone tx_pool checker, can't think of a reason to
  test the pool without the queue (but the reverse is sensible
  somewhat).

• New tester will soon be used to test multi-nonce

commit-id:ba871431

---

Stack:

• feat: add multi-nonce support #429
• feat: add tx_pool checker for mempool test #428 ⬅

⚠️ Part of a stack created by spr. Do not merge manually using the UI - doing so may have unexpected results.

---

This change is 

[giladchase]

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

---

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

// TODO(Ayelet): replace with MempoolState checker.
#[track_caller]
fn _check_mempool_state_eq(

FYI @ayeletstarkware i had to add this in order to properly test multi-nonce, if this is merged the intention is for you to kill it once you have something better.

Code quote:

// TODO(Ayelet): replace with MempoolState checker.
#[track_caller]
fn _check_mempool_state_eq(

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 83.31%. Comparing base (294f10c) to head (da4a07d).

Files	Patch %	Lines
crates/mempool/src/mempool.rs	0.00%	3 Missing ⚠️
crates/mempool/src/transaction_pool.rs	0.00%	3 Missing ⚠️

Additional details and impacted files

@@                  Coverage Diff                  @@
##           spr/main/4b2c49c7     #428      +/-   ##
=====================================================
- Coverage              83.60%   83.31%   -0.30%     
=====================================================
  Files                     37       37              
  Lines                   1720     1726       +6     
  Branches                1720     1726       +6     
=====================================================
  Hits                    1438     1438              
- Misses                   205      211       +6     
  Partials                  77       77              

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

[elintul]

Reviewed 3 of 3 files at r2.
Reviewable status: 2 of 3 files reviewed, 6 unresolved discussions (waiting on @ayeletstarkware and @giladchase)

---

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

    #[cfg(test)]
    pub(crate) fn _tx_pool(&self) -> &TransactionPool {

Temporary name until used?

Code quote:

_tx_pool

---

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

// TODO(Ayelet): replace with MempoolState checker.
#[track_caller]
fn _check_mempool_state_eq(

Suggestion:

_verify_mempool_state

---

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

fn _check_mempool_state_eq(
    mempool: &Mempool,
    expected_txs: &[ThinTransaction],

x2

Suggestion:

expected_pool

---

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

    check_mempool_queue_eq(mempool, expected_queue);

    let expected_txs: HashMap<_, _> =

Do you think it's better here than a turbofish after collect?

Code quote:

: HashMap<_, _>

---

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

        expected_txs.iter().cloned().map(|tx| (tx.tx_hash, tx)).collect();

    assert_eq!(mempool._tx_pool()._tx_pool(), &expected_txs);

Suggestion:

        expected_txs.iter().cloned().map(|tx| (tx.tx_hash, tx)).collect();
    assert_eq!(mempool._tx_pool()._tx_pool(), &expected_txs);

---

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

    #[cfg(test)]
    pub(crate) fn _tx_pool(&self) -> &HashMap<TransactionHash, ThinTransaction> {

Consider an alias: HashToTransaction, TransactionMapping?
BTW, how come no lifetime parameter is needed? Is it because the lifetime is unambiguous and tied to self's lifetime?

Code quote:

HashMap<TransactionHash, ThinTransaction>

[elintul]

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

[elintul]

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

[elintul]

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

[giladchase]

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

---

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

Previously, elintul (Elin) wrote…

Temporary name until used?

yep, used in next PR

---

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

// TODO(Ayelet): replace with MempoolState checker.
#[track_caller]
fn _check_mempool_state_eq(

changed to assert after offline

---

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

Previously, elintul (Elin) wrote…

Do you think it's better here than a turbofish after collect?

Ya i prefer this over the fish, it's somewhat less readable IMO.
Also it's easier to ignore this way, whereas the fish is injected inside the logic itself, and is harder to ignore.

---

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

Consider an alias: HashToTransaction, TransactionMapping?

Done

BTW, how come no lifetime parameter is needed? Is it because the lifetime is unambiguous and tied to self's lifetime?

yep

[elintul]

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