Merge pull request #2773 from blockstack/fix/2771

Fix: 2771

Author: lgalabru

.github/actions/bitcoin-int-tests/Dockerfile.bitcoin-tests

@@ -21,6 +21,7 @@ RUN cargo test -- --test-threads 1 --ignored tests::neon_integrations::bitcoind_
 RUN cargo test -- --test-threads 1 --ignored tests::neon_integrations::liquid_ustx_integration
 RUN cargo test -- --test-threads 1 --ignored tests::neon_integrations::stx_transfer_btc_integration_test
 RUN cargo test -- --test-threads 1 --ignored tests::neon_integrations::bitcoind_forking_test
+RUN cargo test -- --test-threads 1 --ignored tests::neon_integrations::should_fix_2771
 RUN cargo test -- --test-threads 1 --ignored tests::neon_integrations::pox_integration_test
 RUN cargo test -- --test-threads 1 --ignored tests::bitcoin_regtest::bitcoind_integration_test
 RUN cargo test -- --test-threads 1 --ignored tests::should_succeed_handling_malformed_and_valid_txs

src/burnchains/burnchain.rs

@@ -25,7 +25,7 @@ use std::sync::{
     Arc,
 };
 use std::thread;
-use std::time::Instant;
+use std::time::{Duration, Instant};
 
 use crate::types::chainstate::StacksAddress;
 use crate::types::proof::TrieHash;
@@ -891,8 +891,8 @@ impl Burnchain {
     }
 
     /// Determine if there has been a chain reorg, given our current canonical burnchain tip.
-    /// Return the new chain tip
-    fn sync_reorg<I: BurnchainIndexer>(indexer: &mut I) -> Result<u64, burnchain_error> {
+    /// Return the new chain tip and a boolean signaling the presence of a reorg
+    fn sync_reorg<I: BurnchainIndexer>(indexer: &mut I) -> Result<(u64, bool), burnchain_error> {
         let headers_path = indexer.get_headers_path();
 
         // sanity check -- what is the height of our highest header
@@ -905,7 +905,7 @@ impl Burnchain {
         })?;
 
         if headers_height == 0 {
-            return Ok(0);
+            return Ok((0, false));
         }
 
         // did we encounter a reorg since last sync?  Find the highest common ancestor of the
@@ -921,10 +921,10 @@ impl Burnchain {
                 "Burnchain reorg detected: highest common ancestor at height {}",
                 reorg_height
             );
-            return Ok(reorg_height);
+            return Ok((reorg_height, true));
         } else {
             // no reorg
-            return Ok(headers_height);
+            return Ok((headers_height, false));
         }
     }
 
