wallet: Cleanup and refactor CreateTransactionInternal #22008

#17331 did some refactors and cleanup of CreateTransactionInternal to make it easier to understand, however it is still a bit convoluted even though it doesn't have to be. This PR does additional cleanup and refactoring to CreateTransactionInternal so that it is easier to understand. Some unnecessary code was removed, some variables moved around to where they matter, and several indents removed.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #22155 (wallet test: Add test for subtract fee from recipient behavior by ryanofsky)
• #22100 (refactor: Clean up new wallet spend, receive files added #21207 by ryanofsky)
• #22009 (wallet: Decide which coin selection solution to use based on waste metric by achow101)
• #21206 (refactor: Make CWalletTx sync state type-safe by ryanofsky)
• #20205 (wallet: Properly support a wallet id by achow101)
• #17526 (Use Single Random Draw In addition to knapsack as coin selection fallback by achow101)
• #17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs 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.

utACK 6c0cb3c (the 9 commits in this PR)

Linter seems upset (about indentation?) in the scripted diff commit.

Maybe also move CreateTransactionInternal out of the header?

For code archaeologists wondering about 472e42c2974c3ecca6bdb71f082925d86e3def35 why "Dummy fill vin" was added in the first place: it was introduced in #12699, but it's not clear to me why. @instagibbs?

With #17331 now merged, this is ready for review.

re-utACK a2dd0dbc91da89a5672d2e578998252b1a3ceabc

tACK c2a5bd68ef60d9c64512b205ff6ea4ce7dca8602

The rebase ate my most pressing comment. Thanks @instagibbs for #22042. :heart:

The last rebase broke the fee estimation test on CI, though perhaps a coincidence; I can't reproduce:

self.run_test()
File "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/feature_fee_estimation.py", line 256, in run_test
check_estimates(self.nodes[1], self.fees_per_kb)
File "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/feature_fee_estimation.py", line 140, in check_estimates
check_smart_estimates(node, fees_seen)
File "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/feature_fee_estimation.py", line 130, in check_smart_estimates
% (feerate, last_feerate))
AssertionError: Estimated fee (0.000565) larger than last fee (0.000479) for lower number of confirms

c2a5bd68ef60d9c64512b205ff6ea4ce7dca8602 looks like a correct rebase

Code review ACK c2a5bd68ef60d9c64512b205ff6ea4ce7dca8602. I like inputs_sum and recipients_sum

utACK c2a5bd68ef

A few minor suggestions

Code review & functional test run ACK ca46cdc2af4919093a8fce8693c49f874f57d492

Unfortunately I am going to merge #21207 first so this will need one more rebase before merge sorry.

reACK 96c2c95

Code review ACK 96c2c9520e80ee4fed92f0e1ab859d59fcbdb110. No significant changes since last review, just rebase, moving lock and negative amounts check and fixing up some comments

Maybe another cleanup for later, but the separate fee_needed and nFeeRet variables haven't been needed since the CreateTransactionInternal while loop was removed in 9d3bd74ab4430532d6e53eef8cf77ad999044b14. fee_needed was only introduced before that commit because if a change output was dropped in an early iteration of the loop, fee_needed would hold the cost of the transaction without the change output for the current loop iteration, while nFeeRet would reflect the cost of the transaction with change for future iterations. So fee_needed can be dropped now without changing behavior, which I think would clarify the code:

diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index c8ded4c51e2..45a02f59a18 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -756,9 +756,8 @@ bool CWallet::CreateTransactionInternal(
     nFeeRet = coin_selection_params.m_effective_feerate.GetFee(nBytes);
 
     // Subtract fee from the change output if not subtracting it from recipient outputs
-    CAmount fee_needed = nFeeRet;
     if (!coin_selection_params.m_subtract_fee_outputs) {
-        change_position->nValue -= fee_needed;
+        change_position->nValue -= nFeeRet;
     }
 
     // We want to drop the change to fees if:
@@ -774,17 +773,12 @@ bool CWallet::CreateTransactionInternal(
         // Because we have dropped this change, the tx size and required fee will be different, so let's recalculate those
         tx_sizes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly);
         nBytes = tx_sizes.vsize;
-        fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes);
-    }
-
-    // Update nFeeRet in case fee_needed changed due to dropping the change output
-    if (fee_needed <= change_and_fee - change_amount) {
-        nFeeRet = change_and_fee - change_amount;
+        nFeeRet = coin_selection_params.m_effective_feerate.GetFee(nBytes);
     }
 
     // Reduce output values for subtractFeeFromAmount
     if (coin_selection_params.m_subtract_fee_outputs) {
-        CAmount to_reduce = fee_needed + change_amount - change_and_fee;
+        CAmount to_reduce = nFeeRet + change_amount - change_and_fee;
         int i = 0;
         bool fFirst = true;
         for (const auto& recipient : vecSend)
@@ -816,7 +810,15 @@ bool CWallet::CreateTransactionInternal(
             }
             ++i;
         }
