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

Implement RFC5: State updates Chain type #1069

Merged
merged 25 commits into from
Sep 24, 2020
Merged

Implement RFC5: State updates Chain type #1069

merged 25 commits into from
Sep 24, 2020

Conversation

yaahc
Copy link
Contributor

@yaahc yaahc commented Sep 16, 2020

No description provided.

@yaahc yaahc requested review from teor2345 and a team September 23, 2020 00:11
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, but needs a few tweaks and bug fixes.

I don't understand enough about transactions to review that part of the code, let's get @dconnolly or @hdevalence to review it as well.

In particular, I don't know if every post-Sapling block is guaranteed to have a Sprout and Sapling anchor. If they don't, that will affect this PR:
#902 (review)

There are also a few outstanding design issues, which were part of my final review of the RFC, but didn't get included. I don't think they affect this PR, but please check:
#1090

book/src/dev/rfcs/0005-state-updates.md Outdated Show resolved Hide resolved
book/src/dev/rfcs/0005-state-updates.md Outdated Show resolved Hide resolved
book/src/dev/rfcs/0005-state-updates.md Outdated Show resolved Hide resolved
book/src/dev/rfcs/0005-state-updates.md Outdated Show resolved Hide resolved
book/src/dev/rfcs/0005-state-updates.md Outdated Show resolved Hide resolved
zebra-state/src/memory_state.rs Show resolved Hide resolved
zebra-state/src/memory_state.rs Show resolved Hide resolved
zebra-state/src/memory_state.rs Show resolved Hide resolved
zebra-state/src/memory_state.rs Outdated Show resolved Hide resolved
zebra-state/src/memory_state.rs Show resolved Hide resolved
@yaahc yaahc requested a review from teor2345 September 23, 2020 18:33
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 great!

I'm not sure if you'd like to merge now, or do some more work, and then merge.

I think it's ok to merge the partial PR, as long as someone else has reviewed the transaction parts.

@yaahc
Copy link
Contributor Author

yaahc commented Sep 23, 2020

I want to get the two proptests that henry mentioned implemented before merging, just having a bit of trouble figuring out how to make arbitrary fake chains 😅

@teor2345
Copy link
Contributor

I want to get the two proptests that henry mentioned implemented before merging, just having a bit of trouble figuring out how to make arbitrary fake chains 😅

Let me know if you need help with that - you might want to start at genesis, and then add various forks.

@teor2345

This comment has been minimized.

@yaahc

This comment has been minimized.

@yaahc yaahc changed the title Implement RFC5: State updates Implement RFC5: State updates Chain structure Sep 24, 2020
@yaahc yaahc changed the title Implement RFC5: State updates Chain structure Implement RFC5: State updates Chain type Sep 24, 2020
@yaahc yaahc requested a review from teor2345 September 24, 2020 02:03
@teor2345
Copy link
Contributor

teor2345 commented Sep 24, 2020

Just following up on the sprout and sapling roots - it doesn't look like we'll need to change the RFC design or implementation:
https://github.com/ZcashFoundation/zebra/pull/902/files#r493427948

Comment on lines +786 to +787
- (non-finalized) if any `Chains` contain an `OutPoint` in their `created_utxos` and not their `spent_utxo` get the `transparent::Output` from `OutPoint`'s transaction
- (finalized) else if `OutPoint` is in `utxos_by_outpoint` return the associated `transparent::Output`.
Copy link
Contributor

Choose a reason for hiding this comment

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

My original utxo_is_available was meant for contextual verification, not prospective verification. Since a chain can fork at any non-finalized block, I don't think we should check spent_utxo here.

Instead, we should check spent_utxo using the original utxo_is_available logic during contextual verification.

Suggested change
- (non-finalized) if any `Chains` contain an `OutPoint` in their `created_utxos` and not their `spent_utxo` get the `transparent::Output` from `OutPoint`'s transaction
- (finalized) else if `OutPoint` is in `utxos_by_outpoint` return the associated `transparent::Output`.
- (non-finalized) if any `Chains` contain `OutPoint` in their `created_utxos`, get the `transparent::Output` from `OutPoint`'s transaction
- (finalized) else if `OutPoint` is in `utxos_by_outpoint` return the associated `transparent::Output`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

applied this in the test PR to avoid needing a new test and approval

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 we're looking pretty good to merge here.

We just need a tweak to the state design, and maybe we also want to get the separate tests PR working?

@yaahc yaahc merged commit 352721b into main Sep 24, 2020
@yaahc yaahc deleted the state-rfc-impl branch November 13, 2020 00:52
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.

2 participants