Skip to content

Conversation

@galt-tr
Copy link
Contributor

@galt-tr galt-tr commented Nov 25, 2025

This pull request refactors the handler functions for P2P message topics in server_helpers.go to improve peer identity verification and logging consistency. The main change is to ensure that the sender's peer ID (fromID) matches the peer ID in the message payload, which helps prevent spoofing and improves network security. Additional improvements include renaming variables for clarity and updating log messages for better traceability.

Peer identity verification and security improvements:

  • Updated the handleBlockTopic, handleSubtreeTopic, and handleRejectedTxTopic functions to check that the sender's peer ID (fromID) matches the peer ID in the message payload, logging an error and returning early if there is a mismatch. This helps prevent spoofed messages from being processed. [1] [2] [3]

Code clarity and consistency:

  • Renamed the from parameter to fromID in the handler functions to clarify its meaning and updated all usages throughout these functions. [1] [2] [3]
  • Updated log messages and comments to use the new fromID terminology and to clarify the source of messages, improving readability and debugging. [1] [2] [3]

Consistent peer tracking and metrics:

  • Ensured that all peer tracking, byte counting, and message time updates use fromID for consistency and accuracy. [1] [2] [3] [4] [5]

These changes collectively make the P2P server code more robust against spoofing, easier to maintain, and simpler to debug.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 25, 2025

🤖 Claude Code Review

Status: Complete


Current Review:

The PR adds important security validation by checking that fromID matches the message payload's peer ID across all P2P handlers. This prevents spoofing attacks. However, there are several comment inconsistencies where "fromID" appears in comments where it should be "from":

Lines with incorrect comment formatting:

  • Line 86: "Track bytes received fromID this message" should be "Track bytes received from this message"
  • Line 89: "Skip notifications fromID banned peers" should be "Skip notifications from banned peers"
  • Line 94: "Skip notifications fromID unhealthy peers" should be "Skip notifications from unhealthy peers"
  • Line 304: "Track bytes received fromID this message" should be "Track bytes received from this message"
  • Line 311: "Skip notifications fromID unhealthy peers" should be "Skip notifications from unhealthy peers"
  • Line 316: "Rejected TX messages fromID other peers" should be "Rejected TX messages from other peers"
  • Line 318: "remove fromID our mempool" should be "remove from our mempool"

These appear to be unintentional replacements during the refactoring. The security logic is sound, but the comment inconsistencies reduce code clarity.

History:

  • ✅ Fixed: Grammar issues in peer ID validation comments ("does matches" → "matches")
  • ✅ Fixed: Copy-paste error in handleBlockTopic comment (now correctly says "block peer ID")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants