Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

furszy
Copy link
Member

@furszy furszy commented Mar 22, 2024

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 wallet: re-activate "AmountWithFeeExceedsBalance" error bitcoin/bitcoin#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
Copy link
Contributor

DrahtBot commented Mar 22, 2024

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.

@DrahtBot
Copy link
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

Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK dfbd145

This PR solves the error described in #805, even #806 fixes other 2 related errors.
wallet/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)?

Copy link
Member Author

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, bitcoin/bitcoin#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.

Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cr ACK dfbd145

@furszy furszy force-pushed the 2024_gui_clean_createTransaction branch from dfbd145 to 7fb81d2 Compare April 18, 2024 11:47
Copy link
Member Author

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased after #806 merge.

@hebasto
Copy link
Member

hebasto commented May 12, 2024

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

@hebasto
Copy link
Member

hebasto commented May 12, 2024

cc @achow101 @ryanofsky

src/wallet/util_spend.h Outdated Show resolved Hide resolved
@furszy furszy force-pushed the 2024_gui_clean_createTransaction branch from 7fb81d2 to 4d941b5 Compare May 16, 2024 21:00
Copy link
Member Author

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated per feedback. Thanks @ryanofsky!

@DrahtBot
Copy link
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

Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-ACK 4d941b5

  • 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 (95aaaa4), you forgot to remove the change from the makefile as it's not longer needed.

@furszy furszy force-pushed the 2024_gui_clean_createTransaction branch from 4d941b5 to edc3f9a Compare May 17, 2024 01:40
@furszy
Copy link
Member Author

furszy commented May 17, 2024

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

Removed. Thanks.

@furszy
Copy link
Member Author

furszy commented May 17, 2024

CI failure is unrelated.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
if(!fSubtractFeeFromAmount && (total + nFeeRequired) > nBalance)
{
return SendCoinsReturn(AmountWithFeeExceedsBalance);
Copy link
Contributor

@ryanofsky ryanofsky May 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "gui: remove unreachable AmountWithFeeExceedsBalance error" (355199c)

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 edc3f9a) 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
Copy link
Contributor

ryanofsky commented May 17, 2024

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?

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.

…error

This was previously implemented at the GUI level but has been broken since #20640
furszy added 3 commits May 22, 2024 11:12
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.
So it can be used by external modules without requiring
wallet.h dependency.
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.
@furszy furszy force-pushed the 2024_gui_clean_createTransaction branch from edc3f9a to 0b92ee2 Compare May 22, 2024 14:13
Copy link
Member Author

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

hebasto commented Jul 15, 2024

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 furszy marked this pull request as draft July 15, 2024 19:24
@furszy
Copy link
Member Author

furszy commented Jul 15, 2024

Drafted until bitcoin/bitcoin#25269 gets merged. This is a continuation of that work.

@vasild
Copy link
Contributor

vasild commented Jul 22, 2024

Concept ACK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants