fix(rpc): Add `getblock` RPC fields to support the latest version of `zcash/lightwalletd` (#6134)

* Stabilise the BestChainBlockHash state request

* Always include the block hash in the getblock RPC response

* Make the lightwalletd integration tests compatible with zcash/lightwalletd

* Update getblock RPC snapshots

* Return the correct missing block error code

zebra-rpc/src/methods.rs

@@ -613,24 +613,62 @@ where
                 // This RPC is used in `lightwalletd`'s initial sync of 2 million blocks,
                 // so it needs to load all its fields very efficiently.
                 //
-                // Currently, we get the transaction IDs from an index, which is much more
+                // Currently, we get the block hash and transaction IDs from indexes,
-                // efficient than loading all the block data and hashing all the transactions.
+                // which is much more efficient than loading all the block data,
+                // then hashing the block header and all the transactions.
 
-                // TODO: look up the hash if we only have a height,
+                // Get the block hash from the height -> hash index, if needed
-                //       and look up the height if we only have a hash
-                let hash = hash_or_height.hash().map(GetBlockHash);
-                let height = hash_or_height.height();
-
-                // TODO: do these state  queries in parallel?
-
-                // Get transaction IDs from the transaction index
                 //
                 // # Concurrency
                 //
+                // For consistency, this lookup must be performed first, then all the other
+                // lookups must be based on the hash.
+                //
+                // All possible responses are valid, even if the best chain changes. Clients
+                // must be able to handle chain forks, including a hash for a block that is
+                // later discovered to be on a side chain.
+
+                let hash = match hash_or_height {
+                    HashOrHeight::Hash(hash) => hash,
+                    HashOrHeight::Height(height) => {
+                        let request = zebra_state::ReadRequest::BestChainBlockHash(height);
+                        let response = state
+                            .ready()
+                            .and_then(|service| service.call(request))
+                            .await
+                            .map_err(|error| Error {
+                                code: ErrorCode::ServerError(0),
+                                message: error.to_string(),
+                                data: None,
+                            })?;
+
+                        match response {
+                            zebra_state::ReadResponse::BlockHash(Some(hash)) => hash,
+                            zebra_state::ReadResponse::BlockHash(None) => {
+                                return Err(Error {
+                                    code: MISSING_BLOCK_ERROR_CODE,
+                                    message: "block height not in best chain".to_string(),
+                                    data: None,
+                                })
+                            }
+                            _ => unreachable!("unmatched response to a block hash request"),
+                        }
+                    }
+                };
+
+                // TODO: do the txids and confirmations state queries in parallel?
+
+                // Get transaction IDs from the transaction index by block hash
+                //
+                // # Concurrency
+                //
+                // We look up by block hash so the hash, transaction IDs, and confirmations
+                // are consistent.
+                //
                 // 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.
-                let request = zebra_state::ReadRequest::TransactionIdsForBlock(hash_or_height);
+                let request = zebra_state::ReadRequest::TransactionIdsForBlock(hash.into());
                 let response = state
                     .ready()
                     .and_then(|service| service.call(request))
@@ -656,6 +694,9 @@ where
                 //
                 // # Concurrency
                 //
+                // We look up by block hash so the hash, transaction IDs, and confirmations
+                // are consistent.
+                //
                 // All possible responses are valid, even if a block is added to the chain, or
                 // the best chain changes. Clients must be able to handle chain forks, including
                 // different confirmation values before or after added blocks, and switching
@@ -664,34 +705,31 @@ where
                 // From <https://zcash.github.io/rpc/getblock.html>
                 const NOT_IN_BEST_CHAIN_CONFIRMATIONS: i64 = -1;
 
-                let confirmations = if let Some(hash) = hash_or_height.hash() {
+                let request = zebra_state::ReadRequest::Depth(hash);
-                    let request = zebra_state::ReadRequest::Depth(hash);
+                let response = state
-                    let response = state
+                    .ready()
-                        .ready()
+                    .and_then(|service| service.call(request))
-                        .and_then(|service| service.call(request))
+                    .await
-                        .await
+                    .map_err(|error| Error {
-                        .map_err(|error| Error {
+                        code: ErrorCode::ServerError(0),
-                            code: ErrorCode::ServerError(0),
+                        message: error.to_string(),
-                            message: error.to_string(),
+                        data: None,
-                            data: None,
+                    })?;
-                        })?;
 
-                    match response {
+                let confirmations = match response {
-                        // Confirmations are one more than the depth.
+                    // Confirmations are one more than the depth.
-                        // Depth is limited by height, so it will never overflow an i64.
+                    // Depth is limited by height, so it will never overflow an i64.
-                        zebra_state::ReadResponse::Depth(Some(depth)) => Some(i64::from(depth) + 1),
+                    zebra_state::ReadResponse::Depth(Some(depth)) => i64::from(depth) + 1,
-                        zebra_state::ReadResponse::Depth(None) => {
+                    zebra_state::ReadResponse::Depth(None) => NOT_IN_BEST_CHAIN_CONFIRMATIONS,
-                            Some(NOT_IN_BEST_CHAIN_CONFIRMATIONS)
+                    _ => unreachable!("unmatched response to a depth request"),
-                        }
-                        _ => unreachable!("unmatched response to a depth request"),
-                    }
-                } else {
-                    // TODO: make Depth support heights as well
-                    None
                 };
 
+                // TODO:  look up the height if we only have a hash,
+                //        this needs a new state request for the height -> hash index
+                let height = hash_or_height.height();
+
                 Ok(GetBlock::Object {
-                    hash,
+                    hash: GetBlockHash(hash),
                     confirmations,
                     height,
                     tx,
@@ -1285,7 +1323,7 @@ pub struct SentTransactionHash(#[serde(with = "hex")] transaction::Hash);
 
 /// Response to a `getblock` RPC request.
 ///
-/// See the notes for the [`Rpc::get_block` method].
+/// See the notes for the [`Rpc::get_block`] method.
 #[derive(Clone, Debug, Eq, PartialEq, serde::Serialize)]
 #[serde(untagged)]
 pub enum GetBlock {
@@ -1294,13 +1332,11 @@ pub enum GetBlock {
     /// The block object.
     Object {
         /// The hash of the requested block.
-        #[serde(skip_serializing_if = "Option::is_none")]
+        hash: GetBlockHash,
-        hash: Option<GetBlockHash>,
 
         /// The number of confirmations of this block in the best chain,
         /// or -1 if it is not in the best chain.
-        #[serde(skip_serializing_if = "Option::is_none")]
+        confirmations: i64,
-        confirmations: Option<i64>,
 
         /// The height of the requested block.
         #[serde(skip_serializing_if = "Option::is_none")]

zebra-rpc/src/methods/tests/snapshots/get_block_verbose_height_verbosity_1@mainnet_10.snap

@@ -3,6 +3,8 @@ source: zebra-rpc/src/methods/tests/snapshot.rs
 expression: block
 ---
 {
+  "hash": "0007bc227e1c57a4a70e237cad00e7b7ce565155ab49166bc57397a26d339283",
+  "confirmations": 10,
   "height": 1,
   "tx": [
     "851bf6fbf7a976327817c738c489d7fa657752445430922d94c983c0b9ed4609"

zebra-rpc/src/methods/tests/snapshots/get_block_verbose_height_verbosity_1@testnet_10.snap

@@ -3,6 +3,8 @@ source: zebra-rpc/src/methods/tests/snapshot.rs
 expression: block
 ---
 {
+  "hash": "025579869bcf52a989337342f5f57a84f3a28b968f7d6a8307902b065a668d23",
+  "confirmations": 10,
   "height": 1,
   "tx": [
     "f37e9f691fffb635de0999491d906ee85ba40cd36dae9f6e5911a8277d7c5f75"

zebra-rpc/src/methods/tests/snapshots/get_block_verbose_height_verbosity_default@mainnet_10.snap

@@ -3,6 +3,8 @@ source: zebra-rpc/src/methods/tests/snapshot.rs
 expression: block
 ---
 {
+  "hash": "0007bc227e1c57a4a70e237cad00e7b7ce565155ab49166bc57397a26d339283",
+  "confirmations": 10,
   "height": 1,
   "tx": [
     "851bf6fbf7a976327817c738c489d7fa657752445430922d94c983c0b9ed4609"

zebra-rpc/src/methods/tests/snapshots/get_block_verbose_height_verbosity_default@testnet_10.snap

@@ -3,6 +3,8 @@ source: zebra-rpc/src/methods/tests/snapshot.rs
 expression: block
 ---
 {
+  "hash": "025579869bcf52a989337342f5f57a84f3a28b968f7d6a8307902b065a668d23",
+  "confirmations": 10,
   "height": 1,
   "tx": [
     "f37e9f691fffb635de0999491d906ee85ba40cd36dae9f6e5911a8277d7c5f75"

zebra-rpc/src/methods/tests/vectors.rs

@@ -128,8 +128,8 @@ async fn rpc_getblock() {
         assert_eq!(
             get_block,
             GetBlock::Object {
-                hash: None,
+                hash: GetBlockHash(block.hash()),
-                confirmations: None,
+                confirmations: (blocks.len() - i).try_into().expect("valid i64"),
                 height: Some(Height(i.try_into().expect("valid u32"))),
                 tx: block
                     .transactions
@@ -150,8 +150,8 @@ async fn rpc_getblock() {
         assert_eq!(
             get_block,
             GetBlock::Object {
-                hash: Some(GetBlockHash(block.hash())),
+                hash: GetBlockHash(block.hash()),
-                confirmations: Some((blocks.len() - i).try_into().expect("valid i64")),
+                confirmations: (blocks.len() - i).try_into().expect("valid i64"),
                 height: None,
                 tx: block
                     .transactions
@@ -172,8 +172,8 @@ async fn rpc_getblock() {
         assert_eq!(
             get_block,
             GetBlock::Object {
-                hash: None,
+                hash: GetBlockHash(block.hash()),
-                confirmations: None,
+                confirmations: (blocks.len() - i).try_into().expect("valid i64"),
                 height: Some(Height(i.try_into().expect("valid u32"))),
                 tx: block
                     .transactions
@@ -194,8 +194,8 @@ async fn rpc_getblock() {
         assert_eq!(
             get_block,
             GetBlock::Object {
-                hash: Some(GetBlockHash(block.hash())),
+                hash: GetBlockHash(block.hash()),
-                confirmations: Some((blocks.len() - i).try_into().expect("valid i64")),
+                confirmations: (blocks.len() - i).try_into().expect("valid i64"),
                 height: None,
                 tx: block
                     .transactions

zebra-state/src/request.rs

@@ -53,6 +53,23 @@ impl HashOrHeight {
         }
     }
 
+    /// Unwrap the inner hash or attempt to retrieve the hash for a given
+    /// height if one exists.
+    ///
+    /// # Consensus
+    ///
+    /// In the non-finalized state, a height can have multiple valid hashes.
+    /// We typically use the hash that is currently on the best chain.
+    pub fn hash_or_else<F>(self, op: F) -> Option<block::Hash>
+    where
+        F: FnOnce(block::Height) -> Option<block::Hash>,
+    {
+        match self {
+            HashOrHeight::Hash(hash) => Some(hash),
+            HashOrHeight::Height(height) => op(height),
+        }
+    }
+
     /// Returns the hash if this is a [`HashOrHeight::Hash`].
     pub fn hash(&self) -> Option<block::Hash> {
         if let HashOrHeight::Hash(hash) = self {
@@ -801,7 +818,6 @@ pub enum ReadRequest {
     /// Returns [`ReadResponse::BestChainNextMedianTimePast`] when successful.
     BestChainNextMedianTimePast,
 
-    #[cfg(feature = "getblocktemplate-rpcs")]
     /// Looks up a block hash by height in the current best chain.
     ///
     /// Returns
@@ -862,7 +878,6 @@ impl ReadRequest {
                 "best_chain_tip_nullifiers_anchors"
             }
             ReadRequest::BestChainNextMedianTimePast => "best_chain_next_median_time_past",
-            #[cfg(feature = "getblocktemplate-rpcs")]
             ReadRequest::BestChainBlockHash(_) => "best_chain_block_hash",
             #[cfg(feature = "getblocktemplate-rpcs")]
             ReadRequest::ChainInfo => "chain_info",

zebra-state/src/response.rs

@@ -137,7 +137,6 @@ pub enum ReadResponse {
     /// Contains the median-time-past for the *next* block on the best chain.
     BestChainNextMedianTimePast(DateTime32),
 
-    #[cfg(feature = "getblocktemplate-rpcs")]
     /// Response to [`ReadRequest::BestChainBlockHash`](crate::ReadRequest::BestChainBlockHash) with the
     /// specified block hash.
     BlockHash(Option<block::Hash>),
@@ -231,13 +230,13 @@ impl TryFrom<ReadResponse> for Response {
                 Err("there is no corresponding Response for this ReadResponse")
             }
 
-            #[cfg(feature = "getblocktemplate-rpcs")]
-            ReadResponse::ValidBlockProposal => Ok(Response::ValidBlockProposal),
-
-            #[cfg(feature = "getblocktemplate-rpcs")]
             ReadResponse::BlockHash(_) => {
                 Err("there is no corresponding Response for this ReadResponse")
             }
+
+            #[cfg(feature = "getblocktemplate-rpcs")]
+            ReadResponse::ValidBlockProposal => Ok(Response::ValidBlockProposal),
+
             #[cfg(feature = "getblocktemplate-rpcs")]
             ReadResponse::ChainInfo(_) | ReadResponse::SolutionRate(_) => {
                 Err("there is no corresponding Response for this ReadResponse")

zebra-state/src/service.rs

@@ -1621,8 +1621,7 @@ impl Service<ReadRequest> for ReadStateService {
                 .boxed()
             }
 
-            // Used by get_block_hash RPC.
+            // Used by the get_block and get_block_hash RPCs.
-            #[cfg(feature = "getblocktemplate-rpcs")]
             ReadRequest::BestChainBlockHash(height) => {
                 let timer = CodeTimer::start();
 

zebrad/tests/common/lightwalletd.rs

@@ -247,10 +247,15 @@ where
     fn with_lightwalletd_config(self, zebra_rpc_listener: SocketAddr) -> Result<Self> {
         use std::fs;
 
+        // zcash/lightwalletd requires rpcuser and rpcpassword, or a zcashd cookie file
+        // But when a lightwalletd with this config is used by Zebra,
+        // Zebra ignores any authentication and provides access regardless.
         let lightwalletd_config = format!(
             "\
             rpcbind={}\n\
             rpcport={}\n\
+            rpcuser=xxxxx
+            rpcpassword=xxxxx
             ",
             zebra_rpc_listener.ip(),
             zebra_rpc_listener.port(),