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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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?
2797@@ -2798,6 +2798,12 @@ bool CWallet::CreateTransactionInternal(
2798 {
2799 LOCK(cs_wallet);
2800
2801+ CMutableTransaction txNew; // The resulting transaction that we make
2802+ txNew.nLockTime = GetLocktimeForNewTransaction(chain(), GetLastBlockHash(), GetLastBlockHeight());
2803+
2804+ CoinSelectionParams coin_selection_params; // Parameters for coin selection, init with dummy
2805+ coin_selection_params.m_avoid_partial_spends = coin_control.m_avoid_partial_spends;
commit d58f1fb0fdd43ed186073f9db477d9afd760c637 Move variable initializations to where they are used;
Nit: the commit message does not mention m_avoid_partial_spends, but m_avoid_reuse, was this a mix-up?
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:
0self.run_test()
1File "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/feature_fee_estimation.py", line 256, in run_test
2check_estimates(self.nodes[1], self.fees_per_kb)
3File "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/feature_fee_estimation.py", line 140, in check_estimates
4check_smart_estimates(node, fees_seen)
5File "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/feature_fee_estimation.py", line 130, in check_smart_estimates
6% (feerate, last_feerate))
7AssertionError: Estimated fee (0.000565) larger than last fee (0.000479) for lower number of confirms
c2a5bd68ef60d9c64512b205ff6ea4ce7dca8602 looks like a correct rebase
inputs_sum and recipients_sum
2794@@ -2795,287 +2795,270 @@ bool CWallet::CreateTransactionInternal(
2795 FeeCalculation& fee_calc_out,
2796 bool sign)
2797 {
2798- CAmount nValue = 0;
2799+ LOCK(cs_wallet);
In cb81bede9b Move cs_wallet lock in CreateTransactionInternal to top of function and Remove extraneous scope in CreateTransactionInternal
Seems like curly braces were supposed to limit the scope of the lock (?) but I am unsure why it should be released in the middle of CreateTransation(). If the intention is to hold this lock for all of CreateTransaction() (I don’t see why we’d want to release it in between calls to CreateTransactionInternal()), why not grab the lock in CreateTransation() and annotate that CreateTransactionInternal() requires cs_wallet?
2815+ unsigned int outputs_to_subtract_fee_from = 0; // The number of outputs which we are subtracting the fee from
2816+ for (const auto& recipient : vecSend) {
2817+ if (recipients_sum < 0 || recipient.nAmount < 0) {
2818 error = _("Transaction amounts must not be negative");
2819 return false;
2820 }
In 806c32f6ef Move empty recipients vector check to beginning in CreateTransaction
I think this check for non-negative amounts should be moved to the beginning of CreateTransaction() as well, since it’s a sanitization step similar to checking for empty vector. It doesn’t need to be checked more than once (and doesn’t need the lock).
It could also be
0if (std::any_of(vecSend.cbegin(), vecSend.cend(), [](const auto& recipient){ return recipient.nAmount < 0; }))
3118- // Dummy fill vin for maximum size estimation
3119- //
3120- for (const auto& coin : setCoins) {
3121- txNew.vin.push_back(CTxIn(coin.outpoint,CScript()));
3122- }
3123+ // Subtract fee from the change output if not subtrating it from recipient outputs
In bc0035a417 Remove extraneous scope in CreateTransactionInternal
0 // Subtract fee from the change output if not subtracting it from recipient outputs
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.
It isn't necessary to not lock parts of this function. Just lock the
whole thing and get rid of an indent.
These brackets were restricting a scope for no apparent reason. Remove
them and dedent.
nValue is the sum of the intended recipient amounts, so name it that for
clarity.
nValueToSelect is the coin selection target value, so name it
selection_target for clarity.
Renamed to outputs_to_subtract_fee_from for clarity.
Ensuring that the recipients vector is not empty and that the amounts
are non-negative can be done in CreateTransaction rather than
CreateTransactionInternal. Additionally, these checks should happen as
soon as possible, so they are done at the beginning of
CreateTransaction.
- txNew nLockTime setting to txNew init
- FeeCalc to the fee estimation fetching
- setCoins to prior to SelectCoins
- nBytes to CalculateMaximumSignedTxSize call
- tx_sizes to CalculateMaximumSignedTxSize call
- coin_selection_params.m_avoid_partial_spends to params init
Instead of setting this afterwards based on the results from the loop,
just do it inside of the loop itself.
Fixed some styling nearby
It's unnecessary to fill in the vin with dummy inputs, calculate the
fee, then fill in the vin with the actual inputs. Just fill the vin with
the actual inputs the first time.
SelectCoinsMinConf is a bit of a misnomer now since it really just does
all of the coin selection given some parameters. So rename this to
something less annoying to say and makes a bit more sense.
-BEGIN VERIFY SCRIPT-
sed -i 's/SelectCoinsMinConf/AttemptSelection/g' $(git grep -l SelectCoinsMinConf ./src)
-END VERIFY SCRIPT-
843
844- // Include the fees for things that aren't inputs, excluding the change output
845- const CAmount not_input_fees = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.tx_noinputs_size);
846- CAmount nValueToSelect = nValue + not_input_fees;
847+ // Shuffle selected coins and fill in final vin
848+ std::vector<CInputCoin> selected_coins(setCoins.begin(), setCoins.end());
In b583f73354 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?
setCoinsRet can be traced back to 0.1.0.
1051- txNew.vin.push_back(CTxIn(coin.outpoint, CScript(), nSequence));
1052- }
1053+ // Give up if change keypool ran out and change is required
1054+ if (scriptChange.empty() && nChangePosInOut != -1) {
1055+ return false;
1056+ }
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:
0diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
1index c8ded4c51e2..45a02f59a18 100644
2--- a/src/wallet/spend.cpp
3+++ b/src/wallet/spend.cpp
4@@ -756,9 +756,8 @@ bool CWallet::CreateTransactionInternal(
5 nFeeRet = coin_selection_params.m_effective_feerate.GetFee(nBytes);
6
7 // Subtract fee from the change output if not subtracting it from recipient outputs
8- CAmount fee_needed = nFeeRet;
9 if (!coin_selection_params.m_subtract_fee_outputs) {
10- change_position->nValue -= fee_needed;
11+ change_position->nValue -= nFeeRet;
12 }
13
14 // We want to drop the change to fees if:
15@@ -774,17 +773,12 @@ bool CWallet::CreateTransactionInternal(
16 // Because we have dropped this change, the tx size and required fee will be different, so let's recalculate those
17 tx_sizes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly);
18 nBytes = tx_sizes.vsize;
19- fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes);
20- }
21-
22- // Update nFeeRet in case fee_needed changed due to dropping the change output
23- if (fee_needed <= change_and_fee - change_amount) {
24- nFeeRet = change_and_fee - change_amount;
25+ nFeeRet = coin_selection_params.m_effective_feerate.GetFee(nBytes);
26 }
27
28 // Reduce output values for subtractFeeFromAmount
29 if (coin_selection_params.m_subtract_fee_outputs) {
30- CAmount to_reduce = fee_needed + change_amount - change_and_fee;
31+ CAmount to_reduce = nFeeRet + change_amount - change_and_fee;
32 int i = 0;
33 bool fFirst = true;
34 for (const auto& recipient : vecSend)
35@@ -816,7 +810,15 @@ bool CWallet::CreateTransactionInternal(
36 }
37 ++i;
38 }
39- nFeeRet = fee_needed;
40+ } else if (nFeeRet <= change_and_fee - change_amount) {
41+ // If dropping the change output covered the fee, update the returned
42+ // fee amount. Note that that in subtract-fee-from-recipients case
43+ // above, if the change output is dropped, the change dust value will
44+ // be paid to recipients, rather than to the miner (`to_reduce` above
45+ // will be negative). In that case, the dust amount sent is an
46+ // additional cost of the transaction, but it's not considered part of
47+ // the fee since it isn't paid to the miner, so nFeeRet doesn't change here.
48+ nFeeRet = change_and_fee - change_amount;
49 }
50
51 // 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.
777+ nFeeRet = coin_selection_params.m_effective_feerate.GetFee(nBytes);
778 }
779
780- // Update nFeeRet in case fee_needed changed due to dropping the change output
781- if (fee_needed <= change_and_fee - change_amount) {
782+ // Update nFeeRet in case it changed due to dropping the change output
In commit “Remove unneeded fee_needed variable” (303a6645dc604e1d074a1aa6253f4dc8a1c3109e)
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.”
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