Decouple checkpoints from Params() #6055
Conversation
|
@jtimon you'll probably be interested in this |
| if (fImporting || fReindex || chainActive.Height() < Checkpoints::GetTotalBlocksEstimate()) | ||
| if (fImporting || fReindex) | ||
| return true; | ||
| if (fCheckpointsEnabled && chainActive.Height() < Checkpoints::GetTotalBlocksEstimate(Params().Checkpoints())) |
There was a problem hiding this comment.
Can you declare a chainparams var at the beginning of the function?
That will make simpler to turn it into a parameter later
|
I like it. utACK apart from the nits. |
|
ACK (it-works test, did not perform a negative test) |
ce0ea10 to
6a1389d
Compare
|
@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. |
| @@ -19,21 +20,22 @@ BOOST_FIXTURE_TEST_SUITE(Checkpoints_tests, BasicTestingSetup) | |||
|
|
|||
| BOOST_AUTO_TEST_CASE(sanity) | |||
| { | |||
| const Checkpoints::CCheckpointData& checkpoints = Params().Checkpoints(); | |||
There was a problem hiding this comment.
Can you replace with the following?
const Checkpoints::CCheckpointData& checkpoints = Params(CBaseChainParams::MAIN).Checkpoints();
|
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. |
6a1389d to
2407135
Compare
|
@jtimon's nit addressed. |
|
utACK |
| @@ -68,7 +68,7 @@ class CChainParams | |||
| const std::vector<CDNSSeedData>& DNSSeeds() const { return vSeeds; } | |||
| const std::vector<unsigned char>& Base58Prefix(Base58Type type) const { return base58Prefixes[type]; } | |||
| const std::vector<CAddress>& FixedSeeds() const { return vFixedSeeds; } | |||
| virtual const Checkpoints::CCheckpointData& Checkpoints() const = 0; | |||
There was a problem hiding this comment.
How did a virtual sneak into here?! In any case, kudos for getting rid of it.
There was a problem hiding this comment.
It had to be. Mainnet and testnet had different implementations for this method.
The result after this PR is obviously better, though.
|
utACK |
|
Needs rebase. |
This drops the virtual call and simplifies the logic
Pass checkpoint data in as necessary
This pertains to app-state, so it doesn't make sense to handle inside the checkpoint functions.
2407135 to
a8cdaf5
Compare
|
rebased after 739d615 |
|
reutACK |
a8cdaf5 checkpoints: move the checkpoints enable boolean into main (Cory Fields) 11982d3 checkpoints: Decouple checkpoints from Params (Cory Fields) 6996823 checkpoints: make checkpoints a member of CChainParams (Cory Fields) 9f13a10 checkpoints: store mapCheckpoints in CCheckpointData rather than a pointer (Cory Fields)
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.