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

Mempool component and storage #2651

Merged
merged 18 commits into from
Aug 27, 2021
Merged

Mempool component and storage #2651

merged 18 commits into from
Aug 27, 2021

Conversation

dconnolly
Copy link
Contributor

@dconnolly dconnolly commented Aug 20, 2021

Motivation

First iteration of the main Mempool as a service that wraps a mempool::Storage manager. The crawler and tx download and verify stream will be internal to this component (privileged) while consumers of mempool will query it via the Request/Response protocol.

Specifications / Designs

https://docs.google.com/document/d/10PP6wKlnS5gannRph_EQ8V9diR7byddf3W2YFZ564FU/edit#heading=h.1v3bcppsa6wq
https://jamboard.google.com/d/1HUhdqx8afJUqgiBdBcjCw0aAfDod9R9KBJeAH24U1Qg/viewer?f=0

Solution

Resolves #2589, #2591, #2620

Review

Reviewer Checklist

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

Follow Up Work

@dconnolly dconnolly changed the base branch from main to mempool-errors August 20, 2021 08:41
@mpguerra mpguerra linked an issue Aug 23, 2021 that may be closed by this pull request
@dconnolly
Copy link
Contributor Author

I think this also resolves #2620 and #2608 and #2607

@dconnolly

This comment has been minimized.

Base automatically changed from mempool-errors to main August 25, 2021 18:39
@dconnolly dconnolly changed the title [WIP] Mempool component Mempool component and storage Aug 25, 2021
@dconnolly dconnolly marked this pull request as ready for review August 25, 2021 20:59
@dconnolly dconnolly force-pushed the mempool-component branch 2 times, most recently from c00a50f to 5b32f5b Compare August 25, 2021 21:17
@oxarbitrage
Copy link
Contributor

I added a very basic test for the storage in 4ea0067 and i fixed a bug in 08269b4 (The drain should be called only after the mempool is full or it will panic).

Another issue i found that maybe was already discussed is that all transactions get moved from the verified into rejected as new transactions get in. This means we can end with a huge amount of them in this state. Is there a purpose for keeping all the ids here ?

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.

Just some typos, clarifications, and name suggestions

zebrad/src/components/mempool.rs Outdated Show resolved Hide resolved
zebrad/src/components/mempool/error.rs Outdated Show resolved Hide resolved
zebrad/src/components/mempool/storage.rs Outdated Show resolved Hide resolved
zebrad/src/components/mempool/storage.rs Outdated Show resolved Hide resolved
zebrad/src/components/mempool/storage.rs Outdated Show resolved Hide resolved
zebrad/src/components/mempool/storage.rs Outdated Show resolved Hide resolved
@teor2345
Copy link
Contributor

Another issue i found that maybe was already discussed is that all transactions get moved from the verified into rejected as new transactions get in. This means we can end with a huge amount of them in this state. Is there a purpose for keeping all the ids here ?

We keep IDs because we don't want to re-download, re-verify, or re-store transactions we've recently rejected.

I've added a task to put a size limit on the rejected list, but I don't think it should block this PR.
(I'd really like to get this PR merged, so we can start connecting it up to other modules.)

Also we'll clear the transactions and rejected IDs every time we reset the mempool, for example, when we switch chain forks. (Approximately every 300 blocks.)

@dconnolly dconnolly enabled auto-merge (squash) August 26, 2021 23:22
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.

I'd like to fix some indexing bugs in the tests before we merge.

zebrad/src/components/mempool/storage/tests.rs Outdated Show resolved Hide resolved
zebrad/src/components/mempool/tests.rs Outdated Show resolved Hide resolved
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.

All comments were addressed, i think we can do more work in here but is good enough IMHO to get started so i will merge it as it is also blocking other PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants