fix(state): Fix minute-long delays in block verification after a chain fork #6122

teor2345 · 2023-02-09T05:01:51Z

Motivation

Zebra can take multiple minutes to rebuild note commitment trees after a chain fork in the non-finalized chain. This also delays mining template updates and verifying mined blocks.

It only impacts mainnet at the moment, it is caused by a large number of shielded transactions in blocks.

Closes #4794.

Specifications

This is a data structure and processing refactor. The consensus rule checks don't change, but we need to correctly store data so that we implement the consensus rules correctly.

Complex Code or Requirements

This PR deletes a lot of complex code.

It temporarily stores the finalized tip trees and anchors in the tree indexes at the finalized tip height. These temporary entries are deleted when the first root block in the chain is finalized.

Solution

Replace sprout, sapling, orchard, and history tree fields with lookup methods
Remove unused tree rebuild code
Replace a custom clone function with derive(Clone)
Remove unused arguments

Related changes:

Refactor tree and anchor addition and removal functions
Print sprout and sapling tree Nodes as hex when debugging
Show full debug info when tests fail because chains aren't equal

Testing

We already have good test coverage for this code. I am also running a partial and full sync locally on both mainnet and testnet.

Review

There is an initial refactor to get the height indexes working in 98dfa87.
Then this PR repeats similar changes for sprout, sapling, and orchard. The best example is probably commit 3b0abd4.
The history tree change is slightly different, it's commit 09e5148.

This is a blocker for mining pools on mainnet, so it is a routine bug fix.

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 do further refactors, but this seemed like enough for now.

codecov · 2023-02-09T06:29:24Z

Codecov Report

Merging #6122 (0e2335e) into main (4f28929) will decrease coverage by 0.12%.
The diff coverage is 84.71%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6122      +/-   ##
==========================================
- Coverage   78.14%   78.03%   -0.12%     
==========================================
  Files         304      304              
  Lines       39087    39148      +61     
==========================================
+ Hits        30546    30549       +3     
- Misses       8541     8599      +58

arya2

Looks good! I really like the simplified fork()/parent_chain() methods.

zebra-chain/src/sapling/tree.rs

zebra-state/src/service/non_finalized_state/chain.rs

zebra-state/src/service/non_finalized_state.rs

zebra-state/src/service/non_finalized_state/chain.rs

…thod

…redundant code

arya2

Thank you!

