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

WIP: Reject double-spends of shielded nullifiers and UTXOs #2420

Closed
wants to merge 2 commits into from

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Jun 30, 2021

Motivation

Zebra needs to check the double-spend consensus rule for shielded spends in the finalized state.

Specifications

See #2231

Designs

The state RFC is updated by this PR:
https://github.com/ZcashFoundation/zebra/blob/main/book/src/dev/rfcs/0005-state-updates.md#rocksdb-data-structures

Solution

Closes #2231.

Before adding a block to a non-finalized chain:

  • Check for nullifier double-spends
  • Check for UTXO double-spends

Testing

  • Test that duplicate sprout nullifiers are rejected
  • Test that duplicate sapling and orchard nullifiers are rejected
  • Test that duplicate spends of UTXOs are rejected

I've started a cached state test for commit be39530 here:
https://github.com/ZcashFoundation/zebra/actions/runs/985595375

Review

@jvff can do the initial review on this PR.

It's somewhat urgent, because the design and code changes will conflict with the new column families for anchors, history, and value pools.

Reviewer Checklist

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

@teor2345 teor2345 added C-design Category: Software design work A-consensus Area: Consensus rule updates NU-1 Sapling Network Upgrade: Sapling specific tasks NU Sprout Network Upgrade: Sprout specific tasks (before Overwinter) A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement NU-5 Network Upgrade: NU5 specific tasks P-Medium labels Jun 30, 2021
@teor2345 teor2345 requested a review from jvff June 30, 2021 08:17
@teor2345 teor2345 self-assigned this Jun 30, 2021
@teor2345 teor2345 force-pushed the reject-nullifier-double-spends branch from 273165f to d7f33dc Compare June 30, 2021 08:19
@teor2345
Copy link
Contributor Author

The macOS failure is unrelated, it looks like #2163.

Most of the first ~30 peers we tried failed, and took up the entire timeout. That will get better when each initial handshake is in its own task.

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.

Looks good, but I did add a warning note about possibly making the code more cautious of any future rocksdb changes. As far as I can tell, this is just an extra precaution, and it would still work if left as is 👍

zebra-state/src/service/finalized_state.rs Outdated Show resolved Hide resolved
zebra-state/src/service/finalized_state/merge_operator.rs Outdated Show resolved Hide resolved
@teor2345 teor2345 marked this pull request as draft July 2, 2021 06:22
@teor2345 teor2345 changed the title WIP: Reject double-spends of shielded nullifiers in the finalized state WIP: Reject double-spends of shielded nullifiers and UTXOs Jul 2, 2021
@teor2345
Copy link
Contributor Author

teor2345 commented Jul 2, 2021

This PR got a lot more complicated than I expected.

The code works, but the check is in the wrong place in the finalized state.

I need to move the check to the non-finalized state, and probably split the UTXO and nullifier changes.

@teor2345 teor2345 force-pushed the reject-nullifier-double-spends branch from 8639df5 to 815268a Compare July 7, 2021 04:48
@teor2345 teor2345 force-pushed the reject-nullifier-double-spends branch 4 times, most recently from 09c3180 to c7ee621 Compare July 8, 2021 06:01
@teor2345 teor2345 force-pushed the reject-nullifier-double-spends branch from c7ee621 to f46b030 Compare July 9, 2021 00:27
teor2345 added 2 commits July 9, 2021 18:39
We need to keep the order of UTXOs when we're verifying a single block,
but the block order is irrelevant for UTXOs stored in the state.
Reject transparent output double-spends
- Check that transparent spends use outputs from earlier in their block

Document how duplicate nullifiers are rejected by the finalized state.

Tests for duplicate sprout nullifiers at different levels
Nullifiers can be duplicated within the same JoinSplit, transaction,
block, or chain.

And disable a failing test.
@teor2345 teor2345 mentioned this pull request Jul 12, 2021
1 task
@teor2345 teor2345 force-pushed the reject-nullifier-double-spends branch from f46b030 to 950d6ef Compare July 16, 2021 02:14
@teor2345
Copy link
Contributor Author

I'm going to open a PR that obsoletes this PR some time over the next day or two.

@teor2345 teor2345 closed this Jul 16, 2021
@teor2345 teor2345 mentioned this pull request Jul 20, 2021
3 tasks
@teor2345 teor2345 deleted the reject-nullifier-double-spends branch March 21, 2022 21:34
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-design Category: Software design work C-enhancement Category: This is an improvement NU Sprout Network Upgrade: Sprout specific tasks (before Overwinter) 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.

Stop double-spends by checking nullifiers and UTXO spends in each non-finalized chain
3 participants