refactor: update priority queue to use ThinPriorityTransaction

[MohammadNassar1]

This change is 

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 91.30435% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84.60%. Comparing base (19d8527) to head (8e1f437).

Files	Patch %	Lines
crates/mempool/src/mempool.rs	83.33%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #195      +/-   ##
==========================================
+ Coverage   84.55%   84.60%   +0.04%     
==========================================
  Files          28       28              
  Lines        1360     1364       +4     
  Branches     1360     1364       +4     
==========================================
+ Hits         1150     1154       +4     
  Misses        151      151              
  Partials       59       59              

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

[elintul]

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

---

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

            tx_store: TransactionStore::default(),
            state: HashMap::default(),
        };

Unrelated to this PR, do we want to impl. Default for Mempool?
@giladchase, feel free to answer too.

Code quote:

        let mut mempool = Mempool {
            txs_queue: TransactionPriorityQueue::default(),
            tx_store: TransactionStore::default(),
            state: HashMap::default(),
        };

---

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

            // if the key was not present, otherwise returns the old value while updating
            // the new value.
            let prev_value = mempool.state.insert(input.account.address, input.account.state);

Please unpack earlier.

Code quote:

input.account.address, input.account.state

---

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

            // if the key was not present, otherwise returns the old value while updating
            // the new value.
            let prev_value = mempool.state.insert(input.account.address, input.account.state);

Suggestion:

existing_account_state

---

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

            );

            mempool.txs_queue.push(input.tx.clone().into());

Why this change?

Code quote:

mempool.txs_queue.push(input.tx.clone().into());

[MohammadNassar1]

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

---

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

Previously, elintul (Elin) wrote…

Why this change?

Now I store ThinPriorityTransaction in txs_queue.
And the transaction is stored in tx_store.

[MohammadNassar1]

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

---

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

Previously, elintul (Elin) wrote…

Unrelated to this PR, do we want to impl. Default for Mempool?
@giladchase, feel free to answer too.

Currently empty returns default Mempool.
Do you want to replace it with default trait impl?

[elintul]

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

---

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

Previously, elintul (Elin) wrote…

Please unpack earlier.

Use unpacking syntax instead: let MempoolInput { sender address, state } = mempool_input.

[ayeletstarkware]

Reviewed 6 of 6 files at r5, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @elintul, @giladchase, and @MohammadNassar1)

---

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

            // the new value.
            let address = input.account.sender_address;
            let state = input.account.state;

why is it necessary?

Code quote:

            let address = input.account.sender_address;
            let state = input.account.state;

---

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

        let mut txs: Vec<ThinTransaction> = Vec::default();
        for pq_tx in &pq_txs {
            let tx = self.tx_store.remove(&pq_tx.tx_hash)?;

Why do you delete the txs from the tx_store?

---

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

        .iter()
        .zip(mempool_txs)
        .all(|(a, b)| <ThinTransaction as Into<ThinPriorityTransaction>>::into(a.clone()) == *b);

I find it hard to read
consider changing to something like this:

Code snippet:

fn check_mempool_txs_eq(mempool: &Mempool, expected_txs: &[ThinTransaction]) -> bool {
    let mempool_txs = mempool.txs_queue.iter();

    // Convert and compare transactions
    let transactions_match = expected_txs.iter().zip(mempool_txs).all(|(expected, actual)| {
        let expected_converted: ThinPriorityTransaction = expected.clone().into();
        expected_converted == *actual
    });

    transactions_match
}

---

crates/mempool/src/priority_queue.rs line 26 at r5 (raw file):

}

