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

Disallow deployment of deprecated _sol_alloc_free syscall #24986

Merged
merged 2 commits into from
May 11, 2022

Conversation

jackcmay
Copy link
Contributor

@jackcmay jackcmay commented May 4, 2022

Problem

Programs are now built with an in-program and customizable heap implementation. There is no more need for the built-in bump allocator syscall. To end-of-life that syscall, future deployments depending on it should fail.

Summary of Changes

  • Add C in-program allocator
  • Prevent deployments of programs that depend on _sol_alloc_free

Fixes #
Feature Gate Issue: #24985

@jackcmay jackcmay added the feature-gate Pull Request adds or modifies a runtime feature gate label May 4, 2022
@jackcmay jackcmay requested a review from Lichtso May 4, 2022 22:28
@jackcmay jackcmay force-pushed the no-new-alloc-free-dependencies branch 3 times, most recently from 9c9afc1 to 70c0532 Compare May 5, 2022 21:44
@@ -176,6 +178,13 @@ impl ProgramSubCommands for App<'_, '_> {
.long("allow-excessive-deploy-account-balance")
.takes_value(false)
.help("Use the designated program id even if the account already holds a large balance of SOL")
)
.arg(
Arg::with_name("enable_sol_alloc_free")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@CriesofCarrots, @jstarry By default after these changes are released, the cli will no longer deploy programs that depend on sol_alloc_free syscall unless the caller specifies this hidden flag. This should be no issue for rust developers because rust programs haven't used this syscall for a long time. C programs that explicitly call alloc/free will fail to deploy unless they pick up the latest SDK. What do you think about this behavior, think we should instead default to allowing it for now?

Copy link
Member

Choose a reason for hiding this comment

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

My opinion is that the earlier we prevent clients from hitting code paths that will soon be feature gated off, the better. So I'm of the opinion that we don't need to have any flag at all. But if there's some use-case that you're thinking of, a hidden flag to enable seems fine. Definitely in favor of defaulting to disallow here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Justin, and also lean toward no flag at all.
If you do feel the hidden flag is necessary, can we add some documentation about when it will no longer work, and when it can be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only reason we would need the flag is if someone is building with an older tool chain but deploying it with this version and doing so before the alloc_free disable feature gate is activated

@jackcmay jackcmay force-pushed the no-new-alloc-free-dependencies branch 2 times, most recently from ea8d115 to 519ce6a Compare May 6, 2022 20:16
@jackcmay jackcmay force-pushed the no-new-alloc-free-dependencies branch from 519ce6a to 71faf4a Compare May 10, 2022 22:10
@jackcmay jackcmay force-pushed the no-new-alloc-free-dependencies branch from 71faf4a to 623f7bf Compare May 10, 2022 22:22
@jackcmay jackcmay added the automerge Merge this Pull Request automatically once CI passes label May 11, 2022
@codecov
Copy link

codecov bot commented May 11, 2022

Codecov Report

Merging #24986 (623f7bf) into master (69a0ff9) will increase coverage by 0.0%.
The diff coverage is 83.1%.

@@            Coverage Diff            @@
##           master   #24986     +/-   ##
=========================================
  Coverage    82.0%    82.1%             
=========================================
  Files         598      610     +12     
  Lines      165882   167804   +1922     
=========================================
+ Hits       136125   137853   +1728     
- Misses      29757    29951    +194     

@mergify mergify bot merged commit 8f1d4c1 into solana-labs:master May 11, 2022
jackcmay added a commit to jackcmay/solana that referenced this pull request May 12, 2022
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request May 12, 2022
jeffwashington pushed a commit to jackcmay/solana that referenced this pull request May 12, 2022
jeffwashington pushed a commit to jackcmay/solana that referenced this pull request May 12, 2022
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request May 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this Pull Request automatically once CI passes feature-gate Pull Request adds or modifies a runtime feature gate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants