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.

Sign up for GitHub

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

Jump to bottom

change(rpc): Avoid re-verifying transactions in blocks if those transactions are in the mempool #8951

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

change(rpc): Avoid re-verifying transactions in blocks if those transactions are in the mempool #8951

wants to merge 7 commits into from

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Oct 19, 2024 •
edited
Loading

Motivation

We want the transaction verifier to avoid re-verifying transactions for the block verifier that have already been verified for the mempool.

Closes #5674.

Solution

  • Adds transaction_hash and known_outpoint_hashes fields on transaction::Request::Block.
  • Adds a mempool request for querying a transaction and its dependencies by its mined id
  • Adds and calls a try_find_verified_unmined_tx() method on the transaction verifier which queries the mempool for the transaction, then checks that all of its dependencies are present in the set of known_outpoint_hashes.

Tests

  • Adds a skips_verification_of_block_transactions_in_mempool test

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 Oct 19, 2024
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Oct 19, 2024
@arya2 arya2 added I-slow Problems with performance or responsiveness A-rpc Area: Remote Procedure Call interfaces P-Low ❄️ and removed C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels Oct 19, 2024
@arya2 arya2 force-pushed the avoid-reverifying-mined-mempool-txs branch from 3633bb6 to 837c077 Compare October 19, 2024 05:10
@github-actions github-actions bot added C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels Oct 19, 2024
@arya2 arya2 changed the title change(rpc): Avoid re-verifying transactions for the block verifier that are present in the mempool change(rpc): Avoid re-verifying transactions in blocks that are present in the mempool Oct 19, 2024
@arya2 arya2 changed the title change(rpc): Avoid re-verifying transactions in blocks that are present in the mempool change(rpc): Avoid re-verifying transactions in blocks if those transactions that are present in the mempool Oct 19, 2024
@arya2 arya2 changed the title change(rpc): Avoid re-verifying transactions in blocks if those transactions that are present in the mempool change(rpc): Avoid re-verifying transactions in blocks if those transactions are in the mempool Oct 19, 2024
@mpguerra mpguerra linked an issue Oct 21, 2024 that may be closed by this pull request
Automatically verify transactions in blocks when the same transaction is already in the mempool #5674
Open
@arya2 arya2 added the do-not-merge Tells Mergify not to merge this PR label Oct 26, 2024
Base automatically changed from verify-orphaned-mempool-txs to main November 18, 2024 12:16
@arya2 arya2 force-pushed the avoid-reverifying-mined-mempool-txs branch from c902901 to 1d63b0e Compare November 19, 2024 06:09
@mpguerra mpguerra removed the do-not-merge Tells Mergify not to merge this PR label Nov 21, 2024
@arya2 arya2 marked this pull request as ready for review November 21, 2024 23:55
@arya2 arya2 requested review from a team as code owners November 21, 2024 23:55
@arya2 arya2 requested review from oxarbitrage and removed request for a team November 21, 2024 23:55
@arya2 arya2 removed the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Nov 22, 2024
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Nov 22, 2024
@arya2 arya2 force-pushed the avoid-reverifying-mined-mempool-txs branch from e1a3691 to e0861ec Compare November 22, 2024 19:53
oxarbitrage
oxarbitrage previously approved these changes Nov 29, 2024
View reviewed changes
Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

I like how the problem is resolved in this pull request. The code and logic looks good and the test too. I left a few minor and optional comments and a question.

zebra-consensus/src/transaction/tests.rs Outdated Show resolved Hide resolved
zebra-consensus/src/transaction/tests.rs Show resolved Hide resolved
zebra-rpc/src/methods/tests/snapshot/get_block_template_rpcs.rs Show resolved Hide resolved
@arya2 arya2 added the do-not-merge Tells Mergify not to merge this PR label Dec 3, 2024
@arya2 arya2 mentioned this pull request Dec 3, 2024
Merged
@arya2 arya2 force-pushed the avoid-reverifying-mined-mempool-txs branch from e0861ec to 4d16805 Compare December 3, 2024 17:37
@arya2 arya2 removed the do-not-merge Tells Mergify not to merge this PR label Dec 3, 2024
@arya2 arya2 added do-not-merge Tells Mergify not to merge this PR and removed do-not-merge Tells Mergify not to merge this PR labels Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oxarbitrage oxarbitrage oxarbitrage left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@arya2 arya2

Labels
A-rpc Area: Remote Procedure Call interfaces C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG I-slow Problems with performance or responsiveness P-Low ❄️
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Automatically verify transactions in blocks when the same transaction is already in the mempool
3 participants
@arya2 @oxarbitrage @mpguerra