Conversation
Performance Measurements ⏳
|
There was a problem hiding this comment.
Pull request overview
This PR starts integrating “VAT” support into the runtime by introducing a new CoW-managed fd_top_votes_t state tracked per bank and stored in a shared pool alongside existing CoW fields (e.g. epoch leaders, cost tracker).
Changes:
- Add a fixed maximum footprint constant for
fd_top_votes_tstorage. - Extend
fd_bank/fd_banksshared-memory layout with a newtop_votesCoW pool, per-bank dirty/index tracking, and a dedicated pool lock. - Implement
fd_bank_top_votes_query()/fd_bank_top_votes_modify()and wire pool initialization/join/release paths into banks lifecycle (new/join/advance_root/prune/clear).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/flamenco/stakes/fd_top_votes.h |
Introduces FD_TOP_VOTES_MAX_FOOTPRINT used to size bank-owned top-votes storage. |
src/flamenco/runtime/fd_bank.h |
Adds fd_bank_top_votes_t pool type, bank/banks offsets + locks + query/modify API declarations. |
src/flamenco/runtime/fd_bank.c |
Allocates/joins top-votes pool in shared memory and implements bank-level query/modify + pool release logic. |
| bank->top_votes_pool_offset = (ulong)top_votes_pool - (ulong)bank; | ||
| } | ||
|
|
||
| static inline fd_bank_top_votes_t * | ||
| fd_bank_get_top_votes_pool( fd_bank_data_t * bank ) { | ||
| return fd_type_pun( (uchar *)bank + bank->top_votes_pool_offset ); |
There was a problem hiding this comment.
fd_bank_set_top_votes_pool()/fd_bank_get_top_votes_pool() store and recover the pool pointer via fd_type_pun rather than using fd_bank_top_votes_pool_leave()/fd_bank_top_votes_pool_join(). This deviates from the existing epoch_leaders/cost_tracker pool pattern and makes the NULL checks in fd_bank_top_votes_{query,modify} ineffective (the getter cannot fail/validate magic). Consider mirroring the established pattern: store the "leave" pointer (shared-memory base) and have the getter return fd_bank_top_votes_pool_join(...), so misconfiguration/corruption is detected early and behavior is consistent across CoW pools.
| bank->top_votes_pool_offset = (ulong)top_votes_pool - (ulong)bank; | |
| } | |
| static inline fd_bank_top_votes_t * | |
| fd_bank_get_top_votes_pool( fd_bank_data_t * bank ) { | |
| return fd_type_pun( (uchar *)bank + bank->top_votes_pool_offset ); | |
| void * top_votes_pool_mem = fd_bank_top_votes_pool_leave( top_votes_pool ); | |
| if( FD_UNLIKELY( !top_votes_pool_mem ) ) { | |
| FD_LOG_CRIT(( "Failed to leave top votes pool" )); | |
| } | |
| bank->top_votes_pool_offset = (ulong)top_votes_pool_mem - (ulong)bank; | |
| } | |
| static inline fd_bank_top_votes_t * | |
| fd_bank_get_top_votes_pool( fd_bank_data_t * bank ) { | |
| return fd_bank_top_votes_pool_join( (uchar *)bank + bank->top_votes_pool_offset ); |
| #define FD_TOP_VOTES_ALIGN (128UL) | ||
|
|
||
| #define FD_TOP_VOTES_MAX_FOOTPRINT (1000000UL) | ||
|
|
There was a problem hiding this comment.
FD_TOP_VOTES_MAX_FOOTPRINT is set to a hard-coded 1,000,000 bytes and is used to size every fd_bank_top_votes pool element. Given fd_top_votes is always constructed with vote_accounts_max=2000 in this PR, this constant likely over-allocates shared memory per fork by a large margin. Consider defining an explicit FD_TOP_VOTES_VOTE_ACCOUNTS_MAX and sizing the footprint constant to match fd_top_votes_footprint(FD_TOP_VOTES_VOTE_ACCOUNTS_MAX) (and/or adding a runtime assert that the computed footprint fits) to avoid unnecessary memory/IPC pressure as max_fork_width grows.
| #define FD_TOP_VOTES_ALIGN (128UL) | |
| #define FD_TOP_VOTES_MAX_FOOTPRINT (1000000UL) | |
| #define FD_TOP_VOTES_ALIGN (128UL) | |
| #define FD_TOP_VOTES_VOTE_ACCOUNTS_MAX (2000UL) | |
| #define FD_TOP_VOTES_MAX_FOOTPRINT \ | |
| fd_top_votes_footprint( FD_TOP_VOTES_VOTE_ACCOUNTS_MAX ) |
| for( ulong i=0UL; i<max_fork_width; i++ ) { | ||
| fd_bank_top_votes_t * top_votes_ele = fd_bank_top_votes_pool_ele( top_votes_pool, i ); | ||
| fd_top_votes_t * top_votes = fd_top_votes_join( fd_top_votes_new( top_votes_ele->data, 2000UL, seed ) ); /* TODO:FIXME: Magic number*/ | ||
| if( FD_UNLIKELY( !top_votes ) ) { | ||
| FD_LOG_WARNING(( "Failed to create top votes" )); | ||
| return NULL; |
There was a problem hiding this comment.
In fd_banks_new(), fd_top_votes_new() is called with a hard-coded 2000UL ("TODO:FIXME: Magic number"). This should be a named constant shared with the VAT design (e.g. FD_TOP_VOTES_VOTE_ACCOUNTS_MAX) so the value is not duplicated and future changes don’t risk inconsistent sizing/logic (especially since the backing buffer size is also fixed via FD_TOP_VOTES_MAX_FOOTPRINT).
| fd_bank_top_votes_t * top_votes_pool = fd_bank_top_votes_pool_join( fd_bank_top_votes_pool_new( top_votes_pool_mem, max_fork_width ) ); | ||
| if( FD_UNLIKELY( !top_votes_pool ) ) { | ||
| FD_LOG_WARNING(( "Failed to create top votes pool" )); | ||
| return NULL; | ||
| } | ||
| fd_banks_set_top_votes_pool( banks_data, top_votes_pool ); | ||
| for( ulong i=0UL; i<max_fork_width; i++ ) { | ||
| fd_bank_top_votes_t * top_votes_ele = fd_bank_top_votes_pool_ele( top_votes_pool, i ); | ||
| fd_top_votes_t * top_votes = fd_top_votes_join( fd_top_votes_new( top_votes_ele->data, 2000UL, seed ) ); /* TODO:FIXME: Magic number*/ | ||
| if( FD_UNLIKELY( !top_votes ) ) { | ||
| FD_LOG_WARNING(( "Failed to create top votes" )); | ||
| return NULL; | ||
| } | ||
| fd_top_votes_init( top_votes ); | ||
| } | ||
|
|
There was a problem hiding this comment.
The new top-votes CoW pool integration adds non-trivial behavior (pool init in fd_banks_new, CoW modify/query, and pool-element release during advance_root/prune/clear) but there are no corresponding bank-level tests. There are existing unit tests in runtime/test_bank.c that exercise epoch_leaders CoW/pool behavior; adding analogous coverage for top_votes would help catch regressions (e.g., cloning vs reset, correct release on root advance).
| static inline fd_bank_top_votes_t * | ||
| fd_banks_get_top_votes_pool( fd_banks_data_t * banks_data ) { | ||
| return fd_type_pun( (uchar *)banks_data + banks_data->top_votes_pool_offset ); | ||
| } | ||
|
|
||
| static inline void | ||
| fd_banks_set_top_votes_pool( fd_banks_data_t * banks_data, | ||
| fd_bank_top_votes_t * top_votes_pool ) { | ||
| banks_data->top_votes_pool_offset = (ulong)top_votes_pool - (ulong)banks_data; | ||
| } |
There was a problem hiding this comment.
fd_banks_set_top_votes_pool()/fd_banks_get_top_votes_pool() also use raw pointer arithmetic + fd_type_pun instead of the fd_bank_top_votes_pool_{leave,join} APIs. This makes joins less robust (no magic/alignment validation) and is inconsistent with fd_banks_set_epoch_leaders_pool()/get_epoch_leaders_pool. Consider storing the pool "leave" pointer and re-joining via fd_bank_top_votes_pool_join() for consistency and better corruption detection.
Performance Measurements ⏳
|
Performance Measurements ⏳
|
| FD_TEST( fd_top_votes_query( top_votes, vote_pubkey, &node_out, &stake_out, &slot_out, ×tamp_out ) ); | ||
| FD_TEST( !memcmp( &node_out, expected_node, sizeof(fd_pubkey_t) ) ); | ||
| FD_TEST( stake_out==expected_stake ); | ||
| FD_TEST( slot_out==expected_stake ); | ||
| FD_TEST( timestamp_out==(long)expected_stake ); |
There was a problem hiding this comment.
assert_vote_present compares slot_out and timestamp_out against expected_stake, which means the test will still pass even if last_vote_slot/last_vote_timestamp are wired incorrectly. Consider extending assert_vote_present to take expected_slot and expected_timestamp (and use different values in inserts) so the metadata fields are actually validated.
|
|
||
| ulong last_vote_slot; | ||
| long last_vote_timestamp; | ||
| int found = fd_top_votes_query( top_votes, &pubkey, NULL, NULL, &last_vote_slot, &last_vote_timestamp ); |
There was a problem hiding this comment.
fd_sysvar_clock.c calls fd_top_votes_query(top_votes, ...) unconditionally, but fd_bank_top_votes_query() can return NULL when the bank hasn't allocated a top-votes pool element yet. This will dereference a NULL pointer inside fd_top_votes_query; add a NULL check and fall back to the accdb path when top_votes is NULL.
| int found = fd_top_votes_query( top_votes, &pubkey, NULL, NULL, &last_vote_slot, &last_vote_timestamp ); | |
| int found = 0; | |
| if( FD_LIKELY( top_votes ) ) | |
| found = fd_top_votes_query( top_votes, &pubkey, NULL, NULL, &last_vote_slot, &last_vote_timestamp ); |
| /* fd_top_votes_update inserts a new vote account into the top votes set | ||
| given a vote account, node account, last vote slot, last vote | ||
| timestamp, and a stake. The node account, last vote slot, and last | ||
| vote timestamp are just metadata for the structure. If the vote | ||
| account isn't in the top max_vote_accounts in terms of stake, it is | ||
| ignored and is treated as a no-op. If the vote account ties the | ||
| minimum stake and the struct is full, all elements with that stake | ||
| are removed. */ |
There was a problem hiding this comment.
The doc comment for fd_top_votes_insert still says "fd_top_votes_update inserts ...". This is confusing now that fd_top_votes_update exists again with different semantics; update the comment to refer to fd_top_votes_insert (and consider tightening wording around the parameters order: stake vs last_vote_slot/timestamp).
| void | ||
| fd_top_votes_update( fd_top_votes_t * top_votes, | ||
| fd_pubkey_t const * pubkey, | ||
| ulong last_vote_slot, | ||
| long last_vote_timestamp ) { | ||
| vote_ele_t * pool = get_pool( top_votes ); | ||
| map_t * map = get_map( top_votes ); | ||
|
|
||
| ushort idx = (ushort)map_idx_query_const( map, pubkey, USHORT_MAX, pool ); | ||
| if( FD_UNLIKELY( idx==USHORT_MAX ) ) return; | ||
| vote_ele_t * ele = pool_ele( pool, idx ); | ||
| ele->last_vote_slot = last_vote_slot; | ||
| ele->last_vote_timestamp = last_vote_timestamp; | ||
| } |
There was a problem hiding this comment.
fd_top_votes_update is new behavior (updates slot/timestamp for an existing entry) but there are no unit tests exercising it or verifying that fd_top_votes_query returns the updated metadata. Add a test case that inserts an entry, calls fd_top_votes_update with different values, and then queries the metadata to ensure it changes as expected.
Performance Measurements ⏳
|
Performance Measurements ⏳
|
Performance Measurements ⏳
|
Performance Measurements ⏳
|
Performance Measurements ⏳
|
Performance Measurements ⏳
|
Performance Measurements ⏳
|
Performance Measurements ⏳
|
| FD_SCRATCH_ALLOC_FINI( l, fd_top_votes_align() ); | ||
|
|
||
| if( FD_UNLIKELY( FD_SCRATCH_ALLOC_FINI( l, fd_top_votes_align() ) != (ulong)top_votes + fd_top_votes_footprint( vote_accounts_max ) ) ) { |
There was a problem hiding this comment.
fd_top_votes_new calls FD_SCRATCH_ALLOC_FINI( l, fd_top_votes_align() ) twice in a row. The macro mutates l, so calling it twice is redundant/confusing and makes it easier to accidentally introduce layout bugs later. Consider capturing the return value from a single FD_SCRATCH_ALLOC_FINI call and using that for the layout check (or remove the first call).
| FD_SCRATCH_ALLOC_FINI( l, fd_top_votes_align() ); | |
| if( FD_UNLIKELY( FD_SCRATCH_ALLOC_FINI( l, fd_top_votes_align() ) != (ulong)top_votes + fd_top_votes_footprint( vote_accounts_max ) ) ) { | |
| ulong l_end = FD_SCRATCH_ALLOC_FINI( l, fd_top_votes_align() ); | |
| if( FD_UNLIKELY( l_end != (ulong)top_votes + fd_top_votes_footprint( vote_accounts_max ) ) ) { |
Performance Measurements ⏳
|
Performance Measurements ⏳
|
Performance Measurements ⏳
|
Performance Measurements ⏳
|
| fd_top_votes_t * top_votes = fd_bank_top_votes_modify( bank ); | ||
| for( ushort i=0; i<txn_out->accounts.cnt; i++ ) { | ||
| /* We are only interested in saving writable accounts and the fee |
There was a problem hiding this comment.
fd_runtime_commit_txn unconditionally calls fd_bank_top_votes_modify( bank ) before scanning accounts. This can trigger an unnecessary CoW allocation + ~194KB memcpy for transactions that don't touch any vote accounts. Consider lazily calling fd_bank_top_votes_modify only when a vote-program-owned account is encountered (or when the VAT feature is active and the cache is actually needed).
todo: ledger with vat enabled, pass vat set of validators across replay_epoch,