fix: ensure consistent network ID usage in P2P subscriber #2884

tac0turtle · 2025-11-24T15:30:04Z

Overview

align usage of network info

github-actions · 2025-11-24T15:30:19Z

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

Build	Format	Lint	Breaking	Updated (UTC)
`✅ passed`	`⏩ skipped`	`✅ passed`	`✅ passed`	Nov 24, 2025, 3:31 PM

gemini-code-assist · 2025-11-24T15:30:19Z

Summary of Changes

Hello @tac0turtle, I'm Gemini Code Assist¹! 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 standardizes the generation and usage of network identifiers within the P2P subscriber setup. By centralizing the derivation of the networkID based on the chainID obtained from the P2P layer, it ensures a unified approach across the P2P infrastructure, improving maintainability and correctness. This change resolves an inconsistency in how network IDs were previously handled.

Highlights

Consistent Network ID Usage: The P2P subscriber now consistently derives its network ID by first fetching the chain ID from the P2P layer's info and then processing it through the getNetworkID helper, ensuring alignment with other P2P components.
Refactored Network ID Retrieval: Redundant logic for fetching network information and deriving the network ID within the setupP2PInfrastructure function has been removed, as the network ID is now computed once and reused for both the subscriber and the P2P server.
Removed getChainID Function: The getChainID helper function has been removed, streamlining the codebase by integrating its logic directly into the setupP2PInfrastructure method where the chain ID is obtained and used to derive the network ID.

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.

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 · 2025-11-24T15:30:21Z

Claude finished @tac0turtle's task —— View job

Code Review Complete ✓

I've completed a comprehensive review of this PR. Here's my detailed feedback:

Summary

This PR standardizes network ID usage in the P2P subscriber by ensuring the subscriber, server, and exchange all derive their network ID consistently from p2p.Info() chain ID rather than using two different sources (genesis.ChainID for subscriber vs p2p.Info() chain ID for server/exchange).

Verdict: ✅ Approve with minor observation

What Changed

Before:

Subscriber used getChainID() → genesis.ChainID + "-" + syncType
Server/Exchange used getNetworkID(network) → p2p.Info() chainID + "-" + syncType
Two separate calls to p2p.Info() - one for subscriber, one for server/exchange
Inconsistent source of truth for network identification

After:

Single call to p2p.Info() at the beginning of setupP2PInfrastructure
All components (subscriber, server, exchange) use the same networkID derived from p2p.Info() chain ID
Removed redundant getChainID() method
Removed duplicate p2p.Info() call

Detailed Analysis

1. Code Quality & Architecture ✅

Strengths:

DRY Principle Applied: Eliminated code duplication by consolidating network ID derivation to a single location (pkg/sync/sync_service.go:256-260)
Single Source of Truth: Using p2p.Info() chain ID consistently ensures all P2P components are on the same network
Improved Clarity: The logic flow is now clearer - fetch chain ID once, derive network ID once, use everywhere
Better Variable Naming: Changed network to chainID which more accurately reflects what p2p.Info() returns

Observation:

The comment on line 262 says "Create subscriber but DON'T start it yet" - this is accurate and important. The staged initialization (create → init store → start) is a good pattern for managing component lifecycle.

2. Correctness & Bug Fix ✅

Critical Fix Identified:

This PR fixes a potential network mismatch bug:

Root Cause: genesis.ChainID (from config/genesis file) could theoretically differ from p2p.Info() chain ID (from P2P client state)
Impact: If they differed, the subscriber would be on a different P2P topic than the server/exchange, causing complete communication failure
Fix: Now all components derive from the same p2p.Info() chain ID, guaranteeing topic alignment

Location: pkg/sync/sync_service.go:256-266

// OLD (BUGGY):
// Subscriber: syncService.genesis.ChainID + "-" + syncType
// Server/Exchange: p2p.Info() chainID + "-" + syncType

// NEW (CORRECT):
_, _, chainID, err := syncService.p2p.Info()
networkID := syncService.getNetworkID(chainID)  // Used by all components

3. Error Handling & Resilience ✅

Error handling for p2p.Info() is appropriate with context wrapping
Early return pattern prevents cascade failures
Error message is descriptive: "error while fetching the network"

4. Performance & Resource Efficiency ✅

Improvement:

Reduced from 2 calls to p2p.Info() to 1 call
Minimal performance impact (just method call overhead), but cleaner code
No new allocations or goroutines introduced

