Floating network relay fee increase, if memory pool grows too large.

jgarzik commented at 3:18 am on July 9, 2015: contributor

Create a relay fee that adjusts to floods which cause the memory pool to grow too large (and thus crash the node). A periodic memory pool janitor runs, scanning the memory pool total byte size.

If the memory pool is below a “low water” byte size limit, the -minrelaytxfee minimum relay fee setting is used.

If the memory pool is above a “high water” byte size limit, the minimum relay fee setting is increased according to the following algorithm:

take the top half of the mempool in terms of fee/kb - fee rate
take the lowest fee rate as the minimum relay fee

The memory pool may continue to grow beyond the high water mark, though fees to further fill memory pools become increasingly more expensive, until new blocks reduce the pressure.

The newly increased relay fee remains intact until the memory pool size falls below the “low water” limit.

The default “low water” limit is 1/2 of the supplied “high water” limit.

Floating network relay fee increase, if memory pool grows too large.

Create a relay fee that adjusts to floods which cause the memory pool to
grow too large.  A periodic memory pool janitor runs, scanning the
memory pool total byte size.

If the memory pool is below a "low water" byte size limit, the
-minrelaytxfee minimum relay fee setting is used.

If the memory pool is above a "high water" byte size limit, the
minimum relay fee setting is increased according to the following
algorithm:
- take the top half of the mempool in terms of fee/kb - fee rate
- take the lowest fee rate as the minimum relay fee

The memory pool may continue to grow beyond the high water mark, though
fees to further fill memory pools become increasingly more expensive,
until new blocks reduce the pressure.

The newly increased relay fee remains intact until the memory pool
size falls below the "low water" limit.

The default "low water" limit is 1/2 of the supplied "high water" limit.

e459798a3c

luke-jr commented at 3:27 am on July 9, 2015: member

Concept ACK, but why run on a schedule rather than after/before accepting new txns?

jgarzik commented at 4:58 am on July 9, 2015: contributor

It takes a while for the condition to clear once noticed. You don’t want to run it too frequently.

luke-jr commented at 5:05 am on July 9, 2015: member

I mean for setting the condition and increasing the minimum fee.. otherwise someone just needs to flood you as fast as they can before it kicks in, right?

jgarzik commented at 6:03 am on July 9, 2015: contributor

@luke-jr That is the ideal, but it requires a bit more effort. Must avoid adding too much code to the per-tx fast path.

A likely solution is perform inexpensive per-tx checks on total mempool size, then trigger some background action. Such a solution would trigger upon the first tx exceeding high water limit.

luke-jr commented at 6:07 am on July 9, 2015: member

First tx exceeding the limit trigger SGTM

randy-waterhouse commented at 7:33 am on July 9, 2015: contributor

Concept ACK. Nice work Jeff.

ashleyholman commented at 10:31 am on July 9, 2015: contributor

Let’s imagine the memory pool hits the low watermark and at that time it consists of 80% spam transactions at 100 bits/kb. Then the lowest fee rate in the top half will be 100bits/kb and so further spam at the same rate will still enter the pool? This will continue so long as the spammer can produce >50% of the transactions arriving at the node.

in src/poolman.cpp: in e459798a3c outdated

81+    }
82+
83+    // if interval too small or zero, janitor is disabled
84+    static const int janitorSaneInterval = 10;
85+    if ((janitorInterval < janitorSaneInterval) ||
86+        (PoolmanHighWater <= 0)) {

jonasschnelli commented at 11:21 am on July 9, 2015:

Just ran into this sane check (while testing with 5s interval). Maybe it would be worth to add a LogPrint("mempool", "Janitor has been disabled because of insane interval") or similar.

jgarzik commented at 4:35 am on July 10, 2015:

leaning towards no, but will consider… need a conditional inside that code block as the 0=disabled case should not trigger a message about insanity :)

in src/poolman.cpp: in e459798a3c outdated

89+        PoolmanHighWater = 0;
90+    }
91+
92+    // start mempool janitor
93+    if (janitorInterval > 0)
94+        scheduler.scheduleEvery(&TxMempoolJanitor, janitorInterval * 1000);

jonasschnelli commented at 11:23 am on July 9, 2015:

rm *1000?

TheBlueMatt commented at 7:21 pm on July 9, 2015:

Yea, scheduleEvery’s second arg is seconds, not millis.

in src/init.cpp: in e459798a3c outdated

281@@ -281,8 +282,11 @@ std::string HelpMessage(HelpMessageMode mode)
282     }
283     strUsage += HelpMessageOpt("-datadir=<dir>", _("Specify data directory"));
284     strUsage += HelpMessageOpt("-dbcache=<n>", strprintf(_("Set database cache size in megabytes (%d to %d, default: %d)"), nMinDbCache, nMaxDbCache, nDefaultDbCache));
285+    strUsage += HelpMessageOpt("-janitorinterval=<n>", _("Number of seconds between each mempool janitor run (default: 10 minutes, 0=disable)"));

Diapolo commented at 11:32 am on July 9, 2015:

This should be flexible, so please use something like above with -dbcache. Also update to 0 = disable (spaces usage), while you are on it.

Also true for the other 2 options!

jgarzik commented at 4:30 am on July 10, 2015:

Added some whitespace. It does not make sense to use strprintf() in these cases, however.

in src/poolman.cpp: in e459798a3c outdated

0@@ -0,0 +1,96 @@
1+

Diapolo commented at 11:48 am on July 9, 2015:

Nit: Missing license, missing header group ordering and also std namespace polution.

in src/poolman.h: in e459798a3c outdated

0@@ -0,0 +1,8 @@
1+#ifndef __BITCOIN_POOLMAN_H__

Diapolo commented at 11:49 am on July 9, 2015:

Nit: Missing license header and wrong header naming string, should be BITCOIN_POOLMAN_H here and at the header end.

in src/txmempool.cpp: in e459798a3c outdated

310@@ -311,6 +311,19 @@ void CTxMemPool::queryHashes(vector<uint256>& vtxid)
311         vtxid.push_back((*mi).first);
312 }
313 
314+void CTxMemPool::queryFees(vector<CFeeRate>& vfees)
315+{
316+    vfees.clear();

Diapolo commented at 11:50 am on July 9, 2015:

The .clear() doesn’t need a lock?

jonasschnelli commented at 11:57 am on July 9, 2015:

No. It doesn’t.

in src/poolman.cpp: in e459798a3c outdated

23+        janitorInit = true;
24+        origTxFee = ::minRelayTxFee;
25+    }
26+
27+    // query mempool stats
28+    unsigned long totalTx = mempool.size();

Diapolo commented at 11:51 am on July 9, 2015:

Nit: size_t?

jonasschnelli commented at 11:58 am on July 9, 2015:

IMO okay because: unsigned long CTxMemPool::size()

jonasschnelli commented at 12:23 pm on July 9, 2015: contributor

Tested a bit. Had to add three commits to get it running: https://github.com/jonasschnelli/bitcoin/commits/2015/07/mempool_janitor

Nice work!

I’m not sure but it feels that it somehow doesn’t prevent from flooded mempools. Example: if someone is flooding your node with 1tx/s with a correct minRelayTxFee (lets assume 1000/kb), the janitor will always keep the minRelayTxFee at 1000/kb, because the back of the vector of all feerates / 2 will very likely to still be 1000/kb.

I have set up a test: https://github.com/jonasschnelli/bitcoin/commit/086a59fc2084126dfbf8c28829cc236bfd3635f2

Sure this is not a real world example, but i would say even if <1/2 of the tx would contain higher fees, the floating relay fee would still stick at 1000/kb.

Maybe it should somehow take the mempool size into the calculation (correlated with the block size).

m-schmoock commented at 3:04 pm on July 9, 2015: none

@jgarzik [quote luke]Concept ACK, but why run on a schedule rather than after/before accepting new txns? [/quote] [quote jgarzik ] It takes a while for the condition to clear once noticed. You don’t want to run it too frequently. [/quote]

Isn’t a integer set and compare per transaction that keeps track of the mempool size a very, very fast operation? Using scheduling for this seems overkill to me.

in src/poolman.cpp: in e459798a3c outdated

70+    // mempool janitor execution interval.  default interval: 10 min
71+    int janitorInterval = GetArg("-janitorinterval", (60 * 10));
72+
73+    // mempool janitor low water, high water marks
74+    PoolmanHighWater = GetArg("-mempoolhighwater", 0);
75+    if (PoolmanHighWater <= 0)

