Policy: Refactor: inline main:GetMinRelayFee() in main:AcceptToMemoryPool() #5709

1. 
   jtimon commented at 8:10 pm on January 25, 2015: contributor

   main::GetMinRelayFee() is only called from main::AcceptToMemoryPool(). By in-lining in we can make some code simplifications and test that no new money is being created before calling main::AreInputsStandard(), instead of doing it in https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L1032 after all those free transaction special cases. Even removing the nFees < 0 check would be safe since it is repeated in https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L1073 via https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L1471 But moving it up seems more DoS-safe. I’ve done the refactor step by step to to help with review.
2. laanwj added the label Improvement on Jan 26, 2015
3. jtimon force-pushed on Feb 3, 2015
4. 
   jtimon commented at 7:57 pm on February 3, 2015: contributor

   Needed rebase
5. jtimon force-pushed on Feb 4, 2015
6. jtimon renamed this:
   Change: Refactor: inline main:GetMinRelayFee() in main:AcceptToMemoryPool()
   Policy: Change: Refactor: inline main:GetMinRelayFee() in main:AcceptToMemoryPool()
   on Mar 25, 2015
7. 
   jtimon commented at 7:29 pm on June 25, 2015: contributor

   @luke-jr you once said you would like GetMinRelayFee to become part of policy, I would prefer to destroy it (and then pick it up as part of a new CPolicy method) instead. Do you like this? By the way, in case it wasn’t clear, this is supposed to be a 1 commit PR, as said it is just separated for easier review.
8. 
   jtimon commented at 4:27 pm on June 26, 2015: contributor

   I bet it will take very long until I have to rebase this because the refactored code is way too ugly for anyone to want to touch it. Hopefully by the time this gets any attention, +47 −67 while 0-functional-changes will be enough of an argument in favor of this single-commit refactor (I know, I have to s/SQUASHME:/fixup! but as said that may never happen because this may never need rebase). I would love to hear @morcos ’s opinion.
9. in src/main.cpp: in f03b16a53f outdated

   973@@ -1003,6 +974,13 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa
   974         view.SetBackend(dummy);
   975         }
   976 
   977+        CAmount nValueOut = tx.GetValueOut();
   978+        CAmount nFees = nValueIn-nValueOut;
   979+        // Don't accept it if it can't get into a block
   980+        if (nFees < 0)
   981+            return state.DoS(0, error("%s: negative fees %s, %d", __func__, hash.ToString(), nFees),
   982+                             REJECT_INSUFFICIENTFEE, "insufficient fee");
   

   ---

   ---

   

   jtimon commented at 4:33 pm on June 26, 2015:

   Probably “insufficient fee” should be replaced with “bad-txns-fee-negative” to better keep track of the validation duplication.
10. jtimon renamed this:
    Policy: Change: Refactor: inline main:GetMinRelayFee() in main:AcceptToMemoryPool()
    Policy: Refactor: inline main:GetMinRelayFee() in main:AcceptToMemoryPool()
    on Jun 26, 2015
11. 
    morcos commented at 5:16 pm on June 26, 2015: member

    I took a first pass at reviewing this. I think if you’re going to do a refactor here, you should improve the readability of the code. Although I like this code better than the original in some ways, it feels like you over optimized it instead of clarifying it.

    In any case, I don’t feel strongly, if you get some concept ACK’s, I’m happy to test it for you.

    I don’t think your second commit is right though, why do you think minRelayTxFee is in MoneyRange already?

12. 
    jtimon commented at 11:17 am on June 27, 2015: contributor

    Well, I was hoping that by doing the stuff change by change it could become obvious for reviewers that this doesn’t change anything functionally, but thanks for the testing offer (probably better to do it after some more review like you say).

    I don’t think your second commit is right though, why do you think minRelayTxFee is in MoneyRange already?

    You are right, I thought, ParseMoney had a moneyRange check. I should add one there or in https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L817

    The other place where minRelayTxFee is initialized is in https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L67

    I will fix that commit.

    Re optimizing: that’s just a nice side effect. The main goal is to reduce the amount of code before moving it to policy.

13. jtimon force-pushed on Jun 29, 2015
14. 
    jtimon commented at 9:03 am on June 29, 2015: contributor

    Updated fixing the second commit. Also reword “SQUASHME:” with “fixup!” in the commit descriptions.
15. Refactor: inline GetMinRelayFee() in AcceptToMemoryPool() 9287be6937
16. fixup! Both minRelayTxFee and 0 are already guaranteed to be in MoneyRange() 8b5382fdbe
17. fixup! mempool.ApplyDeltas() already locks mempool.cs 63b4eb78e4
18. fixup! Unify ifs 50e20124bc
19. fixup! Rather not calculate nMinFee at all unless fLimitFree c7840a0fc5
20. fixup! Get rid of nMinFee variable 9aa15aa8c8
21. fixup! in fact, you want to check that new money is not being created sooner than that bcd1484fa0
22. fixup! Micro-optimization: local bool variable to avoid doing the same check 3 times dc4b2082dd
23. fixup! Move fValidateFee condition out 0ffc049354
24. fixup! join 2 similar ifs 96141eb448
25. jtimon force-pushed on Jul 6, 2015
26. 
    jtimon commented at 11:36 pm on July 6, 2015: contributor

    Rebased
27. jtimon closed this on Jul 16, 2015

    ---

28. DrahtBot locked this on Sep 8, 2021

Contributors
jtimon morcos

Labels
Refactoring