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 skip preflight to CLI #3339

Merged
merged 7 commits into from
Nov 6, 2024
Merged

Conversation

Woody4618
Copy link

Problem

CLI was missing the option to set skip-preflight option.
This is especially useful because some RPC providers are not using swqos when preflight is enabled.
This is speeding up program deployments by a lot using these RPC providers.

Summary of Changes

I added skip-preflight as global flag and changed all the places where transactions are sent to use that flag.
Some test had the skip preflight flag set to true, but it was not actually used. So these tests that were parsing the responses are adjusted to handle both cases.

Refactoring?

In an additional step if would maybe be nice to remove CommitmentConfig from the CliConfig and move it into the RpcSendTransactionConfig. But that looked like a bit of a bigger refactoring so I didnt include it.
Another option would also be moving RpcSendTransactionConfig directly into RpcClient to enable it everywhere. But that is an even bigger refactoring.

@Woody4618 Woody4618 changed the title Add skip preflight Add skip preflight to CLI Oct 28, 2024
@mergify mergify bot requested a review from a team October 28, 2024 23:57
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.

Really good work overall, thanks for the contribution! Just a few things to fixup, then we can land this

cargo-registry/src/client.rs Show resolved Hide resolved
cli/src/clap_app.rs Outdated Show resolved Hide resolved
rpc-client/src/nonblocking/rpc_client.rs Outdated Show resolved Hide resolved
client/src/send_and_confirm_transactions_in_parallel.rs Outdated Show resolved Hide resolved
cli/tests/transfer.rs Outdated Show resolved Hide resolved
cli/src/program_v4.rs Show resolved Hide resolved
cli/src/program.rs Show resolved Hide resolved
cli/tests/program.rs Outdated Show resolved Hide resolved
@Woody4618
Copy link
Author

Thank you very much for the review! Pushed the changes :)

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.

Just a couple of little nits, then we can get this in!

client/src/send_and_confirm_transactions_in_parallel.rs Outdated Show resolved Hide resolved
client/src/send_and_confirm_transactions_in_parallel.rs Outdated Show resolved Hide resolved
client/src/send_and_confirm_transactions_in_parallel.rs Outdated Show resolved Hide resolved
client/src/send_and_confirm_transactions_in_parallel.rs Outdated Show resolved Hide resolved
client/src/send_and_confirm_transactions_in_parallel.rs Outdated Show resolved Hide resolved
@joncinque joncinque added the CI Pull Request is ready to enter CI label Nov 6, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Nov 6, 2024
@joncinque
Copy link

Be sure to run ./cargo nightly fmt --all from the top of the repo, looks like there are some formatting issues. You can also run ./scripts/cargo-clippy-nightly.sh for any lint issues.

@Woody4618
Copy link
Author

It looks like the linter does not want me to use deprecated functions. But since its the fallback. How can i not use them?
image

@joncinque
Copy link

It looks like the linter does not want me to use deprecated functions. But since its the fallback. How can i not use them?

You can add #[allow(deprecated)] for the deprecated functions

@Woody4618
Copy link
Author

Can i Ignore this? when running ./scripts/cargo-clippy-nightly.sh

image

@joncinque
Copy link

Can i Ignore this? when running ./scripts/cargo-clippy-nightly.sh

You can always push your change and see if it passes CI, but it's better to run things locally. Are you missing a protobuf compiler on your machine?

@Woody4618
Copy link
Author

I have it installed yes, via homebrew.
protoc --version
libprotoc 28.3
Looks like the linting worked though.

@joncinque joncinque added the CI Pull Request is ready to enter CI label Nov 6, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Nov 6, 2024
@joncinque
Copy link

Ah darn, sorry, I forgot to mention -- can you add an entry in the changelog for this feature? Under 2.2.0, how about something like:

  * Changes
    * CLI:
      * Add global `--skip-preflight` option for skipping preflight checks on all transactions sent through RPC. This flag, along with `--use-rpc`, can improve success rate with program deployments using the public RPC nodes.

@Woody4618
Copy link
Author

Added the changelog entry

@joncinque
Copy link

Added the changelog entry

Might need a rebase -- there's a conflict

Copy link

mergify bot commented Nov 6, 2024

If this PR represents a change to the public RPC API:

  1. Make sure it includes a complementary update to rpc-client/ (example)
  2. Open a follow-up PR to update the JavaScript client @solana/web3.js (example)

Thank you for keeping the RPC clients in sync with the server API @Woody4618.

Copy link

mergify bot commented Nov 6, 2024

The Firedancer team maintains a line-for-line reimplementation of the
native programs, and until native programs are moved to BPF, those
implementations must exactly match their Agave counterparts.
If this PR represents a change to a native program implementation (not
tests), please include a reviewer from the Firedancer team. And please
keep refactors to a minimum.

@joncinque
Copy link

@Woody4618 please double-check it, but this should be correct

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.

Thanks for your contribution!

@joncinque joncinque requested a review from febo November 6, 2024 12:30
@joncinque
Copy link

@febo can I get an approval here? I cleaned up the branch after a rebase, and it won't let me merge since I was the last pusher

@joncinque joncinque added the automerge automerge Merge this Pull Request automatically once CI passes label Nov 6, 2024
@mergify mergify bot merged commit 2dc1640 into anza-xyz:master Nov 6, 2024
49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge automerge Merge this Pull Request automatically once CI passes community need:merge-assist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants