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, policies}`, `CreateParams::keymap`, `LoadParams::{keymap, extract_keys}`, and `TxBuilder::policy_path` in favor of caller-owned keys with `bitcoin::Psbt::sign` / `Wallet::sign_with_signers`, and `TxBuilder::set_condition`.
- Changed `FullyNodedExport::export_wallet` to not export secret keys anymore.
- Added `TxBuilder::set_condition` as replacement for `TxBuilder::policy_path`.

Before submitting

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

[codecov]

Codecov Report

❌ Patch coverage is 88.23529% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.94%. Comparing base (fc88144) to head (73c6736).

Files with missing lines	Patch %	Lines
src/wallet/mod.rs	85.96%	7 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #505      +/-   ##
==========================================
+ Coverage   81.84%   81.94%   +0.10%     
==========================================
  Files          25       25              
  Lines        6487     6491       +4     
  Branches      296      296              
==========================================
+ Hits         5309     5319      +10     
+ Misses       1078     1069       -9     
- Partials      100      103       +3     

Flag	Coverage Δ
rust	81.94% <88.23%> (+0.10%)	⬆️

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.

[ValuedMammal]

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.

That sounds extreme. The whole policy path ceremony in create_tx exists to come up with the "requirements" which is just a Condition struct with two optional fields, so what you could do is deprecate TxBuilder::policy_path and provide a set_condition method to pass the Condition directly and that will forever decouple the policy from TxBuilder. policy_path is broken anyway because of the unstable policy id as you mention (#123).

[ValuedMammal]

On second thought I think it should be merged to master and backported to 3.x, so we don't lose track of the tests migration and export feature.

[ValuedMammal]

Do you want to leave the export_with_keymap feature for a future discussion and treat the wallet export as a watch-only format for now?

[ValuedMammal]

ACK 515bee8; ran the tests locally