Suggestion for "add(rpc): getblock: return transaction details with v…

…erbosity=2" (#9084) * Replaces multiple service calls (per transaction) with a single call to the state service for all of a block's transactions. * adjustments to reuse code from getrawtransaction --------- Co-authored-by: Conrado Gouvea <[email protected]>
ZcashFoundation · Dec 13, 2024 · 08f0721 · 08f0721
1 parent a860577
commit 08f0721
Show file tree

Hide file tree

Showing 2 changed files with 35 additions and 53 deletions.
diff --git a/zebra-rpc/src/methods.rs b/zebra-rpc/src/methods.rs
@@ -760,7 +760,6 @@ where
             None
         };
 
-        let self_clone = self.clone();
         async move {
             let hash_or_height: HashOrHeight = hash_or_height.parse().map_server_error()?;
 
@@ -811,6 +810,12 @@ where
                     next_block_hash,
                 } = *block_header;
 
+                let transactions_request = match verbosity {
+                    1 => zebra_state::ReadRequest::TransactionIdsForBlock(hash_or_height),
+                    2 => zebra_state::ReadRequest::Block(hash_or_height),
+                    _other => panic!("get_block_header_fut should be none"),
+                };
+
                 // # Concurrency
                 //
                 // We look up by block hash so the hash, transaction IDs, and confirmations
@@ -824,7 +829,7 @@ where
                     // A block's transaction IDs are never modified, so all possible responses are
                     // valid. Clients that query block heights must be able to handle chain forks,
                     // including getting transaction IDs from any chain fork.
-                    zebra_state::ReadRequest::TransactionIdsForBlock(hash_or_height),
+                    transactions_request,
                     // Orchard trees
                     zebra_state::ReadRequest::OrchardTree(hash_or_height),
                 ];
@@ -842,33 +847,29 @@ where
                         .iter()
                         .map(|tx_id| GetBlockTransaction::Hash(*tx_id))
                         .collect(),
+                    zebra_state::ReadResponse::Block(block) => block
+                        .ok_or_server_error("Block not found")?
+                        .transactions
+                        .iter()
+                        .map(|tx| {
+                            let GetRawTransaction::Object(tx_obj) =
+                                GetRawTransaction::from_transaction(
+                                    tx.clone(),
+                                    Some(height),
+                                    confirmations
+                                        .try_into()
+                                        .expect("should be less than max block height, i32::MAX"),
+                                    true,
+                                )
+                            else {
+                                unreachable!("an Object must be returned when verbose is true");
+                            };
+                            GetBlockTransaction::Object(tx_obj)
+                        })
+                        .collect(),
                     _ => unreachable!("unmatched response to a transaction_ids_for_block request"),
                 };
 
-                let tx = if verbosity >= 2 {
-                    let mut tx_obj_vec = vec![];
-                    let mut tx_futs = FuturesOrdered::new();
-                    let tx_len = tx.len();
-                    for txid in tx {
-                        let GetBlockTransaction::Hash(txid) = txid else {
-                            unreachable!("must be a Hash")
-                        };
-                        tx_futs
-                            .push_back(self_clone.get_raw_transaction(txid.encode_hex(), Some(1)));
-                    }
-                    for _ in 0..tx_len {
-                        let get_tx_result =
-                            tx_futs.next().await.expect("`tx_futs` should not be empty");
-                        let GetRawTransaction::Object(tx_obj) = get_tx_result? else {
-                            unreachable!("must return Object");
-                        };
-                        tx_obj_vec.push(GetBlockTransaction::Object(tx_obj));
-                    }
-                    tx_obj_vec
-                } else {
-                    tx
-                };
-
                 let orchard_tree_response = futs.next().await.expect("`futs` should not be empty");
                 let zebra_state::ReadResponse::OrchardTree(orchard_tree) =
                     orchard_tree_response.map_server_error()?

diff --git a/zebra-rpc/src/methods/tests/vectors.rs b/zebra-rpc/src/methods/tests/vectors.rs
@@ -242,31 +242,12 @@ async fn rpc_getblock() {
         );
     }
 
-    // With verbosity=2, the RPC calls getrawtransaction which queries the
-    // mempool, which we need to mock since we used a MockService for it. This
-    // function returns a future that mocks that query. This is similar to what
-    // we use in the getrawtransaction test, but here we don't bother checking
-    // if the request is correct.
-    let make_mempool_req = || {
-        let mut mempool = mempool.clone();
-        async move {
-            mempool
-                .expect_request_that(|_request| true)
-                .await
-                .respond(mempool::Response::Transactions(vec![]));
-        }
-    };
-
     // Make height calls with verbosity=2 and check response
     for (i, block) in blocks.iter().enumerate() {
-        let get_block_req = rpc.get_block(i.to_string(), Some(2u8));
-
-        // Run both the getblock request and the mocked mempool request.
-        // This assumes a single mempool query, i.e. that there is a single
-        // transaction the block. If the test vectors changes and the block
-        // has more than one transaction, this will need to be adjusted.
-        let (response, _) = futures::join!(get_block_req, make_mempool_req());
-        let get_block = response.expect("We should have a GetBlock struct");
+        let get_block = rpc
+            .get_block(i.to_string(), Some(2u8))
+            .await
+            .expect("We should have a GetBlock struct");
 
         let (expected_nonce, expected_final_sapling_root) =
             get_block_data(&read_state, block.clone(), i).await;
@@ -310,10 +291,10 @@ async fn rpc_getblock() {
 
     // Make hash calls with verbosity=2 and check response
     for (i, block) in blocks.iter().enumerate() {
-        let get_block_req = rpc.get_block(blocks[i].hash().to_string(), Some(2u8));
-
-        let (response, _) = futures::join!(get_block_req, make_mempool_req());
-        let get_block = response.expect("We should have a GetBlock struct");
+        let get_block = rpc
+            .get_block(blocks[i].hash().to_string(), Some(2u8))
+            .await
+            .expect("We should have a GetBlock struct");
 
         let (expected_nonce, expected_final_sapling_root) =
             get_block_data(&read_state, block.clone(), i).await;