Deprecate wallet-owned signing APIs

[noahjoeris]

Description

Partially addresses #70

Deprecates wallet-owned signing APIs, migrating examples, tests, and documentation toward caller-owned keys using bitcoin::Psbt::sign and Wallet::sign_with_signers.

Depends on:

• fix(key_map): derivation_path for Xprv with key origin info rust-bitcoin/rust-miniscript#872
• fix(keymap): backport use of correct derivation_path for keys with origin rust-bitcoin/rust-miniscript#867

Notes to the reviewers

• I migrated examples, docs and tests to use bitcoin::Psbt::sign where possible. And used Wallet::sign_with_signers if we need SignOptions.

Changelog notice

• Deprecated Wallet::{add_signer, set_keymap, set_keymaps, get_signers, sign}, CreateParams::keymap, and LoadParams::{keymap, extract_keys} in favor of caller-owned keys and bitcoin::Psbt::sign and Wallet::sign_with_signers.
• Deprecated FullyNodedExport::export_wallet; use FullyNodedExport::export_wallet_with_keymaps to supply keymaps explicitly.

Before submitting

• I followed the contribution guidelines
• This PR breaks the existing API

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.13%. Comparing base (58fe631) to head (c810938).
⚠️ Report is 12 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #505      +/-   ##
==========================================
+ Coverage   80.96%   81.13%   +0.16%     
==========================================
  Files          24       24              
  Lines        5489     5501      +12     
  Branches      247      247              
==========================================
+ Hits         4444     4463      +19     
+ Misses        968      958      -10     
- Partials       77       80       +3     

Flag	Coverage Δ
rust	81.13% <100.00%> (+0.16%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[oleonardolima]

overall it's looking good, it's best if you fix the first commit and let the last one just adding the deprecation notice. let's us know when it's ready for final review.

[noahjoeris]

Thanks for spotting this. It seems I added the change to the last commit and not the first, my bad. I cleaned up the commits.

[oleonardolima]

utACK c810938

[noahjoeris]

I still need to look into Wallet::policies and Wallet::create_tx and what to do with them. They also use self.signers

[noahjoeris]

For Wallet::policies(), I suggest deprecating it and adding something like Wallet::policies_with_signers(keychain, signers).

One tricky part is TxBuilder::finish() -> Wallet::create_tx(). It currently extracts policies with the wallet signers.

A possible migration path is adding TxBuilder::policy(policy, keychain). Then callers that use policies_with_signers(...) can pass the resulting policy into the builder. Existing callers can keep the current fallback behavior of using wallet-owned signers during the deprecation window.

Downside: policy-path users get one more builder call, and Policy becomes more directly part of the API. Maybe that's acceptable with our intention of slowly moving away from TxBuilder.

[vera945]

Neither policies nor create_tx needs deprecation. Here's the reasoning:

create_tx calls extract_policy only to invoke requires_path() and get_condition(). Both methods operate entirely on the descriptor's structural tree — SatisfiableItem — and never read the contribution field, which is the only output that depends on the signers container. So passing SignersContainer::default() is valid without changing the behavior. This is verifiable by creating a wallet from xpub descriptors and calling build_tx normally.

policies similarly behaves this way for watch-only wallets. The contribution field will be Satisfaction::None for all leaves, which correctly describes a wallet that holds no signing keys. If a caller wants to evaluate what their external signing keys can contribute, the right answer is wallet.public_descriptor(keychain).extract_policy(their_signers, ...), which is already the pattern #505 is establishing.

[noahjoeris]

No, passing empty signers won't work. It can create different Policy IDs, and policy_path relies on those IDs.

A path built from Wallet::policies() can fail when create_tx() re-extracts with empty signers, because get_condition() looks up selections by self.id.

[noahjoeris]

So after some discussion with @thunderbiscuit and @notmandatory we concluded that we cannot remove the signers fields unless we update TxBuilder to accept a spending policy to be passed in. If we don't want to update it (from what I heard) then the wallet signer removal is coupled to the TxBuilder removal. And it might make sense to deprecate both at the same time once we integrated create_psbt.

[ItoroD]

@noahjoeris Just curious, is there any reason we call secp from the wallet wallet.secp_ctx() instead of just creating one? Does wallet add anything special to Secp256k1 ?

[noahjoeris]

We simply reuse it to avoid initializing another one. nothing special is added, creating and using your own context is equivalent.