Skip to content

wallet: support bip388 policy with external signer #33008

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

wallet: support bip388 policy with external signer #33008

wants to merge 6 commits into from
+423 −3

Conversation

Sjors
Copy link
Member

@Sjors Sjors commented Jul 18, 2025 •
edited
Loading

External signers such as Ledger, BitBox02 and Jade, when used with multisig, use BIP388 to display (and constrain) descriptor policies to their users.

At least in the case of Ledger (no others so far, see #33008 (comment)) this requires us to hold on to an hmac, which proves to the device that the user previously reviewed and approved it.

This PR adds a new RPC call registerpolicy which converts our descriptor(s) into a BIP388 policy (partially implemented), calls a new HWI method register (implemented) and stores the resulting HMAC (implemented).

When signing a transaction using HWI's signtx it passes the HMAC along with the PSBT (TODO).

For convenience the HMAC can be retrieved via getwalletinfo (implemented). This is useful for scenario's where we can't (yet) complete a transaction via HWI calls.

Note that the HMAC itself is not specified in BIP388 and may be different for different hardware vendors. So we'll store and echo any hex string the device gives us.

An alternative approach to directly supporting BIP388 policies would be to pass the involved descriptors to HWI and have it figure out how to derive a policy.

Testing:

TODO:

  • derive policy using Descriptor class instead regex madness, implement all constraints
  • pass hmac along when signing

Depends on:

Potential followups:

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 18, 2025 •
edited
Loading

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

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #33034 (wallet: Store transactions in a separate sqlite table by achow101)
  • #32958 (wallet/refactor: change PSBTError to PSBTResult and remove std::optionalcommon::PSBTResult and return common::PSBTResult by kevkevinpal)
  • #32895 (wallet: Prepare for future upgrades by recording versions of last client to open and decrypt by achow101)
  • #32876 (refactor: use options struct for signing and PSBT operations by Sjors)
  • #32861 (Have createwalletdescriptor auto-detect an unused(KEY) by Sjors)
  • #32784 (wallet: derivehdkey RPC to get xpub at arbitrary path by Sjors)
  • #32652 (wallet: add codex32 argument to addhdkey by roconnor-blockstream)
  • #32489 (wallet: Add exportwatchonlywallet RPC to export a watchonly version of a wallet by achow101)
  • #29136 (wallet: addhdkey RPC to add just keys to wallets via new unused(KEY) descriptor 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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

  • furth -> further [typo in TODO comment]
  • manangers -> managers [misspelling in doc comment “scriptpubkey manangers”]
  • Mananager -> Manager [typo in error message “ScriptPubKeyMananager”]

drahtbot_id_4_m

@Sjors Sjors mentioned this pull request Jul 18, 2025
Open
@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed.
Task previous releases, depends DEBUG: https://github.com/bitcoin/bitcoin/runs/46270694515
LLM reason (✨ experimental): The CI failure is caused by a compilation error due to a redundant declaration of 'RPCHelpMan wallet::registerpolicy()' which is treated as an error because all warnings are enabled as errors.

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.

This was referenced Jul 18, 2025
Open
Open
Open
Open
Draft
Draft
Draft
Draft
Merged
Open
Open
Merged
Open
@Sjors Sjors force-pushed the 2025/07/bip388-register branch from 8e9c541 to 061789c Compare July 21, 2025 08:28
@Sjors Sjors mentioned this pull request Jul 21, 2025
Open
@Sjors Sjors force-pushed the 2025/07/bip388-register branch from 061789c to 391584d Compare July 21, 2025 09:06
@Sjors Sjors mentioned this pull request Jul 21, 2025
Draft
1 task
@DrahtBot DrahtBot mentioned this pull request Jul 22, 2025
Draft
@Sjors Sjors force-pushed the 2025/07/bip388-register branch from 391584d to a551603 Compare July 24, 2025 18:38
@Sjors Sjors force-pushed the 2025/07/bip388-register branch 2 times, most recently from bc842eb to f87abd6 Compare August 5, 2025 19:08
@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 5, 2025

🚧 At least one of the CI tasks failed.
Task tidy: https://github.com/bitcoin/bitcoin/runs/47448999684
LLM reason (✨ experimental): The CI failure is due to an error flagged by clang-tidy in wallet.cpp at line 2621.

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.

@Sjors Sjors force-pushed the 2025/07/bip388-register branch from f87abd6 to d27a29a Compare August 6, 2025 08:29
@DrahtBot DrahtBot removed the CI failed label Aug 6, 2025
@Sjors Sjors force-pushed the 2025/07/bip388-register branch 2 times, most recently from 4330f54 to f2c78a5 Compare August 6, 2025 13:26
@pythcoiner
Copy link

At least in the case of Ledger (haven't tested the others) this requires us to hold on to an hmac, which proves to the device that the user previously reviewed and approved it.

In the range of devices supporting miniscript, only Ledger requires the software to store the Proof Of Registration, other devices stores it internally(SpecterDiy, BitBox, Coldcard, Jade) or not at all (Krux).

@Sjors Sjors force-pushed the 2025/07/bip388-register branch from f2c78a5 to f581e7a Compare August 25, 2025 12:16
@Sjors
Copy link
Member Author

Sjors commented Aug 25, 2025

@pythcoiner thanks, updated the description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Sjors @DrahtBot @pythcoiner