Skip to content

Drop delayed headers logic and fix duplicate initial headers sync by handling block inv correctly #2032

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
UdjinM6 merged 2 commits into from
Apr 20, 2018
Merged

Drop delayed headers logic and fix duplicate initial headers sync by handling block inv correctly #2032

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f585450
Drop custom logic for delaying GETHEADERS
UdjinM6 Feb 8, 2018
753ed61
Fix duplicate initial headers sync
UdjinM6 Apr 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion qa/rpc-tests/bip68-112-113-p2p.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
4 changes: 2 additions & 2 deletions qa/rpc-tests/maxuploadtarget.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 = []
Expand Down
4 changes: 0 additions & 4 deletions src/chainparams.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -586,7 +583,6 @@ class CRegTestParams : public CChainParams {
pchMessageStart[1] = 0xc1;
pchMessageStart[2] = 0xb7;
pchMessageStart[3] = 0xdc;
nDelayGetHeadersTime = std::numeric_limits<int64_t>::max(); // never delay GETHEADERS in regtests
nDefaultPort = 19994;
nPruneAfterHeight = 1000;

Expand Down
2 changes: 0 additions & 2 deletions src/chainparams.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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; }
Expand All @@ -96,7 +95,6 @@ class CChainParams
//! Raw pub key bytes for the broadcast alert signing key.
std::vector<unsigned char> vAlertPubKey;
int nDefaultPort;
int64_t nDelayGetHeadersTime;
uint64_t nPruneAfterHeight;
std::vector<CDNSSeedData> vSeeds;
std::vector<unsigned char> base58Prefixes[MAX_BASE58_TYPES];
Expand Down
9 changes: 0 additions & 9 deletions src/net.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -774,9 +774,6 @@ class CNode
// Used for headers announcements - unfiltered blocks to relay
// Also protected by cs_inventory
std::vector<uint256> 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<uint256> vDelayedGetHeaders;
// Used for BIP35 mempool sending, also protected by cs_inventory
bool fSendMempool;

Expand Down Expand Up @@ -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();
Expand Down
92 changes: 29 additions & 63 deletions src/net_processing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<bool>& interruptMsgProc)
{
LogPrint("net", "received: %s (%u bytes) peer=%d\n", SanitizeString(strCommand), vRecv.size(), pfrom->id);
Expand Down Expand Up @@ -1700,31 +1682,31 @@ 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);
}

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) {
Copy link
Copy Markdown

@codablock codablock Apr 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand this correct, a block that is being announced while we are not catched up, this blocks header won't be requested from any peer. We assume that the initial sync peer would provide it when it reaches the tip...but what if that peer was a bad choice and he didn't get this block? We'd not see the header of the new block until another block is mined on top of that one.

Copy link
Copy Markdown
Author

@UdjinM6 UdjinM6 Apr 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, same as with initial headers sync - we don't care about recent blocks or other peers until we are close to the tip announced by the peer we started syncing from. Once we are close, we start requesting headers from everyone.

Copy link
Copy Markdown

@codablock codablock Apr 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And if the initial sync peer is so bad that it never lets us reach near-tip height? In that case we'll not request headers from other peers until timeout kicks in? The timeout is quite long if I remember correct, so experience in this case won't be optimal, but it should sort itself out after some time.

Copy link
Copy Markdown
Author

@UdjinM6 UdjinM6 Apr 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it's Timeout = base + per_header * (expected number of headers) where base is 15 minutes and per_header is 1 ms per header. Plus, you can also specify custom -maxtipage if you know there are some huge forks at the time you are trying to sync, so new peers will be picked up quicker.

// 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,
// 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
Expand Down Expand Up @@ -2464,9 +2446,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;
Expand Down Expand Up @@ -2540,8 +2520,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());
Expand Down Expand Up @@ -3050,8 +3028,7 @@ class CompareInvMempoolOrder

bool SendMessages(CNode* pto, CConnman& connman, const std::atomic<bool>& 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)
Expand Down Expand Up @@ -3139,7 +3116,7 @@ bool SendMessages(CNode* pto, CConnman& connman, const std::atomic<bool>& 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++;
Expand All @@ -3158,17 +3135,6 @@ bool SendMessages(CNode* pto, CConnman& connman, const std::atomic<bool>& 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.
Expand Down Expand Up @@ -3469,7 +3435,7 @@ bool SendMessages(CNode* pto, CConnman& connman, const std::atomic<bool>& interr
// Check for headers sync timeouts
if (state.fSyncStarted && state.nHeadersSyncTimeout < std::numeric_limits<int64_t>::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.
Expand Down