Conversation
Esplora may return a non-empty estimate map that still has no usable entry for some of our confirmation targets, as `convert_fee_rate` only yields a value if the map holds a block count at or below the requested one. We'd then substitute 1 sat/vb, which is well below the default we'd otherwise use for urgent targets: `UrgentOnChainSweep` would end up at 250 sats/kwu instead of 5000. Note this isn't a case of a missing fallback, but of an actively harmful one: `OnchainFeeEstimator::estimate_fee_rate` already falls back to `get_fallback_rate_for_target` whenever it misses the cache, so inserting 1 sat/vb only serves to shadow the value we'd have used anyway. Here we therefore fall back to the target's own default. We deliberately skip the post-estimation adjustments for it, as there is no estimate to adjust, and so that we land on the same rate the cache-miss path would have given us. To allow testing this without an Esplora server, we move the cache construction to `build_fee_rate_cache` and add coverage for sparse maps, for empty maps on and off Mainnet, and for the unchanged complete-map behavior. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
I've assigned @tnull as a reviewer! |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Please excuse the delay here!
Yes, we're currently considering Esplora/Electrum servers to be somewhat trusted (for fee rates, but also privacy-wise). It could make sense to apply additional checks to the returned fee rates (i.e., also somewhat bound them), but this is generally pretty delicate as we'd still need to make sure that we can handle legitimate fee rate spikes.
As for falling back to default values - yes, that's somewhat legitimate also but a) different networks (e.g., regtest) probably need to be handled differently and b) if we do this, we'd probably at the very least implement the same behavior across all three chain sources?
3 participants
Fixes #1028.
Esplora may return a non-empty estimate map that still has no usable entry for a given confirmation target, since convert_fee_rate only yields a value if the map holds a block count at or below the requested one. A map of {144:
2.0, 1008: 1.0} therefore leaves every target below 144 blocks — UrgentOnChainSweep, OnchainPayment, ChannelCloseMinimum, ... — without an estimate.
Today we substitute 1 sat/vb in that case. That's not a missing fallback but an actively harmful one: OnchainFeeEstimator::estimate_fee_rate already falls back to get_fallback_rate_for_target whenever it misses the cache, so
writing 1 sat/vb into the cache only serves to shadow the value we'd otherwise have used. For UrgentOnChainSweep that's 250 sats/kwu where the default is 5000 — a 20x underestimate on a target whose whole point is timeliness.
So here we fall back to the target's own default instead. The post-estimation adjustments are deliberately skipped for it: there's no estimate to adjust, and skipping them lands us on exactly the rate the cache-miss path would
have produced, which keeps the two paths consistent.
One open question for reviewers: whether that's the behaviour you want, or whether the fallback should go through apply_post_estimation_adjustments anyway. It only matters for MaximumFeeEstimate (8000 unadjusted vs. 11300
adjusted) and MinAllowedNonAnchorChannelRemoteFee (unchanged, as the subtraction clamps back to the floor). Happy to flip it — it's a one-line change plus one expected value in the test.
To make this testable without an Esplora server, the cache construction moves out of update_fee_rate_estimates into a pure build_fee_rate_cache. The new tests cover sparse maps, empty maps on and off Mainnet, and assert the
complete-map behaviour is unchanged.
I confirmed the sparse test fails against the previous behaviour (250 vs. 5000 sats/kwu for UrgentOnChainSweep, 250 vs. 5000 for OnchainPayment).