Decouple checkpoints from Params()

theuni commented at 0:47 am on April 24, 2015: member

See individual commit messages for more info.

This moves checkpoints directly into chainparams, then disconnects the checkpoints from Params. In addition, Checkpoints::fEnabled is moved into main as fCheckpointsEnabled because it’s up to the app layer to control that behavior.

All that remains is to parametrize the checkpoints usage of mapBlockIndex, which requires some code shuffling first.

theuni commented at 0:47 am on April 24, 2015: member

@jtimon you’ll probably be interested in this

in src/main.cpp: in 1e253f40c7 outdated

1194@@ -1194,7 +1195,9 @@ CAmount GetBlockValue(int nHeight, const CAmount& nFees)
1195 bool IsInitialBlockDownload()
1196 {
1197     LOCK(cs_main);
1198-    if (fImporting || fReindex || chainActive.Height() < Checkpoints::GetTotalBlocksEstimate())
1199+    if (fImporting || fReindex)
1200+        return true;
1201+    if (fCheckpointsEnabled && chainActive.Height() < Checkpoints::GetTotalBlocksEstimate(Params().Checkpoints()))

jtimon commented at 7:51 am on April 24, 2015:

Can you declare a chainparams var at the beginning of the function? That will make simpler to turn it into a parameter later

in src/main.cpp: in 1e253f40c7 outdated

1926@@ -1924,7 +1927,7 @@ void static UpdateTip(CBlockIndex *pindexNew) {
1927     LogPrintf("%s: new best=%s  height=%d  log2_work=%.8g  tx=%lu  date=%s progress=%f  cache=%u\n", __func__,
1928       chainActive.Tip()->GetBlockHash().ToString(), chainActive.Height(), log(chainActive.Tip()->nChainWork.getdouble())/log(2.0), (unsigned long)chainActive.Tip()->nChainTx,
1929       DateTimeStrFormat("%Y-%m-%d %H:%M:%S", chainActive.Tip()->GetBlockTime()),
1930-      Checkpoints::GuessVerificationProgress(chainActive.Tip()), (unsigned int)pcoinsTip->GetCacheSize());
1931+      Checkpoints::GuessVerificationProgress(Params().Checkpoints(), chainActive.Tip()), (unsigned int)pcoinsTip->GetCacheSize());

jtimon commented at 7:52 am on April 24, 2015:

chainparams var?

in src/main.cpp: in 1e253f40c7 outdated

2220@@ -2218,7 +2221,9 @@ bool ActivateBestChain(CValidationState &state, CBlock *pblock) {
2221         if (!fInitialDownload) {
2222             uint256 hashNewTip = pindexNewTip->GetBlockHash();
2223             // Relay inventory, but don't relay old inventory during initial block download.
2224-            int nBlockEstimate = Checkpoints::GetTotalBlocksEstimate();
2225+            int nBlockEstimate = 0;
2226+            if (fCheckpointsEnabled)
2227+                nBlockEstimate = Checkpoints::GetTotalBlocksEstimate(Params().Checkpoints());

jtimon commented at 7:53 am on April 24, 2015:

chainparams var?

in src/main.cpp: in 1e253f40c7 outdated

2546@@ -2542,9 +2547,9 @@ bool CheckBlock(const CBlock& block, CValidationState& state, bool fCheckPOW, bo
2547 
2548 bool ContextualCheckBlockHeader(const CBlockHeader& block, CValidationState& state, CBlockIndex * const pindexPrev)
2549 {
2550-    const Consensus::Params& consensusParams = Params().GetConsensus();

jtimon commented at 8:01 am on April 24, 2015:

Can you leave the consensusParams there? it will look redundant but it will get fixed after #5975 and #5968. In fact maybe it makes sense for this to wait for #5975 and maybe use the BlockMap& blockIndexMap param

unknown commented at 1:31 am on May 27, 2015:

@jtimon Of course, the loose reference to consensusParams is now causing an unused variable warning. It is not clear to me when you expect this to be fixed.

jtimon commented at 3:28 am on May 27, 2015:

In #5975, which was older than this and is double-linked to this one. In fact, the variable is being used by GetNextWorkRequired and isSuperMajority and I’m not getting that warning…Are you sure you sure you got that warning? Are you on master?

jtimon commented at 3:16 pm on May 27, 2015:

Oh, now I get the warning you’re complaining about, which has nothing to do with this PR. https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L4844 was introduced in https://github.com/theuni/bitcoin/commit/e8e8904d#diff-7ec3c68a81efff79b6ca22ac1f1eabbaR4503 and should have been removed when consensusParams.nPowTargetSpacing was not used in the function anymore. Not sure who’s fault is, but probably whoever did it didn’t noticed while rebasing. Something trivial to fix though.

sdaftuar commented at 5:22 pm on May 27, 2015:

#5976 accidentally introduced an unused variable; I’ve opened #6195 as a fix.

unknown commented at 10:03 pm on May 27, 2015:

@jtimon Yes, that was it. Thanks for the prompt and judicious response. We could all learn from it.

jtimon commented at 10:12 pm on May 27, 2015:

thank you 21E14 for noticing the warning and sdaftuar for realising the mistake fast and fixing it.

in src/main.cpp: in 1e253f40c7 outdated

2556@@ -2552,7 +2557,7 @@ bool ContextualCheckBlockHeader(const CBlockHeader& block, CValidationState& sta
2557     int nHeight = pindexPrev->nHeight+1;
2558 
2559     // Check proof of work
2560-    if (block.nBits != GetNextWorkRequired(pindexPrev, &block, Params().GetConsensus()))
2561+    if (block.nBits != GetNextWorkRequired(pindexPrev, &block, chainParams.GetConsensus()))

jtimon commented at 8:01 am on April 24, 2015:

please use consensusParams or leave it as it is.

in src/main.cpp: in 1e253f40c7 outdated

2587+            return state.DoS(100, error("%s: forked chain older than last checkpoint (height %d)", __func__, nHeight));
2588+    }
2589 
2590     // Reject block.nVersion=1 blocks when 95% (75% on testnet) of the network has upgraded:
2591-    if (block.nVersion < 2 && IsSuperMajority(2, pindexPrev, Params().RejectBlockOutdatedMajority()))
2592+    if (block.nVersion < 2 && IsSuperMajority(2, pindexPrev, chainParams.RejectBlockOutdatedMajority()))

jtimon commented at 8:02 am on April 24, 2015:

please use consensusParams.nMajorityRejectBlockOutdated or leave it as it is.

in src/main.cpp: in 1e253f40c7 outdated

2595                              REJECT_OBSOLETE, "bad-version");
2596     }
2597 
2598     // Reject block.nVersion=2 blocks when 95% (75% on testnet) of the network has upgraded:
2599-    if (block.nVersion < 3 && IsSuperMajority(3, pindexPrev, Params().RejectBlockOutdatedMajority()))
2600+    if (block.nVersion < 3 && IsSuperMajority(3, pindexPrev, chainParams.RejectBlockOutdatedMajority()))

jtimon commented at 8:02 am on April 24, 2015:

please use consensusParams.nMajorityRejectBlockOutdated or leave it as it is.

in src/main.cpp: in 1e253f40c7 outdated

2956@@ -2949,7 +2957,7 @@ bool static LoadBlockIndexDB()
2957     LogPrintf("%s: hashBestChain=%s height=%d date=%s progress=%f\n", __func__,
2958         chainActive.Tip()->GetBlockHash().ToString(), chainActive.Height(),
2959         DateTimeStrFormat("%Y-%m-%d %H:%M:%S", chainActive.Tip()->GetBlockTime()),
2960-        Checkpoints::GuessVerificationProgress(chainActive.Tip()));
2961+        Checkpoints::GuessVerificationProgress(Params().Checkpoints(), chainActive.Tip()));

jtimon commented at 8:03 am on April 24, 2015:

chainparams var?

jtimon commented at 8:05 am on April 24, 2015: contributor

I like it. utACK apart from the nits. This will conflict with #5975, mainly in ContextualCheckBlockHeader. Maybe it makes sense to rebase on top of that?

jgarzik commented at 1:02 pm on April 24, 2015: contributor

ACK (it-works test, did not perform a negative test)

theuni force-pushed on Apr 24, 2015

theuni commented at 4:23 pm on April 24, 2015: member

@jtimon updated to address most of your nits.

Checkpoints::GetLastCheckpoint() has not been updated to use the BlockMap param, because that requires some code shuffling first. BlockMap can’t be forward-declared, so we’d have to include main.h from checkpoints.h. I think it may make sense instead to just move GetLastCheckpoint() into main.

As for rebasing on top of #5975, I really think that kind of PR shuffling only slows things down. This should be a quick rebase for either one of us depending on which gets merged first, but this way neither one is holding up the other.

in src/test/Checkpoints_tests.cpp: in 6a1389d7bb outdated

19@@ -19,21 +20,22 @@ BOOST_FIXTURE_TEST_SUITE(Checkpoints_tests, BasicTestingSetup)
20 
21 BOOST_AUTO_TEST_CASE(sanity)
22 {
23+    const Checkpoints::CCheckpointData& checkpoints = Params().Checkpoints();

jtimon commented at 10:19 am on April 25, 2015:

Can you replace with the following?

0const Checkpoints::CCheckpointData& checkpoints = Params(CBaseChainParams::MAIN).Checkpoints();

jtimon commented at 10:22 am on April 25, 2015: contributor

Well, #5975 exists to accelerate a libconsensus move-only, moving the checkpoint stuff to consensus to later take it out again seems less clear. But fair enough on “neither one is holding up the other”.

utACK apart from that last little nit.

theuni force-pushed on Apr 29, 2015

theuni commented at 1:17 am on April 29, 2015: member

@jtimon’s nit addressed.

sipa commented at 7:36 am on April 30, 2015: member

utACK

in src/chainparams.h: in 2407135381 outdated

67@@ -68,7 +68,7 @@ class CChainParams
68     const std::vector<CDNSSeedData>& DNSSeeds() const { return vSeeds; }
69     const std::vector<unsigned char>& Base58Prefix(Base58Type type) const { return base58Prefixes[type]; }
70     const std::vector<CAddress>& FixedSeeds() const { return vFixedSeeds; }
71-    virtual const Checkpoints::CCheckpointData& Checkpoints() const = 0;

laanwj commented at 12:42 pm on April 30, 2015:

How did a virtual sneak into here?! In any case, kudos for getting rid of it.

sipa commented at 3:50 pm on April 30, 2015:

It had to be. Mainnet and testnet had different implementations for this method.

The result after this PR is obviously better, though.

laanwj commented at 12:43 pm on April 30, 2015: member

utACK

sipa commented at 3:27 pm on April 30, 2015: member

Needs rebase.

checkpoints: store mapCheckpoints in CCheckpointData rather than a pointer 9f13a10548

checkpoints: make checkpoints a member of CChainParams

This drops the virtual call and simplifies the logic

699682304f

checkpoints: Decouple checkpoints from Params

Pass checkpoint data in as necessary

11982d366d

checkpoints: move the checkpoints enable boolean into main

This pertains to app-state, so it doesn't make sense to handle inside the
checkpoint functions.

a8cdaf5c96

theuni force-pushed on May 1, 2015

theuni commented at 3:18 am on May 1, 2015: member

rebased after 739d6155d3bbdeca38bb19daf70e6ff0af022455

sipa commented at 12:43 pm on May 1, 2015: member

reutACK

laanwj added the label Improvement on May 6, 2015

laanwj merged this on May 6, 2015

laanwj closed this on May 6, 2015

laanwj referenced this in commit 00820f921d on May 6, 2015

MarcoFalke locked this on Sep 8, 2021

Decouple checkpoints from Params() #6055