Implement on-the-fly mempool size limitation #6448

1. 
   jtimon commented at 5:36 pm on July 16, 2015: contributor

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

2. jtimon force-pushed on Jul 16, 2015
3. 
   petertodd commented at 11:19 am on July 17, 2015: contributor

   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)

4. laanwj added the label Mempool on Jul 17, 2015
5. 
   jtimon commented at 9:07 pm on July 17, 2015: contributor

   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).
6. 
   jtimon commented at 9:09 pm on July 17, 2015: contributor

   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).
7. 
   sipa commented at 0:32 am on July 18, 2015: member

   I think your rebasing failed a bit here, as several of my changes are inside your ‘‘prepare’’ commit now.
8. 
   jtimon commented at 5:29 am on July 18, 2015: contributor

   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.
9. jtimon force-pushed on Jul 18, 2015
10. jtimon force-pushed on Jul 20, 2015
11. Optimizations: Consensus: In main::AcceptToMemoryPool, main::ConnectBlock, 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

    d2f3c16a59
12. Optimization: Unify (fLimitFree && nFees - nFeesDeleted < ::minRelayTxFee.GetFee(nSize)) condition

    The following condition won't be checked anymore unless fLimitFree == true:
    
    GetBoolArg("-relaypriority", true) && !AllowFree(view.GetPriority(tx, chainActive.Height() + 1))

    4db612d3f8
13. Cleanup: GetMinRelayFee() -> AllowBelowMinRelayFee() abbcdb1063
14. Mempool: Prepare AcceptToMemoryPool to support mempool replacements

    With some code taken from Pieter Wuille's #6421

    4c51b3fc56
15. Mempool: Optimization: Introduce CTxChecksCache 935ee1ec87
16. Implement on-the-fly mempool size limitation. 2e779b8103
17. jtimon force-pushed on Jul 20, 2015
18. 
    jtimon commented at 10:09 pm on July 20, 2015: contributor

    I updated this with move movements and introducing a cache for the transaction checks and the fee calculations. Now, even maintaining the dub “reject everything when full” replacement policy, and even selecting -maxmempool=0 this should be safe to use unless I am missing something (and unless the user also selects -relaybeyondmempool=0).

    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

    Now apart from #6416 it also contains #6445.

19. jtimon closed this on Jul 26, 2015

    ---

20. MarcoFalke locked this on Sep 8, 2021

Contributors
jtimon petertodd sipa

Labels
Mempool