feat: allow updating LDK chain source via UI#2019
feat: allow updating LDK chain source via UI#2019Dunsin-cyber wants to merge 8 commits intogetAlby:masterfrom
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds LDK on-chain source configuration: config validation helpers for Esplora/Electrum, a Config API method and request model, tests, frontend settings UI/route, HTTP/Wails endpoints to save user overrides, mocks updates, and LDK selection logic that prefers user overrides over environment fallbacks. ChangesChain Source feature (single coherent DAG)
Sequence Diagram(s)sequenceDiagram
actor User
participant FE as Frontend
participant API as HTTP Service
participant CFG as Config Layer
participant DB as Database
participant LDK as LDK Client
User->>FE: Open Chain Source settings / enter data
FE->>FE: client-side validation
User->>FE: Click Save
FE->>API: PATCH /api/ldk-onchain-source (UpdateChainConfigRequest)
API->>API: ensure LN backend == LDK
API->>CFG: ValidateChainSource(chainSource, url)
CFG->>CFG: validateEsplora / validateElectrum (HTTP or TCP check)
CFG-->>API: validation result
alt Validation success
API->>DB: SetUpdate() user override fields
DB-->>API: persisted
API-->>FE: 200 OK
FE->>User: success toast
else Validation failed
API-->>FE: 400 error
FE->>User: show error
end
Note over LDK,DB: On next LDK init
LDK->>DB: query UserChainSource
alt user override exists and valid
LDK->>LDK: configure user-selected chain source
else
LDK->>LDK: use environment-based chain source
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@config/models.go`:
- Around line 94-105: The struct comment for UpdateChainConfigRequest's
ChainSource is incorrect and lists "bitcoind" whereas the handler expects
"bitcoind_rpc"; update the inline comment on the ChainSource field in type
UpdateChainConfigRequest to list "bitcoind_rpc" (so it reads something like
`"esplora", "electrum", "bitcoind_rpc", or "default"`) to match the
handler/clients and avoid 400 errors.
In `@config/tests/config_test.go`:
- Around line 365-412: The tests "Esplora - Connection Refused" and "Electrum -
Connection Refused" use a fixed port 127.0.0.1:54321 which can be flaky; replace
the hardcoded port with a dynamically reserved/free port that you immediately
close to simulate refusal. Update the cases in config_test.go to call a helper
(e.g., getFreePort or reserveFreePort) that does net.Listen("tcp",
"127.0.0.1:0"), captures the assigned port, closes the listener, and returns the
host:port string; then use that host:port in the Esplora URL and in the electrum
URL prefixes ("tcp://"+addr or "ssl://"+addr) and keep the same shouldError and
errorContains expectations. Ensure the helper is added to the test file (e.g.,
getFreePort) and used for both offending test cases.
In `@frontend/src/screens/settings/ChainSource.tsx`:
- Around line 61-98: The validateForm function currently allows invalid schemes
and missing ports; update it to require exact schemes and backend-compatible
formats: for esplora (in validateForm, checking formData.chainSource ===
"esplora") require formData.url.startsWith("http://") ||
formData.url.startsWith("https://") (not just "http"), and additionally ensure
the URL includes a host (e.g., non-empty hostname after the scheme). For
electrum (formData.chainSource === "electrum") require formData.url to start
with "ssl://" or "tcp://" and validate that the remainder contains host:port
(i.e., a colon and a numeric port), and that the port is a valid integer in
range 1-65535. For bitcoind_rpc keep the existing presence checks on
formData.host, formData.port, formData.user, formData.pass but tighten port
validation to ensure Number(formData.port) is an integer between 1 and 65535.
Add clear validationError messages referencing the specific field failures.
In `@http/http_service.go`:
- Around line 1600-1652: The code ignores errors returned by
httpSvc.cfg.SetUpdate in each switch case, which can mask DB write failures;
update the handler to check the returned error from every SetUpdate call (e.g.,
in the "default", "esplora", "electrum", and "bitcoind_rpc" branches) and if any
call returns an error, return an HTTP error response (e.g., c.JSON with
http.StatusInternalServerError and an ErrorResponse containing err.Error()) so
the API surfaces config write failures instead of reporting success; keep
existing input validation (ValidateChainSource and the strconv.ParseUint check)
and return early on the first SetUpdate error.
In `@lnclient/ldk/ldk.go`:
- Around line 163-176: The current bitcoind_rpc branch accepts an invalid or
empty UserBitcoindPort because it ignores ParseUint errors and treats a parsed 0
as valid; update the logic around host, portStr, strconv.ParseUint and
sourceConfigured so you only call builder.SetChainSourceBitcoindRpc and set
sourceConfigured = true when ParseUint returns no error and the parsed port is
within valid range (>0 and <=65535); if parsing fails or yields 0, do not mark
the override as configured so env fallback can proceed. Reference the host,
portStr, strconv.ParseUint call, builder.SetChainSourceBitcoindRpc and
sourceConfigured in your changes.
In `@wails/wails_handlers.go`:
- Around line 1210-1218: The JSON unmarshal error logging currently prints the
raw request body (which can contain Bitcoind RPC credentials) in
wails_handlers.go; update the error branch around the
config.UpdateChainConfigRequest decode so you do not log the raw body — replace
the "body" field with a redacted placeholder (e.g. "<redacted>" or a
masked/hashed summary) when calling logger.Logger.WithFields/WithError in that
error case, leaving other fields ("route", "method", error) intact; locate the
payload decode code that constructs payload :=
&config.UpdateChainConfigRequest{} and the subsequent json.Unmarshal error
handling and change the body field used in the log to a redacted value.
- Around line 1223-1275: The SetUpdate calls in the switch cases (e.g.,
cfg.SetUpdate in the "default", "esplora", "electrum", and "bitcoind_rpc"
branches) currently ignore returned errors which can cause a success response
without persisted changes; update each SetUpdate invocation to capture its
error, and if non-nil return a WailsRequestRouterResponse containing the error
(similar to how validation errors are returned), ensuring you short-circuit
further updates on failure so callers receive an accurate error when
cfg.SetUpdate fails.
🧹 Nitpick comments (1)
frontend/src/screens/settings/ChainSource.tsx (1)
103-109: Prevent double-submit while save is in-flight.
handleSubmitcan be triggered multiple times beforeisLoadingflips, resulting in duplicate requests.🛡️ Simple guard against repeat submits
const handleSubmit = async () => { - if (!validateForm()) { + if (isLoading || !validateForm()) { return; } setIsLoading(true);
|
I think we might need a screen at startup for adjusting that setting if the node doesn't start up because the user entered wrong/bad values there, otherwise it will be stuck with a dead node which will require manual intervention somehow. |
@frnandu I don't think we can do that as it can be a security exploit (we had this same thought for a few other things). I think if the user accidentally breaks it then they need to manually edit the config on the machine. |
|
@frnandu I am not even sure if we should add this feature in the UI for the reason above. Do you think it's really needed or the user can do it manually via env or some other method? |
|
I think for startos/umbrel and alike changing things manually in physical files might be difficult for most users, and/or upgrades of such OSes will overwrite things and create frustration. |
|
@frnandu Start9 I think supports setting env variables through a UI. I wonder if Umbrel has something similar |
This would need to be after user unlocks (enters password) right? |
We also had the issue with disconnecting the alby account (e.g. if you are blocked on an IP level then it's hard to use the hub) This would be a fundamental change to how the startup works because providing the unlock password is tied to starting the node, which would fail in this case. I don't think we should make a change here without a very strong reason. |
|
Another option would be to do test connection before storing the new values. |
|
@frnandu I think we did the most important thing here already which is show the current values to the user - maybe we can link a guide for users who want to change it? |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
lnclient/ldk/ldk.go (1)
179-187:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject port
0before marking the Bitcoind override configured.
strconv.ParseUint(..., 16)still accepts"0", so this branch can callSetChainSourceBitcoindRpc(..., 0, ...)and setsourceConfigured = true. That leaves a broken user override active instead of falling back.Suggested fix
port, err := strconv.ParseUint(portStr, 10, 16) - if err != nil { + if err != nil || port == 0 { logger.Logger.WithError(err).Warn("Invalid user bitcoind RPC port; falling back to env config") break }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lnclient/ldk/ldk.go` around lines 179 - 187, The parsed port value from strconv.ParseUint can be 0, so before calling builder.SetChainSourceBitcoindRpc and setting sourceConfigured/chainSource, check that the parsed uint64 port is non-zero; if port==0, log a warning via logger.Logger.WithError or logger.Logger.Warn about an invalid (zero) RPC port and fall through (do not call SetChainSourceBitcoindRpc and do not set sourceConfigured or chainSource). Update the logic around strconv.ParseUint, SetChainSourceBitcoindRpc, sourceConfigured and chainSource to enforce this validation.wails/wails_handlers.go (1)
1242-1249:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedact the decode-error log for this payload.
This request body can contain Bitcoind RPC credentials and the unlock password, so logging
bodyhere leaks secrets into the app logs.Suggested fix
if err != nil { logger.Logger.WithFields(logrus.Fields{ "route": route, "method": method, - "body": body, + "body": "<redacted>", }).WithError(err).Error("Failed to decode request to wails router") return WailsRequestRouterResponse{Body: nil, Error: err.Error()} }As per coding guidelines "Never log sensitive data such as seeds, macaroons, or tokens".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wails/wails_handlers.go` around lines 1242 - 1249, The log currently includes the raw request body which may contain sensitive Bitcoind RPC credentials/unlock passwords; remove or redact the "body" field in the logger call inside the JSON decode error path (the block that unmarshals into config.UpdateChainConfigRequest and calls logger.Logger.WithFields with "route" and "method"); replace "body" with a non-sensitive indicator (e.g., "body_redacted": true or "body_len": len(body)) or omit it entirely, keeping the existing WithError(err).Error(...) message so the error is recorded without leaking secrets.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lnclient/ldk/ldk.go`:
- Around line 167-171: The code treats decrypted RPC credentials as valid even
when cfg.Get returns an error, causing empty user/pass to override env
fallbacks; update the calls that read UserBitcoindUser and UserBitcoindPass (and
the related UserBitcoindHost/UserBitcoindPort reads) to check the error returned
from cfg.Get and only accept the returned value as an override when err == nil,
otherwise leave the corresponding variables unset (or nil/empty marker) so the
env-based fallback logic is used; reference the cfg.Get calls and the variables
host, portStr, user, pass and the encryptionKey to locate and fix the logic
around those reads.
In `@wails/wails_handlers.go`:
- Around line 1284-1341: Move the setUpdate("UserChainSource", ...) call to
after all dependent setUpdate calls succeed in each non-default branch (esplora,
electrum, bitcoind_rpc) so the override flag is only written last; i.e., in the
esplora, electrum, and bitcoind_rpc case blocks, persist UserEsploraUrl /
UserElectrumUrl /
UserBitcoindHost/UserBitcoindPort/UserBitcoindUser/UserBitcoindPass (using
setUpdate with payload and unlockPassword where applicable) and only if all
those setUpdate calls return nil then call setUpdate("UserChainSource",
"<source>", "") to flip the source, preserving existing validation
(cfg.ValidateChainSource, strconv.ParseUint, cfg.CheckUnlockPassword) and error
handling around setUpdate failures.
- Around line 1234-1239: This route handler for "/api/ldk-onchain-source"
currently allows any HTTP method to mutate config; add an early check on the
incoming request's Method (from the WailsRequestRouterRequest passed into the
router handler) and return a WailsRequestRouterResponse with an appropriate
Error message if the method is not "PATCH", so state changes only happen for
PATCH requests; update the case block that contains the LNBackendType check to
perform this method validation before reading/modifying config.
---
Duplicate comments:
In `@lnclient/ldk/ldk.go`:
- Around line 179-187: The parsed port value from strconv.ParseUint can be 0, so
before calling builder.SetChainSourceBitcoindRpc and setting
sourceConfigured/chainSource, check that the parsed uint64 port is non-zero; if
port==0, log a warning via logger.Logger.WithError or logger.Logger.Warn about
an invalid (zero) RPC port and fall through (do not call
SetChainSourceBitcoindRpc and do not set sourceConfigured or chainSource).
Update the logic around strconv.ParseUint, SetChainSourceBitcoindRpc,
sourceConfigured and chainSource to enforce this validation.
In `@wails/wails_handlers.go`:
- Around line 1242-1249: The log currently includes the raw request body which
may contain sensitive Bitcoind RPC credentials/unlock passwords; remove or
redact the "body" field in the logger call inside the JSON decode error path
(the block that unmarshals into config.UpdateChainConfigRequest and calls
logger.Logger.WithFields with "route" and "method"); replace "body" with a
non-sensitive indicator (e.g., "body_redacted": true or "body_len": len(body))
or omit it entirely, keeping the existing WithError(err).Error(...) message so
the error is recorded without leaking secrets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b763a2e-2ac7-43c8-b63c-3b2840fa3500
📒 Files selected for processing (6)
config/models.gofrontend/src/screens/settings/ChainSource.tsxhttp/http_service.golnclient/ldk/ldk.goservice/start.gowails/wails_handlers.go
✅ Files skipped from review due to trivial changes (1)
- service/start.go
🚧 Files skipped from review as they are similar to previous changes (2)
- http/http_service.go
- frontend/src/screens/settings/ChainSource.tsx
| host, _ := cfg.Get("UserBitcoindHost", "") | ||
| portStr, _ := cfg.Get("UserBitcoindPort", "") | ||
| // User and pass are encrypted at rest with the unlock password | ||
| user, _ := cfg.Get("UserBitcoindUser", encryptionKey) | ||
| pass, _ := cfg.Get("UserBitcoindPass", encryptionKey) |
There was a problem hiding this comment.
Don’t treat unreadable encrypted RPC credentials as a valid user override.
The reads for UserBitcoindUser and UserBitcoindPass ignore cfg.Get errors. If decryption fails or the wrong encryptionKey is passed, this branch still marks the override configured and starts Bitcoind RPC with empty credentials, which blocks env fallback.
Suggested fix
- user, _ := cfg.Get("UserBitcoindUser", encryptionKey)
- pass, _ := cfg.Get("UserBitcoindPass", encryptionKey)
+ user, err := cfg.Get("UserBitcoindUser", encryptionKey)
+ if err != nil {
+ logger.Logger.WithError(err).Warn("Failed to read user Bitcoind RPC username; falling back to env config")
+ break
+ }
+ pass, err := cfg.Get("UserBitcoindPass", encryptionKey)
+ if err != nil {
+ logger.Logger.WithError(err).Warn("Failed to read user Bitcoind RPC password; falling back to env config")
+ break
+ }Also applies to: 173-187
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lnclient/ldk/ldk.go` around lines 167 - 171, The code treats decrypted RPC
credentials as valid even when cfg.Get returns an error, causing empty user/pass
to override env fallbacks; update the calls that read UserBitcoindUser and
UserBitcoindPass (and the related UserBitcoindHost/UserBitcoindPort reads) to
check the error returned from cfg.Get and only accept the returned value as an
override when err == nil, otherwise leave the corresponding variables unset (or
nil/empty marker) so the env-based fallback logic is used; reference the cfg.Get
calls and the variables host, portStr, user, pass and the encryptionKey to
locate and fix the logic around those reads.
3 participants
fixes #1985
Implemented handlers in both the HTTP server and Wails app to allow configuring the LDK chain source. You can now switch between Esplora, Electrum, and Bitcoind RPC, or reset to defaults.
Note: We will be able to display the current chain source in the UI once #2013 is merged.
Alby.Hub.-compressed.mp4
Summary by CodeRabbit
New Features
Validation
Tests