help: Generate checkpoint height from chainparams #19241

luke-jr commented at 11:59 PM on June 10, 2020: member

Not sure if this is worth putting in Core, but might as well until checkpoints are removed entirely.

laanwj commented at 5:13 PM on June 11, 2020: member

ACK 6ba3f6d49e09024af9bce7a184aaaf6caf766c40 re-ACK 916d3596c493fec44da86aeb92b61eafeea0b596 This is preferable to hardcoding the value in the string.

laanwj added the label Docs on Jun 11, 2020

in src/init.cpp:485 in 6ba3f6d49e outdated

 481 | @@ -482,7 +482,7 @@ void SetupServerArgs()
 482 |          "(0-4, default: %u)", DEFAULT_CHECKLEVEL), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
 483 |      gArgs.AddArg("-checkblockindex", strprintf("Do a consistency check for the block tree, chainstate, and other validation data structures occasionally. (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
 484 |      gArgs.AddArg("-checkmempool=<n>", strprintf("Run checks every <n> transactions (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
 485 | -    gArgs.AddArg("-checkpoints", strprintf("Enable rejection of any forks from the known historical chain until block 295000 (default: %u)", DEFAULT_CHECKPOINTS_ENABLED), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
 486 | +    gArgs.AddArg("-checkpoints", strprintf("Enable rejection of any forks from the known historical chain until block %s (default: %s)", defaultChainParams->Checkpoints().mapCheckpoints.rbegin()->first, DEFAULT_CHECKPOINTS_ENABLED), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);

donaloconnor commented at 8:54 AM on June 12, 2020:

I'd prefer adding a function to CCheckpointData to obtain this rather than exposing the map details here.

laanwj commented at 10:43 AM on June 12, 2020:

Good idea.

DrahtBot commented at 1:47 PM on July 21, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17783 (util: Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior by ryanofsky)
#17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Needs rebase on Jul 30, 2020

luke-jr commented at 11:10 PM on August 19, 2020: member

Abstracted checkpoint access and rebased

luke-jr force-pushed on Aug 19, 2020

sipa commented at 12:20 AM on August 20, 2020: member

utACK 40ef5d4100c575db35fa736857d41a7c0c7df6de

DrahtBot removed the label Needs rebase on Aug 20, 2020

donaloconnor commented at 7:44 AM on August 20, 2020: contributor

utACK https://github.com/bitcoin/bitcoin/commit/40ef5d4100c575db35fa736857d41a7c0c7df6de

in src/init.cpp:509 in 40ef5d4100 outdated

 505 | @@ -506,7 +506,7 @@ void SetupServerArgs(NodeContext& node)
 506 |      argsman.AddArg("-checklevel=<n>", strprintf("How thorough the block verification of -checkblocks is: %s (0-4, default: %u)", Join(CHECKLEVEL_DOC, ", "), DEFAULT_CHECKLEVEL), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
 507 |      argsman.AddArg("-checkblockindex", strprintf("Do a consistency check for the block tree, chainstate, and other validation data structures occasionally. (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
 508 |      argsman.AddArg("-checkmempool=<n>", strprintf("Run checks every <n> transactions (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
 509 | -    argsman.AddArg("-checkpoints", strprintf("Enable rejection of any forks from the known historical chain until block 295000 (default: %u)", DEFAULT_CHECKPOINTS_ENABLED), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
 510 | +    argsman.AddArg("-checkpoints", strprintf("Enable rejection of any forks from the known historical chain until block %s (default: %s)", defaultChainParams->Checkpoints().GetHeight(), DEFAULT_CHECKPOINTS_ENABLED), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);

MarcoFalke commented at 4:24 PM on August 20, 2020:

boolean values represented as strings are not supported by the argparser. We shouldn't imply that nor should this method be changed to be inconsistent with all other boolean default values

luke-jr commented at 6:20 PM on August 20, 2020:

Fixed

MarcoFalke changes_requested

help: Generate checkpoint height from chainparams 916d3596c4

luke-jr force-pushed on Aug 20, 2020

laanwj merged this on Sep 15, 2020

laanwj closed this on Sep 15, 2020

sidhujag referenced this in commit 09111fe349 on Sep 15, 2020

in src/chainparams.h:29 in 916d3596c4

  22 | @@ -23,6 +23,11 @@ typedef std::map<int, uint256> MapCheckpoints;
  23 |  
  24 |  struct CCheckpointData {
  25 |      MapCheckpoints mapCheckpoints;
  26 | +
  27 | +    int GetHeight() const {
  28 | +        const auto& final_checkpoint = mapCheckpoints.rbegin();
  29 | +        return final_checkpoint->first /* height */;

jnewbery commented at 2:42 PM on September 15, 2020:

I think ideally, any begin/end/rbegin/rend iterator should be tested before deferencing:

return final_checkpoint == mapCheckpoints.rend() ? 0 : final_checkpoint->first;

or similar.

DrahtBot locked this on Feb 15, 2022