wallet: Cleanup and refactor CreateTransactionInternal #22008

achow101 · 2021-05-20T20:35:58Z

#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.

DrahtBot · 2021-05-21T03:55:09Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

Sjors

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 472e42c why "Dummy fill vin" was added in the first place: it was introduced in #12699, but it's not clear to me why. @instagibbs?

achow101 · 2021-05-25T15:47:45Z

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

Sjors · 2021-05-25T16:13:18Z

re-utACK a2dd0db

src/wallet/wallet.cpp

murchandamus · 2021-05-26T18:10:33Z

tACK c2a5bd6

The rebase ate my most pressing comment. Thanks @instagibbs for #22042. ❤️

Sjors · 2021-05-26T18:26:32Z

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

c2a5bd6 looks like a correct rebase

ryanofsky · 2021-05-26T18:59:50Z

Code review ACK c2a5bd6. I like inputs_sum and recipients_sum

glozow

utACK c2a5bd6

A few minor suggestions

src/wallet/wallet.cpp

meshcollider

Code review & functional test run ACK ca46cdc

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

It isn't necessary to not lock parts of this function. Just lock the whole thing and get rid of an indent.

glozow

reACK 96c2c95

glozow · 2021-06-03T15:48:09Z

src/wallet/spend.cpp

-            const CAmount not_input_fees = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.tx_noinputs_size);
-            CAmount nValueToSelect = nValue + not_input_fees;
+    // Shuffle selected coins and fill in final vin
+    std::vector<CInputCoin> selected_coins(setCoins.begin(), setCoins.end());


In b583f73 Move vin filling to before final fee setting (but not super relevant to this PR)

Question: is there a reason setCoins (and setCoinsRet, etc. in the coin selection solvers) needs to be a std::set instead of a std::vector? It doesn't seem like we get much benefit out of using a set, and we convert to vector here anyway?

No reason other than it's always been that way. setCoinsRet can be traced back to 0.1.0.

src/wallet/spend.cpp

ryanofsky

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

ryanofsky · 2021-06-04T13:42:47Z

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 9d3bd74. 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

achow101 · 2021-06-04T17:07:49Z

@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.

ryanofsky

Reluctant code review ACK 303a664 because it does what is described, but I would prefer previous push 96c2c95 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 303a664 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)

ryanofsky · 2021-06-04T17:42:07Z

src/wallet/spend.cpp

    }

-    // Update nFeeRet in case fee_needed changed due to dropping the change output
-    if (fee_needed <= change_and_fee - change_amount) {
+    // Update nFeeRet in case it changed due to dropping the change output


In commit "Remove unneeded fee_needed variable" (303a664)

I think this comment is a big vague and maybe even misleading because this code isn't just updating the return value but also increasing the fee paid in the subtract-from-outputs case. I would maybe say something like "// Increase nFeeRet to reflect extra fee paid by giving up the small change amount which is smaller than the cost of the change output."

achow101 · 2021-06-04T18:10:50Z

I've reverted back to 96c2c95

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

ryanofsky

Code review ACK 96c2c95 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.

This commit does not change behavior in any way. It just removes a variable and adds a comment to explain nFeeRet return value better. The fee_needed variable became redundant after commit 9d3bd74 removed looping in CreateTransactionInternal. This change was originally posted bitcoin#22008 (comment) and discussed in the thread of that PR and in IRC. It might be followed up with more changes later to alter subtract from recipient behavior, but this commit just cleans up and documents the existing code.

ryanofsky · 2021-06-07T15:20:17Z

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

ryanofsky · 2021-06-07T15:25:20Z

This is probably close to ready to being merged

Up to date ACKs

glozow #22008 (review)
ryanofsky #22008 (review)

Previous ACKS

Sjors #22008 (review)
Sjors #22008 (comment)
Xekyo #22008 (comment)
ryanofsky #22008 (comment)
glozow #22008 (review)
meshcollider #22008 (review)

This commit does not change behavior in any way. It just removes a variable and adds a comment to explain nFeeRet return value better. The fee_needed variable became redundant after commit 9d3bd74 removed looping in CreateTransactionInternal. This change was originally posted bitcoin#22008 (comment) and discussed in the thread of that PR and in IRC. It might be followed up with more changes later to alter subtract from recipient behavior, but this commit just cleans up and documents the existing code.

meshcollider

re-utACK 96c2c95

…ternal

This commit does not change behavior in any way. It just removes a variable and adds a comment to explain nFeeRet return value better. The fee_needed variable became redundant after commit 9d3bd74 removed looping in CreateTransactionInternal. This change was originally posted bitcoin#22008 (comment) and discussed in the thread of that PR and in IRC. It might be followed up with more changes later to alter subtract from recipient behavior, but this commit just cleans up and documents the existing code.

This isn't nearly as bad as it looks -- use `git diff -w` to see past the indentation changes.

fanquake added the Wallet label May 20, 2021

DrahtBot mentioned this pull request May 21, 2021

wallet: Decide which coin selection solution to use based on waste metric #22009

Merged

Sjors reviewed May 21, 2021

View reviewed changes

achow101 mentioned this pull request May 21, 2021

wallet: Introduce SelectionResult for encapsulating a coin selection solution #22019

Merged

DrahtBot mentioned this pull request May 24, 2021

Replace size/weight estimate tuple with struct for named fields #22042

Merged

achow101 force-pushed the refactor-createtx branch from 6c0cb3c to 263ed9a Compare May 25, 2021 15:31

achow101 marked this pull request as ready for review May 25, 2021 15:47

achow101 force-pushed the refactor-createtx branch from 263ed9a to a2dd0db Compare May 25, 2021 15:49

DrahtBot added the Needs rebase label May 26, 2021

achow101 force-pushed the refactor-createtx branch from a2dd0db to c2a5bd6 Compare May 26, 2021 16:53

murchandamus reviewed May 26, 2021

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

DrahtBot removed the Needs rebase label May 26, 2021

glozow reviewed May 28, 2021

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

achow101 force-pushed the refactor-createtx branch from c2a5bd6 to ca46cdc Compare May 28, 2021 17:22

DrahtBot mentioned this pull request May 29, 2021

wallet: Properly support a wallet id #20205

Closed

meshcollider reviewed May 30, 2021

View reviewed changes

DrahtBot added the Needs rebase label May 30, 2021

Move cs_wallet lock in CreateTransactionInternal to top of function

b299596

It isn't necessary to not lock parts of this function. Just lock the whole thing and get rid of an indent.

achow101 force-pushed the refactor-createtx branch from ca46cdc to 96c2c95 Compare May 30, 2021 18:18

DrahtBot removed the Needs rebase label May 30, 2021

DrahtBot mentioned this pull request May 31, 2021

refactor: Clean up new wallet spend, receive files added #21207 #22100

Merged

glozow reviewed Jun 4, 2021

View reviewed changes

ryanofsky approved these changes Jun 4, 2021

View reviewed changes

achow101 force-pushed the refactor-createtx branch 2 times, most recently from 69f4aed to 303a664 Compare June 4, 2021 17:17

ryanofsky approved these changes Jun 4, 2021

View reviewed changes

achow101 force-pushed the refactor-createtx branch from 303a664 to 96c2c95 Compare June 4, 2021 18:09

ryanofsky approved these changes Jun 4, 2021

View reviewed changes

ryanofsky mentioned this pull request Jun 4, 2021

wallet test: Add test for subtract fee from recipient behavior #22155

Merged

meshcollider reviewed Jun 9, 2021

View reviewed changes

meshcollider merged commit 58f8b15 into bitcoin:master Jun 9, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 10, 2021

Merge bitcoin#22008: wallet: Cleanup and refactor CreateTransactionIn…

c3ea2f5

…ternal

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge 58f8b15 into merged_master (Bitcoin PR bitcoin/bitcoin#22008)

a5d97b3

This isn't nearly as bad as it looks -- use `git diff -w` to see past the indentation changes.

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

wallet: Cleanup and refactor CreateTransactionInternal #22008

wallet: Cleanup and refactor CreateTransactionInternal #22008

Uh oh!

Conversation

achow101 commented May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Sjors left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented May 25, 2021

Uh oh!

Sjors commented May 25, 2021

Uh oh!

Uh oh!

murchandamus commented May 26, 2021

Uh oh!

Sjors commented May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented May 26, 2021

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

meshcollider left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

glozow Jun 3, 2021

Choose a reason for hiding this comment

Uh oh!

achow101 Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Jun 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Jun 4, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jun 4, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Jun 7, 2021

Uh oh!

ryanofsky commented Jun 7, 2021

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 commented May 20, 2021 •

edited

Loading

DrahtBot commented May 21, 2021 •

edited

Loading

Sjors left a comment •

edited

Loading

Sjors commented May 26, 2021 •

edited

Loading

meshcollider left a comment •

edited

Loading

ryanofsky commented Jun 4, 2021 •

edited

Loading