Set effective_value when initializing a COutput #25083

ishaanam · 2022-05-07T22:00:44Z

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 · 2022-05-08T06:24:08Z

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.

src/wallet/spend.h

src/wallet/spend.cpp

src/bench/coin_selection.cpp

src/wallet/spend.h

src/wallet/coinselection.h

murchandamus

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.

murchandamus · 2022-05-09T15:21:40Z

src/wallet/coinselection.h

+        }
+    }
+
+    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)


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.

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.

Okay, thanks for the guidance

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)

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

murchandamus · 2022-05-09T15:24:41Z

src/wallet/rpc/spend.cpp

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


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

src/wallet/test/coinselector_tests.cpp

murchandamus

Approach ACK b07500f

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.

src/wallet/coinselection.cpp

src/wallet/coinselection.h

murchandamus · 2022-05-13T13:54:06Z

ACK f5a7495

src/wallet/spend.cpp

ishaanam · 2022-05-16T00:39:39Z

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.

murchandamus · 2022-05-16T13:40:50Z

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

achow101

ACK 1f1a75f

src/wallet/coinselection.h

murchandamus · 2022-05-16T15:51:41Z

re-ACK 1f1a75f

glozow

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

src/wallet/spend.h

ishaanam · 2022-05-21T04:08:41Z

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

src/wallet/test/coinselector_tests.cpp

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.

achow101 · 2022-05-21T17:15:30Z

re-ACK 6fbb0ed

w0xlt

Code Review ACK 6fbb0ed

furszy

Looks good, have been touching this area lately, code review ACK 6fbb0ed.

murchandamus · 2022-05-23T16:45:26Z

re-ACK 6fbb0ed

maflcko · 2022-05-25T06:16:23Z

Looks like this crashes

https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=47613

1021e4c

…ssing fee rate. c97e961 fuzz: coinselection, add missing fee rate. (furszy) Pull request description: Fixing bitcoin/bitcoin#25083 (comment). Without the fee rate, 'GroupOutputs' will crash at group insertion time `OutputGroup::Insert` because now `output.GetEffectiveValue()` asserts that the value exists. ACKs for top commit: achow101: ACK c97e961 ishaanam: ACK c97e961 Xekyo: ACK c97e961 brunoerg: ACK c97e961 Tree-SHA512: f495886a5078842103e0f05a9018d7cb491967d8362f893dd7b21132b98e94321df860c50acb69e84d7232779f5915322ca6b5f99908e05e0480012580ee9d5d

DrahtBot added RPC/REST/ZMQ Wallet labels May 7, 2022

This was referenced May 8, 2022

wallet: remove extra wtx lookup in 'AvailableCoins' + several code cleanups. #25005

Merged

wallet: return error msg for "too-long-mempool-chain" #24845

Merged

refactor: Prepare for moving ArgsManager out of util/system #24811

Closed

achow101 reviewed May 8, 2022

View reviewed changes

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

This was referenced May 8, 2022

wallet: increase BnB upper limit #24752

Closed

wallet: Improve AvailableCoins performance by reducing duplicated operations #24699

Merged

ishaanam force-pushed the draft-CalculateEffectiveValue2 branch from b1adf9f to 7da2200 Compare May 8, 2022 14:21

achow101 reviewed May 8, 2022

View reviewed changes

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

achow101 reviewed May 8, 2022

View reviewed changes

src/bench/coin_selection.cpp Outdated Show resolved Hide resolved

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

achow101 reviewed May 8, 2022

View reviewed changes

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

DrahtBot mentioned this pull request May 8, 2022

wallet: avoid mixing different OutputTypes during coin selection #24584

Merged

ishaanam force-pushed the draft-CalculateEffectiveValue2 branch from 7da2200 to b07500f Compare May 9, 2022 02:12

ishaanam marked this pull request as ready for review May 9, 2022 02:24

murchandamus reviewed May 9, 2022

View reviewed changes

murchandamus reviewed May 12, 2022

View reviewed changes

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

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

src/wallet/coinselection.h Show resolved Hide resolved

ishaanam force-pushed the draft-CalculateEffectiveValue2 branch from b07500f to f5a7495 Compare May 13, 2022 00:26

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

glozow reviewed May 15, 2022

View reviewed changes

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

ishaanam force-pushed the draft-CalculateEffectiveValue2 branch from f5a7495 to 1f1a75f Compare May 16, 2022 00:10

achow101 reviewed May 16, 2022

View reviewed changes

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

glozow reviewed May 20, 2022

View reviewed changes

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

ishaanam force-pushed the draft-CalculateEffectiveValue2 branch from 1f1a75f to 5bd3b71 Compare May 21, 2022 03:53

achow101 reviewed May 21, 2022

View reviewed changes

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

ishaanam force-pushed the draft-CalculateEffectiveValue2 branch from 5bd3b71 to 6fbb0ed Compare May 21, 2022 15:29

w0xlt approved these changes May 23, 2022

View reviewed changes

furszy approved these changes May 23, 2022

View reviewed changes

maflcko assigned achow101 May 23, 2022

achow101 merged commit 3368f84 into bitcoin:master May 23, 2022

furszy mentioned this pull request May 25, 2022

fuzz: fix crash at coinselection, add missing fee rate. #25213

Merged

ishaanam deleted the draft-CalculateEffectiveValue2 branch June 23, 2022 02:52

bitcoin locked and limited conversation to collaborators Jun 23, 2023

Set effective_value when initializing a COutput #25083

Set effective_value when initializing a COutput #25083

Uh oh!

Conversation

ishaanam commented May 7, 2022

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

murchandamus left a comment

Choose a reason for hiding this comment

Uh oh!

murchandamus May 9, 2022

Choose a reason for hiding this comment

Uh oh!

laanwj May 12, 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!

murchandamus May 12, 2022

Choose a reason for hiding this comment

Uh oh!

jonatack May 12, 2022

Choose a reason for hiding this comment

Uh oh!

murchandamus May 9, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

murchandamus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

murchandamus commented May 13, 2022

Uh oh!

Uh oh!

ishaanam commented May 16, 2022

Uh oh!

murchandamus commented May 16, 2022

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

murchandamus commented May 16, 2022

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ishaanam commented May 21, 2022

Uh oh!

Uh oh!

achow101 commented May 21, 2022

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

murchandamus commented May 23, 2022

Uh oh!

maflcko commented May 25, 2022

Uh oh!

Uh oh!

DrahtBot commented May 8, 2022 •

edited

Loading

laanwj May 12, 2022 •

edited

Loading