Skip to content

Commit

Permalink
fix(consensus): preventing config update reverts (#3148)
Browse files Browse the repository at this point in the history
Random failures of consensus_global_config RPC may cause config reverts,
until the consensus_genesis() RPC is fully deprecated. Although the
problems of this sort are transient, this pr adds extra protection from
this kind of situations to prevent unnecessary binary restarts.
  • Loading branch information
pompon0 authored Oct 23, 2024
1 parent b29be7d commit caee55f
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 25 deletions.
64 changes: 40 additions & 24 deletions core/lib/dal/src/consensus_dal/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,48 @@ use crate::{Core, CoreDal};
#[cfg(test)]
mod tests;

/// Hash of the batch.
pub fn batch_hash(info: &StoredBatchInfo) -> attester::BatchHash {
attester::BatchHash(Keccak256::from_bytes(info.hash().0))
}

/// Verifies that the transition from `old` to `new` is admissible.
pub fn verify_config_transition(old: &GlobalConfig, new: &GlobalConfig) -> anyhow::Result<()> {
anyhow::ensure!(
old.genesis.chain_id == new.genesis.chain_id,
"changing chain_id is not allowed: old = {:?}, new = {:?}",
old.genesis.chain_id,
new.genesis.chain_id,
);
// Note that it may happen that the fork number didn't change,
// in case the binary was updated to support more fields in genesis struct.
// In such a case, the old binary was not able to connect to the consensus network,
// because of the genesis hash mismatch.
// TODO: Perhaps it would be better to deny unknown fields in the genesis instead.
// It would require embedding the genesis either as a json string or protobuf bytes within
// the global config, so that the global config can be parsed with
// `deny_unknown_fields:false` while genesis would be parsed with
// `deny_unknown_fields:true`.
anyhow::ensure!(
old.genesis.fork_number <= new.genesis.fork_number,
"transition to a past fork is not allowed: old = {:?}, new = {:?}",
old.genesis.fork_number,
new.genesis.fork_number,
);
new.genesis.verify().context("genesis.verify()")?;
// This is a temporary hack until the `consensus_genesis()` RPC is disabled.
if new
== (&GlobalConfig {
genesis: old.genesis.clone(),
registry_address: None,
seed_peers: [].into(),
})
{
anyhow::bail!("new config is equal to truncated old config, which means that it was sourced from the wrong endpoint");
}
Ok(())
}

/// Storage access methods for `zksync_core::consensus` module.
#[derive(Debug)]
pub struct ConsensusDal<'a, 'c> {
Expand Down Expand Up @@ -94,6 +132,8 @@ impl ConsensusDal<'_, '_> {
if got == want {
return Ok(());
}
verify_config_transition(got, want)?;

// If genesis didn't change, just update the config.
if got.genesis == want.genesis {
let s = zksync_protobuf::serde::Serialize;
Expand All @@ -112,30 +152,6 @@ impl ConsensusDal<'_, '_> {
txn.commit().await?;
return Ok(());
}

// Verify the genesis change.
anyhow::ensure!(
got.genesis.chain_id == want.genesis.chain_id,
"changing chain_id is not allowed: old = {:?}, new = {:?}",
got.genesis.chain_id,
want.genesis.chain_id,
);
// Note that it may happen that the fork number didn't change,
// in case the binary was updated to support more fields in genesis struct.
// In such a case, the old binary was not able to connect to the consensus network,
// because of the genesis hash mismatch.
// TODO: Perhaps it would be better to deny unknown fields in the genesis instead.
// It would require embedding the genesis either as a json string or protobuf bytes within
// the global config, so that the global config can be parsed with
// `deny_unknown_fields:false` while genesis would be parsed with
// `deny_unknown_fields:true`.
anyhow::ensure!(
got.genesis.fork_number <= want.genesis.fork_number,
"transition to a past fork is not allowed: old = {:?}, new = {:?}",
got.genesis.fork_number,
want.genesis.fork_number,
);
want.genesis.verify().context("genesis.verify()")?;
}

// Reset the consensus state.
Expand Down
7 changes: 6 additions & 1 deletion core/node/consensus/src/en.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,12 @@ impl EN {
let old = old;
loop {
if let Ok(new) = self.fetch_global_config(ctx).await {
if new != old {
// We verify the transition here to work around the situation
// where `consenus_global_config()` RPC fails randomly and fallback
// to `consensus_genesis()` RPC activates.
if new != old
&& consensus_dal::verify_config_transition(&old, &new).is_ok()
{
return Err(anyhow::format_err!(
"global config changed: old {old:?}, new {new:?}"
)
Expand Down

0 comments on commit caee55f

Please sign in to comment.