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

furszy commented at 12:55 pm on March 22, 2024: member

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 commented at 12:55 pm on March 22, 2024: contributor

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
Concept ACK	ryanofsky, 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/26732 ([WIP] wallet: tx creation, don’t select outputs from txes that are being replaced by furszy)

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.

furszy force-pushed on Mar 22, 2024

DrahtBot added the label CI failed on Mar 22, 2024

DrahtBot commented at 1:09 pm on March 22, 2024: contributor

🚧 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

DrahtBot removed the label CI failed on Mar 22, 2024

pablomartin4btc commented at 2:54 am on April 2, 2024: contributor

tACK dfbd145676255f82bad529976928dff1939c5e48

0wallet/interfaces.cpp:289:57: runtime error: implicit conversion from type 'int' of value -1 (32-bit, signed) to type 'unsigned int' changed the value to 4294967295 (32-bit, unsigned)

I agree with the reasoning behind the removal of some arguments and the unreachable exception.

Out of curiosity, could you please add some refs on “The input-output ‘change_pos’ ref arg from createTransaction, which has been a source of bugs in the past.”, if possible?

Are there other components/ functions that you think could be moved into the new util_spend.h file (perhaps as a follow-up)?

furszy commented at 4:16 am on April 2, 2024: member

Out of curiosity, could you please add some refs on “The input-output ‘change_pos’ ref arg from createTransaction, which has been a source of bugs in the past.”, if possible?

Sure, https://github.com/bitcoin/bitcoin/pull/29065.

Are there other components/ functions that you think could be moved into the new util_spend.h file (perhaps as a follow-up)?

Not off the top of my head. Matter of continue breaking the GUI classes dependency on wallet.h.

in src/interfaces/wallet.h:146 in dfbd145676 outdated

