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

Reject duplicate Sapling and Orchard nullifiers #2497

Merged
merged 14 commits into from
Jul 20, 2021

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Jul 15, 2021

Motivation

Zebra needs to reject duplicate sapling and orchard nullifiers, to prevent double-spends.

See #2231 for details.
This PR finishes the shielded part of #2231, but does not close that ticket.

Solution

This code is almost identical to the sprout code in the previous PR:

  • Add sapling and orchard duplicate nullifier errors
  • Reject duplicate finalized sapling and orchard nullifiers
  • Reject duplicate non-finalized sapling and orchard nullifiers

These tests are based on the previous PR, with sapling and orchard-specific changes:

  • Refactor sprout nullifier tests to remove common code
  • Add sapling nullifier tests
  • Add orchard nullifier tests

The specific test changes are:

  • sapling and orchard only have one nullifier per Spend/Action, so there is no need for "same Spend/Action" tests
  • we can't reliably make orchard nullifiers distinct, because they must be valid Pallas points, so we just skip the test case if they are the same
  • various minor structural and validation changes

Review

@jvff can review this PR - it's not urgent at all.
This code is unlikely to conflict with any other code.

Reviewer Checklist

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

Follow Up Work

Prevent transparent double-spends #2231

@teor2345 teor2345 added A-consensus Area: Consensus rule updates NU-1 Sapling Network Upgrade: Sapling specific tasks A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement NU-5 Network Upgrade: NU5 specific tasks P-Medium labels Jul 15, 2021
@teor2345 teor2345 requested a review from jvff July 15, 2021 02:14
@teor2345 teor2345 self-assigned this Jul 15, 2021
@teor2345 teor2345 changed the title WIP: Reject duplicate Sapling and Orchard nullifiers Reject duplicate Sapling and Orchard nullifiers Jul 15, 2021
@teor2345 teor2345 marked this pull request as ready for review July 15, 2021 05:43
jvff
jvff previously approved these changes Jul 16, 2021
Copy link
Contributor

@jvff jvff left a comment

Choose a reason for hiding this comment

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

Everything looks good. There are only a few questions I'm a bit unsure of. Two are in the code, and another follows below. I also ended up adding too many optional suggestions. Feel free to ignore them, I don't know why but for some reason I'm feeling more creative than normal. I apologize for any noise this creates 😅

I'm not sure this is a valid question, but would it make sense to add another test that adds two different nullifiers? The current acceptance tests only add one nullifier 🤔

This could go further and mirror all of the rejection tests but with different nullifiers to check if they are accepted. That's a lot of tests, so if this is something that's useful, it could be done in a separate PR.

zebra-state/src/service/finalized_state.rs Show resolved Hide resolved
zebra-state/src/service/check/tests/prop.rs Outdated Show resolved Hide resolved
zebra-state/src/service/check/tests/prop.rs Show resolved Hide resolved
zebra-state/src/service/check/tests/prop.rs Outdated Show resolved Hide resolved
zebra-state/src/service/check/tests/prop.rs Outdated Show resolved Hide resolved
zebra-state/src/service/check/tests/prop.rs Show resolved Hide resolved
@teor2345
Copy link
Contributor Author

I'm not sure this is a valid question, but would it make sense to add another test that adds two different nullifiers? The current acceptance tests only add one nullifier 🤔

This could go further and mirror all of the rejection tests but with different nullifiers to check if they are accepted. That's a lot of tests, so if this is something that's useful, it could be done in a separate PR.

The integration tests already add thousands of nullifiers. So I'm not sure if adding two nullifiers in the unit tests gives us significantly better test coverage.

(There are already ~15 tests across the 3 shielded pools.)

teor2345 added 8 commits July 19, 2021 13:04
Reject duplicate sapling and orchard nullifiers in a new block,
when the block is added to a non-finalized chain,
and the duplicate nullifier is already in the finalized state.
Reject duplicate sapling and orchard nullifiers in a new block,
when the block is added to a non-finalized chain,
and the duplicate nullifier is in:
* the same shielded data,
* the same transaction,
* the same block, or
* an earlier block in the non-finalized chain.
Test that the state rejects duplicate sapling nullifiers in a new block,
when the block is added to a non-finalized chain,
and the duplicate nullifier is in:
* the same shielded data,
* the same transaction,
* the same block,
* an earlier block in the non-finalized chain, or
* the finalized state.
Test that the state rejects duplicate orchard nullifiers in a new block,
when the block is added to a non-finalized chain,
and the duplicate nullifier is in:
* the same shielded data,
* the same transaction,
* the same block,
* an earlier block in the non-finalized chain, or
* the finalized state.
@teor2345 teor2345 force-pushed the sapling-orchard-duplicate-nullifiers branch from 53a115b to e6ba566 Compare July 19, 2021 03:11
@teor2345 teor2345 mentioned this pull request Jul 19, 2021
1 task
jvff
jvff previously approved these changes Jul 19, 2021
@teor2345 teor2345 enabled auto-merge (squash) July 19, 2021 22:54
@teor2345 teor2345 merged commit 544be14 into main Jul 20, 2021
@teor2345 teor2345 deleted the sapling-orchard-duplicate-nullifiers branch July 20, 2021 00:39
mpguerra added a commit that referenced this pull request Jul 28, 2021
mpguerra added a commit that referenced this pull request Jul 29, 2021
* Draft CHANGELOG for Zebra 1.0.0-alpha.14

* Add PR #2533 to CHANGELOG

* Apply suggestions from code review

Co-authored-by: teor <[email protected]>

* Remove entry about updating the changelog

* move #2497

* add #2529

* Add a missing space

* Add #2458, #2525, #2486, #2542 and  #2539 to CHANGELOG

Co-authored-by: teor <[email protected]>
Co-authored-by: Deirdre Connolly <[email protected]>
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 A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement NU-1 Sapling Network Upgrade: Sapling specific tasks NU-5 Network Upgrade: NU5 specific tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants