mirrors/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2024-11-19 01:42:58 +01:00
Code Issues Projects Releases Wiki Activity

Merge bitcoin/bitcoin#30666: validation: fix m_best_header tracking and BLOCK_FAILED_CHILD assignment

Browse Source 
0bd53d913c test: add test for getchaintips behavior with invalid chains (Martin Zumsande)
ccd98ea4c8 test: cleanup rpc_getchaintips.py (Martin Zumsande)
f5149ddb9b validation: mark blocks building on an invalid block as BLOCK_FAILED_CHILD (Martin Zumsande)
783cb7337f validation: call RecalculateBestHeader in InvalidChainFound (Martin Zumsande)
9275e9689a rpc: call RecalculateBestHeader as part of reconsiderblock (Martin Zumsande)
a51e91783a validation: add RecalculateBestHeader() function (Martin Zumsande)

Pull request description:

  `m_best_header` (the most-work header not known to be on an invalid chain) can be wrong in the context of invalidation / reconsideration of blocks. This can happen naturally (a valid header is received and stored in our block tree db; when the full block arrives, it is found to be invalid) or triggered by the user with the `invalidateblock` / `reconsiderblock` rpc.

  We don't currently use `m_best_header` for any critical things (see OP of #16974 for a list that still seems up-to-date), so it being wrong affects mostly rpcs.

  This PR proposes to recalculate it if necessary by looping over the block index and finding the best header. It also suggest to mark headers between an invalidatetd block and the previous `m_best_header` as invalid, so they won't be considered in the recalculation.
  It adds tests to `rpc_invalidateblock.py` and `rpc_getchaintips.py` that fail on master.

  One alternative to this suggested in the past would be to introduce a continuous tracking of header tips (#12138).
  While this might be more performant, it is also more complicated, and situations where we need this data are only be remotely triggerable by paying the cost of creating a valid PoW header for an invalid block.
  Therefore I think it isn't necessary to optimise for performance here, plus the solution in this PR doesn't perform any extra steps in the normal node operation where no invalidated blocks are encountered.

  Fixes  #26245

ACKs for top commit:
  fjahr:
    reACK 0bd53d913c
  achow101:
    ACK 0bd53d913c
  TheCharlatan:
    Re-ACK 0bd53d913c

Tree-SHA512: 23c2fc42d7c7bb4f9b4ba4949646b3d0031dd29ed15484e436afd66cd821ed48e0f16a1d02f45477b5d0d73a006f6e81a56b82d9721e0dee2e924219f528b445
This commit is contained in:
Ava Chow 2024-11-14 16:54:41 -05:00
commit 85bcfeea23
No known key found for this signature in database
GPG Key ID: 17565732E08E5E41
5 changed files with 113 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
src/rpc/blockchain.cpp
View File

@ -1647,6 +1647,7 @@ void ReconsiderBlock(ChainstateManager& chainman, uint256 block_hash) {
}
chainman.ActiveChainstate().ResetBlockFailureFlags(pblockindex);
chainman.RecalculateBestHeader();
}
BlockValidationState state;

25
src/validation.cpp
View File

@ -2056,8 +2056,9 @@ 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.m_best_header = m_chain.Tip();
m_chainman.RecalculateBestHeader();
}
LogPrintf("%s: invalid block=%s height=%d log2_work=%f date=%s\n", __func__,
@ -3793,6 +3794,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);
@ -6404,6 +6416,17 @@ std::optional<int> 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);

8
src/validation.h
View File

@ -729,6 +729,9 @@ public:
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);
@ -1314,6 +1317,11 @@ public:
//! nullopt.
std::optional<int> 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<CScriptCheck>& GetCheckQueue() { return m_script_check_queue; }
~ChainstateManager();

71
test/functional/rpc_getchaintips.py
View File

@ -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
@ -18,44 +22,73 @@ class GetChainTipsTest (BitcoinTestFramework):
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)
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()

29
test/functional/rpc_invalidateblock.py
View File

@ -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,12 +39,33 @@ class InvalidateTest(BitcoinTestFramework):
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)
@ -83,6 +108,8 @@ class InvalidateTest(BitcoinTestFramework):
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)