fix(rpc): Omit transactions with transparent coinbase spends that are immature at the next block height from block templates

[arya2]

Motivation

The getblocktemplate method can currently construct invalid block templates if there are mempool transactions with transparent coinbase spends that are immature at the next block height.

This PR adds a maturity_height field to the transaction verifier's mempool response and uses it in get_block_template() to filter out transactions that would be invalid at the next block height.

Closes #6482.

Solution

• Adds maturity_height: Option<Height> to VerifiedUnminedTx in the transaction verifier
• Filters out transactions that would be invalid at the next block height in fetch_mempool_transactions()

Review

Anyone can review.

Reviewer Checklist

• Will the PR name make sense to users?
  • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
• Are the PR labels correct?
• Does the code do what the ticket and PR says?
  • Does it change concurrent code, unsafe code, or consensus rules?
• How do you know it works? Does it have tests?

Follow Up Work

Once authorizing data is bound to the block data during semantic validation, return errors for block requests to replace this check in the state service:

• Add hashAuthDataRoot to Block network messages, and semantically verify it #2336

[teor2345]

The getblocktemplate method can currently construct invalid block templates if there are mempool transactions with transparent coinbase spends that are immature at the next block height.

So this is kind of a big question, but should these invalid transactions be in the mempool at all?

If we make the changes in this PR, we'll store invalid transactions in the mempool, and relay them to other nodes. And those invalid transactions could cause valid transactions to be rejected from the mempool due to conflicts, or due to mempool capacity limits. (They also waste CPU and bandwidth across the Zcash network.)

Looking at our design principles, we'd be breaking one of the core properties of Zebra's implementation: we don't store invalid data. It would be particularly confusing to store an invalid transaction in a type called VerifiedUnminedTx:

zebra/zebra-chain/src/transaction/unmined.rs

Lines 294 to 299 in 166526a

	/// A verified unmined transaction, and the corresponding transaction fee.
	///
	/// This transaction has been fully verified, in the context of the mempool.
	#[derive(Clone, Debug, PartialEq)]
	#[cfg_attr(any(test, feature = "proptest-impl"), derive(Arbitrary))]
	pub struct VerifiedUnminedTx {

As an alternative, if the mempool transaction verifier checks transactions against the best tip height, and returns a StorageEffectsTip error if they can't be spent yet, they won't be stored in our mempool. This means:

• they can't be mined into blocks
• they won't be relayed to other nodes

And when the tip updates, Zebra will re-download them and try to verify them again. Which is what we want.

(If we want to store unmined transactions that could potentially become valid, like zcashd, then that's a bigger decision. I think it should probably go back to the whole team.)

I'm sorry I have to give feedback like this, because I think this is a neat idea. (And it's quite a clever workaround!) But I'm concerned it could cause maintenance issues like subtle bugs and developer confusion.

[codecov]

Codecov Report

Merging #6510 (e6c6c0f) into main (d1990ca) will increase coverage by 0.07%.
The diff coverage is 93.83%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6510      +/-   ##
==========================================
+ Coverage   77.72%   77.79%   +0.07%     
==========================================
  Files         304      305       +1     
  Lines       39665    40225     +560     
==========================================
+ Hits        30830    31295     +465     
- Misses       8835     8930      +95     

[arya2]

So this is kind of a big question, but should these invalid transactions be in the mempool at all?

I don't think so, they're not in the zcashd mempool.

If we make the changes in this PR, we'll store invalid transactions in the mempool, and relay them to other nodes. And those invalid transactions could cause valid transactions to be rejected from the mempool due to conflicts, or due to mempool capacity limits. (They also waste CPU and bandwidth across the Zcash network.)

Great point, it looks like zcashd always immediately rejects transactions with immature transparent coinbase spends at the next block height from the mempool (at the end of AcceptToMemoryPool()), Zebra should do the same (or at least avoid relaying these transactions).

(If we want to store unmined transactions that could potentially become valid, like zcashd, then that's a bigger decision. I think it should probably go back to the whole team.)

I thought zcashd stashes these (it stashes other invalid transactions that may become valid but not these, whoops).

I'm sorry I have to give feedback like this, because I think this is a neat idea. (And it's quite a clever workaround!) But I'm concerned it could cause maintenance issues like subtle bugs and developer confusion.

Hey, that's what it takes to make good code, and I took the risk by not looking closely enough at what zcashd is doing. Thank you for helping catch the mistake.

[arya2]

Updated in d8c8160 so it rejects transactions with spends that aren't valid at the next block height.

Unified transparent_coinbase_spend() with transparent_coinbase_spend_maturity() in a45a6f7.

I left the TODO for moving the check out of the state, I still think it would work, parallelize the check, and possibly avoid duplicate lookups.

[teor2345]

This seems like something that we could merge as-is, but I'd like to improve the test coverage and method documentation.

Requested changes were made

[teor2345]

Thanks, let's get this merged to fix CI!