forest: tighten sentinel block conditions#9004
Merged
emwang-jump merged 1 commit intomainfrom Apr 6, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens the conditions under which fd_forest_blk_insert() will treat an existing block as a “sentinel” and update its parent_slot, aiming to avoid unintended parent rewrites for non-sentinel blocks in the forest data structure.
Changes:
- Hoists forest map/pool pointer acquisition to the top of
fd_forest_blk_insert(). - Narrows the parent update path to only trigger when the existing element is a sentinel (
slot == parent_slot). - Adds an internal invariant check before removing the sentinel from
subtrees/orphaned.
Performance Measurements ⏳
|
Performance Measurements ⏳
|
Performance Measurements ⏳
|
emwang-jump
force-pushed
the
emwang/sentinel-blk-fix
branch
from
f20657b to
b8dd577
Compare
Performance Measurements ⏳
|
lidatong
reviewed
Mar 24, 2026
Comment on lines
+931
to
+942
| fd_forest_blk_t * ele = query( forest, slot ); | ||
| if( FD_LIKELY( ele ) ) { | ||
| /* potentially may need to update the parent_slot, if this | ||
| this was a sentinel block that was created for a confirmed msg. | ||
| This update is only allowed once */ | ||
| if( FD_UNLIKELY( ele->slot == ele->parent_slot && ele->parent_slot != parent_slot ) ) { | ||
| ele->parent_slot = parent_slot; | ||
| FD_TEST( fd_forest_subtrees_ele_query( subtrees, &slot, NULL, pool ) || fd_forest_orphaned_ele_query( orphaned, &slot, NULL, pool ) ); | ||
| subtrees_orphaned_remove( forest, slot ); // if this is a sentinel block, then it must be orphaned | ||
| } else { | ||
| return ele; | ||
| } |
Member
There was a problem hiding this comment.
this needs a lot more docs... i assume there's some assumptions around these comparisons, if someone sends a shred with a different parent_off vs. tower giving a sentinel confirmation?
| slot. Returns the inserted forest ele. */ | ||
| slot >= forest->root. blk_insert can also be called to create a | ||
| sentinel block, i.e. a placeholder block that we know exists but | ||
| don't know the parent slot of. The caller should pass in parent_slot |
Member
There was a problem hiding this comment.
this should use ULONG_MAX, not its own slot
Comment on lines
+729
to
+733
| equal to the slot. In this case, the block inserted will remain an | ||
| orphan/subtree at least until the next blk_insert is called with a | ||
| different parent_slot, after which point no more updates to the | ||
| parent_slot will be allowed. For non-sentinel blocks, blk insert is | ||
| idempotent, and can be called multiple times with the same slot. |
Member
There was a problem hiding this comment.
Suggested change
| equal to the slot. In this case, the block inserted will remain an | |
| orphan/subtree at least until the next blk_insert is called with a | |
| different parent_slot, after which point no more updates to the | |
| parent_slot will be allowed. For non-sentinel blocks, blk insert is | |
| idempotent, and can be called multiple times with the same slot. | |
| equal to the slot. In this case, the block inserted will always be | |
| orphaned (there can never exist a blk with slot ULONG_MAX) | |
| When blk_insert is called for the same slot with an actual parent_slot, the blk's parent_slot will be updated. Note that parent_slot can otherwise not be updated to the | |
| parent_slot will be allowed. blk_insert is | |
| idempotent, and can be called multiple times with the same slot. |
Performance Measurements ⏳
|
Performance Measurements ⏳
|
Comment on lines
+836
to
+837
| /* re-trigger continuation of chained merkle verification if this FEC | ||
| set enables it */ | ||
| set enables it TODOOO MOVE TO AFTER_SHRED? */ |
There was a problem hiding this comment.
The updated block comment includes a stray/informal "TODOOO" note. Please either remove this from production code or change it to a standard "TODO:" with a clear action/owner, and keep the comment wrapped consistently with surrounding style.
| if( FD_UNLIKELY( ele->parent_slot == ULONG_MAX && parent_slot != ULONG_MAX ) ) { | ||
| ele->parent_slot = parent_slot; | ||
| FD_TEST( fd_forest_subtrees_ele_query( subtrees, &slot, NULL, pool ) || fd_forest_orphaned_ele_query( orphaned, &slot, NULL, pool ) ); | ||
| subtrees_orphaned_remove( forest, slot ); // if this is a sentinel block, then it must be orphaned |
There was a problem hiding this comment.
This inline comment is inaccurate: sentinel blocks created with parent_slot==ULONG_MAX are inserted into subtrees, not necessarily orphaned. Since the code asserts the element is in either subtrees or orphaned, the comment should be updated to match reality to avoid confusion during future maintenance/debugging.
Suggested change
| subtrees_orphaned_remove( forest, slot ); // if this is a sentinel block, then it must be orphaned | |
| subtrees_orphaned_remove( forest, slot ); /* if this is a sentinel block, it must be present in | |
| either subtrees or orphaned (as asserted above) */ |
Comment on lines
+1074
to
+1099
| fd_forest_blk_t * new_parent = fd_forest_query( forest, parent_slot ); | ||
| remove_and_unlink( forest, ele ); | ||
|
|
||
| /* insert ele to correct map */ | ||
| if( FD_UNLIKELY( fd_forest_frontier_ele_remove( frontier, &parent_slot, NULL, pool ) ) ) { | ||
| fd_forest_ancestry_ele_insert( ancestry, new_parent, pool ); | ||
| if( ele->child != ULONG_MAX ) fd_forest_ancestry_ele_insert( ancestry, ele, pool ); | ||
| else fd_forest_frontier_ele_insert( frontier, ele, pool ); | ||
| } else if( FD_LIKELY( fd_forest_ancestry_ele_query( ancestry, &parent_slot, NULL, pool ) ) ) { | ||
| if( ele->child != ULONG_MAX ) fd_forest_ancestry_ele_insert( ancestry, ele, pool ); | ||
| else fd_forest_frontier_ele_insert( frontier, ele, pool ); | ||
| } else if( FD_UNLIKELY( fd_forest_orphaned_ele_query( orphaned, &parent_slot, NULL, pool ) || | ||
| fd_forest_subtrees_ele_query( subtrees, &parent_slot, NULL, pool ) ) ) { | ||
| fd_forest_orphaned_ele_insert( orphaned, ele, pool ); | ||
| } else { | ||
| ele->parent = fd_forest_pool_idx_null( pool ); | ||
| fd_forest_subtrees_ele_insert( subtrees, ele, pool ); | ||
| fd_forest_subtlist_ele_push_tail( fd_forest_subtlist( forest ), ele, pool ); | ||
| requests_insert( forest, fd_forest_orphreqs( forest ), fd_forest_orphlist( forest ), fd_forest_pool_idx( pool, ele ) ); | ||
| } | ||
|
|
||
| if( FD_LIKELY( new_parent ) ) { | ||
| ele->parent = fd_forest_pool_idx( pool, new_parent ); | ||
| ele->parent_slot = parent_slot; /* update parent slot */ | ||
| link( forest, new_parent, ele ); | ||
| } |
There was a problem hiding this comment.
parent_update() calls remove_and_unlink(), which does not clear ele->sibling. If ele previously had a non-null sibling in its old parent's child list, that stale sibling pointer will carry over when ele is linked under new_parent, corrupting the new parent's child/sibling chain. Ensure ele->sibling (and any other linkage fields that must be fresh) is reset to fd_forest_pool_idx_null(pool) before relinking.
Comment on lines
+1116
to
+1121
| /* insert child */ | ||
| if( FD_LIKELY( fd_forest_ancestry_ele_query( ancestry, &head->parent_slot, NULL, pool ) ) ) { | ||
| if( head->child != ULONG_MAX ) fd_forest_ancestry_ele_insert( ancestry, head, pool ); | ||
| else fd_forest_frontier_ele_insert( frontier, head, pool ); | ||
| } else { | ||
| fd_forest_orphaned_ele_insert( orphaned, head, pool ); |
There was a problem hiding this comment.
parent_update() removes ele from consumed and from the appropriate requests map/list via remove_and_unlink(), but it never re-inserts ele into the correct requests/consumed structures after changing its parent. Additionally, the BFS loop removes descendants from the forest maps but does not remove/migrate them between the main vs orphan request lists. This can silently drop repair scheduling for affected slots (or leave stale entries in the wrong list). After reparenting, rebuild/migrate request-list and consumed state for ele (and any moved descendants) to match their new connectivity.
Suggested change
| /* insert child */ | |
| if( FD_LIKELY( fd_forest_ancestry_ele_query( ancestry, &head->parent_slot, NULL, pool ) ) ) { | |
| if( head->child != ULONG_MAX ) fd_forest_ancestry_ele_insert( ancestry, head, pool ); | |
| else fd_forest_frontier_ele_insert( frontier, head, pool ); | |
| } else { | |
| fd_forest_orphaned_ele_insert( orphaned, head, pool ); | |
| /* insert child and update requests to match new connectivity */ | |
| if( FD_LIKELY( fd_forest_ancestry_ele_query( ancestry, &head->parent_slot, NULL, pool ) ) ) { | |
| if( head->child != ULONG_MAX ) fd_forest_ancestry_ele_insert( ancestry, head, pool ); | |
| else fd_forest_frontier_ele_insert( frontier, head, pool ); | |
| /* node is now connected to main ancestry; ensure it's in main requests */ | |
| requests_insert( forest, | |
| fd_forest_requests( forest ), | |
| fd_forest_reqlist( forest ), | |
| fd_forest_pool_idx( pool, head ) ); | |
| } else { | |
| fd_forest_orphaned_ele_insert( orphaned, head, pool ); | |
| /* node is now orphaned; ensure it's in orphan requests */ | |
| requests_insert( forest, | |
| fd_forest_orphreqs( forest ), | |
| fd_forest_orphlist( forest ), | |
| fd_forest_pool_idx( pool, head ) ); |
emwang-jump
force-pushed
the
emwang/sentinel-blk-fix
branch
from
fcea5c7 to
3e50a57
Compare
Performance Measurements ⏳
|
emwang-jump
force-pushed
the
emwang/sentinel-blk-fix
branch
from
3e50a57 to
9ab86aa
Compare
Performance Measurements ⏳
|
Performance Measurements ⏳
|
lidatong
previously approved these changes
Apr 6, 2026
Member
lidatong
left a comment
lidatong
left a comment
There was a problem hiding this comment.
good catch on the bug and the fix itself seems correct (the test seems to exercise it properly)
big question to ask is how can we simplify forest, specifically by deleting code? one way is to look at it from the highest-level and define what are the primitives (eg. is there a need for a lot of one-off helpers like remove_and_unlink, or can it be defined using tree / forest combinators?).
for example, the BFS traversal to orphan the tree's descendants is kinda the opposite of what happens in insert when it pushes the connected outwards
also, i was thinking about it, and it might be out of place for consumed to be tracked by forest not policy and similarly requests -> inflight
probably a revisit of all this after the release cut
Comment on lines
+1065
to
+1068
| /* Updates the parent slot of a block, and updates the map state for the | ||
| old parent, new parent, and children of the block. If the block has | ||
| children, the children are "moved" with the block to the new fork. | ||
| incredibly annoying. */ |
Member
There was a problem hiding this comment.
Suggested change
| /* Updates the parent slot of a block, and updates the map state for the | |
| old parent, new parent, and children of the block. If the block has | |
| children, the children are "moved" with the block to the new fork. | |
| incredibly annoying. */ | |
| /* Updates a forest_blk_t's parent, which requires updates to the blk itself, the blk's old parent, the new parent, and all its descendants. */ |
Comment on lines
+1104
to
+1112
| if( FD_UNLIKELY( blk == ele ) ) continue; | ||
| if( FD_UNLIKELY( fd_forest_pool_ele( pool, blk->parent ) == ele ) ) { | ||
| blk->parent = fd_forest_pool_idx_null( pool ); | ||
| fd_forest_subtrees_ele_insert( subtrees, blk, pool ); | ||
| fd_forest_subtlist_ele_push_tail( fd_forest_subtlist( forest ), blk, pool ); | ||
| requests_insert( forest, fd_forest_orphreqs( forest ), fd_forest_orphlist( forest ), fd_forest_pool_idx( pool, blk ) ); | ||
| } else { | ||
| fd_forest_orphaned_ele_insert( orphaned, blk, pool ); | ||
| } |
Member
There was a problem hiding this comment.
need to document each of these
Suggested change
| if( FD_UNLIKELY( blk == ele ) ) continue; | |
| if( FD_UNLIKELY( fd_forest_pool_ele( pool, blk->parent ) == ele ) ) { | |
| blk->parent = fd_forest_pool_idx_null( pool ); | |
| fd_forest_subtrees_ele_insert( subtrees, blk, pool ); | |
| fd_forest_subtlist_ele_push_tail( fd_forest_subtlist( forest ), blk, pool ); | |
| requests_insert( forest, fd_forest_orphreqs( forest ), fd_forest_orphlist( forest ), fd_forest_pool_idx( pool, blk ) ); | |
| } else { | |
| fd_forest_orphaned_ele_insert( orphaned, blk, pool ); | |
| } | |
| /* this is blk itself, ... */ | |
| if( FD_UNLIKELY( blk == ele ) ) continue; | |
| /* this is the direct child, ... */ | |
| else if( FD_UNLIKELY( fd_forest_pool_ele( pool, blk->parent ) == ele ) ) { | |
| blk->parent = fd_forest_pool_idx_null( pool ); | |
| fd_forest_subtrees_ele_insert( subtrees, blk, pool ); | |
| fd_forest_subtlist_ele_push_tail( fd_forest_subtlist( forest ), blk, pool ); | |
| requests_insert( forest, fd_forest_orphreqs( forest ), fd_forest_orphlist( forest ), fd_forest_pool_idx( pool, blk ) ); | |
| /* this is a descendant, ... */ | |
| } else { | |
| fd_forest_orphaned_ele_insert( orphaned, blk, pool ); | |
| } |
|
|
||
| /* re-trigger continuation of chained merkle verification if this FEC | ||
| set enables it */ | ||
| set enables it TODO MOVE TO AFTER_SHRED? */ |
Member
There was a problem hiding this comment.
yeah, i think in general there can be some re-organizing / re-structuring that can improve readability
…ed trigger conditions
emwang-jump
force-pushed
the
emwang/sentinel-blk-fix
branch
from
33461b4 to
358642c
Compare
lidatong
approved these changes
Apr 6, 2026
Performance Measurements ⏳
|
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.
fixes https://github.com/firedancer-io/auditor-internal/issues/457 and missing check confirm case.
Also explicitly fixes equivocating blocks where two versions have different parents. i.e. ability to update parent if the FEC set is a confirmed one.