Fix syncer download order and add sync tests (#3168)

* Refactor so that RetryLimit::Future is std::marker::Sync

* Make the syncer future std::marker::Send by spawning tips futures

* Download synced blocks in chain order, not HashSet order

* Improve MockService failure messages

* Add closure-based responses to the MockService API

* Move MockChainTip to zebra-chain

* Add a MockChainTipSender type alias

* Support MockChainTip in ChainSync and its downloader

* Add syncer tests for obtain tips, extend tips, and wrong block hashes

* Add block too high tests for obtain tips and extend tips

* Add syncer tests for duplicate FindBlocks response hashes

* Allow longer request delays for mocked services in syncer tests

zebra-chain/Cargo.toml

@@ -9,7 +9,7 @@ edition = "2018"
 
 [features]
 default = []
-proptest-impl = ["proptest", "proptest-derive", "zebra-test", "rand", "rand_chacha"]
+proptest-impl = ["proptest", "proptest-derive", "zebra-test", "rand", "rand_chacha", "tokio"]
 bench = ["zebra-test"]
 
 [dependencies]
@@ -60,6 +60,7 @@ proptest-derive = { version = "0.3.0", optional = true }
 
 rand = { version = "0.8", optional = true }
 rand_chacha = { version = "0.3", optional = true }
+tokio = { version = "1.15.0", optional = true }
 
 # ZF deps
 ed25519-zebra = "3.0.0"

zebra-chain/src/chain_tip.rs

@@ -4,6 +4,9 @@ use std::sync::Arc;
 
 use crate::{block, transaction};
 
+#[cfg(any(test, feature = "proptest-impl"))]
+pub mod mock;
+
 /// An interface for querying the chain tip.
 ///
 /// This trait helps avoid dependencies between:

zebra-chain/src/chain_tip/mock.rs

@@ -0,0 +1,48 @@
+//! Mock [`ChainTip`]s for use in tests.
+
+use std::sync::Arc;
+
+use tokio::sync::watch;
+
+use crate::{block, chain_tip::ChainTip, transaction};
+
+/// A sender that sets the `best_tip_height` of a [`MockChainTip`].]
+pub type MockChainTipSender = watch::Sender<Option<block::Height>>;
+
+/// A mock [`ChainTip`] implementation that allows setting the `best_tip_height` externally.
+#[derive(Clone, Debug)]
+pub struct MockChainTip {
+    best_tip_height: watch::Receiver<Option<block::Height>>,
+}
+
+impl MockChainTip {
+    /// Create a new [`MockChainTip`].
+    ///
+    /// Returns the [`MockChainTip`] instance and the endpoint to modiy the current best tip
+    /// height.
+    ///
+    /// Initially, the best tip height is [`None`].
+    pub fn new() -> (Self, MockChainTipSender) {
+        let (sender, receiver) = watch::channel(None);
+
+        let mock_chain_tip = MockChainTip {
+            best_tip_height: receiver,
+        };
+
+        (mock_chain_tip, sender)
+    }
+}
+
+impl ChainTip for MockChainTip {
+    fn best_tip_height(&self) -> Option<block::Height> {
+        *self.best_tip_height.borrow()
+    }
+
+    fn best_tip_hash(&self) -> Option<block::Hash> {
+        unreachable!("Method not used in tests");
+    }
+
+    fn best_tip_mined_transaction_ids(&self) -> Arc<[transaction::Hash]> {
+        unreachable!("Method not used in tests");
+    }
+}

zebra-network/src/peer/minimum_peer_version/tests.rs

@@ -1,54 +1,17 @@
-use std::sync::Arc;
+//! Test utilities and tests for minimum network peer version requirements.
 
-use tokio::sync::watch;
-
-use zebra_chain::{block, chain_tip::ChainTip, parameters::Network, transaction};
+use zebra_chain::{
+    chain_tip::mock::{MockChainTip, MockChainTipSender},
+    parameters::Network,
+};
 
 use super::MinimumPeerVersion;
 
 #[cfg(test)]
 mod prop;
 
-/// A mock [`ChainTip`] implementation that allows setting the `best_tip_height` externally.
-#[derive(Clone)]
-pub struct MockChainTip {
-    best_tip_height: watch::Receiver<Option<block::Height>>,
-}
-
-impl MockChainTip {
-    /// Create a new [`MockChainTip`].
-    ///
-    /// Returns the [`MockChainTip`] instance and the endpoint to modiy the current best tip
-    /// height.
-    ///
-    /// Initially, the best tip height is [`None`].
-    pub fn new() -> (Self, watch::Sender<Option<block::Height>>) {
-        let (sender, receiver) = watch::channel(None);
-
-        let mock_chain_tip = MockChainTip {
-            best_tip_height: receiver,
-        };
-
-        (mock_chain_tip, sender)
-    }
-}
-
-impl ChainTip for MockChainTip {
-    fn best_tip_height(&self) -> Option<block::Height> {
-        *self.best_tip_height.borrow()
-    }
-
-    fn best_tip_hash(&self) -> Option<block::Hash> {
-        unreachable!("Method not used in `MinimumPeerVersion` tests");
-    }
-
-    fn best_tip_mined_transaction_ids(&self) -> Arc<[transaction::Hash]> {
-        unreachable!("Method not used in `MinimumPeerVersion` tests");
-    }
-}
-
 impl MinimumPeerVersion<MockChainTip> {
-    pub fn with_mock_chain_tip(network: Network) -> (Self, watch::Sender<Option<block::Height>>) {
+    pub fn with_mock_chain_tip(network: Network) -> (Self, MockChainTipSender) {
         let (chain_tip, best_tip_height) = MockChainTip::new();
         let minimum_peer_version = MinimumPeerVersion::new(chain_tip, network);
 

zebra-network/src/policies.rs

@@ -6,7 +6,7 @@ use tower::retry::Policy;
 /// A very basic retry policy with a limited number of retry attempts.
 ///
 /// XXX Remove this when https://github.com/tower-rs/tower/pull/414 lands.
-#[derive(Clone, Debug)]
+#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)]
 pub struct RetryLimit {
     remaining_tries: usize,
 }
@@ -21,25 +21,23 @@ impl RetryLimit {
 }
 
 impl<Req: Clone + std::fmt::Debug, Res, E: std::fmt::Debug> Policy<Req, Res, E> for RetryLimit {
-    type Future = Pin<Box<dyn Future<Output = Self> + Send + 'static>>;
+    type Future = Pin<Box<dyn Future<Output = Self> + Send + Sync + 'static>>;
 
     fn retry(&self, req: &Req, result: Result<&Res, &E>) -> Option<Self::Future> {
         if let Err(e) = result {
             if self.remaining_tries > 0 {
                 tracing::debug!(?req, ?e, remaining_tries = self.remaining_tries, "retrying");
+
                 let remaining_tries = self.remaining_tries - 1;
+                let retry_outcome = RetryLimit { remaining_tries };
 
                 Some(
-                    async move {
-                        // Let other tasks run, so we're more likely to choose a different peer,
-                        // and so that any notfound inv entries win the race to the PeerSet.
-                        //
-                        // TODO: move syncer retries into the PeerSet,
-                        //       so we always choose different peers (#3235)
-                        tokio::task::yield_now().await;
-                        RetryLimit { remaining_tries }
-                    }
-                    .boxed(),
+                    // Let other tasks run, so we're more likely to choose a different peer,
+                    // and so that any notfound inv entries win the race to the PeerSet.
+                    //
+                    // TODO: move syncer retries into the PeerSet,
+                    //       so we always choose different peers (#3235)
+                    Box::pin(tokio::task::yield_now().map(move |()| retry_outcome)),
                 )
             } else {
                 None