arkd-wallet: source UTXOs from NBXplorer events and cache wallet key paths - #1093
arkd-wallet: source UTXOs from NBXplorer events and cache wallet key paths#1093bitcoin-coder-bob wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. WalkthroughThis PR adds a bounded LRU cache for script-to-key-path mappings. The wallet populates and reads the cache during UTXO operations and private-key derivation. NBXplorer now builds UTXOs directly from matching websocket transaction events. ChangesKey Path Caching for UTXO and Key Derivation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change sources matching wallet UTXOs from NBXplorer notifications and caches key paths to reduce lookup overhead. No concrete merge-blocking risk is established in the supplied evidence. Sequence Diagram(s)sequenceDiagram
participant WebsocketReader
participant EventHandler as utxosFromTransactionEvent
participant NotificationsChannel
WebsocketReader->>EventHandler: newtransaction event
EventHandler->>EventHandler: validate group and parse outputs
EventHandler->>NotificationsChannel: UTXOs with key paths
sequenceDiagram
participant Caller
participant Wallet as getPrivateKeyFromScript
participant KeyPathCache
participant NBXplorer
Caller->>Wallet: script
Wallet->>KeyPathCache: get(script)
alt Cache hit
KeyPathCache-->>Wallet: derivation scheme and key path
Wallet-->>Caller: derived private key
else Cache miss
Wallet->>NBXplorer: lookup script
NBXplorer-->>Wallet: key path result
Wallet->>KeyPathCache: set successful mapping
Wallet-->>Caller: derived private key or nil
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Code Review — arkd-wallet: source UTXOs from NBXplorer events and cache wallet key paths
Reviewer: Arkana (automated, aggressive)
Verdict: Request changes (1 actionable issue) + request human sign-off (signing path modified)
Summary
Clean, well-tested PR. The two optimizations are sound:
- Parsing UTXO data directly from the
newtransactionevent payload instead of re-fetching the entire group UTXO set — eliminates an O(n) HTTP call per event. - Caching
script → (derivationScheme, keyPath)to skip per-input NBXplorer lookups during signing.
Test coverage is thorough (event parsing, cache hit/miss/fallback/concurrency, edge cases). The json.RawMessage change for event.Data and the Confirmations int → uint32 alignment are correct improvements. The behavioral fix in getPrivateKeyFromScript (skipping schemes that return an empty KeyPath instead of trying to derive from "") is a genuine bug fix.
No cross-repo breakage: ports.Utxo is only consumed within pkg/arkd-wallet/, and the new KeyPath field is added with named-field construction everywhere.
🔴 ACTIONABLE — Unbounded cache growth
pkg/arkd-wallet/core/application/wallet/keypath_cache.go
The keyPathCache map grows without bound. Every UTXO the wallet ever sees (via ListConnectorUtxos, SelectUtxos, withdrawAll, getBalance, and event notifications) adds an entry that is never evicted.
For a long-running arkd server managing many watched addresses (e.g. thousands of boarding UTXOs per round across many rounds), this is a slow memory leak. The comment says "cached entries never need invalidation" — true for correctness, but irrelevant for memory management. Scripts from spent UTXOs will never be looked up again, yet their entries persist forever.
Options (pick one):
- LRU with a reasonable cap (e.g. 10k–50k entries). A signing operation only needs the scripts of the inputs being signed, so eviction is always safe — the NBXplorer fallback path still works.
- Periodic sweep: clear the cache when the wallet detects a new round/block (coarse but simple).
- Explicit eviction after signing: remove entries for scripts that were just signed (they're spent now).
The first option is simplest and safest. An LRU miss just falls back to the existing HTTP path, so there's zero correctness risk.
🟡 STYLE — Positional struct init is fragile
pkg/arkd-wallet/core/application/wallet/service.go:65
return &wallet{opts, newOutpointLocker(time.Minute), nil, newKeyPathCache(), make(chan bool)}This uses positional field initialization. If anyone reorders the wallet struct fields, this silently breaks at compile time only if types mismatch — otherwise it's a silent bug. Prefer named fields:
return &wallet{
WalletOptions: opts,
locker: newOutpointLocker(time.Minute),
keyPaths: newKeyPathCache(),
readyCh: make(chan bool),
}Not blocking, but worth fixing while you're here.
🟢 LOOKS GOOD
utxosFromTransactionEvent— pure function, well-scoped, correctTrackedSourcefiltering, proper error propagation. Themake([]ports.Utxo, 0, len(newTxEvent.Outputs))pre-allocation is a nice touch.event.Data→json.RawMessage— correct elimination of the redundant marshal→unmarshal roundtrip.matchedOutputtype — clean mapping of the NBXplorer event payload. Fields match the NBXplorer 2.6.7 schema.castUtxonow carriesKeyPath— correct one-line addition.getPrivateKeyFromScriptempty-KeyPath guard — this is a bug fix: the old code would pass""toderiveKeyon the first scheme match with empty KeyPath, likely erroring out and halting the loop before trying the connector scheme. Now it correctly falls through.- Test coverage — event parsing (group vs derivation-scheme vs newblock, single/multi/empty outputs, confirmations, malformed JSON, invalid txid, keyPath passthrough), cache (set/get/miss/overwrite/concurrent), signing path (locked wallet, cache hit, cache miss + backfill, connector fallback, empty KeyPath fallback, unknown script). Benchmark is well-structured.
⚠️ PROTOCOL-ADJACENT — Human review required
This PR modifies getPrivateKeyFromScript, which is on the signing path. The change is a caching layer — the underlying deriveKey call and its parameters are unchanged — so the risk of fund loss is minimal. However, per protocol-critical review rules:
A human must review and approve this PR before merge. Specifically, verify:
- The cache can never return a stale/wrong
(derivationScheme, keyPath)for a given script (it can't, because the mapping is immutable by derivation, but a human should confirm this assumption holds for all address types the wallet manages). - The empty-KeyPath guard in the fallback path is correct behavior (it is — NBXplorer returns empty KeyPath for scripts not derived from the queried scheme).
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Follow-up Review — new commit 923414a
Reviewer: Arkana (automated, aggressive)
Verdict: ✅ Approve (Arkana side)
Both issues from my previous review are resolved:
🔴 Unbounded cache → Fixed
sync.RWMutex + map[string]keyPathEntry replaced with hashicorp/golang-lru/v2 lru.Cache capped at 50,000 entries. The library is concurrency-safe internally, so the manual mutex removal is correct. Eviction of old entries is safe — a cache miss falls back to the NBXplorer HTTP path with zero correctness risk.
New TestKeyPathCache_Bounded test verifies the cap holds and oldest entries are evicted. Good.
🟡 Positional struct init → Fixed
wallet{} in New() now uses named fields. Clean.
No new issues in the incremental diff. The go.mod addition of hashicorp/golang-lru/v2 v2.0.7 is appropriate.
⚠️ Human review still required
This PR modifies getPrivateKeyFromScript (signing path). Per protocol-critical rules, a human must also approve before merge. The two things to verify remain unchanged from my first review:
- The
script → (derivationScheme, keyPath)mapping is truly immutable for all address types the wallet manages. - The empty-KeyPath guard correctly skips schemes that don't own the script.
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Follow-up Review — new commit 923414a
Reviewer: Arkana (automated, aggressive)
Verdict: ✅ Approve (Arkana side)
Both issues from my previous review are resolved:
🔴 Unbounded cache → Fixed
sync.RWMutex + map[string]keyPathEntry replaced with hashicorp/golang-lru/v2 lru.Cache capped at 50,000 entries. The library is concurrency-safe internally, so the manual mutex removal is correct. Eviction of old entries is safe — a cache miss falls back to the NBXplorer HTTP path with zero correctness risk.
New TestKeyPathCache_Bounded test verifies the cap holds and oldest entries are evicted. Good.
🟡 Positional struct init → Fixed
wallet{} in New() now uses named fields. Clean.
No new issues in the incremental diff. The go.mod addition of hashicorp/golang-lru/v2 v2.0.7 is appropriate.
⚠️ Human review still required
This PR modifies getPrivateKeyFromScript (signing path). Per protocol-critical rules, a human must also approve before merge. The two things to verify remain unchanged from my first review:
- The
script → (derivationScheme, keyPath)mapping is truly immutable for all address types the wallet manages. - The empty-KeyPath guard correctly skips schemes that don't own the script.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Catchup review — head 923414a
Re-verified the PR at the current head. My earlier concerns (unbounded cache, positional struct init) are resolved; the LRU cap + eviction test and named-field construction are in place. No new blockers on this pass — a few small residual notes only.
Confirmed correct
- Cache scoping.
cacheKeyPathsis invoked with the exact derivation scheme used for the enclosingNbxplorer.GetUtxoscall at every call site (service.go:344,397,842,851,999). Because the main and connector accounts are distinct BIP86 xpubs,script → (scheme, keyPath)is deterministic — no way to seed a wrong-key entry. - LRU + concurrency.
hashicorp/golang-lru/v2is safe for concurrent Add/Get. Cache is separate fromkeyMgr, so lock/unlock cycles do not corrupt it; on a Restore-to-a-different-seed, stale entries would failderiveKeywith "invalid xpub" rather than sign with a wrong key. - Small bug fix in the fallback. Previously
getPrivateKeyFromScript(service.go:1013) took the first non-error response fromGetScriptPubKeyDetailsand passed the (possibly empty)KeyPathstraight toderiveKey, which errors on""and aborted signing before ever trying the connector scheme. The newif scriptPubKeyDetails.KeyPath == "" { continue }correctly probes the other scheme and only returnsnil, nilwhen neither owns the script — surfacing the "not a wallet script" error at the SignTransaction level as intended.
Residual notes (non-blocking)
Confirmationstype widenedint → uint32innewTransactionEvent.TransactionData(types.go:141). Newtransaction events from NBXplorer are only sent for accepted mempool/confirmed txs (value ≥ 0), so this is fine in practice — but any future path that surfaces reorg-invalidated txs through this same struct (NBXplorer uses -1 elsewhere) would silently fail to unmarshal. Worth a mental note.matchedOutput.Value uint64(types.go:132) assumes the BTCMoneyserializer emits a bare integer, matchingutxoResponse.Value. Correct for BTC/NBXplorer 2.6.7. There is no in-tree integration test against a real NBXplorer instance that exercises the new field names (outputs,matchedOutput.*); the mocked-JSON unit tests are the sole shape check. A single e2e inarkade-regtestcovering the deposit-notification path would harden this against NBXplorer version drift.- Log-spam surface. The new
log.Errorf("failed to parse transaction event: %s", err)inGetAddressNotificationsfires on every unparseable outer frame with no rate limit. Previously errors were swallowed. If a broken upstream ever emits a stream of non-JSON frames this could flood logs. Minor. - Cross-repo impact.
ports.Utxolives inside thepkg/arkd-walletsubmodule and is not consumed by the external SDKs — safe.
Comment-only from me; not blocking merge.
|
This PR has been open for 3+ days without review. @bitcoin-coder-bob is anyone looking at this? |
|
This PR has been open for 30+ days without review. @bitcoin-coder-bob is anyone looking at this? |
|
This PR has been open for 5 days without review. @bitcoin-coder-bob is anyone looking at this? |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
This PR (NBXplorer UTXO efficiency) has been open 76 days without review. @bitcoin-coder-bob is anyone picking this up?
|
This PR has been open 80+ days without a formal review. @bitcoin-coder-bob — this is the bottom of the arkd-wallet stack; getting this merged would unblock the chain above it. |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana review — #1093 (sha 923414a)
arkd-wallet: source UTXOs from NBXplorer events and cache wallet key paths
Key path cache
- LRU cache (50k entries) for script → (derivation scheme, key path)
- Entries are immutable (same script always maps to the same key path), so there are no staleness concerns
- LRU eviction is safe: a miss falls back to the NBXplorer per-input lookup
set()correctly ignores empty script or key path entries- Good test coverage: basic set/get, miss, empty rejection, bounded eviction, concurrent access
Looks correct
- Cache is populated at
ListConnectorUtxoscall time, then hits during signing — right placement - The 50k entry bound keeps memory bounded even for a long-running operator wallet with heavy connector churn
- Concurrent test exercises the
hashicorp/golang-lru/v2thread-safety, which is the correct dependency
One note
Only saw keypath_cache.go and part of service.go in the diff — the NBXplorer event-sourcing aspect of the PR title isn't covered here. Confirm the event-based UTXO path doesn't introduce any ordering assumptions (e.g. that a UTXO event always arrives before the signing call that needs its key path).
Verdict: cache layer looks well-designed and tested. Performance-only change, no correctness concerns here.
|
This PR has been open for 12+ weeks without a review. @bitcoin-coder-bob — is anyone looking at this? Is it still waiting on other PRs in the stack, or does it need a reviewer assigned? |
…iency # Conflicts: # pkg/arkd-wallet/go.mod
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Follow-up Review — merge commit `e3dda22`
Status: 1 commit since `923414a` — a merge of `origin/master` into the branch. Both prior issues remain Fixed. Zero remaining actionable issues in the incremental diff.
What changed since `923414a`
Commit `e3dda22` is a pure merge commit (parents: `923414a` + `f863e48`). It brings 118 master commits into the branch. The only conflict was `pkg/arkd-wallet/go.mod`, resolved correctly by preserving the `github.com/hashicorp/golang-lru/v2 v2.0.7` entry the PR introduced. No `pkg/arkd-wallet/core/` source files changed in this incremental step.
The notable master commit included here is `f863e48` ("Migrate to btcd v0.26 and the v2 sub-modules", PR #1173). The PR's test imports — `github.com/btcsuite/btcd/chaincfg/v2` and `github.com/btcsuite/btcd/wire/v2` — are consistent with that migration, and CI shows no compile failures.
Prior-issue closure
| # | Finding | Status |
|---|---|---|
| 1 | 🔴 Unbounded cache growth (keyPathCache map leaked indefinitely) |
Fixed in `923414a` — LRU cap of 50 000 entries via hashicorp/golang-lru/v2. TestKeyPathCache_Bounded verifies the cap. |
| 2 | 🟡 Positional struct init in New() (wallet{}) |
Fixed in `923414a` — named fields used throughout. |
New findings (incremental diff)
go.mod conflict resolution — `pkg/arkd-wallet/go.mod` correctly keeps hashicorp/golang-lru/v2 v2.0.7 from the PR side and absorbs all master-side dependency bumps. No phantom or dropped dependency. No issue.
Nothing else in the incremental diff touches wallet logic. No new findings.
⚠️ Standing requirement — human review still required
This PR modifies `getPrivateKeyFromScript` (signing path). Arkana's approval covers the caching and event-parsing mechanics. A human must also approve before merge, confirming:
- The `script → (derivationScheme, keyPath)` mapping is immutable for all address types this wallet manages.
- The empty-KeyPath guard correctly skips schemes that don't own the queried script.
…vent Regroup only for the tests. The parse failure of a notification frame is benign for the wallet and was previously swallowed, so it logs at warn.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Follow-up Review — head `7138bb9`
Status: 1 commit since `e3dda22`; 1 prior residual note addressed; 2 non-blocking residual notes unchanged; 0 new findings.
What changed since `e3dda22`
Commit `7138bb9` ("arkd-wallet: group the key path cache tests, warn on an unparseable event") touches exactly two files:
-
pkg/arkd-wallet/core/application/wallet/keypath_cache_test.go—TestKeyPathCache_BoundedandTestKeyPathCache_Concurrentre-homed ast.Run("bounded", …)andt.Run("concurrent", …)subtests insideTestKeyPathCache. Pure organisational refactor; test logic is byte-for-byte identical. -
pkg/arkd-wallet/core/infrastructure/nbxplorer/service.go:702—log.Errorf("failed to parse transaction event: …")→log.Warnf(…). One-line change.
Prior-issue closure
| # | Finding | Status |
|---|---|---|
| 🔴 Unbounded cache growth | Fixed in 923414a — LRU cap 50 000 + TestKeyPathCache_Bounded. Not regressed. |
|
🟡 Positional struct init in New() |
Fixed in 923414a. Not regressed. |
|
| Log-spam on malformed frames (residual note #3, catchup review) | Fixed in 7138bb9 — demoted to Warnf. Directly addresses the concern. |
|
Confirmations int → uint32 widening in newTransactionEvent.TransactionData (residual note #1) |
Still open (non-blocking). Reorg-invalidated frames from a future NBXplorer version that emits -1 would silently fail to unmarshal. No change in this commit; awareness recorded. |
|
No in-tree integration test for outputs/matchedOutput.* field names against a live NBXplorer instance (residual note #2) |
Still open (non-blocking). Unit tests with mocked JSON remain the sole shape check; e2e coverage would harden against version drift. No change in this commit. |
New findings
None. The incremental diff is two cosmetic changes with no logic impact.
⚠️ Standing requirement — human review still required
This PR modifies getPrivateKeyFromScript (signing path). Arkana's approval covers the caching and event-parsing mechanics. A human must also approve before merge, confirming:
- The
script → (derivationScheme, keyPath)mapping is immutable for all address types this wallet manages. - The empty-
KeyPathguard correctly skips schemes that don't own the queried script.
2 participants
Summary
Two efficiency fixes to how
arkd-wallettalks to NBXplorer. Both eliminate HTTPcalls for data NBXplorer already hands us in payloads we were discarding.
1. Build deposit UTXOs from the
newtransactionevent payloadThe websocket notification handler previously called
searchNewUTXOson everynewtransactionevent, which didGET /v1/cryptos/BTC/groups/{groupID}/utxos(the entire group UTXO set) and filtered it client-side by txid. That work scales
linearly with the number of watched scripts and fires constantly.
NBXplorer 2.6.7's
newtransactionevent already contains the matched outputs. Wenow parse them directly via a pure
utxosFromTransactionEvent(message, groupID),scoped to
trackedSource == "GROUP:{groupID}"(the identifier NBXplorer reportsfor addresses added through the group addresses endpoint). No extra HTTP call, and
the cost no longer depends on group size.
event.Datais nowjson.RawMessagetodrop a redundant re-marshal;
ports.UtxogainedKeyPath, carried bycastUtxo.2. Cache
script -> keyPathto skip per-input lookups when signinggetPrivateKeyFromScriptused up to twoGetScriptPubKeyDetailsHTTP calls perinput. The
script -> {scheme, keyPath}mapping is immutable, and UTXO listingsalready return
keyPath(we were dropping it). A bounded LRU cache(50k entries) is populated from the wallet's account
GetUtxoscall sites andconsulted first; the HTTP path remains as a fallback that backfills the cache. A
cache miss is always safe (it just falls back), so eviction has zero correctness
risk and memory stays flat on a long-running server. Warm-cache signing makes zero
metadata calls.
Benchmarks
BenchmarkNotificationProcessing— new event-parse is constant regardless ofgroup size; the old fetch-and-filter scales linearly:
(allocations at 10k: 30,141 -> 27; bytes/op: 13.9 MB -> 1.6 KB.) #2 is covered by
call-count tests: cache hit = 0 lookups, connector fallback = 2.
Testing
go test -race ./core/...— all subtests pass.empty outputs, non-zero confirmations, malformed JSON, invalid txid, keyPath
passthrough.
oldest-first), 100-goroutine concurrency;
getPrivateKeyFromScriptcache-hit/fallback/empty-keyPath behavior.
golangci-lint(v2.9.0): no new issues vsmaster.Review feedback addressed
asserting the bound and LRU eviction. (Note: the cache is populated only from
the wallet's account UTXO listings, not from event notifications.)
walletis now constructed with named fields.Notes
schemes are ignored exactly as before. Malformed events are now logged (benign).
(the two accounts are distinct taproot xpubs), so a stale/wrong entry is not
possible.
/events?lastEventId, single-RPC block-time, the/balanceendpoint, andreserve=truefor change addresses.Summary by CodeRabbit
Improvements
Tests