stake-pool-cli: Add support for priority fees #6499
Conversation
stake-pool/cli/src/client.rs
Outdated
| /// Helper function to add a compute unit limit instruction to a given set | ||
| /// of instructions | ||
| /// | ||
| /// Returns true if the instruction was added, false if it wasn't. The false | ||
| /// case is used for offline signing, where we cannot access the network. |
There was a problem hiding this comment.
think this function was adapted from elsewhere and not the comment? since it doesnt return a bool and this cli doesnt appear to have offline signing
There was a problem hiding this comment.
Oops, yep that was some bad copy-pasta, removed that bit
| // Overwrite the compute unit limit instruction with the actual units consumed | ||
| let compute_unit_limit = u32::try_from(units_consumed)?; | ||
| instructions | ||
| .last_mut() | ||
| .expect("Compute budget instruction was added earlier") | ||
| .data = ComputeBudgetInstruction::set_compute_unit_limit(compute_unit_limit).data; |
There was a problem hiding this comment.
should we add a bit for fudge factor in case cu cost is nondeterministic? not sure how likely this is. if theres a complex struct that needs to be parsed and it changes before the transaction lands, it could be possible in theory
There was a problem hiding this comment.
Yeah I've thought about this a bit... we could do this through a new variant of --with-compute-unit-limit where you can say something like SIMULATED+10 which means "add 10 to whatever the simulation results say". I'm working on something for the token cli towards that direction.
My thinking was to start with this, and once people complain about it, we can add it.
There was a problem hiding this comment.
if it was a matter of poor ux for a new feature i would agree, but this
if let Some(compute_unit_limit) = config.compute_unit_limit {
instructions.push(ComputeBudgetInstruction::set_compute_unit_limit(
compute_unit_limit,
));
} else {
add_compute_unit_limit_from_simulation(
&config.rpc_client,
&mut instructions,
&config.fee_payer.pubkey(),
&recent_blockhash,
)?;
}adds a simulated compute limit to transactions when the flag isnt used, making existing flows more brittle. perhaps this was unintentional?
There was a problem hiding this comment.
It was intentional, but then I've gone back the other way with #6550 -- what do you think about that kind of approach?
TLDR you have --with-compute-unit-limit, you can specify SIMULATED or a number. If it's not specified, it doesn't use anything. But if you specify --with-compute-unit-price without --with-compute-unit-limit, it fails. What do you think about that approach?
There was a problem hiding this comment.
i like the design you have there for sure!
There was a problem hiding this comment.
Ok I'll do that here too then
stake-pool/cli/src/main.rs
Outdated
| .arg(compute_unit_price_arg().validator(is_parsable::<u64>)) | ||
| .arg( | ||
| Arg::with_name(COMPUTE_UNIT_LIMIT_ARG.name) | ||
| .long(COMPUTE_UNIT_LIMIT_ARG.long) | ||
| .takes_value(true) | ||
| .value_name("COMPUTE-UNIT-LIMIT") | ||
| .help(COMPUTE_UNIT_LIMIT_ARG.help) | ||
| .validator(is_parsable::<u32>) | ||
| ) |
There was a problem hiding this comment.
these need to be .global(true), they dont show up for subcommands when running the cli
There was a problem hiding this comment.
Ah yes, thanks! It turns out I misunderstood how global worked this whole time
|
Ok, I've updated this to be similar to the token CLI work, let me know how it looks! |
|
missed your latest push, tested create pool by hand and with no flags it doesnt add compute ixns, with budget flag it demands limit, budget with limit simulated works, budget with limit with number works, budget alone works |
Problem
Due to network congestion mainnet and the lack of priority fee support, stake pool users can't interact with their pool on mainnet.
Solution
A few things to provide a full solution:
--with-compute-unit-priceflag to set micro-lamports per CU for all transactions in a command--with-compute-unit-limitflag to set the number of CUs for all transactions in a command--with-compute-unit-limitis not set, simulate the transaction and use the number of compute units reportedAs part of this, the idea is to funnel all transaction creation through
checked_transaction_with_signerorchecked_transaction_with_signers_and_additional_fee, which will do the simulation work and add the compute budget instructions.Since there is some refactor, it might be easier to go through this commit by commit.