Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

multi: Rework old fork rejection logic. #2945

Merged
merged 5 commits into from May 19, 2022

Conversation

davecgh
Copy link
Member

@davecgh davecgh commented May 10, 2022

This requires #2942 and #2943.

Historically, checkpoints have been manually specified and used both for various optimizations and to protect against cheap DoS attacks by rejecting forks deep in history. The primary motivation for manually specifying the checkpoints was said optimizations, however, those optimization portions have now been decoupled from checkpoints in favor of other methods, so the only remaining use of checkpoints is the aforementioned rejection of forks deep in history.

Thus, this reworks the old fork rejection logic to make it automatic based on an ancestor that is two weeks worth of blocks behind the hard-coded assumevalid block (clamped to the genesis block if necessary) that is manually specified at each release as opposed to needing to additionally manually specify checkpoints.

Note the distinction here between the hard-coded assumevalid block specified at each release and the potentially overridden value specified by a user via CLI configuration options. This is an important distinction because old fork rejection affects consensus and thus must not move around based on user configuration.

This approach means the user can still specify a more recent assume valid hash or disable assume valid optimizations independently without affecting the consensus logic while still removing the need to manually specify additional checkpoints.

The changes have been split into multiple commits to help ease review.

The following is a high level overview of the changes:

  • Removes the findcheckpoints utility
  • Caches the number of expected blocks in two weeks for the network to avoid repeated calculation
  • Modifies the blockchain.Config struct as follows:
    • Removes the LastestCheckpoint option
    • Adds a AllowOldForks option
  • Removes all code related to manually specified checkpoints and finding candidates
  • Removes the following errors since they are no longer used:
    • ErrBadCheckpoint
    • ErrCheckpointTimeTooOld
  • Renames checkpointNode to rejectForksCheckpoint to make it clear that is the only thing it applies to
    • Adds logic to potentially reject forks deep in history as follows:
      • Do not reject old forks if they are manually allowed or the hard-coded assumevalid block is not specified for the network
      • Automatically choose an ancestor that is two weeks worth of blocks prior to the hard-coded assumevalid block (clamped to the genesis block if necessary)
  • Replaces the --nocheckpoints CLI option with --allowoldforks
    • Updates doc.go to match the changes to the CLI options
  • Deprecates the following:
    • chaincfg.LatestCheckpointHeight
    • chaincfg.Checkpoints
  • Removes all chaincfg.Checkpoints entries since they are no longer used

@davecgh davecgh added non-forking consensus Changes that involve modifying consensus code without causing any forking changes. cli flag change Issues and/or pull requests that involve a change to the available CLI flags. labels May 10, 2022
@davecgh davecgh added this to the 1.8.0 milestone May 10, 2022
@davecgh davecgh force-pushed the multi_rework_old_fork_rejection branch 2 times, most recently from 67a452e to ca75fea Compare May 13, 2022 07:23
Copy link
Member

@rstaudt2 rstaudt2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. It's nice to consolidate this logic and not deal with both checkpoints and assume valid separately. I also think referring to the concept as allowing old forks is more self-explanatory than calling it checkpoints.

I don't see any logic issues and ran a few full syncs on mainnet without issue with various combinations of --allowoldforks and --assumevalid options.

blockchain/chain.go Outdated Show resolved Hide resolved
// Calculate the expected number of blocks in 2 weeks and cache it in order
// to avoid repeated calculation.
const timeInTwoWeeks = time.Hour * 24 * 14
expectedBlksInTwoWeeks := int64(timeInTwoWeeks / params.TargetTimePerBlock)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, good call to cache this.

server.go Outdated Show resolved Hide resolved
chaincfg/params.go Show resolved Hide resolved
@davecgh davecgh force-pushed the multi_rework_old_fork_rejection branch 3 times, most recently from 1c50347 to e01c1fc Compare May 15, 2022 19:54
chaincfg/params.go Outdated Show resolved Hide resolved
@davecgh davecgh force-pushed the multi_rework_old_fork_rejection branch from e01c1fc to 185df25 Compare May 16, 2022 04:49
This utility is no longer be necessary since the manual checkpoint logic
is being reworked in upcoming commits.
This adds a new field to the blockchain instance to store a cached
version of the number of expected blocks in two weeks to avoid repeated
calculation.
Historically, checkpoints have been manually specified and used both for
various optimizations and to protect against cheap DoS attacks by
rejecting forks deep in history.  The primary motivation for manually
specifying the checkpoints was said optimizations, however, those
optimization portions have now been decoupled from checkpoints in favor
of other methods, so the only remaining use of checkpoints is the
aforementioned rejection of forks deep in history.

Thus, this reworks the old fork rejection logic to make it automatic
based on an ancestor that is two weeks worth of blocks behind the
hard-coded assumevalid block (clamped to the genesis block if necessary)
that is manually specified at each release as opposed to needing to
additionally manually specify checkpoints.

Note the distinction here between the hard-coded assumevalid block
specified at each release and the potentially overridden value specified
by a user via CLI configuration options.  This is an important
distinction because old fork rejection affects consensus and thus must
not move around based on user configuration.

This approach means the user can still specify a more recent assume
valid hash or disable assume valid optimizations independently without
affecting the consensus logic while still removing the need to manually
specify additional checkpoints.

The following is a high level overview of the changes:
- Modifies the blockchain.Config struct as follows:
  - Removes the LastestCheckpoint option
  - Adds a AllowOldForks option
- Removes all code related to manually specified checkpoints and finding
  candidates
- Removes the following errors since they are no longer used:
  - ErrBadCheckpoint
  - ErrCheckpointTimeTooOld
- Renames checkpointNode to rejectForksCheckpoint to make it clear that
  is the only thing it applies to
- Adds logic to potentially reject forks deep in history as follows:
  - Do not reject old forks if they are manually allowed or the
    hard-coded assumevalid block is not specified for the network
  - Automatically choose an ancestor that is two weeks worth of blocks
    prior to the hard-coded assumevalid block (clamped to the genesis
    block if necessary)
- Replaces --nocheckpoints CLI option with --allowoldforks
  - Updates doc.go to match the changes to the CLI options
This method is no longer used since manual checkpoints no longer exist.
This deprecates the Checkpoints field of the chain parameters and
removes all manually-specified entries for all networks since manual
checkpoints no longer exist.
@davecgh davecgh force-pushed the multi_rework_old_fork_rejection branch from 185df25 to 55ac703 Compare May 19, 2022 18:05
@davecgh davecgh merged commit 55ac703 into decred:master May 19, 2022
@davecgh davecgh deleted the multi_rework_old_fork_rejection branch May 19, 2022 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli flag change Issues and/or pull requests that involve a change to the available CLI flags. non-forking consensus Changes that involve modifying consensus code without causing any forking changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants