cashubtc/cdk
1
0
Fork 0
Code Issues 25 Pull requests 23 Projects Releases 43 Packages Wiki Activity Actions

WIP: Sig all fixes #1208

Closed
lescuer97 wants to merge 3 commits from sig_all_fixes into main
pull from: sig_all_fixes
merge into: cashubtc:main
Conversation 10 Commits 3 Files changed 7 +526 -70
lescuer97 commented 2025-10-24 15:01:16 +00:00 (Migrated from github.com)
Copy link

Description

  • Make cdk mint and wallet use the correct signing algo when proofs utilize sig_all.
  • Add Melt Proofs options to melt function. there is no great way to do sign P2PK proofs other wise. (could be not needed, because we could just require anyone can pay proofs)

Notes to the reviewers

Suggested CHANGELOG Updates

CHANGED

ADDED

REMOVED

FIXED

Checklist

### Description - Make cdk mint and wallet use the correct signing algo when proofs utilize sig_all. - Add Melt Proofs options to melt function. there is no great way to do sign P2PK proofs other wise. (could be not needed, because we could just require `anyone can pay` proofs) ----- ### Notes to the reviewers <!-- In this section you can include notes directed to the reviewers, like explaining why some parts of the PR were done in a specific way --> ----- ### Suggested [CHANGELOG](https://github.com/cashubtc/cdk/blob/main/CHANGELOG.md) Updates <!-- Please do not edit the actual changelog but note what you changed here. --> #### CHANGED #### ADDED #### REMOVED #### FIXED ---- ### Checklist * [ ] I followed the [code style guidelines](https://github.com/cashubtc/cdk/blob/main/CODE_STYLE.md) * [ ] I ran `just final-check` before committing
thesimplekid commented 2025-10-24 15:02:36 +00:00 (Migrated from github.com)
Copy link

Duplicate of https://github.com/cashubtc/cdk/pull/1206?

Duplicate of https://github.com/cashubtc/cdk/pull/1206?
lescuer97 commented 2025-10-24 15:06:47 +00:00 (Migrated from github.com)
Copy link

Duplicate of #1206?

seems like it

> Duplicate of #1206? seems like it
lescuer97 commented 2025-10-27 19:25:32 +00:00 (Migrated from github.com)
Copy link

Duplicate of #1206?

I'm doing a bit more by fixing swaps signing and allowing for melts. Plus added the new Test Vectors.

> Duplicate of #1206? I'm doing a bit more by fixing swaps signing and allowing for melts. Plus added the new Test Vectors.
thesimplekid commented 2025-10-27 20:09:14 +00:00 (Migrated from github.com)
Copy link

We also have https://github.com/cashubtc/cdk/pull/1212 is this over lapping as well?

We also have https://github.com/cashubtc/cdk/pull/1212 is this over lapping as well?
lescuer97 commented 2025-10-27 22:25:05 +00:00 (Migrated from github.com)
Copy link

We also have #1212 is this over lapping as well?

lol, it seems over lapping, that pr changes quite a bit of stuff though

> We also have #1212 is this over lapping as well? lol, it seems over lapping, that pr changes quite a bit of stuff though
SatsAndSports commented 2025-10-27 22:58:49 +00:00 (Migrated from github.com)
Copy link

We also have #1212 is this over lapping as well?

lol, it seems over lapping, that pr changes quite a bit of stuff though

A lot of overlap in my PR #1212 yes 😀

When I started working on SIG_ALL stuff a week or two ago for the Spillman channel, I found a few bugs and so I ended up doing #1212 to fix it all, in particular with a single Rust trait and a single function to do all the verification of any spending condition:

  • swap with SIG_ALL was always rejected
  • locktimes and refund keys were not handled correctly in multiple places
  • and I think there was other stuff I can't quite remember now

I guess you're finding some or all of the same issues

> > We also have #1212 is this over lapping as well? > > lol, it seems over lapping, that pr changes quite a bit of stuff though A lot of overlap in my PR #1212 yes 😀 When I started working on SIG_ALL stuff a week or two ago for the Spillman channel, I found a few bugs and so I ended up doing #1212 to fix it all, in particular with a single Rust trait and a single function to do all the verification of any spending condition: - swap with SIG_ALL was always rejected - locktimes and refund keys were not handled correctly in multiple places - and I think there was other stuff I can't quite remember now I guess you're finding some or all of the same issues
SatsAndSports commented 2025-10-29 18:39:29 +00:00 (Migrated from github.com)
Copy link

Hi,
I've finally made time to look at this in more detail. First, I copied your tests into my PR and they were very helpful, including exposing an error in my code. I had forgotten to do the hex conversion of the preimage

I've now checked out your PR, and ran all my tests against them. My PR has a bunch of new tests defined in crates/cdk/src/mint/swap/spending_conditions. There are 29 of them, and they pass when they are run within that PR.

Using the code in this PR, some of those tests fail. I'll pick one of them now to do a deeper dive. Of course, maybe my code - or my test - is wrong 😀

cargo test -p cdk mint::swap::spending_conditions:: -- --show-output


failures:
    mint::swap::spending_conditions::htlc_sigall_spending_conditions_tests::test_htlc_sig_all_locktime_after_expiry
    mint::swap::spending_conditions::htlc_sigall_spending_conditions_tests::test_htlc_sig_all_multisig_2of3
    mint::swap::spending_conditions::htlc_sigall_spending_conditions_tests::test_htlc_sig_all_requiring_preimage_and_one_signature
    mint::swap::spending_conditions::htlc_spending_conditions_tests::test_htlc_locktime_after_expiry
    mint::swap::spending_conditions::p2pk_sigall_spending_conditions_tests::test_p2pk_sig_all_locktime_after_expiry
    mint::swap::spending_conditions::p2pk_sigall_spending_conditions_tests::test_p2pk_sig_all_locktime_after_expiry_no_refund_anyone_can_spend
    mint::swap::spending_conditions::p2pk_sigall_spending_conditions_tests::test_p2pk_sig_all_locktime_before_expiry
    mint::swap::spending_conditions::p2pk_sigall_spending_conditions_tests::test_p2pk_sig_all_mixed_proofs_different_data
    mint::swap::spending_conditions::p2pk_sigall_spending_conditions_tests::test_p2pk_sig_all_more_signatures_than_required
    mint::swap::spending_conditions::p2pk_sigall_spending_conditions_tests::test_p2pk_sig_all_multisig_2of3
    mint::swap::spending_conditions::p2pk_sigall_spending_conditions_tests::test_p2pk_sig_all_multisig_before_locktime
    mint::swap::spending_conditions::p2pk_sigall_spending_conditions_tests::test_p2pk_sig_all_multisig_locktime
    mint::swap::spending_conditions::p2pk_sigall_spending_conditions_tests::test_p2pk_sig_all_refund_multisig_2of2
    mint::swap::spending_conditions::p2pk_sigall_spending_conditions_tests::test_p2pk_sig_all_requires_transaction_signature
    mint::swap::spending_conditions::p2pk_sigall_spending_conditions_tests::test_p2pk_sig_all_signed_by_wrong_person
    mint::swap::spending_conditions::p2pk_sigall_spending_conditions_tests::test_sig_all_should_reject_if_the_output_amounts_are_swapped
    mint::swap::spending_conditions::p2pk_spending_conditions_tests::test_p2pk_locktime_after_expiry_no_refund_anyone_can_spend

test result: FAILED. 12 passed; 17 failed; 0 ignored; 0 measured; 62 filtered out; finished in 7.22s
Hi, I've finally made time to look at this in more detail. First, I copied your tests into my PR and they were very helpful, including exposing an error in my code. I had forgotten to do the hex conversion of the preimage I've now checked out your PR, and ran all my tests against them. My PR has a bunch of new tests defined in `crates/cdk/src/mint/swap/spending_conditions`. There are 29 of them, and they pass when they are run within that PR. Using the code in this PR, some of those tests fail. I'll pick one of them now to do a deeper dive. **Of course, maybe my code - or my test - is wrong 😀** `cargo test -p cdk mint::swap::spending_conditions:: -- --show-output` ``` failures: mint::swap::spending_conditions::htlc_sigall_spending_conditions_tests::test_htlc_sig_all_locktime_after_expiry mint::swap::spending_conditions::htlc_sigall_spending_conditions_tests::test_htlc_sig_all_multisig_2of3 mint::swap::spending_conditions::htlc_sigall_spending_conditions_tests::test_htlc_sig_all_requiring_preimage_and_one_signature mint::swap::spending_conditions::htlc_spending_conditions_tests::test_htlc_locktime_after_expiry mint::swap::spending_conditions::p2pk_sigall_spending_conditions_tests::test_p2pk_sig_all_locktime_after_expiry mint::swap::spending_conditions::p2pk_sigall_spending_conditions_tests::test_p2pk_sig_all_locktime_after_expiry_no_refund_anyone_can_spend mint::swap::spending_conditions::p2pk_sigall_spending_conditions_tests::test_p2pk_sig_all_locktime_before_expiry mint::swap::spending_conditions::p2pk_sigall_spending_conditions_tests::test_p2pk_sig_all_mixed_proofs_different_data mint::swap::spending_conditions::p2pk_sigall_spending_conditions_tests::test_p2pk_sig_all_more_signatures_than_required mint::swap::spending_conditions::p2pk_sigall_spending_conditions_tests::test_p2pk_sig_all_multisig_2of3 mint::swap::spending_conditions::p2pk_sigall_spending_conditions_tests::test_p2pk_sig_all_multisig_before_locktime mint::swap::spending_conditions::p2pk_sigall_spending_conditions_tests::test_p2pk_sig_all_multisig_locktime mint::swap::spending_conditions::p2pk_sigall_spending_conditions_tests::test_p2pk_sig_all_refund_multisig_2of2 mint::swap::spending_conditions::p2pk_sigall_spending_conditions_tests::test_p2pk_sig_all_requires_transaction_signature mint::swap::spending_conditions::p2pk_sigall_spending_conditions_tests::test_p2pk_sig_all_signed_by_wrong_person mint::swap::spending_conditions::p2pk_sigall_spending_conditions_tests::test_sig_all_should_reject_if_the_output_amounts_are_swapped mint::swap::spending_conditions::p2pk_spending_conditions_tests::test_p2pk_locktime_after_expiry_no_refund_anyone_can_spend test result: FAILED. 12 passed; 17 failed; 0 ignored; 0 measured; 62 filtered out; finished in 7.22s ```
SatsAndSports commented 2025-10-29 19:03:29 +00:00 (Migrated from github.com)
Copy link

I see one of the problems now, @lescuer97

None of the SIG_ALL tests (the ones I copied from my PR into this PR's cod) pass. This is because the code doesn't do SIG_ALL verification. This is an older pre-existing bug.

process_swap_request calls verify_inputs. This doesn't understand SIG_ALL, and therefore it immediately rejects it and the swap is rejected. This happens before the code reaches any SIG_ALL-checking code.

That problem, and a few other problems, are why I did a rewrite of all the verification of spending conditions

I see one of the problems now, @lescuer97 None of the SIG_ALL tests (the ones I copied from my PR into this PR's cod) pass. This is because the code doesn't do SIG_ALL verification. This is an older pre-existing bug. `process_swap_request` calls `verify_inputs`. This doesn't understand SIG_ALL, and therefore it immediately rejects it and the swap is rejected. This happens before the code reaches any SIG_ALL-checking code. That problem, and a few other problems, are why I did a rewrite of all the verification of spending conditions
robwoodgate (Migrated from github.com) requested changes 2025-11-08 11:39:59 +00:00
crates/cashu/src/nuts/nut11/mod.rs
robwoodgate (Migrated from github.com) commented 2025-11-08 11:39:52 +00:00
Copy link

The current iteration of the spec removed the keyset_id.

The current iteration of [the spec](https://github.com/cashubtc/nuts/pull/302) removed the keyset_id.
thesimplekid commented 2025-11-12 15:02:26 +00:00 (Migrated from github.com)
Copy link

close for https://github.com/cashubtc/cdk/pull/1212

close for https://github.com/cashubtc/cdk/pull/1212

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
robwoodgate
Labels
Clear labels   
DB & Storage

   
Deployment

   
Error Handling & Logging

   
Maintenance

   
Payment Backend

   
backport

   
backport v0.13.x

   
backport v0.14.x

   
backport v0.15.x

   
bindings

   
blocked

   
bug

Something isn't working

  
cdk-sql

   
ci

   
cli

   
deps

   
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
enhancement

New feature or request

  
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
invalid

This doesn't seem right

  
keep-open

   
ldk-node-ui

   
migrations

   
mint

   
mutation-testing

   
needs rebase

   
needs review

   
new nut

This PR is held pending the merge of a new or updated NUT

  
nut change

   
question

Further information is requested

  
ready

   
rust-version

   
rustfmt

   
stacked hold

PR is based on top of another PR, so should be merged in sequence. See comments for details.

  
stale

   
testing

   
wallet

   
weekly-report

   
wontfix

This will not be worked on

No labels
DB & Storage
Deployment
Error Handling & Logging
Maintenance
Payment Backend
backport
backport v0.13.x
backport v0.14.x
backport v0.15.x
bindings
blocked
bug
cdk-sql
ci
cli
deps
documentation
duplicate
enhancement
good first issue
help wanted
invalid
keep-open
ldk-node-ui
migrations
mint
mutation-testing
needs rebase
needs review
new nut
nut change
question
ready
rust-version
rustfmt
stacked hold
stale
testing
wallet
weekly-report
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
cashubtc/cdk!1208
No description provided.