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

Update Non-Finalized State for Transaction v5 #1982

Closed
4 tasks done
teor2345 opened this issue Apr 7, 2021 · 7 comments
Closed
4 tasks done

Update Non-Finalized State for Transaction v5 #1982

teor2345 opened this issue Apr 7, 2021 · 7 comments
Assignees
Labels
A-consensus Area: Consensus rule updates C-enhancement Category: This is an improvement NU-5 Network Upgrade: NU5 specific tasks

Comments

@teor2345
Copy link
Contributor

teor2345 commented Apr 7, 2021

Is your feature request related to a problem? Please describe.

Transaction v5 introduces Orchard nullifiers, which we need to include in the non-finalized state. We also need to handle sapling nullifiers, transparent UTXOs, and transaction IDs for transaction v5.

Describe the solution you'd like

It's hard to test this code using test vectors, because they need to be in a chain with valid previous block hashes.

Describe alternatives you've considered

These changes are required for NU5.

Additional context

Nullifier sets are validated in #958.

@teor2345 teor2345 added A-consensus Area: Consensus rule updates C-enhancement Category: This is an improvement S-needs-triage Status: A bug report needs triage NU-5 Network Upgrade: NU5 specific tasks P-Medium labels Apr 7, 2021
@teor2345 teor2345 added this to the 2021 Sprint 9 milestone Apr 7, 2021
@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Apr 13, 2021
@oxarbitrage
Copy link
Contributor

  • Implement UpdateWith for Transaction::V5

    • handle orchard nullifiers, sapling nullifiers, transparent UTXOs, and transaction IDs
    • re-enable the NU5 override in impl Strategy for PreparedChain

This is not clear to me.

  1. There is no UpdateWith for Transaction::V4, Transaction::V3 or any other version in https://github.com/ZcashFoundation/zebra/blob/main/zebra-state/src/service/non_finalized_state/chain.rs (all UpdateWith of the project are located in that file).
  2. I can't see any Nu5 override here https://github.com/ZcashFoundation/zebra/blob/main/zebra-state/src/service/non_finalized_state/arbitrary.rs#L45-L67

@teor2345
Copy link
Contributor Author

teor2345 commented May 24, 2021

There are two match transaction { V5 => unimplemented(), } in the PreparedBlock UpdateWith implementation:
https://github.com/ZcashFoundation/zebra/blob/main/zebra-state/src/service/non_finalized_state/chain.rs#L176
https://github.com/ZcashFoundation/zebra/blob/main/zebra-state/src/service/non_finalized_state/chain.rs#L236

I'm not sure why we do transaction updates inside block updates. Transaction updates might be easier to understand as a separate implementation. We'd have two shorter functions, rather than one long one.

@teor2345
Copy link
Contributor Author

2. I can't see any Nu5 override here https://github.com/ZcashFoundation/zebra/blob/main/zebra-state/src/service/non_finalized_state/arbitrary.rs#L45-L67

There is a network_upgrade_override here. It is currently disabled using None. You should replace it with NU5 after making these changes:
https://github.com/ZcashFoundation/zebra/blob/main/zebra-state/src/service/non_finalized_state/arbitrary.rs#L54

@teor2345 teor2345 added the S-blocked Status: Blocked on other tasks label May 24, 2021
@teor2345
Copy link
Contributor Author

teor2345 commented May 24, 2021

Blocked by #2050, which implements v5 transaction IDs

@teor2345
Copy link
Contributor Author

Blocked by #2050, which implements v5 transaction IDs

Thinking about this dependency again.

We're not committing any v5 transaction IDs to state yet, so we can do these PRs in any order.

(If we do accidentally commit v5 IDs to state because we're missing verification, we just need to update the state version and rebuild. But that shouldn't happen, because the transaction validation code is marked unimplemented!)

@teor2345 teor2345 removed the S-blocked Status: Blocked on other tasks label May 24, 2021
@mpguerra
Copy link
Contributor

mpguerra commented Jun 1, 2021

I see all checkbox items above are ticked, what else is outstanding here?

@teor2345
Copy link
Contributor Author

teor2345 commented Jun 1, 2021

I see all checkbox items above are ticked, what else is outstanding here?

It's all done, we should have said Closes #1982 on PR #2185

@teor2345 teor2345 closed this as completed Jun 1, 2021
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 C-enhancement Category: This is an improvement NU-5 Network Upgrade: NU5 specific tasks
Projects
None yet
Development

No branches or pull requests

3 participants