Refactor mempool tests

[jvff]

Motivation

I was working on tests for #2682, and I saw an opportunity to tweak unmined_transactions_in_blocks a bit, but ended up getting carried away and refactored some other parts as well. I'm not sure if these are good ideas, so any feedback is welcome.

Solution

The biggest change is that unmined_transactions_in_blocks receives a range of block heights instead of a maximum height, and returns an iterator instead of a tuple with a Vec and the transaction count. The code that use this function were also tweaked a bit in an attempt to make clearer how the unmined transactions were being used and to remove some redundancy.

Review

Reviewer Checklist

• Code implements Specs and Designs
• Tests for Expected Behaviour
• Tests for Errors

[conradoplg]

In #2770 I created a test that had to split the tx list in three parts (the first, the rest except the last, the last, and a reference to the second-to-last). I tried to come up with a non "C-like" way that did not uses indices everywhere, but it relied on some Vec methods (pop()). Here you also had to convert it back to Vec, so maybe unmined_transactions_in_blocks could be reverted to return Vec again, but leaving the other improvements? (Not really sure if that would be better)

[jvff]

In #2770 I created a test that had to split the tx list in three parts (the first, the rest except the last, the last, and a reference to the second-to-last). I tried to come up with a non "C-like" way that did not uses indices everywhere, but it relied on some Vec methods (pop()). Here you also had to convert it back to Vec, so maybe unmined_transactions_in_blocks could be reverted to return Vec again, but leaving the other improvements? (Not really sure if that would be better)

Interesting problem! Regarding keeping the return type as Vec, I think that's not necessary, you can call .collect() after the function call and get the same result. But if you want to do it without using Vec methods, my first thought would be to try something like:

let mut transactions = unmined_transactions_in_blocks(height, network);
let first_transaction = transactions.next().unwrap();
let last_transaction = transactions.next_back().unwrap();

// Then use the "middle" transactions in `transactions`

Another option for when the iterator is not a DoubleEndedIterator is:

let mut transactions = unmined_transactions_in_blocks(height, network);
let first_transactions = transactions.next().unwrap();
let mut transactions: Vec<_> = transactions.collect();
let last_transaction = transactions.pop().unwrap();

I updated the PR to return a DoubleEndedIterator. I hope this helps 😅

[conradoplg]

I updated the PR to return a DoubleEndedIterator. I hope this helps

That's great, the best of both worlds 😁
I'm glad with this, let's wait for @oxarbitrage to take a look

[teor2345]

I don't mind what you do here - whatever works best for writing tests.

But I want to encourage you to merge this refactor soon, so it doesn't block other work, or cause rework.

[oxarbitrage]

This changes are good to me.

[dconnolly]

👍

[dconnolly]

This will break when we implement ZIP-401

[jvff]

In this case it might make sense to change the assertions to be:

1. Does the storage contain exactly MEMPOOL_SIZE transactions?
2. Are all transactions in the mempool storage from the unmined_transactions list?
3. Are all transaction IDs in the mempool from the unmined_transactions list (maybe minus the "accepted" transactions)?
4. (Maybe) Are they all unique?

Playing with an idea here:

let mut remaining_transactions: HashMap<UnminedTxId> = unmined_transactions
    .into_iter()
    .map(|transaction| transaction.id)
    .collect();

let stored_transactions = storage.transactions();

assert_eq!(stored_transactions.len(), MEMPOOL_SIZE));

for transaction in stored_transactions {
    assert!(remaining_transactions.remove(&transaction.id));
}

let rejected_transactions = storage.rejected_transactions().into_iter().collect();

assert_eq!(remaining_transactions, rejected_transactions);