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 Sprout anchors check #3236

Closed
conradoplg opened this issue Dec 15, 2021 · 1 comment · Fixed by #3283
Closed

Fix Sprout anchors check #3236

conradoplg opened this issue Dec 15, 2021 · 1 comment · Fixed by #3283
Assignees
Labels
A-consensus Area: Consensus rule updates C-bug Category: This is a bug NU Sprout Network Upgrade: Sprout specific tasks (before Overwinter) NU-5 Network Upgrade: NU5 specific tasks

Comments

@conradoplg
Copy link
Collaborator

conradoplg commented Dec 15, 2021

Motivation

There are at least two blocks with transactions that fail to validate the Sprout anchor check.

We disabled the check for now, but we must investigate the issue, fix it, and restore the check.

One of the transactions is https://explorer.zcha.in/transactions/5e53ebca23efd85774677f342e8dca5493865b3892dcf344c94284fa50d11102, the fifth tx in block 1_047_908, which fails to find the anchor 019c435cd1e8aca9a4165f7e126ac6e548952439d50213f4d15c546df9d49b61 for its second JoinSplit.

Another missing anchor is 3ad623811ffa4fe8498b23f3d6bb4e086dca32269afef6c8e572fd9ee6d0c0ea which seems to be used in a block near 1_057_736.

Specifications & Solutions

We currently fetch the output note commitment tree from the most recently validated block, and we build the interstitial note commitment tree by adding note commitments from the JoinSplits in the tx being currently validated by following the linear order of the JoinSplits in the tx. This turns out to be incorrect.

The issue is that we should look up and fetch the tree that the anchor of the previous JoinSplit refers to, and add the commitments of the previous JoinSplit to this tree. The root of this new tree is available for a check against the anchor of the current JoinSplit. This new tree is also available for the next JoinSplit. (Since the anchor of the current JoinSplit might refer to it, and the current JoinSplit will become the previous one for the next JoinSplit).

Spec:
image

For a reference, here's a transaction builder: https://github.com/zcash/zcash/blob/2cee089697ef817ca5c3d26d0c5db973e009037f/src/transaction_builder.cpp#L622. The relevant code is between lines 622 and 655.

The solution will require fetching a note commitment tree for each Sprout anchor. Note that multiple anchors might refer to the same tree since the roots of non-interstitial note commitment trees are stored on a per-block basis. We discussed that storing the trees only for the anchors in the non-finalized state might be sufficient. We don't need to store multiple trees for Sapling or Orchard because these shielded pools don't require fetching the trees according to anchors.

Consensus rules:
image

@conradoplg conradoplg added C-enhancement Category: This is an improvement S-needs-triage Status: A bug report needs triage labels Dec 15, 2021
@teor2345 teor2345 added A-consensus Area: Consensus rule updates C-bug Category: This is a bug NU Sprout Network Upgrade: Sprout specific tasks (before Overwinter) NU-5 Network Upgrade: NU5 specific tasks P-Medium and removed C-enhancement Category: This is an improvement labels Dec 15, 2021
@mpguerra
Copy link
Contributor

@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Dec 22, 2021
@mergify mergify bot closed this as completed in #3283 Jan 18, 2022
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 C-bug Category: This is a bug NU Sprout Network Upgrade: Sprout specific tasks (before Overwinter) NU-5 Network Upgrade: NU5 specific tasks
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants