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: fix program deployment elf verification issue #32448

Merged

Conversation

ananas-block
Copy link
Contributor

Problem

Deployment of solana programs using the alt bn128 syscall fails with cli versions 1.16.*, with the following error:

Error: ELF error: ELF error: Unresolved symbol (sol_alt_bn128_group_op) at instruction #16736 (ELF file offset 0x20a18

See this example for steps to replicate the issue:
https://github.com/ananas-block/exec_test/tree/altbn_deploy

Summary of Changes

  • replaces FeatureSet::default() with FeatureSet::all_enabled()

Fixes #
#32447

@mergify mergify bot added community Community contribution need:merge-assist labels Jul 10, 2023
@mergify mergify bot requested a review from a team July 10, 2023 22:21
@ananas-block ananas-block changed the title solana cli 1.16.3: fix program deployment elf verification issue solana cli: fix program deployment elf verification issue Jul 10, 2023
@ananas-block ananas-block changed the title solana cli: fix program deployment elf verification issue cli: fix program deployment elf verification issue Jul 10, 2023
@joncinque
Copy link
Contributor

It looks like this was modified in #31007 -- while default is certainly overly restrictive, I don't think all_enabled is the right option, since one of the reasons for the check is to make sure that you're not trying to use a syscall that hasn't been enabled yet.

I'm thinking the best option is to build the current feature set, similar to

solana/cli/src/feature.rs

Lines 802 to 835 in dd75d09

let mut features = vec![];
for feature_ids in feature_ids.chunks(MAX_MULTIPLE_ACCOUNTS) {
let mut feature_chunk = rpc_client
.get_multiple_accounts(feature_ids)
.unwrap_or_default()
.into_iter()
.zip(feature_ids)
.map(|(account, feature_id)| {
let feature_name = FEATURE_NAMES.get(feature_id).unwrap();
account
.and_then(status_from_account)
.map(|feature_status| CliFeature {
id: feature_id.to_string(),
description: feature_name.to_string(),
status: feature_status,
})
.unwrap_or_else(|| {
inactive = true;
CliFeature {
id: feature_id.to_string(),
description: feature_name.to_string(),
status: CliFeatureStatus::Inactive,
}
})
})
.filter(|feature| match (filter, &feature.status) {
(Some(min_activation), CliFeatureStatus::Active(activation)) => {
activation > &min_activation
}
_ => true,
})
.collect::<Vec<_>>();
features.append(&mut feature_chunk);
}
, and then pass that in.

@Lichtso what do you think? Since you made the FeatureSet::default() change, maybe you'll have a good idea about how to improve this.

@Lichtso
Copy link
Contributor

Lichtso commented Jul 11, 2023

We could fetch the feature set from the network, but it would not change much I think. The loader program on the network verifies the deployment anyway. I am fine with switching it back to FeatureSet::all_enabled().

@joncinque
Copy link
Contributor

In that case, how about we create an issue to do this correctly, but merge this + backport to unblock people?

@Lichtso Lichtso added the v1.16 PRs that should be backported to v1.16 label Jul 12, 2023
@joncinque joncinque added the CI Pull Request is ready to enter CI label Jul 12, 2023
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Jul 12, 2023
@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

Merging #32448 (d4526a7) into master (3fa3d26) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           master   #32448   +/-   ##
=======================================
  Coverage    82.1%    82.1%           
=======================================
  Files         778      778           
  Lines      210051   210051           
=======================================
+ Hits       172529   172565   +36     
+ Misses      37522    37486   -36     

@joncinque joncinque merged commit 724e0fe into solana-labs:master Jul 12, 2023
mergify bot pushed a commit that referenced this pull request Jul 12, 2023
cli: fix progam deployment read_and_verify_elf
(cherry picked from commit 724e0fe)
joncinque pushed a commit that referenced this pull request Jul 12, 2023
…#32448) (#32463)

cli: fix program deployment elf verification issue  (#32448)

cli: fix progam deployment read_and_verify_elf
(cherry picked from commit 724e0fe)

Co-authored-by: ananas-block <[email protected]>
@ananas-block ananas-block deleted the cli-fix-program-deployment branch July 14, 2023 19:11
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 v1.16 PRs that should be backported to v1.16
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants