Skip to content

Commit

Permalink
feat: (DB migration) Rename recursion_scheduler_level_vk_hash to snar…
Browse files Browse the repository at this point in the history
…k_wrapper_vk_hash (#2809)

## What ❔

We have a configuration field `recursion_scheduler_level_vk_hash` which
actually stores `snark_wrapper_vk_hash` inside.
It happened because an old config value was utilized for the new purpose
some time ago.
This PR changes the name of field in a non-breaking way:

- `serde` (de)serialization happens with both `alias` and
`rename(serialize = "..")`, so that we serialize the field the same way
as before, but can deserialize either way. This is used for env configs
and API.
- `protobuf` deserialization is done by introducing a new field, and
reading whatever one is available.
- `protobuf` serialization always produced the _new_ field, so newly
generated configs should have new field name.
- ~~⚠️ DB column names was left as-is, because renaming DB columns is
not a trivial process.~~
- Upd: Migration was added. It copies the old column to the new one and
switches to the new one right away.

## Why ❔

Having incorrect name that doesn't represent the value stored is
confusing and can lead to errors.
  • Loading branch information
popzxc authored Sep 6, 2024
1 parent 6db091e commit 64f9551
Show file tree
Hide file tree
Showing 36 changed files with 246 additions and 189 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

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

25 changes: 24 additions & 1 deletion core/lib/basic_types/src/protocol_version.rs
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,12 @@ impl Detokenize for VerifierParams {

#[derive(Debug, Clone, Copy, Default, Eq, PartialEq, Serialize, Deserialize)]
pub struct L1VerifierConfig {
pub recursion_scheduler_level_vk_hash: H256,
// Rename is required to not introduce breaking changes in the API for existing clients.
#[serde(
alias = "recursion_scheduler_level_vk_hash",
rename(serialize = "recursion_scheduler_level_vk_hash")
)]
pub snark_wrapper_vk_hash: H256,
}

impl From<ProtocolVersionId> for VmVersion {
Expand Down Expand Up @@ -394,4 +399,22 @@ mod tests {

assert_eq!(version, unpacked);
}

#[test]
fn test_verifier_config_serde() {
let de = [
r#"{"recursion_scheduler_level_vk_hash": "0x1111111111111111111111111111111111111111111111111111111111111111"}"#,
r#"{"snark_wrapper_vk_hash": "0x1111111111111111111111111111111111111111111111111111111111111111"}"#,
];
for de in de.iter() {
let _: L1VerifierConfig = serde_json::from_str(de)
.unwrap_or_else(|err| panic!("Failed deserialization. String: {de}, error {err}"));
}
let ser = L1VerifierConfig {
snark_wrapper_vk_hash: H256::repeat_byte(0x11),
};
let ser_str = serde_json::to_string(&ser).unwrap();
let expected_str = r#"{"recursion_scheduler_level_vk_hash":"0x1111111111111111111111111111111111111111111111111111111111111111"}"#;
assert_eq!(ser_str, expected_str);
}
}
3 changes: 3 additions & 0 deletions core/lib/config/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ rand.workspace = true
secrecy.workspace = true
serde = { workspace = true, features = ["derive"] }

[dev-dependencies]
serde_json.workspace = true

[features]
default = []
observability_ext = ["zksync_vlog", "tracing"]
45 changes: 43 additions & 2 deletions core/lib/config/src/configs/genesis.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,14 @@ pub struct GenesisConfig {
pub l1_chain_id: L1ChainId,
pub sl_chain_id: Option<SLChainId>,
pub l2_chain_id: L2ChainId,
pub recursion_scheduler_level_vk_hash: H256,
// Note: `serde` isn't used with protobuf config. The same alias is implemented in
// `zksync_protobuf_config` manually.
// Rename is required to not introduce breaking changes in the API for existing clients.
#[serde(
alias = "recursion_scheduler_level_vk_hash",
rename(serialize = "recursion_scheduler_level_vk_hash")
)]
pub snark_wrapper_vk_hash: H256,
pub fee_account: Address,
pub dummy_verifier: bool,
pub l1_batch_commit_data_generator_mode: L1BatchCommitmentMode,
Expand All @@ -37,7 +44,7 @@ impl GenesisConfig {
GenesisConfig {
genesis_root_hash: Some(H256::repeat_byte(0x01)),
rollup_last_leaf_index: Some(26),
recursion_scheduler_level_vk_hash: H256::repeat_byte(0x02),
snark_wrapper_vk_hash: H256::repeat_byte(0x02),
fee_account: Default::default(),
genesis_commitment: Some(H256::repeat_byte(0x17)),
bootloader_hash: Default::default(),
Expand All @@ -54,3 +61,37 @@ impl GenesisConfig {
}
}
}

#[cfg(test)]
mod tests {
use super::GenesisConfig;

// This test checks that serde overrides (`rename`, `alias`) work for `snark_wrapper_vk_hash` field.
#[test]
fn genesis_serde_snark_wrapper_vk_hash() {
let genesis = GenesisConfig::for_tests();
let genesis_str = serde_json::to_string(&genesis).unwrap();

// Check that we use backward-compatible name in serialization.
// If you want to remove this check, make sure that all the potential clients are updated.
assert!(
genesis_str.contains("recursion_scheduler_level_vk_hash"),
"Serialization should use backward-compatible name"
);

let genesis2: GenesisConfig = serde_json::from_str(&genesis_str).unwrap();
assert_eq!(genesis, genesis2);

let genesis_json = r#"{
"snark_wrapper_vk_hash": "0x1111111111111111111111111111111111111111111111111111111111111111",
"l1_chain_id": 1,
"l2_chain_id": 1,
"fee_account": "0x1111111111111111111111111111111111111111",
"dummy_verifier": false,
"l1_batch_commit_data_generator_mode": "Rollup"
}"#;
serde_json::from_str::<GenesisConfig>(genesis_json).unwrap_or_else(|err| {
panic!("Failed to parse genesis config with a new name: {}", err)
});
}
}
2 changes: 1 addition & 1 deletion core/lib/config/src/testonly.rs
Original file line number Diff line number Diff line change
Expand Up @@ -728,7 +728,7 @@ impl Distribution<configs::GenesisConfig> for EncodeDist {
l1_chain_id: L1ChainId(self.sample(rng)),
sl_chain_id: None,
l2_chain_id: L2ChainId::default(),
recursion_scheduler_level_vk_hash: rng.gen(),
snark_wrapper_vk_hash: rng.gen(),
dummy_verifier: rng.gen(),
l1_batch_commit_data_generator_mode: match rng.gen_range(0..2) {
0 => L1BatchCommitmentMode::Rollup,
Expand Down

This file was deleted.

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

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

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

This file was deleted.

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
UPDATE protocol_patches SET recursion_scheduler_level_vk_hash = snark_wrapper_vk_hash WHERE recursion_scheduler_level_vk_hash = ''::bytea;
ALTER TABLE protocol_patches DROP COLUMN snark_wrapper_vk_hash;
ALTER TABLE protocol_patches ALTER COLUMN recursion_scheduler_level_vk_hash DROP DEFAULT;
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
ALTER TABLE protocol_patches ADD COLUMN snark_wrapper_vk_hash BYTEA NOT NULL DEFAULT ''::bytea;
ALTER TABLE protocol_patches ALTER COLUMN recursion_scheduler_level_vk_hash SET DEFAULT ''::bytea;
UPDATE protocol_patches SET snark_wrapper_vk_hash = recursion_scheduler_level_vk_hash;
-- Default was only needed to migrate old rows, we don't want this field to be forgotten by accident after migration.
ALTER TABLE protocol_patches ALTER COLUMN snark_wrapper_vk_hash DROP DEFAULT;

-- Old column should be removed once the migration is on the mainnet.
COMMENT ON COLUMN protocol_patches.recursion_scheduler_level_vk_hash IS 'This column is deprecated and will be removed in the future. Use snark_wrapper_vk_hash instead.';
6 changes: 2 additions & 4 deletions core/lib/dal/src/models/storage_protocol_version.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ pub struct StorageProtocolVersion {
pub minor: i32,
pub patch: i32,
pub timestamp: i64,
pub recursion_scheduler_level_vk_hash: Vec<u8>,
pub snark_wrapper_vk_hash: Vec<u8>,
pub bootloader_code_hash: Vec<u8>,
pub default_account_code_hash: Vec<u8>,
}
Expand All @@ -29,9 +29,7 @@ pub(crate) fn protocol_version_from_storage(
},
timestamp: storage_version.timestamp as u64,
l1_verifier_config: L1VerifierConfig {
recursion_scheduler_level_vk_hash: H256::from_slice(
&storage_version.recursion_scheduler_level_vk_hash,
),
snark_wrapper_vk_hash: H256::from_slice(&storage_version.snark_wrapper_vk_hash),
},
base_system_contracts_hashes: BaseSystemContractsHashes {
bootloader: H256::from_slice(&storage_version.bootloader_code_hash),
Expand Down
20 changes: 8 additions & 12 deletions core/lib/dal/src/protocol_versions_dal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,16 +71,14 @@ impl ProtocolVersionsDal<'_, '_> {
sqlx::query!(
r#"
INSERT INTO
protocol_patches (minor, patch, recursion_scheduler_level_vk_hash, created_at)
protocol_patches (minor, patch, snark_wrapper_vk_hash, created_at)
VALUES
($1, $2, $3, NOW())
ON CONFLICT DO NOTHING
"#,
version.minor as i32,
version.patch.0 as i32,
l1_verifier_config
.recursion_scheduler_level_vk_hash
.as_bytes(),
l1_verifier_config.snark_wrapper_vk_hash.as_bytes(),
)
.instrument("save_protocol_version#patch")
.with_arg("version", &version)
Expand Down Expand Up @@ -235,7 +233,7 @@ impl ProtocolVersionsDal<'_, '_> {
protocol_versions.bootloader_code_hash,
protocol_versions.default_account_code_hash,
protocol_patches.patch,
protocol_patches.recursion_scheduler_level_vk_hash
protocol_patches.snark_wrapper_vk_hash
FROM
protocol_versions
JOIN protocol_patches ON protocol_patches.minor = protocol_versions.id
Expand Down Expand Up @@ -268,7 +266,7 @@ impl ProtocolVersionsDal<'_, '_> {
let row = sqlx::query!(
r#"
SELECT
recursion_scheduler_level_vk_hash
snark_wrapper_vk_hash
FROM
protocol_patches
WHERE
Expand All @@ -282,16 +280,14 @@ impl ProtocolVersionsDal<'_, '_> {
.await
.unwrap()?;
Some(L1VerifierConfig {
recursion_scheduler_level_vk_hash: H256::from_slice(
&row.recursion_scheduler_level_vk_hash,
),
snark_wrapper_vk_hash: H256::from_slice(&row.snark_wrapper_vk_hash),
})
}

pub async fn get_patch_versions_for_vk(
&mut self,
minor_version: ProtocolVersionId,
recursion_scheduler_level_vk_hash: H256,
snark_wrapper_vk_hash: H256,
) -> DalResult<Vec<VersionPatch>> {
let rows = sqlx::query!(
r#"
Expand All @@ -301,12 +297,12 @@ impl ProtocolVersionsDal<'_, '_> {
protocol_patches
WHERE
minor = $1
AND recursion_scheduler_level_vk_hash = $2
AND snark_wrapper_vk_hash = $2
ORDER BY
patch DESC
"#,
minor_version as i32,
recursion_scheduler_level_vk_hash.as_bytes()
snark_wrapper_vk_hash.as_bytes()
)
.instrument("get_patch_versions_for_vk")
.fetch_all(self.storage)
Expand Down
2 changes: 1 addition & 1 deletion core/lib/env_config/src/genesis.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ impl FromEnv for GenesisConfig {
l1_chain_id: L1ChainId(network_config.network.chain_id().0),
sl_chain_id: Some(network_config.network.chain_id()),
l2_chain_id: network_config.zksync_network_id,
recursion_scheduler_level_vk_hash: contracts_config.snark_wrapper_vk_hash,
snark_wrapper_vk_hash: contracts_config.snark_wrapper_vk_hash,
fee_account: state_keeper
.fee_account_addr
.context("Fee account required for genesis")?,
Expand Down
17 changes: 10 additions & 7 deletions core/lib/protobuf_config/src/genesis.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,13 @@ impl ProtoRepr for proto::Genesis {
0.into(),
)
};
// Check either of fields, use old name as a fallback.
let snark_wrapper_vk_hash = match (&prover.snark_wrapper_vk_hash, &prover.recursion_scheduler_level_vk_hash) {
(Some(x), _) => parse_h256(x).context("snark_wrapper_vk_hash")?,
(_, Some(x)) => parse_h256(x).context("recursion_scheduler_level_vk_hash")?,
_ => anyhow::bail!("Either snark_wrapper_vk_hash or recursion_scheduler_level_vk_hash should be presented"),
};

Ok(Self::Type {
protocol_version: Some(protocol_version),
genesis_root_hash: Some(
Expand Down Expand Up @@ -75,9 +82,7 @@ impl ProtoRepr for proto::Genesis {
l2_chain_id: required(&self.l2_chain_id)
.and_then(|x| L2ChainId::try_from(*x).map_err(|a| anyhow::anyhow!(a)))
.context("l2_chain_id")?,
recursion_scheduler_level_vk_hash: required(&prover.recursion_scheduler_level_vk_hash)
.and_then(|x| parse_h256(x))
.context("recursion_scheduler_level_vk_hash")?,
snark_wrapper_vk_hash,
fee_account: required(&self.fee_account)
.and_then(|x| parse_h160(x))
.context("fee_account")?,
Expand All @@ -104,11 +109,9 @@ impl ProtoRepr for proto::Genesis {
l1_chain_id: Some(this.l1_chain_id.0),
l2_chain_id: Some(this.l2_chain_id.as_u64()),
prover: Some(proto::Prover {
recursion_scheduler_level_vk_hash: Some(format!(
"{:?}",
this.recursion_scheduler_level_vk_hash
)),
recursion_scheduler_level_vk_hash: None, // Deprecated field.
dummy_verifier: Some(this.dummy_verifier),
snark_wrapper_vk_hash: Some(format!("{:?}", this.snark_wrapper_vk_hash)),
}),
l1_batch_commit_data_generator_mode: Some(
proto::L1BatchCommitDataGeneratorMode::new(
Expand Down
Loading

0 comments on commit 64f9551

Please sign in to comment.