wallet/refactor: Update SignPSBTInput to return util::Expected<void, PSBTError> and remove PSBTError:Ok #32958

pull kevkevinpal wants to merge 2 commits into bitcoin:master from kevkevinpal:PSBTErrorToPSBTResult changing 7 files +28 −25
  1. kevkevinpal commented at 2:06 PM on July 13, 2025: contributor

    Description

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

    What this changes

    • Updates SignPSBTInput to return util::Expected<void, PSBTError>
    • Removes PSBTError:Ok from the PSBTError Enum
  2. DrahtBot commented at 2:06 PM on July 13, 2025: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK rkrux
    Concept ACK naiyoma

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #21283 (Implement BIP 370 PSBTv2 by achow101)

    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.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  3. kevkevinpal force-pushed on Jul 13, 2025
  4. DrahtBot added the label CI failed on Jul 13, 2025
  5. DrahtBot commented at 2:12 PM on July 13, 2025: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

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

    <details><summary>Hints</summary>

    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.

    </details>

  6. kevkevinpal force-pushed on Jul 13, 2025
  7. kevkevinpal force-pushed on Jul 13, 2025
  8. kevkevinpal force-pushed on Jul 13, 2025
  9. DrahtBot removed the label CI failed on Jul 13, 2025
  10. naiyoma commented at 5:51 PM on August 11, 2025: contributor

    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.

  11. in src/qt/psbtoperationsdialog.cpp:62 in 595909674e outdated 
      61 | @@ -62,7 +62,7 @@ void PSBTOperationsDialog::openWithPSBT(PartiallySignedTransaction psbtx)
  62 |          const auto err{m_wallet_model->wallet().fillPSBT(std::nullopt, /*sign=*/false, /*bip32derivs=*/true, &n_could_sign, m_transaction_data, complete)};
    naiyoma commented at 6:06 PM on August 11, 2025:

    nit: rename err -> result

    kevkevinpal commented at 7:05 PM on August 11, 2025:

    Thanks for the review! pushed an update in d242c75c5c0eb7d5515cc99fda39ff98a6fdd5af

  12. kevkevinpal force-pushed on Aug 11, 2025
  13. naiyoma commented at 8:42 PM on August 13, 2025: contributor

    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.

  14. DrahtBot added the label Needs rebase on Oct 24, 2025
  15. kevkevinpal force-pushed on Oct 24, 2025
  16. kevkevinpal commented at 8:46 PM on October 24, 2025: contributor

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

  17. DrahtBot removed the label Needs rebase on Oct 24, 2025
  18. kevkevinpal requested review from naiyoma on Oct 26, 2025
  19. in src/qt/sendcoinsdialog.cpp:510 in 86d9a82cc8 
     508 | @@ -509,7 +509,7 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked)
 509 |          // Fill without signing
 510 |          const auto err{model->wallet().fillPSBT(std::nullopt, /*sign=*/false, /*bip32derivs=*/true, /*n_signed=*/nullptr, psbtx, complete)};
    TumaBitcoiner commented at 3:59 PM on November 21, 2025: 
            const auto result{model->wallet().fillPSBT(std::nullopt, /*sign=*/false, /*bip32derivs=*/true, /*n_signed=*/nullptr, psbtx, complete)};

    Should this be defined as result too, as you did in other parts of the code?

    kevkevinpal commented at 5:26 PM on November 21, 2025:

    Thanks, updated in 4e038c4b7a6fbfaafe1f2570592e2bb751f9be09

  20. in src/qt/sendcoinsdialog.cpp:512 in 86d9a82cc8 
     508 | @@ -509,7 +509,7 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked)
 509 |          // Fill without signing
 510 |          const auto err{model->wallet().fillPSBT(std::nullopt, /*sign=*/false, /*bip32derivs=*/true, /*n_signed=*/nullptr, psbtx, complete)};
 511 |          assert(!complete);
 512 | -        assert(!err);
 513 | +        assert(err == PSBTResult::OK);
    TumaBitcoiner commented at 3:59 PM on November 21, 2025: 
            assert(result== PSBTResult::OK);

    As above.

    kevkevinpal commented at 5:26 PM on November 21, 2025:

    Thanks, updated in 4e038c4b7a6fbfaafe1f2570592e2bb751f9be09

  21. in src/qt/sendcoinsdialog.cpp:526 in 86d9a82cc8 
     524 | @@ -525,7 +525,7 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked)
 525 |              // from being called prematurely and is not expensive.
 526 |              const auto err{model->wallet().fillPSBT(std::nullopt, /*sign=*/false, /*bip32derivs=*/true, /*n_signed=*/nullptr, psbtx, complete)};
    TumaBitcoiner commented at 4:03 PM on November 21, 2025: 
                const auto result{model->wallet().fillPSBT(std::nullopt, /*sign=*/false, /*bip32derivs=*/true, /*n_signed=*/nullptr, psbtx, complete)};
    kevkevinpal commented at 5:26 PM on November 21, 2025:

    Thanks, updated in 4e038c4b7a6fbfaafe1f2570592e2bb751f9be09

  22. in src/qt/sendcoinsdialog.cpp:528 in 86d9a82cc8 
     524 | @@ -525,7 +525,7 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked)
 525 |              // from being called prematurely and is not expensive.
 526 |              const auto err{model->wallet().fillPSBT(std::nullopt, /*sign=*/false, /*bip32derivs=*/true, /*n_signed=*/nullptr, psbtx, complete)};
 527 |              assert(!complete);
 528 | -            assert(!err);
 529 | +            assert(err == PSBTResult::OK);
    TumaBitcoiner commented at 4:03 PM on November 21, 2025: 
                assert(result== PSBTResult::OK);
    kevkevinpal commented at 5:26 PM on November 21, 2025:

    Thanks, updated in 4e038c4b7a6fbfaafe1f2570592e2bb751f9be09

  23. in src/qt/walletmodel.cpp:523 in 86d9a82cc8 
     520 | @@ -521,7 +521,7 @@ bool WalletModel::bumpFee(Txid hash, Txid& new_hash)
 521 |          PartiallySignedTransaction psbtx(mtx);
 522 |          bool complete = false;
 523 |          const auto err{wallet().fillPSBT(std::nullopt, /*sign=*/false, /*bip32derivs=*/true, nullptr, psbtx, complete)};
    TumaBitcoiner commented at 4:03 PM on November 21, 2025: 
            const auto result{wallet().fillPSBT(std::nullopt, /*sign=*/false, /*bip32derivs=*/true, nullptr, psbtx, complete)};
    kevkevinpal commented at 5:26 PM on November 21, 2025:

    Thanks, updated in 4e038c4b7a6fbfaafe1f2570592e2bb751f9be09

  24. in src/qt/walletmodel.cpp:524 in 86d9a82cc8 
     520 | @@ -521,7 +521,7 @@ bool WalletModel::bumpFee(Txid hash, Txid& new_hash)
 521 |          PartiallySignedTransaction psbtx(mtx);
 522 |          bool complete = false;
 523 |          const auto err{wallet().fillPSBT(std::nullopt, /*sign=*/false, /*bip32derivs=*/true, nullptr, psbtx, complete)};
 524 | -        if (err || complete) {
 525 | +        if (err != PSBTResult::OK || complete) {
    TumaBitcoiner commented at 4:04 PM on November 21, 2025: 
            if (result != PSBTResult::OK || complete) {
    kevkevinpal commented at 5:26 PM on November 21, 2025:

    Thanks, updated in 4e038c4b7a6fbfaafe1f2570592e2bb751f9be09

  25. in src/wallet/feebumper.cpp:340 in 86d9a82cc8 
     337 | @@ -338,7 +338,7 @@ bool SignTransaction(CWallet& wallet, CMutableTransaction& mtx) {
 338 |          bool complete;
 339 |          wallet.FillPSBT(psbtx, complete, std::nullopt, /*sign=*/false, /*bip32derivs=*/true);
 340 |          auto err{wallet.FillPSBT(psbtx, complete, std::nullopt, /*sign=*/true, /*bip32derivs=*/false)};
    TumaBitcoiner commented at 4:07 PM on November 21, 2025: 
            auto result{wallet.FillPSBT(psbtx, complete, std::nullopt, /*sign=*/true, /*bip32derivs=*/false)};
    kevkevinpal commented at 5:26 PM on November 21, 2025:

    Thanks, updated in 4e038c4b7a6fbfaafe1f2570592e2bb751f9be09

  26. in src/wallet/feebumper.cpp:341 in 86d9a82cc8 
     337 | @@ -338,7 +338,7 @@ bool SignTransaction(CWallet& wallet, CMutableTransaction& mtx) {
 338 |          bool complete;
 339 |          wallet.FillPSBT(psbtx, complete, std::nullopt, /*sign=*/false, /*bip32derivs=*/true);
 340 |          auto err{wallet.FillPSBT(psbtx, complete, std::nullopt, /*sign=*/true, /*bip32derivs=*/false)};
 341 | -        if (err) return false;
 342 | +        if (err != PSBTResult::OK) return false;
    TumaBitcoiner commented at 4:07 PM on November 21, 2025: 
            if (result != PSBTResult::OK) return false;
    kevkevinpal commented at 5:26 PM on November 21, 2025:

    Thanks, updated in 4e038c4b7a6fbfaafe1f2570592e2bb751f9be09

  27. in src/wallet/rpc/spend.cpp:118 in 86d9a82cc8 
     115 | @@ -116,8 +116,8 @@ static UniValue FinishTransaction(const std::shared_ptr<CWallet> pwallet, const
 116 |      bool complete;
 117 |      pwallet->FillPSBT(psbtx, complete, std::nullopt, /*sign=*/false, /*bip32derivs=*/true);
 118 |      const auto err{pwallet->FillPSBT(psbtx, complete, std::nullopt, /*sign=*/true, /*bip32derivs=*/false)};
    TumaBitcoiner commented at 4:08 PM on November 21, 2025: 
        const auto result{pwallet->FillPSBT(psbtx, complete, std::nullopt, /*sign=*/true, /*bip32derivs=*/false)};
    kevkevinpal commented at 5:26 PM on November 21, 2025:

    Thanks, updated in 4e038c4b7a6fbfaafe1f2570592e2bb751f9be09

  28. in src/wallet/rpc/spend.cpp:1193 in 86d9a82cc8 
    1190 | @@ -1191,7 +1191,7 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
1191 |          PartiallySignedTransaction psbtx(mtx);
1192 |          bool complete = false;
1193 |          const auto err{pwallet->FillPSBT(psbtx, complete, std::nullopt, /*sign=*/false, /*bip32derivs=*/true)};
    TumaBitcoiner commented at 4:09 PM on November 21, 2025: 
            const auto result{pwallet->FillPSBT(psbtx, complete, std::nullopt, /*sign=*/false, /*bip32derivs=*/true)};
    kevkevinpal commented at 5:26 PM on November 21, 2025:

    Thanks, updated in 4e038c4b7a6fbfaafe1f2570592e2bb751f9be09

  29. in src/wallet/rpc/spend.cpp:1194 in 86d9a82cc8 
    1190 | @@ -1191,7 +1191,7 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
1191 |          PartiallySignedTransaction psbtx(mtx);
1192 |          bool complete = false;
1193 |          const auto err{pwallet->FillPSBT(psbtx, complete, std::nullopt, /*sign=*/false, /*bip32derivs=*/true)};
1194 | -        CHECK_NONFATAL(!err);
1195 | +        CHECK_NONFATAL(err == PSBTResult::OK);
    TumaBitcoiner commented at 4:09 PM on November 21, 2025: 
            CHECK_NONFATAL(result == PSBTResult::OK);
    kevkevinpal commented at 5:26 PM on November 21, 2025:

    Thanks, updated in 4e038c4b7a6fbfaafe1f2570592e2bb751f9be09

  30. in src/wallet/rpc/spend.cpp:1681 in 86d9a82cc8 
    1678 | @@ -1679,8 +1679,8 @@ RPCHelpMan walletprocesspsbt()
1679 |      if (sign) EnsureWalletIsUnlocked(*pwallet);
1680 |  
1681 |      const auto err{wallet.FillPSBT(psbtx, complete, nHashType, sign, bip32derivs, nullptr, finalize)};
    TumaBitcoiner commented at 4:09 PM on November 21, 2025: 
        const auto result{wallet.FillPSBT(psbtx, complete, nHashType, sign, bip32derivs, nullptr, finalize)};
    kevkevinpal commented at 5:26 PM on November 21, 2025:

    Thanks, updated in 4e038c4b7a6fbfaafe1f2570592e2bb751f9be09

  31. in src/wallet/rpc/spend.cpp:119 in 86d9a82cc8 
     115 | @@ -116,8 +116,8 @@ static UniValue FinishTransaction(const std::shared_ptr<CWallet> pwallet, const
 116 |      bool complete;
 117 |      pwallet->FillPSBT(psbtx, complete, std::nullopt, /*sign=*/false, /*bip32derivs=*/true);
 118 |      const auto err{pwallet->FillPSBT(psbtx, complete, std::nullopt, /*sign=*/true, /*bip32derivs=*/false)};
 119 | -    if (err) {
 120 | -        throw JSONRPCPSBTError(*err);
 121 | +    if (err != PSBTResult::OK) {
    TumaBitcoiner commented at 4:12 PM on November 21, 2025: 
        if (result != PSBTResult::OK) {
    kevkevinpal commented at 5:26 PM on November 21, 2025:

    Thanks, updated in 4e038c4b7a6fbfaafe1f2570592e2bb751f9be09

  32. in src/wallet/rpc/spend.cpp:120 in 86d9a82cc8 
     115 | @@ -116,8 +116,8 @@ static UniValue FinishTransaction(const std::shared_ptr<CWallet> pwallet, const
 116 |      bool complete;
 117 |      pwallet->FillPSBT(psbtx, complete, std::nullopt, /*sign=*/false, /*bip32derivs=*/true);
 118 |      const auto err{pwallet->FillPSBT(psbtx, complete, std::nullopt, /*sign=*/true, /*bip32derivs=*/false)};
 119 | -    if (err) {
 120 | -        throw JSONRPCPSBTError(*err);
 121 | +    if (err != PSBTResult::OK) {
 122 | +        throw JSONRPCPSBTError(err);
    TumaBitcoiner commented at 4:12 PM on November 21, 2025: 
            throw JSONRPCPSBTError(result);
    kevkevinpal commented at 5:26 PM on November 21, 2025:

    Thanks, updated in 4e038c4b7a6fbfaafe1f2570592e2bb751f9be09

  33. in src/wallet/rpc/spend.cpp:1682 in 86d9a82cc8 
    1678 | @@ -1679,8 +1679,8 @@ RPCHelpMan walletprocesspsbt()
1679 |      if (sign) EnsureWalletIsUnlocked(*pwallet);
1680 |  
1681 |      const auto err{wallet.FillPSBT(psbtx, complete, nHashType, sign, bip32derivs, nullptr, finalize)};
1682 | -    if (err) {
1683 | -        throw JSONRPCPSBTError(*err);
1684 | +    if (err != PSBTResult::OK) {
    TumaBitcoiner commented at 4:12 PM on November 21, 2025: 
        if (result != PSBTResult::OK) {
    kevkevinpal commented at 5:26 PM on November 21, 2025:

    Thanks, updated in 4e038c4b7a6fbfaafe1f2570592e2bb751f9be09

  34. in src/wallet/rpc/spend.cpp:1683 in 86d9a82cc8 
    1678 | @@ -1679,8 +1679,8 @@ RPCHelpMan walletprocesspsbt()
1679 |      if (sign) EnsureWalletIsUnlocked(*pwallet);
1680 |  
1681 |      const auto err{wallet.FillPSBT(psbtx, complete, nHashType, sign, bip32derivs, nullptr, finalize)};
1682 | -    if (err) {
1683 | -        throw JSONRPCPSBTError(*err);
1684 | +    if (err != PSBTResult::OK) {
1685 | +        throw JSONRPCPSBTError(err);
    TumaBitcoiner commented at 4:13 PM on November 21, 2025: 
            throw JSONRPCPSBTError(result);
    kevkevinpal commented at 5:26 PM on November 21, 2025:

    Thanks, updated in 4e038c4b7a6fbfaafe1f2570592e2bb751f9be09

  35. in src/wallet/rpc/spend.cpp:1823 in 86d9a82cc8 
    1820 | @@ -1821,8 +1821,8 @@ RPCHelpMan walletcreatefundedpsbt()
1821 |      bool bip32derivs = request.params[4].isNull() ? true : request.params[4].get_bool();
1822 |      bool complete = true;
1823 |      const auto err{wallet.FillPSBT(psbtx, complete, std::nullopt, /*sign=*/false, /*bip32derivs=*/bip32derivs)};
    TumaBitcoiner commented at 4:13 PM on November 21, 2025: 
        const auto result{wallet.FillPSBT(psbtx, complete, std::nullopt, /*sign=*/false, /*bip32derivs=*/bip32derivs)};
    kevkevinpal commented at 5:26 PM on November 21, 2025:

    Thanks, updated in 4e038c4b7a6fbfaafe1f2570592e2bb751f9be09

  36. in src/wallet/rpc/spend.cpp:1824 in 86d9a82cc8 
    1820 | @@ -1821,8 +1821,8 @@ RPCHelpMan walletcreatefundedpsbt()
1821 |      bool bip32derivs = request.params[4].isNull() ? true : request.params[4].get_bool();
1822 |      bool complete = true;
1823 |      const auto err{wallet.FillPSBT(psbtx, complete, std::nullopt, /*sign=*/false, /*bip32derivs=*/bip32derivs)};
1824 | -    if (err) {
1825 | -        throw JSONRPCPSBTError(*err);
1826 | +    if (err != PSBTResult::OK) {
    TumaBitcoiner commented at 4:13 PM on November 21, 2025: 
        if (result != PSBTResult::OK) {
    kevkevinpal commented at 5:25 PM on November 21, 2025:

    Thanks, updated in 4e038c4b7a6fbfaafe1f2570592e2bb751f9be09

  37. in src/wallet/rpc/spend.cpp:1825 in 86d9a82cc8 
    1820 | @@ -1821,8 +1821,8 @@ RPCHelpMan walletcreatefundedpsbt()
1821 |      bool bip32derivs = request.params[4].isNull() ? true : request.params[4].get_bool();
1822 |      bool complete = true;
1823 |      const auto err{wallet.FillPSBT(psbtx, complete, std::nullopt, /*sign=*/false, /*bip32derivs=*/bip32derivs)};
1824 | -    if (err) {
1825 | -        throw JSONRPCPSBTError(*err);
1826 | +    if (err != PSBTResult::OK) {
1827 | +        throw JSONRPCPSBTError(err);
    TumaBitcoiner commented at 4:13 PM on November 21, 2025: 
            throw JSONRPCPSBTError(result);
    kevkevinpal commented at 5:25 PM on November 21, 2025:

    Thanks, updated in 4e038c4b7a6fbfaafe1f2570592e2bb751f9be09

  38. in src/wallet/wallet.cpp:2159 in 86d9a82cc8 
    2156 | @@ -2157,7 +2157,7 @@ std::optional<PSBTError> CWallet::FillPSBT(PartiallySignedTransaction& psbtx, bo
2157 |      for (ScriptPubKeyMan* spk_man : GetAllScriptPubKeyMans()) {
2158 |          int n_signed_this_spkm = 0;
2159 |          const auto error{spk_man->FillPSBT(psbtx, txdata, sighash_type, sign, bip32derivs, &n_signed_this_spkm, finalize)};
    TumaBitcoiner commented at 4:15 PM on November 21, 2025: 
            const auto result{spk_man->FillPSBT(psbtx, txdata, sighash_type, sign, bip32derivs, &n_signed_this_spkm, finalize)};
    kevkevinpal commented at 5:25 PM on November 21, 2025:

    Thanks, updated in 4e038c4b7a6fbfaafe1f2570592e2bb751f9be09

  39. in src/wallet/wallet.cpp:2160 in 86d9a82cc8 
    2156 | @@ -2157,7 +2157,7 @@ std::optional<PSBTError> CWallet::FillPSBT(PartiallySignedTransaction& psbtx, bo
2157 |      for (ScriptPubKeyMan* spk_man : GetAllScriptPubKeyMans()) {
2158 |          int n_signed_this_spkm = 0;
2159 |          const auto error{spk_man->FillPSBT(psbtx, txdata, sighash_type, sign, bip32derivs, &n_signed_this_spkm, finalize)};
2160 | -        if (error) {
2161 | +        if (error != PSBTResult::OK) {
    TumaBitcoiner commented at 4:15 PM on November 21, 2025: 
            if (result != PSBTResult::OK) {
    kevkevinpal commented at 5:25 PM on November 21, 2025:

    Thanks, updated in 4e038c4b7a6fbfaafe1f2570592e2bb751f9be09

  40. in src/wallet/wallet.cpp:2161 in 86d9a82cc8 
    2156 | @@ -2157,7 +2157,7 @@ std::optional<PSBTError> CWallet::FillPSBT(PartiallySignedTransaction& psbtx, bo
2157 |      for (ScriptPubKeyMan* spk_man : GetAllScriptPubKeyMans()) {
2158 |          int n_signed_this_spkm = 0;
2159 |          const auto error{spk_man->FillPSBT(psbtx, txdata, sighash_type, sign, bip32derivs, &n_signed_this_spkm, finalize)};
2160 | -        if (error) {
2161 | +        if (error != PSBTResult::OK) {
2162 |              return error;
    TumaBitcoiner commented at 4:15 PM on November 21, 2025: 
                return result;
    kevkevinpal commented at 5:25 PM on November 21, 2025:

    Thanks, updated in 4e038c4b7a6fbfaafe1f2570592e2bb751f9be09

  41. TumaBitcoiner commented at 4:17 PM on November 21, 2025: none

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

  42. kevkevinpal force-pushed on Nov 21, 2025
  43. DrahtBot added the label CI failed on Nov 21, 2025
  44. DrahtBot commented at 5:36 PM on November 21, 2025: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task Windows-cross to x86_64: https://github.com/bitcoin/bitcoin/actions/runs/19578345228/job/56069612685</sub> <sub>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.</sub>

    <details><summary>Hints</summary>

    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.

    </details>

  45. kevkevinpal force-pushed on Nov 21, 2025
  46. kevkevinpal force-pushed on Nov 21, 2025
  47. DrahtBot removed the label CI failed on Nov 21, 2025
  48. kevkevinpal force-pushed on Dec 31, 2025
  49. kevkevinpal commented at 7:22 PM on December 31, 2025: contributor

    rebased to dd51b84

  50. DrahtBot added the label Needs rebase on Jan 19, 2026
  51. kevkevinpal force-pushed on Mar 13, 2026
  52. kevkevinpal commented at 7:54 PM on March 13, 2026: contributor

    Rebased to 4e92ac8

  53. DrahtBot added the label CI failed on Mar 13, 2026
  54. DrahtBot removed the label Needs rebase on Mar 13, 2026
  55. DrahtBot commented at 2:31 PM on March 18, 2026: contributor

    Could turn into draft while CI is red?

  56. kevkevinpal force-pushed on Mar 18, 2026
  57. kevkevinpal force-pushed on Mar 18, 2026
  58. kevkevinpal force-pushed on Mar 18, 2026
  59. kevkevinpal force-pushed on Mar 18, 2026
  60. DrahtBot removed the label CI failed on Mar 18, 2026
  61. DrahtBot added the label Needs rebase on Mar 25, 2026
  62. kevkevinpal force-pushed on Apr 1, 2026
  63. kevkevinpal commented at 3:37 PM on April 1, 2026: contributor

    Rebased to f6a5a43

  64. DrahtBot removed the label Needs rebase on Apr 1, 2026
  65. sedited removed review request from naiyoma on Apr 18, 2026
  66. sedited requested review from rkrux on Apr 18, 2026
  67. sedited requested review from naiyoma on Apr 18, 2026
  68. in src/common/messages.cpp:104 in cbea448eb6 
     100 | @@ -101,22 +101,22 @@ bool FeeModeFromString(std::string_view mode_string, FeeEstimateMode& fee_estima
 101 |      return false;
 102 |  }
 103 |  
 104 | -bilingual_str PSBTErrorString(PSBTError err)
 105 | +bilingual_str PSBTErrorString(PSBTResult result)
    rkrux commented at 11:04 AM on April 20, 2026:

    Comment about the commit cbea448eb64c7aedfbbb3de9c786bf9a2b496ba6: Many (18-19) files are being updated here and a scripted diff should be used to rename PSBTError to PSBTResult.

    In order to address the err variable name, it can be renamed to result in a preceding commit before the scripted diff one. Reading PSBTError result in one commit in 4 files is still okay I guess because an error can be thought of as a subset of a result.

    kevkevinpal commented at 3:36 PM on April 20, 2026:

    Thanks for the review! Updated to ec04253

    That makes sense, I moved all the err -> result changes to the first commit 6efcc85 and then the following commit 4f24d3a is a scripted diff to make PSBTError -> PSBTResult.

    Do you think it makes sense for me to do this for PSBTErrorString and RPCErrorFromPSBTError aswell?

  69. in src/qt/sendcoinsdialog.cpp:509 in d5e6cc23ff 
     505 | @@ -506,7 +506,7 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked)
 506 |          // Fill without signing
 507 |          const auto err{model->wallet().fillPSBT(std::nullopt, /*sign=*/false, /*bip32derivs=*/true, /*n_signed=*/nullptr, psbtx, complete)};
 508 |          assert(!complete);
 509 | -        assert(!err);
 510 | +        assert(err == PSBTResult::OK);
    rkrux commented at 11:08 AM on April 20, 2026:

    In d5e6cc23ff2d289583d38a40604944de1bd8960f: This commit updates few err to result (like in src/qt/psbtoperationsdialog.cpp file) and rest are done in the following commit f6a5a434d196890371fb12b454cefaf4a2fe8217. I am unable to see how this partial split is beneficial, update all of them in one commit instead?

    kevkevinpal commented at 3:36 PM on April 20, 2026:

    Yea that makes sense I updated this to move all the err to result changes in the first commit

  70. rkrux commented at 11:17 AM on April 20, 2026: contributor

    ~Concept A-C-K~ f6a5a434d196890371fb12b454cefaf4a2fe8217 but the commit structure can be altered a bit.

    Edit: Retracting a-c-k because of alternate approach suggested in #32958 (comment).

  71. kevkevinpal force-pushed on Apr 20, 2026
  72. kevkevinpal force-pushed on Apr 20, 2026
  73. DrahtBot added the label CI failed on Apr 20, 2026
  74. DrahtBot commented at 3:35 PM on April 20, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/24675190871/job/72157018860</sub> <sub>LLM reason (✨ experimental): CI failed due to a C++ build error: psbtoperationsdialog.cpp uses an undeclared identifier err (compiler error).</sub>

    <details><summary>Hints</summary>

    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.

    </details>

  75. kevkevinpal force-pushed on Apr 20, 2026
  76. DrahtBot removed the label CI failed on Apr 20, 2026
  77. ryanofsky commented at 2:35 PM on April 21, 2026: contributor
    • Updates PSBTError to PSBTResult because we now have PSBTResult::OK

    I haven't looked at earlier discussion but curious why combining success & failure codes into one enum type is good. I'd think the opposite would be better: having separate types for success and failures would be more compatible with util::Expected, make mapping errors into strings more straightforward, and provide more compile-time safety in general. Anything seems fine, but it might be better to drop the PSBTResult::OK value here and use util::Expected instead.

  78. rkrux commented at 2:56 PM on April 21, 2026: contributor

    I haven't looked at earlier discussion but curious why combining success & failure codes into one enum type is good.

    This thought did cross my mind initially when I reviewed this PR yesterday but I guess I didn't look further into #31622.

    I'd think the opposite would be better: having separate types for success and failures would be more compatible with util::Expected

    I think this is a cleaner approach. Last year when the older PR was merged, util::Expected didn't exist but now that it does, I too think it's better updating the return value of SignPSBTInput and removing PSBTError::OK.

    Though this would be a tangential approach to the one in this PR, I feel it would be worth it.

  79. kevkevinpal commented at 11:45 AM on April 22, 2026: contributor

    I think this is a cleaner approach. Last year when the older PR was merged, util::Expected didn't exist but now that it does, I too think it's better updating the return value of SignPSBTInput and removing PSBTError::OK.

    Yeah, this makes sense. I can either update this PR or open another one to update SignPSBTInput and remove PSBTError::OK, so that we can use util::Expected instead.

    I'll also take the same approach for https://github.com/bitcoin/bitcoin/pull/35105

  80. rkrux commented at 11:54 AM on April 22, 2026: contributor

    I can either update this PR or open another one to update

    Updating this PR should be fine I suppose.

  81. kevkevinpal force-pushed on Apr 22, 2026
  82. kevkevinpal renamed this:
    wallet/refactor: change PSBTError to PSBTResult and remove std::optional<common::PSBTResult> and return common::PSBTResult
    wallet/refactor: Update SignPSBTInput to return util::Expected<void, PSBTError> and remove PSBTError:Ok
    on Apr 22, 2026
  83. kevkevinpal commented at 8:52 PM on April 22, 2026: contributor

    Updating this PR should be fine I suppose

    I pushed b5b12f8 and c72a8ff

    SignPSBT is now using util::Expected and we also removed PSBTError::Ok

    Let me know if this approach looks good and I can attempt to do something similar for https://github.com/bitcoin/bitcoin/pull/35105

  84. in src/wallet/scriptpubkeyman.cpp:1380 in b5b12f82cf 
    1375 | @@ -1376,9 +1376,9 @@ std::optional<PSBTError> DescriptorScriptPubKeyMan::FillPSBT(PartiallySignedTran
1376 |              }
1377 |          }
1378 |  
1379 | -        PSBTError res = SignPSBTInput(HidingSigningProvider(keys.get(), /*hide_secret=*/!sign, /*hide_origin=*/!bip32derivs), psbtx, i, &txdata, sighash_type, nullptr, finalize);
1380 | -        if (res != PSBTError::OK && res != PSBTError::INCOMPLETE) {
1381 | -            return res;
1382 | +        const auto sign_result = SignPSBTInput(HidingSigningProvider(keys.get(), /*hide_secret=*/!sign, /*hide_origin=*/!bip32derivs), psbtx, i, &txdata, sighash_type, nullptr, finalize);
1383 | +        if (!sign_result && sign_result.error() != PSBTError::INCOMPLETE) {
    rkrux commented at 2:24 PM on April 23, 2026: 
    - if (!sign_result && sign_result.error() != PSBTError::INCOMPLETE) {
+ if (!sign_result.has_value() && sign_result.error() != PSBTError::INCOMPLETE) {
    kevkevinpal commented at 4:18 PM on April 23, 2026:

    Thanks for the review!

    Updated in 10f15a5

  85. in src/rpc/rawtransaction.cpp:202 in b5b12f82cf 
     197 | @@ -198,7 +198,8 @@ PartiallySignedTransaction ProcessPSBT(const std::string& psbt_string, const std
 198 |          // We only actually care about those if our signing provider doesn't hide private
 199 |          // information, as is the case with `descriptorprocesspsbt`
 200 |          // Only error for mismatching sighash types as it is critical that the sighash to sign with matches the PSBT's
 201 | -        if (SignPSBTInput(provider, psbtx, /*index=*/i, &txdata, sighash_type, /*out_sigdata=*/nullptr, finalize) == common::PSBTError::SIGHASH_MISMATCH) {
 202 | +        const auto sign_result = SignPSBTInput(provider, psbtx, /*index=*/i, &txdata, sighash_type, /*out_sigdata=*/nullptr, finalize);
 203 | +        if (!sign_result && sign_result.error() == common::PSBTError::SIGHASH_MISMATCH) {
    rkrux commented at 2:25 PM on April 23, 2026: 
    - if (!sign_result && sign_result.error() == common::PSBTError::SIGHASH_MISMATCH) {
+ if (!sign_result.has_value() && sign_result.error() == common::PSBTError::SIGHASH_MISMATCH) {
    kevkevinpal commented at 4:18 PM on April 23, 2026:

    Thanks for the review!

    Updated in 10f15a5

  86. rkrux commented at 2:34 PM on April 23, 2026: contributor

    Let me know if this approach looks good

    This looks quite good to me, the diff is also shorter. Thanks @ryanofsky for this suggestion.

  87. kevkevinpal force-pushed on Apr 23, 2026
  88. DrahtBot added the label CI failed on Apr 23, 2026
  89. DrahtBot removed the label CI failed on Apr 23, 2026
  90. DrahtBot added the label Needs rebase on Apr 29, 2026
  91. kevkevinpal force-pushed on Apr 30, 2026
  92. kevkevinpal commented at 2:35 PM on April 30, 2026: contributor

    rebased and pushed 9514af4

  93. DrahtBot removed the label Needs rebase on Apr 30, 2026
  94. DrahtBot added the label Needs rebase on May 5, 2026
  95. refactor: SignPSBTInput now uses util:Expected 3660678b95
  96. refactor: remove unused PSBTError::Ok 6cca38e2b9
  97. kevkevinpal force-pushed on May 5, 2026
  98. kevkevinpal commented at 4:36 PM on May 5, 2026: contributor

    rebased again to 6cca38e

  99. DrahtBot removed the label Needs rebase on May 5, 2026
  100. in src/node/psbt.cpp:75 in 3660678b95 
      71 | @@ -72,7 +72,8 @@ PSBTAnalysis AnalyzePSBT(PartiallySignedTransaction psbtx)
  72 |  
  73 |              // Figure out what is missing
  74 |              SignatureData outdata;
  75 | -            bool complete = SignPSBTInput(DUMMY_SIGNING_PROVIDER, psbtx, i, &txdata, /*options=*/{}, &outdata) == PSBTError::OK;
  76 | +            const auto sign_result = SignPSBTInput(DUMMY_SIGNING_PROVIDER, psbtx, i, &txdata, /*options*/{}, &outdata);
    rkrux commented at 1:28 PM on May 6, 2026:

    In 3660678b953c7de8c1d2c670296df90649630f5d "refactor: SignPSBTInput now uses util:Expected"

    This is the only place a non-null outdata is passed in the last argument and the only values needed from it are missing_* ones. I think the following diff can make it easier to parse but it's out of scope of this PR.

    <details> <summary>MissingSignatureData struct extraction</summary>

    diff --git a/src/node/psbt.cpp b/src/node/psbt.cpp
index b0988d8682..894c40230f 100644
--- a/src/node/psbt.cpp
+++ b/src/node/psbt.cpp
@@ -71,19 +71,19 @@ PSBTAnalysis AnalyzePSBT(PartiallySignedTransaction psbtx)
             input_analysis.is_final = false;
 
             // Figure out what is missing
-            SignatureData outdata;
-            const auto sign_result = SignPSBTInput(DUMMY_SIGNING_PROVIDER, psbtx, i, &txdata, /*options*/{}, &outdata);
+            MissingSignatureData out_missing_data;
+            const auto sign_result = SignPSBTInput(DUMMY_SIGNING_PROVIDER, psbtx, i, &txdata, /*options*/{}, &out_missing_data);
             bool complete = sign_result.has_value();
 
             // Things are missing
             if (!complete) {
-                input_analysis.missing_pubkeys = outdata.missing_pubkeys;
-                input_analysis.missing_redeem_script = outdata.missing_redeem_script;
-                input_analysis.missing_witness_script = outdata.missing_witness_script;
-                input_analysis.missing_sigs = outdata.missing_sigs;
+                input_analysis.missing_pubkeys = out_missing_data.missing_pubkeys;
+                input_analysis.missing_redeem_script = out_missing_data.missing_redeem_script;
+                input_analysis.missing_witness_script = out_missing_data.missing_witness_script;
+                input_analysis.missing_sigs = out_missing_data.missing_sigs;
 
                 // If we are only missing signatures and nothing else, then next is signer
-                if (outdata.missing_pubkeys.empty() && outdata.missing_redeem_script.IsNull() && outdata.missing_witness_script.IsNull() && !outdata.missing_sigs.empty()) {
+                if (out_missing_data.missing_pubkeys.empty() && out_missing_data.missing_redeem_script.IsNull() && out_missing_data.missing_witness_script.IsNull() && !out_missing_data.missing_sigs.empty()) {
                     input_analysis.next = PSBTRole::SIGNER;
                 } else {
                     input_analysis.next = PSBTRole::UPDATER;
diff --git a/src/psbt.cpp b/src/psbt.cpp
index 994a875bb5..1604013f18 100644
--- a/src/psbt.cpp
+++ b/src/psbt.cpp
@@ -641,7 +641,7 @@ std::optional<PrecomputedTransactionData> PrecomputePSBTData(const PartiallySign
     return txdata;
 }
 
-util::Expected<void, PSBTError> SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index, const PrecomputedTransactionData* txdata, const common::PSBTFillOptions& options,  SignatureData* out_sigdata)
+util::Expected<void, PSBTError> SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index, const PrecomputedTransactionData* txdata, const common::PSBTFillOptions& options,  MissingSignatureData* out_missing_data)
 {
     PSBTInput& input = psbt.inputs.at(index);
     std::optional<CMutableTransaction> unsigned_tx = psbt.GetUnsignedTx();
@@ -747,11 +747,11 @@ util::Expected<void, PSBTError> SignPSBTInput(const SigningProvider& provider, P
     }
 
     // Fill in the missing info
-    if (out_sigdata) {
-        out_sigdata->missing_pubkeys = sigdata.missing_pubkeys;
-        out_sigdata->missing_sigs = sigdata.missing_sigs;
-        out_sigdata->missing_redeem_script = sigdata.missing_redeem_script;
-        out_sigdata->missing_witness_script = sigdata.missing_witness_script;
+    if (out_missing_data) {
+        out_missing_data->missing_pubkeys = sigdata.missing_pubkeys;
+        out_missing_data->missing_sigs = sigdata.missing_sigs;
+        out_missing_data->missing_redeem_script = sigdata.missing_redeem_script;
+        out_missing_data->missing_witness_script = sigdata.missing_witness_script;
     }
 
     if (!sig_complete) return util::Unexpected{PSBTError::INCOMPLETE};
diff --git a/src/psbt.h b/src/psbt.h
index 266f23dce3..531115c7c8 100644
--- a/src/psbt.h
+++ b/src/psbt.h
@@ -1646,7 +1646,7 @@ bool PSBTInputSignedAndVerified(const PartiallySignedTransaction& psbt, unsigned
  * txdata should be the output of PrecomputePSBTData (which can be shared across
  * multiple SignPSBTInput calls). If it is nullptr, a dummy signature will be created.
  **/
-[[nodiscard]] util::Expected<void, PSBTError> SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index, const PrecomputedTransactionData* txdata, const common::PSBTFillOptions& options, SignatureData* out_sigdata = nullptr);
+[[nodiscard]] util::Expected<void, PSBTError> SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index, const PrecomputedTransactionData* txdata, const common::PSBTFillOptions& options, MissingSignatureData* out_missing_data = nullptr);
 
 /**  Reduces the size of the PSBT by dropping unnecessary `non_witness_utxos` (i.e. complete previous transactions) from a psbt when all inputs are segwit v1. */
 void RemoveUnnecessaryTransactions(PartiallySignedTransaction& psbtx);
diff --git a/src/script/sign.h b/src/script/sign.h
index 44bc5060fe..fc69eb28e7 100644
--- a/src/script/sign.h
+++ b/src/script/sign.h
@@ -75,10 +75,17 @@ extern const BaseSignatureCreator& DUMMY_MAXIMUM_SIGNATURE_CREATOR;
 
 typedef std::pair<CPubKey, std::vector<unsigned char>> SigPair;
 
+struct MissingSignatureData {
+    std::vector<CKeyID> missing_pubkeys; ///< KeyIDs of pubkeys which could not be found
+    std::vector<CKeyID> missing_sigs; ///< KeyIDs of pubkeys for signatures which could not be found
+    uint160 missing_redeem_script; ///< ScriptID of the missing redeemScript (if any)
+    uint256 missing_witness_script; ///< SHA256 of the missing witnessScript (if any)
+};
+
 // This struct contains information from a transaction input and also contains signatures for that input.
 // The information contained here can be used to create a signature and is also filled by ProduceSignature
 // in order to construct final scriptSigs and scriptWitnesses.
-struct SignatureData {
+struct SignatureData: MissingSignatureData {
     bool complete = false; ///< Stores whether the scriptSig and scriptWitness are complete
     bool witness = false; ///< Stores whether the input this SigData corresponds to is a witness input
     CScript scriptSig; ///< The scriptSig of an input. Contains complete signatures or the traditional partial signatures format
@@ -93,10 +100,6 @@ struct SignatureData {
     std::map<std::pair<XOnlyPubKey, uint256>, std::vector<unsigned char>> taproot_script_sigs; ///< (Partial) schnorr signatures, indexed by XOnlyPubKey and leaf_hash.
     std::map<XOnlyPubKey, std::pair<std::set<uint256>, KeyOriginInfo>> taproot_misc_pubkeys; ///< Miscellaneous Taproot pubkeys involved in this input along with their leaf script hashes and key origin data. Also includes the Taproot internal and output keys (may have no leaf script hashes).
     std::map<CKeyID, XOnlyPubKey> tap_pubkeys; ///< Misc Taproot pubkeys involved in this input, by hash. (Equivalent of misc_pubkeys but for Taproot.)
-    std::vector<CKeyID> missing_pubkeys; ///< KeyIDs of pubkeys which could not be found
-    std::vector<CKeyID> missing_sigs; ///< KeyIDs of pubkeys for signatures which could not be found
-    uint160 missing_redeem_script; ///< ScriptID of the missing redeemScript (if any)
-    uint256 missing_witness_script; ///< SHA256 of the missing witnessScript (if any)
     std::map<std::vector<uint8_t>, std::vector<uint8_t>> sha256_preimages; ///< Mapping from a SHA256 hash to its preimage provided to solve a Script
     std::map<std::vector<uint8_t>, std::vector<uint8_t>> hash256_preimages; ///< Mapping from a HASH256 hash to its preimage provided to solve a Script
     std::map<std::vector<uint8_t>, std::vector<uint8_t>> ripemd160_preimages; ///< Mapping from a RIPEMD160 hash to its preimage provided to solve a Script

    </details>

    kevkevinpal commented at 1:45 PM on May 6, 2026:

    Thanks for the review!

    This looks good. I agree that this would help with parsing. Probably out of the scope of the PR. Maybe we can do a follow-up after this is merged?

  101. rkrux commented at 1:28 PM on May 6, 2026: contributor

    lgtm ACK 6cca38e2b92967b3c057407319e39c6aebadd032

Contributors
kevkevinpalDrahtBotnaiyomaTumaBitcoinerrkruxryanofsky


naiyoma

Linked (view graph)
#31622 psbt: add non-default sighash types to PSBTs and unify sighash type match checking#35105 Refactor: Updated TransactionError to use util::Expected and removed TransactionError:OK#35201 Refactor: Updated signMessage to use util::Expected and removed SigningResult::OK
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-07 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me