wallet, refactor: return out-params of CreateTransaction() as optional struct #20640

theStack commented at 4:03 AM on December 13, 2020: contributor

The method CWallet::CreateTransaction currently returns several values in the form of out-parameters:

the actual newly created transaction (CTransactionRef& tx)
its required fee (CAmount& nFeeRate)
the position of the change output (int& nChangePosInOut) -- as the name suggests, this is both an in- and out-param

By returning these values in an optional structure (which returns no value a.k.a. std::nullopt if an error occured), the interfaces is shorter, cleaner (requested change position is now in-param and can be passed by value) and callers don't have to create dummy variables for results that they are not interested in.

Note that the names of the replaced out-variables were kept in CreateTransactionInternal to keep the diff minimal. Also, the fee calculation data (FeeCalculation& fee_calc_out) would be another candidate to put into the structure, but FeeCalculation is currently an opaque data type in the wallet interface and I think it should stay that way.

As a potential follow-up, I think it would make sense to also do the same refactoring for CWallet::FundTransaction, which has a very similar parameter structure.

Suggested by laanwj in #20588 (comment).

fanquake added the label Refactoring on Dec 13, 2020

fanquake added the label Wallet on Dec 13, 2020

theStack force-pushed on Dec 13, 2020

DrahtBot commented at 6:45 AM on December 13, 2020: 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:

#25003 (tracing: fix coin_selection:aps_create_tx_internal calling logic by theStack)
#24897 ([Draft / POC] Silent Payments by w0xlt)
#24845 (wallet: createTransaction, return proper error description for "too-long-mempool-chain" + introduce generic Result classes by furszy)
#24752 (wallet: increase BnB upper limit by S3RK)
#24649 (wallet: do not count wallet utxos as external by S3RK)

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.

DrahtBot cross-referenced this on Dec 13, 2020 from issue Remove names from translatable strings by hebasto

DrahtBot cross-referenced this on Dec 13, 2020 from issue Add Single Random Draw as an additional coin selection algorithm by achow101

DrahtBot cross-referenced this on Dec 13, 2020 from issue Use effective values throughout coin selection by achow101

DrahtBot cross-referenced this on Dec 13, 2020 from issue Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101

in src/wallet/feebumper.cpp:220 in 8e9266fb7e outdated

 219 | -    CAmount fee_ret;
 220 | -    int change_pos_in_out = -1; // No requested location for change
 221 |      bilingual_str fail_reason;
 222 |      FeeCalculation fee_calc_out;
 223 | -    if (!wallet.CreateTransaction(recipients, tx_new, fee_ret, change_pos_in_out, fail_reason, new_coin_control, fee_calc_out, false)) {
 224 | +    auto txr = wallet.CreateTransaction(recipients, /* No requested location for change */ -1, fail_reason, new_coin_control, fee_calc_out, false);

pox commented at 5:27 AM on December 19, 2020:

While it's arguably not, strictly speaking, part of the proposed change, the comment about the change position parameter that's being maintained here is a bit misleading. This is a bit nit-picky, so bare with me please, but I think it's more appropriate to:

Use a const instead of inline comment in order to convey the meaning. e.g. const int RANDOM_CHANGE_POSITION = -1). This value is used more than once, so another reason to use a const.
Convey that passing -1 means "actively set the position to a random one" rather than "no requested location" which doesn't really let the reader know what's going to happen.

theStack commented at 7:26 PM on December 27, 2020:

Thanks a lot for reviewing, good point! I probably was a bit over-enthusiastic in getting rid of as many variables as possible before the call to CreateTransaction ;-) Introducing a RANDOM_CHANGE_POSITION constant makes a lot of sense to me, see latest force-push of the second commit. Not sure yet if putting the constant into the header is a good idea, as it is only used for one single method and hence many modules that don't need the constant would see it as well. Probably a candidate for a follow-up PR?

in src/wallet/wallet.cpp:3132 in 8e9266fb7e outdated

3142 | -            if (use_aps) {
3143 | -                tx = tx2;
3144 | -                nFeeRet = nFeeRet2;
3145 | -                nChangePosInOut = nChangePosInOut2;
3146 | -            }
3147 | +            const bool use_aps = txr_grouped->fee <= txr_ungrouped->fee + m_max_aps_fee;

pox commented at 5:33 AM on December 19, 2020:

This is a lot more readable indeed! 👍

pox commented at 5:36 AM on December 19, 2020: contributor

ACK not tested.

DrahtBot cross-referenced this on Dec 26, 2020 from issue refactor: split CWallet::Create by S3RK

theStack force-pushed on Dec 27, 2020

theStack commented at 7:29 PM on December 27, 2020: contributor

Force-pushed with the proposed changes by @pox (see #20640 (review)), introducing new constants RANDOM_CHANGE_POSITION to be used as second parameter for CWallet::CreateTransaction.

in src/wallet/feebumper.cpp:221 in 5865c38c6a outdated

 220 | -    int change_pos_in_out = -1; // No requested location for change
 221 | +    constexpr int RANDOM_CHANGE_POSITION = -1;
 222 |      bilingual_str fail_reason;
 223 |      FeeCalculation fee_calc_out;
 224 | -    if (!wallet.CreateTransaction(recipients, tx_new, fee_ret, change_pos_in_out, fail_reason, new_coin_control, fee_calc_out, false)) {
 225 | +    auto txr = wallet.CreateTransaction(recipients, RANDOM_CHANGE_POSITION, fail_reason, new_coin_control, fee_calc_out, false);

practicalswift commented at 10:50 PM on December 27, 2020:

Nit: Using auto here obscures the fact that txr is an optional. I think being explicit about the optionality is better in order to avoid mistakes in the future.

Applies throughout this PR.

theStack commented at 8:47 PM on December 30, 2020:

Thanks for reviewing! Agree that being explicit is better here. Done.

stackman27 approved

stackman27 commented at 4:01 AM on December 28, 2020: contributor

ACK Code review and built successful. Also, definitely a good idea to put FeeCalculation& fee_calc_out into this structure

pox commented at 5:02 AM on December 28, 2020: contributor

reACK 5865c38 🙏

theStack force-pushed on Dec 30, 2020

theStack commented at 8:52 PM on December 30, 2020: contributor

Force-pushed with changes as suggested by practicalswift, using explicit datatype std::optional<CreatedTransactionalResult> rather instead of auto for storing return values of CreateTransaction{Internal}(). Should be straight-forward to re-review, see range-diff.

DrahtBot cross-referenced this on Feb 5, 2021 from issue wallet: Avoid requesting fee rates multiple times during coin selection by achow101

DrahtBot cross-referenced this on Feb 17, 2021 from issue rpc: Disallow sendtoaddress and sendmany when private keys disabled by achow101

DrahtBot cross-referenced this on Feb 17, 2021 from issue MOVEONLY: CWallet transaction code out of wallet.cpp/.h by ryanofsky

DrahtBot cross-referenced this on Feb 17, 2021 from issue refactor: Make CWalletTx sync state type-safe by ryanofsky

DrahtBot added the label Needs rebase on Feb 19, 2021

theStack force-pushed on Mar 1, 2021

theStack commented at 12:11 AM on March 1, 2021: contributor

Rebased on master.

DrahtBot removed the label Needs rebase on Mar 1, 2021

DrahtBot cross-referenced this on Mar 2, 2021 from issue rpc: replace wallet raw pointers with references (#18592 rebased) by fanquake

DrahtBot cross-referenced this on Mar 4, 2021 from issue rpc: include_unsafe option for fundrawtransaction by t-bast

DrahtBot added the label Needs rebase on Mar 10, 2021

theStack force-pushed on Mar 31, 2021

theStack commented at 6:38 PM on March 31, 2021: contributor

Rebased on master and changed code to return uninitialized values explicitely via return std::nullopt rather than return {} (see #21498, #21415 (comment) for further explanation why this is preferred).

DrahtBot removed the label Needs rebase on Mar 31, 2021

DrahtBot cross-referenced this on May 20, 2021 from issue wallet: Properly support a wallet id by achow101

DrahtBot cross-referenced this on May 21, 2021 from issue wallet: Decide which coin selection solution to use based on waste metric by achow101

DrahtBot cross-referenced this on May 21, 2021 from issue wallet: Cleanup and refactor CreateTransactionInternal by achow101

DrahtBot cross-referenced this on May 22, 2021 from issue wallet: Introduce SelectionResult for encapsulating a coin selection solution by achow101

DrahtBot added the label Needs rebase on May 25, 2021

theStack force-pushed on May 29, 2021

theStack commented at 12:17 PM on May 29, 2021: contributor

Rebased on master.

DrahtBot removed the label Needs rebase on May 29, 2021

DrahtBot added the label Needs rebase on May 30, 2021

theStack force-pushed on Jun 9, 2021

theStack commented at 3:44 PM on June 9, 2021: contributor

Rebased on master again. (This time it was a bit more complex, as the involved methods have moved from wallet/wallet.cpp to wallet/spend.cpp).

DrahtBot removed the label Needs rebase on Jun 9, 2021

DrahtBot cross-referenced this on Jun 12, 2021 from issue refactor: Clean up new wallet spend, receive files added #21207 by ryanofsky

DrahtBot added the label Needs rebase on Sep 3, 2021

theStack force-pushed on Sep 10, 2021

DrahtBot removed the label Needs rebase on Sep 10, 2021

theStack commented at 8:31 PM on September 10, 2021: contributor

Rebased on master.

DrahtBot cross-referenced this on Sep 11, 2021 from issue consensus: move amount.h into consensus by fanquake

DrahtBot added the label Needs rebase on Oct 4, 2021

theStack force-pushed on Oct 29, 2021

theStack commented at 12:08 AM on October 29, 2021: contributor

Rebased on master again.

DrahtBot removed the label Needs rebase on Oct 29, 2021

DrahtBot cross-referenced this on Oct 29, 2021 from issue Optimize coin selection by dropping BnB upper limit by S3RK

DrahtBot cross-referenced this on Nov 13, 2021 from issue Add `src/node/` and `src/wallet/` code to `node::` and `wallet::` namespaces by ryanofsky

DrahtBot cross-referenced this on Nov 27, 2021 from issue wallet: Split stuff from rpcwallet by MarcoFalke

DrahtBot cross-referenced this on Dec 2, 2021 from issue MOVEONLY: Move wallet backup and encryption RPCs out of rpcwallet by meshcollider

DrahtBot cross-referenced this on Dec 6, 2021 from issue Split up rpcwallet by meshcollider

DrahtBot added the label Needs rebase on Dec 8, 2021

fanquake commented at 10:15 AM on March 22, 2022: member

@achow101 @glozow any Concept ACK / NACK thoughts on this PR?

achow101 commented at 2:25 PM on March 22, 2022: member

Concept ACK

theStack force-pushed on Mar 22, 2022

theStack commented at 3:15 PM on March 22, 2022: contributor

Rebased on master.

DrahtBot removed the label Needs rebase on Mar 22, 2022

DrahtBot cross-referenced this on Mar 22, 2022 from issue Remove unused feebumper code by MarcoFalke

DrahtBot cross-referenced this on Mar 22, 2022 from issue wallet: Use single FastRandomContext when creating a wallet tx by MarcoFalke

DrahtBot added the label Needs rebase on Mar 23, 2022

theStack force-pushed on Mar 31, 2022

theStack commented at 11:14 PM on March 31, 2022: contributor

Rebased on master again.

DrahtBot removed the label Needs rebase on Apr 1, 2022

DrahtBot cross-referenced this on Apr 1, 2022 from issue wallet: do not count wallet utxos as external by S3RK

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

DrahtBot cross-referenced this on Apr 5, 2022 from issue wallet: increase BnB upper limit by S3RK

DrahtBot cross-referenced this on Apr 14, 2022 from issue wallet: return error msg for "too-long-mempool-chain" by furszy

DrahtBot cross-referenced this on Apr 17, 2022 from issue [Draft / POC] Silent Payments by w0xlt

laanwj commented at 5:51 PM on April 21, 2022: member

Concept ACK. Returning things is much cleaner for out-only arguments than using mutable arguments (which would ideally be reserved for inout-arguments only).

furszy commented at 11:15 PM on April 21, 2022: member

First look, concept ACK.

Wasn't aware of this PR and touched the same piece of code in #24845.

-> PR where I introduced two generic versions of the optional return object called OperationResult and CallResult<T> (basically classes that encapsulate the function result: the result object, or in case of failure, the failure reason), which later can be used everywhere in the sources to cleanup similar boilerplate code where we pass a lot of ref args with a single return optional struct like the one introduced here.

So, combining both PRs, we will just return a CallResult<CreatedTransactionResult> which will cleanup the code further.

Will do a deeper code review soon.

in src/wallet/spend.h:102 in 5238022161 outdated

  96 | @@ -95,9 +97,9 @@ struct CreatedTransactionResult
  97 |  /**
  98 |   * Create a new transaction paying the recipients with a set of coins
  99 |   * selected by SelectCoins(); Also create the change output, when needed
 100 | - * @note passing nChangePosInOut as -1 will result in setting a random position
 101 | + * @note passing nChangePos as -1 will result in setting a random position
 102 |   */
 103 | -bool CreateTransaction(CWallet& wallet, const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, FeeCalculation& fee_calc_out, bool sign = true);
 104 | +std::optional<CreatedTransactionResult> CreateTransaction(CWallet& wallet, const std::vector<CRecipient>& vecSend, int nChangePos, bilingual_str& error, const CCoinControl& coin_control, FeeCalculation& fee_calc_out, bool sign = true);

achow101 commented at 5:36 PM on April 22, 2022:

In 5238022161a1ecd361916c6f425439e7c7604758 "wallet: CreateTransaction(): return out-params as (optional) struct"

nit: new variables should be snake_case.

Perhaps we should consider changing this to std::optional<int> and get rid of using -1 as a magic number.

theStack commented at 3:53 PM on May 16, 2022:

nit: new variables should be snake_case.

Thanks, renamed nChangePos to change_pos in both functions CreateTransaction and CreateTransactionInternal in the latest rebase.

Perhaps we should consider changing this to std::optional<int> and get rid of using -1 as a magic number.

Good idea, I think this would make a good follow-up PR candidate.

achow101 commented at 5:38 PM on April 22, 2022: member

ACK 5238022161a1ecd361916c6f425439e7c7604758

DrahtBot added the label Needs rebase on Apr 26, 2022

theStack cross-referenced this on Apr 27, 2022 from issue tracing: fix `coin_selection:aps_create_tx_internal` calling logic by theStack

furszy cross-referenced this on Apr 27, 2022 from issue wallet: remove extra wtx lookup in 'AvailableCoins' + several code cleanups. by furszy

wallet: CreateTransactionInternal(): return out-params as (optional) struct c9fdaa5e3a

wallet: CreateTransaction(): return out-params as (optional) struct 4c5ceb040c

in src/wallet/spend.h:87 in cc3a161704 outdated

  81 | @@ -82,6 +82,16 @@ std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAm
  82 |  std::optional<SelectionResult> SelectCoins(const CWallet& wallet, const std::vector<COutput>& vAvailableCoins, const CAmount& nTargetValue, const CCoinControl& coin_control,
  83 |                   const CoinSelectionParams& coin_selection_params) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
  84 |  
  85 | +struct CreatedTransactionResult

murchandamus commented at 3:45 PM on May 11, 2022:

Optional: I stumble a bit on past tense in the Class name here. I think I'd prefer either CreateTransactionResult to emphasize it's the result of a function called CreateTransaction…, or maybe just either CreatedTransaction or TransactionResult, since having both "result" and "created" feels a bit redundant.

theStack force-pushed on May 16, 2022

theStack commented at 3:57 PM on May 16, 2022: contributor

Rebased on master. Thanks to @furszy for getting the ball rolling on this PR again by pinging me up on IRC (I was planning to wait for #25003, but it seems that this will not be merged soon, as it was suggested to remove the affected tracepoint completely).

DrahtBot removed the label Needs rebase on May 16, 2022

murchandamus commented at 5:45 PM on May 16, 2022: contributor

ACK 4c5ceb040cf50d24201903a9200fb23be88d96fb

achow101 commented at 8:00 PM on May 16, 2022: member

re-ACK 4c5ceb040cf50d24201903a9200fb23be88d96fb

w0xlt approved

w0xlt commented at 1:43 AM on May 17, 2022: contributor

crACK https://github.com/bitcoin/bitcoin/pull/20640/commits/4c5ceb040cf50d24201903a9200fb23be88d96fb

fanquake merged this on May 17, 2022

fanquake closed this on May 17, 2022

theStack deleted the branch on May 17, 2022

sidhujag referenced this in commit 18bc5455df on May 28, 2022

luke-jr cross-referenced this on Jun 18, 2022 from issue wallet: re-activate the not triggered "AmountWithFeeExceedsBalance" error by furszy

furszy cross-referenced this on Jun 27, 2022 from issue refactor: introduce generic 'Result' class and connect it to CreateTransaction and GetNewDestination by furszy

theStack cross-referenced this on Sep 20, 2022 from issue wallet, refactor: FundTransaction(): return out-params as `util::Result` structure by theStack

bitcoin locked this on May 17, 2023