From 9344dee8aa6fef6de33643a96bdecc601e1ff1e7 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Thu, 9 Nov 2017 19:55:02 +0100 Subject: [PATCH] Merge #11580: Do not send (potentially) invalid headers in response to getheaders 725b79a [test] Verify node doesn't send headers that haven't been fully validated (Russell Yanofsky) 3788a84 Do not send (potentially) invalid headers in response to getheaders (Matt Corallo) Pull request description: Nowhere else in the protocol do we send headers which are for blocks we have not fully validated except in response to getheaders messages with a null locator. On my public node I have not seen any such request (whether for an invalid block or not) in at least two years of debug.log output, indicating that this should have minimal impact. Tree-SHA512: c1f6e0cdcdfb78ea577d555f9b3ceb1b4b60eff4f6cf313bfd8b576c9562d797bea73abc23f7011f249ae36dd539c715f3d20487ac03ace60e84e1b77c0c1e1a --- qa/rpc-tests/sendheaders.py | 34 ++++++++++++++++++++++++++++++++++ src/net_processing.cpp | 22 +++++++++------------- 2 files changed, 43 insertions(+), 13 deletions(-) diff --git a/qa/rpc-tests/sendheaders.py b/qa/rpc-tests/sendheaders.py index f03042de74a10..4e4977d5fb39e 100755 --- a/qa/rpc-tests/sendheaders.py +++ b/qa/rpc-tests/sendheaders.py @@ -10,6 +10,17 @@ receive inv's (omitted from testing description below, this is our control). Second node is used for creating reorgs. +test_null_locators +================== + +Sends two getheaders requests with null locator values. First request's hashstop +value refers to validated block, while second request's hashstop value refers to +a block which hasn't been validated. Verifies only the first request returns +headers. + +test_nonnull_locators +===================== + Part 1: No headers announcements before "sendheaders" a. node mines a block [expect: inv] send getdata for the block [expect: block] @@ -279,6 +290,29 @@ def run_test(self): inv_node.wait_for_verack() test_node.wait_for_verack() + self.test_null_locators(test_node) + self.test_nonnull_locators(test_node, inv_node) + + def test_null_locators(self, test_node): + tip = self.nodes[0].getblockheader(self.nodes[0].generate(1)[0]) + tip_hash = int(tip["hash"], 16) + + self.log.info("Verify getheaders with null locator and valid hashstop returns headers.") + test_node.clear_last_announcement() + test_node.get_headers(locator=[], hashstop=tip_hash) + assert_equal(test_node.check_last_announcement(headers=[tip_hash]), True) + + self.log.info("Verify getheaders with null locator and invalid hashstop does not return headers.") + block = create_block(int(tip["hash"], 16), create_coinbase(tip["height"] + 1), tip["mediantime"] + 1) + block.solve() + test_node.send_header_for_blocks([block]) + test_node.clear_last_announcement() + test_node.get_headers(locator=[], hashstop=int(block.hash, 16)) + test_node.sync_with_ping() + assert_equal(test_node.block_announced, False) + test_node.send_message(msg_block(block)) + + def test_nonnull_locators(self, test_node, inv_node): tip = int(self.nodes[0].getbestblockhash(), 16) # PART 1 diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 6cf1f1c9c1150..9395d7c47b0ce 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -771,11 +771,13 @@ bool IsBanned(NodeId pnode) // To prevent fingerprinting attacks, only send blocks/headers outside of the // active chain if they are no more than a month older (both in time, and in -// best equivalent proof of work) than the best header chain we know about. -static bool StaleBlockRequestAllowed(const CBlockIndex* pindex, const Consensus::Params& consensusParams) +// best equivalent proof of work) than the best header chain we know about and +// we fully-validated them at some point. +static bool BlockRequestAllowed(const CBlockIndex* pindex, const Consensus::Params& consensusParams) { AssertLockHeld(cs_main); - return (pindexBestHeader != nullptr) && + if (chainActive.Contains(pindex)) return true; + return pindex->IsValid(BLOCK_VALID_SCRIPTS) && (pindexBestHeader != nullptr) && (pindexBestHeader->GetBlockTime() - pindex->GetBlockTime() < STALE_RELAY_AGE_LIMIT) && (GetBlockProofEquivalentTime(*pindexBestHeader, *pindex, *pindexBestHeader, consensusParams) < STALE_RELAY_AGE_LIMIT); } @@ -1084,14 +1086,9 @@ void static ProcessGetData(CNode* pfrom, const Consensus::Params& consensusParam CValidationState dummy; ActivateBestChain(dummy, Params(), a_recent_block); } - if (chainActive.Contains(mi->second)) { - send = true; - } else { - send = mi->second->IsValid(BLOCK_VALID_SCRIPTS) && - StaleBlockRequestAllowed(mi->second, consensusParams); - if (!send) { - LogPrintf("%s: ignoring request from peer=%i for old block that isn't in the main chain\n", __func__, pfrom->GetId()); - } + send = BlockRequestAllowed(mi->second, consensusParams); + if (!send) { + LogPrintf("%s: ignoring request from peer=%i for old block that isn't in the main chain\n", __func__, pfrom->GetId()); } } // disconnect node in case we have reached the outbound limit for serving historical blocks @@ -1986,8 +1983,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr return true; pindex = (*mi).second; - if (!chainActive.Contains(pindex) && - !StaleBlockRequestAllowed(pindex, chainparams.GetConsensus())) { + if (!BlockRequestAllowed(pindex, chainparams.GetConsensus())) { LogPrintf("%s: ignoring request from peer=%i for old block header that isn't in the main chain\n", __func__, pfrom->GetId()); return true; }