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

DoS protection: Weighted random drop of txs if mempool full #4145

Merged
merged 29 commits into from Oct 21, 2019

Conversation

@Eirik0
Copy link
Contributor

Eirik0 commented Sep 26, 2019

No description provided.

@Eirik0 Eirik0 requested review from str4d, daira and LarryRuane Sep 26, 2019
@Eirik0 Eirik0 added this to Needs Prioritization in Arborist Team via automation Sep 26, 2019
@Eirik0 Eirik0 added this to the v2.1.0 milestone Sep 26, 2019
@Eirik0 Eirik0 requested a review from ebfull Sep 26, 2019
@Eirik0 Eirik0 moved this from Needs Prioritization to Security Issue Backlog in Arborist Team Sep 26, 2019
@Eirik0 Eirik0 moved this from Security Issue Backlog to In Progress in Arborist Team Sep 26, 2019
@Eirik0 Eirik0 added the SECURITY label Sep 26, 2019
Copy link
Contributor

daira left a comment

I didn't do a full review, but this looks to be along the right lines.

qa/rpc-tests/mempool_limit.py Outdated Show resolved Hide resolved
qa/rpc-tests/mempool_limit.py Outdated Show resolved Hide resolved
qa/rpc-tests/mempool_limit.py Outdated Show resolved Hide resolved
qa/rpc-tests/mempool_limit.py Outdated Show resolved Hide resolved
src/gtest/test_mempoollimit.cpp Outdated Show resolved Hide resolved
src/gtest/test_mempoollimit.cpp Outdated Show resolved Hide resolved
src/gtest/test_mempoollimit.cpp Show resolved Hide resolved
src/init.cpp Outdated Show resolved Hide resolved
@Eirik0 Eirik0 changed the title [WIP] DoS protection: Weighted random drop of txs if mempool full DoS protection: Weighted random drop of txs if mempool full Sep 27, 2019
@Eirik0 Eirik0 moved this from In Progress to In Review in Arborist Team Sep 27, 2019
@Eirik0

This comment has been minimized.

Copy link
Contributor Author

Eirik0 commented Sep 27, 2019

This is the implementation for zcash/zips#275

src/init.cpp Outdated Show resolved Hide resolved
src/init.cpp Outdated Show resolved Hide resolved
src/init.cpp Outdated Show resolved Hide resolved
src/init.cpp Outdated Show resolved Hide resolved
src/Makefile.am Outdated Show resolved Hide resolved
src/txmempool.cpp Outdated Show resolved Hide resolved
src/init.cpp Outdated Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
src/txmempool.cpp Outdated Show resolved Hide resolved
src/txmempool.cpp Outdated Show resolved Hide resolved
@Eirik0 Eirik0 force-pushed the Eirik0:limit-mempool branch from 4968b73 to a515b00 Oct 7, 2019
@Eirik0

This comment has been minimized.

Copy link
Contributor Author

Eirik0 commented Oct 7, 2019

I rewrote the representation of the collection of transactions/weights because I was concerned about performance when removing transactions from the mempool. In the first iteration we rebuild the complete list of weighted transactions each time a transaction was removed from the mempool. Now, we store the transactions in a tree and are able to add and remove transactions from the collection in logarithmic time.

I also did some more cleanup and rebased on to master now that #4060 has been merged.

@Eirik0 Eirik0 requested review from str4d and zebambam Oct 7, 2019
Copy link
Contributor

zebambam left a comment

I don't understand some of the locking and also the considerations that go into the RecentlyEvictedList constants. Would be great if we can talk about those so that I can give this a better review.

src/mempoollimit.cpp Outdated Show resolved Hide resolved
pruneList();
if (txIdsAndTimes[txIdsAndTimesIndex].is_initialized()) {
auto txIdAndTime = txIdsAndTimes[txIdsAndTimesIndex];
txIdSet.erase(txIdAndTime.get().first);

This comment has been minimized.

Copy link
@zebambam

zebambam Oct 7, 2019

Contributor

So.. we just overwrite anything existing in there? This seems like someone could fill up the RecentlyEvictedList with permutations of (malleable) transactions and we still end up in DoS. What have I missed?

This comment has been minimized.

Copy link
@ebfull

ebfull Oct 9, 2019

Contributor

Note that the RecentlyEvictedList is not intended to mitigate intentional DoS.

This comment has been minimized.

Copy link
@str4d

str4d Oct 9, 2019

Contributor

It's a ring buffer, so by design it eventually overwrites itself if it is being filled more quickly than the entries expire. RecentlyEvictedList clearly can't be an unbounded buffer, or it would itself be a DoS vector. The question is how large the buffer's bound should be.

This comment has been minimized.

Copy link
@Eirik0

Eirik0 Oct 9, 2019

Author Contributor

We could make the size of the recently evicted list configurable, but I am not sure about how beneficial that would be.

#include "uint256.h"

const size_t DEFAULT_MEMPOOL_TOTAL_WEIGHT_LIMIT = 80000000;
const int64_t DEFAULT_MEMPOOL_EVICTION_MEMORY_MINUTES = 60;

This comment has been minimized.

Copy link
@zebambam

zebambam Oct 7, 2019

Contributor

I don't understand the considerations that went into this figure.

This comment has been minimized.

Copy link
@daira

daira Oct 9, 2019

Contributor

They're described in the draft ZIP.

src/mempoollimit.h Outdated Show resolved Hide resolved
src/mempoollimit.h Outdated Show resolved Hide resolved
Copy link
Contributor

ebfull left a comment

Just did a brief first pass...

I think there need to be more comments explaining the purpose of each piece of the API and the algorithms. As it stands I have to reverse engineer the intentions behind much of the code and understand each edge case you are addressing.

void WeightedTxTree::remove(const uint256& txId)
{
if (txIdToIndexMap.find(txId) == txIdToIndexMap.end()) {
return;

This comment has been minimized.

Copy link
@ebfull

ebfull Oct 9, 2019

Contributor

This should never happen anyway, right?

This comment has been minimized.

Copy link
@Eirik0

Eirik0 Oct 9, 2019

Author Contributor

In testing I found that this does actually happen - somewhere remove is being called on transactions that have already been removed. I did not research where or why, but as it stands this is necessary.

This comment has been minimized.

Copy link
@daira

daira Oct 9, 2019

Contributor

Please file a ticket to investigate this.

This comment has been minimized.

Copy link
@Eirik0

Eirik0 Oct 17, 2019

Author Contributor

I created #4159, but I question how much time it is worth spending on this.

src/mempoollimit.cpp Outdated Show resolved Hide resolved
src/mempoollimit.cpp Outdated Show resolved Hide resolved
WeightedTxInfo WeightedTxInfo::from(const CTransaction& tx, const CAmount& fee)
{
size_t memUsage = RecursiveDynamicUsage(tx);
memUsage += tx.vJoinSplit.size() * JOINSPLIT_SIZE;

This comment has been minimized.

Copy link
@ebfull

ebfull Oct 9, 2019

Contributor

Should these instead be folded into RecursiveDynamicUsage?

This comment has been minimized.

Copy link
@str4d

str4d Oct 9, 2019

Contributor

Yes, they should be (this is another example of inherited logic we've not fully integrated Zcash changes into).

Also, JOINSPLIT_SIZE now depends on the network upgrade level (which means that we computing it, we should amend the computation to trigger the post-Sapling serialization logic in the same way we amend the logic in the transaction serialization itself).

This comment has been minimized.

Copy link
@Eirik0

Eirik0 Oct 9, 2019

Author Contributor

Should this be fixed as part of this issue, or can it be done as follow up?

This comment has been minimized.

Copy link
@Eirik0

Eirik0 Oct 9, 2019

Author Contributor

If we did this as follow up, we could also incorporate this in to z_mergetoaddress which does its own tx size estimation.

src/mempoollimit.cpp Outdated Show resolved Hide resolved
src/mempoollimit.cpp Outdated Show resolved Hide resolved
qa/rpc-tests/mempool_limit.py Outdated Show resolved Hide resolved
qa/rpc-tests/mempool_limit.py Outdated Show resolved Hide resolved
qa/rpc-tests/mempool_limit.py Outdated Show resolved Hide resolved
qa/rpc-tests/mempool_limit.py Show resolved Hide resolved
qa/rpc-tests/mempool_limit.py Outdated Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
src/gtest/test_mempoollimit.cpp Outdated Show resolved Hide resolved
src/txmempool.h Outdated Show resolved Hide resolved
src/init.cpp Outdated Show resolved Hide resolved
src/txmempool.h Outdated Show resolved Hide resolved
src/mempoollimit.h Outdated Show resolved Hide resolved
src/mempoollimit.cpp Outdated Show resolved Hide resolved
src/mempoollimit.cpp Outdated Show resolved Hide resolved
src/mempoollimit.h Outdated Show resolved Hide resolved
WeightedTxInfo WeightedTxInfo::from(const CTransaction& tx, const CAmount& fee)
{
size_t memUsage = RecursiveDynamicUsage(tx);
memUsage += tx.vJoinSplit.size() * JOINSPLIT_SIZE;

This comment has been minimized.

Copy link
@str4d

str4d Oct 9, 2019

Contributor

Yes, they should be (this is another example of inherited logic we've not fully integrated Zcash changes into).

Also, JOINSPLIT_SIZE now depends on the network upgrade level (which means that we computing it, we should amend the computation to trigger the post-Sapling serialization logic in the same way we amend the logic in the transaction serialization itself).

src/mempoollimit.cpp Outdated Show resolved Hide resolved
pruneList();
if (txIdsAndTimes[txIdsAndTimesIndex].is_initialized()) {
auto txIdAndTime = txIdsAndTimes[txIdsAndTimesIndex];
txIdSet.erase(txIdAndTime.get().first);

This comment has been minimized.

Copy link
@str4d

str4d Oct 9, 2019

Contributor

It's a ring buffer, so by design it eventually overwrites itself if it is being filled more quickly than the entries expire. RecentlyEvictedList clearly can't be an unbounded buffer, or it would itself be a DoS vector. The question is how large the buffer's bound should be.

mempool. The maximum total cost is configurable via the parameter
`mempooltotalcostlimit` which defaults to 80,000,000 (up to 20,000 txs). If a
node's total mempool `cost` exceeds this limit the node will evict a random
transaction, weighted by its cost. To prevent a node from re-accepting evicted

This comment has been minimized.

Copy link
@daira

daira Oct 17, 2019

Contributor

This description is no longer accurate because of the separation of cost and eviction weight. The cost is not dependent on the fee; the eviction weight is.

This comment has been minimized.

Copy link
@daira

daira Oct 17, 2019

Contributor

Also, the parameter names do not match the draft ZIP. Either the ZIP or the code (and this description), or both, has to change. Personally I like the . after mempool to separate it into a different namespace. snake_case vs mushedlowercase is a matter of taste.

This comment has been minimized.

Copy link
@Eirik0

Eirik0 Oct 17, 2019

Author Contributor

How about changing this to say ... a random transaction,preferentially picking larger transactions and ones with below the standard fee.To prevent...?

This comment has been minimized.

Copy link
@daira

daira Oct 17, 2019

Contributor

Note that "and its fee" also needs to be deleted from the definition of cost.

This comment has been minimized.

Copy link
@Eirik0

Eirik0 Oct 17, 2019

Author Contributor

I changed the wording a bit here for clarification.

src/mempool_limit.h Outdated Show resolved Hide resolved
src/mempool_limit.h Outdated Show resolved Hide resolved
@daira
daira approved these changes Oct 17, 2019
Copy link
Contributor

daira left a comment

reACK modulo some minor comments, and the issue about parameter names relative to the ZIP. Nice doc improvements.

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

Eirik0 commented Oct 18, 2019

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 18, 2019

⌛️ Trying commit e744cec with merge 5c0b3a6...

zkbot added a commit that referenced this pull request Oct 18, 2019
DoS protection: Weighted random drop of txs if mempool full
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 18, 2019

💔 Test failed - pr-try

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

Eirik0 commented Oct 18, 2019

Intermittent failures.

strUsage += HelpMessageOpt("-mempoolevictionmemoryminutes=<n>", strprintf(_("The number of minutes before allowing rejected transactions to re-enter the mempool. (default: %u)"), DEFAULT_MEMPOOL_EVICTION_MEMORY_MINUTES));
strUsage += HelpMessageOpt("-mempooltotalcostlimit=<n>",strprintf(_("An upper bound on the maximum size in bytes of all transactions in the mempool. (default: %s)"), DEFAULT_MEMPOOL_TOTAL_COST_LIMIT));
strUsage += HelpMessageOpt("-mempool.eviction_memory_minutes=<n>", strprintf(_("The number of minutes before allowing rejected transactions to re-enter the mempool. (default: %u)"), DEFAULT_MEMPOOL_EVICTION_MEMORY_MINUTES));
strUsage += HelpMessageOpt("-mempool.tx_cost_limit=<n>",strprintf(_("An upper bound on the maximum size in bytes of all transactions in the mempool. (default: %s)"), DEFAULT_MEMPOOL_TOTAL_COST_LIMIT));

This comment has been minimized.

Copy link
@LarryRuane

LarryRuane Oct 18, 2019

Contributor

Sorry, but I think the old way is better, because, as Eirik suspected, . does seem to have a special meaning at least in Bitcoin:
https://github.com/bitcoin/bitcoin/blob/b6e34afe9735faf97d6be7a90fafd33ec18c0cbb/src/util/system.cpp#L291
https://github.com/bitcoin/bitcoin/blob/b6e34afe9735faf97d6be7a90fafd33ec18c0cbb/src/util/system.cpp#L439
https://github.com/bitcoin/bitcoin/blob/b6e34afe9735faf97d6be7a90fafd33ec18c0cbb/src/util/system.cpp#L827

I think this relates to "sections" of the configuration file. We haven't merged that yet, but we could someday.

This comment has been minimized.

Copy link
@Eirik0

Eirik0 Oct 21, 2019

Author Contributor

I removed the . and _.

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

Eirik0 commented Oct 21, 2019

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 21, 2019

📌 Commit 40a7156 has been approved by Eirik0

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 21, 2019

⌛️ Testing commit 40a7156 with merge 0b0c724...

zkbot added a commit that referenced this pull request Oct 21, 2019
DoS protection: Weighted random drop of txs if mempool full
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 21, 2019

💔 Test failed - pr-merge

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

Eirik0 commented Oct 21, 2019

Passed on some workers but not others, looks like transient failures.

Running 310 test cases...
0%   10   20   30   40   50   60   70   80   90   100%
|----|----|----|----|----|----|----|----|----|----|
***************************�[0;39;49
m************
********test/rpc_wallet_tests.cpp(294): error: in "rpc_wallet_tests/rpc_wallet": unexpected exception thrown by CallRPC("getblock 0")
test/rpc_wallet_tests.cpp(295): error: in "rpc_wallet_tests/rpc_wallet": unexpected exception thrown by CallRPC("getblock 0 0")
test/rpc_wallet_tests.cpp(296): error: in "rpc_wallet_tests/rpc_wallet": unexpected exception thrown by CallRPC("getblock 0 1")
test/rpc_wallet_tests.cpp(297): error: in "rpc_wallet_tests/rpc_wallet": unexpected exception thrown by CallRPC("getblock 0 2")
****
*** 4 failures are detected in the test module "Bitcoin Test Suite"

I am less familiar with the above transient failure, but those tests pass for me locally.

@Eirik0

This comment has been minimized.

Copy link
Contributor Author

Eirik0 commented Oct 21, 2019

@zkbot retry

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 21, 2019

⌛️ Testing commit 40a7156 with merge fffd5da...

zkbot added a commit that referenced this pull request Oct 21, 2019
DoS protection: Weighted random drop of txs if mempool full
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 21, 2019

☀️ Test successful - pr-merge
Approved by: Eirik0
Pushing fffd5da to master...

@zkbot zkbot merged commit 40a7156 into zcash:master Oct 21, 2019
1 check passed
1 check passed
homu Test successful
Details
Arborist Team automation moved this from In Review to Released (Merged in Master) Oct 21, 2019
@Eirik0 Eirik0 mentioned this pull request Oct 21, 2019
@rex4539

This comment has been minimized.

Copy link
Contributor

rex4539 commented Oct 22, 2019

Merging this broke the compilation on macOS #4164

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Arborist Team
  
Released (Merged in Master)
8 participants
You can’t perform that action at this time.