feat(starknet_mempool): add remove old transaction API to TransactionPool

[dafnamatsry]

No description provided.

[reviewable-StarkWare]

This change is 

[dafnamatsry]

• feat(starknet_mempool): remove old transaction from the mempool in get_txs #4115
• feat(starknet_mempool): clean old transactions in add_tx #4050
• feat(starknet_mempool): add remove old transaction API to TransactionPool #4029 👈 (View in Graphite)
• 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 5 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ayeletstarkware)

---

crates/starknet_mempool/src/transaction_pool.rs line 27 at r3 (raw file):

    txs_by_account: AccountTransactionIndex,
    // Transactions sorted by their time spent in the pool, i.e., in descending order of submission
    // time.

Took me a minute to understand this. Maybe this is easier to compile?

Suggestion:

    // Transactions sorted by their time spent in the pool (i.e. newest to oldest).

---

crates/starknet_mempool/src/transaction_pool.rs line 130 at r3 (raw file):

    }

    // Rmoves the given `removed_txs` from all the pool mappings, except for the one specified in

You should add the Code Spell Checker extension to your vscode.

Suggestion:

Removes

---

crates/starknet_mempool/src/transaction_pool.rs line 135 at r3 (raw file):

        &mut self,
        removed_txs: &Vec<TransactionReference>,
        skip_mapping: PoolMappings,

Maybe have three separate functions, one for each mapping, and call two of them in each place?
I think it's a bit more straight forward and readable.

---

crates/starknet_mempool/src/transaction_pool.rs line 273 at r3 (raw file):

}

impl Eq for SubmissionID {}

Can't the be derived?

Code quote:

impl PartialEq for SubmissionID {
    fn eq(&self, other: &Self) -> bool {
        self.submission_time == other.submission_time && self.tx_hash == other.tx_hash
    }
}

impl Eq for SubmissionID {}

---

crates/starknet_mempool/src/transaction_pool.rs line 278 at r3 (raw file):

// that has been in the pool for a longer time will be considered "greater" than a transaction that
// has been in the pool for a shorter time.
impl Ord for SubmissionID {

Why do yo actually need this reverse comparison? Can't you remove the first entries instead of the last? In a BTreeMap it should be the same complexity.

---

crates/starknet_mempool/src/transaction_pool.rs line 295 at r3 (raw file):

#[derive(Debug, Default, Eq, PartialEq)]
struct TimedTransactionMap {
    txs_by_submission_time: BTreeMap<SubmissionID, TransactionReference>,

Do you ever actually need the tx reference here?

---

crates/starknet_mempool/src/transaction_pool.rs line 301 at r3 (raw file):

impl TimedTransactionMap {
    /// If the transaction with the same transaction hash already exists in the mapping, the old
    /// submission ID is returned.

I don't think this can happen in the flow, it will error with DuplicateTransaction. Maybe add an assertion?

Code quote:

