wallet: Consolidate CInputCoin and COutput #24091

achow101 commented at 10:30 PM on January 17, 2022: member

While working on coin selection code, it occurred to me that CInputCoin is really a subset of COutput and the conversion of a COutput to a CInputCoin does not appear to be all that useful. So this PR adds fields that are present in CInputCoin to COutput and replaces the usage of CInputCoin with COutput.

COutput is also moved to coinselection.h. As part of this move, the usage of CWalletTx is removed from COutput. It is instead replaced by storing a COutPoint and the CTxOut rather than the entire CWalletTx as coin selection does not really need the full CWalletTx. The CWalletTx was only used for figuring out whether the transaction containing the output was from the current wallet, and for the transaction's time. These are now parameters to COutput's constructor.

achow101 added the label Wallet on Jan 17, 2022

in src/wallet/spend.h:60 in 620bc80a58 outdated

  56 | +        : m_outpoint(COutPoint(wtx.GetHash(), iIn)),
  57 | +        m_txout(wtx.tx->vout.at(iIn)),
  58 | +        nDepth(nDepthIn),
  59 | +        fSpendable(fSpendableIn),
  60 | +        fSolvable(fSolvableIn),
  61 | +        fSafe(fSafeIn),

Sjors commented at 12:05 PM on January 18, 2022:

620bc80a5808bd814f6724a9d2838ec70a40116c: warning: field 'fSafe' will be initialized after field 'nInputBytes' [-Wreorder-ctor] (goes away the next commit, but still)

achow101 commented at 3:26 AM on January 19, 2022:

Fixed

in src/wallet/spend.h:52 in 620bc80a58 outdated

  46 | @@ -48,21 +47,34 @@ class COutput
  47 |       */
  48 |      bool fSafe;
  49 |  
  50 | -    COutput(const CWallet& wallet, const CWalletTx& wtx, int iIn, int nDepthIn, bool fSpendableIn, bool fSolvableIn, bool fSafeIn, bool use_max_sig_in = false)
  51 | +    int64_t m_time;
  52 | +
  53 | +    bool m_from_me;

Sjors commented at 12:43 PM on January 18, 2022:

620bc80a5808bd814f6724a9d2838ec70a40116c might be slightly easier to review if you introduce m_from_me in an separate (earlier) commit

achow101 commented at 3:26 AM on January 19, 2022:

Done

in src/wallet/coinselection.h:61 in 864b4edf13 outdated

 108 | @@ -109,6 +109,10 @@ class COutput
 109 |  
 110 |      bool m_from_me;
 111 |  
 112 | +    CAmount effective_value;

Sjors commented at 1:27 PM on January 18, 2022:

864b4edf130afe2b77142ca343022b3de6d3fec5: did you mean m_effective_value? (this changes in the next commit)

achow101 commented at 3:26 AM on January 19, 2022:

Done

in src/wallet/coinselection.h:62 in 864b4edf13 outdated

 108 | @@ -109,6 +109,10 @@ class COutput
 109 |  
 110 |      bool m_from_me;
 111 |  
 112 | +    CAmount effective_value;
 113 | +    CAmount m_fee{0};
 114 | +    CAmount m_long_term_fee{0};

Sjors commented at 1:27 PM on January 18, 2022:

864b4edf130afe2b77142ca343022b3de6d3fec5: this is not set and not used (in this commit)

achow101 commented at 3:26 AM on January 19, 2022:

It is used later, but I didn't want that commit to be too big.

Sjors commented at 1:22 PM on January 25, 2022:

06cf38fce16f327579b008ee8fb78fa851cc8fce: it compiles fine now. But ideally you would move some more code from d840d84f547e911ec88f098d1ece7ed86f100cbc into this commit to set and use them. Not sure if that's practical.

in src/wallet/coinselection.h:39 in 6d54822cbc outdated

  86 | @@ -87,7 +87,7 @@ class COutput
  87 |      int nDepth;
  88 |  
  89 |      /** Pre-computed estimated size of this output as a fully-signed input in a transaction. Can be -1 if it could not be calculated */
  90 | -    int nInputBytes;
  91 | +    int m_input_bytes;

Sjors commented at 1:30 PM on January 18, 2022:

6d54822cbcd466df7f819430e3be87e480e41e77: did you mean to do this in the previous commit (that would make this commit a bit smaller too)?

achow101 commented at 3:26 AM on January 19, 2022:

Done

Sjors commented at 1:34 PM on January 18, 2022: member

Concept ACK

Reviewed the first 3 commits up to 95338d3b4bf3be2492cc654029ebfb28bedda42d, got a bit confused after that, as I think some of 864b4ed accidentally landed in 6d54822.

While you're touching this, it would be helpful to document the COutput class (now in coinselection.h) and how it relates to CWalletTx and COutputEntry (which is in receive.h, but used in the listtransactions RPC). For example, I was somewhat confused to see that CWalletTx does not in fact contain a list of COutputs.

