From 3c39a55e64bedfc384f6a7b9de1410bc8b46a9bb Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Thu, 18 Jul 2024 12:53:57 -0400 Subject: [PATCH 1/3] validation: Add ancestors of reconsiderblock to setBlockIndexCandidates When we call reconsiderblock for some block, ResetBlockFailureFlags puts the descendants of that block into setBlockIndexCandidates (if they meet the criteria, i.e. have more work than the tip etc.) We also clear the failure flags of the ancestors, but we never put any of those into setBlockIndexCandidates this is wrong and could lead to failures in CheckBlockIndex(). --- src/validation.cpp | 13 ++----------- src/validation.h | 2 +- 2 files changed, 3 insertions(+), 12 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index d5e92600d4..495154ad61 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3849,9 +3849,9 @@ void Chainstate::ResetBlockFailureFlags(CBlockIndex *pindex) { int nHeight = pindex->nHeight; - // Remove the invalidity flag from this block and all its descendants. + // Remove the invalidity flag from this block and all its descendants and ancestors. for (auto& [_, block_index] : m_blockman.m_block_index) { - if (!block_index.IsValid() && block_index.GetAncestor(nHeight) == pindex) { + if (!block_index.IsValid() && (block_index.GetAncestor(nHeight) == pindex || pindex->GetAncestor(block_index.nHeight) == &block_index)) { block_index.nStatus &= ~BLOCK_FAILED_MASK; m_blockman.m_dirty_blockindex.insert(&block_index); if (block_index.IsValid(BLOCK_VALID_TRANSACTIONS) && block_index.HaveNumChainTxs() && setBlockIndexCandidates.value_comp()(m_chain.Tip(), &block_index)) { @@ -3863,15 +3863,6 @@ void Chainstate::ResetBlockFailureFlags(CBlockIndex *pindex) { } } } - - // Remove the invalidity flag from all ancestors too. - while (pindex != nullptr) { - if (pindex->nStatus & BLOCK_FAILED_MASK) { - pindex->nStatus &= ~BLOCK_FAILED_MASK; - m_blockman.m_dirty_blockindex.insert(pindex); - } - pindex = pindex->pprev; - } } void Chainstate::TryAddBlockIndexCandidate(CBlockIndex* pindex) diff --git a/src/validation.h b/src/validation.h index c7e70e3adb..8ba126b993 100644 --- a/src/validation.h +++ b/src/validation.h @@ -731,7 +731,7 @@ public: /** Set invalidity status to all descendants of a block */ void SetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - /** Remove invalidity status from a block and its descendants. */ + /** Remove invalidity status from a block, its descendants and ancestors and reconsider them for activation */ void ResetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); /** Replay blocks that aren't fully applied to the database. */ From 86d98b94e5469e6476dc87f54b8ac25074603f0c Mon Sep 17 00:00:00 2001 From: stratospher <44024636+stratospher@users.noreply.github.com> Date: Fri, 28 Feb 2025 23:45:53 +0530 Subject: [PATCH 2/3] test: verify that ancestors of a reconsidered block can become the chain tip when we reconsiderblock, previously only block and it's descendants were considered as chain tip candidates/inserted into setBlockIndexCandidates ex: on this chain, with block 4 invalidated 1 -> 2 -> 3 -> 4 -> 5 -> 6 -> header 7 blocks 4, 5, 6, header 7 have BLOCK_FAILED_* flags set previously: - if we reconsiderblock header 7, the chain would have all the BLOCK_FAILED_* flags cleared but would report chain tip as block 3. - after restart, it reports correct chain tip block 6. now: - if we reconsiderblock header 7, the correct chain tip block 6 is reported since ancestors are also considered as chain tip candidates/inserted into setBlockIndexCandidates. Co-authored-by: Martin Zumsande --- test/functional/rpc_invalidateblock.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/test/functional/rpc_invalidateblock.py b/test/functional/rpc_invalidateblock.py index e2eb033f09..031f1dd86c 100755 --- a/test/functional/rpc_invalidateblock.py +++ b/test/functional/rpc_invalidateblock.py @@ -85,6 +85,23 @@ class InvalidateTest(BitcoinTestFramework): self.wait_until(lambda: self.nodes[0].getblockcount() == 4, timeout=5) self.wait_until(lambda: self.nodes[1].getblockcount() == 4, timeout=5) + self.log.info("Verify that ancestors can become chain tip candidates when we reconsider blocks") + # Invalidate node0's current chain (1' -> 2' -> 3' -> 4') so that we don't reorg back to it in this test + badhash = self.nodes[0].getblockhash(1) + self.nodes[0].invalidateblock(badhash) + # Reconsider the tip so that node0's chain becomes this chain again : 1 -> 2 -> 3 -> 4 -> 5 -> 6 -> header 7 + self.nodes[0].reconsiderblock(tip) + blockhash_3 = self.nodes[0].getblockhash(3) + blockhash_4 = self.nodes[0].getblockhash(4) + blockhash_6 = self.nodes[0].getblockhash(6) + assert_equal(self.nodes[0].getbestblockhash(), blockhash_6) + + # Invalidate block 4 so that chain becomes : 1 -> 2 -> 3 + self.nodes[0].invalidateblock(blockhash_4) + assert_equal(self.nodes[0].getbestblockhash(), blockhash_3) + assert_equal(self.nodes[0].getblockchaininfo()['blocks'], 3) + assert_equal(self.nodes[0].getblockchaininfo()['headers'], 3) + self.log.info("Verify that we reconsider all ancestors as well") blocks = self.generatetodescriptor(self.nodes[1], 10, ADDRESS_BCRT1_UNSPENDABLE_DESCRIPTOR, sync_fun=self.no_op) assert_equal(self.nodes[1].getbestblockhash(), blocks[-1]) @@ -111,6 +128,14 @@ class InvalidateTest(BitcoinTestFramework): # Should report consistent blockchain info assert_equal(self.nodes[1].getblockchaininfo()["headers"], self.nodes[1].getblockchaininfo()["blocks"]) + # Reconsider the header + self.nodes[0].reconsiderblock(block.hash) + # Since header doesn't have block data, it can't be chain tip + # Check if it's possible for an ancestor (with block data) to be the chain tip + assert_equal(self.nodes[0].getbestblockhash(), blockhash_6) + assert_equal(self.nodes[0].getblockchaininfo()['blocks'], 6) + assert_equal(self.nodes[0].getblockchaininfo()['headers'], 7) + self.log.info("Verify that invalidating an unknown block throws an error") assert_raises_rpc_error(-5, "Block not found", self.nodes[1].invalidateblock, "00" * 32) assert_equal(self.nodes[1].getbestblockhash(), blocks[-1]) From 8cc3ac6c2328873e57c31f237d194c5f22b6748a Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Mon, 24 Mar 2025 14:54:55 -0400 Subject: [PATCH 3/3] validation: Don't use IsValid() to filter for invalid blocks IsValid() also returns false for blocks that have not been validated yet up to the default validity level of BLOCK_VALID_TRANSACTIONS but are not marked as invalid - e.g. if we only know the header. Here, we specifically want to filter for invalid blocks. Also removes the default arg from IsValid() which is now unused outside of tests, to prevent this kind of misuse for the future. Co-authored-by: TheCharlatan --- src/chain.h | 2 +- src/test/fuzz/chain.cpp | 2 +- src/validation.cpp | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/chain.h b/src/chain.h index c6d1640768..f5bfdb2fb4 100644 --- a/src/chain.h +++ b/src/chain.h @@ -292,7 +292,7 @@ public: std::string ToString() const; //! Check whether this block index entry is valid up to the passed validity level. - bool IsValid(enum BlockStatus nUpTo = BLOCK_VALID_TRANSACTIONS) const + bool IsValid(enum BlockStatus nUpTo) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { AssertLockHeld(::cs_main); diff --git a/src/test/fuzz/chain.cpp b/src/test/fuzz/chain.cpp index 0363f317b6..09053e4815 100644 --- a/src/test/fuzz/chain.cpp +++ b/src/test/fuzz/chain.cpp @@ -30,7 +30,7 @@ FUZZ_TARGET(chain) (void)disk_block_index->GetMedianTimePast(); (void)disk_block_index->GetUndoPos(); (void)disk_block_index->HaveNumChainTxs(); - (void)disk_block_index->IsValid(); + (void)disk_block_index->IsValid(BLOCK_VALID_TRANSACTIONS); } const CBlockHeader block_header = disk_block_index->GetBlockHeader(); diff --git a/src/validation.cpp b/src/validation.cpp index 495154ad61..72ed6e8fe9 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3851,7 +3851,7 @@ void Chainstate::ResetBlockFailureFlags(CBlockIndex *pindex) { // Remove the invalidity flag from this block and all its descendants and ancestors. for (auto& [_, block_index] : m_blockman.m_block_index) { - if (!block_index.IsValid() && (block_index.GetAncestor(nHeight) == pindex || pindex->GetAncestor(block_index.nHeight) == &block_index)) { + if ((block_index.nStatus & BLOCK_FAILED_MASK) && (block_index.GetAncestor(nHeight) == pindex || pindex->GetAncestor(block_index.nHeight) == &block_index)) { block_index.nStatus &= ~BLOCK_FAILED_MASK; m_blockman.m_dirty_blockindex.insert(&block_index); if (block_index.IsValid(BLOCK_VALID_TRANSACTIONS) && block_index.HaveNumChainTxs() && setBlockIndexCandidates.value_comp()(m_chain.Tip(), &block_index)) {