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

Merged
merged 7 commits into from
Dec 19, 2024
Merged

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

merged 7 commits into from
Dec 19, 2024

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
Closed
@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
@arya2 arya2 requested a review from oxarbitrage December 13, 2024 21:21
oxarbitrage
oxarbitrage approved these changes Dec 19, 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.

looks good, i already reviewed most of this code before.

mergify bot added a commit that referenced this pull request Dec 19, 2024
@mergify
Merge of #8951
5d43c48
@mergify mergify bot mentioned this pull request Dec 19, 2024
Closed
@mergify mergify bot merged commit 1974fea into main Dec 19, 2024
147 checks passed
@mergify mergify bot deleted the avoid-reverifying-mined-mempool-txs branch December 19, 2024 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@upbqdn upbqdn upbqdn left review comments

@oxarbitrage oxarbitrage oxarbitrage approved these changes

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
4 participants
@arya2 @upbqdn @oxarbitrage @mpguerra