chainer: cleanup abandon references - #11348
Open
emwang-jump wants to merge 2 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Version processing order must be corrected or made order-independent before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Removes chainer abandonment of turbine slot versions, keeping turbine and certificate versions live while replay deduplicates deliveries.
Changes:
- Removes abandonment state and processing.
- Delivers shared FECs across both versions.
- Updates documentation and duplicate-stream tests.
File summaries
| File | Review |
|---|---|
src/discof/rotor/test_rotor_tile.c |
Updates duplicate-delivery integration expectations. |
src/discof/rotor/fd_rotor_tile.h |
Documents replay deduplication. |
src/discof/chainer/test_chainer.c |
Updates chainer expectations and ordering assertions. |
src/discof/chainer/fd_chainer.h |
Updates APIs and documentation; fix “slots fec” to “slot’s FEC.” |
src/discof/chainer/fd_chainer.c |
Removes abandonment logic, but map insertion order causes certificate versions to be processed before turbine versions, failing the new ordering assertions. |
Review details
Suppressed comments (3)
src/discof/chainer/fd_chainer.c:638
- Advancing every same-ID version can enqueue identical descendants multiple times in one BFS. When an ancestor activates two completed versions of block P, each P version scans all slot versions whose
parent_block_idis P and pushes the same children; with N children the queue can reach 2N entries although it is sized for only the number of live slot versions. This can tripbfs_push_tail's full-deque CRIT for a sufficiently wide fork. Deduplicate per traversal or let only one version per{slot, block_id}perform the descendant cascade.
/* all versions advance */
chainer_advance( chainer, slotv );
src/discof/chainer/fd_chainer.c:638
- This now emits two live delivery streams for one logical block, but the added rotor tests stop at the replay-message log. Replay’s current re-key path (
fd_replay_tile.c:3781-3795) was written under the opposite invariant and removes an existing{slot, block_id}entry as stale; existing replay tests cover stale or already-completed entries, not two simultaneously live banks. Add a replay-tile test that creates both{slot, 0}and{slot, block_id}banks, exercises both completion orders, and verifies deduplication plus cleanup of the losing bank.
/* all versions advance */
chainer_advance( chainer, slotv );
src/discof/chainer/fd_chainer.h:375
- Use the possessive “slot’s” and capitalize the initialism “FEC”; the current sentence is grammatically incorrect.
updating information on what a slots fec set count, parent slot, and
parent block id are. This mirrors the Alpenglow repair type
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
emwang-jump
force-pushed
the
emwang/chainer-fix
branch
from
September 8, 2026 15:50
dd34907 to
033ae9c
Compare
┌─ ⚡ PERF · 16f7825 vs main@07934f1 ─────────────────────────────────
│ SUITE BASELINE NEW Δ
│ replay tps, mainnet 26,763 tps 26,749 tps · -0.05%
│ bench tps, localnet 128,645 tps 128,646 tps · 0.00%
│ snapshot load, testnet 15.01 s 14.92 s · -0.59%
│ mem total, mainnet 171.43 GiB 171.43 GiB · 0.00%
│ mem total, testnet 102.31 GiB 102.31 GiB · 0.00%
│ clean compile, firedancer 357.5 cpu·s 356.6 cpu·s · -0.25%
│ binary size, firedancer 85.81 MB 85.79 MB · -0.02%
├─────────────────────────────────────────────────────────────────────
@@ 0 REGRESSIONS · 0 WARNINGS · 0 IMPROVED · 7 NOISE @@
└─────────────────────────────────────────────────────────────────────history · 2 pushes ┌─ HISTORY · Δ vs main, per push, newest first ─────────────────────────
│ HEAD TPS BENCH SNAP MEM·M MEM·T COMPILE BINARY
│ 16f7825 -0.05% 0.00% -0.59% 0.00% 0.00% -0.25% -0.02%
!│ 033ae9c +0.16% 0.00% +0.34% 0.00% 0.00% +2.09% -0.02%
└─────────────────────────────────────────────────────────────────────── |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The turbine-first ordering regression must be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/discof/chainer/fd_chainer.h:388
- The possessive is missing, and “what … are” makes this public API description grammatically incorrect. Describe the fields directly.
updating information on what a slots fec set count, parent slot, and
parent block id are. This mirrors the Alpenglow repair type
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
Comment on lines
+629
to
+633
| for( ulong _i=slotv_iter_init( chainer, slot ); | ||
| _i!=ULONG_MAX; | ||
| _i=slotv_iter_next( chainer, _i ) ) { | ||
| fd_chainer_slotv_t * slotv = slotv_iter_ele( chainer, _i ); | ||
| if( FD_LIKELY( slotv->turbine && !slotv->abandoned ) ) { turbine = slotv; break; } | ||
| } | ||
| if( FD_UNLIKELY( fd_chainer_slotv_fecs( chainer, slotv )[ k ]!=fec_idx ) ) continue; |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Replay-level race coverage is missing, and related contracts and comments need correction.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
src/discof/chainer/fd_chainer.h:387
- Use the possessive form “slot's.”
src/discof/chainer/fd_chainer.h:57
- This overstates replay's deduplication: as
fd_rotor_tile.hnow explains, replay allocates both banks and executes both prefixes; it only skips the second slot-complete delivery. Please describe that narrower guarantee so callers do not assume duplicate FEC work is eliminated.
the same value the votor version learned from the cert. Replay owns
the deduplication of that second stream (see fd_rotor_tile.h), and
fd_chainer_publish prunes the non-canonical duplicate when the slot
roots.
src/discof/rotor/test_rotor_tile.c:1441
- The claimed delivery order is reversed in this scenario. The turbine version is created first and the cert version second, while the MAP_MULTI chain iterates newest-first, so the cert copy is queued first. The assertions deliberately avoid requiring an order later in this test; make this overview do the same.
fill the shared FEC, and the final set is delivered once per version,
turbine first (its block_id finalizes to the cert's), then the cert
version. Replay dedups the second stream (see fd_rotor_tile.h). */
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Balanced
Comment on lines
+650
to
651
| /* all versions advance */ | ||
| chainer_advance( chainer, slotv ); |
Comment on lines
+51
to
+56
| Replay handles the second stream. The verified copy's FEC 0 misses | ||
| the turbine bank (keyed {slot, 0}) and allocates its own bank keyed | ||
| {slot, A'}, and its mid-slot FECs are ingested into it. Whichever | ||
| copy completes first re-keys onto {slot, A'}, unlinking the other | ||
| copy's map entry; the other copy's slot-complete FEC then finds the | ||
| completed bank and is skipped. The loser is left an incomplete |
Comment on lines
+51
to
+56
| Replay handles the second stream. The verified copy's FEC 0 misses | ||
| the turbine bank (keyed {slot, 0}) and allocates its own bank keyed | ||
| {slot, A'}, and its mid-slot FECs are ingested into it. Whichever | ||
| copy completes first re-keys onto {slot, A'}, unlinking the other | ||
| copy's map entry; the other copy's slot-complete FEC then finds the | ||
| completed bank and is skipped. The loser is left an incomplete |
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.
No description provided.