Skip to content
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

add tx_priority_fee argument for program deploy subcommand #312

Conversation

Nagaprasadvr
Copy link

@Nagaprasadvr Nagaprasadvr commented Mar 19, 2024

Issue - #21

Add Transaction priority fee argument in terms of micro LAMPORTS to solana program deploy subcommand

Problem
solana program deploy command could take a lot of time to finish due to program code size .Transaction execution speeds
can be increased or txs can be prioritised by setting compute_unit_price (priority fee) for every tx this price is set and txs
are executes faster than normal therefore making program deployment faster.
usage - solana program deploy --tx-priority-fee 1000000

Summary of Changes
solana/cli/program - added tx_priority_fee logic

@mergify mergify bot requested a review from a team March 19, 2024 04:41
@Nagaprasadvr
Copy link
Author

apologies for creating a new pr
#144 was closed because i had forked solana-labs repo not agave so had to create this pr again!

@Nagaprasadvr
Copy link
Author

Nagaprasadvr commented Mar 19, 2024

found a workaround for setting compute units based on programs
@jstarry @t-nelson @tao-stones @joncinque

i have added 2000 cu when program is neither system_program or bpf_upgradable_loader

image

@Nagaprasadvr Nagaprasadvr force-pushed the feat-add-tx-priority-fee-and-max-retries-for-program-deploy branch from 3b368f8 to 8e10829 Compare March 19, 2024 13:08
Copy link

@jstarry jstarry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of the set_compute_budget_ixs_if_needed call-sites pass the actual message instructions into the function so the compute_unit_limit isn't being set properly.

Comment on lines +2285 to +2288
let mut initial_instructions: Vec<Instruction> = Vec::new();

set_compute_budget_ixs_if_needed(&mut initial_instructions, &compute_unit_price);
initial_instructions.extend(ixs);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compute budget instructions should only be added if !ixs.is_empty()

Comment on lines +2447 to +2450
let mut initial_instructions: Vec<Instruction> = Vec::new();

(initial_message, write_messages, balance_needed)
set_compute_budget_ixs_if_needed(&mut initial_instructions, &compute_unit_price);
initial_instructions.extend(ixs);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, need to check if !ixs.is_empty() first

Comment on lines +2807 to +2812
ixs.push(ComputeBudgetInstruction::set_compute_unit_limit(
compute_units_required_for_ixs + compute_units_for_compute_budget_ixs,
));
ixs.push(ComputeBudgetInstruction::set_compute_unit_price(
*compute_unit_price,
));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compute budget instructions need to be inserted at the beginning of the instruction list

@@ -247,6 +247,7 @@ fn run_transactions_dos(
upgrade_authority_signer_index: 0,
is_final: true,
max_len: None,
compute_unit_price,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
compute_unit_price,
compute_unit_price: None,


let upgrade_authority = Keypair::new();

config.json_rpc_url = "https://api.devnet.solana.com".to_string();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not be using devnet for tests...

@jstarry
Copy link

jstarry commented Mar 21, 2024

@Nagaprasadvr thanks for getting this started but I'm going to take this over since the tests need a lot of work and I want this PR merged relatively quickly if possible.

@Nagaprasadvr
Copy link
Author

@Nagaprasadvr thanks for getting this started but I'm going to take this over since the tests need a lot of work and I want this PR merged relatively quickly if possible.

sure im happy with that ! program deployments have been failing recently for big programs hope this fix will help that !

@jstarry
Copy link

jstarry commented Mar 21, 2024

Moved to #364

@jstarry jstarry closed this Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants