diff --git a/qa/rpc-tests/p2p-compactblocks.py b/qa/rpc-tests/p2p-compactblocks.py index 9ecced0dd..e0b72e684 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) @@ -300,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() @@ -589,9 +592,9 @@ 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. - MAX_GETBLOCKTXN_DEPTH = 15 + # 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 while (current_height >= chain_height - MAX_GETBLOCKTXN_DEPTH): @@ -623,18 +626,24 @@ 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): # Test that requesting old compactblocks doesn't work. - MAX_CMPCTBLOCK_DEPTH = 11 + 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) @@ -648,6 +657,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) diff --git a/qa/rpc-tests/test_framework/authproxy.py b/qa/rpc-tests/test_framework/authproxy.py index 1a94bf5fe..7b051545c 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'}) diff --git a/qa/rpc-tests/test_framework/util.py b/qa/rpc-tests/test_framework/util.py index 47cebf4f6..bf3f2b2db 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 @@ -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-dump.py b/qa/rpc-tests/wallet-dump.py index a37096a40..c6dc2e3d1 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 diff --git a/qa/rpc-tests/wallet.py b/qa/rpc-tests/wallet.py index e43f6ea5d..3c0dc0f4e 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) diff --git a/src/init.cpp b/src/init.cpp index f2b13b627..eab8de8d0 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) + ". " + diff --git a/src/main.cpp b/src/main.cpp index 96734b839..191bcff4c 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) { @@ -4851,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(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 @@ -5397,8 +5401,20 @@ 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() - 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; } @@ -5727,6 +5743,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); + } + BlockTransactionsRequest req; for (size_t i = 0; i < cmpctblock.BlockTxCount(); i++) { if (!partialBlock.IsTxAvailable(i)) diff --git a/src/main.h b/src/main.h index daf884337..024f35cd3 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 diff --git a/src/scheduler.cpp b/src/scheduler.cpp index 52777b61f..27c03f154 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