Feat/electrum support#160
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new ElectrumX-backed explorer and multiplexed Electrum client, wires Electrum selection into SDK init/options (WithElectrumExplorer, optional Esplora package broadcast), updates LoadArkClient/init defaults to electrum regtest, and adds extensive Electrum tests, docs, README examples and test wiring. ChangesElectrum Explorer + SDK integration
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant SDK as SDK Client
participant Init as Explorer Factory
participant ElecSvc as ElectrumX Explorer
participant Server as ElectrumX Server
participant Cache as Local Cache
end
SDK->>Init: newExplorer(url, network, opts)
Init->>Init: detect scheme (tcp:// or ssl://)
Init->>ElecSvc: NewExplorer(url,...)
SDK->>ElecSvc: Start()
ElecSvc->>Server: TCP/TLS connect & server.version
Server-->>ElecSvc: version response
ElecSvc->>ElecSvc: start listener & keepAlive
SDK->>ElecSvc: SubscribeForAddresses(addrs)
ElecSvc->>Server: blockchain.scripthash.subscribe
Server-->>ElecSvc: initial status
Server->>ElecSvc: push notification
ElecSvc->>ElecSvc: pollAddress() -> listunspent
ElecSvc->>Cache: update cache
ElecSvc->>SDK: OnchainAddressEvent
sequenceDiagram
rect rgba(200,255,200,0.5)
participant Client as ElectrumX Client
participant Conn as TCP/TLS Conn
participant Reader as JSON Reader
participant Handler as Response Handler
end
Client->>Conn: dial (tcp/ssl)
Client->>Conn: send server.version
loop read loop
Reader->>Conn: read newline JSON frame
Conn-->>Reader: frame
Reader->>Handler: parse JSON
alt response with id
Handler->>Handler: dispatch to pending request channel
else notification
Handler->>Handler: deliver to subscriber listeners (non-blocking)
end
end
par keepalive
Client->>Conn: server.ping (periodic)
and reconnect
Client->>Client: reconnect/backoff, replay subscriptions (bounded parallelism)
end
sequenceDiagram
rect rgba(255,200,200,0.5)
participant Explorer as ElectrumX Explorer
participant Cache as TX Cache
participant Server as ElectrumX Server
participant Esplora as Esplora REST
end
Explorer->>Cache: GetTxHex(txid) -> cache hit?
alt cache hit
Cache-->>Explorer: return tx hex
else cache miss
Explorer->>Server: blockchain.transaction.get(txid, verbose=true)
Server-->>Explorer: tx hex
Explorer->>Cache: cache tx hex
Explorer-->>Caller: return tx hex
end
Caller->>Explorer: Broadcast(txHexs...)
alt multiple txs + esplora configured
Explorer->>Esplora: POST /txs/package
Esplora-->>Explorer: package response (txids)
else sequential broadcast
Explorer->>Server: blockchain.transaction.broadcast (per tx)
Server-->>Explorer: txid or error
end
Explorer->>Cache: cache broadcast tx hex on success
Explorer-->>Caller: txid / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 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 |
|
@arkanaai @coderabbitai review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
Arkana Review — ElectrumX ExplorerSolid piece of work. The self-contained TCP client, the 🔴 Bug:
|
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
Electrum Explorer Review — #160
Solid piece of work overall. The test coverage is thorough, the parity bugs caught during the audit are real, and the mock-server test harness is well designed. That said, I found several issues that need to be fixed before merge — two are concurrency bugs that will bite in production.
🔴 CRITICAL — Goroutine leak on reconnect
explorer/electrum/client.go:246-253
When reconnect() succeeds it spawns fresh go c.listen() and go c.keepAlive() goroutines, but it never cancels or signals the old keepAlive() goroutine. The old listen() is the caller of reconnect(), so it exits on return — fine. But keepAlive() is a separate long-running goroutine that uses c.request() → c.getConn(), so after reconnect it happily pings on the new connection. After N reconnects you have N+1 keepAlive goroutines all firing pings on the same connection.
Fix: give each connect/reconnect cycle its own cancellable context (child of c.ctx), and cancel the previous cycle's context before starting new goroutines.
// sketch
c.cycleMu.Lock()
if c.cycleCancel != nil {
c.cycleCancel()
}
cycleCtx, cycleCancel := context.WithCancel(c.ctx)
c.cycleCancel = cycleCancel
c.cycleMu.Unlock()
go c.listen(cycleCtx)
go c.keepAlive(cycleCtx)🔴 CRITICAL — Concurrent writes to TCP connection under RLock
explorer/electrum/client.go:293-299
conn := c.getConn() // RLock
// ...
c.connMu.RLock()
_, err = conn.Write(data) // multiple goroutines can reach here concurrently
c.connMu.RUnlock()connMu is an RWMutex, and request() takes RLock for the write. Since multiple goroutines can hold RLock simultaneously, concurrent conn.Write() calls can interleave bytes on the wire when the JSON-RPC message exceeds the kernel's atomic write threshold. JSON-RPC framing will be corrupted.
Fix: use a dedicated writeMu sync.Mutex for serializing writes to the connection, separate from connMu (which protects the conn pointer).
🟡 HIGH — Start() silently swallows connection failure
explorer/electrum/explorer.go:108-112
func (e *explorerSvc) Start() {
if err := e.client.connect(); err != nil {
log.WithError(err).Error("electrum explorer: failed to connect")
return // caller has no idea
}If the ElectrumX server is unreachable, Start() logs and returns. Every subsequent RPC call will hit "not connected" — but the caller (e.g. Unlock() in client.go:108) has no indication the explorer is broken. For an L2 wallet, silently running with a dead explorer means the user thinks they're online but UTXOs, broadcasts, and event tracking are all non-functional.
Options: (a) return an error from Start() (breaking interface change), or (b) set a flag and surface it via GetConnectionCount() == 0, or (c) retry connection in background with backoff (like reconnect does).
🟡 HIGH — Electrum explorer ignores regtest poll interval
init.go:55-66
var pollInterval time.Duration
if info.Network == arklib.BitcoinRegTest.Name {
pollInterval = 2 * time.Second
}
explorerSvc, err := newExplorer(explorerUrl, networkFromString(info.Network), true, pollInterval)Then in newExplorer() at line 168-171:
if strings.HasPrefix(url, "tcp://") || strings.HasPrefix(url, "ssl://") {
return electrum_explorer.NewExplorer(
url, net, electrum_explorer.WithTracker(tracker),
) // pollInterval is silently dropped!
}The 2-second regtest poll interval is forwarded to mempool but never to electrum. Electrum defaults to 10 seconds. On regtest, this means 5× slower UTXO detection when using the electrum backend — the opposite of what operators expect.
Fix: pass electrum_explorer.WithPollInterval(pollInterval) when pollInterval > 0.
🟡 HIGH — Broadcast caches tx hex before confirming success
explorer/electrum/explorer.go:210-212
e.cacheMu.Lock()
e.cache[txid] = txHex
e.cacheMu.Unlock()
// ... then broadcast, which might failThe tx hex is cached before the broadcast RPC. If broadcast fails (e.g. invalid tx, fee too low), the cache still holds the entry. A subsequent GetTxHex(txid) will return the hex of a transaction that was never actually broadcast. Downstream code that checks GetTxHex to determine if a tx exists will get false positives.
Fix: cache only after successful broadcast, or clear cache on broadcast failure.
🟡 HIGH — Unbounded tx cache (memory leak)
explorer/electrum/explorer.go:62
cache map[string]string // txid → hexThis cache grows without bound. A long-running SDK instance processing boarding, unrolls, and restores will accumulate every tx hex it has ever seen. A typical Ark tx is ~500 bytes hex, so 100k txs ≈ 50MB. There's no eviction policy.
Fix: use an LRU cache with a bounded size, or periodically prune entries older than some threshold.
🟠 MEDIUM — Inconsistent unconfirmed BlockTime sentinel values
GetTxBlockTime returns (false, -1, nil) for unconfirmed txs (explorer.go:389), but GetTxs returns BlockTime: 0 for unconfirmed txs (explorer.go:269 — blocktime stays 0 when not confirmed). The test at line 1306 of the test file confirms this intentional mismatch.
The consumer code in client.go uses CreatedAt.IsZero() to detect unconfirmed, so this probably doesn't cause bugs today, but it's a behavioral inconsistency that differs from what the PR description promises ("Status.BlockTime=0 (not -1) for unconfirmed").
🟠 MEDIUM — No option for custom TLS certificates
explorer/electrum/client.go:84-87
return tls.DialWithDialer(
&net.Dialer{Timeout: 10 * time.Second},
"tcp", addr, &tls.Config{MinVersion: tls.VersionTLS12},
)Self-hosted ElectrumX servers commonly use self-signed certificates. This will fail with x509: certificate signed by unknown authority. Consider adding a WithTLSConfig(*tls.Config) option, or at minimum documenting this limitation.
🟢 LOW — No scanner buffer limit
explorer/electrum/client.go:127-128
scanner := bufio.NewScanner(conn)Default bufio.Scanner max token size is 64KB. A malicious or buggy ElectrumX server sending a line >64KB will cause scanner.Scan() to return false with bufio.ErrTooLong, triggering a reconnect loop. For robustness, consider scanner.Buffer(buf, maxSize) with a reasonable limit (e.g. 1MB for large verbose txs).
🟢 LOW — subsMu2 naming
explorer/electrum/client.go:43-44
storedSubs []string
subsMu2 sync.Mutex // protects storedSubssubsMu2 is a code smell. If storedSubs and subs have different lifecycles that require separate locks, rename to something descriptive like storedSubsMu.
✅ What looks good
math.Roundfor BTC→sat conversion — correct fix for float truncationscriptToAddressfor derivingOutput.Address— fixes the silent UTXO drop inaddressTxHistoryToUtxosWithTracker(true/false)logic is now correctnetworkToChainParamscovers regtest and mutinynet- Test coverage is thorough: mock server approach exercises the full stack
diffUTXOslogic is clean and correctparseBitcoinTxhandles witness/non-witness/PSBT fallback chain- Cross-repo impact: no breaking changes to the
explorer.Explorerinterface or public API — this is purely additive
Cross-repo impact assessment
- ✅
explorer.Explorerinterface unchanged — no downstream breakage - ✅
WithExplorerURLstill works,WithElectrumExploreris additive - ✅
LoadArkClientauto-detectstcp:///ssl://prefix — backward compatible - ✅
introspector-reviewtest suite (primary consumer) unaffected
Bottom line: Fix the goroutine leak and the concurrent write race. Those are production bugs. The poll interval and cache issues are important but less urgent. Everything else is cleanup.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
explorer/electrum/explorer.go (3)
108-122: ⚖️ Poor tradeoffConsider returning error from Start() to indicate connection failure.
If
connect()fails,Start()logs the error and returns without any indication to the caller. This makes it impossible for callers to handle connection failures gracefully (e.g., retry with different URL, fail fast).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@explorer/electrum/explorer.go` around lines 108 - 122, Change explorerSvc.Start from a void method to return an error so callers can handle connection failures: have Start call e.client.connect() and if it returns an error, return that error instead of only logging it; keep existing behavior for e.noTracking/e.stopTracking (return nil when not starting tracking), continue to initialize stopCh, set e.stopTracking with sync.OnceFunc and start go e.pollLoop(stopCh) on success, and update callers of Start to handle the returned error accordingly (propagate/retry/fail fast as needed).
529-533: 💤 Low valueIgnored error from addrToScript could cause empty script in events.
If
addrToScriptfails at line 529,scriptwill be an empty string, and all UTXOs in the diff event will have emptyScriptfields. This could cause issues for callers that rely on the script.Consider logging the error
- script, _ := addrToScript(addr, e.netParams) + script, err := addrToScript(addr, e.netParams) + if err != nil { + log.WithError(err).Warnf("electrum: failed to convert address to script: %s", addr) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@explorer/electrum/explorer.go` around lines 529 - 533, The call to addrToScript in explorer.go ignores its error and can produce an empty script in the diff event; change the code to capture the error (script, err := addrToScript(...)), and if err != nil log or return early instead of proceeding to diffUTXOs so you don't emit events with empty Script fields — update the handling around addrToScript, diffUTXOs, and the surrounding function (references: addrToScript, diffUTXOs, variables script/newUTXOs/state.utxos) to either propagate the error or skip emitting the event after logging the failure.
283-298: 💤 Low valueSilent error handling may mask issues in spend detection.
Errors at lines 284-290 and 292-298 are silently ignored with
continue. This could causeGetTxOutspendsto returnSpent: falsefor outputs that are actually spent if there's a transient RPC error or malformed data. Consider logging errors at debug level for troubleshooting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@explorer/electrum/explorer.go` around lines 283 - 298, The loop in GetTxOutspends silently skips errors from scriptToScripthash, e.client.request("blockchain.scripthash.get_history") and json.Unmarshal which can hide transient RPC or parse failures; update the error handling in the loop around scriptToScripthash(out.ScriptPubKey.Hex), the request call, and the json.Unmarshal into electrumHistoryEntry to emit debug-level logs (including error, txid/vtx.Txid or current tx id, vout index i, and the script hex) before continuing so failures are visible during troubleshooting while preserving the continue behavior.explorer/electrum/client.go (1)
246-254: ⚖️ Poor tradeoffMultiple listen/keepAlive goroutines may run concurrently during reconnect.
When
reconnect()is called (e.g., from the oldlisten()goroutine after connection loss), it starts newlisten()andkeepAlive()goroutines at lines 252-253. However, if the oldkeepAlive()goroutine hasn't exited yet (its next ping hasn't failed), it may continue running until its next tick. This could cause duplicate pings or unexpected behavior.Consider adding a mechanism to signal the old goroutines to exit before starting new ones, such as using a per-connection context that gets cancelled on reconnect.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@explorer/electrum/client.go` around lines 246 - 254, The reconnect path can spawn duplicate listen/keepAlive goroutines; introduce a per-connection context on the client struct (e.g., c.connCtx and cancelConnCtx) that you cancel before replacing the connection in setConn(conn) or before launching new goroutines in reconnect(), then pass that context into listen() and keepAlive() so those functions return when ctx.Done() is signalled; ensure reconnect() cancels the previous context, sets up a fresh context, calls c.setConn(conn)/c.handshake(), and only then starts go c.listen(ctx) and go c.keepAlive(ctx).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@explorer/electrum/client.go`:
- Around line 328-338: The scripthash is appended to c.storedSubs before calling
c.request, but on request error the entry is removed from c.subs only, leaving a
failed scripthash in c.storedSubs to be replayed on reconnect; update the error
path in the subscribe flow (the method that calls
c.request("blockchain.scripthash.subscribe", ...)) to also remove the scripthash
from c.storedSubs (under the c.subsMu2 lock) when c.request returns an error,
ensuring both maps/slices are cleaned consistently.
- Around line 293-302: The current code calls c.getConn(), then uses
c.connMu.RLock() around conn.Write(data), which permits concurrent writers and
can interleave JSON-RPC frames; change this to serialize writes by using a full
write lock: either replace c.connMu.RLock()/RUnlock() with
c.connMu.Lock()/Unlock() around conn.Write(data) or introduce a dedicated write
mutex (e.g., c.writeMu) and Lock/Unlock it around conn.Write(data) so all writes
to the connection (in the send function that uses conn and connMu) are
serialized and cannot interleave.
- Around line 147-153: The send to the per-request channel can panic if another
goroutine closes it after you release c.pendMu; change the lookup/send logic in
the response loop to atomically remove the channel from c.pending while holding
c.pendMu (e.g., lock, ch, ok := c.pending[resp.ID]; if ok { delete(c.pending,
resp.ID) }; unlock) and then perform a guarded send (use a non-blocking select
or a done check) so a closed channel or racing close won't panic; alternatively,
stop closing individual channels in the close() method (only delete entries) and
let request timeouts clean up to avoid the race—apply this change around the
code referencing c.pendMu, c.pending, resp.ID, ch and the close()
implementation.
In `@explorer/electrum/explorer_test.go`:
- Around line 89-114: The two sub-tests in TestAddressToScripthash are
placeholders with no assertions; either remove the test entirely (if coverage
exists via GetUtxos) or export a test helper that calls the internal
addressToScripthash and assert its behavior: add an exported
AddressToScripthashForTest that wraps addressToScripthash, then in
TestAddressToScripthash call AddressToScripthashForTest(addr,
&chaincfg.MainNetParams) and assert no error, length == 64 and lowercase hex
regexp, and also assert addr != addr2 produce different scripthashes to validate
determinism and uniqueness.
In `@explorer/electrum/explorer.go`:
- Around line 423-429: The TOCTOU bug in SubscribeForAddresses arises because
SubscribeForAddresses currently uses e.subscribedMu.RLock() to check
e.subscribedMap[addr] then releases before adding, allowing concurrent calls to
create duplicates; fix by ensuring the check-and-add is atomic: acquire
e.subscribedMu.Lock() (not RLock) around the check and insertion into
e.subscribedMap in SubscribeForAddresses (or use the pattern of preparing any
RPC work outside the lock and then re-acquiring e.subscribedMu.Lock() to
re-check and insert to avoid holding the lock during RPCs), ensuring you never
perform RPC calls while holding the lock and referencing the methods/fields
e.subscribedMu, subscribedMap, and SubscribeForAddresses.
In `@explorer/electrum/utils.go`:
- Around line 131-133: The async call to l.remove(from broadcast) can
double-close listener channels when clear closes them first; update remove to
avoid closing an already-closed channel (or make close idempotent) by adding a
per-listener closed flag or removing-and-nil-ing the channel before closing, and
use that same guard when clear and any teardown path close channels; modify the
remove, clear and any goroutine that calls remove (e.g., broadcast) to consult
that guard (or atomically swap out and close only the previously-held channel)
so close of closed channel panics cannot occur.
- Around line 78-85: The PSBT fallback branch currently discards the PSBT
parse/extract errors by returning the earlier deserialize error variable `err`;
update the error handling in the `psbt.NewFromRawBytes` and `psbt.Extract`
blocks to return the actual error variable `err3` (or rename to a clearer name)
so the function returns the real PSBT parsing/extraction error when `ptx, err3
:= psbt.NewFromRawBytes(strings.NewReader(txStr), true)` or `txFromPartial, err3
:= psbt.Extract(ptx)` fail; ensure both error returns use the PSBT error
variable instead of `err`.
In `@init.go`:
- Around line 165-171: newExplorer currently ignores the pollInterval parameter
for the Electrum path; update the electrum branch so the
electrum_explorer.NewExplorer call includes a poll interval option (e.g.,
electrum_explorer.WithPollInterval(pollInterval)) in addition to
electrum_explorer.WithTracker(tracker), so that newExplorer(url, net, tracker,
pollInterval) actually applies the polling setting for tcp:// and ssl:// URLs;
modify the electrum_explorer.NewExplorer invocation in the newExplorer function
(and the adjacent electrum branch lines around the existing
electrum_explorer.WithTracker call) to pass the pollInterval option.
---
Nitpick comments:
In `@explorer/electrum/client.go`:
- Around line 246-254: The reconnect path can spawn duplicate listen/keepAlive
goroutines; introduce a per-connection context on the client struct (e.g.,
c.connCtx and cancelConnCtx) that you cancel before replacing the connection in
setConn(conn) or before launching new goroutines in reconnect(), then pass that
context into listen() and keepAlive() so those functions return when ctx.Done()
is signalled; ensure reconnect() cancels the previous context, sets up a fresh
context, calls c.setConn(conn)/c.handshake(), and only then starts go
c.listen(ctx) and go c.keepAlive(ctx).
In `@explorer/electrum/explorer.go`:
- Around line 108-122: Change explorerSvc.Start from a void method to return an
error so callers can handle connection failures: have Start call
e.client.connect() and if it returns an error, return that error instead of only
logging it; keep existing behavior for e.noTracking/e.stopTracking (return nil
when not starting tracking), continue to initialize stopCh, set e.stopTracking
with sync.OnceFunc and start go e.pollLoop(stopCh) on success, and update
callers of Start to handle the returned error accordingly (propagate/retry/fail
fast as needed).
- Around line 529-533: The call to addrToScript in explorer.go ignores its error
and can produce an empty script in the diff event; change the code to capture
the error (script, err := addrToScript(...)), and if err != nil log or return
early instead of proceeding to diffUTXOs so you don't emit events with empty
Script fields — update the handling around addrToScript, diffUTXOs, and the
surrounding function (references: addrToScript, diffUTXOs, variables
script/newUTXOs/state.utxos) to either propagate the error or skip emitting the
event after logging the failure.
- Around line 283-298: The loop in GetTxOutspends silently skips errors from
scriptToScripthash, e.client.request("blockchain.scripthash.get_history") and
json.Unmarshal which can hide transient RPC or parse failures; update the error
handling in the loop around scriptToScripthash(out.ScriptPubKey.Hex), the
request call, and the json.Unmarshal into electrumHistoryEntry to emit
debug-level logs (including error, txid/vtx.Txid or current tx id, vout index i,
and the script hex) before continuing so failures are visible during
troubleshooting while preserving the continue behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 648fc3ff-1e45-4b7a-bb9b-c62dcc914e89
📒 Files selected for processing (11)
README.mdclient.goexplorer/electrum/client.goexplorer/electrum/explorer.goexplorer/electrum/explorer_test.goexplorer/electrum/opts.goexplorer/electrum/types.goexplorer/electrum/utils.goinit.goinit_opts.goinit_opts_test.go
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@explorer/electrum/client.go`:
- Around line 141-159: The close() implementation currently calls c.cancel()
which permanently cancels the client's root context and breaks subsequent
Start()/Stop() cycles; remove the call to c.cancel() and instead cancel only the
per-cycle context (use c.cycleMu and c.cycleCancel) and any connection-specific
context so that c.ctx remains usable for future request()/connect()/reconnect()
calls; ensure close() still closes the active net.Conn from getConn(), cancels
and nils c.cycleCancel, and drains/cleans c.pending exactly as before so the
client instance can be restarted without a canceled root context.
- Around line 215-223: The read lock is released before sending into the
channel, allowing unsubscribeLocal() to close ch under subsMu.Lock() and cause a
panic; keep c.subsMu.RLock() held for the duration of the non-blocking send so
the channel cannot be closed concurrently. Concretely, in the block that reads
c.subs[scripthash] (use symbols subsMu, subs, ch, unsubscribeLocal), move the
c.subsMu.RUnlock() to after the select that attempts to send status (or
otherwise perform the send while still holding the RLock), preserving the
non-blocking select path and then release the RLock once the send attempt
completes. Ensure no additional blocking send is introduced.
In `@explorer/electrum/explorer.go`:
- Around line 130-147: Stop() currently only clears maps and doesn't signal
per-address subscription goroutines spawned by SubscribeForAddresses (the
goroutines that range over notifCh), causing goroutine leaks; modify Stop() to
iterate over active subscriptions (use subscribedMap and/or scripthashToAddr),
close each subscription's notifCh or send a shutdown signal and/or call the
corresponding client unsubscribe method to terminate the goroutine, then
wait/ensure those goroutines have exited (e.g., by removing entries only after
closing channels or using a waitgroup), and finally clear listeners and reset
maps under the existing subscribedMu and scripthashToAddrMu to avoid races.
- Around line 543-624: pollAddress can run concurrently for the same addr and
double-diff/broadcast because it reads state.utxos then later writes; fix by
serializing per-address: add a per-subscription mutex to the subscription state
type (e.g., add mu sync.Mutex to the struct stored in subscribedMap and make
subscribedMap hold pointers, e.g., map[string]*subscriptionState), then in
pollAddress after loading state from subscribedMap lock state.mu (defer unlock)
before reading state.utxos, computing diffUTXOs, and updating state.utxos; this
ensures only one poll/notification path mutates and diffs a given address at a
time while keeping other addresses concurrent.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 34876ebd-7540-48c2-8f2f-3ccd66b5f2c8
📒 Files selected for processing (6)
explorer/electrum/client.goexplorer/electrum/explorer.goexplorer/electrum/explorer_test.goexplorer/electrum/opts.goexplorer/electrum/utils.goinit.go
🚧 Files skipped from review as they are similar to previous changes (1)
- explorer/electrum/opts.go
There was a problem hiding this comment.
Arkana Re-Review — New Commits (154dd4c, ae3b4d0, bc44bb4)
All critical and high issues from my previous review have been addressed. The fixes are well-structured with matching test coverage. Approving from the code side — still needs a human sign-off since this touches protocol-adjacent infrastructure (explorer backend for UTXO tracking).
✅ Issues Fixed
| Previous Finding | Status |
|---|---|
| 🔴 Goroutine leak on reconnect | Fixed — cycle-context pattern (newCycleCtx/cycleCancel) cancels old keepAlive before starting new pair. TestKeepaliveGoroutineDoesNotLeak validates. |
| 🔴 Concurrent writes under RLock | Fixed — dedicated writeMu sync.Mutex serializes conn.Write(). TestConcurrentRequestsDoNotInterleave validates with 20 concurrent workers. |
| 🔴 Clean EOF doesn't trigger reconnect | Fixed — listen() now reconnects on any scanner exit (nil error or not), gated by cycle context. TestCleanEOFTriggersReconnect validates. |
🟡 Start() swallows connection failure |
Fixed — falls through to reconnect() in background goroutine with exponential backoff. |
| 🟡 Regtest poll interval not forwarded | Fixed in init.go:168-173 — WithPollInterval passed to electrum path. |
| 🟡 Broadcast caches before success | Fixed — setCacheTx called only after successful broadcast or already-in-chain. |
| 🟡 Unbounded tx cache | Fixed — txCacheMaxSize = 1024 with random eviction in setCacheTx. |
🟠 subsMu2 naming |
Fixed — renamed to storedSubsMu with comment explaining the separation. |
| 🟠 No custom TLS config | Fixed — WithTLSConfig(*tls.Config) option added in opts.go. |
| 🟢 No scanner buffer limit | Fixed — maxScannerBytes = 1 << 20 (1 MB). |
✅ Additional Improvements (Not Previously Flagged)
close()vsshutdown()separation (client.go:141-167): Clean split —close()tears down the current cycle without killing the root context (used during reconnect),shutdown()is terminal. Correct design.listen()notification send under RLock (client.go:223-231):subsMu.RUnlock()moved after the channel send, preventing a race withunsubscribeLocalclosing the channel. Good catch.listen()deletes pending entry before unlock (client.go:197-205): Preventsclose()from closing a channel thatlisten()is about to send on. Correct.- Per-address mutex in
pollAddress(explorer.go:569-570):state.muserializes concurrent polls for the same address, preventing duplicate events. Correct. SubscribeForAddressesdouble-check (explorer.go:481-487): Re-checks under write lock to handle concurrent subscriptions. Properly unsubscribes on conflict.notifWginStop()(explorer.go:151): Ensures all per-address notification goroutines exit before Stop returns. No leaked goroutines.subscribe()rollback on failure (client.go:416-424): Cleans up bothsubsandstoredSubsif the RPC fails.parseBitcoinTxerror propagation (utils.go:79,83): Now returnserr3instead of the staleerrfrom the first deserialization attempt.listeners.removenil check (utils.go:145-147): Prevents double-close panic.
🟡 Minor — Non-Blocking
-
isConnected()stale aftershutdown()—close()callsconn.Close()but doesn't nil outc.conn(client.go:151-153). AfterStop(),GetConnectionCount()returns 1 instead of 0. Considerc.setConn(nil)inclose(). -
Failed resubscription on reconnect is warn-only (
client.go:340-341) — Ifblockchain.scripthash.subscribefails during reconnect, the address silently loses push notifications until the next reconnect. The poll loop still runs so this isn't catastrophic, but the address falls back to poll-interval-only detection. Worth a TODO for retry logic. -
GetTxBlockTimevsGetTxsunconfirmed sentinel — Still returns-1vs0respectively. Documented in tests, not a functional bug today, but an API consistency wart.
LGTM. Ship it after human sign-off.
There was a problem hiding this comment.
Arkana Re-Review — New Commits (154dd4c, ae3b4d0, bc44bb4)
All critical and high issues from my previous review have been addressed. The fixes are well-structured with matching test coverage. Approving from the code side — still needs a human sign-off since this touches protocol-adjacent infrastructure (explorer backend for UTXO tracking).
✅ Issues Fixed
| Previous Finding | Status |
|---|---|
| 🔴 Goroutine leak on reconnect | Fixed — cycle-context pattern (newCycleCtx/cycleCancel) cancels old keepAlive before starting new pair. TestKeepaliveGoroutineDoesNotLeak validates. |
| 🔴 Concurrent writes under RLock | Fixed — dedicated writeMu sync.Mutex serializes conn.Write(). TestConcurrentRequestsDoNotInterleave validates with 20 concurrent workers. |
| 🔴 Clean EOF doesn't trigger reconnect | Fixed — listen() now reconnects on any scanner exit (nil error or not), gated by cycle context. TestCleanEOFTriggersReconnect validates. |
🟡 Start() swallows connection failure |
Fixed — falls through to reconnect() in background goroutine with exponential backoff. |
| 🟡 Regtest poll interval not forwarded | Fixed in init.go:168-173 — WithPollInterval passed to electrum path. |
| 🟡 Broadcast caches before success | Fixed — setCacheTx called only after successful broadcast or already-in-chain. |
| 🟡 Unbounded tx cache | Fixed — txCacheMaxSize = 1024 with random eviction in setCacheTx. |
🟠 subsMu2 naming |
Fixed — renamed to storedSubsMu with comment explaining the separation. |
| 🟠 No custom TLS config | Fixed — WithTLSConfig(*tls.Config) option added in opts.go. |
| 🟢 No scanner buffer limit | Fixed — maxScannerBytes = 1 << 20 (1 MB). |
✅ Additional Improvements (Not Previously Flagged)
close()vsshutdown()separation (client.go:141-167): Clean split —close()tears down the current cycle without killing the root context (used during reconnect),shutdown()is terminal. Correct design.listen()notification send under RLock (client.go:223-231):subsMu.RUnlock()moved after the channel send, preventing a race withunsubscribeLocalclosing the channel. Good catch.listen()deletes pending entry before unlock (client.go:197-205): Preventsclose()from closing a channel thatlisten()is about to send on. Correct.- Per-address mutex in
pollAddress(explorer.go:569-570):state.muserializes concurrent polls for the same address, preventing duplicate events. Correct. SubscribeForAddressesdouble-check (explorer.go:481-487): Re-checks under write lock to handle concurrent subscriptions. Properly unsubscribes on conflict.notifWginStop()(explorer.go:151): Ensures all per-address notification goroutines exit before Stop returns. No leaked goroutines.subscribe()rollback on failure (client.go:416-424): Cleans up bothsubsandstoredSubsif the RPC fails.parseBitcoinTxerror propagation (utils.go:79,83): Now returnserr3instead of the staleerrfrom the first deserialization attempt.listeners.removenil check (utils.go:145-147): Prevents double-close panic.
🟡 Minor — Non-Blocking
-
isConnected()stale aftershutdown()—close()callsconn.Close()but doesn't nil outc.conn(client.go:151-153). AfterStop(),GetConnectionCount()returns 1 instead of 0. Considerc.setConn(nil)inclose(). -
Failed resubscription on reconnect is warn-only (
client.go:340-341) — Ifblockchain.scripthash.subscribefails during reconnect, the address silently loses push notifications until the next reconnect. The poll loop still runs so this isn't catastrophic, but the address falls back to poll-interval-only detection. Worth a TODO for retry logic. -
GetTxBlockTimevsGetTxsunconfirmed sentinel — Still returns-1vs0respectively. Documented in tests, not a functional bug today, but an API consistency wart.
LGTM. Ship it after human sign-off.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Also @bitcoin-coder-bob we should replace mempool explorer and use electrum one in all of our test cases. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/hd_wallet_test.go (1)
539-580:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkip the test or confirm boarding-only key discovery is working.
The test
TestHDWalletRecoversBoardingOnlyFundedKeyswill fail CI. The comment correctly identifies thatdiscoverHDWalletKeysonly queries the indexer for offchain VTXO activity (viaderiveOffchainScriptsBatchandqueryUsedScripts); it does not check boarding addresses. Keys with only boarding UTXO activity are never discovered and never allocated viaw.NewKey(), so they won't appear ingetAllocatedAddresses()during wallet restore. The restored wallet's boarding address tracking is only set up vialistenForOnchainTxs, which registers only addresses for already-allocated keys. Consequently,require.Eventuallywill timeout waiting for the boarding UTXO in the balance.Either skip this test with
t.Skip()and a link to the tracking issue, or confirm that the Electrum explorer changes inadvertently enable boarding-only key discovery before merging.🤖 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 `@test/e2e/hd_wallet_test.go` around lines 539 - 580, The test TestHDWalletRecoversBoardingOnlyFundedKeys is expected to fail because discoverHDWalletKeys (via deriveOffchainScriptsBatch and queryUsedScripts) does not detect boarding-only UTXOs and keys are only allocated via w.NewKey(), so the restored wallet never registers the boarding address (listenForOnchainTxs only tracks already-allocated keys); either skip this flaky test by adding t.Skip("boarding-only key discovery not supported; see <tracking-issue-link>") at the top of TestHDWalletRecoversBoardingOnlyFundedKeys, or alternatively validate that the Electrum explorer/indexer changes actually cause discoverHDWalletKeys to detect boarding-only addresses before merging (i.e., add a note in the test asserting the explorer behavior or remove the skip once discoverHDWalletKeys/getAllocatedAddresses are updated).
🤖 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 `@Makefile`:
- Line 61: The Makefile default ARK_ELECTRUM_URL is set to tcp://127.0.0.1:50000
which mismatches the test and SDK defaults and causes integration tests to
target the wrong Electrum port; update the Makefile command that sets
ARK_ELECTRUM_URL (the line with
"@ARK_ELECTRUM_URL=$${ARK_ELECTRUM_URL:-tcp://127.0.0.1:50000} ...") to use
tcp://127.0.0.1:50001 so it matches test/e2e/utils_test.go and init.go and the
common nigiri/electrs port.
---
Outside diff comments:
In `@test/e2e/hd_wallet_test.go`:
- Around line 539-580: The test TestHDWalletRecoversBoardingOnlyFundedKeys is
expected to fail because discoverHDWalletKeys (via deriveOffchainScriptsBatch
and queryUsedScripts) does not detect boarding-only UTXOs and keys are only
allocated via w.NewKey(), so the restored wallet never registers the boarding
address (listenForOnchainTxs only tracks already-allocated keys); either skip
this flaky test by adding t.Skip("boarding-only key discovery not supported; see
<tracking-issue-link>") at the top of
TestHDWalletRecoversBoardingOnlyFundedKeys, or alternatively validate that the
Electrum explorer/indexer changes actually cause discoverHDWalletKeys to detect
boarding-only addresses before merging (i.e., add a note in the test asserting
the explorer behavior or remove the skip once
discoverHDWalletKeys/getAllocatedAddresses are updated).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 03b0640d-d1ba-47b3-adaa-82c1b32aa588
📒 Files selected for processing (16)
Makefileclient.goclient_test.goexplorer/electrum/break_test.goexplorer/electrum/client.goexplorer/electrum/doc.goexplorer/electrum/explorer.goexplorer/electrum/explorer_test.goexplorer/electrum/opts.goexplorer/electrum/types.goexplorer/electrum/utils.goinit.goinit_opts.gotest/docker/docker-compose.ymltest/e2e/hd_wallet_test.gotest/e2e/utils_test.go
✅ Files skipped from review due to trivial changes (2)
- explorer/electrum/doc.go
- explorer/electrum/types.go
🚧 Files skipped from review as they are similar to previous changes (5)
- init_opts.go
- explorer/electrum/opts.go
- client.go
- explorer/electrum/utils.go
- explorer/electrum/explorer.go
|
Responding to the comments from: #165 I have made electrum the default for tests, addressed the test concerns outlined, and brought in the electrum doc provided. The only test I considered push back against was |
bitcoin-coder-bob
force-pushed
the
feat/electrum-support
branch
from
68c4fc4 to
69a92a0
Compare
|
@coderabbitai @arkanaai review |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
Arkana Review — ElectrumX ExplorerGood PR overall. The transport layer is well-engineered and the eight bug-fix callouts in the description are all real and correctly fixed. A few things worth discussing before merging: 🔴 Breaking: default regtest URL changed
🟡
|
|
|
@coderabbitai @arkanaai review |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
Arkana Code Review — ElectrumX Explorer SupportSolid addition overall. The transport is clean and self-contained (no third-party go-electrum dependency), the goroutine lifecycle is well thought-out, and the bug fixes bundled in are legitimate catches. A few things worth discussing before merge: 🔴 Issues1. In hexes = append(hexes, txHex)
...
e.setCacheTx(txid, txHex) // ← cached here
...
resp, err := http.Post(url, ...) // ← but this can failIf the package broadcast fails, the cache has stale entries and subsequent 2. Stale comment in // electrs-esplora does not support verbose transactions, so we decode the
// raw TX to get an output script, then look up the tx in the scripthash historyThis comment says "electrs-esplora" but we're in the Electrum explorer. The real reason is that 🟡 Design notes3.
Worth a quick pass to confirm all 4. The PR description correctly notes this is O(outputs × history_depth) round-trips. During exit/unroll flows, 5. Regtest default URL change is a breaking change Changing the regtest default from 6. The validation that ✅ What's well done
SummaryFix the 🤖 Arkana |
There was a problem hiding this comment.
Arkana Re-Review — Commit 79ad9d6
All issues from the previous review are resolved:
broadcastPackagecache-before-success — ✅ Fixed.setCacheTxnow runs only after the HTTP 200 check, so failed broadcasts don't pollute the cache.- Stale
GetTxBlockTimecomment — ✅ Fixed. Now correctly referencesblockchain.transaction.get. - Break test clarity — ✅ Comments now clearly state whether each test is a regression guard or documents current behaviour.
- Error message — ✅
WithElectrumPackageBroadcastURLvalidation message is clearer. GetTxOutspendstest — ✅ Now usesrequire.Errorinstead of conditional assertion — strictly better.
No new concerns. LGTM.
🤖 Arkana
addressed most of your changes in 79ad9d6, pushing back on:
|
bitcoin-coder-bob
force-pushed
the
feat/electrum-support
branch
from
64ebe9a to
c3c73c0
Compare
There was a problem hiding this comment.
Arkana Re-Review — New Commits (13a47ef, c3c73c0)
Scope of changes since last review (79ad9d6):
- Merge from master — brings in Contract Manager (#145) and Auto Settle (#169)
discover.goremoval — dead code after contract manager merge
Verification:
- ✅
go build ./...— compiles clean, no errors - ✅ No dangling code references to removed functions (
discoverHDWalletKeys,deriveOffchainScriptsBatch, etc.) - ✅ Merge resolution looks correct — no conflicts with electrum code
Minor nit (non-blocking): Two stale comment references to discoverHDWalletKeys remain:
explorer/electrum/break_test.go:196explorer/electrum/doc.go:75
These are documentation-only — can be cleaned up in a follow-up.
LGTM. Previous approval still stands.
🤖 Arkana
bitcoin-coder-bob
force-pushed
the
feat/electrum-support
branch
from
c3c73c0 to
2ac7edb
Compare
There was a problem hiding this comment.
Arkana Re-Review — New Commits (c0e3eeb, 2c9bfc6)
🤖 Arkana
Scope: Two commits since last review — concurrency/lifecycle fixes and test stabilization.
c0e3eeb — "comment, context, and locking fixes"
1. Stop()/Lock() ordering fix ✅ Correct.
stopFn() (context cancel) now fires before Explorer().Stop() in both client.go:Stop() and init.go:Lock(). This is the right order: cancel the background context first so listenForOnchainTxs, periodicRefreshDb, etc. start exiting and stop calling explorer methods, then tear down the explorer. Previously the reverse order created a window where goroutines called methods on a stopped explorer.
2. notifWg.Add(1) race fix (explorer.go:617-628) ✅ Correct.
Moving notifWg.Add(1) inside subscribedMu and gating it on e.stopped prevents the classic WaitGroup panic where Add races with Wait. The stopped flag is set under the same lock in Stop() before notifWg.Wait(), so the two are now properly serialized.
3. NewBoardingAddress retry loop (funding.go:81-101) ✅ Correct.
Subscribe now retries with 3s backoff using bgCtx for cancellation. Previously a transient failure silently dropped the subscription forever — the user would never see boarding deposits. The bgCtx nil guard (context.Background() fallback) is defensive and correct; safeCheck() ensures bgCtx is always set in practice.
4. bgCtx field (client.go:57, init.go:142) ✅ Clean.
Stored during Unlock(), used only by the retry goroutine in NewBoardingAddress. No synchronization issue — safeCheck() enforces happens-before.
5. Stale comments (break_test.go:196, doc.go:75) ✅ Fixed per previous nit — discoverHDWalletKeys → ScanContracts.
2c9bfc6 — "add WithoutAutoSettle() to fix flaky tests"
Test-only change. Adds WithoutAutoSettle() to tests that don't exercise auto-settle, preventing the auto-settle scheduler from interfering with test assertions. Straightforward and correct.
No issues found.
Previous approval stands. All concurrency fixes are well-reasoned and correctly implemented.
There was a problem hiding this comment.
Arkana Re-Review — New Commits (c0e3eeb, 2c9bfc6)
🤖 Arkana
Scope: Two commits since last review — concurrency/lifecycle fixes and test stabilization.
c0e3eeb — "comment, context, and locking fixes"
1. Stop()/Lock() ordering fix ✅ Correct.
stopFn() (context cancel) now fires before Explorer().Stop() in both client.go:Stop() and init.go:Lock(). This is the right order: cancel the background context first so listenForOnchainTxs, periodicRefreshDb, etc. start exiting and stop calling explorer methods, then tear down the explorer. Previously the reverse order created a window where goroutines called methods on a stopped explorer.
2. notifWg.Add(1) race fix (explorer.go:617-628) ✅ Correct.
Moving notifWg.Add(1) inside subscribedMu and gating it on e.stopped prevents the classic WaitGroup panic where Add races with Wait. The stopped flag is set under the same lock in Stop() before notifWg.Wait(), so the two are now properly serialized.
3. NewBoardingAddress retry loop (funding.go:81-101) ✅ Correct.
Subscribe now retries with 3s backoff using bgCtx for cancellation. Previously a transient failure silently dropped the subscription forever — the user would never see boarding deposits. The bgCtx nil guard (context.Background() fallback) is defensive and correct; safeCheck() ensures bgCtx is always set in practice.
4. bgCtx field (client.go:57, init.go:142) ✅ Clean.
Stored during Unlock(), used only by the retry goroutine in NewBoardingAddress. No synchronization issue — safeCheck() enforces happens-before.
5. Stale comments (break_test.go:196, doc.go:75) ✅ Fixed per previous nit — discoverHDWalletKeys → ScanContracts.
2c9bfc6 — "add WithoutAutoSettle() to fix flaky tests"
Test-only change. Adds WithoutAutoSettle() to tests that don't exercise auto-settle, preventing the auto-settle scheduler from interfering with test assertions. Straightforward and correct.
No issues found.
Previous approval stands. All concurrency fixes are well-reasoned and correctly implemented.
2 participants
closes #68
What
Adds an ElectrumX-backed
Explorerimplementation so users can point the SDK at a self-hosted server instead of the third-party mempool.space API. The new backend speaks JSON-RPC over a persistent TCP or SSL connection and covers every operation the SDK requires: boarding, unrolls, and restore.A new
WithElectrumExplorerinit option selects the backend. A companionWithElectrumPackageBroadcastURLoption supplies an esplora-compatible REST base URL for broadcasting transaction packages (required for zero-fee v3 transactions with P2A anchors that Bitcoin Core rejects viasendrawtransaction). The stored URL is self-describing, soLoadArkClientpicks up the right implementation automatically from the persisted config.HD wallet restore now also queries the explorer for boarding-address transaction history during key discovery, so keys funded only via boarding (on-chain, never receiving an offchain VTXO) are recovered correctly.
Usage
Pass
WithElectrumExplorerwhen callingInitto swap the explorer backend:LoadArkClientneeds no extra option — it reads thetcp://orssl://prefix from the stored config and routes to the electrum backend automatically:The default (no option, or
WithExplorerURL) continues to use mempool.space unchanged.Why
The existing mempool.space explorer is convenient but it is a third-party service with rate limits, potential downtime, and no support for private/regtest environments. ElectrumX is the standard self-hosted indexer for Bitcoin and is already used in production by the ocean project. Offering it as a first-class backend gives operators full control over their infrastructure without giving up any SDK functionality.
How
The transport is a self-contained TCP client (no third-party go-electrum dependency) modeled on the ocean project's electrum scanner. It multiplexes all requests over a single connection using atomic JSON-RPC IDs, routes push notifications to per-address channels, and replays subscriptions on reconnect.
A
newExplorerfactory selects the implementation based on the URL prefix:tcp://orssl://routes to electrum; everything else routes to mempool.space. This keeps the selection logic in one place for bothInitandLoadArkClient. The default regtest explorer URL is nowtcp://127.0.0.1:50001(electrum) instead of the previous mempool.space local URL.The explorer event channel is pre-registered before the sync signal is sent, so events fired immediately after unlock (e.g., from
NewBoardingAddress) are not dropped beforelistenForOnchainTxsstarts consuming.SubscribeForAddressesretries with a 3-second backoff instead of bailing on first failure.The
Explorerinterface is implemented method-by-method against ElectrumX primitives:GetTxHex—blockchain.transaction.getBroadcast—blockchain.transaction.broadcast(sequential for CPFP; parent lands in mempool before child is sent)GetTxs—blockchain.scripthash.get_history+ verbose tx fetchGetUtxos—blockchain.scripthash.listunspentGetTxOutspends— derived by scanning scripthash history for each output (ElectrumX has no dedicated endpoint)GetTxBlockTime— verbose txblocktimefield, with fallback toblockchain.block.headerparsingGetFeeRate—blockchain.estimatefeeblockchain.scripthash.subscribepush notifications with a polling fallbackParity with the mempool explorer
Getting the field mapping right between a float-denominated JSON-RPC protocol and the
uint64-denominated internal types requires care. Issues caught and fixed during the audit:uint64(value * 1e8)silently drops satoshis for amounts like1.23456789 BTC(gives123456788instead of123456789). Fixed withmath.Round.GetTxsandGetTxBlockTimewere fetchingblockchain.block.headereven though the verbose tx response already carriesblocktime. Now uses it directly, falling back to the header only when the field is zero.Output.Addressmissing:addressTxHistoryToUtxosfilters outputs byvout.Address != address, so every UTXO would silently drop ifAddresswas empty. Fixed by deriving it from the output script viatxscript.ParsePkScript.CreatedAtandSpentBynot populated on events:pollAddressnow enriches newly seen, confirmed, and spent UTXOs with block timestamps and spending txids respectively.WithTracker(true)no-op: the option was written asif !withTracker { svc.noTracking = true }and never cleared the flag for thetruecase.networkToChainParamswas missing cases forBitcoinRegTestandBitcoinMutinyNet, causing address decoding to fail silently.syncChwas signalled. Any UTXO event fired during the gap (e.g., fromNewBoardingAddresscalled immediately afterUnlock) was silently lost. Fixed by pre-registering the channel before the sync signal.SubscribeForAddressesnot retried: A transient connection error during subscription would abort the listener permanently. Now retries with a 3-second backoff until the context is cancelled.Test coverage
All methods are tested against a mock TCP server built with
net.Pipe. Tests pin every field of every return type against the interface contract:Status.BlockTime=0(not-1) for unconfirmed transactions,Input.Txid/Vout/Address/Amount/Scriptpopulated from prevout data, per-outputSpent/SpentBycorrectness for multi-vout transactions, locked vs spendable split inGetRedeemedVtxosBalance, multi-txBroadcastreturning the first txid,GetTxHexserved from cache afterBroadcast, and push-notification-triggered events carrying correctCreatedAtandSpentByvalues.Integration (e2e) tests now run against an electrum backend rather than mempool.space.
TestHDWalletRecoversBoardingOnlyFundedKeys, previously skipped, is now enabled and covers boarding-only funded key recovery.