Skip to content
This repository was archived by the owner on Mar 11, 2025. It is now read-only.

stake-pool-cli: Add support for priority fees #6499

Merged
joncinque merged 7 commits into from
Apr 25, 2024
Merged

stake-pool-cli: Add support for priority fees #6499

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f7813d9
stake-pool-cli: Add function to simulate tx for CUs used
joncinque Mar 26, 2024
5667136
Add compute unit price and limit args
joncinque Mar 26, 2024
9be8d75
Refactor to always use `checked_transaction_*`
joncinque Mar 26, 2024
20906a7
Add compute budget instructions
joncinque Mar 26, 2024
6b1b878
Fix creation to create second transaction *after* sending first one
joncinque Mar 26, 2024
d2c03d7
Address feedback
joncinque Apr 4, 2024
d99baa9
Specify "SIMULATED" explicitly
joncinque Apr 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 38 additions & 2 deletions stake-pool/cli/src/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,19 @@ use {
rpc_config::{RpcAccountInfoConfig, RpcProgramAccountsConfig},
rpc_filter::{Memcmp, RpcFilterType},
},
solana_program::{borsh1::try_from_slice_unchecked, program_pack::Pack, pubkey::Pubkey, stake},
solana_program::{
borsh1::try_from_slice_unchecked, hash::Hash, instruction::Instruction, message::Message,
program_pack::Pack, pubkey::Pubkey, stake,
},
solana_sdk::{compute_budget::ComputeBudgetInstruction, transaction::Transaction},
spl_stake_pool::{
find_withdraw_authority_program_address,
state::{StakePool, ValidatorList},
},
std::collections::HashSet,
};

type Error = Box<dyn std::error::Error>;
pub(crate) type Error = Box<dyn std::error::Error>;

pub fn get_stake_pool(
rpc_client: &RpcClient,
Expand Down Expand Up @@ -146,3 +150,35 @@ pub(crate) fn get_all_stake(
.map(|(address, _)| address)
.collect())
}

/// Helper function to add a compute unit limit instruction to a given set
/// of instructions
pub(crate) fn add_compute_unit_limit_from_simulation(
rpc_client: &RpcClient,
instructions: &mut Vec<Instruction>,
payer: &Pubkey,
blockhash: &Hash,
) -> Result<(), Error> {
// add a max compute unit limit instruction for the simulation
const MAX_COMPUTE_UNIT_LIMIT: u32 = 1_400_000;
instructions.push(ComputeBudgetInstruction::set_compute_unit_limit(
MAX_COMPUTE_UNIT_LIMIT,
));

let transaction = Transaction::new_unsigned(Message::new_with_blockhash(
instructions,
Some(payer),
blockhash,
));
let simulation_result = rpc_client.simulate_transaction(&transaction)?.value;
let units_consumed = simulation_result
.units_consumed
.ok_or("No units consumed on simulation")?;
// 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;
Comment on lines +177 to +182
Copy link
Contributor

@2501babe 2501babe Apr 4, 2024

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

Copy link
Contributor Author

@joncinque joncinque Apr 4, 2024

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.

Copy link
Contributor

@2501babe 2501babe Apr 5, 2024

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?

Copy link
Contributor Author

@joncinque joncinque Apr 5, 2024

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?

Copy link
Contributor

@2501babe 2501babe Apr 5, 2024

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!

Copy link
Contributor Author

@joncinque joncinque Apr 6, 2024

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

Ok(())
}
Loading