95338d3b4bf3be2492cc654029ebfb28bedda42d: do you want to move AvailableCoins, ListCoins, etc from spend.h to coinselection.h too? Those all use COutput. Or is that more involved?

achow101 commented at 3:27 AM on January 19, 2022: member

I've split up a few commits and introduced the variables added to COutput in more separate commits.

do you want to move AvailableCoins, ListCoins, etc from spend.h to coinselection.h too? Those all use COutput. Or is that more involved?

No. COutput is only being moved to avoid a circular dependency.

achow101 force-pushed on Jan 19, 2022

MarcoFalke commented at 2:22 PM on January 21, 2022: member

Concept ACK. I need this for some wallet stuff.

MarcoFalke added the label Refactoring on Jan 21, 2022

DrahtBot cross-referenced this on Jan 22, 2022 from issue wallet: BIP 326 sequence based anti-fee-snipe for taproot inputs by MarcoFalke

DrahtBot commented at 9:23 PM on January 22, 2022: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24644 (wallet: add tracepoints and algorithm information to coin selection by achow101)
#24494 (wallet: generate random change target for each tx for better privacy by glozow)
#24128 (wallet: BIP 326 sequence based anti-fee-snipe for taproot inputs by MarcoFalke)

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.

kiminuo commented at 11:33 PM on January 22, 2022: contributor

Just read commits up to 1f475858590a50a8a3f37a02df2c2d7b803f9834 (wallet: Remove CWalletTx from COutput's constructor) and so far I have not found any obvious issue. Nice PR.

promag commented at 9:36 AM on January 24, 2022: member

Light code review ACK 2278038d3f9c2488421cf7ed1131ac77a6f22380.

nit, drop COutput forward declaration in wallet.h

fanquake requested review from instagibbs on Jan 25, 2022

instagibbs commented at 6:06 AM on January 25, 2022: member

concept ACK

in src/wallet/spend.cpp:285 in 3a28d99d6e outdated

 281 | @@ -280,7 +282,7 @@ std::map<CTxDestination, std::vector<COutput>> ListCoins(const CWallet& wallet)
 282 |                  CTxDestination address;
 283 |                  if (ExtractDestination(FindNonChangeParentOutput(wallet, *wtx.tx, output.n).scriptPubKey, address)) {
 284 |                      result[address].emplace_back(
 285 | -                        wallet, wtx, output.n, depth, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ false, wtx.GetTxTime());
 286 | +                        wallet, wtx, output.n, depth, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ false, wtx.GetTxTime(), CachedTxIsFromMe(wallet, it->second, ISMINE_ALL));

Sjors commented at 12:49 PM on January 25, 2022:

3a28d99d6e145bea5ec93ee4fccbf7d0dfabbbaa : Why it->second rather than just wtx?

achow101 commented at 9:00 PM on March 16, 2022:

Done

in src/wallet/spend.h:55 in 1f47585859 outdated

  51 | @@ -52,9 +52,9 @@ class COutput
  52 |      /** Whether the transaction containing this output is sent from the owning wallet */
  53 |      bool m_from_me;
  54 |  
  55 | -    COutput(const CWallet& wallet, const CWalletTx& wtx, int iIn, int depth, int input_bytes, bool spendable, bool solvable, bool safe, int64_t time, bool from_me)

Sjors commented at 1:15 PM on January 25, 2022:

1f475858590a50a8a3f37a02df2c2d7b803f9834: it seems wallet was already unused at this point, so you could drop it in another commit. But at least the commit message should mention them both.

achow101 commented at 9:00 PM on March 16, 2022:

Done

in src/bench/coin_selection.cpp:85 in d840d84f54 outdated

  81 | @@ -82,9 +82,9 @@ static void add_coin(const CAmount& nValue, int nInput, std::vector<OutputGroup>
  82 |      CMutableTransaction tx;
  83 |      tx.vout.resize(nInput + 1);
  84 |      tx.vout[nInput].nValue = nValue;
  85 | -    CInputCoin coin(MakeTransactionRef(tx), nInput);
  86 | +    COutput output(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), 0, -1, true, true, true, 0, true);

Sjors commented at 1:25 PM on January 25, 2022:

d840d84f547e911ec88f098d1ece7ed86f100cbc: document booleans?

achow101 commented at 9:00 PM on March 16, 2022:

Done

Sjors commented at 1:26 PM on January 25, 2022: member

Reviewed everything except d840d84f547e911ec88f098d1ece7ed86f100cbc, which still gives me a bit of a headache, but will look at it later.

achow101 force-pushed on Jan 25, 2022

in src/wallet/spend.h:51 in 4fd154a2b4 outdated

  47 | @@ -48,15 +48,19 @@ class COutput
  48 |       */
  49 |      bool m_safe;
  50 |  
  51 | -    COutput(const CWallet& wallet, const CWalletTx& wtx, int iIn, int depth, bool spendable, bool solvable, bool safe, bool use_max_sig_in = false)
  52 | +    /** The time of the transaction containing this output */

instagibbs commented at 7:58 AM on January 26, 2022:

time first seen? time of block inclusion? Good time to just make explicit here

achow101 commented at 6:16 PM on March 16, 2022:

It's whatever the CWalletTx reports as its time. It's not entirely clear what that is, and depends on nTimeSmart being... smart.

achow101 commented at 8:57 PM on March 16, 2022:

Added a mention of nTimeSmart

in src/wallet/spend.cpp:198 in 4fd154a2b4 outdated

 194 | @@ -195,7 +195,7 @@ void AvailableCoins(const CWallet& wallet, std::vector<COutput>& vCoins, const C
 195 |              bool solvable = provider ? IsSolvable(*provider, wtx.tx->vout[i].scriptPubKey) : false;
 196 |              bool spendable = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (((mine & ISMINE_WATCH_ONLY) != ISMINE_NO) && (coinControl && coinControl->fAllowWatchOnly && solvable));
 197 |  
 198 | -            vCoins.push_back(COutput(wallet, wtx, i, nDepth, spendable, solvable, safeTx, (coinControl && coinControl->fAllowWatchOnly)));
 199 | +            vCoins.emplace_back(wallet, wtx, i, nDepth, spendable, solvable, safeTx, wtx.GetTxTime(), (coinControl && coinControl->fAllowWatchOnly));

instagibbs commented at 8:28 AM on January 26, 2022:

while we're here can we annotate use_max_sig_in arg?

achow101 commented at 6:18 PM on March 16, 2022:

That gets dropped in a later commit anyways.

achow101 commented at 8:57 PM on March 16, 2022:

Done anyways.

in src/wallet/spend.cpp:436 in 18a6e09f0a outdated

 429 | @@ -432,10 +430,10 @@ std::optional<SelectionResult> SelectCoins(const CWallet& wallet, const std::vec
 430 |      {
 431 |          for (const COutput& out : vCoins) {
 432 |              if (!out.m_spendable) continue;
 433 | -            /* Set depth, from_me, ancestors, and descendants to 0 or false as these don't matter for preset inputs as no actual selection is being done.
 434 | +            /* Set ancestors and descendants to 0 as these don't matter for preset inputs as no actual selection is being done.
 435 |               * positive_only is set to false because we want to include all preset inputs, even if they are dust.
 436 |               */
 437 | -            preset_inputs.Insert(out.GetInputCoin(), 0, false, 0, 0, false);
 438 | +            preset_inputs.Insert(out, 0, 0, false);

Sjors commented at 12:36 PM on January 28, 2022:

18a6e09 nit: document the remaining bools while touching this

achow101 commented at 8:57 PM on March 16, 2022:

Done

Sjors approved

Sjors commented at 12:39 PM on January 28, 2022: member

utACK 3f5ed1858eba99253153f6b692625af075107f60

instagibbs approved

instagibbs commented at 6:00 AM on January 31, 2022: member

ACK 3f5ed1858eba99253153f6b692625af075107f60

murchandamus commented at 10:28 PM on February 7, 2022: contributor

Concept ACK

in src/wallet/spend.h:30 in 757428cd11 outdated

  26 | @@ -27,16 +27,16 @@ class COutput
  27 |       * If > 0: the tx is on chain and has this many confirmations.
  28 |       * If = 0: the tx is waiting confirmation.
  29 |       * If < 0: a conflicting tx is on chain and has this many confirmations. */
  30 | -    int nDepth;
  31 | +    int m_depth;

ryanofsky commented at 4:17 AM on February 8, 2022:

In commit "scripted-diff: Rename COutput member variables" (757428cd11128fc6f298e283d1bd2e89eab49800)

Minor: IMO, it would be better to make this a struct instead of a class and not add the m_ prefixes. The rationale for m_ prefixes is to be able to easily distinguish local variables and state variables inside instance methods. For a data structure that doesn't have private state or instance methods that aren't trivial accessors, using m_ prefix just confuses whether this is intended to be a struct defining a shared a data representation, or a class that's supposed to manage its own internal state. It's nice to pick a lane and minimize cruft and noise.

achow101 commented at 8:58 PM on March 16, 2022:

Dropped the m_ prefix.

in src/wallet/coinselection.cpp:54 in 18a6e09f0a outdated

  49 | @@ -50,8 +50,8 @@ struct {
  50 |   * The Branch and Bound algorithm is described in detail in Murch's Master Thesis:
  51 |   * https://murch.one/wp-content/uploads/2016/11/erhardt2016coinselection.pdf
  52 |   *
  53 | - * @param const std::vector<CInputCoin>& utxo_pool The set of UTXOs that we are choosing from.
  54 | - *        These UTXOs will be sorted in descending order by effective value and the CInputCoins'
  55 | + * @param const std::vector<OutputGroup>& utxo_pool The set of UTXOs that we are choosing from.
  56 | + *        These UTXOs will be sorted in descending order by effective value and the OutputGroups'

kallewoof commented at 10:30 AM on February 8, 2022:

In 18a6e09f0a50736a8113b6e6d918d93aa19d5a22

Maybe The set of UTXO groups that we are choosing from / These UTXO groups will be sorted in[...] is more accurate now.

achow101 commented at 8:58 PM on March 16, 2022:

Done

in src/wallet/spend.cpp:473 in 18a6e09f0a outdated

 467 | @@ -470,27 +468,28 @@ std::optional<SelectionResult> SelectCoins(const CWallet& wallet, const std::vec
 468 |              input_bytes = CalculateMaximumSignedInputSize(txout, &coin_control.m_external_provider, /* use_max_sig */ true);
 469 |          }
 470 |  
 471 | -        CInputCoin coin(outpoint, txout, input_bytes);
 472 | -        if (coin.m_input_bytes == -1) {
 473 | +        /* Set some defaults for depth, spendable, solvable, safe, time, and from_me as these don't matter for preset inputs since no selection is being done. */
 474 | +        COutput output(outpoint, txout, /*depth=*/ 0, input_bytes, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ false);
 475 | +        if (output.m_input_bytes == -1) {

kallewoof commented at 10:50 AM on February 8, 2022:

In 18a6e09f0a50736a8113b6e6d918d93aa19d5a22

You're literally feeding it this value on the previous line now, so this is going to be a bit confusing in the future. I think moving this to check that CalculateMaximumSignedInputSize() does not return -1 on L468 seems correct.

achow101 commented at 8:58 PM on March 16, 2022:

Done

achow101 commented at 3:48 PM on March 17, 2022:

Actually we still need this check here because input_bytes could be set by either CalculateMaximumSignedInputSize or GetVirtualTransactionSize, depending on whether the input weight is provided (this may have been a hidden merge conflict though so not apparent in the diff).

in src/wallet/spend.cpp:492 in 18a6e09f0a outdated

 496 |  
 497 |      // remove preset inputs from vCoins so that Coin Selection doesn't pick them.
 498 |      for (std::vector<COutput>::iterator it = vCoins.begin(); it != vCoins.end() && coin_control.HasSelected();)
 499 |      {
 500 | -        if (setPresetCoins.count(it->GetInputCoin()))
 501 | +        if (setPresetCoins.count(it->m_outpoint))

kallewoof commented at 10:52 AM on February 8, 2022:

In 18a6e09f0a50736a8113b6e6d918d93aa19d5a22

Style/nit: It is perfectly possible to do s/setPresetCoins/preset_coins in this commit without changing diff count, as it touches all references to setPresetCoins.

achow101 commented at 8:58 PM on March 16, 2022:

Done

kallewoof commented at 10:59 AM on February 8, 2022: member

Concept/utACK 3f5ed1858eba99253153f6b692625af075107f60

bitcoin deleted a comment on Feb 8, 2022

in src/wallet/spend.h:54 in 4fd154a2b4 outdated

  47 | @@ -48,15 +48,19 @@ class COutput
  48 |       */
  49 |      bool m_safe;
  50 |  
  51 | -    COutput(const CWallet& wallet, const CWalletTx& wtx, int iIn, int depth, bool spendable, bool solvable, bool safe, bool use_max_sig_in = false)
  52 | +    /** The time of the transaction containing this output */
  53 | +    int64_t m_time;
  54 | +
  55 | +    COutput(const CWallet& wallet, const CWalletTx& wtx, int iIn, int depth, bool spendable, bool solvable, bool safe, int64_t time, bool use_max_sig_in = false)

ryanofsky commented at 7:41 PM on February 8, 2022:

In commit "wallet: Store tx time in COutput" (4fd154a2b40c1a9b0dca264fb2d07215d48bae63)

This change seems unsafe. It is not always obvious when COutput is being constructed (due to emplaces and brace initializers), so if a call site was passing use_max_sig = true as this ninth argument, now it will be interpreted as use_max_sig = false with time=1.

Simple suggestion to avoid this problem would be to require all arguments and drop = false default value. Even better would be to treat this a struct and access fields by name instead of a (int, int bool, bool, bool int, bool) parameter list.

EDIT: I see later commit drops use_max_sig argument altogether. Still it would be nice to get rid of the default value now, so it is easier to known that next few commits are not applying wrong parameter values.

achow101 commented at 8:58 PM on March 16, 2022:

I've added an intermediate commit removing the default value.

in src/wallet/spend.h:57 in 521a703d05 outdated

  50 | @@ -51,7 +51,10 @@ class COutput
  51 |      /** The time of the transaction containing this output */
  52 |      int64_t m_time;
  53 |  
  54 | -    COutput(const CWallet& wallet, const CWalletTx& wtx, int iIn, int depth, bool spendable, bool solvable, bool safe, int64_t time, bool use_max_sig_in = false)
  55 | +    /** Whether the transaction containing this output is sent from the owning wallet */
  56 | +    bool m_from_me;
  57 | +
  58 | +    COutput(const CWallet& wallet, const CWalletTx& wtx, int iIn, int depth, bool spendable, bool solvable, bool safe, int64_t time, bool from_me, bool use_max_sig_in = false)

ryanofsky commented at 8:12 PM on February 8, 2022:

In commit "wallet: Store whether a COutput is from the wallet" (521a703d056c13d85dbaabd289d2cc7edfd671c0)

Again use_max_sig_in = false default argument makes this hard to verify and means it could silently break. This again suggests dropping default argument in earlier commit.

in src/wallet/spend.cpp:156 in 521a703d05 outdated

 161 | @@ -162,6 +162,8 @@ void AvailableCoins(const CWallet& wallet, std::vector<COutput>& vCoins, const C
 162 |              continue;
 163 |          }
 164 |  
 165 | +        bool tx_from_me = CachedTxIsFromMe(wallet, wtx, ISMINE_ALL);

ryanofsky commented at 8:15 PM on February 8, 2022:

In commit "wallet: Store whether a COutput is from the wallet" (521a703d056c13d85dbaabd289d2cc7edfd671c0)

This seems ok, but just reading commit description and looking at this code this doesn't seem to be an improvement. Two CachedTxIsFromMe calls are added and two are dropped which is neutral, but COutput parameter list is growing even more, which seems bad.

I'm assuming something justifies this in a later commit, but it would be good to hint at whatever this might be in commit description,

EDIT: After reviewing PR, still not exactly clear what benefit of this change is. But seems basically neutral.

achow101 commented at 7:49 PM on March 16, 2022:

The purpose is that when filtering UTXOs, we need to know whether they belong to the wallet. Since we will be removing access to CWallet and CWalletTx in the future, this information must be stored in COutput itself.

in src/wallet/spend.h:54 in 353d7bf8e7 outdated

  50 | @@ -54,24 +51,17 @@ class COutput
  51 |      /** Whether the transaction containing this output is sent from the owning wallet */
  52 |      bool m_from_me;
  53 |  
  54 | -    COutput(const CWallet& wallet, const CWalletTx& wtx, int iIn, int depth, bool spendable, bool solvable, bool safe, int64_t time, bool from_me, bool use_max_sig_in = false)
  55 | +    COutput(const CWallet& wallet, const CWalletTx& wtx, int iIn, int depth, int input_bytes, bool spendable, bool solvable, bool safe, int64_t time, bool from_me)

ryanofsky commented at 8:18 PM on February 8, 2022:

In commit "wallet: Provide input bytes to COutput" (353d7bf8e7c800aa19cbcc52fe0636667c754cec)

Nice to drop the use_max_sig argument here, but this should definitely drop the CWallet argument too, which is no longer needed here

EDIT: I see it is dropped in later commit

ryanofsky approved

ryanofsky commented at 8:43 PM on February 8, 2022: contributor

Code review ACK 3f5ed1858eba99253153f6b692625af075107f60. All my suggestions are minor, and you can ignore them. This is a nice simplification and the changes are generally pretty easy to follow. Main feedback would be to drop the struct m_ prefixes (Ugly!)

DrahtBot cross-referenced this on Mar 8, 2022 from issue wallet: generate random change target for each tx for better privacy by glozow

achow101 force-pushed on Mar 16, 2022

achow101 requested review from ryanofsky on Mar 16, 2022

achow101 requested review from instagibbs on Mar 16, 2022

achow101 requested review from Sjors on Mar 16, 2022

achow101 requested review from kallewoof on Mar 16, 2022

achow101 requested review from promag on Mar 16, 2022

Sjors commented at 1:27 PM on March 17, 2022: member

CI seems a bit unhappy

wallet: cleanup COutput constructor c7c64db41e

scripted-diff: Rename COutput member variables

Update the member variables to match the new style

-BEGIN VERIFY SCRIPT-
sed -i 's/fSpendableIn/spendable/' $(git grep -l "fSpendableIn")
sed -i 's/fSpendable/spendable/' $(git grep -l "fSpendable")
sed -i 's/fSolvableIn/solvable/' $(git grep -l "fSolvableIn")
sed -i 's/fSolvable/solvable/' $(git grep -l "fSolvable")
sed -i 's/fSafeIn/safe/' $(git grep -l "fSafeIn")
sed -i 's/fSafe/safe/' $(git grep -l "fSafe")
sed -i 's/nInputBytes/input_bytes/' $(git grep -l "nInputBytes")
sed -i 's/nDepthIn/depth/' $(git grep -l "nDepthIn" src/wallet src/bench)
sed -i 's/nDepth/depth/' src/wallet/spend.h
sed -i 's/\.nDepth/.depth/' $(git grep -l "\.nDepth" src/wallet/)
sed -i 's/nDepth, FormatMoney/depth, FormatMoney/' src/wallet/spend.cpp
-END VERIFY SCRIPT-

10379f007f

wallet: Remove use_max_sig default value

As we change the constructor for COutput, it becomes somewhat dangerous
if there are default values.

46022953ee

wallet: Store tx time in COutput b799814bbd

wallet: Store whether a COutput is from the wallet

Instead of determining whether the containing transaction is from the
wallet dynamically as needed, just pass it in to COutput and store it.
The transaction ownership isn't going to change.

d51f27d3bb

achow101 force-pushed on Mar 17, 2022

in src/wallet/spend.h:41 in 03e54175a0 outdated

  37 | @@ -38,9 +38,6 @@ class COutput
  38 |      /** Whether we know how to spend this output, ignoring the lack of keys */
  39 |      bool solvable;
  40 |  
  41 | -    /** Whether to use the maximum sized, 72 byte signature when calculating the size of the input spend. This should only be set when watch-only outputs are allowed */

ryanofsky commented at 6:22 PM on March 17, 2022:

In commit "wallet: Provide input bytes to COutput" (03e54175a04359c6639773a02d7e57fbdd18c400)

Minor: This seems like a useful comment describing how use_max_sig is supposed to be set. Could consider moving it into the GetTxSpendSize comment in spend.h

achow101 commented at 6:55 PM on March 23, 2022:

Moved the comment.

in src/wallet/coinselection.h:67 in 03245baac1 outdated

 109 | @@ -110,6 +110,10 @@ class COutput
 110 |      /** Whether the transaction containing this output is sent from the owning wallet */
 111 |      bool from_me;
 112 |  
 113 | +    CAmount effective_value;
 114 | +    CAmount fee{0};
 115 | +    CAmount long_term_fee{0};

ryanofsky commented at 6:52 PM on March 17, 2022:

In commit "coinselection: Add effective value and fees to COutput" (03245baac1b8a9bef2cad09942cdd6a618e3b7ef)

Note (just for understanding): These new members come from the existing CInputCoin struct and are added here unchanged so COutput can replace CInputCoin later

glozow commented at 3:15 PM on March 21, 2022:

in 03245baac1b8a9bef2cad09942cdd6a618e3b7ef

Please add doxygen comments for the new members?

achow101 commented at 7:01 PM on March 23, 2022:

Done

in src/wallet/coinselection.cpp:54 in f2cf89eefe outdated

  49 | @@ -50,8 +50,8 @@ struct {
  50 |   * The Branch and Bound algorithm is described in detail in Murch's Master Thesis:
  51 |   * https://murch.one/wp-content/uploads/2016/11/erhardt2016coinselection.pdf
  52 |   *
  53 | - * @param const std::vector<CInputCoin>& utxo_pool The set of UTXOs that we are choosing from.
  54 | - *        These UTXOs will be sorted in descending order by effective value and the CInputCoins'
  55 | + * @param const std::vector<OutputGroup>& utxo_pool The set of UTXO groups that we are choosing from.
  56 | + *        These UTXO groups will be sorted in descending order by effective value and the OutputGroups'

ryanofsky commented at 7:18 PM on March 17, 2022:

In commit "coinselection: Use COutput instead of CInputCoin" (f2cf89eefefec98aa6f5f7a8d3f6f0abc0e0d526)

Note: parameter isn't actually changing here, obsolete comment is just being brought up to date with current code. (At first I thought this change was more significant than it really is)

in src/wallet/coinselection.h:147 in f2cf89eefe outdated

 142 | +        return outpoint != rhs.outpoint;
 143 | +    }
 144 | +
 145 | +    bool operator==(const COutput& rhs) const {
 146 | +        return outpoint == rhs.outpoint;
 147 | +    }

ryanofsky commented at 7:28 PM on March 17, 2022:

In commit "coinselection: Use COutput instead of CInputCoin" (f2cf89eefefec98aa6f5f7a8d3f6f0abc0e0d526)

Minor: operator< above makes sense I think, but these operator== operator!= methods are only used by tests and don't really describe full equality. I'd suggest dropping these == != methods and just passing a lambda to std::mismatch in the test so it can explicitly specify the equality it checks for.

glozow commented at 4:58 PM on March 21, 2022:

+1 to helpers in the tests instead of these operators

achow101 commented at 6:55 PM on March 23, 2022:

Done in an additional commit.

ryanofsky approved

ryanofsky commented at 7:31 PM on March 17, 2022: contributor

Code review ACK 1a3e984a09343ac1d804f6106a52c9cf527f7dba

Nice simplification and easy review!

Sjors approved

Sjors commented at 9:37 AM on March 18, 2022: member

re-utACK 1a3e984a09343ac1d804f6106a52c9cf527f7dba

in src/wallet/spend.h:54 in 10379f007f outdated

  55 |          i(iIn),
  56 | -        nDepth(nDepthIn),
  57 | -        nInputBytes(-1),
  58 | -        fSpendable(fSpendableIn),
  59 | -        fSolvable(fSolvableIn),
  60 | +        depth(depth),

glozow commented at 2:47 PM on March 21, 2022:

style nit in 10379f007fd2c18f4cd24d0a0783d6d929f45556:

prefix member variables with m_? That also distinguishes these variable names.

achow101 commented at 5:04 PM on March 21, 2022:

#24091 (review)

glozow commented at 5:05 PM on March 21, 2022:

Ah I missed that, thanks :+1:

in src/wallet/spend.h:59 in c7c64db41e outdated

  54 | +        nDepth(nDepthIn),
  55 | +        nInputBytes(-1),
  56 | +        fSpendable(fSpendableIn),
  57 | +        fSolvable(fSolvableIn),
  58 | +        use_max_sig(use_max_sig_in),
  59 | +        fSafe(fSafeIn)

glozow commented at 2:48 PM on March 21, 2022:

in c7c64db41e1718584aa2f30ff27f60ab0966de62

bracket initialization?

achow101 commented at 6:48 PM on March 23, 2022:

I would, but it causes conflicts with every other commit in this PR. The effort is not worth it.

in src/bench/coin_selection.cpp:61 in 03e54175a0 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(wallet, *wtx, /*iIn=*/ 0, /*depth=*/ 6 * 24, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx->GetTxTime(), /*from_me=*/ true, /*use_max_sig_in=*/ false);
  62 | +        coins.emplace_back(wallet, *wtx, /*iIn=*/ 0, /*depth=*/ 6 * 24, GetTxSpendSize(wallet, *wtx, 0), /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx->GetTxTime(), /*from_me=*/ true);

glozow commented at 3:02 PM on March 21, 2022:

nit in 03e54175a04359c6639773a02d7e57fbdd18c400

Should label these with variable name, since everything else is labeled

achow101 commented at 6:57 PM on March 23, 2022:

I would, but it causes conflicts with every other commit in this PR. The effort is not worth it.

in src/wallet/rpc/coins.cpp:660 in 875b34f21b outdated

 659 |  
 660 |          UniValue entry(UniValue::VOBJ);
 661 | -        entry.pushKV("txid", out.tx->GetHash().GetHex());
 662 | -        entry.pushKV("vout", out.i);
 663 | +        entry.pushKV("txid", out.outpoint.hash.GetHex());
 664 | +        entry.pushKV("vout", (int)out.outpoint.n);

glozow commented at 3:26 PM on March 21, 2022:

nit in 875b34f21be03f1308007e6d6212cd0a20e157e2:

Instead of C-style cast, bracket cast or just use out.i ?

murchandamus commented at 4:18 PM on March 23, 2022:

Isn't out.i gone after this commit?

achow101 commented at 6:48 PM on March 23, 2022:

out.i is removed by this PR.

glozow commented at 5:00 PM on March 21, 2022: member

light code review ACK 1a3e984a09343ac1d804f6106a52c9cf527f7dba, just a few nits

in src/bench/coin_selection.cpp:61 in 46022953ee 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(wallet, *wtx, 0 /* iIn */, 6 * 24 /* depth */, true /* spendable */, true /* solvable */, true /* safe */);
  62 | +        coins.emplace_back(wallet, *wtx, /*iIn=*/ 0, /*depth=*/ 6 * 24, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*use_max_sig_in=*/ false);

murchandamus commented at 2:15 PM on March 23, 2022:

In 46022953ee2e8113167bafd1fd48a383a578b13c I missed at first that you added explicit values for use_max_sig_in in a few places while removing the default value. The style amendment first made me think that the default value removal was the only code change. In the future, perhaps consider splitting style edits and functional code changes.

in src/wallet/interfaces.cpp:114 in b799814bbd outdated

 110 | @@ -111,6 +111,17 @@ WalletTxOut MakeWalletTxOut(const CWallet& wallet,
 111 |      return result;
 112 |  }
 113 |  
 114 | +WalletTxOut MakeWalletTxOut(const CWallet& wallet,

murchandamus commented at 2:44 PM on March 23, 2022:

In b799814bbd53736b79495072f3c9e05989a465e8: It isn't obvious to me why we needed a new constructor for WalletTxOut and how that relates to adding time to COutput. Could you perhaps add something to the commit message that elaborates the thought process here, or split it off if it's unrelated?

achow101 commented at 6:53 PM on March 23, 2022:

MakeWalletTxOut is used to create entries for the GUI. Part of that is the transaction's time, retrieved with CWalletTx::GetTxTime. This constructor is called for each COutput returned by ListCoins. As COutput will no longer have access to the CWalletTx later in this PR, it is necessary for it to be able to have access to the time so that WalletTxOut can have the correct time set. That is why this change exists, and it is, in fact, related to storing time in the COutput.

in src/wallet/spend.h:63 in b799814bbd outdated

  60 |          spendable(spendable),
  61 |          solvable(solvable),
  62 |          use_max_sig(use_max_sig_in),
  63 | -        safe(safe)
  64 | +        safe(safe),
  65 | +        time(time)

murchandamus commented at 2:45 PM on March 23, 2022:

In b799814bbd53736b79495072f3c9e05989a465e8: We already have depth to determine the "age" of UTXOs. It's not obvious to me why we need time in COutput. It doesn't ever seem to get used in this PR. Could you please elaborate your motivation?

achow101 commented at 6:47 PM on March 23, 2022:

The GUI uses it, and it is used by this PR. I would not have made this change otherwise. See the change to src/wallet/interfaces.cpp

in src/wallet/spend.cpp:196 in d51f27d3bb outdated

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

murchandamus commented at 2:50 PM on March 23, 2022:

Nit d51f27d3bb0d6e3ca55bcd23ce53e4fe413a9360: inconsistent from_me and tx_from_me

achow101 commented at 6:56 PM on March 23, 2022:

I would, but it causes conflicts with most of the other commit in this PR. The effort is not worth it.

in src/wallet/spend.cpp:474 in f2cf89eefe outdated

 472 |          }
 473 | -        coin.effective_value = coin.txout.nValue - coin_selection_params.m_effective_feerate.GetFee(coin.m_input_bytes);
 474 | +
 475 | +        /* Set some defaults for depth, spendable, solvable, safe, time, and from_me as these don't matter for preset inputs since no selection is being done. */
 476 | +        COutput output(outpoint, txout, /*depth=*/ 0, input_bytes, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ false);
 477 | +        output.effective_value = output.txout.nValue - coin_selection_params.m_effective_feerate.GetFee(output.input_bytes);

murchandamus commented at 4:35 PM on March 23, 2022:

Probably for a follow-up, but I think

COutput could have a function that takes a feerate, and sets the COutput.effective_value, or perhaps we could even have COutput take a feerate in the constructor.

achow101 commented at 6:56 PM on March 23, 2022:

I'll leave that for a followup.

murchandamus commented at 4:46 PM on March 23, 2022: contributor

ACK, thanks for doing, I'm glad to see CInputCoin go away. Got some nits and questions.

w0xlt commented at 6:07 PM on March 23, 2022: contributor

Code Review ACK https://github.com/bitcoin/bitcoin/pull/24091/commits/1a3e984a09343ac1d804f6106a52c9cf527f7dba

wallet: Provide input bytes to COutput 0ba4d1916e

wallet: Replace CWalletTx in COutput with COutPoint and CTxOut

Instead of having a pointer to the CWalletTx in COutput, we can just
store the COutPoint and the CTxOut as those are the only things we need
from the CWalletTx. Other things CWalletTx used to provide were time and
fIsFromMe but these are also being stored by COutput.

14d04d5ad1

wallet: Remove CWallet and CWalletTx from COutput's constructor 42e974e15c

moveonly: move COutput to coinselection.h f0821230b8

achow101 force-pushed on Mar 23, 2022

coinselection: Add effective value and fees to COutput 14fbb57b79

coinselection: Use COutput instead of CInputCoin

Also rename setPresetCoins to preset_coins

70f31f1a81

coinselection: Remove CInputCoin

It is no longer needed as everything it was doing is now done by COutput

f6c39c6adb

coinselection: Remove COutput operators == and !=

These operators are used only by the tests in std::mismatch. As
std::mismatch can take a binary predicate, we can use a lambda that
achieves the same instead.

049003fe68

achow101 force-pushed on Mar 23, 2022

DrahtBot cross-referenced this on Mar 24, 2022 from issue wallet: add tracepoints and algorithm information to coin selection by achow101

ryanofsky approved

ryanofsky commented at 5:10 PM on March 24, 2022: contributor

Code review ACK 049003fe68a4183f6f20da16f58f10079d1e02df, just adding comments and removing == operators since last review

w0xlt approved

w0xlt commented at 5:57 PM on March 24, 2022: contributor

reACK 049003f

MarcoFalke requested review from Sjors on Mar 24, 2022

MarcoFalke requested review from glozow on Mar 24, 2022

MarcoFalke requested review from murchandamus on Mar 24, 2022

murchandamus approved

murchandamus commented at 6:44 PM on March 24, 2022: contributor

reACK 049003fe68a4183f6f20da16f58f10079d1e02df

fanquake merged this on Mar 24, 2022

fanquake closed this on Mar 24, 2022

MarcoFalke cross-referenced this on Mar 25, 2022 from issue refactor: Fix coinselection.h include, Make COutput a struct by MarcoFalke

fanquake referenced this in commit 6b1f93700c on Mar 25, 2022

mzumsande cross-referenced this on Mar 27, 2022 from issue fuzz: add target for coinselection algorithms by mzumsande

bitcoin locked this on Jun 9, 2023