feat: add Tor hidden service and outbound .onion peer connections#2038
feat: add Tor hidden service and outbound .onion peer connections#2038sat-engineer wants to merge 2 commits intogetAlby:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds optional Tor hidden-service support: new tor package, LDK integration (SOCKS proxy and onion service lifecycle), new config flags, API and frontend exposure of Tor status and onion address, plus UI for Tor settings and copyable Tor node URI. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Config as Config (LDK_TOR_*)
participant LDK as LDK Service
participant TorSvc as Tor Service
participant CtrlPort as Tor Control Port
participant Frontend as Frontend UI
App->>Config: load Tor env vars
App->>LDK: init with config
LDK->>LDK: setup Tor SOCKS proxy (if configured)
LDK->>TorSvc: Start(cfg) after initial sync
TorSvc->>CtrlPort: connect & authenticate
CtrlPort-->>TorSvc: auth ok
TorSvc->>TorSvc: load/create onion key
TorSvc->>CtrlPort: ADD_ONION (map onionPort -> target)
CtrlPort-->>TorSvc: return ServiceID (onion address)
TorSvc-->>LDK: onion address available
LDK-->>App: GetNodeConnectionInfo includes TorAddress/TorPort
App->>Frontend: API returns info with torEnabled and TorAddress
Frontend->>Frontend: display TorSettings / copy Tor URI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
sat-engineer
force-pushed
the
feat/tor-socks-outbound
branch
from
51ea2e8 to
3555a9d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@frontend/src/screens/settings/TorSettings.tsx`:
- Around line 19-24: hasTor currently only checks torAddress which allows
torPort to be undefined and produce a URI like pubkey@address:undefined; update
the guard so hasTor requires both torAddress and torPort (e.g., check truthiness
of nodeConnectionInfo?.torAddress and nodeConnectionInfo?.torPort) and keep
torUri building logic (using nodeConnectionInfo?.pubkey, torAddress and torPort)
unchanged so the component only renders a URI when both values exist.
In `@go.mod`:
- Around line 273-274: go.mod currently replaces github.com/getAlby/ldk-node-go
with github.com/sat-engineer/ldk-node-go v0.0.0-20260207201420-907daba13824 but
go.sum records a different pseudo-version (v0.0.0-20260207201420-ed1bf13381b8);
update the replace directive in go.mod to use the exact pseudo-version present
in go.sum (v0.0.0-20260207201420-ed1bf13381b8) or, if the intended commit is the
one ending in 907daba13824, update go.sum (via `go get module@<commit>` or
running `go mod tidy`) so both files reference the same commit for
github.com/sat-engineer/ldk-node-go; ensure the module name
github.com/sat-engineer/ldk-node-go and the pseudo-version commit suffix are
identical between go.mod and go.sum.
In `@lnclient/ldk/ldk.go`:
- Around line 1065-1072: GetNodeConnectionInfo reads ls.torService without
synchronization while Shutdown can set it to nil under shutdownMutex; to fix,
protect the read by taking the same lock used in Shutdown before accessing
ls.torService (e.g., change shutdownMutex to a RWMutex and use RLock/RUnlock in
GetNodeConnectionInfo or acquire shutdownMutex.Lock/Unlock briefly to copy
ls.torService into a local variable), then use the local variable to call
IsRunning/GetOnionAddress; alternatively, replace ls.torService with an
atomic.Pointer and use Load to safely read it—ensure the Shutdown assignment
uses the matching atomic Store or the same mutex so both reads and writes are
synchronized.
In `@tor/tor.go`:
- Around line 152-156: The conditional logging block checking "if resp.Key !=
nil && key == nil" is dead because loadOrCreateKey always returns a non-nil key
or errors; remove this unreachable block. If the original intent was to detect a
generated key differing from our provided key, replace the check with an
explicit comparison between resp.Key and key (using the appropriate equality
method or field comparison) and log via logger.Logger.Warn when they differ;
otherwise delete the entire resp.Key != nil && key == nil branch. Ensure
references to loadOrCreateKey, resp.Key, key, and logger.Logger.Warn are updated
accordingly.
- Around line 67-116: Start() currently holds s.mu across the long retry loop
(net.DialTimeout) which blocks Stop()/GetOnionAddress()/IsRunning(); to fix,
check and return if s.running while holding s.mu, then unlock s.mu before
starting the connection-retry loop (the block containing net.DialTimeout and
maxRetries), and once a connection is successfully created re-acquire s.mu
before setting shared state (e.g. s.running = true and storing the connection in
the Service struct like s.controlConn) and initializing any other fields; also
re-check s.running after re-acquiring the lock to avoid races where Stop()
started during the retry loop, and ensure you close the newly created conn if
you decide not to use it.
🧹 Nitpick comments (3)
tor/tor.go (1)
265-274:encodeOnionV3does not produce a valid v3 onion address.The function base64-encodes the first 8 bytes of the public key, whereas a v3 onion address is
base32(pubkey || checksum || version)over all 32 bytes. Since the comment says "this is just for logging" and the real address comes fromresp.ServiceID, this isn't functionally broken, but the function name and doc are misleading — it's really just a truncated fingerprint for log identification.Consider renaming to something like
pubkeyFingerprintto avoid confusion.lnclient/ldk/ldk.go (1)
353-383: Magic number9735is repeated across three locations — use the constant.
9735appears at lines 356, 374, and 1070. Thetorpackage already definesDefaultOnionServicePort = 9735. Use it here to avoid drift.Proposed fix
- localPort := 9735 + localPort := albytor.DefaultOnionServicePort // Parse the listening port from config if addrs := strings.Split(cfg.GetEnv().LDKListeningAddresses, ","); len(addrs) > 0 { addr := strings.TrimSpace(addrs[0]) @@ -371,7 +371,7 @@ TorControlPassword: cfg.GetEnv().LDKTorControlPassword, TargetHost: cfg.GetEnv().LDKTorTargetHost, LocalPort: localPort, - OnionServicePort: 9735, + OnionServicePort: albytor.DefaultOnionServicePort, DataDir: filepath.Join(newpath, "tor"), }And at line 1070:
- nodeConnectionInfo.TorPort = 9735 + nodeConnectionInfo.TorPort = albytor.DefaultOnionServicePortapi/api.go (1)
1251-1252:TorEnabledis exposed unconditionally regardless of backend type.
LDKTorEnabledis an LDK-specific config, butinfo.TorEnabledis set for all backend types (LND, Phoenix, Cashu). If a non-LDK user somehow setsLDK_TOR_ENABLED=true, the API would report Tor as enabled even though it has no effect. Consider gating onbackendType == config.LDKBackendType.Proposed fix
- info.TorEnabled = api.cfg.GetEnv().LDKTorEnabled + info.TorEnabled = backendType == config.LDKBackendType && api.cfg.GetEnv().LDKTorEnabled
| // Include Tor onion address if Tor service is running | ||
| if ls.torService != nil && ls.torService.IsRunning() { | ||
| onionAddr := ls.torService.GetOnionAddress() | ||
| if onionAddr != "" { | ||
| nodeConnectionInfo.TorAddress = onionAddr | ||
| nodeConnectionInfo.TorPort = 9735 | ||
| } | ||
| } |
There was a problem hiding this comment.
Data race: ls.torService is read without synchronization.
GetNodeConnectionInfo reads ls.torService (line 1066) while Shutdown writes it to nil (line 552) under shutdownMutex — but GetNodeConnectionInfo doesn't acquire that mutex. This is a data race under the Go memory model.
A simple fix: read the pointer once into a local variable under a lock, or use atomic.Pointer.
Minimal fix sketch
// Include Tor onion address if Tor service is running
- if ls.torService != nil && ls.torService.IsRunning() {
- onionAddr := ls.torService.GetOnionAddress()
+ torSvc := ls.torService // snapshot
+ if torSvc != nil && torSvc.IsRunning() {
+ onionAddr := torSvc.GetOnionAddress()
if onionAddr != "" {
nodeConnectionInfo.TorAddress = onionAddr
- nodeConnectionInfo.TorPort = 9735
+ nodeConnectionInfo.TorPort = albytor.DefaultOnionServicePort
}
}Note: Even this snapshot approach still has a race on the read of ls.torService. You'd need either sync.RWMutex or atomic.Pointer[albytor.Service] to fully fix it. Alternatively, protect both the assignment in Shutdown and the read here with the same mutex.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Include Tor onion address if Tor service is running | |
| if ls.torService != nil && ls.torService.IsRunning() { | |
| onionAddr := ls.torService.GetOnionAddress() | |
| if onionAddr != "" { | |
| nodeConnectionInfo.TorAddress = onionAddr | |
| nodeConnectionInfo.TorPort = 9735 | |
| } | |
| } | |
| // Include Tor onion address if Tor service is running | |
| torSvc := ls.torService // snapshot | |
| if torSvc != nil && torSvc.IsRunning() { | |
| onionAddr := torSvc.GetOnionAddress() | |
| if onionAddr != "" { | |
| nodeConnectionInfo.TorAddress = onionAddr | |
| nodeConnectionInfo.TorPort = albytor.DefaultOnionServicePort | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@lnclient/ldk/ldk.go` around lines 1065 - 1072, GetNodeConnectionInfo reads
ls.torService without synchronization while Shutdown can set it to nil under
shutdownMutex; to fix, protect the read by taking the same lock used in Shutdown
before accessing ls.torService (e.g., change shutdownMutex to a RWMutex and use
RLock/RUnlock in GetNodeConnectionInfo or acquire shutdownMutex.Lock/Unlock
briefly to copy ls.torService into a local variable), then use the local
variable to call IsRunning/GetOnionAddress; alternatively, replace ls.torService
with an atomic.Pointer and use Load to safely read it—ensure the Shutdown
assignment uses the matching atomic Store or the same mutex so both reads and
writes are synchronized.
3555a9d to
73783e9
Compare
…ections Add Go-native Tor hidden service management and wire up SOCKS5 proxy support for outbound .onion peer connections via LDK-node. Backend: - New tor/ package: manages Tor hidden service via control port using cretz/bine library. Handles key persistence, retry logic, and lifecycle management. - Wire up builder.SetTorProxyAddress() in LDK service init when LDK_TOR_SOCKS_HOST is set, with DNS resolution for Docker hostnames. - Expose onion address in node connection info API. - New env vars: LDK_TOR_ENABLED, LDK_TOR_CONTROL_HOST/PORT/PASSWORD, LDK_TOR_TARGET_HOST, LDK_TOR_SOCKS_HOST/PORT. Frontend: - Tor settings page showing hidden service status and onion address. - Tor URI copy button on Channels page. - Settings sidebar entry for Tor (LDK backend only). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
sat-engineer
force-pushed
the
feat/tor-socks-outbound
branch
from
73783e9 to
fb3245f
Compare
If two goroutines raced through Start(), the losing goroutine would close the control connection but leave an orphaned hidden service on the Tor daemon. Now we call DelOnion before closing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
sat-engineer
force-pushed
the
feat/tor-socks-outbound
branch
from
a5c1dd7 to
770895d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lnclient/ldk/ldk.go`:
- Around line 194-209: The current Tor SOCKS proxy setup swallows
SetTorProxyAddress failures: when cfg.GetEnv().LDKTorEnabled &&
cfg.GetEnv().LDKTorSocksHost is set and builder.SetTorProxyAddress(torSocksAddr)
returns an error, change the behavior to surface the failure instead of
continuing silently — return or propagate an error from the surrounding function
(or call a controlled shutdown) so startup fails fast; locate the block using
SetTorProxyAddress, builder, LDKTorSocksHost and LDKTorSocksPort and replace the
current logger-only handling with code that returns a wrapped error (including
socks_addr and original err) to the caller (or if the function signature
disallows returning, escalate to a fatal exit), ensuring callers/users are
notified that Tor proxy setup failed.
🧹 Nitpick comments (2)
lnclient/ldk/ldk.go (1)
935-942: Duplicate alias lookup logic — consider extracting a helper.Lines 936-939 duplicate the same pattern from lines 137-139 in
NewLDKService. Both readNodeAliasconfig and default to"Alby Hub". If the default or key name changes, both sites need updating.♻️ Extract a small helper
func getNodeAlias(cfg config.Config) string { alias, _ := cfg.Get("NodeAlias", "") if alias == "" { alias = "Alby Hub" } return alias }Then use
getNodeAlias(ls.cfg)in both places.tor/tor.go (1)
96-116: Retry loop: 30 retries × 2s sleep = ~60s, plus up to 5s dial timeout per attempt.The total worst-case startup time is ~3.5 minutes if Tor never becomes available. This is acceptable for a daemon startup path, but worth noting that the caller in
ldk.gotreats failure as non-fatal, so this won't block the node indefinitely.One consideration: there's no context/cancellation mechanism passed to
Start(). IfShutdown()is called whileStart()is retrying, the retry loop will run to completion before the service can be stopped. For an MVP this is acceptable sinceStop()is no longer blocked by the mutex, but you may want to add acontext.Contextparameter in the future for graceful cancellation.
| // Configure Tor SOCKS proxy for outbound .onion connections | ||
| if cfg.GetEnv().LDKTorEnabled && cfg.GetEnv().LDKTorSocksHost != "" { | ||
| // Resolve hostname to IP — SetTorProxyAddress requires a numeric IP:port | ||
| socksHost := cfg.GetEnv().LDKTorSocksHost | ||
| if addrs, err := net.LookupHost(socksHost); err == nil && len(addrs) > 0 { | ||
| socksHost = addrs[0] | ||
| } else if err != nil { | ||
| logger.Logger.WithError(err).WithField("host", socksHost).Warn("Failed to resolve Tor SOCKS host, using raw hostname") | ||
| } | ||
| torSocksAddr := fmt.Sprintf("%s:%d", socksHost, cfg.GetEnv().LDKTorSocksPort) | ||
| if err := builder.SetTorProxyAddress(torSocksAddr); err != nil { | ||
| logger.Logger.WithError(err).WithField("socks_addr", torSocksAddr).Error("Failed to set Tor SOCKS proxy address") | ||
| } else { | ||
| logger.Logger.WithField("socks_addr", torSocksAddr).Info("Configured Tor SOCKS proxy for outbound .onion connections") | ||
| } | ||
| } |
There was a problem hiding this comment.
SOCKS proxy setup failure is silently swallowed — node will not be able to connect to .onion peers.
When SetTorProxyAddress fails (line 204), only an error is logged but startup continues normally. The user may not notice their node can't reach .onion peers. Consider either returning an error (since the user explicitly set LDK_TOR_ENABLED=true and LDK_TOR_SOCKS_HOST), or at minimum making the log level more prominent (e.g., Warn instead of Error, since Error without action is confusing).
🤖 Prompt for AI Agents
In `@lnclient/ldk/ldk.go` around lines 194 - 209, The current Tor SOCKS proxy
setup swallows SetTorProxyAddress failures: when cfg.GetEnv().LDKTorEnabled &&
cfg.GetEnv().LDKTorSocksHost is set and builder.SetTorProxyAddress(torSocksAddr)
returns an error, change the behavior to surface the failure instead of
continuing silently — return or propagate an error from the surrounding function
(or call a controlled shutdown) so startup fails fast; locate the block using
SetTorProxyAddress, builder, LDKTorSocksHost and LDKTorSocksPort and replace the
current logger-only handling with code that returns a wrapped error (including
socks_addr and original err) to the caller (or if the function signature
disallows returning, escalate to a fatal exit), ensuring callers/users are
notified that Tor proxy setup failed.
|
Hi, thanks for the PR. But we don't need any special logic inside Alby Hub for LND which supports TOR. Should this be inside LDK-Node instead? |
2 participants
Summary
tor/package: Go-native Tor hidden service management via control port (cretz/bine library), with key persistence, retry logic, and lifecycle managementbuilder.SetTorProxyAddress()in LDK service init whenLDK_TOR_SOCKS_HOSTis set, with DNS resolution for Docker hostnamesLDK_TOR_ENABLED,LDK_TOR_CONTROL_HOST/PORT/PASSWORD,LDK_TOR_TARGET_HOST,LDK_TOR_SOCKS_HOST/PORTCloses #2035
Related PRs
This is part of a 3-PR series enabling Tor .onion-to-.onion Lightning peer connections in Alby Hub:
Dependency chain: PR 1 → PR 2 → PR 3
Test plan
go test ./tor/ -race— 21 tests passing (key management, service lifecycle, concurrency, encoding)go build ./...passes🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
Summary by CodeRabbit
New Features