dgenr8 commented at 4:31 pm on July 9, 2015:

Don’t allow janitor to compete with confirmation. Minimum highwater should be something like 1 hour of maximum confirmation bytes.

jgarzik commented at 4:26 am on July 10, 2015:

The scanning interval and the high water mark are two very different things. Presumably the high water mark should be ~2 days, less a small factor.

dgenr8 commented at 0:31 am on July 11, 2015:

@jgarzik Suppose highwater and max_blocksize are 1MB, and transaction flow is exactly 1MB/10min. Whenever you get a slow block, you’re going end up raising minrelayfee. So 0 is too low a minimum … it needs to be high enough that the adjustment is not triggered for reasonably slow blocks.

jgarzik commented at 3:43 am on July 11, 2015:

“0 is too low a minimum” I have no idea what this is talking about, sorry…

And your highwater value should be quite larger than 1MB.

randy-waterhouse commented at 4:24 am on July 11, 2015:

The scanning interval and the high water mark are two very different things. Presumably the high water mark should be ~2 days, less a small factor.

So default highwater ~ 288*max_block_size gives a buffer of 2 days of TX, less small factor, say 256*max_block_size. Then janitor on 2 day TX expiry and minrelaytxfee float are targetting similar time/space constraints when system is at max capacity.

dgenr8 commented at 7:37 pm on July 11, 2015:

And your highwater value should be quite larger than 1MB.

That’s what I mean

