contributor-rewards, solana-cli, validator-debt: fix the hardcoded 400ms slot times - #411
Conversation
…0ms slot times Three independent fixes rather than one shared helper, because the five call sites that divided wall clock by 400ms want genuinely different answers. contributor-rewards: EpochFinder::find_epoch_at_timestamp now verifies its estimate against real block times instead of trusting a slot-duration constant. The estimate drifts by roughly 30k slots per day of lookback on mainnet, and no fixed constant survives the SIMD-0525 rollout, so a seeded guess alone selects the wrong epoch near a boundary. Because that epoch picks the leader schedule contributor rewards are computed against, this now errors where it used to return a wrong answer. solana-cli: the two duplicate constants in shreds pay and shreds publisher-rewards prepare-offchain-message collapse into one 350ms NOMINAL_SLOT_DURATION. Both users want a reproducible number more than an accurate one, so it stays cluster-independent. validator-debt: delete the two dead timestamp to Solana epoch paths and both SLOT_TIME_DURATION_SECONDS constants. JoinedSolanaEpochs::try_new already does this mapping correctly for the production path with no slot-duration constant, and the deleted code had no callers. This also removes an unsigned subtraction that would panic. Refs malbeclabs/infra#2317
Widen the absent-block classifier. getBlockTime reports "this slot has no
block" in three shapes depending on what the endpoint has behind it: a coded
skipped-slot error, a JSON null that RpcClient turns into
RpcError::ForUser("Block Not Found"), and block-not-available for a slot the
endpoint has not rooted. Matching only the first meant the forward walk over a
skipped epoch boundary never happened, so on any endpoint without long term
storage the search aborted at most epoch boundaries. A pruned ledger stays a
hard error so it fails closed, and neither settled case is retried.
Reject a timestamp ahead of the chain tip. The search accepts a candidate with
no upper bound on the grounds that the next epoch has produced no block yet, so
a read endpoint lagging behind the target timestamp would otherwise resolve it
to whatever epoch the lagging tip sits in, with no error.
Replace calculate_epoch_from_slot with EpochSchedule::get_epoch, which is the
same arithmetic for normal epochs but is warmup-aware and does not underflow
below first_normal_slot.
Stop running the epoch search twice per snapshot: fetch_leader_schedule already
reports the epoch it resolved.
Also: rename SLOT_DURATION_US to SEED_SLOT_DURATION_US and make it private so it
cannot be mistaken for a usable slot duration, drop the two ValidatorRewards
trait methods that only the deleted validator-debt code reached, say in
--valid-for's help that the window is converted at a nominal 350ms per slot, and
correct the validator-debt changelog (two pub functions plus two private
helpers, the subtraction wrapped in release rather than panicking, and
estimate_block_time_for_skipped_slot still assumes 0.4s per slot implicitly).
Refs malbeclabs/infra#2317
ben-dz
left a comment
There was a problem hiding this comment.
Mostly good, three things worth fixing, none blocking. The 350ms seed points the first probe older than the target, so a timestamp inside the endpoint's retention can still hard-fail; the 128-slot boundary walk hand-rolls getBlocksWithLimit and turns a long skip run into a permanent failure; and create_snapshot writes a schedule-less snapshot to the canonical filename and exits 0. Plus a repetition nit.
Bias the seed so the search only walks backward. A seed faster than the real slot rate over-counts the slots elapsed, so the seed epoch lands at or below the answer and the first probe is that epoch's first slot, older than the target. A pruned probe then fails the search for a target that is itself readable, and demand.rs propagates that with ? and aborts the rewards run. What matters is the direction of the error rather than its size, so the seed moves to 500ms, deliberately slower than any real cluster rate, which puts the seed at or after the answer and keeps the oldest slot probed inside the answer's own epoch. A compile-time assert pins the inequality the argument rests on. The forward step stays, because the local clock rather than the chain decides where the seed lands, so a clock running ahead can still overshoot. Resolve epoch boundaries with getBlocksWithLimit instead of a capped forward walk. The RPC answers "the first block at or after this slot" in one round trip, so the 128-slot cap and its bail are gone. A run of skipped slots past that cap is a settled fact, not a transient condition, so the cap turned a boundary that is perfectly readable into a permanent failure that no rerun could clear. Ok(None) now has exactly one meaning, that the epoch has not started; an endpoint reporting no block for an epoch already in the past is missing history for that range rather than describing an empty epoch, and answering None there would walk the search backward past the right answer. Carry the resolved bound across search steps. Stepping earlier turns the candidate's own start into the new candidate's upper bound and stepping later does the reverse, so only the bound on the far side of the move needs a lookup. The pre-loop upper-bound fetch stays, since splitting the decision across the call site to make it lazy would cost the pure function that the boundary tests exercise. Validate the snapshot before writing it. create_snapshot warns and continues when the leader schedule cannot be fetched, but validate treats a missing schedule as an error and every consumer rejects such a snapshot, so the command exited 0 having written an unusable file to the canonical filename and a snapshot then export-shapley chain reported step one green and died on step two. The warn-and-continue predates this branch; its reachability is what changed. Refs malbeclabs/infra#2317
|
All four addressed in 3c94525. Replies are on the individual threads; the short version:
Two things I did not change, both flagged in the thread replies rather than silently skipped:
One consequence of the
|
ben-dz
left a comment
There was a problem hiding this comment.
Two new issues in the rewritten try_epoch_start_block_time: the ensure! bounds the returned block only at the epoch boundary, so an interior block-history gap silently dates the epoch late (round 1's walk failed closed after 128 slots); and an epoch that has started but has no finalized block yet now errors rather than bounding nothing. Neither blocks, but the first turns on whether the production endpoint can have interior gaps.
Reject a first block more than 432 slots into the epoch. Last round I replaced the capped forward walk with getBlocksWithLimit and bounded the result only at the epoch boundary, which swapped a fail-closed cap for a fail-open one: the call steps over a gap in the endpoint's own block history without reporting it, so a gap of up to a full epoch passed silently, dated the epoch hours late, and resolved timestamps inside the gap to the previous epoch and its leader schedule. The walk this replaced failed closed after 128 slots, so the silent window was about 45 seconds rather than two days. Removing the cap was right for the reason it was removed, that a settled skip run past a work cap is an unrecoverable failure on a readable boundary, but those are two separate bounds and collapsing them was the mistake. The budget bounds trust rather than work: exceeding it costs no extra round trips and says the answer is not credible. It is min'd with the next epoch's first slot so a cluster with epochs shorter than the budget, such as a local test validator, cannot accept a block from a later epoch. Measure "has this epoch produced a block" against the chain tip slot rather than getSlot. getSlot can name a blockless slot, which is why try_chain_tip_block exists at all, so a just-started epoch whose opening slots were all skipped hit the empty-result error path and aborted the rewards run through demand.rs where the previous revision resolved to the candidate epoch. Since the chain tip slot has a block, an epoch starting at or before it must have one too, which makes the error's claim provable rather than assumed. Also stop retrying settled errors from getBlocksWithLimit. That call had no .when() predicate, so a pruned range slept through the whole backoff schedule before returning the same answer. Both retry sites now share is_settled_block_error. Refs malbeclabs/infra#2317
|
All three addressed in c67728d. Individual replies are on the threads. The interior-gap one was a regression I introduced last round, not a pre-existing gap, and it is worth naming precisely because the fix is not "put the cap back". Your round-1 point stands: a work cap is wrong, since a settled skip run past it is unrecoverable on a readable boundary. My mistake was treating that as the only bound in play. The distance from the boundary is also a trust bound, and dropping the sequential walk did not remove the need for it — it just made keeping it free, since The blockless-tip one was an internal inconsistency: I wrote The retry predicate is now shared as New pure-function tests cover the budget edge (exclusive), a short skip run, a 100k-slot history gap, and the short-epoch case where the Nothing else changed, and I am not aware of any finding from either round still outstanding. |
ben-dz
left a comment
There was a problem hiding this comment.
lgtm — I don't love having anywhere that the 350ms is hard coded, but it seems to be only left in solana-cli's NOMINAL_SLOT_DURATION (command/shreds/mod.rs:33), with two consumers, so its pretty narrow.
…tries and comments Prose only, no behavior change. Trims the changelog bullets to what changed and why, and cuts comments that restated the code or carried review-thread history rather than reasoning. Two comments were also factually stale after the getBlocksWithLimit change: both described a forward walk over skipped slots that no longer exists. Refs malbeclabs/infra#2317
bgm-malbeclabs
left a comment
There was a problem hiding this comment.
looks good - thanks for getting rid of some of the old cruft
…no leader schedule (#413) ## Summary of Changes * The scheduler no longer writes a snapshot it cannot use: a failed leader-schedule fetch propagates instead of becoming `leader_schedule: null`, and `create_epoch_snapshot` validates before saving. * A failed fetch was one `warn!`, then an unusable snapshot uploaded over the epoch's canonical S3 key; the tick then failed reading it back with `Missing leader schedule`, a symptom whose cause survived only in the log. #411 fixed this for the `snapshot` CLI command but not for the scheduler, which is the path that runs in production. * Scheduler failures now log the full cause chain (`{:#}`). Plain `{}` prints only the outermost context, so the newly propagated reason would still have been dropped. * **Security:** `EpochFinder`'s retry logs printed `reqwest`'s error verbatim, which includes the request URL. That URL carries the read endpoint's API key on mainnet-beta and journald ships to Loki, so those logs now strip it. This matters because the companion PR points mainnet-beta's reads at the keyed endpoint. * **Behavior change:** a `--dry-run` tick that previously marked the epoch processed with an unusable snapshot now fails and retries — nothing validated on that path, since `calculate_rewards` never ran. Both deployed environments run with dry-run off. ## Testing Verification * New `tests/test_snapshot_validate.rs` builds a `CompleteSnapshot` from the existing `testnet_snapshot.json` and `leader-schedule-epoch-89.json` fixtures: `validate()` is `Ok` on a complete snapshot, and names the right issue when the leader schedule is missing or empty.
…0ms slot times (malbeclabs/doublezero-offchain#411) ## Summary of Changes Five call sites divided wall clock by a hardcoded 400ms slot duration. They get three independent fixes rather than one shared helper, because they want different answers: the two accuracy-critical sites stop depending on a slot-duration constant at all, and the two that keep one now share a single value. * **`contributor-rewards`** — `find_epoch_at_timestamp` verifies its estimate against real block times. The old estimate drifted ~30k slots per day of lookback and no fixed constant survives the SIMD-0525 rollout. That epoch picks the leader schedule rewards are computed against, so the search errors rather than guessing. * **`solana-cli`** — the duplicate constants in `shreds pay` and `prepare-offchain-message` collapse into one 350ms `NOMINAL_SLOT_DURATION`, deliberately cluster-independent: a `~` prefixed estimate and a deadline slot the CLI and operator must both compute want reproducibility over accuracy. * **`validator-debt`** — deletes two dead timestamp-to-epoch paths and both constants. `rpc.rs` already does this mapping correctly for production; the deleted code had no callers and contained an unsigned subtraction that panicked in debug and wrapped in release. ### Before merging * **`--valid-for` windows lengthen.** `--valid-for 1h` is now 10,285 slots, not 9,000. Mainnet reaches 350ms at epoch 1020 (2026-08-21); until then the flag grants ~68 minutes. Testnet runs at 200ms, so ~34 minutes. The help text states the conversion rate. * **The demand path can now fail where it used to be wrong.** `ingestor/demand.rs` propagates a leader-schedule error, so a backfill older than the endpoint's ledger retention errors instead of silently mis-estimating. The snapshot paths warn and continue, but `snapshot` now validates before writing rather than leaving an unusable file behind. * **350ms needs one more bump** when SIMD-0525 finishes stepping mainnet to 200ms. Nothing enforces it; the constants' comments record the schedule. ## Testing Verification * Pure-function tests for the search's per-step decision (both bounds, the unbounded current-epoch case, the unstarted-epoch case), the absent-block classifier (each of the three shapes `getBlockTime` uses, plus a pruned ledger failing closed), and the epoch-boundary skip budget (exclusive edge, a history gap, and a short-epoch cluster). No RPC mock, and no new dependency. * CLI assertions recomputed by hand from the 350ms derivation, with the arithmetic in each test comment. The deletions are checked by `clippy -Dwarnings`, which catches every orphaned import and unused constant.
…no leader schedule (malbeclabs/doublezero-offchain#413) ## Summary of Changes * The scheduler no longer writes a snapshot it cannot use: a failed leader-schedule fetch propagates instead of becoming `leader_schedule: null`, and `create_epoch_snapshot` validates before saving. * A failed fetch was one `warn!`, then an unusable snapshot uploaded over the epoch's canonical S3 key; the tick then failed reading it back with `Missing leader schedule`, a symptom whose cause survived only in the log. malbeclabs/doublezero-offchain#411 fixed this for the `snapshot` CLI command but not for the scheduler, which is the path that runs in production. * Scheduler failures now log the full cause chain (`{:#}`). Plain `{}` prints only the outermost context, so the newly propagated reason would still have been dropped. * **Security:** `EpochFinder`'s retry logs printed `reqwest`'s error verbatim, which includes the request URL. That URL carries the read endpoint's API key on mainnet-beta and journald ships to Loki, so those logs now strip it. This matters because the companion PR points mainnet-beta's reads at the keyed endpoint. * **Behavior change:** a `--dry-run` tick that previously marked the epoch processed with an unusable snapshot now fails and retries — nothing validated on that path, since `calculate_rewards` never ran. Both deployed environments run with dry-run off. ## Testing Verification * New `tests/test_snapshot_validate.rs` builds a `CompleteSnapshot` from the existing `testnet_snapshot.json` and `leader-schedule-epoch-89.json` fixtures: `validate()` is `Ok` on a complete snapshot, and names the right issue when the leader schedule is missing or empty.
Summary of Changes
Five call sites divided wall clock by a hardcoded 400ms slot duration. They get three independent fixes rather than one shared helper, because they want different answers: the two accuracy-critical sites stop depending on a slot-duration constant at all, and the two that keep one now share a single value.
contributor-rewards—find_epoch_at_timestampverifies its estimate against real block times. The old estimate drifted ~30k slots per day of lookback and no fixed constant survives the SIMD-0525 rollout. That epoch picks the leader schedule rewards are computed against, so the search errors rather than guessing.solana-cli— the duplicate constants inshreds payandprepare-offchain-messagecollapse into one 350msNOMINAL_SLOT_DURATION, deliberately cluster-independent: a~prefixed estimate and a deadline slot the CLI and operator must both compute want reproducibility over accuracy.validator-debt— deletes two dead timestamp-to-epoch paths and both constants.rpc.rsalready does this mapping correctly for production; the deleted code had no callers and contained an unsigned subtraction that panicked in debug and wrapped in release.Fixes malbeclabs/infra#2317
Before merging
--valid-forwindows lengthen.--valid-for 1his now 10,285 slots, not 9,000. Mainnet reaches 350ms at epoch 1020 (2026-08-21); until then the flag grants ~68 minutes. Testnet runs at 200ms, so ~34 minutes. The help text states the conversion rate.ingestor/demand.rspropagates a leader-schedule error, so a backfill older than the endpoint's ledger retention errors instead of silently mis-estimating. The snapshot paths warn and continue, butsnapshotnow validates before writing rather than leaving an unusable file behind.Follow-ups to file
doublezero-solana-client-tools. Two now exist and each names the other in a comment. Unify once the new one has run a few epochs in production.estimate_block_time_for_skipped_slotinvalidator-debt/src/rpc.rsstill assumes 0.4s per slot implicitly — the last live 400ms assumption in the repo.SolanaClientErroris logged with{:?}in five places iningestor/epoch.rs; aReqwestvariant renders the request URL, which usually carries an?api-key=. Pre-existing at four, so it wants one pass over all five.Testing Verification
getBlockTimeuses, plus a pruned ledger failing closed), and the epoch-boundary skip budget (exclusive edge, a history gap, and a short-epoch cluster). No RPC mock, and no new dependency.clippy -Dwarnings, which catches every orphaned import and unused constant.prepare-offchain-message --valid-for 1h --jsonreporting adeadline_slotexactly 10,285 above current, and a snapshot near an epoch boundary resolving to the epoch its boundary block times imply. Both need mainnet after epoch 1020.