fix(state): Avoid temporary failures verifying the first non-finalized block or attempting to fork the chain before the final checkpoint

[arya2]

Motivation

This PR updates the can_fork_chain_at() method in zebra-state to ignore blocks below the finalized tip and fixes a bug where the first block past the final checkpoint will fail verification if it arrives in zebra-state before the final checkpoint block is written to disk.

Close #6388
Close #5125

Part of #5709

Solution

• Add a can_fork_chain_at_hashes flag to SentHashes
• Add and use a can_fork_chain_at(hash) method to SentHashes that checks the new flag
• When closing finalized block write channel:
  • Replace block_write_sent_hashes with an empty SentHashes and mark can_fork_chain_at_hashes as true
  • Start committing queued non-finalized child blocks of finalized tip

Related:

• Adds shrink_to_fit() calls in prune_by_height()

Review

This is low-priority.

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?

[codecov]

Codecov Report

Merging #6810 (5b51b9f) into main (77ad91c) will increase coverage by 0.60%.
Report is 66 commits behind head on main.
The diff coverage is 68.88%.

❗ Current head 5b51b9f differs from pull request most recent head 55bf5af. Consider uploading reports for the commit 55bf5af to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6810      +/-   ##
==========================================
+ Coverage   77.53%   78.14%   +0.60%     
==========================================
  Files         310      308       -2     
  Lines       41933    41055     -878     
==========================================
- Hits        32513    32081     -432     
+ Misses       9420     8974     -446     

[teor2345]

"non-finalized" and "finalized" are outdated terms that we're trying to move away from, because they were very confusing during the audit. When we're talking about the verification status of blocks, we use "semantically verified", "checkpoint verified" or "contextually verified".

But it's ok to say "finalized queue" when talking about block storage, or which state blocks are queued for.

There's a list of name changes in ticket #6793 which might be helpful.

Edit: name changes have finished

Moved blockers to another ticket

[teor2345]

Just marking this as needing changes.

[teor2345]

I updated my review because the name changes in ticket #6793 have merged.

(There's no need to work on this PR any time soon, please check with Pili first.)

This PR has been simplified and rewritten

[arya2]

@Mergifyio update

[mergify]

update

✅ Branch has been successfully updated

[arya2]

@Mergifyio update

[mergify]

update

✅ Branch has been successfully updated

[oxarbitrage]

I think this looks good, is short, clear and it seems to work.

What i am not totally sure is if it closes all the stuff that are inside the related tickets.

I will like @teor2345 to make the final review as they have more context in the tickets this is closing. I think the next few days could be a good time to merge this in.

[teor2345]

Thanks for this fix!

What i am not totally sure is if it closes all the stuff that are inside the related tickets.

That's a good question.

#5125 is partially resolved, I opened #7352 with the remaining work. (Which I don't think we should fix unless it actually happens.)

I checked the logs and #5709 is not resolved, it seems to be just as frequent as before this fix. (The warnings are still there, and syncing takes about the same amount of time.)