IBD using chainwork instead of height and not using header timestamps

gmaxwell commented at 0:47 am on November 1, 2016: contributor

This introduces a ‘minimum chain work’ chainparam which is intended to be the known amount of work in the chain for the network at the time of software release. If you don’t have this much work, you’re not yet caught up.

This is used instead of the count of blocks test from checkpoints.

This criteria is trivial to keep updated as there is no element of subjectivity, trust, or position dependence to it. It is also a more reliable metric of sync status than a block count.

This is the first step in a larger change to eliminate checkpoints completely, but it’s independently a good idea.

This PR makes IsInitialBlockDownload no longer use header-only timestamps.

This avoids a corner case (mostly visible on testnet) where bogus headers can keep nodes in IsInitialBlockDownload.

MarcoFalke commented at 10:05 am on November 1, 2016: member

utACK f3f8d2e

Nit: GetTotalBlocksEstimate is no longer used (only in tests). Feel free to remove that now.

in doc/release-process.md: in f3f8d2e855 outdated

46@@ -47,6 +47,7 @@ Update the following:
47 - `doc/README.md` and `doc/README_windows.txt`
48 - `doc/Doxyfile`: `PROJECT_NUMBER` contains the full version
49 - `contrib/gitian-descriptors/*.yml`: usually one'd want to do this on master after branching off the release - but be sure to at least do it before a new major release
50+- `src/chainparams.cpp`: periodically update nMinimumChainWork with information from the getblockchaininfo rpc.

MarcoFalke commented at 10:06 am on November 1, 2016:

Nit: I think you want to move this up to the section " Before every minor and major release:"

MarcoFalke added the label Refactoring on Nov 1, 2016

MarcoFalke added the label Consensus on Nov 1, 2016

dcousens approved

Leviathn commented at 0:57 am on November 2, 2016: none

utACK - certainly a more reliable metric.

IBD check uses minimumchain work instead of checkpoints.

This introduces a 'minimum chain work' chainparam which is intended
 to be the known amount of work in the chain for the network at the
 time of software release.  If you don't have this much work, you're
 not yet caught up.

This is used instead of the count of blocks test from checkpoints.

This criteria is trivial to keep updated as there is no element of
subjectivity, trust, or position dependence to it. It is also a more
reliable metric of sync status than a block count.

fd46136dfa

Remove GetTotalBlocksEstimate and checkpoint tests that test nothing.

GetTotalBlocksEstimate is no longer used and it was the only thing
 the checkpoint tests were testing.

Since checkpoints are on their way out it makes more sense to remove
 the test file than to cook up a new pointless test.

2082b5574c

IsInitialBlockDownload no longer uses header-only timestamps.

This avoids a corner case (mostly visible on testnet) where bogus
 headers can keep nodes in IsInitialBlockDownload.

e141beb6a9

gmaxwell commented at 4:10 am on November 2, 2016: contributor

@MarcoFalke nits addressed, I think.

in src/chainparams.cpp: in e141beb6a9

 95@@ -96,6 +96,9 @@ class CMainParams : public CChainParams {
 96         consensus.vDeployments[Consensus::DEPLOYMENT_SEGWIT].nStartTime = 1479168000; // November 15th, 2016.
 97         consensus.vDeployments[Consensus::DEPLOYMENT_SEGWIT].nTimeout = 1510704000; // November 15th, 2017.
 98 
 99+        // The best chain should have at least this much work.
100+        consensus.nMinimumChainWork = uint256S("0x0000000000000000000000000000000000000000002cb971dd56d1c583c20f90");

rebroad commented at 5:25 am on November 2, 2016:

Where is this number from please?

MarcoFalke commented at 9:05 am on November 2, 2016:

@rebroad It is mentioned in the doc change in this very pull…

gmaxwell commented at 1:28 am on November 3, 2016:

You can just run the getblockchaininfo rpc per the instructions and observe that its a number equal to or greater than that in order to verify it. :)

MarcoFalke commented at 9:07 am on November 2, 2016: member

re-utACK e141beb6a9816b7e1e680fb0a8bae16d42a3e557

laanwj commented at 7:53 pm on November 2, 2016: member

utACK e141beb

sipa commented at 7:04 am on November 3, 2016: member

utACK e141beb6a9816b7e1e680fb0a8bae16d42a3e557

sipa commented at 7:06 am on November 3, 2016: member

I wonder if this should be backported to 0.13.2, as it fixes a visible issue on testnet.

sipa merged this on Nov 3, 2016

sipa closed this on Nov 3, 2016

sipa referenced this in commit 508404de98 on Nov 3, 2016

laanwj added this to the milestone 0.13.2 on Nov 3, 2016

laanwj added the label Needs backport on Nov 3, 2016

laanwj removed the label Needs backport on Nov 3, 2016

laanwj removed this from the milestone 0.13.2 on Nov 3, 2016

jtimon commented at 10:11 pm on November 3, 2016: contributor

utACK e141beb6a9816b7e1e680fb0a8bae16d42a3e557 very clean to read commit by commit.

in src/main.cpp: in e141beb6a9

1748-    if (!state)
1749-        latchToFalse.store(true, std::memory_order_relaxed);
1750-    return state;
1751+    if (chainActive.Tip()->nChainWork < UintToArith256(chainParams.GetConsensus().nMinimumChainWork))
1752+        return true;
1753+    if (chainActive.Tip()->GetBlockTime() < (GetTime() - nMaxTipAge))

mruddy commented at 12:58 pm on November 17, 2016:

Any particular reason to use GetTime here over GetAdjustedTime?

laanwj referenced this in commit e591c1049f on Dec 8, 2016

zkbot referenced this in commit ebd25d1744 on Jul 16, 2018

zkbot referenced this in commit 6d605ffbbe on Jul 17, 2018

zkbot referenced this in commit 3835cbb57f on Jul 17, 2018

MarcoFalke locked this on Sep 8, 2021

IBD using chainwork instead of height and not using header timestamps #9053