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

theStack · 2020-12-13T04:03:13Z

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

DrahtBot · 2020-12-13T06:45:06Z

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.

pox

ACK not tested.

src/wallet/feebumper.cpp

src/wallet/wallet.cpp

theStack · 2020-12-27T19:29:22Z

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

src/wallet/feebumper.cpp

stackman27

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

pox · 2020-12-28T05:02:26Z

reACK 5865c38
🙏

theStack · 2020-12-30T20:52:30Z

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.

theStack · 2021-03-01T00:11:11Z

Rebased on master.

theStack · 2021-03-31T18:38:38Z

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

theStack · 2022-03-31T23:14:09Z

Rebased on master again.

laanwj · 2022-04-21T17:51:48Z

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

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.

achow101

ACK 5238022

achow101 · 2022-04-22T17:36:54Z

src/wallet/spend.h

 */
-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);
+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);


In 5238022 "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.

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.

…struct

theStack · 2022-05-16T15:57:14Z

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

murchandamus

ACK 4c5ceb0

murchandamus · 2022-05-11T15:45:43Z

src/wallet/spend.h

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

+struct CreatedTransactionResult


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.

achow101 · 2022-05-16T20:00:36Z

re-ACK 4c5ceb0

w0xlt

crACK 4c5ceb0

…nsactio…

fanquake added Refactoring Wallet labels Dec 13, 2020

theStack force-pushed the 202012-refactor-wallet-createtransaction-return_out_params_in_optstruct branch from 6dc88c6 to 8e9266f Compare December 13, 2020 04:33

pox reviewed Dec 19, 2020

View reviewed changes

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

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

DrahtBot mentioned this pull request Dec 26, 2020

refactor: split CWallet::Create #20773

Merged

theStack force-pushed the 202012-refactor-wallet-createtransaction-return_out_params_in_optstruct branch from 8e9266f to 5865c38 Compare December 27, 2020 19:17

practicalswift reviewed Dec 27, 2020

View reviewed changes

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

stackman27 approved these changes Dec 28, 2020

View reviewed changes

theStack force-pushed the 202012-refactor-wallet-createtransaction-return_out_params_in_optstruct branch from 5865c38 to c32e2b1 Compare December 30, 2020 20:46

DrahtBot mentioned this pull request Feb 5, 2021

wallet: Avoid requesting fee rates multiple times during coin selection #21083

Merged

This was referenced Feb 17, 2021

rpc: Disallow sendtoaddress and sendmany when private keys disabled #21201

Merged

MOVEONLY: CWallet transaction code out of wallet.cpp/.h #21207

Merged

refactor: Make CWalletTx sync state type-safe #21206

Merged

DrahtBot added the Needs rebase label Feb 19, 2021

theStack force-pushed the 202012-refactor-wallet-createtransaction-return_out_params_in_optstruct branch from c32e2b1 to 3a07695 Compare March 1, 2021 00:09

DrahtBot removed the Needs rebase label Mar 1, 2021

This was referenced Mar 2, 2021

rpc: replace wallet raw pointers with references (#18592 rebased) #21331

Merged

rpc: include_unsafe option for fundrawtransaction #21359

Merged

DrahtBot added the Needs rebase label Mar 10, 2021

theStack force-pushed the 202012-refactor-wallet-createtransaction-return_out_params_in_optstruct branch from 3a07695 to 5832404 Compare March 31, 2021 18:35

DrahtBot added the Needs rebase label Mar 23, 2022

theStack force-pushed the 202012-refactor-wallet-createtransaction-return_out_params_in_optstruct branch from dc1c6d8 to 5238022 Compare March 31, 2022 23:11

DrahtBot removed the Needs rebase label Apr 1, 2022

This was referenced Apr 1, 2022

wallet: do not count wallet utxos as external #24649

Merged

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

Merged

wallet: increase BnB upper limit #24752

Closed

This was referenced Apr 14, 2022

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

Merged

[Draft / POC] Silent Payments #24897

Closed

furszy reviewed Apr 21, 2022

View reviewed changes

achow101 reviewed Apr 22, 2022

View reviewed changes

DrahtBot added the Needs rebase label Apr 26, 2022

theStack mentioned this pull request Apr 27, 2022

tracing: fix coin_selection:aps_create_tx_internal calling logic #25003

Merged

furszy mentioned this pull request Apr 27, 2022

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

Merged

theStack added 2 commits May 16, 2022 17:37

wallet: CreateTransactionInternal(): return out-params as (optional) …

c9fdaa5

…struct

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

4c5ceb0

theStack force-pushed the 202012-refactor-wallet-createtransaction-return_out_params_in_optstruct branch from 5238022 to 4c5ceb0 Compare May 16, 2022 15:49

DrahtBot removed the Needs rebase label May 16, 2022

murchandamus reviewed May 16, 2022

View reviewed changes

w0xlt approved these changes May 17, 2022

View reviewed changes

fanquake merged commit 1ab389b into bitcoin:master May 17, 2022

theStack deleted the 202012-refactor-wallet-createtransaction-return_out_params_in_optstruct branch May 17, 2022 10:30

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 28, 2022

Merge bitcoin#20640: wallet, refactor: return out-params of CreateTra…

18bc545

…nsactio…

luke-jr mentioned this pull request Jun 18, 2022

wallet: re-activate the not triggered "AmountWithFeeExceedsBalance" error #25269

Closed

furszy mentioned this pull request Jun 27, 2022

refactor: introduce generic 'Result' class and connect it to CreateTransaction and GetNewDestination #25218

Merged

theStack mentioned this pull request Sep 20, 2022

wallet, refactor: FundTransaction(): return out-params as util::Result structure #26129

Closed

bitcoin locked and limited conversation to collaborators May 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

theStack commented Dec 13, 2020 •

edited

DrahtBot commented Dec 13, 2020 •

edited

pox left a comment

theStack commented Dec 27, 2020

stackman27 left a comment •

edited

pox commented Dec 28, 2020

theStack commented Dec 30, 2020 •

edited

theStack commented Mar 1, 2021

theStack commented Mar 31, 2021

theStack commented Mar 31, 2022

laanwj commented Apr 21, 2022

furszy left a comment

achow101 left a comment

achow101 Apr 22, 2022

theStack May 16, 2022

theStack commented May 16, 2022

murchandamus left a comment

murchandamus May 11, 2022

achow101 commented May 16, 2022

w0xlt left a comment

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

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

Conversation

theStack commented Dec 13, 2020 • edited

DrahtBot commented Dec 13, 2020 • edited

Conflicts

pox left a comment

Choose a reason for hiding this comment

theStack commented Dec 27, 2020

stackman27 left a comment • edited

Choose a reason for hiding this comment

pox commented Dec 28, 2020

theStack commented Dec 30, 2020 • edited

theStack commented Mar 1, 2021

theStack commented Mar 31, 2021

theStack commented Mar 31, 2022

laanwj commented Apr 21, 2022

furszy left a comment

Choose a reason for hiding this comment

achow101 left a comment

Choose a reason for hiding this comment

achow101 Apr 22, 2022

Choose a reason for hiding this comment

theStack May 16, 2022

Choose a reason for hiding this comment

theStack commented May 16, 2022

murchandamus left a comment

Choose a reason for hiding this comment

murchandamus May 11, 2022

Choose a reason for hiding this comment

achow101 commented May 16, 2022

w0xlt left a comment

Choose a reason for hiding this comment

theStack commented Dec 13, 2020 •

edited

DrahtBot commented Dec 13, 2020 •

edited

stackman27 left a comment •

edited

theStack commented Dec 30, 2020 •

edited