feat(mempool): add pending queue to tx queue

[MohammadNassar1]

This change is 

[elintul]

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

---

crates/mempool/src/transaction_queue.rs line 15 at r1 (raw file):

    priority_queue: BTreeSet<PriorityTransaction>,
    // queue of transaction with low gas price.
    pending_queue: BTreeSet<PendingTransaction>,

Suggestion:

// A queue holding the transaction that with nonces that match account nonces. 
// Note: the derived comparison functionality considers the order guaranteed by the data structures
// used.
#[derive(Debug, Default, Eq, PartialEq)]
pub struct TransactionQueue {
    // Transactions with gas price above base price (sorted by tip).
    priority_queue: BTreeSet<PriorityTransaction>,
    // Transactions with gas price below base price (sorted by price).
    pending_queue: BTreeSet<PendingTransaction>,

---

crates/mempool/src/transaction_queue.rs line 16 at r1 (raw file):

    // queue of transaction with low gas price.
    pending_queue: BTreeSet<PendingTransaction>,
    gas_price_threshold: u128,

Will the mempool need it as well?

Code quote:

gas_price_threshold: u128,

---

crates/mempool/src/transaction_queue.rs line 33 at r1 (raw file):

             time."
        );
        if tx_reference.get_l2_gas_price() < Some(self.gas_price_threshold) {

This shouldn't be Option for >= V3; like tip.

Code quote:

tx_reference.get_l2_gas_price()

---

crates/mempool/src/transaction_queue.rs line 33 at r1 (raw file):

             time."
        );
        if tx_reference.get_l2_gas_price() < Some(self.gas_price_threshold) {

Is there a more ergonomic way that isn't if-let?

Suggestion:

        );
        
        if tx_reference.get_l2_gas_price() < Some(self.gas_price_threshold) {

---

crates/mempool/src/transaction_queue.rs line 35 at r1 (raw file):

        if tx_reference.get_l2_gas_price() < Some(self.gas_price_threshold) {
            assert!(
                self.pending_queue.insert(tx_reference.into()),

Define a variable.

Code quote:

tx_reference.into()

---

crates/mempool/src/transaction_queue.rs line 70 at r1 (raw file):

    /// This is well-defined, since there is at most one transaction per address in the queue.
    pub fn remove(&mut self, address: ContractAddress) -> bool {
        if let Some(tx) = self.address_to_tx.remove(&address) {

Throughout this file, please; tx.clone() makes me wonder if we're cloning full transactions...

Suggestion:

tx_reference

---

crates/mempool/src/transaction_queue.rs line 70 at r1 (raw file):

    /// This is well-defined, since there is at most one transaction per address in the queue.
    pub fn remove(&mut self, address: ContractAddress) -> bool {
        if let Some(tx) = self.address_to_tx.remove(&address) {

How about changing the order, so that false is an early return?

Code quote:

if let Some(tx)

[MohammadNassar1]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

crates/mempool/src/transaction_queue.rs line 16 at r1 (raw file):

Previously, elintul (Elin) wrote…

Will the mempool need it as well?

Currently, I think it's only relevant for the transaction queue.
When the transaction eviction is ready, the mempool or ranking manager must know the gap price threshold.

---

crates/mempool/src/transaction_queue.rs line 70 at r1 (raw file):

Previously, elintul (Elin) wrote…

Throughout this file, please; tx.clone() makes me wonder if we're cloning full transactions...

Done.

[elintul]

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

---

crates/mempool/src/transaction_queue.rs line 16 at r1 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Currently, I think it's only relevant for the transaction queue.
When the transaction eviction is ready, the mempool or ranking manager must know the gap price threshold.

Why u128? I wonder if we should call it base_gas_price, or we might apply some function on it (0.8x)?

[MohammadNassar1]

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

---

crates/mempool/src/transaction_queue.rs line 16 at r1 (raw file):

Previously, elintul (Elin) wrote…

Why u128? I wonder if we should call it base_gas_price, or we might apply some function on it (0.8x)?

-> Why u128?
This is how it defined in resource_bounds.

the 0.8x is for gateway, in tx queue, it must send to Batcher only txs above the gas_price.

---

crates/mempool/src/transaction_queue.rs line 33 at r1 (raw file):

Previously, elintul (Elin) wrote…

This shouldn't be Option for >= V3; like tip.

changed

---

crates/mempool/src/transaction_queue.rs line 35 at r1 (raw file):

Previously, elintul (Elin) wrote…

Define a variable.

Note that the tx_reference.into()in pending and in priority queue have different types.
So, do you suggest to define two variables?

[elintul]

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

---

crates/mempool/src/transaction_queue.rs line 16 at r1 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

-> Why u128?
This is how it defined in resource_bounds.

the 0.8x is for gateway, in tx queue, it must send to Batcher only txs above the gas_price.

Link doesn't work.

---

crates/mempool/src/transaction_queue.rs line 35 at r1 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Note that the tx_reference.into()in pending and in priority queue have different types.
So, do you suggest to define two variables?

Right, nope.

---

crates/mempool/src/transaction_queue.rs line 36 at r3 (raw file):

        if tx_reference.get_l2_gas_price() < self.gas_price_threshold {
            // self.gas_price_threshold} {

[MohammadNassar1]

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

---

crates/mempool/src/transaction_queue.rs line 16 at r1 (raw file):

Previously, elintul (Elin) wrote…

Link doesn't work.

sequencer/crates/starknet_api/src/transaction.rs

Line 884 in 6af5e47

pub max_price_per_unit: u128,

---

crates/mempool/src/transaction_queue.rs line 36 at r3 (raw file):

        if tx_reference.get_l2_gas_price() < self.gas_price_threshold {
            // self.gas_price_threshold} {

Done.

[elintul]

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

---

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

             time."
        );

Can the assert code duplication be avoided? Maybe taking the target queue first?

[MohammadNassar1]

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

---

crates/mempool/src/transaction_queue.rs line 26 at r6 (raw file):

Previously, elintul (Elin) wrote…

Empty, meaning: this address does not exist in the Q?

yes

---

crates/mempool/src/transaction_queue.rs line 39 at r8 (raw file):

Previously, elintul (Elin) wrote…

You should assert that tx_already_existed is None. Let's call it "[optional/maybe]_already_existing_tx".

It should return true, and that's what we check in the assert.

insert docstr:
/// - If the set did not previously contain an equal value, `true` is

/// returned.

/// - If the set already contained an equal value, `false` is returned, and

/// the entry is not updated.

[MohammadNassar1]

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

---

crates/mempool/src/transaction_queue.rs line 39 at r8 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

It should return true, and that's what we check in the assert.

insert docstr:
/// - If the set did not previously contain an equal value, `true` is

/// returned.

/// - If the set already contained an equal value, `false` is returned, and

/// the entry is not updated.

renamed to new_tx_inserted. wdyt?

[giladchase]

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

---

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

Previously, elintul (Elin) wrote…

Please move to Slack.

Resolved on slack, have the changes been updated?

---

crates/mempool/src/transaction_queue.rs line 70 at r9 (raw file):

        self.priority_queue.remove(&tx_reference.clone().into())
            || self.pending_queue.remove(&tx_reference.into())

Who calls this function? what happens when there's a false?

Code quote:

        self.priority_queue.remove(&tx_reference.clone().into())
            || self.pending_queue.remove(&tx_reference.into())

[MohammadNassar1]

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

---

crates/mempool/src/transaction_queue.rs line 70 at r9 (raw file):

Previously, giladchase wrote…

Who calls this function? what happens when there's a false?

It's called in commit_block and add_tx.
Currently, it doesn't check the return value.

[elintul]

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

---

crates/mempool/src/transaction_queue.rs line 39 at r8 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

renamed to new_tx_inserted. wdyt?

Shouldn't I'd be looking here?

---

crates/mempool/src/transaction_queue.rs line 70 at r9 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

It's called in commit_block and add_tx.
Currently, it doesn't check the return value.

So we don't assume the address exists there?
Something to be mindful of when an already-used data structure changes the inside operation of its API, and hint the reviewer it's a thought path you've taken. :)

[ayeletstarkware]

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

---

crates/mempool/src/transaction_queue.rs line 13 at r9 (raw file):

#[derive(Debug, Default, Eq, PartialEq)]
pub struct TransactionQueue {
    // Transactions with gas price above base price (sorted by tip).

above gas price threshold?

Code quote:

above base price

---

crates/mempool/src/transaction_queue.rs line 17 at r9 (raw file):

    // Transactions with gas price below base price (sorted by price).
    pending_queue: BTreeSet<PendingTransaction>,
    gas_price_threshold: u128,

Should this be the first field of the struct? It might make the difference between the queues clearer.

Code quote:

gas_price_threshold: u128,

[MohammadNassar1]

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

---

crates/mempool/src/transaction_queue.rs line 39 at r8 (raw file):

Previously, elintul (Elin) wrote…

Shouldn't I'd be looking here?

We use BTreeSet, here.

---

crates/mempool/src/transaction_queue.rs line 17 at r9 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Should this be the first field of the struct? It might make the difference between the queues clearer.

Good point, updated accordingly

---

crates/mempool/src/transaction_queue.rs line 70 at r9 (raw file):

Previously, elintul (Elin) wrote…

So we don't assume the address exists there?
Something to be mindful of when an already-used data structure changes the inside operation of its API, and hint the reviewer it's a thought path you've taken. :)

Let's add assert in our code.
I'll do it in a separate PR.

[elintul]

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

---

crates/mempool/src/transaction_queue.rs line 70 at r9 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Let's add assert in our code.
I'll do it in a separate PR.

To assert what?

---

crates/mempool/src/transaction_queue.rs line 33 at r10 (raw file):

        );

        let new_tx_inserted = if tx_reference.get_l2_gas_price() < self.gas_price_threshold {

Suggestion:

new_tx_successfully_inserted

[MohammadNassar1]

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

---

crates/mempool/src/transaction_queue.rs line 70 at r9 (raw file):

Previously, elintul (Elin) wrote…

To assert what?

That remove returns true.

[elintul]

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

---

crates/mempool/src/transaction_queue.rs line 70 at r9 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

That remove returns true.

Do we care about the result?

[giladchase]

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

---

crates/mempool/src/transaction_queue.rs line 70 at r9 (raw file):

Previously, elintul (Elin) wrote…

Do we care about the result?

I'm not sure we do 🤔

Returning a result and asserting in the callsites:
As a pure DS it makes sense to return bool here, also this DS doesn't really need to know about our assumption that this removal can never fail in order to do its job.

Not returning a result and asserting inside the remove:
Documents our assumption that removals never fail in a central location, which prevents future callsites or refactorings of the callsites from violating this assertion (for example if someone calls this method in a test without asserting the result).
Less duplication of assertions in the callsites.

[MohammadNassar1]

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

---

crates/mempool/src/transaction_queue.rs line 70 at r9 (raw file):

Previously, giladchase wrote…

I'm not sure we do 🤔

Returning a result and asserting in the callsites:
As a pure DS it makes sense to return bool here, also this DS doesn't really need to know about our assumption that this removal can never fail in order to do its job.

Not returning a result and asserting inside the remove:
Documents our assumption that removals never fail in a central location, which prevents future callsites or refactorings of the callsites from violating this assertion (for example if someone calls this method in a test without asserting the result).
Less duplication of assertions in the callsites.

I think that as a pure DS, we should return a boolean value.
We can add an assertion in the mempool.rs code to ensure the consistency between the transaction pool and transaction queue. Specifically, we should check that the queue contains the transaction, and if so, the remove operation should return true.

[giladchase]

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

---

crates/mempool/src/transaction_queue.rs line 70 at r9 (raw file):

Specifically, we should check that the queue contains the transaction, and if so, the remove operation should return true.

Is that the intended behavior in all callsites of this function?

[MohammadNassar1]

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

---

crates/mempool/src/transaction_queue.rs line 70 at r9 (raw file):

Previously, giladchase wrote…

Specifically, we should check that the queue contains the transaction, and if so, the remove operation should return true.

Is that the intended behavior in all callsites of this function?

As mentioned earlier, we currently call this method in two places, and it should return true. To ensure consistency, we can add a check.

[giladchase]

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

---

crates/mempool/src/transaction_queue.rs line 70 at r9 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

As mentioned earlier, we currently call this method in two places, and it should return true. To ensure consistency, we can add a check.

You mentioned add_tx and commit_block, does the remove in commit_block always return true?

[MohammadNassar1]

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

---

crates/mempool/src/transaction_queue.rs line 70 at r9 (raw file):

Previously, giladchase wrote…

You mentioned add_tx and commit_block, does the remove in commit_block always return true?

You are correct, in commit_block it may return false.
While in add_tx it should always return true.

[ayeletstarkware]

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

[MohammadNassar1]

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

---

crates/mempool/src/transaction_queue.rs line 70 at r9 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

You are correct, in commit_block it may return false.
While in add_tx it should always return true.

Added an assert in align_to_account_state in a separate PR.

[giladchase]

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