discovery: fix race on remoteUpdateHorizon in GossipSyncer

[NishantBansal2003]

Change Description

This commit fixes a race condition in GossipSyncer related to remoteUpdateHorizon.
The race could occur when the gossiper is flushing a pending batch of new announcements to the network while concurrently processing a GossipTimestampRange message from a peer.

Steps to Test

The issue was discovered during state machine fuzzing of the gossip protocol operation.

[gemini-code-assist]

Summary of Changes

Hello @NishantBansal2003, 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 resolves a critical race condition in the GossipSyncer component. The issue arose from concurrent access to the remoteUpdateHorizon field, potentially leading to inconsistent data when the gossiper was simultaneously sending announcements and processing peer updates. The fix ensures that the remoteUpdateHorizon value is read atomically, preventing data corruption and improving the robustness of the gossip protocol's synchronization mechanism.

Highlights

• Race Condition Fix: Addressed a race condition within the GossipSyncer related to the remoteUpdateHorizon field, which could manifest during concurrent gossip flushing and GossipTimestampRange message processing.
• Thread-Safe Access: Implemented a pattern to safely capture the remoteUpdateHorizon value into a local variable under a lock, ensuring consistent state reads for subsequent operations.
• Code Simplification: Removed redundant locking and unlocking calls around the startTime and endTime calculations, as the remoteUpdateHorizon value is now safely held in a local variable.

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

[gemini-code-assist]

Code Review

This pull request correctly fixes a race condition in GossipSyncer when accessing remoteUpdateHorizon. By introducing a lock to safely read the pointer into a local variable at the beginning of FilterGossipMsgs, the change effectively prevents both the data race and a potential nil pointer dereference. The previous implementation had a time-of-check to time-of-use bug that is now resolved. The fix is clean, correct, and well-contained. I approve these changes.

[Roasbeef]

Nice! Can we add a repro test here that would fail with the race condition detector but pass with this patch? One way to do this would be a mini property based test so it'll run many times per test invocation to make sure the issue is addressed.

[lightninglabs-deploy]

🟠 PR Severity: HIGH

Race fix in gossip syncer | 1 file | 12 lines changed

🟠 High (1 file)

• discovery/syncer.go - Fixes race condition on remoteUpdateHorizon in GossipSyncer, part of the gossip protocol implementation

Analysis

This PR addresses a race condition in the gossip syncer component (discovery/*), which is classified as HIGH severity according to the repository's classification scheme. The gossip protocol is critical for network graph synchronization and peer communication.

Why HIGH severity:

• The discovery/* package handles the Lightning Network gossip protocol
• Race conditions in concurrent code can lead to unpredictable behavior and potential crashes
• The gossip syncer is responsible for keeping channel graph state synchronized across peers
• While not directly handling funds or on-chain operations (which would be CRITICAL), gossip protocol correctness is essential for network health

No severity bump applied:

• Only 1 file changed (threshold: >20 files)
• Only 12 lines changed (threshold: >500 lines)
• Single package affected

This fix requires review by a knowledgeable engineer familiar with the gossip protocol and concurrent programming patterns in lnd.

---

_{To override, add a severity-override-{critical,high,medium,low} label.}

[lightninglabs-deploy]

🟠 PR Severity: HIGH

discovery/syncer.go race fix | 1 file (excl. tests) | 12 lines changed

🟠 High (1 file)

• discovery/syncer.go - Gossip protocol synchronization, fixes race condition on remoteUpdateHorizon field

🟢 Tests (1 file)

• discovery/gossiper_test.go - Test coverage for race fix

Analysis

This PR is classified as HIGH severity because it modifies the discovery/syncer.go file, which is part of the gossip protocol implementation. The discovery package handles critical network graph synchronization between Lightning nodes.

Key considerations:

• Race condition fix: The PR addresses a data race on the remoteUpdateHorizon field in GossipSyncer, which could lead to undefined behavior in concurrent scenarios
• Gossip protocol impact: Any issues in gossip synchronization can affect node connectivity and network graph consistency
• Small, focused change: The fix is minimal (7 additions, 5 deletions) with clear scope, reducing risk
• Test coverage: Includes comprehensive test coverage to verify the race fix

While the change is small and well-contained, the gossip protocol is essential for maintaining an accurate view of the Lightning Network topology. A race condition in this area could potentially cause nodes to miss routing updates or have inconsistent network views.

Recommendation: This PR requires review from an engineer knowledgeable about the discovery/gossip subsystem to ensure the synchronization fix is correct and doesn't introduce new edge cases.

---

_{To override, add a severity-override-{critical,high,medium,low} label.}

[Roasbeef]

Needs a rebase!

[Roasbeef]

LGTM 🥦

[lightninglabs-deploy]

🟠 PR Severity: HIGH

discovery/* | 1 file (excl. tests) | 12 lines changed

🟠 High (1 file)

• discovery/syncer.go - GossipSyncer race condition fix in gossip protocol synchronization

🟢 Test Files (1 file - not counted)

• discovery/gossiper_test.go - Test coverage for race condition fix

Analysis

This PR fixes a race condition in the GossipSyncer related to remoteUpdateHorizon. The race could occur when the gossiper is flushing pending announcements while concurrently processing a GossipTimestampRange message from a peer.

Severity Rationale:

• Changes are in discovery/* package (gossip protocol) → HIGH per classification rules
• Minimal scope: Only 1 production file changed with 12 net lines
• No severity bump criteria met (not >20 files, not >500 lines, single package)
• Race condition fixes in gossip protocol require careful review but don't affect critical consensus or fund safety

Review Focus:

• Verify the mutex protection correctly guards all accesses to remoteUpdateHorizon
• Ensure no deadlock potential introduced by the lock ordering
• Confirm test coverage adequately demonstrates the race is fixed

---

_{To override, add a severity-override-{critical,high,medium,low} label.}

[Roasbeef]

No changelog here is np as it's a small bug fix.