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

When a parent block is rejected, also reject its children #2479

Merged
merged 2 commits into from
Jul 13, 2021

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Jul 9, 2021

Motivation

After Zebra rejects a queued non-finalized block, it keeps on processing that block's children, as if their parent was valid.

This might cause panics or hangs, or break consensus rules in the non-finalized state.

I'm not sure if we should merge this fix, leave it for later, or turn it into a ticket.

Equivalent zcashd Behaviour

zcashd implements a similar "drop all children of invalid blocks" behaviour:

Pruned nodes may have entries in setBlockIndexCandidates for
which block files have been deleted. Remove those as candidates
for the most work chain if we come across them; we can't switch
to a chain unless we have all the non-active-chain parent blocks.

https://github.com/zcash/zcash/blob/c34162d6ddfaa8d6276aa0628cee838a51ad9df7/src/main.cpp#L3905

See also BLOCK_FAILED_CHILD:

BLOCK_FAILED_CHILD = 64, //! descends from failed block

https://github.com/zcash/zcash/blob/785803382a27c85301bc75b34576c3704966fe87/src/chain.h#L176

And its uses:
https://github.com/zcash/zcash/blob/c34162d6ddfaa8d6276aa0628cee838a51ad9df7/src/main.cpp#L3919
https://github.com/zcash/zcash/blob/c34162d6ddfaa8d6276aa0628cee838a51ad9df7/src/main.cpp#L4144

Designs

Non-finalized state validation:
https://github.com/ZcashFoundation/zebra/blob/main/book/src/dev/rfcs/0005-state-updates.md#pubsuper-fn-queue_and_commit_non_finalized_blocksmut-self-new-arcblock---tokiosynconeshotreceiverblockhash

Solution

  • When a parent block errors, reject all descendant blocks

Review

Do you think we should merge this fix, leave it for later, or turn it into a ticket?

We might add tests for this error case as part of upcoming PRs.
We could hold this PR until it has tests.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

@teor2345 teor2345 added C-bug Category: This is a bug A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code S-needs-triage Status: A bug report needs triage I-consensus Zebra breaks a Zcash consensus rule I-panic Zebra panics with an internal error message I-hang A Zebra component stops responding to requests labels Jul 9, 2021
@teor2345 teor2345 added this to the 2021 Sprint 13 milestone Jul 9, 2021
@teor2345 teor2345 self-assigned this Jul 9, 2021
@teor2345 teor2345 requested a review from oxarbitrage July 12, 2021 02:35
@teor2345 teor2345 marked this pull request as ready for review July 12, 2021 02:39
@teor2345 teor2345 force-pushed the reject-child-blocks branch from 288ea66 to e26862e Compare July 13, 2021 00:41
@teor2345 teor2345 merged commit 6676eb9 into main Jul 13, 2021
@teor2345 teor2345 deleted the reject-child-blocks branch July 13, 2021 23:12
@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code C-bug Category: This is a bug I-consensus Zebra breaks a Zcash consensus rule I-hang A Zebra component stops responding to requests I-panic Zebra panics with an internal error message
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants