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

Do we need to validate nSpendsSapling, nOutputsSapling, and nActionsOrchard limits in V5 transactions? #2379

Closed
teor2345 opened this issue Jun 23, 2021 · 5 comments · Fixed by #3069
Assignees
Labels
A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement NU-5 Network Upgrade: NU5 specific tasks

Comments

@teor2345
Copy link
Contributor

teor2345 commented Jun 23, 2021

Motivation

We need to validate Orchard transactions from NU5 onwards.

Orchard signatures and proofs are already covered by other tickets, but we should check all the fields are being validated.

Specifications

TODO: Are there any other orchard or NU5 consensus rules?

Shielded Input and Output Limits

[NU5 onward] nSpendsSapling, nOutputsSapling, and nActionsOrchard MUST all be less than 2^16.

Explanation:

[NU5 onward] The rule that nSpendsSapling, nOutputsSapling, and nActionsOrchard MUST all be less than 2^16, is technically redundant because a transaction that could violate this rule would not fit within the 2 MB block size limit. It is included in order to simplify the security argument for balance preservation.

Note:

  • We could implement this rule directly, but we already limit these sizes using T::max_allocation for spends, outputs, and actions. So we could just add a comment about this rule.
  • We could also skip this rule, but add a compile-time assertion that it is redundant, using the same minimum spend, output, and action sizes as T::max_allocation

https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus

Solution

TODO: Work out how to check each consensus rule

@teor2345 teor2345 added A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code 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 Jun 23, 2021
@teor2345 teor2345 added this to the 2021 Sprint 14 milestone Jun 23, 2021
@teor2345 teor2345 changed the title Validate Orchard in V5 transactions Validate remaining Orchard consensus rules in V5 transactions Jun 23, 2021
@teor2345
Copy link
Contributor Author

I removed this rule, because it is validated by the orchard proof circuit (#1912):

Orchard Flags

[NU5 onward] The flags in flagsOrchard allow a version 5 transaction to declare that no funds are spent from Orchard notes (by setting enableSpendsOrchard to 0), or that no new Orchard notes with nonzero values are created (by setting enableOutputsOrchard to 0).

Explanation:

This has two primary purposes. First, the enableSpendsOrchard flag is set to 0 in version 5 coinbase transactions to ensure that they cannot spend from existing Orchard outputs. This maintains a restriction present in coinbase transactions for transparent, Sprout, or Sapling funds, which would not otherwise be enforceable in the combined Action transfer design. Second, if a security vulnerability were found that affected only the input side, or only the output side of the Action circuit, it would be possible to use these flags in a soft fork (i.e. a strictly contracting consensus change) to effectively “switch off” non-zero-valued transfers only on the relevant side. Setting either of these flags to 0 does not affect the presence or validation of spend authorization signatures, or other consensus rules associated with Action descriptions. These note spending and creation consensus rules are specified as part of the Orchard Action statement (§ 4.17.4 ‘Action Statement (Orchard)’ on p. 58).

https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus

@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Jun 28, 2021
@teor2345 teor2345 changed the title Validate remaining Orchard consensus rules in V5 transactions Validate nSpendsSapling, nOutputsSapling, and nActionsOrchard limits in V5 transactions Jul 22, 2021
@teor2345 teor2345 added P-Low and removed P-Medium labels Jul 22, 2021
@teor2345
Copy link
Contributor Author

This rule is technically redundant, so I'm moving it to sprint 21.

@mpguerra
Copy link
Contributor

Moving to Sprint 24 as a final sanity check before we decide to remove it from the NU5 mainnet activation/stable release goal

@daira
Copy link
Contributor

daira commented Oct 26, 2021

I removed this rule, because it is validated by the orchard proof circuit (#1912):

Orchard Flags

[NU5 onward] The flags in flagsOrchard allow a version 5 transaction to declare that no funds are spent from Orchard notes (by setting enableSpendsOrchard to 0), or that no new Orchard notes with nonzero values are created (by setting enableOutputsOrchard to 0).

Explanation:

This has two primary purposes. First, the enableSpendsOrchard flag is set to 0 in version 5 coinbase transactions to ensure that they cannot spend from existing Orchard outputs. This maintains a restriction present in coinbase transactions for transparent, Sprout, or Sapling funds, which would not otherwise be enforceable in the combined Action transfer design. Second, if a security vulnerability were found that affected only the input side, or only the output side of the Action circuit, it would be possible to use these flags in a soft fork (i.e. a strictly contracting consensus change) to effectively “switch off” non-zero-valued transfers only on the relevant side. Setting either of these flags to 0 does not affect the presence or validation of spend authorization signatures, or other consensus rules associated with Action descriptions. These note spending and creation consensus rules are specified as part of the Orchard Action statement (§ 4.17.4 ‘Action Statement (Orchard)’ on p. 58).

https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus

Is this comment on the right issue? The enable* flags are independent of the limits on the number of spends, outputs, and actions.

@teor2345
Copy link
Contributor Author

Is this comment on the right issue? The enable* flags are independent of the limits on the number of spends, outputs, and actions.

Previously, this ticket had a list of extra Orchard rules we needed to validate.

Once I removed the redundant rule, I renamed the ticket based on the single remaining rule.

@teor2345 teor2345 changed the title Validate nSpendsSapling, nOutputsSapling, and nActionsOrchard limits in V5 transactions Do we need to validate nSpendsSapling, nOutputsSapling, and nActionsOrchard limits in V5 transactions? Nov 8, 2021
@mpguerra mpguerra removed this from the 2021 Sprint 24 milestone Nov 15, 2021
@mpguerra mpguerra added this to the 2021 Sprint 23 milestone Nov 15, 2021
mergify bot pushed a commit that referenced this issue May 23, 2023
* ZIPs were updated to remove ambiguity, this was tracked in #1267.

* #2105 was fixed by #3039 and #2379 was closed by #3069

* #2230 was a duplicate of #2231 which was closed by #2511

* #3235 was obsoleted by #2156 which was fixed by #3505

* #1850 was fixed by #2944, #1851 was fixed by #2961 and #2902 was fixed by #2969

* We migrated to Rust 2021 edition in Jan 2022 with #3332

* #1631 was closed as not needed

* #338 was fixed by #3040 and #1162 was fixed by #3067

* #2079 was fixed by #2445

* #4794 was fixed by #6122

* #1678 stopped being an issue

* #3151 was fixed by #3934

* #3204 was closed as not needed

* #1213 was fixed by #4586

* #1774 was closed as not needed

* #4633 was closed as not needed

* Clarify behaviour of difficulty spacing

Co-authored-by: teor <teor@riseup.net>

* Update comment to reflect implemented behaviour

Co-authored-by: teor <teor@riseup.net>

* Update comment to reflect implemented behaviour when retrying block downloads

Co-authored-by: teor <teor@riseup.net>

* Update `TODO` to remove closed issue and clarify when we might want to fix

Co-authored-by: teor <teor@riseup.net>

* Update `TODO` to remove closed issue and clarify what we might want to change in future

Co-authored-by: teor <teor@riseup.net>

* Clarify benefits of how we do block verification

Co-authored-by: teor <teor@riseup.net>

* Fix rustfmt errors

---------

Co-authored-by: teor <teor@riseup.net>
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-5 Network Upgrade: NU5 specific tasks
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants