diff --git a/src/validation.cpp b/src/validation.cpp --- a/src/validation.cpp +++ b/src/validation.cpp @@ -144,6 +144,27 @@ /** chainwork for the last block that preciousblock has been applied to. */ arith_uint256 nLastPreciousChainwork = 0; +/** + * In order to efficiently track invalidity of headers, we keep the set of + * blocks which we tried to connect and found to be invalid here (ie which + * were set to BLOCK_FAILED_VALID since the last restart). We can then + * walk this set and check if a new header is a descendant of something in + * this set, preventing us from having to walk mapBlockIndex when we try + * to connect a bad block and fail. + * + * While this is more complicated than marking everything which descends + * from an invalid block as invalid at the time we discover it to be + * invalid, doing so would require walking all of mapBlockIndex to find all + * descendants. Since this case should be very rare, keeping track of all + * BLOCK_FAILED_VALID blocks in a set should be just fine and work just as + * well. + * + * Because we alreardy walk mapBlockIndex in height-order at startup, we go + * ahead and mark descendants of invalid blocks as FAILED_CHILD at that time, + * instead of putting things in this set. + */ +std::set g_failed_blocks; + /** Dirty block index entries. */ std::set setDirtyBlockIndex; @@ -1085,6 +1106,7 @@ const CValidationState &state) { if (!state.CorruptionPossible()) { pindex->nStatus = pindex->nStatus.withFailed(); + g_failed_blocks.insert(pindex); setDirtyBlockIndex.insert(pindex); InvalidChainFound(pindex); } @@ -2994,21 +3016,18 @@ CBlockIndex *pindex, bool invalidate) { AssertLockHeld(cs_main); - // Mark the block as either invalid or parked. - pindex->nStatus = invalidate ? pindex->nStatus.withFailed() - : pindex->nStatus.withParked(); - setDirtyBlockIndex.insert(pindex); + // We first disconnect backwards and then mark the blocks as invalid. + // This prevents a case where pruned nodes may fail to invalidateblock + // and be left unable to start as they have no tip candidates (as there + // are no blocks that meet the "have data and are not invalid per + // nStatus" criteria for inclusion in setBlockIndexCandidates). + + bool pindex_was_in_chain = false; + CBlockIndex *invalid_walk_tip = chainActive.Tip(); DisconnectedBlockTransactions disconnectpool; while (chainActive.Contains(pindex)) { - CBlockIndex *pindexWalk = chainActive.Tip(); - if (pindexWalk != pindex) { - pindexWalk->nStatus = invalidate - ? pindexWalk->nStatus.withFailedParent() - : pindexWalk->nStatus.withParkedParent(); - setDirtyBlockIndex.insert(pindexWalk); - } - + pindex_was_in_chain = true; // ActivateBestChain considers blocks already in chainActive // unconditionally valid already, so force disconnect away from it. if (!DisconnectTip(config, state, &disconnectpool)) { @@ -3019,6 +3038,24 @@ } } + // Now mark the blocks we just disconnected as descendants invalid + // (note this may not be all descendants). + while (pindex_was_in_chain && invalid_walk_tip != pindex) { + invalid_walk_tip->nStatus = + invalidate ? invalid_walk_tip->nStatus.withFailedParent() + : invalid_walk_tip->nStatus.withParkedParent(); + setDirtyBlockIndex.insert(invalid_walk_tip); + invalid_walk_tip = invalid_walk_tip->pprev; + } + + // Mark the block as either invalid or parked. + pindex->nStatus = invalidate ? pindex->nStatus.withFailed() + : pindex->nStatus.withParked(); + setDirtyBlockIndex.insert(pindex); + if (invalidate) { + g_failed_blocks.insert(pindex); + } + // DisconnectTip will add transactions to disconnectpool; try to add these // back to the mempool. disconnectpool.updateMempoolForReorg(config, true); @@ -3081,6 +3118,9 @@ pindex->GetAncestor(pindexBase->nHeight) == pindexBase) { pindex->nStatus = newStatus; setDirtyBlockIndex.insert(pindex); + if (newStatus.isValid()) { + g_failed_blocks.erase(pindex); + } if (pindex->IsValid(BlockValidity::TRANSACTIONS) && pindex->nChainTx && setBlockIndexCandidates.value_comp()(chainActive.Tip(), pindex)) { @@ -3109,6 +3149,9 @@ if (pindex->nStatus != newStatus) { pindex->nStatus = newStatus; setDirtyBlockIndex.insert(pindex); + if (newStatus.isValid()) { + g_failed_blocks.erase(pindex); + } } pindex = pindex->pprev; } @@ -3766,6 +3809,28 @@ return error("%s: Consensus::ContextualCheckBlockHeader: %s, %s", __func__, hash.ToString(), FormatStateMessage(state)); } + + if (!pindexPrev->IsValid(BlockValidity::SCRIPTS)) { + for (const CBlockIndex *failedit : g_failed_blocks) { + if (pindexPrev->GetAncestor(failedit->nHeight) == failedit) { + assert(failedit->nStatus.hasFailed()); + CBlockIndex *invalid_walk = pindexPrev; + while (invalid_walk != failedit) { + invalid_walk->nStatus = + invalid_walk->nStatus.withFailedParent(); + setDirtyBlockIndex.insert(invalid_walk); + invalid_walk = invalid_walk->pprev; + } + return state.DoS(100, + error("%s: prev block invalid", __func__), + REJECT_INVALID, "bad-prevblk"); + } + } + } + + // Should never assert since valid blocks should never be part of + // g_failed_blocks + assert(g_failed_blocks.find(pindexPrev) == g_failed_blocks.end()); } if (pindex == nullptr) { @@ -4355,6 +4420,11 @@ } } + if (!pindex->nStatus.hasFailed() && pindex->pprev && + pindex->pprev->nStatus.hasFailed()) { + pindex->nStatus = pindex->nStatus.withFailedParent(); + setDirtyBlockIndex.insert(pindex); + } if (pindex->IsValid(BlockValidity::TRANSACTIONS) && (pindex->nChainTx || pindex->pprev == nullptr)) { setBlockIndexCandidates.insert(pindex); @@ -4841,6 +4911,7 @@ nLastBlockFile = 0; nBlockSequenceId = 1; setDirtyBlockIndex.clear(); + g_failed_blocks.clear(); setDirtyFileInfo.clear(); versionbitscache.Clear(); diff --git a/test/functional/abc-invalid-chains.py b/test/functional/abc-invalid-chains.py new file mode 100755 --- /dev/null +++ b/test/functional/abc-invalid-chains.py @@ -0,0 +1,145 @@ +#!/usr/bin/env python3 +# Copyright (c) 2019 The Bitcoin developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-ilncense.php. + +import time + +from test_framework.test_framework import BitcoinTestFramework +from test_framework.comptool import RejectResult, TestInstance, TestManager +from test_framework.mininode import network_thread_start +from test_framework.util import assert_equal +from test_framework.blocktools import ( + create_block, + create_coinbase, +) + + +class InvalidChainsTest(BitcoinTestFramework): + def set_test_params(self): + self.num_nodes = 1 + self.setup_clean_chain = True + self.tip = None + self.blocks = {} + self.block_heights = {} + self.extra_args = [["-whitelist=127.0.0.1"]] + + def next_block(self, number): + if self.tip == None: + base_block_hash = self.genesis_hash + block_time = int(time.time()) + 1 + else: + base_block_hash = self.tip.sha256 + block_time = self.tip.nTime + 1 + + height = self.block_heights[base_block_hash] + 1 + coinbase = create_coinbase(height) + coinbase.rehash() + block = create_block(base_block_hash, coinbase, block_time) + + block.solve() + self.tip = block + self.block_heights[block.sha256] = height + assert number not in self.blocks + self.blocks[number] = block + return block + + def run_test(self): + self.test = TestManager(self, self.options.tmpdir) + self.test.add_all_connections(self.nodes) + network_thread_start() + self.test.run() + + def get_tests(self): + node = self.nodes[0] + self.genesis_hash = int(node.getbestblockhash(), 16) + self.block_heights[self.genesis_hash] = 0 + + # returns a test case that asserts that the current tip was accepted + def accepted(expectedTipHash=None): + if expectedTipHash is None: + return TestInstance([[self.tip, True]]) + else: + return TestInstance([[self.tip, True, expectedTipHash]]) + + # returns a test case that asserts that the current tip was rejected + def rejected(reject=None): + if reject is None: + return TestInstance([[self.tip, False]]) + else: + return TestInstance([[self.tip, reject]]) + + # move the tip back to a previous block + def tip(number): + self.tip = self.blocks[number] + + # shorthand for functions + block = self.next_block + + # Generate some valid blocks + block(0) + yield accepted() + block(1) + yield accepted() + block(2) + yield accepted() + + # Explicitly invalidate blocks 1 and 2 + # See below for why we do this + node.invalidateblock(self.blocks[1].hash) + assert_equal(self.blocks[0].hash, node.getbestblockhash()) + node.invalidateblock(self.blocks[2].hash) + assert_equal(self.blocks[0].hash, node.getbestblockhash()) + + # Mining on top of blocks 1 or 2 is rejected + tip(1) + block(11) + yield rejected(RejectResult(16, b'bad-prevblk')) + + tip(2) + block(21) + yield rejected(RejectResult(16, b'bad-prevblk')) + + # Reconsider block 2 to remove invalid status from *both* 1 and 2 + # The goal is to test that block 1 is not retaining any internal state + # that prevents us from accepting blocks building on top of block 1 + node.reconsiderblock(self.blocks[2].hash) + assert_equal(self.blocks[2].hash, node.getbestblockhash()) + + # Mining on the block 1 chain should be accepted + # (needs to mine two blocks because less-work chains are not processed) + test = TestInstance(sync_every_block=False) + tip(1) + block(12) + test.blocks_and_transactions.append([self.tip, None]) + block(13) + test.blocks_and_transactions.append([self.tip, True]) + yield test + + # Mining on the block 2 chain should still be accepted + # (needs to mine two blocks because less-work chains are not processed) + test = TestInstance(sync_every_block=False) + tip(2) + block(22) + test.blocks_and_transactions.append([self.tip, None]) + block(23) + test.blocks_and_transactions.append([self.tip, True]) + yield test + + # Invalidating the block 2 chain should reject new blocks on that chain + node.invalidateblock(self.blocks[2].hash) + assert_equal(self.blocks[13].hash, node.getbestblockhash()) + + # Mining on the block 2 chain should be rejected + tip(23) + block(24) + yield rejected(RejectResult(16, b'bad-prevblk')) + + # Continued mining on the block 1 chain is still ok + tip(13) + block(14) + yield accepted() + + +if __name__ == '__main__': + InvalidChainsTest().main() diff --git a/test/functional/p2p_unrequested_blocks.py b/test/functional/p2p_unrequested_blocks.py --- a/test/functional/p2p_unrequested_blocks.py +++ b/test/functional/p2p_unrequested_blocks.py @@ -361,9 +361,7 @@ headers_message = msg_headers() headers_message.headers.append(CBlockHeader(block_293)) test_node.send_message(headers_message) - # FIXME: Uncomment this line once Core backport 015a525 is completed. - # Current behavior does not ban peers that give us headers on invalid chains. - # test_node.wait_for_disconnect() + test_node.wait_for_disconnect() # 9. Connect node1 to node0 and ensure it is able to sync connect_nodes(self.nodes[0], self.nodes[1])