impl From<Vec<ThinTransaction>> for TransactionPriorityQueue {

Why did you delete it?

---

crates/mempool/src/priority_queue.rs line 28 at r5 (raw file):

#[derive(Clone, Debug, Default)]
pub struct ThinPriorityTransaction {

Why not PrioritizedTransaction? It referred to ThinTransaction before without mentioning it.

Code quote:

ThinPriorityTransaction

---

crates/mempool/src/transaction_store.rs line 2 at r5 (raw file):

use std::collections::HashMap;

Perhaps you could remove the commit - 92bfb61: feat: add transaction store data structure so it doesn't appear to be part of the PR.

[ayeletstarkware]

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

---

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

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Why do you delete the txs from the tx_store?

I guess it's part of inserting it into a staging area? If so, add a todo in this PR

[MohammadNassar1]

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

---

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

Previously, ayeletstarkware (Ayelet Zilber) wrote…

why is it necessary?

Removed

---

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

Previously, ayeletstarkware (Ayelet Zilber) wrote…

I guess it's part of inserting it into a staging area? If so, add a todo in this PR

This Pr doesn't implement staging area.
So currently we delete tx in get_txs.

[MohammadNassar1]

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

---

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

Previously, ayeletstarkware (Ayelet Zilber) wrote…

I find it hard to read
consider changing to something like this:

Done.

---

crates/mempool/src/priority_queue.rs line 26 at r5 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Why did you delete it?

Not in use anymore.

---

crates/mempool/src/priority_queue.rs line 28 at r5 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Why not PrioritizedTransaction? It referred to ThinTransaction before without mentioning it.

Changed back to PrioritizedTransaction

---

crates/mempool/src/transaction_store.rs line 2 at r5 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Perhaps you could remove the commit - 92bfb61: feat: add transaction store data structure so it doesn't appear to be part of the PR.

Done.

[giladchase]

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

---

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

Previously, MohammadNassar1 (mohammad-starkware) wrote…

+1

rust doesn't support format!("{foo.bar}"), or similarly format!("{foo.bar:?}".

It only supports format!("{foo.bar}"), or similarly format!("{foo.bar:?}".

I can't find it in the diff now, but for a while the access to sender address was through tx.sender_address, which made it impossible to use the former form, but now it is possible to use the latter form since sender_address is unpacked into a variable.

---

crates/mempool/src/priority_queue.rs line 29 at r12 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Since it's a pub (crate) file, it needs to be used. Alternatively, I can add "allow dead code" instead.
WDYT?

Let's add to mempool iter_queue() -> Impl Iterator which uses this.

It's a very useful API method to expose, even if we don't use it outside of tests.

[MohammadNassar1]

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

---

crates/mempool/src/priority_queue.rs line 29 at r12 (raw file):

Previously, giladchase wrote…

Let's add to mempool iter_queue() -> Impl Iterator which uses this.

It's a very useful API method to expose, even if we don't use it outside of tests.

What mempool iterator should return? only the txs in priority queue?

[elintul]

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

---

crates/mempool/src/transaction_pool.rs line 32 at r12 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

We return a duplicated error if the transaction already exists.
and panic we added for the case that cannot happen (consistency error) that tx appears in one (account) mapping and doesn't appear in the other (store) mapping.
Then, panic is added in the second if condition.

We should return MempoolError::DuplicateTransaction when the transaction hash is duplicated; it is not what currently happens.

[MohammadNassar1]

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

---

crates/mempool/src/transaction_pool.rs line 32 at r12 (raw file):

Previously, elintul (Elin) wrote…

We should return MempoolError::DuplicateTransaction when the transaction hash is duplicated; it is not what currently happens.

Shouldn't we return a duplication error if we have a transaction from the same account and the same nonce?
We don't support fee escalation, so we don't allow adding another tx with the same nonce.

[giladchase]

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

---

crates/mempool/src/priority_queue.rs line 29 at r12 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

What mempool iterator should return? only the txs in priority queue?

Yep.
That's what id expect iter_queue() to return, I think.
Do you think it's OK?

[MohammadNassar1]

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

---

crates/mempool/src/priority_queue.rs line 29 at r12 (raw file):

Previously, giladchase wrote…

Yep.
That's what id expect iter_queue() to return, I think.
Do you think it's OK?

What about the rest of the txs? (that are in store and not in tx_queue yet)

[giladchase]

Reviewable status: 6 of 11 files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware and @elintul)

---

crates/mempool/src/priority_queue.rs line 29 at r12 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

What about the rest of the txs? (that are in store and not in tx_queue yet)

I think in terms of API, that might be less useful to iterate.
I can see why someone would want to iterate the queue, to inspect what's in store, but iterating over ineligible txs I think would not be as useful.

[MohammadNassar1]

Reviewable status: 6 of 11 files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

crates/mempool/src/priority_queue.rs line 29 at r12 (raw file):

Previously, giladchase wrote…

I think in terms of API, that might be less useful to iterate.
I can see why someone would want to iterate the queue, to inspect what's in store, but iterating over ineligible txs I think would not be as useful.

Done

[elintul]

Reviewed 5 of 5 files at r15, 2 of 2 files at r17, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MohammadNassar1)

---

crates/mempool/src/transaction_pool.rs line 32 at r12 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Shouldn't we return a duplication error if we have a transaction from the same account and the same nonce?
We don't support fee escalation, so we don't allow adding another tx with the same nonce.

This is not exactly the error you're returning; it's not guaranteed that a transaction of same address and nonce will have the same hash. Let's either not implement this part, or add a new error variant. In any case, add a TODO here for fee escalation.

[MohammadNassar1]

Reviewable status: 10 of 11 files reviewed, all discussions resolved (waiting on @elintul)

---

crates/mempool/src/transaction_pool.rs line 32 at r12 (raw file):

Previously, elintul (Elin) wrote…

This is not exactly the error you're returning; it's not guaranteed that a transaction of same address and nonce will have the same hash. Let's either not implement this part, or add a new error variant. In any case, add a TODO here for fee escalation.

Done

[MohammadNassar1]

Reviewable status: 7 of 11 files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

crates/mempool/src/transaction_queue.rs line 51 at r21 (raw file):

}

impl From<Vec<TransactionReference>> for TransactionQueue {

Implemented in another PR

Code quote:

impl From<Vec<TransactionReference>> for TransactionQueue

[MohammadNassar1]

Reviewable status: 7 of 11 files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware, @elintul, and @giladchase)

---

crates/mempool/src/transaction_queue.rs line 51 at r21 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Implemented in another PR

I mean removed in anther PR :)

[elintul]

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

[giladchase]

Reviewed 1 of 5 files at r15, 2 of 3 files at r21, 2 of 2 files at r22, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @MohammadNassar1)

a discussion (no related file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Thanks!
I opened this PR. it rename PriorityTransaction -> QueuedTransaction, and PrioritizedTransaction -> QueuedTransaction. Also changed QueuedTransaction to be private.

---

---

-- commits line 2 at r23:
Note that the new commit message hints that this PR could have been broken down further, into two independent commits.

But let's get this merged as is for now.

Suggestion:

feat: change TransactionReference structure and change the return value of pop_last_chunk

Instead of reviewing PRs, she is having fun in London and hanging out with friends.

[MohammadNassar1]

Reopen the branch