(I left a do-not-merge label in case you'd like a second reviewer for the consensus rules)

teor2345 · 2023-02-09T23:48:53Z

I am also running a partial and full sync locally on both mainnet and testnet.

The partial syncs on both networks seem normal, as does the full sync on testnet. I am still waiting for the full sync on mainnet to fully checkpoint, this is also normal with the latency to where I am.

Before the transaction spam we were getting about 0.3% of blocks with a chain fork on mainnet. So if there was anything seriously wrong with this PR, I'd expect my mainnet partial sync would have failed by now.

teor2345 · 2023-02-10T01:26:20Z

(I left a do-not-merge label in case you'd like a second reviewer for the consensus rules)

I think @upbqdn or @conradoplg changed this code last, can one of you review this PR?

upbqdn · 2023-02-10T17:56:06Z

I'm reviewing this PR.

upbqdn

This PR looks great. It was rewarding to see the gradual simplification in each commit.

* ZIPs were updated to remove ambiguity, this was tracked in #1267. * #2105 was fixed by #3039 and #2379 was closed by #3069 * #2230 was a duplicate of #2231 which was closed by #2511 * #3235 was obsoleted by #2156 which was fixed by #3505 * #1850 was fixed by #2944, #1851 was fixed by #2961 and #2902 was fixed by #2969 * We migrated to Rust 2021 edition in Jan 2022 with #3332 * #1631 was closed as not needed * #338 was fixed by #3040 and #1162 was fixed by #3067 * #2079 was fixed by #2445 * #4794 was fixed by #6122 * #1678 stopped being an issue * #3151 was fixed by #3934 * #3204 was closed as not needed * #1213 was fixed by #4586 * #1774 was closed as not needed * #4633 was closed as not needed * Clarify behaviour of difficulty spacing Co-authored-by: teor <teor@riseup.net> * Update comment to reflect implemented behaviour Co-authored-by: teor <teor@riseup.net> * Update comment to reflect implemented behaviour when retrying block downloads Co-authored-by: teor <teor@riseup.net> * Update `TODO` to remove closed issue and clarify when we might want to fix Co-authored-by: teor <teor@riseup.net> * Update `TODO` to remove closed issue and clarify what we might want to change in future Co-authored-by: teor <teor@riseup.net> * Clarify benefits of how we do block verification Co-authored-by: teor <teor@riseup.net> * Fix rustfmt errors --------- Co-authored-by: teor <teor@riseup.net>

teor2345 added C-bug Category: This is a bug A-consensus Area: Consensus rule updates P-Medium ⚡ C-security Category: Security issues A-rpc Area: Remote Procedure Call interfaces A-state Area: State / database changes labels Feb 9, 2023

teor2345 requested a review from a team as a code owner February 9, 2023 05:01

teor2345 self-assigned this Feb 9, 2023

teor2345 requested a review from a team as a code owner February 9, 2023 05:01

teor2345 requested review from natalieesk and oxarbitrage and removed request for a team February 9, 2023 05:01

github-actions bot added C-removed labels Feb 9, 2023

teor2345 removed the request for review from natalieesk February 9, 2023 05:02

arya2 reviewed Feb 9, 2023

View reviewed changes

teor2345 added 12 commits February 10, 2023 09:38

Split Chain fields into sections

b4ee9de

Replace Chain.sprout_note_commitment_tree with a lookup method

81329b5

Add TODOs

7ab480e

Show full debug info when tests fail because chains aren't equal

087e2eb

Print sprout and sapling tree Nodes as hex when debugging

c0b7ddd

Correctly revert temporary finalized tip trees and anchors

3b8796b

Fix tests

141f967

Refactor removal functions

a1cf486

Replace the Chain.sapling_note_commitment_tree field with a lookup me…

25fa6ce

…thod

Replace the Chain.orchard_note_commitment_tree field with a lookup me…

e4c393f

…thod

Replace the Chain.history_tree field with a lookup method and remove …

742641a

…redundant code

Update comments

838cf15

teor2345 added 4 commits February 10, 2023 09:38

Ignore clippy::unwrap_in_result

319d985

Remove redundant fork() Result

7ea4bf8

Put conditional code in blocks

6a26fe7

fastmod history_tree_at_tip history_block_commitment_tree zebra-state

0e2335e

teor2345 force-pushed the remove-duplicate-chain-fields branch from 05607ac to 0e2335e Compare February 9, 2023 23:42

arya2 approved these changes Feb 9, 2023

View reviewed changes

arya2 added the do-not-merge Tells Mergify not to merge this PR label Feb 9, 2023

mpguerra requested a review from upbqdn February 10, 2023 09:05

upbqdn approved these changes Feb 13, 2023

View reviewed changes

arya2 removed the do-not-merge Tells Mergify not to merge this PR label Feb 13, 2023

mergify bot added a commit that referenced this pull request Feb 13, 2023

Merge of #6122

f0348cf

mergify bot mentioned this pull request Feb 13, 2023

merge queue: embarking main (09faf48) and #6122 together #6152

Closed

mergify bot merged commit 9452487 into main Feb 13, 2023

mergify bot deleted the remove-duplicate-chain-fields branch February 13, 2023 21:44

mpguerra added a commit that referenced this pull request May 19, 2023

#4794 was fixed by #6122

778cce7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(state): Fix minute-long delays in block verification after a chain fork #6122

fix(state): Fix minute-long delays in block verification after a chain fork #6122

teor2345 commented Feb 9, 2023 •

edited by upbqdn

Loading

codecov bot commented Feb 9, 2023 •

edited

Loading

arya2 left a comment

arya2 left a comment •

edited

Loading

teor2345 commented Feb 9, 2023

teor2345 commented Feb 10, 2023

upbqdn commented Feb 10, 2023

upbqdn left a comment

fix(state): Fix minute-long delays in block verification after a chain fork #6122

fix(state): Fix minute-long delays in block verification after a chain fork #6122

Conversation

teor2345 commented Feb 9, 2023 • edited by upbqdn Loading

Motivation

Specifications

Complex Code or Requirements

Solution

Testing

Review

Reviewer Checklist

Follow Up Work

codecov bot commented Feb 9, 2023 • edited Loading

Codecov Report

arya2 left a comment

Choose a reason for hiding this comment

arya2 left a comment • edited Loading

Choose a reason for hiding this comment

teor2345 commented Feb 9, 2023

teor2345 commented Feb 10, 2023

upbqdn commented Feb 10, 2023

upbqdn left a comment

Choose a reason for hiding this comment

teor2345 commented Feb 9, 2023 •

edited by upbqdn

Loading

codecov bot commented Feb 9, 2023 •

edited

Loading

arya2 left a comment •

edited

Loading