wallet/refactor: change PSBTError to PSBTResult and remove std::optional<common::PSBTResult> and return common::PSBTResult #32958

kevkevinpal · 2025-07-13T14:06:28Z

Description

This is a follow-up to #31622 (comment) and #31622 (comment)

What this changes

Updates PSBTError to PSBTResult
because we now have PSBTResult::OK
Updates PSBTErrorString to PSBTResultString
Updates RPCErrorFromPSBTError to RPCErrorFromPSBTResult
Removes std::optional<common::PSBTResult> and instead returns just common::PSBTResult
replacing when we return null with PSBTResult::OK

DrahtBot · 2025-07-13T14:06:32Z

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32958.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	naiyoma

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33008 (wallet: support bip388 policy with external signer by Sjors)
#32876 (refactor: use options struct for signing and PSBT operations by Sjors)
#32857 (wallet: allow skipping script paths by Sjors)
#31650 (refactor: Avoid copies by using const references or by move-construction by maflcko)

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.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

SignPSBTInput(DUMMY_SIGNING_PROVIDER, psbtx, i, &txdata, std::nullopt, &outdata) in src/node/psbt.cpp
SignPSBTInput(DUMMY_SIGNING_PROVIDER, psbtx, i, nullptr, std::nullopt) in src/node/psbt.cpp
SignPSBTInput(DUMMY_SIGNING_PROVIDER, psbtx, i, &txdata, input.sighash_type, nullptr, true) in src/psbt.cpp
FillPSBT(psbtx, complete, std::nullopt, /sign=/true, /bip32derivs=/false) in src/wallet/feebumper.cpp
FillPSBT(psbtx, complete, std::nullopt, /sign=/true, /bip32derivs=/false) in src/wallet/rpc/spend.cpp
FillPSBT(psbtx, complete, std::nullopt, false, true) in src/wallet/test/psbt_wallet_tests.cpp
SignPSBTInput(HidingSigningProvider(...), psbtx, i, &txdata, sighash_type, nullptr, finalize) in src/wallet/scriptpubkeyman.cpp
DescriptorScriptPubKeyMan::FillPSBT(psbt, txdata, sighash_type, false, bip32derivs, n_signed, finalize) in src/wallet/external_signer_scriptpubkeyman.cpp

No suggestions were made for calls that already use named comments for the literals or where the literal is among the first two arguments.

^2025-12-31

DrahtBot · 2025-07-13T14:12:30Z

🚧 At least one of the CI tasks failed.
_{Task ARM, unit tests, no functional tests: https://github.com/bitcoin/bitcoin/runs/45878284919}
_{LLM reason (✨ experimental): The CI failure is caused by compilation errors due to incorrect handling of PSBTResult objects in psbtoperationsdialog.cpp.}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

Possibly 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.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

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

naiyoma

Concept ACK

l have looked at 87736a986d510e1ea72dfa7051284b3335d124a0, 595909674ea70c34363053831ffc633975605865 and 125431e6afaf10816a09c6ba6a41c2af086b5558 changes, LGTM

makes sense to rename since PSBTError::OK is not an error.

PSBTError is consistent with TransactionError, https://github.com/bitcoin/bitcoin/blob/master/src/node/types.h#L24 so maybe TransactionError should also be renamed in a follow-up PR.

naiyoma · 2025-08-11T18:06:04Z

src/qt/psbtoperationsdialog.cpp

@@ -62,7 +62,7 @@ void PSBTOperationsDialog::openWithPSBT(PartiallySignedTransaction psbtx)
        const auto err{m_wallet_model->wallet().fillPSBT(std::nullopt, /*sign=*/false, /*bip32derivs=*/true, &n_could_sign, m_transaction_data, complete)};


nit: rename err -> result

Thanks for the review! pushed an update in d242c75c5c0eb7d5515cc99fda39ff98a6fdd5af

naiyoma

utack d242c75c5c0eb7d5515cc99fda39ff98a6fdd5af
Haven't done thorough testing yet, but returning an explicit PSBTResult::OK when there's no error is clearer and more optimal than using an optional wrapper.

kevkevinpal · 2025-10-24T20:46:13Z

Just rebased the PR to 86d9a82 since @DrahtBot was upset

TumaBitcoiner

I made some suggestions to keep consistency between naming and files. Sometimes result was used, other times err.

src/qt/sendcoinsdialog.cpp

src/qt/walletmodel.cpp

src/wallet/rpc/spend.cpp

src/wallet/wallet.cpp

DrahtBot · 2025-11-21T17:36:43Z

🚧 At least one of the CI tasks failed.
_{Task Windows-cross to x86_64: https://github.com/bitcoin/bitcoin/actions/runs/19578345228/job/56069612685}
_{LLM reason (✨ experimental): C++ compilation failure in wallet/spend.cpp due to using a const PSBTResult where pushKV is invoked, causing a type/return mismatch.}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

Possibly 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.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

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

PSBTError now contains the PSBTError::Ok type which means it makes sense to rename it into PSBTResult since there is a case where it does not error

PSBTResult has an enum value of OK, so it makes sense to return OK insead of a {}. This change makes it so we check for an OK value instead of null/{}

Since changing PSBTError -> PSBTResult it makes sense to change the variable names to reflect that it is not always an error and sometimes can have the value of OK

kevkevinpal · 2025-12-31T19:22:33Z

rebased to dd51b84

kevkevinpal force-pushed the PSBTErrorToPSBTResult branch from 4786148 to 0748f9b Compare July 13, 2025 14:12

DrahtBot added the CI failed label Jul 13, 2025

kevkevinpal force-pushed the PSBTErrorToPSBTResult branch 3 times, most recently from 1a246a9 to a92955d Compare July 13, 2025 14:27

kevkevinpal mentioned this pull request Jul 13, 2025

Update PSBTError to PSBTResult and to not use Optional for return type kevkevinpal/bitcoin#60

Open

DrahtBot removed the CI failed label Jul 13, 2025

naiyoma reviewed Aug 11, 2025

View reviewed changes

kevkevinpal force-pushed the PSBTErrorToPSBTResult branch from a92955d to d242c75 Compare August 11, 2025 19:04

naiyoma reviewed Aug 13, 2025

View reviewed changes

DrahtBot added the Needs rebase label Oct 24, 2025

kevkevinpal force-pushed the PSBTErrorToPSBTResult branch from d242c75 to 86d9a82 Compare October 24, 2025 20:43

DrahtBot removed the Needs rebase label Oct 24, 2025

kevkevinpal requested a review from naiyoma October 26, 2025 00:23

TumaBitcoiner reviewed Nov 21, 2025

View reviewed changes

kevkevinpal force-pushed the PSBTErrorToPSBTResult branch from 4e038c4 to 5c69aca Compare November 21, 2025 17:36

DrahtBot added the CI failed label Nov 21, 2025

kevkevinpal force-pushed the PSBTErrorToPSBTResult branch 2 times, most recently from 02c8c5d to a8c4145 Compare November 21, 2025 18:19

DrahtBot removed the CI failed label Nov 21, 2025

kevkevinpal added 5 commits December 31, 2025 14:21

refactor: Updated PSBTError to PSBTResult

45acac8

PSBTError now contains the PSBTError::Ok type which means it makes sense to rename it into PSBTResult since there is a case where it does not error

refactor: rename PSBTErrorString to PSBTResultString

d2d4177

refactor: RPCErrorFromPSBTError to RPCErrorFromPSBTResult

f530980

wallet: remove std::optional when PSBTResult is returned

8cc7e0e

PSBTResult has an enum value of OK, so it makes sense to return OK insead of a {}. This change makes it so we check for an OK value instead of null/{}

refactor: rename err/error -> result when a PSBTResult

dd51b84

Since changing PSBTError -> PSBTResult it makes sense to change the variable names to reflect that it is not always an error and sometimes can have the value of OK

kevkevinpal force-pushed the PSBTErrorToPSBTResult branch from a8c4145 to dd51b84 Compare December 31, 2025 19:22

		@@ -62,7 +62,7 @@ void PSBTOperationsDialog::openWithPSBT(PartiallySignedTransaction psbtx)
		const auto err{m_wallet_model->wallet().fillPSBT(std::nullopt, /sign=/false, /bip32derivs=/true, &n_could_sign, m_transaction_data, complete)};

wallet/refactor: change PSBTError to PSBTResult and remove std::optional<common::PSBTResult> and return common::PSBTResult #32958

Are you sure you want to change the base?

wallet/refactor: change PSBTError to PSBTResult and remove std::optional<common::PSBTResult> and return common::PSBTResult #32958

Uh oh!

Conversation

kevkevinpal commented Jul 13, 2025

Description

What this changes

Uh oh!

DrahtBot commented Jul 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)

Uh oh!

DrahtBot commented Jul 13, 2025

Uh oh!

naiyoma left a comment

Choose a reason for hiding this comment

Uh oh!

naiyoma Aug 11, 2025

Choose a reason for hiding this comment

Uh oh!

kevkevinpal Aug 11, 2025

Choose a reason for hiding this comment

Uh oh!

naiyoma left a comment

Choose a reason for hiding this comment

Uh oh!

kevkevinpal commented Oct 24, 2025

Uh oh!

TumaBitcoiner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Nov 21, 2025

Uh oh!

kevkevinpal commented Dec 31, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DrahtBot commented Jul 13, 2025 •

edited

Loading