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

cli: (program/buffer deploy) introduce --fee-payer argument #34341

Merged

Conversation

norwnd
Copy link
Contributor

@norwnd norwnd commented Dec 6, 2023

Problem

solana program deploy and solana program write-buffer could use a --fee-payer argument.

Summary of Changes

This adds --fee-payer to solana program deploy and solana program write-buffer cli commands. It's effectively a spin-off from #33860.

@mergify mergify bot added community Community contribution need:merge-assist labels Dec 6, 2023
@mergify mergify bot requested a review from a team December 6, 2023 17:23
Copy link
Contributor

@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 good for the most part!

As a reminder, please avoid putting in unrelated changes, it makes the review much more complicated for me. These code paths have little gotchas all over the place, so changing a parameter can have unintended consequences, so I have to trace every single possible path to make sure it's ok.

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

let result = if do_deploy {
let (min_rent_exempt_program_balance, min_rent_exempt_program_data_balance) = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, why is this change for the rent-exemption of the base program included? In general, please avoid putting in unrelated changes, it makes each review much more difficult.

Copy link
Contributor Author

@norwnd norwnd Dec 7, 2023

Choose a reason for hiding this comment

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

Sorry, why is this change for the rent-exemption of the base program included?

I believe this rewrite isn't changing any functionality, just a refactoring.

The original motivation was the need to accommodate --sign-only for deploy, it's not really necessary now (after we decided to ditch offline signing for deploy), but I was also hoping to make it a bit more readable (by grouping related stuff together) so I don't have to "untangle it" again next time I have to go through that code.

For reviewer(s) it's not as pleasant of a thing ... I get that, trying to find the right balance between those ^

PS: when lots of small changes touch same parts of the code it's typically more efficient and safe (no merge conflicts to resolve, can test the final result just once while aware of all the relevant context) to change it all in one fell swoop, that's why it's all here.

Copy link
Contributor

Choose a reason for hiding this comment

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

PS: when lots of small changes touch same parts of the code it's typically more efficient and safe (no merge conflicts to resolve, can test the final result just once while aware of all the relevant context) to change it all in one fell swoop, that's why it's all here.

While it's definitely more efficient for the developer to just make all the changes at once, it's much less efficient for the maintainers because they need to deal with a refactor and a code change at the same time.

Those kinds of pull requests are often the most dangerous. If you look back at the bugs introduced in codebases, it's almost always because of a refactor that also changed behavior. Because they're often big, the reviewer doesn't look at it so closely, and assumes that tests will catch any issues.

While you can work however you want, when it comes to these repos, if you do a refactor, you will be asked to do it in a separate PR.

cli/src/program.rs Show resolved Hide resolved
program_len: usize,
programdata_len: usize,
minimum_balance: u64,
min_rent_exempt_program_balance: u64,
Copy link
Contributor

@joncinque joncinque Dec 6, 2023

Choose a reason for hiding this comment

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

Can you revert this change? There are some tricky code paths around here when it comes to deployments for the non-upgradeable loader, and this creates more footguns.

In the case of a buffer that already exists, they need to pass the program's rent-exemption in min_rent_exempt_program_data_balance, but in the case of a new deployment, they would need to pass that value in min_rent_exempt_program_balance, which is very mistake-prone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how this change makes it more of a foot-gun, if anything I was hoping to remove a foot-gun or two with it - by making it explicit what fn do_process_program_write_and_deploy actually expects as inputs,

it's not strictly needed anymore (it was for enabling --sign-only in deploy) but it kinda makes me sad to discover this code is "that rigid", unless it's not expected to change much in the future I'd say it's a sign that refactoring(s) are needed to untangle it a bit.

So, do you want me to revert the parts touching "balances" ? Or something else ?

Copy link
Contributor

@joncinque joncinque Dec 7, 2023

Choose a reason for hiding this comment

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

It's not easy to know, because this affects some old code paths that were removed. The two accounts for program and program-data are only for the upgradeable loader. The non-upgradeable loader doesn't have this concept, which is why this function only uses one parameter, and why the min_rent_exempt_program_balance is only fetched when needed for the upgradeable loader.

This was removed in #28399, but we have a project that might need to bring it back by interacting with the non-upgradeable loader, which is why I want to keep these code paths as they were, to avoid breaking that case.

Since loader-v4 exists, this code is not expected to change in the future, which is why it's less helpful to refactor at this point. The code here is unfortunately very brittle and prone to breaking, which is why I keep insisting on minimal changes.

Edit: so the part to revert is separating out the two required balances -- please keep it all in the one parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, that 1dca8e5 look about right ?

@joncinque joncinque added the CI Pull Request is ready to enter CI label Dec 7, 2023
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Dec 7, 2023
Copy link
Contributor

@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.

Great work, thanks so much!

Copy link

codecov bot commented Dec 7, 2023

Codecov Report

Merging #34341 (1dca8e5) into master (5c00a33) will increase coverage by 0.0%.
Report is 17 commits behind head on master.
The diff coverage is 56.9%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #34341   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         819      819           
  Lines      220422   220437   +15     
=======================================
+ Hits       180652   180734   +82     
+ Misses      39770    39703   -67     

@joncinque joncinque merged commit 8c6239c into solana-labs:master Dec 7, 2023
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution need:merge-assist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants