Set effective_value when initializing a COutput

ishaanam commented at 10:00 pm on May 7, 2022: member

Previously in COutput, effective_value was initialized as the absolute value of the txout and the fee as 0. effective_value along with the fee was calculated outside of the COutput constructor and set after the object had been initialized. These changes will allow either the fee or the feerate to be passed in a COutput constructor and the fee and effective_value are calculated and set in the constructor. As a result, AvailableCoins also needs to be passed the feerate when utxos are being spent. When balance is calculated or the coins are being listed and feerate is neither available nor required, AvailableCoinsListUnspent is used instead, which runs AvailableCoins while providing the default value for feerate. Unit tests for the calculation of effective value have also been added.

DrahtBot added the label RPC/REST/ZMQ on May 7, 2022

DrahtBot added the label Wallet on May 7, 2022

DrahtBot commented at 6:24 am on May 8, 2022: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25005 (wallet: remove extra wtx lookup in ‘AvailableCoins’ + several code cleanups. by furszy)
#24584 (wallet: avoid mixing different OutputTypes during coin selection by josibake)

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.

in src/wallet/spend.h:40 in b1adf9f3d4 outdated

34@@ -35,7 +35,9 @@ TxSize CalculateMaximumSignedTxSize(const CTransaction& tx, const CWallet* walle
35 /**
36  * populate vCoins with vector of available COutputs.
37  */
38-void AvailableCoins(const CWallet& wallet, std::vector<COutput>& vCoins, const CCoinControl* coinControl = nullptr, const CAmount& nMinimumAmount = 1, const CAmount& nMaximumAmount = MAX_MONEY, const CAmount& nMinimumSumAmount = MAX_MONEY, const uint64_t nMaximumCount = 0) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
39+void AvailableCoins(const CWallet& wallet, std::vector<COutput>& vCoins, const CCoinControl* coinControl = nullptr, const CAmount& nMinimumAmount = 1, std::optional<CFeeRate> feerate = std::nullopt, const CAmount& nMaximumAmount = MAX_MONEY, const CAmount& nMinimumSumAmount = MAX_MONEY, const uint64_t nMaximumCount = 0) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
40+
41+void AvailableCoinsListUnspent(const CWallet& wallet, std::vector<COutput>& vCoins, const CCoinControl* coinControl = nullptr, const CAmount& nMinimumAmount = 1, const CAmount& nMaximumAmount = MAX_MONEY, const CAmount& nMinimumSumAmount = MAX_MONEY, const uint64_t nMaximumCount = 0);

achow101 commented at 1:36 pm on May 8, 2022:

0void AvailableCoinsListUnspent(const CWallet& wallet, std::vector<COutput>& vCoins, const CCoinControl* coinControl = nullptr, const CAmount& nMinimumAmount = 1, const CAmount& nMaximumAmount = MAX_MONEY, const CAmount& nMinimumSumAmount = MAX_MONEY, const uint64_t nMaximumCount = 0) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);

This should fix the clang error.

ishaanam commented at 5:53 pm on May 8, 2022:

Thanks, it’s fixed now

ishaanam force-pushed on May 8, 2022

in src/wallet/spend.cpp:194 in 7da2200c91 outdated

190@@ -191,8 +191,12 @@ void AvailableCoins(const CWallet& wallet, std::vector<COutput>& vCoins, const C
191             bool solvable = provider ? IsSolvable(*provider, wtx.tx->vout[i].scriptPubKey) : false;
192             bool spendable = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (((mine & ISMINE_WATCH_ONLY) != ISMINE_NO) && (coinControl && coinControl->fAllowWatchOnly && solvable));
193             int input_bytes = GetTxSpendSize(wallet, wtx, i, (coinControl && coinControl->fAllowWatchOnly));
194-
195-            vCoins.emplace_back(COutPoint(wtx.GetHash(), i), wtx.tx->vout.at(i), nDepth, input_bytes, spendable, solvable, safeTx, wtx.GetTxTime(), tx_from_me);
196+

achow101 commented at 2:58 pm on May 8, 2022:

There is extra whitepace on this line that the linter doesn’t like.

in src/bench/coin_selection.cpp:61 in 7da2200c91 outdated

57@@ -58,7 +58,7 @@ static void CoinSelection(benchmark::Bench& bench)
58     // Create coins
59     std::vector<COutput> coins;
60     for (const auto& wtx : wtxs) {
61-        coins.emplace_back(COutPoint(wtx->GetHash(), 0), wtx->tx->vout.at(0), /*depth=*/6 * 24, GetTxSpendSize(wallet, *wtx, 0), /*spendable=*/true, /*solvable=*/true, /*safe=*/true, wtx->GetTxTime(), /*from_me=*/true);
62+        coins.emplace_back(COutPoint(wtx->GetHash(), 0), wtx->tx->vout.at(0), /*depth=*/6 * 24, GetTxSpendSize(wallet, *wtx, 0), /*spendable=*/true, /*solvable=*/true, /*safe=*/true, wtx->GetTxTime(), /*from_me=*/true, /*fee=*/ 0);

achow101 commented at 3:00 pm on May 8, 2022:

s/fee/fees

Here and elsewhere. clang-tidy is complaining about this.

ishaanam commented at 1:32 pm on May 9, 2022:

I’ve changed it everywhere, thanks.

in src/wallet/spend.h:38 in 7da2200c91 outdated

34@@ -35,7 +35,9 @@ TxSize CalculateMaximumSignedTxSize(const CTransaction& tx, const CWallet* walle
35 /**
36  * populate vCoins with vector of available COutputs.
37  */
38-void AvailableCoins(const CWallet& wallet, std::vector<COutput>& vCoins, const CCoinControl* coinControl = nullptr, const CAmount& nMinimumAmount = 1, const CAmount& nMaximumAmount = MAX_MONEY, const CAmount& nMinimumSumAmount = MAX_MONEY, const uint64_t nMaximumCount = 0) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
39+void AvailableCoins(const CWallet& wallet, std::vector<COutput>& vCoins, const CCoinControl* coinControl = nullptr, const CAmount& nMinimumAmount = 1, std::optional<CFeeRate> feerate = std::nullopt, const CAmount& nMaximumAmount = MAX_MONEY, const CAmount& nMinimumSumAmount = MAX_MONEY, const uint64_t nMaximumCount = 0) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);

achow101 commented at 3:03 pm on May 8, 2022:

nit: I think it would be better to put feerate after coinControl to maintain the grouping of similar kinds of parameters.

ishaanam commented at 1:35 pm on May 9, 2022:

Yeah, I’ve changed the order.

in src/wallet/coinselection.h:88 in 7da2200c91 outdated

86+            fee = fees;
87+            effective_value = txout.nValue - fee.value();
88+        }
89+    }
90+
91+    COutput(const COutPoint& outpoint, const CTxOut& txout, int depth, int input_bytes, bool spendable, bool solvable, bool safe, int64_t time, bool from_me, const CFeeRate feerate)

achow101 commented at 3:09 pm on May 8, 2022:

I think it would be better to have feerate be a std::optional<CFeeRate with a default of std::nullopt. This would allow us to do some simplification in AvailableCoins where the CFeeRate can just be passed in without checking whether it has a value.

ishaanam commented at 1:38 pm on May 9, 2022:

I agree, it looks a lot cleaner now.

ishaanam force-pushed on May 9, 2022

ishaanam marked this as ready for review on May 9, 2022

in src/wallet/coinselection.h:86 in b07500f28c outdated

84+            fee = input_bytes < 0 ? 0 : feerate.value().GetFee(input_bytes);
85+            effective_value = txout.nValue - fee.value();
86+        }
87+    }
88+
89+    COutput(const COutPoint& outpoint, const CTxOut& txout, int depth, int input_bytes, bool spendable, bool solvable, bool safe, int64_t time, bool from_me, const CAmount fees)

Xekyo commented at 3:21 pm on May 9, 2022:

Naming-nit: Sorry to disagree with previous review, but I think that we actually always use fee everywhere, so the plural here feels a bit odd. I notice that the parameter name would clash with the class member name, since they’d then both would be named fee.

Perhaps we should use this PR as an opportunity to rename the class members of COutput to use the m_ prefix as suggested by the Style Guide? This would allow us to use fee for the parameter as the member variable would then be COutput.m_fee.

laanwj commented at 12:05 pm on May 12, 2022:

Perhaps we should use this PR as an opportunity to rename the class members of COutput to use the m_ prefix as suggested by the Style Guide?

Dunno. FWIW m_ meant is for fully-fledged, self contained classes, not ‘dumb structs’ which simply keep some data together (like COutput seems to be). The rationale for it is that within methods it’s easier to distinguish local variables from class variables. Classes with no methods beyond construction/comparison don’t need that and only adds more verbosity.

Xekyo commented at 5:30 pm on May 12, 2022:

Okay, thanks for the guidance

jonatack commented at 5:36 pm on May 12, 2022:

FWIW m_ meant is for fully-fledged, self contained classes, not ‘dumb structs’ which simply keep some data together (like COutput seems to be). The rationale for it is that within methods it’s easier to distinguish local variables from class variables. Classes with no methods beyond construction/comparison don’t need that and only adds more verbosity.

I wonder if there would be rough consensus on the following (I wouldn’t be against)

 0--- a/doc/developer-notes.md
 1+++ b/doc/developer-notes.md
 2@@ -88,8 +88,10 @@ required when doing so would need changes to significant pieces of existing
 3 code.
 4   - Variable (including function arguments) and namespace names are all lowercase and may use `_` to
 5     separate words (snake_case).
 6-    - Class member variables have a `m_` prefix.
 7-    - Global variables have a `g_` prefix.
 8+  - Class member variables have an `m_` prefix in cases where it makes it easier
 9+    to distinguish them from local variables in the class (otherwise, it isn't
10+    needed and only adds more verbosity).
11+  - Global variables have a `g_` prefix.
12   - Constant names are all uppercase, and use `_` to separate words.
13   - Enumerator constants may be `snake_case`, `PascalCase` or `ALL_CAPS`.

in src/wallet/rpc/spend.cpp:1401 in b07500f28c outdated

1398@@ -1399,7 +1399,7 @@ RPCHelpMan sendall()
1399                     total_input_value += tx->tx->vout[input.prevout.n].nValue;
1400                 }
1401             } else {
1402-                AvailableCoins(*pwallet, all_the_utxos, &coin_control, /*nMinimumAmount=*/0);
1403+                AvailableCoins(*pwallet, all_the_utxos, &coin_control, fee_rate, /*nMinimumAmount=*/0);

Xekyo commented at 3:24 pm on May 9, 2022:

Nit: fee_rate here, but feerate in all the other places I see here. Should this perhaps be made consistent?

in src/wallet/test/coinselector_tests.cpp:903 in b07500f28c outdated

843+    COutput output4(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/ 1, input_bytes, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ false, fees);
844+    BOOST_CHECK_EQUAL(output4.GetEffectiveValue(), expected_ev1);
845+
846+    // input bytes unknown (input_bytes = -1), pass fees in constructor
847+    COutput output5(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/ 1, /*input_bytes=*/ -1, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ false, /*fees=*/ 0);
848+    BOOST_CHECK_EQUAL(output5.GetEffectiveValue(), nValue); // The effective value should be equal to the absolute value if input_bytes is -1

Xekyo commented at 3:36 pm on May 9, 2022:

Would these additional testcases perhaps be interesting?: What happens if a non-zero fee is passed while input_bytes is -1? What happens if a fee greater than nValue is passed while input_bytes is -1?

ishaanam commented at 5:49 pm on May 9, 2022:

I think that in both of these cases the assertion on src/wallet/coinselection.h:98 will just fail. fee must be 0 whenever input_bytes has a negative value.

Xekyo commented at 10:08 pm on May 11, 2022:

Ah good to see that you’re stating your assumptions. :)

Xekyo commented at 3:37 pm on May 9, 2022: member

This looks like it’s almost ready to go. I have a couple nits about names, that we might need to get some advice from more experienced contributors on, and a couple more potential test cases that I’m not sure what the outcome would be for.

in src/wallet/coinselection.cpp:378 in b07500f28c outdated

