Skip to content

Comments

feat: remove default gossip peers#2090

Open
Anshumancanrock wants to merge 1 commit intogetAlby:masterfrom
Anshumancanrock:master
Open

feat: remove default gossip peers#2090
Anshumancanrock wants to merge 1 commit intogetAlby:masterfrom
Anshumancanrock:master

Conversation

@Anshumancanrock
Copy link

@Anshumancanrock Anshumancanrock commented Feb 21, 2026

Fixes: #2083

Removes the default gossip peers for the LDK backend. These peers were putting unnecessary stress on LSPs by peering without necessarily opening a channel.

Changes

  • ldk.go: Removed the hardcoded gossip peer connection block that connected to LSP nodes on mainnet startup

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 21, 2026

📝 Walkthrough

Walkthrough

Removed the background P2P gossip connection routine from the LDK client's Bitcoin network initialization, eliminating the goroutine that established connections to hard-coded LSP peers and associated connection handling logic.

Changes

Cohort / File(s) Summary
P2P Gossip Connection Removal
lnclient/ldk/ldk.go
Deleted the entire goroutine block handling P2P gossip connections for Bitcoin network, including peer connection attempts, address parsing, error handling, logging, and associated delay/loop logic (41 lines removed).

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐰 A gossip thread is gone, we say hooray!
No more chatter 'bout peers every day,
LDK flows cleaner, lighter, and bright,
One less routine keeps our code in flight! 🚀

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'feat: remove default gossip peers' accurately reflects the primary change in the changeset—removing the hardcoded gossip peer connection logic from ldk.go.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
lnclient/ldk/ldk.go (1)

185-192: LGTM — logic is correct.

The fallback only activates on network == "bitcoin" (the network string variable is never mutated by the switch below), so mainnet gets the default RGS URL and all other networks are unaffected.

Two minor suggestions:

  1. Log field naming: "gossipSource" uses camelCase, while every other log field in this file uses snake_case (e.g. "rpc_host", "chain_source", "node_alias").

  2. Inline string constant: The hardcoded RGS URL could be a named constant to make it more discoverable if it ever needs to change.

♻️ Proposed refinements
+const defaultMainnetRgsSource = "https://rapidsync.lightningdevkit.org/snapshot"
+
 const resetRouterKey = "ResetRouter"
 gossipSource := cfg.GetEnv().LDKGossipSource
 if gossipSource == "" && network == "bitcoin" {
-    gossipSource = "https://rapidsync.lightningdevkit.org/snapshot"
+    gossipSource = defaultMainnetRgsSource
 }
 if gossipSource != "" {
-    logger.Logger.WithField("gossipSource", gossipSource).Info("LDK RGS instance set")
+    logger.Logger.WithField("gossip_source", gossipSource).Info("LDK RGS instance set")
     builder.SetGossipSourceRgs(gossipSource)
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lnclient/ldk/ldk.go` around lines 185 - 192, The log field uses camelCase and
the RGS URL is inline; change the log field key passed to
logger.Logger.WithField from "gossipSource" to snake_case "gossip_source" and
replace the hardcoded URL string used when gossipSource is empty with a named
constant (e.g., defaultRGSURL) defined near the top of this package/file; update
the code paths that reference gossipSource and
builder.SetGossipSourceRgs(gossipSource) to use the new constant when
appropriate so behavior is unchanged but the URL is discoverable and logs follow
the existing snake_case convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@lnclient/ldk/ldk.go`:
- Around line 185-192: The log field uses camelCase and the RGS URL is inline;
change the log field key passed to logger.Logger.WithField from "gossipSource"
to snake_case "gossip_source" and replace the hardcoded URL string used when
gossipSource is empty with a named constant (e.g., defaultRGSURL) defined near
the top of this package/file; update the code paths that reference gossipSource
and builder.SetGossipSourceRgs(gossipSource) to use the new constant when
appropriate so behavior is unchanged but the URL is discoverable and logs follow
the existing snake_case convention.

@Anshumancanrock
Copy link
Author

hi @rolznz , Removed the hardcoded gossip peers and defaulted mainnet to RGS when LDK_GOSSIP_SOURCE isn't set. Scoped the fallback to mainnet only since the RGS URL is network-specific. Would you want defaults for other networks too, or is explicit config fine there?

@rolznz
Copy link
Contributor

rolznz commented Feb 21, 2026

Hi, thanks for the PR.

In the issue there is no mention/request to change the RGS configuration.

@Anshumancanrock
Copy link
Author

Hi @rolznz , You're right that's a separate concern from what the issue asked for. I've updated the PR to only remove the hardcoded gossip peers as the issue requested. Please review it and let me know if any changes are needed. Thanks !

@Anshumancanrock Anshumancanrock changed the title feat: remove default gossip peers and use RGS for LDK gossip sync feat: remove default gossip peers Feb 21, 2026
Copy link
Contributor

@rolznz rolznz left a comment

Choose a reason for hiding this comment

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

utACK

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.

Remove default gossip peers for LDK backend

2 participants