From 82a48f3967abb289e161d0b9f37febc287bc9a93 Mon Sep 17 00:00:00 2001 From: Samuel Laferriere <9342524+samlaf@users.noreply.github.com> Date: Mon, 3 Aug 2026 13:04:26 -0400 Subject: [PATCH] fix: reject duplicate validator keys, canonicalize digest order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two ways a genesis file could quietly mean something other than what it says. A repeated node public key silently shrank the validator set. The genesis committee is inserted into consensus state keyed by node key (`get_initial_state` -> `set_account`) and consensus reads the committee back out of that map, so two entries naming one key launched a network with one fewer validator than the file listed, computing quorum over the smaller set, while startup logged the file's count. Nothing rejected it at load. A repeated consensus key is now rejected for the same reason — it is one signing identity under two names. Repeated withdrawal_credentials stay legal: one operator may run several validators and be paid at one address. Keys are compared as decoded bytes, so the same key written two ways (`0x` prefix, upper case) is still caught. Hex-decodability is now required at load rather than deferred to committee construction, since neither that comparison nor the ordering below is defined without it. Validator order was part of chain identity. `config_digest` hashes the SSZ encoding of the validator list, which follows stored order, so two files naming the same set in different orders derived different chain domains and their nodes could not authenticate each other as peers. The digest now sorts by node key before hashing. Sorting rather than rejecting unsorted input means no emitter has to be trusted to have sorted, an obligation every future writer of a genesis file would otherwise carry. That is a semantic change to a value deriving every consensus signing domain, but only for files that were unsorted. Every genesis Summit's tooling emits is already in node-key order, and the frozen digest vector for example_genesis.toml is unchanged. Bumping the domain tag would instead change every digest, including already-canonical ones, forcing a coordinated restart on running networks for no gain, so the tag stays at -v1. `config_digest` panics on a validator key that is not hex, matching `genesis_hash` directly above it: validate rejects it at load, and a digest derived from a key we could not read would silently place a node in a chain domain of its own. Left for a follow-up: the digest still hashes the key, hash, and address fields as their hex *text*, so two files that parse to identical values but spell them differently — `0x` prefix present or not, hex case — still derive different chain domains. That is the same class of accident this commit removes for ordering, and it is the reason a genesis file cannot yet be treated as mere transport for its values. The fix is to digest the decoded bytes as fixed-size SSZ fields, which changes every digest and so requires a GENESIS_CONFIG_DOMAIN_TAG bump — free now, a hard fork once any network pins a digest. Worth doing; kept out of this commit so this change stays tag-neutral. Tests: duplicate node key, the same key respelled, duplicate consensus key, repeated withdrawal credentials accepted, non-hex keys rejected, digest unchanged under reversal, and the frozen vector. --- types/src/genesis.rs | 153 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 152 insertions(+), 1 deletion(-) diff --git a/types/src/genesis.rs b/types/src/genesis.rs index 69e799b6..7a4a479c 100644 --- a/types/src/genesis.rs +++ b/types/src/genesis.rs @@ -11,6 +11,7 @@ use commonware_cryptography::{Hasher as _, Sha256}; use commonware_utils::{from_hex, from_hex_formatted}; use serde::{Deserialize, Serialize}; use ssz::Encode as _; +use std::collections::HashSet; use std::net::SocketAddr; #[derive(Debug, Clone, Serialize, Deserialize, ssz_derive::Encode)] @@ -207,10 +208,29 @@ impl Genesis { /// spec-stable, and complete — a new field is automatically included unless /// explicitly `#[ssz(skip_serializing)]`'d). Per-validator `ip_address` is /// skipped: it is network topology, not consensus identity. + /// + /// The validator list is put in node-key order before hashing. SSZ encodes a + /// list in its stored order, so without this the order an operator happened + /// to write the file in would be part of the chain identity: two files + /// naming the same validator set in different orders would derive different + /// domains, and their nodes could not authenticate each other. Canonicalizing + /// here makes the digest a function of the set, and means no emitter has to + /// be trusted to have sorted it. + /// + /// Panics if a validator's `node_public_key` is not hex, like + /// [`genesis_hash`](Self::genesis_hash) and for the same reason: `validate` + /// rejects it at load time. Refusing to return is the safe failure here — a + /// digest derived from a key we could not read would silently put this node + /// in a chain domain of its own. pub fn config_digest(&self) -> [u8; 32] { + let mut canonical = self.clone(); + canonical.validators.sort_by_cached_key(|v| { + from_hex_formatted(&v.node_public_key).expect("bad validator node_public_key") + }); + let mut hasher = Sha256::new(); hasher.update(GENESIS_CONFIG_DOMAIN_TAG); - hasher.update(&self.as_ssz_bytes()); + hasher.update(&canonical.as_ssz_bytes()); hasher.finalize().0 } @@ -222,6 +242,7 @@ impl Genesis { } fn validate(&self) -> Result<(), Box> { + self.validate_no_duplicate_keys()?; // Genesis epoch length must satisfy the same bounds as a runtime // EpochLength protocol-parameter update (hence the shared ProtocolParam // validation). An oversized launch value defers every epoch-boundary @@ -305,6 +326,55 @@ impl Genesis { Ok(()) } + /// No identity may appear twice in the validator set. + /// + /// The genesis committee is inserted into consensus state keyed by node + /// public key (`get_initial_state` -> `set_account`), so a repeated node key + /// silently collapses the set: the network launches with fewer validators + /// than the file lists and computes quorum over the smaller set, while + /// startup reports the file's count. A repeated consensus key is rejected for + /// the same reason — it is a second name for one signing identity. Repeated + /// `withdrawal_credentials` are fine: one operator may run several validators + /// and be paid at one address. + /// + /// Comparison is on decoded bytes, so a key repeated in a different spelling + /// (`0x` prefix, upper case) is still caught; both spellings are accepted + /// everywhere else a key is read. + fn validate_no_duplicate_keys(&self) -> Result<(), Box> { + let mut node_keys = HashSet::with_capacity(self.validators.len()); + let mut consensus_keys = HashSet::with_capacity(self.validators.len()); + for validator in &self.validators { + let node_key = from_hex_formatted(&validator.node_public_key).ok_or_else(|| { + format!( + "validator node_public_key is not valid hex: {:?}", + validator.node_public_key + ) + })?; + let consensus_key = + from_hex_formatted(&validator.consensus_public_key).ok_or_else(|| { + format!( + "validator consensus_public_key is not valid hex: {:?}", + validator.consensus_public_key + ) + })?; + if !node_keys.insert(node_key) { + return Err(format!( + "duplicate validator node_public_key: {:?}", + validator.node_public_key + ) + .into()); + } + if !consensus_keys.insert(consensus_key) { + return Err(format!( + "duplicate validator consensus_public_key: {:?}", + validator.consensus_public_key + ) + .into()); + } + } + Ok(()) + } + pub fn ip_of(&self, target_public_key: &PublicKey) -> Option { for validator in &self.validators { #[allow(clippy::collapsible_if)] @@ -670,6 +740,87 @@ mod tests { } } + /// Validator order is how a file happens to be written, not who is in the + /// set, so it must not move the digest: two operators handed the same + /// validators in different orders have to derive the same chain domain. + #[test] + fn config_digest_ignores_validator_order() { + let base = Genesis::load_from_file("../example_genesis.toml").unwrap(); + let mut reordered = base.clone(); + reordered.validators.reverse(); + + assert_ne!( + reordered.validators[0].node_public_key, base.validators[0].node_public_key, + "the reversal must actually have moved validators" + ); + assert_eq!(reordered.config_digest(), base.config_digest()); + } + + /// Frozen digest for the committed example genesis, whose validators are + /// already in node-key order. Canonicalizing the order inside the digest + /// must leave every already-ordered genesis — which is every genesis Summit's + /// own tooling has emitted — hashing exactly as before. + #[test] + fn config_digest_matches_frozen_vector() { + let genesis = Genesis::load_from_file("../example_genesis.toml").unwrap(); + assert_eq!( + commonware_utils::hex(&genesis.config_digest()), + "659162923344cebe204fa4552cc431ad72d60caaae2611336cff4a600c9b64c2" + ); + } + + /// One repeated node key means the consensus state silently holds one fewer + /// validator than the file names, so genesis must refuse to load. + #[test] + fn rejects_duplicate_validator_node_key() { + let mut genesis = Genesis::load_from_file("../example_genesis.toml").unwrap(); + genesis.validators[1].node_public_key = genesis.validators[0].node_public_key.clone(); + assert!(genesis.validate().is_err()); + } + + /// The same key spelled differently is the same key: duplicates are compared + /// on decoded bytes, not on text. + #[test] + fn rejects_duplicate_validator_node_key_respelled() { + let mut genesis = Genesis::load_from_file("../example_genesis.toml").unwrap(); + genesis.validators[1].node_public_key = + format!("0x{}", genesis.validators[0].node_public_key.to_uppercase()); + assert!(genesis.validate().is_err()); + } + + /// Two validators sharing a consensus key are one signing identity wearing + /// two names, which the stake and quorum accounting would double-count. + #[test] + fn rejects_duplicate_validator_consensus_key() { + let mut genesis = Genesis::load_from_file("../example_genesis.toml").unwrap(); + genesis.validators[1].consensus_public_key = + genesis.validators[0].consensus_public_key.clone(); + assert!(genesis.validate().is_err()); + } + + /// Withdrawal credentials are an address, not an identity: one operator + /// running several validators pays out to a single address. + #[test] + fn accepts_repeated_withdrawal_credentials() { + let mut genesis = Genesis::load_from_file("../example_genesis.toml").unwrap(); + genesis.validators[1].withdrawal_credentials = + genesis.validators[0].withdrawal_credentials.clone(); + assert!(genesis.validate().is_ok()); + } + + /// A key that isn't hex can be neither compared nor ordered, so it cannot be + /// left to fail later at committee construction. + #[test] + fn rejects_non_hex_validator_keys() { + let mut genesis = Genesis::load_from_file("../example_genesis.toml").unwrap(); + genesis.validators[0].node_public_key = "not-a-key".into(); + assert!(genesis.validate().is_err()); + + let mut genesis = Genesis::load_from_file("../example_genesis.toml").unwrap(); + genesis.validators[0].consensus_public_key = "not-a-key".into(); + assert!(genesis.validate().is_err()); + } + /// A validator's `ip_address` is network topology, not consensus identity, /// so it is excluded from the digest: changing it must NOT change identity. #[test]