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

Add configuration option for checkpoint limit. #911

Closed
5 tasks done
hdevalence opened this issue Aug 17, 2020 · 4 comments · Fixed by #931
Closed
5 tasks done

Add configuration option for checkpoint limit. #911

hdevalence opened this issue Aug 17, 2020 · 4 comments · Fixed by #931
Assignees
Labels
A-rust Area: Updates to Rust code

Comments

@hdevalence
Copy link
Contributor

hdevalence commented Aug 17, 2020

We require checkpointing up to Sapling activation (419200 on mainnet), but our checkpoint list extends beyond Sapling activation in order to allow fast sync. We need to provide a way for users to configure whether Zebra performs full verification post-Sapling, or uses checkpointing.

One option would be to provide a configuration field that specifies the block height to stop checkpointing. However, we then have to handle bounds on the configured height, alignment with the checkpoint list, etc. And it's not clear that this configurability is really required -- there's a use for full verification, and a use for checkpointing, but if users are willing to checkpoint as a fast sync, there's no reason to stop early.

A simpler alternative is to provide a checkpoint_sync: bool field, defaulting to false. If the field is set to false, Zebra stops checkpointing at the first checkpoint above Sapling activation. If the field is set to true, Zebra stops checkpointing at the last checkpoint in the hardcoded checkpoint list.

This field could either be in the ZebradConfig structure or in a zebra_consensus::Config structure included as a separate consensus section. Although I don't expect that there will be other consensus configuration options (they are consensus parameters), I think it is cleaner to have a section of 1 option rather than have a standalone option while all other options are in sections. This config structure should be passed to the chain verifier.

  • Create zebra_consensus::Config
  • Add checkpoint_sync: bool with Defaulting to false;
  • Ensure the Config struct allows missing fields (cf Make the state Config load unspecified fields from defaults #890);
  • Pass the Config to the chain verifier;
  • Use the field to prune the checkpoint list before constructing the chain verifier's checkpoint verifier.
@hdevalence hdevalence added E-easy A-rust Area: Updates to Rust code labels Aug 17, 2020
@hdevalence hdevalence added this to the Validate transactions. milestone Aug 17, 2020
@teor2345 teor2345 self-assigned this Aug 20, 2020
@teor2345
Copy link
Contributor

* Add `checkpoint_sync: bool` with `Default`ing to `false`;

@hdevalence what are the reasons behind this default?

I think most users will want to sync quickly, rather than running a full verification every time.

@teor2345
Copy link
Contributor

Tweaked the description to include testnet Sapling activation.

@hdevalence
Copy link
Contributor Author

@teor2345 well, the thought was that verifying the chain data increases the security of the network, so it should be the default, but I don't have strong feelings about it. Changing a default is easy.

@teor2345
Copy link
Contributor

teor2345 commented Aug 20, 2020

I think you're right.

If we do a full verify as the default, then we increase the security of the network, and we lower the severity of any security issues introduced by checkpointing.

I'd much rather be dealing with security issues that don't happen in the default configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust Area: Updates to Rust code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants