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

Refactor spend conflict checks in the mempool storage to increase performance #2784

Closed
Tracked by #2744 ...
jvff opened this issue Sep 21, 2021 · 4 comments · Fixed by #2826
Closed
Tracked by #2744 ...

Refactor spend conflict checks in the mempool storage to increase performance #2784

jvff opened this issue Sep 21, 2021 · 4 comments · Fixed by #2826
Assignees
Labels
C-enhancement Category: This is an improvement S-needs-investigation Status: Needs further investigation

Comments

@jvff
Copy link
Contributor

jvff commented Sep 21, 2021

Motivation

ZIP-401 prevents memory DoS by setting a maximum mempool size. This mempool size is much larger than Zebra's current mempool size.

PR #2765 implemented a simple algorithm to check if a transaction has spend conflicts with another transaction already in the mempool.

That algorithm works well for a mempool with few transactions. But as the size of the mempool increases, performance will be impacted. The algorithm is O(n*m), where n is the number of spent UTXOs and revealed nullifiers in the transaction and m is the number of spent UTXOs and revealed nullifiers in all transactions already in the mempool.

So as part of implementing ZIP-401, we need to make the spend conflict checks more efficient. Otherwise, we replace a memory DoS with a CPU DoS.

Specifications

Adoption of this proposal would increase robustness of Zcash nodes against denial-of-service attack, in particular attacks that attempt to exhaust node memory.

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

The default value of 80000000 for mempooltxcostlimit represents no more than 40 blocks’ worth of transactions in the worst case, which is the default expiration height after the Blossom network upgrade.

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

Designs

The original idea was to to cache the spent UTXOs and revealed nullifiers from all transactions in
the mempool using HashSets. This would mean that the conflict check would only be O(n), but it adds complexity to the code to add a transaction to the mempool and to remove a transaction from the mempool, because the caches must be updated.

If this idea is implemented, the has_spend_conflicts method could be updated to something like:

fn has_spend_conflicts<'slf, 'tx, Extractor, Outputs, Output>(
    &'slf self,
    tx: &'tx UnminedTx,
    extractor: Extractor,
    existing_outputs: &'slf HashSet<Output>,
) -> bool
where
    'slf: 'tx,
    Extractor: Fn(&'tx Transaction) -> Outputs,
    Outputs: IntoIterator<Item = Output>,
    Output: Eq + Hash + 'tx,
{
    let new_outputs = extractor(&tx.transaction).into_iter();

    new_outputs.any(|output| existing_outputs.contains(output))
}
@jvff jvff added C-enhancement Category: This is an improvement S-needs-triage Status: A bug report needs triage labels Sep 21, 2021
@teor2345 teor2345 added this to the 2021 Sprint 19 milestone Sep 21, 2021
@teor2345 teor2345 added the S-needs-investigation Status: Needs further investigation label Sep 21, 2021
@teor2345
Copy link
Contributor

If we need to look up the conflicting transaction, we might want a HashMap<Output, transaction::Hash>.

But I don't know if we'll need to do that - I guess we'll find out!

@teor2345
Copy link
Contributor

We moved the eviction tickets to the next sprint, so I'm going to move this one too.

@mpguerra
Copy link
Contributor

@mpguerra
Copy link
Contributor

mpguerra commented Oct 4, 2021

This definitely seems optional, so I'm going to un-schedule it for now, it will still be tracked in the mempool epic for when we think the mempool will be large enough that this is necessary

@mpguerra mpguerra removed this from the 2021 Sprint 20 milestone Oct 4, 2021
@mpguerra mpguerra added the P-Low label Oct 4, 2021
@mpguerra mpguerra added this to the 2021 Sprint 20 milestone Oct 4, 2021
@mpguerra mpguerra removed P-Low S-needs-triage Status: A bug report needs triage labels Oct 4, 2021
@jvff jvff self-assigned this Oct 5, 2021
@mpguerra mpguerra linked a pull request Oct 6, 2021 that will close this issue
3 tasks
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 S-needs-investigation Status: Needs further investigation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants