change(mempool) reject transactions with spent outpoints or nullifiers

[arya2]

Motivation

To reject transactions in the mempool when they're invalidated by recently committed blocks.

Closes #2631

Solution

• Add the transactions of committed blocks to the ChainTipBlock
• Reject and remove transactions in the mempool that are using spent outpoints or nullifiers from the latest chain tip
• Reject mined transactions

Review

This PR is part of regular scheduled work.

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?
• How do you know it works? Does it have tests?

[codecov]

Codecov Report

Merging #5434 (51b841b) into main (a2dba8c) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5434      +/-   ##
==========================================
- Coverage   79.12%   79.12%   -0.01%     
==========================================
  Files         308      308              
  Lines       39324    39394      +70     
==========================================
+ Hits        31115    31170      +55     
- Misses       8209     8224      +15     

[upbqdn]

This looks good to me.

I don't know how often is poll_ready actually called, so my concern is that if there happen to be two or more blocks mined fast one after another (which can happen), Zebra would skip some blocks. This applies to other checks as well, for example, in remove_same_effects.

[teor2345]

Looks great!

(And it's simpler than I thought 🙂)

I have a minor suggestion about naming things.

[teor2345]

A possible alternative may be to pass the non_finalized_state_receiver to the mempool so that it can check the spent_utxos and nullifiers of all recently committed blocks on the best chain, rather than just the ChainTipBlock.

This isn't needed, because the mempool only updates by one block at a time. If there are multiple blocks, it assumes it has fallen out of sync, and resets itself.

[teor2345]

This is a bug fix. We want to do it to improve the mempool, it's just a higher priority now we're doing mining RPCs.

So don't think we need to put it behind the getblocktemplate-rpcs feature, because it's a fix that we want in production. (And it's ok for the audit as well, because it's more like zcashd.)

[teor2345]

I don't know how often is poll_ready actually called, so my concern is that if there happen to be two or more blocks mined fast one after another (which can happen), Zebra would skip some blocks. This applies to other checks as well, for example, in remove_same_effects.

If Zebra gets multiple blocks between mempool updates, it drops all the mempool transactions, and starts fresh with an empty mempool.

This is implemented using a Reset, rather than a Grow:

zebra/zebra-state/src/service/chain_tip.rs

Lines 419 to 433 in 3825caa

	/// Resets can happen for different reasons:
	/// - a newly created or cloned [`ChainTipChange`], which is behind the
	/// current tip,
	/// - extending the chain with a network upgrade activation block,
	/// - switching to a different best [`Chain`][1], also known as a rollback, and
	/// - receiving multiple blocks since the previous change.
	///
	/// To keep the code and tests simple, Zebra performs the same reset
	/// actions, regardless of the reset reason.
	///
	/// `Reset`s do not have the transaction hashes from the tip block, because
	/// all transactions should be cleared by a reset.
	///
	/// [1]: super::non_finalized_state::Chain
	Reset {

We track this as part of ChainTipChange.last_tip_change():

zebra/zebra-state/src/service/chain_tip.rs

Lines 543 to 549 in 3825caa

	if Some(block.previous_block_hash) != self.last_change_hash
	|| NetworkUpgrade::is_activation_height(self.network, block.height)
	{
	TipAction::reset_with(block)
	} else {
	TipAction::grow_with(block)
	}

[arya2]

I don't know how often is poll_ready actually called, so my concern is that if there happen to be two or more blocks mined fast one after another (which can happen), Zebra would skip some blocks. This applies to other checks as well, for example, in remove_same_effects.

I was also concerned about this.

If Zebra gets multiple blocks between mempool updates, it drops all the mempool transactions, and starts fresh with an empty mempool.

Good to know that's not an issue.

[teor2345]

I think this looks good, just a few minor clarifications.

Do you need help with the test failures?
I know we've done some fixes on the main branch, so I'll update this PR to the latest main.

Edit: it was an unrelated test failure

[teor2345]

@Mergifyio update

[mergify]

update

✅ Branch has been successfully updated

[teor2345]

Ah, the test failure was a ssh issue:

Connection timed out during banner exchange

https://github.com/ZcashFoundation/zebra/actions/runs/3300645552/jobs/5445970948#step:6:112

[teor2345]

Looks good, happy for this to go in without any of my suggestions.
(Or anyone can re-approve once they are done.)