Conversation
Performance Measurements ⏳
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the notar (notary) epoch handling to be based on the root slot's epoch instead of the completed slot's epoch. The changes simplify the notar data structures by removing dual-epoch tracking (current and previous) and replacing the fd_notar_update_voters function with a new reindex_notar function that is called only when the root slot transitions to a new epoch.
Changes:
- Added epoch tracking to tower blocks and replaced epoch-per-slot logic with epoch-per-root logic
- Introduced
reindex_notarfunction to handle voter bit position remapping during epoch transitions - Removed previous epoch tracking fields from notar structures (prev_vtrs, prev_stake, prev_bit, epoch field)
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/discof/tower/fd_tower_tile.c | Added reindex_notar function and modified epoch transition logic to check root slot epochs instead of replay slot epochs |
| src/choreo/tower/fd_tower_stakes.h | Reformatted struct field alignment (cosmetic change) |
| src/choreo/tower/fd_tower_blocks.h | Added epoch field to fd_tower_blk struct to support epoch-based logic |
| src/choreo/notar/test_notar.c | Commented out test_update_voters test (removed test coverage) |
| src/choreo/notar/fd_notar.h | Removed epoch tracking, prev_vtrs field, and fd_notar_update_voters API; simplified vtr structure |
| src/choreo/notar/fd_notar.c | Removed fd_notar_update_voters implementation and prev_vtrs initialization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lidatong
force-pushed
the
chali/fix/7833
branch
from
3b1b62f to
94cd7ad
Compare
Performance Measurements ⏳
|
lidatong
force-pushed
the
chali/fix/7833
branch
from
94cd7ad to
c8233f3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lidatong
force-pushed
the
chali/fix/7833
branch
2 times, most recently
from
12f0e92 to
36c8454
Compare
Performance Measurements ⏳
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Performance Measurements ⏳
|
Performance Measurements ⏳
|
lidatong
force-pushed
the
chali/fix/7833
branch
2 times, most recently
from
7a555ac to
25622f4
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Performance Measurements ⏳
|
Performance Measurements ⏳
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Performance Measurements ⏳
|
Performance Measurements ⏳
|
| fd_compact_tower_sync_serde_t compact_tower_sync_serde; | ||
| uchar vote_txn[FD_TPU_PARSED_MTU]; | ||
| ulong notar_reindex[ FD_VOTER_MAX ]; | ||
| fd_hash_t notar_removed[ FD_VOTER_MAX ]; |
There was a problem hiding this comment.
notar_removed/removed is declared as fd_hash_t but is used to store voter addresses (fd_pubkey_t). This is a type mismatch (won't compile) and also makes the subsequent fd_notar_vtr_query( ..., removed[i], ... ) call incorrect. Change the buffer to fd_pubkey_t (or adjust the code to store hashes intentionally and query with the correct key type).
| fd_hash_t notar_removed[ FD_VOTER_MAX ]; | |
| fd_pubkey_t notar_removed[ FD_VOTER_MAX ]; |
| FD_TEST( oldr_tower_blk->epoch==newr_tower_blk->epoch || oldr_tower_blk->epoch+1==newr_tower_blk->epoch ); /* root can only move forward one epoch */ | ||
| if( FD_UNLIKELY( slot_completed->slot==ctx->init_slot || oldr_tower_blk->epoch+1==newr_tower_blk->epoch ) ) { | ||
| FD_TEST( newr_tower_blk->epoch==slot_completed->epoch ); /* if the new root's epoch has advanced, it must be in the same epoch as current slot_completed (old root, new root, slot_completed cannot span >2 epochs) */ | ||
| reindex_notar( ctx, out.root_slot ); | ||
| } |
There was a problem hiding this comment.
The epoch reindex condition is currently slot_completed->slot==ctx->init_slot && oldr_tower_blk->epoch+1==newr_tower_blk->epoch, but the surrounding comment says to reindex when entering a new epoch (and typically also on initialization). With &&, reindexing will be skipped in the common cases where either (a) we're at init_slot but not crossing an epoch, or (b) we cross an epoch later. Update the condition so it triggers whenever reindexing is required (e.g., init OR epoch advanced).
| fd_wksp_t * wksp = fd_wksp_new_anonymous( fd_cstr_to_shmem_page_sz( _page_sz ), page_cnt, fd_shmem_cpu_idx( numa_idx ), "wksp", 0UL ); | ||
| FD_TEST( wksp ); | ||
|
|
||
| test_update_voters( wksp ); | ||
| // test_update_voters( wksp ); |
There was a problem hiding this comment.
test_update_voters (and its invocation) are commented out, leaving this unit test effectively doing nothing. Update/replace this test to exercise the new notar epoch/voter reindex behavior introduced by this PR so regressions are still caught in CI.
Performance Measurements ⏳
|
Closes #7833