Skip to content

Commit

Permalink
Rewrite GetData handling to match the zcashd implementation
Browse files Browse the repository at this point in the history 
`zcashd` silently ignores missing blocks, but sends found transactions
followed by a `NotFound` message:
https://github.com/zcash/zcash/blob/e7b425298f6d9a54810cb7183f00be547e4d9415/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.
  • Loading branch information
@teor2345
teor2345 committed Dec 14, 2020
1 parent b8646fa commit 9823dbd
Show file tree
Hide file tree
Showing 4 changed files with 117 additions and 33 deletions.
125 changes: 96 additions & 29 deletions zebra-network/src/peer/connection.rs
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -91,34 +84,60 @@ 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,
mut transactions,
},
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,
transactions,
},
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)
Expand All @@ -127,34 +146,69 @@ 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,
transactions,
}
}
}
// `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,
mut blocks,
},
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)
Expand All @@ -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 }
}
Expand Down
10 changes: 10 additions & 0 deletions zebra-network/src/protocol/external/message.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<InventoryHash>),

/// A `block` message.
Expand All @@ -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<InventoryHash>),

Expand Down
4 changes: 2 additions & 2 deletions zebra-network/src/protocol/internal/request.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
11 changes: 9 additions & 2 deletions zebrad/src/components/inbound.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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?
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"),
})
})
Expand All @@ -177,6 +179,11 @@ impl Service<zn::Request> 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()
}
Expand Down

0 comments on commit 9823dbd

Please sign in to comment.