wallet: Consolidate CInputCoin and COutput #24091

achow101 · 2022-01-17T22:30:14Z

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.

Sjors

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?

src/wallet/spend.h

src/wallet/coinselection.h

Sjors · 2022-01-18T13:27:44Z

src/wallet/coinselection.h

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

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

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.

src/wallet/coinselection.h

achow101 · 2022-01-19T03:27:49Z

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.

maflcko

Concept ACK. I need this for some wallet stuff.

DrahtBot · 2022-01-22T21:23:25Z

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 · 2022-01-22T23:33:57Z

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

Light code review ACK 2278038d3f9c2488421cf7ed1131ac77a6f22380.

nit, drop COutput forward declaration in wallet.h

instagibbs · 2022-01-25T06:06:03Z

concept ACK

Sjors

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

src/wallet/spend.cpp

src/wallet/spend.h

Sjors · 2022-01-25T13:22:35Z

src/wallet/coinselection.h

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.

src/bench/coin_selection.cpp

Sjors

utACK 3f5ed1858eba99253153f6b692625af075107f60

src/wallet/spend.cpp

instagibbs

ACK 3f5ed1858eba99253153f6b692625af075107f60

src/wallet/spend.h

src/wallet/spend.cpp

murchandamus · 2022-02-07T22:28:57Z

Concept ACK

kallewoof

Concept/utACK 3f5ed1858eba99253153f6b692625af075107f60

src/wallet/coinselection.cpp

src/wallet/spend.cpp

ryanofsky

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!)

src/wallet/spend.h

src/wallet/spend.cpp

src/wallet/spend.h

Sjors · 2022-03-17T13:27:45Z

CI seems a bit unhappy

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-

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.

ryanofsky

Code review ACK 1a3e984a09343ac1d804f6106a52c9cf527f7dba

Nice simplification and easy review!

src/wallet/spend.h

ryanofsky · 2022-03-17T18:52:05Z

src/wallet/coinselection.h

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

ryanofsky · 2022-03-17T19:18:29Z

src/wallet/coinselection.cpp

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)

src/wallet/coinselection.h

Sjors

re-utACK 1a3e984a09343ac1d804f6106a52c9cf527f7dba

glozow

light code review ACK 1a3e984a09343ac1d804f6106a52c9cf527f7dba, just a few nits

src/wallet/spend.h

src/bench/coin_selection.cpp

src/wallet/coinselection.h

src/wallet/rpc/coins.cpp

src/wallet/coinselection.h

murchandamus

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

murchandamus · 2022-03-23T14:15:09Z

