discovery: default trickledelay to 1 if less is set

[hieblmi]

This PR defaults the peer gossip trickle delay to 1 if its config value is set to <=0.

Observing the following error if --trickledelay=0:

panic: non-positive interval for NewTicker


goroutine 2212 [running]:

time.NewTicker(0x4000b47160?)

	time/tick.go:38 +0xc0

github.com/lightningnetwork/lnd/discovery.(*AuthenticatedGossiper).networkHandler(0x400086b2b0, {0x27384c8, 0x4002832b40})

	github.com/lightningnetwork/lnd/discovery/gossiper.go:1466 +0xd8

created by github.com/lightningnetwork/lnd/discovery.(*AuthenticatedGossiper).start in goroutine 2057

	github.com/lightningnetwork/lnd/discovery/gossiper.go:697 +0x15c

[gemini-code-assist]

Summary of Changes

Hello @hieblmi, 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 introduces a crucial fix to enhance the stability of the network's gossip mechanism. It addresses a specific scenario where an invalid configuration for the TrickleDelay could lead to a program crash. By ensuring that the delay is always a positive duration, the change prevents panics and guarantees the continuous operation of the peer gossip handler, improving overall system robustness.

Highlights

• Panic Prevention: This pull request resolves a critical runtime panic that occurred when the peer gossip TrickleDelay configuration was set to a non-positive value (e.g., 0s), which caused time.NewTicker to fail.
• Defaulting TrickleDelay: A safeguard has been implemented in the networkHandler to check the TrickleDelay value. If it is found to be less than or equal to 0, it is now automatically defaulted to 1 nanosecond, ensuring a valid duration for time.NewTicker.

🧠 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 panic that occurs when trickledelay is set to a non-positive value. The change introduces a check to ensure the delay is positive before creating a ticker. My review includes a suggestion to refactor this validation logic into the constructor for better code organization.

[ziggie1984]

yes let's bring this change in for robustness.

[lightninglabs-deploy]

🟠 PR Severity: HIGH

discovery/gossiper.go | 3 files | 83 lines changed

🟠 High (1 file)

• discovery/gossiper.go - Gossip protocol implementation (trickle delay configuration)

🟢 Low (1 file)

• docs/release-notes/release-notes-0.21.0.md - Documentation update

📝 Test Files (1 file)

• discovery/gossiper_test.go - Test coverage for trickle delay fix

Analysis

This PR modifies the gossip protocol implementation in discovery/gossiper.go to ensure the trickle delay has a minimum value of 1 when a value less than 1 is configured. This affects how quickly gossip messages are broadcast to peers.

The discovery/* package is classified as HIGH severity because it handles the gossip protocol, which is responsible for propagating channel announcements, node announcements, and channel updates across the Lightning Network. Incorrect behavior in gossip timing could affect network topology propagation.

The change is small (9 lines in production code) and appears to be a defensive fix to prevent invalid configuration values. However, given its impact on peer-to-peer message propagation timing, a knowledgeable engineer review is appropriate.

No severity bump applied: The PR touches only 1 production file with 9 lines changed (well below the thresholds of >20 files or >500 lines).

---

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

[lightninglabs-deploy]

🟠 PR Severity: HIGH

gossiper.go change | 3 files | 82 lines changed

🟠 High (1 file)

• discovery/gossiper.go - Gossip protocol configuration change (trickle delay default value)

🟢 Low (1 file)

• docs/release-notes/release-notes-0.21.0.md - Documentation update

📋 Test Files (1 file - excluded from severity)

• discovery/gossiper_test.go - Test coverage for trickle delay changes

Analysis

This PR modifies the gossip protocol's trickle delay configuration in discovery/gossiper.go. The change ensures that if a trickle delay value less than 1 is configured, it defaults to 1 to prevent potential issues with the gossip timing mechanism.

Why HIGH severity:

• Changes core gossip protocol behavior (discovery/* is classified as HIGH)
• Modifies configuration validation logic that affects network message propagation
• Small, focused change (1 production file, 9 lines) - no severity bump needed

The change is defensive (setting a safe default) rather than introducing new functionality, which reduces risk. However, it still touches the gossip protocol which is a critical component for network health and message propagation.

---

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

[ziggie1984]

@gemini-code-assist does it make sense to mix the constructor of the AuthenticatedGossiper with the validation of values or should this config value be checked when we parse the config

[gemini-code-assist]

That's a good question, @ziggie1984, and it touches on an important design consideration regarding separation of concerns.

There are valid arguments for both approaches:

1. Validating in the constructor (New function, as currently implemented):

   • Pros: This ensures that any AuthenticatedGossiper instance is always created with a valid TrickleDelay, preventing the time.NewTicker panic immediately upon object instantiation. It acts as a robust safeguard for the AuthenticatedGossiper's internal state, regardless of how the Config object was populated (e.g., directly in code, from tests, or from various config sources).
   • Cons: It does mix the responsibility of object construction with input validation/correction, which can sometimes be seen as a violation of the Single Responsibility Principle.

2. Validating during config parsing:

   • Pros: This would centralize all configuration validation in one place, ensuring that the Config object itself is always valid before being passed to any component. It cleanly separates the concern of config parsing and validation from the component's construction logic.
   • Cons: It relies on all paths that create or modify a Config object to go through this validation. If a Config struct can be manually constructed or partially updated without full validation, then invalid values could still propagate. The New function would then either have to trust the Config (risking the panic) or re-validate, leading to duplication.

Given that the primary goal here is to prevent a critical runtime panic (non-positive interval for NewTicker) that directly affects the AuthenticatedGossiper's operation, performing this validation and defaulting within the New function is a pragmatic and robust solution. It guarantees that the AuthenticatedGossiper will always receive a usable TrickleDelay value. The log.Infof message also provides valuable debugging information if an unexpected non-positive value was provided in the configuration.

While a more comprehensive, centralized config validation layer could be beneficial for the entire application, for this specific critical parameter, the current approach in the constructor provides immediate and reliable protection against a known panic.

[ziggie1984]

Needs a rebase, and also consider moving the check into the validate function maybe ?

[hieblmi]

I added commit dd29d56 to fix a resource leak in the sqldb tests.

[lightninglabs-deploy]

🟠 PR Severity: HIGH

gossip discovery fix | 4 files | 80 lines changed

🟠 High (1 file)

• sqldb/postgres_fixture.go - SQL database fixture modification (sqldb/* category)

🟡 Medium (1 file)

• config.go - Main configuration file, adds validation for trickledelay parameter

🟢 Low (2 files)

• config_test.go - Test coverage for config validation
• docs/release-notes/release-notes-0.21.0.md - Release notes update

Analysis

This PR fixes a panic when --trickledelay=0 is set by defaulting the value to 1 if a non-positive value is provided. The highest severity file is sqldb/postgres_fixture.go, which falls under the sqldb/* category (HIGH severity).

While the core fix in config.go is straightforward parameter validation, the inclusion of SQL database fixture changes requires review by a knowledgeable engineer familiar with database test infrastructure. The PR is small (2 non-test files, 17 substantive lines changed) and doesn't meet bump criteria.

Recommendation: Focus review on ensuring the postgres fixture changes are appropriate and that the trickledelay validation logic handles edge cases correctly.

---

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

[ziggie1984]

LGTM (pending CI)

[yyforyongyu]

LGTM 🛡️