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

Limit the size and age of the ZIP-401 rejected transaction ID list #2932

Merged
merged 18 commits into from
Oct 27, 2021

Conversation

conradoplg
Copy link
Collaborator

@conradoplg conradoplg commented Oct 21, 2021

Motivation

ZIP-401 specifies that the list of TXIDs must have a maximum size and entries older than a specified threshold must be removed.

Specifications

https://zips.z.cash/zip-0401#specification

Designs

Some of the design was discussed in #2759

Solution

  • Create a EvictedList that encapsulates both behaviours
  • Use the EvictedList for both rejected and expired transactions
    • I'm using in both because that was easier with the current Storage design, and seems harmless (expired transactions will also have a timeout)

I tried creating a generic OrderedMap data structure, that EvictedList would use, in order to organize the code. However I noticed it would be more work since it's tricky to handle/document the behavior that removing a item from the list might not really remove a item from the list (when a value was refreshed and an stale entry is left behind). So I left the entire logic inside EvictedList (where the removal methods are not exposed).

Closes #2759
Closes #2958

Review

Anyone can review. @teor2345 discussed the design.

This may require adjustments after #2889 so it may be better to wait for that to be merged first.

Reviewer Checklist

  • Does it make sense to use EvictList for the expired transaction, or should change Storage so that only rejected list uses it?
  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

Follow Up Work

@teor2345 teor2345 added C-enhancement Category: This is an improvement I-unbounded-growth Zebra keeps using resources, without any limit P-High labels Oct 21, 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.

This looks good, but I think contains_key can return transaction IDs that have expired since the last insert.

I also have a few minor suggestions, feel free to ignore them.

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

Nice catch that we might need to remove multiple entries in insert()!

I think we might need to prune_old() in len() to meet our DoS performance goals.
(I didn't realise this until you fixed len().)

I also think we need to limit the length of ordered_entries, even if its entries haven't expired.

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

We could merge this as-is, but we're waiting on #2889.

I have a few optional suggestions for comments and diagnostics.
(The assertions should never fail, so we might never see these messages.)

zebrad/src/components/mempool/storage/eviction_list.rs Outdated Show resolved Hide resolved
zebrad/src/components/mempool/storage/eviction_list.rs Outdated Show resolved Hide resolved
zebrad/src/components/mempool/storage/eviction_list.rs Outdated Show resolved Hide resolved
teor2345
teor2345 previously approved these changes Oct 26, 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.

There aren't any blockers for merging this PR.

It just needs to be rebased or merged with main after PR #2889 merges.

@conradoplg
Copy link
Collaborator Author

There aren't any blockers for merging this PR.

It just needs to be rebased or merged with main after PR #2889 merges.

Done

conradoplg and others added 2 commits October 27, 2021 12:29
Co-authored-by: teor <teor@riseup.net>
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, let's get it merged!

@teor2345 teor2345 enabled auto-merge (squash) October 27, 2021 20:06
@teor2345 teor2345 merged commit 46fb33a into main Oct 27, 2021
@teor2345 teor2345 deleted the limit-age-rejected-list branch October 27, 2021 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: This is an improvement I-unbounded-growth Zebra keeps using resources, without any limit
Projects
None yet
3 participants