You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
In rc.1, we (accidentally?) removed logging the node version after a completed handshake.
Proposed Changes
This info is very useful for debugging, e.g. if we see repeated invalid messages from a certain peer and we want to know what software sent those messages. This PR readds the field.
Analyze code quality, security, performance, testing, and documentation
Provide comprehensive feedback
Summary
This PR re-adds the node_version field to the debug log statement after a successful handshake. The change is simple, focused, and addresses the stated issue effectively.
Review Findings
Code Quality ✅
Clean and minimal: The change adds exactly one field to an existing structured logging statement
Consistent with existing code: Follows the same pattern as other fields (our_subnets, their_subnets, matching_subnets)
Proper formatting: Uses the % format specifier for structured logging, consistent with tracing/slog patterns
Security ✅
No security concerns: Logging node version information is safe and commonly used for debugging
No sensitive data exposure: Node version is public metadata already exchanged during handshake
Performance ✅
Negligible impact: Adding one field to an existing debug log has no measurable performance impact
Debug-level logging: This only executes when debug logging is enabled and after successful handshake (not in hot path)
Context and Design
The change is in the record_handshake_subnet_match_metrics() method at anchor/network/src/network.rs:583. This method is called after a successful handshake to:
Count matching subnets between local and remote peers
Log handshake completion details for debugging
Record metrics about subnet matches
The node_version field is already available in their_metadata.node_version (from NodeMetadata struct) and was previously logged before rc.1. This PR correctly restores that field.
Testing Considerations ⚠️
While the change itself is correct, there are no tests for this logging behavior. Consider:
The logging occurs in production code path (record_handshake_subnet_match_metrics)
There are no unit tests verifying the log output format
This is typical for logging code, but structured log assertions could be valuable
Recommendation: Not blocking for this PR, but consider adding integration tests that verify handshake logging behavior in the future, especially for critical debugging information.
Documentation ✅
Clear PR description: The rationale (useful for debugging invalid messages) is well explained
Self-documenting code: The field name node_version is clear and matches the struct field name
Recommendation
Approve - This is a straightforward fix that restores useful debugging information. The change is:
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue Addressed
In
rc.1, we (accidentally?) removed logging the node version after a completed handshake.Proposed Changes
This info is very useful for debugging, e.g. if we see repeated invalid messages from a certain peer and we want to know what software sent those messages. This PR readds the field.