replay, leader pipeline: implement remove_simple_vote_from_cost_model#8386
replay, leader pipeline: implement remove_simple_vote_from_cost_model#8386topointon-jump merged 1 commit intomainfrom
Conversation
Performance Measurements ⏳
|
8a3a0ec to
f2fae06
Compare
Performance Measurements ⏳
|
f2fae06 to
07a2963
Compare
Performance Measurements ⏳
|
There was a problem hiding this comment.
Pull request overview
This PR implements cost model changes needed for the stop_use_static_simple_vote_tx_cost and bls_pubkey_management_in_vote_account features. Before stop_use_static_simple_vote_tx_cost, simple vote transactions had a hardcoded cost value (3428 CUs). After the feature activates, they go through the full cost model calculation. Additionally, after bls_pubkey_management_in_vote_account, some vote instructions (authorize instructions) charge additional CUs for BLS proof-of-possession verification.
The PR adds the new feature flag, updates the cost tracker to conditionally use the static cost, adds proper feature-gating in execution tiles, and updates pack's cost estimates to use a conservative upper bound that works both before and after the features activate.
Note: This PR is marked as WIP and contains known issues (FIXME comments) in fd_bank_tile.c that need to be addressed.
Changes:
- Added
stop_use_static_simple_vote_tx_costfeature flag registration - Updated cost tracker to feature-gate simple vote static cost usage
- Updated execution tiles to feature-gate simple vote cost overrides
- Updated pack cost estimates to use upper bounds for simple votes to avoid feature-gating
- Removed vote program from builtin cost table
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/flamenco/features/feature_map.json | Added stop_use_static_simple_vote_tx_cost feature registration |
| src/flamenco/features/fd_features_generated.h | Generated header updates for new feature (count and ID) |
| src/flamenco/features/fd_features_generated.c | Generated implementation for new feature lookup |
| src/flamenco/runtime/fd_cost_tracker.h | Added FD_SIMPLE_VOTE_USAGE_COST constant (3428 CUs) |
| src/flamenco/runtime/fd_cost_tracker.c | Feature-gated simple vote cost logic; changed cost checks to use is_simple_vote flag |
| src/disco/pack/fd_pack_cost.h | Removed vote program from builtin table; added BLS verification cost; updated simple vote cost to use upper bound |
| src/discof/execle/fd_execle_tile.c | Feature-gated simple vote fixed cost logic in microblock and bundle handlers |
| src/discoh/bank/fd_bank_tile.c | Added FIXME comments noting missing feature-gating (known WIP issue) |
Comments suppressed due to low confidence (1)
src/discoh/bank/fd_bank_tile.c:262
- The FIXME comment correctly identifies that this code needs to be feature-gated on stop_use_static_simple_vote_tx_cost. Currently, the code always uses FD_PACK_VOTE_DEFAULT_COMPUTE_UNITS for simple votes regardless of whether the feature is active, which means it will continue using the static cost even after the feature activates. This is inconsistent with the changes in fd_execle_tile.c where proper feature-gating was added. The missing feature gate could cause overpacked blocks to go undetected after the feature activates, as the FIXME notes.
/* FIXME: we likely have to feature-gate this on
stop_use_static_simple_vote_tx_cost otherwise we could fail to
detect overpacked blocks. */
int is_simple_vote = 0;
if( FD_UNLIKELY( is_simple_vote = fd_txn_is_simple_vote_transaction( TXN(txn), txn->payload ) ) ) {
/* Simple votes are charged fixed amounts of compute regardless of
the real cost they incur. fd_ext_bank_load_and_execute_txns
returns the real cost, however, so we override it here. */
actual_execution_cus = FD_PACK_VOTE_DEFAULT_COMPUTE_UNITS;
actual_acct_data_cus = 0U;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
07a2963 to
7ea3111
Compare
Performance Measurements ⏳
|
7ea3111 to
1a1e60d
Compare
Performance Measurements ⏳
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1a1e60d to
9881f93
Compare
9881f93 to
b8919a9
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 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Performance Measurements ⏳
|
b8919a9 to
94e9874
Compare
Performance Measurements ⏳
|
94e9874 to
4a73896
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 4 comments.
Comments suppressed due to low confidence (1)
src/discoh/bank/fd_bank_tile.c:270
- In bank tile, simple-vote CU overrides are applied unconditionally. Once
remove_simple_vote_from_cost_modelis active, these overrides will under-report actual execution / loaded-account costs, which can inflate rebates and hide cases where actual CUs exceed requested (potentially letting pack overfill a block). Please gate this override on the feature being inactive (similar tofd_execle_tile.c), or plumb a feature-activation query from the ext bank so the tile can make the decision correctly.
int is_simple_vote = 0;
if( FD_UNLIKELY( is_simple_vote = fd_txn_is_simple_vote_transaction( TXN(txn), txn->payload ) ) ) {
/* TODO: remove this once remove_simple_vote_from_cost_model is
activated */
/* Simple votes are charged fixed amounts of compute regardless of
the real cost they incur. fd_ext_bank_load_and_execute_txns
returns the real cost, however, so we override it here. */
actual_execution_cus = FD_PACK_VOTE_DEFAULT_COMPUTE_UNITS;
actual_acct_data_cus = 0U;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Performance Measurements ⏳
|
4a73896 to
a3798f9
Compare
a3798f9 to
6dc1397
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 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Performance Measurements ⏳
|
6dc1397 to
2d8a820
Compare
Performance Measurements ⏳
|
2d8a820 to
b143be4
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 2 comments.
Comments suppressed due to low confidence (1)
src/discoh/bank/fd_bank_tile.c:271
- The simple-vote CU override here is unconditional. Once the remove_simple_vote_from_cost_model feature is active, simple votes should be charged their real execution + loaded-account-data cost (e.g., authorize w/ BLS PoP verification), so overriding to FD_PACK_VOTE_DEFAULT_COMPUTE_UNITS/0 will under-account consumed CUs and over-rebate. Gate this override on !FD_FEATURE_ACTIVE_BANK(ctx->_bank, remove_simple_vote_from_cost_model) (similar to fd_execle_tile.c), and only apply the fixed-cost override pre-activation.
int is_simple_vote = 0;
if( FD_UNLIKELY( is_simple_vote = fd_txn_is_simple_vote_transaction( TXN(txn), txn->payload ) ) ) {
/* TODO: remove this once remove_simple_vote_from_cost_model is
activated */
/* Simple votes are charged fixed amounts of compute regardless of
the real cost they incur. fd_ext_bank_load_and_execute_txns
returns the real cost, however, so we override it here. */
actual_execution_cus = FD_PACK_VOTE_DEFAULT_COMPUTE_UNITS;
actual_acct_data_cus = 0U;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* TODO: remove this once remove_simple_vote_from_cost_model is | ||
| activated */ | ||
| if( FD_UNLIKELY( fd_txn_is_simple_vote_transaction( TXN(txns + i), txns[ i ].payload ) ) ) { | ||
| /* Although bundles dont typically contain simple votes, we want | ||
| to charge them correctly anyways. */ | ||
| /* FIXME: before remove_simple_vote_from_cost_model is activated | ||
| we need to change fd_ext_bank_load_and_execute_txns to | ||
| return the real cost of the transaction and remove | ||
| this conditional logic. | ||
|
|
||
| The plan is to do this in the next version of | ||
| Frankendancer as remove_simple_vote_from_cost_model is | ||
| an Agave 4.0 feature. */ | ||
| consumed_cus[ i ] = FD_PACK_VOTE_DEFAULT_COMPUTE_UNITS; | ||
| } else { |
There was a problem hiding this comment.
Same as handle_microblock: this simple-vote override should be conditional on remove_simple_vote_from_cost_model not being active. After activation, forcing consumed_cus[i] to FD_PACK_VOTE_DEFAULT_COMPUTE_UNITS will under-account real vote instruction costs and can distort rebates/scheduling.
Cost model changes needed for
remove_simple_vote_from_cost_modelandbls_pubkey_management_in_vote_account.As of
remove_simple_vote_from_cost_model, simple vote transactions no longer have a hardcoded value in the cost model.As of
bls_pubkey_management_in_vote_account, the vote program is no longer treated as a builtin in the cost model, and some vote instructions are more expensive (those which perform BLS proof-of-possession verification).To avoid feature-gating pack code, we increase the estimate for simple vote transactions which pack uses. Once
remove_simple_vote_from_cost_modelhas been activated everywhere, we can make the is_simple_vote classifier stricter and use a tighter estimate.