P2p: fix indefinite exchange of the same header list between peers when they are on different branches of a deep fork

Author: ImplOfAnImpl

CHANGELOG.md

@@ -81,6 +81,10 @@ The format is loosely based on [Keep a Changelog](https://keepachangelog.com/en/
     - Fixed a potential indefinite stalling of a particular connection when both nodes start sending
       large amounts of data to each other.
 
+    - Fixed a potential indefinite exchange of the same header list requests and responses if
+      the node and the peer are on different branches of a deep fork (such as the one happened
+      due to the recent hard fork).
+
   - Wallet CLI and RPC: the commands `account-utxos` and `standalone-multisig-utxos` and their RPC
     counterparts now return correct decimal amounts for tokens with non-default number of decimals.
 

chainstate/src/detail/error_classification.rs

@@ -450,7 +450,10 @@ impl BlockProcessingErrorClassification for PropertyQueryError {
             | PropertyQueryError::InvalidStartingBlockHeightForMainchainBlocks(_)
             | PropertyQueryError::InvalidBlockHeightRange { .. }
             | PropertyQueryError::UnsupportedTokenV0InOrder(_)
-            | PropertyQueryError::TokenInfoMissing(_) => BlockProcessingErrorClass::General,
+            | PropertyQueryError::TokenInfoMissing(_)
+            | PropertyQueryError::InvariantErrorLocatorDistancesWrongOrder => {
+                BlockProcessingErrorClass::General
+            }
             // Note: these errors are strange - sometimes they don't look like General, judging
             // by the code that uses them. But other times some of them seem to just wrap storage
             // errors.

chainstate/src/detail/query.rs

@@ -175,7 +175,7 @@ impl<'a, S: BlockchainStorageRead, V: TransactionVerificationStrategy> Chainstat
         }
     }
 
