Policy: Prepare AcceptToMemoryPool for encapsulated alternative replacement policies #6416

1. 
   jtimon commented at 12:38 pm on July 10, 2015: contributor

   As usual with policy changes, this could be cleaner after #6068, but I’d rather not wait anymore to propose this, which has been in my mind since #5071 (because I don’t want policy to depend on txmempool). It would be good that all alternative replacement policy proposals are built on top of this, since the encapsulation makes review easier. Feedback from people that have been working on alternative replacement policies (for example, @petertodd and @luke-jr ) or people that are working on the mempool is specially welcomed. @morcos I’m thinking that the new CTxMemPool::addUnchecked that removes conflicts before inserting the new transaction may need some call the the fee estimator, but I’m not certain yet. What do you think?
2. jtimon force-pushed on Jul 10, 2015
3. jtimon force-pushed on Jul 11, 2015
4. jtimon renamed this:
   Prepare AcceptToMemoryPool for encapsulated alternative replacement policies
   Policy: Prepare AcceptToMemoryPool for encapsulated alternative replacement policies
   on Jul 11, 2015
5. jtimon force-pushed on Jul 12, 2015
6. jtimon force-pushed on Jul 16, 2015
7. 
   jtimon commented at 4:16 pm on July 16, 2015: contributor

   Updated with code from #6421.
8. jtimon force-pushed on Jul 16, 2015
9. 
   jtimon commented at 10:55 am on July 17, 2015: contributor

   @morcos here’s #6449 on top of #6416 : https://github.com/jtimon/bitcoin/commits/morcos-6449-over-6416

   Staging replacements and then remove them at the end of AcceptToMemoryPool in some way or another has been something that many people have tried, but working on alternatives to the “first seen” spend conflicts resolution policy was never a priority. Now it seems more priority than ever: apart from capping the mempool, we need some form of RBF so that people can bid up their transactions. But it is more prioritary to cap the mempool, fine. Fortunately, some of the first required steps for a “full mempool replacement policy” are the same as what “spend conflicts replacement policies” needs. Will that mean that some of those common steps for zerconf-safe-RBF will be taken sooner because it’s a priority for the mempool cap? I’m afraid no, any work on this direction, as well as any other improvement to AcceptToMemoryPool() [like #6445 #6420 #6068 ] will be frozen (or mousewheel rebased) until #6421 is fully finished, discussed, tested, merged and improved by later PRs. The reasoning being that, “anything that is simple to do before should be simple to do later”. @sipa @laanwj I hope I’m wrong about my prediction, but if that’s the plan I would prefer to know it as soon as possible to close this and some other PRs ( #6448 #6445 #6420 #6068) until #6421. Even though I believe some of the work mempool-cap work can be greatly simplified and made less disruptive to the rest of the development precisely after those PRs, if I’m the only who believes so, I guess it’s just better that I close my related PRs so that people can focus their “mental power” in reviewing only #6421 and PRs based on it.

10. laanwj added the label Refactoring on Jul 17, 2015
11. jtimon force-pushed on Jul 18, 2015
12. 
    jtimon commented at 9:05 am on July 18, 2015: contributor

    @morcos I would prefer to even go further with the preparations and move more things out of main, see https://github.com/jtimon/bitcoin/commits/policy-replacement-alt (which is closer to #6449 ), but I think it is unlikely that those kind of changes are welcomed for 0.11.1 (even thought it would mean that we can safely change AcceptToMemoryPool in 0.12.99 without having to worry about generating conflicts for a solution that can be backported to 0.11). What I’m trying to avoid is that other changes to AcceptToMemoryPool (of which I maintain many) get locked until 0.11.1, but maybe it’s a lost cause… What do you think @laanwj ?
13. jtimon force-pushed on Jul 20, 2015
14. 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
15. 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
16. Cleanup: GetMinRelayFee() -> AllowBelowMinRelayFee() abbcdb1063
17. jtimon force-pushed on Jul 20, 2015
18. Mempool: Prepare AcceptToMemoryPool to support mempool replacements

    With some code taken from Pieter Wuille's #6421

    4c51b3fc56
19. jtimon force-pushed on Jul 20, 2015
20. jtimon closed this on Jul 26, 2015

    ---

21. MarcoFalke locked this on Sep 8, 2021

Contributors
jtimon

Labels
Refactoring