Skip to content

Conversation

@tac0turtle
Copy link
Contributor

Overview

Implement custom gater in order to bypass so we can try to reconnect to a sequencer

@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 introduces a new feature that allows specific, pre-configured peers to bypass the standard libp2p connection gater blocks. This is particularly useful for maintaining stable connections with critical network participants, such as sequencers, even if their peer scoring might otherwise lead to them being blocked. The implementation involves a custom gater that prioritizes an allowlist, ensuring that designated peers can always connect.

Highlights

  • Custom Connection Gater: Introduced a new allowlistGater that wraps the BasicConnectionGater from libp2p. This custom gater allows explicitly configured peers to bypass standard connection blocking mechanisms, ensuring critical connections remain active.
  • Configuration Update: The P2PConfig struct's AllowedPeers field comment has been updated to clarify that it now accepts comma-separated peer multiaddrs and that these allowed peers will override any existing peer ID blocks.
  • P2P Client Integration: The Client struct in the P2P package now includes a hostGater field. A new configureHostGater method has been added to initialize this hostGater using the allowlistGater if AllowedPeers are specified, otherwise falling back to the default gater. The libp2p host creation now uses this configured hostGater.
  • Unit Testing: A new test file allowlist_gater_test.go has been added to verify that the allowlistGater correctly overrides peer blocks for allowed peer IDs during both dial and secured connection interception.
  • Changelog Entry: The CHANGELOG.md file has been updated to document the new feature, stating that explicitly configured peers can now bypass libp2p gater blocks caused by go-header peer scoring.

🧠 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.

Changelog
  • CHANGELOG.md
    • Added an entry for allowing explicitly configured peers to bypass libp2p gater blocks caused by go-header peer scoring.
  • pkg/config/config.go
    • Updated the comment for the AllowedPeers configuration field to reflect that it now accepts peer multiaddrs and overrides peer ID blocks.
  • pkg/p2p/allowlist_gater.go
    • New file added, introducing the allowlistGater struct and its methods to implement the logic for allowing specific peers to bypass connection blocks.
  • pkg/p2p/allowlist_gater_test.go
    • New file added, containing unit tests for the allowlistGater to ensure its functionality of overriding peer blocks for allowed peers.
  • pkg/p2p/client.go
    • Imported connmgr package.
    • Added hostGater field to the Client struct to manage connection gating.
    • Added configureHostGater method to initialize hostGater based on AllowedPeers configuration.
    • Modified the listen method to use the hostGater when creating the libp2p host.
Activity
  • The pull request was created by tac0turtle, introducing the feature to allowlisted peers to bypass gater blocks.
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.

@claude
Copy link
Contributor

claude bot commented Feb 3, 2026

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


Code Review Complete

I've completed a comprehensive review of this PR. The implementation is solid and well-structured. Here are my findings:


✅ Strengths

  1. Clean Architecture: The allowlist gater follows the decorator pattern correctly, wrapping the base gater and only overriding specific behaviors.

  2. Minimal Surface Area: Only the necessary interception points (InterceptPeerDial and InterceptSecured) are modified to check the allowlist.

  3. Good Test Coverage: The test verifies the core functionality - that allowlisted peers bypass blocks while non-allowlisted peers remain blocked.

  4. Documentation: The CHANGELOG entry clearly explains the feature, and the config comment accurately describes the behavior.


🔍 Issues and Recommendations

1. Inconsistent Bypass Logic in InterceptAddrDial ⚠️

Location: pkg/p2p/allowlist_gater.go:36-38

func (g *allowlistGater) InterceptAddrDial(p peer.ID, a ma.Multiaddr) (allow bool) {
	return g.base.InterceptAddrDial(p, a)
}

Issue: The InterceptAddrDial method doesn't check the allowlist before delegating to the base gater. This creates an inconsistency where:

  • InterceptPeerDial bypasses blocks for allowlisted peers ✓
  • InterceptSecured bypasses blocks for allowlisted peers ✓
  • InterceptAddrDial does NOT bypass blocks for allowlisted peers ✗

Why it matters: According to libp2p's connection lifecycle, InterceptAddrDial is called for each multiaddr before dialing. If the base gater blocks an address for an allowlisted peer, the connection attempt could still fail at this stage, defeating the purpose of the allowlist.

Recommendation: Apply the same allowlist check:

func (g *allowlistGater) InterceptAddrDial(p peer.ID, a ma.Multiaddr) (allow bool) {
	if g.isAllowed(p) {
		return true
	}
	return g.base.InterceptAddrDial(p, a)
}

Fix this →


2. Missing InterceptAccept Bypass Logic ⚠️

Location: pkg/p2p/allowlist_gater.go:40-42

func (g *allowlistGater) InterceptAccept(cma network.ConnMultiaddrs) (allow bool) {
	return g.base.InterceptAccept(cma)
}

Issue: InterceptAccept handles inbound connections and doesn't check the allowlist. This means:

  • Allowlisted peers can dial out to this node ✓ (via InterceptPeerDial)
  • This node can dial out to allowlisted peers ✓
  • But allowlisted peers might be blocked when trying to connect inbound to this node ✗

Why it matters: If the base gater blocks inbound connections from an allowlisted peer (e.g., due to IP-based blocks or connection limits), the allowlist won't help for inbound connections. This creates asymmetric behavior.

Challenge: InterceptAccept only receives ConnMultiaddrs (connection addresses), not the peer ID yet, since peer ID is only known after the connection is secured. This means you can't check the allowlist at this stage.

Recommendation: This is acceptable as-is because:

  1. The peer ID isn't available during InterceptAccept
  2. The InterceptSecured check (which happens after handshake) already allows allowlisted peers
  3. Address-based blocks are rare compared to peer ID blocks

However, add a comment explaining this limitation:

// InterceptAccept handles inbound connections. We cannot check the allowlist here
// because the peer ID is not yet known (only determined after connection upgrade).
// Allowlisted peers are handled in InterceptSecured after the handshake completes.
func (g *allowlistGater) InterceptAccept(cma network.ConnMultiaddrs) (allow bool) {
	return g.base.InterceptAccept(cma)
}

3. Test Coverage Gaps 📋

Location: pkg/p2p/allowlist_gater_test.go:14-32

Missing test cases:

  1. InterceptAddrDial behavior: Not tested (would catch issue ABCI interface #1 above)
  2. Empty allowlist: What happens when allowlist is empty or nil?
  3. Multiple allowed peers: Test with multiple peers in the allowlist
  4. Both inbound and outbound directions: Only tests DirInbound for InterceptSecured

Recommendation: Add test cases:

func TestAllowlistGaterWithMultiplePeers(t *testing.T) { ... }
func TestAllowlistGaterEmptyAllowlist(t *testing.T) { ... }
func TestAllowlistGaterInterceptAddrDial(t *testing.T) { ... }
func TestAllowlistGaterBothDirections(t *testing.T) { ... }

4. Nil Check Missing ⚠️

Location: pkg/p2p/client.go:399-406

func (c *Client) configureHostGater() {
	allowed := c.allowedPeerIDs()
	if len(allowed) == 0 {
		c.hostGater = c.gater
		return
	}
	c.hostGater = newAllowlistGater(c.gater, allowed)
}

Issue: If c.gater is nil (unlikely but possible in edge cases), this will create an allowlistGater with a nil base, causing panics when the base gater methods are called.

Recommendation: Add defensive nil check:

func (c *Client) configureHostGater() {
	if c.gater == nil {
		return
	}
	allowed := c.allowedPeerIDs()
	if len(allowed) == 0 {
		c.hostGater = c.gater
		return
	}
	c.hostGater = newAllowlistGater(c.gater, allowed)
}

5. Configuration Interaction 🤔

Location: pkg/p2p/client.go:147-155

c.logger.Debug().Str("blacklist", c.conf.BlockedPeers).Msg("blocking blacklisted peers")
if err := c.setupBlockedPeers(c.parseAddrInfoList(c.conf.BlockedPeers)); err != nil {
	return err
}

c.logger.Debug().Str("whitelist", c.conf.AllowedPeers).Msg("allowing whitelisted peers")
if err := c.setupAllowedPeers(c.parseAddrInfoList(c.conf.AllowedPeers)); err != nil {
	return err
}

Observation: The setupAllowedPeers method calls c.gater.UnblockPeer() for each allowed peer, but the allowlist gater already bypasses blocks for these peers. This creates redundancy.

Current flow:

  1. A peer is blocked (via BlockedPeers config or go-header peer scoring)
  2. setupAllowedPeers calls UnblockPeer on the base gater
  3. The allowlist gater also bypasses blocks

Why this matters: If a peer gets re-blocked after startup (e.g., by go-header peer scoring), setupAllowedPeers won't help because it only runs at startup. The allowlist gater correctly handles this, but the UnblockPeer call is misleading.

Recommendation: Either:

  • Remove the setupAllowedPeers call entirely and rely solely on the allowlist gater
  • Or add a comment explaining that setupAllowedPeers handles startup-time unblocking while the allowlist gater handles runtime blocks

6. Type Safety 💡

Location: pkg/p2p/allowlist_gater.go:12-14

type allowlistGater struct {
	allowlist map[peer.ID]struct{}
	base      *conngater.BasicConnectionGater
}

Observation: The struct doesn't explicitly declare that it implements connmgr.ConnectionGater. While Go's duck typing handles this, it's not immediately obvious.

Recommendation: Add compile-time interface check:

// Ensure allowlistGater implements connmgr.ConnectionGater
var _ connmgr.ConnectionGater = (*allowlistGater)(nil)

📝 Minor Style & Documentation

  1. Naming inconsistency: pkg/p2p/client.go:252-254 - The variable is named hostGater but the comment says "controls libp2p connection gating for the host". Consider renaming the comment or add clarity: // hostGater is the connection gater used by the libp2p host (may be allowlistGater or base gater)

  2. Logging opportunity: When the allowlist gater is configured, consider logging how many peers are in the allowlist:

func (c *Client) configureHostGater() {
	allowed := c.allowedPeerIDs()
	if len(allowed) == 0 {
		c.hostGater = c.gater
		return
	}
	c.logger.Info().Int("count", len(allowed)).Msg("configuring allowlist gater with allowed peers")
	c.hostGater = newAllowlistGater(c.gater, allowed)
}

🔒 Security Considerations

No security issues identified. The implementation:

  • ✅ Only bypasses blocks for explicitly configured peers
  • ✅ Doesn't expose credentials or sensitive data
  • ✅ Properly delegates to base gater for non-allowlisted peers
  • ✅ Uses peer IDs (cryptographically verified) rather than IP addresses

⚡ Performance Considerations

Efficient implementation:

  • ✅ Map lookup for allowlist check is O(1)
  • ✅ No unnecessary allocations
  • ✅ No goroutine leaks
  • ✅ Minimal overhead on connection gating

🎯 Summary

Overall Assessment: This is a well-designed feature that solves the problem of maintaining stable connections to critical peers (sequencers). The implementation is clean and follows Go idioms.

Must Fix:

  1. Add allowlist check to InterceptAddrDial for consistency

Should Fix:
2. Add comment explaining InterceptAccept limitation
3. Add nil check in configureHostGater
4. Expand test coverage

Nice to Have:
5. Clarify setupAllowedPeers interaction
6. Add compile-time interface check
7. Add logging for allowlist configuration


@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2026

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

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedFeb 3, 2026, 2:08 PM

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 introduces an allowlistGater to ensure connections to specified peers can bypass libp2p's connection gating, which is a valuable feature for maintaining connectivity to critical nodes like sequencers. The implementation is generally solid, with a new gater implementation and corresponding tests.

My review includes two main points for pkg/p2p/client.go:

  1. A suggestion to simplify the code in the listen function by removing a redundant check.
  2. A more critical observation about a design issue where pre-configured hosts passed via NewClientWithHost will not use the new allowlist functionality. This could be problematic if that function is used in production-like scenarios.

Overall, the changes are a good step forward. Addressing the identified issues will improve the robustness and clarity of the implementation.

return c.startWithHost(ctx, c.host)
}

c.configureHostGater()
Copy link
Contributor

Choose a reason for hiding this comment

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

high

Calling configureHostGater() here is correct for the case where the client creates its own host. However, the logic path for pre-injected hosts (the if c.host != nil block above) completely bypasses this new allowlist functionality. If NewClientWithHost is used in scenarios that require the allowlist (like connecting to a sequencer), this will not work as expected.

This is a design issue that should be addressed. A potential solution could involve refactoring how hosts are injected, perhaps by moving gater configuration into NewClient and adjusting NewClientWithHost and its callers to correctly create the host with the configured gater.

Comment on lines +252 to +260
hostGater := c.hostGater
if hostGater == nil {
hostGater = c.gater
}
return libp2p.New(
libp2p.ListenAddrs(maddr),
libp2p.Identity(c.privKey),
libp2p.ConnectionGater(hostGater),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The logic to select the gater can be simplified. Since configureHostGater() is called before listen() and always initializes c.hostGater, the check for nil is redundant. You can directly use c.hostGater when creating the new libp2p host.

return libp2p.New(
		libp2p.ListenAddrs(maddr),
		libp2p.Identity(c.privKey),
		libp2p.ConnectionGater(c.hostGater),
	)

@codecov
Copy link

codecov bot commented Feb 3, 2026

Codecov Report

❌ Patch coverage is 81.81818% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.22%. Comparing base (e8e8946) to head (3cf46f9).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
pkg/p2p/allowlist_gater.go 71.42% 6 Missing ⚠️
pkg/p2p/client.go 91.30% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3045      +/-   ##
==========================================
+ Coverage   56.17%   56.22%   +0.04%     
==========================================
  Files         118      119       +1     
  Lines       12066    12109      +43     
==========================================
+ Hits         6778     6808      +30     
- Misses       4543     4555      +12     
- Partials      745      746       +1     
Flag Coverage Δ
combined 56.22% <81.81%> (+0.04%) ⬆️

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 closed this Feb 4, 2026
@tac0turtle tac0turtle deleted the marko/allowlist-gater branch February 4, 2026 12:43
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