5. Testing & Reliability ⚠️

Observation:

The existing tests in pkg/sync/sync_service_test.go pass a consistent chainId to both the genesis and p2pClient:

// Line 35, 53, 91, 126, 144:
chainId := "test-chain-id"
p2pClient, err := p2p.NewClientWithHost(..., chainId, ...)
genesisDoc := genesispkg.Genesis{ChainID: chainId, ...}

Recommendation:
Consider adding a test case that validates the fix - i.e., ensure that even if genesis.ChainID differs from the P2P client's chain ID, the system uses the P2P client's chain ID consistently. This would catch regressions of this bug pattern.

Test scenario:

// Hypothetical test
genesisDoc := genesispkg.Genesis{ChainID: "genesis-chain", ...}
p2pClient := p2p.NewClient(..., "p2p-chain", ...)
// Verify subscriber, server, exchange all use "p2p-chain"

However, this may not be a realistic scenario if the P2P client always validates chain ID matches genesis. Worth investigating whether such a mismatch is even possible in production.

6. Documentation & Code Comments ✅

Existing comments remain accurate
Function documentation is clear
The helper method getNetworkID has an obvious purpose from its implementation

Minor suggestion: Could add a comment explaining why we use p2p.Info() chain ID rather than genesis.ChainID:

// Fetch chain ID from P2P layer to ensure all P2P components (subscriber, server, exchange) 
// use the same network identifier for topic subscriptions and peer discovery
_, _, chainID, err := syncService.p2p.Info()

7. Security ✅

No security implications identified
No credential handling or input validation concerns in this change

Workflow File Deletion

The deletion of project/.github/workflows/test.yml appears in the diff but was actually removed in a separate merged PR #2882 ("chore: remove extra github action yml file"). This is just showing up in the diff due to the merge commit. Not part of this PR's changes.

Alignment with Issue #1020

✅ Issue Resolution: The PR description says "closes #1020" which is about "Use parametrized topic ID in P2P layer of go-header library."

While the issue description talks about future support for parametrized topic IDs, this PR correctly aligns the current implementation by ensuring consistent network ID usage across all P2P components. This is a prerequisite for proper topic ID parameterization.

Repository Guidelines Compliance ✅

Per CLAUDE.md:

✅ Follows Go conventions and idiomatic style
✅ Error wrapping with context using fmt.Errorf
✅ Small, focused change
✅ No security vulnerabilities introduced
✅ No goroutine leaks or concurrency issues

Final Recommendation

✅ LGTM (Looks Good To Me)

This is a high-quality bug fix that:

Fixes a potential network mismatch issue
Improves code quality by eliminating duplication
Enhances maintainability
Has no breaking changes
Properly closes issue Use parametrized topic ID in P2P layer of go-header library #1020

Suggested Next Steps:

✅ Merge after CI passes
Consider adding a test case for chain ID consistency (low priority)
Consider adding an explanatory comment about why p2p.Info() is the source of truth (optional)

gemini-code-assist

Code Review

This pull request refactors the P2P infrastructure setup to ensure consistent usage of the network ID for both the P2P subscriber and server. The change correctly centralizes fetching the chainID from the p2p client, removing redundant code and an unused helper function (getChainID). This improves code clarity, maintainability, and consistency. The changes are well-executed and address the described issue. I have added one minor suggestion to improve an error message for better debuggability.

gemini-code-assist · 2025-11-24T15:31:52Z

pkg/sync/sync_service.go

+	if err != nil {
+		return nil, fmt.Errorf("error while fetching the network: %w", err)
+	}


The error message "error while fetching the network" is a bit vague. A more specific message, such as "error while fetching p2p info", would be more descriptive of what syncService.p2p.Info() does and would aid in debugging, as the function primarily retrieves information from the p2p client rather than performing a network fetch.

Suggested change

if err != nil {

return nil, fmt.Errorf("error while fetching the network: %w", err)

}

if err != nil {

return nil, fmt.Errorf("error while fetching p2p info: %w", err)

}

codecov · 2025-11-24T15:34:55Z

Codecov Report

❌ Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.81%. Comparing base (8cd0fb8) to head (76f152d).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/sync/sync_service.go	60.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2884      +/-   ##
==========================================
+ Coverage   64.76%   64.81%   +0.05%     
==========================================
  Files          81       81              
  Lines        7350     7347       -3     
