chore(mempool): add transaction queue content

[MohammadNassar1]

This change is 

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 3.30%. Comparing base (f68bd90) to head (cff617d).
Report is 23 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #886       +/-   ##
==========================================
- Coverage   74.18%   3.30%   -70.88%     
==========================================
  Files         359      87      -272     
  Lines       36240   11276    -24964     
  Branches    36240   11276    -24964     
==========================================
- Hits        26884     373    -26511     
- Misses       7220   10885     +3665     
+ Partials     2136      18     -2118     

Flag	Coverage Δ
	3.30% <ø> (-70.88%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[MohammadNassar1]

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

---

crates/mempool/src/transaction_queue_test_utils.rs line 17 at r1 (raw file):

    priority_queue: Option<BTreeSet<PriorityTransaction>>,
    pending_queue: Option<BTreeSet<PendingTransaction>>,
    address_to_tx: Option<HashMap<ContractAddress, TransactionReference>>,

Why address_to_tx is part of the content?

The TransactionQueueContent is used to test the TransactinoQueue functionality.
I think we need to check this field as well, as it can cover a potential bug. For example, when the pending/priority transaction queues are aligned, but for some reason, the tx_to_address is not.

Code quote:

 address_to_tx: Option<HashMap<ContractAddress, TransactionReference>>,

[MohammadNassar1]

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

---

crates/mempool/src/transaction_queue_test_utils.rs line 53 at r1 (raw file):

        P: IntoIterator<Item = TransactionReference> + Copy,
    {
        self._priority_queue = Some(priority_txs.into_iter().map(Into::into).collect());

Is collect needed?

Answer:
Yes, to collect an iterator to a set.

Code quote:

collect()

[MohammadNassar1]

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

---

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

        assert_eq!(self.pending_queue.as_ref().unwrap(), &tx_queue.pending_queue);
        assert_eq!(self.address_to_tx.as_ref().unwrap(), &tx_queue.address_to_tx);
    }

Why gas_price_threshold is not checked:
Since it's an input for the Mempool and there is no code within the Mempool that modifies its value.

Code quote:

    pub fn _assert_eq_priority_queue(&self, tx_queue: &TransactionQueue) {
        assert_eq!(self.priority_queue.as_ref().unwrap(), &tx_queue.priority_queue);
        assert_eq!(self.address_to_tx.as_ref().unwrap(), &tx_queue.address_to_tx);
    }

    pub fn _assert_eq_pending_queue(&self, tx_queue: &TransactionQueue) {
        assert_eq!(self.pending_queue.as_ref().unwrap(), &tx_queue.pending_queue);
        assert_eq!(self.address_to_tx.as_ref().unwrap(), &tx_queue.address_to_tx);
    }

[elintul]

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

---

crates/mempool/src/transaction_queue_test_utils.rs line 17 at r1 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Why address_to_tx is part of the content?

The TransactionQueueContent is used to test the TransactinoQueue functionality.
I think we need to check this field as well, as it can cover a potential bug. For example, when the pending/priority transaction queues are aligned, but for some reason, the tx_to_address is not.

Have we checked this until now?

---

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

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Why gas_price_threshold is not checked:
Since it's an input for the Mempool and there is no code within the Mempool that modifies its value.

Then why is it needed in the content?

---

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

        P: IntoIterator<Item = TransactionReference>,
    {
        self._pending_queue = Some(pending_txs.into_iter().map(Into::into).collect());

Not clear, from is better.

Code quote:

Into::into

---

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

        self._address_to_tx.get_or_insert_with(HashMap::new).extend(
            self._pending_queue.as_ref().unwrap().iter().map(|tx| (tx.sender_address, tx.0)),
        );

Very unreadable + duplicated, there must be a better way.

Code quote:

        self._address_to_tx.get_or_insert_with(HashMap::new).extend(
            self._pending_queue.as_ref().unwrap().iter().map(|tx| (tx.sender_address, tx.0)),
        );

[MohammadNassar1]

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

---

crates/mempool/src/transaction_queue_test_utils.rs line 17 at r1 (raw file):

Previously, elintul (Elin) wrote…

Have we checked this until now?

It hasn't been checked until now, but I think it should be checked based on the reason I wrote above.

[elintul]

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

---

crates/mempool/src/transaction_queue_test_utils.rs line 17 at r1 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

It hasn't been checked until now, but I think it should be checked based on the reason I wrote above.

Are you sure it hasn't been checked until now?

[MohammadNassar1]

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

---

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

Previously, elintul (Elin) wrote…

Then why is it needed in the content?

Assert for gas_price_threshold is not needed.
However, with_gas_price_threshold is necessary in the tests because we need to initialize a mempool with a specific gas price threshold in some tests.

[MohammadNassar1]

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

---

crates/mempool/src/transaction_queue_test_utils.rs line 17 at r1 (raw file):

Previously, elintul (Elin) wrote…

Are you sure it hasn't been checked until now?

Ooooh sorry,
it was checked here:

Code snippet:

    fn assert_eq_transaction_queue_content(&self, mempool: &Mempool) {
        assert_eq!(self.tx_queue.as_ref().unwrap(), &mempool.tx_queue);
    }

[MohammadNassar1]

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

---

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

Previously, elintul (Elin) wrote…

Not clear, from is better.

Done.

---

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

Previously, elintul (Elin) wrote…

Very unreadable + duplicated, there must be a better way.

To make it more readable, I can split it into two lines.

For the duplication, I assume you mean the duplication with priority_queue code.
Note that priority queue hold different type of transactions, so

[MohammadNassar1]

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

---

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

impl TransactionQueueContentBuilder {
    pub fn _new() -> Self {

It's redundant, default can be used instead.

Code quote:

  pub fn _new() -> Self {

[MohammadNassar1]

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

---

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

Previously, MohammadNassar1 (mohammad-starkware) wrote…

To make it more readable, I can split it into two lines.

For the duplication, I assume you mean the duplication with priority_queue code.
Note that priority queue hold different type of transactions, so

I solved it in a different way.
Please ignore the above comment.

[MohammadNassar1]

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

---

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

Previously, MohammadNassar1 (mohammad-starkware) wrote…

I solved it in a different way.
Please ignore the above comment.

I've modified the code to enhance its readability.

The issue arises because the iterator input is of type IntoIterator, which implements into_iter. This takes ownership of the elements, which is necessary for moving the values into both the queue and address_to_tx.

The new solution first collects the transactions into a vector (which supports iter) and then uses this vector to populate both the queue and address_to_tx.

Although there’s still a single line of duplication, it now seems acceptable and keeps the code straightforward.

[elintul]

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

---

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

use crate::transaction_queue::{PendingTransaction, PriorityTransaction, TransactionQueue};

// Utils.

Delete, the file contains only utils; please be mindful.

Code quote:

// Utils.

---

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

    priority_queue: Option<BTreeSet<PriorityTransaction>>,
    pending_queue: Option<BTreeSet<PendingTransaction>>,
    address_to_tx: Option<HashMap<ContractAddress, TransactionReference>>,

Repetitive, please alias.

Code quote:

HashMap<ContractAddress, TransactionReference>

---

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

    priority_queue: Option<BTreeSet<PriorityTransaction>>,
    pending_queue: Option<BTreeSet<PendingTransaction>>,
    address_to_tx: Option<HashMap<ContractAddress, TransactionReference>>,

Consider aliasing the types, at least the mapping type which is cumbersome.

Code quote:

    priority_queue: Option<BTreeSet<PriorityTransaction>>,
    pending_queue: Option<BTreeSet<PendingTransaction>>,
    address_to_tx: Option<HashMap<ContractAddress, TransactionReference>>,

---

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

    pub fn _assert_eq_priority_and_pending_queues(&self, tx_queue: &TransactionQueue) {
        self._assert_eq_priority_queue(tx_queue);
        self._assert_eq_pending_queue(tx_queue);

Address mapping is checked twice...

Code quote:

        self._assert_eq_priority_queue(tx_queue);
        self._assert_eq_pending_queue(tx_queue);

---

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

    }

    pub fn _assert_eq_priority_queue(&self, tx_queue: &TransactionQueue) {

x3
Please be consistent with MempoolContent naming...

Suggestion:

priority_queue_content

[elintul]

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

---

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

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Assert for gas_price_threshold is not needed.
However, with_gas_price_threshold is necessary in the tests because we need to initialize a mempool with a specific gas price threshold in some tests.

Add a succinct comment on why it isn't checked in the struct's docstring.

---

crates/mempool/src/transaction_queue_test_utils.rs line 51 at r4 (raw file):

        P: IntoIterator<Item = TransactionReference>,
    {
        let priority_txs_vec: Vec<TransactionReference> = priority_txs.into_iter().collect();

As pointed out many times before, variable_name_type isn't a good practice since it overwhelms the reader with moot variables. Please shadow the input argument.

Code quote:

priority_txs_vec

---

crates/mempool/src/transaction_queue_test_utils.rs line 55 at r4 (raw file):

        self._address_to_tx
            .get_or_insert_with(HashMap::new)
            .extend(priority_txs_vec.iter().map(|tx| (tx.sender_address, *tx)));

I don't understand this line? Why not use same code as in transaction_queue.rs?

Code quote:

        self._address_to_tx
            .get_or_insert_with(HashMap::new)
            .extend(priority_txs_vec.iter().map(|tx| (tx.sender_address, *tx)));

[MohammadNassar1]

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

---

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

Previously, elintul (Elin) wrote…

Add a succinct comment on why it isn't checked in the struct's docstring.

Done

---

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

Previously, elintul (Elin) wrote…

Repetitive, please alias.

Done.

This alias was added to transaction_queue.rs file. It was used there as well.

---

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

Previously, elintul (Elin) wrote…

Consider aliasing the types, at least the mapping type which is cumbersome.

I've aliased the HashMap. I think priority and pending queues are clear.
Let me know what you think.

Also, TransactionQueueContent fields are aligned with the TransactionQueue struct.

---

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

Previously, elintul (Elin) wrote…

Address mapping is checked twice...

Done.

---

crates/mempool/src/transaction_queue_test_utils.rs line 51 at r4 (raw file):

Previously, elintul (Elin) wrote…

As pointed out many times before, variable_name_type isn't a good practice since it overwhelms the reader with moot variables. Please shadow the input argument.

Done

[MohammadNassar1]

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

---

crates/mempool/src/transaction_queue_test_utils.rs line 55 at r4 (raw file):

Previously, elintul (Elin) wrote…

I don't understand this line? Why not use same code as in transaction_queue.rs?

Let me explain:

In the transaction_queue.rs code, we have a single case where we insert a single transaction; thus, we use self.address_to_tx.insert.

However, in this case, we have a vector of transactions, so I used extend to add all the transactions to the map at once.

The get_or_insert_with function ensures that if the map is empty, an empty map is initialized. If the map already contains entries (such as when with_priority and with_pending are both called), it prevents overwriting the existing transaction mappings.

[giladchase]

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

---

crates/mempool/src/transaction_queue_test_utils.rs line 17 at r1 (raw file):

I think we need to check this field as well, as it can cover a potential bug.

Bugs are always possible.
We test behavior and API, not implementation details, and certainly not implementation details of aux structs, unless we have a real good reason to do so.
I think you have another PR up right now where we had to give the same exact comment.

---

crates/mempool/src/transaction_queue_test_utils.rs line 53 at r1 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Is collect needed?

Answer:
Yes, to collect an iterator to a set.

This suffices, you don't need to convert priority_txs into a vector for this.

            .extend(priority_txs.clone().into_iter().map(|tx| (tx.sender_address, tx)));

[elintul]

Reviewable status: 0 of 2 files reviewed, 7 unresolved discussions (waiting on @ayeletstarkware and @MohammadNassar1)

---

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

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Done.

This alias was added to transaction_queue.rs file. It was used there as well.

No full type written anywhere, except for alias definition? Always make sure to mention to the reviewer you've went through all the codebase and made sure.

[elintul]

Reviewed 1 of 2 files at r5.
Reviewable status: 1 of 2 files reviewed, 8 unresolved discussions (waiting on @ayeletstarkware and @MohammadNassar1)

---

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

use crate::mempool::TransactionReference;

type AddressToTransactionReference = HashMap<ContractAddress, TransactionReference>;

Should appear below sub-modules, as in all our code. Please be mindful.

Code quote:

type AddressToTransactionReference = HashMap<ContractAddress, TransactionReference>;

---

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

/// Enables customized (and potentially inconsistent) creation for unit testing.
/// The `gas_price_threshold` is used for builing the struct, but it is not checked. This is because
/// it's an input parameter for the Mempool, and no logic within the Mempool modifies its value.

Be succinct; and as mentioned before, refrain from using variable names in documentation.
Also, the second sentence is out of scope; a docstring should almost never refer to impl. details which most likely will become outdated, or other components. This was also discussed and pointed out in the past...

Suggestion:

// Note: gas price threshold is only used for builing the queue (non-test) struct.

[elintul]

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

---

crates/mempool/src/transaction_queue_test_utils.rs line 53 at r1 (raw file):

Previously, giladchase wrote…

This suffices, you don't need to convert priority_txs into a vector for this.

            .extend(priority_txs.clone().into_iter().map(|tx| (tx.sender_address, tx)));

Right, you're only iterating; you can stay lazy in line 56.

---

crates/mempool/src/transaction_queue_test_utils.rs line 55 at r4 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Let me explain:

In the transaction_queue.rs code, we have a single case where we insert a single transaction; thus, we use self.address_to_tx.insert.

However, in this case, we have a vector of transactions, so I used extend to add all the transactions to the map at once.

The get_or_insert_with function ensures that if the map is empty, an empty map is initialized. If the map already contains entries (such as when with_priority and with_pending are both called), it prevents overwriting the existing transaction mappings.

Is there a simpler way? This line is cumbersome.
And again, it's duplicated.

---

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

        P: IntoIterator<Item = TransactionReference>,
    {
        let pending_txs_vec: Vec<TransactionReference> = pending_txs.into_iter().collect();

Why is this still here? :(

Code quote:

pending_txs_vec

---

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

            Some(pending_txs_vec.into_iter().map(PendingTransaction::from).collect());

        self

Consistency, even in newlines.

Suggestion:

            Some(pending_txs_vec.into_iter().map(PendingTransaction::from).collect());
        self

[elintul]

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

[MohammadNassar1]

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

---

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

Previously, elintul (Elin) wrote…

No full type written anywhere, except for alias definition? Always make sure to mention to the reviewer you've went through all the codebase and made sure.

You're right.
I searched for HashMap<ContractAddress, TransactionReference>, and it seems it only appears in its definition and is not used elsewhere.

---

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

Previously, elintul (Elin) wrote…

Why is this still here? :(

Done.

[elintul]

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

---

crates/mempool/src/transaction_queue_test_utils.rs line 17 at r7 (raw file):

/// Note: gas price threshold is only used for builing the queue (non-test) struct.
/// The `gas_price_threshold` is used for builing the struct, but it is not checked. This is because
/// it's an input parameter for the Mempool, and no logic within the Mempool modifies its value.

Suggestion:

/// Note: gas price threshold is only used for builing the queue (non-test) struct.

[MohammadNassar1]

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

---

crates/mempool/src/transaction_queue_test_utils.rs line 17 at r1 (raw file):

Previously, giladchase wrote…

I think we need to check this field as well, as it can cover a potential bug.

Bugs are always possible.
We test behavior and API, not implementation details, and certainly not implementation details of aux structs, unless we have a real good reason to do so.
I think you have another PR up right now where we had to give the same exact comment.

I'm not sure I fully understand your point.

The address_to_tx field is checked, but I didn't create a specific utility, builder, or assertion for it. Instead, it's asserted as part of the transaction assertions. These lines:

Code snippet:

        assert_eq!(self.priority_queue.as_ref().unwrap(), &tx_queue.priority_queue);
        assert_eq!(self.pending_queue.as_ref().unwrap(), &tx_queue.pending_queue);
        assert_eq!(self.address_to_tx.as_ref().unwrap(), &tx_queue.address_to_tx);

---

crates/mempool/src/transaction_queue_test_utils.rs line 17 at r7 (raw file):

/// Note: gas price threshold is only used for builing the queue (non-test) struct.
/// The `gas_price_threshold` is used for builing the struct, but it is not checked. This is because
/// it's an input parameter for the Mempool, and no logic within the Mempool modifies its value.

Done.

[elintul]

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

---

crates/mempool/src/transaction_queue_test_utils.rs line 17 at r1 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

I'm not sure I fully understand your point.

The address_to_tx field is checked, but I didn't create a specific utility, builder, or assertion for it. Instead, it's asserted as part of the transaction assertions. These lines:

I think what Gilad means, is that this mapping is implicitly tested in many tests we have, and there's no need to over-complicate this test util.

[MohammadNassar1]

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

---

crates/mempool/src/transaction_queue_test_utils.rs line 17 at r1 (raw file):

Previously, elintul (Elin) wrote…

I think what Gilad means, is that this mapping is implicitly tested in many tests we have, and there's no need to over-complicate this test util.

Oooh, Could you please point out such a test?
I see all the tests in mempool_test use the mempool_content assertion, which uses the transaction_queue_content.

[elintul]

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

---

crates/mempool/src/transaction_queue_test_utils.rs line 17 at r1 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Oooh, Could you please point out such a test?
I see all the tests in mempool_test use the mempool_content assertion, which uses the transaction_queue_content.

Every test that touches the Q also checks this mapping; in case it isn't consistent with the Qs we'd have much bigger problems. Please see if you can get convinced of this independently, and share your opinion.
Note I'm not necessarily saying you need to go with a specific direction, I'm asking you to mitigate the thought process, as the code writer; this should not be a discussion in the PR, and should have happened prior to it.