373@@ -380,8 +374,8 @@ CAmount GetSelectionWaste(const std::set<COutput>& inputs, CAmount change_cost,
374     CAmount waste = 0;
375     CAmount selected_effective_value = 0;
376     for (const COutput& coin : inputs) {
377-        waste += coin.fee - coin.long_term_fee;
378-        selected_effective_value += use_effective_value ? coin.effective_value : coin.txout.nValue;
379+        waste += coin.GetFee() - coin.long_term_fee;
380+        selected_effective_value += use_effective_value ? coin.GetEffectiveValue(): coin.txout.nValue;

Xekyo commented at 6:02 pm on May 12, 2022:

style nit:

0        selected_effective_value += use_effective_value ? coin.GetEffectiveValue() : coin.txout.nValue;

in src/wallet/coinselection.h:24 in b07500f28c outdated

19@@ -20,6 +20,14 @@ static constexpr CAmount CHANGE_UPPER{1000000};
20 
21 /** A UTXO under consideration for use in funding a new transaction. */
22 struct COutput {
23+private:
24+    /** The output's value minus fees required to spend it. Initialized as the output's absolute value. */

Xekyo commented at 6:04 pm on May 12, 2022:

This comment seems to be outdated. FWIU, we should be initializing this with the correct value now.

ishaanam commented at 10:01 pm on May 12, 2022:

It is outdated, I forgot to update this comment.

in src/wallet/coinselection.h:90 in b07500f28c outdated

 96+          safe{safe},
 97+          time{time},
 98+          from_me{from_me}
 99+    {
100+        // if input_bytes unknown, the fees should be 0, if input_bytes is known, then the fees should be a positive integer
101+        assert((input_bytes < 0 && fees == 0) || (input_bytes > 0 && fees >= 0));

Xekyo commented at 6:06 pm on May 12, 2022:

Optional:

if input_bytes is known, then the fees should be a positive integer

I see that some of the tests for this constructor pass fees = 0. What would need to happen for fees to be zero when input_bytes is greater than zero? IIRC, when SFFO is being used, we use the actual value of the UTXO instead of its effective value, so there too, a fee greater than zero should follow if both feerate and input_bytes are known. So, I wonder here whethere fees = 0 only occurs in the context of tests, or whether this occurs perhaps also in the context of listing unspents outside of transaction building. If so, it could perhaps be mentioned in the comment here.

ishaanam commented at 10:14 pm on May 12, 2022:

Yes, fees = 0 only happens in the context of tests so far. I will mention this here.

Xekyo commented at 6:24 pm on May 12, 2022: member

Approach ACK b07500f28cff352239a6ffbf1da9f4d2bc965838

I noticed that the lines in the commit message are pretty long, please break lines in the body of the commit message after 72 characters. It would be great if the comment could get updated, but I don’t think any of my remarks are blockers.

ishaanam force-pushed on May 13, 2022

Xekyo commented at 1:54 pm on May 13, 2022: member

ACK f5a7495c546c13c985bd92a030af092b17c67108

in src/wallet/spend.cpp:214 in f5a7495c54 outdated

210@@ -211,13 +211,19 @@ void AvailableCoins(const CWallet& wallet, std::vector<COutput>& vCoins, const C
211     }
212 }
213 
214+// Allows the 'feerate' optional parameter of AvailableCoins to be skipped when utxos are not being spent

glozow commented at 7:17 pm on May 15, 2022:

This comment would fit better above the AvailableCoinsListUnspent declaration in wallet/spend.h so someone looking to understand how to use the interface at a glance can see it. Also, consider making it doxygen-compatible.

ishaanam commented at 0:18 am on May 16, 2022:

done, I also clarified the comment a bit as well

glozow removed the label RPC/REST/ZMQ on May 15, 2022

ishaanam force-pushed on May 16, 2022

ishaanam commented at 0:39 am on May 16, 2022: member

At this point, I’ve addressed all of the suggestions provided by @achow101 @glozow and @Xekyo (thank you!) except for the fee_rate nit which I’m still unsure whether to act upon or not.

Xekyo commented at 1:40 pm on May 16, 2022: member

At this point, I’ve addressed all of the suggestions provided by @achow101 @glozow and @Xekyo (thank you!) except for the fee_rate nit which I’m still unsure whether to act upon or not.

Unless more people start asking for the fee_rate change, don’t worry about that. :smile:

in src/wallet/coinselection.h:95 in 1f1a75f3d2 outdated

93+          input_bytes{input_bytes},
94+          spendable{spendable},
95+          solvable{solvable},
96+          safe{safe},
97+          time{time},
98+          from_me{from_me}

achow101 commented at 3:12 pm on May 16, 2022:

nit: This could be condensed by using constructor delegation

0        : COutput(outpoint, txout, depth, input_bytes, spendable, solvable, safe, time, from_me)

ishaanam commented at 9:44 pm on May 20, 2022:

I will do this the next time I modify this PR

ishaanam commented at 4:00 am on May 21, 2022:

Done

achow101 commented at 3:13 pm on May 16, 2022: member

ACK 1f1a75f3d2277f05549395f80b5e1c2d932a7d3f

Xekyo commented at 3:51 pm on May 16, 2022: member

re-ACK 1f1a75f3d2277f05549395f80b5e1c2d932a7d3f

in src/wallet/spend.h:41 in 1f1a75f3d2 outdated

34@@ -35,7 +35,13 @@ TxSize CalculateMaximumSignedTxSize(const CTransaction& tx, const CWallet* walle
35 /**
36  * populate vCoins with vector of available COutputs.
37  */
38-void AvailableCoins(const CWallet& wallet, std::vector<COutput>& vCoins, const CCoinControl* coinControl = nullptr, const CAmount& nMinimumAmount = 1, const CAmount& nMaximumAmount = MAX_MONEY, const CAmount& nMinimumSumAmount = MAX_MONEY, const uint64_t nMaximumCount = 0) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
39+void AvailableCoins(const CWallet& wallet, std::vector<COutput>& vCoins, const CCoinControl* coinControl = nullptr, std::optional<CFeeRate> feerate = std::nullopt, const CAmount& nMinimumAmount = 1, const CAmount& nMaximumAmount = MAX_MONEY, const CAmount& nMinimumSumAmount = MAX_MONEY, const uint64_t nMaximumCount = 0) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
40+
41+/**
42+ * Runs AvailableCoins but allows the `feerate` optional parameter of

glozow commented at 11:15 pm on May 20, 2022:

nit

0 * Wrapper function for AvailableCoins which skips the `feerate` parameter. Use this function
1 * to list all available coins (e.g. listunspent RPC) while not intending to fund a transaction.

ishaanam commented at 3:59 am on May 21, 2022:

Done, this sounds much better

glozow commented at 11:24 pm on May 20, 2022: member

almost ACK, needs a rebase, 1 nit for your consideration

ishaanam force-pushed on May 21, 2022

ishaanam commented at 4:08 am on May 21, 2022: member

Implemented changes suggested by @glozow and @achow101 and rebased due to silent merge conflict

in src/wallet/test/coinselector_tests.cpp:368 in 5bd3b71304 outdated

376+        LOCK(wallet->cs_wallet);
377+        wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
378+        wallet->SetupDescriptorScriptPubKeyMans();
379+
380+        std::vector<COutput> coins;
381+

achow101 commented at 3:06 pm on May 21, 2022:

This setup does not need to be repeated for each of these cases. You can just do coins.clear() prior to calling add_coin again.

ishaanam commented at 3:53 pm on May 21, 2022:

Makes sense, I’ve changed it

Set effective_value when initializing a COutput

Previously in COutput, effective_value was initialized as the absolute
value of the txout, and fee as 0. effective_value along with fee were
calculated outside of the COutput constructor and set after the
object had been initialized. These changes will allow either the fee
or the feerate to be passed in a COutput constructor. If either are
provided, fee and effective_value are calculated and set in the
constructor. As a result, AvailableCoins also needs to be passed the
feerate when utxos are being spent. When balance is calculated or the
coins are being listed and feerate is neither available nor required,
AvailableCoinsListUnspent is used instead, which runs AvailableCoins
while providing the default value for feerate. Unit tests for the
calculation of effective value have also been added.

6fbb0edac2

ishaanam force-pushed on May 21, 2022