multi: validate server-provided signing data and clean up comments#1148
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request strengthens the security and robustness of the client by enforcing strict validation on all signing data and public keys received from the server. By ensuring that MuSig2 nonces, signatures, and public keys conform to expected formats, the client is better protected against potential errors or malicious inputs. The changes also include minor documentation and test cleanup to improve the overall quality of the codebase. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request focuses on improving MuSig2 signature and nonce validation, refactoring server public key parsing, and correcting various typos across the codebase. Specifically, it introduces strict length checks for MuSig2 nonces and partial signatures in both the withdrawal manager and sweep batcher, updates test mocks to return valid dummy signing data instead of nil values, and adds comprehensive unit tests for these validation paths. There are no review comments to address, so no additional feedback is provided.
|
@claude review this |
Code ReviewPR Summary: This PR hardens MuSig2 handling by validating server-provided signing data (nonces, partial signatures, public keys) before use, and fixes several comment/typo issues across the codebase. OverviewA solid security-hardening PR. The core change is rejecting malformed server data early — before passing it to the signer — in three distinct code paths:
Additionally, Positive Observations
Issues and Suggestions1. Missing negative tests for The new guards in 2. Missing negative test for
3. Duplicated The function is defined independently in both 4. Zero-value mock nonce/sig may mask downstream signing failures
5. Minor: The function copies Test Coverage
Typo/Comment FixesAll correct and appreciated: "manger" to "manager", "dispach" to "dispatch", "dns" to "data source name", "sever" to "server" (x4), "cancelation" to "cancellation", "at wide" to "at large", "prepush" to "prepayment", "timout" to "timeout". SummaryThe security hardening is correct and well-targeted. The main gap is that two of the three new validation paths ( |
hieblmi
force-pushed
the
harden-musig2-handling
branch
from
b681f73 to
3045e82
Compare
starius
left a comment
starius
left a comment
There was a problem hiding this comment.
LGTM! 🎉
Thanks for fixing it!
| }, nil | ||
| } | ||
|
|
||
| func parseServerPubKey(name string, keyBytes []byte) ([33]byte, error) { |
| return nil, err | ||
| } | ||
|
|
||
| var receiverInternalKey [33]byte |
There was a problem hiding this comment.
s/33/btcec.PubKeyBytesLenCompressed/
Here and below
Server-supplied nonces and partial signatures are consumed by the static address loop-in and withdrawal MuSig2 signing paths. Reject nil signing info, wrong nonce lengths, and wrong partial signature lengths before registering nonces or combining signatures, so malformed responses cannot be silently zero-padded into signing attempts. Add withdrawal coverage for nil and malformed server signing data.
The cooperative batch sweep path receives a server nonce and partial signature before constructing a keyspend witness. Validate both byte slice lengths before registering the nonce or combining signatures, so malformed server responses fail explicitly instead of being zero-padded into fixed-size MuSig2 buffers. Update batcher test helpers to return size-correct placeholder signing data under the stricter validation.
Loop-in and loop-out responses carry compressed server public keys that are copied into fixed-size fields and later used for HTLC construction. Validate the length and parse each compressed key before storing it, and validate the MuSig2 loop-in receiver internal key as well. This turns short or unparsable server keys into explicit errors instead of silently zero-padding short responses or accepting an invalid internal key. Update root test mocks to return size-correct MuSig2 signing data under the stricter checks.
hieblmi
force-pushed
the
harden-musig2-handling
branch
from
d61f207 to
cc0392a
Compare
2 participants
This PR does: