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

[theStack]

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]

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.

[theStack]

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.

[stackman27]

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

[pox]

reACK 5865c38
🙏

[theStack]

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]

Rebased on master.

[theStack]

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]

Rebased on master again.

[laanwj]

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

[achow101]

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]

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.

[theStack]

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]

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]

re-ACK 4c5ceb0

[w0xlt]

crACK 4c5ceb0