Report null instead of a wrong fee rate when an estimate is unavailable - #15
Report null instead of a wrong fee rate when an estimate is unavailable#15sanket1729 wants to merge 2 commits into
Conversation
Simulation cost grows with the block target: getExpectedBlocksMined evaluates a Poisson tail of 4 * target entries and then simulates that many blocks, so a target of 1_000_000 spends roughly two minutes of CPU. Neither the numOfBlocks argument nor the blockTargets constructor list had an upper bound, and bitcoin-augur-server passes an HTTP query parameter straight into numOfBlocks, so one unauthenticated request can saturate a core. Reject targets above MAX_BLOCK_TARGET, one week of blocks.
1 sat/vB meant three different things: a real recommendation, "no data", and "the simulation failed". Callers could not tell them apart, so an unanswerable request came back as the cheapest possible fee. - A confidence level above 1 - exp(-blockTarget) cannot be met at any fee rate. runSimulations returned bucket 0 for it, which is a real fee rate. - findBestIndex flagged failure with bucketMax + 1, whose fee rate is 22247.84 sat/vB. That only stayed hidden because it sits above the default maxFeeRate; raising maxFeeRate surfaced it as an estimate. - enforceMonotonicity clamped each target to the previous one, so a collapsed short target dragged every longer target in the column down with it. - The short/long blend ramp 1 - (1 - t/144)^2 turns back down past the window and reaches -35 at 1008 blocks, extrapolating the two estimates apart instead of averaging them. It also ignored longTermWindowDuration. - InflowCalculator divided by a zero span whenever no block height had two snapshots, the normal state for a per-block collector: Infinity, then NaN in every bucket, then a table of nulls with no indication why. It also measured the span in whole seconds, truncating sub-second spacing to zero. - Unsorted blockTargets broke the monotonicity walk. - mineBlock treated a negative bucket weight as freed capacity. - getNearestBlockTarget resolved exact ties by map iteration order and could overflow on extreme inputs. Fixture weights are now seeded so the new assertions are reproducible.
|
There are lots of comments in here that reference historically how things used to be. I think that might be confusing/unnecessary for a new reader coming in here with a fresh set of eyes with no context. What do you think about removing them? Could do a pass for "previously", "used to be", etc. maybe if you agree? |
| // independent of maxFeeRate. The old out-of-band bucket sentinel did not: it relied on the | ||
| // gap between exp(1001/100) and DEFAULT_MAX_FEE_RATE, so raising maxFeeRate surfaced | ||
| // 22247.84 sat/vB as a real estimate. |
There was a problem hiding this comment.
"The old out-of-band bucket sentinel" this might be confusing/unnecessary to a new reader?
| // nothing to normalize. Dividing anyway yielded Infinity, then 0.0 * Infinity = NaN in every | ||
| // bucket, and those NaNs voided the whole fee table with no indication of why. |
There was a problem hiding this comment.
"Dividing anyway" this might be confusing/unnecessary to a new reader?
| // observable between two snapshots at the same height, so the measured span is zero. Dividing by | ||
| // it produced Infinity, then 0.0 * Infinity = NaN in every bucket, and those NaNs propagated | ||
| // through the simulation until every cell of the fee table came back null. |
| // Monotonicity assumes ascending targets. With an unsorted list the longest target was visited | ||
| // first and became the bound for every shorter one. |
There was a problem hiding this comment.
Same. Etc I'll stop flagging all of them bug you get the idea 😄
| // Comparing in Long avoids overflow on extreme inputs, and the second comparator makes the | ||
| // result independent of map iteration order -- previously an exact tie returned whichever key | ||
| // the map happened to yield first. | ||
| return estimates.keys.minWithOrNull( |
There was a problem hiding this comment.
🤖 Now that unanswerable cells are nulled, convertToFeeEstimate still adds a BlockTarget with an empty probabilities map, so this can return a target with no fee rate at all — targets [1, 6] at p=0.9 returns 1 for a request of 2. Consider skipping targets whose probabilities map is empty.
| if (simdSnapshots.groupingBy { it.blockHeight }.eachCount().none { it.value > 1 }) { | ||
| return FeeEstimate(emptyMap(), orderedSnapshots.last().timestamp) |
There was a problem hiding this comment.
This changes the observable response shape for sparse history:
Before: {3: {}, 6: {}, …, 144: {}} — configured targets were present with no probability values.
After: {} — no target keys are returned.
Maybe we should do a full version bump for anyone that might be using the public endpoint.
| if (simdSnapshots.groupingBy { it.blockHeight }.eachCount().none { it.value > 1 }) { | ||
| return FeeEstimate(emptyMap(), orderedSnapshots.last().timestamp) |
There was a problem hiding this comment.
Another thing about this line...Augur checks inflow availability across all history but calculates inflow separately for the 30m/24h windows. After a collection gap, old data can satisfy the guard while the recent window is treated as zero inflow, potentially underpricing short targets. Maybe the estimator should track availability per window and either use the available horizon or return no estimate, rather than treating missing observations as measured zero inflow. Not sure if this is too complicated 🤔
2 participants
1 sat/vBmeant three different things — a real recommendation, "no data", and "the simulation failed" — so an unanswerable request came back as the cheapest possible fee. Nine bugs: the bucket-0 fallback, monotonicity spreading it down a whole column, a sentinel fee rate that leaks oncemaxFeeRateis raised, a blend ramp that goes negative past the long-term window and ignoredlongTermWindowDuration, an inflow divide-by-zero when no block height has two snapshots plus whole-second truncation, unsortedblockTargets, negative bucket weight read as freed block capacity, and a nondeterministicgetNearestBlockTargetthat could overflow.Behaviour change: cells that cannot be answered now return
nullinstead of a plausible number.Stacked on #14.