From a51e91783aac0beefcb604be159eb1cb96a39051 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Tue, 23 Apr 2024 18:08:35 -0400 Subject: [PATCH 1/6] validation: add RecalculateBestHeader() function It recalculates m_best_header by looping over the entire block index. Even though this is not very performant, it will only be used in rare situations that cannot be triggered by others without a cost: As part of to invalidateblock / reconsiderblock rpcs, or when a block with an accepted header with valid PoW turns out to be invalid later during full validation. --- src/validation.cpp | 11 +++++++++++ src/validation.h | 5 +++++ 2 files changed, 16 insertions(+) diff --git a/src/validation.cpp b/src/validation.cpp index 8e4ea8eda2f62..7430ec60d1133 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -6396,6 +6396,17 @@ std::optional ChainstateManager::GetSnapshotBaseHeight() const return base ? std::make_optional(base->nHeight) : std::nullopt; } +void ChainstateManager::RecalculateBestHeader() +{ + AssertLockHeld(cs_main); + m_best_header = ActiveChain().Tip(); + for (auto& entry : m_blockman.m_block_index) { + if (!(entry.second.nStatus & BLOCK_FAILED_MASK) && m_best_header->nChainWork < entry.second.nChainWork) { + m_best_header = &entry.second; + } + } +} + bool ChainstateManager::ValidatedSnapshotCleanup() { AssertLockHeld(::cs_main); diff --git a/src/validation.h b/src/validation.h index 059ae52bdf4ca..61a9586d7445a 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1321,6 +1321,11 @@ class ChainstateManager //! nullopt. std::optional GetSnapshotBaseHeight() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + //! If, due to invalidation / reconsideration of blocks, the previous + //! best header is no longer valid / guaranteed to be the most-work + //! header in our block-index not known to be invalid, recalculate it. + void RecalculateBestHeader() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + CCheckQueue& GetCheckQueue() { return m_script_check_queue; } ~ChainstateManager(); From 9275e9689a426964f5eaee65e356754a0548d926 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Wed, 14 Aug 2024 16:47:49 -0400 Subject: [PATCH 2/6] rpc: call RecalculateBestHeader as part of reconsiderblock Co-authored-by: Fabian Jahr --- src/rpc/blockchain.cpp | 1 + test/functional/rpc_invalidateblock.py | 2 ++ 2 files changed, 3 insertions(+) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 7ffd03e71af5a..4a7af39b15144 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1648,6 +1648,7 @@ void ReconsiderBlock(ChainstateManager& chainman, uint256 block_hash) { } chainman.ActiveChainstate().ResetBlockFailureFlags(pblockindex); + chainman.RecalculateBestHeader(); } BlockValidationState state; diff --git a/test/functional/rpc_invalidateblock.py b/test/functional/rpc_invalidateblock.py index db79d55259007..c7c7d578bbba2 100755 --- a/test/functional/rpc_invalidateblock.py +++ b/test/functional/rpc_invalidateblock.py @@ -83,6 +83,8 @@ def run_test(self): self.nodes[1].reconsiderblock(blocks[-4]) # Should be back at the tip by now assert_equal(self.nodes[1].getbestblockhash(), blocks[-1]) + # Should report consistent blockchain info + assert_equal(self.nodes[1].getblockchaininfo()["headers"], self.nodes[1].getblockchaininfo()["blocks"]) 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) From 783cb7337f72a3c7b2e74efd677a8ff0c375fe10 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Wed, 14 Aug 2024 17:28:55 -0400 Subject: [PATCH 3/6] validation: call RecalculateBestHeader in InvalidChainFound This means that it is being called in two situations: 1.) As part of the invalidateblock rpc 2.) When we receive a block for which we have a valid header in our block index, but the block turns out to be invalid --- src/validation.cpp | 2 +- test/functional/rpc_invalidateblock.py | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/validation.cpp b/src/validation.cpp index 7430ec60d1133..1ccf5656fc631 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2046,7 +2046,7 @@ void Chainstate::InvalidChainFound(CBlockIndex* pindexNew) m_chainman.m_best_invalid = pindexNew; } if (m_chainman.m_best_header != nullptr && m_chainman.m_best_header->GetAncestor(pindexNew->nHeight) == pindexNew) { - m_chainman.m_best_header = m_chain.Tip(); + m_chainman.RecalculateBestHeader(); } LogPrintf("%s: invalid block=%s height=%d log2_work=%f date=%s\n", __func__, diff --git a/test/functional/rpc_invalidateblock.py b/test/functional/rpc_invalidateblock.py index c7c7d578bbba2..f83bc901a2aac 100755 --- a/test/functional/rpc_invalidateblock.py +++ b/test/functional/rpc_invalidateblock.py @@ -41,6 +41,8 @@ def run_test(self): self.nodes[0].invalidateblock(badhash) assert_equal(self.nodes[0].getblockcount(), 4) assert_equal(self.nodes[0].getbestblockhash(), besthash_n0) + # Should report consistent blockchain info + assert_equal(self.nodes[0].getblockchaininfo()["headers"], self.nodes[0].getblockchaininfo()["blocks"]) self.log.info("Make sure we won't reorg to a lower work chain:") self.connect_nodes(1, 2) From f5149ddb9b7de3559943d7fda0f440e59413dfb5 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Tue, 23 Apr 2024 17:36:39 -0400 Subject: [PATCH 4/6] validation: mark blocks building on an invalid block as BLOCK_FAILED_CHILD Without doing so, header-only chains building on a chain that will be marked as invalid would still be eligible for m_best_header. This improves both getblockchaininfo and getchaintips behavior. While this adds an iteration over the entire block index, it can only be triggered by the user (invalidateblock) or by others at a cost (the header needs to be accepted in the first place, so it needs valid PoW). Co-authored-by: TheCharlatan --- src/validation.cpp | 12 ++++++++++++ src/validation.h | 3 +++ test/functional/rpc_invalidateblock.py | 25 ++++++++++++++++++++++++- 3 files changed, 39 insertions(+), 1 deletion(-) diff --git a/src/validation.cpp b/src/validation.cpp index 1ccf5656fc631..dcaa8dba26602 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2045,6 +2045,7 @@ void Chainstate::InvalidChainFound(CBlockIndex* pindexNew) if (!m_chainman.m_best_invalid || pindexNew->nChainWork > m_chainman.m_best_invalid->nChainWork) { m_chainman.m_best_invalid = pindexNew; } + SetBlockFailureFlags(pindexNew); if (m_chainman.m_best_header != nullptr && m_chainman.m_best_header->GetAncestor(pindexNew->nHeight) == pindexNew) { m_chainman.RecalculateBestHeader(); } @@ -3785,6 +3786,17 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde return true; } +void Chainstate::SetBlockFailureFlags(CBlockIndex* invalid_block) +{ + AssertLockHeld(cs_main); + + for (auto& [_, block_index] : m_blockman.m_block_index) { + if (block_index.GetAncestor(invalid_block->nHeight) == invalid_block && !(block_index.nStatus & BLOCK_FAILED_MASK)) { + block_index.nStatus |= BLOCK_FAILED_CHILD; + } + } +} + void Chainstate::ResetBlockFailureFlags(CBlockIndex *pindex) { AssertLockHeld(cs_main); diff --git a/src/validation.h b/src/validation.h index 61a9586d7445a..29ff6bd94cd4e 100644 --- a/src/validation.h +++ b/src/validation.h @@ -735,6 +735,9 @@ class Chainstate EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex) LOCKS_EXCLUDED(::cs_main); + /** 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. */ void ResetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); diff --git a/test/functional/rpc_invalidateblock.py b/test/functional/rpc_invalidateblock.py index f83bc901a2aac..e2eb033f09c75 100755 --- a/test/functional/rpc_invalidateblock.py +++ b/test/functional/rpc_invalidateblock.py @@ -6,6 +6,10 @@ from test_framework.test_framework import BitcoinTestFramework from test_framework.address import ADDRESS_BCRT1_UNSPENDABLE_DESCRIPTOR +from test_framework.blocktools import ( + create_block, + create_coinbase, +) from test_framework.util import ( assert_equal, assert_raises_rpc_error, @@ -35,15 +39,34 @@ def run_test(self): self.connect_nodes(0, 1) self.sync_blocks(self.nodes[0:2]) assert_equal(self.nodes[0].getblockcount(), 6) - badhash = self.nodes[1].getblockhash(2) + + # Add a header to the tip of node 0 without submitting the block. This shouldn't + # affect results since this chain will be invalidated next. + tip = self.nodes[0].getbestblockhash() + block_time = self.nodes[0].getblock(self.nodes[0].getbestblockhash())['time'] + 1 + block = create_block(int(tip, 16), create_coinbase(self.nodes[0].getblockcount()), block_time, version=4) + block.solve() + self.nodes[0].submitheader(block.serialize().hex()) + assert_equal(self.nodes[0].getblockchaininfo()["headers"], self.nodes[0].getblockchaininfo()["blocks"] + 1) self.log.info("Invalidate block 2 on node 0 and verify we reorg to node 0's original chain") + badhash = self.nodes[1].getblockhash(2) self.nodes[0].invalidateblock(badhash) assert_equal(self.nodes[0].getblockcount(), 4) assert_equal(self.nodes[0].getbestblockhash(), besthash_n0) # Should report consistent blockchain info assert_equal(self.nodes[0].getblockchaininfo()["headers"], self.nodes[0].getblockchaininfo()["blocks"]) + self.log.info("Reconsider block 6 on node 0 again and verify that the best header is set correctly") + self.nodes[0].reconsiderblock(tip) + assert_equal(self.nodes[0].getblockchaininfo()["headers"], self.nodes[0].getblockchaininfo()["blocks"] + 1) + + self.log.info("Invalidate block 2 on node 0 and verify we reorg to node 0's original chain again") + self.nodes[0].invalidateblock(badhash) + assert_equal(self.nodes[0].getblockcount(), 4) + assert_equal(self.nodes[0].getbestblockhash(), besthash_n0) + assert_equal(self.nodes[0].getblockchaininfo()["headers"], self.nodes[0].getblockchaininfo()["blocks"]) + self.log.info("Make sure we won't reorg to a lower work chain:") self.connect_nodes(1, 2) self.log.info("Sync node 2 to node 1 so both have 6 blocks") From ccd98ea4c88fc1aa959e41e0686d8dff00a44209 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Wed, 29 May 2024 16:48:16 -0400 Subject: [PATCH 5/6] test: cleanup rpc_getchaintips.py Remove whitespace that doesn't conform with pep8 and turn some comments into log messages. --- test/functional/rpc_getchaintips.py | 39 +++++++++++++++-------------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/test/functional/rpc_getchaintips.py b/test/functional/rpc_getchaintips.py index 1bd4413ce1ec2..e6d8544d22c75 100755 --- a/test/functional/rpc_getchaintips.py +++ b/test/functional/rpc_getchaintips.py @@ -18,44 +18,45 @@ def set_test_params(self): self.num_nodes = 4 def run_test(self): + self.log.info("Test getchaintips behavior with two chains of different length") tips = self.nodes[0].getchaintips() assert_equal(len(tips), 1) assert_equal(tips[0]['branchlen'], 0) assert_equal(tips[0]['height'], 200) assert_equal(tips[0]['status'], 'active') - # Split the network and build two chains of different lengths. + self.log.info("Split the network and build two chains of different lengths.") self.split_network() self.generate(self.nodes[0], 10, sync_fun=lambda: self.sync_all(self.nodes[:2])) self.generate(self.nodes[2], 20, sync_fun=lambda: self.sync_all(self.nodes[2:])) - tips = self.nodes[1].getchaintips () - assert_equal (len (tips), 1) + tips = self.nodes[1].getchaintips() + assert_equal(len(tips), 1) shortTip = tips[0] - assert_equal (shortTip['branchlen'], 0) - assert_equal (shortTip['height'], 210) - assert_equal (tips[0]['status'], 'active') + assert_equal(shortTip['branchlen'], 0) + assert_equal(shortTip['height'], 210) + assert_equal(tips[0]['status'], 'active') - tips = self.nodes[3].getchaintips () - assert_equal (len (tips), 1) + tips = self.nodes[3].getchaintips() + assert_equal(len(tips), 1) longTip = tips[0] - assert_equal (longTip['branchlen'], 0) - assert_equal (longTip['height'], 220) - assert_equal (tips[0]['status'], 'active') + assert_equal(longTip['branchlen'], 0) + assert_equal(longTip['height'], 220) + assert_equal(tips[0]['status'], 'active') - # Join the network halves and check that we now have two tips + self.log.info("Join the network halves and check that we now have two tips") # (at least at the nodes that previously had the short chain). - self.join_network () + self.join_network() - tips = self.nodes[0].getchaintips () - assert_equal (len (tips), 2) - assert_equal (tips[0], longTip) + tips = self.nodes[0].getchaintips() + assert_equal(len(tips), 2) + assert_equal(tips[0], longTip) - assert_equal (tips[1]['branchlen'], 10) - assert_equal (tips[1]['status'], 'valid-fork') + assert_equal(tips[1]['branchlen'], 10) + assert_equal(tips[1]['status'], 'valid-fork') tips[1]['branchlen'] = 0 tips[1]['status'] = 'active' - assert_equal (tips[1], shortTip) + assert_equal(tips[1], shortTip) if __name__ == '__main__': GetChainTipsTest(__file__).main() From 0bd53d913c1c2ffd2d0779f01bc51c81537b6992 Mon Sep 17 00:00:00 2001 From: Martin Zumsande Date: Fri, 16 Aug 2024 11:38:55 -0400 Subject: [PATCH 6/6] test: add test for getchaintips behavior with invalid chains This test would fail to mark the chain tip as "invalid" instead of "headers-only" without the previous commit marking the headers as BLOCK_FAILED_CHILD. --- test/functional/rpc_getchaintips.py | 32 +++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/test/functional/rpc_getchaintips.py b/test/functional/rpc_getchaintips.py index e6d8544d22c75..226e995307cef 100755 --- a/test/functional/rpc_getchaintips.py +++ b/test/functional/rpc_getchaintips.py @@ -10,6 +10,10 @@ - verify that getchaintips now returns two chain tips. """ +from test_framework.blocktools import ( + create_block, + create_coinbase, +) from test_framework.test_framework import BitcoinTestFramework from test_framework.util import assert_equal @@ -58,5 +62,33 @@ def run_test(self): tips[1]['status'] = 'active' assert_equal(tips[1], shortTip) + self.log.info("Test getchaintips behavior with invalid blocks") + self.disconnect_nodes(0, 1) + n0 = self.nodes[0] + tip = int(n0.getbestblockhash(), 16) + start_height = self.nodes[0].getblockcount() + # Create invalid block (too high coinbase) + block_time = n0.getblock(n0.getbestblockhash())['time'] + 1 + invalid_block = create_block(tip, create_coinbase(start_height+1, nValue=100), block_time) + invalid_block.solve() + + block_time += 1 + block2 = create_block(invalid_block.sha256, create_coinbase(2), block_time, version=4) + block2.solve() + + self.log.info("Submit headers-only chain") + n0.submitheader(invalid_block.serialize().hex()) + n0.submitheader(block2.serialize().hex()) + tips = n0.getchaintips() + assert_equal(len(tips), 3) + assert_equal(tips[0]['status'], 'headers-only') + + self.log.info("Submit invalid block that invalidates the headers-only chain") + n0.submitblock(invalid_block.serialize().hex()) + tips = n0.getchaintips() + assert_equal(len(tips), 3) + assert_equal(tips[0]['status'], 'invalid') + + if __name__ == '__main__': GetChainTipsTest(__file__).main()