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

Skip download and verification if the transaction is already in the mempool or state #2718

Merged
merged 13 commits into from
Sep 8, 2021
Merged

Skip download and verification if the transaction is already in the mempool or state #2718

merged 13 commits into from
Sep 8, 2021

Conversation

conradoplg
Copy link
Collaborator

@conradoplg conradoplg commented Aug 31, 2021 •
edited
Loading

Motivation

We should only download/verify a transaction if it's not already in the mempool or state.

Specifications

Designs

Solution

Pass a mempool reference to the downloader. Query mempool and state to skip download if applicable.

This adds a new request to the mempool to check if a tx was rejected.

Closes #2708

Review

Since this modifies the mempool instantiation and adds a new request, it would be good to review soon to avoid conflicts.

Reviewer Checklist

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

Follow Up Work

Handling pushed transactions in #2727

Base automatically changed from transaction-sync-verifier to main September 2, 2021 00:06
@mpguerra mpguerra linked an issue Sep 2, 2021 that may be closed by this pull request
Skip download and verification if the transaction is already in the mempool or state #2708
Closed
@conradoplg conradoplg marked this pull request as ready for review September 2, 2021 14:59
@conradoplg conradoplg mentioned this pull request Sep 2, 2021
Merged
3 tasks
@mpguerra mpguerra requested a review from dconnolly September 7, 2021 08:59
oxarbitrage
oxarbitrage reviewed Sep 7, 2021
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 think it will be good to make the tests, the rest looks all good to me.

zebrad/src/commands/start.rs Show resolved Hide resolved
zebrad/src/commands/start.rs Outdated Show resolved Hide resolved
zebrad/src/components/inbound.rs Outdated Show resolved Hide resolved
zebrad/src/components/inbound/tests.rs Outdated Show resolved Hide resolved
zebrad/src/components/mempool.rs Show resolved Hide resolved
zebrad/src/components/mempool/storage.rs Show resolved Hide resolved
conradoplg
conradoplg commented Sep 8, 2021
View reviewed changes
Copy link
Collaborator Author

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

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

Addressed some comments, I won't touch the tests for now since @oxarbitrage offered to help with them as discussed in Discord (thanks!)

zebrad/src/commands/start.rs Show resolved Hide resolved
zebrad/src/commands/start.rs Outdated Show resolved Hide resolved
zebrad/src/components/inbound.rs Outdated Show resolved Hide resolved
zebrad/src/components/inbound/tests.rs Outdated Show resolved Hide resolved
@oxarbitrage oxarbitrage mentioned this pull request Sep 8, 2021
Closed
dconnolly
dconnolly approved these changes Sep 8, 2021
View reviewed changes
Copy link
Contributor

@dconnolly dconnolly left a comment

Choose a reason for hiding this comment

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

I have left a comment that's non-blocking but just wanted to double-check, otherwise looks great!

zebrad/src/components/mempool/downloads.rs Show resolved Hide resolved
@dconnolly dconnolly enabled auto-merge (rebase) September 8, 2021 18:19
@dconnolly dconnolly disabled auto-merge September 8, 2021 18:19
@dconnolly dconnolly enabled auto-merge (squash) September 8, 2021 18:19
@dconnolly dconnolly merged commit a2993e8 into main Sep 8, 2021
@dconnolly dconnolly deleted the check-existing-txs branch September 8, 2021 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teor2345 teor2345 teor2345 left review comments

@oxarbitrage oxarbitrage oxarbitrage approved these changes

@dconnolly dconnolly dconnolly approved these changes

Assignees

@conradoplg conradoplg

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Skip download and verification if the transaction is already in the mempool or state
4 participants
@conradoplg @dconnolly @teor2345 @oxarbitrage