Implement on-the-fly mempool size limitation #6448
Conversation
jtimon
force-pushed
the
mempool-cap-0.12.99
branch
from
f0cad7e to
6f7574b
Compare
|
The refactoring is reasonable. However without expiration of low fee transactions it's a pretty dangerous change, as the moment you fill up the mempool with junk that miners don't or can't mine your node stops relaying transactions - better to have nodes crash with OOM errors and get restarted than that. The simplest thing to do is for the purpose of the pull-req, disable the limit by setting it to something ridiculously high by default to match the existing behavior. (or just add in the expiration code) |
|
Yes, a ridiculously high value is what I was discussing with @morcos the other day on IRC (the actual value is open for bike-shedding). Another possibility it's to just start with #6416, which would already be a step in the right direction for conflict replacement policies (ie FS, RBF, zeroconf-safe-RBF, etc). |
|
Even if the default would be ridiculously high, at least we would be giving the option of the cap for people who really don't want their node to crash (maybe they prefer to restart it periodically to cleanup the mempool or something). |
|
I think your rebasing failed a bit here, as several of my changes are inside your ''prepare'' commit now. |
|
Sipa, it's not several of your commits, it's a small subset of the last commit in #6421 (in addition to the fixup I proposed there). This is indicated in the commit message. The second commit is also a subset of that commit. Maybe reading the rebased version of #6421 (read the initial description of the PR) makes this clearer. |
jtimon
force-pushed
the
mempool-cap-0.12.99
branch
2 times, most recently
from
54e1cd2 to
e585d1c
Compare
…lock, and miner::CreateNewBlock and In all of them, reject transactions creating new money earlier. Consensus::CheckTxInputs gets nTxFee as output parameter and is separated from main::CheckInputs [renamed CheckInputsScripts] - Consensus::CheckTxInputs (called by the rest): Don't calculate nValueOut twice Don't check nFees < 0 twice - main::AcceptToMemoryPool: Don't call CCoinsViewCache::HaveInputs twice Don't calculate nValueIn 3 times Don't calculate nValueOut 5 times - miner::CreateNewBlock: Don't call CCoinsViewCache::HaveInputs twice Don't calculate nValueIn twice Don't calculate nValueOut 3 times - main::ConnectBlock: Still call CCoinsViewCache::HaveInputs twice Don't calculate nValueIn twice Don't calculate nValueOut 3 times
…xFee.GetFee(nSize)) condition
The following condition won't be checked anymore unless fLimitFree == true:
GetBoolArg("-relaypriority", true) && !AllowFree(view.GetPriority(tx, chainActive.Height() + 1))
With some code taken from Pieter Wuille's bitcoin#6421
jtimon
force-pushed
the
mempool-cap-0.12.99
branch
from
e585d1c to
2e779b8
Compare
|
I updated this with move movements and introducing a cache for the transaction checks and the fee calculations. With this, full nodes that don't need fee estimation can function without mempool as suggested by @petertodd . The rebased version of #6421 is still on https://github.com/jtimon/bitcoin/tree/limitpool_rebased |
4 participants
This is a reduced version of @sipa 's #6421 that caps the size of the mempool,
but this one just introduces a dumb policy that simply rejects all replacements for now.
That policy can be replaced, by #6421 (a rebased version can be found at https://github.com/jtimon/bitcoin/tree/limitpool_rebased ) or something else when it's properly tested.
This dumb policy is similar to the one we're using for resolving conflicts in spends.
That spending conflicts policy should be located in the same place because it is likely that there can be some synergies.
For example, it would be also good to implement zeroconf-safe-RBF (also known as FSS-RBF), so that payers can increase the fees when they get their transactions stuck.
For this reason, this includes #6416 (although we can make a version without that if many people are already buidling on top of #6421 and they think the rebase on top of https://github.com/jtimon/bitcoin/tree/limitpool_rebased is too painful).