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

[sdk] Add ZK Elgamal Proof program feature gate #1679

Merged
merged 8 commits into from
Jun 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

40 changes: 40 additions & 0 deletions programs/sbf/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions runtime/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ solana-transaction-status = { workspace = true }
solana-version = { workspace = true }
solana-vote = { workspace = true }
solana-vote-program = { workspace = true }
solana-zk-elgamal-proof-program = { workspace = true }
solana-zk-sdk = { workspace = true }

Choose a reason for hiding this comment

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

nit: Sorry I'm only just noticing this now, but it seems like we only need this dependency for the program id -- can we also have it in solana-zk-elgamal-proof-program and remove this direct dependency? This can be done in a follow-up

solana-zk-token-proof-program = { workspace = true }
solana-zk-token-sdk = { workspace = true }
static_assertions = { workspace = true }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,10 @@ mod tests {
solana_zk_token_sdk::zk_token_proof_program::id(),
Some(feature_set::zk_token_sdk_enabled::id())
)]
#[test_case(
solana_zk_sdk::zk_elgamal_proof_program::id(),
Some(feature_set::zk_elgamal_proof_program_enabled::id())
)]
fn test_target_program_builtin(program_address: Pubkey, activation_feature: Option<Pubkey>) {
let migration_target = CoreBpfMigrationTargetType::Builtin;
let mut bank = create_simple_test_bank(0);
Expand Down
30 changes: 30 additions & 0 deletions runtime/src/bank/builtins/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,13 @@ pub static BUILTINS: &[BuiltinPrototype] = &[
program_id: solana_sdk::loader_v4::id(),
entrypoint: solana_loader_v4_program::Entrypoint::vm,
}),
testable_prototype!(BuiltinPrototype {
core_bpf_migration_config: None,
name: zk_elgamal_proof_program,
enable_feature_id: Some(feature_set::zk_elgamal_proof_program_enabled::id()),
program_id: solana_zk_sdk::zk_elgamal_proof_program::id(),
entrypoint: solana_zk_elgamal_proof_program::Entrypoint::vm,
}),
];

pub static STATELESS_BUILTINS: &[StatelessBuiltinPrototype] = &[StatelessBuiltinPrototype {
Expand Down Expand Up @@ -328,6 +335,25 @@ mod test_only {
datapoint_name: "migrate_builtin_to_core_bpf_loader_v4_program",
};
}

pub mod zk_elgamal_proof_program {
pub mod feature {
solana_sdk::declare_id!("EYtuxScWqGWmcPEDmeUsEt3iPkvWE26EWLfSxUvWP2WN");
}
pub mod source_buffer {
solana_sdk::declare_id!("AaVrLPurAUmjw6XRNGr6ezQfHaJWpBGHhcRSJmNjoVpQ");
}
pub mod upgrade_authority {
solana_sdk::declare_id!("EyGkQYHgynUdvdNPNiWbJQk9roFCexgdJQMNcWbuvp78");
}
buffalojoec marked this conversation as resolved.
Show resolved Hide resolved
pub const CONFIG: super::CoreBpfMigrationConfig = super::CoreBpfMigrationConfig {
source_buffer_address: source_buffer::id(),
upgrade_authority_address: Some(upgrade_authority::id()),
feature_id: feature::id(),
migration_target: super::CoreBpfMigrationTargetType::Builtin,
datapoint_name: "migrate_builtin_to_core_bpf_zk_elgamal_proof_program",
};
}
}

#[cfg(test)]
Expand Down Expand Up @@ -377,6 +403,10 @@ mod tests {
&super::BUILTINS[10].core_bpf_migration_config,
&Some(super::test_only::loader_v4::CONFIG)
);
assert_eq!(
&super::BUILTINS[11].core_bpf_migration_config,
&Some(super::test_only::zk_elgamal_proof_program::CONFIG)
);
// Feature Gate has a live migration config, so it has no test-only
// configs to test here.
}
Expand Down
2 changes: 1 addition & 1 deletion runtime/src/bank/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7985,7 +7985,7 @@ fn test_reserved_account_keys() {

assert_eq!(
bank.get_reserved_account_keys().len(),
29,
30,
"after activating the new feature, bank should have new active reserved keys"
);
}
Expand Down
5 changes: 5 additions & 0 deletions sdk/src/feature_set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -824,6 +824,10 @@ pub mod migrate_address_lookup_table_program_to_core_bpf {
solana_sdk::declare_id!("C97eKZygrkU4JxJsZdjgbUY7iQR7rKTr4NyDWo2E5pRm");
}

pub mod zk_elgamal_proof_program_enabled {
solana_sdk::declare_id!("zkhiy5oLowR7HY4zogXjCjeMXyruLqBwSWH21qcFtnv");
}

lazy_static! {
/// Map of feature identifiers to user-visible description
pub static ref FEATURE_NAMES: HashMap<Pubkey, &'static str> = [
Expand Down Expand Up @@ -1025,6 +1029,7 @@ lazy_static! {
(migrate_config_program_to_core_bpf::id(), "Migrate Config program to Core BPF #1378"),
(enable_get_epoch_stake_syscall::id(), "Enable syscall: sol_get_epoch_stake #884"),
(migrate_address_lookup_table_program_to_core_bpf::id(), "Migrate Address Lookup Table program to Core BPF #1651"),
(zk_elgamal_proof_program_enabled::id(), "Enable ZkElGamalProof program SIMD-0153"),
/*************** ADD NEW FEATURES HERE ***************/
]
.iter()
Expand Down
6 changes: 6 additions & 0 deletions sdk/src/reserved_account_keys.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ mod zk_token_proof_program {
solana_sdk::declare_id!("ZkTokenProof1111111111111111111111111111111");
}

// Inline zk-elgamal-proof program id since it isn't available in the sdk
mod zk_elgamal_proof_program {
solana_sdk::declare_id!("ZkE1Gama1Proof11111111111111111111111111111");
}

// ReservedAccountKeys is not serialized into or deserialized from bank
// snapshots but the bank requires this trait to be implemented anyways.
#[cfg(all(RUSTC_WITH_SPECIALIZATION, feature = "frozen-abi"))]
Expand Down Expand Up @@ -162,6 +167,7 @@ lazy_static! {
ReservedAccount::new_active(stake::program::id()),
ReservedAccount::new_active(system_program::id()),
ReservedAccount::new_active(vote::program::id()),
ReservedAccount::new_pending(zk_elgamal_proof_program::id(), feature_set::add_new_reserved_account_keys::id()),
Copy link

Choose a reason for hiding this comment

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

This should have used the zk_elgamal_proof_program_enabled feature id. By using add_new_reserved_account_keys, you've now pushed back the minimum version that we can enable the add_new_reserved_account_keys feature from v2.0 to v2.1

Choose a reason for hiding this comment

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

Maybe I'm missing something, but this PR was added before 2.0 was branched, so it's part of 2.0, along with the add_new_reserved_account_keys. So they are part of the same release and can be enabled in the normal cadence.

You're right that it would have made more sense to use zk_elgamal_proof_program_enabled here though. With add_new_reserved_account_keys, the zk elgamal proof program key will be a reserved key before the program is enabled, which is a bit strange. But I don't see why it would push back enabling add_new_reserved_account_keys to v2.1

Copy link

Choose a reason for hiding this comment

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

Argh, apologies, I spelled Elgamal wrong when grepping git history. So this PR is fine as is, sorry for the noise. I do think it would have more sense to use the zk_elgamal_proof_program_enabled feature anyways but it's not a big deal in my opinion to reserve the id before it's enabled and so not worth switching at this point.

ReservedAccount::new_pending(zk_token_proof_program::id(), feature_set::add_new_reserved_account_keys::id()),

// sysvars
Expand Down