    /// If the transaction with the same transaction hash already exists in the mapping, the old
    /// submission ID is returned.

[dafnamatsry]

Reviewable status: 1 of 5 files reviewed, 7 unresolved discussions (waiting on @alonh5 and @ayeletstarkware)

---

crates/starknet_mempool/src/transaction_pool.rs line 27 at r3 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Took me a minute to understand this. Maybe this is easier to compile?

Done.

---

crates/starknet_mempool/src/transaction_pool.rs line 130 at r3 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

You should add the Code Spell Checker extension to your vscode.

Done. (And installed)

---

crates/starknet_mempool/src/transaction_pool.rs line 135 at r3 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Maybe have three separate functions, one for each mapping, and call two of them in each place?
I think it's a bit more straight forward and readable.

Done.
I agree it's more strait forward. The downside is that there is no one place that ensures all the mappings are aligned.

---

crates/starknet_mempool/src/transaction_pool.rs line 273 at r3 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Can't the be derived?

Done.

---

crates/starknet_mempool/src/transaction_pool.rs line 278 at r3 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Why do yo actually need this reverse comparison? Can't you remove the first entries instead of the last? In a BTreeMap it should be the same complexity.

The split_off function I use removes the greatest entries (and I did't find an equivalent for the smallest).

I could have removed elements one by one, but I think the split_off is more efficient.

---

crates/starknet_mempool/src/transaction_pool.rs line 295 at r3 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Do you ever actually need the tx reference here?

It is used as the return value of remove_txs_older_than, which is later used to remove from the account index. And will also be used to remove from the queue in the next PR.

---

crates/starknet_mempool/src/transaction_pool.rs line 301 at r3 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

I don't think this can happen in the flow, it will error with DuplicateTransaction. Maybe add an assertion?

I did the same as in AccountTransactionIndex to be consistent.
There is an assertion above, when calling this function.

[alonh5]

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

---

crates/starknet_mempool/src/transaction_pool.rs line 295 at r3 (raw file):

Previously, dafnamatsry wrote…

It is used as the return value of remove_txs_older_than, which is later used to remove from the account index. And will also be used to remove from the queue in the next PR.

Got it, thanks.

---

crates/starknet_mempool/src/transaction_pool.rs line 301 at r3 (raw file):

Previously, dafnamatsry wrote…

I did the same as in AccountTransactionIndex to be consistent.
There is an assertion above, when calling this function.

Oh right. Don't you think it's better to assert inside the function and not return anything? For both

[dafnamatsry]

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

---

crates/starknet_mempool/src/transaction_pool.rs line 301 at r3 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Oh right. Don't you think it's better to assert inside the function and not return anything? For both

I personally like it this way, it makes these maps more generic (the behaviour is just like HashMap).
But either way is fine I think...

[alonh5]

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

---

crates/starknet_mempool/src/transaction_pool.rs line 301 at r3 (raw file):

Previously, dafnamatsry wrote…

I personally like it this way, it makes these maps more generic (the behaviour is just like HashMap).
But either way is fine I think...

Okay, not crucial for me.

[ayeletstarkware]

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

---

crates/starknet_mempool/src/transaction_pool.rs line 90 at r4 (raw file):

        self.remove_from_timed_mapping(&removed_txs);

        self.capacity.remove_n(removed_txs.len());

Since those removals are related, should we merge them into a single remove_transactions function? and use it in remove_txs_older_than as well

Code quote:

        self.remove_from_main_mapping(&removed_txs);
        self.remove_from_timed_mapping(&removed_txs);

        self.capacity.remove_n(removed_txs.len());

---

crates/starknet_mempool/src/transaction_pool.rs line 294 at r4 (raw file):

impl TimedTransactionMap {
    /// If the transaction with the same transaction hash already exists in the mapping, the old
    /// submission ID is returned.

Does this scenario occur when a transaction is submitted with a higher fee?

Suggestion:

/// If a transaction with the same transaction hash already exists in the mapping, the previous
/// submission ID is returned.

---

crates/starknet_mempool/src/transaction_pool.rs line 311 at r4 (raw file):

    /// Removes all transactions that were submitted to the pool before the given duration.
    #[allow(dead_code)]
    pub fn remove_txs_older_than(&mut self, duration: Duration) -> Vec<TransactionReference> {

Is this function tested in the next PRs?

[dafnamatsry]

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

---

crates/starknet_mempool/src/transaction_pool.rs line 90 at r4 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Since those removals are related, should we merge them into a single remove_transactions function? and use it in remove_txs_older_than as well

That's what I did in the beginning, and Alon asked to split it (you can see the previous revisions).
Note that every public remove calls 2 different remove_from_.. functions.

---

crates/starknet_mempool/src/transaction_pool.rs line 294 at r4 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Does this scenario occur when a transaction is submitted with a higher fee?

Yes, the tx is inserted to all the mappings like any other transaction.
A new submission time will be associated with the new tx.

---

crates/starknet_mempool/src/transaction_pool.rs line 311 at r4 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Is this function tested in the next PRs?

Yes!

[ayeletstarkware]

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