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

theStack commented at 4:03 am on December 13, 2020: member

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: 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:

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

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.

theStack force-pushed on Dec 27, 2020

theStack commented at 7:29 pm on December 27, 2020: member

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: member

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 added the label Needs rebase on Feb 19, 2021

theStack force-pushed on Mar 1, 2021

theStack commented at 0:11 am on March 1, 2021: member

Rebased on master.

DrahtBot removed the label Needs rebase on Mar 1, 2021

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: member

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 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: member

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: member

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 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: member

Rebased on master.

DrahtBot added the label Needs rebase on Oct 4, 2021

theStack force-pushed on Oct 29, 2021

theStack commented at 0:08 am on October 29, 2021: member

Rebased on master again.

DrahtBot removed the label Needs rebase on Oct 29, 2021

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: member

Rebased on master.

DrahtBot removed the label Needs rebase on Mar 22, 2022

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: member

Rebased on master again.

DrahtBot removed the label Needs rebase on Apr 1, 2022

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

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

Xekyo 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: member

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

Xekyo commented at 5:45 pm on May 16, 2022: member

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

DrahtBot locked this on May 17, 2023

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

Conflicts