diff --git a/zebra-network/src/peer/connection.rs b/zebra-network/src/peer/connection.rs index 24322335a64..7b63d890ac3 100644 --- a/zebra-network/src/peer/connection.rs +++ b/zebra-network/src/peer/connection.rs @@ -1,18 +1,11 @@ -// NOT A PLACE OF HONOR -// -// NO ESTEEMED DEED IS COMMEMORATED HERE -// -// NOTHING VALUED IS HERE -// -// What is here was dangerous and repulsive to us. This message is a warning -// about danger. -// -// 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. +//! Zcash peer connection protocol handing for Zebra. +//! +//! Maps the external Zcash/Bitcoin protocol to Zebra's internal request/response +//! protocol. +//! +//! This module contains a lot of undocumented state, assumptions and invariants. +//! And it's unclear if these assumptions match the `zcashd` implementation. +//! It should be refactored into a cleaner set of request/response pairs (#1515). use std::collections::HashSet; use std::sync::Arc; @@ -91,6 +84,9 @@ impl Handler { } } (Handler::Peers, Message::Addr(addrs)) => Handler::Finished(Ok(Response::Peers(addrs))), + // `zcashd` returns transactions contiguously (but not necessarily immediately). + // It uses `NotFound` if any transactions are missing: + // https://github.com/zcash/zcash/blob/e7b425298f6d9a54810cb7183f00be547e4d9415/src/main.cpp#L5617 ( Handler::TransactionsByHash { mut hashes, @@ -98,20 +94,38 @@ impl Handler { }, Message::Tx(transaction), ) => { + // assumptions: + // - the transaction list is contiguous + // - missing transaction hashes are included in a `NotFound` message if hashes.remove(&transaction.hash()) { + // we are in the middle of the contiguous transaction response transactions.push(transaction); + if hashes.is_empty() { + Handler::Finished(Ok(Response::Transactions(transactions))) + } else { + Handler::TransactionsByHash { + hashes, + transactions, + } + } } else { + // we are before or after the contiguous block response ignored_msg = Some(Message::Tx(transaction)); - } - if hashes.is_empty() { - Handler::Finished(Ok(Response::Transactions(transactions))) - } else { - Handler::TransactionsByHash { - hashes, - transactions, + if !transactions.is_empty() { + warn!("unexpected transaction from peer: transaction responses should be contiguous, followed by notfound. Using partial received transactions as the peer response"); + // TODO: does the caller need a list of missing transactions? + Handler::Finished(Ok(Response::Transactions(transactions))) + } else { + // If the caller doesn't know any of the transactions, + // we'll get a `NotFound` with all the hashes + Handler::TransactionsByHash { + hashes, + transactions, + } } } } + // `zcashd` peers actually return this response ( Handler::TransactionsByHash { hashes, @@ -119,6 +133,11 @@ impl Handler { }, Message::NotFound(items), ) => { + // assumptions: + // - the peer eventually returns a transaction or a `NotFound` entry + // for each hash + // - all `NotFound` entries are contained in a single message + // - the `NotFound` message comes after the transactions let hash_in_hashes = |item: &InventoryHash| { if let InventoryHash::Tx(hash) = item { hashes.contains(hash) @@ -127,8 +146,21 @@ impl Handler { } }; if items.iter().all(hash_in_hashes) { - Handler::Finished(Err(PeerError::NotFound(items))) + // all hashes have a transaction or a `NotFound` entry + if !transactions.is_empty() { + // TODO: does the caller need a list of missing transactions? + Handler::Finished(Ok(Response::Transactions(transactions))) + } else { + Handler::Finished(Err(PeerError::NotFound(items))) + } } else { + // waiting for more transactions or a complete `NotFound` message + if items.iter().any(hash_in_hashes) { + debug!( + not_found_transactions = ?items.iter().cloned().filter(hash_in_hashes).count(), + remaining_transactions = ?hashes.len(), + "notfound containing some of the remaining requested transactions"); + } ignored_msg = Some(Message::NotFound(items)); Handler::TransactionsByHash { hashes, @@ -136,6 +168,9 @@ impl Handler { } } } + // `zcashd` returns blocks sequentially (but not necessarily immediately). + // It silently skips missing blocks, rather than using `NotFound`: + // https://github.com/zcash/zcash/blob/e7b425298f6d9a54810cb7183f00be547e4d9415/src/main.cpp#L5523 ( Handler::BlocksByHash { mut hashes, @@ -143,18 +178,37 @@ impl Handler { }, Message::Block(block), ) => { + // assumptions: + // - the block list is contiguous + // - missing blocks are silently skipped (rather than using `NotFound`) if hashes.remove(&block.hash()) { + // we are in the middle of the contiguous block response blocks.push(block); + if hashes.is_empty() { + Handler::Finished(Ok(Response::Blocks(blocks))) + } else { + Handler::BlocksByHash { hashes, blocks } + } } else { + // we are before or after the contiguous block response ignored_msg = Some(Message::Block(block)); - } - if hashes.is_empty() { - Handler::Finished(Ok(Response::Blocks(blocks))) - } else { - Handler::BlocksByHash { hashes, blocks } + if !blocks.is_empty() { + // TODO: does the caller need a list of missing blocks? + Handler::Finished(Ok(Response::Blocks(blocks))) + } else { + // TODO: what if the peer doesn't know any of the blocks + // we asked for? + Handler::BlocksByHash { hashes, blocks } + } } } + // peers are allowed to return this response, but `zcashd` never does (Handler::BlocksByHash { hashes, blocks }, Message::NotFound(items)) => { + // assumptions: + // - the peer eventually returns a block or a `NotFound` entry + // for each hash + // - all `NotFound` entries are contained in a single message + // - the `NotFound` message comes after the blocks let hash_in_hashes = |item: &InventoryHash| { if let InventoryHash::Block(hash) = item { hashes.contains(hash) @@ -163,8 +217,21 @@ impl Handler { } }; if items.iter().all(hash_in_hashes) { - Handler::Finished(Err(PeerError::NotFound(items))) + // all hashes have a block or a `NotFound` entry + if !blocks.is_empty() { + // TODO: does the caller need a list of missing blocks? + Handler::Finished(Ok(Response::Blocks(blocks))) + } else { + Handler::Finished(Err(PeerError::NotFound(items))) + } } else { + // waiting for more blocks or a complete `NotFound` message + if items.iter().any(hash_in_hashes) { + debug!( + not_found_blocks = ?items.iter().cloned().filter(hash_in_hashes).count(), + remaining_blocks = ?hashes.len(), + "notfound containing some of the remaining requested blocks"); + } ignored_msg = Some(Message::NotFound(items)); Handler::BlocksByHash { hashes, blocks } } diff --git a/zebra-network/src/protocol/external/message.rs b/zebra-network/src/protocol/external/message.rs index 930faed81ef..ac20e59aefd 100644 --- a/zebra-network/src/protocol/external/message.rs +++ b/zebra-network/src/protocol/external/message.rs @@ -193,7 +193,12 @@ pub enum Message { /// content of a specific object, and is usually sent after /// receiving an `inv` packet, after filtering known elements. /// + /// `zcashd` sends a response containing all known items, in a contiguous + /// list. Missing blocks are silently skipped. Missing transaction hashes are + /// included in a single `NotFound` message following the transactions. + /// /// [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), /// A `block` message. @@ -208,7 +213,12 @@ pub enum Message { /// A `notfound` message. /// + /// `zcashd` returns `notfound` if a requested transaction (`Tx`) isn't + /// available in its mempool or state. But missing blocks and headers are + /// silently skipped. + /// /// [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), diff --git a/zebra-network/src/protocol/internal/request.rs b/zebra-network/src/protocol/internal/request.rs index b5b18e9cd98..364f0a0d543 100644 --- a/zebra-network/src/protocol/internal/request.rs +++ b/zebra-network/src/protocol/internal/request.rs @@ -78,7 +78,7 @@ pub enum Request { /// Returns [`Response::Transactions`](super::Response::Transactions). TransactionsByHash(HashSet), - /// 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, }, - /// 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 diff --git a/zebrad/src/components/inbound.rs b/zebrad/src/components/inbound.rs index 416d56c3243..fea7a8d5289 100644 --- a/zebrad/src/components/inbound.rs +++ b/zebrad/src/components/inbound.rs @@ -167,8 +167,10 @@ impl Service 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? - zs::Response::Block(None) => Err("missing block".into()), + // `zcashd` ignores missing blocks in GetData responses, + // rather than including them in a trailing `NotFound` + // message + zs::Response::Block(None) => Ok(None), _ => unreachable!("wrong response from state"), }) }) @@ -177,6 +179,11 @@ impl Service for Inbound { .boxed() } zn::Request::TransactionsByHash(_transactions) => { + // `zcashd` returns a list of found blocks, followed by a + // `NotFound` message if any transactions are missing. `zcashd` + // says that 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 debug!("ignoring unimplemented request"); async { Ok(zn::Response::Nil) }.boxed() }