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

pull furszy wants to merge 4 commits into bitcoin-core:master from furszy:2024_gui_clean_createTransaction changing 7 files +65 −43
  1. 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.

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

    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.

  3. furszy force-pushed on Mar 22, 2024
  4. DrahtBot added the label CI failed on Mar 22, 2024
  5. 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

  6. DrahtBot removed the label CI failed on Mar 22, 2024
  7. 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)?

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

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

  10. pablomartin4btc approved
  11. pablomartin4btc commented at 11:58 pm on April 2, 2024: contributor
    cr ACK dfbd145676255f82bad529976928dff1939c5e48
  12. DrahtBot added the label Needs rebase on Apr 18, 2024
  13. furszy force-pushed on Apr 18, 2024
  14. furszy commented at 11:48 am on April 18, 2024: member
    Rebased after #806 merge.
  15. DrahtBot removed the label Needs rebase on Apr 18, 2024
  16. 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?
  17. hebasto commented at 1:59 pm on May 12, 2024: member
  18. 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.

  19. furszy force-pushed on May 16, 2024
  20. furszy commented at 9:03 pm on May 16, 2024: member
    updated per feedback. Thanks @ryanofsky!
  21. DrahtBot added the label CI failed on May 16, 2024
  22. 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

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

  24. furszy force-pushed on May 17, 2024
  25. 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.

  26. furszy commented at 2:00 pm on May 17, 2024: member
    CI failure is unrelated.
  27. 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.

  28. 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.
  29. 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.

  30. 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
  31. 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
  32. refactor: move CreatedTransactionResult to types.h
    So it can be used by external modules without requiring
    wallet.h dependency.
    9cd8462a6a
  33. 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
  34. furszy force-pushed on May 22, 2024
  35. 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.

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

  37. furszy marked this as a draft on Jul 15, 2024
  38. 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.
  39. vasild commented at 12:53 pm on July 22, 2024: contributor
    Concept ACK

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-06 05:20 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me