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

Store orchard nullifiers into the state #2185

Merged
merged 22 commits into from
Jun 1, 2021

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented May 21, 2021

Motivation

We need to store nullifier data for orchard in the state. #1983

Solution

Add a new column to the database (and to the spec) to store the new data.

The code in this pull request has:

Review

@dconnolly can take a look. I followed the advise of using the sapling nullifiers as a guide.

@oxarbitrage
Copy link
Contributor Author

oxarbitrage commented May 21, 2021

this PR seems to also cover partially #1982

Copy link
Contributor

@dconnolly dconnolly 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, two small changes I'd like to see 😎

zebra-chain/src/orchard/note/nullifiers.rs Outdated Show resolved Hide resolved
zebra-state/src/service/finalized_state/disk_format.rs Outdated Show resolved Hide resolved
zebra-state/src/constants.rs Show resolved Hide resolved
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.

Let's split off the changes from #1982 into their own PR, so we don't accidentally forget any changes.

I also suggested a few updates to comments to make them more accurate.

zebra-chain/src/orchard/shielded_data.rs Outdated Show resolved Hide resolved
zebra-chain/src/orchard/shielded_data.rs Outdated Show resolved Hide resolved
zebra-chain/src/orchard/note/nullifiers.rs Outdated Show resolved Hide resolved
book/src/dev/rfcs/0005-state-updates.md Show resolved Hide resolved
book/src/dev/rfcs/0005-state-updates.md Show resolved Hide resolved
zebra-chain/src/transaction.rs Show resolved Hide resolved
@teor2345
Copy link
Contributor

I just filled in the test plan for this change:

  • Integration Tests (empty nullifier lists): Rebuild the cached state for the integration CI tests
  • Integration Tests (actual nullifiers): NU5 testnet activation

We'll have to come up with a better plan for testing the stable release in NU6.
But for NU5 we can just test alphas after testnet activation.

@teor2345
Copy link
Contributor

I just filled in the test plan for this change:

  • Integration Tests (empty nullifier lists): Rebuild the cached state for the integration CI tests
  • Integration Tests (actual nullifiers): NU5 testnet activation

We'll have to come up with a better plan for testing the stable release in NU6.
But for NU5 we can just test alphas after testnet activation.

We should also be able to write unit tests that take a NU5 test vector and commit it to an ephemeral state. So I've added those.

@mpguerra mpguerra linked an issue May 24, 2021 that may be closed by this pull request
5 tasks
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.

I'd like to remove the "add anchor" and "remove anchor" comments, because we're not actually doing that yet. (This is an old problem with the comments, but we should fix it now.)

@oxarbitrage oxarbitrage mentioned this pull request May 25, 2021
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.

I think your test vector might have found a bug in Zebra. Nice!

Let's change the tests to use random data (you'll need PR #2208), and do a few small cleanups.

zebra-state/src/service/non_finalized_state/chain.rs Outdated Show resolved Hide resolved
zebra-state/src/service/non_finalized_state/chain.rs Outdated Show resolved Hide resolved
zebra-state/src/service/non_finalized_state/chain.rs Outdated Show resolved Hide resolved
zebra-state/src/service/finalized_state/tests/vectors.rs Outdated Show resolved Hide resolved
zebra-state/src/service/finalized_state/tests/vectors.rs Outdated Show resolved Hide resolved
zebra-state/src/service/finalized_state/tests/vectors.rs Outdated Show resolved Hide resolved
zebra-state/src/service/finalized_state/tests/vectors.rs Outdated Show resolved Hide resolved
zebra-test/src/vectors.rs Outdated Show resolved Hide resolved
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.

This is looking good.

I did a bunch of work in #2221 that should make implementing these proptests easier.

zebra-chain/src/block/arbitrary.rs Outdated Show resolved Hide resolved
zebra-chain/src/block/arbitrary.rs Outdated Show resolved Hide resolved
zebra-state/src/service/finalized_state/tests/vectors.rs Outdated Show resolved Hide resolved
zebra-state/src/service/finalized_state/tests/vectors.rs Outdated Show resolved Hide resolved
zebra-state/src/service/non_finalized_state/arbitrary.rs Outdated Show resolved Hide resolved
teor2345
teor2345 previously approved these changes Jun 1, 2021
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, I'll just apply a comment fix then merge.

zebra-state/src/service/finalized_state/tests/prop.rs Outdated Show resolved Hide resolved
@teor2345 teor2345 enabled auto-merge (squash) June 1, 2021 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Store Orchard nullifiers in the finalized state
3 participants