Validate spends of transparent coinbase outputs

[teor2345]

Motivation

Zebra needs to check the consensus rules that restrict how transparent outputs of coinbase transactions can be spent.

Specifications

See #2329 and #2330.

Designs

These designs are more complex than we actually need:

https://github.com/ZcashFoundation/zebra/blob/main/book/src/dev/rfcs/0004-asynchronous-script-verification.md , particularly the changes in PR #1970

https://github.com/ZcashFoundation/zebra/blob/main/book/src/dev/rfcs/0005-state-updates.md#requestawaitspendableutxo--outpoint-outpoint-spend_height-height-spend_restriction-spendrestriction-

Solution

Implementation:

• Add a CoinbaseSpendRestriction enum and Transaction method
• Check UTXO spendability during contextual validation, using known valid UTXOs

Test fixes:

• Skip created UTXOs in the genesis block in arbitrary block chains
• Refactor out a new_transaction_ordered_outputs function
• Add Transaction::outputs_mut for tests
• Only spend valid UTXOs in arbitrary block chains

Closes #2329
Closes #2330
Closes #2410

WIP Tests

Skip

• non-coinbase UTXOs
  • existing unit tests, acceptance tests, and cached state tests

Success

• coinbase UTXO spent to shielded outputs after 100 or more blocks
  • existing unit tests, acceptance tests, and cached state tests
• coinbase UTXO spent to shielded outputs after exactly 100 blocks
  • modified existing state request test

Failure

• coinbase UTXO spent to non-shielded outputs after 100 or more blocks
• coinbase UTXO spent to shielded outputs after less than 100 blocks

Test coverage

• make the proptest chain lengths slightly longer (ideally using a single constant)
• make sure that restriction-checked and unchecked proptest chains contain UTXO prevout spends

We could also do tests for finalized/non-finalized state, and both non-shielded and immature. But these tests are the essential ones.

I'm also running a full sync test locally on mainnet and testnet.

Review

I'd like an initial review from @jvff while I'm working on the rest of the tests.

Reviewer Checklist

• Code implements Specs and Designs
• Tests for Expected Behaviour
• Tests for Errors

Follow Up Work

Move the design changes in PR #1970 to an "alternatives" section, and explain how the simpler design works.
(Or just revert most of PR #1970.)

---

This change is 

[jvff]

Reviewed 10 of 14 files at r1, 1 of 1 files at r2, 1 of 1 files at r3.
Reviewable status: 12 of 14 files reviewed, 4 unresolved discussions (waiting on @teor2345)

---

zebra-chain/src/transparent/utxo.rs, line 84 at r3 (raw file):

    SomeTransparentOutputs,

    /// The UTXO is spent in a transaction with all shielded outputs

Nit: maybe replace all with only?

    /// The UTXO is spent in a transaction with only shielded outputs.

[jvff]

Reviewed 2 of 14 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @teor2345)

[teor2345]

Reviewable status: 7 of 14 files reviewed, 3 unresolved discussions (waiting on @daira, @jvff, and @teor2345)

---

zebra-chain/src/transparent/utxo.rs, line 84 at r3 (raw file):

Previously, jvff (Janito Vaqueiro Ferreira Filho) wrote…

Nit: maybe replace all with only?

    /// The UTXO is spent in a transaction with only shielded outputs.

Fixed in 4452d77

---

zebra-state/src/error.rs, line 104 at r3 (raw file):

Previously, jvff (Janito Vaqueiro Ferreira Filho) wrote…

Nit: Would it make sense to start breaking the long error messages in multi-line strings?

    #[error(
        "immature transparent coinbase spend: \
        attempt to spend {outpoint:?} at {spend_height:?}, \
        but spends are invalid before {min_spend_height:?}, \
        which is {MIN_TRANSPARENT_COINBASE_MATURITY:?} blocks \
        after it was created at {created_height:?}"
    )]

Fixed in ee0ea49

[jvff]

Reviewed 1 of 5 files at r6, 9 of 10 files at r7, 3 of 8 files at r8, 2 of 3 files at r9.
Reviewable status: 15 of 18 files reviewed, 5 unresolved discussions (waiting on @daira, @jvff, and @teor2345)

[jvff]

Reviewed 2 of 8 files at r8, 1 of 3 files at r9.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @daira and @teor2345)

[teor2345]

Dismissed @daira from a discussion.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jvff and @teor2345)

[teor2345]

Reviewable status: 10 of 24 files reviewed, 1 unresolved discussion (waiting on @jvff and @teor2345)

---

zebra-chain/src/block/arbitrary.rs, line 368 at r9 (raw file):

Previously, teor2345 (teor) wrote…

Once all the tests are working, I'll split this into a bunch of different methods

Here are the new commits I added:

Test that generated chains contain at least one transparent spend 48e4b6b
Make generated chains long enough for reliable tests 0df08ca
Split chain generation into 3 functions da95c01
Test that unshielded and immature transparent coinbase spends fail 6965782