142@@ -142,11 +143,10 @@ class Wallet
143     virtual void listLockedCoins(std::vector<COutPoint>& outputs) = 0;
144 
145     //! Create transaction.
146-    virtual util::Result<CTransactionRef> createTransaction(const std::vector<wallet::CRecipient>& recipients,
147+    virtual util::Result<CreatedTransactionResult> createTransaction(const std::vector<wallet::CRecipient>& recipients,

pablomartin4btc commented at 11:57 pm on April 2, 2024:

nit: the tx won’t be created when an error is raised during the createTransaction’s call (eg no recipient) so the obj should represent the correct thing, unless I misunderstanding the use case here… ok I see now this is how src/util/result.h works, so ignore my correction, I find it a bit weird but perhaps cos I’m not used to it yet. As a ref this change could be useful https://github.com/bitcoin/bitcoin/pull/25665

0    virtual util::Result<CreateTransactionResult> createTransaction(const std::vector<wallet::CRecipient>& recipients,

furszy commented at 1:01 pm on April 3, 2024:

CreatedTransactionResult is an existing struct. The suggestion wouldn’t compile. We could rename the struct if that is generating confusion.

pablomartin4btc commented at 4:50 pm on April 3, 2024:

It’s ok, I got confused at first glance but then read more about how util::Result is being used in all our code.

https://github.com/bitcoin-core/gui/blob/0d509bab45d292caeaf34600e57b5928757c6005/src/util/result.h#L19-L33

pablomartin4btc approved

pablomartin4btc commented at 11:58 pm on April 2, 2024: contributor

cr ACK dfbd145676255f82bad529976928dff1939c5e48

DrahtBot added the label Needs rebase on Apr 18, 2024

furszy force-pushed on Apr 18, 2024

furszy commented at 11:48 am on April 18, 2024: member

Rebased after #806 merge.

DrahtBot removed the label Needs rebase on Apr 18, 2024

hebasto commented at 1:58 pm on May 12, 2024: member

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

hebasto commented at 1:59 pm on May 12, 2024: member

cc @achow101 @ryanofsky

in src/wallet/util_spend.h:13 in 6d59564646 outdated

 8+#include <policy/fees.h>
 9+#include <wallet/transaction.h>
10+
11+#include <optional>
12+
13+struct CreatedTransactionResult

ryanofsky commented at 4:14 pm on May 13, 2024:

In commit “refactor: move CreatedTransactionResult to an util file” (6d59564646cd6ec087c9cab20f9d5707ec5dd4f9)

Would suggest maybe moving this struct to wallet/types.h instead of introducing a new header. That file is meant to hold wallet types that are used outside of the wallet library. (For context see https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1543379055 which talks about wallet/types.h, node/types.h, and common/types.h files).

Or if you would prefer to use a separate header, would suggest spend_types.h instead of util_spend.h so it is grouped with spend.h and spend.cpp alphabetically, and to be clear the header should only contain types, not spending functions which would probably be inappropriate to call from the GUI. Would also suggest keeping the wallet:: namespace for consistency. If you look at other wallet types used in the GUI like AddressPurpose, isminetype, and CCoinControl, they are all in the wallet namespace.

furszy commented at 9:02 pm on May 16, 2024:

Would suggest maybe moving this struct to wallet/types.h instead of introducing a new header. That file is meant to hold wallet types that are used outside of the wallet library. (For context see https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1543379055 which talks about wallet/types.h, node/types.h, and common/types.h files).

Nice, sure. I’m on the same boat, I just wrote this without thinking on a general convention.

furszy force-pushed on May 16, 2024

furszy commented at 9:03 pm on May 16, 2024: member

updated per feedback. Thanks @ryanofsky!

DrahtBot added the label CI failed on May 16, 2024

DrahtBot commented at 9:22 pm on May 16, 2024: contributor

🚧 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

pablomartin4btc commented at 11:18 pm on May 16, 2024: contributor

re-ACK 4d941b5ff43e6e7efa83913ded8cd5806ef92dbe

Changes from my last review: CreatedTransactionResult struct moved as suggested by @ryanofsky explaining reasoning behind the convention. I like the idea of keeping the namespace, not only for consistency but for clarity and making the code more readable. I think it would be good to document this somewhere, perhaps in the developer notes to begin with (?).

nit: on 2nd. commit (95aaaa4f023c6a6b629c36cfb9d8abd4bab1cb66), you forgot to remove the change from the makefile as it’s not longer needed.

furszy force-pushed on May 17, 2024

furszy commented at 1:40 am on May 17, 2024: member

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 commented at 2:00 pm on May 17, 2024: member

CI failure is unrelated.

in src/qt/walletmodel.cpp:221 in 355199c7a6 outdated

213@@ -214,12 +214,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact
214         if (fSubtractFeeFromAmount && newTx)
215             transaction.reassignAmounts(nChangePosRet);
216 
217-        if(!newTx)
218-        {
219-            if(!fSubtractFeeFromAmount && (total + nFeeRequired) > nBalance)
220-            {
221-                return SendCoinsReturn(AmountWithFeeExceedsBalance);

ryanofsky commented at 6:16 pm on May 17, 2024:

In commit “gui: remove unreachable AmountWithFeeExceedsBalance error” (355199c7a6c58eb812dd7ef8e204e6e068c142ae)

This is a nice catch noticing that this code doesn’t work. But I think the fact that it doesn’t work is a pretty unfortunate problem, and the current PR will make fixing it harder.

It seems like bitcoin/bitcoin#20640 unintentionally introduced a bug where the more accurate error message “The total exceeds your balance when the %1 transaction fee is included” can never trigger, and instead only less informative “Insufficient funds” or “The amount exceeds your balance” errors will be shown.

It looks like there are few ways this could be fixed:

An easy way to fix it would be to restore the CAmount& fee output parameter removed by #20640 so it is always returned and this code can work the way it was originally intended.
A slightly more complicated but cleaner fix would be to use CreatedTransactionResult as a return type instead of util::Result<CreatedTransactionResult> and add a bilingual_str error field to CreatedTransactionResult so fee information and an error string can be returned together.
Another alternative fix could be to build on bitcoin/bitcoin#25665 and keep using util::Result, but return the fee in the new util::Result failure field.
And the best but probably most complicated fix would be to remove error message code from the GUI and instead generate detailed error messages in wallet code. This would require bigger changes in the GUI code and might require changing the interface to provide the wallet with more information about transactions.

Against that background, I think this PR (as of edc3f9a733aaa1a1cc2547013d09c27828151e8a) makes some nice cleanups, but I don’t like the current changes because I think they make all of the fixes above except maybe the last one harder to implement.

So I don’t think I would ACK this PR currently, I’d happily ACK it if it either (1) fixed the problem, maybe using one of the easier fixes suggested above, or (2) did not fix the problem, but at least did not make it harder to fix in the future. This could be implemented by keeping most of the changes in the PR, but not deleting this GUI code in this commit and not deleting the CAmount& fee output parameter from the wallet interface after this commit.

ryanofsky commented at 6:26 pm on May 17, 2024: contributor

Concept ACK edc3f9a733aaa1a1cc2547013d09c27828151e8a, but this has some changes I like and other changes I don’t think are good. Left a more detailed comment below.

ryanofsky commented at 6:37 pm on May 17, 2024: contributor

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.

wallet: introduce "tx amount exceeds balance when fees are included" error

This was previously implemented at the GUI level but has been broken since #20640

900e5ed51b

gui: remove unreachable AmountWithFeeExceedsBalance error

Since #20640, the 'createTransaction' does no longer retrieve the
fee if the process fails due to insufficient funds.

But, since #25269, 'createTransaction' retrieves an error message
indicating that the total transaction amount exceeds the wallet
available balance when fees are included.

So this enum is no longer needed.

a4007a447e

refactor: move CreatedTransactionResult to types.h

So it can be used by external modules without requiring
wallet.h dependency.

9cd8462a6a

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

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

Reasons for the refactoring:
- The 'change_pos' ref argument has been a source of bugs in the past.
- The 'fee' ref argument is currently only set when the transaction creation process succeeds.

0b92ee2bc7

furszy force-pushed on May 22, 2024

furszy commented at 2:24 pm on May 22, 2024: member

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.

hebasto commented at 3:35 pm on July 15, 2024: member

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 marked this as a draft on Jul 15, 2024

furszy commented at 7:25 pm on July 15, 2024: member

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

vasild commented at 12:53 pm on July 22, 2024: contributor

Concept ACK

refactor: interfaces, make ‘createTransaction’ less error-prone #807

Reviews

Conflicts