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

[ZIP 401] Addressing mempool denial-of-service #275

Merged
merged 14 commits into from
Nov 7, 2019

Conversation

daira
Copy link
Collaborator

@daira daira commented Sep 16, 2019

Signed-off-by: Daira Hopwood daira@jacaranda.org

Done Criteria:

  • ZIP and implementation are checked for consistency.

@daira daira added the ZIP idea label Sep 16, 2019
@daira daira requested review from gtank, str4d and zebambam September 16, 2019 20:04
@daira daira unassigned gtank and zebambam Sep 16, 2019
@daira daira force-pushed the zip-daira-mempool-limitation branch from 14bd7e7 to ef957b9 Compare September 16, 2019 20:06
gtank
gtank previously requested changes Sep 17, 2019
drafts/zip-daira-mempool-limitation.rst Outdated Show resolved Hide resolved
drafts/zip-daira-mempool-limitation.rst Outdated Show resolved Hide resolved
drafts/zip-daira-mempool-limitation.rst Outdated Show resolved Hide resolved
drafts/zip-daira-mempool-limitation.rst Outdated Show resolved Hide resolved
drafts/zip-daira-mempool-limitation.rst Outdated Show resolved Hide resolved
drafts/zip-daira-mempool-limitation.rst Outdated Show resolved Hide resolved
value five times as likely to be chosen for eviction.

The default value of 80000000 for ``mempool.tx_cost_limit`` represents no more
than 40 blocks’ worth of transactions in the worst case, which is the default
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider expanding this explanation.

drafts/zip-daira-mempool-limitation.rst Outdated Show resolved Hide resolved
drafts/zip-daira-mempool-limitation.rst Outdated Show resolved Hide resolved
drafts/zip-daira-mempool-limitation.rst Outdated Show resolved Hide resolved
@lindanlee
Copy link

I've reviewed this proposal, with the purpose of whether there's anything that the draft could do differently to give users more confidence they know what's happening in the case of network conditions where mempools are full.

I think that this proposal is good!

I don't think that users need to know precisely why the mempool is full (I.e. if there is legitimate transaction volume, or if there is a denial of service attack). My rationale for this is that either of these cases would necessitate that the user take the same course of action--whether that would be sending the transaction again, or to send it with a higher fee. If they would benefit from a different strategy (i.e. if they should not try sending it again if there is a DoS but should try again if it's just full), then the difference should be made to the user.

If my understanding is correct, since the dropped transactions are weighted towards ones with lower fees but randomly selected, a person can try sending the same transaction with no change in fee and have a reasonable chance of not having the transaction dropped again.

I really like the fact that this randomness allows the user to send their transaction by trying the transaction again with the same fee, instead of one that deterministically drops the lowest, which will result in a user trying the transaction again and again with no chance of succeeding. My assumption here is that many users will be using a light wallet that sets the fee for them, or that they do not know how to change the fee.

This proposal also allows for simple automated recovery--i.e. a wallet could try sending a dropped transaction again after a period of time. It's also true that you can automatically retry transactions with a fee increase, but that'd be a bit more difficult to get correctly and require some knowledge of the average fee in the mempool, would require consent from/notice to the user to spend more of their money, and would be hard to get it implemented consistently (which ultimately results in different experiences in different platforms, which is confusing).

drafts/zip-daira-mempool-limitation.rst Outdated Show resolved Hide resolved
drafts/zip-daira-mempool-limitation.rst Outdated Show resolved Hide resolved
drafts/zip-daira-mempool-limitation.rst Outdated Show resolved Hide resolved
drafts/zip-daira-mempool-limitation.rst Outdated Show resolved Hide resolved
@zebambam zebambam requested a review from geffenz September 18, 2019 20:23
Copy link

@geffenz geffenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be noted when transactions get dropped that will put an additional burden on nodes/wallets/exchanges to validate and respond, but the COST logic-structure will help enforce the standard fee.

My main UX concerns are around communication. An automated system like Linda mentioned would be a great next step, but communicating both network status and transaction droppage to a user is also a necessary requirement moving forward.

Since those concerns are outside this ZIP's goal, I approve!

Signed-off-by: Daira Hopwood <daira@jacaranda.org>
@daira daira force-pushed the zip-daira-mempool-limitation branch from 8675fae to 8e1c43b Compare September 26, 2019 15:55
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
@daira daira dismissed zebambam’s stale review September 29, 2019 11:45

Please re-review after changes. Note that I'm on vacation and won't be updating this zip, so please also reassign as necessary.

@daira daira requested a review from gtank September 29, 2019 11:45
@daira daira dismissed gtank’s stale review September 29, 2019 11:46

Please re-review after changes.

@daira daira changed the title Add ZIP Draft: Addressing mempool denial-of-service ZIP 401: Addressing mempool denial-of-service Oct 17, 2019
@daira daira changed the title ZIP 401: Addressing mempool denial-of-service [ZIP 401] Addressing mempool denial-of-service Oct 17, 2019
Copy link

@Eirik0 Eirik0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

@Eirik0 Eirik0 force-pushed the zip-daira-mempool-limitation branch from 8df0e4c to 4d66799 Compare November 7, 2019 19:31
@Eirik0
Copy link

Eirik0 commented Nov 7, 2019

Force pushed because make was not working correctly for me. It would delete index.html rather than rewrite it. I am running ubuntu under WSL, so this issue may be specific to that platform.

Signed-off-by: Daira Hopwood <daira@jacaranda.org>
Copy link
Collaborator Author

@daira daira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

@daira daira merged commit a213e28 into zcash:master Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants