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

Implement prioritization fees for program deploy using cli #239

Conversation

godmodegalactus
Copy link

Problem

With cluster with high traffic where transactions are not processed without any prioritization fees. There is no way to set prioritization fee for program deployment.

Summary of Changes

Adding a way to set prioritization fees while doing program deployment using CLI.
Fixing a minor bug related to failed to join all the futures while program deployment.

Fixes #

@godmodegalactus
Copy link
Author

No idea why clippy is failing, works fine on local. Could you give me access to solana kite.

fixing minor bug while cli program deploy
@godmodegalactus godmodegalactus force-pushed the implement_prioritization_fees_for_program_deploy_using_cli branch from ab01098 to 9c9117f Compare March 14, 2024 10:18
Copy link

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looks really good overall, thanks for your contribution! Mostly small things to fixup

@@ -90,6 +91,7 @@ pub enum ProgramCliCommand {
max_len: Option<usize>,
allow_excessive_balance: bool,
skip_fee_check: bool,
prioritization_fees: Option<u64>,

Choose a reason for hiding this comment

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

nit: Can you call this compute_unit_price?

@@ -99,6 +101,7 @@ pub enum ProgramCliCommand {
sign_only: bool,
dump_transaction_message: bool,
blockhash_query: BlockhashQuery,
prioritization_fees: Option<u64>,

Choose a reason for hiding this comment

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

same nit: Can you call this compute_unit_price?

@@ -108,6 +111,7 @@ pub enum ProgramCliCommand {
buffer_authority_signer_index: SignerIndex,
max_len: Option<usize>,
skip_fee_check: bool,
prioritization_fees: Option<u64>,

Choose a reason for hiding this comment

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

same nit: Can you call this compute_unit_price?

@@ -266,6 +281,17 @@ impl ProgramSubCommands for App<'_, '_> {
"Upgrade authority [default: the default configured keypair]",
),
)
.arg(

Choose a reason for hiding this comment

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

@@ -228,6 +232,17 @@ impl ProgramSubCommands for App<'_, '_> {
[default: the length of the original deployed program]",
),
)
.arg(

Choose a reason for hiding this comment

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

Comment on lines +2450 to +2451
let prioritization_ix =
prioritization_fees.map(compute_budget::ComputeBudgetInstruction::set_compute_unit_price);

Choose a reason for hiding this comment

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

Same here, let's include the compute unit limit instruction

{
complete_partial_program_init(
&bpf_loader_upgradeable::id(),
let (initial_message, write_messages, balance_needed) = if let Some(buffer_signer) =

Choose a reason for hiding this comment

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

Maybe I'm going cross-eyed this morning, but did anything actually change over here? It looks like only whitespace diffs

)
let buffer_signer_pubkey = buffer_signer.pubkey();
let upgrade_authority_pubkey = upgrade_authority.pubkey();
let create_msg = |offset: u32, bytes: Vec<u8>, prioritization_ix: Option<Instruction>| {

Choose a reason for hiding this comment

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

Same here, let's remove the Option<Instruction> and just capture from the outside

(initial_message, write_messages, balance_needed)
} else {
(None, vec![], 0)
};

// Create and add final message
let final_message = Message::new_with_blockhash(

Choose a reason for hiding this comment

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

Can you also include the priority fees on this transaction?

@@ -585,16 +585,16 @@ pub fn process_deploy_program(
};

// Create and add write messages
let create_msg = |offset: u32, bytes: Vec<u8>| {
let create_msg = |offset: u32, bytes: Vec<u8>, _prioritization_fees_ix: Option<Instruction>| {

Choose a reason for hiding this comment

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

Once create_msg doesn't take the Option<Instruction>, you'll be able to revert these changes

@joncinque joncinque added CI Pull Request is ready to enter CI v1.18 labels Mar 14, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Mar 14, 2024
Copy link

mergify bot commented Mar 14, 2024

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

@joncinque
Copy link

The clippy error is unrelated to your change, don't worry about that.

@joncinque
Copy link

CI seems to be picking up some old errors, do you need to rebase?

@Nagaprasadvr
Copy link

Nagaprasadvr commented Mar 15, 2024

this is a duplicate of #144 opened last week
heres the issue
#21
@joncinque @godmodegalactus

@godmodegalactus
Copy link
Author

this is a duplicate of #144 opened last week heres the issue #21 @joncinque @godmodegalactus

Have you already finished working on it ?

@godmodegalactus
Copy link
Author

CI seems to be picking up some old errors, do you need to rebase?

Yes had rebased it, will try it again.

@Nagaprasadvr
Copy link

this is a duplicate of #144 opened last week heres the issue #21 @joncinque @godmodegalactus

Have you already finished working on it ?

Waiting for review

@godmodegalactus
Copy link
Author

godmodegalactus commented Mar 15, 2024

this is a duplicate of #144 opened last week heres the issue #21 @joncinque @godmodegalactus

Have you already finished working on it ?

Waiting for review

Then go ahead merging your PR, let this PR stay open for a while because people might use this branch to deploy their programs.

@jstarry
Copy link

jstarry commented Mar 21, 2024

Fixing here: #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.

5 participants