feat: support for getting name proof from Handshake peer #425
Conversation
📝 WalkthroughWalkthroughThe diff adds MsgGetProof (Root, Key) with Encode/Decode and extends MsgProof to include Root, Key and a *handshake.Proof plus Encode/Decode. decodeMessage now recognizes MessageGetProof and MessageProof and uses the new types. Peer.GetProof(name, rootHash) was added: it derives Key = sha3.Sum256(name), sends MsgGetProof, awaits a response, asserts it is a MsgProof, and returns the embedded Proof. A unit test was added for MsgGetProof encode/decode round-trip. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/handshake/protocol/peer.go (1)
220-240: Remove redundant type conversion.The implementation correctly computes the SHA3-256 hash and handles the request/response flow. However, Line 225 has a redundant type conversion:
sha3.Sum256already returns[32]byte, so the explicit[32]byte(key)cast is unnecessary.Apply this diff to simplify:
key := sha3.Sum256([]byte(name)) getProofMsg := &MsgGetProof{ Root: rootHash, - Key: [32]byte(key), + Key: key, }internal/handshake/protocol/messages.go (1)
447-450: Clarify the intentional receive-only design of MsgProof.Verification confirms MsgProof is receive-only in this implementation—it's only populated via
Decode()when receiving protocol messages (peer.go:235), never sent viaEncode(). The suggested documentation update improves clarity for future maintainers:func (m *MsgProof) Encode() []byte { - // NOTE: this is not implemented because proof encoding is not implemented + // NOTE: MsgProof is only received, never sent. Encoding is intentionally unimplemented. + // If sending proofs becomes necessary, implement handshake.Proof.Encode() first. return []byte{} }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/handshake/protocol/messages.go(2 hunks)internal/handshake/protocol/peer.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/handshake/protocol/peer.go (2)
internal/handshake/spv.go (1)
Proof(63-74)internal/handshake/protocol/messages.go (3)
MsgGetProof(420-423)MessageGetProof(31-31)MsgProof(441-445)
internal/handshake/protocol/messages.go (1)
internal/handshake/spv.go (1)
Proof(63-74)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
internal/handshake/protocol/messages.go (2)
86-89: LGTM! Message routing correctly added.The decoder routing for GetProof and Proof messages follows the established pattern and integrates cleanly with existing message types.
420-439: LGTM! Well-structured encoding/decoding implementation.The MsgGetProof implementation correctly validates payload length and handles the Root and Key fields symmetrically.
internal/handshake/protocol/peer.go (1)
11-11: LGTM! Import correctly added.The
crypto/sha3import is necessary for computing the name hash in the GetProof method.
agaffney
force-pushed
the
feat/handshake-get-proof
branch
2 times, most recently
from
c74b780 to
0cc8dbf
Compare
Signed-off-by: Aurora Gaffney <aurora@blinklabs.io>
agaffney
force-pushed
the
feat/handshake-get-proof
branch
from
0cc8dbf to
a925341
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/handshake/protocol/messages.go (1)
447-450: Consider documenting or enforcing the Encode limitation.The
Encodemethod returns an empty byte slice with a comment noting it's not implemented. While this may be intentional (ifMsgProofis only received and never sent), it could lead to subtle bugs if someone attempts to send aMsgProofmessage.Consider one of these approaches:
Option 1: Add a panic to make the limitation explicit
func (m *MsgProof) Encode() []byte { - // NOTE: this is not implemented because proof encoding is not implemented - return []byte{} + // MsgProof encoding is not supported; this message is only received, not sent + panic("MsgProof.Encode is not implemented") }Option 2: Add a TODO if this should be implemented later
func (m *MsgProof) Encode() []byte { - // NOTE: this is not implemented because proof encoding is not implemented + // TODO: Implement proof encoding to support sending MsgProof messages return []byte{} }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/handshake/protocol/messages.go(2 hunks)internal/handshake/protocol/messages_test.go(1 hunks)internal/handshake/protocol/peer.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/handshake/protocol/messages_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/handshake/protocol/peer.go (2)
internal/handshake/spv.go (1)
Proof(63-74)internal/handshake/protocol/messages.go (3)
MsgGetProof(420-423)MessageGetProof(31-31)MsgProof(441-445)
internal/handshake/protocol/messages.go (1)
internal/handshake/spv.go (1)
Proof(63-74)
🔇 Additional comments (6)
internal/handshake/protocol/peer.go (2)
11-11: LGTM: Standard library import for SHA3 hashing.The addition of
crypto/sha3is appropriate for computing the key hash in the GetProof method.
222-222: Verify domain name normalization requirements against Handshake protocol specification.The review concern is valid: domain names hashed without normalization would produce different keys for equivalent domains (e.g., "Example.com" vs "example.com"), breaking Merkle tree lookups. The codebase shows normalization patterns elsewhere (hashes normalized in genesis.go and network.go), yet no domain normalization occurs in
GetProof()at line 222.However, I cannot definitively determine whether this requires fixing because:
GetProofhas no existing callers in the codebase (new API)- No protocol documentation found specifying whether domain names should be normalized before hashing
- No domain normalization functions exist in the codebase to indicate expected behavior
Verify the Handshake protocol specification to confirm:
- Whether domain names must be normalized (lowercase, punycode encoding) before hashing
- Whether normalization responsibility belongs in
GetProof()or at the caller level- Test with case-varied and international domain names to ensure correct behavior
internal/handshake/protocol/messages.go (4)
31-32: LGTM: Message type constants follow existing conventions.The new message type constants are properly defined and don't conflict with existing values.
86-89: LGTM: decodeMessage cases follow existing pattern.The new message type cases are correctly integrated into the decode switch statement.
420-439: LGTM: MsgGetProof implementation is correct.The struct definition and encode/decode methods are well-implemented with proper length validation to prevent panics.
452-465: LGTM: MsgProof.Decode properly validates and decodes.The decode method correctly validates the minimum payload length (addressing the previous review comment), extracts Root and Key fields, and delegates Proof decoding to the
handshake.Proof.Decodemethod.
| // GetProof requests a proof for a domain name from the network peer | ||
| func (p *Peer) GetProof(name string, rootHash [32]byte) (*handshake.Proof, error) { | ||
| key := sha3.Sum256([]byte(name)) | ||
| getProofMsg := &MsgGetProof{ | ||
| Root: rootHash, | ||
| Key: key, | ||
| } | ||
| if err := p.sendMessage(MessageGetProof, getProofMsg); err != nil { | ||
| return nil, err | ||
| } | ||
| // Wait for Proof response | ||
| msg, err := p.receiveMessage() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| msgProof, ok := msg.(*MsgProof) | ||
| if !ok { | ||
| return nil, fmt.Errorf("unexpected message: %T", msg) | ||
| } | ||
| return msgProof.Proof, nil | ||
| } |
There was a problem hiding this comment.
Validate response Root and Key match the request.
The method doesn't verify that the returned MsgProof contains the same Root and Key that were requested. A malicious or buggy peer could return a proof for a different domain name or tree root, which would be accepted without validation. This could lead to incorrect proof verification or security vulnerabilities.
Apply this diff to add validation:
func (p *Peer) GetProof(name string, rootHash [32]byte) (*handshake.Proof, error) {
key := sha3.Sum256([]byte(name))
getProofMsg := &MsgGetProof{
Root: rootHash,
Key: key,
}
if err := p.sendMessage(MessageGetProof, getProofMsg); err != nil {
return nil, err
}
// Wait for Proof response
msg, err := p.receiveMessage()
if err != nil {
return nil, err
}
msgProof, ok := msg.(*MsgProof)
if !ok {
return nil, fmt.Errorf("unexpected message: %T", msg)
}
+ if msgProof.Root != rootHash {
+ return nil, errors.New("proof root hash mismatch")
+ }
+ if msgProof.Key != key {
+ return nil, errors.New("proof key mismatch")
+ }
return msgProof.Proof, nil
}🤖 Prompt for AI Agents
internal/handshake/protocol/peer.go around lines 220 to 240: after receiving and
type-asserting the MsgProof, verify that msgProof.Root equals the requested
rootHash and msgProof.Key equals the requested key (sha3.Sum256([]byte(name)));
if either comparison fails, return a descriptive error (e.g., "proof root
mismatch" or "proof key mismatch" including expected vs actual) instead of
accepting the proof, otherwise return msgProof.Proof as before.
Summary by CodeRabbit
New Features
Tests