Revert "Stop returning NotFound errors, use the response instead" (#3124)
* Revert "Stop returning NotFound errors, use the response instead"
This reverts commit 45871f6915.
* Fix clippy warnings
* Downgrade a frequent log to debug level
This commit is contained in:
teor
GitHub
parent
ebc40f8af7
commit
ab471b0db0
|
|
@ -160,20 +160,19 @@ impl Handler {
|
|||
// connection open, so the inbound service can process transactions from good
|
||||
// peers (case 2).
|
||||
ignored_msg = Some(Message::Tx(transaction));
|
||||
|
||||
if !pending_ids.is_empty() {
|
||||
if !transactions.is_empty() {
|
||||
// if our peers start sending mixed solicited and unsolicited transactions,
|
||||
// we should update this code to handle those responses
|
||||
info!(
|
||||
"unexpected transaction from peer: \
|
||||
transaction responses should be sent in a continuous sequence, \
|
||||
followed by notfound. \
|
||||
Using partial received transactions as the peer response"
|
||||
);
|
||||
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");
|
||||
// TODO: does the caller need a list of missing transactions? (#1515)
|
||||
Handler::Finished(Ok(Response::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 missing_transaction_ids = pending_ids.iter().map(Into::into).collect();
|
||||
Handler::Finished(Err(PeerError::NotFound(missing_transaction_ids)))
|
||||
}
|
||||
|
||||
// TODO: does the caller need a list of missing transactions? (#1515)
|
||||
Handler::Finished(Ok(Response::Transactions(transactions)))
|
||||
}
|
||||
}
|
||||
// `zcashd` peers actually return this response
|
||||
|
|
@ -194,27 +193,24 @@ impl Handler {
|
|||
// hashes from the handler. If we're not in sync with the peer, we should return
|
||||
// what we got so far, and log an error.
|
||||
let missing_transaction_ids: HashSet<_> = transaction_ids(&missing_invs).collect();
|
||||
|
||||
if missing_transaction_ids != pending_ids {
|
||||
trace!(?missing_invs, ?missing_transaction_ids, ?pending_ids);
|
||||
info!(
|
||||
"unexpected notfound message from peer: \
|
||||
all remaining transaction hashes should be listed in the notfound. \
|
||||
Using partial received transactions as the peer response"
|
||||
);
|
||||
// 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_transaction_ids.len() != missing_invs.len() {
|
||||
trace!(?missing_invs, ?missing_transaction_ids, ?pending_ids);
|
||||
info!(
|
||||
"unexpected notfound message from peer: \
|
||||
notfound contains duplicate hashes or non-transaction hashes. \
|
||||
Using partial received transactions as the peer response"
|
||||
);
|
||||
error!("unexpected notfound message from peer: notfound contains duplicate hashes or non-transaction hashes. Using partial received transactions as the peer response");
|
||||
}
|
||||
|
||||
// TODO: does the caller need a list of missing transactions? (#1515)
|
||||
Handler::Finished(Ok(Response::Transactions(transactions)))
|
||||
if !transactions.is_empty() {
|
||||
// TODO: does the caller need a list of missing transactions? (#1515)
|
||||
Handler::Finished(Ok(Response::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? (#1515)
|
||||
Handler::Finished(Err(PeerError::NotFound(missing_invs)))
|
||||
}
|
||||
}
|
||||
// `zcashd` returns requested blocks in a single batch of messages.
|
||||
// Other blocks or non-blocks messages can come before or after the batch.
|
||||
|
|
@ -256,19 +252,19 @@ impl Handler {
|
|||
// 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 !pending_hashes.is_empty() {
|
||||
// if our peers start sending mixed solicited and unsolicited blocks,
|
||||
// we should update this code to handle those responses
|
||||
info!(
|
||||
"unexpected block from peer: \
|
||||
block responses should be sent in a continuous sequence. \
|
||||
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 {
|
||||
// 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 = pending_hashes
|
||||
.iter()
|
||||
.map(|h| InventoryHash::Block(*h))
|
||||
.collect();
|
||||
Handler::Finished(Err(PeerError::NotFound(items)))
|
||||
}
|
||||
|
||||
// TODO: does the caller need a list of missing blocks? (#1515)
|
||||
Handler::Finished(Ok(Response::Blocks(blocks)))
|
||||
}
|
||||
}
|
||||
// peers are allowed to return this response, but `zcashd` never does
|
||||
|
|
@ -296,37 +292,24 @@ impl Handler {
|
|||
})
|
||||
.cloned()
|
||||
.collect();
|
||||
|
||||
if missing_blocks != pending_hashes {
|
||||
trace!(?items, ?missing_blocks, ?pending_hashes);
|
||||
info!(
|
||||
"unexpected notfound message from peer: \
|
||||
all remaining block hashes should be listed in the notfound. \
|
||||
Using partial received blocks as the peer response"
|
||||
);
|
||||
// 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, ?pending_hashes);
|
||||
info!(
|
||||
"unexpected notfound message from peer: \
|
||||
notfound contains duplicate hashes or non-block hashes. \
|
||||
Using partial received blocks as the peer response"
|
||||
);
|
||||
error!("unexpected notfound message from peer: notfound contains duplicate hashes or non-block hashes. Using partial received blocks as the peer response");
|
||||
}
|
||||
|
||||
if !pending_hashes.is_empty() {
|
||||
// if our peers start sending mixed solicited and unsolicited blocks,
|
||||
// we should update this code to handle those responses
|
||||
info!(
|
||||
"unexpected block from peer: \
|
||||
block responses should be sent in a continuous sequence. \
|
||||
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 {
|
||||
// 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? (#1515)
|
||||
Handler::Finished(Err(PeerError::NotFound(items)))
|
||||
}
|
||||
|
||||
// TODO: does the caller need a list of missing blocks? (#1515)
|
||||
Handler::Finished(Ok(Response::Blocks(blocks)))
|
||||
}
|
||||
(Handler::FindBlocks, Message::Inv(items))
|
||||
if items
|
||||
|
|
@ -556,11 +539,11 @@ where
|
|||
pending @ State::AwaitingResponse { .. } => {
|
||||
// Drop the un-consumed request message,
|
||||
// because we can't process multiple messages at the same time.
|
||||
info!(
|
||||
debug!(
|
||||
new_request = %request_msg
|
||||
.as_ref()
|
||||
.map(|m| m.to_string())
|
||||
.unwrap_or_else(|| "None".to_string()),
|
||||
.unwrap_or_else(|| "None".into()),
|
||||
awaiting_response = %pending,
|
||||
"ignoring new request while awaiting a response"
|
||||
);
|
||||
|
|
|
|||
|
|
@ -5,6 +5,8 @@ use thiserror::Error;
|
|||
use tracing_error::TracedError;
|
||||
use zebra_chain::serialization::SerializationError;
|
||||
|
||||
use crate::protocol::external::InventoryHash;
|
||||
|
||||
/// A wrapper around `Arc<PeerError>` that implements `Error`.
|
||||
#[derive(Error, Debug, Clone)]
|
||||
#[error(transparent)]
|
||||
|
|
@ -44,6 +46,13 @@ pub enum PeerError {
|
|||
/// to shed load.
|
||||
#[error("Internal services over capacity")]
|
||||
Overloaded,
|
||||
|
||||
// TODO: stop closing connections on these errors (#2107)
|
||||
// log info or debug logs instead
|
||||
//
|
||||
/// We requested data that the peer couldn't find.
|
||||
#[error("Remote peer could not find items: {0:?}")]
|
||||
NotFound(Vec<InventoryHash>),
|
||||
}
|
||||
|
||||
/// A shared error slot for peer errors.
|
||||
|
|
|
|||
|
|
@ -145,6 +145,9 @@ pub enum Message {
|
|||
/// - parses received `addrv2` messages, ignoring some address types,
|
||||
/// - but does not send `addrv2` messages.
|
||||
///
|
||||
///
|
||||
/// The list contains `0..=MAX_META_ADDR` addresses.
|
||||
///
|
||||
/// Because some address types are ignored, the deserialized vector can be empty,
|
||||
/// even if the peer sent addresses. This is not an error.
|
||||
///
|
||||
|
|
@ -163,6 +166,8 @@ pub enum Message {
|
|||
/// If supplied, the `stop` parameter specifies the last header to request.
|
||||
/// Otherwise, an inv packet with the maximum number (500) are sent.
|
||||
///
|
||||
/// The known blocks list contains zero or more block hashes.
|
||||
///
|
||||
/// [Bitcoin reference](https://en.bitcoin.it/wiki/Protocol_documentation#getheaders)
|
||||
GetBlocks {
|
||||
/// Hashes of known blocks, ordered from highest height to lowest height.
|
||||
|
|
@ -177,6 +182,8 @@ pub enum Message {
|
|||
/// objects. It can be received unsolicited, or in reply to
|
||||
/// `getblocks`.
|
||||
///
|
||||
/// The list contains zero or more inventory hashes.
|
||||
///
|
||||
/// [Bitcoin reference](https://en.bitcoin.it/wiki/Protocol_documentation#inv)
|
||||
/// [ZIP-239](https://zips.z.cash/zip-0239)
|
||||
Inv(Vec<InventoryHash>),
|
||||
|
|
@ -192,6 +199,8 @@ pub enum Message {
|
|||
/// If supplied, the `stop` parameter specifies the last header to request.
|
||||
/// Otherwise, the maximum number of block headers (160) are sent.
|
||||
///
|
||||
/// The known blocks list contains zero or more block hashes.
|
||||
///
|
||||
/// [Bitcoin reference](https://en.bitcoin.it/wiki/Protocol_documentation#getheaders)
|
||||
GetHeaders {
|
||||
/// Hashes of known blocks, ordered from highest height to lowest height.
|
||||
|
|
@ -206,6 +215,8 @@ pub enum Message {
|
|||
///
|
||||
/// Each block header is accompanied by a transaction count.
|
||||
///
|
||||
/// The list contains zero or more headers.
|
||||
///
|
||||
/// [Bitcoin reference](https://en.bitcoin.it/wiki/Protocol_documentation#headers)
|
||||
Headers(Vec<block::CountedHeader>),
|
||||
|
||||
|
|
@ -220,6 +231,8 @@ pub enum Message {
|
|||
/// included in a single `NotFound` message following the transactions.
|
||||
/// Other item or non-item messages can come before or after the batch.
|
||||
///
|
||||
/// The list contains zero or more inventory hashes.
|
||||
///
|
||||
/// [Bitcoin reference](https://en.bitcoin.it/wiki/Protocol_documentation#getdata)
|
||||
/// [ZIP-239](https://zips.z.cash/zip-0239)
|
||||
/// [zcashd code](https://github.com/zcash/zcash/blob/e7b425298f6d9a54810cb7183f00be547e4d9415/src/main.cpp#L5523)
|
||||
|
|
@ -247,6 +260,8 @@ pub enum Message {
|
|||
/// But when a peer requests blocks or headers, any missing items are
|
||||
/// silently skipped, without any `NotFound` messages.
|
||||
///
|
||||
/// The list contains zero or more inventory hashes.
|
||||
///
|
||||
/// [Bitcoin reference](https://en.bitcoin.it/wiki/Protocol_documentation#notfound)
|
||||
/// [ZIP-239](https://zips.z.cash/zip-0239)
|
||||
/// [zcashd code](https://github.com/zcash/zcash/blob/e7b425298f6d9a54810cb7183f00be547e4d9415/src/main.cpp#L5632)
|
||||
|
|
@ -265,6 +280,8 @@ pub enum Message {
|
|||
///
|
||||
/// This was defined in [BIP37], which is included in Zcash.
|
||||
///
|
||||
/// Zebra currently ignores this message.
|
||||
///
|
||||
/// [Bitcoin reference](https://en.bitcoin.it/wiki/Protocol_documentation#filterload.2C_filteradd.2C_filterclear.2C_merkleblock)
|
||||
/// [BIP37]: https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki
|
||||
FilterLoad {
|
||||
|
|
@ -288,6 +305,8 @@ pub enum Message {
|
|||
///
|
||||
/// This was defined in [BIP37], which is included in Zcash.
|
||||
///
|
||||
/// Zebra currently ignores this message.
|
||||
///
|
||||
/// [Bitcoin reference](https://en.bitcoin.it/wiki/Protocol_documentation#filterload.2C_filteradd.2C_filterclear.2C_merkleblock)
|
||||
/// [BIP37]: https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki
|
||||
FilterAdd {
|
||||
|
|
@ -304,6 +323,8 @@ pub enum Message {
|
|||
///
|
||||
/// This was defined in [BIP37], which is included in Zcash.
|
||||
///
|
||||
/// Zebra currently ignores this message.
|
||||
///
|
||||
/// [Bitcoin reference](https://en.bitcoin.it/wiki/Protocol_documentation#filterload.2C_filteradd.2C_filterclear.2C_merkleblock)
|
||||
/// [BIP37]: https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki
|
||||
FilterClear,
|
||||
|
|
|
|||
|
|
@ -63,6 +63,8 @@ pub enum Request {
|
|||
/// block. This routing is only used for request sets of size 1.
|
||||
/// Otherwise, it is routed using the normal load-balancing strategy.
|
||||
///
|
||||
/// The list contains zero or more block hashes.
|
||||
///
|
||||
/// # Returns
|
||||
///
|
||||
/// Returns [`Response::Blocks`](super::Response::Blocks).
|
||||
|
|
@ -80,6 +82,8 @@ pub enum Request {
|
|||
/// the transaction. This routing is only used for request sets of size 1.
|
||||
/// Otherwise, it is routed using the normal load-balancing strategy.
|
||||
///
|
||||
/// The list contains zero or more unmined transaction IDs.
|
||||
///
|
||||
/// # Returns
|
||||
///
|
||||
/// Returns [`Response::Transactions`](super::Response::Transactions).
|
||||
|
|
@ -88,6 +92,8 @@ pub enum Request {
|
|||
/// Request block hashes of subsequent blocks in the chain, given hashes of
|
||||
/// known blocks.
|
||||
///
|
||||
/// The known blocks list contains zero or more block hashes.
|
||||
///
|
||||
/// # Returns
|
||||
///
|
||||
/// Returns
|
||||
|
|
@ -106,6 +112,8 @@ pub enum Request {
|
|||
/// `inv` messages will always have exactly one block hash.
|
||||
FindBlocks {
|
||||
/// Hashes of known blocks, ordered from highest height to lowest height.
|
||||
//
|
||||
// TODO: make this into an IndexMap - an ordered unique list of hashes (#2244)
|
||||
known_blocks: Vec<block::Hash>,
|
||||
/// Optionally, the last block hash to request.
|
||||
stop: Option<block::Hash>,
|
||||
|
|
@ -114,12 +122,16 @@ pub enum Request {
|
|||
/// Request headers of subsequent blocks in the chain, given hashes of
|
||||
/// known blocks.
|
||||
///
|
||||
/// The known blocks list contains zero or more block hashes.
|
||||
///
|
||||
/// # Returns
|
||||
///
|
||||
/// Returns
|
||||
/// [`Response::BlockHeaders`](super::Response::BlockHeaders).
|
||||
FindHeaders {
|
||||
/// Hashes of known blocks, ordered from highest height to lowest height.
|
||||
//
|
||||
// TODO: make this into an IndexMap - an ordered unique list of hashes (#2244)
|
||||
known_blocks: Vec<block::Hash>,
|
||||
/// Optionally, the last header to request.
|
||||
stop: Option<block::Hash>,
|
||||
|
|
@ -150,8 +162,10 @@ pub enum Request {
|
|||
/// v4 transactions use a legacy transaction ID, and
|
||||
/// v5 transactions use a witnessed transaction ID.
|
||||
///
|
||||
/// The peer set routes this request specially, sending it to *every*
|
||||
/// available peer.
|
||||
/// The list contains zero or more transaction IDs.
|
||||
///
|
||||
/// The peer set routes this request specially, sending it to *half of*
|
||||
/// the available peers.
|
||||
///
|
||||
/// # Returns
|
||||
///
|
||||
|
|
@ -166,8 +180,8 @@ pub enum Request {
|
|||
/// [`Request::BlocksByHash`] against the "inbound" service passed to
|
||||
/// [`zebra_network::init`].
|
||||
///
|
||||
/// The peer set routes this request specially, sending it to *every*
|
||||
/// available peer.
|
||||
/// The peer set routes this request specially, sending it to *half of*
|
||||
/// the available peers.
|
||||
///
|
||||
/// # Returns
|
||||
///
|
||||
|
|
|
|||
|
|
@ -26,24 +26,49 @@ pub enum Response {
|
|||
Nil,
|
||||
|
||||
/// A list of peers, used to respond to `GetPeers`.
|
||||
///
|
||||
/// The list contains `0..=MAX_META_ADDR` peers.
|
||||
//
|
||||
// TODO: make this into a HashMap<SocketAddr, MetaAddr> - a unique list of peer addresses (#2244)
|
||||
Peers(Vec<MetaAddr>),
|
||||
|
||||
/// A list of blocks.
|
||||
///
|
||||
/// The list contains zero or more blocks.
|
||||
//
|
||||
// TODO: split this into found and not found (#2726)
|
||||
Blocks(Vec<Arc<Block>>),
|
||||
|
||||
/// A list of block hashes.
|
||||
///
|
||||
/// The list contains zero or more block hashes.
|
||||
//
|
||||
// TODO: make this into an IndexMap - an ordered unique list of hashes (#2244)
|
||||
BlockHashes(Vec<block::Hash>),
|
||||
|
||||
/// A list of block headers.
|
||||
///
|
||||
/// The list contains zero or more block headers.
|
||||
//
|
||||
// TODO: make this into a HashMap<block::Hash, CountedHeader> - a unique list of headers (#2244)
|
||||
// split this into found and not found (#2726)
|
||||
BlockHeaders(Vec<block::CountedHeader>),
|
||||
|
||||
/// A list of unmined transactions.
|
||||
///
|
||||
/// The list contains zero or more unmined transactions.
|
||||
//
|
||||
// TODO: split this into found and not found (#2726)
|
||||
Transactions(Vec<UnminedTx>),
|
||||
|
||||
/// A list of unmined transaction IDs.
|
||||
///
|
||||
/// v4 transactions use a legacy transaction ID, and
|
||||
/// v5 transactions use a witnessed transaction ID.
|
||||
///
|
||||
/// The list contains zero or more transaction IDs.
|
||||
//
|
||||
// TODO: make this into a HashSet - a unique list of transaction IDs (#2244)
|
||||
TransactionIds(Vec<UnminedTxId>),
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue