Rewrite GetData handling to match the zcashd implementation (#1518)

* Rewrite GetData handling to match the zcashd implementation

`zcashd` silently ignores missing blocks, but sends found transactions
followed by a `NotFound` message:
e7b425298f/src/main.cpp (L5497)

This is significantly different to the behaviour expected by the old
Zebra connection state machine, which expected `NotFound` for blocks.

Also change Zebra's GetData responses to peer request so they ignore
missing blocks.

* Stop hanging on incomplete transaction or block responses

Instead, if the peer sends an unexpected block, unexpected transaction,
or NotFound message:
1. end the request, and return a partial response containing any items
   that were successfully received
2. if none of the expected blocks or transactions were received, return
   an error, and close the connection

zebra-network/src/peer/connection.rs

@@ -1,18 +1,11 @@
-// NOT A PLACE OF HONOR
+//! Zcash peer connection protocol handing for Zebra.
-//
+//!
-// NO ESTEEMED DEED IS COMMEMORATED HERE
+//! Maps the external Zcash/Bitcoin protocol to Zebra's internal request/response
-//
+//! protocol.
-// NOTHING VALUED IS HERE
+//!
-//
+//! This module contains a lot of undocumented state, assumptions and invariants.
-// What is here was dangerous and repulsive to us. This message is a warning
+//! And it's unclear if these assumptions match the `zcashd` implementation.
-// about danger.
+//! It should be refactored into a cleaner set of request/response pairs (#1515).
-//
-// The danger is in a particular module... it increases towards a center... the
-// center of danger is pub async fn Connection::run the danger is still present,
-// in your time, as it was in ours.
-//
-// The danger is to the mind. The danger is unleashed only if you substantially
-// disturb this code. This code is best shunned and left encapsulated.
 
 use std::collections::HashSet;
 use std::sync::Arc;
@@ -91,6 +84,10 @@ impl Handler {
                 }
             }
             (Handler::Peers, Message::Addr(addrs)) => Handler::Finished(Ok(Response::Peers(addrs))),
+            // `zcashd` returns requested transactions in a single batch of messages.
+            // Other transaction or non-transaction messages can come before or after the batch.
+            // After the transaction batch, `zcashd` sends `NotFound` if any transactions are missing:
+            // https://github.com/zcash/zcash/blob/e7b425298f6d9a54810cb7183f00be547e4d9415/src/main.cpp#L5617
             (
                 Handler::TransactionsByHash {
                     mut hashes,
@@ -98,20 +95,52 @@ impl Handler {
                 },
                 Message::Tx(transaction),
             ) => {
+                // assumptions:
+                //   - the transaction messages are sent in a single continous batch
+                //   - missing transaction hashes are included in a `NotFound` message
                 if hashes.remove(&transaction.hash()) {
+                    // we are in the middle of the continous transaction messages
                     transactions.push(transaction);
+                    if hashes.is_empty() {
+                        Handler::Finished(Ok(Response::Transactions(transactions)))
+                    } else {
+                        Handler::TransactionsByHash {
+                            hashes,
+                            transactions,
+                        }
+                    }
                 } else {
+                    // We got a transaction we didn't ask for. If the caller doesn't know any of the
+                    // transactions, they should have sent a `NotFound` with all the hashes, rather
+                    // than an unsolicited transaction.
+                    //
+                    // So either:
+                    // 1. The peer implements the protocol badly, skipping `NotFound`.
+                    //    We should cancel the request, so we don't hang waiting for transactions
+                    //    that will never arrive.
+                    // 2. The peer sent an unsolicited transaction.
+                    //    We should ignore the transaction, and wait for the actual response.
+                    //
+                    // We end the request, so we don't hang on bad peers (case 1). But we keep the
+                    // connection open, so the inbound service can process transactions from good
+                    // peers (case 2).
                     ignored_msg = Some(Message::Tx(transaction));
-                }
+                    if !transactions.is_empty() {
-                if hashes.is_empty() {
+                        // if our peers start sending mixed solicited and unsolicited transactions,
-                    Handler::Finished(Ok(Response::Transactions(transactions)))
+                        // we should update this code to handle those responses
-                } else {
+                        error!("unexpected transaction from peer: transaction responses should be sent in a continuous batch, followed by notfound. Using partial received transactions as the peer response");
-                    Handler::TransactionsByHash {
+                        // TODO: does the caller need a list of missing transactions? (#1515)
-                        hashes,
+                        Handler::Finished(Ok(Response::Transactions(transactions)))
-                        transactions,
+                    } else {
+                        // TODO: is it really an error if we ask for a transaction hash, but the peer
+                        // doesn't know it? Should we close the connection on that kind of error?
+                        // Should we fake a NotFound response here? (#1515)
+                        let items = hashes.iter().map(|h| InventoryHash::Tx(*h)).collect();
+                        Handler::Finished(Err(PeerError::NotFound(items)))
                     }
                 }
             }
+            // `zcashd` peers actually return this response
             (
                 Handler::TransactionsByHash {
                     hashes,
@@ -119,23 +148,46 @@ impl Handler {
                 },
                 Message::NotFound(items),
             ) => {
-                let hash_in_hashes = |item: &InventoryHash| {
+                // assumptions:
-                    if let InventoryHash::Tx(hash) = item {
+                //   - the peer eventually returns a transaction or a `NotFound` entry
-                        hashes.contains(hash)
+                //     for each hash
-                    } else {
+                //   - all `NotFound` entries are contained in a single message
-                        false
+                //   - the `NotFound` message comes after the transaction messages
-                    }
+                //
-                };
+                // If we're in sync with the peer, then the `NotFound` should contain the remaining
-                if items.iter().all(hash_in_hashes) {
+                // hashes from the handler. If we're not in sync with the peer, we should return
-                    Handler::Finished(Err(PeerError::NotFound(items)))
+                // what we got so far, and log an error.
+                let missing_transactions: HashSet<_> = items
+                    .iter()
+                    .filter_map(|inv| match &inv {
+                        InventoryHash::Tx(tx) => Some(tx),
+                        _ => None,
+                    })
+                    .cloned()
+                    .collect();
+                if missing_transactions != hashes {
+                    trace!(?items, ?missing_transactions, ?hashes);
+                    // if these errors are noisy, we should replace them with debugs
+                    error!("unexpected notfound message from peer: all remaining transaction hashes should be listed in the notfound. Using partial received transactions as the peer response");
+                }
+                if missing_transactions.len() != items.len() {
+                    trace!(?items, ?missing_transactions, ?hashes);
+                    error!("unexpected notfound message from peer: notfound contains duplicate hashes or non-transaction hashes. Using partial received transactions as the peer response");
+                }
+
+                if !transactions.is_empty() {
+                    // TODO: does the caller need a list of missing transactions? (#1515)
+                    Handler::Finished(Ok(Response::Transactions(transactions)))
                 } else {
-                    ignored_msg = Some(Message::NotFound(items));
+                    // TODO: is it really an error if we ask for a transaction hash, but the peer
-                    Handler::TransactionsByHash {
+                    // doesn't know it? Should we close the connection on that kind of error? (#1515)
-                        hashes,
+                    Handler::Finished(Err(PeerError::NotFound(items)))
-                        transactions,
-                    }
                 }
             }
+            // `zcashd` returns requested blocks in a single batch of messages.
+            // Other blocks or non-blocks messages can come before or after the batch.
+            // `zcashd` silently skips missing blocks, rather than sending a final `NotFound` message.
+            // https://github.com/zcash/zcash/blob/e7b425298f6d9a54810cb7183f00be547e4d9415/src/main.cpp#L5523
             (
                 Handler::BlocksByHash {
                     mut hashes,
@@ -143,30 +195,80 @@ impl Handler {
                 },
                 Message::Block(block),
             ) => {
+                // assumptions:
+                //   - the block messages are sent in a single continuous batch
+                //   - missing blocks are silently skipped
+                //     (there is no `NotFound` message at the end of the batch)
                 if hashes.remove(&block.hash()) {
+                    // we are in the middle of the continuous block messages
                     blocks.push(block);
+                    if hashes.is_empty() {
+                        Handler::Finished(Ok(Response::Blocks(blocks)))
+                    } else {
+                        Handler::BlocksByHash { hashes, blocks }
+                    }
                 } else {
+                    // We got a block we didn't ask for.
+                    //
+                    // So either:
+                    // 1. The peer doesn't know any of the blocks we asked for.
+                    //    We should cancel the request, so we don't hang waiting for blocks that
+                    //    will never arrive.
+                    // 2. The peer sent an unsolicited block.
+                    //    We should ignore that block, and wait for the actual response.
+                    //
+                    // We end the request, so we don't hang on forked or lagging peers (case 1).
+                    // But we keep the connection open, so the inbound service can process blocks
+                    // from good peers (case 2).
                     ignored_msg = Some(Message::Block(block));
-                }
+                    if !blocks.is_empty() {
-                if hashes.is_empty() {
+                        // TODO: does the caller need a list of missing blocks? (#1515)
-                    Handler::Finished(Ok(Response::Blocks(blocks)))
+                        Handler::Finished(Ok(Response::Blocks(blocks)))
-                } else {
+                    } else {
-                    Handler::BlocksByHash { hashes, blocks }
+                        // TODO: is it really an error if we ask for a block hash, but the peer
+                        // doesn't know it? Should we close the connection on that kind of error?
+                        // Should we fake a NotFound response here? (#1515)
+                        let items = hashes.iter().map(|h| InventoryHash::Block(*h)).collect();
+                        Handler::Finished(Err(PeerError::NotFound(items)))
+                    }
                 }
             }
+            // peers are allowed to return this response, but `zcashd` never does
             (Handler::BlocksByHash { hashes, blocks }, Message::NotFound(items)) => {
-                let hash_in_hashes = |item: &InventoryHash| {
+                // assumptions:
-                    if let InventoryHash::Block(hash) = item {
+                //   - the peer eventually returns a block or a `NotFound` entry
-                        hashes.contains(hash)
+                //     for each hash
-                    } else {
+                //   - all `NotFound` entries are contained in a single message
-                        false
+                //   - the `NotFound` message comes after the block messages
-                    }
+                //
-                };
+                // If we're in sync with the peer, then the `NotFound` should contain the remaining
-                if items.iter().all(hash_in_hashes) {
+                // hashes from the handler. If we're not in sync with the peer, we should return
-                    Handler::Finished(Err(PeerError::NotFound(items)))
+                // what we got so far, and log an error.
+                let missing_blocks: HashSet<_> = items
+                    .iter()
+                    .filter_map(|inv| match &inv {
+                        InventoryHash::Block(b) => Some(b),
+                        _ => None,
+                    })
+                    .cloned()
+                    .collect();
+                if missing_blocks != hashes {
+                    trace!(?items, ?missing_blocks, ?hashes);
+                    // if these errors are noisy, we should replace them with debugs
+                    error!("unexpected notfound message from peer: all remaining block hashes should be listed in the notfound. Using partial received blocks as the peer response");
+                }
+                if missing_blocks.len() != items.len() {
+                    trace!(?items, ?missing_blocks, ?hashes);
+                    error!("unexpected notfound message from peer: notfound contains duplicate hashes or non-block hashes. Using partial received blocks as the peer response");
+                }
+
+                if !blocks.is_empty() {
+                    // TODO: does the caller need a list of missing blocks? (#1515)
+                    Handler::Finished(Ok(Response::Blocks(blocks)))
                 } else {
-                    ignored_msg = Some(Message::NotFound(items));
+                    // TODO: is it really an error if we ask for a block hash, but the peer
-                    Handler::BlocksByHash { hashes, blocks }
+                    // doesn't know it? Should we close the connection on that kind of error? (#1515)
+                    Handler::Finished(Err(PeerError::NotFound(items)))
                 }
             }
             (Handler::FindBlocks, Message::Inv(items))

zebra-network/src/protocol/external/message.rs

@@ -193,7 +193,13 @@ pub enum Message {
     /// content of a specific object, and is usually sent after
     /// receiving an `inv` packet, after filtering known elements.
     ///
+    /// `zcashd` returns requested items in a single batch of messages.
+    /// Missing blocks are silently skipped. Missing transaction hashes are
+    /// included in a single `NotFound` message following the transactions.
+    /// Other item or non-item messages can come before or after the batch.
+    ///
     /// [Bitcoin reference](https://en.bitcoin.it/wiki/Protocol_documentation#getdata)
+    /// [zcashd code](https://github.com/zcash/zcash/blob/e7b425298f6d9a54810cb7183f00be547e4d9415/src/main.cpp#L5523)
     GetData(Vec<InventoryHash>),
 
     /// A `block` message.
@@ -208,7 +214,16 @@ pub enum Message {
 
     /// A `notfound` message.
     ///
+    /// When a peer requests a list of transaction hashes, `zcashd` returns:
+    ///   - a batch of messages containing found transactions, then
+    ///   - a `NotFound` message containing a list of transaction hashes that
+    ///      aren't available in its mempool or state.
+    ///
+    /// But when a peer requests blocks or headers, any missing items are
+    /// silently skipped, without any `NotFound` messages.
+    ///
     /// [Bitcoin reference](https://en.bitcoin.it/wiki/Protocol_documentation#notfound)
+    /// [zcashd code](https://github.com/zcash/zcash/blob/e7b425298f6d9a54810cb7183f00be547e4d9415/src/main.cpp#L5632)
     // See note above on `Inventory`.
     NotFound(Vec<InventoryHash>),
 

zebra-network/src/protocol/internal/request.rs

@@ -78,7 +78,7 @@ pub enum Request {
     /// Returns [`Response::Transactions`](super::Response::Transactions).
     TransactionsByHash(HashSet<transaction::Hash>),
 
-    /// Request block hashes of subsequent blocks in the chain, giving hashes of
+    /// Request block hashes of subsequent blocks in the chain, given hashes of
     /// known blocks.
     ///
     /// # Returns
@@ -104,7 +104,7 @@ pub enum Request {
         stop: Option<block::Hash>,
     },
 
-    /// Request headers of subsequent blocks in the chain, giving hashes of
+    /// Request headers of subsequent blocks in the chain, given hashes of
     /// known blocks.
     ///
     /// # Returns

zebrad/src/components/inbound.rs

@@ -167,8 +167,10 @@ impl Service<zn::Request> for Inbound {
                     .try_filter_map(|rsp| {
                         futures::future::ready(match rsp {
                             zs::Response::Block(Some(block)) => Ok(Some(block)),
-                            // XXX: check how zcashd handles missing blocks?
+                            // `zcashd` ignores missing blocks in GetData responses,
-                            zs::Response::Block(None) => Err("missing block".into()),
+                            // rather than including them in a trailing `NotFound`
+                            // message
+                            zs::Response::Block(None) => Ok(None),
                             _ => unreachable!("wrong response from state"),
                         })
                     })
@@ -177,6 +179,13 @@ impl Service<zn::Request> for Inbound {
                     .boxed()
             }
             zn::Request::TransactionsByHash(_transactions) => {
+                // `zcashd` returns a list of found transactions, followed by a
+                // `NotFound` message if any transactions are missing. `zcashd`
+                // says that Simplified Payment Verification (SPV) clients rely on
+                // this behaviour - are there any of them on the Zcash network?
+                // https://github.com/zcash/zcash/blob/e7b425298f6d9a54810cb7183f00be547e4d9415/src/main.cpp#L5632
+                // We'll implement this request once we have a mempool:
+                // https://en.bitcoin.it/wiki/Protocol_documentation#getdata
                 debug!("ignoring unimplemented request");
                 async { Ok(zn::Response::Nil) }.boxed()
             }