ZIP-221 and ZIP-244 commitment validation in non-finalized state #2609
Conversation
|
Moved to draft because I forgot about the checkpoint verifier. But otherwise it's OK to review. |
There was a problem hiding this comment.
This looks good so far.
I'd like to see a NU5 test, can we open a ticket for that?
(We might need to wait until we have the NU5 testnet activation height.)
We could wait for NU5 testnet activation and see if it works.
But if it doesn't, all our CI would fail, even on unrelated work.
conradoplg
force-pushed
the
zip221-validation
branch
2 times, most recently
from
c8a7858
to
988963d
Compare
|
Would you mind rebasing this PR on |
conradoplg
force-pushed
the
zip221-validation
branch
from
988963d
to
d39b44b
Compare
Done! |
Co-authored-by: teor <teor@riseup.net>
conradoplg
force-pushed
the
zip221-validation
branch
from
d39b44b
to
d497a2a
Compare
teor2345
changed the title
I've started a run here https://github.com/ZcashFoundation/zebra/actions/runs/1137516612 |
There was a problem hiding this comment.
I'd like to get the cached state tests passing before we merge this PR, then I think we're good to go here.
I've started a run here https://github.com/ZcashFoundation/zebra/actions/runs/1137516612
It passed!
| // Test committing the next block with the wrong commitment | ||
| let next_block = activation_block.make_fake_child(); | ||
| let err = state | ||
| .commit_block(next_block.clone().prepare(), &finalized_state) | ||
| .unwrap_err(); | ||
| match err { | ||
| crate::ValidateContextError::InvalidBlockCommitment( | ||
| zebra_chain::block::CommitmentError::InvalidChainHistoryRoot { .. }, | ||
| ) => {} | ||
| _ => panic!( | ||
| "Error must be InvalidBlockCommitment::InvalidChainHistoryRoot instead of {:?}", | ||
| err | ||
| ), | ||
| }; |
| // Test committing the next block with the correct commitment | ||
| let next_block = next_block.set_block_commitment(tree.hash().into()); | ||
| state | ||
| .commit_block(next_block.prepare(), &finalized_state) | ||
| .unwrap(); |
|
@dconnolly sorry! I think I've merged it before you finished reading through it 😅 |
No worries 😁 |
Motivation
ZIP-221 specifies a history tree to which each block must commit to. ZIP-244 expands this commitment to also include the authentication data commitment.
This adds both checks.
Specifications
https://zips.z.cash/zip-0221#block-header-semantics-and-consensus-rules
https://zips.z.cash/zip-0244#block-header-changes
Designs
N/A
Solution
Compute the correct commitment and compare with the commitment in the block header.
The ZIP-244 path is not tested because there aren't post-Nu5 blocks yet and I couldn't come up with a way to test it. Suggestions are welcome.
Review
#2547 must be reviewed & merged first.
This (hopefully) finishes what we need for minimal validation post-Nu5, so must be reviewed before that.
Reviewer Checklist
Follow Up Work
N/A
This change is