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

Optionally support lightweight checkpointing #6048

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

amesgen
Copy link
Member

@amesgen amesgen commented Dec 4, 2024

Description

Closes #5730

This PR adds optional support for lightweight checkpointing. Concretely, a file can contain a list of checkpoints (each consisting of a block number and a corresponding block hash). When validating a header/block with a block number with a corresponding checkpoint, we consider the header/block to be invalid if their actual hash does not coincide with the hash from the checkpoint.

This is only expected to be used in certain disaster recovery scenarios, so ideally never. See CIP-0135 for more details.

Warning

Specifying a checkpoints file requires great care; incorrect entries can lead the node to be stuck on adversarial chains forever!

Concretely, the node configuration file has two new optional entries:

CheckpointsFile: "/path/to/checkpoints.json"
CheckpointsFileHash: "a71c47262163947daaefb6aa1112acf34cb5ded6841d51e27dd642eb2de355a3"

The checkpoints.json file has the following format:

{
  "checkpoints": [
    {"blockNo": 3, "hash": "52b7912de176ab76c233d6e08ccdece53ac1863c08cc59d3c5dec8d924d9b536"}
  ]
}

Testing

Component-level tests for this feature already exist in Consensus, but end-to-end tests might still be useful. I manually tested this PR locally, but a test in CI would be nice. I am not familiar with the various tests in this repo; maybe a test under Cardano.Testnet.Test could work?

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. These may include:
    • golden tests
    • property tests
    • roundtrip tests
    • integration tests
      See Runnings tests for more details
  • Any changes are noted in the CHANGELOG.md for affected package
  • The version bounds in .cabal files are updated
  • CI passes. See note on CI. The following CI checks are required:
    • Code is linted with hlint. See .github/workflows/check-hlint.yml to get the hlint version
    • Code is formatted with stylish-haskell. See .github/workflows/stylish-haskell.yml to get the stylish-haskell version
    • Code builds on Linux, MacOS and Windows for ghc-8.10.7 and ghc-9.2.7
  • Self-reviewed the diff

Note on CI

If your PR is from a fork, the necessary CI jobs won't trigger automatically for security reasons.
You will need to get someone with write privileges. Please contact IOG node developers to do this
for you.

@amesgen amesgen requested review from a team as code owners December 4, 2024 18:36
@amesgen amesgen self-assigned this Dec 4, 2024
@amesgen amesgen force-pushed the amesgen/lightweight-checkpointing branch from 7323965 to 7aceb49 Compare December 4, 2024 18:40
Copy link
Contributor

@nfrisby nfrisby left a comment

Choose a reason for hiding this comment

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

This all looks great to me, thanks. I'm not Approving since I'm not a code reviewer for this repository.


pure $ WrapCheckpointsMap $ CheckpointsMap $ Map.fromList checkpoints
where
parseCardanoHash
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe some genesis hash config field parser already has this logic somewhere?

parseJSON = Aeson.withObject "CheckpointsMap" $ \o -> do
checkpointList :: [Aeson.Object] <- o Aeson..: "checkpoints"

checkpoints :: [(BlockNo, HeaderHash (CardanoBlock StandardCrypto))] <-
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I'd consider a {blockNoHashPairs: [[3, "abcdef"], [100, "32cdef"]]} format, to avoid the obnoxious per-entry repetition of the field names. But I don't have a lot of experience regarding the widely-expected UX for this sort of thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah 👍, I have no strong opinions here; one tiny advantage of the current format is that it is impossible to forgot that the numbers are referring to block numbers (and not e.g. slot numbers), even when looking at individual checkpoints only; but that is not a major thing.

@amesgen amesgen force-pushed the amesgen/lightweight-checkpointing branch from 7aceb49 to b65350a Compare December 4, 2024 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

[FR] - Add support for reading checkpoint data
2 participants