0     PoolmanHighWater = GetArg("-mempoolhighwater", 0);
1-    if (PoolmanHighWater <= 0)
2+    // Protect from taking action in response to mempool size variation
3+    // resulting from normal block interval variation
4+    if (PoolmanHighWater < 6 * MAX_BLOCK_SIZE) {
5+        PoolmanHighWater = 0;
6         PoolmanLowWater = -1;
7-    else {
8+    } else {
9         PoolmanLowWater = GetArg("-mempoollowwater", -1);

andreas-cubits commented at 6:20 pm on July 9, 2015: none

@jonasschnelli You are correct that this patch would not effectively prevent mempool flooding if the spam transactions all have the same fee/kb and make up for more than half of the mempool. Sorting by unique fee/kb values and then using the lowest entry of the top half as minRelayTxFee would however seem to fix this.

TheBlueMatt commented at 7:35 pm on July 9, 2015: member

I’m not a big fan of increasing the dust limit dynamically as a part of this. I’d much prefer to only increase the relay fee itself and not touch dust. Still, it seems the majority of what this patch rejects are dust transactions, not rejecting free transactions using the rate limiter.

in src/poolman.cpp: in e459798a3c outdated

0@@ -0,0 +1,96 @@
1+
2+#include <algorithm>
3+#include <vector>
4+#include "poolman.h"
5+#include "main.h"

jtimon commented at 7:55 pm on July 9, 2015:

what do you need from main?

jgarzik commented at 8:23 pm on July 9, 2015:

The minimum relay fee global

jtimon commented at 8:28 pm on July 9, 2015:

Oh, sure, it is still not an attribute of CStandardPolicy…nevermind then.

in src/poolman.cpp: in e459798a3c outdated

10+using namespace std;
11+
12+static int64_t PoolmanLowWater, PoolmanHighWater;
13+static bool janitorInit = false;
14+static bool floatingFee = false;
15+static CFeeRate origTxFee;

jtimon commented at 7:56 pm on July 9, 2015:

Can’t these be attributes of the class?

jgarzik commented at 8:23 pm on July 9, 2015:

which class?

jtimon commented at 8:31 pm on July 9, 2015:

A new class could be created to avoid new globals here. Or as said, this could be incorporated to the fee estimator.

in src/poolman.cpp: in e459798a3c outdated

73+    // mempool janitor low water, high water marks
74+    PoolmanHighWater = GetArg("-mempoolhighwater", 0);
75+    if (PoolmanHighWater <= 0)
76+        PoolmanLowWater = -1;
77+    else {
78+        PoolmanLowWater = GetArg("-mempoollowwater", -1);

jtimon commented at 7:57 pm on July 9, 2015:

Can’t all GetArg() calls be made from a function/method that is only called from init?

jgarzik commented at 8:23 pm on July 9, 2015:

Um, it is made from a function that is only called from init.

jtimon commented at 8:29 pm on July 9, 2015:

Yes, sorry. Fast read.

jtimon commented at 7:59 pm on July 9, 2015: contributor

Apart from the nits, I wonder if this could be integrated with the fee estimator in policy/fees.o

TheBlueMatt commented at 9:29 pm on July 9, 2015: member

@jtimon You mean like https://github.com/TheBlueMatt/bitcoin/commit/e68aee1e93d82ccb4c7310149581c8c5d738cd4a ? :p Currently limits mempool to only a few MBs

randy-waterhouse commented at 0:45 am on July 10, 2015: contributor

@jtimon @TheBlueMatt integrating fee estimation with mempool management risks integrating wallet function with network function of the node.

TheBlueMatt commented at 0:49 am on July 10, 2015: member

@randy-waterhouse Huh? fee estimation code is managed by mempool…it is called by the wallet, but it is not a “wallet function”. (dunno if it makes sense to use it to manage minimum tx fee, though, it may not work so great when looking at many-block confirmations, I havent dug into it much)

poolman: fix minor nits [squashme] 6c449bdcfe

jgarzik commented at 4:33 am on July 10, 2015: contributor

Fixed bug found by @jonasschnelli and some minor nits.

Will address the @luke-jr feedback RE triggering next.

in src/poolman.cpp: in 6c449bdcfe

25+        origTxFee = ::minRelayTxFee;
26+    }
27+
28+    // query mempool stats
29+    unsigned long totalTx = mempool.size();
30+    int64_t totalSize = mempool.GetTotalTxSize();

jtimon commented at 8:12 am on July 10, 2015:

Can the mempool be passed as a parameter to TxMempoolJanitor() instead of using main.o’s global?

jtimon commented at 8:17 am on July 10, 2015: contributor

@jtimon @TheBlueMatt integrating fee estimation with mempool management risks integrating wallet function with network function of the node.

This (and the estimator) are policy code. Assuming #6068 ever gets merged, I plan to:

Turn minRelayTxFee global into a CStandardPolicy attribute (decoupling the mempool from that global, but coupling it to CPolicy [which now returns a ref to the estimator]).
Decouple the mempool from CPolicy.

In fact I’ve already done this, but it is bitrotten. If the new globals introduced here were attributes of the estimator my plan would probably be easier to execute. If #6068 gets merged, this can be moved to policy/policy instead of policy/fees, whatever makes more sense. At some point we need to stop producing new policy-related globals… @morcos may be interested in this PR too.

morcos commented at 11:16 am on July 10, 2015: member

@jgarzik I saw some of your conversation with @pstratem on -dev IRC last night.
I now agree with you that expiration by age makes the most sense. But I wonder if there is an edge case where somehow the mempool is able to be flooded quickly with mostly low fee transactions that will cause recent (but still oldest) high fee transactions to get booted. Perhaps an age check could trigger booting by fees as well..

Regardless, I think its important to keep the mempool sorted by fees. Fee estimation is badly in need of an improvement which would let it walk the mempool to see how many blocks worth of transactions are already in queue with higher fees. This is the only way to reasonably respond to momentary spikes in transaction volume. What do you think about using #6331 as a framework to add these other improvements…

laanwj added the label Mempool on Jul 13, 2015

ABISprotocol commented at 3:57 am on July 14, 2015: none

If I understand this right, this pull request would reject many dust transactions, not rejecting free transactions; above ‘high water’ byte size limit for memory pool, fees to further fill memory pools become increasingly more expensive, making it dynamic. I would like to see this amended such that dust can be handled differently, managed better, and accepted more (once an hour, or randomly, or sent to an external handler, see for example @petertodd’s repo for handling dust). Rather than viewing this as “dust problems,” I suggest that this be re-examined as an opportunity to look at the dust in the context of microgiving. Thank you for reading.

ABISprotocol http://abis.io

jgarzik commented at 1:07 am on September 15, 2015: contributor

Closing

jgarzik closed this on Sep 15, 2015

DrahtBot locked this on Sep 8, 2021

Floating network relay fee increase, if memory pool grows too large. #6402