From 437549d8e91a0934cf4b4888f9b52c0cd56798f3 Mon Sep 17 00:00:00 2001 From: teor Date: Fri, 4 Sep 2020 08:09:34 +1000 Subject: [PATCH] Always drop the final hash in peer responses (#991) To workaround a zcashd bug that squashes responses together. --- zebrad/src/commands/start/sync.rs | 61 +++++++++++++++++++++++-------- 1 file changed, 46 insertions(+), 15 deletions(-) diff --git a/zebrad/src/commands/start/sync.rs b/zebrad/src/commands/start/sync.rs index cd7a0897..e0d2436c 100644 --- a/zebrad/src/commands/start/sync.rs +++ b/zebrad/src/commands/start/sync.rs @@ -198,6 +198,24 @@ where while let Some(res) = requests.next().await { match res.map_err::(|e| eyre!(e)) { Ok(zn::Response::BlockHashes(hashes)) => { + tracing::trace!(?hashes); + + // zcashd sometimes appends an unrelated hash at the start + // or end of its response. + // + // We can't discard the first hash, because it might be a + // block we want to download. So we just accept any + // out-of-order first hashes. + + // We use the last hash for the tip, and we want to avoid bad + // tips. So we discard the last hash. (We don't need to worry + // about missed downloads, because we will pick them up again + // in ExtendTips.) + let hashes = match hashes.as_slice() { + [] => continue, + [rest @ .., _last] => rest, + }; + let mut first_unknown = None; for (i, &hash) in hashes.iter().enumerate() { if !self.state_contains(hash).await? { @@ -235,7 +253,10 @@ where .retain(|t| !unknown_hashes.contains(&t.expected_next)); self.prospective_tips.insert(new_tip); } else { - tracing::debug!(?new_tip, "discarding tip already queued for download"); + tracing::debug!( + ?new_tip, + "discarding prospective tip: already in download set" + ); } let prev_download_len = download_set.len(); @@ -323,7 +344,14 @@ where } }; - tracing::trace!(?unknown_hashes); + // We use the last hash for the tip, and we want to avoid + // bad tips. So we discard the last hash. (We don't need + // to worry about missed downloads, because we will pick + // them up again in the next ExtendTips.) + let unknown_hashes = match unknown_hashes { + [] => continue, + [rest @ .., _last] => rest, + }; let new_tip = if let Some(end) = unknown_hashes.rchunks_exact(2).next() { CheckedTip { @@ -335,7 +363,7 @@ where continue; }; - tracing::trace!(?hashes); + tracing::trace!(?unknown_hashes); // Make sure we get the same tips, regardless of the // order of peer responses @@ -346,7 +374,10 @@ where .retain(|t| !unknown_hashes.contains(&t.expected_next)); self.prospective_tips.insert(new_tip); } else { - tracing::debug!(?new_tip, "discarding tip already queued for download"); + tracing::debug!( + ?new_tip, + "discarding prospective tip: already in download set" + ); } let prev_download_len = download_set.len(); @@ -370,7 +401,7 @@ where Ok(()) } - /// Queue a download for the genesis block, if it isn't currently known to + /// Download and verify the genesis block, if it isn't currently known to /// our node. async fn request_genesis(&mut self) -> Result<(), Report> { // Due to Bitcoin protocol limitations, we can't request the genesis @@ -379,15 +410,15 @@ where // - responses start with the block *after* the requested block, and // - the genesis hash is used as a placeholder for "no matches". // - // So we just queue the genesis block here. + // So we just download and verify the genesis block here. while !self.state_contains(self.genesis_hash).await? { self.request_blocks(vec![self.genesis_hash]).await?; match self .pending_blocks .next() .await - .expect("inserted a download request") - .expect("block download tasks should not panic") + .expect("inserted a download and verify request") + .expect("block download and verify tasks should not panic") { Ok(hash) => tracing::debug!(?hash, "verified and committed block to state"), Err(e) => tracing::warn!(?e, "could not download genesis block, retrying"), @@ -397,11 +428,13 @@ where Ok(()) } - /// Queue downloads for each block that isn't currently known to our node + /// Queue download and verify tasks for each block that isn't currently known to our node async fn request_blocks(&mut self, hashes: Vec) -> Result<(), Report> { tracing::debug!(hashes.len = hashes.len(), "requesting blocks"); for hash in hashes.into_iter() { - // TODO: remove this check once the sync service is more reliable + // Avoid re-downloading blocks that have already been verified. + // This is particularly important for nodes on slow or unreliable + // networks. if self.state_contains(hash).await? { tracing::debug!( ?hash, @@ -409,7 +442,7 @@ where ); continue; } - // We construct the block download requests sequentially, waiting + // We construct the block requests sequentially, waiting // for the peer set to be ready to process each request. This // ensures that we start block downloads in the order we want them // (though they may resolve out of order), and it means that we @@ -424,7 +457,7 @@ where .map_err(|e| eyre!(e))? .call(zn::Request::BlocksByHash(iter::once(hash).collect())); - tracing::debug!(?hash, "requested block"); + tracing::trace!(?hash, "requested block"); let span = tracing::info_span!("block_fetch_verify", ?hash); let mut verifier = self.verifier.clone(); @@ -452,11 +485,9 @@ where Ok(()) } - /// Returns `Ok(true)` if the hash is present in the state, and `Ok(false)` + /// Returns `true` if the hash is present in the state, and `false` /// if the hash is not present in the state. /// - /// Returns `Err(_)` if an error occurs. - /// /// TODO: handle multiple tips in the state. async fn state_contains(&mut self, hash: block::Hash) -> Result { match self