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

Test if the mempool storage is cleared #2815

Merged

Conversation

jvff
Copy link
Contributor

@jvff jvff commented Sep 30, 2021

Motivation

This started as part of the work for #2723, but @teor2345 realized that this had already been implemented by @conradoplg as part of #2786. I scrapped the original implementation, but thought that maybe the property tests I had written might be useful to keep.

Solution

This PR contains two PRs. Both test if the mempool storage should be cleared, but on different events:

  • a chain reset;
  • a chain catching up to the network, when the block synchronizer downloads a large number of blocks again.

Review

@conradoplg and @teor2345. I think the most important thing to decide in this review is if these tests are useful or just redundant.

Reviewer Checklist

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

@jvff jvff requested review from conradoplg and teor2345 September 30, 2021 14:30
@jvff jvff self-assigned this Sep 30, 2021
Copy link
Collaborator

@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 like the tests and they do seem useful 👍

There are just a few minor issues

zebrad/src/components/mempool/tests/vector.rs Show resolved Hide resolved
zebrad/src/components/mempool/tests/prop.rs Outdated Show resolved Hide resolved
zebrad/src/components/mempool/tests/prop.rs Outdated Show resolved Hide resolved
@jvff jvff force-pushed the test-if-mempool-storage-is-cleared branch 2 times, most recently from 74f0f5c to 68ae5ae Compare September 30, 2021 21:05
teor2345
teor2345 previously approved these changes Oct 1, 2021
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, thanks for adding more test coverage!

I want to merge these now, so I can base the block gossip tests on them.

@teor2345 teor2345 dismissed conradoplg’s stale review October 1, 2021 00:25

It looks like Conrado has approved all the changes

@teor2345
Copy link
Contributor

teor2345 commented Oct 1, 2021

Actually, I'm not sure how to resolve these conflicts, so I'll let @jvff do it.

jvff added 9 commits October 1, 2021 13:01
Make it consistent with other test modules and prepare for adding
property tests.
Make it consistent with the general guidelines followed on other
modules.
Allow these types to be used in other crates for testing purposes.
Make it easy to generate random `ChainTipBlock`s for usage in property
tests.
Reduce the repeated test configuration attributes and make it easier to
see what is test specific and what is part of the general
implementation.
Performs a dummy call just so that `poll_ready` gets called.
Replace the custom dummy requests with the helper method.
A chain reset should force the mempool storage to be cleared so that
transaction verification can restart using the new chain tip.
If the block synchronizer falls behind and then starts catching up
again, the mempool should be disabled and therefore the storage should
be cleared.
@jvff jvff force-pushed the test-if-mempool-storage-is-cleared branch from 68ae5ae to c9a2d92 Compare October 1, 2021 14:14
@jvff jvff requested a review from conradoplg October 1, 2021 14:16
@jvff
Copy link
Contributor Author

jvff commented Oct 1, 2021

Merge conflicts solved. @conradoplg, could you please take another look to see if I haven't missed anything? 😅

Copy link
Collaborator

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

Looks good!

@conradoplg conradoplg enabled auto-merge (squash) October 1, 2021 14:24
@conradoplg conradoplg merged commit 50a5728 into ZcashFoundation:main Oct 1, 2021
@jvff jvff deleted the test-if-mempool-storage-is-cleared branch October 1, 2021 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clear mempool storage if the syncer starts syncing large numbers of blocks again
3 participants