This PR is getting a bit big. If you'd like, I can split these changes into other PRs, and get them merged first:

Refactor out a new_transaction_ordered_outputs function 8ead80c
Add Transaction::outputs_mut for tests 281fd68
Add transparent and shielded input and output methods to Transaction 7580673

The other changes will be a bit harder to split out.

[jvff]

Reviewed 1 of 9 files at r10, 2 of 7 files at r11, 3 of 5 files at r12.
Reviewable status: 16 of 24 files reviewed, 2 unresolved discussions (waiting on @jvff and @teor2345)

---

zebra-state/src/service.rs, line 35 at r12 (raw file):

#[cfg(any(test, feature = "proptest-impl"))]
pub mod arbitrary;

Just curious: is there a guideline for splitting module declarations? I want to know if/how I should them in my PRs 😅

[jvff]

Reviewed 3 of 9 files at r10, 1 of 7 files at r11, 2 of 5 files at r12.
Reviewable status: 22 of 24 files reviewed, 5 unresolved discussions (waiting on @jvff and @teor2345)

---

zebra-chain/src/block/arbitrary.rs, line 1 at r12 (raw file):

//! Randomised property testing for [`Block`]s

Nit: Maybe add a period at the end?

---

zebra-state/src/service/arbitrary.rs, line 21 at r12 (raw file):

use super::*;

/// The minimum height required for reliable non-finalized state property tests.

Minor: Should the documentation say something like maximum height for unreliable...? Also wondering if it would make sense to add a margin of error to it 🤔

---

zebra-state/src/service/check/tests/utxo.rs, line 76 at r12 (raw file):

    let min_spend_height = Height(created_height.0 + MIN_TRANSPARENT_COINBASE_MATURITY);
    let spend_height = Height(min_spend_height.0 - 1);

Minor: Should we add a test where spend_height = min_spend_height?

[jvff]

Reviewed 1 of 9 files at r10, 1 of 7 files at r11.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @teor2345)

---

zebra-chain/src/block/arbitrary.rs, line 368 at r9 (raw file):

Previously, teor2345 (teor) wrote…

Here are the new commits I added:

Test that generated chains contain at least one transparent spend 48e4b6b
Make generated chains long enough for reliable tests 0df08ca
Split chain generation into 3 functions da95c01
Test that unshielded and immature transparent coinbase spends fail 6965782

This PR is getting a bit big. If you'd like, I can split these changes into other PRs, and get them merged first:

Refactor out a new_transaction_ordered_outputs function 8ead80c
Add Transaction::outputs_mut for tests 281fd68
Add transparent and shielded input and output methods to Transaction 7580673

The other changes will be a bit harder to split out.

I think that won't be necessary. Most of it has been reviewed 😁

[teor2345]

Reviewable status: 22 of 24 files reviewed, 2 unresolved discussions (waiting on @jvff)

---

zebra-state/src/service.rs, line 35 at r12 (raw file):

Previously, jvff (Janito Vaqueiro Ferreira Filho) wrote…

Just curious: is there a guideline for splitting module declarations? I want to know if/how I should them in my PRs 😅

I have no idea 🤷

This might be a bad habit of mine from C, where conditional compilation happens using macros in #if ... #endif blocks.

Happy to go with a Rust guideline if we find one.

---

zebra-state/src/service/arbitrary.rs, line 21 at r12 (raw file):

Previously, jvff (Janito Vaqueiro Ferreira Filho) wrote…

Minor: Should the documentation say something like maximum height for unreliable...? Also wondering if it would make sense to add a margin of error to it 🤔

I clarified the comment here, and for PREVOUTS_CHAIN_HEIGHT.

There is already a margin of error built in to PREVOUTS_CHAIN_HEIGHT, currently the failure rate should be around 1 in 10 million test runs.

I just picked a really rare probability, we can tweak it if it becomes an issue later on.

---

zebra-state/src/service/check/tests/utxo.rs, line 76 at r12 (raw file):

Previously, jvff (Janito Vaqueiro Ferreira Filho) wrote…

Minor: Should we add a test where spend_height = min_spend_height?

Good idea, I had one of those, but it might have got deleted when I simplified the PR.

Also I think the chain itself checks this, but it's an easy test to write, so I'll do it now.

[teor2345]

Reviewable status: 21 of 24 files reviewed, all discussions resolved (waiting on @jvff)

---

zebra-state/src/service/check/tests/utxo.rs, line 76 at r12 (raw file):

Previously, teor2345 (teor) wrote…

Good idea, I had one of those, but it might have got deleted when I simplified the PR.

Also I think the chain itself checks this, but it's an easy test to write, so I'll do it now.

Done in b11310f

[teor2345]

I just worked out that this PR is blocking the next PR after #2486.
There's a chain of about 5 PRs we need to do there.

So I might just admin-merge this PR after CI passes.
After the last approval, I added one requested test and some comments.

[teor2345]

Admin-merged, because the PR was approved before minor changes, and it's blocking other work