In the unlikely event that all or most of the listening nodes are restarted into Initial Block Download mode the network might get stuck with little or no listening nodes providing headers or blocks.
The timeout is intended to move nodes out of IBD so they will request and serve blocks and headers to inbound peers.
Edit: By latching IBD across restarts we prevent the network from getting into this state at all.
1304@@ -1305,6 +1305,14 @@ bool CChainState::IsInitialBlockDownload() const
1305 return false;
1306 if (fImporting || fReindex)
1307 return true;
1308+ uint64_t ibdtimeout = gArgs.GetArg("-ibdtimeout", DEFAULT_IBD_TIMEOUT);
1309+ int64_t uptime = GetTime() - GetStartupTime();
1310+ assert(uptime >= 0);
This assumes that GetTime() is monotonic, which I don’t think is true. GetTime() is mockable so may move backwards in tests.
It looks like GetTime() is also deprecated.
Nodes in IBD will not respond to GETHEADERS. Nodes in IBD do not request headers from inbound peers only outbound.
If all or most of the listening nodes are in IBD (emergency hot fix or something) then nobody will be able to get headers.
To get out of this the listening nodes need to request headers from inbound connections, either by exiting IBD or special casing that limit while in IBD.
all or most of the listening nodes are restarted into Initial Block Download mode
Wouldn’t that mean that there hasn’t been a block in 24 hours? Then, waiting another two days for the next block doesn’t seem like a fallback that will be needed.
1304@@ -1305,11 +1305,19 @@ bool CChainState::IsInitialBlockDownload() const
1305 return false;
1306 if (fImporting || fReindex)
1307 return true;
1308+ uint64_t ibdtimeout = gArgs.GetArg("-ibdtimeout", DEFAULT_IBD_TIMEOUT);
1309+ int64_t uptime = GetTime() - GetStartupTime();
The ibdtimeout and uptime localvars can be const.
As @jnewbery mentions, we probably don’t want to use GetTime() here, see src/util/time.h.
Also maybe consider how this interacts with setmocktime
0 // For now, don't change mocktime if we're in the middle of validation, as
1 // this could have an effect on mempool time-based eviction, as well as
2 // IsCurrentForFeeEstimation() and IsInitialBlockDownload().
3 // TODO: figure out the right way to synchronize around mocktime, and
4 // ensure all call sites of GetTime() are accessing this safely.
52@@ -53,6 +53,7 @@ struct DisconnectedBlockTransactions;
53 struct PrecomputedTransactionData;
54 struct LockPoints;
55
56+static const unsigned int DEFAULT_IBD_TIMEOUT = 3600 * 48; // 48 Hours
constexpr and mention that the constant refers to a time duration in seconds.
-ibdtimeout value of 48 hours, which seems long, what other durations users would want to provide, or what guidance can be given to users. This probably needs a functional test if it goes forward.
the tricky part is being equally confident that the various attacks that the IBD specific behaviour is intended to protect against are still protected against– esp because no one reviewing this work may remember all of them
Yes.
If this was a boolean option (default to false), it would be easier to review than this time-based threshold. Obviously, it would also lose the node-fixes-itself-after-three-days feature. Though, I am still sceptical of it’s use. It assumes that all of your connections (inbound, as well as block-feeler, outbound block-relay, …) are out of IBD for three days without a single one of them setting the flag manually.
(Edit: I wrote this before the previous comment, so it is not a reply to that)
@MarcoFalke In IBD we do not request headers from inbound peers. Literally only from a single outbound peer with a timeout.
A setting to disable IBD for your own node might fix your node if your node is listening and receiving inbound connections from nodes which are sync’d with miners. Relying on that would be extremely fragile.
Even if you were able to fix your own node, you can’t then heal the rest of the network without them connecting to you as the single initial sync peer, the odds of which are extremely low.
The timeout prevents the network from being in this state for more than 48 hours with a bunch of nodes stuck in IBD.
I’m assuming that the vast majority of listening nodes are not actively maintained and have no wallet, but are rather used as a kind of gateway, so being down for 48 hours is a nuisance to the rest of the network, but is probably not directly going to cause them to take action.
1322@@ -1312,6 +1323,9 @@ bool CChainState::IsInitialBlockDownload() const
1323 if (m_chain.Tip()->GetBlockTime() < (GetTime() - nMaxTipAge))
1324 return true;
1325 LogPrintf("Leaving InitialBlockDownload (latching to false)\n");
1326+ f = fsbridge::fopen(latch_filename, "wb");
1327+ if (!f) LogPrintf("Failed to write %s \n", latch_filename.string().c_str());
1328+ else fclose(f);
I think we now use bracket syntax for the conditional (after a famous Apple bug iirc)
0- if (!f) LogPrintf("Failed to write %s \n", latch_filename.c_str());
1- else fclose(f);
2+ if (f) {
3+ fclose(f);
4+ } else {
5+ LogPrintf("Failed to write %s\n", latch_filename.c_str());
6+ }
Coding style guidelines doesn’t agree, but I will expand it to new lines to make it easier to read.
(Indeed ironically I argued for using braces everywhere, but we don’t).
https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c
The developer notes state:
0 - If an `if` only has a single-statement `then`-clause, it can appear
1 on the same line as the `if`, without braces. In every other case,
2 braces are required, and the `then` and `else` clauses must appear
3 correctly indented on a new line.
else it would be fine, but with the else then braces are required (at any rate, that is what I’ve been doing).
this prevents a network wide failure in the event that all or most listening
nodes are restarted at approximately the same time
1294@@ -1294,15 +1295,23 @@ void CChainState::InitCoinsCache(size_t cache_size_bytes)
1295 // `const` so that `CValidationInterface` clients (which are given a `const CChainState*`)
1296 // can call it.
1297 //
1298+// The file .ibd_complete is used to latch the datadir to IBD false across restarts
doc/files.md (alternatively, perhaps persist the latch in settings.json, which is also chain-specific).
Users can easily override settings.json as well - they’re just interpreted as command-line options.
So something like --ibd_complete=0/1 would work
Is there anything I need to do here?
Convince other contributors that this is a good idea. Currently, if a node is switched off and then switched back on some time later, it’ll start up in IBD until it catches up to the tip. This seems like good behaviour - we don’t want to download transactions, relay our address, respond to getheaders, etc until we’ve caught back up. If you want other contributors to ACK this PR, you’ll need to convince them that changing that behaviour is an improvement, and does not introduce any regressions/vulnerabilities/etc.
1308 if (m_cached_finished_ibd.load(std::memory_order_relaxed))
1309 return false;
1310+ if (fs::exists(latch_filename)) {
1311+ LogPrintf("Leaving InitialBlockDownload (latching to false)\n");
1312+ m_cached_finished_ibd.store(true, std::memory_order_relaxed);
1313+ return false;
-reindex? If yes, that seems wrong
TL;DR: I think we should just drop the “don’t respond to GETHEADERS while in IBD” behavior, rather than keeping the IsInitialBlockDownload() latch across restarts. Possibly we can also improve the find-another-peer-to-sync-headers-from logic (and, if that’s the real problem, this PR doesn’t currently address it).
These are the things that IsInitialBlockDownload() affects (there are more, but these are probably the most important ones).
getblocktemplate RPC doesn’t work while in IBD.There is also a related behavior that is not controlled by IsInitialBlockDownload(), but by pindexBestHeader->GetBlockTime() > GetAdjustedTime() - 24 * 60 * 60 instead:
I believe it is mostly (1) and (6) we’re concerned about here?
I don’t actually know why (1) exists in the first place. Perhaps it’s trying to minimize time to be synced with the network first before providing services to others? I think that only has a minimal effect, especially when combined with (2). It seems to me that just removing (1) would address most of the concern here.
It seems possible to improve on (6) as well, but it’s fundamentally a tradeoff between bandwidth and (high probability of) fast header syncing. The headers sync timeout is ~15 minutes though, which can be annoying, but is probably short enough that (6) doesn’t actually matter for the scenario under consideration here.
A way to improve (6) (especially useful when combined with removing (1)) would be to treat a “less than 2000 headers received at once” while the last one doesn’t get out us out of IBD as “done syncing with this peer, pick another one”.
These avoid the need to permanently latch to IBD, which seems like a very heavy hammer. Especially the transaction (4) and compact block related behaviours (5) we probably want to keep for nodes that were once out of IBD, but have been offline for a substantial period of time.
(3) does look fairly scary if nodes can actually get stuck in IBD - but it’s also a sign of a deeper problem. I’m not sure if that warrants addressing beyond just trying to make sure we don’t get stuck in IBD.
@pstratem So I suspect that what happened in $ALTCOIN is the headers fetching logic is approximately doing this:
I don’t believe the current PR actually addresses this, because none of the logic above is dependent on IsInitialBlockDownload. If actually all reachable nodes on the network are in “headers 24 hours behind” state, then making them IsIBD()==false would make them respond to headers, but never enough to get close enough to $NOW to get the network unstuck.
Here are a couple alternative ideas (not all necessary, but probably most powerful if all 3 are done):
@sipa Originally the logic was added to not respond to GETHEADERS while in IBD because we relaxed the checkpoint logic, so that we could accept a non-checkpointed chain during initial sync. If a node was in IBD and fed a bogus initial headers chain, then a peer that is on the checkpointed chain would ban us if we gave them a checkpoint-violating header.
Since then, our logic has changed a lot, both around banning and with the addition of minimum-chain-work. An easy improvement would be to respond to getheaders messages as long as our tip is past the minimum chain work that we’re configured with. @pstratem My understanding of block relay is that the scenario that I think you have in mind should not really exist, but perhaps I’m not understanding the scenario. Let’s say the whole network is in IBD for some reason, because no blocks have been found in several days and then everyone restarts their software. At this point, initial headers sync with every peer is timing out every 20 minutes or whatever causing some peer churn (this all sounds bad, but not yet disastrous).
(1) Then a miner finds a block and submits it to their node. That node is not a listening node, but (2) with the new block it will leave IBD. It will also (3) announce that block (via INV) to all its peers, which will in turn immediately (4) send a getheaders to the node, which it will (5) respond to because its out of IBD.
That will (6) trigger block relay to all those peers via a getdata, which in turn will cause those nodes to (7) leave IBD and then (8) announce the block to their peers. Things should progress fine from there, in the same way, I think – what do you think is breaking down exactly?
I guess (1) could break down because miners have to manually cause their own node to leave IBD in order for getblocktemplate to work, via the -maxtipage option. But if you assume miners will quickly figure that out to not waste their hashpower, I think everything should work after that.
That will (6) trigger block relay to all those peers via a getdata, which in turn will cause those nodes to (7) leave IBD and then (8) announce the block to their peers. Things should progress fine from there, in the same way, I think – what do you think is breaking down exactly?
Aha, on further investigation, I think (6) is where things break down: when a node is in IBD because its current tip is too old, if we learn of a new block header from an inbound peer, we will not fetch the block when we get the header via direct-fetch (because CanDirectFetch will return false due to our old tip), and we won’t fetch the block through parallel block fetch (via FindNextBlocksToDownload) if we’re in IBD and we have any outbound peers. I was able to replicate this behavior in a simple regtest setting.
My recollection is that we don’t want to do full-on IBD from inbound peers because users running bitcoind at home might not want to be used for some outbound peer’s initial sync. Assuming we think that is still a reasonable concern, a simple fix might be to try fetching blocks via FindNextBlocksToDownload from inbound peers if we have no blocks in flight, as presumably that means our outbound peers have nothing for us, so we might as well treat that scenario the same as if we have no outbounds at all.
(To be clear, I don’t believe that the headers-sync behaviors with inbound peers are really a problem, because if an inbound peer is the only one to be finding new blocks, we will get their headers via our inv-processing behavior. I think this is just a concern around block fetching.)
Noting that I was operating a signet for a while which crashed, and by the time I rebooted it (life happens) it was a few months later. Now after restarting, I think the IBD latch is keeping my seed/mining node in IBD since it’s in the past, even though I’m producing new blocks. This also prevents this node from relaying the new blocks to peers.
This patch might fix this (pathological) case.
h/t @ajtowns for pointing this out.
Now after restarting, I think the IBD latch is keeping my seed/mining node in IBD since it’s in the past, even though I’m producing new blocks.
Staying in IBD while producing a block with a recent enough timestamp shouldn’t be possible. It may only happen that getblocktemplate refuses to give you a reply due to IBD, but since you produced a block this is probably not something you ran into. (If you were, you could use -maxtipage)
Going to close this for now, since there hasn’t been any activity by the author for about a year after the author acknowledged that the current patch has a bug (https://github.com/bitcoin/bitcoin/pull/21106#discussion_r584207780).
Discussion can continue and this can be reopened any time.
Staying in IBD while producing a block with a recent enough timestamp shouldn’t be possible.
Once started, the signet miner defaults to calculating timestamps as “previous block time + 10 minutes” with optional randomisation, so if the node was down for weeks or more it could take a while to catch up with realtime and hence leave the miner’s node in IBD.
Ok, I see. So this pull could indeed make sense for signet. Or, as an alternative the signet miner could modify the mine script to mine a block with a more recent timestamp or somehow mine the “missing” blocks faster if it is important to have them.
For mainnet, my understanding is that #24171 will be sufficient to address the issue, as explained in #21106 (comment) and the following comment.
Ok, I see. So this pull could indeed make sense for signet. Or, as an alternative the signet miner could modify the mine script to mine a block with a more recent timestamp or somehow mine the “missing” blocks faster if it is important to have them.
It’s just a default: you can run the miner with --set-block-time=-1 --max-blocks=1 to mine a single block at the current time, then restart with --ongoing which will continue from there. (Not sure if that had been implemented when Jeremy was having problems) So I don’t think it’s too important to optimise for that scenario.