-
Notifications
You must be signed in to change notification settings - Fork 38.7k
Floating network relay fee increase, if memory pool grows too large. #6402
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
Conversation
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.
|
Concept ACK, but why run on a schedule rather than after/before accepting new txns? |
|
It takes a while for the condition to clear once noticed. You don't want to run it too frequently. |
|
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? |
|
@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. |
|
First tx exceeding the limit trigger SGTM |
|
Concept ACK. Nice work Jeff. |
|
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
|
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. 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). |
|
@jgarzik 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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);|
@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. |
|
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
Apart from the nits, I wonder if this could be integrated with the fee estimator in policy/fees.o |
|
@jtimon You mean like TheBlueMatt@e68aee1 ? :p |
|
@jtimon @TheBlueMatt integrating fee estimation with mempool management risks integrating wallet function with network function of the node. |
|
@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) |
|
Fixed bug found by @jonasschnelli and some minor nits. Will address the @luke-jr feedback RE triggering next. |
There was a problem hiding this comment.
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?
This (and the estimator) are policy code. Assuming #6068 ever gets merged, I plan to:
In fact I've already done this, but it is bitrotten. @morcos may be interested in this PR too. |
|
@jgarzik I saw some of your conversation with @pstratem on -dev IRC last night. 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... |
|
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. |
|
Closing |
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:
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.