tower: explicitly update tower voted block_ids in reconcile#8957
tower: explicitly update tower voted block_ids in reconcile#8957
Conversation
emwang-jump
force-pushed
the
emwang/backup-vote-reconcile
branch
from
f93d1c1 to
774b840
Compare
emwang-jump
changed the title
There was a problem hiding this comment.
Pull request overview
This PR updates Firedancer’s Tower logic to better support primary/backup validator setups by reconciling local tower state against the on-chain vote account and by removing the standalone tower_leaves structure in favor of using Ghost traversal/state.
Changes:
- Remove
fd_tower_leavesand update tower switching logic to iterate Ghost leaves (with special handling for replayed equivocations). - Extend
fd_tower_reconcileto also updatefd_tower_blocksvote metadata (voted/voted_block_id) when replacing the local tower from the on-chain vote account. - Add/adjust tests for equivocation-related switch behavior and reconcile correctness; add Ghost active fork count and a BFS iterator API.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/discof/tower/fd_tower_tile.c | Removes tower_leaves usage; wires reconcile to update tower_blocks; uses Ghost for active fork count. |
| src/choreo/tower/fd_tower.c | Switch threshold now traverses Ghost; reconcile now updates tower vote metadata in tower_blocks. |
| src/choreo/tower/fd_tower.h | Updates Tower APIs to remove leaves from vote/reset and add blocks to reconcile. |
| src/choreo/tower/fd_tower_blocks.h | Updates block metadata semantics (notably replayed_block_id). |
| src/choreo/tower/test_tower.c | Updates existing switch tests to use Ghost and adds new tests for equivocation and reconcile vote metadata. |
| src/choreo/tower/fd_tower_leaves.h | Removed (leaves tracking no longer used). |
| src/choreo/tower/fd_tower_leaves.c | Removed (leaves tracking no longer used). |
| src/choreo/tower/Local.mk | Drops leaves header/object from build. |
| src/choreo/ghost/fd_ghost_private.h | Adds active_fork_cnt to Ghost state. |
| src/choreo/ghost/fd_ghost.h | Adds fd_ghost_active_fork_cnt and BFS iterator API (with documentation). |
| src/choreo/ghost/fd_ghost.c | Implements active_fork_cnt tracking and BFS iterator helpers. |
| src/choreo/ghost/test_ghost.c | Adds assertions around active_fork_cnt changes after publish. |
| src/app/firedancer-dev/commands/tower.c | Removes tower leaves debug printing. |
You can also share your feedback on Copilot code review. Take the survey.
Performance Measurements ⏳
|
emwang-jump
force-pushed
the
emwang/backup-vote-reconcile
branch
from
774b840 to
bae0453
Compare
Performance Measurements ⏳
|
emwang-jump
force-pushed
the
emwang/backup-vote-reconcile
branch
from
bae0453 to
103e292
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors tower fork-tracking to rely on fd_ghost rather than fd_tower_leaves, and updates fd_tower_reconcile to explicitly synchronize tower_blocks vote metadata (notably voted_block_id) with the reconciled on-chain tower.
Changes:
- Remove
fd_tower_leavesand migrate switch-check / fork counting logic to operate onfd_ghost. - Extend
fd_tower_reconcileto updatefd_tower_blocks_tvote metadata when adopting the on-chain tower (and add tests for reconcile scenarios). - Add
fd_ghosthelpers (active fork count + BFS iterator API) and update ghost tests accordingly.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/discof/tower/fd_tower_tile.c |
Drop tower_leaves usage; pass tower_blocks into reconcile; use ghost-based active fork count. |
src/choreo/tower/test_tower.c |
Update tests to use ghost instead of leaves; add new equivocation and reconcile tests. |
src/choreo/tower/fd_tower_leaves.h |
Removed (tower leaves abstraction deleted). |
src/choreo/tower/fd_tower_leaves.c |
Removed (tower leaves implementation deleted). |
src/choreo/tower/fd_tower_blocks.h |
Adjust block metadata fields / comments (e.g., replayed block id semantics; remove bank_idx). |
src/choreo/tower/fd_tower.h |
Remove leaves from API; extend fd_tower_reconcile signature to accept fd_tower_blocks_t *. |
src/choreo/tower/fd_tower.c |
Switch-check now traverses ghost; reconcile now updates tower_blocks voting metadata. |
src/choreo/tower/Local.mk |
Remove leaves header/object from build. |
src/choreo/ghost/test_ghost.c |
Add assertions for active fork count behavior. |
src/choreo/ghost/fd_ghost_private.h |
Add active_fork_cnt field. |
src/choreo/ghost/fd_ghost.h |
Add fd_ghost_active_fork_cnt and BFS iterator API declarations/docs. |
src/choreo/ghost/fd_ghost.c |
Track active fork count; implement BFS iterator helpers. |
src/app/firedancer-dev/commands/tower.c |
Remove tower leaves printing. |
You can also share your feedback on Copilot code review. Take the survey.
| uchar __attribute__((aligned(8))) vote_acc_buf[ sizeof(fd_vote_acc_t) + 9 ]; | ||
| fd_vote_acc_t __attribute__((aligned(8))) vote_acc; | ||
| memset( &vote_acc_buf, 0, sizeof(vote_acc_buf) ); | ||
| vote_acc.kind = FD_VOTE_ACC_V3; | ||
| vote_acc.v3.votes_cnt = 31; | ||
| for( ulong i = 0; i < 31; i++ ) { | ||
| vote_acc.v3.votes[i] = (fd_vote_acc_vote_t){ .latency = 1, .slot = i + 3, .conf = (uint)(31 - i) }; | ||
| } | ||
|
|
||
| /* Set root to 1. The root option byte follows the last vote entry, | ||
| then 8 bytes of root slot. */ | ||
|
|
||
| uchar * root_option = vote_acc_buf + offsetof(fd_vote_acc_t, v3.votes) + 31*sizeof(fd_vote_acc_vote_t); | ||
| *root_option = 1; /* has root */ | ||
| ulong root_val = 1UL; | ||
| memcpy( root_option + 1, &root_val, sizeof(ulong) ); /* root = slot 1 */ | ||
|
|
||
| /* local_root (0) < on_chain_root (1). Reconcile should still adopt | ||
| the on-chain tower. */ | ||
|
|
||
| ulong local_root = 0; | ||
| fd_tower_reconcile( tower, local_root, (uchar const *)&vote_acc, blocks ); | ||
|
|
| } | ||
| fd_tower_vote_t * new_vote = fd_tower_peek_tail( tower ); | ||
| fd_tower_blk_t * vote_blk = fd_tower_blocks_query( tower_blocks, new_vote->slot ); | ||
| FD_LOG_NOTICE(("reconcile: pushing vote %lu. > root %lu? %d", new_vote->slot, root, new_vote->slot > root)); |
Performance Measurements ⏳
|
| child = blk_pool_ele( blk_pool( ghost ), child->sibling ); /* next sibling */ | ||
| if( FD_UNLIKELY( child ) ) ghost->active_fork_cnt--; /* has a sibling == a fork to be pruned */ |
There was a problem hiding this comment.
| child = blk_pool_ele( blk_pool( ghost ), child->sibling ); /* next sibling */ | |
| if( FD_UNLIKELY( child ) ) ghost->active_fork_cnt--; /* has a sibling == a fork to be pruned */ | |
| child = blk_pool_ele( blk_pool( ghost ), child->sibling ); /* next sibling */ | |
| ghost->width -= !!child; |
| ulong blk_map_gaddr; /* memory offset of the blk_map */ | ||
| ulong vtr_pool_gaddr; /* memory offset of the vtr_pool */ | ||
| ulong vtr_map_gaddr; /* memory offset of the vtr_map */ | ||
| ulong active_fork_cnt; /* incrementally updated number of active forks. cosmetic. */ |
There was a problem hiding this comment.
| ulong active_fork_cnt; /* incrementally updated number of active forks. cosmetic. */ | |
| ulong width; /* incrementally updated number of active forks. cosmetic. */ |
emwang-jump
force-pushed
the
emwang/backup-vote-reconcile
branch
from
103e292 to
5128a1a
Compare
emwang-jump
force-pushed
the
emwang/backup-vote-reconcile
branch
from
5128a1a to
b0dd571
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates Firedancer’s tower reconciliation logic to explicitly synchronize fd_tower_blocks vote metadata (voted/voted_block_id) with the on-chain tower, addressing “ghost votes” scenarios when multiple validators share a vote account (issue #8688).
Changes:
- Extends
fd_tower_reconcileto acceptfd_tower_blocks_t *and updates vote metadata during reconcile. - Removes the prior “bandaid” logic in
fd_tower_vote_and_resetthat patched missingvoted_block_id. - Adds unit tests covering reconcile behavior for (a) adopting on-chain votes and (b) on-chain root ahead of local root.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/discof/tower/fd_tower_tile.c | Updates caller to pass tower_blocks into fd_tower_reconcile. |
| src/choreo/tower/fd_tower.h | Updates fd_tower_reconcile API to include fd_tower_blocks_t *. |
| src/choreo/tower/fd_tower.c | Implements reconcile metadata synchronization and removes prior workaround in vote path. |
| src/choreo/tower/test_tower.c | Adds tests for voted_block_id population and reconcile behavior when on-chain root is ahead. |
| } | ||
| fd_tower_vote_t * new_vote = fd_tower_peek_tail( tower ); | ||
| fd_tower_blk_t * vote_blk = fd_tower_blocks_query( tower_blocks, new_vote->slot ); | ||
| FD_LOG_NOTICE(("reconcile: pushing vote %lu. > root %lu? %d", new_vote->slot, root, new_vote->slot > root)); |
There was a problem hiding this comment.
fd_tower_reconcile logs at FD_LOG_NOTICE for every reconcile and for every vote pushed from the vote account. Reconcile can run frequently, and vote accounts can contain up to 31 votes, so this will produce very high-volume logs in normal operation. Consider removing these logs or downgrading them behind a debug/trace flag so reconcile doesn't become an operational/logging bottleneck.
| FD_LOG_NOTICE(("reconcile: pushing vote %lu. > root %lu? %d", new_vote->slot, root, new_vote->slot > root)); | |
| FD_LOG_DEBUG(( "reconcile: pushing vote %lu. > root %lu? %d", new_vote->slot, root, new_vote->slot > root )); |
| uchar __attribute__((aligned(8))) vote_acc_buf[ sizeof(fd_vote_acc_t) + 9 ]; | ||
| fd_vote_acc_t __attribute__((aligned(8))) vote_acc; | ||
| memset( &vote_acc_buf, 0, sizeof(vote_acc_buf) ); | ||
| vote_acc.kind = FD_VOTE_ACC_V3; | ||
| vote_acc.v3.votes_cnt = 31; | ||
| for( ulong i = 0; i < 31; i++ ) { | ||
| vote_acc.v3.votes[i] = (fd_vote_acc_vote_t){ .latency = 1, .slot = i + 3, .conf = (uint)(31 - i) }; | ||
| } | ||
|
|
||
| /* Set root to 1. The root option byte follows the last vote entry, | ||
| then 8 bytes of root slot. */ | ||
|
|
||
| uchar * root_option = vote_acc_buf + offsetof(fd_vote_acc_t, v3.votes) + 31*sizeof(fd_vote_acc_vote_t); | ||
| *root_option = 1; /* has root */ | ||
| ulong root_val = 1UL; | ||
| memcpy( root_option + 1, &root_val, sizeof(ulong) ); /* root = slot 1 */ | ||
|
|
||
| /* local_root (0) < on_chain_root (1). Reconcile should still adopt | ||
| the on-chain tower. */ | ||
|
|
||
| ulong local_root = 0; | ||
| fd_tower_reconcile( tower, local_root, (uchar const *)&vote_acc, blocks ); | ||
|
|
There was a problem hiding this comment.
In test_reconcile_on_chain_root_ahead, root_option and the serialized root value are written into vote_acc_buf, but fd_tower_reconcile is called with a pointer to vote_acc instead of the buffer. As written, the root option/root slot bytes are not part of the data passed to reconcile, so the test isn't actually exercising the "on-chain root ahead" path. Build the vote account bytes in a single backing buffer (e.g., cast vote_acc_buf to fd_vote_acc_t * and populate it) and pass vote_acc_buf to fd_tower_reconcile.
Performance Measurements ⏳
|
| while( FD_LIKELY( !fd_tower_empty( tower ) ) ) { | ||
| fd_tower_t const * vote = fd_tower_peek_head_const( tower ); | ||
| if( FD_LIKELY( vote->slot > root ) ) break; | ||
| fd_tower_pop_head( tower ); |
There was a problem hiding this comment.
When filtering out stale votes (slots <= root), the code pops votes from the tower but does not clear the corresponding tower_blocks vote metadata. This leaves vote_blk->voted==1 for slots that are no longer in tower, violating the invariant this PR is trying to enforce ("voted iff slot is in the tower") and can affect later logic that keys off voted / voted_block_id.
Clear voted (and any other derived vote metadata if needed) for each slot you pop during this filtering step, similar to how old local votes are cleared earlier in this function.
| fd_tower_pop_head( tower ); | |
| fd_tower_vote_t old_vote = fd_tower_pop_head( tower ); | |
| fd_tower_blk_t * vote_blk = fd_tower_blocks_query( tower_blocks, old_vote.slot ); | |
| if( FD_LIKELY( vote_blk ) ) { | |
| vote_blk->voted = 0; | |
| vote_blk->voted_block_id = 0UL; | |
| } |
There was a problem hiding this comment.
add a comment that this is OK because pruning from tower_forks is asynchronous anyway vs. 1-1 with the tower
copilot is right that IFF is not true here
| /* Verify the tower now matches the on-chain tower: 31 votes, | ||
| slots [3, 3, ..., 33]. */ | ||
|
|
||
| FD_TEST( fd_tower_cnt( tower ) == 31 ); | ||
| for( ulong i = 0; i < 31; i++ ) { | ||
| FD_TEST( fd_tower_peek_index_const( tower, i )->slot == i + 3 ); | ||
| } |
There was a problem hiding this comment.
The comment describing the expected slots is inconsistent with the assertions below: it says slots [3, 3, ..., 33] but the test checks i + 3 (i.e., [3, 4, ..., 33]). Updating this comment will make the test intent clearer and avoid confusion when debugging failures.
Performance Measurements ⏳
|
emwang-jump
force-pushed
the
emwang/backup-vote-reconcile
branch
from
dad6361 to
3214053
Compare
Performance Measurements ⏳
|
| while( FD_LIKELY( !fd_tower_empty( tower ) ) ) { | ||
| fd_tower_t const * vote = fd_tower_peek_head_const( tower ); | ||
| if( FD_LIKELY( vote->slot > root ) ) break; | ||
| fd_tower_pop_head( tower ); |
There was a problem hiding this comment.
add a comment that this is OK because pruning from tower_forks is asynchronous anyway vs. 1-1 with the tower
copilot is right that IFF is not true here
| /* It's possible prev_vote_fork->voted is not set even though we | ||
| popped it from the top of our tower. This can happen when there | ||
| are multiple nodes operating with the same vote account. | ||
|
|
||
| In a typical setup involving a primary staked node and backup | ||
| unstaked node, the two nodes' towers will usually be identical | ||
| but occassionally diverge when one node observes a minority fork | ||
| the other doesn't. As a result, one node might be locked out | ||
| from voting for a fork that the other node is not. | ||
|
|
||
| This becomes a problem in our primary-backup setup when the | ||
| unstaked node is locked out but the staked node is not. The | ||
| staked node ultimately lands the vote into the on-chain vote | ||
| account, so it's possible when the unstaked node reads back their | ||
| on-chain vote account to diff against their local tower, there | ||
| are votes in there they themselves did not vote for due to | ||
| lockout (fd_tower_reconcile). | ||
|
|
||
| As a result, `voted_block_id` will not be set for slots in their | ||
| tower, which normally would be an invariant violation because the | ||
| node must have set this value when they voted for the slot (and | ||
| pushed it to their tower). | ||
|
|
||
| So here we manually set the voted_block_id to replayed_block_id | ||
| if not already set. We know we must have replayed it, because to | ||
| observe the on-chain tower you must have replayed all the slots | ||
| in the tower. */ |
There was a problem hiding this comment.
can you keep this block comment, and combine it with the one below
| There are special considerations for reconciling a backup validator | ||
| with the on-chain tower. When running a Firedancer node as a backup | ||
| for a staked validator, reconcile must set voted_block_id for slots | ||
| in the on-chain tower that the backup never locally voted for. We | ||
| set voted_block_id to replayed_block_id, ie. the most recent block_id | ||
| we replayed for that slot. |
There was a problem hiding this comment.
replace this paragraph with above comment
| ulong tower_root, | ||
| uchar const * vote_acc ); | ||
| uchar const * vote_acc, | ||
| fd_tower_blocks_t * blocks ); |
There was a problem hiding this comment.
third argument
tower
tower_root
tower_blocks
emwang-jump
force-pushed
the
emwang/backup-vote-reconcile
branch
from
32d1a21 to
a5f8787
Compare
| /* It's possible our local root is newer than the on-chain root | ||
| (even though the tower is older). The most likely reason is | ||
| booting from a snapshot where snapshot slot > on-chain root. | ||
| Filter out stale votes <= local root. | ||
|
|
||
| After filtering, the tower may be empty if all on-chain votes | ||
| were below our local root. In Agave this is handled by falling | ||
| back to local_root as the last_voted_slot. Here it is fine to | ||
| leave the tower empty because vote_and_reset handles an empty | ||
| tower (case 0). | ||
|
|
||
| /* Fast forward our tower to tower_root by retaining only votes > | ||
| local tower root. */ | ||
| There's no need to clear voted metadata in tower_blocks, which | ||
| breaks our invariant that voted==1 iff slot is in tower. This is | ||
| fine because pruning from tower_blocks is asynchronous with the | ||
| tower. */ | ||
|
|
||
| if( FD_LIKELY( on_chain_root == ULONG_MAX || tower_root > on_chain_root ) ) { | ||
| while( FD_LIKELY( !fd_tower_empty( tower ) ) ) { | ||
| fd_tower_t const * vote = fd_tower_peek_head_const( tower ); | ||
| if( FD_LIKELY( vote->slot > root ) ) break; | ||
| if( FD_LIKELY( vote->slot > tower_root ) ) break; | ||
| fd_tower_pop_head( tower ); | ||
| } | ||
| } |
There was a problem hiding this comment.
The function/header comment says reconcile “updates tower_blocks vote metadata to match the updated tower”, but after the fast-forward step (popping votes <= tower_root) the corresponding tower_blocks entries (when present) can remain with voted=1 even though those slots are no longer in tower. Either clear vote_blk->voted for each slot popped in this filtering loop (when fd_tower_blocks_query returns non-NULL), or adjust the function/header comment to explicitly document that tower_blocks vote metadata may intentionally remain set for filtered-out votes and therefore is not guaranteed to mirror the final tower contents.
|
|
||
| In a typical setup involving a primary staked node and backup | ||
| unstaked node, the two nodes' towers will usually be identical | ||
| but occassionally diverge when one node observes a minority fork |
There was a problem hiding this comment.
Typo in comment: “occassionally” should be “occasionally”.
| but occassionally diverge when one node observes a minority fork | |
| but occasionally diverge when one node observes a minority fork |
| push_vote( tower, 1 ); | ||
| push_vote( tower, 2 ); | ||
|
|
||
| /* Construct a mock on-chain vote account (v3) with 32 votes |
There was a problem hiding this comment.
The comment says “mock on-chain vote account (v3) with 32 votes (slots 3..33)”, but 3..33 inclusive is 31 votes and votes_cnt is set to 31. Update the comment to match the actual vote count.
| /* Construct a mock on-chain vote account (v3) with 32 votes | |
| /* Construct a mock on-chain vote account (v3) with 31 votes |
| fd_tower_reconcile( tower, local_root, vote_acc_buf, blocks ); | ||
|
|
||
| /* Verify the tower now matches the on-chain tower: 31 votes, | ||
| slots [3, 3, ..., 33]. */ |
There was a problem hiding this comment.
Comment typo: “slots [3, 3, ..., 33]” should reflect the increasing sequence (e.g., [3, 4, ..., 33]).
| slots [3, 3, ..., 33]. */ | |
| slots [3, 4, ..., 33]. */ |
Performance Measurements ⏳
|
fixes #8688