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

pull gmaxwell wants to merge 3 commits into bitcoin:master from gmaxwell:no_checkpoint_for_ibd changing 8 files +20 −48

gmaxwell commented at 12: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:None 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 12: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.
dcousens cross-referenced this on Nov 2, 2016 from issue Ignore getheaders prior to passing all checkpoints. by rebroad

in src/chainparams.cpp:None 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 cross-referenced this on Nov 3, 2016 from issue Use a proper factory for creating chainparams by jtimon
jtimon commented at 10:11 PM on November 3, 2016: contributor

utACK e141beb6a9816b7e1e680fb0a8bae16d42a3e557 very clean to read commit by commit.
jtimon cross-referenced this on Nov 3, 2016 from issue Testchains: Introduce custom chain whose constructor... by jtimon

in src/main.cpp:None 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?

gmaxwell cross-referenced this on Dec 6, 2016 from issue [0.13 Backport] IBD using chainwork instead of height and not using header timestamp (#9053) by gmaxwell
laanwj referenced this in commit e591c1049f on Dec 8, 2016
codablock cross-referenced this on Aug 21, 2017 from issue Backport "assumed valid blocks" feature from Bitcoin 0.13 by codablock
str4d cross-referenced this on Jan 16, 2018 from issue Bitcoin 0.12 chainparams cleanups by str4d
str4d cross-referenced this on May 14, 2018 from issue InitialBlockDownload upstream changes by str4d
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
scravy cross-referenced this on Nov 13, 2018 from issue WIP: Work in Progress by scravy
bitcoin locked this on Sep 8, 2021

Contributors

Labels

Refactoring Consensus

Linked (view graph)

#8855 Use a proper factory for creating chainparams #8994 Testchains: Introduce custom chain whose constructor... #9061 Ignore getheaders prior to passing all checkpoints.#9293 [0.13 Backport] IBD using chainwork instead of height and not using header timestamp (#9053)