Document consensus rules from Zcash spec: 7.1.2 Transaction Consensus Rules

[mpguerra]

TODO

Open a zcash/zips PR to split the spec section into multiple sub-sections. That way, there will be links to each sub-section, and the rules will be easier to find.

Tasks

For each consensus rule:

• check that the rule is implemented in Zebra
• make sure the latest version of the rule is quoted in the docs for the code that implements it
• make sure the code implements the latest version of the rule

Split

• Document consensus rules from Zcash spec 7.1.2: Transaction Header #3425
• Document consensus rules from Zcash spec 7.1.2: Transaction Size #3426
• Document consensus rules from Zcash spec 7.1.2: Coinbase transactions #3427
• Document consensus rules from Zcash spec 7.1.2: Shielded pools #3428

Consensus rules

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

Implementation

• Check Transaction cardinality at parse time, to limit memory usage #1917
  • PRs:
    • Check nSpendsSapling, nOutputsSapling, and nActionsOrchard 2^16 limit #3069
    • Add missing nSpendsSapling limit check #3076
• Implement transaction::check for Transaction Version 5 and Orchard #1980
  • PRs:
    • Update has_inputs_and_outputs to check V5 transactions #2229
    • Move the check in transaction::check::sapling_balances_match to V4 deserialization #2234
    • Update transaction::check::coinbase_tx_no_joinsplit_or_spend to validate V5 coinbase transactions with Orchard shielded data #2236
    • Further test new transaction consensus rules #2246
• Implement Transparent and Sapling validation for transaction v5 #1981
  • PRs:
    • Refactor Sprout Join Split validation by transaction verifier #2371
    • Validate V5 transactions with Sapling shielded data #2437
• Validate bindingSigOrchard for Transaction::V5 #2103
  • PR: Add orchard binding_verification_key #2441
• Validate Transaction::V5 Halo2 proofs #2105
  • PR: Verify Halo2 proofs as part of V5 transaction verification #3039

[teor2345]

@mpguerra this is a very large task, so I'd like to split it between multiple people.

We could open a zcash/zips PR to split the spec section into multiple sub-sections?
That way there will be links to the sub-sections.

[mpguerra]

@mpguerra this is a very large task, so I'd like to split it between multiple people.

We can possibly split it between the people that worked on the listed issues/PRs. Before we start working on this we should double check that there aren't any other issues/PRs that contributed towards implementing this section of the spec.

[mpguerra]

It would also be nice if these issues could be used by the auditors as a reference and so we should make sure the description is as complete and accurate as possible

[teor2345]

@mpguerra this is a very large task, so I'd like to split it between multiple people.

We can possibly split it between the people that worked on the listed issues/PRs. Before we start working on this we should double check that there aren't any other issues/PRs that contributed towards implementing this section of the spec.

Thanks, please re-assign me to some of the sub-tasks after you've split this task.

Here is the split I'd suggest, because it matches the structure of Zebra's code (and also the spec):

• transaction header (including expiry height)
• transaction size (including item counts)
• coinbase transactions (including coinbase-specific item counts and values)
• shielded pools: the rule is specific to a shielded pool (this is different from the network upgrade range of the rule, even if the network upgrade has the same name as a shielded pool)

It would also be nice if these issues could be used by the auditors as a reference and so we should make sure the description is as complete and accurate as possible

This is unusual - I've never seen a project that keeps closed tickets up to date as the code changes. So I assumed that closed tickets are out of scope for the audits. Tickets are based on what we knew at the time they were closed. But we want to audit the current state of the software, so we need something that stays up to date.

So I think we should update the documentation next to the code we're getting audited. That way, we can maintain that documentation as we change the code, so it will be ready for future audits.

I don't know if this is related, but was it confusing to use open tickets to track questions for the auditors?

Let's talk about auditing closed tickets and tracking auditor questions at the Engineering sync.

[mpguerra]

It would also be nice if these issues could be used by the auditors as a reference and so we should make sure the description is as complete and accurate as possible

This is unusual - I've never seen a project that keeps closed tickets up to date as the code changes. So I assumed that closed tickets are out of scope for the audits. Tickets are based on what we knew at the time they were closed. But we want to audit the current state of the software, so we need something that stays up to date.

So I think we should update the documentation next to the code we're getting audited. That way, we can maintain that documentation as we change the code, so it will be ready for future audits.

I wasn't suggesting that we update the closed tickets. I think the tickets as they are can contain useful information about how and where a consensus rule was implemented in addition to the documentation within the code. This will obviously only be valid for the NU5 network upgrade and I assume we'll do an audit before any new network upgrade is scheduled.

[teor2345]

All the sub-tickets in this ticket have been completed.