Refactor mempool tests (#2771)

* Remove return of redundant vector length

An attempt to improve readability a bit by not returning a tuple with a
value that can be obtained from a single return type.

* Refactor `unmined_transactions_in_blocks`

Use a more functional style to try to make it a bit clearer.

* Use ranges in `unmined_transactions_in_blocks`

Allow a finer control over the block range to extract the transactions
from.

* Refactor mempool test code to improve clarity

It was previously not clear that only the first genesis transaction was
being used. The remaining transactions in the genesis block were
discarded and then readded later.

* Replace `oneshot` with `call`

Remove a redundant internal `ready_and` call.

* Return an `impl Iterator` instead of a `Vec<_>`

Remove unnecessary deserializations and heap allocations.

* Refactor `mempool_storage_basic_for_network` test

Make the separation between the transactions expected to be in the
mempool and those expected to be rejected clearer.

* Replace `Iterator` with `DoubleEndedIterator`

Allow getting the last transaction more easily.

Co-authored-by: Alfredo Garcia <oxarbitrage@gmail.com>
Co-authored-by: Conrado Gouvea <conrado@zfnd.org>

zebrad/src/components/inbound/tests.rs

@@ -253,13 +253,16 @@ async fn setup(
 }
 
 fn add_some_stuff_to_mempool(mempool_service: &mut Mempool, network: Network) -> Vec<UnminedTx> {
-    // get the genesis block transactions from the Zcash blockchain.
-    let genesis_transactions = unmined_transactions_in_blocks(0, network);
+    // get the genesis block coinbase transaction from the Zcash blockchain.
+    let genesis_transactions: Vec<_> = unmined_transactions_in_blocks(..=0, network)
+        .take(1)
+        .collect();
+
     // Insert the genesis block coinbase transaction into the mempool storage.
     mempool_service
         .storage()
-        .insert(genesis_transactions.1[0].clone())
+        .insert(genesis_transactions[0].clone())
         .unwrap();
 
-    genesis_transactions.1
+    genesis_transactions
 }

zebrad/src/components/mempool/storage/tests.rs

@@ -1,3 +1,5 @@
+use std::ops::RangeBounds;
+
 use super::*;
 
 use zebra_chain::{
@@ -15,11 +17,10 @@ fn mempool_storage_crud_mainnet() {
     // Create an empty storage instance
     let mut storage: Storage = Default::default();
 
-    // Get transactions from the first 10 blocks of the Zcash blockchain
-    let (_, unmined_transactions) = unmined_transactions_in_blocks(10, network);
-
     // Get one (1) unmined transaction
-    let unmined_tx = &unmined_transactions[0];
+    let unmined_tx = unmined_transactions_in_blocks(.., network)
+        .next()
+        .expect("at least one unmined transaction");
 
     // Insert unmined tx into the mempool.
     let _ = storage.insert(unmined_tx.clone());
@@ -49,77 +50,75 @@ fn mempool_storage_basic_for_network(network: Network) -> Result<()> {
     let mut storage: Storage = Default::default();
 
     // Get transactions from the first 10 blocks of the Zcash blockchain
-    let (total_transactions, unmined_transactions) = unmined_transactions_in_blocks(10, network);
+    let unmined_transactions: Vec<_> = unmined_transactions_in_blocks(..=10, network).collect();
+    let total_transactions = unmined_transactions.len();
 
     // Insert them all to the storage
     for unmined_transaction in unmined_transactions.clone() {
         storage.insert(unmined_transaction)?;
     }
 
+    // Separate transactions into the ones expected to be in the mempool and those expected to be
+    // rejected.
+    let rejected_transaction_count = total_transactions - MEMPOOL_SIZE;
+    let expected_to_be_rejected = &unmined_transactions[..rejected_transaction_count];
+    let expected_in_mempool = &unmined_transactions[rejected_transaction_count..];
+
     // Only MEMPOOL_SIZE should land in verified
     assert_eq!(storage.verified.len(), MEMPOOL_SIZE);
 
     // The rest of the transactions will be in rejected
-    assert_eq!(storage.rejected.len(), total_transactions - MEMPOOL_SIZE);
+    assert_eq!(storage.rejected.len(), rejected_transaction_count);
 
     // Make sure the last MEMPOOL_SIZE transactions we sent are in the verified
-    for tx in unmined_transactions.iter().rev().take(MEMPOOL_SIZE) {
+    for tx in expected_in_mempool {
         assert!(storage.contains(&tx.id));
     }
 
     // Anything greater should not be in the verified
-    for tx in unmined_transactions
-        .iter()
-        .take(unmined_transactions.len() - MEMPOOL_SIZE)
-    {
+    for tx in expected_to_be_rejected {
         assert!(!storage.contains(&tx.id));
     }
 
     // Query all the ids we have for rejected, get back `total - MEMPOOL_SIZE`
     let all_ids: HashSet<UnminedTxId> = unmined_transactions.iter().map(|tx| tx.id).collect();
-    let rejected_ids: HashSet<UnminedTxId> = unmined_transactions
-        .iter()
-        .take(total_transactions - MEMPOOL_SIZE)
-        .map(|tx| tx.id)
-        .collect();
+
     // Convert response to a `HashSet` as we need a fixed order to compare.
     let rejected_response: HashSet<UnminedTxId> =
         storage.rejected_transactions(all_ids).into_iter().collect();
 
+    let rejected_ids = expected_to_be_rejected.iter().map(|tx| tx.id).collect();
+
     assert_eq!(rejected_response, rejected_ids);
 
     // Use `contains_rejected` to make sure the first id stored is now rejected
-    assert!(storage.contains_rejected(&unmined_transactions[0].id));
+    assert!(storage.contains_rejected(&expected_to_be_rejected[0].id));
     // Use `contains_rejected` to make sure the last id stored is not rejected
-    assert!(!storage.contains_rejected(&unmined_transactions[unmined_transactions.len() - 1].id));
+    assert!(!storage.contains_rejected(&expected_in_mempool[0].id));
 
     Ok(())
 }
 
 pub fn unmined_transactions_in_blocks(
-    last_block_height: u32,
+    block_height_range: impl RangeBounds<u32>,
     network: Network,
-) -> (usize, Vec<UnminedTx>) {
-    let mut transactions = vec![];
-    let mut total = 0;
-
-    let block_iter = match network {
+) -> impl DoubleEndedIterator<Item = UnminedTx> {
+    let blocks = match network {
         Network::Mainnet => zebra_test::vectors::MAINNET_BLOCKS.iter(),
         Network::Testnet => zebra_test::vectors::TESTNET_BLOCKS.iter(),
     };
 
-    for (&height, block) in block_iter {
-        if height <= last_block_height {
-            let block = block
+    // Deserialize the blocks that are selected based on the specified `block_height_range`.
+    let selected_blocks = blocks
+        .filter(move |(&height, _)| block_height_range.contains(&height))
+        .map(|(_, block)| {
+            block
                 .zcash_deserialize_into::<Block>()
-                .expect("block is structurally valid");
+                .expect("block test vector is structurally valid")
+        });
 
-            for transaction in block.transactions.iter() {
-                transactions.push(UnminedTx::from(transaction));
-                total += 1;
-            }
-        }
-    }
-
-    (total, transactions)
+    // Extract the transactions from the blocks and warp each one as an unmined transaction.
+    selected_blocks
+        .flat_map(|block| block.transactions)
+        .map(UnminedTx::from)
 }

zebrad/src/components/mempool/tests.rs

@@ -26,7 +26,12 @@ async fn mempool_service_basic() -> Result<(), Report> {
             .await;
 
     // get the genesis block transactions from the Zcash blockchain.
-    let genesis_transactions = unmined_transactions_in_blocks(0, network);
+    let mut unmined_transactions = unmined_transactions_in_blocks(..=10, network);
+    let genesis_transaction = unmined_transactions
+        .next()
+        .expect("Missing genesis transaction");
+    let more_transactions = unmined_transactions;
+
     // Start the mempool service
     let mut service = Mempool::new(
         network,
@@ -37,7 +42,7 @@ async fn mempool_service_basic() -> Result<(), Report> {
         chain_tip_change,
     );
     // Insert the genesis block coinbase transaction into the mempool storage.
-    service.storage.insert(genesis_transactions.1[0].clone())?;
+    service.storage.insert(genesis_transaction.clone())?;
 
     // Test `Request::TransactionIds`
     let response = service
@@ -61,7 +66,7 @@ async fn mempool_service_basic() -> Result<(), Report> {
         .ready_and()
         .await
         .unwrap()
-        .oneshot(Request::TransactionsById(
+        .call(Request::TransactionsById(
             genesis_transactions_hash_set.clone(),
         ))
         .await
@@ -73,12 +78,11 @@ async fn mempool_service_basic() -> Result<(), Report> {
 
     // Make sure the transaction from the blockchain test vector is the same as the
     // response of `Request::TransactionsById`
-    assert_eq!(genesis_transactions.1[0], transactions[0]);
+    assert_eq!(genesis_transaction, transactions[0]);
 
     // Insert more transactions into the mempool storage.
     // This will cause the genesis transaction to be moved into rejected.
-    let more_transactions = unmined_transactions_in_blocks(10, network);
-    for tx in more_transactions.1.iter().skip(1) {
+    for tx in more_transactions {
         service.storage.insert(tx.clone())?;
     }