Super Duper MemPool Limiter #6470

morcos · 2015-07-23T23:08:18Z

@sipa @sdaftuar @jtimon @petertodd
OK, this is the best combination of approaches I could put together.

I took #6455 and I removed the floating relay fee commit. I really like that idea, but it needs to be much slower acting I think and not subject to potential abuse. That can be a later improvement.

I added 3 things:

Reserved space between the soft cap and hard cap. The soft cap is currently set to 70% of the hard cap. Once the soft cap is hit, you first try to evict as in 6455, but if you fail, you have another chance to get in if you are still under the hard cap. There are 10 "rate zones" between the hard cap and soft cap and the effective minRelayRate to get into the mempool doubles for each additional zone.
Any required minRelayFee for your transaction alone is considered inside the StageTrimToSize loop.
Periodically (once a second) we use the knowledge that surplus fees over the minRelayRate must have been paid if the size of the mempool is over the soft cap. We use these fees in aggregate to try to trim from the bottom of the mempool. This allows us to aggregate many small high fee transactions to evict a low paying large transaction or long chain.

The reject rates in my test setup have dropped to 0.3% for 30k feerate tx's and 0.05% for 60k feerate tx's. (See other results in #6455).

I made an attempt to tweak the looping parameters in StageTrimToSize to something that I think made sense, but with the contrived test setup, and only one set of simulation data, they are probably best evaluated on the basis of intuition and not relying entirely on the resulting rejection rates.

It turns out the slowest part of StageTrimToSize was GetRand() by a long shot, so I hacked it out, but I'm sure @sipa will want to replace my hack with something nicer.

The code works as is, but could still use some work, but I think its time to get more eyes on this suggestion for a plan forward.

jtimon · 2015-07-23T23:16:33Z

src/main.cpp

@@ -852,6 +877,12 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa
                                   hash.ToString(), nSigOps, MAX_STANDARD_TX_SIGOPS),
                             REJECT_NONSTANDARD, "bad-txns-too-many-sigops");

+        // Expire old transactions before trying to replace low-priority ones.
+        int expired = pool.Expire(GetTime() - GetArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY) * 60 * 60);


Can't - GetArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY) * 60 * 60 be inside pool.Expire?, it may become an attribute of the txmempool in the future or something.

I would prefer not doing that, and keeping CTxMempool as much as possible a dumb data structure - the decisions about what happens to it (policy?) should stay out of it, IMHO.

EDIT: We're already failing at that pretty badly anyway, it seems, with the feerate index and the trim code inside CTxMempool. Too bad, but disregard this comment.

It is just as easy to move things from txmempool to the policy dir than it is from main (if not easier, given that main is always the part with more development conflicts [unrelated things in theory conflict in code there]).
But whatever, I guess it will be a new global in main...

laanwj · 2015-07-24T07:56:29Z

re: GetRand slowness, wouldn't insecure_rand() be good enough here?
If you need both fast and cryptographic randomness you'd have to wait for #5885

sipa · 2015-07-24T17:53:58Z

I like the approach; I'll review the code in detail soon.

dgenr8 · 2015-07-25T16:26:24Z

The idea that a transaction must be "paid for" when evicted by an unrelated transaction is mistaken. It creates zero incentive for the evicted author to pay more fees in the future. He got what he wanted -- his tx was relayed.

If the decision does not affect the cost, the cost should not affect the decision.

sipa · 2015-07-25T16:35:41Z

The assumption is that when a transaction is evicted, it won't be mined, thus it won't have paid anything at all.

dgenr8 · 2015-07-25T16:55:29Z

@sipa In that case, it also received no benefit.

sipa · 2015-07-25T17:09:33Z

It got relayed, which has network costs.

The purpose of this payment requirement is to prevent someone from spamming the network by constantly replacing the lowest priority one, and only paying once.

dgenr8 · 2015-07-25T17:24:09Z

For DoS protection, all that's needed is (new fee/kb) - (old fee/kb) >= (minimum fee/kb).

sipa · 2015-07-27T14:36:04Z

@dgenr8 No, that would let a small transaction erase a large transaction, making space for several new transactions to be cheaply relayed.

sipa · 2015-07-27T14:55:36Z

src/main.cpp

+
+        if (!mempool.StageTrimToSize(softcap, entry, stagedelete, nFeesRequired, nFeesDeleted)) {
+            size_t expsize = mempool.DynamicMemoryUsage() + mempool.GuessDynamicMemoryUsage(entry);
+            if (expsize > hardcap)


Please use { } around the then block.

No objection to changing, but just want to understand what the style request is related to this. All then blocks should have braces or only because the else block is multi-line? A single-line then block without braces appears as example code in developer-notes.md.

Oh, I thought we changed that. Never mind then. I personally dislike those, as they easily lead to mistakes when merging different patches (see the OSX SSL bug that was likely the result of it), but I shouldn't ask that as long as our notes use it.

The fact is that braces are not necessary in our clang style. And btw, when they're used, the shouldn't be in the next line but in the same line as the if/for/while. I'm happy changing the style, but that's in clang-format.

morcos · 2015-07-27T15:40:56Z

@jtimon I'd like to add some sanity checking for the command line arguments that are passed in. I assume that should go in init.cpp? Where is the appropriate place for me to store variables such as hard cap, mempool expiry time, etc.. so they aren't recalculated every time.

sipa · 2015-07-27T16:25:43Z

src/txmempool.cpp

+    indexed_transaction_set::nth_index<1>::type::reverse_iterator it = mapTx.get<1>().rbegin();
+    int fails = 0; // Number of mempool transactions iterated over that were not included in the stage.
+    int itertotal = 0;
+    int iterextra = mustTrimAllSize ? 10 : 100; //Allow many more iterations to find large size during SurplusTrim


Do you mind turning these into arguments, that get passed from the 2 callsites instead?

sipa · 2015-07-27T16:28:15Z

Needs rebase.

dgenr8 · 2015-07-27T18:31:33Z

that would let a small transaction erase a large transaction, making space for several new transactions to be cheaply relayed

@sipa That is good. You don't want a 500KB spam-monster tx paying (minimum fee/kb) to require a regular-sized tx to pay 1001 * (minimum fee/kb) to dislodge it. Bad guy could get 1000 little txes relayed anyway by sending them first -- the pay-for-evicted rule creates a race to be first or pay double.

sdaftuar · 2015-07-27T18:37:51Z

@dgenr8 While that is a potential problem for optimizing what gets into the mempool, note that this PR is attempting to provide a solution so that many small transactions can be used to evict large transaction packages.

morcos · 2015-07-27T19:51:47Z

@dgenr8 This conversation has been spread over a couple of PR's and IRC, so I apologize if I'm repeating a prior argument, but I think it would be good to explain the reasoning behind the logic in this pull.

Prior to limiting the mempool (or having RBF implemented), we had protection against bandwidth spamming attacks by requiring all transactions that were relayed to be accepted into the mempool and have either fee or priority. Since those transactions are now in the mempool and can't be recalled, those fees are now at risk for being consumed by being included in a block. This pull isn't meant to provide some new protection against that attack, but is only meant to protect against OOM attacks by ever expanding mempools. However, it must be sure not to break that pre-existing protection.

Once we create a way for transactions to be removed from the mempool, we have to realize that the fees placed on those transactions are no longer at risk. The approach we've been using is to say that (modulo transactions that have already been mined) the mempool must contain enough fees to pay for the relay cost of all transactions that have been historically broadcast whether they are still in the mempool or not. What this prevents is someone replacing or evicting their own transactions "cheaply" and thereby achieving free relay of their original transactions. The fact that there might be a race to get into the mempool before it fills up is a slight aberration that occurs as a one off. We still need a mechanism by which to adjust the minimum relay fee over time in the event that it is too small to dissuade spam.

Your concern about a monster transaction of low fees preventing a small transaction from getting in the mempool is addressed in 4 separate ways in this pull.

Multiple attempts are made to evict transactions, so being unlucky and trying against one large transaction is insufficient to block you.
Some portion of the mempool space is reserved for higher and higher fee transactions. That space is not cheaply fillable with spam and should serve to act as a temporarily higher min relay rate which will keep the relay path open for high fee paying tx's (large or small).
Any tx's occupying the reserve space are periodically looked at in aggregate and used to remove large tx's or chains of tx's from the bottom of the mempool that would otherwise be unevictable.
There is a time based eviction mechanism as a last fall back.

No doubt further improvements are possible, but I think this is a good step in the right direction.

dgenr8 · 2015-07-28T00:43:02Z

@morcos Thanks. I feel the capping part of this pull is too complex, and could be simplifed (a little) by removing the "pay for eviction" idea, which has no good incentive effect.

The person paid is the miner, who doesn't need to be paid ~double to mine the evictor tx. The person who pays is the unlucky evictor. Evictee gets a chance at cheap fees, and others who come after eviction could pay even less, since there might then be extra space for them.

The case where evictor == evictee could be handled explicitly, if it were actually possible to target this kind of replacement with all the factors that affect the selection of the evictees, such as propagation variation, node start time variation, mining, and nodes having different mempool size limits.

morcos · 2015-07-28T21:23:32Z

I rebased, addressed many of the comments and did some various cleanups in place.

Please note I changed the defaults for -maxmempool and -mempoolexpiry to 500MB and 1 week respectively.

jtimon · 2015-07-29T10:34:04Z

@morcos everybody is creating new globals in main, but I hate that. I would at the very least move them to globals/server ot something, but whatever...
It would be much nicer if you make them an attribute of an existing class that is used as a global (say, mempool or globalPolicy). Here's a commit in which you can see what I would like to move towards: jtimon@5a42e27
I've been trying to do something like that since 2014 but unfortunately there's still no right place for introducing new policy options and people keep doing everything in main. Next step is #6068 (still without the preparations in util that you could reuse, let me know if you want me to separate those preparations).

Even if you don't do it "the right way" ( @luke-jr wanted to create a dynamic GUI form for command line options using something like jtimon@5a42e27#diff-01e64f27a2a21a3116825fa22aee0537R30 ), for the case of -maxmempool and -mempoolexpiry, you could at least put them in CTxMempool.
If we want them in policy (no strong opinion), the easiest thing IMO is leaving them as locals for now (even though that's not very efficient) and hide them in CStandardPolicy or the policy estimator later.

But, whatever, there's many things to cleanup already and everybody is doing it wrong, even for new policy options (like fRequireStandard), so why would you spend any "mental power" on putting new things in the right place when you're actually solving an urgent problem?
Do everything in main like everybody else and hopefully it will be cleaned up eventually.

morcos · 2015-07-29T14:23:33Z

@jtimon OK thanks. Yes I'm going to leave them as locals for now and if we end up adding sanity checking it can just be done separately in init.cpp for now. But I do like your idea of moving them to a policy class. It puts all the logic of what are the reasonable parameters for these arguments in the same place..

ABISprotocol · 2015-07-30T02:35:35Z

I have a few questions,

I noted that @morcos stated that he "removed the floating relay fee commit," but is there another commit added that provides some (roughly) equivalent function? It strikes me that having a floating relay fee added has a necessary dynamic effect ~ the purpose of the floating relay, as discussed in Limited mempool + floating relay fee + rejection caching + mempool expiry #6455, is to dampen the effect of the limited memory pool, and make it more constant over time.
Noted from prior remarks I have made in Limited mempool + floating relay fee + rejection caching + mempool expiry #6455 that a mempool is not necessarily needed (e.g., relay nodes would not need to be using a mempool; the wallet nodes, would be using a mempool, as was indicated).
It was my understanding that there was a resolution that "Mempool limiting and dynamic fee determination are superior to a static parameter change" at the end of Restore minimum feerate to 10000 satoshis #6201, which when closed led to Limited mempool + floating relay fee + rejection caching + mempool expiry #6455 and now, to this pull request.

So, why not time based expiration as well as a floating relay fee?  Just curious.

sipa · 2015-07-30T13:05:12Z

Testing & benchmarking.

morcos · 2015-07-30T16:31:57Z

@ABISprotocol, I still believe we need a floating relay fee. I just think it needs to act over a much longer time horizon and be less abusable than the one implemented in the commit I removed.

ABISprotocol · 2015-08-03T07:44:50Z

@morcos @sipa In what pull request or issue should I look for the floating relay fee, or is that TBD?

morcos · 2015-08-03T15:32:54Z

@ABISprotocol This was the commit I removed, sipa@6498673. But I think the correct answer is TBD.

ABISprotocol · 2015-08-05T01:01:22Z

@morcos @sipa Please refer to the number of the pull request for the the floating relay fee in this one at such time when it is created so that the discussion / progress on this can be followed. So far I have been following this as follows:
#6201
#6455
I'm assuming there will be another pullreq (with a floating relay fee) TBD, my request is that when it is created, please refer to the number of that pullreq here. Thank you in advance.

morcos · 2015-08-05T15:54:16Z

Rebased now that #6498 has been merged

Indexes on: - Tx Hash - Fee Rate (fee-per-kb)

The mempool will now have a soft cap set below its hard cap. After it fills up to the soft cap, transactions much first try using StageTrimToSize to evict the amount of size they are adding. If they fail they can still be let into the mempool if they pass a higher relay minimum. It doubles 10 times between the soft cap and hard cap.

StageTrimToSize will make several attempts to find a set of transactions it can evict from the mempool to make room for the new transaction. It should be aware of any required minimum relay fee that needs to be paid for by the new transaction after accounting for the fees of the deleted transactions.

Use reserve space between soft cap and hard cap as a reservoir of surplus fees that have been paid above the minRelayTxFee and occasionally use the aggregate usage there to trim from the bottom of the mempool.

Improve logging Use insecure_rand in TrimMempool Tweak logic of TrimMempool Add occasional larger SurplusTrim. Bypass eviction on disconnected block txs Additional SurplusTrim for bypassed size Acquire locks appropriately

morcos · 2015-08-14T19:03:03Z

Rebased and squashed various cleanups and small changes into the last commit.

morcos · 2015-09-02T17:20:35Z

Closing in lieu of #6557, will reopen if we decide for some reason we want the mempool limiting without the descendant package tracking...

ABISprotocol · 2015-09-03T06:08:20Z

I'm beginning to wonder just how far down the rabbit hole this all goes. And what happened to the floating relay fee bit?

jtimon reviewed Jul 23, 2015
View reviewed changes

morcos force-pushed the surplusTrim branch from 6b4af96 to c4d87ae Compare July 24, 2015 02:05

laanwj added the Mempool label Jul 24, 2015

sipa reviewed Jul 27, 2015
View reviewed changes

morcos mentioned this pull request Jul 27, 2015

Keep track of recently rejected transactions with a rolling bloom filter #6452

Closed

sipa reviewed Jul 27, 2015
View reviewed changes

sipa mentioned this pull request Jul 27, 2015

Limited mempool + floating relay fee + rejection caching + mempool expiry #6455

Closed

morcos force-pushed the surplusTrim branch from c4d87ae to fb001d5 Compare July 28, 2015 21:18

sipa mentioned this pull request Aug 3, 2015

Keep track of recently rejected transactions with a rolling bloom filter (cont'd) #6498

Merged

morcos force-pushed the surplusTrim branch from fb001d5 to 591f424 Compare August 5, 2015 15:51

sipa mentioned this pull request Aug 8, 2015

txoutsbyaddress index #5048

Closed

ashleyholman and others added 9 commits August 11, 2015 11:23

TxMemPool: Change mapTx to a boost::multi_index_container

bb93e2c

Indexes on: - Tx Hash - Fee Rate (fee-per-kb)

Move orphan tx handling to a separate log class

9dc4e70

Implement on-the-fly mempool size limitation.

04cf4ba

Mempool expiry

dac6496

Refactor STTS to be usable for surplus trimming as well

2dd1b4d

Implement Surplus Trim.

40c38f8

Use reserve space between soft cap and hard cap as a reservoir of surplus fees that have been paid above the minRelayTxFee and occasionally use the aggregate usage there to trim from the bottom of the mempool.

Various improvements.

6e8d3dd

Improve logging Use insecure_rand in TrimMempool Tweak logic of TrimMempool Add occasional larger SurplusTrim. Bypass eviction on disconnected block txs Additional SurplusTrim for bypassed size Acquire locks appropriately

morcos force-pushed the surplusTrim branch from 591f424 to 6e8d3dd Compare August 14, 2015 19:02

sdaftuar mentioned this pull request Aug 14, 2015

Mempool limiting with descendant package tracking #6557

Closed

jtimon mentioned this pull request Aug 25, 2015

Policy: Remove free transactions special case code #6584

Closed

gavinandresen mentioned this pull request Sep 2, 2015

Limit number of transactions in the memory pool bitcoinxt/bitcoinxt#56

Closed

morcos closed this Sep 2, 2015

ABISprotocol mentioned this pull request Oct 20, 2015

Limit mempool by throwing away the cheapest txn and setting min relay fee to it #6722

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Super Duper MemPool Limiter #6470

Super Duper MemPool Limiter #6470

Conversation

morcos commented Jul 23, 2015

jtimon Jul 23, 2015

Choose a reason for hiding this comment

sipa Jul 27, 2015

Choose a reason for hiding this comment

jtimon Jul 29, 2015

Choose a reason for hiding this comment

laanwj commented Jul 24, 2015

sipa commented Jul 24, 2015

dgenr8 commented Jul 25, 2015

sipa commented Jul 25, 2015

dgenr8 commented Jul 25, 2015

sipa commented Jul 25, 2015

dgenr8 commented Jul 25, 2015

sipa commented Jul 27, 2015

sipa Jul 27, 2015

Choose a reason for hiding this comment

morcos Jul 27, 2015

Choose a reason for hiding this comment

sipa Jul 27, 2015 via email

Choose a reason for hiding this comment

jtimon Jul 29, 2015

Choose a reason for hiding this comment

morcos commented Jul 27, 2015

sipa Jul 27, 2015

Choose a reason for hiding this comment

sipa commented Jul 27, 2015

dgenr8 commented Jul 27, 2015

sdaftuar commented Jul 27, 2015

morcos commented Jul 27, 2015

dgenr8 commented Jul 28, 2015

morcos commented Jul 28, 2015

jtimon commented Jul 29, 2015

morcos commented Jul 29, 2015

ABISprotocol commented Jul 30, 2015

sipa commented Jul 30, 2015

morcos commented Jul 30, 2015

ABISprotocol commented Aug 3, 2015

morcos commented Aug 3, 2015

ABISprotocol commented Aug 5, 2015

morcos commented Aug 5, 2015

morcos commented Aug 14, 2015

morcos commented Sep 2, 2015

ABISprotocol commented Sep 3, 2015