-
Notifications
You must be signed in to change notification settings - Fork 109
(ABANDONED, replaced by PR 1212) fix for SIG_ALL and P2PK, ensuring it works with swap and melt. Also, after the locktime, support SIG_ALL with refund keys #1195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
|
verify_proofs was too strict, blocking swap and mint from using SIG_ALLverify_proofs was too strict, blocking swap and mint from using SIG_ALL
…e working. This commit adds of of tracing, and skips the signature verification for SIG_ALL
…ast_one_sig_all_proof' is a better-named alternative
…and it has been overhauled to be aware of locktime. If the locktime has passed, then it is based on refund keys instead of the 'normal' pubkeys
|
The description above was written by a human. Here's how Claude Code summarizes all these changes: ============== |
verify_proofs was too strict, blocking swap and mint from using SIG_ALL
kwsantiago
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments. Also you have decent test coverage but consider adding tests for:
- Locktime expired with refund keys
- Locktime expired without refund keys (anyone can spend)
- Mixed
SigAllandSigInputsproofs in the same transaction SigAllwith insufficient signatures at the transaction level- HTLC proofs with the
SigAllflag
| let msg: &[u8] = self.secret.as_bytes(); | ||
|
|
||
| // Ensure this is only called for SigInputs proofs | ||
| assert_eq!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assert_eq! will panic and crash the mint if the function is called incorrectly. In production, this could be exploited to DoS the mint.
Please replace with proper error handling, something like:
if spending_conditions.sig_flag != SigFlag::SigInputs {
return Err(Error::IncorrectSigFlag); // or appropriate error type
}
| Ok((refund_keys, refund_sigs_required)) | ||
| } else { | ||
| // Locktime passed but no refund keys - anyone can spend (0 sigs required) | ||
| Ok((vec![], 0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new get_sig_all_required_sigs() function introduces "anyone can spend" behavior when:
- Locktime has passed
- AND no refund keys are present
- Returns
required_sigs = 0which causes immediate success
Questions
- Is this behavior intentional and documented in the NUT-11 spec?
- Could existing proofs with locktime-only (no refund keys) become unexpectedly spendable?
- Should there be migration warnings for wallets that may have created such proofs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm tempted to add a new function called something like enforce_all_nut10_conditions. This would have full responsibility for checking anything related to NUT-10:
- is there any proof with a nut-10 secret?
- if yes, are the conditions (pre-images, locktimes, signatures, ... everything) for the proof satisfied?
- if there any SIG_ALL, don't forget all the special stuff
This function would NOT be responsible for anything that is unrelated to nut-10:
- has this proof already beent spent (or
PENDING)? - was it signed with the correct mint key?
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. One suggestion is to make sure the function name clearly indicates it returns a bool or Result, verify_nut10_conditions() or validate_nut10_conditions() might be slightly clearer than enforce.
|
|
||
| /// Get the signature flag that should be enforced for a set of proofs and the | ||
| /// public keys that signatures are valid for | ||
| pub fn enforce_sig_flag(proofs: Proofs) -> EnforceSigFlag { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change. If any external code was using enforce_sig_flag() or the EnforceSigFlag type, it will break.
Should this be noted in CHANGELOG as a breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I had forgotten that publicly-facing functions shouldn't be changed!
I was focussed on getting P2PK+SIG_ALL to work, before and after the locktime, and I didn't think about other things like that
| .unwrap_or_default() | ||
| .try_into()?; | ||
|
|
||
| if conditions.sig_flag == nuts::nut11::SigFlag::SigAll { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good fix but the conditions are extracted by parsing tags. If the tags are malformed or missing, this could fail silently (empty default). Might want to add validation that tags were actually present for P2PK secrets.
| let (relevant_pubkeys, relevant_num_sigs_required) = get_sig_all_required_sigs(self.inputs())?; | ||
|
|
||
| if relevant_num_sigs_required == 0 { | ||
| return Ok(()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This early exit for "anyone can spend" looks correct but needs to be verified against NUT-11 spec for locktime behavior.
|
I'm abandoning this PR and replacing it with #1212 I wrote this PR to help fix some bugs that were affecting the Spillman channel, but I discovered new bugs later (in this PR, and also in the existing code), so it made sense to redo it. Also, the 'swap_saga' PR which was just merged also made this PR invalid Thanks to anyone who reviewed this one, e.g. @kwsantiago , I hope I've addressed your concerns in the new PR |

Update 2025-10-25: Abandoning this for various reasons.
Description
While implementing the unidirectional channel proposal, I found that I couldn't swap with P2PK SIG_ALL. To fix that, I had to make more and more changes, especially to ensure that locktime and refunds work as expected.
I mostly ignored HTLCs+SIG_ALL in this. I'll think about that next.
See also the separate comment below with a full description of this PR written by Claude
The problem
There is a
validate_sig_flagandverify_sig_all, and they are called fromprocess_swap_requestandverify_melt_request. But in reality the code doesn't reach those lines in the case of SIG_ALLMelt and swap call
verify_inputs, which calledverify_proofs, andverify_proofsrejects SIG_ALL proofs (as it doesn't know what the outputs are and therefore it can't verify the SIG_ALL).Therefore, SIG_ALL transactions don't work in melt or swap; they are rejected before it reaches the code which would validate them.
Changes in this PR, to address the SIG_ALL issue:
This PR does the following:
verify_inputsonly for SIG_INPUTS proofs, skipping SIG_ALL. Renamed toverify_p2pk_for_sig_inputto make this clear.verify_proofsnow silently accepts any SIG_ALL input without checking the signature, as that checking is already done elsewhereverify_sig_all. However, before this PR, it worked only with pre-locktime pubkeys; it didn't know that it should use refund keys after the locktime has expired. This PR therefore refactorsverify_sig_allsuch that it uses the locktime to decide which set of keys (refund keys, or[data]+Secret.data.pubkeys) should be used.enforce_sig_flag. It included some complex logic, but in practice most of its response was ignored; the rest of the library only usedenforce_sig_flagto do the binary test "does this set of proofs include at least one SIG_ALL?". This PR therefore renames it tohas_at_least_one_sig_all_proofto simply this.While I've tested this with P2PKs inside swap and melt, and my Spillman channel code (not in this PR), I haven't thought so much about HTLCs. So maybe more work is needed for HTLCs
Suggested CHANGELOG Updates
...
FIXED
verify_proofsto accept SIG_ALL proofs. The signature verification is done else whereChecklist
just final-checkbefore committing