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): Replace a chain length assertion with a NotReadyToBeCommitted error #7072

Merged
merged 1 commit into from
Jun 27, 2023

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Jun 27, 2023

Motivation

The auditors were concerned about the possibility of a panic in a state verification check.

Solution

Replace the assertion with a block verification error.

Review

This is a low-priority audit cleanup.

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?

Follow Up Work

We should change the tests that currently check for success to check for this error instead, and remove the test workaround.

@teor2345 teor2345 added C-cleanup Category: This is a cleanup P-Low ❄️ I-panic Zebra panics with an internal error message A-state Area: State / database changes C-audit Category: Issues arising from audit findings labels Jun 27, 2023
@teor2345 teor2345 self-assigned this Jun 27, 2023
@teor2345 teor2345 requested a review from a team as a code owner June 27, 2023 00:25
@teor2345 teor2345 requested review from arya2 and removed request for a team June 27, 2023 00:25
@github-actions github-actions bot added C-bug Category: This is a bug C-removed labels Jun 27, 2023
@codecov
Copy link

codecov bot commented Jun 27, 2023

Codecov Report

Merging #7072 (804c93a) into main (a6f35af) will increase coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7072      +/-   ##
==========================================
+ Coverage   77.52%   77.54%   +0.01%     
==========================================
  Files         310      310              
  Lines       41694    41694              
==========================================
+ Hits        32325    32330       +5     
+ Misses       9369     9364       -5     

Copy link
Contributor

@dconnolly dconnolly left a comment

Choose a reason for hiding this comment

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

lgtm!

mergify bot added a commit that referenced this pull request Jun 27, 2023
@mergify mergify bot merged commit 941be29 into main Jun 27, 2023
@mergify mergify bot deleted the remove-difficulty-assert branch June 27, 2023 06:50
@teor2345 teor2345 mentioned this pull request Jun 28, 2023
44 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-audit Category: Issues arising from audit findings C-bug Category: This is a bug C-cleanup Category: This is a cleanup I-panic Zebra panics with an internal error message
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants