feat: initial handshake network peer support #422
Conversation
📝 WalkthroughWalkthroughThis PR adds a handshake protocol under internal/handshake/protocol: a messaging layer with framing, encoding/decoding, Message interface, concrete message types (including MsgVersion, NetAddress, MsgVerack) and many placeholder message types; a Peer type that manages TCP connections and performs a Version/Verack handshake (Connect, NewPeer, Close, send/receive and handshake flow); and unit tests for MsgVersion encode/decode. It also adds license header comments to internal/handshake/protocol/genesis/genesis.go and internal/handshake/protocol/network.go. No existing exported APIs outside this subsystem were removed or changed. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (2)internal/handshake/protocol/messages_test.go (1)
internal/handshake/protocol/peer.go (1)
⏰ 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)
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 |
agaffney
force-pushed
the
feat/handshake-peer
branch
from
89b2383 to
6d37e6f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
internal/handshake/protocol/messages_test.go (1)
16-78: LGTM: Solid round-trip encoding test.The test validates both decode and encode paths using known-good data from hsd and cdnsd. The round-trip verification (decode → encode → compare) provides confidence in the implementation.
Consider adding error case tests for robustness:
- Malformed/truncated input data
- Invalid lengths
- Out-of-bounds field values
internal/handshake/protocol/peer.go (1)
94-115: Consider adding read timeouts to prevent indefinite blocking.The
receiveMessagemethod could block indefinitely if a peer stops sending data. This could be exploited for DoS or cause goroutine leaks.Consider setting a read deadline before
io.ReadFullcalls:func (p *Peer) receiveMessage() (Message, error) { + if err := p.conn.SetReadDeadline(time.Now().Add(30 * time.Second)); err != nil { + return nil, fmt.Errorf("set read deadline: %w", err) + } headerBuf := make([]byte, messageHeaderLength) if _, err := io.ReadFull(p.conn, headerBuf); err != nil { return nil, fmt.Errorf("read header: %w", err) } // ... rest of function }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/handshake/protocol/genesis/genesis.go(1 hunks)internal/handshake/protocol/messages.go(1 hunks)internal/handshake/protocol/messages_test.go(1 hunks)internal/handshake/protocol/network.go(1 hunks)internal/handshake/protocol/peer.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/handshake/protocol/peer.go (1)
internal/handshake/protocol/messages.go (8)
Message(37-40)MsgVersion(102-111)NetAddress(160-167)MessageVersion(19-19)MsgVerack(209-209)MsgVerack(211-214)MsgVerack(216-219)MessageVerack(20-20)
internal/handshake/protocol/messages_test.go (1)
internal/handshake/protocol/messages.go (3)
Message(37-40)MsgVersion(102-111)NetAddress(160-167)
🪛 GitHub Actions: golangci-lint
internal/handshake/protocol/messages.go
[error] 116-116: golangci-lint: Error return value of binary.Write is not checked (errcheck).
🪛 GitHub Check: lint
internal/handshake/protocol/peer.go
[failure] 123-123:
G115: integer overflow conversion int64 -> uint64 (gosec)
internal/handshake/protocol/messages.go
[failure] 50-50:
G115: integer overflow conversion int -> uint32 (gosec)
[failure] 189-189:
Error return value of binary.Write is not checked (errcheck)
[failure] 174-174:
Error return value of binary.Write is not checked (errcheck)
[failure] 172-172:
Error return value of binary.Write is not checked (errcheck)
[failure] 131-131:
Error return value of binary.Write is not checked (errcheck)
[failure] 125-125:
Error return value of binary.Write is not checked (errcheck)
[failure] 120-120:
Error return value of binary.Write is not checked (errcheck)
[failure] 118-118:
Error return value of binary.Write is not checked (errcheck)
[failure] 116-116:
Error return value of binary.Write is not checked (errcheck)
🔇 Additional comments (5)
internal/handshake/protocol/network.go (1)
1-6: LGTM: License header added.The MIT license header is properly formatted and appropriate for the project.
internal/handshake/protocol/genesis/genesis.go (1)
1-6: LGTM: License header added.The MIT license header is properly formatted and consistent with other files in the PR.
internal/handshake/protocol/peer.go (1)
118-164: LGTM: Handshake protocol implementation.The handshake flow correctly implements the Version/Verack exchange protocol: send Version, await Verack, await peer's Version, send Verack. Error handling and message type validation are appropriate.
internal/handshake/protocol/messages.go (2)
37-73: LGTM: Clean message framing architecture.The
Messageinterface and encode/decode helper functions provide a clean abstraction for the protocol messaging layer. The header structure and payload handling are well-designed.
160-207: LGTM: NetAddress encoding handles IPv4/IPv6 correctly.The NetAddress implementation correctly handles both IPv4 and IPv6 addresses with proper encoding (IPv4-mapped IPv6 format for IPv4). The use of big-endian for Port while using little-endian for other fields appears intentional based on the protocol specification.
| header := &msgHeader{ | ||
| NetworkMagic: networkMagic, | ||
| MessageType: msgType, | ||
| PayloadLength: uint32(len(payload)), |
There was a problem hiding this comment.
Address integer overflow conversion.
Static analysis flags the conversion of len(payload) (int) to uint32. On 64-bit systems, len() could theoretically exceed math.MaxUint32, though the check at line 43 prevents this in practice.
Make the bounds check explicit:
func encodeMessage(msgType uint8, payload []byte, networkMagic uint32) ([]byte, error) {
- if len(payload) > messageMaxPayloadLength {
+ payloadLen := len(payload)
+ if payloadLen > messageMaxPayloadLength {
return nil, errors.New("payload is too large")
}
+ if payloadLen > math.MaxUint32 {
+ return nil, errors.New("payload exceeds uint32 maximum")
+ }
- msg := make([]byte, messageHeaderLength+len(payload))
+ msg := make([]byte, messageHeaderLength+payloadLen)
header := &msgHeader{
NetworkMagic: networkMagic,
MessageType: msgType,
- PayloadLength: uint32(len(payload)),
+ PayloadLength: uint32(payloadLen),
}You'll need to import math for math.MaxUint32.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: lint
[failure] 50-50:
G115: integer overflow conversion int -> uint32 (gosec)
🤖 Prompt for AI Agents
In internal/handshake/protocol/messages.go around line 50, the code casts
len(payload) to uint32 without an explicit bounds check against math.MaxUint32;
update the code to explicitly check that len(payload) <= math.MaxUint32
(importing the math package) and return an error if it exceeds that limit, then
safely cast to uint32 when setting PayloadLength to avoid any integer overflow
on 64-bit systems.
| func (m *MsgVersion) Encode() []byte { | ||
| buf := new(bytes.Buffer) | ||
| // Protocol version | ||
| binary.Write(buf, binary.LittleEndian, m.Version) |
There was a problem hiding this comment.
Fix unchecked error returns from binary.Write (pipeline failure).
Multiple binary.Write calls have unchecked error returns, which is flagged by the linter and causing pipeline failures. While binary.Write to bytes.Buffer rarely fails (only on OOM), the errors should be handled for correctness.
Apply this pattern for error checking:
func (m *MsgVersion) Encode() []byte {
buf := new(bytes.Buffer)
// Protocol version
- binary.Write(buf, binary.LittleEndian, m.Version)
+ if err := binary.Write(buf, binary.LittleEndian, m.Version); err != nil {
+ panic(fmt.Sprintf("failed to write version: %v", err))
+ }
// Services
- binary.Write(buf, binary.LittleEndian, m.Services)
+ if err := binary.Write(buf, binary.LittleEndian, m.Services); err != nil {
+ panic(fmt.Sprintf("failed to write services: %v", err))
+ }
// ... repeat for other binary.Write callsApply similar fixes to:
- Line 118 (Services)
- Line 120 (Time)
- Line 125 (Nonce)
- Line 131 (Height)
- Line 172 (Time in NetAddress)
- Line 174 (Services in NetAddress)
- Line 189 (Port in NetAddress)
Alternatively, change Encode methods to return ([]byte, error) and propagate errors properly.
Also applies to: 118-118, 120-120, 125-125, 131-131, 172-172, 174-174, 189-189
🧰 Tools
🪛 GitHub Actions: golangci-lint
[error] 116-116: golangci-lint: Error return value of binary.Write is not checked (errcheck).
🪛 GitHub Check: lint
[failure] 116-116:
Error return value of binary.Write is not checked (errcheck)
🤖 Prompt for AI Agents
internal/handshake/protocol/messages.go lines 116,118,120,125,131,172,174,189:
binary.Write calls currently ignore returned errors; update each call to capture
the error (err := binary.Write(...)) and handle it by returning the error up the
stack (modify the Encode/Marshal methods to return ([]byte, error) or error as
appropriate) or at minimum wrap and return fmt.Errorf(...) so callers can handle
failures; ensure signatures and all call sites are updated to propagate the new
error return.
| func (m *MsgVersion) Decode(data []byte) error { | ||
| m.Version = binary.LittleEndian.Uint32(data[0:4]) | ||
| m.Services = binary.LittleEndian.Uint64(data[4:12]) | ||
| m.Time = binary.LittleEndian.Uint64(data[12:20]) | ||
| if err := m.Remote.Decode(data[20:108]); err != nil { | ||
| return err | ||
| } | ||
| m.Nonce = [8]byte(data[108:116]) | ||
| userAgentLength := int(data[116]) | ||
| m.Agent = string(data[117 : 117+userAgentLength]) | ||
| m.Height = binary.LittleEndian.Uint32(data[117+userAgentLength : 117+userAgentLength+4]) | ||
| noRelayByte := data[117+userAgentLength+4] | ||
| m.NoRelay = false | ||
| if noRelayByte == 1 { | ||
| m.NoRelay = true | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Add bounds checking to prevent panic.
MsgVersion.Decode accesses data[] at fixed offsets without validating that data is sufficiently long. If malformed or truncated data is provided, this will panic rather than returning an error.
Add length validation:
func (m *MsgVersion) Decode(data []byte) error {
+ // Minimum length: 4 + 8 + 8 + 88 + 8 + 1 + 0 + 4 + 1 = 122 bytes (with 0-length agent)
+ if len(data) < 122 {
+ return fmt.Errorf("data too short for MsgVersion: got %d bytes, need at least 122", len(data))
+ }
m.Version = binary.LittleEndian.Uint32(data[0:4])
m.Services = binary.LittleEndian.Uint64(data[4:12])
m.Time = binary.LittleEndian.Uint64(data[12:20])
if err := m.Remote.Decode(data[20:108]); err != nil {
return err
}
m.Nonce = [8]byte(data[108:116])
userAgentLength := int(data[116])
+ if len(data) < 117+userAgentLength+5 {
+ return fmt.Errorf("data too short for agent string and remaining fields: got %d bytes, need %d", len(data), 117+userAgentLength+5)
+ }
m.Agent = string(data[117 : 117+userAgentLength])
m.Height = binary.LittleEndian.Uint32(data[117+userAgentLength : 117+userAgentLength+4])
noRelayByte := data[117+userAgentLength+4]
m.NoRelay = false
if noRelayByte == 1 {
m.NoRelay = true
}
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (m *MsgVersion) Decode(data []byte) error { | |
| m.Version = binary.LittleEndian.Uint32(data[0:4]) | |
| m.Services = binary.LittleEndian.Uint64(data[4:12]) | |
| m.Time = binary.LittleEndian.Uint64(data[12:20]) | |
| if err := m.Remote.Decode(data[20:108]); err != nil { | |
| return err | |
| } | |
| m.Nonce = [8]byte(data[108:116]) | |
| userAgentLength := int(data[116]) | |
| m.Agent = string(data[117 : 117+userAgentLength]) | |
| m.Height = binary.LittleEndian.Uint32(data[117+userAgentLength : 117+userAgentLength+4]) | |
| noRelayByte := data[117+userAgentLength+4] | |
| m.NoRelay = false | |
| if noRelayByte == 1 { | |
| m.NoRelay = true | |
| } | |
| return nil | |
| } | |
| func (m *MsgVersion) Decode(data []byte) error { | |
| // Minimum length: 4 + 8 + 8 + 88 + 8 + 1 + 0 + 4 + 1 = 122 bytes (with 0-length agent) | |
| if len(data) < 122 { | |
| return fmt.Errorf("data too short for MsgVersion: got %d bytes, need at least 122", len(data)) | |
| } | |
| m.Version = binary.LittleEndian.Uint32(data[0:4]) | |
| m.Services = binary.LittleEndian.Uint64(data[4:12]) | |
| m.Time = binary.LittleEndian.Uint64(data[12:20]) | |
| if err := m.Remote.Decode(data[20:108]); err != nil { | |
| return err | |
| } | |
| m.Nonce = [8]byte(data[108:116]) | |
| userAgentLength := int(data[116]) | |
| if len(data) < 117+userAgentLength+5 { | |
| return fmt.Errorf("data too short for agent string and remaining fields: got %d bytes, need %d", len(data), 117+userAgentLength+5) | |
| } | |
| m.Agent = string(data[117 : 117+userAgentLength]) | |
| m.Height = binary.LittleEndian.Uint32(data[117+userAgentLength : 117+userAgentLength+4]) | |
| noRelayByte := data[117+userAgentLength+4] | |
| m.NoRelay = false | |
| if noRelayByte == 1 { | |
| m.NoRelay = true | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In internal/handshake/protocol/messages.go around lines 141 to 158,
MsgVersion.Decode reads fixed offsets from data without verifying length which
can cause panics on truncated input; add explicit bounds checks before each
slice/index operation (ensure data length >= required minimum at start for the
fixed-size header, then validate there are enough bytes for Remote.Decode call,
nonce, userAgent length byte, the userAgent payload, and the following height
and noRelay byte), validate userAgentLength is sane and won’t overflow slice
bounds, propagate or wrap errors from Remote.Decode, and return a descriptive
error instead of letting a panic occur.
| func (p *Peer) Connect(address string) error { | ||
| if p.conn != nil { | ||
| return errors.New("connection already established") | ||
| } | ||
| var err error | ||
| p.conn, err = net.DialTimeout("tcp", address, dialTimeout) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| p.address = address | ||
| if err := p.handshake(); err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Clean up connection on handshake failure.
If the handshake fails after establishing the connection, the connection remains open but the Peer is in an invalid state. This could leak resources.
Apply this diff:
func (p *Peer) Connect(address string) error {
if p.conn != nil {
return errors.New("connection already established")
}
var err error
p.conn, err = net.DialTimeout("tcp", address, dialTimeout)
if err != nil {
return err
}
p.address = address
if err := p.handshake(); err != nil {
+ p.conn.Close()
+ p.conn = nil
return err
}
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (p *Peer) Connect(address string) error { | |
| if p.conn != nil { | |
| return errors.New("connection already established") | |
| } | |
| var err error | |
| p.conn, err = net.DialTimeout("tcp", address, dialTimeout) | |
| if err != nil { | |
| return err | |
| } | |
| p.address = address | |
| if err := p.handshake(); err != nil { | |
| return err | |
| } | |
| return nil | |
| } | |
| func (p *Peer) Connect(address string) error { | |
| if p.conn != nil { | |
| return errors.New("connection already established") | |
| } | |
| var err error | |
| p.conn, err = net.DialTimeout("tcp", address, dialTimeout) | |
| if err != nil { | |
| return err | |
| } | |
| p.address = address | |
| if err := p.handshake(); err != nil { | |
| p.conn.Close() | |
| p.conn = nil | |
| return err | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
internal/handshake/protocol/peer.go around lines 50 to 64: when Connect
establishes p.conn then calls p.handshake, a failed handshake leaves the TCP
connection open and p in an invalid state; update the error path so that if
handshake() returns an error you close the connection (call p.conn.Close()), set
p.conn = nil (and optionally clear p.address), then return the handshake error;
ensure you only close when p.conn is non-nil and preserve the original error
returned by handshake.
internal/handshake/protocol/peer.go
Outdated
| if _, err := rand.Read(nonce); err != nil { | ||
| return err | ||
| } | ||
| timeNow := uint64(time.Now().Unix()) |
There was a problem hiding this comment.
Address integer overflow conversion flagged by static analysis.
The static analysis tool flags this conversion from int64 to uint64. While time.Now().Unix() will always be positive in practice, an explicit check or comment would clarify intent and satisfy the linter.
Apply this diff:
- timeNow := uint64(time.Now().Unix())
+ timeNowUnix := time.Now().Unix()
+ if timeNowUnix < 0 {
+ return fmt.Errorf("invalid timestamp: %d", timeNowUnix)
+ }
+ timeNow := uint64(timeNowUnix)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| timeNow := uint64(time.Now().Unix()) | |
| timeNowUnix := time.Now().Unix() | |
| if timeNowUnix < 0 { | |
| return fmt.Errorf("invalid timestamp: %d", timeNowUnix) | |
| } | |
| timeNow := uint64(timeNowUnix) |
🧰 Tools
🪛 GitHub Check: lint
[failure] 123-123:
G115: integer overflow conversion int64 -> uint64 (gosec)
🤖 Prompt for AI Agents
In internal/handshake/protocol/peer.go around line 123, the direct cast
uint64(time.Now().Unix()) triggers a static-analysis warning about potential
integer overflow; replace the direct cast with an explicit int64 capture and
guard: store time.Now().Unix() in a local int64 variable, check if it is
negative and handle by setting it to 0 (or return an error if negative is
unacceptable), then cast the non-negative value to uint64; add a brief comment
explaining the guard to satisfy the linter.
6d37e6f to
17e810c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
internal/handshake/protocol/peer.go (2)
60-62: Close connection when handshake fails.If
handshake()returns an error we leave the TCP connection open andp.connnon‑nil, so the peer stays in an invalid state and leaks resources. Please close the socket and reset the peer fields before returning the error.p.address = address if err := p.handshake(); err != nil { + _ = p.conn.Close() + p.conn = nil + p.address = "" return err }
67-74: Reset state after closing a connection.
Closeshuts down the socket but leavesp.connandp.addresspopulated, so subsequentConnectcalls will refuse to run even though the underlying connection is gone. Reset the fields once the close succeeds.func (p *Peer) Close() error { - if p.conn == nil { + conn := p.conn + if conn == nil { return errors.New("connection is not established") } - if err := p.conn.Close(); err != nil { + p.conn = nil + p.address = "" + if err := conn.Close(); err != nil { return err } return nil }internal/handshake/protocol/messages.go (1)
141-156: Add length guards in MsgVersion.Decode.
MsgVersion.Decodeslices fixed offsets before checking the input length. Malformed or truncated payloads will panic the process. Please validate the buffer length (including the agent length byte and remaining fields) before slicing.func (m *MsgVersion) Decode(data []byte) error { + const minPayloadWithoutAgent = 122 + if len(data) < minPayloadWithoutAgent { + return fmt.Errorf("data too short for MsgVersion: got %d bytes", len(data)) + } m.Version = binary.LittleEndian.Uint32(data[0:4]) m.Services = binary.LittleEndian.Uint64(data[4:12]) m.Time = binary.LittleEndian.Uint64(data[12:20]) if err := m.Remote.Decode(data[20:108]); err != nil { return err } m.Nonce = [8]byte(data[108:116]) userAgentLength := int(data[116]) + requiredLen := 117 + userAgentLength + 5 + if len(data) < requiredLen { + return fmt.Errorf("data too short for agent (%d bytes) and remaining fields: got %d bytes", userAgentLength, len(data)) + } m.Agent = string(data[117 : 117+userAgentLength]) m.Height = binary.LittleEndian.Uint32(data[117+userAgentLength : 117+userAgentLength+4]) noRelayByte := data[117+userAgentLength+4]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/handshake/protocol/genesis/genesis.go(1 hunks)internal/handshake/protocol/messages.go(1 hunks)internal/handshake/protocol/messages_test.go(1 hunks)internal/handshake/protocol/network.go(1 hunks)internal/handshake/protocol/peer.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/handshake/protocol/network.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/handshake/protocol/genesis/genesis.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/handshake/protocol/peer.go (1)
internal/handshake/protocol/messages.go (8)
Message(37-40)MsgVersion(102-111)NetAddress(160-167)MessageVersion(19-19)MsgVerack(209-209)MsgVerack(211-214)MsgVerack(216-219)MessageVerack(20-20)
internal/handshake/protocol/messages_test.go (1)
internal/handshake/protocol/messages.go (3)
Message(37-40)MsgVersion(102-111)NetAddress(160-167)
⏰ 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)
* manage a TCP connection with a peer * send/receive messages * encode/decode message headers * encode/decode Version/Verack messages, with tests * perform initial protocol handshake Signed-off-by: Aurora Gaffney <aurora@blinklabs.io>
agaffney
force-pushed
the
feat/handshake-peer
branch
from
17e810c to
5a48937
Compare
Summary by CodeRabbit
New Features
Tests
Chores