Skip to content

feat: add p2pk_signing_keys to SendOptions#1835

Merged
thesimplekid merged 4 commits into
cashubtc:mainfrom
vnprc:send-p2pk-signing-keys
May 21, 2026
Merged

feat: add p2pk_signing_keys to SendOptions#1835
thesimplekid merged 4 commits into
cashubtc:mainfrom
vnprc:send-p2pk-signing-keys

Conversation

@vnprc
Copy link
Copy Markdown
Contributor

@vnprc vnprc commented Apr 3, 2026

Add p2pk_signing_keys: Vec<SecretKey> and allow_locked_proofs: bool to SendOptions so wallets holding P2PK-locked proofs can spend them through the standard prepare_send / confirm flow without a separate manual swap.

Behaviour

  • When p2pk_signing_keys is non-empty and allow_locked_proofs is false (the default), all selected proofs are routed through a swap so the resulting token contains fresh, unconditioned proofs.
  • Setting allow_locked_proofs: true opts in to the less-private passthrough path: proofs are signed and retain their spending conditions in the token. This is useful for multi-hop or offline flows where the next holder intentionally receives signed-but-still-locked proofs.

SIG_ALL incompatibility

SigFlag::SigAll is incompatible with passthrough signing: the signature would need to commit to swap outputs that do not exist at signing time. confirm() now calls enforce_sig_flag() on the proofs-to-send set and returns nut11::Error::SigAllNotSupportedHere early rather than silently producing an unspendable token.

Changes

  • cdk-common: add p2pk_signing_keys and allow_locked_proofs to SendOptions, with doc comments explaining the SigAll incompatibility.
  • cdk/wallet/util.rs: extract sign_proofs() with four unit tests covering correct key, wrong key, plain proof, and empty-keys cases.
  • cdk/wallet/send/saga/mod.rs: shadow force_swap to true in internal_prepare when signing keys are provided and passthrough is not opted in; call sign_proofs() in confirm() for both proofs_to_swap (default path) and proofs_to_send (passthrough path); add SigAll check with an explanatory block comment before the passthrough signing block.
  • cdk-ffi: add p2pk_signing_keys and allow_locked_proofs to FFI SendOptions and update both From conversions.
  • Integration tests: add four pure tests covering the normal signing flow, the exact-denomination short-circuit regression, the passthrough opt-in, and the SigAll rejection.

Description

Add p2pk_signing_keys: Vec<SecretKey> and allow_locked_proofs: bool to SendOptions so wallets holding P2PK-locked proofs can spend them through the standard prepare_send / confirm flow without a separate manual swap.

Default behaviour (allow_locked_proofs: false): all selected proofs are routed through a swap, producing a clean token with no spending conditions. The caller provides signing keys only to authorize the swap inputs.

Passthrough (allow_locked_proofs: true): proofs are signed in-place and retain their spending conditions. Useful for multi-hop or offline flows where the next holder intentionally receives signed-but-still-locked proofs.

SigAll rejection: SigFlag::SigAll is incompatible with passthrough because the signature would need to commit to swap outputs that don't exist at signing time. confirm() detects this via enforce_sig_flag() and returns nut11::Error::SigAllNotSupportedHere early rather than silently producing an unspendable token.

Also fixes a force-swap short-circuit bug where proofs summing exactly to the send amount bypassed the swap entirely, causing locked proofs to escape into the token unsigned.

Test plan

  • cargo test -p cdk wallet::util — 5 unit tests pass
  • test_p2pk_send_options_signing_keys — normal signing flow
  • test_p2pk_signing_keys_exact_denomination_short_circuit — force-swap regression
  • test_p2pk_allow_locked_proofs_passthrough — passthrough opt-in
  • test_p2pk_allow_locked_proofs_rejects_sig_all — SigAll rejection

Notes to the reviewers


Suggested CHANGELOG Updates

CHANGED

ADDED

REMOVED

FIXED


Checklist

@vnprc
Copy link
Copy Markdown
Contributor Author

vnprc commented Apr 3, 2026

see #1750 for the first PR attempt

@thesimplekid thesimplekid requested a review from lescuer97 April 4, 2026 07:55
Comment thread crates/cdk-common/src/wallet/mod.rs Outdated
Comment thread crates/cdk/src/wallet/send/saga/mod.rs Outdated
Comment thread crates/cdk/src/wallet/util.rs
Copy link
Copy Markdown
Contributor

@lescuer97 lescuer97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some comments around the code and proper flow

Comment thread crates/cdk-common/src/wallet/mod.rs Outdated
Comment thread crates/cdk-common/src/wallet/mod.rs Outdated
Comment thread crates/cdk/src/wallet/send/saga/mod.rs Outdated
Comment thread CHANGELOG.md
@github-project-automation github-project-automation Bot moved this from Backlog to In progress in CDK Apr 5, 2026
@vnprc vnprc force-pushed the send-p2pk-signing-keys branch from 060ff2b to 04c584e Compare April 10, 2026 15:30
@vnprc
Copy link
Copy Markdown
Contributor Author

vnprc commented Apr 10, 2026

thanks y'all. i rebased against main and all comments addressed. ready for another look

@thesimplekid thesimplekid self-requested a review April 10, 2026 15:32
@vnprc
Copy link
Copy Markdown
Contributor Author

vnprc commented May 14, 2026

@thesimplekid any ETA on this? should i rebase?

Comment thread crates/cdk/src/wallet/send/saga/mod.rs Outdated
continue;
};

let Ok(data_key) = crate::nuts::PublicKey::from_str(secret.secret_data().data()) else {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This signability filter only checks the P2PK data key, but P2PK conditions can also require additional pubkeys via n_sigs. A proof where the wallet has the data key but not enough additional required keys will pass this filter, then fail later at swap/confirm with a mint rejection instead of being excluded as unsignable. Can this reuse the same requirements logic as verification, or sign and verify before treating the proof as selectable?

&options.p2pk_signing_keys,
)
.await?;
if !keys.is_empty() {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In passthrough mode this signs only when a matching key is found, but it does not fail if no key is found or if the proof is only partially signed. That can create a token containing locked proofs that the recipient cannot redeem. Before creating the token, this should verify the passthrough proofs satisfy their spending conditions and return an error if they do not.

Comment thread crates/cdk-ffi/src/types/wallet.rs Outdated
p2pk_signing_keys: opts
.p2pk_signing_keys
.into_iter()
.filter_map(|k| k.try_into().ok())
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This silently drops invalid FFI secret keys. ReceiveOptions uses a fallible conversion and returns an error for invalid p2pk_signing_keys; send should do the same so callers do not get a later InsufficientFunds or mint rejection caused by a key that was ignored during conversion.

Copy link
Copy Markdown
Collaborator

@thesimplekid thesimplekid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed a few things and yes could you squash and rebase and we can get it merged thanks.

@vnprc vnprc force-pushed the send-p2pk-signing-keys branch from 04c584e to 7daf77c Compare May 15, 2026 19:12
@vnprc
Copy link
Copy Markdown
Contributor Author

vnprc commented May 15, 2026

ready for re-review. my agent also found an ffi bug that i will be opening a different pr to fix

@codecov
Copy link
Copy Markdown

codecov Bot commented May 15, 2026

Codecov Report

❌ Patch coverage is 89.44338% with 55 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.23%. Comparing base (2018181) to head (25c274c).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
crates/cdk/src/wallet/util.rs 84.67% 21 Missing ⚠️
crates/cdk/src/wallet/send/saga/mod.rs 94.27% 19 Missing ⚠️
crates/cdk-ffi/src/types/wallet.rs 36.36% 14 Missing ⚠️
crates/cdk-ffi/src/wallet.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1835      +/-   ##
==========================================
+ Coverage   65.91%   66.23%   +0.31%     
==========================================
  Files         330      330              
  Lines       58129    58641     +512     
==========================================
+ Hits        38317    38841     +524     
+ Misses      19812    19800      -12     

☔ View full report in Codecov by Sentry.
📢 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.

@vnprc
Copy link
Copy Markdown
Contributor Author

vnprc commented May 15, 2026

related bug fix: #1970

@thesimplekid thesimplekid force-pushed the send-p2pk-signing-keys branch from 5dd6e16 to d66bab6 Compare May 18, 2026 15:03
@vnprc vnprc force-pushed the send-p2pk-signing-keys branch from d66bab6 to 2c898e1 Compare May 19, 2026 13:25
@vnprc
Copy link
Copy Markdown
Contributor Author

vnprc commented May 19, 2026

oh shit my agent force pushed this branch. woops. i will try to reconcile the two force pushes.

@vnprc
Copy link
Copy Markdown
Contributor Author

vnprc commented May 19, 2026

ah ok nm, it looks like codex did the right thing. please let me know if the force push was destructive. i tried to put guardrails in place but it seems like controlling agents is an impossible task. ¯\_(ツ)_/¯

@thesimplekid thesimplekid force-pushed the send-p2pk-signing-keys branch from 9523a00 to c2d29dc Compare May 19, 2026 19:42
@thesimplekid
Copy link
Copy Markdown
Collaborator

We can avoid needed the direct secp dep by just changing the error to not use transparent, I did this in this commit bf2757f. I also think it is more explixt and better if we use a enum vs the bool, I added that here c2d29dc. If you're happy with those changes I can merge and do any further rebasing once CI completes.

@thesimplekid thesimplekid force-pushed the send-p2pk-signing-keys branch from c2d29dc to f0b2baa Compare May 20, 2026 08:28
@thesimplekid thesimplekid force-pushed the send-p2pk-signing-keys branch from f0b2baa to 0ce89bb Compare May 20, 2026 08:28
@vnprc
Copy link
Copy Markdown
Contributor Author

vnprc commented May 20, 2026

LGTM

I have confirmed that hashpool mints ehash with this PR branch as it is. Sorry about the dependency churn, I need to work on my agent harness and put more human in the loop before pushing to a PR branch. Thanks for all the fee and enum improvements! Great stuff!

@thesimplekid thesimplekid force-pushed the send-p2pk-signing-keys branch from 0ce89bb to e10a579 Compare May 20, 2026 22:10
vnprc and others added 4 commits May 21, 2026 17:01
Replace the send-side `allow_locked_proofs` boolean with an explicit
`P2PKLockedProofSendMode` enum. The default remains swapping P2PK-locked
proofs, while direct signed passthrough is now represented as `SignAndSend`.

Update the wallet re-export, FFI conversions/defaults, and P2PK integration
tests to use the new mode.
@thesimplekid thesimplekid force-pushed the send-p2pk-signing-keys branch from e10a579 to 25c274c Compare May 21, 2026 15:14
@thesimplekid thesimplekid merged commit 25c274c into cashubtc:main May 21, 2026
7 checks passed
@github-project-automation github-project-automation Bot moved this from In progress to Done in CDK May 21, 2026
vnprc added a commit to vnprc/hashpool that referenced this pull request May 22, 2026
PR cashubtc/cdk#1835 (P2PK signing keys) was merged on 2026-05-21.
The vnprc/cdk fork is no longer needed.

Changes:
- roles/Cargo.toml, protocols/Cargo.toml: redirect [patch.crates-io]
  from vnprc/cdk@9523a003 to cashubtc/cdk@1572941d for all 9 cdk crates
- devenv.nix: update cdkRepo/cdkCommit variables and build:cdk:cli task
- roles/mint/Cargo.toml: bump cdk-ehash rev b7edd52 -> c1a11ba
  (add onchain: None to SettingsResponse, new field in upstream CDK)
- roles/mint/src/main.rs: update HttpCache construction to async
  HttpCache::from_config(...).await? (sync From impl removed in CDK)
- roles/Cargo.lock: pin MSRV-sensitive transitive deps to pre-1.88
  versions for devenv Rust 1.86.0 compatibility
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants