Various mint bugfixes for swap and melt. SIG_INPUTS+SIG_ALL, locktimes, P2PK+HTLC. Also updates the SIG_ALL message for amount-switching #1212

2025-10-26 22:02:19 +00:00

					
				@ -189,1 +188,3 @@

				            }

				        // Check if we have enough valid signatures

				        if valid_sig_count >= relevant_num_sigs_required {

				            Ok(())

I think these should be more than debug asserts and should return errors at runtime.

Debug asserts should be used for checking our developer assumptions and only in places where we can only reach them in private functions, for example. For things that a user could do, like calling sign_p2pk on the wrong proof type, we should return an error.

I think these should be more than debug asserts and should return errors at runtime. Debug asserts should be used for checking our developer assumptions and only in places where we can only reach them in private functions, for example. For things that a user could do, like calling sign_p2pk on the wrong proof type, we should return an error.

thesimplekid (Migrated from github.com) reviewed

crates/cashu/src/nuts/nut00/mod.rs

I think we can remove these comments. In general, comments are really helpful and we should use more of them to describe why we're doing something rather than how. Since extend() works directly on the Vec here and the initialization flow is pretty clear from the code itself, these comments might be a bit redundant.

That said, leaning towards more comments is a good instinct

I think we can remove these comments. In general, comments are really helpful and we should use more of them to describe *why* we're doing something rather than *how*. Since `extend()` works directly on the Vec here and the initialization flow is pretty clear from the code itself, these comments might be a bit redundant. That said, leaning towards more comments is a good instinct

					
				@ -105,6 +108,431 @@ impl Secret {

				    }

Does this need to be public? Maybe can be just public to the crate seems like an internal fn.

pub trait SpendingConditionVerification {

Save a word?

```suggestion pub trait SpendingConditionVerification { ``` Save a word?

					
				@ -108,0 +165,4 @@

				                let data_pubkey = PublicKey::from_str(secret.secret_data().data())?;

				                primary_keys.push(data_pubkey);

				                // Add any additional pubkeys from conditions

These are helpful comments. May make sense to even link to parts of spec.

					
				@ -108,0 +376,4 @@

				            // Check if secret is a nut10 secret with conditions

				            if let Ok(secret) = Secret::try_from(&proof.secret) {

				                // Verify this function isn't being called with SIG_ALL proofs (development check)

				                if let Ok(conditions) = super::Conditions::try_from(

Do we check this?

					
				@ -887,132 +860,8 @@ impl From<Tag> for Vec<String> {

				}

				impl SwapRequest {

I think this version number is wrong. But we don't need to mark as deprecated we can just remove it if its not needed or wrong. We will include it in a minor release since we're pre 1.0 its okay

					
				@ -1246,4 +919,1 @@

				            Err(Error::SpendConditionsNotMet)

				        }

				    }

				}

Lets make sure its right before merging.

Can you clarify this comment.

2025-10-27 12:02:49 +00:00

Should return errors here as well not debug asserts

SatsAndSports (Migrated from github.com) reviewed

2025-10-27 12:58:48 +00:00

					
				@ -189,1 +188,3 @@

				            }

				        // Check if we have enough valid signatures

				        if valid_sig_count >= relevant_num_sigs_required {

				            Ok(())

Agreed. I'll return an error to the user instead of the debug_assert. I'll add new errors in some cases:

For things that a user could do, like calling sign_p2pk on the wrong proof type, we should return an error.

I assumed (without thinking really) that verify_p2pk was private and wouldn't be called directly by the user. But I was wrong, it's public and users might therefore call it directly.

====

Thanks for all the comments yesterday, I'll try to work through them today

Agreed. I'll return an error to the user instead of the `debug_assert`. I'll add new errors in some cases: <img width="1161" height="369" alt="image" src="https://github.com/user-attachments/assets/c2677c6a-7c30-470b-b0f3-b5b1b0cacf4c" /> > For things that a user could do, like calling sign_p2pk on the wrong proof type, we should return an error. I assumed (without thinking really) that `verify_p2pk` was private and wouldn't be called directly by the user. But I was wrong, it's public and users might therefore call it directly. ==== Thanks for all the comments yesterday, I'll try to work through them today

SatsAndSports (Migrated from github.com) reviewed

SatsAndSports (Migrated from github.com) reviewed

2025-10-27 18:29:06 +00:00

crates/cashu/src/nuts/nut00/mod.rs

Agreed. I've removed those comments. I'll push it later today

SatsAndSports (Migrated from github.com) reviewed

2025-10-27 18:34:47 +00:00

2025-10-27 22:03:33 +00:00

					
				@ -887,132 +860,8 @@ impl From<Tag> for Vec<String> {

				}

				impl SwapRequest {

Agreed. I've removed it (verify_sig_all), and will push it later today

There is another option, if anybody wants to keep a public function with the same name and which is about SIG_ALL checks. The trait gives this class a function called verify_full_sig_all_check. We could rename that function to verify_sig_all and make it public

Agreed. I've removed it (`verify_sig_all`), and will push it later today There is another option, if anybody wants to keep a public function with the same name and which is about SIG_ALL checks. The trait gives this class a function called `verify_full_sig_all_check`. We could rename that function to `verify_sig_all` and make it public

SatsAndSports (Migrated from github.com) reviewed

2025-10-27 22:04:11 +00:00

					
				@ -1246,4 +919,1 @@

				            Err(Error::SpendConditionsNotMet)

				        }

				    }

				}

I've now removed that, including that warning.

I added that warning earlier, to remind myself to look at that function. Anyway, it's removed now. It wasn't handling locktime

I've now removed that, including that warning. I added that warning earlier, to remind myself to look at that function. Anyway, it's removed now. It wasn't handling locktime

SatsAndSports (Migrated from github.com) reviewed

2025-10-27 22:09:11 +00:00

Done. Errors instead of debug_assert

Done. Errors instead of `debug_assert`

SatsAndSports (Migrated from github.com) reviewed

2025-10-27 22:12:20 +00:00

I've rewritten that comment in the code. Copied here also:

        // While a Witness is usually needed in a P2PK or HTLC proof, it's not                                                                    
        // always needed. If we are past the locktime, and there are no refund                                                                    
        // keys, then the proofs are anyone-can-spend:                                                                                            
        //     NUT-11: "If the tag locktime is the unix time and the mint's local                                                                 
        //              clock is greater than locktime, the Proof becomes spendable                                                               
        //              by anyone, except if [there are no refund keys]"                                                                          
        // Therefore, this function should not extract any Witness unless it                                                                      
        // is needed to get a preimage or signatures.

I've rewritten that comment in the code. Copied here also: ``` // While a Witness is usually needed in a P2PK or HTLC proof, it's not // always needed. If we are past the locktime, and there are no refund // keys, then the proofs are anyone-can-spend: // NUT-11: "If the tag locktime is the unix time and the mint's local // clock is greater than locktime, the Proof becomes spendable // by anyone, except if [there are no refund keys]" // Therefore, this function should not extract any Witness unless it // is needed to get a preimage or signatures. ```

SatsAndSports (Migrated from github.com) reviewed

2025-10-27 22:17:46 +00:00

					
				@ -105,6 +108,431 @@ impl Secret {

				    }

You're right. Changed from pub to pub(crate)

You're right. Changed from `pub` to `pub(crate)`

SatsAndSports (Migrated from github.com) reviewed

2025-10-27 22:20:50 +00:00

					
				@ -105,6 +108,431 @@ impl Secret {

				    }

Agreed. Changing from VerificationForSpendingConditions to SpendingConditionVerification

Agreed. Changing from `VerificationForSpendingConditions` to `SpendingConditionVerification`

SatsAndSports (Migrated from github.com) reviewed

2025-10-27 22:55:01 +00:00

					
				@ -108,0 +376,4 @@

				            // Check if secret is a nut10 secret with conditions

				            if let Ok(secret) = Secret::try_from(&proof.secret) {

				                // Verify this function isn't being called with SIG_ALL proofs (development check)

				                if let Ok(conditions) = super::Conditions::try_from(

That function is private and is only called from one place, where verify_spending_conditions has already verified that there is no SIG_ALL in any input. So the checking is done there

That function is private and is only called from one place, where `verify_spending_conditions` has already verified that there is no SIG_ALL in any input. So the checking is done there

SatsAndSports commented

(Migrated from github.com)

I think I've read and dealt with all the comments that you made yesterday, @thesimplekid

Although, before I forget, there are a couple of strange things about this PR now:

I marked some tests as #[ignore]. If I remember correctly, all of them are based on the test vectors for SIG_ALL and I haven't updated them into this PR yet
I'm not sure if the HTLC data and preimage need to be converted to and from hex. The tests currently pass, but my code her perhaps doesn't follow the spec. I had hoped that the HTLCWitness object would (de-)serialize automatically, or perhaps have a method to do any conversion

I think I've read and dealt with all the comments that you made yesterday, @thesimplekid Although, before I forget, there are a couple of strange things about this PR now: - I marked some tests as `#[ignore]`. If I remember correctly, all of them are based on the test vectors for SIG_ALL and I haven't updated them into this PR yet - I'm not sure if the HTLC `data` and `preimage` need to be converted to and from hex. The tests currently pass, but my code her perhaps doesn't follow the spec. I had hoped that the `HTLCWitness` object would (de-)serialize automatically, or perhaps have a method to do any conversion

thesimplekid (Migrated from github.com) reviewed

2025-10-28 03:57:38 +00:00

2025-10-29 18:25:33 +00:00

					
				@ -108,0 +376,4 @@

				            // Check if secret is a nut10 secret with conditions

				            if let Ok(secret) = Secret::try_from(&proof.secret) {

				                // Verify this function isn't being called with SIG_ALL proofs (development check)

				                if let Ok(conditions) = super::Conditions::try_from(

We should add a debug assert then since its something in our control we're committing not to do.

SatsAndSports commented

(Migrated from github.com)

Here's an update on the latest batch of changes, just pushed above:

more new functions to improve code re-use. For example, verify_htlc_preimage which does the hex processing and verifies that the preimage has the required hash. This is shared between both the SIG_INPUTS path and the SIG_ALL path. Also, a new method preimage_data in HTLCWitness to help with this. And tidied up create_test_hash_and_preimage as a test helper in these tests.
many tests added and updated, copied from PR #1208 . Very helpful to have those @lescuer97 ! It exposed an error in how I was processing the hashing of pre-images; I wasn't doing the hex decoding.
This PR still has the 29 new tests, covering many combinations of SIG_INPUTS/SIG_ALL, P2PK/HTLC, pre-locktime/post-locktime. These tests directly ensure that process_swap_request and process_melt_request are making the right decision to accept or reject. Our goal is not just to have correct individual functions, such as Proof.verify_htlc(), but to ensure that they are being calling correctly by process_*_request

Next, I'll look at @lescuer97's #1208. I'll probably do a git checkout and then run those 29 tests from this PR (they're all under /crates/cdk/src/mint/swap/spending_conditions, in order to see how that code does with those tests.

Here's an update on the latest batch of changes, just pushed above: - more new functions to improve code re-use. For example, `verify_htlc_preimage` which does the hex processing and verifies that the preimage has the required hash. This is shared between both the SIG_INPUTS path and the SIG_ALL path. Also, a new method `preimage_data` in `HTLCWitness` to help with this. And tidied up `create_test_hash_and_preimage` as a test helper in these tests. - many tests added and updated, copied from PR #1208 . Very helpful to have those @lescuer97 ! It exposed an error in how I was processing the hashing of pre-images; I wasn't doing the hex decoding. - This PR still has the 29 new tests, covering many combinations of SIG_INPUTS/SIG_ALL, P2PK/HTLC, pre-locktime/post-locktime. These tests directly ensure that `process_swap_request` and `process_melt_request` are making the right decision to accept or reject. Our goal is not just to have correct individual functions, such as `Proof.verify_htlc()`, but to ensure that they are being calling correctly by `process_*_request` Next, I'll look at @lescuer97's #1208. I'll probably do a `git checkout` and then run those 29 tests from this PR (they're all under `/crates/cdk/src/mint/swap/spending_conditions`, in order to see how that code does with those tests.

SatsAndSports commented

2025-11-08 16:02:28 +00:00

(Migrated from github.com)

TODOs (today hopefully):

sync with 'origin/main', with tests still passing
update melt to use the verify_conditions, with some tests
just format, and anything else related to linting
are there some #[ignore] tests? Maybe can be activated again the with new test vectors. Answer: resolved, those tests have now been updated to the new test vectors that are copied in from nuts/tests/11-test.md
deal with the 'unused' warnings
some comments just refer to 'swap', but this works for 'melt' too. Update the comments
is melt's verify_inputs now doing redundant checks? (Answer: no, it's not. Nothing go do here

TODOs (today hopefully): - [x] sync with 'origin/main', with tests still passing - [x] update melt to use the verify_conditions, with some tests - [x] `just format`, and anything else related to linting - [x] are there some `#[ignore]` tests? Maybe can be activated again the with new test vectors. Answer: resolved, those tests have now been updated to the new test vectors that are copied in from `nuts/tests/11-test.md` - [x] deal with the 'unused' warnings - [ ] some comments just refer to 'swap', but this works for 'melt' too. Update the comments - [x] is melt's `verify_inputs` now doing redundant checks? (Answer: no, it's not. Nothing go do here

thesimplekid (Migrated from github.com) reviewed

2025-11-09 15:30:06 +00:00

thesimplekid (Migrated from github.com) left a comment

Nice work mostly superficial organization comments other then that looks good.

crates/cashu/src/nuts/nut03.rs

					
				@ -91,6 +91,32 @@ impl SwapRequest {

				    }

We don't include the keyset_id anymore right?

crates/cashu/src/nuts/nut05.rs

Same here we don't include keyset_id

Can you use where here https://github.com/cashubtc/cdk/blob/main/CODE_STYLE.md#generics

2025-11-09 19:02:16 +00:00

					
				@ -108,0 +194,4 @@

				    witness: &super::nut14::HTLCWitness,

				    secret: &Secret,

				) -> Result<(), super::nut14::Error> {

				    use bitcoin::hashes::sha256::Hash as Sha256Hash;

Can we move this to the top with other imports

crates/cdk/src/mint/melt/mod.rs

					
				@ -0,0 +21,4 @@

				use crate::amount::to_unit;

				use crate::nuts::MeltQuoteState;

				use crate::types::PaymentProcessorKey;

				use crate::util::unix_time;

We should rename this test mod to just test and not spending conditions.

crates/cdk/src/mint/melt/spending_conditions/htlc_sigall_spending_conditions_tests.rs

I don't think we should need the cfg test here since the whole mod is a test.

crates/cdk/src/mint/swap/mod.rs

					
				@ -3,12 +3,15 @@ use cdk_prometheus::METRICS;

				use swap_saga::SwapSaga;

We should rename this test mod to just test and not spending conditions.

SatsAndSports (Migrated from github.com) reviewed

crates/cashu/src/nuts/nut05.rs

Correct. This comment is out of date

SatsAndSports (Migrated from github.com) reviewed

2025-11-09 19:02:25 +00:00

crates/cashu/src/nuts/nut03.rs

					
				@ -91,6 +91,32 @@ impl SwapRequest {

				    }

Correct. This comment is out of date

SatsAndSports commented

2025-11-11 22:06:07 +00:00

(Migrated from github.com)

I've just pushed a few changes. I think I've taken account of all remaining review comments, @thesimplekid

remove two misleading references to 'keyset_id' in comments
per the style guide, used where with generics, and brought the use std... to the top
rename the spending_conditions folders to tests, as they contain just tests
move the test helpers into the existing test_helpers folder
remove some redundant cfg(test) lines
rerun just format
merge with origin/main

I've just pushed a few changes. I think I've taken account of all remaining review comments, @thesimplekid - remove two misleading references to 'keyset_id' in comments - per the style guide, used `where` with generics, and brought the `use std...` to the top - rename the `spending_conditions` folders to `tests`, as they contain just tests - move the test helpers into the existing `test_helpers` folder - remove some redundant `cfg(test)` lines - rerun `just format` - merge with `origin/main`

SatsAndSports (Migrated from github.com) reviewed

2025-11-12 10:49:32 +00:00