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

change(mempool): Re-verify mempool transactions after a chain fork, rather than re-downloading them all #5841

Merged
merged 8 commits into from
Dec 12, 2022

Conversation

teor2345
Copy link
Contributor

Motivation

Zebra drops all its mempool transactions when there is a chain fork. But it would be faster and put less load on the network if we re-verified them instead.

I discovered this bug while testing the getblocktemplate RPC.

Complex Code or Requirements

We need to make sure we clear the mempool, then re-verify all the stored or pending transactions.

Otherwise, we could break a consensus rule.

Solution

Related bug fixes:

Related cleanups:

  • Use a marker struct for cancelling mempool tasks
  • Make mempool proptests easier to debug

This doesn't need to be behind a feature, because it's a bug fix for the production mempool as well.

Review

This is a routine bug fix.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

@teor2345 teor2345 added A-consensus Area: Consensus rule updates C-enhancement Category: This is an improvement P-Medium ⚡ I-slow Problems with performance or responsiveness I-remote-node-overload Zebra can overload other nodes on the network A-rpc Area: Remote Procedure Call interfaces A-concurrency Area: Async code, needs extra work to make it work properly. labels Dec 12, 2022
@teor2345 teor2345 requested a review from a team as a code owner December 12, 2022 00:46
@teor2345 teor2345 self-assigned this Dec 12, 2022
@teor2345 teor2345 requested a review from a team as a code owner December 12, 2022 00:46
@teor2345 teor2345 requested review from oxarbitrage and removed request for a team December 12, 2022 00:46
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Dec 12, 2022
@teor2345 teor2345 removed the request for review from a team December 12, 2022 01:27
@codecov
Copy link

codecov bot commented Dec 12, 2022

Codecov Report

Merging #5841 (3cfde8c) into main (f1e3fda) will increase coverage by 0.01%.
The diff coverage is 68.42%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5841      +/-   ##
==========================================
+ Coverage   78.70%   78.72%   +0.01%     
==========================================
  Files         308      308              
  Lines       38786    38836      +50     
==========================================
+ Hits        30528    30573      +45     
- Misses       8258     8263       +5     

@teor2345 teor2345 removed the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Dec 12, 2022
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, thanks for the fix.

@mergify mergify bot deleted the re-verify-mempool-on-fork branch December 12, 2022 23:19
teor2345 added a commit that referenced this pull request Feb 6, 2023
…ather than re-downloading them all (#5841) - git cherry-pick --strategy=ort --strategy-option=ours

* Move Drop from mempool::ActiveState to mempool::Downloads, to avoid bugs

* Re-verify mempool transactions after a fork

And add a marker struct for mempool download cancellation.

* Update README based on recent mitigations for some issues, tidy format

* Make mempool proptests easier to debug

* Make UnminedTx Display text much smaller

* Update tests for mempool transaction re-verification after forks

* Retry all stored and pending transactions

* Fix a test to check for mempool reset retries
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. A-consensus Area: Consensus rule updates A-rpc Area: Remote Procedure Call interfaces C-enhancement Category: This is an improvement I-remote-node-overload Zebra can overload other nodes on the network I-slow Problems with performance or responsiveness
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants