From 3788a8479b4efd481f3e91419bcf347113375112 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 30 Oct 2017 10:41:05 -0400 Subject: [PATCH 1/2] Do not send (potentially) invalid headers in response to getheaders 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. --- src/net_processing.cpp | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 4c3aacaf5f..b26caf377f 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -755,11 +755,13 @@ void Misbehaving(NodeId pnode, int howmuch) // 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); } @@ -1038,14 +1040,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; } From 725b79a9cf9d6af3a9a7a31407f2795fe640f3c6 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Wed, 1 Nov 2017 18:57:26 -0400 Subject: [PATCH 2/2] [test] Verify node doesn't send headers that haven't been fully validated --- test/functional/sendheaders.py | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/test/functional/sendheaders.py b/test/functional/sendheaders.py index 60d107b248..159dfd3004 100755 --- a/test/functional/sendheaders.py +++ b/test/functional/sendheaders.py @@ -10,6 +10,17 @@ Setup: 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] @@ -229,6 +240,29 @@ class SendHeadersTest(BitcoinTestFramework): inv_node.sync_with_ping() test_node.sync_with_ping() + 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