Add transaction downloader and verifier

[conradoplg]

Motivation

We need to download transactions and verify them in order to implement the mempool.

Closes #2637

Specifications

Designs

Solution

This basically copies zebrad/src/components/inbound/downloads.rs and adapts it to work with transactions.

Currently any AdvertiseTransactionIds (inv) are downloaded and verified, and the result is just logged.

Review

@teor2345 already reviewed the draft so they may want to finish it, otherwise @dconnolly can review it

Reviewer Checklist

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

Follow Up Work

What's missing and will be done in separate PRs / issues:

• Checking if the tx is already in the mempool/state before downloading&verifying Skip download and verification if the transaction is already in the mempool or state #2708
• Checking if we synced to the tip before accepting transactions Activate the download and verify stream when the mempool is activated #2628 & Cancel download and verify tasks when the mempool is deactivated #2629
• The Elapsed error issue I described in Add transaction downloader and verifier #2679 (comment)
  • part of Dedupe Verification error types (Block, Chain) #1186
• Tests (can be done in Add debug configs for mempool acceptance tests #2690 and Add zebrad acceptance tests for the mempool #2691 )
• Handling pushed txs (will be done in Verify inbound PushTransactions #2692)
• Ensuring we validate any mempool-specific rules (will be done in Validate mempool-specific transaction consensus rules #2707. In this PR I've just added the check for coinbase inputs)

[teor2345]

There are zebrad/src/components/sync/downloads.rs and zebrad/src/components/inbound/downloads.rs. The sync version is used for blocks that Zebra itself downloads, while the inbound is used for pushed/gossiped blocks. They are very similar. So the question is:

• Should I create two Downloads for transactions, one e.g. in inbound/tx_downloads.rs and the other in mempool/downloads.rs? Or should I take the chance to dedupe it and try to use the same implementation for both purposes?

I suggest a single mempool downloader, based on inbound/downloads.rs, because it has these features:

• skipping duplicate downloads
• limiting the size of the download queue

I don't think there's any reason to have multiple downloaders for different sources of transaction IDs.
If we do need to distinguish requests from different sources, we should consider having a request enum, so we don't have to de-duplicate multiple streams.

• Should I also dedupe the existing downloads.rs?

This is a good idea, because it helps with de-duplicating block downloads - #862.
(I've been wanting to do something like this for a while.)

But our goal is to have a minimal working mempool over the next 3 sprints.
So I think this is a change we can do later, I've updated #862 with this change.

Another question:

• Which height do I pass to transaction::Request::Mempool? The height of the best chain tip?

The height of the next block after the best chain tip:

zebra/zebra-consensus/src/transaction.rs

Lines 82 to 90 in a4b0b46

	Mempool {
	/// The transaction itself.
	transaction: UnminedTx,
	/// The height of the next block.
	///
	/// The next block is the first block that could possibly contain a
	/// mempool transaction.
	height: block::Height,
	},

I'm happy to check back on this ticket occasionally, but I'd like @dconnolly or someone else to take the lead on this review.

[conradoplg]

Thanks @teor2345!

Current status:

I've changed it to be more similar to the inbound downloader.

It's now mostly done, and seems to be working. (You can set the zebrad log for debug and search for verified transaction for the mempool), or use grafana. I've included a dashboard, but be aware it's the first time I've created one 😅

What's missing & questions:

• Need to handle PushTransaction messages, which will require a verify method instead of download_and_verify
  • Apparently they are very rare so maybe we can get away with not implementing it?
• Do we need to wait to be near the tip before accepting transactions? (I think we do)
• The block download checks if the block is already in the state before proceeding. Should we do the same? (i.e. check if the transaction is already in the mempool before downloading and verifying it?) I think we should
• I need help defining MAX_INBOUND_CONCURRENCY, TRANSACTION_DOWNLOAD_TIMEOUT, TRANSACTION_VERIFY_TIMEOUT. I believe they can have the same values used for blocks, but I'd like to double check.
• There's a weird issue that some transaction verifications fail with Err(Elapsed(())) (which seems OK) while a few fail with Err(InternalDowncastError("downcast to known transaction error type failed, original error: Elapsed(())")). Not sure what's happening.
• Need to try writing some tests

Here's what the dashboard looks like

[teor2345]

What's missing & questions:

• Need to handle PushTransaction messages, which will require a verify method instead of download_and_verify
  • Apparently they are very rare so maybe we can get away with not implementing it?

I think PushTransaction (tx) messages are used by wallets to announce new transactions.
So I think we'll need to support them eventually.

But we can add them in another PR. (I created ticket #2692 for this.)

[teor2345]

• Do we need to wait to be near the tip before accepting transactions? (I think we do)

Yes, using SyncStatus from PR #2685.

But this can happen in another PR, because SyncStatus currently always returns ready.
See tickets #2628 and #2629.

If the syncer isn't near the tip, we should ignore all pushed transactions and transaction IDs. We don't have enough information to get reliable verification results.

[teor2345]

• The block download checks if the block is already in the state before proceeding. Should we do the same? (i.e. check if the transaction is already in the mempool before downloading and verifying it?) I think we should

Yes, before we:

• download transaction IDs, or
• verify downloaded transactions or PushTransactions,

we need to check if the transaction ID is in any of:

• the mempool transactions
• the mempool rejected IDs list
• the best chain in the state (non-finalized or finalized) using Request::Transaction
  • for v5 transactions, all transactions with the same non-malleable ID (mined_id) should be rejected

(I opened #2689 to improve the performance of Request::Transaction in the finalized state.)

[teor2345]

• I need help defining MAX_INBOUND_CONCURRENCY, TRANSACTION_DOWNLOAD_TIMEOUT, TRANSACTION_VERIFY_TIMEOUT. I believe they can have the same values used for blocks, but I'd like to double check.

The current values seem fine, but we should document the rationale, just in case there are timeout issues later.
I wrote detailed comments on that part of the code.

[teor2345]

• There's a weird issue that some transaction verifications fail with Err(Elapsed(())) (which seems OK) while a few fail with Err(InternalDowncastError("downcast to known transaction error type failed, original error: Elapsed(())")). Not sure what's happening.

That error comes from this code, where we need to add a specific case for each downcast:

zebra/zebra-consensus/src/error.rs

Lines 92 to 94 in 4240950

	impl From<BoxError> for TransactionError {
	fn from(mut err: BoxError) -> Self {
	// TODO: handle redpallas::Error, ScriptInvalid, InvalidSignature

These errors would be clearer if we mapped each timeout to a specific error variant:

zebra/zebra-consensus/src/script.rs

Line 44 in 4240950

state: Timeout::new(state, UTXO_LOOKUP_TIMEOUT),

zebra/zebrad/src/components/inbound.rs

Lines 178 to 187 in 8cc6eef

	let downloads = Box::pin(Downloads::new(
	Timeout::new(outbound.clone(), BLOCK_DOWNLOAD_TIMEOUT),
	Timeout::new(verifier, BLOCK_VERIFY_TIMEOUT),
	self.state.clone(),
	));
	let tx_downloads = Box::pin(TxDownloads::new(
	Timeout::new(outbound, TRANSACTION_DOWNLOAD_TIMEOUT),
	Timeout::new(tx_verifier, TRANSACTION_VERIFY_TIMEOUT),
	self.state.clone(),
	));

This specific issue looks like a race between the transaction verifier timeout and the UTXO timeout:

zebra/zebrad/src/components/sync.rs

Line 125 in 8cc6eef

pub(super) const TRANSACTION_VERIFY_TIMEOUT: Duration = Duration::from_secs(180);

zebra/zebra-consensus/src/script.rs

Line 23 in 4240950

const UTXO_LOOKUP_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(3 * 60);

Feel free to do some quick fixes here, or to open another ticket.

[teor2345]

• Need to try writing some tests

👍

But don't worry about it too much in this PR.

It might be hard to get all the right context in place for mempool download and verify tests. We ended up testing the syncer and inbound service using zebrad acceptance tests. So I just opened tickets #2690 and #2691 for tests and test configs.

[teor2345]

Looks good so far.

Here are a bunch of details that might be helpful, including minor fixes and comment tweaks.

[conradoplg]

I've addressed comments.

I'm working on checking for existing txs in mempool/state but that requires passing some stuff around so I'll leave it to a separate a PR to keep this one simple, so I'm moving this out of draft.

[teor2345]

I opened or found tickets for all of the follow up work

[teor2345]

Blocker: Zebra panics with:

internal error: entered unreachable code: Invalid transaction version: after Overwinter activation transaction versions 1 and 2 are rejected

https://github.com/ZcashFoundation/zebra/pull/2679/checks?check_run_id=3466894074#step:12:689

It looks like we're not checking that v4 transactions are valid for the requested network upgrade. We'll need to add a check like verify_v5_transaction_network_upgrade, but for v4 transactions.

It should be the first check we do on v4 transactions.

[conradoplg]

Blocker: Zebra panics with:

internal error: entered unreachable code: Invalid transaction version: after Overwinter activation transaction versions 1 and 2 are rejected

https://github.com/ZcashFoundation/zebra/pull/2679/checks?check_run_id=3466894074#step:12:689

It looks like we're not checking that v4 transactions are valid for the requested network upgrade. We'll need to add a check like verify_v5_transaction_network_upgrade, but for v4 transactions.

It should be the first check we do on v4 transactions.

Should be solved in d90463c. Also (partially?) solves #2480

[conradoplg]

I've also added a block_ prefix to downloads and verifier to better distinguish from the new tx_ ones, in a3b73f5

[teor2345]

I have a few very minor suggestions, but no blockers any more.

Happy to approve or to let someone else take a look.

Blocking panic was resolved

[teor2345]

I think we're good to go here.