Fix an incorrect assertion when the block locator is at the tip (#2789)

* Fix an incorrect assertion when the block locator is at the tip

This might have been triggered by receiving block hash gossips
from the new Zebra code.

* Add missing tests for zebra-state requests and responses

Specifically:
* `BlockLocator` (populated state only)
* `FindBlockHashes`
* `FindBlockHeaders`

* Test `FindBlock*` before and after the current block

* Add a specific test for bug #2789

* Refactor collect_best_chain_hashes to avoid manual index calculations

* Reword a comment

Co-authored-by: Alfredo Garcia <oxarbitrage@gmail.com>

zebra-chain/src/block/header.rs

@@ -124,7 +124,7 @@ impl Header {
 /// A header with a count of the number of transactions in its block.
 ///
 /// This structure is used in the Bitcoin network protocol.
-#[derive(Debug, Clone, PartialEq, Eq)]
+#[derive(Clone, Copy, Debug, Eq, PartialEq)]
 #[cfg_attr(any(test, feature = "proptest-impl"), derive(Arbitrary))]
 pub struct CountedHeader {
     /// The header for a block

zebra-chain/src/block/tests/preallocate.rs

@@ -76,7 +76,7 @@ proptest! {
         let max_allocation: usize = CountedHeader::max_allocation().try_into().unwrap();
         let mut smallest_disallowed_vec = Vec::with_capacity(max_allocation + 1);
         for _ in 0..(CountedHeader::max_allocation()+1) {
-            smallest_disallowed_vec.push(header.clone());
+            smallest_disallowed_vec.push(header);
         }
         let smallest_disallowed_serialized = smallest_disallowed_vec.zcash_serialize_to_vec().expect("Serialization to vec must succeed");
         // Check that our smallest_disallowed_vec is only one item larger than the limit

zebra-state/src/service.rs

@@ -1,4 +1,5 @@
 use std::{
+    convert::TryInto,
     future::Future,
     pin::Pin,
     sync::Arc,
@@ -411,7 +412,8 @@ impl StateService {
     ///   * adding the `stop` hash to the list, if it is in the best chain, or
     ///   * adding `max_len` hashes to the list.
     ///
-    /// Returns an empty list if the state is empty.
+    /// Returns an empty list if the state is empty,
+    /// or if the `intersection` is the best chain tip.
     pub fn collect_best_chain_hashes(
         &self,
         intersection: Option<block::Hash>,
@@ -424,6 +426,11 @@ impl StateService {
         let chain_tip_height = if let Some((height, _)) = self.best_tip() {
             height
         } else {
+            tracing::info!(
+                response_len = ?0,
+                "responding to peer GetBlocks or GetHeaders with empty state",
+            );
+
             return Vec::new();
         };
 
@@ -437,7 +444,12 @@ impl StateService {
                 .expect("the Find response height does not exceed Height::MAX")
         } else {
             // start at genesis, and return max_len hashes
-            block::Height((max_len - 1) as _)
+            let max_height = block::Height(
+                max_len
+                    .try_into()
+                    .expect("max_len does not exceed Height::MAX"),
+            );
+            (max_height - 1).expect("max_len is at least 1")
         };
 
         let stop_height = stop.map(|hash| self.best_height_by_hash(hash)).flatten();
@@ -489,11 +501,12 @@ impl StateService {
                 .unwrap_or(true),
             "the list must not contain the intersection hash"
         );
+        if let (Some(stop), Some((_, res_except_last))) = (stop, res.split_last()) {
             assert!(
-            stop.map(|hash| !res[..(res.len() - 1)].contains(&hash))
-                .unwrap_or(true),
+                !res_except_last.contains(&stop),
                 "if the stop hash is in the list, it must be the final hash"
             );
+        }
 
         res
     }

zebra-state/src/service/tests.rs

@@ -4,7 +4,7 @@ use futures::stream::FuturesUnordered;
 use tower::{buffer::Buffer, util::BoxService, Service, ServiceExt};
 
 use zebra_chain::{
-    block::{self, Block},
+    block::{self, Block, CountedHeader},
     chain_tip::ChainTip,
     fmt::SummaryDebug,
     parameters::{Network, NetworkUpgrade},
@@ -52,15 +52,61 @@ async fn populated_state(
 async fn test_populated_state_responds_correctly(
     mut state: Buffer<BoxService<Request, Response, BoxError>, Request>,
 ) -> Result<()> {
-    let blocks = zebra_test::vectors::MAINNET_BLOCKS
+    let blocks: Vec<Arc<Block>> = zebra_test::vectors::MAINNET_BLOCKS
         .range(0..=LAST_BLOCK_HEIGHT)
-        .map(|(_, block_bytes)| block_bytes.zcash_deserialize_into::<Arc<Block>>().unwrap());
+        .map(|(_, block_bytes)| block_bytes.zcash_deserialize_into().unwrap())
+        .collect();
+
+    let block_hashes: Vec<block::Hash> = blocks.iter().map(|block| block.hash()).collect();
+    let block_headers: Vec<CountedHeader> = blocks
+        .iter()
+        .map(|block| CountedHeader {
+            header: block.header,
+            transaction_count: block.transactions.len(),
+        })
+        .collect();
 
     for (ind, block) in blocks.into_iter().enumerate() {
         let mut transcript = vec![];
         let height = block.coinbase_height().unwrap();
         let hash = block.hash();
 
+        transcript.push((
+            Request::Depth(block.hash()),
+            Ok(Response::Depth(Some(LAST_BLOCK_HEIGHT - height.0))),
+        ));
+
+        // these requests don't have any arguments, so we just do them once
+        if ind == LAST_BLOCK_HEIGHT as usize {
+            transcript.push((Request::Tip, Ok(Response::Tip(Some((height, hash))))));
+
+            let locator_hashes = vec![
+                block_hashes[LAST_BLOCK_HEIGHT as usize],
+                block_hashes[(LAST_BLOCK_HEIGHT - 1) as usize],
+                block_hashes[(LAST_BLOCK_HEIGHT - 2) as usize],
+                block_hashes[(LAST_BLOCK_HEIGHT - 4) as usize],
+                block_hashes[(LAST_BLOCK_HEIGHT - 8) as usize],
+                block_hashes[0],
+            ];
+
+            transcript.push((
+                Request::BlockLocator,
+                Ok(Response::BlockLocator(locator_hashes)),
+            ));
+        }
+
+        // Spec: transactions in the genesis block are ignored.
+        if height.0 != 0 {
+            for transaction in &block.transactions {
+                let transaction_hash = transaction.hash();
+
+                transcript.push((
+                    Request::Transaction(transaction_hash),
+                    Ok(Response::Transaction(Some(transaction.clone()))),
+                ));
+            }
+        }
+
         transcript.push((
             Request::Block(hash.into()),
             Ok(Response::Block(Some(block.clone()))),
@@ -71,26 +117,11 @@ async fn test_populated_state_responds_correctly(
             Ok(Response::Block(Some(block.clone()))),
         ));
 
-        transcript.push((
-            Request::Depth(block.hash()),
-            Ok(Response::Depth(Some(LAST_BLOCK_HEIGHT - height.0))),
-        ));
-
-        if ind == LAST_BLOCK_HEIGHT as usize {
-            transcript.push((Request::Tip, Ok(Response::Tip(Some((height, hash))))));
-        }
-
-        // Consensus-critical bug in zcashd: transactions in the genesis block
-        // are ignored.
+        // Spec: transactions in the genesis block are ignored.
         if height.0 != 0 {
             for transaction in &block.transactions {
                 let transaction_hash = transaction.hash();
 
-                transcript.push((
-                    Request::Transaction(transaction_hash),
-                    Ok(Response::Transaction(Some(transaction.clone()))),
-                ));
-
                 let from_coinbase = transaction.has_valid_coinbase_transaction_inputs();
                 for (index, output) in transaction.outputs().iter().cloned().enumerate() {
                     let outpoint = transparent::OutPoint {
@@ -108,6 +139,76 @@ async fn test_populated_state_responds_correctly(
             }
         }
 
+        let mut append_locator_transcript = |split_ind| {
+            let block_hashes = block_hashes.clone();
+            let (known_hashes, next_hashes) = block_hashes.split_at(split_ind);
+
+            let block_headers = block_headers.clone();
+            let (_, next_headers) = block_headers.split_at(split_ind);
+
+            // no stop
+            transcript.push((
+                Request::FindBlockHashes {
+                    known_blocks: known_hashes.iter().rev().cloned().collect(),
+                    stop: None,
+                },
+                Ok(Response::BlockHashes(next_hashes.to_vec())),
+            ));
+
+            transcript.push((
+                Request::FindBlockHeaders {
+                    known_blocks: known_hashes.iter().rev().cloned().collect(),
+                    stop: None,
+                },
+                Ok(Response::BlockHeaders(next_headers.to_vec())),
+            ));
+
+            // stop at the next block
+            transcript.push((
+                Request::FindBlockHashes {
+                    known_blocks: known_hashes.iter().rev().cloned().collect(),
+                    stop: next_hashes.get(0).cloned(),
+                },
+                Ok(Response::BlockHashes(
+                    next_hashes.get(0).iter().cloned().cloned().collect(),
+                )),
+            ));
+
+            transcript.push((
+                Request::FindBlockHeaders {
+                    known_blocks: known_hashes.iter().rev().cloned().collect(),
+                    stop: next_hashes.get(0).cloned(),
+                },
+                Ok(Response::BlockHeaders(
+                    next_headers.get(0).iter().cloned().cloned().collect(),
+                )),
+            ));
+
+            // stop at a block that isn't actually in the chain
+            // tests bug #2789
+            transcript.push((
+                Request::FindBlockHashes {
+                    known_blocks: known_hashes.iter().rev().cloned().collect(),
+                    stop: Some(block::Hash([0xff; 32])),
+                },
+                Ok(Response::BlockHashes(next_hashes.to_vec())),
+            ));
+
+            transcript.push((
+                Request::FindBlockHeaders {
+                    known_blocks: known_hashes.iter().rev().cloned().collect(),
+                    stop: Some(block::Hash([0xff; 32])),
+                },
+                Ok(Response::BlockHeaders(next_headers.to_vec())),
+            ));
+        };
+
+        // split before the current block, and locate the current block
+        append_locator_transcript(ind);
+
+        // split after the current block, and locate the next block
+        append_locator_transcript(ind + 1);
+
         let transcript = Transcript::from(transcript);
         transcript.check(&mut state).await?;
     }
@@ -157,6 +258,20 @@ async fn empty_state_still_responds_to_requests() -> Result<()> {
             Ok(Response::Block(None)),
         ),
         // No check for AwaitUTXO because it will wait if the UTXO isn't present
+        (
+            Request::FindBlockHashes {
+                known_blocks: vec![block.hash()],
+                stop: None,
+            },
+            Ok(Response::BlockHashes(Vec::new())),
+        ),
+        (
+            Request::FindBlockHeaders {
+                known_blocks: vec![block.hash()],
+                stop: None,
+            },
+            Ok(Response::BlockHeaders(Vec::new())),
+        ),
     ]
     .into_iter();
     let transcript = Transcript::from(iter);