fix(rpc): use the correct RPC error code for missing blocks (#3977)

* Return error code -8 for missing blocks, like zcashd does

* Test that getblock returns error code -8 for missing blocks

* Make RegexSet failures easier to read in logs

* Update lightwalletd acceptance tests for log message changes

* Add extra failure strings for lightwalletd logs

* Improve link formatting

Co-authored-by: Deirdre Connolly <durumcrustulum@gmail.com>

Co-authored-by: Deirdre Connolly <durumcrustulum@gmail.com>

zebra-rpc/src/methods.rs

@@ -29,6 +29,12 @@ use zebra_node_services::{mempool, BoxError};
 #[cfg(test)]
 mod tests;
 
+/// The RPC error code used by `zcashd` for missing blocks.
+///
+/// `lightwalletd` expects error code `-8` when a block is not found:
+/// <https://github.com/adityapk00/lightwalletd/blob/c1bab818a683e4de69cd952317000f9bb2932274/common/common.go#L251-L254>
+pub const MISSING_BLOCK_ERROR_CODE: ErrorCode = ErrorCode::ServerError(-8);
+
 #[rpc(server)]
 /// RPC method signatures.
 pub trait Rpc {
@@ -78,6 +84,8 @@ pub trait Rpc {
     ) -> BoxFuture<Result<SentTransactionHash>>;
 
     /// Returns the requested block by height, as a [`GetBlock`] JSON string.
+    /// If the block is not in Zebra's state, returns
+    /// [error code `-8`.](https://github.com/zcash/zcash/issues/5758)
     ///
     /// zcashd reference: [`getblock`](https://zcash.github.io/rpc/getblock.html)
     ///
@@ -91,7 +99,7 @@ pub trait Rpc {
     /// mode for all getblock calls: <https://github.com/zcash/lightwalletd/blob/v0.4.9/common/common.go#L232>
     ///
     /// `lightwalletd` only requests blocks by height, so we don't support
-    /// getting blocks by hash but we do need to send the height number as a string.
+    /// getting blocks by hash. (But we parse the height as a JSON string, not an integer).
     ///
     /// The `verbosity` parameter is ignored but required in the call.
     #[rpc(name = "getblock")]
@@ -386,7 +394,7 @@ where
             match response {
                 zebra_state::ReadResponse::Block(Some(block)) => Ok(GetBlock(block.into())),
                 zebra_state::ReadResponse::Block(None) => Err(Error {
-                    code: ErrorCode::ServerError(0),
+                    code: MISSING_BLOCK_ERROR_CODE,
                     message: "Block not found".to_string(),
                     data: None,
                 }),

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

@@ -2,6 +2,7 @@
 
 use std::sync::Arc;
 
+use jsonrpc_core::ErrorCode;
 use tower::buffer::Buffer;
 
 use zebra_chain::{
@@ -85,7 +86,7 @@ async fn rpc_getblock() {
 }
 
 #[tokio::test]
-async fn rpc_getblock_error() {
+async fn rpc_getblock_parse_error() {
     zebra_test::init();
 
     let mut mempool: MockService<_, _, _, BoxError> = MockService::build().for_unit_tests();
@@ -110,6 +111,54 @@ async fn rpc_getblock_error() {
     state.expect_no_requests().await;
 }
 
+#[tokio::test]
+async fn rpc_getblock_missing_error() {
+    zebra_test::init();
+
+    let mut mempool: MockService<_, _, _, BoxError> = MockService::build().for_unit_tests();
+    let mut state: MockService<_, _, _, BoxError> = MockService::build().for_unit_tests();
+
+    // Init RPC
+    let rpc = RpcImpl::new(
+        "RPC test",
+        Buffer::new(mempool.clone(), 1),
+        Buffer::new(state.clone(), 1),
+        NoChainTip,
+        Mainnet,
+    );
+
+    // Make sure Zebra returns the correct error code `-8` for missing blocks
+    // https://github.com/adityapk00/lightwalletd/blob/c1bab818a683e4de69cd952317000f9bb2932274/common/common.go#L251-L254
+    let block_future = tokio::spawn(rpc.get_block("0".to_string(), 0u8));
+
+    // Make the mock service respond with no block
+    let response_handler = state
+        .expect_request(zebra_state::ReadRequest::Block(Height(0).into()))
+        .await;
+    response_handler.respond(zebra_state::ReadResponse::Block(None));
+
+    let block_response = block_future.await;
+    let block_response = block_response
+        .expect("unexpected panic in spawned request future")
+        .expect_err("unexpected success from missing block state response");
+    assert_eq!(block_response.code, ErrorCode::ServerError(-8),);
+
+    // Now check the error string the way `lightwalletd` checks it
+    assert_eq!(
+        serde_json::to_string(&block_response)
+            .expect("unexpected error serializing JSON error")
+            .split(':')
+            .nth(1)
+            .expect("unexpectedly low number of error fields")
+            .split(',')
+            .next(),
+        Some("-8")
+    );
+
+    mempool.expect_no_requests().await;
+    state.expect_no_requests().await;
+}
+
 #[tokio::test]
 async fn rpc_getbestblockhash() {
     zebra_test::init();

zebra-test/src/command.rs

@@ -775,7 +775,7 @@ impl<T> TestChild<T> {
             stream_name
         )
         .context_from(self)
-        .with_section(|| format!("{:?}", success_regexes).header("Match Regex:"));
+        .with_section(|| format!("{:#?}", success_regexes.patterns()).header("Match Regex:"));
 
         Err(report)
     }

zebrad/tests/acceptance.rs

@@ -1072,18 +1072,22 @@ const LIGHTWALLETD_FAILURE_MESSAGES: &[&str] = &[
     //
     // get_block_chain_info
     //
-    // TODO: enable these checks after PR #3891 merges
-    //
     // invalid sapling height
-    //"Got sapling height 0",
+    "Got sapling height 0",
     // missing BIP70 chain name, should be "main" or "test"
-    //" chain  ",
+    " chain  ",
     // missing branchID, should be 8 hex digits
-    //" branchID \"",
+    " branchID \"",
+    // get_block
     //
-    // TODO: complete this list for each RPC with fields?
-    // get_info
-    // get_raw_transaction
+    // a block error other than "-8: Block not found"
+    "error requesting block",
+    // a missing block with an incorrect error code
+    "Block not found",
+    //
+    // TODO: complete this list for each RPC with fields, if that RPC generates logs
+    // get_info - doesn't generate logs
+    // get_raw_transaction - might not generate logs
     // z_get_tree_state
     // get_address_txids
     // get_address_balance
@@ -1206,9 +1210,9 @@ fn lightwalletd_integration() -> Result<()> {
     //
     // TODO: expect Ingestor log when we're using cached state (#3511)
     //       "Ingestor adding block to cache"
-    let result = lightwalletd.expect_stdout_line_matches(
-        r#"error requesting block: 0: Block not found","height":419200"#,
-    );
+    let result = lightwalletd.expect_stdout_line_matches(regex::escape(
+        "Waiting for zcashd height to reach Sapling activation height (419200)",
+    ));
     let (_, zebrad) = zebrad.kill_on_error(result)?;
 
     // (next RPC)