wallet: Move fee underpayment check to after all fee has been set #26643

1. 
   achow101 commented at 8:41 PM on December 5, 2022: member

   Currently the fee underpayment check occurs right after we calculate what the transaction's fee should be. However the fee paid by the transaction at that time does not always match. Notably, when doing SFFO, the fee paid at that time will almost always be less than the fee required, which then required having a bypass of the underpayment check that results in SFFO payments going through when they should not.

   This PR moves the underpayment check to after fees have been finalized so that we always check whether the fee is being underpaid. This removes the exception for SFFO and unifies this behavior for both SFFO and non-SFFO txs.

2. 
   DrahtBot commented at 8:41 PM on December 5, 2022: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	furszy, S3RK, glozow
   Stale ACK	john-moffett

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #26661 (wallet: Coin Selection, return accurate error messages by furszy)
   • #25273 (wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction by achow101)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

3. DrahtBot added the label Wallet on Dec 5, 2022
4. in src/wallet/spend.cpp:959 in 427e6f95ba outdated

    957 | @@ -952,19 +958,6 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
    958 |      CAmount fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes);
    959 |      CAmount current_fee = result->GetSelectedValue() - recipients_sum - change_amount;
   

   ---

   ---

   

   S3RK commented at 8:23 AM on December 6, 2022:

   Should we define it the same way as down below?

   -CAmount current_fee = result->GetSelectedValue() - recipients_sum - change_amount;
   +CAmount current_fee = result->GetSelectedValue() - CalculateOutputValue(txNew);
   

5. in src/wallet/spend.cpp:760 in 427e6f95ba outdated

    756 | @@ -756,6 +757,11 @@ static void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng
    757 |      }
    758 |  }
    759 |  
    760 | +static CAmount CalculateOutputValue(const CMutableTransaction& tx)
   

   ---

   ---

   

   S3RK commented at 8:23 AM on December 6, 2022:

   Have you considered making it a member of CMutableTransaction?

6. DrahtBot added the label Needs rebase on Dec 6, 2022
7. wallet: Rename nFeeRet in CreateTransactionInternal to current_fee

   nFeeRet represents the fee that the transaction currently pays. Update
   it's name to reflect that.

   e5daf976d5
8. achow101 force-pushed on Dec 6, 2022
9. achow101 marked this as ready for review on Dec 6, 2022
10. DrahtBot removed the label Needs rebase on Dec 6, 2022
11. john-moffett approved
12. 
    john-moffett commented at 4:41 PM on December 7, 2022: contributor

    ACK 212e1497c3ab4d01a791179dc7b93236a1206d2b

13. in src/wallet/spend.cpp:963 in 1574bfa0b6 outdated

     972 | -    if (nChangePosInOut != -1 && fee_needed < current_fee) {
     973 | -        auto& change = txNew.vout.at(nChangePosInOut);
     974 | -        change.nValue += current_fee - fee_needed;
     975 | -        current_fee = fee_needed;
     976 | -    }
     977 | +    CAmount current_fee = result->GetSelectedValue() - CalculateOutputValue(txNew);
    

    ---

    ---

    

    furszy commented at 7:54 PM on December 8, 2022:

    In 1574bfa0:

    Hmm, guess that the CalculateOutputValue is meant to be a guard? Because, atm, there seems to not be possible to have recipients_sum + change_amount != CalculateOutputValue(txNew)

    If that is the case, what about adding an Assume here?

    ---

    

    achow101 commented at 9:26 PM on December 8, 2022:

    Done

14. in src/wallet/spend.cpp:984 in 212e1497c3 outdated

     980 |      }
     981 |  
     982 |      // Reduce output values for subtractFeeFromAmount
     983 |      if (coin_selection_params.m_subtract_fee_outputs) {
     984 | -        CAmount to_reduce = fee_needed - nFeeRet;
     985 | +        CAmount to_reduce = fee_needed - current_fee;
    

    ---

    ---

    

    furszy commented at 9:04 PM on December 8, 2022:

    This is probably too much but.. could fee_needed be lower than current_fee? Because.. if that happens, to_reduce will be negative so instead of reducing value from the outputs, we would be adding it.

    ---

    

    achow101 commented at 9:16 PM on December 8, 2022:

    Yes, with a sufficiently large change output script and feerate, this is possible, and (currently) expected behavior. We test for it at spend_tests.cpp:61.

    ---

    

    furszy commented at 1:30 PM on December 9, 2022:

    Interesting. That is the opposite to what someone would expect from a functionality called "subtract fee from outputs". but.. all good anyway, something for later.

15. achow101 force-pushed on Dec 8, 2022
16. 
    furszy commented at 2:44 PM on December 9, 2022: member

    There is something that is making some noise inside me.

    Based on the convo that we had above #26643 (review).

    Inside 8c8e1738, you moved the change output increment due a fees overpay below the SFFO functionality. So, now if there is a fee surplus, instead of adding it to the change output first, we will add it to the recipients amount if SFFO is enabled. (if there is an extra fee, "fee needed" will be lower than "current fee" so the SFFO to_reduce amount will be negative, so instead of subtract the amount we will add it to the recipients amount. Sending more coins than what the user specified to send to each SFFO enabled output instead of sending them back to us in the change output).

    Should create a test for this (the current SubtractFee tests aren't contemplating change). It smells bad.

17. wallet: Move fee underpayment check to after fee setting

    It doesn't make sense to be checking whether the fee paid is underpaying
    before we've finished setting the fees. So do that after we have done
    the reduction for SFFO and change adjustment for fee overpayment.

    c1a84f108e
18. wallet: Sanity check fee paid cannot be negative

    We need to check that the fee is not negative even before it is
    finalized. The setting of fees for SFFO may adjust the fee to be
    "correct" and no longer negative, but erroneously reduce the amounts too
    far. So we need to check this condition before we do those adjustments.

    798430d127
19. achow101 force-pushed on Dec 9, 2022
20. 
    achow101 commented at 8:15 PM on December 9, 2022: member

    Inside 8c8e173, you moved the change output increment due a fees overpay below the SFFO functionality. So, now if there is a fee surplus, instead of adding it to the change output first, we will add it to the recipients amount if SFFO is enabled. (if there is an extra fee, "fee needed" will be lower than "current fee" so the SFFO to_reduce amount will be negative, so instead of subtract the amount we will add it to the recipients amount. Sending more coins than what the user specified to send to each SFFO enabled output instead of sending them back to us in the change output).

    I've flipped it back around so the order stays the same.

    However, I don't think the scenario you describe is even at all possible. The only times that fee_needed < current_fee is possible are if the change was small and thrown away as fees, or due to rounding errors in the fee estimates for coin selection. The the former case, the condition you mention cannot be reached because there is no change output. Because SFFO does not account for fees at all (it doesn't include any fees in the selection target and it only uses the real values during the selection algorithms), the latter case cannot be reached. Thus the scenario you describe is not possible.

21. 
    furszy commented at 12:24 AM on December 10, 2022: member

    ok great, thanks. The noice is no longer there.

    Because SFFO does not account for fees at all (it doesn't include any fees in the selection target and it only uses the real values during the selection algorithms), the latter case cannot be reached. Thus the scenario you describe is not possible.

    In that case, what about Assume(current_fee == 0) for SFFO?.

22. furszy approved
23. 
    furszy commented at 12:25 AM on December 10, 2022: member

    Code review ACK 798430d

24. fanquake requested review from john-moffett on Dec 10, 2022
25. fanquake removed review request from john-moffett on Dec 10, 2022
26. fanquake requested review from S3RK on Dec 10, 2022
27. 
    S3RK commented at 7:45 AM on December 12, 2022: contributor

    Code review ACK 798430d127521d088c081ee625912a704f415990

28. fanquake requested review from glozow on Dec 12, 2022
29. 
    glozow commented at 2:12 PM on December 12, 2022: member

    utACK 798430d127, code looks correct to me

30. achow101 merged this on Dec 13, 2022
31. achow101 closed this on Dec 13, 2022

    ---

32. sidhujag referenced this in commit 2b40d86f29 on Dec 14, 2022
33. bitcoin locked this on Dec 13, 2023

Contributors

achow101DrahtBotS3RKjohn-moffettfurszyglozow

Review Requested
S3RK