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 interstitial sprout anchors check #3283

Merged
merged 29 commits into from
Jan 18, 2022

Conversation

conradoplg
Copy link
Collaborator

@conradoplg conradoplg commented Dec 23, 2021

Motivation

There was an issue with the interstitial Sprout anchor check, described in #3236

Specifications

image

https://zips.z.cash/protocol/protocol.pdf#joinsplit

Designs

Solution

  • Change state to add the matching note commitment tree with each Sprout anchor
  • Also add it to non-finalized state (Chain)
  • When checking the anchor, build the interstitial trees starting from the tree that matches the anchor (and not from the tree of the finalized tip)
  • Update state documentation

Closes #3236 and #3210.

Review

Not particularly urgent, but it's the final consensus check. Usually @dconnolly and/or @upbqdn could review but @teor2345 might want to take a look.

After accepting but before merging we need to update the cached state in CI (or disable the CI job until we do that).

There is no specific test - I tested by syncing to the tip:

image

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

Follow Up Work

@codecov
Copy link

codecov bot commented Dec 23, 2021

Codecov Report

Merging #3283 (365e429) into main (6c787dd) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3283      +/-   ##
==========================================
- Coverage   77.83%   77.76%   -0.08%     
==========================================
  Files         266      266              
  Lines       31449    31471      +22     
==========================================
- Hits        24478    24472       -6     
- Misses       6971     6999      +28     

upbqdn
upbqdn previously approved these changes Jan 3, 2022
Copy link
Member

@upbqdn upbqdn left a comment

Choose a reason for hiding this comment

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

All looks good. It's handy that the commitment trees could be stored under sprout_anchors.
I left only a few minor suggestions.

book/src/dev/rfcs/0005-state-updates.md Outdated Show resolved Hide resolved
zebra-state/src/service/check/anchors.rs Outdated Show resolved Hide resolved
@upbqdn
Copy link
Member

upbqdn commented Jan 3, 2022

I can regenerate the cached state once someone else approves this PR.

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks good to me, but it needs a cached state rebuild.

@teor2345
Copy link
Contributor

teor2345 commented Jan 3, 2022

@upbqdn Conrado isn't available this week, so can you push all the changes you think are needed to this PR's branch?

I can re-approve after you're done.

I can regenerate the cached state once someone else approves this PR.

I think it's ready to go, after you make whatever changes you think are needed.

Just checking, are you going to regenerate the cached state, update the image in this PR, then merge it to main?
(That way, the tests won't fail on main, because there's no gap between the version and cached state update.)

The hash in this line needs to be updated:

--create-disk name="zebrad-cache-$SHORT_SHA-mainnet-canopy",image=zebrad-cache-13c6a826-mainnet-canopy \

Co-authored-by: Marek <mail@marek.onl>
@teor2345 teor2345 added A-consensus Area: Consensus rule updates NU-5 Network Upgrade: NU5 specific tasks P-Medium labels Jan 4, 2022
@upbqdn
Copy link
Member

upbqdn commented Jan 4, 2022

Just checking, are you going to regenerate the cached state, update the image in this PR, then merge it to main?

Yes, exactly. I'm going to follow the steps in the onboarding doc. I refined them recently, so it should be smooth.

Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

Looks good. I apologize again for the hairiness of the spec here.

zebra-state/src/service/check/anchors.rs Outdated Show resolved Hide resolved
@upbqdn
Copy link
Member

upbqdn commented Jan 12, 2022

This PR also partially documents the consensus rules for #3211.

@upbqdn
Copy link
Member

upbqdn commented Jan 18, 2022

The following statement should not be a consensus rule
image

This PR documents all consensus rules mentioned in #3210 except for the statement above, so it also closes #3210.

See zcash/zips#582 for more details regarding the statement above.

@conradoplg
Copy link
Collaborator Author

I managed to recreate the cached state, updated it and it works 🎉

(But now CI is failing 😢 , see #3366)

dconnolly
dconnolly previously approved these changes Jan 18, 2022
Co-authored-by: Deirdre Connolly <deirdre@zfnd.org>
@conradoplg conradoplg dismissed stale reviews from dconnolly via 9c7b1fd January 18, 2022 18:57
@conradoplg
Copy link
Collaborator Author

@dconnolly I don't know why it dismissed your review, I just accepted your suggestion 😕

dconnolly
dconnolly previously approved these changes Jan 18, 2022
@conradoplg
Copy link
Collaborator Author

@dconnolly oops, fixed formatting 😅

@mergify mergify bot merged commit 4aeabd0 into main Jan 18, 2022
@mergify mergify bot deleted the fix-interstitial-sprout-anchors-check branch January 18, 2022 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Area: Consensus rule updates NU-5 Network Upgrade: NU5 specific tasks
Projects
None yet
5 participants