refactor: interfaces, make 'createTransaction' less error-prone

[furszy]

trafficstars

Bundle all function's outputs inside the util::Result returned object.

Removals:

• The input-output 'change_pos' ref arg from createTransaction, which has been a source of bugs in the past.
• The 'fee' ref arg from createTransaction, which is currently only set when the transaction creation process succeeds.
• The currently unreachable AmountWithFeeExceedsBalance error (more info about its re-introduction at https://github.com/bitcoin/bitcoin/pull/25269).

Additionally, this PR moves the CreatedTransactionResult struct into its own file. This change is made to avoid further expanding the GUI dependencies on wallet.h. Structurally, the GUI should only access the model/interfaces and never the wallet directly.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky
Concept ACK	hebasto, vasild
Stale ACK	pablomartin4btc

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #bitcoin/bitcoin/32562 (doc: remove // for ... comments by fanquake)

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]

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin-core/gui/runs/22979285609}

[hebasto]

While introducing a new wallet/util_spend.h header, it seems reasonable to address all related IWYU warnings, no?

[hebasto]

cc @achow101 @ryanofsky

[DrahtBot]

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin-core/gui/runs/25073685372}

[furszy]

nit: on 2nd. commit (https://github.com/bitcoin-core/gui/commit/95aaaa4f023c6a6b629c36cfb9d8abd4bab1cb66), you forgot to remove the change from the makefile as it's not longer needed.

Removed. Thanks.

[furszy]

CI failure is unrelated.

[ryanofsky]

I just noticed https://github.com/bitcoin/bitcoin/pull/25269, which I hadn't seen when I posted my comment above. It looks like that PR was closed for inactivity but it does look like a good approach that solves the issue I didn't like in my comment. (Though maybe that PR is still a slight regression because it doesn't include the amount of the fee in the error message.) I wonder if you'd consider reopening that PR and maybe including the changes in this PR there, or basing this PR on that one?

Or if you no longer think that PR is worth pursuing, I think you could separate the cleanups here from the issue that PR was trying to address by leaving the CAmount& fee output parameter and GUI code intact here, but still doing the other cleanups.

[hebasto]

Concept ACK.

Additionally, this PR moves the CreatedTransactionResult struct into its own file.

This part of the PR description seems no longer relevant.

Thanks for the review ryanofsky!

I just noticed bitcoin/bitcoin#25269, which I hadn't seen when I posted my comment above. It looks like that PR was closed for inactivity but it does look like a good approach that solves the issue I didn't like in my comment. (Though maybe that PR is still a slight regression because it doesn't include the amount of the fee in the error message.) I wonder if you'd consider reopening that PR and maybe including the changes in this PR there, or basing this PR on that one?

Yeah sure, I closed it for lack of movement after 6 months. I just reopen it, reworking part of it, and rebased this one on top. I think it make sense to re-introduce the functionality there and cleanup the GUI code here.

I agree on focusing this PR on the code cleanup only.

[furszy]

Drafted until https://github.com/bitcoin/bitcoin/pull/25269 gets merged. This is a continuation of that work.

[vasild]

Concept ACK

[DrahtBot]

🐙 This pull request conflicts with the target branch and needs rebase.

[achow101]

Are you still working on this?