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.

Sign up for GitHub

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

Jump to bottom

fix(state): Avoid panicking in zebra-state initialization when checkpoints are missing #7770

Closed
wants to merge 2 commits into from
Closed

fix(state): Avoid panicking in zebra-state initialization when checkpoints are missing #7770

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
92def54
avoids panic when checkpoints are missing
arya2 Oct 18, 2023
fd50038
Merge branch 'main' into fix-no-checkpoint-panic
teor2345 Oct 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions zebra-state/src/service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,8 +390,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));
Copy link
Collaborator

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.

Copy link
Collaborator

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).


let non_finalized_state_queued_blocks = QueuedBlocks::default();
let pending_utxos = PendingUtxos::default();
Expand Down
Loading