From 3b4ac43bc371561b5fa559bccb17dc0702bd4b69 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 11 Oct 2017 15:38:56 -0400 Subject: [PATCH 1/6] Rewrite p2p-acceptblock in preparation for slight behavior changes Removes checking whitelisted behavior (which will be removed, the difference in behavior here makes little sense) and no longer requires that blocks at the same work as our tip be dropped if not requested (in part because we *do* request those blocks). --- test/functional/p2p-acceptblock.py | 236 +++++++++++++++-------------- 1 file changed, 122 insertions(+), 114 deletions(-) diff --git a/test/functional/p2p-acceptblock.py b/test/functional/p2p-acceptblock.py index 27ae0c27e..9b2c1a73f 100755 --- a/test/functional/p2p-acceptblock.py +++ b/test/functional/p2p-acceptblock.py @@ -4,42 +4,32 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test processing of unrequested blocks. -Since behavior differs when receiving unrequested blocks from whitelisted peers -versus non-whitelisted peers, this tests the behavior of both (effectively two -separate tests running in parallel). +Setup: two nodes, node0+node1, not connected to each other. Node1 will have +nMinimumChainWork set to 0x10, so it won't process low-work unrequested blocks. -Setup: three nodes, node0+node1+node2, not connected to each other. Node0 does not -whitelist localhost, but node1 does. They will each be on their own chain for -this test. Node2 will have nMinimumChainWork set to 0x10, so it won't process -low-work unrequested blocks. - -We have one NodeConn connection to each, test_node, white_node, and min_work_node, -respectively. +We have one NodeConn connection to node0 called test_node, and one to node1 +called min_work_node. The test: 1. Generate one block on each node, to leave IBD. 2. Mine a new block on each tip, and deliver to each node from node's peer. - The tip should advance for node0 and node1, but node2 should skip processing - due to nMinimumChainWork. + The tip should advance for node0, but node1 should skip processing due to + nMinimumChainWork. -Node2 is unused in tests 3-7: +Node1 is unused in tests 3-7: -3. Mine a block that forks the previous block, and deliver to each node from - corresponding peer. - Node0 should not process this block (just accept the header), because it is - unrequested and doesn't have more work than the tip. - Node1 should process because this is coming from a whitelisted peer. +3. Mine a block that forks from the genesis block, and deliver to test_node. + Node0 should not process this block (just accept the header), because it + is unrequested and doesn't have more or equal work to the tip. -4. Send another block that builds on the forking block. - Node0 should process this block but be stuck on the shorter chain, because - it's missing an intermediate block. - Node1 should reorg to this longer chain. +4a,b. Send another two blocks that build on the forking block. + Node0 should process the second block but be stuck on the shorter chain, + because it's missing an intermediate block. -4b.Send 288 more blocks on the longer chain. +4c.Send 288 more blocks on the longer chain (the number of blocks ahead + we currently store). Node0 should process all but the last block (too far ahead in height). - Send all headers to Node1, and then send the last block in that chain. - Node1 should accept the block because it's coming from a whitelisted peer. 5. Send a duplicate of the block in #3 to Node0. Node0 should not process the block because it is unrequested, and stay on @@ -52,7 +42,7 @@ Node2 is unused in tests 3-7: 7. Send Node0 the missing block again. Node0 should process and the tip should advance. -8. Test Node2 is able to sync when connected to node0 (which should have sufficient +8. Test Node1 is able to sync when connected to node0 (which should have sufficient work on its chain). """ @@ -71,8 +61,8 @@ class AcceptBlockTest(BitcoinTestFramework): def set_test_params(self): self.setup_clean_chain = True - self.num_nodes = 3 - self.extra_args = [[], ["-whitelist=127.0.0.1"], ["-minimumchainwork=0x10"]] + self.num_nodes = 2 + self.extra_args = [[], ["-minimumchainwork=0x10"]] def setup_network(self): # Node0 will be used to test behavior of processing unrequested blocks @@ -84,132 +74,149 @@ class AcceptBlockTest(BitcoinTestFramework): def run_test(self): # Setup the p2p connections and start up the network thread. - test_node = NodeConnCB() # connects to node0 (not whitelisted) - white_node = NodeConnCB() # connects to node1 (whitelisted) - min_work_node = NodeConnCB() # connects to node2 (not whitelisted) + test_node = NodeConnCB() # connects to node0 + min_work_node = NodeConnCB() # connects to node1 connections = [] connections.append(NodeConn('127.0.0.1', p2p_port(0), self.nodes[0], test_node)) - connections.append(NodeConn('127.0.0.1', p2p_port(1), self.nodes[1], white_node)) - connections.append(NodeConn('127.0.0.1', p2p_port(2), self.nodes[2], min_work_node)) + connections.append(NodeConn('127.0.0.1', p2p_port(1), self.nodes[1], min_work_node)) test_node.add_connection(connections[0]) - white_node.add_connection(connections[1]) - min_work_node.add_connection(connections[2]) + min_work_node.add_connection(connections[1]) NetworkThread().start() # Start up network handling in another thread # Test logic begins here test_node.wait_for_verack() - white_node.wait_for_verack() min_work_node.wait_for_verack() - # 1. Have nodes mine a block (nodes1/2 leave IBD) + # 1. Have nodes mine a block (leave IBD) [ n.generate(1) for n in self.nodes ] tips = [ int("0x" + n.getbestblockhash(), 0) for n in self.nodes ] # 2. Send one block that builds on each tip. - # This should be accepted by nodes 1/2 + # This should be accepted by node0 blocks_h2 = [] # the height 2 blocks on each node's chain block_time = int(time.time()) + 1 - for i in range(3): + for i in range(2): blocks_h2.append(create_block(tips[i], create_coinbase(2), block_time)) blocks_h2[i].solve() block_time += 1 test_node.send_message(msg_block(blocks_h2[0])) - white_node.send_message(msg_block(blocks_h2[1])) - min_work_node.send_message(msg_block(blocks_h2[2])) + min_work_node.send_message(msg_block(blocks_h2[1])) - for x in [test_node, white_node, min_work_node]: + for x in [test_node, min_work_node]: x.sync_with_ping() assert_equal(self.nodes[0].getblockcount(), 2) - assert_equal(self.nodes[1].getblockcount(), 2) - assert_equal(self.nodes[2].getblockcount(), 1) - self.log.info("First height 2 block accepted by node0/node1; correctly rejected by node2") + assert_equal(self.nodes[1].getblockcount(), 1) + self.log.info("First height 2 block accepted by node0; correctly rejected by node1") - # 3. Send another block that builds on the original tip. - blocks_h2f = [] # Blocks at height 2 that fork off the main chain - for i in range(2): - blocks_h2f.append(create_block(tips[i], create_coinbase(2), blocks_h2[i].nTime+1)) - blocks_h2f[i].solve() - test_node.send_message(msg_block(blocks_h2f[0])) - white_node.send_message(msg_block(blocks_h2f[1])) + # 3. Send another block that builds on genesis. + block_h1f = create_block(int("0x" + self.nodes[0].getblockhash(0), 0), create_coinbase(1), block_time) + block_time += 1 + block_h1f.solve() + test_node.send_message(msg_block(block_h1f)) - for x in [test_node, white_node]: - x.sync_with_ping() + test_node.sync_with_ping() + tip_entry_found = False for x in self.nodes[0].getchaintips(): - if x['hash'] == blocks_h2f[0].hash: + if x['hash'] == block_h1f.hash: assert_equal(x['status'], "headers-only") + tip_entry_found = True + assert(tip_entry_found) + assert_raises_rpc_error(-1, "Block not found on disk", self.nodes[0].getblock, block_h1f.hash) + + # 4. Send another two block that build on the fork. + block_h2f = create_block(block_h1f.sha256, create_coinbase(2), block_time) + block_time += 1 + block_h2f.solve() + test_node.send_message(msg_block(block_h2f)) - for x in self.nodes[1].getchaintips(): - if x['hash'] == blocks_h2f[1].hash: - assert_equal(x['status'], "valid-headers") + test_node.sync_with_ping() + # Since the earlier block was not processed by node, the new block + # can't be fully validated. + tip_entry_found = False + for x in self.nodes[0].getchaintips(): + if x['hash'] == block_h2f.hash: + assert_equal(x['status'], "headers-only") + tip_entry_found = True + assert(tip_entry_found) - self.log.info("Second height 2 block accepted only from whitelisted peer") + # But this block should be accepted by node since it has equal work. + # TODO: We currently drop this block but likely shouldn't + #self.nodes[0].getblock(block_h2f.hash) + self.log.info("Second height 2 block accepted, but not reorg'ed to") - # 4. Now send another block that builds on the forking chain. - blocks_h3 = [] - for i in range(2): - blocks_h3.append(create_block(blocks_h2f[i].sha256, create_coinbase(3), blocks_h2f[i].nTime+1)) - blocks_h3[i].solve() - test_node.send_message(msg_block(blocks_h3[0])) - white_node.send_message(msg_block(blocks_h3[1])) + # 4b. Now send another block that builds on the forking chain. + block_h3 = create_block(block_h2f.sha256, create_coinbase(3), block_h2f.nTime+1) + block_h3.solve() + test_node.send_message(msg_block(block_h3)) - for x in [test_node, white_node]: - x.sync_with_ping() - # Since the earlier block was not processed by node0, the new block + test_node.sync_with_ping() + # Since the earlier block was not processed by node, the new block # can't be fully validated. + tip_entry_found = False for x in self.nodes[0].getchaintips(): - if x['hash'] == blocks_h3[0].hash: + if x['hash'] == block_h3.hash: assert_equal(x['status'], "headers-only") + tip_entry_found = True + assert(tip_entry_found) + self.nodes[0].getblock(block_h3.hash) + + # But this block should be accepted by node since it has more work. + self.nodes[0].getblock(block_h3.hash) + self.log.info("Unrequested more-work block accepted") + + # 4c. Now mine 288 more blocks and deliver; all should be processed but + # the last (height-too-high) on node (as long as its not missing any headers) + tip = block_h3 + all_blocks = [] + for i in range(288): + next_block = create_block(tip.sha256, create_coinbase(i + 4), tip.nTime+1) + next_block.solve() + all_blocks.append(next_block) + tip = next_block + + # Now send the block at height 5 and check that it wasn't accepted (missing header) + test_node.send_message(msg_block(all_blocks[1])) + test_node.sync_with_ping() + assert_raises_rpc_error(-5, "Block not found", self.nodes[0].getblock, all_blocks[1].hash) + assert_raises_rpc_error(-5, "Block not found", self.nodes[0].getblockheader, all_blocks[1].hash) - # But this block should be accepted by node0 since it has more work. - self.nodes[0].getblock(blocks_h3[0].hash) - self.log.info("Unrequested more-work block accepted from non-whitelisted peer") + # The block at height 5 should be accepted if we provide the missing header, though + headers_message = msg_headers() + headers_message.headers.append(CBlockHeader(all_blocks[0])) + test_node.send_message(headers_message) + test_node.send_message(msg_block(all_blocks[1])) + test_node.sync_with_ping() + self.nodes[0].getblock(all_blocks[1].hash) - # Node1 should have accepted and reorged. - assert_equal(self.nodes[1].getblockcount(), 3) - self.log.info("Successfully reorged to length 3 chain from whitelisted peer") + # Now send the blocks in all_blocks + for i in range(288): + test_node.send_message(msg_block(all_blocks[i])) + test_node.sync_with_ping() - # 4b. Now mine 288 more blocks and deliver; all should be processed but - # the last (height-too-high) on node0. Node1 should process the tip if - # we give it the headers chain leading to the tip. - tips = blocks_h3 - headers_message = msg_headers() - all_blocks = [] # node0's blocks - for j in range(2): - for i in range(288): - next_block = create_block(tips[j].sha256, create_coinbase(i + 4), tips[j].nTime+1) - next_block.solve() - if j==0: - test_node.send_message(msg_block(next_block)) - all_blocks.append(next_block) - else: - headers_message.headers.append(CBlockHeader(next_block)) - tips[j] = next_block - - time.sleep(2) # Blocks 1-287 should be accepted, block 288 should be ignored because it's too far ahead for x in all_blocks[:-1]: self.nodes[0].getblock(x.hash) assert_raises_rpc_error(-1, "Block not found on disk", self.nodes[0].getblock, all_blocks[-1].hash) - headers_message.headers.pop() # Ensure the last block is unrequested - white_node.send_message(headers_message) # Send headers leading to tip - white_node.send_message(msg_block(tips[1])) # Now deliver the tip - white_node.sync_with_ping() - self.nodes[1].getblock(tips[1].hash) - self.log.info("Unrequested block far ahead of tip accepted from whitelisted peer") - # 5. Test handling of unrequested block on the node that didn't process # Should still not be processed (even though it has a child that has more # work). - test_node.send_message(msg_block(blocks_h2f[0])) - # Here, if the sleep is too short, the test could falsely succeed (if the - # node hasn't processed the block by the time the sleep returns, and then - # the node processes it and incorrectly advances the tip). - # But this would be caught later on, when we verify that an inv triggers - # a getdata request for this block. + # The node should have requested the blocks at some point, so + # disconnect/reconnect first + connections[0].disconnect_node() + test_node.wait_for_disconnect() + + test_node = NodeConnCB() # connects to node (not whitelisted) + connections[0] = NodeConn('127.0.0.1', p2p_port(0), self.nodes[0], test_node) + test_node.add_connection(connections[0]) + + test_node.wait_for_verack() + test_node.send_message(msg_block(block_h1f)) + test_node.send_message(msg_block(block_h2f)) # This should not be required + test_node.sync_with_ping() assert_equal(self.nodes[0].getblockcount(), 2) self.log.info("Unrequested block that would complete more-work chain was ignored") @@ -220,27 +227,28 @@ class AcceptBlockTest(BitcoinTestFramework): with mininode_lock: # Clear state so we can check the getdata request test_node.last_message.pop("getdata", None) - test_node.send_message(msg_inv([CInv(2, blocks_h3[0].sha256)])) + test_node.send_message(msg_inv([CInv(2, block_h3.sha256)])) test_node.sync_with_ping() with mininode_lock: getdata = test_node.last_message["getdata"] # Check that the getdata includes the right block - assert_equal(getdata.inv[0].hash, blocks_h2f[0].sha256) + assert_equal(getdata.inv[0].hash, block_h1f.sha256) self.log.info("Inv at tip triggered getdata for unprocessed block") # 7. Send the missing block for the third time (now it is requested) - test_node.send_message(msg_block(blocks_h2f[0])) + test_node.send_message(msg_block(block_h1f)) + test_node.send_message(msg_block(block_h2f)) # This should not be required test_node.sync_with_ping() assert_equal(self.nodes[0].getblockcount(), 290) self.log.info("Successfully reorged to longer chain from non-whitelisted peer") - # 8. Connect node2 to node0 and ensure it is able to sync - connect_nodes(self.nodes[0], 2) - sync_blocks([self.nodes[0], self.nodes[2]]) - self.log.info("Successfully synced nodes 2 and 0") + # 8. Connect node1 to node0 and ensure it is able to sync + connect_nodes(self.nodes[0], 1) + sync_blocks([self.nodes[0], self.nodes[1]]) + self.log.info("Successfully synced nodes 1 and 0") [ c.disconnect_node() for c in connections ] From 3d9c70ca0fd5d42f31114b689714af1825f0ed30 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 11 Oct 2017 16:01:51 -0400 Subject: [PATCH 2/6] Stop always storing blocks from whitelisted peers There is no reason to wish to store blocks on disk always just because a peer is whitelisted. This appears to be a historical quirk to avoid breaking things when the accept limits were added. --- src/net_processing.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 50ac76924..c093d1d57 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2480,11 +2480,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr LogPrint(BCLog::NET, "received block %s peer=%d\n", pblock->GetHash().ToString(), pfrom->GetId()); - // Process all blocks from whitelisted peers, even if not requested, - // unless we're still syncing with the network. - // Such an unrequested block may still be processed, subject to the - // conditions in AcceptBlock(). - bool forceProcessing = pfrom->fWhitelisted && !IsInitialBlockDownload(); + bool forceProcessing = false; const uint256 hash(pblock->GetHash()); { LOCK(cs_main); From 932f118e6a3779bb3d6c3cc83963cf34ac150e42 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 11 Oct 2017 16:04:13 -0400 Subject: [PATCH 3/6] Accept unrequested blocks with work equal to our tip This is a simple change that makes our accept requirements the same as our request requirements, (ever so slightly) further decoupling our consensus logic from our FindNextBlocksToDownload logic in net_processing. --- src/validation.cpp | 8 ++++---- test/functional/p2p-acceptblock.py | 5 +---- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 866e0c9fb..9ea8c31b7 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3115,7 +3115,7 @@ static bool AcceptBlock(const std::shared_ptr& pblock, CValidation // process an unrequested block if it's new and has enough work to // advance our tip, and isn't too many blocks ahead. bool fAlreadyHave = pindex->nStatus & BLOCK_HAVE_DATA; - bool fHasMoreWork = (chainActive.Tip() ? pindex->nChainWork > chainActive.Tip()->nChainWork : true); + bool fHasMoreOrSameWork = (chainActive.Tip() ? pindex->nChainWork >= chainActive.Tip()->nChainWork : true); // Blocks that are too out-of-order needlessly limit the effectiveness of // pruning, because pruning will not delete block files that contain any // blocks which are too close in height to the tip. Apply this test @@ -3132,9 +3132,9 @@ static bool AcceptBlock(const std::shared_ptr& pblock, CValidation // and unrequested blocks. if (fAlreadyHave) return true; if (!fRequested) { // If we didn't ask for it: - if (pindex->nTx != 0) return true; // This is a previously-processed block that was pruned - if (!fHasMoreWork) return true; // Don't process less-work chains - if (fTooFarAhead) return true; // Block height is too high + if (pindex->nTx != 0) return true; // This is a previously-processed block that was pruned + if (!fHasMoreOrSameWork) return true; // Don't process less-work chains + if (fTooFarAhead) return true; // Block height is too high // Protect against DoS attacks from low-work chains. // If our tip is behind, a peer could try to send us diff --git a/test/functional/p2p-acceptblock.py b/test/functional/p2p-acceptblock.py index 9b2c1a73f..36e0bd9ec 100755 --- a/test/functional/p2p-acceptblock.py +++ b/test/functional/p2p-acceptblock.py @@ -142,8 +142,7 @@ class AcceptBlockTest(BitcoinTestFramework): assert(tip_entry_found) # But this block should be accepted by node since it has equal work. - # TODO: We currently drop this block but likely shouldn't - #self.nodes[0].getblock(block_h2f.hash) + self.nodes[0].getblock(block_h2f.hash) self.log.info("Second height 2 block accepted, but not reorg'ed to") # 4b. Now send another block that builds on the forking chain. @@ -215,7 +214,6 @@ class AcceptBlockTest(BitcoinTestFramework): test_node.wait_for_verack() test_node.send_message(msg_block(block_h1f)) - test_node.send_message(msg_block(block_h2f)) # This should not be required test_node.sync_with_ping() assert_equal(self.nodes[0].getblockcount(), 2) @@ -239,7 +237,6 @@ class AcceptBlockTest(BitcoinTestFramework): # 7. Send the missing block for the third time (now it is requested) test_node.send_message(msg_block(block_h1f)) - test_node.send_message(msg_block(block_h2f)) # This should not be required test_node.sync_with_ping() assert_equal(self.nodes[0].getblockcount(), 290) From 015a5258adffb0cf394f387a95ac9c8afc34cfc3 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 19 Oct 2017 16:55:31 -0400 Subject: [PATCH 4/6] Reject headers building on invalid chains by tracking invalidity This tracks the set of all known invalid-themselves blocks (ie blocks which we attempted to connect but which were found to be invalid). This is used to cheaply check if new headers build on an invalid chain. While we're at it we also resolve an edge-case in invalidateblock on pruned nodes which results in them needing a reindex if they fail to reorg. --- src/validation.cpp | 74 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 66 insertions(+), 8 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 9ea8c31b7..60f44c595 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -156,6 +156,26 @@ namespace { /** 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; @@ -1180,6 +1200,7 @@ void static InvalidChainFound(CBlockIndex* pindexNew) void static InvalidBlockFound(CBlockIndex *pindex, const CValidationState &state) { if (!state.CorruptionPossible()) { pindex->nStatus |= BLOCK_FAILED_VALID; + g_failed_blocks.insert(pindex); setDirtyBlockIndex.insert(pindex); setBlockIndexCandidates.erase(pindex); InvalidChainFound(pindex); @@ -2533,17 +2554,18 @@ bool InvalidateBlock(CValidationState& state, const CChainParams& chainparams, C { AssertLockHeld(cs_main); - // Mark the block itself as invalid. - pindex->nStatus |= BLOCK_FAILED_VALID; - setDirtyBlockIndex.insert(pindex); - setBlockIndexCandidates.erase(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(); - pindexWalk->nStatus |= BLOCK_FAILED_CHILD; - setDirtyBlockIndex.insert(pindexWalk); - setBlockIndexCandidates.erase(pindexWalk); + pindex_was_in_chain = true; // ActivateBestChain considers blocks already in chainActive // unconditionally valid already, so force disconnect away from it. if (!DisconnectTip(state, chainparams, &disconnectpool)) { @@ -2554,6 +2576,21 @@ bool InvalidateBlock(CValidationState& state, const CChainParams& chainparams, C } } + // 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 |= BLOCK_FAILED_CHILD; + setDirtyBlockIndex.insert(invalid_walk_tip); + setBlockIndexCandidates.erase(invalid_walk_tip); + invalid_walk_tip = invalid_walk_tip->pprev; + } + + // Mark the block itself as invalid. + pindex->nStatus |= BLOCK_FAILED_VALID; + setDirtyBlockIndex.insert(pindex); + setBlockIndexCandidates.erase(pindex); + g_failed_blocks.insert(pindex); + // DisconnectTip will add transactions to disconnectpool; try to add these // back to the mempool. UpdateMempoolForReorg(disconnectpool, true); @@ -2591,6 +2628,7 @@ bool ResetBlockFailureFlags(CBlockIndex *pindex) { // Reset invalid block marker if it was pointing to one of those. pindexBestInvalid = nullptr; } + g_failed_blocks.erase(it->second); } it++; } @@ -3066,6 +3104,21 @@ static bool AcceptBlockHeader(const CBlockHeader& block, CValidationState& state return state.DoS(100, error("%s: prev block invalid", __func__), REJECT_INVALID, "bad-prevblk"); if (!ContextualCheckBlockHeader(block, state, chainparams, pindexPrev, GetAdjustedTime())) return error("%s: Consensus::ContextualCheckBlockHeader: %s, %s", __func__, hash.ToString(), FormatStateMessage(state)); + + if (!pindexPrev->IsValid(BLOCK_VALID_SCRIPTS)) { + for (const CBlockIndex* failedit : g_failed_blocks) { + if (pindexPrev->GetAncestor(failedit->nHeight) == failedit) { + assert(failedit->nStatus & BLOCK_FAILED_VALID); + CBlockIndex* invalid_walk = pindexPrev; + while (invalid_walk != failedit) { + invalid_walk->nStatus |= BLOCK_FAILED_CHILD; + setDirtyBlockIndex.insert(invalid_walk); + invalid_walk = invalid_walk->pprev; + } + return state.DoS(100, error("%s: prev block invalid", __func__), REJECT_INVALID, "bad-prevblk"); + } + } + } } if (pindex == nullptr) pindex = AddToBlockIndex(block); @@ -3492,6 +3545,10 @@ bool static LoadBlockIndexDB(const CChainParams& chainparams) pindex->nChainTx = pindex->nTx; } } + if (!(pindex->nStatus & BLOCK_FAILED_MASK) && pindex->pprev && (pindex->pprev->nStatus & BLOCK_FAILED_MASK)) { + pindex->nStatus |= BLOCK_FAILED_CHILD; + setDirtyBlockIndex.insert(pindex); + } if (pindex->IsValid(BLOCK_VALID_TRANSACTIONS) && (pindex->nChainTx || pindex->pprev == nullptr)) setBlockIndexCandidates.insert(pindex); if (pindex->nStatus & BLOCK_FAILED_MASK && (!pindexBestInvalid || pindex->nChainWork > pindexBestInvalid->nChainWork)) @@ -3882,6 +3939,7 @@ void UnloadBlockIndex() nLastBlockFile = 0; nBlockSequenceId = 1; setDirtyBlockIndex.clear(); + g_failed_blocks.clear(); setDirtyFileInfo.clear(); versionbitscache.Clear(); for (int b = 0; b < VERSIONBITS_NUM_BITS; b++) { From 00dcda60f6ee63d35bec640f27efe2338dd71270 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 11 Oct 2017 16:57:43 -0400 Subject: [PATCH 5/6] [qa] test that invalid blocks on an invalid chain get a disconnect --- test/functional/p2p-acceptblock.py | 84 ++++++++++++++++++++++++++++-- 1 file changed, 80 insertions(+), 4 deletions(-) diff --git a/test/functional/p2p-acceptblock.py b/test/functional/p2p-acceptblock.py index 36e0bd9ec..220b77636 100755 --- a/test/functional/p2p-acceptblock.py +++ b/test/functional/p2p-acceptblock.py @@ -42,16 +42,20 @@ Node1 is unused in tests 3-7: 7. Send Node0 the missing block again. Node0 should process and the tip should advance. -8. Test Node1 is able to sync when connected to node0 (which should have sufficient -work on its chain). +8. Create a fork which is invalid at a height longer than the current chain + (ie to which the node will try to reorg) but which has headers built on top + of the invalid block. Check that we get disconnected if we send more headers + on the chain the node now knows to be invalid. +9. Test Node1 is able to sync when connected to node0 (which should have sufficient + work on its chain). """ from test_framework.mininode import * from test_framework.test_framework import BitcoinTestFramework from test_framework.util import * import time -from test_framework.blocktools import create_block, create_coinbase +from test_framework.blocktools import create_block, create_coinbase, create_transaction class AcceptBlockTest(BitcoinTestFramework): def add_options(self, parser): @@ -240,9 +244,81 @@ class AcceptBlockTest(BitcoinTestFramework): test_node.sync_with_ping() assert_equal(self.nodes[0].getblockcount(), 290) + self.nodes[0].getblock(all_blocks[286].hash) + assert_equal(self.nodes[0].getbestblockhash(), all_blocks[286].hash) + assert_raises_rpc_error(-1, "Block not found on disk", self.nodes[0].getblock, all_blocks[287].hash) self.log.info("Successfully reorged to longer chain from non-whitelisted peer") - # 8. Connect node1 to node0 and ensure it is able to sync + # 8. Create a chain which is invalid at a height longer than the + # current chain, but which has more blocks on top of that + block_289f = create_block(all_blocks[284].sha256, create_coinbase(289), all_blocks[284].nTime+1) + block_289f.solve() + block_290f = create_block(block_289f.sha256, create_coinbase(290), block_289f.nTime+1) + block_290f.solve() + block_291 = create_block(block_290f.sha256, create_coinbase(291), block_290f.nTime+1) + # block_291 spends a coinbase below maturity! + block_291.vtx.append(create_transaction(block_290f.vtx[0], 0, b"42", 1)) + block_291.hashMerkleRoot = block_291.calc_merkle_root() + block_291.solve() + block_292 = create_block(block_291.sha256, create_coinbase(292), block_291.nTime+1) + block_292.solve() + + # Now send all the headers on the chain and enough blocks to trigger reorg + headers_message = msg_headers() + headers_message.headers.append(CBlockHeader(block_289f)) + headers_message.headers.append(CBlockHeader(block_290f)) + headers_message.headers.append(CBlockHeader(block_291)) + headers_message.headers.append(CBlockHeader(block_292)) + test_node.send_message(headers_message) + + test_node.sync_with_ping() + tip_entry_found = False + for x in self.nodes[0].getchaintips(): + if x['hash'] == block_292.hash: + assert_equal(x['status'], "headers-only") + tip_entry_found = True + assert(tip_entry_found) + assert_raises_rpc_error(-1, "Block not found on disk", self.nodes[0].getblock, block_292.hash) + + test_node.send_message(msg_block(block_289f)) + test_node.send_message(msg_block(block_290f)) + + test_node.sync_with_ping() + self.nodes[0].getblock(block_289f.hash) + self.nodes[0].getblock(block_290f.hash) + + test_node.send_message(msg_block(block_291)) + + # At this point we've sent an obviously-bogus block, wait for full processing + # without assuming whether we will be disconnected or not + try: + # Only wait a short while so the test doesn't take forever if we do get + # disconnected + test_node.sync_with_ping(timeout=1) + except AssertionError: + test_node.wait_for_disconnect() + + test_node = NodeConnCB() # connects to node (not whitelisted) + connections[0] = NodeConn('127.0.0.1', p2p_port(0), self.nodes[0], test_node) + test_node.add_connection(connections[0]) + + NetworkThread().start() # Start up network handling in another thread + test_node.wait_for_verack() + + # We should have failed reorg and switched back to 290 (but have block 291) + assert_equal(self.nodes[0].getblockcount(), 290) + assert_equal(self.nodes[0].getbestblockhash(), all_blocks[286].hash) + assert_equal(self.nodes[0].getblock(block_291.hash)["confirmations"], -1) + + # Now send a new header on the invalid chain, indicating we're forked off, and expect to get disconnected + block_293 = create_block(block_292.sha256, create_coinbase(293), block_292.nTime+1) + block_293.solve() + headers_message = msg_headers() + headers_message.headers.append(CBlockHeader(block_293)) + test_node.send_message(headers_message) + test_node.wait_for_disconnect() + + # 9. Connect node1 to node0 and ensure it is able to sync connect_nodes(self.nodes[0], 1) sync_blocks([self.nodes[0], self.nodes[1]]) self.log.info("Successfully synced nodes 1 and 0") From f3d4adfa6ff5db180ec09d93f78cdc8bfda26f96 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 30 Oct 2017 21:28:42 -0400 Subject: [PATCH 6/6] Make p2p-acceptablock not an extended test --- test/functional/test_runner.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 80a5ffefb..ca36426a0 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -125,6 +125,7 @@ BASE_SCRIPTS= [ 'minchainwork.py', 'p2p-fingerprint.py', 'uacomment.py', + 'p2p-acceptblock.py', ] EXTENDED_SCRIPTS = [ @@ -152,7 +153,6 @@ EXTENDED_SCRIPTS = [ 'txn_clone.py --mineblock', 'notifications.py', 'invalidateblock.py', - 'p2p-acceptblock.py', 'replace-by-fee.py', ]