Fall back to per-target defaults for sparse Esplora fee estimates - #1042
Open
Jolah1 wants to merge 1 commit into
Open
Fall back to per-target defaults for sparse Esplora fee estimates#1042Jolah1 wants to merge 1 commit into
Jolah1 wants to merge 1 commit into
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! |
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
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.
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).