-        nFeeRet = fee_needed;
+    } else if (nFeeRet <= change_and_fee - change_amount) {
+        // If dropping the change output covered the fee, update the returned
+        // fee amount. Note that that in subtract-fee-from-recipients case
+        // above, if the change output is dropped, the change dust value will
+        // be paid to recipients, rather than to the miner (`to_reduce` above
+        // will be negative). In that case, the dust amount sent is an
+        // additional cost of the transaction, but it's not considered part of
+        // the fee since it isn't paid to the miner, so nFeeRet doesn't change here.
+        nFeeRet = change_and_fee - change_amount;
     }
 
     // Give up if change keypool ran out and change is required

@ryanofsky I agree that fee_needed can be dropped, but I don't think your diff is necessarily correct. Looking at it now, I don't think the current behavior is correct either (and your diff maintains this behavior). We shouldn't ever be paying more to the recipient than the sender expects. The excess, if it is dust, should be burned as fees. So your diff, and the current behavior, is incorrect in that if the dropped change output is more than fees, then recipients will receive more than requested by the sender.

By simply removing fee_needed, I think we can get the behavior that we want. In this case, after the change is dropped, we will update nFeeRet to be the dropped change amount. Then to_reduce ends up being 0 and the recipients don't pay any fees, but also don't get anything back, so the excess is paid as fees. I believe that this is the intended behavior, and is simpler to understand too.

Reluctant code review ACK 303a6645dc604e1d074a1aa6253f4dc8a1c3109e because it does what is described, but I would prefer previous push 96c2c9520e80ee4fed92f0e1ab859d59fcbdb110 over this push, and then addressing fee_needed behavior and cleanup in a followup PR, ideally including a unit test so we are sure this continues to function as intended.

I wouldn't completely object to changing the behavior like 303a6645dc604e1d074a1aa6253f4dc8a1c3109e does, since the difference is only for small amounts (the cost of a change output) and only in the-subtract-from-recipient case, so I don't think it is too important. But I do think the current behavior does make sense, and I preserved it intentionally in #22008 (comment) and not by accident. Thinking about use-cases, if I enable subtracting-from-recipient, it seems likely I'm transferring funds internally and not making an external payment, and so would prefer to keep my funds instead of overpaying the miner. Even if this is an external payment, then the subtract-from-recipient option already implies the recipient is not sensitive to the exact amount, and so if I'm topping up an external account or a channel, or paying for an online service, again the new behavior would be throwing away my own money/credits to give to a miner.

The one part of current behavior that is perhaps is less than ideal is the fact that the returned nFeeRet doesn't reflect the full cost of sending the transaction. I would fix this just by changing the else if (nFeeRet.. to if (nFeeRet... and dropping the long "note" in #22008 (comment)

I've reverted back to 96c2c9520e80ee4fed92f0e1ab859d59fcbdb110

I think we should discuss the intended behavior of subtract fee from recipients during the wallet meeting today.

Code review ACK 96c2c9520e80ee4fed92f0e1ab859d59fcbdb110 also acked previously (was reverted).

Will check out the IRC. I will say I don't see a need to tackle fee_needed in this PR since the PR is already doing a bunch of things and has already had a good amount of review. fee_needed logic also seems more tied to removing the while loop in #17331 than anything done here. And especially if it will change behavior, I think it could be better as standalone PR so it can be discussed and understood without having wade through a ton of refactoring.

re: #22008 (comment)

Maybe another cleanup for later, but the separate fee_needed and nFeeRet variables haven't been needed [...]

fee_needed cleanup was discussed more in IRC and is addressed in #22155 for now adding by a unit test to exercise and check the relevant behavior, since there isn't other test coverage

This is probably close to ready to being merged

Up to date ACKs

glozow #22008#pullrequestreview-675439314 ryanofsky #22008#pullrequestreview-676223208

Previous ACKS

Sjors #22008#pullrequestreview-665540991 Sjors #22008 (comment) Xekyo #22008 (comment) ryanofsky #22008 (comment) glozow #22008#pullrequestreview-671176599 meshcollider #22008#pullrequestreview-671810704

re-utACK 96c2c9520e80ee4fed92f0e1ab859d59fcbdb110