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): Remove an outdated anchor assertion in a comment #6450

Merged
merged 1 commit into from
Apr 3, 2023

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Apr 3, 2023

Motivation

There is an outdated assertion in the state Sprout anchor checking code, which has already been commented out.

This assertion could be expensive, so we can only do it during tests.

Close #6389

Specifications

This assertion is not consensus-critical, the existing production code already constructs the anchor map correctly.

Solution

  • Remove the commented-out assertion
  • Update the TODO to only apply to tests

Review

This is an audit fix, it should be reviewed first.

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 could fix the tests, but it's not important at all.

@teor2345 teor2345 added C-cleanup Category: This is a cleanup P-Medium ⚡ A-state Area: State / database changes C-audit Category: Issues arising from audit findings C-tech-debt Category: Code maintainability issues labels Apr 3, 2023
@teor2345 teor2345 requested a review from a team as a code owner April 3, 2023 06:44
@teor2345 teor2345 self-assigned this Apr 3, 2023
@teor2345 teor2345 requested review from oxarbitrage and removed request for a team April 3, 2023 06:44
@github-actions github-actions bot added C-bug Category: This is a bug C-removed labels Apr 3, 2023
@codecov
Copy link

codecov bot commented Apr 3, 2023

Codecov Report

Merging #6450 (59de880) into main (6795656) will decrease coverage by 0.07%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6450      +/-   ##
==========================================
- Coverage   77.82%   77.76%   -0.07%     
==========================================
  Files         304      304              
  Lines       39637    39637              
==========================================
- Hits        30848    30824      -24     
- Misses       8789     8813      +24     

Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

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

This looks good!

zebra-state/src/service/check/anchors.rs Show resolved Hide resolved
mergify bot added a commit that referenced this pull request Apr 3, 2023
@mergify mergify bot merged commit f556c4a into main Apr 3, 2023
@mergify mergify bot deleted the anchor-assertion branch April 3, 2023 23:20
@mpguerra mpguerra mentioned this pull request Apr 12, 2023
36 tasks
@arya2 arya2 mentioned this pull request Apr 18, 2023
36 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 C-tech-debt Category: Code maintainability issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Re-enable or delete the assertion in fetch_sprout_final_treestates()
2 participants