feat(mempool): get txs return requested txs after replenishing queue

[ayeletstarkware]

This change is 

[codecov-commenter]

Codecov Report

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

Project coverage is 81.85%. Comparing base (a70b025) to head (af61ce5).

Files	Patch %	Lines
crates/mempool/src/mempool.rs	80.00%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@                             Coverage Diff                             @@
##           ayelet/mempool/enqueue-next-eligable-tx     #499      +/-   ##
===========================================================================
+ Coverage                                    81.81%   81.85%   +0.03%     
===========================================================================
  Files                                           42       42              
  Lines                                         1848     1857       +9     
  Branches                                      1848     1857       +9     
===========================================================================
+ Hits                                          1512     1520       +8     
  Misses                                         259      259              
- Partials                                        77       78       +1     

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

[MohammadNassar1]

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

---

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

    pub fn get_txs(&mut self, n_txs: usize) -> MempoolResult<Vec<ThinTransaction>> {
        let mut eligible_txs: Vec<ThinTransaction> = Vec::with_capacity(n_txs);
        let mut remaining_txs = n_txs;

Suggestion:

n_remaining_txs

---

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

            eligible_txs.extend(popped_txs);
        }

Suggestion:

            }
            self.enqueue_next_eligible_txs(&popped_txs);

            remaining_txs -= popped_txs.len();
            eligible_txs.extend(popped_txs);
        }

---

crates/mempool/src/mempool_test.rs line 180 at r1 (raw file):

fn test_get_txs_multi_nonce() {
    // Setup.
    let tx_address_0_nonce_0 =

As you only use a single address in this test.

Suggestion:

tx_nonce_0

[MohammadNassar1]

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

---

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

    // back. TODO: Consider renaming to `pop_txs` to be more consistent with the standard
    // library.
    pub fn get_txs(&mut self, n_txs: usize) -> MempoolResult<Vec<ThinTransaction>> {

The method's code is getting longer, so I'm trying to think of a way to split it into functions.

What do you think about separating the queue and pool removal processes?

The recursive part is only relevant for the queue, not the pool.
Therefore, it can be implemented in the queue, and the output would be a list of transaction hashes. Then, in another loop, we can remove transactions from the pool.

Code quote:

 get_txs(&mut self, n_txs: usize)

[ayeletstarkware]

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

---

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

            eligible_txs.extend(popped_txs);
        }

why these gaps?

[MohammadNassar1]

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

---

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

Previously, ayeletstarkware (Ayelet Zilber) wrote…

why these gaps?

I don't think we need to add a gap between two logically related lines.

[elintul]

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

---

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

Previously, MohammadNassar1 (mohammad-starkware) wrote…

The method's code is getting longer, so I'm trying to think of a way to split it into functions.

What do you think about separating the queue and pool removal processes?

The recursive part is only relevant for the queue, not the pool.
Therefore, it can be implemented in the queue, and the output would be a list of transaction hashes. Then, in another loop, we can remove transactions from the pool.

WDYT:

pub fn get_txs(&mut self, n_txs: usize) -> MempoolResult<Vec<ThinTransaction>> {
    let mut eligible_tx_refs: Vec<TransactionReference> = Vec::with_capacity(n_txs);
    let mut n_remaining_txs = n_txs;

    while n_remaining_txs > 0 && !self.tx_queue.is_empty() {
        let chunk = self.tx_queue.pop_chunk(n_remaining_txs);

        for tx_ref in &chunk {
            let current_account_state = Account {
                sender_address: tx_ref.sender_address,
                state: AccountState { nonce: tx_ref.nonce },
            };

            if let Some(next_eligible_tx_ref) = self.tx_pool.get_next_eligible_tx(current_account_state)? {
                self.tx_queue.insert(next_eligible_tx_ref);
            }
        }

        n_remaining_txs -= chunk.len();
        eligible_tx_refs.extend(chunk);
    }

    let mut eligible_txs: Vec<ThinTransaction> = Vec::with_capacity(n_txs);
    for tx_ref in eligible_tx_refs {
        let tx = self.tx_pool.remove(tx_ref.tx_hash)?;
        eligible_txs.push(tx);
    }

    Ok(eligible_txs)
}

Would need to change the return value of pop_chunk.

---

crates/mempool/src/mempool_test.rs line 197 at r2 (raw file):

    // Assert: all transactions are returned.
    assert_eq!(txs, &[tx_nonce_0, tx_nonce_1, tx_nonce_2]);
    let expected_mempool_state = MempoolState::new([], []);

empty()?

Code quote:

MempoolState::new([], []);

[ayeletstarkware]

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

---

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

Previously, elintul (Elin) wrote…

WDYT:

pub fn get_txs(&mut self, n_txs: usize) -> MempoolResult<Vec<ThinTransaction>> {
    let mut eligible_tx_refs: Vec<TransactionReference> = Vec::with_capacity(n_txs);
    let mut n_remaining_txs = n_txs;

    while n_remaining_txs > 0 && !self.tx_queue.is_empty() {
        let chunk = self.tx_queue.pop_chunk(n_remaining_txs);

        for tx_ref in &chunk {
            let current_account_state = Account {
                sender_address: tx_ref.sender_address,
                state: AccountState { nonce: tx_ref.nonce },
            };

            if let Some(next_eligible_tx_ref) = self.tx_pool.get_next_eligible_tx(current_account_state)? {
                self.tx_queue.insert(next_eligible_tx_ref);
            }
        }

        n_remaining_txs -= chunk.len();
        eligible_tx_refs.extend(chunk);
    }

    let mut eligible_txs: Vec<ThinTransaction> = Vec::with_capacity(n_txs);
    for tx_ref in eligible_tx_refs {
        let tx = self.tx_pool.remove(tx_ref.tx_hash)?;
        eligible_txs.push(tx);
    }

    Ok(eligible_txs)
}

Would need to change the return value of pop_chunk.

done. I made some small changes.

---

crates/mempool/src/mempool_test.rs line 197 at r2 (raw file):

Previously, elintul (Elin) wrote…

empty()?

in a different PR.

[elintul]

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

---

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

            self.enqueue_next_eligible_txs(&chunk)?;

            n_remaining_txs -= chunk.len();

Suggestion:

            let chunk = self.tx_queue.pop_chunk(n_remaining_txs);
            self.enqueue_next_eligible_txs(&chunk)?;
            n_remaining_txs -= chunk.len();

---

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

        let eligible_txs = eligible_tx_references
            .into_iter()
            .map(|tx_ref| self.tx_pool.remove(tx_ref.tx_hash))

No external effects in map, please.

Code quote:

.map(|tx_ref| self.tx_pool.remove(tx_ref.tx_hash))

[ayeletstarkware]

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

---

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

Previously, elintul (Elin) wrote…

No external effects in map, please.

Done.

[elintul]

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