==========================================
+ Hits         4760     4762       +2     
+ Misses       2049     2043       -6     
- Partials      541      542       +1

Flag	Coverage Δ
combined	`64.81% <60.00%> (+0.05%)`	⬆️

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.

alpe

With this change, we would have 2 different networks for headers and data.
uAck

tac0turtle · 2025-11-25T08:51:45Z

With this change, we would have 2 different networks for headers and data. uAck

true, im not sure that is what we want. will double checl

tac0turtle · 2025-11-25T08:51:57Z

With this change, we would have 2 different networks for headers and data. uAck

true, im not sure that is what we want. will double check

tac0turtle · 2025-11-25T10:51:36Z

talking to hlib it seems this change is actually needed. The rollout of this will be breaking, nodes not on the same version wont be able to sync all the data.

julienrbrt

I read above that it is breaking. I think it warrants a changelog then.

tac0turtle · 2025-11-26T15:09:23Z

oh nvm this isnt breaking. its only breaking if you are using a incorrect genesis file loocally, otherwise the network is the same

* main: refactor: omit unnecessary reassignment (#2892) build(deps): Bump the all-go group across 5 directories with 6 updates (#2881) chore: fix inconsistent method name in retryWithBackoffOnPayloadStatus comment (#2889) fix: ensure consistent network ID usage in P2P subscriber (#2884)

* main: build(deps): Bump mdast-util-to-hast from 13.2.0 to 13.2.1 in /docs in the npm_and_yarn group across 1 directory (#2900) refactor(block): centralize timeout in client (#2903) build(deps): Bump the all-go group across 2 directories with 3 updates (#2898) chore: bump default timeout (#2902) fix: revert default db (#2897) refactor: remove obsolete // +build tag (#2899) fix:da visualiser namespace (#2895) refactor: omit unnecessary reassignment (#2892) build(deps): Bump the all-go group across 5 directories with 6 updates (#2881) chore: fix inconsistent method name in retryWithBackoffOnPayloadStatus comment (#2889) fix: ensure consistent network ID usage in P2P subscriber (#2884) build(deps): Bump golangci/golangci-lint-action from 9.0.0 to 9.1.0 (#2885) build(deps): Bump actions/checkout from 5 to 6 (#2886)

tac0turtle added 2 commits November 24, 2025 15:44

remove extra github action yml file

11f5608

fix: ensure consistent network ID usage in P2P subscriber

c3bf903

github-actions bot assigned tac0turtle Nov 24, 2025

Merge branch 'main' into marko/1020

76f152d

gemini-code-assist bot reviewed Nov 24, 2025

View reviewed changes

tac0turtle requested a review from julienrbrt November 24, 2025 15:32

alpe approved these changes Nov 25, 2025

View reviewed changes

julienrbrt approved these changes Nov 25, 2025

View reviewed changes

tac0turtle merged commit 3d7c3a8 into main Nov 26, 2025
30 of 31 checks passed

tac0turtle deleted the marko/1020 branch November 26, 2025 15:09

fix: ensure consistent network ID usage in P2P subscriber #2884

fix: ensure consistent network ID usage in P2P subscriber #2884

Uh oh!

Conversation

tac0turtle commented Nov 24, 2025

Overview

Uh oh!

github-actions bot commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist bot commented Nov 24, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

claude bot commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Complete ✓

Summary

What Changed

Detailed Analysis

1. Code Quality & Architecture ✅

2. Correctness & Bug Fix ✅

3. Error Handling & Resilience ✅

4. Performance & Resource Efficiency ✅

5. Testing & Reliability ⚠️

6. Documentation & Code Comments ✅

7. Security ✅

Workflow File Deletion

Alignment with Issue #1020

Repository Guidelines Compliance ✅

Final Recommendation

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alpe left a comment

Choose a reason for hiding this comment

Uh oh!

tac0turtle commented Nov 25, 2025

Uh oh!

tac0turtle commented Nov 25, 2025

Uh oh!

tac0turtle commented Nov 25, 2025

Uh oh!

julienrbrt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tac0turtle commented Nov 26, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented Nov 24, 2025 •

edited

Loading

claude bot commented Nov 24, 2025 •

edited

Loading

codecov bot commented Nov 24, 2025 •

edited

Loading

julienrbrt left a comment •

edited

Loading