src/bench/coin_selection.cpp

    std::vector<COutput> coins;
    for (const auto& wtx : wtxs) {
-        coins.emplace_back(wallet, *wtx, 0 /* iIn */, 6 * 24 /* depth */, true /* spendable */, true /* solvable */, true /* safe */);
+        coins.emplace_back(wallet, *wtx, /*iIn=*/ 0, /*depth=*/ 6 * 24, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*use_max_sig_in=*/ false);


In 4602295 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.

src/wallet/interfaces.cpp

src/wallet/spend.h

src/wallet/spend.cpp

src/wallet/rpc/coins.cpp

murchandamus · 2022-03-23T16:35:55Z

src/wallet/spend.cpp

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.

I'll leave that for a followup.

w0xlt

Code Review ACK 1a3e984

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.

Also rename setPresetCoins to preset_coins

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

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.

ryanofsky

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

w0xlt

reACK 049003f

murchandamus

reACK 049003f

…ake COutput a struct fab287c Clarify that COutput is a struct, not a class (MarcoFalke) fa61cdf wallet: Fix coinselection include (MarcoFalke) Pull request description: * Fix include (see commit message) * `{}`-init, see bitcoin/bitcoin#24091 (comment) * `struct`, see bitcoin/bitcoin#24091 (comment) ACKs for top commit: theStack: Code-review ACK fab287c Tree-SHA512: dd2cfb9c06a92295dbd8fbb6d56afcf00ebda2a0440e301d392cd183d1b9cd87626311d539e302a9e6c6521d69d6183c74a51934e3fc16e64a5dcaba60c7e3ce

achow101 added the Wallet label Jan 17, 2022

Sjors reviewed Jan 18, 2022

View reviewed changes

achow101 force-pushed the consolidate-coutput-cinputcoin branch from efa72a5 to 2278038 Compare January 19, 2022 16:58

maflcko reviewed Jan 21, 2022

View reviewed changes

maflcko added the Refactoring label Jan 21, 2022

DrahtBot mentioned this pull request Jan 22, 2022

wallet: BIP 326 sequence based anti-fee-snipe for taproot inputs #24128

Closed

promag reviewed Jan 24, 2022

View reviewed changes

fanquake requested a review from instagibbs January 25, 2022 03:00

Sjors reviewed Jan 25, 2022

View reviewed changes

achow101 force-pushed the consolidate-coutput-cinputcoin branch from 2278038 to 3f5ed18 Compare January 25, 2022 16:34

Sjors approved these changes Jan 28, 2022

View reviewed changes

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

instagibbs approved these changes Jan 31, 2022

View reviewed changes

src/wallet/spend.h Outdated Show resolved Hide resolved

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

kallewoof reviewed Feb 8, 2022

View reviewed changes

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

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

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

bitcoin deleted a comment from taveechaimekwan Feb 8, 2022

ryanofsky approved these changes Feb 8, 2022

View reviewed changes

src/wallet/spend.h Outdated Show resolved Hide resolved

src/wallet/spend.h Outdated Show resolved Hide resolved

src/wallet/spend.h Outdated Show resolved Hide resolved

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

src/wallet/spend.h Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Mar 8, 2022

wallet: generate random change target for each tx for better privacy #24494

Merged

achow101 force-pushed the consolidate-coutput-cinputcoin branch from 3f5ed18 to 98812f1 Compare March 16, 2022 20:57

achow101 requested review from Sjors, instagibbs, kallewoof, promag and ryanofsky March 16, 2022 21:00

achow101 added 2 commits March 17, 2022 10:49

wallet: cleanup COutput constructor

c7c64db

achow101 added 2 commits March 17, 2022 11:00

wallet: Store tx time in COutput

b799814

wallet: Store whether a COutput is from the wallet

d51f27d

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.

achow101 force-pushed the consolidate-coutput-cinputcoin branch from 98812f1 to 1a3e984 Compare March 17, 2022 15:55

ryanofsky approved these changes Mar 17, 2022

View reviewed changes

Sjors approved these changes Mar 18, 2022

View reviewed changes

glozow reviewed Mar 21, 2022

View reviewed changes

murchandamus reviewed Mar 23, 2022

View reviewed changes

w0xlt reviewed Mar 23, 2022

View reviewed changes

achow101 added 4 commits March 23, 2022 14:32

wallet: Provide input bytes to COutput

0ba4d19

wallet: Remove CWallet and CWalletTx from COutput's constructor

42e974e

moveonly: move COutput to coinselection.h

f082123

achow101 force-pushed the consolidate-coutput-cinputcoin branch from 1a3e984 to 6c05fa5 Compare March 23, 2022 18:55

achow101 added 4 commits March 23, 2022 15:01

coinselection: Add effective value and fees to COutput

14fbb57

coinselection: Use COutput instead of CInputCoin

70f31f1

Also rename setPresetCoins to preset_coins

coinselection: Remove CInputCoin

f6c39c6

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

coinselection: Remove COutput operators == and !=

049003f

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.

achow101 force-pushed the consolidate-coutput-cinputcoin branch from 6c05fa5 to 049003f Compare March 23, 2022 19:01

DrahtBot mentioned this pull request Mar 24, 2022

wallet: add tracepoints and algorithm information to coin selection #24644

Merged

ryanofsky approved these changes Mar 24, 2022

View reviewed changes

w0xlt approved these changes Mar 24, 2022

View reviewed changes

maflcko requested review from Sjors, glozow and murchandamus March 24, 2022 17:58

murchandamus approved these changes Mar 24, 2022

View reviewed changes

fanquake merged commit 3740cdd into bitcoin:master Mar 24, 2022

maflcko mentioned this pull request Mar 25, 2022

refactor: Fix coinselection.h include, Make COutput a struct #24666

Merged

mzumsande mentioned this pull request Mar 27, 2022

fuzz: add target for coinselection algorithms #24602

Merged

bitcoin locked and limited conversation to collaborators Jun 9, 2023

wallet: Consolidate CInputCoin and COutput #24091

wallet: Consolidate CInputCoin and COutput #24091

Uh oh!

Conversation

achow101 commented Jan 17, 2022

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!

Uh oh!

Uh oh!

Uh oh!

Sjors Jan 18, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 commented Jan 19, 2022

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jan 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

kiminuo commented Jan 22, 2022

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Jan 25, 2022

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

murchandamus commented Feb 7, 2022

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Sjors commented Mar 17, 2022

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Sjors left a comment •

edited

Loading

Sjors Jan 18, 2022 •

edited

Loading

DrahtBot commented Jan 22, 2022 •

edited

Loading

murchandamus left a comment •

edited

Loading