From 08f0721ad12bf0b0e9bb2ca1d36e0cc8e1c5d355 Mon Sep 17 00:00:00 2001 From: Arya Date: Fri, 13 Dec 2024 09:44:48 -0500 Subject: [PATCH] Suggestion for "add(rpc): getblock: return transaction details with verbosity=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 --- zebra-rpc/src/methods.rs | 53 +++++++++++++------------- zebra-rpc/src/methods/tests/vectors.rs | 35 ++++------------- 2 files changed, 35 insertions(+), 53 deletions(-) diff --git a/zebra-rpc/src/methods.rs b/zebra-rpc/src/methods.rs index f4521e5ced6..012ee13e1e6 100644 --- 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 index 5f76c468266..722e89e5664 100644 --- 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;