Skip to content

Conversation

@jgarzik
Copy link
Contributor

@jgarzik jgarzik commented Jul 9, 2015

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.

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.
@luke-jr
Copy link
Member

luke-jr commented Jul 9, 2015

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

@jgarzik
Copy link
Contributor Author

jgarzik commented Jul 9, 2015

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

@luke-jr
Copy link
Member

luke-jr commented Jul 9, 2015

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
Copy link
Contributor Author

jgarzik commented Jul 9, 2015

@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
Copy link
Member

luke-jr commented Jul 9, 2015

First tx exceeding the limit trigger SGTM

@randy-waterhouse
Copy link
Contributor

Concept ACK. Nice work Jeff.

@ashleyholman
Copy link
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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 :)

@jonasschnelli
Copy link
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: jonasschnelli@086a59f

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
Copy link

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

That's what I mean

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

@andreas-cubits
Copy link

@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
Copy link
Contributor

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.

src/poolman.cpp Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

what do you need from main?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The minimum relay fee global

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@jtimon
Copy link
Contributor

jtimon commented Jul 9, 2015

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

@TheBlueMatt
Copy link
Contributor

@jtimon You mean like TheBlueMatt@e68aee1 ? :p
Currently limits mempool to only a few MBs

@randy-waterhouse
Copy link
Contributor

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

@TheBlueMatt
Copy link
Contributor

@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)

@jgarzik
Copy link
Contributor Author

jgarzik commented Jul 10, 2015

Fixed bug found by @jonasschnelli and some minor nits.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@jtimon
Copy link
Contributor

jtimon commented Jul 10, 2015

@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:

  1. 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]).
  2. 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
Copy link
Contributor

morcos commented Jul 10, 2015

@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...

@ABISprotocol
Copy link

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
Copy link
Contributor Author

jgarzik commented Sep 15, 2015

Closing

@jgarzik jgarzik closed this Sep 15, 2015
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.