[Mempool] Fix mempool limiting and replace-by-fee for PrioritiseTransaction #7062

1. 
   sdaftuar commented at 5:36 pm on November 19, 2015: member

   This fixes the behavior of the mempool limiting to respect any fee deltas applied via PrioritiseTransaction.

   (Note that the first 5 commits here are introduced by @morcos in #6898.)

   It also includes a couple other fixes relating to PrioritiseTransaction:

   • Updates RBF logic
   • Updates mempool acceptance logic
2. 
   sdaftuar commented at 8:11 pm on November 19, 2015: member

   This will create merge conflicts for #6871 – let’s try to get that reviewed and merged first, and I can resolve the merge conflicts in this PR afterward.
3. jonasschnelli added the label Mempool on Nov 20, 2015
4. 
   petertodd commented at 8:35 pm on November 20, 2015: contributor

   Note that this leaks information about what transactions you’ve prioritised to the outside world even more so than before. Probably unavoidable right now, but in the long run it’d be better if we had a separate mempool for mining that kept priority deltas. Transactions themselves can be shared across both mempools and reference counted.
5. in src/rpcblockchain.cpp: in f60b9035c8 outdated

   251             "    \"startingpriority\" : n, (numeric) priority when transaction entered pool\n"
   252             "    \"currentpriority\" : n,  (numeric) transaction priority now\n"
   253             "    \"descendantcount\" : n,  (numeric) number of in-mempool descendant transactions (including this one)\n"
   254             "    \"descendantsize\" : n,   (numeric) size of in-mempool descendants (including this one)\n"
   255-            "    \"descendantfees\" : n,   (numeric) fees of in-mempool descendants (including this one)\n"
   256+            "    \"descendantfees\" : n,   (numeric) modified fees of in-mempool descendants (including this one)\n"
   

   ---

   ---

   

   petertodd commented at 8:36 pm on November 20, 2015:

   The term “modified fees” won’t mean anything to the reader. How about “fees of in-mempool descendants (including this one) with priority deltas applied”

   ---

   

   sdaftuar commented at 1:25 pm on November 24, 2015:

   I was trying to make a reference to the term “modifedfee” which I added above without having to redefine the concept, what if I just do: “modified fees (see above) of in-mempool descendants (including this one)”
6. 
   petertodd commented at 8:43 pm on November 20, 2015: contributor

   Otherwise, utACK, though will want to recheck rebased version once #6871 is merged of course.
7. sdaftuar force-pushed on Nov 30, 2015
8. sdaftuar renamed this:
   [Mempool] Fix mempool limiting for PrioritiseTransaction
   [Mempool] Fix mempool limiting and replace-by-fee for PrioritiseTransaction
   on Nov 30, 2015
9. 
   sdaftuar commented at 9:55 pm on November 30, 2015: member

   Rebased. @petertodd This pull has now been updated to use fee deltas for the replace-by-fee calculation, and the replace-by-fee rpc test has been updated to exercise that logic. (I cherry-picked #7137, so if that gets merged first I’ll rebase this one, or I can close that pull if that’s easier.)
10. in qa/rpc-tests/replace-by-fee.py: in 4b0c38c310 outdated

    71+            if (mempool_size == new_size):
    72+                # We might have stuck transactions; assume that
    73+                # the transactions we needed mined were mined, and
    74+                # don't worry about what is left.
    75+                break
    76+            mempool_size = new_size
    

    ---

    ---

    

    petertodd commented at 1:50 am on December 1, 2015:

    FYI, I’ve found bugs before by erroring out if any txs are stuck; not sure this is a good idea.

    ---

    

    sdaftuar commented at 2:57 pm on December 1, 2015:

    Fair enough – I ran into an issue where the really large transaction created in one of the tests was too big to mined, but I should be able to rework that test and make this check error out instead.
11. sdaftuar force-pushed on Dec 1, 2015
12. 
    sdaftuar commented at 5:57 pm on December 1, 2015: member

    Rebased and changed rpc test to error out if the mempool ends up with stuck transactions.
13. sdaftuar force-pushed on Dec 1, 2015
14. 
    sdaftuar commented at 7:34 pm on December 1, 2015: member

    Rebased now that #6898 is merged
15. laanwj added this to the milestone 0.12.0 on Dec 2, 2015
16. sdaftuar force-pushed on Dec 2, 2015
17. 
    sdaftuar commented at 4:28 pm on December 2, 2015: member

    Updated so that now mempool acceptance is also determined by including a transaction’s fee delta from PrioritiseTransaction.

    In doing so, I realized that GetMinRelayFee is not very useful, so I scrapped it. With the DEFAULT_BLOCK_PRIORITY_SIZE set to 0, this function was stopping all free transactions from making it in (unless they were prioritised using PrioritiseTransaction). Scrapping it means that there is a behavior change from before, where now we will allow in transactions between 50kb and 100kb that are free as long as they have sufficient priority. Given that when the mempool is full we don’t allow free transactions at all, and given that we expire things after 3 days, I think this should be okay.

    EDIT: Actually, there’s a bug in GetMinRelayFee so that the comparison with DEFAULT_BLOCK_PRIORITY_SIZE was incorrect; the comparison is comparing two things as unsigned int’s, but the right hand side is negative… So this test isn’t preventing anything from getting into the mempool now that the DEFAULT_BLOCK_PRIORITY_SIZE is 0.

    0if (nBytes < (DEFAULT_BLOCK_PRIORITY_SIZE - 1000))      
    1        nMinFee = 0;
    

18. 
    morcos commented at 4:50 pm on December 2, 2015: member

    utACK (modulo comment above)
19. sdaftuar force-pushed on Dec 2, 2015
20. 
    sdaftuar commented at 5:59 pm on December 2, 2015: member

    Addressed @morcos’ nit (and squashed into first commit).
21. Fix mempool limiting for PrioritiseTransaction

    Redo the feerate index to be based on mining score, rather than fee.
    
    Update mempool_packages.py to test prioritisetransaction's effect on
    package scores.

    eb306664e7
22. Update replace-by-fee logic to use fee deltas 9ef2a25603
23. Use fee deltas for determining mempool acceptance 27fae3484c
24. Remove GetMinRelayFee

    One test in AcceptToMemoryPool was to compare a transaction's fee
    agains the value returned by GetMinRelayFee. This value was zero for
    all small transactions.  For larger transactions (between
    DEFAULT_BLOCK_PRIORITY_SIZE and MAX_STANDARD_TX_SIZE), this function
    was preventing low fee transactions from ever being accepted.
    
    With this function removed, we will now allow transactions in that range
    with fees (including modifications via PrioritiseTransaction) below
    the minRelayTxFee, provided that they have sufficient priority.

    901b01d674
25. 
    laanwj commented at 4:13 pm on December 21, 2015: member

    utACK
26. laanwj merged this on Dec 21, 2015
27. laanwj closed this on Dec 21, 2015

    ---

28. laanwj referenced this in commit c24337964f on Dec 21, 2015
29. laanwj referenced this in commit 12c469b236 on Dec 21, 2015
30. furszy referenced this in commit 1f010c0969 on Jan 23, 2021
31. MarcoFalke locked this on Sep 8, 2021

Contributors
sdaftuar petertodd morcos laanwj

Labels
Mempool

Milestone
0.12.0