wallet: Introduce WalletError with machine-readable error code #35690

pseudoramdom commented at 12:32 PM on July 9, 2026: none

Per discussion in #35436 (comment), WalletError is split out so that it can be reused by multiple wallet interface changes (#34861 in particular)

Introduce a wallet::WalletError, a generic wallet-layer error type that contains

a machine-readable WalletErrorCode for programmatic handling
a translated user-facing bilingual_str message

The initial enum is intentionally small. WALLET_ERROR is used for generic failures that callers should display to the user. The intention is to have more specific codes only when the callers can handle the condition differently.

DrahtBot added the label Wallet on Jul 9, 2026

DrahtBot commented at 12:32 PM on July 9, 2026: contributor

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/35690.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	achow101, polespinasa, davidgumberg

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34861 (wallet: Add importdescriptors interface by polespinasa)
#34617 (fees: wallet: remove block policy fee estimator internals from wallet by ismaelsadeeq)
#34075 (fees: Introduce Mempool Based Fee Estimation to reduce overestimation by ismaelsadeeq)

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.

pseudoramdom commented at 12:32 PM on July 9, 2026: none

cc @polespinasa

polespinasa commented at 12:50 PM on July 9, 2026: member

Thanks for this :)

So bringing back my comment #35436 (comment)

Maybe not adding all of them as I initially suggested, but for example #34861 needs, apart from the ones this PR adds:

enum class WalletErrorCode {
        INVALID_DESCRIPTOR,
        INVALID_PARAMETER,
        MISC_ERROR
};

Are we open to add those to this enum, even if some like INVALID_DESCRIPTOR are not as much generic as others? If we don't then we need multiple structs to handle different error codes which adds a lot of complexity.

DrahtBot added the label CI failed on Jul 9, 2026

achow101 commented at 6:33 PM on July 9, 2026: member

Are we open to add those to this enum, even if some like INVALID_DESCRIPTOR are not as much generic as others? If we don't then we need multiple structs to handle different error codes which adds a lot of complexity.

Ugh, I don't like either approach. Preferably parameter validation should occur before calling the interface functions so that we don't need INVALID_DESCRIPTOR and INVALID_PARAMETER. MISC_ERROR is only returned by rescan issues, so those could possibly be a more specific enum for rescan failure that I guess could be actionable by callers.

polespinasa commented at 6:58 PM on July 9, 2026: member

Preferably parameter validation should occur before calling the interface functions so that we don't need INVALID_DESCRIPTOR and INVALID_PARAMETER

That would mean duplicating a lot of code. For example, we throw INVALID_DESCRIPTOR in multiple occasions because things like "Range should not be specified for an un-ranged descriptor", "Cannot have multipath descriptor while also specifying 'internal'", same for INVALID_PARAMETER, which is used for things like Ranged descriptors should not have a label or Active descriptors must be ranged.

I am not sure duplicating all those checks is a good idea.

so those could possibly be a more specific enum for rescan failure that I guess could be actionable by callers.

Care to propose pseudo-code? I am not sure I follow you.

achow101 commented at 7:13 PM on July 9, 2026: member

That would mean duplicating a lot of code. For example, we throw INVALID_DESCRIPTOR in multiple occasions because things like "Range should not be specified for an un-ranged descriptor", "Cannot have multipath descriptor while also specifying 'internal'", same for INVALID_PARAMETER, which is used for things like Ranged descriptors should not have a label or Active descriptors must be ranged.

I am not sure duplicating all those checks is a good idea.

I guess a single INVALID_PARAMETER enum would also be fine. I don't really want to be getting that granular about how invalid the parameters are.

so those could possibly be a more specific enum for rescan failure that I guess could be actionable by callers.

Care to propose pseudo-code? I am not sure I follow you.

Instead of MISC_ERROR, we could have RESCAN_FAILURE as that is what the importdescriptors reports MISC_ERROR for anyways.

davidgumberg commented at 11:39 PM on July 9, 2026: contributor

Instead of MISC_ERROR, we could have RESCAN_FAILURE as that is what the importdescriptors reports MISC_ERROR for anyways.

Sorry I'm not sure I follow why these all can't fall under MISC_ERROR since it's just an error message that gets printed for the user in both cases, right? Or is there something different that the caller does for each error?

davidgumberg commented at 11:45 PM on July 9, 2026: contributor

ACK https://github.com/bitcoin/bitcoin/pull/35690/commits/152632166a67621cc9ae4ade5246c9b16e1e6f42

It's a bit weird to add stub code like this, but it makes sense since otherwise there is a bit of awkwardness with both #35436 and #34681 needing this, merging this so both of those can be reviewed in parallel seems fine.

It makes sense to me to provide machine-readable error codes for cases where there is something interesting the caller can do with the error code. For now just a wallet locked and an unlock error code, seem reasonable, and we can probably bikeshed what other specific error codes should or shouldn't get added in the PR's where interfaces are being added that want them.

pseudoramdom force-pushed on Jul 10, 2026

pseudoramdom commented at 7:22 AM on July 10, 2026: none

Force pushed to c6ed33f001f36ebd91298792e3802d63aea813c5 to address comment in #35436 (review) to remove WALLET_ prefix from the error cases.

pseudoramdom commented at 7:23 AM on July 10, 2026: none

we can probably bikeshed what other specific error codes should or shouldn't get added in the PR's where interfaces are being added that want them

Agree.

in src/wallet/types.h:54 in c6ed33f001 outdated

  49 | +//! differently. For errors that should only be displayed to the user, use
  50 | +//! WALLET_ERROR and provide the user-facing details in WalletError::message.
  51 | +enum class WalletErrorCode {
  52 | +    //! Generic wallet error. Callers may present the accompanying message to
  53 | +    //! the user.
  54 | +    GENERIC_ERROR,

maflcko commented at 8:00 AM on July 10, 2026:

nit: would be good to avoid ALL_CAPS going forward for new code. The dev notes aren't updated yet (c.f. #35588 (review)), but lately there was a scripted-diff roughly every two weeks to rename an ALL_CAPS to PascalCase or snake_case, due to a third-party macro clash.

pseudoramdom force-pushed on Jul 10, 2026

pseudoramdom commented at 8:18 AM on July 10, 2026: none

Force pushed to ~~d4ebd859023a78dd14d81d43555ae3d2a102c104~~ a8223bb4e62c8facd7e99eb05221c67cdcf0b52c to address #35690 (review) and use PascalCase

pseudoramdom force-pushed on Jul 10, 2026

wallet: Introduce WalletError with machine-readable error code

Introduce WalletError as a generic wallet-layer error type that can carry a machine-readable WalletErrorCode and a translated user-facing message.

The WalletErrorCode::GenericError code is intended for failures that callers should only display to the user. More specific codes should only be added when callers can handle the condition differently.

a8223bb4e6

pseudoramdom force-pushed on Jul 10, 2026

DrahtBot removed the label CI failed on Jul 10, 2026

polespinasa commented at 2:48 PM on July 10, 2026: member

I guess a single INVALID_PARAMETER enum would also be fine. I don't really want to be getting that granular about how invalid the parameters are.

How do you translate them if we use the same INVALID_PARAMETER for different codes? This only matter for not breaking the current RPC obviously, for new interfaces it does not matter at all.

achow101 commented at 8:58 PM on July 10, 2026: member

How do you translate them if we use the same INVALID_PARAMETER for different codes?

I would be okay with changing the error codes.

achow101 commented at 9:00 PM on July 10, 2026: member

ACK a8223bb4e62c8facd7e99eb05221c67cdcf0b52c

DrahtBot requested review from davidgumberg on Jul 10, 2026

polespinasa commented at 9:19 PM on July 10, 2026: member

How do you translate them if we use the same INVALID_PARAMETER for different codes?

I would be okay with changing the error codes.

Let's discuss this in #34861

polespinasa commented at 9:19 PM on July 10, 2026: member

ACK a8223bb4e62c8facd7e99eb05221c67cdcf0b52c

davidgumberg commented at 10:01 PM on July 10, 2026: contributor

ACK a8223bb4e62c8facd7e99eb05221c67cdcf0b52c