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

Restore and update mempool tests #2966

Merged
merged 7 commits into from
Oct 28, 2021
Merged

Restore and update mempool tests #2966

merged 7 commits into from
Oct 28, 2021

Conversation

conradoplg
Copy link
Collaborator

Motivation

See #2959

Specifications

Designs

Solution

Restore and update the tests. In most cases, instead of working with a transaction count, a cost limit was derived by summing the costs of some of the transactions that will be inserted.

Closes #2959

Review

I just focused on restoring the tests and still want to think a bit more about if these tests still test everything that we want or if something is missing. But since we want to get this done soon I'm opening the PR as is. Feel free to suggest additions, otherwise I can also amend the tests in a later PR.

Anyone can review, but @teor2345 and/or @dconnolly might be particularly interested.

This seems to be one of the last pending issues for the beta so it's best to review it soon.

Reviewer Checklist

  • All deleted tests were restored
  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

Follow Up Work

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 good to me, thanks for making this happen!

@teor2345 teor2345 enabled auto-merge (squash) October 28, 2021 01:09
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.

It looks like one of the tests sometimes fails:

---- components::mempool::storage::tests::vectors::mempool_storage_basic stdout ----

The application panicked (crashed).
Message:  assertion failed: !some_rejected_transactions.is_empty()
Location: zebrad/src/components/mempool/storage/tests/vectors.rs:104

https://github.com/ZcashFoundation/zebra/runs/4029234056?check_suite_focus=true#step:10:1124

This intermittent test failure is a blocker for merging this PR.

@teor2345
Copy link
Contributor

The same failure also happened in the coverage build, along with:

 ---- components::mempool::tests::vector::mempool_queue stdout ----
Error: 
   0: mempool storage has a cached chain rejection for any transaction with the same effects
   1: transaction evicted from the mempool due to ZIP-401 denial of service limits

Location:
   /rustc/e269e6bf47f40c9046cd44ab787881d700099252/library/core/src/result.rs:1915

@teor2345 teor2345 disabled auto-merge October 28, 2021 01:56
@teor2345
Copy link
Contributor

If the randomised tests are quick, we might want to run them multiple times, to make sure we catch any intermittent failures.

@conradoplg
Copy link
Collaborator Author

If the randomised tests are quick, we might want to run them multiple times, to make sure we catch any intermittent failures.

Good idea! I did that and fixed the tests in ca21735

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'm fine with these retries for now 👍

@dconnolly dconnolly requested a review from teor2345 October 28, 2021 18:59
@oxarbitrage oxarbitrage dismissed teor2345’s stale review October 28, 2021 20:12

teor approved then requested changes for failing tests that are now fixed

@oxarbitrage oxarbitrage enabled auto-merge (squash) October 28, 2021 20:12
@oxarbitrage oxarbitrage merged commit df65b8c into main Oct 28, 2021
@oxarbitrage oxarbitrage deleted the update-mempool-tests branch October 28, 2021 20:55
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.

Update mempool tests after ZIP-401 changes
4 participants