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

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

Closed
wants to merge 2 commits into from

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Oct 18, 2023

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 for Testnet and clear initial/cached testnet peers.

Solution

  • Always store queued UTXOs when there are no checkpoints
    (or when the max_checkpoint_height is lower than checkpoint_verify_concurrency_limit)

Review

Anyone can review.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

@arya2 arya2 added C-bug Category: This is a bug P-Medium ⚡ A-state Area: State / database changes C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels Oct 18, 2023
@arya2 arya2 self-assigned this Oct 18, 2023
@arya2 arya2 requested a review from a team as a code owner October 18, 2023 18:45
@arya2 arya2 requested review from teor2345 and removed request for a team October 18, 2023 18:45
@@ -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));
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:

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

@mpguerra
Copy link
Contributor

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

@teor2345
Copy link
Collaborator

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.

@arya2 arya2 changed the title fix(state): Avoid panicking when checkpoints are missing fix(state): Avoid panicking in zebra-state initialization when checkpoints are missing Oct 23, 2023
@mpguerra
Copy link
Contributor

Are there any next steps for this PR or are we just closing without merging?

@arya2
Copy link
Contributor Author

arya2 commented Oct 24, 2023

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.

@arya2 arya2 closed this Oct 24, 2023
@teor2345
Copy link
Collaborator

teor2345 commented Oct 24, 2023

Are there any next steps for this PR or are we just closing without merging?

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.

@teor2345 teor2345 mentioned this pull request Oct 25, 2023
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-state Area: State / database changes C-bug Category: This is a bug C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants