fix a variety of issues with SIG_ALL and its interaction with P2PK and HTLC#1008
fix a variety of issues with SIG_ALL and its interaction with P2PK and HTLC#1008SatsAndSports wants to merge 5 commits into
Conversation
|
Update a few days later: ready for review now |
| from .errors import InvalidProofsError | ||
| from .secret import Secret, SecretKind | ||
|
|
||
| if TYPE_CHECKING: |
There was a problem hiding this comment.
Found a fix, i.e. removing the TYPE_CHECKING import and if entirely. But I'm curious what you think before pushing the fix
TL:DR it's about an import that was only used by ruff and not actually used at runtime. Therefore, the idea is to avoid this unnecessary-at-runtime import, perhaps making runtime slightly more efficient and perhaps making the code more maintainable by signalling to maintainers which imports are really needed for which purpose
This PR introduces a new function def sig_all_swap_message(proofs: List["Proof"], outputs: List["BlindedMessage"]) -> str:. I'll double check if this new function is necessary; maybe it's redundant in this draft PR
(With the fix, we can remove the " from that signature and simply use List[Proof] and List[BlindedMessage]. But that's a slightly different issue to the issue described in this comment)
ruff needs an import of Proof and BlindedMessage, otherwise we get this error : cashu/core/p2pk.py:76:64: F821 Undefined name BlindedMessage. So that requires importing those two. However, as this import is needed only by ruff (and maybe also by mypy?) and is not actually used at runtime, we can use the if TYPE_CHECKING guard to skip the runtime import.
Question. TYPE_CHECKING allows us to skip imports that aren't needed at runtime, and which are needed only for linting and type-checking. It might make the code more maintainable, by helping readers understand what code is really needed. Fewer runtime deps means a smaller chance of things going run in complex ways at runtime. But it might make things less maintainable for maintainers who don't know TYPE_CHECKING (I discovered it only yesterday!). Should we leave this as-is, using TYPE_CHECKING like this, or just go with the import at runtime?
There was a problem hiding this comment.
I've removed that TYPE_CHECKING stuff. I find it interesting, and tempted to keep it. But it's a broader issue, unrelated to the bug-fixing scope of this PR, and therefore I've removed it
be6b51b to
97d0cbe
Compare
dceb2d5 to
e6b23b6
Compare
21d1401 to
c62a4a4
Compare
…em now all the time; swap/melt now has the appropriate checks elsewhere
| "does not enforce spending conditions in this path. Treating it " | ||
| "as a plain secret." | ||
| ) | ||
| except Exception: |
There was a problem hiding this comment.
Are auth tokens supposed to follow NUT-10?
If I remember correctly, the existing data structure for an auth proof doesn't have a witness field and therefore I guess it couldn't have been enforced. Therefore, I added this code to make it more explicit.
I couldn't see an easily way to integrate the NUT-10 logic into auth, in the same way that it's integrated into swap and melt.
Should we add something more explicit in the nuts, perhaps adjust NUT-10 to emphasize that it's for swap and melt and not for auth?
❌ 3 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
|
@SatsAndSports needs rebase on the latest main |
Fixes Issue: #1009 which reports a variety of issues related to P2PK and HTLC and SIG_ALL. Even if some of those features have been partially implemented and tested together, problems arise when they are combined; for example, a post-locktime HTLC with SIG_ALL
Starting with the mint:
The existing code had NUT-10 code (i.e. P2PK and HTLC) in multiple places. In this PR, following the CDK architecture, all the NUT-10 logic is moved into a single boolean function. This unifies checking for swap and melt and minimizes code duplication.
The main functions in
cashu/mint/conditions.pynow are:_verify_transactionis a new entry point for all verification of any swap or melt._verify_input_output_spending_conditionsis an existing function that has been rewritten to verify the inputs and outputs (and quote, for a melt), looking just at the NUT-10 spending conditions (i.e. P2PK and HTLC). It returns True or raises an error. It delegates to various new helpers to deal with all the various P2PK or HTLC or SIG_ALL._verify_p2pk_or_htlc_spending_requirementsis the low level function to count valid signatures and, if necessary, the pre-image. This one function is reused across all the various contexts, for any NUT-10 secret that appears in the inputs of any swap or melt.DataStructures: This PR introduces a similar
SpendingRequirementsclass as used in the CDK, to unify the requirements encoded in a P2PK/HTLC Secret.Wallet: The only wallet change here is to keep up with the new SIG_ALL message format.