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

Restore the previous non-finalized chain if a block is invalid #2478

Merged
merged 1 commit into from
Jul 9, 2021

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Jul 9, 2021

Motivation

  1. In PR Refactor: Return errors from state update methods to prepare for contextual validation #2417, we started returning errors from the non-finalized state UpdateWith methods.

But when there is an error, we implicitly drop the full non-finalized chain, and reset the state to the finalized tip. This is surprising, and might cause bugs in future. It might also make the code hard to test.

(And throwing away 100 blocks is pretty inefficient, too.)

  1. The mandatory checkpoint check only applies to existing chains, which is inconsistent. It could be a source of bugs, and it makes testing harder.

Solution

  • Restore the parent chain when a block update fails
  • Change the Chain.push method to make it easier to use correctly
  • Move the mandatory checkpoint check to validate_and_commit, so it applies to new chains and existing chains

Review

@conradoplg and I worked on the original PR together. This fix conflicts with a bunch of upcoming state PRs, so we should try to get it in soon.

These upcoming PRs will also add tests for this fix.

Reviewer Checklist

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

Previously, we would implicitly drop the full non-finalized chain,
and reset the state to the finalized tip.
@teor2345 teor2345 added C-bug Category: This is a bug A-rust Area: Updates to Rust code P-Medium I-slow Problems with performance or responsiveness I-usability Zebra is hard to understand or use labels Jul 9, 2021
@teor2345 teor2345 added this to the 2021 Sprint 13 milestone Jul 9, 2021
@teor2345 teor2345 requested a review from conradoplg July 9, 2021 08:19
@teor2345 teor2345 self-assigned this Jul 9, 2021
Copy link
Collaborator

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

@conradoplg conradoplg merged commit 6d24ee1 into main Jul 9, 2021
@conradoplg conradoplg deleted the restore-chain-on-error branch July 9, 2021 14:22
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 C-bug Category: This is a bug I-slow Problems with performance or responsiveness I-usability Zebra is hard to understand or use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants