chainbackends: fix double-conversion in LndClientFeeEstimator - #238
Merged
Conversation
lndclient.WalletKitClient.EstimateFeeRate returns chainfee.SatPerKWeight directly (it does SatPerKWeight(resp.SatPerKw) internally). The adapter was incorrectly assigning the result to a variable named satPerVByte and converting again via SatPerVByte().FeePerKWeight(), inflating the fee rate by ~250x. Fix by returning the SatPerKWeight value directly without double-conversion.
![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 refactors the EstimateFeePerKW method in LndClientFeeEstimator to remove a redundant fee rate conversion. The EstimateFeeRate call now directly returns the expected chainfee.SatPerKWeight value, and the code has been updated with explanatory comments and minor formatting adjustments. I have no feedback to provide.
bhandras
approved these changes
Apr 7, 2026
Roasbeef
added a commit
that referenced
this pull request
Apr 10, 2026
Three landed PRs since the last broad gardening sweep touch subsystems whose CLAUDE.md/AGENTS.md did not yet reflect the new shape: PR #225 (send-in-round-followup), PR #238 (fee-estimator fix, no doc impact), and PR #239 (partial-unroll-harness-accessor). The most significant structural changes are the introduction of a data-driven pkScript ownership path and a dedicated actor that materializes round-produced VTXOs from indexer push notifications. The round FSM no longer relies on a per-intent IsOwner flag. Instead, buildOwnedClientVTXOs resolves ownership at round confirmation time by asking a round.OwnedScriptChecker whether each VTXO's pkScript is recognized by the local wallet, and round.OwnedScriptRegistrar persists locally-owned scripts at intent-build time and inside handleRegisterIntent for incoming intents whose owner key has a non-zero KeyLocator. The darepod package exposes both interfaces as thin adapters over the OOR owned-receive-scripts store, which means the same table now backs three abstractions (OwnedScriptChecker, OwnedScriptRegistrar, and vtxo.OwnedScriptLookup). The per-package docs pick up these new interfaces, the adapter types, and the invariants they enforce; ARCHITECTURE.md gains a new "Data-Driven Script Ownership" pattern section so agents encountering the code can follow the flow without re-deriving it from commits. The vtxo package now hosts an IncomingVTXOHandler actor that decodes arkrpc.IncomingVTXOEvent push notifications, materializes the VTXO descriptor via lib/scripts.VTXOTapScript, persists it, and notifies the VTXO manager via VTXOsMaterializedNotification. darepod registers the actor under vtxo.IncomingVTXOServiceKey and wires a new MethodIncomingVTXO route into the EventRouter. The vtxo CLAUDE.md now documents the handler's inputs, validation rules (only VTXO_CREATED events, bounds-checked values, nil-safe pkScripts), and the fact that CommitmentTxID comes from the event rather than the leaf txid. The darepod doc picks up the route registration and the service-key bootstrap; ARCHITECTURE.md mentions the new route under the RPC-over-Mailbox pattern. Smaller updates round out the sweep. wallet/CLAUDE.md describes the hardened handleSendVTXOs path: recipient amounts are now bounded by MaxSatoshi, the running total uses overflow-safe accumulation, and the old releaseAndFail helper is replaced by a deferred release that uses context.WithoutCancel so cleanup survives caller disconnect. The darepod doc records the matching RPC-side validation cap (maxRecipients = 256) and notes that s.clk is now a cached clock instance so sub-stores share a single Clock for deterministic tests, plus the new GetStoredVTXO harness accessor used by partial unroll itests. Round, darepod, vtxo, and wallet docs all cross-reference the new ownership interfaces where relevant so navigating the per-package graph reaches the same explanation from any entry point. make doc-check is clean after these changes.
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
lndclient.WalletKitClient.EstimateFeeRatereturnschainfee.SatPerKWeightdirectly (it doesSatPerKWeight(resp.SatPerKw)internally)LndClientFeeEstimator.EstimateFeePerKWwas assigning the result tosatPerVByteand converting again viaSatPerVByte().FeePerKWeight(), inflating the fee rate by ~250xSatPerKWeightvalue directly without double-conversionThis bug was dormant until lightninglabs/darepo#222 wired
LndClientFeeEstimatorinto the server's round subsystem for batch commitment transactions.Test plan
lndclient.WalletKitClient.EstimateFeeRatereturn type ischainfee.SatPerKWeightinlndclient@v1.0.1make fmt-checkpassesmake lintpasses