From f585450062cb450cd24d79311ffb36abac22c8a8 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Thu, 8 Feb 2018 05:38:24 +0300 Subject: [PATCH 1/2] Drop custom logic for delaying GETHEADERS Reverts "Fix duplicate headers download in initial sync (#1589)" and all following fixes This reverts commit 169afafd5078b0540a74bbdf95e8359f60768b04. --- src/chainparams.cpp | 4 --- src/chainparams.h | 2 -- src/net.h | 9 ------ src/net_processing.cpp | 69 ++++++------------------------------------ 4 files changed, 9 insertions(+), 75 deletions(-) diff --git a/src/chainparams.cpp b/src/chainparams.cpp index ca7ca4e03de2..6a790fae98a8 100644 --- a/src/chainparams.cpp +++ b/src/chainparams.cpp @@ -191,7 +191,6 @@ class CMainParams : public CChainParams { pchMessageStart[3] = 0xbd; vAlertPubKey = ParseHex("048240a8748a80a286b270ba126705ced4f2ce5a7847b3610ea3c06513150dade2a8512ed5ea86320824683fc0818f0ac019214973e677acd1244f6d0571fc5103"); nDefaultPort = 9999; - nDelayGetHeadersTime = 24 * 60 * 60; nPruneAfterHeight = 100000; genesis = CreateGenesisBlock(1390095618, 28917698, 0x1e0ffff0, 1, 50 * COIN); @@ -339,7 +338,6 @@ class CTestNetParams : public CChainParams { pchMessageStart[3] = 0xff; vAlertPubKey = ParseHex("04517d8a699cb43d3938d7b24faaff7cda448ca4ea267723ba614784de661949bf632d6304316b244646dea079735b9a6fc4af804efb4752075b9fe2245e14e412"); nDefaultPort = 19999; - nDelayGetHeadersTime = 24 * 60 * 60; nPruneAfterHeight = 1000; genesis = CreateGenesisBlock(1390666206UL, 3861367235UL, 0x1e0ffff0, 1, 50 * COIN); @@ -470,7 +468,6 @@ class CDevNetParams : public CChainParams { pchMessageStart[3] = 0xff; vAlertPubKey = ParseHex("04517d8a699cb43d3938d7b24faaff7cda448ca4ea267723ba614784de661949bf632d6304316b244646dea079735b9a6fc4af804efb4752075b9fe2245e14e412"); nDefaultPort = 19999; - nDelayGetHeadersTime = 24 * 60 * 60; nPruneAfterHeight = 1000; genesis = CreateGenesisBlock(1417713337, 1096447, 0x207fffff, 1, 50 * COIN); @@ -586,7 +583,6 @@ class CRegTestParams : public CChainParams { pchMessageStart[1] = 0xc1; pchMessageStart[2] = 0xb7; pchMessageStart[3] = 0xdc; - nDelayGetHeadersTime = std::numeric_limits::max(); // never delay GETHEADERS in regtests nDefaultPort = 19994; nPruneAfterHeight = 1000; diff --git a/src/chainparams.h b/src/chainparams.h index a4ebe40e3c71..034e186bc73d 100644 --- a/src/chainparams.h +++ b/src/chainparams.h @@ -69,7 +69,6 @@ class CChainParams bool DefaultConsistencyChecks() const { return fDefaultConsistencyChecks; } /** Policy: Filter transactions that do not match well-defined patterns */ bool RequireStandard() const { return fRequireStandard; } - int64_t DelayGetHeadersTime() const { return nDelayGetHeadersTime; } uint64_t PruneAfterHeight() const { return nPruneAfterHeight; } /** Make miner stop after a block is found. In RPC, don't return until nGenProcLimit blocks are generated */ bool MineBlocksOnDemand() const { return fMineBlocksOnDemand; } @@ -96,7 +95,6 @@ class CChainParams //! Raw pub key bytes for the broadcast alert signing key. std::vector vAlertPubKey; int nDefaultPort; - int64_t nDelayGetHeadersTime; uint64_t nPruneAfterHeight; std::vector vSeeds; std::vector base58Prefixes[MAX_BASE58_TYPES]; diff --git a/src/net.h b/src/net.h index f472958e8f16..cd135e5f230d 100644 --- a/src/net.h +++ b/src/net.h @@ -774,9 +774,6 @@ class CNode // Used for headers announcements - unfiltered blocks to relay // Also protected by cs_inventory std::vector vBlockHashesToAnnounce; - // Blocks received by INV while headers chain was too far behind. These are used to delay GETHEADERS messages - // Also protected by cs_inventory - std::vector vDelayedGetHeaders; // Used for BIP35 mempool sending, also protected by cs_inventory bool fSendMempool; @@ -921,12 +918,6 @@ class CNode vBlockHashesToAnnounce.push_back(hash); } - void PushDelayedGetHeaders(const uint256 &hash) - { - LOCK(cs_inventory); - vDelayedGetHeaders.push_back(hash); - } - void AskFor(const CInv& inv); void CloseSocketDisconnect(); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 4012b636a0d0..cc63a4ee1cff 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1302,24 +1302,6 @@ inline void static SendBlockTransactions(const CBlock& block, const BlockTransac connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::BLOCKTXN, resp)); } -bool static CheckGoodHeadersSyncPeer(CNode* pfrom, const CChainParams& chainparams, int nCount) -{ - AssertLockHeld(cs_main); - - bool fGenesis = pindexBestHeader->GetBlockHash() == chainparams.GetConsensus().hashGenesisBlock; - bool fDevNetGenesis = !chainparams.GetConsensus().hashDevnetGenesisBlock.IsNull() && pindexBestHeader->GetBlockHash() == chainparams.GetConsensus().hashDevnetGenesisBlock; - if (!fGenesis && !fDevNetGenesis && nCount < MAX_HEADERS_RESULTS && GetAdjustedTime() - pindexBestHeader->GetBlockTime() > chainparams.DelayGetHeadersTime()) { - // peer has sent us a HEADERS message below maximum size and we are still quite far from being fully - // synced, this means we probably got a bad peer for initial sync and need to continue with another one. - // By disconnecting we force to start a new iteration of initial headers sync in SendMessages - // TODO should we handle whitelisted peers here as we do in headers sync timeout handling? - pfrom->fDisconnect = true; - return error("detected bad peer for initial headers sync, disconnecting peer=%d", pfrom->id); - } - - return true; -} - bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStream& vRecv, int64_t nTimeReceived, const CChainParams& chainparams, CConnman& connman, const std::atomic& interruptMsgProc) { LogPrint("net", "received: %s (%u bytes) peer=%d\n", SanitizeString(strCommand), vRecv.size(), pfrom->id); @@ -1701,30 +1683,13 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr if (inv.type == MSG_BLOCK) { UpdateBlockAvailability(pfrom->GetId(), inv.hash); if (!fAlreadyHave && !fImporting && !fReindex && !mapBlocksInFlight.count(inv.hash)) { - // Always send GETHEADERS when we are still on the devnet genesis block. Otherwise we'll never sync. - // This is because after startup of the node, we are in IBD mode, which will only be left when recent - // blocks arrive. At the same time, we won't get any blocks from peers because we keep delaying - // GETHEADERS - bool fDevNetGenesis = !chainparams.GetConsensus().hashDevnetGenesisBlock.IsNull() && pindexBestHeader->GetBlockHash() == chainparams.GetConsensus().hashDevnetGenesisBlock; - - if (!fDevNetGenesis && (GetAdjustedTime() - pindexBestHeader->GetBlockTime() > chainparams.DelayGetHeadersTime())) { - // We are pretty far from being completely synced at the moment. If we would initiate a new - // chain of GETHEADERS/HEADERS now, we may end up downnloading the full chain from multiple - // peers at the same time, slowing down the initial sync. At the same time, we don't know - // if the peer we got this INV from may have a chain we don't know about yet, so we HAVE TO - // send a GETHEADERS message at some point in time. This is delayed to later in SendMessages - // when the headers chain has catched up enough. - LogPrint("net", "delaying getheaders (%d) %s to peer=%d\n", pindexBestHeader->nHeight, inv.hash.ToString(), pfrom->id); - pfrom->PushDelayedGetHeaders(inv.hash); - } else { - // We used to request the full block here, but since headers-announcements are now the - // primary method of announcement on the network, and since, in the case that a node - // fell back to inv we probably have a reorg which we should get the headers for first, - // we now only provide a getheaders response here. When we receive the headers, we will - // then ask for the blocks we need. - connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::GETHEADERS, chainActive.GetLocator(pindexBestHeader), inv.hash)); - LogPrint("net", "getheaders (%d) %s to peer=%d\n", pindexBestHeader->nHeight, inv.hash.ToString(), pfrom->id); - } + // We used to request the full block here, but since headers-announcements are now the + // primary method of announcement on the network, and since, in the case that a node + // fell back to inv we probably have a reorg which we should get the headers for first, + // we now only provide a getheaders response here. When we receive the headers, we will + // then ask for the blocks we need. + connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::GETHEADERS, chainActive.GetLocator(pindexBestHeader), inv.hash)); + LogPrint("net", "getheaders (%d) %s to peer=%d\n", pindexBestHeader->nHeight, inv.hash.ToString(), pfrom->id); } } else @@ -2464,9 +2429,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr if (nCount == 0) { // Nothing interesting. Stop asking this peers for more headers. - // See if it's a peer with "good" headers though. - LOCK(cs_main); - return CheckGoodHeadersSyncPeer(pfrom, chainparams, nCount); + return true; } const CBlockIndex *pindexLast = NULL; @@ -2540,8 +2503,6 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr // from there instead. LogPrint("net", "more getheaders (%d) to end to peer=%d (startheight:%d)\n", pindexLast->nHeight, pfrom->id, pfrom->nStartingHeight); connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::GETHEADERS, chainActive.GetLocator(pindexLast), uint256())); - } else if (!CheckGoodHeadersSyncPeer(pfrom, chainparams, nCount)) { - return false; } bool fCanDirectFetch = CanDirectFetch(chainparams.GetConsensus()); @@ -3050,8 +3011,7 @@ class CompareInvMempoolOrder bool SendMessages(CNode* pto, CConnman& connman, const std::atomic& interruptMsgProc) { - const CChainParams chainParams = Params(); - const Consensus::Params& consensusParams = chainParams.GetConsensus(); + const Consensus::Params& consensusParams = Params().GetConsensus(); { // Don't send anything until the version handshake is complete if (!pto->fSuccessfullyConnected || pto->fDisconnect) @@ -3158,17 +3118,6 @@ bool SendMessages(CNode* pto, CConnman& connman, const std::atomic& interr } } - if (GetAdjustedTime() - pindexBestHeader->GetBlockTime() <= chainParams.DelayGetHeadersTime()) { - // Headers chain has catched up enough so we can send out GETHEADER messages which were initially meant to - // be sent directly after INV was received - LOCK(pto->cs_inventory); - BOOST_FOREACH(const uint256 &hash, pto->vDelayedGetHeaders) { - LogPrint("net", "process delayed getheaders (%d) to peer=%d\n", pindexBestHeader->nHeight, pto->id); - connman.PushMessage(pto, msgMaker.Make(NetMsgType::GETHEADERS, chainActive.GetLocator(pindexBestHeader), hash)); - } - pto->vDelayedGetHeaders.clear(); - } - // Resend wallet transactions that haven't gotten in a block yet // Except during reindex, importing and IBD, when old wallet // transactions become unconfirmed and spams other nodes. From 753ed61fb3ed361e86989a7989114dd6c2df6c76 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Sat, 14 Apr 2018 21:38:53 +0300 Subject: [PATCH 2/2] Fix duplicate initial headers sync --- qa/rpc-tests/bip68-112-113-p2p.py | 3 ++- qa/rpc-tests/maxuploadtarget.py | 4 ++-- src/net_processing.cpp | 23 ++++++++++++++++++++--- 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/qa/rpc-tests/bip68-112-113-p2p.py b/qa/rpc-tests/bip68-112-113-p2p.py index 932a8978dfbc..2915029d0b63 100755 --- a/qa/rpc-tests/bip68-112-113-p2p.py +++ b/qa/rpc-tests/bip68-112-113-p2p.py @@ -98,8 +98,9 @@ def __init__(self): def setup_network(self): # Must set the blockversion for this test + # Must also set '-maxtipage=600100' to allow syncing from very old blocks self.nodes = start_nodes(self.num_nodes, self.options.tmpdir, - extra_args=[['-debug', '-whitelist=127.0.0.1', '-blockversion=4']], + extra_args=[['-debug', '-whitelist=127.0.0.1', '-blockversion=4', '-maxtipage=600100']], binary=[self.options.testbinary]) def run_test(self): diff --git a/qa/rpc-tests/maxuploadtarget.py b/qa/rpc-tests/maxuploadtarget.py index 2e69940ade6c..e624c28126f2 100755 --- a/qa/rpc-tests/maxuploadtarget.py +++ b/qa/rpc-tests/maxuploadtarget.py @@ -91,7 +91,7 @@ def __init__(self): def setup_network(self): # Start a node with maxuploadtarget of 200 MB (/24h) self.nodes = [] - self.nodes.append(start_node(0, self.options.tmpdir, ["-debug", "-maxuploadtarget=200", "-blockmaxsize=999000"])) + self.nodes.append(start_node(0, self.options.tmpdir, ["-debug", "-maxuploadtarget=200", "-blockmaxsize=999000", "-maxtipage="+str(2*60*60*24*7)])) def run_test(self): # Advance all nodes 2 weeks in the future @@ -205,7 +205,7 @@ def run_test(self): #stop and start node 0 with 1MB maxuploadtarget, whitelist 127.0.0.1 print("Restarting nodes with -whitelist=127.0.0.1") stop_node(self.nodes[0], 0) - self.nodes[0] = start_node(0, self.options.tmpdir, ["-debug", "-whitelist=127.0.0.1", "-maxuploadtarget=1", "-blockmaxsize=999000"]) + self.nodes[0] = start_node(0, self.options.tmpdir, ["-debug", "-whitelist=127.0.0.1", "-maxuploadtarget=1", "-blockmaxsize=999000", "-maxtipage="+str(2*60*60*24*7)]) #recreate/reconnect 3 test nodes test_nodes = [] diff --git a/src/net_processing.cpp b/src/net_processing.cpp index cc63a4ee1cff..7da845946121 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1682,7 +1682,24 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr if (inv.type == MSG_BLOCK) { UpdateBlockAvailability(pfrom->GetId(), inv.hash); - if (!fAlreadyHave && !fImporting && !fReindex && !mapBlocksInFlight.count(inv.hash)) { + + if (fAlreadyHave || fImporting || fReindex || mapBlocksInFlight.count(inv.hash)) { + continue; + } + + CNodeState *state = State(pfrom->id); + if (!state) { + continue; + } + + // Download if this is a nice peer, or we have no nice peers and this one might do. + bool fFetch = state->fPreferredDownload || (nPreferredDownload == 0 && !pfrom->fOneShot); + // Only actively request headers from a single peer, unless we're close to end of initial download. + if ((nSyncStarted == 0 && fFetch) || pindexBestHeader->GetBlockTime() > GetAdjustedTime() - nMaxTipAge) { + // Make sure to mark this peer as the one we are currently syncing with etc. + state->fSyncStarted = true; + state->nHeadersSyncTimeout = GetTimeMicros() + HEADERS_DOWNLOAD_TIMEOUT_BASE + HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER * (GetAdjustedTime() - pindexBestHeader->GetBlockTime())/(chainparams.GetConsensus().nPowTargetSpacing); + nSyncStarted++; // We used to request the full block here, but since headers-announcements are now the // primary method of announcement on the network, and since, in the case that a node // fell back to inv we probably have a reorg which we should get the headers for first, @@ -3099,7 +3116,7 @@ bool SendMessages(CNode* pto, CConnman& connman, const std::atomic& interr bool fFetch = state.fPreferredDownload || (nPreferredDownload == 0 && !pto->fClient && !pto->fOneShot); // Download if this is a nice peer, or we have no nice peers and this one might do. if (!state.fSyncStarted && !pto->fClient && !fImporting && !fReindex) { // Only actively request headers from a single peer, unless we're close to end of initial download. - if ((nSyncStarted == 0 && fFetch) || pindexBestHeader->GetBlockTime() > GetAdjustedTime() - 6 * 60 * 60) { // NOTE: was "close to today" and 24h in Bitcoin + if ((nSyncStarted == 0 && fFetch) || pindexBestHeader->GetBlockTime() > GetAdjustedTime() - nMaxTipAge) { state.fSyncStarted = true; state.nHeadersSyncTimeout = GetTimeMicros() + HEADERS_DOWNLOAD_TIMEOUT_BASE + HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER * (GetAdjustedTime() - pindexBestHeader->GetBlockTime())/(consensusParams.nPowTargetSpacing); nSyncStarted++; @@ -3418,7 +3435,7 @@ bool SendMessages(CNode* pto, CConnman& connman, const std::atomic& interr // Check for headers sync timeouts if (state.fSyncStarted && state.nHeadersSyncTimeout < std::numeric_limits::max()) { // Detect whether this is a stalling initial-headers-sync peer - if (pindexBestHeader->GetBlockTime() <= GetAdjustedTime() - 6*60*60) { // was 24*60*60 in bitcoin + if (pindexBestHeader->GetBlockTime() <= GetAdjustedTime() - nMaxTipAge) { if (nNow > state.nHeadersSyncTimeout && nSyncStarted == 1 && (nPreferredDownload - state.fPreferredDownload >= 1)) { // Disconnect a (non-whitelisted) peer if it is our only sync peer, // and we have others we could be using instead.