-
Notifications
You must be signed in to change notification settings - Fork 100
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
fix(state): Avoid panicking in zebra-state initialization when checkpoints are missing #7770
Conversation
@@ -389,8 +389,7 @@ impl StateService { | |||
let full_verifier_utxo_lookahead = max_checkpoint_height | |||
- HeightDiff::try_from(checkpoint_verify_concurrency_limit) | |||
.expect("fits in HeightDiff"); | |||
let full_verifier_utxo_lookahead = | |||
full_verifier_utxo_lookahead.expect("unexpected negative height"); | |||
let full_verifier_utxo_lookahead = full_verifier_utxo_lookahead.unwrap_or(block::Height(0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Zebra should not panic without checkpoints.
I think this code change is helpful, but the explanation isn't correct.
Zebra must have checkpoints up to mandatory_checkpoint_height()
. If it doesn't, then it could break consensus rules:
zebra/zebra-chain/src/parameters/network.rs
Lines 96 to 102 in 290ccf2
/// Get the mandatory minimum checkpoint height for this network. | |
/// | |
/// Mandatory checkpoints are a Zebra-specific feature. | |
/// If a Zcash consensus rule only applies before the mandatory checkpoint, | |
/// Zebra can skip validation of that rule. | |
pub fn mandatory_checkpoint_height(&self) -> Height { | |
// Currently this is after the ZIP-212 grace period. |
Is there a specific situation or bug that this change fixes?
That might be helpful context to put in the PR description.
@@ -389,8 +389,7 @@ impl StateService { | |||
let full_verifier_utxo_lookahead = max_checkpoint_height | |||
- HeightDiff::try_from(checkpoint_verify_concurrency_limit) | |||
.expect("fits in HeightDiff"); | |||
let full_verifier_utxo_lookahead = | |||
full_verifier_utxo_lookahead.expect("unexpected negative height"); | |||
let full_verifier_utxo_lookahead = full_verifier_utxo_lookahead.unwrap_or(block::Height(0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, there is a significant amount of consensus code that isn't implemented due to the mandatory checkpoint, including:
- difficulty adjustment for heights less than 17
- slow start block subsidy
- activating NU5 or NU6 at genesis or block 1 (which allows transactions v4,v5,v6)
- contextual validation of the genesis block (it has to be a checkpoint)
If the eventual goal of this PR series is to make all those things work, then this list should become a tracking issue. Until we do all those things, Zebra won't be compatible with zcashd
in an alternate testnet.
However, if the eventual goal is to test ZSAs, then forking the current testnet chain by adding NU6 at a nearby height might be more reliable (and much faster to implement, although slower to sync).
Would it be worth setting up a call with them to discuss potential approaches? Seems like this is a problem that could benefit from some brainstorming with the whole team. Also, if we need to do anything on our side we should plan for it and schedule accordingly :) |
Yes, I think a good next step is to check what their goals actually are, then offer them some alternatives. Until we know that, it's tricky to plan for these kinds of changes. |
Are there any next steps for this PR or are we just closing without merging? |
Let's close without merging and revisit this if needed. |
The next step is a meeting or discussion with the developers working on ZSAs, so we can help them work through their testing needs for their Zebra integration. |
Motivation
Zebra should not panic without checkpoints.
Qedit asked about how to set up a local testnet with a different genesis block and a different network ID for local testing, one approach would be to modify the
genesis_hash()
return value forTestnet
and clear initial/cached testnet peers.Solution
(or when the
max_checkpoint_height
is lower thancheckpoint_verify_concurrency_limit
)Review
Anyone can review.
Reviewer Checklist