Dial primary plus fallback arkd-wallets#1099
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
f68b468 to
5ee109f
Compare
3332bdd to
b1742ec
Compare
b1742ec to
841da39
Compare
03bc417 to
517fe5c
Compare
141b8d3 to
8af3323
Compare
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Arkana Code Review — bob/arkd-multi-wallet-dial
Plumbing PR to dial and validate fallback LP wallets at startup. The actual sweep fallback wiring is deferred. Overall structure is clean — good network validation, good cleanup on dial failure, solid test coverage. A few issues to address:
🔴 Must Fix
1. Fallback wallets not locked on shutdown — security gap
internal/interface/grpc/service.go (new code in stop()): Fallback wallets get Close() only. The primary wallet gets Lock(ctx) + Close() in application/service.go:394-397. An unlocked arkd-wallet that's still running after arkd shuts down is network-accessible with full signing authority. Lock fallbacks before closing them.
// Current (stop):
for _, fb := range s.appConfig.FallbackWalletServices() {
fb.Close()
}
// Suggested:
for _, fb := range s.appConfig.FallbackWalletServices() {
_ = fb.Lock(context.Background()) // best-effort, same as primary
fb.Close()
}2. No dedup or self-reference check on fallback addresses
internal/config/config.go (dialFallbackWallets): If a user accidentally sets ARKD_WALLET_FALLBACK_ADDRS=localhost:6060 (same as primary) or a:6060,a:6060, arkd opens duplicate connections to the same wallet. When sweep fallback logic lands, this could cause double-spend attempts or lock contention. Add validation:
- Reject any fallback addr that matches
c.WalletAddr - Reject duplicate entries in the list
🟡 Suggestions
3. FallbackWalletServices() returns the backing slice directly
internal/config/config.go:664: Callers can mutate the returned slice. In a codebase where bugs lose money, return a copy:
func (c *Config) FallbackWalletServices() []ports.WalletService {
out := make([]ports.WalletService, len(c.walletFallbacks))
copy(out, c.walletFallbacks)
return out
}4. ensureWalletReady error messages use index, not address
internal/interface/grpc/service.go:671-686: Error messages say "fallback wallet 0 is not initialized" — an operator has to count their comma-separated list to figure out which one. Include the address in the error for operability. You have access to s.appConfig.WalletFallbackAddrs for this.
5. Docker compose: wallet-2 has no restart: unless-stopped health check
docker-compose.regtest.yml: The primary wallet has a dependency chain, but wallet-2 only depends on nbxplorer. Not blocking, but consider adding a healthcheck or at least a comment noting that arkd will crash-loop until wallet-2 is ready (which is the intended behavior per ensureWalletReady).
✅ What looks good
- Network mismatch is a hard fatal — correct, prevents silent misconfiguration
closeWallets()cleanup on partial dial failure — proper resource management- Test coverage:
TestParseWalletFallbackAddrscovers edge cases;TestDialFallbackWalletscovers same-network, mismatch, and dial-error paths with Close() assertion counting - E2E setup correctly initializes/unlocks wallet-2 before arkd starts
newWalletClientindirection for testability is a good pattern- README accurately describes the current scope and explicitly says sweep wiring is deferred
513eca3 to
9e5e7cb
Compare
a60692f to
835c866
Compare
There was a problem hiding this comment.
🔍 Arkana Follow-up Review — bob/arkd-multi-wallet-dial
Reviewed the force-push (rebase onto updated #1098). No substantive code changes to this PR's layer — all 5 findings from my previous review remain unaddressed.
🔴 Still open — must fix
1. Fallback wallets not locked on shutdown
internal/interface/grpc/service.go:235-237 — still just fb.Close(), no fb.Lock(). The primary wallet gets Lock() + Close() in application/service.go. An unlocked fallback arkd-wallet remains network-accessible with signing authority after arkd shuts down.
2. No dedup or self-reference check on fallback addresses
internal/config/config.go:dialFallbackWallets — still no validation that a fallback addr ≠ c.WalletAddr or that there are no duplicate entries. When the sweep fallback in #1101 lands, duplicates could cause double-spend attempts or lock contention.
🟡 Still open — suggestions
3. FallbackWalletServices() returns backing slice (config.go:668-671)
4. Error messages use index not address (service.go:684-706)
These are the same as before. See my original review for details and suggested fixes.
No new issues introduced by the rebase. Blocking on findings 1 and 2.
835c866 to
d6d6423
Compare
There was a problem hiding this comment.
🔍 Arkana Review #3 — bob/arkd-multi-wallet-dial
Third pass after another force-push (rebase). Diff unchanged — no new code to review. All previous findings remain open.
🔴 Still blocking
1. Fallback wallets not locked on shutdown — internal/interface/grpc/service.go:235-237
Primary gets Lock() + Close(). Fallbacks only get Close(). An unlocked arkd-wallet with signing authority stays network-accessible after arkd shuts down.
2. No dedup / self-reference check — internal/config/config.go:dialFallbackWallets
No validation that a fallback addr ≠ primary WalletAddr, or that fallback list has no duplicates. With #1101's sweep fallback, duplicates → double-spend risk or lock contention.
🟡 Still open
3. FallbackWalletServices() returns backing slice — config.go:668-671
4. Error messages use index not address — service.go:684-706
See review #1 for details and suggested fixes on all four items.
d6d6423 to
57edf9d
Compare
There was a problem hiding this comment.
🔍 Arkana Review #4 — bob/arkd-multi-wallet-dial
Fourth pass after rebase. No code changes — diff identical to reviews #1–#3. All findings remain open.
🔴 Blocking
1. Fallback wallets not locked on shutdown — internal/interface/grpc/service.go:235-237
Primary gets Lock() + Close(). Fallbacks only get Close(). Unlocked arkd-wallet with signing authority stays network-accessible after arkd shuts down.
2. No dedup / self-reference check — internal/config/config.go:dialFallbackWallets
No validation that fallback addr ≠ primary WalletAddr or that the list has no duplicates. With #1101's sweep fallback, duplicates → double-spend risk or lock contention.
🟡 Open
3. FallbackWalletServices() returns backing slice — config.go:668-671
4. Error messages use index not address — service.go:684-706
See review #1 for details and suggested fixes on all four items.
Add ARKD_WALLET_FALLBACK_ADDRS so arkd can connect to a primary arkd-wallet plus additional LP wallets. Fallbacks are dialed and validated (reachable, same network, initialized and unlocked) at startup; the primary is unchanged and remains the sole source of the forfeit pubkey, addresses and signing. Fallbacks are not used by sweep yet. The regtest compose stack now runs a second arkd-wallet so the existing e2e suite exercises arkd with a fallback plugged in.
edf1888 to
5d9ad68
Compare
57edf9d to
03d4fe9
Compare
There was a problem hiding this comment.
🔍 Arkana Review #5 — bob/arkd-multi-wallet-dial
Fifth pass after rebase. Diff identical to reviews #1–#4. No code changes — all findings remain open.
🔴 Blocking
1. Fallback wallets not locked on shutdown — internal/interface/grpc/service.go:235-237
Primary gets Lock() + Close(). Fallbacks only get Close(). Unlocked arkd-wallet with signing authority stays network-accessible after arkd shuts down.
2. No dedup / self-reference check — internal/config/config.go:dialFallbackWallets
No validation that fallback addr ≠ primary WalletAddr or that the list has no duplicates. With #1101's sweep fallback, duplicates → double-spend risk or lock contention.
🟡 Open
3. FallbackWalletServices() returns backing slice — config.go:668-671
4. Error messages use index not address — service.go:684-706
See review #1 for details and suggested fixes on all four items.
Pair each dialed fallback wallet with its address (FallbackWallet) so ensureWalletReady reports the specific wallet that failed (host:port) instead of a positional index, which isn't actionable when the error surfaces. Also clarify the comment on why fallback balance isn't checked.
There was a problem hiding this comment.
🔍 Arkana Review #6 — bob/arkd-multi-wallet-dial
Reviewed new commit 98f24639 — introduces FallbackWallet struct pairing Addr + Service, renames FallbackWalletServices() → FallbackWallets(), and updates ensureWalletReady errors to use %q with fb.Addr instead of positional index.
New commit assessment
Good: The FallbackWallet struct is a clean solution to finding #4. Error messages now name the wallet by host:port — much better for operators. The type change is well-propagated through dialFallbackWallets, closeWallets, stop(), and ensureWalletReady.
Tests: TestDialFallbackWallets still works correctly — the fakeFallbackWallet satisfies ports.WalletService which is wrapped in FallbackWallet.Service. No test gaps introduced.
🟠 New issue from this commit
5. Stacked PR #1101 will break — #1101 calls c.FallbackWalletServices() (the old name) and expects []ports.WalletService (the old type) in at least 3 call sites. After this rename, #1101 won't compile when rebased. Not blocking for this PR, but Bob should rebase #1101 onto this branch before merging either.
Findings status
| # | Finding | Status |
|---|---|---|
| 1 | 🔴 Fallback wallets not locked on shutdown (service.go:238-239) |
Still open — stop() still only calls fb.Service.Close(), no Lock() |
| 2 | 🔴 No dedup / self-reference check (config.go:851-875) |
Still open — no validation that fallback addr ≠ primary or no duplicates |
| 3 | 🟡 FallbackWallets() returns backing slice (config.go:679-681) |
Still open |
| 4 | ✅ Error messages use index not address | Fixed by this commit |
| 5 | 🟠 Stacked PR #1101 uses old API name | New — compile break in downstream |
Blocking on findings 1 and 2. Finding 5 is a heads-up for the PR stack.
On this branch arkd only dials and readiness-checks fallback wallets at startup; sweep signing isn't wired up yet. Reword the docker-compose and dialFallbackWallets comments so they don't imply active sweep-fallback use.
There was a problem hiding this comment.
🔍 Arkana Review #7 — bob/arkd-multi-wallet-dial
New commit 2738e658 is comment-only — rewords the docker-compose comment and dialFallbackWallets godoc to clarify sweep signing isn't wired up yet. No logic changes. No new issues.
Findings status (unchanged from review #6)
| # | Finding | Status |
|---|---|---|
| 1 | 🔴 Fallback wallets not locked on shutdown (service.go:238-239) — stop() calls fb.Service.Close() but not Lock(). Primary gets both. An unlocked arkd-wallet with signing authority stays network-accessible after arkd shuts down. |
Still open |
| 2 | 🔴 No dedup / self-reference check (config.go:853-875) — no validation that a fallback addr ≠ primary WalletAddr or that the fallback list has no duplicates. With #1101's sweep fallback, duplicates → double-spend risk or lock contention. |
Still open |
| 3 | 🟡 FallbackWallets() returns backing slice (config.go:681) — caller can mutate c.walletFallbacks. Return a copy or document the contract. |
Still open |
| 4 | ✅ Error messages use index not address | Fixed in 98f2463 |
| 5 | 🟠 Stacked PR #1101 uses old API name — #1101 calls FallbackWalletServices() (old name) and expects []ports.WalletService. Needs rebase after this lands. |
Still open |
Blocking on findings 1 and 2.
…er fallback Fail fast at dial time if a fallback address equals the primary WalletAddr or repeats another fallback, instead of silently dialing the same wallet twice (pointless now, double-spend/lock-contention risk once fallbacks sign sweeps). Matching is by literal address.
There was a problem hiding this comment.
🔍 Arkana Review #8 — bob/arkd-multi-wallet-dial
New commit ba566dab adds dedup and self-reference validation in dialFallbackWallets. Assessment:
✅ Finding #2 is now fixed
The implementation is correct:
- Primary check (
config.go:861):addr == c.WalletAddrrejects before dialing, avoiding a wasted connection. Cleans up already-dialed fallbacks viacloseWallets(). - Duplicate check (
config.go:865-868):seenmap with literal string matching. Also rejects before dialing and cleans up. - Test coverage: Two new test cases verify error messages, cleanup (Close counts), and that no unnecessary dials are made (
require.Equal(t, 1, calls)). Good.
One note: the matching is literal (addr == c.WalletAddr). Addresses like localhost:6060 vs 127.0.0.1:6060 vs [::1]:6060 would bypass this check. This is acceptable for now since the operator controls both values and the PR description documents this, but worth a follow-up comment in the code.
Updated findings status
| # | Finding | Status |
|---|---|---|
| 1 | 🔴 Fallback wallets not locked on shutdown (service.go:238-239) — stop() calls fb.Service.Close() but not Lock(). Primary gets both. An unlocked arkd-wallet with signing authority stays network-accessible after arkd shuts down. |
Still open |
| 2 | ✅ No dedup / self-reference check | Fixed in ba566dab |
| 3 | 🟡 FallbackWallets() returns backing slice (config.go:681) — caller can mutate c.walletFallbacks. Return a copy or document the contract. |
Still open |
| 4 | ✅ Error messages use index not address | Fixed in 98f2463 |
| 5 | 🟠 Stacked PR #1101 uses old API name — #1101 calls FallbackWalletServices() (old name) and expects []ports.WalletService. Needs rebase after this lands. |
Still open |
Still blocking on finding #1. The lock-on-shutdown gap is a real security concern for when fallback wallets start signing sweeps in #1101.
1: arkd never unlocks wallets anymore so it shouldnt lock them either |
1 participant
Why
Next step towards letting a single
arkdfront more than one LP. This teachesarkdto connect to a primaryarkd-walletplus N fallback wallets belonging to additional liquidity providers. It is purely additive: the primary path is unchanged and fallbacks are dialed and validated but not yet used by sweep (that wiring lands in a follow-up). Very simple: we just support adding other wallets and try connecting to them, we dont actually use the wallets yet for sweeps/fallbacks. The PR is just a small step to introduce new wallet support before actually using the wallets for functionality.Stacked PRs (merge top-down):
add Settings domain with DB persistence and admin CRUD API #939— ✅ merged intomastermaster, merges firstbob/arkd-require-unlocked-walletbob/arkd-multi-wallet-dial)This PR depends on #1098 landing first.
What changes
Config
ARKD_WALLET_FALLBACK_ADDRS(comma-separatedhost:port). Parsed with an explicit comma split (parseWalletFallbackAddrs), notviper.GetStringSlice, because the latter splits on whitespace, not commas (verified empirically), and would silently mis-parse the list.walletService()dials the primary (unchanged), thendialFallbackWallets()dials each fallback via an indirectednewWalletClient(so it is unit-testable) and hard-fails if a fallback is unreachable or on a different network than the primary, closing any partially-dialed connections to avoid leaks. It also rejects a fallback whose address duplicates the primary or another fallback.Config.walletFallbacksand exposed viaFallbackWallets()(each pairing the dial address with the wallet client). The primary staysc.wallet, so GetInfo, the forfeit pubkey/address, connector address, scanning, signing, and sweep are all untouched.Startup / shutdown
ensureWalletReady()now also requires each fallback wallet to be initialized and unlocked (hard-fail); the zero-balance warning stays primary-only.stop()closes the fallback connections (the primary is closed by the app service).Exercised end to end
docker-compose.regtest.ymlnow runs a secondarkd-wallet(arkd-wallet-2, host port 6061, its own datadir/volume/signer key) wired intoarkdviaARKD_WALLET_FALLBACK_ADDRS=arkd-wallet-2:6060.arkdstarts, so the entire existing e2e suite now runs with a fallback wallet plugged in.Docs / tests
ARKD_WALLET_FALLBACK_ADDRSrow and a "Configuring multiple LP wallets" subsection.TestParseWalletFallbackAddrs(parsing edge cases) andTestDialFallbackWallets(same-network success; network-mismatch hard-fail with cleanup; dial-error hard-fail with cleanup; fallback-equals-primary hard-fail; duplicate-fallback hard-fail).Operator impact
Every wallet, primary and fallback, must be initialized and unlocked out of band and must be on the same network as the primary;
arkdvalidates this at startup and refuses to start otherwise.Testing
go build ./...,go vet,make lint(0 issues), the new config unit tests, and the e2e package compile all pass.Status
Draft: depends on #1098 landing first; the sweep fallback that actually uses these wallets is #1101.