Fix IsInitialBlockDownload which was broken by headers first.

rdponticelli commented at 6:50 pm on October 28, 2014: contributor

gavinandresen commented at 7:17 pm on October 28, 2014: contributor

How did you test this?

rdponticelli commented at 7:59 pm on October 28, 2014: contributor

I tested this by running it over a derivative of #4701 on syncing, reindexing and up-to-date nodes. Those cases were broken before this patch, and work well after.

The dependence on local time is ugly, but that was there before, anyway…

rdponticelli force-pushed on Oct 28, 2014

Fix IsInitialBlockDownload which was broken by headers first. a2d0fc658a

rdponticelli force-pushed on Oct 28, 2014

gmaxwell commented at 11:06 pm on October 28, 2014: contributor

This gets rid of the nLastUpdate static, I’m happy about that. This seems reasonable to me. utACK.

laanwj added the label Bug on Oct 29, 2014

in src/main.cpp: in a2d0fc658a outdated

1184-        pindexLastBest = chainActive.Tip();
1185-        nLastUpdate = GetTime();
1186-    }
1187-    return (GetTime() - nLastUpdate < 10 &&
1188-            chainActive.Tip()->GetBlockTime() < GetTime() - 24 * 60 * 60);
1189+    return (chainActive.Height() < pindexBestHeader->nHeight - 24 * 6 ||

TheBlueMatt commented at 5:15 am on October 30, 2014:

Shouldnt you just check if the best header index has received the block contents? Nodes could be mean and not relay contents, but that means either they’re running an incredibly expensive annoyance or you’re behind a strange sybil.

gmaxwell commented at 5:18 am on October 30, 2014:

I don’t think we want to eroniously think we’re caught up when the best tip is far behind the local time (e.g. because we’re broken for some reason).

OTOH, there is a … somewhat corner case that might be wort worring about: With this proposed code, if the network goes 24 hours without a block (uh what?) everyone running this will stop mining. :P

TheBlueMatt commented at 5:21 am on October 30, 2014:

Sorry, I meant just like walk pindexBestHeader back N blocks, if we still havent received the data for that block, consider us to be in IBD.

gmaxwell commented at 6:22 am on October 30, 2014:

I’m not sure what behavior we want here. If the best header is still a day back, then we know we’re not in sync.

TheBlueMatt commented at 6:38 am on October 30, 2014:

I wasnt suggesting it exclusively…sorry if that was unclear.

laanwj commented at 10:03 am on November 3, 2014:

With this proposed code, if the network goes 24 hours without a block (uh what?) everyone running this will stop mining. :P

Arguably, we should never stop mining whatever happens. During a run of Bitcoin Core we should never go from !IsInitialBlockDownload() to IsInitialBlockDownload(). That just doesn’t make sense, so we can add a check to prevent it.

rdponticelli commented at 7:49 pm on November 5, 2014:

Wouldn’t a deep reorg caused by the reconnection of some long split nodes cause a possible valid case of !IsInitialBlockDownload() to IsInitialBlockDownload()?

gmaxwell commented at 8:03 pm on November 5, 2014:

By definition in that case you’ll be on a longer chain than what already got you past the initial download check; so I think it shouldn’t.

rdponticelli commented at 8:13 pm on November 5, 2014:

But it would be valid for the nodes going from the shortest chain to the longest one, right?

gmaxwell commented at 8:34 pm on November 5, 2014:

I think what we’re trying to approximate here is “are you caught up to at least the point that you should have been at when you got the software”, without having to insert a block height or total work into every download as it ships. And it’s okay if it sometimes falsely returns false too early. It’s not really okay if it returns true too late.

Esp since right now it’ll make you stop mining, meaning you can potentially get a remotely triggerable attack out of it. So probably I think wumpus’ idea is good: once it turns on, never go off again. … and then we can be pretty conservative with turning it on the first time.

sipa commented at 12:55 pm on November 4, 2014: member

In what ways is IsInitialBlockDownload broken now? Not a criticism, just trying to understand :)

rdponticelli commented at 3:00 pm on November 4, 2014: contributor

It randomly returns true or false during all the IBD.

rdponticelli commented at 1:08 pm on November 6, 2014: contributor

Added a commit implementing @laanwj suggestion to deal with @gmaxwell’s concern.

sipa commented at 1:13 pm on November 6, 2014: member

A single boolean would suffice, no?

rdponticelli force-pushed on Nov 6, 2014

rdponticelli commented at 2:32 pm on November 6, 2014: contributor

Simplified to use a single boolean.

gmaxwell commented at 5:48 pm on November 6, 2014: contributor

utACK.

in src/main.cpp: in fc37ef38ee outdated

1184-        pindexLastBest = chainActive.Tip();
1185-        nLastUpdate = GetTime();
1186-    }
1187-    return (GetTime() - nLastUpdate < 10 &&
1188-            chainActive.Tip()->GetBlockTime() < GetTime() - 24 * 60 * 60);
1189+    static bool lockibdstate;

Diapolo commented at 6:57 pm on November 6, 2014:

Nit: Can you make this lockIbdState or perhaps even lockIBDState to be readable? What do the others think.

laanwj commented at 10:32 am on November 7, 2014:

I don’t mind how it’s called (scope is just local anyhow), although I’d like an explicit

0static bool lockibdstate = false;

…for readability

rdponticelli commented at 6:57 pm on November 7, 2014:

Added.

TheBlueMatt commented at 11:56 pm on November 6, 2014: member

utACK: http://bitcoin.ninja/TheBlueMatt-5158.txt

Add a locking mechanism to IsInitialBlockDownload to ensure it never goes from false to true. 9ec75c5ef4

rdponticelli force-pushed on Nov 7, 2014

gmaxwell commented at 8:21 pm on November 7, 2014: contributor

still ACK (though only tested pretty lightly)

sipa commented at 4:22 pm on November 17, 2014: member

Note that #5241 reduces the number of conditionals that are based on IsInitialBlockdownload().

sipa commented at 4:23 pm on November 17, 2014: member

utACK

TheBlueMatt commented at 5:07 am on November 20, 2014: member

utACK only commithashes 9ec75c5ef4182a38e261beaafdc94325785cc7c5 + a2d0fc658a7b21d2d41ef2a6c657d24114b6c49e: http://bitcoin.ninja/TheBlueMatt-5158.txt

laanwj merged this on Nov 26, 2014

laanwj closed this on Nov 26, 2014

laanwj referenced this in commit 9ff0bc9beb on Nov 26, 2014

DrahtBot locked this on Sep 8, 2021

Fix IsInitialBlockDownload which was broken by headers first. #5158