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

Merged
merged 3 commits into from Nov 3, 2016

Projects

None yet

9 participants

@gmaxwell
Member
gmaxwell commented Nov 1, 2016 edited

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
Member

utACK f3f8d2e

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

doc/release-process.md
@@ -47,6 +47,7 @@ Update the following:
- `doc/README.md` and `doc/README_windows.txt`
- `doc/Doxyfile`: `PROJECT_NUMBER` contains the full version
- `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
+- `src/chainparams.cpp`: periodically update nMinimumChainWork with information from the getblockchaininfo rpc.
@MarcoFalke
MarcoFalke Nov 1, 2016 Member

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

@Leviathn
Leviathn commented Nov 2, 2016

utACK - certainly a more reliable metric.

gmaxwell added some commits Oct 22, 2016
@gmaxwell gmaxwell 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.
fd46136
@gmaxwell gmaxwell 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.
2082b55
@gmaxwell gmaxwell IsInitialBlockDownload no longer uses header-only timestamps.
This avoids a corner case (mostly visible on testnet) where bogus
 headers can keep nodes in IsInitialBlockDownload.
e141beb
@gmaxwell
Member
gmaxwell commented Nov 2, 2016

@MarcoFalke nits addressed, I think.

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

Where is this number from please?

@MarcoFalke
MarcoFalke Nov 2, 2016 Member

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

@gmaxwell
gmaxwell Nov 3, 2016 Member

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
Member

re-utACK e141beb

@laanwj
Member
laanwj commented Nov 2, 2016

utACK e141beb

@sipa
Member
sipa commented Nov 3, 2016

utACK e141beb

@sipa
Member
sipa commented Nov 3, 2016

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

@sipa sipa merged commit e141beb into bitcoin:master Nov 3, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sipa sipa added a commit that referenced this pull request Nov 3, 2016
@sipa sipa Merge #9053: IBD using chainwork instead of height and not using head…
…er timestamps


e141beb IsInitialBlockDownload no longer uses header-only timestamps. (Gregory Maxwell)
2082b55 Remove GetTotalBlocksEstimate and checkpoint tests that test nothing. (Gregory Maxwell)
fd46136 IBD check uses minimumchain work instead of checkpoints. (Gregory Maxwell)
508404d
@laanwj laanwj added this to the 0.13.2 milestone Nov 3, 2016
@laanwj laanwj added Needs backport and removed Needs backport labels Nov 3, 2016
@laanwj laanwj removed this from the 0.13.2 milestone Nov 3, 2016
@jtimon jtimon referenced this pull request Nov 3, 2016
Open

Use a proper factory for creating chainparams #8855

6 of 6 tasks complete
@jtimon
Contributor
jtimon commented Nov 3, 2016 edited

utACK e141beb very clean to read commit by commit.

@jtimon jtimon referenced this pull request Nov 3, 2016
Open

Testchains: Introduce custom chain whose constructor... #8994

3 of 5 tasks complete
- return state;
+ if (chainActive.Tip()->nChainWork < UintToArith256(chainParams.GetConsensus().nMinimumChainWork))
+ return true;
+ if (chainActive.Tip()->GetBlockTime() < (GetTime() - nMaxTipAge))
@mruddy
mruddy Nov 17, 2016 Contributor

Any particular reason to use GetTime here over GetAdjustedTime?

@gmaxwell gmaxwell added a commit to gmaxwell/bitcoin that referenced this pull request Dec 6, 2016
@gmaxwell gmaxwell 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.

Github-Pull: #9053
Rebased-From: fd46136
ad20cdd
@gmaxwell gmaxwell added a commit to gmaxwell/bitcoin that referenced this pull request Dec 6, 2016
@gmaxwell gmaxwell 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.

Github-Pull: #9053
Rebased-From: 2082b55
5b93eee
@gmaxwell gmaxwell added a commit to gmaxwell/bitcoin that referenced this pull request Dec 6, 2016
@gmaxwell gmaxwell IsInitialBlockDownload no longer uses header-only timestamps.
This avoids a corner case (mostly visible on testnet) where bogus
 headers can keep nodes in IsInitialBlockDownload.

Github-Pull: #9053
Rebased-From: e141beb
5998a09
@laanwj laanwj added a commit that referenced this pull request Dec 8, 2016
@laanwj laanwj Merge #9293: [0.13 Backport] IBD using chainwork instead of height an…
…d not using header timestamp (#9053)


5998a09 IsInitialBlockDownload no longer uses header-only timestamps. (Gregory Maxwell)
5b93eee Remove GetTotalBlocksEstimate and checkpoint tests that test nothing. (Gregory Maxwell)
ad20cdd IBD check uses minimumchain work instead of checkpoints. (Gregory Maxwell)
4c71fc4 Remove duplicate nBlocksEstimate cmp (we already checked IsIBD()) (Matt Corallo)
e591c10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment