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.

Sign up for GitHub

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

Jump to bottom

can_fork_chain_at() should ignore blocks below the finalized tip #6388

Closed
teor2345 opened this issue Mar 22, 2023 · 5 comments · Fixed by #6810
Closed

can_fork_chain_at() should ignore blocks below the finalized tip #6388

teor2345 opened this issue Mar 22, 2023 · 5 comments · Fixed by #6810
Assignees
@arya2
Labels
A-consensus Area: Consensus rule updates A-state Area: State / database changes C-cleanup Category: This is a cleanup C-tech-debt Category: Code maintainability issues I-heavy Problems with excessive memory, disk, or CPU usage S-needs-triage Status: A bug report needs triage

Comments

@teor2345
Copy link
Contributor

teor2345 commented Mar 22, 2023 •
edited
Loading

Motivation

We can't fork the chain at a finalized block height below the finalized tip height, but the sent_non_finalized_blocks data structure contains the heights of some finalized blocks close to the final checkpoint.

We might need to store the queued finalized block list separately, rather than re-using an existing data structure that other code depends on.

(This is an existing bug, but the code in the linked audit PR added more finalized blocks to the data structure.)

Originally posted by @teor2345 in #6335 (comment)

Scheduling

We reviewed this code, and checked that even if it was called incorrectly, the write block task would return a similar error. So this is just a technical debt cleanup.
@mpguerra mpguerra added this to Zebra Mar 22, 2023
@github-project-automation github-project-automation bot moved this to 🆕 New in Zebra Mar 22, 2023
@teor2345 teor2345 added A-consensus Area: Consensus rule updates A-state Area: State / database changes A-concurrency Area: Async code, needs extra work to make it work properly. C-audit Category: Issues arising from audit findings P-Medium ⚡ C-security Category: Security issues I-hang A Zebra component stops responding to requests I-slow Problems with performance or responsiveness labels Mar 22, 2023
@mpguerra
Copy link
Contributor

Whilst it seems like we want to fix it, I don't think we should classify this as an audit fix...

The reason why we were working on #6335 to close #862 was not because #862 was identified as a potential issue from the audit. Instead, #862 was flagged as an issue that was mentioned in a TODO comment but was closed, potentially making the TODO comment redundant...

We should finish off #6335 and #862 since we have already started work on them, however I would like to prioritise actual audit issues first and fix these other items before a stable release.

@mpguerra mpguerra added S-needs-triage Status: A bug report needs triage and removed C-audit Category: Issues arising from audit findings labels Mar 23, 2023
@teor2345 teor2345 added I-heavy Problems with excessive memory, disk, or CPU usage and removed I-slow Problems with performance or responsiveness labels Mar 23, 2023
@teor2345
Copy link
Contributor Author

teor2345 commented Mar 23, 2023 •
edited
Loading

I just checked the code that validates and writes non-finalized blocks, it will correctly return a NotReadyToBeCommitted error without panicking, even if it sends a block based on these incorrect block hashes. So this is mainly a performance and maintainability issue.

The code that writes finalized blocks ignores the incorrect block hashes, it just uses a single most recently sent hash.

@teor2345
Copy link
Contributor Author

We've actually seen this issue in production, so this is a medium priority.

@mpguerra
Copy link
Contributor

Hey team! Please add your planning poker estimate with Zenhub @arya2 @conradoplg @dconnolly @oxarbitrage @teor2345 @upbqdn

@mpguerra mpguerra mentioned this issue May 10, 2023
Closed
9 tasks
@teor2345 teor2345 added C-cleanup Category: This is a cleanup P-Low ❄️ C-tech-debt Category: Code maintainability issues and removed P-Medium ⚡ C-security Category: Security issues I-hang A Zebra component stops responding to requests A-concurrency Area: Async code, needs extra work to make it work properly. labels May 18, 2023
@teor2345
Copy link
Contributor Author

I just remembered that we reviewed this code, and checked that even if it was called incorrectly, the write block task would return a similar error.

So @Pili I don't think it's a blocker for the stable release at all, and I think it's actually a low priority.

@arya2 arya2 self-assigned this Jun 2, 2023
@arya2 arya2 mentioned this issue Jun 2, 2023
Merged
6 tasks
@arya2 arya2 mentioned this issue Jul 3, 2023
Merged
6 tasks
arya2 added a commit that referenced this issue Jul 25, 2023
@arya2
Fix #6388, rename sent_hashes field
6ffccd5
@mergify mergify bot closed this as completed in #6810 Aug 21, 2023
mergify bot added a commit that referenced this issue Aug 21, 2023
@arya2 @mergify
fix(state): Avoid temporary failures verifying the first non-finalize…
b413e3e 
…d block or attempting to fork the chain before the final checkpoint (#6810)

* Fix #6388, rename sent_hashes field

* Removes prune_by_height, uses new SentHashes instead

* update queue_and_commit_to_non_finalized_state to start with children of non-finalized tip when dropping the finalized block write sender

* revert rename for now

* removes outdated TODO

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
@github-project-automation github-project-automation bot moved this from 🆕 New to ✅ Done in Zebra Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@arya2 arya2

Labels
A-consensus Area: Consensus rule updates A-state Area: State / database changes C-cleanup Category: This is a cleanup C-tech-debt Category: Code maintainability issues I-heavy Problems with excessive memory, disk, or CPU usage S-needs-triage Status: A bug report needs triage
Projects
Zebra
Archived in project
Milestone
No milestone
3 participants
@mpguerra @arya2 @teor2345