Add `--with-compute-unit-price` to cli program deploy commands #364

jstarry · 2024-03-21T16:20:56Z

Problem

It's not possible to set a compute unit price or limit on deploy transactions. This causes deploys to be very slow when transaction priority fees increase on the cluster.

Summary of Changes

Add compute unit price and limit instructions to deploy instructions when the --with-compute-unit-price flag is passed for solana program deploy and solana program write-buffer
Compute unit limit is set by first simulating each type of deploy transaction and then using the number of compute units consumed

Fixes #21

jstarry · 2024-03-21T16:27:05Z

cli/src/program.rs

+    let mut compute_unit_limit: u32 = 300; // 2 * 150 for compute budget ixs
+    for ix in ixs.iter() {
+        if ix.program_id == bpf_loader_upgradeable::id() {
+            compute_unit_limit += 2370;
+            match ix.data.first() {
+                Some(2) // DeployWithMaxDataLen
+                | Some(6) // ExtendProgram
+                => {
+                    // add compute for native system program invocation
+                    compute_unit_limit += 150;
+                }
+                _ => {}
+            }
+        } else if ix.program_id == system_program::id() {
+            compute_unit_limit += 150;
+        } else {
+            panic!(
+                "Couldn't estimate compute unit limit for unexpected program {}",
+                ix.program_id
+            );
+        }
+    }


I'm not really in favor of having compute unit limit logic here in the cli client. I'd prefer to get the units consumed from the simulate transaction call.

Yeah that would be great, I've been trying to figure out a nice API to do it. No need to change what you've done though

I would feel better knowing that deploys won't get broken in the future if we increase the compute unit limit for certain instructions. Let me know what you think of the simulation code!

codecov-commenter · 2024-03-21T17:57:30Z

Codecov Report

Attention: Patch coverage is 36.44860% with 68 lines in your changes are missing coverage. Please review.

Project coverage is 81.9%. Comparing base (b2f4fb3) to head (01faab4).
Report is 10 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##           master     #364     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         838      838             
  Lines      227368   227412     +44     
=========================================
+ Hits       186238   186266     +28     
- Misses      41130    41146     +16

joncinque

Looks great! Just a nit on the test, but nothing stopping this from going in

joncinque · 2024-03-21T22:42:06Z

cli/src/program.rs

+    let mut compute_unit_limit: u32 = 300; // 2 * 150 for compute budget ixs
+    for ix in ixs.iter() {
+        if ix.program_id == bpf_loader_upgradeable::id() {
+            compute_unit_limit += 2370;
+            match ix.data.first() {
+                Some(2) // DeployWithMaxDataLen
+                | Some(6) // ExtendProgram
+                => {
+                    // add compute for native system program invocation
+                    compute_unit_limit += 150;
+                }
+                _ => {}
+            }
+        } else if ix.program_id == system_program::id() {
+            compute_unit_limit += 150;
+        } else {
+            panic!(
+                "Couldn't estimate compute unit limit for unexpected program {}",
+                ix.program_id
+            );
+        }
+    }


Yeah that would be great, I've been trying to figure out a nice API to do it. No need to change what you've done though

joncinque · 2024-03-21T22:50:09Z

cli/src/program.rs

+            compute_unit_limit += 2370;
+            match ix.data.first() {
+                Some(2) // DeployWithMaxDataLen
+                | Some(6) // ExtendProgram


Thanks for thinking about the future by including this even though it isn't used in this PR ❤️

joncinque · 2024-03-21T22:51:06Z

cli/src/program.rs

+                    // add compute for native system program invocation
+                    compute_unit_limit += 150;


Confirming this is correct

joncinque · 2024-03-21T22:51:44Z

cli/src/program.rs

+    let (initial_message, write_messages, balance_needed) = if let Some(buffer_signer) =
+        buffer_signer


thanks rustfmt 🙃

joncinque · 2024-03-21T23:04:02Z

cli/tests/program.rs

+            match try_from_slice_unchecked(&tx.message.instructions[0].data) {
+                Ok(ComputeBudgetInstruction::SetComputeUnitPrice(price))
+                    if price == compute_unit_price => {}
+                ix => assert!(false, "unexpected ix {ix:?}"),
+            }


nit: I believe all of these checks could use matches! to be simpler, ie:

Suggested change

match try_from_slice_unchecked(&tx.message.instructions[0].data) {

Ok(ComputeBudgetInstruction::SetComputeUnitPrice(price))

if price == compute_unit_price => {}

ix => assert!(false, "unexpected ix {ix:?}"),

}

assert!(

matches!(

try_from_slice_unchecked(&tx.message.instructions[0].data),

Ok(ComputeBudgetInstruction::SetComputeUnitPrice(price)) if price == compute_unit_price

),

"unexpected ix {ix:?}"

);

joncinque

Still looks great! Just a couple of nits, but nothing to stop this from going in

cli/src/program.rs

mergify · 2024-03-22T15:15:02Z

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.

* add set compute units arg for program deploy * update master changes * remove duplicates * fixes and tests * remove extra lines * feedback * Use simulation to determine compute units consumed * feedback --------- Co-authored-by: NagaprasadVr <nagaprasadvr246@gmail.com> (cherry picked from commit 2ee606d)

… (backport of #364) (#392) Add `--with-compute-unit-price` to cli program deploy commands (#364) * add set compute units arg for program deploy * update master changes * remove duplicates * fixes and tests * remove extra lines * feedback * Use simulation to determine compute units consumed * feedback --------- Co-authored-by: NagaprasadVr <nagaprasadvr246@gmail.com> (cherry picked from commit 2ee606d) Co-authored-by: Justin Starry <justin@anza.xyz>

tarunjais28 · 2024-07-19T02:32:06Z

Hi @joncinque, after adding --with-compute-unit-price on solana program deploy command still it is taking a days and not getting deployed to mainnet, is there a way for fast mainnet deployment?
solana program deploy target/deploy/program.so --url https://api.mainnet-beta.solana.com --with-compute-unit-price 1000000 --max-sign-attempts 1000

It's more than a day and still the program is not deployed

joncinque · 2024-07-19T09:37:45Z

@tarunjais28 it still can be difficult to get transactions to the leader due to stake-weighted quality of service setups, but you're doing the right thing. You can also try adding the --use-rpc flag to send transactions through RPC instead of TPU. If you have additional questions, please ask them on the Solana Stack Exchange https://solana.stackexchange.com/

Nagaprasadvr and others added 4 commits March 21, 2024 16:18

add set compute units arg for program deploy

ab14e33

update master changes

d36c451

remove duplicates

9587c2a

fixes and tests

1ee834e

jstarry requested a review from joncinque March 21, 2024 16:20

jstarry mentioned this pull request Mar 21, 2024

add tx_priority_fee argument for program deploy subcommand #312

Closed

remove extra lines

498fb0e

jstarry commented Mar 21, 2024

View reviewed changes

jstarry mentioned this pull request Mar 21, 2024

Implement prioritization fees for program deploy using cli #239

Closed

joncinque previously approved these changes Mar 21, 2024

View reviewed changes

feedback

01faab4

jstarry dismissed joncinque’s stale review via 01faab4 March 22, 2024 08:11

Use simulation to determine compute units consumed

083223e

joncinque previously approved these changes Mar 22, 2024

View reviewed changes

cli/src/program.rs Outdated Show resolved Hide resolved

cli/src/program.rs Outdated Show resolved Hide resolved

feedback

5c355ad

jstarry dismissed joncinque’s stale review via 5c355ad March 22, 2024 15:14

jstarry added automerge automerge Merge this Pull Request automatically once CI passes v1.18 labels Mar 22, 2024

joncinque approved these changes Mar 22, 2024

View reviewed changes

mergify bot merged commit 2ee606d into anza-xyz:master Mar 22, 2024
50 checks passed

mergify bot mentioned this pull request Mar 22, 2024

v1.18: Add --with-compute-unit-price to cli program deploy commands (backport of #364) #392

Merged

willhickey mentioned this pull request Mar 28, 2024

v1.18 commits - please ignore #475

Closed

This was referenced Apr 1, 2024

Add priority fee option/instruction for solana program deploy solana-labs/solana#34444

Closed

[Feature Request] Blockhash expired. 5 retries remaining. solana-labs/solana#30642

Closed

anza-xyz locked as resolved and limited conversation to collaborators Jul 19, 2024

jstarry deleted the cli/deploy-priority-fee branch July 19, 2024 10:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `--with-compute-unit-price` to cli program deploy commands #364

Add `--with-compute-unit-price` to cli program deploy commands #364

jstarry commented Mar 21, 2024 •

edited

Loading

jstarry Mar 21, 2024

joncinque Mar 21, 2024

jstarry Mar 22, 2024

codecov-commenter commented Mar 21, 2024 •

edited

Loading

joncinque left a comment

joncinque Mar 21, 2024

joncinque Mar 21, 2024

joncinque Mar 21, 2024

joncinque Mar 21, 2024

joncinque Mar 21, 2024

joncinque left a comment

mergify bot commented Mar 22, 2024

tarunjais28 commented Jul 19, 2024 •

edited

Loading

joncinque commented Jul 19, 2024

		// add compute for native system program invocation
		compute_unit_limit += 150;

		let (initial_message, write_messages, balance_needed) = if let Some(buffer_signer) =
		buffer_signer

Add --with-compute-unit-price to cli program deploy commands #364

Add --with-compute-unit-price to cli program deploy commands #364

Conversation

jstarry commented Mar 21, 2024 • edited Loading

Problem

Summary of Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 21, 2024 • edited Loading

Codecov Report

joncinque left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joncinque left a comment

Choose a reason for hiding this comment

mergify bot commented Mar 22, 2024

tarunjais28 commented Jul 19, 2024 • edited Loading

joncinque commented Jul 19, 2024

Add `--with-compute-unit-price` to cli program deploy commands #364

Add `--with-compute-unit-price` to cli program deploy commands #364

jstarry commented Mar 21, 2024 •

edited

Loading

codecov-commenter commented Mar 21, 2024 •

edited

Loading

tarunjais28 commented Jul 19, 2024 •

edited

Loading