From 152b7fb25fc1e5255ab038a8a3f6a3ce1c540892 Mon Sep 17 00:00:00 2001 From: James O'Beirne Date: Tue, 13 Feb 2018 22:37:36 -0500 Subject: [PATCH 1/2] [tests] Add a (failing) test for waitforblockheight Demonstrates the presence of a bug in in `validation.cpp:InvalidateBlock` which will update `rpc/blockchain.cpp:latestblock` erroneously. --- test/functional/rpc_blockchain.py | 57 +++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/test/functional/rpc_blockchain.py b/test/functional/rpc_blockchain.py index 7cf2abe6f0..7b04e4465f 100755 --- a/test/functional/rpc_blockchain.py +++ b/test/functional/rpc_blockchain.py @@ -32,6 +32,18 @@ from test_framework.util import ( assert_is_hex_string, assert_is_hash_string, ) +from test_framework.blocktools import ( + create_block, + create_coinbase, +) +from test_framework.messages import ( + msg_block, +) +from test_framework.mininode import ( + P2PInterface, + network_thread_start, +) + class BlockchainTest(BitcoinTestFramework): def set_test_params(self): @@ -46,6 +58,7 @@ class BlockchainTest(BitcoinTestFramework): self._test_getdifficulty() self._test_getnetworkhashps() self._test_stopatheight() + self._test_waitforblockheight() assert self.nodes[0].verifychain(4, 0) def _test_getblockchaininfo(self): @@ -227,6 +240,50 @@ class BlockchainTest(BitcoinTestFramework): self.start_node(0) assert_equal(self.nodes[0].getblockcount(), 207) + def _test_waitforblockheight(self): + self.log.info("Test waitforblockheight") + + node = self.nodes[0] + + # Start a P2P connection since we'll need to create some blocks. + node.add_p2p_connection(P2PInterface()) + network_thread_start() + node.p2p.wait_for_verack() + + current_height = node.getblock(node.getbestblockhash())['height'] + + # Create a fork somewhere below our current height, invalidate the tip + # of that fork, and then ensure that waitforblockheight still + # works as expected. + # + # (Previously this was broken based on setting + # `rpc/blockchain.cpp:latestblock` incorrectly.) + # + b20hash = node.getblockhash(20) + b20 = node.getblock(b20hash) + + def solve_and_send_block(prevhash, height, time): + b = create_block(prevhash, create_coinbase(height), time) + b.solve() + node.p2p.send_message(msg_block(b)) + node.p2p.sync_with_ping() + return b + + b21f = solve_and_send_block(int(b20hash, 16), 21, b20['time'] + 1) + b22f = solve_and_send_block(b21f.sha256, 22, b21f.nTime + 1) + + node.invalidateblock(b22f.hash) + + def assert_waitforheight(height, timeout=2): + assert_equal( + node.waitforblockheight(height, timeout)['height'], + current_height) + + assert_waitforheight(0) + assert_waitforheight(current_height - 1) + assert_waitforheight(current_height) + assert_waitforheight(current_height + 1) + if __name__ == '__main__': BlockchainTest().main() From f98b543522687a4ff93ad7d53fa3cc67b5c6d752 Mon Sep 17 00:00:00 2001 From: James O'Beirne Date: Tue, 13 Feb 2018 22:39:04 -0500 Subject: [PATCH 2/2] Only call NotifyBlockTip when the active chain changes Previously, if `invalidateblock` was called on a block in a branch, NotifyBlockTip would be called on that block's predecessor, creating an incorrect `rpc/blockchain.cpp:latestblock` value. Only call NotifyBlockTip if the chain being modified is activeChain. --- src/validation.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/validation.cpp b/src/validation.cpp index d9e877f2eb..6b96344d36 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2731,7 +2731,11 @@ bool CChainState::InvalidateBlock(CValidationState& state, const CChainParams& c } InvalidChainFound(pindex); - uiInterface.NotifyBlockTip(IsInitialBlockDownload(), pindex->pprev); + + // Only notify about a new block tip if the active chain was modified. + if (pindex_was_in_chain) { + uiInterface.NotifyBlockTip(IsInitialBlockDownload(), pindex->pprev); + } return true; } bool InvalidateBlock(CValidationState& state, const CChainParams& chainparams, CBlockIndex *pindex) {