refactor: send_handshake and receive_handshake#102
Conversation
Also updated any calls to send_handshake to accomodate the changes Co-authored-by: kurealnum <oscar.gaske.cs@gmail.com>
Updated any related code + documentation Co-authored-by: kurealnum <oscar.gaske.cs@gmail.com>
Co-authored-by: kurealnum <oscar.gaske.cs@gmail.com>
Co-authored-by: kurealnum <oscar.gaske.cs@gmail.com>
|
Caution Review failedThe pull request is closed. Walkthroughsend_handshake was simplified to only transmit handshake bytes; a new recv_handshake reads/parses a 68-byte handshake and returns (PeerId, reserved). append_peer now calls send_handshake then recv_handshake; tests and logging were updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
Client->>Server: send_handshake(our_id, info_hash)
Note right of Client: send-only, no response read
Server->>Server: recv_handshake() — read 68 bytes & parse Handshake
Server-->>Server: return (peer_id, reserved)
Server->>Server: set id, update peer.reserved, determine_supported()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Suggested labelsenhancement Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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)
crates/libtortillas/src/torrent/mod.rs (1)
390-394: Use try_init to avoid panics when multiple tests initialize tracinginit() will panic if called more than once across tests. Use try_init().ok() to make it idempotent.
- tracing_subscriber::fmt() + tracing_subscriber::fmt() .with_target(true) .with_env_filter("libtortillas=trace,off") .pretty() - .init(); + .try_init() + .ok();crates/libtortillas/src/protocol/stream.rs (1)
141-151: Avoid unwrap in instrumentation fields for remote_addrremote_addr().unwrap() inside #[instrument(...)] will panic if the socket is closed before this call. Prefer deferring remote_addr into the function body where you can handle errors, or record it conditionally.
Example:
let remote = self.remote_addr().map(|a| a.to_string()).unwrap_or_else(|_| "<unknown>".into()); trace!(%remote, "Sending handshake");If you want, I can patch this to move context fields into explicit trace! calls for safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
crates/libtortillas/src/protocol/stream.rs(4 hunks)crates/libtortillas/src/torrent/mod.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-18T18:25:24.810Z
Learnt from: kurealnum
PR: artrixdotdev/tortillas#99
File: crates/libtortillas/src/engine/mod.rs:153-155
Timestamp: 2025-08-18T18:25:24.810Z
Learning: The peer stream handling logic in the Engine's EngineMessage::IncomingPeer handler in crates/libtortillas/src/engine/mod.rs is temporary and planned to be migrated to separate code in the future, so extensive refactoring may not be worthwhile in this location.
Applied to files:
crates/libtortillas/src/torrent/mod.rscrates/libtortillas/src/protocol/stream.rs
🧬 Code Graph Analysis (2)
crates/libtortillas/src/torrent/mod.rs (4)
crates/libtortillas/src/protocol/stream.rs (1)
fmt(217-219)crates/libtortillas/src/peer/mod.rs (3)
fmt(45-54)fmt(71-73)Peer(31-42)crates/libtortillas/src/peer/supports.rs (1)
Peer(33-65)crates/libtortillas/src/protocol/messages.rs (2)
Handshake(505-592)Handshake(494-503)
crates/libtortillas/src/protocol/stream.rs (1)
crates/libtortillas/src/protocol/messages.rs (6)
new(471-473)new(507-521)from_bytes(169-371)from_bytes(545-591)Handshake(494-503)Handshake(505-592)
Co-authored-by: kurealnum <oscar.gaske.cs@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
crates/libtortillas/src/protocol/stream.rs (1)
165-181: Return the full Handshake and support variable-length parsing; enables validation upstreamCurrently, recv_handshake discards protocol/info_hash and assumes a fixed 68 bytes. Returning the full Handshake allows callers to validate against MAGIC_STRING and the expected info_hash (you already have validate_handshake available), and reading pstrlen first makes parsing robust.
Apply this diff:
- #[instrument(skip(self), fields(%self))] - pub async fn recv_handshake(&mut self) -> Result<(PeerId, [u8; 8]), PeerActorError> { - // Handshakes will always be 68 bytes - // - // mem::size_of::<Handshake>() is 72 bytes, for some reason. We think it might - // be due to the Arc<>(s) in the Handshake struct - let mut buf = [0u8; 68]; - - self.read_exact(&mut buf).await?; - - let Handshake { - peer_id, reserved, .. - } = Handshake::from_bytes(&buf) - .map_err(|e| PeerActorError::HandshakeFailed { reason: e.into() })?; - - Ok((peer_id, reserved)) - } + #[instrument(skip(self), fields(protocol = self.protocol()))] + pub async fn recv_handshake(&mut self) -> Result<Handshake, PeerActorError> { + // Read protocol string length (pstrlen), then the rest dynamically: + let mut pstrlen = [0u8; 1]; + self.read_exact(&mut pstrlen).await?; + let protocol_len = pstrlen[0] as usize; + + // Remaining bytes: protocol_len + 8 (reserved) + 20 (info_hash) + 20 (peer_id) + let mut rest = vec![0u8; protocol_len + 8 + 20 + 20]; + self.read_exact(&mut rest).await?; + + let mut buf = Vec::with_capacity(1 + rest.len()); + buf.push(pstrlen[0]); + buf.extend_from_slice(&rest); + + let handshake = Handshake::from_bytes(&buf) + .map_err(|e| PeerActorError::HandshakeFailed { reason: e.into() })?; + + Ok(handshake) + }If you want, I can follow up with the minimal call-site changes in torrent/mod.rs (included below) and tests.
crates/libtortillas/src/torrent/mod.rs (1)
112-129: Validate the received handshake before accepting the sessionThe new flow sets peer_id/reserved without validating protocol string or info_hash, which can accept a peer from the wrong torrent. Use validate_handshake with the full Handshake (see stream.rs suggestion to return Handshake).
Apply this diff (assumes recv_handshake returns Handshake):
- match stream.send_handshake(our_id, Arc::clone(&info_hash)).await { - Ok(_) => match stream.recv_handshake().await { - Ok((peer_id, reserved)) => { - id = Some(peer_id); - peer.reserved = reserved; - peer.determine_supported().await; - stream - } - Err(err) => { - warn!(error = %err, "Failed to receive handshake from peer; exiting"); - return; - } - }, - Err(err) => { - warn!(error = %err, "Failed to send handshake to peer; exiting"); - return; - } - } + match stream.send_handshake(our_id, Arc::clone(&info_hash)).await { + Ok(_) => match stream.recv_handshake().await { + Ok(handshake) => { + if let Err(err) = validate_handshake( + &handshake, + peer.socket_addr(), + Arc::clone(&info_hash), + ) { + warn!(error = %err, "Received invalid handshake from peer; exiting"); + return; + } + id = Some(handshake.peer_id); + peer.reserved = handshake.reserved; + peer.determine_supported().await; + stream + } + Err(err) => { + warn!(error = %err, "Failed to receive handshake from peer; exiting"); + return; + } + }, + Err(err) => { + warn!(error = %err, "Failed to send handshake to peer; exiting"); + return; + } + }Additionally, add the missing import (outside this hunk):
use crate::protocol::stream::{PeerSend, PeerStream, validate_handshake};Related fix in the Some(mut stream) branch to avoid moving info_hash (existing compile hazard):
// Replace at the handshake construction line in the Some(mut stream) arm: let handshake = Handshake::new(Arc::clone(&info_hash), our_id);I can submit a consolidated patch if you want this change adopted across both files.
🧹 Nitpick comments (4)
crates/libtortillas/src/protocol/stream.rs (3)
146-150: Avoid unwrap() in instrumentation field; prevent panic when formatting remote_addrUsing unwrap() inside the #[instrument(...)] fields can panic if peer_addr retrieval fails. Prefer a fallible conversion.
Apply this diff:
- remote_addr = self.remote_addr().unwrap().to_string(), + remote_addr = self + .remote_addr() + .map(|a| a.to_string()) + .unwrap_or_else(|e| format!("<err:{e}>")),
165-166: Nit: Avoid using %self in instrumentation because Display for PeerStream unwrapsDisplay for PeerStream calls remote_addr().unwrap(), which can panic. Either avoid %self here or change Display/remote_addr to be infallible/logging. The diff above switches to a safer field.
372-385: Test update aligns with new handshake split; consider validating protocol/info_hash tooGood coverage for the new send-then-recv flow. Once recv_handshake returns a Handshake, you can assert protocol and info_hash as well to catch cross-torrent handshakes in tests.
crates/libtortillas/src/torrent/mod.rs (1)
391-395: Use try_init() in tests to avoid global subscriber re-initialization panicstracing_subscriber::fmt().init() will panic if another test already set the global subscriber. try_init() is safer in a multi-threaded test suite.
Apply this diff:
- tracing_subscriber::fmt() - .with_target(true) - .with_env_filter("libtortillas=trace,off") - .pretty() - .init(); + let _ = tracing_subscriber::fmt() + .with_target(true) + .with_env_filter("libtortillas=trace,off") + .pretty() + .try_init();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
crates/libtortillas/src/protocol/stream.rs(4 hunks)crates/libtortillas/src/torrent/mod.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-18T18:25:24.810Z
Learnt from: kurealnum
PR: artrixdotdev/tortillas#99
File: crates/libtortillas/src/engine/mod.rs:153-155
Timestamp: 2025-08-18T18:25:24.810Z
Learning: The peer stream handling logic in the Engine's EngineMessage::IncomingPeer handler in crates/libtortillas/src/engine/mod.rs is temporary and planned to be migrated to separate code in the future, so extensive refactoring may not be worthwhile in this location.
Applied to files:
crates/libtortillas/src/torrent/mod.rscrates/libtortillas/src/protocol/stream.rs
🧬 Code Graph Analysis (2)
crates/libtortillas/src/torrent/mod.rs (3)
crates/libtortillas/src/protocol/stream.rs (1)
fmt(214-216)crates/libtortillas/src/peer/supports.rs (1)
Peer(33-65)crates/libtortillas/src/protocol/messages.rs (2)
Handshake(505-592)Handshake(494-503)
crates/libtortillas/src/protocol/stream.rs (1)
crates/libtortillas/src/protocol/messages.rs (6)
new(471-473)new(507-521)from_bytes(169-371)from_bytes(545-591)Handshake(505-592)Handshake(494-503)
🔇 Additional comments (2)
crates/libtortillas/src/protocol/stream.rs (2)
18-18: Import cleanup looks goodNarrowing tracing imports to what’s actually used keeps things tidy.
152-160: Refactor of send_handshake to “send-only” is solid; error propagation via ? is idiomaticThe simplified API and
?propagation is clean. Once the instrumentation unwrap is addressed, this is good to go.
Co-authored-by: kurealnum <oscar.gaske.cs@gmail.com>
2 participants
See #97, and the TODO list under it.
Summary by CodeRabbit
Refactor
Tests
Documentation