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

Send mined transaction IDs to the download/verify task for cancellation #2786

Merged
merged 8 commits into from
Sep 30, 2021
Merged

Send mined transaction IDs to the download/verify task for cancellation #2786

merged 8 commits into from
Sep 30, 2021

Conversation

conradoplg
Copy link
Collaborator

@conradoplg conradoplg commented Sep 21, 2021 •
edited
Loading

Motivation

When a block is mined, there is no reason for the mempool to keep downloading/verifying transactions with the same ID, since they won't be mined again. Therefore, cancel any downloads with those IDs.

Specifications

Designs

Solution

Use ChainTipChange to check when a block was mined and for each of its transactions, cancel any outstanding download/verify tasks for that txid.

Closes #2711

Review

This is a draft for now because it may depend on #2777
It's also lacking tests.

Reviewer Checklist

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

Follow Up Work

teor2345
teor2345 reviewed Sep 22, 2021
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks like a well-structured PR.

I think I've mostly commented about performance issues you're already aware of.

Happy to re-review after #2777 merges.

conradoplg
conradoplg commented Sep 22, 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.

Answered some comments, but will still work further on this

@conradoplg conradoplg force-pushed the send-mined-txids-for-cancellation-2711 branch from 3239f9c to eba7315 Compare September 29, 2021 19:26
conradoplg
conradoplg commented Sep 29, 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.

I fixed the test and solved conflicts, thanks a lot @teor2345 and @jvff!

@conradoplg conradoplg marked this pull request as ready for review September 29, 2021 19:33
@conradoplg conradoplg force-pushed the send-mined-txids-for-cancellation-2711 branch from b973272 to 13bc5d2 Compare September 29, 2021 20:26
teor2345
teor2345 approved these changes Sep 30, 2021
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks great!

The new code is readable and efficient, and the tests check that it works.

@teor2345 teor2345 merged commit 18acec6 into main Sep 30, 2021
@teor2345 teor2345 deleted the send-mined-txids-for-cancellation-2711 branch September 30, 2021 02:09
@jvff jvff mentioned this pull request Sep 30, 2021
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jvff jvff jvff left review comments

@teor2345 teor2345 teor2345 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.

Send mined transaction IDs to the download and verify task for cancellation
3 participants
@conradoplg @jvff @teor2345