feat(starknet_mempool): remove old transaction from the mempool in get_txs

[dafnamatsry]

No description provided.

[reviewable-StarkWare]

This change is 

[dafnamatsry]

• feat(starknet_mempool): remove old transaction from the mempool in get_txs #4115 👈 (View in Graphite)
• feat(starknet_mempool): clean old transactions in add_tx #4050
• feat(starknet_mempool): add remove old transaction API to TransactionPool #4029
• refactor(starknet_mempool): small refactor to memppol_test #3996
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[alonh5]

Reviewed 3 of 3 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alonh5 and @ayeletstarkware)

---

crates/starknet_mempool/src/mempool_test.rs line 977 at r2 (raw file):

    // Only the second transaction should be returned from get_txs, and the first should be removed.
    assert_eq!(mempool.get_txs(2).unwrap(), vec![second_tx.tx.clone()]);

This isn't testing the get_txs removal because you're calling add_tx before.

Code quote:

    // Only the second transaction should be returned from get_txs, and the first should be removed.
    assert_eq!(mempool.get_txs(2).unwrap(), vec![second_tx.tx.clone()]);

---

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

                    .remove(tx.tx_hash)
                    .expect("Transaction hash from queue must appear in pool.");
            }

Consider extracting into function.

Code quote:

            // Divide the chunk into transactions that are old and no longer valid and those that
            // remain valid.
            let (old_txs, valid_txs): (Vec<_>, Vec<_>) = chunk.into_iter().partition(|tx| {
                self.tx_pool
                    .get_submission_time(tx.tx_hash)
                    .expect("Transaction hash from queue must appear in pool.")
                    .elapsed()
                    > self.config.transaction_ttl
            });

            // Remove old transactions from the pool.
            for tx in old_txs {
                self.tx_pool
                    .remove(tx.tx_hash)
                    .expect("Transaction hash from queue must appear in pool.");
            }

[dafnamatsry]

Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @alonh5 and @ayeletstarkware)

---

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

Previously, alonh5 (Alon Haramati) wrote…

Consider extracting into function.

Done.

[alonh5]

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

---

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

    // Given a chunk of transactions, removes from the pool those that are old, and returns the
    // remaining valid ones.
    // Note: This function assumes that the given transactions were already removed from the queue.

Suggestion:

    /// Given a chunk of transactions, removes from the pool those that are old, and returns the
    /// remaining valid ones.
    /// Note: This function assumes that the given transactions were already removed from the queue.

[dafnamatsry]

Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @alonh5 and @ayeletstarkware)

---

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

    // Given a chunk of transactions, removes from the pool those that are old, and returns the
    // remaining valid ones.
    // Note: This function assumes that the given transactions were already removed from the queue.

Done.

[alonh5]

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

[dafnamatsry]

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

---

crates/starknet_mempool/src/mempool_test.rs line 977 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

This isn't testing the get_txs removal because you're calling add_tx before.

Right! thanks :)
Had to change the mempool a bit, to use the clock.now()

[alonh5]

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

---

crates/starknet_mempool/src/mempool.rs line 431 at r6 (raw file):

            let tx_submission_time = self
                .tx_pool
                .get_submission_time(tx.tx_hash)

Instead of cloning the clock and having it in both places, how about you implement is_expired for tx pool instead of get_submission_time?

[dafnamatsry]

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

---

crates/starknet_mempool/src/mempool.rs line 431 at r6 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Instead of cloning the clock and having it in both places, how about you implement is_expired for tx pool instead of get_submission_time?

It would require implementing it both in TransactionPool and TimedTransactionMap. I think it's easier to just hold the clock. Might be useful for future things we might have as well.

[alonh5]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ayeletstarkware)