Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(consensus): Avoid a concurrency bug when verifying transactions in blocks that are already present in the mempool #9118

Merged
merged 5 commits into from
Jan 31, 2025

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Jan 14, 2025

Motivation

This PR fixes a bug in find_verified_unmined_tx() where a TransparentInputNotFound is returned if a transaction's dependencies are missing from the block containing it. The mempool removes any transaction ids mined onto the best chain from its transaction dependencies, but it's possible for a transaction to depend on outputs that aren't in the same block or in the state, but elsewhere in the verification pipeline waiting to be contextually validated.

Solution

Wait for the UTXOs to arrive in the state instead of returning an error when a transaction's dependencies in the mempool are unavailable in the block.

Related changes:

  • Return TransparentInputNotFound errors from tx verifier instead of InternalDowncastErrors when AwaitUtxo requests timeout, and
  • Rename try_find_verified_unmined_tx to find_verified_unmined_tx

Tests

Updates the skips_verification_of_block_transactions_in_mempool test to check that the tx verifier falls back on waiting for UTXOs to arrive in the state service if a transaction's dependencies are missing from its block.

PR Author's Checklist

  • The PR name will make sense to users.
  • The PR provides a CHANGELOG summary.
  • The solution is tested.
  • The documentation is up to date.
  • The PR has a priority label.

PR Reviewer's Checklist

  • The PR Author's checklist is complete.
  • The PR resolves the issue.

@arya2 arya2 self-assigned this Jan 14, 2025
@arya2 arya2 requested a review from a team as a code owner January 14, 2025 17:49
@arya2 arya2 requested review from upbqdn and removed request for a team January 14, 2025 17:49
@github-actions github-actions bot added C-bug Category: This is a bug C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels Jan 14, 2025
@mpguerra
Copy link
Contributor

Is this the issue that was reported in Sentry?

@arya2
Copy link
Contributor Author

arya2 commented Jan 15, 2025

Is this the issue that was reported in Sentry?

Yep, the issue was that dependencies of transactions in the mempool aren't cleared until they've been committed to the best chain, but there could be multiple blocks being validated in parallel while the mempool is enabled, so the tx verifier needs to wait for those UTXOs as it would if the transaction hadn't already been verified for the mempool.

Copy link

sentry-io bot commented Jan 16, 2025

Sentry Issue: ZEBRAD-T

@arya2 arya2 added A-concurrency Area: Async code, needs extra work to make it work properly. P-Medium ⚡ and removed C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels Jan 16, 2025
Copy link
Member

@upbqdn upbqdn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand it right that the tx dependencies must always originate from the mempool but can be moved to the state by the time we try to check if a tx can depend on them? In that case, should we update these docs: https://github.com/zcashfoundation/zebra/blob/2293cda36dfc74fe0200ff0682f4d2dc75e29881/zebra-node-services/src/mempool/transaction_dependencies.rs#L10-L19?

Should we also update this documentation: https://github.com/zcashfoundation/zebra/blob/2293cda36dfc74fe0200ff0682f4d2dc75e29881/zebra-state/src/request.rs#L691?

zebra-consensus/src/transaction.rs Outdated Show resolved Hide resolved
zebra-consensus/src/transaction.rs Outdated Show resolved Hide resolved
zebra-consensus/src/transaction.rs Outdated Show resolved Hide resolved
@arya2 arya2 requested a review from a team as a code owner January 30, 2025 07:04
@arya2 arya2 requested review from oxarbitrage and removed request for a team January 30, 2025 07:04
@arya2
Copy link
Contributor Author

arya2 commented Jan 30, 2025

Do I understand it right that the tx dependencies must always originate from the mempool but can be moved to the state by the time we try to check if a tx can depend on them?

Yep, tx dependencies are added when validating mempool txs with spends that are missing in the state but available in the mempool, and they're removed when transactions are mined onto the best chain, or if the mempool is reset. I've added a note to hint that the transaction verifier will need to wait for UTXOs to arrive in the state as well. I think it could still be much clearer if you have any suggestions.

mergify bot added a commit that referenced this pull request Jan 31, 2025
@mergify mergify bot merged commit 13e8e79 into main Jan 31, 2025
145 checks passed
@mergify mergify bot deleted the fixes-auto-tx-verification-bug branch January 31, 2025 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-concurrency Area: Async code, needs extra work to make it work properly. C-bug Category: This is a bug C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG P-Medium ⚡
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants