btcwbackend: add neutrino+btcwallet wallet mode - #228
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new wallet backend, btcwbackend, which integrates btcwallet with neutrino to provide a self-contained SPV wallet implementation. The new backend includes a boarding adapter for Ark protocol participation and a chain backend for event-driven notifications. Key feedback points out a potential nil pointer dereference in the server initialization when the wallet is not auto-unlocked, a logic inconsistency in GetTransaction where the implemented fallback behavior does not match the documentation, and a resource lifecycle issue in RegisterBlocks caused by using a potentially short-lived caller context for a background subscription.
| case WalletTypeBtcwallet: | ||
| // If the btcwallet is already started (auto-unlock | ||
| // succeeded), use its chain backend. | ||
| if s.btcwWallet.IsSome() { | ||
| w := s.btcwWallet.UnsafeFromSome() | ||
| s.chainBackend = w.ChainBackend() | ||
| alreadyStarted = true | ||
| } | ||
|
|
||
| // Unlike lwwallet, we cannot create a standalone | ||
| // neutrino chain backend without the wallet since | ||
| // neutrino requires the full service to be running. | ||
| // If the wallet is not yet started, the chain backend | ||
| // will be initialized when startBtcwallet is called. | ||
|
|
There was a problem hiding this comment.
This case fails to handle the scenario where the wallet is not yet started (e.g., when auto-unlock fails). Unlike the WalletTypeLwwallet case, s.chainBackend is not assigned a value here if s.btcwWallet.IsNone(). This will cause a nil pointer dereference and panic later in the function when s.chainBackend.Start() is called (on line 1038). Since the daemon is designed to support a locked wallet state for this backend, this function must ensure s.chainBackend is initialized or handle the deferred initialization correctly to avoid crashing the daemon on startup.
| epochChan := make(chan *chainsource.BlockEpoch, 10) | ||
|
|
||
| go func() { | ||
| defer close(epochChan) | ||
| defer event.Cancel() | ||
|
|
||
| for { | ||
| select { | ||
| case lndEpoch, ok := <-event.Epochs: | ||
| if !ok { | ||
| return | ||
| } | ||
|
|
||
| if lndEpoch.Hash == nil { | ||
| continue | ||
| } | ||
|
|
||
| var timestamp int64 | ||
| if lndEpoch.BlockHeader != nil { | ||
| ts := lndEpoch.BlockHeader.Timestamp | ||
| timestamp = ts.Unix() | ||
| } | ||
|
|
||
| epoch := &chainsource.BlockEpoch{ | ||
| Hash: *lndEpoch.Hash, | ||
| Height: lndEpoch.Height, | ||
| Timestamp: timestamp, | ||
| } | ||
|
|
||
| select { | ||
| case epochChan <- epoch: | ||
|
|
||
| case <-ctx.Done(): | ||
| return | ||
| } | ||
|
|
||
| case <-ctx.Done(): | ||
| return | ||
| } | ||
| } | ||
| }() | ||
|
|
||
| return &chainsource.BlockRegistration{ | ||
| Epochs: epochChan, | ||
| Cancel: event.Cancel, | ||
| }, nil |
There was a problem hiding this comment.
The background goroutine in RegisterBlocks incorrectly uses the input ctx to control its lifetime. This is inconsistent with RegisterConf and RegisterSpend, which use a separate long-lived context. If the caller passes a short-lived context (e.g., from a gRPC request), the block subscription will be terminated prematurely when that context is cancelled, even if the registration is intended to remain active until the returned Cancel function is called.
References
- Follow consistent patterns for resource lifecycle management across similar methods in the same package. (link)
- Long-lived background tasks should use a dedicated context linked to their specific lifecycle rather than inheriting a potentially short-lived caller context.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7809b4db98
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if s.btcwWallet.IsSome() { | ||
| w := s.btcwWallet.UnsafeFromSome() | ||
| s.chainBackend = w.ChainBackend() | ||
| alreadyStarted = true | ||
| } |
There was a problem hiding this comment.
When wallet.type=btcwallet starts without an already-open wallet (no env seed or no auto-unlock password), this branch leaves s.chainBackend unset, but initChainBackend still reaches the existing s.chainBackend.Start() call below. That causes a nil-interface dereference during daemon startup, so the process dies before InitWallet/UnlockWallet RPC can ever be used. Please either create a usable backend in this path or explicitly defer chain-source initialization until startBtcwallet has populated s.chainBackend.
Useful? React with 👍 / 👎.
| maxConfs = math.MaxInt32 | ||
| } | ||
|
|
||
| addrs := b.SnapshotAddrs() |
There was a problem hiding this comment.
This should be removed, instead returning all the addresses. On restart, we don't re-populate this, so it can cause us to miss outputs.
|
@claude review this |
|
Claude finished @Roasbeef's task in 14m 51s —— View job PR Review: btcwbackend — neutrino+btcwallet wallet mode
Overall the design is solid — the three-interface pattern is clean, the Critical1. Nil chain backend panics when wallet is locked at startup When chainActor := chainsource.NewChainSourceActor(
chainsource.ChainSourceConfig{
Backend: s.chainBackend, // nil here!
...
},
)
chainSourceRef := actor.RegisterWithSystem(...)The actor captures the nil The The fix needs to either:
Medium2. Address filter in This is also flagged by @Roasbeef. Removing the filter (return all UTXOs from btcwallet's 3. The function comment says it fetches from the containing block via neutrino if the tx isn't in the wallet store, but the implementation returns an error immediately: tx, err := b.BtcWallet.FetchTx(txid)
if err != nil || tx == nil {
return nil, nil, fmt.Errorf("transaction %s not found in wallet", txid)
}Either implement the neutrino P2P fallback (using 4. No validation that The PR description notes that a fee URL is "required on mainnet". There is no config validation enforcing this. Starting on mainnet without Low / Nits5. Gemini's The comment at 6. In the 7. var WalletPassphrase = []byte("lwwallet")Now shared between lwwallet and btcwbackend, but still named/valued 8. Neutrino stop errors are logged at 9. No unit or integration tests PR acknowledges this. Follow-up PRs should include at least:
Summary
|
|
@claude Re the critical finding: the nil The address filter removal in GetTransaction docstring is also cleaned up in the same fixup. FeeURL validation was already required unconditionally for btcwallet mode in an earlier commit. |
|
Claude finished @Roasbeef's task in 2m 9s —— View job Claude finished @Roasbeef's task in 14m 51s —— View job Thanks for the follow-up. Verified each point against the current code:
Critical finding — still a real bugThe gating argument is correct about message timing: nothing sends messages to the chain source actor before
type ChainSourceActor struct {
cfg ChainSourceConfig // Backend field captured at construction
}When So after unlock:
The lwwallet comparison doesn't apply here: for lwwallet, The minimal fix is to defer both construction and registration of the chain source actor into the goroutine that waits on Items confirmed fixed in the latest fixupAddress filter removal ( GetTransaction docstring ( FeeURL validation ( case WalletTypeBtcwallet:
if c.Wallet.FeeURL == "" {
return fmt.Errorf("wallet.feeurl is required " +
"when wallet.type is btcwallet")
}No mainnet-only condition — required in all networks. Good. |
In this commit, we introduce the btcwbackend package which provides a
fully self-contained, in-process Bitcoin wallet backed by btcwallet
with neutrino (BIP 157/158 compact block filters) as the chain
backend. This gives users a lightweight SPV option that connects
directly to the Bitcoin P2P network without requiring an external
Esplora server or LND node.
The package implements all three core interfaces needed by the daemon:
- chainsource.ChainBackend via NeutrinoNotifier for event-driven
confirmation, spend, and block notifications, with
chainfee.WebAPIEstimator for fee estimation (since neutrino has
no mempool visibility).
- wallet.BoardingBackend via BoardingBackendAdapter, which uses
btcwallet's native ListUnspent for UTXO tracking after importing
taproot scripts (unlike lwwallet which bypasses btcwallet and
queries Esplora directly for non-default scope UTXOs).
- round.ClientWallet via the Wallet struct which embeds
input.Signer from btcwallet and exposes DeriveNextKey, mirroring
the lwwallet approach.
The chain backend translates between lnd's chainntnfs event types
and the chainsource registration types using the same goroutine
forwarding pattern as chainbackends.LNDBackend.
Add WalletTypeBtcwallet constant ("btcwallet") as a third wallet
backend option alongside "lnd" and "lwwallet". The WalletConfig
struct gains fields for neutrino peer configuration (ConnectPeers,
AddPeers), data directory, fee estimation URL, and filter
persistence.
Config validation enforces that wallet.feeurl is set on mainnet
when using the btcwallet backend, since neutrino has no mempool
visibility for fee estimation.
Wire the new neutrino+btcwallet backend into all daemon switch sites
that branch on wallet type. The Server struct gains a btcwWallet
field alongside the existing lwWallet, and six switch sites now
handle WalletTypeBtcwallet:
- Start(): calls tryAutoUnlockBtcwallet (same seed/password flow
as lwwallet)
- initChainBackend(): uses btcwbackend.ChainBackend
- initWalletActor(): uses btcwbackend.BoardingBackend
- initRoundActor(): uses btcwbackend.Wallet as ClientWallet
- initVTXOManager(): uses btcwbackend.Wallet as VTXOWallet
- initOORActor(): uses btcwbackend.Wallet as input.Signer
The GenSeed, InitWallet, and UnlockWallet RPCs now accept both
lwwallet and btcwallet modes via a new isSelfManagedWallet() helper
and startSelfManagedWallet() dispatcher that routes to the correct
wallet start function. The deriveIdentityPubkey helper is similarly
updated to derive from whichever self-managed wallet is active.
The btcwbackend package imports neutrino, walletdb, and kvdb directly, so these move from indirect to direct in go.mod.
Fix all critical and high-severity findings identified during code
review:
- C1: Fix nil chainBackend panic when btcwallet is not auto-unlocked
at startup. The initChainBackend btcwallet case now returns early,
deferring chain backend init to startBtcwallet.
- H1: startBtcwallet now assigns s.chainBackend and starts it when
the chain backend was deferred at startup.
- H3: Add missing btcwallet case to oorReceiveKeyOps so OOR receive
works with the neutrino backend.
- H4: Require wallet.feeurl unconditionally for btcwallet mode
(not just mainnet), since neutrino never has mempool visibility.
Medium and low fixes:
- M1: Fix data race on importedAddrs length in log statement by
capturing the count while holding the lock.
- M3: Add WarnS log messages for silently skipped UTXOs in
ListUnspent.
- M5: Add btcwWallet.Stop() to server shutdown path to prevent
neutrino resource leaks.
- M6: Fix inaccurate GetTransaction GoDoc that described an
unimplemented neutrino fallback.
- M9: Clean up btcwallet in constructor error path when
NewChainBackend fails.
- L4: Change BestBlock log level from InfoS to DebugS.
- L5: Wire logger through NewChainBackend constructor.
- L12: Cache KeyRing on BoardingBackendAdapter construction
instead of allocating per DeriveNextKey call.
- H2: Document that importedAddrs is repopulated on restart by
the wallet actor's handleStartupRecovery.
Fix critical lifecycle bugs and goroutine safety issues identified
in the second review pass:
- C1/C2: Make ChainBackend.Start()/Stop() idempotent using
sync.Once, eliminating double-start and double-stop panics
when both Wallet and daemon call lifecycle methods.
- H1: Use walletdb.Open with Create fallback for neutrino DB
so daemon restart doesn't fail on existing database files.
- H2: Fix RegisterBlocks to use independent notifyCtx instead
of caller's context for goroutine lifecycle, matching the
pattern already used by RegisterConf and RegisterSpend.
- H3: Wrap channel sends in RegisterConf and RegisterSpend
goroutines with select on notifyCtx.Done() to prevent
blocking when callers abandon registrations.
- H7: Document correct shutdown ordering (btcwallet before
neutrino service) in Wallet.Stop().
Refactor the duplicated btcwallet wrapping code into a new
walletcore package. Both lwwallet (Esplora-backed) and btcwbackend
(neutrino-backed) now embed walletcore.Wallet and
walletcore.BoardingBackendBase instead of duplicating HD key
management, signing, address generation, balance queries, and
boarding address tracking.
The walletcore package provides:
- Wallet: embeds input.Signer, holds BtcWallet/KeyRing/ChainParams,
and exposes DeriveNextKey, DeriveKey, NewAddress, Balance,
ListUnspentWitness, ConfirmedBalance, InternalWallet.
- BoardingBackendBase: holds BtcWallet/KeyRing/ChainKeyScope/
ImportedAddrs and provides DeriveNextKey, ImportTaprootScript,
SnapshotAddrs. Chain-specific adapters embed this and implement
ListUnspent, GetTransaction, GetBlock.
- Config: base fields (Seed, ChainParams, RecoveryWindow, DBDir,
Log) that btcwbackend.Config embeds.
- CoinTypeForNet, WalletPassphrase, DefaultBlockCacheSize:
shared constants/helpers formerly duplicated in both packages.
This eliminates ~300 lines of duplicated wallet methods and ensures
both backends stay in sync for shared functionality.
Fix walletdb.Open and walletdb.Create calls that were missing the required readOnly bool parameter added in a recent btcwallet update. Add open-then-create fallback for the height hint cache DB so the chain backend works on first run. Expose bitcoind's P2P port (18444) from the Docker harness so neutrino-backed test clients can sync headers and compact block filters directly from the regtest node.
da6da82 to
63a5f57
Compare
Enable compact block filter serving on the bitcoind harness container (-blockfilterindex=1, -peerblockfilters=1, -listen=1, -bind=0.0.0.0:18444) so neutrino clients can sync headers and cfilters via the P2P protocol. Add a background sync wait in startBtcwallet that polls BestBlock until neutrino reports a non-zero height before marking the wallet ready. This prevents the daemon from accepting boarding requests before the chain backend can detect confirmations.
db: preserve context cancellation in mailbox pull
2 participants
This PR introduces a third wallet backend mode (
wallet.type=btcwallet) thatuses btcwallet with neutrino (BIP 157/158 compact block filters) as the chain
source. This provides a fully self-contained, SPV-based wallet that connects
directly to the Bitcoin P2P network—no external Esplora server or lnd node
required.
Motivation
The existing wallet backends each carry an external dependency:
lndmoderequires a full lnd node, and
lwwalletmode requires a running Esplora/mempool.space instance. For users who want a lightweight, zero-dependency
setup, neutrino gives us BIP 157/158 compact block filter support over the raw
P2P network, which is exactly what lnd itself uses in its lightest
configuration.
Design
The new
btcwbackend/package implements the same three interfaces as theexisting backends:
chainsource.ChainBackend— Uses lnd'sNeutrinoNotifierforevent-driven confirmation/spend/block notifications (no polling). Fee
estimation is handled by
chainfee.WebAPIEstimatorwith a configurable feeURL, since neutrino has no mempool visibility.
wallet.BoardingBackend— Wraps btcwallet for key derivation andtaproot script import. Unlike the lwwallet adapter (which bypasses btcwallet
and queries Esplora directly for non-default scope UTXOs), this adapter uses
btcwallet's native
ListUnspent—afterImportTaprootScript, neutrino'scompact block filter matching naturally detects matching outputs during sync.
round.ClientWallet— TheWalletstruct embedsinput.Signerfrombtcwallet and exposes
DeriveNextKey, satisfying the interface directly(same pattern as lwwallet).
The daemon wiring in
darepod/addsWalletTypeBtcwalletarms to all sixwallet-type switch sites, plus a
tryAutoUnlockBtcwalletfunction that reusesthe existing seed management and wallet unlock flow.
Configuration
Closes #149.
Test plan
go build ./btcwbackend/... ./darepod/...compiles cleanlymake lint-changed-localpasses with zero issues--wallet.type=btcwalletonsignet, verify chain sync and wallet operations