-    pub fn get_locator(&self) -> Result<Locator, PropertyQueryError> {
+    pub fn get_locator_from_best_block(&self) -> Result<Locator, PropertyQueryError> {
         let best_block_index = self.chainstate_ref.get_best_block_index()?;
         let height = best_block_index.block_height();
         self.get_locator_from_height(height)
@@ -185,11 +185,48 @@ impl<'a, S: BlockchainStorageRead, V: TransactionVerificationStrategy> Chainstat
         &self,
         height: BlockHeight,
     ) -> Result<Locator, PropertyQueryError> {
-        let headers = locator_tip_distances()
+        let block_id_results = locator_tip_distances()
             .map_while(|dist| height - dist)
             .map(|ht| self.chainstate_ref.get_block_id_by_height(ht));
 
-        itertools::process_results(headers, |iter| iter.flatten().collect::<Vec<_>>())
+        itertools::process_results(block_id_results, |iter| iter.flatten().collect::<Vec<_>>())
+            .map(Locator::new)
+    }
+
+    pub fn get_locator_from_block_id(
+        &self,
+        block_id: &Id<Block>,
+    ) -> Result<Locator, PropertyQueryError> {
+        let mut last_child_block_index =
+            self.chainstate_ref.get_existing_gen_block_index(block_id.into())?;
+        let mut already_on_mainchain = false;
+        let starting_height = last_child_block_index.block_height();
+
+        let block_id_results = locator_tip_distances()
+            .map_while(|dist| starting_height - dist)
+            .map(|height| -> Result<_, PropertyQueryError> {
+                if !already_on_mainchain {
+                    let last_child_block_id_from_height = self
+                        .chainstate_ref
+                        .get_block_id_by_height(last_child_block_index.block_height())?;
+                    already_on_mainchain =
+                        last_child_block_id_from_height == Some(last_child_block_index.block_id());
+                }
+
+                let block_id = if already_on_mainchain {
+                    self.chainstate_ref
+                        .get_block_id_by_height(height)?
+                        .ok_or(PropertyQueryError::InvariantErrorLocatorDistancesWrongOrder)?
+                } else {
+                    last_child_block_index =
+                        self.chainstate_ref.get_ancestor(&last_child_block_index, height)?;
+                    last_child_block_index.block_id()
+                };
+
+                Ok(block_id)
+            });
+
+        itertools::process_results(block_id_results, |iter| iter.collect::<Vec<_>>())
             .map(Locator::new)
     }
 

chainstate/src/interface/chainstate_interface.rs

@@ -97,11 +97,14 @@ pub trait ChainstateInterface: Send + Sync {
     ///
     /// This returns a relatively short sequence even for a long chain. Such sequence can be used
     /// to quickly find a common ancestor between different chains.
-    fn get_locator(&self) -> Result<Locator, ChainstateError>;
+    fn get_locator_from_best_block(&self) -> Result<Locator, ChainstateError>;
 
-    /// Returns a locator starting from the specified height.
+    /// Returns a locator starting from the specified height on the main chain.
     fn get_locator_from_height(&self, height: BlockHeight) -> Result<Locator, ChainstateError>;
 
+    /// Returns a locator starting from the specified block (not necessarily on the main chain).
+    fn get_locator_from_block_id(&self, block_id: &Id<Block>) -> Result<Locator, ChainstateError>;
+
     /// Returns mainchain block ids with heights in the range start_height..end_height using
     /// the given step;
     fn get_block_ids_as_checkpoints(

chainstate/src/interface/chainstate_interface_impl.rs

@@ -232,11 +232,11 @@ where
     }
 
     #[tracing::instrument(skip_all)]
-    fn get_locator(&self) -> Result<Locator, ChainstateError> {
+    fn get_locator_from_best_block(&self) -> Result<Locator, ChainstateError> {
         self.chainstate
             .query()
             .map_err(ChainstateError::from)?
-            .get_locator()
+            .get_locator_from_best_block()
             .map_err(ChainstateError::FailedToReadProperty)
     }
 
@@ -249,6 +249,15 @@ where
             .map_err(ChainstateError::FailedToReadProperty)
     }
 
+    #[tracing::instrument(skip_all, fields(block_id = %block_id))]
+    fn get_locator_from_block_id(&self, block_id: &Id<Block>) -> Result<Locator, ChainstateError> {
+        self.chainstate
+            .query()
+            .map_err(ChainstateError::from)?
+            .get_locator_from_block_id(block_id)
+            .map_err(ChainstateError::FailedToReadProperty)
+    }
+
     #[tracing::instrument(skip(self))]
     fn get_block_ids_as_checkpoints(
         &self,

chainstate/src/interface/chainstate_interface_impl_delegation.rs

@@ -130,14 +130,18 @@ where
         self.deref().get_mainchain_blocks(from, max_count)
     }
 
-    fn get_locator(&self) -> Result<Locator, ChainstateError> {
-        self.deref().get_locator()
+    fn get_locator_from_best_block(&self) -> Result<Locator, ChainstateError> {
+        self.deref().get_locator_from_best_block()
     }
 
     fn get_locator_from_height(&self, height: BlockHeight) -> Result<Locator, ChainstateError> {
         self.deref().get_locator_from_height(height)
     }
 
+    fn get_locator_from_block_id(&self, block_id: &Id<Block>) -> Result<Locator, ChainstateError> {
+        self.deref().get_locator_from_block_id(block_id)
+    }
+
     fn get_block_ids_as_checkpoints(
         &self,
         start_height: BlockHeight,

chainstate/src/lib.rs

@@ -46,7 +46,7 @@ pub use crate::{
         TransactionVerifierStorageError, MEDIAN_TIME_SPAN,
     },
 };
-pub use chainstate_types::{BlockIndex, GenBlockIndex, PropertyQueryError};
+pub use chainstate_types::{BlockIndex, GenBlockIndex, GenBlockIndexRef, PropertyQueryError};
 pub use constraints_value_accumulator;
 pub use detail::tx_verification_strategy::*;
 pub use interface::{chainstate_interface, chainstate_interface_impl_delegation};

chainstate/test-suite/src/tests/syncing_tests.rs

@@ -58,15 +58,15 @@ fn process_a_trivial_block(#[case] seed: Seed) {
 #[rstest]
 #[trace]
 #[case(Seed::from_entropy())]
-fn get_locator(#[case] seed: Seed) {
+fn get_locator_from_best_block(#[case] seed: Seed) {
     utils::concurrency::model(move || {
         let mut rng = make_seedable_rng(seed);
         let mut btf = TestFramework::builder(&mut rng)
             // With the heavy checks enabled, this test takes several minutes to complete in debug builds.
             .with_chainstate_config(ChainstateConfig::new().with_heavy_checks_enabled(false))
             .build();
 
-        let locator = btf.chainstate.get_locator().unwrap();
+        let locator = btf.chainstate.get_locator_from_best_block().unwrap();
         assert_eq!(locator.len(), 1);
         assert_eq!(&locator[0], &btf.genesis().get_id());
 
@@ -79,7 +79,7 @@ fn get_locator(#[case] seed: Seed) {
             blocks += new_blocks;
 
             // Check the locator length.
-            let locator = btf.chainstate.get_locator().unwrap();
+            let locator = btf.chainstate.get_locator_from_best_block().unwrap();
             assert_eq!(
                 locator.len(),
                 blocks.next_power_of_two().trailing_zeros() as usize + 1
@@ -95,6 +95,12 @@ fn get_locator(#[case] seed: Seed) {
                     btf.chainstate.get_block_id_from_height(idx.unwrap()).unwrap().unwrap();
                 assert_eq!(&expected, header);
             }
+
+            // Check that get_locator_from_block_id returns the same locator.
+            let best_block_id = btf.to_chain_block_id(&btf.chainstate.get_best_block_id().unwrap());
+            let locator_from_block_id =
+                btf.chainstate.get_locator_from_block_id(&best_block_id).unwrap();
+            assert_eq!(locator, locator_from_block_id);
         }
     });
 }
@@ -134,6 +140,88 @@ fn get_locator_from_height(#[case] seed: Seed) {
                     btf.chainstate.get_block_id_from_height(idx.unwrap()).unwrap().unwrap();
                 assert_eq!(&expected, header);
             }
+
+            // Check that get_locator_from_block_id returns the same locator.
+            let block_id = btf.to_chain_block_id(
+                &btf.chainstate.get_block_id_from_height(height.into()).unwrap().unwrap(),
+            );
+            let locator_from_block_id =
+                btf.chainstate.get_locator_from_block_id(&block_id).unwrap();
+            assert_eq!(locator, locator_from_block_id);
+        }
+    });
+}
+
+#[rstest]
+#[trace]
+#[case(Seed::from_entropy())]
+fn get_locator_from_block_id(#[case] seed: Seed) {
+    utils::concurrency::model(move || {
+        let mut rng = make_seedable_rng(seed);
+
+        let mut tf1 = TestFramework::builder(&mut rng)
+            // Heavy checks are useless here and they increase test run time noticeably
+            .with_chainstate_config(ChainstateConfig::new().with_heavy_checks_enabled(false))
+            .with_chain_config(
+                chain::config::create_unit_test_config_builder()
+                    // Increase max_depth_for_reorg so that we can create a fork more than 1000
+                    // blocks deep.
+                    .max_depth_for_reorg(10_000.into())
+                    .build(),
+            )
+            .build();
+
+        let mainchain_len: usize = rng.gen_range(1000..2000);
+        let stalechain_len = rng.gen_range(mainchain_len / 2..mainchain_len);
+        let fork_height = rng.gen_range(0..stalechain_len);
+
+        let mainchain_block_ids = tf1
+            .create_chain_return_ids(&tf1.genesis().get_id().into(), mainchain_len, &mut rng)
+            .unwrap();
+        let stale_branch_parent_id = if fork_height == 0 {
+            tf1.genesis().get_id().into()
+        } else {
+            mainchain_block_ids[fork_height - 1]
+        };
+
+        let stale_branch_block_ids = tf1
+            .create_chain_return_ids(
+                &stale_branch_parent_id,
+                stalechain_len - fork_height,
+                &mut rng,
+            )
+            .unwrap();
+
+        let whole_stalechain = mainchain_block_ids[0..fork_height]
+            .iter()
+            .chain(&stale_branch_block_ids[..])
+            .map(|block_id| {
+                tf1.chainstate.get_block(&tf1.to_chain_block_id(block_id)).unwrap().unwrap()
+            });
+
+        let mut tf2 = TestFramework::builder(&mut rng)
+            .with_chainstate_config(ChainstateConfig::new().with_heavy_checks_enabled(false))
+            .build();
+
+        for block in whole_stalechain {
+            tf2.chainstate.process_block(block, BlockSource::Local).unwrap();
+        }
+
+        assert_eq!(tf1.best_block_id(), *mainchain_block_ids.last().unwrap());
+        assert_eq!(tf2.best_block_id(), *stale_branch_block_ids.last().unwrap());
+
+        for _ in 0..8 {
+            let stale_block_idx = rng.gen_range(0..stale_branch_block_ids.len());
+            let height = (stale_block_idx + fork_height + 1) as u64;
+
+            let stale_block_id = tf1.to_chain_block_id(&stale_branch_block_ids[stale_block_idx]);
+
+            let locator_from_id =
+                tf1.chainstate.get_locator_from_block_id(&stale_block_id).unwrap();
+            let locator_from_height_in_tf2 =
+                tf2.chainstate.get_locator_from_height(height.into()).unwrap();
+
+            assert_eq!(locator_from_id, locator_from_height_in_tf2);
         }
     });
 }
@@ -158,7 +246,7 @@ fn get_mainchain_headers_by_locator(#[case] seed: Seed) {
 
         // The locator is from this exact chain, so get_mainchain_headers_by_locator
         // should return an empty sequence.
-        let locator = tf.chainstate.get_locator().unwrap();
+        let locator = tf.chainstate.get_locator_from_best_block().unwrap();
         assert_eq!(
             tf.chainstate.get_mainchain_headers_by_locator(&locator, header_limit).unwrap(),
             vec![],
@@ -211,11 +299,11 @@ fn get_headers_genesis(#[case] seed: Seed) {
         let genesis_id: Id<GenBlock> = btf.genesis().get_id().into();
 
         btf.create_chain(&genesis_id, rng.gen_range(64..128), &mut rng).unwrap();
-        let locator_1 = btf.chainstate.get_locator().unwrap();
+        let locator_1 = btf.chainstate.get_locator_from_best_block().unwrap();
 
         let chain_length = rng.gen_range(1200..2000);
         btf.create_chain(&genesis_id, chain_length, &mut rng).unwrap();
-        let locator_2 = btf.chainstate.get_locator().unwrap();
+        let locator_2 = btf.chainstate.get_locator_from_best_block().unwrap();
         assert_ne!(locator_1, locator_2);
         assert!(locator_1.len() < locator_2.len());
 
@@ -254,7 +342,7 @@ fn get_headers_branching_chains(#[case] seed: Seed) {
             tf.create_chain(&tf.genesis().get_id().into(), common_height, &mut rng).unwrap();
 
         tf.create_chain(&common_block_id, rng.gen_range(100..250), &mut rng).unwrap();
-        let locator = tf.chainstate.get_locator().unwrap();
+        let locator = tf.chainstate.get_locator_from_best_block().unwrap();
         tf.create_chain(&common_block_id, rng.gen_range(250..500), &mut rng).unwrap();
 
         let headers = tf.chainstate.get_mainchain_headers_by_locator(&locator, 2000).unwrap();
@@ -540,15 +628,15 @@ fn get_headers_different_chains(#[case] seed: Seed) {
         tf2.create_chain(&prev_id, rng.gen_range(256..512), &mut rng).unwrap();
 
         let header_count_limit = rng.gen_range(1000..3000);
-        let locator = tf1.chainstate.get_locator().unwrap();
+        let locator = tf1.chainstate.get_locator_from_best_block().unwrap();
         let headers = tf2
             .chainstate
             .get_mainchain_headers_by_locator(&locator, header_count_limit)
             .unwrap();
         let id = *headers[0].prev_block_id();
         tf1.gen_block_index(&id); // This panics if the ID is not found
 
-        let locator = tf2.chainstate.get_locator().unwrap();
+        let locator = tf2.chainstate.get_locator_from_best_block().unwrap();
         let headers = tf1
             .chainstate
             .get_mainchain_headers_by_locator(&locator, header_count_limit)
@@ -898,7 +986,7 @@ fn get_block_ids_as_checkpoints(#[case] seed: Seed) {
         let mut rng = make_seedable_rng(seed);
         let mut btf = TestFramework::builder(&mut rng).build();
 
-        let locator = btf.chainstate.get_locator().unwrap();
+        let locator = btf.chainstate.get_locator_from_best_block().unwrap();
         assert_eq!(locator.len(), 1);
         assert_eq!(&locator[0], &btf.genesis().get_id());
 

chainstate/types/src/error.rs

@@ -26,7 +26,7 @@ pub enum PropertyQueryError {
     StorageError(#[from] crate::storage_result::Error),
     #[error("Best block index not found")]
     BestBlockIndexNotFound,
-    #[error("Block not found {0}")]
+    #[error("Block not found: {0}")]
     BlockNotFound(Id<Block>),
     #[error("Block index not found for block {0}")]
     BlockIndexNotFound(Id<GenBlock>),
@@ -39,7 +39,7 @@ pub enum PropertyQueryError {
     },
     #[error("Block for height {0} not found")]
     BlockForHeightNotFound(BlockHeight),
-    #[error("Error obtaining ancestor")]
+    #[error("Error obtaining ancestor: {0}")]
     GetAncestorError(#[from] GetAncestorError),
     #[error("Genesis block has no header")]
     GenesisHeaderRequested,
@@ -60,6 +60,8 @@ pub enum PropertyQueryError {
     },
     #[error("Token info missing for token {0:x}")]
     TokenInfoMissing(TokenId),
+    #[error("Locator distances have wrong order")]
+    InvariantErrorLocatorDistancesWrongOrder,
 }
 
 #[derive(Error, Debug, PartialEq, Eq, Clone)]

mocks/src/chainstate.rs

@@ -76,8 +76,9 @@ mockall::mock! {
             max_count: usize,
         ) -> Result<Vec<Block>, ChainstateError>;
         fn get_block_header(&self, block_id: &Id<Block>) -> Result<Option<SignedBlockHeader>, ChainstateError>;
-        fn get_locator(&self) -> Result<Locator, ChainstateError>;
+        fn get_locator_from_best_block(&self) -> Result<Locator, ChainstateError>;
         fn get_locator_from_height(&self, height: BlockHeight) -> Result<Locator, ChainstateError>;
+        fn get_locator_from_block_id(&self, block_id: &Id<Block>) -> Result<Locator, ChainstateError>;
         fn get_block_ids_as_checkpoints(
             &self,
             start_height: BlockHeight,