Eliminate fee overpaying edge case when subtracting fee from recipients #10942

1. 
   morcos commented at 9:27 pm on July 27, 2017: member

   I’m not sure if this is the cause of the issue in #10034 , but this was a known edge case. I just didn’t realize how simple the fix is.

   Could use a couple more eyes to make sure nothing silly can go wrong here, but if we all agree it’s this simple, we can add this as another 0.15 bug fix.

2. MarcoFalke added this to the milestone 0.15.0 on Jul 27, 2017
3. MarcoFalke added the label Wallet on Jul 27, 2017
4. in src/wallet/wallet.cpp:2823 in 242c38b4d2 outdated

   2817@@ -2820,6 +2818,12 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
   2818                     }
   2819                 }
   2820 
   2821+                // If subtracting fee from recipients, we now know what fee we
   2822+                // need to subtract, we have no reason to reselect inputs
   2823+                if (nSubtractFeeFromAmount && nFeeRet == 0) {
   

   ---

   ---

   

   promag commented at 10:00 pm on July 27, 2017:

   Nit, nSubtractFeeFromAmount > 0.

   ---

   

   promag commented at 10:03 pm on July 27, 2017:

   nFeeRet == 0 should not be necessary? So the first iteration should have enough inputs for when nSubtractFeeFromAmount > 0, or am I missing something?

   ---

   

   promag commented at 10:20 pm on July 27, 2017:

   Actually, because of

   0                    // Never create dust outputs; if we would, just
   1                    // add the dust to the fee.
   2                    if (IsDust(newTxOut, discard_rate))
   3                    {
   4                        nChangePosInOut = -1;
   5                        nFeeRet += nChange;
   6                    }
   

   nFeeRet can be > 0, and in that case there is no need to pick new inputs too.

   ---

   

   morcos commented at 2:07 am on July 28, 2017:

   Good catch. The nFeeRet == 0 check wasn’t really doing anything useful anyway, because pick_new_inputs doesn’t ever get reset to true. But the prior check that I edited the comment on asserts that we are always on our final pass if pick_new_inputs is false.
5. Eliminate fee overpaying edge case when subtracting fee from recipients 49d903e696
6. morcos force-pushed on Jul 28, 2017
7. 
   promag commented at 6:28 am on July 28, 2017: member

   utACK 49d903e6.
8. 
   instagibbs commented at 2:10 am on July 31, 2017: member

   utACK https://github.com/bitcoin/bitcoin/pull/10942/commits/49d903e6961b42feda9036897f3ff0103dfccd4b
9. 
   sipa commented at 4:08 am on July 31, 2017: member

   Concept ACK
10. 
    Leviathn commented at 10:01 pm on July 31, 2017: none

    Concept ACK
11. 
    laanwj commented at 1:01 pm on August 1, 2017: member

    utACK 49d903e, looks sane to me
12. 
    sdaftuar commented at 5:38 pm on August 2, 2017: member

    utACK; agree this would be good to include in 0.15 as a bugfix.

    Perhaps edit the PR description to explain this a bit better (as that is now included in the merge commit)?

13. 
    TheBlueMatt commented at 5:52 pm on August 2, 2017: member

    utACK, it would be good to clean this function up to calculate change output after calculating the fee at some point post-15.
14. gmaxwell approved
15. 
    gmaxwell commented at 7:49 am on August 3, 2017: contributor

    utACK
16. 
    jonasschnelli commented at 7:51 am on August 3, 2017: contributor

    utACK 49d903e6961b42feda9036897f3ff0103dfccd4b
17. laanwj merged this on Aug 3, 2017
18. laanwj closed this on Aug 3, 2017

    ---

19. laanwj referenced this in commit 2e857bb619 on Aug 3, 2017
20. PastaPastaPasta referenced this in commit 63d259e5f5 on Aug 6, 2019
21. PastaPastaPasta referenced this in commit 0441bf2cbd on Aug 6, 2019
22. PastaPastaPasta referenced this in commit 778404621a on Aug 6, 2019
23. PastaPastaPasta referenced this in commit bdc7ca1fdc on Aug 7, 2019
24. PastaPastaPasta referenced this in commit 4d6bc0e571 on Aug 8, 2019
25. PastaPastaPasta referenced this in commit e72a2e278f on Aug 12, 2019
26. barrystyle referenced this in commit 013237fbcd on Jan 22, 2020
27. MarcoFalke locked this on Sep 8, 2021

Contributors
morcos promag instagibbs sipa Leviathn laanwj sdaftuar TheBlueMatt gmaxwell jonasschnelli

Labels
Wallet

Milestone
0.15.0