@@ -959,7 +959,7 @@ impl Burnchain {
 
         // handle reorgs
         let orig_header_height = indexer.get_headers_height()?; // 1-indexed
-        let sync_height = Burnchain::sync_reorg(indexer)?;
+        let (sync_height, _) = Burnchain::sync_reorg(indexer)?;
         if sync_height + 1 < orig_header_height {
             // a reorg happened
             warn!(
@@ -1170,9 +1170,8 @@ impl Burnchain {
         let db_height = burn_chain_tip.block_height;
 
         // handle reorgs
-        let orig_header_height = indexer.get_headers_height()?; // 1-indexed
-        let sync_height = Burnchain::sync_reorg(indexer)?;
-        if sync_height + 1 < orig_header_height {
+        let (sync_height, did_reorg) = Burnchain::sync_reorg(indexer)?;
+        if did_reorg {
             // a reorg happened
             warn!(
                 "Dropping headers higher than {} due to burnchain reorg",
@@ -1186,6 +1185,23 @@ impl Burnchain {
 
         // fetch all headers, no matter what
         let mut end_block = indexer.sync_headers(sync_height, None)?;
+        if did_reorg && sync_height > 0 {
+            // a reorg happened, and the last header fetched
+            // is on a smaller fork than the one we just
+            // invalidated. Wait for more blocks.
+            while end_block < db_height {
+                if let Some(ref should_keep_running) = should_keep_running {
+                    if !should_keep_running.load(Ordering::SeqCst) {
+                        return Err(burnchain_error::CoordinatorClosed);
+                    }
+                }
+                let end_height = target_block_height_opt.unwrap_or(0).max(db_height);
+                info!("Burnchain reorg happened at height {} invalidating chain tip {} but only {} headers presents on canonical chain. Retry in 2s", sync_height, db_height, end_block);
+                thread::sleep(Duration::from_millis(2000));
+                end_block = indexer.sync_headers(sync_height, Some(end_height))?;
+            }
+        }
+
         let mut start_block = sync_height;
         if db_height < start_block {
             start_block = db_height;
@@ -1327,7 +1343,8 @@ impl Burnchain {
                     while let Ok(Some(burnchain_block)) = db_recv.recv() {
                         debug!("Try recv next parsed block");
 
-                        if burnchain_block.block_height() == 0 {
+                        let block_height = burnchain_block.block_height();
+                        if block_height == 0 {
                             continue;
                         }
 

testnet/stacks-node/src/tests/neon_integrations.rs

@@ -1307,6 +1307,97 @@ fn bitcoind_forking_test() {
     // but we're able to keep on mining
     assert_eq!(account.nonce, 3);
 
+    // Let's create another fork, deeper
+    let burn_header_hash_to_fork = btc_regtest_controller.get_block_hash(206);
+    btc_regtest_controller.invalidate_block(&burn_header_hash_to_fork);
+    btc_regtest_controller.build_next_block(10);
+
+    thread::sleep(Duration::from_secs(5));
+    next_block_and_wait(&mut btc_regtest_controller, &blocks_processed);
+
+    let account = get_account(&http_origin, &miner_account);
+    assert_eq!(account.balance, 0);
+    assert_eq!(account.nonce, 3);
+
+    next_block_and_wait(&mut btc_regtest_controller, &blocks_processed);
+
+    let account = get_account(&http_origin, &miner_account);
+    assert_eq!(account.balance, 0);
+    // but we're able to keep on mining
+    assert_eq!(account.nonce, 3);
+
+    channel.stop_chains_coordinator();
+}
+
+#[test]
+#[ignore]
+fn should_fix_2771() {
+    if env::var("BITCOIND_TEST") != Ok("1".into()) {
+        return;
+    }
+
+    let (conf, miner_account) = neon_integration_test_conf();
+
+    let mut btcd_controller = BitcoinCoreController::new(conf.clone());
+    btcd_controller
+        .start_bitcoind()
+        .map_err(|_e| ())
+        .expect("Failed starting bitcoind");
+
+    let mut btc_regtest_controller = BitcoinRegtestController::new(conf.clone(), None);
+
+    btc_regtest_controller.bootstrap_chain(201);
+
+    eprintln!("Chain bootstrapped...");
+
+    let mut run_loop = neon::RunLoop::new(conf);
+    let blocks_processed = run_loop.get_blocks_processed_arc();
+
+    let channel = run_loop.get_coordinator_channel().unwrap();
+
+    thread::spawn(move || run_loop.start(None, 0));
+
+    // give the run loop some time to start up!
+    wait_for_runloop(&blocks_processed);
+
+    // first block wakes up the run loop
+    next_block_and_wait(&mut btc_regtest_controller, &blocks_processed);
+
+    // first block will hold our VRF registration
+    next_block_and_wait(&mut btc_regtest_controller, &blocks_processed);
+
+    let mut sort_height = channel.get_sortitions_processed();
+    eprintln!("Sort height: {}", sort_height);
+
+    while sort_height < 210 {
+        next_block_and_wait(&mut btc_regtest_controller, &blocks_processed);
+        sort_height = channel.get_sortitions_processed();
+        eprintln!("Sort height: {}", sort_height);
+    }
+
+    // okay, let's figure out the burn block we want to fork away.
+    let reorg_height = 208;
+    warn!("Will trigger re-org at block {}", reorg_height);
+    let burn_header_hash_to_fork = btc_regtest_controller.get_block_hash(reorg_height);
+    btc_regtest_controller.invalidate_block(&burn_header_hash_to_fork);
+    btc_regtest_controller.build_next_block(1);
+    thread::sleep(Duration::from_secs(5));
+
+    // The test here consists in producing a canonical chain with 210 blocks.
+    // Once done, we invalidate the block 208, and instead of rebuilding directly
+    // a longer fork with N blocks (as done in the bitcoind_forking_test)
+    // we slowly add some more blocks.
+    // Without the patch, this behavior ends up crashing the node with errors like:
+    // WARN [1626791307.078061] [src/chainstate/coordinator/mod.rs:535] [chains-coordinator] ChainsCoordinator: could not retrieve  block burnhash=40bdbf0dda349642bdf4dd30dd31af4f0c9979ce12a7c17485245d0a6ddd970b
+    // WARN [1626791307.078098] [src/chainstate/coordinator/mod.rs:308] [chains-coordinator] Error processing new burn block: NonContiguousBurnchainBlock(UnknownBlock(40bdbf0dda349642bdf4dd30dd31af4f0c9979ce12a7c17485245d0a6ddd970b))
+    // And the burnchain db ends up in the same state we ended up while investigating 2771.
+    // With this patch, the node is able to entirely register this new canonical fork, and then able to make progress and finish successfully.
+    while sort_height < 213 {
+        next_block_and_wait(&mut btc_regtest_controller, &blocks_processed);
+        sort_height = channel.get_sortitions_processed();
+        eprintln!("Sort height: {}", sort_height);
+    }
+
     channel.stop_chains_coordinator();
 }