Optional Cli parameter to add additional-fee to solana ping #23513
Conversation
tao-stones
force-pushed
the
additional-fee-for-ping
branch
2 times, most recently
from
521af9a
to
67fb6d3
Compare
cli/src/cluster_query.rs
Outdated
| if let Some(compute_budget_ix) = build_compute_budget_instruction(additional_fee) { | ||
| Message::new(&[ix, compute_budget_ix], Some(&config.signers[0].pubkey())) | ||
| } else { | ||
| Message::new(&[ix], Some(&config.signers[0].pubkey())) | ||
| } |
There was a problem hiding this comment.
| if let Some(compute_budget_ix) = build_compute_budget_instruction(additional_fee) { | |
| Message::new(&[ix, compute_budget_ix], Some(&config.signers[0].pubkey())) | |
| } else { | |
| Message::new(&[ix], Some(&config.signers[0].pubkey())) | |
| } | |
| let mut ixs = vec![system_instruction::transfer(&config.signers[0].pubkey(), &to, lamports)]; | |
| if let Some(additional_fee) = additional_fee { | |
| ixs.push(build_compute_budget_instruction(additional_fee)); | |
| } | |
| Message::new(&ixs, Some(&config.signers[0].pubkey())) |
Nit: I think this is slightly easier to grok, and more DRY.
Will need to remove previous line (github wouldn't let me make a suggestion on it since it's not part of diff)
There was a problem hiding this comment.
IIRC compute budget IXs MUST be first (but after nonce! 😝)
There was a problem hiding this comment.
think it just needs to be in the first 3 IXs: https://github.com/solana-labs/solana/blob/master/program-runtime/src/compute_budget.rs#L112-L115
There was a problem hiding this comment.
Ah, "otherwise ignored." I must've missed that during compute budget review, or else I'd have complained 😅
@jackcmay should ComputeBudget instructions be prefixed by convention even though we aren't enforcing it?
cli/src/cluster_query.rs
Outdated
| additional_fee = default_fee_structure | ||
| .get_max_fee(signature_count, write_lock_count) | ||
| .try_into() | ||
| .unwrap(); |
There was a problem hiding this comment.
Can we make this an expect() and explain why we think get_max_fee() won't ever return > u32::MAX?
There was a problem hiding this comment.
yep, remove this block of code
cli/src/cluster_query.rs
Outdated
| if let Some(compute_budget_ix) = build_compute_budget_instruction(additional_fee) { | ||
| Message::new(&[ix, compute_budget_ix], Some(&config.signers[0].pubkey())) | ||
| } else { | ||
| Message::new(&[ix], Some(&config.signers[0].pubkey())) | ||
| } |
There was a problem hiding this comment.
IIRC compute budget IXs MUST be first (but after nonce! 😝)
Co-authored-by: Tyera Eulberg <[email protected]> Co-authored-by: Trent Nelson <[email protected]>
tao-stones
force-pushed
the
additional-fee-for-ping
branch
from
cc1c2c6
to
a29ff69
Compare
correct the help message for arg Co-authored-by: Tyera Eulberg <[email protected]>
mergify
bot
dismissed stale reviews from CriesofCarrots and t-nelson
Pull request has been modified.
tao-stones
added
the
automerge
Codecov Report
@@ Coverage Diff @@
## master #23513 +/- ##
=========================================
+ Coverage 81.3% 81.4% +0.1%
=========================================
Files 572 575 +3
Lines 155876 156618 +742
=========================================
+ Hits 126815 127618 +803
+ Misses 29061 29000 -61 |
|
This PR has been automatically locked since there has not been any activity in past 14 days after it was merged. |
Problem
During testnet bench tps test, Solana Ping transactions are not landing, therefore not showing on Explorer status. Can add additional-fee to Ping transactions to allow landing even during bench tps test.
Note: it requires
tx_wide_compute_capfeature to be turned on, and leader-qos (#23257) landed in testnetSummary of Changes
additional-fee, if it is provided, acompute-budgetinstruction with additional_fee will be included ping transaction to increase its fee-per-cu.0is specified as additional-fee value, default lamports for 1 signature and 1 write lock will be used.Fixes #