From 3107280e14f7ca63c693937aac490794ae746a09 Mon Sep 17 00:00:00 2001 From: mrbandrews Date: Tue, 15 Nov 2016 15:37:46 -0500 Subject: [qa] add assert_raises_message to check specific error message Github-Pull: #9168 Rebased-From: 307acdd3df03082295ac0f7fe9eba7dd35973bc4 --- qa/rpc-tests/test_framework/util.py | 8 ++++++-- qa/rpc-tests/wallet.py | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/qa/rpc-tests/test_framework/util.py b/qa/rpc-tests/test_framework/util.py index 47cebf4f6e..c5f270591e 100644 --- a/qa/rpc-tests/test_framework/util.py +++ b/qa/rpc-tests/test_framework/util.py @@ -508,10 +508,14 @@ def assert_greater_than(thing1, thing2): raise AssertionError("%s <= %s"%(str(thing1),str(thing2))) def assert_raises(exc, fun, *args, **kwds): + assert_raises_message(exc, None, fun, *args, **kwds) + +def assert_raises_message(exc, message, fun, *args, **kwds): try: fun(*args, **kwds) - except exc: - pass + except exc as e: + if message is not None and message not in e.error['message']: + raise AssertionError("Expected substring not found:"+e.error['message']) except Exception as e: raise AssertionError("Unexpected exception raised: "+type(e).__name__) else: diff --git a/qa/rpc-tests/wallet.py b/qa/rpc-tests/wallet.py index e43f6ea5d2..3c0dc0f4ea 100755 --- a/qa/rpc-tests/wallet.py +++ b/qa/rpc-tests/wallet.py @@ -71,7 +71,7 @@ class WalletTest (BitcoinTestFramework): unspent_0 = self.nodes[2].listunspent()[0] unspent_0 = {"txid": unspent_0["txid"], "vout": unspent_0["vout"]} self.nodes[2].lockunspent(False, [unspent_0]) - assert_raises(JSONRPCException, self.nodes[2].sendtoaddress, self.nodes[2].getnewaddress(), 20) + assert_raises_message(JSONRPCException, "Insufficient funds", self.nodes[2].sendtoaddress, self.nodes[2].getnewaddress(), 20) assert_equal([unspent_0], self.nodes[2].listlockunspent()) self.nodes[2].lockunspent(True, [unspent_0]) assert_equal(len(self.nodes[2].listlockunspent()), 0) -- cgit v1.2.3 From 1d4c884cd325150c68b8b129e4dc18a933866509 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Wed, 2 Nov 2016 09:46:55 -0400 Subject: [qa] Increase wallet-dump RPC timeout Increase wallet-dump RPC timeout from 30 seconds to 1 minute. This avoids a timeout error that seemed to happen regularly (around 50% of builds) on a particular jenkins server during the first getnewaddress RPC call made by the test. The failing stack trace looked like: Unexpected exception caught during testing: timeout('timed out',) File ".../bitcoin/qa/rpc-tests/test_framework/test_framework.py", line 146, in main self.run_test() File ".../bitcoin/qa/rpc-tests/wallet-dump.py", line 73, in run_test addr = self.nodes[0].getnewaddress() File ".../bitcoin/qa/rpc-tests/test_framework/coverage.py", line 49, in __call__ return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs) File ".../bitcoin/qa/rpc-tests/test_framework/authproxy.py", line 145, in __call__ response = self._request('POST', self.__url.path, postdata.encode('utf-8')) File ".../bitcoin/qa/rpc-tests/test_framework/authproxy.py", line 121, in _request return self._get_response() File ".../bitcoin/qa/rpc-tests/test_framework/authproxy.py", line 160, in _get_response http_response = self.__conn.getresponse() File "/usr/lib/python3.4/http/client.py", line 1171, in getresponse response.begin() File "/usr/lib/python3.4/http/client.py", line 351, in begin version, status, reason = self._read_status() File "/usr/lib/python3.4/http/client.py", line 313, in _read_status line = str(self.fp.readline(_MAXLINE + 1), "iso-8859-1") File "/usr/lib/python3.4/socket.py", line 374, in readinto return self._sock.recv_into(b) Github-Pull: #9077 Rebased-From: 8463aaa63c5ac76421c4d2754ea9e17a31584c93 --- qa/rpc-tests/test_framework/util.py | 4 ++-- qa/rpc-tests/wallet-dump.py | 6 +++++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/qa/rpc-tests/test_framework/util.py b/qa/rpc-tests/test_framework/util.py index c5f270591e..bf3f2b2db1 100644 --- a/qa/rpc-tests/test_framework/util.py +++ b/qa/rpc-tests/test_framework/util.py @@ -327,7 +327,7 @@ def start_node(i, dirname, extra_args=None, rpchost=None, timewait=None, binary= return proxy -def start_nodes(num_nodes, dirname, extra_args=None, rpchost=None, binary=None): +def start_nodes(num_nodes, dirname, extra_args=None, rpchost=None, timewait=None, binary=None): """ Start multiple bitcoinds, return RPC connections to them """ @@ -336,7 +336,7 @@ def start_nodes(num_nodes, dirname, extra_args=None, rpchost=None, binary=None): rpcs = [] try: for i in range(num_nodes): - rpcs.append(start_node(i, dirname, extra_args[i], rpchost, binary=binary[i])) + rpcs.append(start_node(i, dirname, extra_args[i], rpchost, timewait=timewait, binary=binary[i])) except: # If one node failed to start, stop the others stop_nodes(rpcs) raise diff --git a/qa/rpc-tests/wallet-dump.py b/qa/rpc-tests/wallet-dump.py index a37096a40c..c6dc2e3d10 100755 --- a/qa/rpc-tests/wallet-dump.py +++ b/qa/rpc-tests/wallet-dump.py @@ -61,7 +61,11 @@ class WalletDumpTest(BitcoinTestFramework): self.extra_args = [["-keypool=90"]] def setup_network(self, split=False): - self.nodes = start_nodes(self.num_nodes, self.options.tmpdir, self.extra_args) + # Use 1 minute timeout because the initial getnewaddress RPC can take + # longer than the default 30 seconds due to an expensive + # CWallet::TopUpKeyPool call, and the encryptwallet RPC made later in + # the test often takes even longer. + self.nodes = start_nodes(self.num_nodes, self.options.tmpdir, self.extra_args, timewait=60) def run_test (self): tmpdir = self.options.tmpdir -- cgit v1.2.3 From da4926b1d28c600076d3eb35df60981298194697 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Wed, 2 Nov 2016 15:08:54 -0400 Subject: [qa] Add more helpful RPC timeout message Replace previous timeout('timed out',) exception with more detailed error. Github-Pull: #9077 Rebased-From: e89614b6abf28d7fe201c3db44a0df6e4db6de03 --- qa/rpc-tests/test_framework/authproxy.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/qa/rpc-tests/test_framework/authproxy.py b/qa/rpc-tests/test_framework/authproxy.py index 1a94bf5fe9..7b051545c8 100644 --- a/qa/rpc-tests/test_framework/authproxy.py +++ b/qa/rpc-tests/test_framework/authproxy.py @@ -42,6 +42,7 @@ import base64 import decimal import json import logging +import socket try: import urllib.parse as urlparse except ImportError: @@ -157,7 +158,15 @@ class AuthServiceProxy(object): return self._request('POST', self.__url.path, postdata.encode('utf-8')) def _get_response(self): - http_response = self.__conn.getresponse() + try: + http_response = self.__conn.getresponse() + except socket.timeout as e: + raise JSONRPCException({ + 'code': -344, + 'message': '%r RPC took longer than %f seconds. Consider ' + 'using larger timeout for calls that take ' + 'longer to return.' % (self._service_name, + self.__conn.timeout)}) if http_response is None: raise JSONRPCException({ 'code': -342, 'message': 'missing HTTP response from server'}) -- cgit v1.2.3 From dccdc3aa34218eecd4a37988857b5d4f3dd884ef Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Fri, 18 Nov 2016 13:08:30 +0100 Subject: test: Fix use-after-free in scheduler tests Make a copy of the boost time-point to wait for, otherwise the head of the queue may be deleted by another thread while this one is waiting, while the boost function still has a reference to it. Although this problem is in non-test code, this is not an actual problem outside of the tests because we use the thread scheduler with only one service thread, so there will never be threads fighting at the head of the queue. The old boost fallback escapes this problem because it passes a scalar value to wait_until instead of a const object reference. Found by running the tests in LLVM-4.0-master asan. Github-Pull: #9186 Rebased-From: 12519bf62b8c49b1c1744eca6ea5b3162a61f962 --- src/scheduler.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/scheduler.cpp b/src/scheduler.cpp index 52777b61f9..27c03f154c 100644 --- a/src/scheduler.cpp +++ b/src/scheduler.cpp @@ -54,9 +54,10 @@ void CScheduler::serviceQueue() #else // Some boost versions have a conflicting overload of wait_until that returns void. // Explicitly use a template here to avoid hitting that overload. - while (!shouldStop() && !taskQueue.empty() && - newTaskScheduled.wait_until<>(lock, taskQueue.begin()->first) != boost::cv_status::timeout) { - // Keep waiting until timeout + while (!shouldStop() && !taskQueue.empty()) { + boost::chrono::system_clock::time_point timeToWaitFor = taskQueue.begin()->first; + if (newTaskScheduled.wait_until<>(lock, timeToWaitFor) == boost::cv_status::timeout) + break; // Exit loop after timeout, it means we reached the time of the event } #endif // If there are multiple threads, the queue can empty while we're waiting (another -- cgit v1.2.3 From eca9b4653788570c25e646dcdfa9ba088e89f20e Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Mon, 14 Nov 2016 09:58:30 -0500 Subject: [qa] Wait for specific block announcement in p2p-compactblocks Change check_announcement_of_new_block() to wait specifically for the announcement of the newly created block, instead of waiting for any announcement at all. A difficult to reproduce failure in check_announcement_of_new_block() that happened in a travis build (https://travis-ci.org/bitcoin/bitcoin/jobs/175198367) might have happened because an older announcement was mistaken for the expected one. The error looked like: Assertion failed: Failed File ".../bitcoin/qa/rpc-tests/test_framework/test_framework.py", line 145, in main self.run_test() File ".../bitcoin/build/../qa/rpc-tests/p2p-compactblocks.py", line 787, in run_test self.test_sendcmpct(self.nodes[1], self.segwit_node, 2, old_node=self.old_node) File ".../bitcoin/build/../qa/rpc-tests/p2p-compactblocks.py", line 201, in test_sendcmpct check_announcement_of_new_block(node, test_node, lambda p: p.last_cmpctblock is None and p.last_inv is not None) File ".../bitcoin/build/../qa/rpc-tests/p2p-compactblocks.py", line 194, in check_announcement_of_new_block assert(predicate(peer)) This commit also changes the assertion failed message above to include more detailed information for debug. Github-Pull: #9159 Rebased-From: dfa44d1b07a6d1022005dba63dd6372739eee8a0 --- qa/rpc-tests/p2p-compactblocks.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/qa/rpc-tests/p2p-compactblocks.py b/qa/rpc-tests/p2p-compactblocks.py index 9ecced0dd3..c20cbede5d 100755 --- a/qa/rpc-tests/p2p-compactblocks.py +++ b/qa/rpc-tests/p2p-compactblocks.py @@ -186,12 +186,15 @@ class CompactBlocksTest(BitcoinTestFramework): def check_announcement_of_new_block(node, peer, predicate): peer.clear_block_announcement() - node.generate(1) - got_message = wait_until(lambda: peer.block_announced, timeout=30) + block_hash = int(node.generate(1)[0], 16) + peer.wait_for_block_announcement(block_hash, timeout=30) assert(peer.block_announced) assert(got_message) + with mininode_lock: - assert(predicate(peer)) + assert predicate(peer), ( + "block_hash={!r}, cmpctblock={!r}, inv={!r}".format( + block_hash, peer.last_cmpctblock, peer.last_inv)) # We shouldn't get any block announcements via cmpctblock yet. check_announcement_of_new_block(node, test_node, lambda p: p.last_cmpctblock is None) -- cgit v1.2.3 From 2ba5d784276783716bbf27b458514c4c3f44f4b6 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Mon, 24 Oct 2016 15:33:14 -0400 Subject: [qa] Fix bug in compactblocks v2 merge Bug caused the wait_for_block_announcement to be called on the wrong node, leading to nondeterminism and occasional test failures. Bug was introduced in merge commit: d075479 Merge #8882: [qa] Fix race conditions in p2p-compactblocks.py and sendheaders.py Underlying commits which conflicted were: 27acfc1 [qa] Update p2p-compactblocks.py for compactblocks v2 6976db2 [qa] Another attempt to fix race condition in p2p-compactblocks.py The first commit changed the test_compactblock_construction function signature and second commit added code which wasn't updated during the merge to use the new arguments. Suhas Daftuar noticed the bug and suggested the fix. Github-Pull: #9058 Rebased-From: 47e9659ecfbe07077a4564591095bd5510e0f917 --- qa/rpc-tests/p2p-compactblocks.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/qa/rpc-tests/p2p-compactblocks.py b/qa/rpc-tests/p2p-compactblocks.py index c20cbede5d..0abb9f9869 100755 --- a/qa/rpc-tests/p2p-compactblocks.py +++ b/qa/rpc-tests/p2p-compactblocks.py @@ -303,8 +303,8 @@ class CompactBlocksTest(BitcoinTestFramework): assert(segwit_tx_generated) # check that our test is not broken # Wait until we've seen the block announcement for the resulting tip - tip = int(self.nodes[0].getbestblockhash(), 16) - assert(self.test_node.wait_for_block_announcement(tip)) + tip = int(node.getbestblockhash(), 16) + assert(test_node.wait_for_block_announcement(tip)) # Now mine a block, and look at the resulting compact block. test_node.clear_block_announcement() -- cgit v1.2.3 From 286e548d87266f3b394d75182f04fb701b2263e8 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Wed, 26 Oct 2016 14:15:16 -0400 Subject: [qa] Fix stale data bug in test_compactblocks_not_at_tip Clear test_node.last_block before requesting blocks in the compactblocks_not_at_tip test so comparisons won't fail if a blocks were received before the test started. The bug doesn't currently cause any problems due to the order tests run, but this will change in the next commit. Github-Pull: #9058 Rebased-From: 55bfddcabbf9e8a3743f77167ba4a43aaba9f948 --- qa/rpc-tests/p2p-compactblocks.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/qa/rpc-tests/p2p-compactblocks.py b/qa/rpc-tests/p2p-compactblocks.py index 0abb9f9869..249afec2b5 100755 --- a/qa/rpc-tests/p2p-compactblocks.py +++ b/qa/rpc-tests/p2p-compactblocks.py @@ -651,6 +651,8 @@ class CompactBlocksTest(BitcoinTestFramework): node.generate(1) wait_until(test_node.received_block_announcement, timeout=30) test_node.clear_block_announcement() + with mininode_lock: + test_node.last_block = None test_node.send_message(msg_getdata([CInv(4, int(new_blocks[0], 16))])) success = wait_until(lambda: test_node.last_block is not None, timeout=30) assert(success) -- cgit v1.2.3 From 36e3b951039b98508badaafa0bbec166c22900d3 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 21 Jun 2016 13:08:29 -0700 Subject: Dont remove a "preferred" cmpctblock peer if they provide a block Github-Pull: #8637 Rebased-From: 02a337defdd854efc78ecba6d1fb19cb1c075f16 --- src/main.cpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 96734b839f..3436caaf10 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -495,9 +495,13 @@ void MaybeSetPeerAsAnnouncingHeaderAndIDs(const CNodeState* nodestate, CNode* pf return; } if (nodestate->fProvidesHeaderAndIDs) { - BOOST_FOREACH(const NodeId nodeid, lNodesAnnouncingHeaderAndIDs) - if (nodeid == pfrom->GetId()) + for (std::list::iterator it = lNodesAnnouncingHeaderAndIDs.begin(); it != lNodesAnnouncingHeaderAndIDs.end(); it++) { + if (*it == pfrom->GetId()) { + lNodesAnnouncingHeaderAndIDs.erase(it); + lNodesAnnouncingHeaderAndIDs.push_back(pfrom->GetId()); return; + } + } bool fAnnounceUsingCMPCTBLOCK = false; uint64_t nCMPCTBLOCKVersion = (nLocalServices & NODE_WITNESS) ? 2 : 1; if (lNodesAnnouncingHeaderAndIDs.size() >= 3) { @@ -5727,6 +5731,12 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, return true; } + if (!fAlreadyInFlight && mapBlocksInFlight.size() == 1 && pindex->pprev->IsValid(BLOCK_VALID_CHAIN)) { + // We seem to be rather well-synced, so it appears pfrom was the first to provide us + // with this block! Let's get them to announce using compact blocks in the future. + MaybeSetPeerAsAnnouncingHeaderAndIDs(nodestate, pfrom, connman); + } + BlockTransactionsRequest req; for (size_t i = 0; i < cmpctblock.BlockTxCount(); i++) { if (!partialBlock.IsTxAvailable(i)) -- cgit v1.2.3 From 76ba1c973948a33bbf87d13c4bd2f2b81fb466a2 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 21 Jun 2016 16:28:38 -0700 Subject: More agressively filter compact block requests Unit test adaptations by Pieter Wuille. Github-Pull: #8637 Rebased-From: fe998e962dc015978f104b782afb7daec3c4d4df --- qa/rpc-tests/p2p-compactblocks.py | 4 ++-- src/main.cpp | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/qa/rpc-tests/p2p-compactblocks.py b/qa/rpc-tests/p2p-compactblocks.py index 249afec2b5..4029669780 100755 --- a/qa/rpc-tests/p2p-compactblocks.py +++ b/qa/rpc-tests/p2p-compactblocks.py @@ -594,7 +594,7 @@ class CompactBlocksTest(BitcoinTestFramework): def test_getblocktxn_handler(self, node, test_node, version): # bitcoind won't respond for blocks whose height is more than 15 blocks # deep. - MAX_GETBLOCKTXN_DEPTH = 15 + MAX_GETBLOCKTXN_DEPTH = 10 chain_height = node.getblockcount() current_height = chain_height while (current_height >= chain_height - MAX_GETBLOCKTXN_DEPTH): @@ -635,7 +635,7 @@ class CompactBlocksTest(BitcoinTestFramework): def test_compactblocks_not_at_tip(self, node, test_node): # Test that requesting old compactblocks doesn't work. - MAX_CMPCTBLOCK_DEPTH = 11 + MAX_CMPCTBLOCK_DEPTH = 6 new_blocks = [] for i in range(MAX_CMPCTBLOCK_DEPTH): test_node.clear_block_announcement() diff --git a/src/main.cpp b/src/main.cpp index 3436caaf10..2e09b6cf75 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -4855,7 +4855,7 @@ void static ProcessGetData(CNode* pfrom, const Consensus::Params& consensusParam // and we don't feel like constructing the object for them, so // instead we respond with the full, non-compact block. bool fPeerWantsWitness = State(pfrom->GetId())->fWantsCmpctWitness; - if (mi->second->nHeight >= chainActive.Height() - 10) { + if (CanDirectFetch(Params().GetConsensus()) && mi->second->nHeight >= chainActive.Height() - 5) { CBlockHeaderAndShortTxIDs cmpctblock(block, fPeerWantsWitness); pfrom->PushMessageWithFlag(fPeerWantsWitness ? 0 : SERIALIZE_TRANSACTION_NO_WITNESS, NetMsgType::CMPCTBLOCK, cmpctblock); } else @@ -5401,8 +5401,8 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, return true; } - if (it->second->nHeight < chainActive.Height() - 15) { - LogPrint("net", "Peer %d sent us a getblocktxn for a block > 15 deep", pfrom->id); + if (it->second->nHeight < chainActive.Height() - 10) { + LogPrint("net", "Peer %d sent us a getblocktxn for a block > 10 deep", pfrom->id); return true; } -- cgit v1.2.3 From 3d23a0eaa3cfa349833e42345daf954e9530bdfd Mon Sep 17 00:00:00 2001 From: instagibbs Date: Wed, 22 Jun 2016 08:18:22 -0400 Subject: Add cmpctblock to debug help list Github-Pull: #8637 Rebased-From: b2e93a343ec2dc7d255b970e6ee45e9c390f7ed0 --- src/init.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/init.cpp b/src/init.cpp index f2b13b627a..eab8de8d02 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -412,7 +412,7 @@ std::string HelpMessage(HelpMessageMode mode) strUsage += HelpMessageOpt("-limitdescendantsize=", strprintf("Do not accept transactions if any ancestor would have more than kilobytes of in-mempool descendants (default: %u).", DEFAULT_DESCENDANT_SIZE_LIMIT)); strUsage += HelpMessageOpt("-bip9params=deployment:start:end", "Use given start/end times for specified bip9 deployment (regtest-only)"); } - string debugCategories = "addrman, alert, bench, coindb, db, http, libevent, lock, mempool, mempoolrej, net, proxy, prune, rand, reindex, rpc, selectcoins, tor, zmq"; // Don't translate these and qt below + string debugCategories = "addrman, alert, bench, cmpctblock, coindb, db, http, libevent, lock, mempool, mempoolrej, net, proxy, prune, rand, reindex, rpc, selectcoins, tor, zmq"; // Don't translate these and qt below if (mode == HMM_BITCOIN_QT) debugCategories += ", qt"; strUsage += HelpMessageOpt("-debug=", strprintf(_("Output debugging information (default: %u, supplying is optional)"), 0) + ". " + -- cgit v1.2.3 From 2cad5db6f752ad8fa2d047b67a137de76eb9c982 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Wed, 31 Aug 2016 17:35:59 +0200 Subject: Align constant names for maximum compact block / blocktxn depth Github-Pull: #8637 Rebased-From: 3ac6de0a045cc9b2047ceb19af970e7ffbf905fa --- qa/rpc-tests/p2p-compactblocks.py | 4 ++-- src/main.cpp | 6 +++--- src/main.h | 5 +++++ 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/qa/rpc-tests/p2p-compactblocks.py b/qa/rpc-tests/p2p-compactblocks.py index 4029669780..0c73032c9d 100755 --- a/qa/rpc-tests/p2p-compactblocks.py +++ b/qa/rpc-tests/p2p-compactblocks.py @@ -635,9 +635,9 @@ class CompactBlocksTest(BitcoinTestFramework): def test_compactblocks_not_at_tip(self, node, test_node): # Test that requesting old compactblocks doesn't work. - MAX_CMPCTBLOCK_DEPTH = 6 + MAX_CMPCTBLOCK_DEPTH = 5 new_blocks = [] - for i in range(MAX_CMPCTBLOCK_DEPTH): + for i in range(MAX_CMPCTBLOCK_DEPTH + 1): test_node.clear_block_announcement() new_blocks.append(node.generate(1)[0]) wait_until(test_node.received_block_announcement, timeout=30) diff --git a/src/main.cpp b/src/main.cpp index 2e09b6cf75..db020a4495 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -4855,7 +4855,7 @@ void static ProcessGetData(CNode* pfrom, const Consensus::Params& consensusParam // and we don't feel like constructing the object for them, so // instead we respond with the full, non-compact block. bool fPeerWantsWitness = State(pfrom->GetId())->fWantsCmpctWitness; - if (CanDirectFetch(Params().GetConsensus()) && mi->second->nHeight >= chainActive.Height() - 5) { + if (CanDirectFetch(consensusParams) && mi->second->nHeight >= chainActive.Height() - MAX_CMPCTBLOCK_DEPTH) { CBlockHeaderAndShortTxIDs cmpctblock(block, fPeerWantsWitness); pfrom->PushMessageWithFlag(fPeerWantsWitness ? 0 : SERIALIZE_TRANSACTION_NO_WITNESS, NetMsgType::CMPCTBLOCK, cmpctblock); } else @@ -5401,8 +5401,8 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, return true; } - if (it->second->nHeight < chainActive.Height() - 10) { - LogPrint("net", "Peer %d sent us a getblocktxn for a block > 10 deep", pfrom->id); + if (it->second->nHeight < chainActive.Height() - MAX_BLOCKTXN_DEPTH) { + LogPrint("net", "Peer %d sent us a getblocktxn for a block > %i deep", pfrom->id, MAX_BLOCKTXN_DEPTH); return true; } diff --git a/src/main.h b/src/main.h index daf884337d..024f35cd39 100644 --- a/src/main.h +++ b/src/main.h @@ -89,6 +89,11 @@ static const unsigned int BLOCK_STALLING_TIMEOUT = 2; /** Number of headers sent in one getheaders result. We rely on the assumption that if a peer sends * less than this number, we reached its tip. Changing this value is a protocol upgrade. */ static const unsigned int MAX_HEADERS_RESULTS = 2000; +/** Maximum depth of blocks we're willing to serve as compact blocks to peers + * when requested. For older blocks, a regular BLOCK response will be sent. */ +static const int MAX_CMPCTBLOCK_DEPTH = 5; +/** Maximum depth of blocks we're willing to respond to GETBLOCKTXN requests for. */ +static const int MAX_BLOCKTXN_DEPTH = 10; /** Size of the "block download window": how far ahead of our current height do we fetch? * Larger windows tolerate larger download speed differences between peer, but increase the potential * degree of disordering of blocks on disk (which make reindexing and in the future perhaps pruning -- cgit v1.2.3 From e8461666ecd7f0f89fba92a377c0f8beffa233e9 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Wed, 26 Oct 2016 14:53:18 -0400 Subject: Modify getblocktxn handler not to drop requests for old blocks The current getblocktxn implementation drops and ignores requests for old blocks, which causes occasional sync_block timeouts during the p2p-compactblocks.py test as reported in https://github.com/bitcoin/bitcoin/issues/8842. The p2p-compactblocks.py test setup creates many new blocks in a short period of time, which can lead to getblocktxn requests for blocks below the hardcoded depth limit of 10 blocks. This commit changes the getblocktxn handler not to ignore these requests, so the peer nodes in the test setup will reliably be able to sync. The protocol change is documented in BIP-152 update "Allow block responses to getblocktxn requests" at https://github.com/bitcoin/bips/pull/469. The protocol change is not expected to affect nodes running outside the test environment, because there shouldn't normally be lots of new blocks being rapidly added that need to be synced. Github-Pull: #9058 Rebased-From: dac53b58b555183ccc0d5e64c428528267cd98b3 Github-Pull: #9160 Rebased-From: ec34648766c4052816e4072cc61ad429430bcfd9 --- qa/rpc-tests/p2p-compactblocks.py | 12 +++++++++--- src/main.cpp | 14 +++++++++++++- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/qa/rpc-tests/p2p-compactblocks.py b/qa/rpc-tests/p2p-compactblocks.py index 0c73032c9d..e0b72e6840 100755 --- a/qa/rpc-tests/p2p-compactblocks.py +++ b/qa/rpc-tests/p2p-compactblocks.py @@ -592,8 +592,8 @@ class CompactBlocksTest(BitcoinTestFramework): assert_equal(int(node.getbestblockhash(), 16), block.sha256) def test_getblocktxn_handler(self, node, test_node, version): - # bitcoind won't respond for blocks whose height is more than 15 blocks - # deep. + # bitcoind will not send blocktxn responses for blocks whose height is + # more than 10 blocks deep. MAX_GETBLOCKTXN_DEPTH = 10 chain_height = node.getblockcount() current_height = chain_height @@ -626,11 +626,17 @@ class CompactBlocksTest(BitcoinTestFramework): test_node.last_blocktxn = None current_height -= 1 - # Next request should be ignored, as we're past the allowed depth. + # Next request should send a full block response, as we're past the + # allowed depth for a blocktxn response. block_hash = node.getblockhash(current_height) msg.block_txn_request = BlockTransactionsRequest(int(block_hash, 16), [0]) + with mininode_lock: + test_node.last_block = None + test_node.last_blocktxn = None test_node.send_and_ping(msg) with mininode_lock: + test_node.last_block.block.calc_sha256() + assert_equal(test_node.last_block.block.sha256, int(block_hash, 16)) assert_equal(test_node.last_blocktxn, None) def test_compactblocks_not_at_tip(self, node, test_node): diff --git a/src/main.cpp b/src/main.cpp index db020a4495..191bcff4cc 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -5402,7 +5402,19 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, } if (it->second->nHeight < chainActive.Height() - MAX_BLOCKTXN_DEPTH) { + // If an older block is requested (should never happen in practice, + // but can happen in tests) send a block response instead of a + // blocktxn response. Sending a full block response instead of a + // small blocktxn response is preferable in the case where a peer + // might maliciously send lots of getblocktxn requests to trigger + // expensive disk reads, because it will require the peer to + // actually receive all the data read from disk over the network. LogPrint("net", "Peer %d sent us a getblocktxn for a block > %i deep", pfrom->id, MAX_BLOCKTXN_DEPTH); + CInv inv; + inv.type = State(pfrom->GetId())->fWantsCmpctWitness ? MSG_WITNESS_BLOCK : MSG_BLOCK; + inv.hash = req.blockhash; + pfrom->vRecvGetData.push_back(inv); + ProcessGetData(pfrom, chainparams.GetConsensus()); return true; } @@ -5734,7 +5746,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, if (!fAlreadyInFlight && mapBlocksInFlight.size() == 1 && pindex->pprev->IsValid(BLOCK_VALID_CHAIN)) { // We seem to be rather well-synced, so it appears pfrom was the first to provide us // with this block! Let's get them to announce using compact blocks in the future. - MaybeSetPeerAsAnnouncingHeaderAndIDs(nodestate, pfrom, connman); + MaybeSetPeerAsAnnouncingHeaderAndIDs(nodestate, pfrom); } BlockTransactionsRequest req; -- cgit v1.2.3