Skip to content

chore: remove double p2p setting#3041

Merged
tac0turtle merged 1 commit intomainfrom
marko/fix_netinfo
Feb 2, 2026
Merged

chore: remove double p2p setting#3041
tac0turtle merged 1 commit intomainfrom
marko/fix_netinfo

Conversation

@tac0turtle
Copy link
Contributor

Overview

this pr removes the double p2p/<>/p2p/<> as its malformed to have it twice but the system allows it

@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2026

The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedFeb 2, 2026, 6:10 PM

@claude
Copy link
Contributor

claude bot commented Feb 2, 2026

Claude finished @tac0turtle's task in 2m 1s —— View job


Comprehensive Code Review

  • Read and analyze all changed files
  • Review P2P address generation logic
  • Assess test coverage and correctness
  • Check for code quality issues
  • Verify security considerations
  • Provide comprehensive feedback

Summary

This PR successfully addresses a P2P address formatting issue where the peer ID segment (/p2p/<nodeID>) was being duplicated in listen addresses. The fix is implemented correctly across the P2P client layer, command layer, tests, and utility scripts.

Verdict: APPROVED ✅ - The implementation is sound with proper defensive programming and comprehensive test coverage updates.


🎯 Code Quality & Architecture

Strengths:

  1. Defensive Programming Excellence - The fix in pkg/p2p/client.go:411-428 implements proper defensive logic by checking if the peer ID suffix already exists before appending it. This is a textbook example of idempotent address formatting.
peerIDSuffix := "/p2p/" + c.host.ID().String()
for _, a := range c.host.Addrs() {
    addr := a.String()
    // Only append peer ID if not already present
    if !strings.HasSuffix(addr, peerIDSuffix) {
        addr = addr + peerIDSuffix
    }
    addrs = append(addrs, addr)
}
  1. Clean Separation of Concerns - The command layer (pkg/cmd/p2p.go) was correctly simplified by removing the redundant "Full" address display, as the P2P layer now handles this responsibility.

  2. Test Coverage - Both test cases (TestNetInfoCmd_Success and TestNetInfoCmd_NoPeers) were properly updated to reflect the new behavior, including:

    • Updated mock data to include peer IDs in listen addresses
    • Removed assertions for the now-deleted "Full" address line
    • Added helpful comments explaining the change

Minor Observations:

  1. The logging statement at pkg/p2p/client.go:140 still manually constructs the full address for logging:
    c.logger.Info().Str("address", fmt.Sprintf("%s/p2p/%s", a, c.host.ID())).Msg("listening on address")
    While this works, it could potentially create inconsistency if libp2p's host.Addrs() ever returns addresses that already include the peer ID. Consider using the same defensive logic here for consistency.

🔒 Security

No security concerns identified. The changes are purely cosmetic/formatting and don't affect:

  • Authentication or authorization
  • Input validation (the defensive check actually improves robustness)
  • Credential handling
  • Network security boundaries

⚡ Performance & Resource Efficiency

Impact: Negligible

The changes introduce minimal overhead:

  • One additional strings.HasSuffix check per address (O(n) where n = suffix length)
  • This is executed only during GetNetworkInfo() RPC calls, which are infrequent

The removal of the duplicate "Full" address display actually reduces string formatting overhead in the command layer.


🧪 Testing & Reliability

Strengths:

  1. Comprehensive Test Updates - Both test cases were updated to match the new behavior
  2. Mock Data Accuracy - Test data now correctly includes the peer ID in listen addresses, matching real-world behavior
  3. Test Coverage - Both success and edge cases (no peers) are covered

Recommendations:

  1. Consider adding a unit test specifically for GetNetworkInfo() that validates:

    • Addresses without peer IDs get the suffix appended
    • Addresses already containing peer IDs are not duplicated
    • Mixed scenarios (some with, some without) are handled correctly

    Example test case:

    func TestGetNetworkInfo_IdempotentPeerIDAppending(t *testing.T) {
        // Test that addresses with and without peer IDs are handled correctly
    }
  2. Consider an integration test that verifies the actual output from a running node doesn't contain duplicate /p2p/ segments.


📚 Documentation & Developer Experience

Strengths:

  1. Clear Comments - The test files now include helpful comments like // Listen addresses include peer ID (as returned by the P2P layer)
  2. Self-Documenting Code - The defensive check includes an explanatory comment

Recommendations:

  1. The PR description is minimal. Consider adding:
    • Example of the malformed address (e.g., /ip4/127.0.0.1/tcp/7676/p2p/12D3.../p2p/12D3...)
    • Explanation of why libp2p allows this but it should be fixed
    • Impact on node connectivity (if any)

🔧 Script Changes Review

scripts/run.go and scripts/run-evm-nodes.go:

  1. Address Parsing Improvement (lines 278-289 in run.go):

    • The new parsing logic correctly targets the "Addr:" line instead of the now-removed "Full:" line
    • The stripANSI helper function is properly implemented using regex
    • Good practice: The code checks multiple conditions before accepting an address
  2. Project Root Detection (line 367 in run.go, line 611 in run-evm-nodes.go):

    • Changed marker from "da" to "block"
    • Question: Was the "da" marker incorrect, or was this a related cleanup? This seems unrelated to the P2P address fix. Consider separating this change into a different commit for clearer git history.

🐛 Potential Issues & Edge Cases

