test(early-kickout): integration branch stacking the four open PRs - #16121
Draft
stedfn wants to merge 42 commits into
Draft
test(early-kickout): integration branch stacking the four open PRs#16121stedfn wants to merge 42 commits into
stedfn wants to merge 42 commits into
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t-pr-6 # Conflicts: # CHANGELOG.md # chain/chain/src/store/mod.rs # chain/epoch-manager/src/epoch_info_aggregator.rs # chain/epoch-manager/src/lib.rs # test-loop-tests/src/tests/chunk_producers.rs
…sert Follow-up to the master merge, resolving the debated untrustworthy-sampling concern for the blacklist-aware writer. - seed_chunk_producer_rows: debug_assert that the blacklist-aware sampler yields a producer for every shard that has one. The safety valve in compute_chunk_producer_blacklist guarantees this; the assert enforces the coupling (a None only means an empty shard settlement, nothing to seed). - nonempty_blacklist_anchor_always_has_row: proves the missing-row region and the non-empty-blacklist region are disjoint, so the aggregator's lenient reader never height-samples (which would re-credit a blacklisted producer) while a blacklist is active. - seeded_rows_match_blacklist_aware_sampler: the seeded ChunkProducers row equals the plain height sampler while the blacklist is empty, and equals the blacklist-aware sampler (never the down node) once it is non-empty; the strict consensus reader returns that same row. Chose a writer-side assert over a reader-side one: at the aggregator miss site the per-shard stats are not in scope, and the partial aggregator there reflects a different point in the backward walk, so asserting against it would check the wrong time slice. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t-pr-6 #15841 (PR5) landed on master independently; resolve the duplicate by taking master for the pure-PR5 files (epoch_info.rs adopts master's bounds-safe sampler, adapter.rs) and keeping the branch's PR6 payload (lib.rs seed rewrite, early_kickout.rs tests). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
test_finalize_epoch_large_epoch_length asserts the incremental aggregator visits each block exactly once (epoch_info_aggregator_loop_counter == N). With EarlyKickout active (nightly/spice builds), seed_chunk_producers reads get_epoch_info_aggregator_upto_last once per record_block_info, adding bounded extra walk iterations, so the exact count only holds with the feature compiled out. Gate the assertion with #[cfg(not(feature = "nightly"))] (the complement of the seed body's own cfg); the caching invariant stays covered on the stable build and the kickout seed path is covered by the early_kickout tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Remove stacked-PR references (#15908/PR5/PR6) and narration that just restates the code from the comments this branch added: revert the compute_chunk_producer_blacklist comment churn to master's wording, drop the seed_chunk_producers reader/activation paragraphs (readers and gating are visible in the code), and reword the test module doc. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…up blacklist helper Review response for the mid-epoch chunk-producer kickout writer: - Observability: emit a nightly counter (near_early_kickout_chunk_producer_reassigned_total, by shard) + info log when the seeder reassigns a slot away from a blacklisted producer. The kick detector is "the plain scheduled producer is blacklisted", not "plain != excluded" (the excluding sampler renormalizes, so that would false-fire). - DRY: extract blacklist_for_epoch, shared by the seeder and the get_chunk_producer_blacklist accessor, removing the duplicated epoch-reset check. - Tests: 2-shard per-shard blacklist isolation (+ seeding + metric delta), and a finality-stall regression guard pinning the per-block aggregator-walk cost. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cspell (CI spellcheck) flagged 'unfinalized' in the stall-test comments; it is not in the project dictionary. Reword to 'not-yet-finalized'. Comment-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add early_kickout_e2e.rs (nightly + test_features): - test_early_kickout_reassignment: induce a chunk producer below 80% via the adversarial StopProduce message; assert the offending slot is reassigned (DB-backed resolver returns a different validator for the target's own slots once the grandparent anchor blacklists it) while the shard keeps producing chunks (liveness). Condition-based waits, topology avoids the all-blacklisted safety valve. - test_early_kickout_epoch_sync_bootstrap: a fresh node epoch-syncs INTO a network that already has an active reassignment; assert the seeded first-block rows exist and match the source (no ChunkProducerNotInDB), then that after the node lives through a fresh epoch it reproduces the reassignment and agrees with the source (no synced-vs-source divergence). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Spice decouples chunk execution from block production, so the fresh-node epoch-sync path the test drives does not converge within the deadline. The reassignment test needs no epoch sync and stays enabled under spice. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the drop-shard safety valve in compute_chunk_producer_blacklist
with keep-exactly-one-least-bad: when all distinct producers on a shard
would be blacklisted, retain the highest produced/expected producer via a
deterministic total order (u128 cross-multiplication, tiebreak fewer
expected, then lower validator_id) so the worst offenders are still
reassigned away from while the shard keeps producing.
Both compute_chunk_producer_blacklist and blacklist_for_epoch return
ChunkProducerBlacklist { blacklist, shard_stats }. The seeder is the single
owner of the new observability; the accessor is a pure read. New metrics
(safety_valve_fired counter, blacklist_size gauge) are nightly-gated.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The record-block seeder computed the chunk-producer blacklist from the aggregator walked up to the unfinalized anchor. During a finality stall that rescans the growing not-yet-finalized suffix every block, an O(stall-depth) per-block walk under the epoch-manager write lock. Base the blacklist on the anchor's last-final block instead (BlockInfo::last_final_block_hash / last_finalized_height). This is copied verbatim from the block header, so it is identical across nodes for a given canonical anchor - required because the seeded row is read on the strict consensus path (get_chunk_producer_info_anchored) with no recompute. It also bounds the walk: the aggregator's sync point already sits at the last-final block, so the walk covers only blocks finalized since the previous record (~1 in steady state, 0 during a stall) and never the unfinalized suffix. Gate the start-of-epoch grace window on the last-final block height too, matching the blacklist basis so the seeder is internally consistent. Update the get_chunk_producer_blacklist accessor to the same basis, and the epoch-manager tests: model 2-block finality in the block helper, shift the grace-boundary anchors, and retighten the finality-stall walk guard from O(depth^2) to constant-per-block across two stall depths. Stale/old-fork anchors whose last-final is an ancestor of the cached sync point still walk (rare, non-canonical, bounded by epoch length); a protocol-fixed budget cap for that case is left as a follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t-e2e-tests # Conflicts: # chain/epoch-manager/src/lib.rs # chain/epoch-manager/src/metrics.rs # chain/epoch-manager/src/tests/early_kickout.rs # chain/epoch-manager/src/tests/mod.rs
…t/least-bad-valve-15843 # Conflicts: # chain/epoch-manager/src/adapter.rs # chain/epoch-manager/src/lib.rs # chain/epoch-manager/src/metrics.rs # chain/epoch-manager/src/tests/early_kickout.rs # chain/epoch-manager/src/tests/mod.rs
… blacklist_for_epoch)
Tighten wordy comments and drop one redundant inline note in the keep-one blacklist path. Comments only, no code change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drop an internal plan-label prefix from one test comment. Comments only, no code change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drop a roadmap clause from the module doc; keep the technical meaning (a red run is a real gap the tests guard against). Comments only, no code change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Ports the 3 net-new blacklist-math tests from #16037 onto the keep-one safety-valve branch (#16060), plus their two helpers (epoch_info_for_layout, drive_fork). #16037 sat on the dead least-bad-safety-valve lineage and re-carried ~1134 lines already on master and #16060; only these tests were net-new. This supersedes it. The two math/sampler tests stay plain #[test] (not nightly-gated) so the default CI job keeps covering them. The fork test is strengthened over the #16037 original: it now snapshots the canonical anchor's raw aggregator stats before driving the abandoned fork and asserts they are unchanged after, proving no cross-fork leak at the raw level rather than relying on the post-valve blacklist (which the keep-one valve could otherwise mask). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The changelog line will be added on stabilization, not on the nightly-gated feature PRs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Re-separate the gas-keys and promise-yield bullets that the previous changelog-drop commit accidentally merged onto one line. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t-seed-walk-cost # Conflicts: # chain/epoch-manager/src/adapter.rs # chain/epoch-manager/src/lib.rs # chain/epoch-manager/src/metrics.rs # chain/epoch-manager/src/tests/early_kickout.rs # chain/epoch-manager/src/tests/mod.rs
Reduce the merged seed walk-cost comments to non-obvious rationale only, reword `unfinalized` -> `not-yet-final` and `rescan` -> `re-walk` to satisfy cspell, and drop the CHANGELOG entry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses review nit: the fn had grown to 9 params, four of which were two same-typed adjacent pairs. Bundles them into SeedAnchor and SampleEpoch. No behavior change. Co-Authored-By: Claude <noreply@anthropic.com>
Drops six comments that restate the code or claim something the compiler already enforces. Fixes three that were wrong: the seed_chunk_producers doc called the sample epoch "the epoch after the anchor" (it is the anchor's own epoch except at a boundary), a comment named a parameter the previous commit removed, and the adapter mirror warning claimed a fork that cannot happen while the accessor has no non-test callers. Co-Authored-By: Claude <noreply@anthropic.com>
…ost' into stedfn/early-kickout-integration
…ath-tests-v2' into stedfn/early-kickout-integration # Conflicts: # chain/epoch-manager/src/adapter.rs # chain/epoch-manager/src/lib.rs # chain/epoch-manager/src/metrics.rs # chain/epoch-manager/src/tests/early_kickout.rs # chain/epoch-manager/src/tests/mod.rs
…into stedfn/early-kickout-integration # Conflicts: # CHANGELOG.md # chain/epoch-manager/src/tests/early_kickout.rs # test-loop-tests/src/tests/mod.rs
… into stedfn/early-kickout-integration
…GC probe Integration-branch fixes for the stacked early-kickout PRs. near-epoch-manager gains a `test_features` feature exposing `set_early_kickout_thresholds_for_testing`, which lowers the miss floor and the start-of-epoch grace for the calling thread. Production builds have no `test_features` and read the plain constants, so the seam is a no-op there. The override is thread-local because a test-loop chain runs entirely on the thread that drives its event loop, while cargo runs test functions on parallel threads; a process global would leak a lowered grace into unrelated tests. The e2e tests use the seam so the gate trips in tens of blocks instead of ~1100, and now reach the epoch-sync horizon the sync tests use (the previous depth made the epoch-sync proof invalid and silently degraded the test into a header-sync-from-genesis run). The sync test's anchor probe now asserts it stayed above the GC tail, so its green cannot be read as garbage-collection coverage.
The guard restores a thread-local, so moving it to another thread would make Drop restore that thread's thresholds and leave the installing thread lowered for every test the harness later runs on it. A PhantomData<*const ()> field makes the compiler reject the move. Found by codex review.
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
1 participant
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.
Do not merge. This is a throwaway validation branch, not a landing PR. It exists so CI runs the full suite against the complete early-kickout feature at once. Each source PR still lands separately on its own base.
Stacked on master (f810085):
stedfn/early-kickout-seed-walk-cost- last-final blacklist basisstedfn/early-kickout/blacklist-math-tests-v2- blacklist-math unit testsstefan/early-kickout-e2e-tests- e2e reassignment and epoch-syncstedfn/early-kickout-sync-tests- state-sync and early-kickoutMost of #16086 had already landed on master. Only three tests were still new: resharding shard-id mapping, empty-exclusion sampler equivalence, and fork isolation. Those were ported. Its old 20/50 threshold assertions were dropped.
Fixes made here, to be back-ported
test_features-gated threshold override to the epoch manager. Production blacklists a producer at 100 misses past a 1000-block grace. That is about 1100 blocks, which a test-loop chain cannot run. The override lowers both so the gate trips in tens of blocks. A build withouttest_featuresreads the plain constants, so production behaviour is unchanged. The override is thread-local: a test-loop chain runs entirely on one thread, while cargo runs test functions on parallel threads, so a process global would leak a lowered grace into unrelated tests.Results
near-epoch-managerearly_kickoutnear-chainchunk_producerscargo fmtandclippy --all-features --all-targetsare clean.The failing test found a real bug
test_early_kickout_epoch_sync_bootstrapfails on a genuine divergence, not a test bug.A node that epoch-syncs and then header-syncs credits the stopped producer with 71 chunks it never made. The source node, which processed every block, records 0 produced out of 19 expected for the same validator in the same epoch. Wrong chunk stats change rewards, so the syncing node computes a different next-epoch block producer hash and rejects every header batch with
InvalidNextBPHash. It never joins.Cause:
anchored_chunk_producers_for_aggregatorfalls back to the plain height sampler when aDBCol::ChunkProducersrow is missing. That fallback is only correct while the blacklist is empty, which its own comment says.Two passing tests bound the problem.
test_early_kickout_far_horizon_observeruses the same sync path with no active blacklist.test_early_kickout_reassignmenthas an active blacklist but no sync.The lowered test thresholds are not the cause. Production values only delay the blacklist by about 1100 blocks.