Repository navigation
Conversation
|
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?
Thanks for the review @tnull , and no worries about the delay! (a): turns out regtest doesn't serve estimates at all. I ran the electrs build estimatesmartfee has no data on regtest, so there's nothing for convert_fee_rate What I'd do instead: per-target defaults on mainnet/testnet, keep the 1 sat/vb (b): yes, agreed. On the apply_post_estimation_adjustments question from the description: I don't |
b246cb5 to
6143b78
Compare
Jolah1
changed the title
|
@tnull (a) regtest/signet keep their 1 sat/vb, mainnet/testnet get the per-target defaults. The regtest/signet value stays adjusted as an estimate would be, so MaximumFeeEstimate is still 2775 sats/kwu rather than the raw 250, which (b) All three sources share one get_fallback_fee_rate_for_network. Electrum's -1 and results that aren't numbers were floored to 1 sat/vb too, so this was reachable there on mainnet, and Bitcoin Core no longer discards the |
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. A non-positive entry is treated as no entry at all, as it is no more usable than a missing one and must not pin the target to the floor either. The defaults are sized for a chain that has a fee market, though, and `regtest`/`signet` don't have one: regtest Esplora serves an empty map, as `estimatesmartfee` has nothing to go on there, so the fallback would decide every rate rather than the odd sparse-map target. We therefore stand in the 1 sat/vb we have always used on those two networks, which is the same split `BitcoindChainSource` already makes, and adjust that stand-in exactly as we would an estimate, because that is what those networks have always cached: `MaximumFeeEstimate` stays at 2775 sats/kwu rather than dropping to the raw 250, which matters as LDK sizes a channel's dust exposure limit off it. `get_fallback_fee_rate_for_network` holds that decision so the remaining chain sources can share it. Finally, we log the targets we had to fall back to a default for. On `regtest`/`signet` that is every target on every update, so we keep quiet there, but anywhere else it means we're pricing transactions off defaults rather than off the chain, which shouldn't pass silently. 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, for non-positive entries, for the network-dependent fallback, and for the unchanged complete-map behavior. This change was written with the assistance of Claude Code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016RDZbFbxkpNupzvTFDsL43
A missing estimate should mean the same thing no matter which backend we happen to be on, so have the remaining two chain sources fall back through `get_fallback_fee_rate_for_network` as well. For Electrum that covers the results we can't use: a server that has no estimate for the requested block count answers `-1`, and a result may not be a number at all. Both used to be floored to 1 sat/vb, so the silent downgrade of urgent targets this series removes from the Esplora source was reachable here too, mainnet included. We report the targets we had to fall back for, as the Esplora source now does, and index into the batch results rather than zipping them with the targets, so that a short response could never silently drop the trailing targets from the cache. Note `batch_call` returns one result per request or an error, so that isn't a case we can hit today. For Bitcoin Core it means we no longer abandon the whole update when a single `estimatesmartfee` call fails off mainnet, which kept none of the estimates we did get, and that the fallback no longer runs through the post-estimation adjustments. The regtest/signet rates are unchanged, and mainnet still fails hard. This change was written with the assistance of Claude Code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016RDZbFbxkpNupzvTFDsL43
6143b78 to
260607e
Compare
3 participants
Fixes #1028.
When a chain source gives us no usable estimate for a confirmation target, we used to cache 1 sat/vb for it. That is worse than caching nothing: estimate_fee_rate already falls back to get_fallback_rate_for_target on a cache
miss, so writing 1 sat/vb in only shadows the value we would have used. UrgentOnChainSweep ends up at 250 sats/kwu where its default is 5000.
We cache the target's own default instead, through one get_fallback_fee_rate_for_network that all three chain sources call. The default is not run through the post-estimation adjustments, so it matches what the cache-miss pathreturns.
(a) Regtest Esplora returns an empty map, since estimatesmartfee has nothing to go on there, so the fallback decides every rate rather than the odd starved target. Mainnet-sized defaults make no sense without a fee market, so regtest and signet keep the 1 sat/vb they have always had, which is the split bitcoind.rs already made. The stand-in stays adjusted as an estimate would be, as those networks have always cached it that way: MaximumFeeEstimate at 2775 sats/kwu, not the raw 250, which matters because LDK sizes a channel's dust exposure limit off it.
(b) Electrum answers -1 when it has no estimate for a block count, and a result is not necessarily a number; both were floored to 1 sat/vb, so this was reachable there too, mainnet included. Bitcoin Core no longer abandons the
whole update off mainnet when one estimatesmartfee call fails. Both sources now log which targets fell back, quietly on regtest/signet where all of them do.
Not addressed: the sources still disagree about when to give up on an update entirely, and off mainnet a failed estimatesmartfee overwrites that target's last good cached value with the default.
Testing
Cache construction moves into a pure build_fee_rate_cache, and Electrum's conversion into fee_rate_from_estimate, so both are testable without a server. Unit tests cover sparse and empty maps on and off mainnet, non-positive entries, unusable Electrum results, and the network-dependent fallback including the 2775. The sparse test fails against the old behaviour. On-chain and channel_full_cycle integration tests ran locally against bitcoind 29.0 and electrs.