-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i like the design you have there for sure!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these need to be .global(true)
, they dont show up for subcommands when running the cli
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-price
flag to set micro-lamports per CU for all transactions in a command--with-compute-unit-limit
flag to set the number of CUs for all transactions in a command--with-compute-unit-limit
is 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_signer
orchecked_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.