Issue 1: Inconsistent Address Construction

  • Location: pkg/p2p/client.go:140
  • Description: The logging statement manually constructs addresses without the defensive check
  • Severity: Low
  • Recommendation: Apply the same defensive logic:
    peerIDSuffix := "/p2p/" + c.host.ID().String()
    for _, a := range c.host.Addrs() {
        addr := a.String()
        if !strings.HasSuffix(addr, peerIDSuffix) {
            addr = addr + peerIDSuffix
        }
        c.logger.Info().Str("address", addr).Msg("listening on address")
    }

Issue 2: Root Cause Analysis

  • Question: Why does host.Addrs() sometimes return addresses with peer IDs and sometimes without?
  • Recommendation: Document this behavior or investigate if this is a libp2p version-specific quirk. Understanding the root cause will help prevent similar issues.

Edge Case: Custom Multiaddrs

  • The defensive check uses strings.HasSuffix, which should work for all standard multiaddr formats
  • However, if someone creates a custom multiaddr ending with /p2p/<different_id> (a different peer's ID), this check would incorrectly skip appending the correct ID
  • Severity: Very Low (this scenario is highly unlikely in practice)
  • Mitigation: Consider a more robust check that validates the entire peer ID, not just the suffix presence

📋 Checklist

  • Code follows Go idioms and conventions
  • Error handling is appropriate
  • Tests are updated and comprehensive
  • No security vulnerabilities introduced
  • Performance impact is acceptable
  • Changes are backward compatible
  • Documentation is clear

💡 Suggestions for Follow-up

  1. Add unit test for GetNetworkInfo() covering idempotent peer ID appending
  2. Document libp2p behavior - Why does host.Addrs() sometimes include peer IDs?
  3. Apply defensive logic to the logging statement in startWithHost() for consistency
  4. Separate unrelated changes - The "da""block" marker change should be in a separate commit

Final Recommendation

APPROVE - This PR successfully fixes the duplicate P2P peer ID issue with proper defensive programming and comprehensive test updates. The implementation is sound and production-ready.

Minor suggestions above are for enhancement and can be addressed in follow-up PRs if desired.


@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @tac0turtle, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request resolves an issue where P2P listen addresses were being incorrectly formatted with a duplicate peer ID segment. The changes refine the internal logic for generating these addresses and update the command-line interface to display them correctly. Additionally, utility scripts have been enhanced to robustly parse network addresses and detect project roots.

Highlights

  • P2P Address Formatting Correction: Corrected the logic for constructing and displaying P2P listen addresses to prevent the redundant appending of the /p2p/<nodeID> segment, ensuring addresses are always correctly formatted.
  • Improved Network Info Parsing: Refactored the scripts/run.go utility to accurately parse P2P addresses from network information output, now targeting the 'Addr:' line and incorporating a new helper function to strip ANSI escape codes.
  • Test Case Updates: Updated P2P network information test cases to align with the corrected address formatting and removed assertions for the no longer displayed 'Full' address.
  • Script Project Root Markers: Adjusted project root detection markers in scripts/run-evm-nodes.go and scripts/run.go from 'da' to 'block'.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request successfully resolves the issue of malformed p2p addresses by preventing the duplication of the /p2p/<ID> suffix. The core logic change in pkg/p2p/client.go is correct, and the corresponding updates to the net-info command and its tests are consistent. I've included a few suggestions for the accompanying script changes in scripts/run.go to improve code quality, focusing on removing a redundant check and addressing an inefficient, duplicated helper function.

Comment on lines +281 to +283
if addrIdx == -1 {
continue
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This check is redundant. The outer if condition on line 279 already ensures that line contains "Addr:", so addrIdx will never be -1 here. You can safely remove these three lines.

Comment on lines +382 to +384
func stripANSI(str string) string {
ansiRegex := regexp.MustCompile(`\x1b\[[0-9;]*m`)
return ansiRegex.ReplaceAllString(str, "")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Compiling a regular expression on every function call is inefficient. It's better to compile it once at the package level and reuse the compiled object.

var ansiRegex = regexp.MustCompile(`\x1b\[[0-9;]*m`)

// stripANSI removes ANSI escape sequences from a string.
func stripANSI(str string) string {
	return ansiRegex.ReplaceAllString(str, "")
}

Additionally, an identical stripANSI function already exists in scripts/run-evm-nodes.go. To follow the DRY (Don't Repeat Yourself) principle, it would be best to move this function to a shared utility file within the scripts directory and call it from both run.go and run-evm-nodes.go.

@codecov
Copy link

codecov bot commented Feb 2, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 56.01%. Comparing base (0189cd5) to head (bf3af16).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3041      +/-   ##
==========================================
+ Coverage   55.95%   56.01%   +0.06%     
==========================================
  Files         117      117              
  Lines       11843    11844       +1     
==========================================
+ Hits         6627     6635       +8     
+ Misses       4486     4479       -7     
  Partials      730      730              
Flag Coverage Δ
combined 56.01% <100.00%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tac0turtle tac0turtle enabled auto-merge February 2, 2026 18:14
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utAcK

@tac0turtle tac0turtle added this pull request to the merge queue Feb 2, 2026
Merged via the queue into main with commit 709d4db Feb 2, 2026
33 checks passed
@tac0turtle tac0turtle deleted the marko/fix_netinfo branch February 2, 2026 18:37
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