arkd-wallet: source UTXOs from NBXplorer events and cache wallet key paths - #1093

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:

1. Parsing UTXO data directly from the newtransaction event payload instead of re-fetching the entire group UTXO set — eliminates an O(n) HTTP call per event.
2. 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, correct TrackedSource filtering, proper error propagation. The make([]ports.Utxo, 0, len(newTxEvent.Outputs)) pre-allocation is a nice touch.
• event.Data → json.RawMessage — correct elimination of the redundant marshal→unmarshal roundtrip.
• matchedOutput type — clean mapping of the NBXplorer event payload. Fields match the NBXplorer 2.6.7 schema.
• castUtxo now carries KeyPath — correct one-line addition.
• getPrivateKeyFromScript empty-KeyPath guard — this is a bug fix: the old code would pass "" to deriveKey on 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:

1. 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).
2. 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).

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:

1. The script → (derivationScheme, keyPath) mapping is truly immutable for all address types the wallet manages.
2. The empty-KeyPath guard correctly skips schemes that don't own the script.

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:

1. The script → (derivationScheme, keyPath) mapping is truly immutable for all address types the wallet manages.
2. The empty-KeyPath guard correctly skips schemes that don't own the script.

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. cacheKeyPaths is invoked with the exact derivation scheme used for the enclosing Nbxplorer.GetUtxos call 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/v2 is safe for concurrent Add/Get. Cache is separate from keyMgr, so lock/unlock cycles do not corrupt it; on a Restore-to-a-different-seed, stale entries would fail deriveKey with "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 from GetScriptPubKeyDetails and passed the (possibly empty) KeyPath straight to deriveKey, which errors on "" and aborted signing before ever trying the connector scheme. The new if scriptPubKeyDetails.KeyPath == "" { continue } correctly probes the other scheme and only returns nil, nil when neither owns the script — surfacing the "not a wallet script" error at the SignTransaction level as intended.

Residual notes (non-blocking)

1. Confirmations type widened int → uint32 in newTransactionEvent.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.
2. matchedOutput.Value uint64 (types.go:132) assumes the BTC Money serializer emits a bare integer, matching utxoResponse.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 in arkade-regtest covering the deposit-notification path would harden this against NBXplorer version drift.
3. Log-spam surface. The new log.Errorf("failed to parse transaction event: %s", err) in GetAddressNotifications fires 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.
4. Cross-repo impact. ports.Utxo lives inside the pkg/arkd-wallet submodule and is not consumed by the external SDKs — safe.

Comment-only from me; not blocking merge.

This PR (NBXplorer UTXO efficiency) has been open 76 days without review. @bitcoin-coder-bob is anyone picking this up?

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 ListConnectorUtxos call 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/v2 thread-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.

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

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:

1. The `script → (derivationScheme, keyPath)` mapping is immutable for all address types this wallet manages.
2. The empty-KeyPath guard correctly skips schemes that don't own the queried script.

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:

1. pkg/arkd-wallet/core/application/wallet/keypath_cache_test.go — TestKeyPathCache_Bounded and TestKeyPathCache_Concurrent re-homed as t.Run("bounded", …) and t.Run("concurrent", …) subtests inside TestKeyPathCache. Pure organisational refactor; test logic is byte-for-byte identical.

2. pkg/arkd-wallet/core/infrastructure/nbxplorer/service.go:702 — log.Errorf("failed to parse transaction event: …") → log.Warnf(…). One-line change.

Prior-issue closure

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:

1. The script → (derivationScheme, keyPath) mapping is immutable for all address types this wallet manages.
2. The empty-KeyPath guard correctly skips schemes that don't own the queried script.

Follow-up Review — head `167701c`

Status: 3 files changed since `7138bb9`; 0 prior issues newly resolved; 2 standing non-blocking residuals unchanged; 1 new cosmetic finding (test-only).

All three files in this increment are pure reorganisations — zero logic changes.

Prior-issue closure

Incremental diff

pkg/arkd-wallet/core/application/wallet/keypath_cache.go — one-word comment edit ("immutable: a given" → "immutable, since a given"). No logic impact.

pkg/arkd-wallet/core/application/wallet/service_test.go — fakeNbxplorer struct and newTestWallet helper moved from top of file to bottom. Pure reorganisation; logic and coverage identical.

pkg/arkd-wallet/core/infrastructure/nbxplorer/service_test.go — BenchmarkNotificationProcessing moved earlier in the file, with constants and helpers relocated alongside it. Pure reorganisation, but the move left two comment artefacts:

1. Lines 141–142: The comment that originally documented fetchAndFilterGroupUtxos ("// fetchAndFilterGroupUtxos replicates the previous (pre-optimization) behavior: // on every event it fetched the whole group UTXO set over HTTP and filtered it") is now truncated at two lines and immediately precedes BenchmarkNotificationProcessing, making it read as if it documents the benchmark. The third line ("It is kept here only as a benchmark baseline.") and the func fetchAndFilterGroupUtxos declaration were separated by the move.

2. Lines 280–282 (end of file): Three orphaned comment lines — "// BenchmarkNotificationProcessing compares the per-event work of the new / event-payload parsing against the old approach … the new one" — are stranded at the bottom of the file with no function following them. The function they described is now at line 144.

Neither artefact affects compilation or test behaviour (Go allows comment blocks not attached to declarations), but they are misleading to future readers. Worth a follow-up tidy in the same PR or as a quick post-merge cleanup.

New findings

No logic, protocol, or security findings. The two comment artefacts noted above are the only new observations; they are non-blocking.

⚠️ Standing requirement — human review still required

This PR modifies `getPrivateKeyFromScript` (signing path). Arkana's review covers the caching and event-parsing mechanics. A human must also approve before merge, confirming:

1. The `script → (derivationScheme, keyPath)` mapping is immutable for all address types this wallet manages.
2. The empty-KeyPath guard correctly skips schemes that do not own the queried script.

Follow-up Review — head `93108ae`

Status: 2 cosmetic-only commits since `167701c`; 1 prior finding fixed; 2 standing non-blocking residuals unchanged; 0 new findings.

Prior-issue closure

Incremental diff

Both commits are documentation-only. No logic, no data flow, no interfaces changed.

• `56f37e3` (pkg/arkd-wallet/core/infrastructure/nbxplorer/service_test.go) — restores two doc comments to their correct positions; see prior-issue row above.
• `93108ae` (pkg/arkd-wallet/core/application/wallet/keypath_cache.go) — punctuation fix; one colon → period, next sentence capitalised.

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:

1. The `script → (derivationScheme, keyPath)` mapping is immutable for all address types the wallet manages.
2. The empty-`KeyPath` guard correctly skips schemes that do not own the script, and the "not a wallet script" error surfaces correctly at the `SignTransaction` level.