From c95b0f378ac3c0fd0d997d4d8fa19c81b8074aa6 Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Wed, 28 Oct 2026 10:35:55 +0700 Subject: [PATCH 1/4] fix(drive-abci): refuse a zero chain lock quorum window when loading the config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit verify_chain_lock_locally v0 took the last committed core height modulo the chain lock quorum window, which comes from the config and is copied into the platform state at init chain. A window of 0 divided by zero and panicked while validating a proposal that carries a chain lock. - Loading the config now refuses CHAIN_LOCK_QUORUM_WINDOW=0 with a clear error, so the node does not start. - In verify_chain_lock_locally v0, the remainder is checked: a window of 0 still held by a platform state returns an execution error instead of panicking. The end of the window is computed with wrapping_add, so builds that check arithmetic no longer overflow when the window ends past u32::MAX. No consensus change at any protocol version. v0 serves protocol versions 1-14. For every non-zero window it returns exactly what release builds always computed: the release profile keeps Cargo's default overflow-checks = false, so their arithmetic wraps. A window of 0 gave no verdict before (panic) and gives none now (an error out of run_block_proposal). Adapted for v5.0-dev: verify_chain_lock_locally has only v0 on this line, so the v1 edit is dropped. The chain lock test helpers of v5.1-dev do not exist here either, so the tests live in v0's test module and build their one-quorum state and signed chain lock there. Tests would have caught this in CI: ✖ before the fix, ✔ after, run against v5.0-dev's config loader and v0 arithmetic: - should_reject_zero_chain_lock_quorum_window_when_loading_config: window 0 loaded. - should_return_error_instead_of_panicking_when_chain_lock_quorum_window_is_zero: "attempt to calculate the remainder with a divisor of zero". - should_leave_chain_lock_to_core_without_overflowing_when_quorum_window_ends_past_largest_core_height: "attempt to add with overflow". Its control chain lock, in a window ending below u32::MAX, verifies locally, so the None it expects comes from the wrapped window end. Closes #5504 (cherry picked from commit 09ba154f3489faef33f5b19325b74d9e673aa50c) Co-Authored-By: Claude Opus 5.5 --- packages/rs-drive-abci/src/config.rs | 57 +++++- .../verify_chain_lock_locally/v0/mod.rs | 182 +++++++++++++++++- 2 files changed, 231 insertions(+), 8 deletions(-) diff --git a/packages/rs-drive-abci/src/config.rs b/packages/rs-drive-abci/src/config.rs index 1a0f90a9235..9f9755b861f 100644 --- a/packages/rs-drive-abci/src/config.rs +++ b/packages/rs-drive-abci/src/config.rs @@ -421,9 +421,10 @@ pub struct ChainLockConfig { /// The quorum window (DKG interval) /// On Mainnet Chain Locks are signed using 400_60: One quorum in every 288 blocks and activeQuorumCount is 4. /// On Testnet Chain Locks are signed using 50_60: One quorum in every 24 blocks and activeQuorumCount is 24. + /// Loading the config refuses 0. #[serde( rename = "chain_lock_quorum_window", - deserialize_with = "from_str_or_number" + deserialize_with = "deserialize_chain_lock_quorum_window" )] pub quorum_window: u32, @@ -609,6 +610,24 @@ where Ok(quorum_type) } +/// Chain lock verification finds the DKG interval a core height is in by dividing by the window, +/// so a window of 0 is refused when the config loads instead of failing chain lock verification +/// while validating proposals. +fn deserialize_chain_lock_quorum_window<'de, D>(deserializer: D) -> Result +where + D: serde::Deserializer<'de>, +{ + let quorum_window: u32 = from_str_or_number(deserializer)?; + + if quorum_window == 0 { + return Err(serde::de::Error::custom( + "CHAIN_LOCK_QUORUM_WINDOW must be greater than 0: it is the DKG interval of the chain lock quorum type", + )); + } + + Ok(quorum_window) +} + impl ExecutionConfig { fn default_verify_sum_trees() -> bool { true @@ -1111,6 +1130,42 @@ mod tests { ); } + /// A zero chain lock quorum window would divide by zero while validating a proposal, so + /// loading the config must refuse it instead of letting the node start. The variables are + /// read from `.env.local` without touching the process environment, which other tests set. + #[test] + fn should_reject_zero_chain_lock_quorum_window_when_loading_config() { + let envfile = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")).join(".env.local"); + let vars: Vec<(String, String)> = dotenvy::from_path_iter(envfile.as_path()) + .expect("cannot open .env file") + .collect::>() + .expect("cannot parse .env file"); + let with_chain_lock_quorum_window = |window: &str| -> Vec<(String, String)> { + vars.iter() + .map(|(key, value)| { + if key == "CHAIN_LOCK_QUORUM_WINDOW" { + (key.clone(), window.to_string()) + } else { + (key.clone(), value.clone()) + } + }) + .collect() + }; + + let err = envy::from_iter::<_, PlatformConfig>(with_chain_lock_quorum_window("0")) + .expect_err("a zero chain lock quorum window should fail"); + let msg = err.to_string(); + assert!( + msg.contains("CHAIN_LOCK_QUORUM_WINDOW must be greater than 0"), + "the error should name the offending option, got: {}", + msg + ); + + let config = envy::from_iter::<_, PlatformConfig>(with_chain_lock_quorum_window("1")) + .expect("a chain lock quorum window of 1 should be accepted"); + assert_eq!(config.chain_lock.quorum_window, 1); + } + // --- Core RPC URL composition and defaults --- /// Exercises `ConsensusCoreRpcConfig::url` formatting. diff --git a/packages/rs-drive-abci/src/execution/platform_events/core_chain_lock/verify_chain_lock_locally/v0/mod.rs b/packages/rs-drive-abci/src/execution/platform_events/core_chain_lock/verify_chain_lock_locally/v0/mod.rs index 000d46582a2..d6bbd675e85 100644 --- a/packages/rs-drive-abci/src/execution/platform_events/core_chain_lock/verify_chain_lock_locally/v0/mod.rs +++ b/packages/rs-drive-abci/src/execution/platform_events/core_chain_lock/verify_chain_lock_locally/v0/mod.rs @@ -66,11 +66,19 @@ where let window_width = quorum_config.window; + // A window of 0, which loading the config refuses, is an error here rather than a panic; + // either way the proposal gets no verdict. For every other window the result is the one + // release builds have always computed: their arithmetic wraps, so a window ending past + // the largest core height wraps too. + let offset_in_window = platform_state + .last_committed_core_height() + .checked_rem(window_width) + .ok_or(Error::Execution(ExecutionError::CorruptedCodeExecution( + "the chain lock quorum window is 0", + )))?; // The last block in the window where the quorums would be the same - let last_block_in_window = platform_state.last_committed_core_height() - - platform_state.last_committed_core_height() % window_width - + window_width - - 1; + let last_block_in_window = (platform_state.last_committed_core_height() - offset_in_window) + .wrapping_add(window_width - 1); let verification_height = chain_lock_height.saturating_sub(SIGN_OFFSET); @@ -235,11 +243,21 @@ where #[cfg(test)] mod tests { + use crate::config::ChainLockConfig; + use crate::error::execution::ExecutionError; + use crate::error::Error; use crate::execution::platform_events::core_chain_lock::verify_chain_lock_locally::v0::CHAIN_LOCK_REQUEST_ID_PREFIX; - use crate::test::helpers::setup::TestPlatformBuilder; - use dpp::bls_signatures::{Bls12381G2Impl, Pairing}; + use crate::platform_types::platform_state::{PlatformState, PlatformStateV0Methods}; + use crate::platform_types::signature_verification_quorum_set::{ + SignatureVerificationQuorumSet, SignatureVerificationQuorumSetV0Methods, VerificationQuorum, + }; + use crate::rpc::core::MockCoreRPCLike; + use crate::test::helpers::setup::{TempPlatform, TestPlatformBuilder}; + use dpp::block::block_info::BlockInfo; + use dpp::bls_signatures::{Bls12381G2Impl, Pairing, SecretKey, SignatureSchemes}; use dpp::dashcore::hashes::{sha256d, Hash, HashEngine}; - use dpp::dashcore::{BlockHash, ChainLock, QuorumSigningRequestId}; + use dpp::dashcore::{BlockHash, ChainLock, QuorumHash, QuorumSigningRequestId}; + use dpp::dashcore_rpc::json::QuorumType; use dpp::version::PlatformVersion; #[test] @@ -371,4 +389,154 @@ mod tests { msg ); } + + /// The secret key of the only chain lock quorum held by `state_with_one_chain_lock_quorum` + fn quorum_secret_key() -> SecretKey { + let mut key_bytes = [0u8; 32]; + key_bytes[0] = 1; + key_bytes[31] = 1; + SecretKey::::from_be_bytes(&key_bytes).expect("valid secret key") + } + + const QUORUM_HASH: [u8; 32] = [1u8; 32]; + + /// `platform`'s state with last committed core height `core_height`, holding one llmq_test + /// chain lock quorum with a `quorum_window` block DKG interval + fn state_with_one_chain_lock_quorum( + platform: &TempPlatform, + quorum_window: u32, + core_height: u32, + ) -> PlatformState { + let mut state = platform.platform.state.load().as_ref().clone(); + state.set_last_committed_block_info(None); + state.set_genesis_block_info(Some(BlockInfo { + core_height, + ..Default::default() + })); + + let mut quorum_set = SignatureVerificationQuorumSet::V0( + ChainLockConfig { + quorum_type: QuorumType::LlmqTest, + quorum_size: 3, + quorum_window, + quorum_active_signers: 2, + quorum_rotation: false, + } + .into(), + ); + quorum_set.set_current_quorums( + [( + QuorumHash::from_byte_array(QUORUM_HASH), + VerificationQuorum { + index: None, + public_key: quorum_secret_key().public_key(), + }, + )] + .into_iter() + .collect(), + ); + state.set_chain_lock_validating_quorums(quorum_set); + state + } + + /// A llmq_test chain lock at `height` signed by the quorum of + /// `state_with_one_chain_lock_quorum` + fn sign_chain_lock(height: u32) -> ChainLock { + let block_hash = BlockHash::from_byte_array([7u8; 32]); + + let mut engine = QuorumSigningRequestId::engine(); + engine.input(&[CHAIN_LOCK_REQUEST_ID_PREFIX.len() as u8]); + engine.input(CHAIN_LOCK_REQUEST_ID_PREFIX.as_bytes()); + engine.input(height.to_le_bytes().as_slice()); + let request_id = QuorumSigningRequestId::from_engine(engine); + + let mut reversed_quorum_hash = QUORUM_HASH; + reversed_quorum_hash.reverse(); + + let mut engine = sha256d::Hash::engine(); + engine.input(&[QuorumType::LlmqTest as u8]); + engine.input(reversed_quorum_hash.as_slice()); + engine.input(request_id.as_byte_array()); + engine.input(block_hash.as_byte_array()); + let message_digest = sha256d::Hash::from_engine(engine); + + let signature = quorum_secret_key() + .sign(SignatureSchemes::Basic, message_digest.as_byte_array()) + .expect("signs"); + + ChainLock { + block_height: height, + block_hash, + signature: signature.as_raw_value().to_compressed().into(), + } + } + + fn chain_lock_test_platform() -> TempPlatform { + TestPlatformBuilder::new() + .with_latest_protocol_version() + .build_with_mock_rpc() + .set_initial_state_structure() + } + + #[test] + fn should_return_error_instead_of_panicking_when_chain_lock_quorum_window_is_zero() { + // A chain lock quorum window of 0 can still reach the platform state, which keeps the + // window it was created with. Validating a proposal must then fail with an error + // instead of dividing by zero and taking the node down. + let platform = chain_lock_test_platform(); + let state = state_with_one_chain_lock_quorum(&platform, 0, 1106); + + let result = platform.platform.verify_chain_lock_locally_v0( + 0, + &state, + &sign_chain_lock(1110), + PlatformVersion::latest(), + ); + + assert!( + matches!( + result, + Err(Error::Execution(ExecutionError::CorruptedCodeExecution( + "the chain lock quorum window is 0" + ))) + ), + "a zero quorum window must be refused, got {result:?}" + ); + } + + #[test] + fn should_leave_chain_lock_to_core_without_overflowing_when_quorum_window_ends_past_largest_core_height( + ) { + let platform = chain_lock_test_platform(); + let platform_version = PlatformVersion::latest(); + + // The quorum verifies its chain locks locally while its window ends below u32::MAX + let state = state_with_one_chain_lock_quorum(&platform, 24, 1106); + assert_eq!( + platform + .platform + .verify_chain_lock_locally_v0(0, &state, &sign_chain_lock(1110), platform_version) + .expect("verification runs"), + Some(true) + ); + + // The 24 block window holding core height u32::MAX - 5 would end past u32::MAX. Release + // builds have always wrapped its last block to 7, below any verification height near + // the core height, so the chain lock is left to Core; that must stay so, and must not + // overflow in builds that check arithmetic. + let core_height = u32::MAX - 5; + let state = state_with_one_chain_lock_quorum(&platform, 24, core_height); + assert_eq!( + platform + .platform + .verify_chain_lock_locally_v0( + 0, + &state, + &sign_chain_lock(core_height + 3), + platform_version + ) + .expect("verification runs"), + None + ); + } } From eec4f8df9a7ddf1e4ae305c0a599823026405771 Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Wed, 28 Oct 2026 13:58:05 +0700 Subject: [PATCH 2/4] fix(drive-abci): make a zero quorum window unrepresentable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refusing CHAIN_LOCK_QUORUM_WINDOW=0 when loading the config did not cover every path to verify_chain_lock_locally. That function reads the window from the platform state's chain lock quorum set, which a restarted node restores from its saved state, not from the config, and any code building a QuorumConfig could still put 0 there. - Every quorum window is now a NonZeroU32: the validator set, chain lock and instant lock configs, QuorumLikeConfig::quorum_window, QuorumConfig.window and the saved QuorumConfigForSavingV0.window. - Loading the config refuses 0 for VALIDATOR_SET_QUORUM_WINDOW, CHAIN_LOCK_QUORUM_WINDOW and INSTANT_LOCK_QUORUM_WINDOW with an error that names the variable. Strings and numbers are read as before. - grovedb-bincode 2.1.0 encodes a NonZeroU32 as the u32 it holds and decodes a 0 into DecodeError::NonZeroTypeIsZero, so saved states keep their bytes, and a saved window of 0 fails to restore instead of reaching chain lock verification. - verify_chain_lock_locally v0 takes the remainder with `u32 % NonZeroU32`, which cannot divide by zero, so the checked_rem error branch is gone. The end of the window keeps wrapping_add. No consensus change. For every window, v0 (protocol versions 1-14) computes what release builds always computed: the same remainder, and an end of window that wraps like their unchecked arithmetic. A window of 0 used to panic there and give no verdict; it can no longer be loaded. Adapted for v5.0-dev: - verify_chain_lock_locally has no v1 and the chain lock test helpers do not exist on this line, so their edits are dropped; v0's own test helper now builds its quorum with a 24 block window. - The v5.1-dev storage vectors are not on this line. The restore test lives in fetch_platform_state's tests and patches the two historical states v5.0-dev keeps as fixtures: PLATFORM_STATE_V3_TESTNET (first saved format, Llmq50_60 window 24) and PLATFORM_STATE_V8_DEVNET (second format, LlmqDevnet window 24). - The saved-format tests use bincode::config::standard().with_big_endian() .with_no_limit(), the configuration PlatformState saves with here, since the shared PLATFORM_BINCODE_CONFIG constant is not on this line. - for_saving_v0 and storage_tests have no tests to update on this line. Tests: - should_reject_zero_quorum_windows_when_loading_config now covers all three windows. - should_save_and_restore_window_with_the_bytes_of_a_u32 compares the saved config with a u32 window's encoding across every varint width. should_fail_to_restore_saved_window_of_zero decodes a saved 0. - should_refuse_to_restore_saved_state_holding_a_chain_lock_quorum_window_of_0 stores both fixtures with the chain lock window patched to 0 and fetches them at protocol version 13 (fetch v0) and 14 (fetch v1). The unpatched bytes still restore with window 24. - should_save_historical_platform_states_with_unchanged_bytes decodes both fixtures and re-encodes them to identical bytes. - should_return_error_instead_of_panicking_when_chain_lock_quorum_window_is_zero is removed: a window of 0 can no longer be built. Test would have caught this in CI: ✖ before, ✔ after. Run against the previous commit's code: - VALIDATOR_SET_QUORUM_WINDOW=0 and INSTANT_LOCK_QUORUM_WINDOW=0 loaded. - A saved QuorumConfigForSavingV0 with window 0 decoded. - Both fixtures with a chain lock window of 0 restored at protocol versions 13 and 14, with that window. (cherry picked from commit 13c009c7417abec8527ca4fd4f09f3aaa54e07f0) Co-Authored-By: Claude Opus 5.5 --- packages/rs-drive-abci/src/config.rs | 151 +++++++++++------- .../verify_chain_lock_locally/v0/mod.rs | 58 ++----- .../storage/fetch_platform_state/mod.rs | 135 ++++++++++++++++ .../v0/quorum_config_for_saving_v0.rs | 67 +++++++- .../v0/quorum_set.rs | 11 +- .../v0/quorums.rs | 5 +- .../strategy_tests/test_cases/basic_tests.rs | 5 +- .../test_cases/comprehensive_tests.rs | 5 +- .../test_cases/core_height_increase.rs | 29 ++-- 9 files changed, 336 insertions(+), 130 deletions(-) diff --git a/packages/rs-drive-abci/src/config.rs b/packages/rs-drive-abci/src/config.rs index 9f9755b861f..b4fbafddcb2 100644 --- a/packages/rs-drive-abci/src/config.rs +++ b/packages/rs-drive-abci/src/config.rs @@ -8,6 +8,7 @@ use dpp::util::deserializer::ProtocolVersion; use dpp::version::INITIAL_PROTOCOL_VERSION; use drive::config::{DriveConfig, DEFAULT_EPOCH_TIME_LENGTH_S}; use serde::{de::DeserializeOwned, Deserialize, Deserializer, Serialize}; +use std::num::NonZeroU32; use std::path::PathBuf; /// Configuration for Dash Core RPC client used in consensus logic @@ -307,7 +308,7 @@ pub trait QuorumLikeConfig: Sized { fn quorum_size(&self) -> u16; /// Quorum DKG interval - fn quorum_window(&self) -> u32; + fn quorum_window(&self) -> NonZeroU32; /// Quorum active signers count fn quorum_active_signers(&self) -> u16; @@ -339,9 +340,9 @@ pub struct ValidatorSetConfig { /// On Testnet Chain Locks are signed using 50_60: One quorum in every 24 blocks and activeQuorumCount is 24. #[serde( rename = "validator_set_quorum_window", - deserialize_with = "from_str_or_number" + deserialize_with = "deserialize_validator_set_quorum_window" )] - pub quorum_window: u32, + pub quorum_window: NonZeroU32, /// The number of active signers #[serde( @@ -371,7 +372,7 @@ impl ValidatorSetConfig { Self { quorum_type: QuorumType::Llmq100_67, quorum_size: 100, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).expect("24 is not 0"), quorum_active_signers: 24, quorum_rotation: false, } @@ -387,7 +388,7 @@ impl QuorumLikeConfig for ValidatorSetConfig { self.quorum_size } - fn quorum_window(&self) -> u32 { + fn quorum_window(&self) -> NonZeroU32 { self.quorum_window } @@ -421,12 +422,11 @@ pub struct ChainLockConfig { /// The quorum window (DKG interval) /// On Mainnet Chain Locks are signed using 400_60: One quorum in every 288 blocks and activeQuorumCount is 4. /// On Testnet Chain Locks are signed using 50_60: One quorum in every 24 blocks and activeQuorumCount is 24. - /// Loading the config refuses 0. #[serde( rename = "chain_lock_quorum_window", deserialize_with = "deserialize_chain_lock_quorum_window" )] - pub quorum_window: u32, + pub quorum_window: NonZeroU32, /// The number of active signers #[serde( @@ -449,7 +449,7 @@ impl Default for ChainLockConfig { Self { quorum_type: QuorumType::Llmq400_60, quorum_size: 400, - quorum_window: 24 * 12, + quorum_window: NonZeroU32::new(24 * 12).expect("24 * 12 is not 0"), quorum_active_signers: 4, quorum_rotation: false, } @@ -465,7 +465,7 @@ impl QuorumLikeConfig for ChainLockConfig { self.quorum_size } - fn quorum_window(&self) -> u32 { + fn quorum_window(&self) -> NonZeroU32 { self.quorum_window } @@ -484,7 +484,7 @@ impl ChainLockConfig { Self { quorum_type: QuorumType::Llmq100_67, quorum_size: 100, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).expect("24 is not 0"), quorum_active_signers: 24, quorum_rotation: false, } @@ -514,9 +514,9 @@ pub struct InstantLockConfig { /// On Testnet Chain Locks are signed using 50_60: One quorum in every 24 blocks and activeQuorumCount is 24. #[serde( rename = "instant_lock_quorum_window", - deserialize_with = "from_str_or_number" + deserialize_with = "deserialize_instant_lock_quorum_window" )] - pub quorum_window: u32, + pub quorum_window: NonZeroU32, /// The number of active signers #[serde( @@ -540,7 +540,7 @@ impl Default for InstantLockConfig { quorum_type: QuorumType::Llmq60_75, quorum_active_signers: 32, quorum_size: 60, - quorum_window: 24 * 12, + quorum_window: NonZeroU32::new(24 * 12).expect("24 * 12 is not 0"), quorum_rotation: true, } } @@ -552,7 +552,7 @@ impl InstantLockConfig { Self { quorum_type: QuorumType::Llmq100_67, quorum_size: 100, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).expect("24 is not 0"), quorum_active_signers: 24, quorum_rotation: false, } @@ -568,7 +568,7 @@ impl QuorumLikeConfig for InstantLockConfig { self.quorum_size } - fn quorum_window(&self) -> u32 { + fn quorum_window(&self) -> NonZeroU32 { self.quorum_window } @@ -610,22 +610,41 @@ where Ok(quorum_type) } -/// Chain lock verification finds the DKG interval a core height is in by dividing by the window, -/// so a window of 0 is refused when the config loads instead of failing chain lock verification -/// while validating proposals. -fn deserialize_chain_lock_quorum_window<'de, D>(deserializer: D) -> Result +/// Reads a quorum window. The windows are `NonZeroU32` because chain lock verification divides +/// a core height by the window to find its DKG interval; 0 is refused here with an error naming +/// `setting`, the variable the window was read from. +fn deserialize_quorum_window<'de, D>(deserializer: D, setting: &str) -> Result where D: serde::Deserializer<'de>, { let quorum_window: u32 = from_str_or_number(deserializer)?; - if quorum_window == 0 { - return Err(serde::de::Error::custom( - "CHAIN_LOCK_QUORUM_WINDOW must be greater than 0: it is the DKG interval of the chain lock quorum type", - )); - } + NonZeroU32::new(quorum_window).ok_or_else(|| { + serde::de::Error::custom(format!( + "{setting} must be greater than 0: it is the DKG interval of the quorum type" + )) + }) +} - Ok(quorum_window) +fn deserialize_validator_set_quorum_window<'de, D>(deserializer: D) -> Result +where + D: serde::Deserializer<'de>, +{ + deserialize_quorum_window(deserializer, "VALIDATOR_SET_QUORUM_WINDOW") +} + +fn deserialize_chain_lock_quorum_window<'de, D>(deserializer: D) -> Result +where + D: serde::Deserializer<'de>, +{ + deserialize_quorum_window(deserializer, "CHAIN_LOCK_QUORUM_WINDOW") +} + +fn deserialize_instant_lock_quorum_window<'de, D>(deserializer: D) -> Result +where + D: serde::Deserializer<'de>, +{ + deserialize_quorum_window(deserializer, "INSTANT_LOCK_QUORUM_WINDOW") } impl ExecutionConfig { @@ -723,7 +742,7 @@ impl PlatformConfig { validator_set: ValidatorSetConfig { quorum_type: QuorumType::LlmqTestPlatform, quorum_size: 3, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).expect("24 is not 0"), quorum_active_signers: 2, quorum_rotation: false, }, @@ -731,14 +750,14 @@ impl PlatformConfig { quorum_type: QuorumType::LlmqTest, quorum_active_signers: 2, quorum_size: 3, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).expect("24 is not 0"), quorum_rotation: false, }, instant_lock: InstantLockConfig { quorum_type: QuorumType::LlmqTest, quorum_active_signers: 2, quorum_size: 3, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).expect("24 is not 0"), quorum_rotation: false, }, block_spacing_ms: 5000, @@ -765,14 +784,14 @@ impl PlatformConfig { validator_set: ValidatorSetConfig { quorum_type: QuorumType::LlmqDevnetPlatform, quorum_size: 12, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).expect("24 is not 0"), quorum_active_signers: 8, quorum_rotation: false, }, chain_lock: ChainLockConfig { quorum_type: QuorumType::LlmqDevnetPlatform, quorum_size: 12, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).expect("24 is not 0"), quorum_active_signers: 8, quorum_rotation: false, }, @@ -780,7 +799,7 @@ impl PlatformConfig { quorum_type: QuorumType::LlmqDevnetDip0024, quorum_active_signers: 4, quorum_size: 8, - quorum_window: 48, + quorum_window: NonZeroU32::new(48).expect("48 is not 0"), quorum_rotation: true, }, block_spacing_ms: 5000, @@ -807,7 +826,7 @@ impl PlatformConfig { validator_set: ValidatorSetConfig { quorum_type: QuorumType::Llmq25_67, quorum_size: 25, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).expect("24 is not 0"), quorum_active_signers: 24, quorum_rotation: false, }, @@ -815,14 +834,14 @@ impl PlatformConfig { quorum_type: QuorumType::Llmq50_60, quorum_active_signers: 24, quorum_size: 50, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).expect("24 is not 0"), quorum_rotation: false, }, instant_lock: InstantLockConfig { quorum_type: QuorumType::Llmq60_75, quorum_active_signers: 32, quorum_size: 60, - quorum_window: 24 * 12, + quorum_window: NonZeroU32::new(24 * 12).expect("24 * 12 is not 0"), quorum_rotation: true, }, block_spacing_ms: 5000, @@ -849,7 +868,7 @@ impl PlatformConfig { validator_set: ValidatorSetConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 100, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).expect("24 is not 0"), quorum_active_signers: 24, quorum_rotation: false, }, @@ -857,14 +876,14 @@ impl PlatformConfig { quorum_type: QuorumType::Llmq400_60, quorum_active_signers: 4, quorum_size: 400, - quorum_window: 24 * 12, + quorum_window: NonZeroU32::new(24 * 12).expect("24 * 12 is not 0"), quorum_rotation: false, }, instant_lock: InstantLockConfig { quorum_type: QuorumType::Llmq60_75, quorum_active_signers: 32, quorum_size: 60, - quorum_window: 24 * 12, + quorum_window: NonZeroU32::new(24 * 12).expect("24 * 12 is not 0"), quorum_rotation: true, }, block_spacing_ms: 5000, @@ -977,6 +996,7 @@ mod tests { use dpp::dashcore::Network; use dpp::dashcore_rpc::dashcore_rpc_json::QuorumType; use std::env; + use std::num::NonZeroU32; #[test] fn test_config_from_env() { @@ -1131,19 +1151,20 @@ mod tests { } /// A zero chain lock quorum window would divide by zero while validating a proposal, so - /// loading the config must refuse it instead of letting the node start. The variables are - /// read from `.env.local` without touching the process environment, which other tests set. + /// loading the config must refuse a zero quorum window instead of letting the node start. + /// The variables are read from `.env.local` without touching the process environment, which + /// other tests set. #[test] - fn should_reject_zero_chain_lock_quorum_window_when_loading_config() { + fn should_reject_zero_quorum_windows_when_loading_config() { let envfile = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")).join(".env.local"); let vars: Vec<(String, String)> = dotenvy::from_path_iter(envfile.as_path()) .expect("cannot open .env file") .collect::>() .expect("cannot parse .env file"); - let with_chain_lock_quorum_window = |window: &str| -> Vec<(String, String)> { + let with_var = |name: &str, window: &str| -> Vec<(String, String)> { vars.iter() .map(|(key, value)| { - if key == "CHAIN_LOCK_QUORUM_WINDOW" { + if key == name { (key.clone(), window.to_string()) } else { (key.clone(), value.clone()) @@ -1152,18 +1173,32 @@ mod tests { .collect() }; - let err = envy::from_iter::<_, PlatformConfig>(with_chain_lock_quorum_window("0")) - .expect_err("a zero chain lock quorum window should fail"); - let msg = err.to_string(); - assert!( - msg.contains("CHAIN_LOCK_QUORUM_WINDOW must be greater than 0"), - "the error should name the offending option, got: {}", - msg - ); + type QuorumWindow = fn(&PlatformConfig) -> NonZeroU32; + let settings: [(&str, QuorumWindow); 3] = [ + ("VALIDATOR_SET_QUORUM_WINDOW", |config| { + config.validator_set.quorum_window + }), + ("CHAIN_LOCK_QUORUM_WINDOW", |config| { + config.chain_lock.quorum_window + }), + ("INSTANT_LOCK_QUORUM_WINDOW", |config| { + config.instant_lock.quorum_window + }), + ]; - let config = envy::from_iter::<_, PlatformConfig>(with_chain_lock_quorum_window("1")) - .expect("a chain lock quorum window of 1 should be accepted"); - assert_eq!(config.chain_lock.quorum_window, 1); + for (name, quorum_window) in settings { + let err = envy::from_iter::<_, PlatformConfig>(with_var(name, "0")) + .expect_err("a zero quorum window should fail"); + let msg = err.to_string(); + assert!( + msg.contains(&format!("{name} must be greater than 0")), + "the error should name {name}, got: {msg}" + ); + + let config = envy::from_iter::<_, PlatformConfig>(with_var(name, "1")) + .unwrap_or_else(|e| panic!("{name}=1 should be accepted: {e}")); + assert_eq!(quorum_window(&config).get(), 1, "{name}"); + } } // --- Core RPC URL composition and defaults --- @@ -1235,7 +1270,7 @@ mod tests { let cfg = ValidatorSetConfig::default_100_67(); assert_eq!(cfg.quorum_type(), QuorumType::Llmq100_67); assert_eq!(cfg.quorum_size(), 100); - assert_eq!(cfg.quorum_window(), 24); + assert_eq!(cfg.quorum_window().get(), 24); assert_eq!(cfg.quorum_active_signers(), 24); assert!(!cfg.quorum_rotation()); } @@ -1246,7 +1281,7 @@ mod tests { let cfg = ChainLockConfig::default_100_67(); assert_eq!(cfg.quorum_type(), QuorumType::Llmq100_67); assert_eq!(cfg.quorum_size(), 100); - assert_eq!(cfg.quorum_window(), 24); + assert_eq!(cfg.quorum_window().get(), 24); assert_eq!(cfg.quorum_active_signers(), 24); assert!(!cfg.quorum_rotation()); } @@ -1257,7 +1292,7 @@ mod tests { let cfg = ChainLockConfig::default(); assert_eq!(cfg.quorum_type(), QuorumType::Llmq400_60); assert_eq!(cfg.quorum_size(), 400); - assert_eq!(cfg.quorum_window(), 24 * 12); + assert_eq!(cfg.quorum_window().get(), 24 * 12); assert_eq!(cfg.quorum_active_signers(), 4); assert!(!cfg.quorum_rotation()); } @@ -1268,7 +1303,7 @@ mod tests { let cfg = InstantLockConfig::default(); assert_eq!(cfg.quorum_type(), QuorumType::Llmq60_75); assert_eq!(cfg.quorum_size(), 60); - assert_eq!(cfg.quorum_window(), 24 * 12); + assert_eq!(cfg.quorum_window().get(), 24 * 12); assert_eq!(cfg.quorum_active_signers(), 32); assert!(cfg.quorum_rotation()); } @@ -1279,7 +1314,7 @@ mod tests { let cfg = InstantLockConfig::default_100_67(); assert_eq!(cfg.quorum_type(), QuorumType::Llmq100_67); assert_eq!(cfg.quorum_size(), 100); - assert_eq!(cfg.quorum_window(), 24); + assert_eq!(cfg.quorum_window().get(), 24); assert_eq!(cfg.quorum_active_signers(), 24); assert!(!cfg.quorum_rotation()); } diff --git a/packages/rs-drive-abci/src/execution/platform_events/core_chain_lock/verify_chain_lock_locally/v0/mod.rs b/packages/rs-drive-abci/src/execution/platform_events/core_chain_lock/verify_chain_lock_locally/v0/mod.rs index d6bbd675e85..2f3a9e22364 100644 --- a/packages/rs-drive-abci/src/execution/platform_events/core_chain_lock/verify_chain_lock_locally/v0/mod.rs +++ b/packages/rs-drive-abci/src/execution/platform_events/core_chain_lock/verify_chain_lock_locally/v0/mod.rs @@ -66,19 +66,13 @@ where let window_width = quorum_config.window; - // A window of 0, which loading the config refuses, is an error here rather than a panic; - // either way the proposal gets no verdict. For every other window the result is the one - // release builds have always computed: their arithmetic wraps, so a window ending past - // the largest core height wraps too. - let offset_in_window = platform_state - .last_committed_core_height() - .checked_rem(window_width) - .ok_or(Error::Execution(ExecutionError::CorruptedCodeExecution( - "the chain lock quorum window is 0", - )))?; - // The last block in the window where the quorums would be the same - let last_block_in_window = (platform_state.last_committed_core_height() - offset_in_window) - .wrapping_add(window_width - 1); + // The last block in the window where the quorums would be the same. The window is a + // `NonZeroU32`, so the remainder cannot divide by zero. This is what release builds have + // always computed for every window: their arithmetic wraps, so a window ending past the + // largest core height wraps here too instead of overflowing in builds that check it. + let last_block_in_window = (platform_state.last_committed_core_height() + - platform_state.last_committed_core_height() % window_width) + .wrapping_add(window_width.get() - 1); let verification_height = chain_lock_height.saturating_sub(SIGN_OFFSET); @@ -244,8 +238,6 @@ where #[cfg(test)] mod tests { use crate::config::ChainLockConfig; - use crate::error::execution::ExecutionError; - use crate::error::Error; use crate::execution::platform_events::core_chain_lock::verify_chain_lock_locally::v0::CHAIN_LOCK_REQUEST_ID_PREFIX; use crate::platform_types::platform_state::{PlatformState, PlatformStateV0Methods}; use crate::platform_types::signature_verification_quorum_set::{ @@ -259,6 +251,7 @@ mod tests { use dpp::dashcore::{BlockHash, ChainLock, QuorumHash, QuorumSigningRequestId}; use dpp::dashcore_rpc::json::QuorumType; use dpp::version::PlatformVersion; + use std::num::NonZeroU32; #[test] fn verify_request_id() { @@ -401,10 +394,9 @@ mod tests { const QUORUM_HASH: [u8; 32] = [1u8; 32]; /// `platform`'s state with last committed core height `core_height`, holding one llmq_test - /// chain lock quorum with a `quorum_window` block DKG interval + /// chain lock quorum with a 24 block DKG interval fn state_with_one_chain_lock_quorum( platform: &TempPlatform, - quorum_window: u32, core_height: u32, ) -> PlatformState { let mut state = platform.platform.state.load().as_ref().clone(); @@ -418,7 +410,7 @@ mod tests { ChainLockConfig { quorum_type: QuorumType::LlmqTest, quorum_size: 3, - quorum_window, + quorum_window: NonZeroU32::new(24).unwrap(), quorum_active_signers: 2, quorum_rotation: false, } @@ -478,32 +470,6 @@ mod tests { .set_initial_state_structure() } - #[test] - fn should_return_error_instead_of_panicking_when_chain_lock_quorum_window_is_zero() { - // A chain lock quorum window of 0 can still reach the platform state, which keeps the - // window it was created with. Validating a proposal must then fail with an error - // instead of dividing by zero and taking the node down. - let platform = chain_lock_test_platform(); - let state = state_with_one_chain_lock_quorum(&platform, 0, 1106); - - let result = platform.platform.verify_chain_lock_locally_v0( - 0, - &state, - &sign_chain_lock(1110), - PlatformVersion::latest(), - ); - - assert!( - matches!( - result, - Err(Error::Execution(ExecutionError::CorruptedCodeExecution( - "the chain lock quorum window is 0" - ))) - ), - "a zero quorum window must be refused, got {result:?}" - ); - } - #[test] fn should_leave_chain_lock_to_core_without_overflowing_when_quorum_window_ends_past_largest_core_height( ) { @@ -511,7 +477,7 @@ mod tests { let platform_version = PlatformVersion::latest(); // The quorum verifies its chain locks locally while its window ends below u32::MAX - let state = state_with_one_chain_lock_quorum(&platform, 24, 1106); + let state = state_with_one_chain_lock_quorum(&platform, 1106); assert_eq!( platform .platform @@ -525,7 +491,7 @@ mod tests { // the core height, so the chain lock is left to Core; that must stay so, and must not // overflow in builds that check arithmetic. let core_height = u32::MAX - 5; - let state = state_with_one_chain_lock_quorum(&platform, 24, core_height); + let state = state_with_one_chain_lock_quorum(&platform, core_height); assert_eq!( platform .platform diff --git a/packages/rs-drive-abci/src/execution/storage/fetch_platform_state/mod.rs b/packages/rs-drive-abci/src/execution/storage/fetch_platform_state/mod.rs index 0f9032227ec..5c4d7ec1e28 100644 --- a/packages/rs-drive-abci/src/execution/storage/fetch_platform_state/mod.rs +++ b/packages/rs-drive-abci/src/execution/storage/fetch_platform_state/mod.rs @@ -34,3 +34,138 @@ impl Platform { } } } + +#[cfg(test)] +mod tests { + use crate::error::Error; + use crate::platform_types::platform::Platform; + use crate::platform_types::platform_state::platform_state_for_saving::PlatformStateForSaving; + use crate::platform_types::platform_state::PlatformStateV0Methods; + use crate::platform_types::signature_verification_quorum_set::SignatureVerificationQuorumSetV0Methods; + use crate::rpc::core::MockCoreRPCLike; + use crate::test::fixture::platform_state::{ + PLATFORM_STATE_V3_TESTNET, PLATFORM_STATE_V8_DEVNET, + }; + use crate::test::helpers::setup::TestPlatformBuilder; + use bincode::config::{BigEndian, Configuration, NoLimit, Varint}; + use dpp::dashcore_rpc::json::QuorumType; + use dpp::version::PlatformVersion; + use dpp::ProtocolError; + use std::ops::Deref; + + /// The bincode configuration platform states are saved with + const PLATFORM_BINCODE_CONFIG: Configuration = + bincode::config::standard() + .with_big_endian() + .with_no_limit(); + + /// A saved quorum config: its type, active signers, rotation and window + type SavedQuorumConfig = (QuorumType, u16, bool, u32); + + /// A testnet state in the first saved format and a devnet state in the second, each with + /// the chain lock quorum config it was saved with + fn fixtures() -> [(&'static str, SavedQuorumConfig); 2] { + [ + ( + PLATFORM_STATE_V3_TESTNET.deref(), + (QuorumType::Llmq50_60, 24, false, 24), + ), + ( + PLATFORM_STATE_V8_DEVNET.deref(), + (QuorumType::LlmqDevnet, 4, false, 24), + ), + ] + } + + /// The bytes of `fixture` with the window of its chain lock quorum config, saved as `saved`, + /// set to `window`, encoded the way a `u32` window was + fn with_chain_lock_quorum_window( + fixture: &str, + saved: SavedQuorumConfig, + window: u32, + ) -> Vec { + let encode = |config: SavedQuorumConfig| { + bincode::encode_to_vec(config, PLATFORM_BINCODE_CONFIG).unwrap() + }; + let saved_bytes = encode(saved); + let mut bytes = hex::decode(fixture).unwrap(); + let positions: Vec = bytes + .windows(saved_bytes.len()) + .enumerate() + .filter_map(|(position, window)| (window == saved_bytes).then_some(position)) + .collect(); + assert_eq!( + positions.len(), + 1, + "the chain lock quorum config is saved once" + ); + let (quorum_type, active_signers, rotation, _) = saved; + bytes.splice( + positions[0]..positions[0] + saved_bytes.len(), + encode((quorum_type, active_signers, rotation, window)), + ); + bytes + } + + #[test] + fn should_save_historical_platform_states_with_unchanged_bytes() { + for (fixture, _) in fixtures() { + let bytes = hex::decode(fixture).unwrap(); + let (saved, read): (PlatformStateForSaving, _) = + bincode::decode_from_slice(&bytes, PLATFORM_BINCODE_CONFIG).unwrap(); + assert_eq!(read, bytes.len()); + assert_eq!( + bincode::encode_to_vec(&saved, PLATFORM_BINCODE_CONFIG).unwrap(), + bytes + ); + } + } + + #[test] + fn should_refuse_to_restore_saved_state_holding_a_chain_lock_quorum_window_of_0() { + // Chain lock verification divides by the chain lock quorum window, which a restarted node + // reads from its saved state rather than from its config. A saved window of 0 must stop the + // node from loading that state instead of reaching chain lock verification. + let platform = TestPlatformBuilder::new().build_with_mock_rpc(); + let restore = |bytes: &[u8], platform_version: &PlatformVersion| { + let transaction = platform.drive.grove.start_transaction(); + platform + .drive + .store_platform_state_bytes(bytes, Some(&transaction), platform_version) + .unwrap(); + Platform::::fetch_platform_state( + &platform.drive, + Some(&transaction), + platform_version, + ) + }; + + // Protocol version 13 reads the saved state with fetch_platform_state v0, 14 with v1 + for platform_version in [PlatformVersion::get(13).unwrap(), PlatformVersion::latest()] { + for (fixture, saved) in fixtures() { + let state = restore(&hex::decode(fixture).unwrap(), platform_version) + .unwrap() + .unwrap(); + assert_eq!( + state.chain_lock_validating_quorums().config().window.get(), + saved.3 + ); + + let result = restore( + &with_chain_lock_quorum_window(fixture, saved, 0), + platform_version, + ); + assert!( + matches!( + &result, + Err(Error::Protocol(ProtocolError::PlatformDeserializationError(message))) + if message.contains("NonZeroTypeIsZero") + ), + "protocol version {}, {:?}: got {result:?}", + platform_version.protocol_version, + saved.0 + ); + } + } + } +} diff --git a/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorum_config_for_saving_v0.rs b/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorum_config_for_saving_v0.rs index ddffeec3893..31819953518 100644 --- a/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorum_config_for_saving_v0.rs +++ b/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorum_config_for_saving_v0.rs @@ -2,13 +2,16 @@ use crate::platform_types::signature_verification_quorum_set::QuorumConfig; use bincode::Encode; use dpp::dashcore_rpc::dashcore_rpc_json::QuorumType; use dpp::platform_serialization::de::Decode; +use std::num::NonZeroU32; #[derive(Debug, Clone, Encode, Decode)] pub struct QuorumConfigForSavingV0 { quorum_type: QuorumType, active_signers: u16, rotation: bool, - window: u32, + /// Encoded exactly as a `u32`, so states saved while this field was a `u32` still decode; + /// a saved window of 0 fails to decode instead of reaching chain lock verification. + window: NonZeroU32, } impl From for QuorumConfigForSavingV0 { @@ -32,3 +35,65 @@ impl From for QuorumConfig { } } } + +#[cfg(test)] +mod tests { + use super::*; + use bincode::config::{BigEndian, Configuration, NoLimit, Varint}; + use bincode::error::{DecodeError, IntegerType}; + + /// The bincode configuration platform states are saved with + const PLATFORM_BINCODE_CONFIG: Configuration = + bincode::config::standard() + .with_big_endian() + .with_no_limit(); + + /// The saved config as it was written while the window was a `u32` + fn saved_with_u32_window(window: u32) -> Vec { + bincode::encode_to_vec( + (QuorumType::Llmq400_60, 4u16, false, window), + PLATFORM_BINCODE_CONFIG, + ) + .expect("encodes") + } + + #[test] + fn should_save_and_restore_window_with_the_bytes_of_a_u32() { + // Every varint width: one byte up to 250, then u16, u32 markers + for window in [1, 24, 250, 251, 288, u32::from(u16::MAX) + 1, u32::MAX] { + let config = QuorumConfigForSavingV0 { + quorum_type: QuorumType::Llmq400_60, + active_signers: 4, + rotation: false, + window: NonZeroU32::new(window).unwrap(), + }; + + let bytes = bincode::encode_to_vec(&config, PLATFORM_BINCODE_CONFIG).unwrap(); + assert_eq!(bytes, saved_with_u32_window(window), "window {window}"); + + let (restored, read): (QuorumConfigForSavingV0, _) = + bincode::decode_from_slice(&bytes, PLATFORM_BINCODE_CONFIG).unwrap(); + assert_eq!(read, bytes.len()); + assert_eq!(QuorumConfig::from(restored).window.get(), window); + } + } + + #[test] + fn should_fail_to_restore_saved_window_of_zero() { + // Chain lock verification divides by the window, so a saved 0 must not decode + let result = bincode::decode_from_slice::( + &saved_with_u32_window(0), + PLATFORM_BINCODE_CONFIG, + ); + + assert!( + matches!( + result, + Err(DecodeError::NonZeroTypeIsZero { + non_zero_type: IntegerType::U32 + }) + ), + "got {result:?}" + ); + } +} diff --git a/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorum_set.rs b/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorum_set.rs index 881347bc6ae..a5c97d00572 100644 --- a/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorum_set.rs +++ b/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorum_set.rs @@ -3,6 +3,7 @@ use crate::platform_types::signature_verification_quorum_set::v0::quorums::Quoru use crate::platform_types::signature_verification_quorum_set::VerificationQuorum; use dpp::dashcore::QuorumHash; use dpp::dashcore_rpc::json::QuorumType; +use std::num::NonZeroU32; use std::vec::IntoIter; /// Offset for signature verification @@ -148,7 +149,7 @@ pub struct QuorumConfig { /// Is it a DIP24 rotating quorum or classic pub rotation: bool, /// DKG interval - pub window: u32, + pub window: NonZeroU32, } impl SignatureVerificationQuorumSetV0Methods for SignatureVerificationQuorumSetV0 { @@ -339,7 +340,7 @@ mod tests { ChainLockConfig { quorum_type: QuorumType::Llmq400_60, quorum_size: 400, - quorum_window: 288, + quorum_window: NonZeroU32::new(288).unwrap(), quorum_active_signers: 4, quorum_rotation: false, } @@ -355,7 +356,7 @@ mod tests { assert_eq!(qs.config().quorum_type, QuorumType::Llmq400_60); assert_eq!(qs.config().active_signers, 4); assert!(!qs.config().rotation); - assert_eq!(qs.config().window, 288); + assert_eq!(qs.config().window.get(), 288); assert!(qs.current_quorums().is_empty()); assert!(!qs.has_previous_past_quorums()); } @@ -365,7 +366,7 @@ mod tests { let config = ChainLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 100, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).unwrap(), quorum_active_signers: 24, quorum_rotation: true, }; @@ -374,7 +375,7 @@ mod tests { assert_eq!(qs.config().quorum_type, QuorumType::Llmq100_67); assert_eq!(qs.config().active_signers, 24); assert!(qs.config().rotation); - assert_eq!(qs.config().window, 24); + assert_eq!(qs.config().window.get(), 24); } // ---- set_current_quorums / current_quorums ---- diff --git a/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorums.rs b/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorums.rs index 2781697e1dc..0cf53787f89 100644 --- a/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorums.rs +++ b/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorums.rs @@ -232,6 +232,7 @@ mod tests { use dpp::bls_signatures::{Bls12381G2Impl, SecretKey as BlsPrivateKey}; use dpp::dashcore::hashes::Hash; use dpp::dashcore_rpc::json::QuorumType; + use std::num::NonZeroU32; /// Helper: generate a deterministic BLS public key from a seed byte. fn make_public_key(seed: u8) -> ThresholdBlsPublicKey { @@ -255,7 +256,7 @@ mod tests { quorum_type: QuorumType::Llmq100_67, active_signers: 24, rotation: false, - window: 24, + window: NonZeroU32::new(24).unwrap(), } } @@ -264,7 +265,7 @@ mod tests { quorum_type: QuorumType::Llmq60_75, active_signers, rotation: true, - window: 24, + window: NonZeroU32::new(24).unwrap(), } } diff --git a/packages/rs-drive-abci/tests/strategy_tests/test_cases/basic_tests.rs b/packages/rs-drive-abci/tests/strategy_tests/test_cases/basic_tests.rs index 8447a00afbd..6a9f13185a3 100644 --- a/packages/rs-drive-abci/tests/strategy_tests/test_cases/basic_tests.rs +++ b/packages/rs-drive-abci/tests/strategy_tests/test_cases/basic_tests.rs @@ -8,6 +8,7 @@ mod tests { use dpp::block::extended_block_info::v0::ExtendedBlockInfoV0Getters; use dpp::dashcore_rpc::json::QuorumType; use std::collections::BTreeMap; + use std::num::NonZeroU32; use strategy_tests::{IdentityInsertInfo, StartAddresses, StartIdentities, Strategy}; use drive_abci::config::{ @@ -462,14 +463,14 @@ mod tests { chain_lock: ChainLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 3, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).unwrap(), quorum_active_signers: 24, quorum_rotation: false, }, instant_lock: InstantLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 3, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).unwrap(), quorum_active_signers: 24, quorum_rotation: false, }, diff --git a/packages/rs-drive-abci/tests/strategy_tests/test_cases/comprehensive_tests.rs b/packages/rs-drive-abci/tests/strategy_tests/test_cases/comprehensive_tests.rs index 54140c26377..bde37fc2321 100644 --- a/packages/rs-drive-abci/tests/strategy_tests/test_cases/comprehensive_tests.rs +++ b/packages/rs-drive-abci/tests/strategy_tests/test_cases/comprehensive_tests.rs @@ -36,6 +36,7 @@ mod tests { use rand::prelude::StdRng; use rand::SeedableRng; use simple_signer::signer::SimpleSigner; + use std::num::NonZeroU32; use strategy_tests::frequency::Frequency; use strategy_tests::operations::DocumentAction::DocumentActionReplaceRandom; use strategy_tests::operations::{ @@ -364,14 +365,14 @@ mod tests { chain_lock: ChainLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 10, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).unwrap(), quorum_active_signers: 24, quorum_rotation: false, }, instant_lock: InstantLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 10, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).unwrap(), quorum_active_signers: 24, quorum_rotation: false, }, diff --git a/packages/rs-drive-abci/tests/strategy_tests/test_cases/core_height_increase.rs b/packages/rs-drive-abci/tests/strategy_tests/test_cases/core_height_increase.rs index 19b1b861005..e95dbcabc1c 100644 --- a/packages/rs-drive-abci/tests/strategy_tests/test_cases/core_height_increase.rs +++ b/packages/rs-drive-abci/tests/strategy_tests/test_cases/core_height_increase.rs @@ -20,6 +20,7 @@ mod tests { use drive_abci::test::helpers::setup::TestPlatformBuilder; use itertools::Itertools; use platform_version::version::PlatformVersion; + use std::num::NonZeroU32; use strategy_tests::frequency::Frequency; use strategy_tests::{IdentityInsertInfo, StartAddresses, StartIdentities, Strategy}; #[tokio::test] @@ -255,14 +256,14 @@ mod tests { chain_lock: ChainLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 10, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).unwrap(), quorum_active_signers: 24, quorum_rotation: false, }, instant_lock: InstantLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 10, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).unwrap(), quorum_active_signers: 24, quorum_rotation: false, }, @@ -366,14 +367,14 @@ mod tests { chain_lock: ChainLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 10, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).unwrap(), quorum_active_signers: 24, quorum_rotation: false, }, instant_lock: InstantLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 10, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).unwrap(), quorum_active_signers: 24, quorum_rotation: false, }, @@ -458,14 +459,14 @@ mod tests { chain_lock: ChainLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 10, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).unwrap(), quorum_active_signers: 24, quorum_rotation: false, }, instant_lock: InstantLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 10, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).unwrap(), quorum_active_signers: 24, quorum_rotation: false, }, @@ -545,14 +546,14 @@ mod tests { chain_lock: ChainLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 10, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).unwrap(), quorum_active_signers: 24, quorum_rotation: false, }, instant_lock: InstantLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 10, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).unwrap(), quorum_active_signers: 24, quorum_rotation: false, }, @@ -666,14 +667,14 @@ mod tests { chain_lock: ChainLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 3, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).unwrap(), quorum_active_signers: 24, quorum_rotation: false, }, instant_lock: InstantLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 3, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).unwrap(), quorum_active_signers: 24, quorum_rotation: false, }, @@ -863,14 +864,14 @@ mod tests { chain_lock: ChainLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 3, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).unwrap(), quorum_active_signers: 24, quorum_rotation: false, }, instant_lock: InstantLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 3, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).unwrap(), quorum_active_signers: 24, quorum_rotation: false, }, @@ -1039,14 +1040,14 @@ mod tests { chain_lock: ChainLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 3, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).unwrap(), quorum_active_signers: 24, quorum_rotation: false, }, instant_lock: InstantLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 3, - quorum_window: 24, + quorum_window: NonZeroU32::new(24).unwrap(), quorum_active_signers: 24, quorum_rotation: false, }, From 84f846601002a59731ddf117a3fb6d6a59340463 Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Wed, 28 Oct 2026 14:06:57 +0700 Subject: [PATCH 3/4] fix(drive-abci): make a zero quorum active signers count unrepresentable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Choosing a rotated (DIP24) quorum takes the base-2 logarithm of the quorum's active signers as `31 - active_signers.leading_zeros()`. With 0 active signers that subtraction underflows: debug builds panic, and release builds compute a meaningless signer index. Nothing refused 0. Loading the config accepted VALIDATOR_SET_, CHAIN_LOCK_ and INSTANT_LOCK_QUORUM_ACTIVE_SIGNERS=0, and a saved state holding 0 restored. - The active signers are now a NonZeroU16 wherever they are held: the three quorum configs, QuorumLikeConfig::quorum_active_signers, QuorumConfig.active_signers and the saved QuorumConfigForSavingV0. - Loading the config refuses 0 for the three *_QUORUM_ACTIVE_SIGNERS variables with " must be greater than 0". One generic deserializer, deserialize_non_zero, now reads the windows and the active signers. - grovedb-bincode 2.1.0 encodes a NonZeroU16 as the u16 it holds and decodes a 0 into DecodeError::NonZeroTypeIsZero, so saved states keep their bytes, and a saved count of 0 fails to restore. - choose_rotating_quorum takes NonZeroU16::ilog2(), which cannot underflow. No consensus change. choose_rotating_quorum is not versioned. Every protocol version reaches it through verify_chain_lock_locally v0 and verify_recent_signature_locally v0. For every count from 1 to u16::MAX, ilog2() equals `31 - leading_zeros()` of the count as a u32, so it returns the same quorum. A count of 0 can no longer be loaded from the config or a saved state. Adapted for v5.0-dev: verify_chain_lock_locally v1, the chain lock test helpers, for_saving_v0's tests and storage_tests are not on this line, so their edits are dropped. v0's own test helper and the restore test in fetch_platform_state's tests take the edits instead; the restore test patches the testnet and devnet fixtures that v5.0-dev keeps. Tests: - should_reject_zero_quorum_windows_and_active_signers_when_loading_config covers the three windows and the three active signers counts. - should_save_and_restore_active_signers_and_window_with_the_bytes_of_integers compares the saved config with the plain integers' encoding across every varint width. should_fail_to_restore_saved_active_signers_or_window_of_zero decodes a saved 0 of each. - should_refuse_to_restore_saved_state_holding_zero_chain_lock_active_signers_or_window stores both fixtures with the chain lock config patched and fetches them at protocol versions 13 and 14. Counts and windows of 1 restore; a 0 in either fails. - choose_rotating_quorum_takes_signer_from_log2_of_active_signers_bits pins the chosen signer for 1, 2, 32 and u16::MAX active signers. It passes with ilog2() and with the previous `31 - leading_zeros()` formula alike. Test would have caught this in CI: ✖ before, ✔ after. Run against the previous commit's code: - VALIDATOR_SET_, CHAIN_LOCK_ and INSTANT_LOCK_QUORUM_ACTIVE_SIGNERS=0 all loaded. - INSTANT_LOCK_QUORUM_ACTIVE_SIGNERS=0 with rotation loaded, and choosing a quorum from it panicked with "attempt to subtract with overflow" (quorums.rs:106). - A saved QuorumConfigForSavingV0 with 0 active signers decoded. - Both fixtures with chain lock active signers 0 restored at protocol versions 13 and 14, with that count. After this commit, the zero counts are refused and the restore fails. (cherry picked from commit 98f3778cb5e43ab889549faa398a0984821d1581) Co-Authored-By: Claude Opus 5.5 --- packages/rs-drive-abci/src/config.rs | 162 +++++++++++------- .../verify_chain_lock_locally/v0/mod.rs | 4 +- .../storage/fetch_platform_state/mod.rs | 62 ++++--- .../v0/quorum_config_for_saving_v0.rs | 90 ++++++---- .../v0/quorum_set.rs | 12 +- .../v0/quorums.rs | 42 ++++- .../strategy_tests/test_cases/basic_tests.rs | 6 +- .../test_cases/comprehensive_tests.rs | 6 +- .../test_cases/core_height_increase.rs | 30 ++-- 9 files changed, 252 insertions(+), 162 deletions(-) diff --git a/packages/rs-drive-abci/src/config.rs b/packages/rs-drive-abci/src/config.rs index b4fbafddcb2..2afe7d808d4 100644 --- a/packages/rs-drive-abci/src/config.rs +++ b/packages/rs-drive-abci/src/config.rs @@ -8,8 +8,10 @@ use dpp::util::deserializer::ProtocolVersion; use dpp::version::INITIAL_PROTOCOL_VERSION; use drive::config::{DriveConfig, DEFAULT_EPOCH_TIME_LENGTH_S}; use serde::{de::DeserializeOwned, Deserialize, Deserializer, Serialize}; -use std::num::NonZeroU32; +use std::fmt::Display; +use std::num::{NonZeroU16, NonZeroU32}; use std::path::PathBuf; +use std::str::FromStr; /// Configuration for Dash Core RPC client used in consensus logic #[derive(Clone, Debug, Serialize, Deserialize, Default)] @@ -311,7 +313,7 @@ pub trait QuorumLikeConfig: Sized { fn quorum_window(&self) -> NonZeroU32; /// Quorum active signers count - fn quorum_active_signers(&self) -> u16; + fn quorum_active_signers(&self) -> NonZeroU16; /// Quorum rotation (dip24) or classic fn quorum_rotation(&self) -> bool; @@ -347,9 +349,9 @@ pub struct ValidatorSetConfig { /// The number of active signers #[serde( rename = "validator_set_quorum_active_signers", - deserialize_with = "from_str_or_number" + deserialize_with = "deserialize_validator_set_quorum_active_signers" )] - pub quorum_active_signers: u16, + pub quorum_active_signers: NonZeroU16, /// Whether the quorum is rotated DIP24 or classic #[serde( @@ -373,7 +375,7 @@ impl ValidatorSetConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 100, quorum_window: NonZeroU32::new(24).expect("24 is not 0"), - quorum_active_signers: 24, + quorum_active_signers: NonZeroU16::new(24).expect("24 is not 0"), quorum_rotation: false, } } @@ -392,7 +394,7 @@ impl QuorumLikeConfig for ValidatorSetConfig { self.quorum_window } - fn quorum_active_signers(&self) -> u16 { + fn quorum_active_signers(&self) -> NonZeroU16 { self.quorum_active_signers } @@ -431,9 +433,9 @@ pub struct ChainLockConfig { /// The number of active signers #[serde( rename = "chain_lock_quorum_active_signers", - deserialize_with = "from_str_or_number" + deserialize_with = "deserialize_chain_lock_quorum_active_signers" )] - pub quorum_active_signers: u16, + pub quorum_active_signers: NonZeroU16, /// Whether the quorum is rotated DIP24 or classic #[serde( @@ -450,7 +452,7 @@ impl Default for ChainLockConfig { quorum_type: QuorumType::Llmq400_60, quorum_size: 400, quorum_window: NonZeroU32::new(24 * 12).expect("24 * 12 is not 0"), - quorum_active_signers: 4, + quorum_active_signers: NonZeroU16::new(4).expect("4 is not 0"), quorum_rotation: false, } } @@ -469,7 +471,7 @@ impl QuorumLikeConfig for ChainLockConfig { self.quorum_window } - fn quorum_active_signers(&self) -> u16 { + fn quorum_active_signers(&self) -> NonZeroU16 { self.quorum_active_signers } @@ -485,7 +487,7 @@ impl ChainLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 100, quorum_window: NonZeroU32::new(24).expect("24 is not 0"), - quorum_active_signers: 24, + quorum_active_signers: NonZeroU16::new(24).expect("24 is not 0"), quorum_rotation: false, } } @@ -521,9 +523,9 @@ pub struct InstantLockConfig { /// The number of active signers #[serde( rename = "instant_lock_quorum_active_signers", - deserialize_with = "from_str_or_number" + deserialize_with = "deserialize_instant_lock_quorum_active_signers" )] - pub quorum_active_signers: u16, + pub quorum_active_signers: NonZeroU16, /// Whether the quorum is rotated DIP24 or classic #[serde( @@ -538,7 +540,7 @@ impl Default for InstantLockConfig { // Mainnet Self { quorum_type: QuorumType::Llmq60_75, - quorum_active_signers: 32, + quorum_active_signers: NonZeroU16::new(32).expect("32 is not 0"), quorum_size: 60, quorum_window: NonZeroU32::new(24 * 12).expect("24 * 12 is not 0"), quorum_rotation: true, @@ -553,7 +555,7 @@ impl InstantLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 100, quorum_window: NonZeroU32::new(24).expect("24 is not 0"), - quorum_active_signers: 24, + quorum_active_signers: NonZeroU16::new(24).expect("24 is not 0"), quorum_rotation: false, } } @@ -572,7 +574,7 @@ impl QuorumLikeConfig for InstantLockConfig { self.quorum_window } - fn quorum_active_signers(&self) -> u16 { + fn quorum_active_signers(&self) -> NonZeroU16 { self.quorum_active_signers } @@ -610,41 +612,69 @@ where Ok(quorum_type) } -/// Reads a quorum window. The windows are `NonZeroU32` because chain lock verification divides -/// a core height by the window to find its DKG interval; 0 is refused here with an error naming -/// `setting`, the variable the window was read from. -fn deserialize_quorum_window<'de, D>(deserializer: D, setting: &str) -> Result +/// Reads a quorum setting that has to be greater than 0, refusing 0 with an error naming +/// `setting`, the variable it was read from. Chain lock verification divides a core height by the +/// quorum window to find its DKG interval, and choosing a rotated quorum takes the base-2 +/// logarithm of the active signers, so neither can be 0 wherever it is held. +fn deserialize_non_zero<'de, D, T, N>(deserializer: D, setting: &str) -> Result where D: serde::Deserializer<'de>, + T: Deserialize<'de> + FromStr, + ::Err: Display, + N: TryFrom, { - let quorum_window: u32 = from_str_or_number(deserializer)?; + let value: T = from_str_or_number(deserializer)?; - NonZeroU32::new(quorum_window).ok_or_else(|| { - serde::de::Error::custom(format!( - "{setting} must be greater than 0: it is the DKG interval of the quorum type" - )) - }) + N::try_from(value) + .map_err(|_| serde::de::Error::custom(format!("{setting} must be greater than 0"))) } fn deserialize_validator_set_quorum_window<'de, D>(deserializer: D) -> Result where D: serde::Deserializer<'de>, { - deserialize_quorum_window(deserializer, "VALIDATOR_SET_QUORUM_WINDOW") + deserialize_non_zero::<_, u32, _>(deserializer, "VALIDATOR_SET_QUORUM_WINDOW") +} + +fn deserialize_validator_set_quorum_active_signers<'de, D>( + deserializer: D, +) -> Result +where + D: serde::Deserializer<'de>, +{ + deserialize_non_zero::<_, u16, _>(deserializer, "VALIDATOR_SET_QUORUM_ACTIVE_SIGNERS") } fn deserialize_chain_lock_quorum_window<'de, D>(deserializer: D) -> Result where D: serde::Deserializer<'de>, { - deserialize_quorum_window(deserializer, "CHAIN_LOCK_QUORUM_WINDOW") + deserialize_non_zero::<_, u32, _>(deserializer, "CHAIN_LOCK_QUORUM_WINDOW") +} + +fn deserialize_chain_lock_quorum_active_signers<'de, D>( + deserializer: D, +) -> Result +where + D: serde::Deserializer<'de>, +{ + deserialize_non_zero::<_, u16, _>(deserializer, "CHAIN_LOCK_QUORUM_ACTIVE_SIGNERS") } fn deserialize_instant_lock_quorum_window<'de, D>(deserializer: D) -> Result where D: serde::Deserializer<'de>, { - deserialize_quorum_window(deserializer, "INSTANT_LOCK_QUORUM_WINDOW") + deserialize_non_zero::<_, u32, _>(deserializer, "INSTANT_LOCK_QUORUM_WINDOW") +} + +fn deserialize_instant_lock_quorum_active_signers<'de, D>( + deserializer: D, +) -> Result +where + D: serde::Deserializer<'de>, +{ + deserialize_non_zero::<_, u16, _>(deserializer, "INSTANT_LOCK_QUORUM_ACTIVE_SIGNERS") } impl ExecutionConfig { @@ -743,19 +773,19 @@ impl PlatformConfig { quorum_type: QuorumType::LlmqTestPlatform, quorum_size: 3, quorum_window: NonZeroU32::new(24).expect("24 is not 0"), - quorum_active_signers: 2, + quorum_active_signers: NonZeroU16::new(2).expect("2 is not 0"), quorum_rotation: false, }, chain_lock: ChainLockConfig { quorum_type: QuorumType::LlmqTest, - quorum_active_signers: 2, + quorum_active_signers: NonZeroU16::new(2).expect("2 is not 0"), quorum_size: 3, quorum_window: NonZeroU32::new(24).expect("24 is not 0"), quorum_rotation: false, }, instant_lock: InstantLockConfig { quorum_type: QuorumType::LlmqTest, - quorum_active_signers: 2, + quorum_active_signers: NonZeroU16::new(2).expect("2 is not 0"), quorum_size: 3, quorum_window: NonZeroU32::new(24).expect("24 is not 0"), quorum_rotation: false, @@ -785,19 +815,19 @@ impl PlatformConfig { quorum_type: QuorumType::LlmqDevnetPlatform, quorum_size: 12, quorum_window: NonZeroU32::new(24).expect("24 is not 0"), - quorum_active_signers: 8, + quorum_active_signers: NonZeroU16::new(8).expect("8 is not 0"), quorum_rotation: false, }, chain_lock: ChainLockConfig { quorum_type: QuorumType::LlmqDevnetPlatform, quorum_size: 12, quorum_window: NonZeroU32::new(24).expect("24 is not 0"), - quorum_active_signers: 8, + quorum_active_signers: NonZeroU16::new(8).expect("8 is not 0"), quorum_rotation: false, }, instant_lock: InstantLockConfig { quorum_type: QuorumType::LlmqDevnetDip0024, - quorum_active_signers: 4, + quorum_active_signers: NonZeroU16::new(4).expect("4 is not 0"), quorum_size: 8, quorum_window: NonZeroU32::new(48).expect("48 is not 0"), quorum_rotation: true, @@ -827,19 +857,19 @@ impl PlatformConfig { quorum_type: QuorumType::Llmq25_67, quorum_size: 25, quorum_window: NonZeroU32::new(24).expect("24 is not 0"), - quorum_active_signers: 24, + quorum_active_signers: NonZeroU16::new(24).expect("24 is not 0"), quorum_rotation: false, }, chain_lock: ChainLockConfig { quorum_type: QuorumType::Llmq50_60, - quorum_active_signers: 24, + quorum_active_signers: NonZeroU16::new(24).expect("24 is not 0"), quorum_size: 50, quorum_window: NonZeroU32::new(24).expect("24 is not 0"), quorum_rotation: false, }, instant_lock: InstantLockConfig { quorum_type: QuorumType::Llmq60_75, - quorum_active_signers: 32, + quorum_active_signers: NonZeroU16::new(32).expect("32 is not 0"), quorum_size: 60, quorum_window: NonZeroU32::new(24 * 12).expect("24 * 12 is not 0"), quorum_rotation: true, @@ -869,19 +899,19 @@ impl PlatformConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 100, quorum_window: NonZeroU32::new(24).expect("24 is not 0"), - quorum_active_signers: 24, + quorum_active_signers: NonZeroU16::new(24).expect("24 is not 0"), quorum_rotation: false, }, chain_lock: ChainLockConfig { quorum_type: QuorumType::Llmq400_60, - quorum_active_signers: 4, + quorum_active_signers: NonZeroU16::new(4).expect("4 is not 0"), quorum_size: 400, quorum_window: NonZeroU32::new(24 * 12).expect("24 * 12 is not 0"), quorum_rotation: false, }, instant_lock: InstantLockConfig { quorum_type: QuorumType::Llmq60_75, - quorum_active_signers: 32, + quorum_active_signers: NonZeroU16::new(32).expect("32 is not 0"), quorum_size: 60, quorum_window: NonZeroU32::new(24 * 12).expect("24 * 12 is not 0"), quorum_rotation: true, @@ -996,7 +1026,6 @@ mod tests { use dpp::dashcore::Network; use dpp::dashcore_rpc::dashcore_rpc_json::QuorumType; use std::env; - use std::num::NonZeroU32; #[test] fn test_config_from_env() { @@ -1150,22 +1179,22 @@ mod tests { ); } - /// A zero chain lock quorum window would divide by zero while validating a proposal, so - /// loading the config must refuse a zero quorum window instead of letting the node start. - /// The variables are read from `.env.local` without touching the process environment, which - /// other tests set. + /// A zero quorum window divides by zero in chain lock verification, and zero active signers + /// underflow choosing a rotated quorum, so loading the config must refuse either instead of + /// letting the node start. The variables are read from `.env.local` without touching the + /// process environment, which other tests set. #[test] - fn should_reject_zero_quorum_windows_when_loading_config() { + fn should_reject_zero_quorum_windows_and_active_signers_when_loading_config() { let envfile = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")).join(".env.local"); let vars: Vec<(String, String)> = dotenvy::from_path_iter(envfile.as_path()) .expect("cannot open .env file") .collect::>() .expect("cannot parse .env file"); - let with_var = |name: &str, window: &str| -> Vec<(String, String)> { + let with_var = |name: &str, setting: &str| -> Vec<(String, String)> { vars.iter() .map(|(key, value)| { if key == name { - (key.clone(), window.to_string()) + (key.clone(), setting.to_string()) } else { (key.clone(), value.clone()) } @@ -1173,22 +1202,31 @@ mod tests { .collect() }; - type QuorumWindow = fn(&PlatformConfig) -> NonZeroU32; - let settings: [(&str, QuorumWindow); 3] = [ + type Setting = fn(&PlatformConfig) -> u32; + let settings: [(&str, Setting); 6] = [ ("VALIDATOR_SET_QUORUM_WINDOW", |config| { - config.validator_set.quorum_window + config.validator_set.quorum_window.get() + }), + ("VALIDATOR_SET_QUORUM_ACTIVE_SIGNERS", |config| { + config.validator_set.quorum_active_signers.get().into() }), ("CHAIN_LOCK_QUORUM_WINDOW", |config| { - config.chain_lock.quorum_window + config.chain_lock.quorum_window.get() + }), + ("CHAIN_LOCK_QUORUM_ACTIVE_SIGNERS", |config| { + config.chain_lock.quorum_active_signers.get().into() }), ("INSTANT_LOCK_QUORUM_WINDOW", |config| { - config.instant_lock.quorum_window + config.instant_lock.quorum_window.get() + }), + ("INSTANT_LOCK_QUORUM_ACTIVE_SIGNERS", |config| { + config.instant_lock.quorum_active_signers.get().into() }), ]; - for (name, quorum_window) in settings { + for (name, setting) in settings { let err = envy::from_iter::<_, PlatformConfig>(with_var(name, "0")) - .expect_err("a zero quorum window should fail"); + .expect_err("a zero quorum setting should fail"); let msg = err.to_string(); assert!( msg.contains(&format!("{name} must be greater than 0")), @@ -1197,7 +1235,7 @@ mod tests { let config = envy::from_iter::<_, PlatformConfig>(with_var(name, "1")) .unwrap_or_else(|e| panic!("{name}=1 should be accepted: {e}")); - assert_eq!(quorum_window(&config).get(), 1, "{name}"); + assert_eq!(setting(&config), 1, "{name}"); } } @@ -1271,7 +1309,7 @@ mod tests { assert_eq!(cfg.quorum_type(), QuorumType::Llmq100_67); assert_eq!(cfg.quorum_size(), 100); assert_eq!(cfg.quorum_window().get(), 24); - assert_eq!(cfg.quorum_active_signers(), 24); + assert_eq!(cfg.quorum_active_signers().get(), 24); assert!(!cfg.quorum_rotation()); } @@ -1282,7 +1320,7 @@ mod tests { assert_eq!(cfg.quorum_type(), QuorumType::Llmq100_67); assert_eq!(cfg.quorum_size(), 100); assert_eq!(cfg.quorum_window().get(), 24); - assert_eq!(cfg.quorum_active_signers(), 24); + assert_eq!(cfg.quorum_active_signers().get(), 24); assert!(!cfg.quorum_rotation()); } @@ -1293,7 +1331,7 @@ mod tests { assert_eq!(cfg.quorum_type(), QuorumType::Llmq400_60); assert_eq!(cfg.quorum_size(), 400); assert_eq!(cfg.quorum_window().get(), 24 * 12); - assert_eq!(cfg.quorum_active_signers(), 4); + assert_eq!(cfg.quorum_active_signers().get(), 4); assert!(!cfg.quorum_rotation()); } @@ -1304,7 +1342,7 @@ mod tests { assert_eq!(cfg.quorum_type(), QuorumType::Llmq60_75); assert_eq!(cfg.quorum_size(), 60); assert_eq!(cfg.quorum_window().get(), 24 * 12); - assert_eq!(cfg.quorum_active_signers(), 32); + assert_eq!(cfg.quorum_active_signers().get(), 32); assert!(cfg.quorum_rotation()); } @@ -1315,7 +1353,7 @@ mod tests { assert_eq!(cfg.quorum_type(), QuorumType::Llmq100_67); assert_eq!(cfg.quorum_size(), 100); assert_eq!(cfg.quorum_window().get(), 24); - assert_eq!(cfg.quorum_active_signers(), 24); + assert_eq!(cfg.quorum_active_signers().get(), 24); assert!(!cfg.quorum_rotation()); } diff --git a/packages/rs-drive-abci/src/execution/platform_events/core_chain_lock/verify_chain_lock_locally/v0/mod.rs b/packages/rs-drive-abci/src/execution/platform_events/core_chain_lock/verify_chain_lock_locally/v0/mod.rs index 2f3a9e22364..30e8bd5cbd8 100644 --- a/packages/rs-drive-abci/src/execution/platform_events/core_chain_lock/verify_chain_lock_locally/v0/mod.rs +++ b/packages/rs-drive-abci/src/execution/platform_events/core_chain_lock/verify_chain_lock_locally/v0/mod.rs @@ -251,7 +251,7 @@ mod tests { use dpp::dashcore::{BlockHash, ChainLock, QuorumHash, QuorumSigningRequestId}; use dpp::dashcore_rpc::json::QuorumType; use dpp::version::PlatformVersion; - use std::num::NonZeroU32; + use std::num::{NonZeroU16, NonZeroU32}; #[test] fn verify_request_id() { @@ -411,7 +411,7 @@ mod tests { quorum_type: QuorumType::LlmqTest, quorum_size: 3, quorum_window: NonZeroU32::new(24).unwrap(), - quorum_active_signers: 2, + quorum_active_signers: NonZeroU16::new(2).unwrap(), quorum_rotation: false, } .into(), diff --git a/packages/rs-drive-abci/src/execution/storage/fetch_platform_state/mod.rs b/packages/rs-drive-abci/src/execution/storage/fetch_platform_state/mod.rs index 5c4d7ec1e28..22a583c69bb 100644 --- a/packages/rs-drive-abci/src/execution/storage/fetch_platform_state/mod.rs +++ b/packages/rs-drive-abci/src/execution/storage/fetch_platform_state/mod.rs @@ -77,11 +77,12 @@ mod tests { ] } - /// The bytes of `fixture` with the window of its chain lock quorum config, saved as `saved`, - /// set to `window`, encoded the way a `u32` window was - fn with_chain_lock_quorum_window( + /// The bytes of `fixture` with its chain lock quorum config, saved as `saved`, holding + /// `active_signers` and `window`, encoded the way a `u16` and a `u32` were + fn with_chain_lock_quorum_config( fixture: &str, saved: SavedQuorumConfig, + active_signers: u16, window: u32, ) -> Vec { let encode = |config: SavedQuorumConfig| { @@ -99,7 +100,7 @@ mod tests { 1, "the chain lock quorum config is saved once" ); - let (quorum_type, active_signers, rotation, _) = saved; + let (quorum_type, _, rotation, _) = saved; bytes.splice( positions[0]..positions[0] + saved_bytes.len(), encode((quorum_type, active_signers, rotation, window)), @@ -122,10 +123,11 @@ mod tests { } #[test] - fn should_refuse_to_restore_saved_state_holding_a_chain_lock_quorum_window_of_0() { - // Chain lock verification divides by the chain lock quorum window, which a restarted node - // reads from its saved state rather than from its config. A saved window of 0 must stop the - // node from loading that state instead of reaching chain lock verification. + fn should_refuse_to_restore_saved_state_holding_zero_chain_lock_active_signers_or_window() { + // A restarted node reads its chain lock quorum config from its saved state rather than from + // its config. Choosing a rotated quorum takes the logarithm of the active signers and chain + // lock verification divides by the window, so a saved 0 in either must stop the node from + // loading that state instead of reaching them. let platform = TestPlatformBuilder::new().build_with_mock_rpc(); let restore = |bytes: &[u8], platform_version: &PlatformVersion| { let transaction = platform.drive.grove.start_transaction(); @@ -143,28 +145,34 @@ mod tests { // Protocol version 13 reads the saved state with fetch_platform_state v0, 14 with v1 for platform_version in [PlatformVersion::get(13).unwrap(), PlatformVersion::latest()] { for (fixture, saved) in fixtures() { - let state = restore(&hex::decode(fixture).unwrap(), platform_version) - .unwrap() - .unwrap(); + let state = restore( + &with_chain_lock_quorum_config(fixture, saved, 1, 1), + platform_version, + ) + .unwrap() + .unwrap(); + let restored = state.chain_lock_validating_quorums().config(); assert_eq!( - state.chain_lock_validating_quorums().config().window.get(), - saved.3 + (restored.active_signers.get(), restored.window.get()), + (1, 1) ); - let result = restore( - &with_chain_lock_quorum_window(fixture, saved, 0), - platform_version, - ); - assert!( - matches!( - &result, - Err(Error::Protocol(ProtocolError::PlatformDeserializationError(message))) - if message.contains("NonZeroTypeIsZero") - ), - "protocol version {}, {:?}: got {result:?}", - platform_version.protocol_version, - saved.0 - ); + for (active_signers, window) in [(0, saved.3), (saved.1, 0)] { + let result = restore( + &with_chain_lock_quorum_config(fixture, saved, active_signers, window), + platform_version, + ); + assert!( + matches!( + &result, + Err(Error::Protocol(ProtocolError::PlatformDeserializationError(message))) + if message.contains("NonZeroTypeIsZero") + ), + "protocol version {}, {:?}, active signers {active_signers}, window {window}: got {result:?}", + platform_version.protocol_version, + saved.0 + ); + } } } } diff --git a/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorum_config_for_saving_v0.rs b/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorum_config_for_saving_v0.rs index 31819953518..e4bce87884e 100644 --- a/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorum_config_for_saving_v0.rs +++ b/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorum_config_for_saving_v0.rs @@ -2,15 +2,16 @@ use crate::platform_types::signature_verification_quorum_set::QuorumConfig; use bincode::Encode; use dpp::dashcore_rpc::dashcore_rpc_json::QuorumType; use dpp::platform_serialization::de::Decode; -use std::num::NonZeroU32; +use std::num::{NonZeroU16, NonZeroU32}; +/// The active signers and the window are encoded exactly as the `u16` and `u32` they hold, so +/// states saved while these fields were plain integers still decode; a saved 0 in either fails to +/// decode instead of reaching quorum selection or chain lock verification. #[derive(Debug, Clone, Encode, Decode)] pub struct QuorumConfigForSavingV0 { quorum_type: QuorumType, - active_signers: u16, + active_signers: NonZeroU16, rotation: bool, - /// Encoded exactly as a `u32`, so states saved while this field was a `u32` still decode; - /// a saved window of 0 fails to decode instead of reaching chain lock verification. window: NonZeroU32, } @@ -48,52 +49,67 @@ mod tests { .with_big_endian() .with_no_limit(); - /// The saved config as it was written while the window was a `u32` - fn saved_with_u32_window(window: u32) -> Vec { + /// The saved config as it was written while the active signers and the window were a `u16` + /// and a `u32` + fn saved_with_integers(active_signers: u16, window: u32) -> Vec { bincode::encode_to_vec( - (QuorumType::Llmq400_60, 4u16, false, window), + (QuorumType::Llmq400_60, active_signers, false, window), PLATFORM_BINCODE_CONFIG, ) .expect("encodes") } #[test] - fn should_save_and_restore_window_with_the_bytes_of_a_u32() { - // Every varint width: one byte up to 250, then u16, u32 markers - for window in [1, 24, 250, 251, 288, u32::from(u16::MAX) + 1, u32::MAX] { - let config = QuorumConfigForSavingV0 { - quorum_type: QuorumType::Llmq400_60, - active_signers: 4, - rotation: false, - window: NonZeroU32::new(window).unwrap(), - }; + fn should_save_and_restore_active_signers_and_window_with_the_bytes_of_integers() { + // Every varint width: one byte up to 250, then the u16 and u32 markers + let active_signers_counts = [1, 4, 250, 251, u16::MAX]; + let windows = [1, 24, 250, 251, 288, u32::from(u16::MAX) + 1, u32::MAX]; + for active_signers in active_signers_counts { + for window in windows { + let config = QuorumConfigForSavingV0 { + quorum_type: QuorumType::Llmq400_60, + active_signers: NonZeroU16::new(active_signers).unwrap(), + rotation: false, + window: NonZeroU32::new(window).unwrap(), + }; - let bytes = bincode::encode_to_vec(&config, PLATFORM_BINCODE_CONFIG).unwrap(); - assert_eq!(bytes, saved_with_u32_window(window), "window {window}"); + let bytes = bincode::encode_to_vec(&config, PLATFORM_BINCODE_CONFIG).unwrap(); + assert_eq!( + bytes, + saved_with_integers(active_signers, window), + "active signers {active_signers}, window {window}" + ); - let (restored, read): (QuorumConfigForSavingV0, _) = - bincode::decode_from_slice(&bytes, PLATFORM_BINCODE_CONFIG).unwrap(); - assert_eq!(read, bytes.len()); - assert_eq!(QuorumConfig::from(restored).window.get(), window); + let (restored, read): (QuorumConfigForSavingV0, _) = + bincode::decode_from_slice(&bytes, PLATFORM_BINCODE_CONFIG).unwrap(); + assert_eq!(read, bytes.len()); + let restored = QuorumConfig::from(restored); + assert_eq!(restored.active_signers.get(), active_signers); + assert_eq!(restored.window.get(), window); + } } } #[test] - fn should_fail_to_restore_saved_window_of_zero() { - // Chain lock verification divides by the window, so a saved 0 must not decode - let result = bincode::decode_from_slice::( - &saved_with_u32_window(0), - PLATFORM_BINCODE_CONFIG, - ); + fn should_fail_to_restore_saved_active_signers_or_window_of_zero() { + // Choosing a rotated quorum takes the logarithm of the active signers and chain lock + // verification divides by the window, so a saved 0 in either must not decode + for (bytes, zero_type) in [ + (saved_with_integers(0, 288), IntegerType::U16), + (saved_with_integers(4, 0), IntegerType::U32), + ] { + let result = bincode::decode_from_slice::( + &bytes, + PLATFORM_BINCODE_CONFIG, + ); - assert!( - matches!( - result, - Err(DecodeError::NonZeroTypeIsZero { - non_zero_type: IntegerType::U32 - }) - ), - "got {result:?}" - ); + assert!( + matches!( + &result, + Err(DecodeError::NonZeroTypeIsZero { non_zero_type }) if *non_zero_type == zero_type + ), + "expected a zero {zero_type:?}, got {result:?}" + ); + } } } diff --git a/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorum_set.rs b/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorum_set.rs index a5c97d00572..bb86b2fdfa4 100644 --- a/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorum_set.rs +++ b/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorum_set.rs @@ -3,7 +3,7 @@ use crate::platform_types::signature_verification_quorum_set::v0::quorums::Quoru use crate::platform_types::signature_verification_quorum_set::VerificationQuorum; use dpp::dashcore::QuorumHash; use dpp::dashcore_rpc::json::QuorumType; -use std::num::NonZeroU32; +use std::num::{NonZeroU16, NonZeroU32}; use std::vec::IntoIter; /// Offset for signature verification @@ -145,7 +145,7 @@ pub struct QuorumConfig { /// Type pub quorum_type: QuorumType, /// Active quorum signers count - pub active_signers: u16, + pub active_signers: NonZeroU16, /// Is it a DIP24 rotating quorum or classic pub rotation: bool, /// DKG interval @@ -341,7 +341,7 @@ mod tests { quorum_type: QuorumType::Llmq400_60, quorum_size: 400, quorum_window: NonZeroU32::new(288).unwrap(), - quorum_active_signers: 4, + quorum_active_signers: NonZeroU16::new(4).unwrap(), quorum_rotation: false, } } @@ -354,7 +354,7 @@ mod tests { let qs = SignatureVerificationQuorumSetV0::new(&config); assert_eq!(qs.config().quorum_type, QuorumType::Llmq400_60); - assert_eq!(qs.config().active_signers, 4); + assert_eq!(qs.config().active_signers.get(), 4); assert!(!qs.config().rotation); assert_eq!(qs.config().window.get(), 288); assert!(qs.current_quorums().is_empty()); @@ -367,13 +367,13 @@ mod tests { quorum_type: QuorumType::Llmq100_67, quorum_size: 100, quorum_window: NonZeroU32::new(24).unwrap(), - quorum_active_signers: 24, + quorum_active_signers: NonZeroU16::new(24).unwrap(), quorum_rotation: true, }; let qs: SignatureVerificationQuorumSetV0 = config.into(); assert_eq!(qs.config().quorum_type, QuorumType::Llmq100_67); - assert_eq!(qs.config().active_signers, 24); + assert_eq!(qs.config().active_signers.get(), 24); assert!(qs.config().rotation); assert_eq!(qs.config().window.get(), 24); } diff --git a/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorums.rs b/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorums.rs index 0cf53787f89..6629a885104 100644 --- a/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorums.rs +++ b/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorums.rs @@ -100,10 +100,10 @@ impl Quorums { quorum_config: &QuorumConfig, request_id: &[u8; 32], ) -> Option<(QuorumHash, &Q)> { - let active_signers = quorum_config.active_signers as u32; - - // binary (base-2) logarithm from active_signers - let n = 31 - active_signers.leading_zeros(); + // binary (base-2) logarithm from active_signers: `31 - leading_zeros()` of the count as + // a u32, the same at every protocol version. At least one signer is active, so it cannot + // underflow. + let n = quorum_config.active_signers.ilog2(); // Extract last 64 bits of request_id let b = u64::from_le_bytes( @@ -232,7 +232,7 @@ mod tests { use dpp::bls_signatures::{Bls12381G2Impl, SecretKey as BlsPrivateKey}; use dpp::dashcore::hashes::Hash; use dpp::dashcore_rpc::json::QuorumType; - use std::num::NonZeroU32; + use std::num::{NonZeroU16, NonZeroU32}; /// Helper: generate a deterministic BLS public key from a seed byte. fn make_public_key(seed: u8) -> ThresholdBlsPublicKey { @@ -254,7 +254,7 @@ mod tests { fn make_classic_config() -> QuorumConfig { QuorumConfig { quorum_type: QuorumType::Llmq100_67, - active_signers: 24, + active_signers: NonZeroU16::new(24).unwrap(), rotation: false, window: NonZeroU32::new(24).unwrap(), } @@ -263,7 +263,7 @@ mod tests { fn make_rotating_config(active_signers: u16) -> QuorumConfig { QuorumConfig { quorum_type: QuorumType::Llmq60_75, - active_signers, + active_signers: NonZeroU16::new(active_signers).unwrap(), rotation: true, window: NonZeroU32::new(24).unwrap(), } @@ -458,6 +458,34 @@ mod tests { ); } + #[test] + fn choose_rotating_quorum_takes_signer_from_log2_of_active_signers_bits() { + // The signer is the `log2(active_signers)` bits below the top bit of the request id's + // last 8 bytes. All of them are set here, so the signer is the largest index those bits + // can hold, from a single active signer (no bits) to u16::MAX of them (15 bits). + let request_id = [0xff; 32]; + for (active_signers, signer) in [(1, 0), (2, 1), (32, 31), (u16::MAX, 32767)] { + let quorums: Quorums = [0, signer] + .into_iter() + .map(|index| { + ( + QuorumHash::from_byte_array([index as u8; 32]), + make_verification_quorum(1, Some(index)), + ) + }) + .collect(); + + let (_, chosen) = quorums + .choose_quorum(&make_rotating_config(active_signers), &request_id) + .expect("the signer's quorum is held"); + assert_eq!( + chosen.index, + Some(signer), + "{active_signers} active signers" + ); + } + } + #[test] fn choose_quorum_routes_by_config_rotation_flag() { let hash = QuorumHash::from_byte_array([20u8; 32]); diff --git a/packages/rs-drive-abci/tests/strategy_tests/test_cases/basic_tests.rs b/packages/rs-drive-abci/tests/strategy_tests/test_cases/basic_tests.rs index 6a9f13185a3..254060057e5 100644 --- a/packages/rs-drive-abci/tests/strategy_tests/test_cases/basic_tests.rs +++ b/packages/rs-drive-abci/tests/strategy_tests/test_cases/basic_tests.rs @@ -8,7 +8,7 @@ mod tests { use dpp::block::extended_block_info::v0::ExtendedBlockInfoV0Getters; use dpp::dashcore_rpc::json::QuorumType; use std::collections::BTreeMap; - use std::num::NonZeroU32; + use std::num::{NonZeroU16, NonZeroU32}; use strategy_tests::{IdentityInsertInfo, StartAddresses, StartIdentities, Strategy}; use drive_abci::config::{ @@ -464,14 +464,14 @@ mod tests { quorum_type: QuorumType::Llmq100_67, quorum_size: 3, quorum_window: NonZeroU32::new(24).unwrap(), - quorum_active_signers: 24, + quorum_active_signers: NonZeroU16::new(24).unwrap(), quorum_rotation: false, }, instant_lock: InstantLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 3, quorum_window: NonZeroU32::new(24).unwrap(), - quorum_active_signers: 24, + quorum_active_signers: NonZeroU16::new(24).unwrap(), quorum_rotation: false, }, execution: ExecutionConfig { diff --git a/packages/rs-drive-abci/tests/strategy_tests/test_cases/comprehensive_tests.rs b/packages/rs-drive-abci/tests/strategy_tests/test_cases/comprehensive_tests.rs index bde37fc2321..06a58177fc4 100644 --- a/packages/rs-drive-abci/tests/strategy_tests/test_cases/comprehensive_tests.rs +++ b/packages/rs-drive-abci/tests/strategy_tests/test_cases/comprehensive_tests.rs @@ -36,7 +36,7 @@ mod tests { use rand::prelude::StdRng; use rand::SeedableRng; use simple_signer::signer::SimpleSigner; - use std::num::NonZeroU32; + use std::num::{NonZeroU16, NonZeroU32}; use strategy_tests::frequency::Frequency; use strategy_tests::operations::DocumentAction::DocumentActionReplaceRandom; use strategy_tests::operations::{ @@ -366,14 +366,14 @@ mod tests { quorum_type: QuorumType::Llmq100_67, quorum_size: 10, quorum_window: NonZeroU32::new(24).unwrap(), - quorum_active_signers: 24, + quorum_active_signers: NonZeroU16::new(24).unwrap(), quorum_rotation: false, }, instant_lock: InstantLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 10, quorum_window: NonZeroU32::new(24).unwrap(), - quorum_active_signers: 24, + quorum_active_signers: NonZeroU16::new(24).unwrap(), quorum_rotation: false, }, execution: ExecutionConfig { diff --git a/packages/rs-drive-abci/tests/strategy_tests/test_cases/core_height_increase.rs b/packages/rs-drive-abci/tests/strategy_tests/test_cases/core_height_increase.rs index e95dbcabc1c..2129dc4a685 100644 --- a/packages/rs-drive-abci/tests/strategy_tests/test_cases/core_height_increase.rs +++ b/packages/rs-drive-abci/tests/strategy_tests/test_cases/core_height_increase.rs @@ -20,7 +20,7 @@ mod tests { use drive_abci::test::helpers::setup::TestPlatformBuilder; use itertools::Itertools; use platform_version::version::PlatformVersion; - use std::num::NonZeroU32; + use std::num::{NonZeroU16, NonZeroU32}; use strategy_tests::frequency::Frequency; use strategy_tests::{IdentityInsertInfo, StartAddresses, StartIdentities, Strategy}; #[tokio::test] @@ -257,14 +257,14 @@ mod tests { quorum_type: QuorumType::Llmq100_67, quorum_size: 10, quorum_window: NonZeroU32::new(24).unwrap(), - quorum_active_signers: 24, + quorum_active_signers: NonZeroU16::new(24).unwrap(), quorum_rotation: false, }, instant_lock: InstantLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 10, quorum_window: NonZeroU32::new(24).unwrap(), - quorum_active_signers: 24, + quorum_active_signers: NonZeroU16::new(24).unwrap(), quorum_rotation: false, }, execution: ExecutionConfig { @@ -368,14 +368,14 @@ mod tests { quorum_type: QuorumType::Llmq100_67, quorum_size: 10, quorum_window: NonZeroU32::new(24).unwrap(), - quorum_active_signers: 24, + quorum_active_signers: NonZeroU16::new(24).unwrap(), quorum_rotation: false, }, instant_lock: InstantLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 10, quorum_window: NonZeroU32::new(24).unwrap(), - quorum_active_signers: 24, + quorum_active_signers: NonZeroU16::new(24).unwrap(), quorum_rotation: false, }, execution: ExecutionConfig { @@ -460,14 +460,14 @@ mod tests { quorum_type: QuorumType::Llmq100_67, quorum_size: 10, quorum_window: NonZeroU32::new(24).unwrap(), - quorum_active_signers: 24, + quorum_active_signers: NonZeroU16::new(24).unwrap(), quorum_rotation: false, }, instant_lock: InstantLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 10, quorum_window: NonZeroU32::new(24).unwrap(), - quorum_active_signers: 24, + quorum_active_signers: NonZeroU16::new(24).unwrap(), quorum_rotation: false, }, execution: ExecutionConfig { @@ -547,14 +547,14 @@ mod tests { quorum_type: QuorumType::Llmq100_67, quorum_size: 10, quorum_window: NonZeroU32::new(24).unwrap(), - quorum_active_signers: 24, + quorum_active_signers: NonZeroU16::new(24).unwrap(), quorum_rotation: false, }, instant_lock: InstantLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 10, quorum_window: NonZeroU32::new(24).unwrap(), - quorum_active_signers: 24, + quorum_active_signers: NonZeroU16::new(24).unwrap(), quorum_rotation: false, }, execution: ExecutionConfig { @@ -668,14 +668,14 @@ mod tests { quorum_type: QuorumType::Llmq100_67, quorum_size: 3, quorum_window: NonZeroU32::new(24).unwrap(), - quorum_active_signers: 24, + quorum_active_signers: NonZeroU16::new(24).unwrap(), quorum_rotation: false, }, instant_lock: InstantLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 3, quorum_window: NonZeroU32::new(24).unwrap(), - quorum_active_signers: 24, + quorum_active_signers: NonZeroU16::new(24).unwrap(), quorum_rotation: false, }, execution: ExecutionConfig { @@ -865,14 +865,14 @@ mod tests { quorum_type: QuorumType::Llmq100_67, quorum_size: 3, quorum_window: NonZeroU32::new(24).unwrap(), - quorum_active_signers: 24, + quorum_active_signers: NonZeroU16::new(24).unwrap(), quorum_rotation: false, }, instant_lock: InstantLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 3, quorum_window: NonZeroU32::new(24).unwrap(), - quorum_active_signers: 24, + quorum_active_signers: NonZeroU16::new(24).unwrap(), quorum_rotation: false, }, execution: ExecutionConfig { @@ -1041,14 +1041,14 @@ mod tests { quorum_type: QuorumType::Llmq100_67, quorum_size: 3, quorum_window: NonZeroU32::new(24).unwrap(), - quorum_active_signers: 24, + quorum_active_signers: NonZeroU16::new(24).unwrap(), quorum_rotation: false, }, instant_lock: InstantLockConfig { quorum_type: QuorumType::Llmq100_67, quorum_size: 3, quorum_window: NonZeroU32::new(24).unwrap(), - quorum_active_signers: 24, + quorum_active_signers: NonZeroU16::new(24).unwrap(), quorum_rotation: false, }, execution: ExecutionConfig { From f3fe4ce3e3e8848b539a9f778e3e8439ca9f7374 Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Wed, 28 Oct 2026 14:15:40 +0700 Subject: [PATCH 4/4] refactor(drive-abci): parse quorum settings straight into non-zero integers Review follow-ups to the non-zero quorum window and active signers work. - deserialize_non_zero parses the setting directly into NonZeroU32 or NonZeroU16 through from_str_or_number, which refuses 0, and names the variable on every parse error, not only on 0. The message is now " must be a number greater than 0: ", for example "CHAIN_LOCK_QUORUM_WINDOW must be a number greater than 0: number would be zero for non-zero type". The intermediate integer type parameter and the turbofish at each call are gone. - Comments in verify_chain_lock_locally v0, choose_rotating_quorum and QuorumConfigForSavingV0 now describe what the code computes, not how it used to compute it. v0 still names why its arithmetic is inert for the protocol versions that select it. - The rotating quorum signer test starts with "should". No behaviour change besides the config error text, which should_reject_zero_quorum_windows_and_active_signers_when_loading_config checks for all six settings. (cherry picked from commit 61041b0828035c2d75df0bcf1a9ae73a64668058) Co-Authored-By: Claude Opus 5.5 --- packages/rs-drive-abci/src/config.rs | 36 +++++++++---------- .../verify_chain_lock_locally/v0/mod.rs | 7 ++-- .../v0/quorum_config_for_saving_v0.rs | 6 ++-- .../v0/quorums.rs | 7 ++-- 4 files changed, 27 insertions(+), 29 deletions(-) diff --git a/packages/rs-drive-abci/src/config.rs b/packages/rs-drive-abci/src/config.rs index 2afe7d808d4..fa7740957c6 100644 --- a/packages/rs-drive-abci/src/config.rs +++ b/packages/rs-drive-abci/src/config.rs @@ -612,28 +612,26 @@ where Ok(quorum_type) } -/// Reads a quorum setting that has to be greater than 0, refusing 0 with an error naming -/// `setting`, the variable it was read from. Chain lock verification divides a core height by the -/// quorum window to find its DKG interval, and choosing a rotated quorum takes the base-2 -/// logarithm of the active signers, so neither can be 0 wherever it is held. -fn deserialize_non_zero<'de, D, T, N>(deserializer: D, setting: &str) -> Result +/// Reads a quorum setting that has to be a number greater than 0, refusing anything else with an +/// error naming `setting`, the variable it was read from. Chain lock verification divides a core +/// height by the quorum window to find its DKG interval, and choosing a rotated quorum takes the +/// base-2 logarithm of the active signers, so neither can be 0 wherever it is held. +fn deserialize_non_zero<'de, D, N>(deserializer: D, setting: &str) -> Result where D: serde::Deserializer<'de>, - T: Deserialize<'de> + FromStr, - ::Err: Display, - N: TryFrom, + N: Deserialize<'de> + FromStr, + ::Err: Display, { - let value: T = from_str_or_number(deserializer)?; - - N::try_from(value) - .map_err(|_| serde::de::Error::custom(format!("{setting} must be greater than 0"))) + from_str_or_number(deserializer).map_err(|e| { + serde::de::Error::custom(format!("{setting} must be a number greater than 0: {e}")) + }) } fn deserialize_validator_set_quorum_window<'de, D>(deserializer: D) -> Result where D: serde::Deserializer<'de>, { - deserialize_non_zero::<_, u32, _>(deserializer, "VALIDATOR_SET_QUORUM_WINDOW") + deserialize_non_zero(deserializer, "VALIDATOR_SET_QUORUM_WINDOW") } fn deserialize_validator_set_quorum_active_signers<'de, D>( @@ -642,14 +640,14 @@ fn deserialize_validator_set_quorum_active_signers<'de, D>( where D: serde::Deserializer<'de>, { - deserialize_non_zero::<_, u16, _>(deserializer, "VALIDATOR_SET_QUORUM_ACTIVE_SIGNERS") + deserialize_non_zero(deserializer, "VALIDATOR_SET_QUORUM_ACTIVE_SIGNERS") } fn deserialize_chain_lock_quorum_window<'de, D>(deserializer: D) -> Result where D: serde::Deserializer<'de>, { - deserialize_non_zero::<_, u32, _>(deserializer, "CHAIN_LOCK_QUORUM_WINDOW") + deserialize_non_zero(deserializer, "CHAIN_LOCK_QUORUM_WINDOW") } fn deserialize_chain_lock_quorum_active_signers<'de, D>( @@ -658,14 +656,14 @@ fn deserialize_chain_lock_quorum_active_signers<'de, D>( where D: serde::Deserializer<'de>, { - deserialize_non_zero::<_, u16, _>(deserializer, "CHAIN_LOCK_QUORUM_ACTIVE_SIGNERS") + deserialize_non_zero(deserializer, "CHAIN_LOCK_QUORUM_ACTIVE_SIGNERS") } fn deserialize_instant_lock_quorum_window<'de, D>(deserializer: D) -> Result where D: serde::Deserializer<'de>, { - deserialize_non_zero::<_, u32, _>(deserializer, "INSTANT_LOCK_QUORUM_WINDOW") + deserialize_non_zero(deserializer, "INSTANT_LOCK_QUORUM_WINDOW") } fn deserialize_instant_lock_quorum_active_signers<'de, D>( @@ -674,7 +672,7 @@ fn deserialize_instant_lock_quorum_active_signers<'de, D>( where D: serde::Deserializer<'de>, { - deserialize_non_zero::<_, u16, _>(deserializer, "INSTANT_LOCK_QUORUM_ACTIVE_SIGNERS") + deserialize_non_zero(deserializer, "INSTANT_LOCK_QUORUM_ACTIVE_SIGNERS") } impl ExecutionConfig { @@ -1229,7 +1227,7 @@ mod tests { .expect_err("a zero quorum setting should fail"); let msg = err.to_string(); assert!( - msg.contains(&format!("{name} must be greater than 0")), + msg.contains(&format!("{name} must be a number greater than 0")), "the error should name {name}, got: {msg}" ); diff --git a/packages/rs-drive-abci/src/execution/platform_events/core_chain_lock/verify_chain_lock_locally/v0/mod.rs b/packages/rs-drive-abci/src/execution/platform_events/core_chain_lock/verify_chain_lock_locally/v0/mod.rs index 30e8bd5cbd8..ef44bc8cac3 100644 --- a/packages/rs-drive-abci/src/execution/platform_events/core_chain_lock/verify_chain_lock_locally/v0/mod.rs +++ b/packages/rs-drive-abci/src/execution/platform_events/core_chain_lock/verify_chain_lock_locally/v0/mod.rs @@ -67,9 +67,10 @@ where let window_width = quorum_config.window; // The last block in the window where the quorums would be the same. The window is a - // `NonZeroU32`, so the remainder cannot divide by zero. This is what release builds have - // always computed for every window: their arithmetic wraps, so a window ending past the - // largest core height wraps here too instead of overflowing in builds that check it. + // `NonZeroU32`, so the remainder cannot divide by zero, and a window ending past the + // largest core height wraps instead of overflowing where overflow is checked. For every + // window this equals the wrapping `u32` arithmetic of release builds, which the protocol + // versions selecting this method validate their chains with. let last_block_in_window = (platform_state.last_committed_core_height() - platform_state.last_committed_core_height() % window_width) .wrapping_add(window_width.get() - 1); diff --git a/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorum_config_for_saving_v0.rs b/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorum_config_for_saving_v0.rs index e4bce87884e..85780a761ed 100644 --- a/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorum_config_for_saving_v0.rs +++ b/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorum_config_for_saving_v0.rs @@ -4,9 +4,9 @@ use dpp::dashcore_rpc::dashcore_rpc_json::QuorumType; use dpp::platform_serialization::de::Decode; use std::num::{NonZeroU16, NonZeroU32}; -/// The active signers and the window are encoded exactly as the `u16` and `u32` they hold, so -/// states saved while these fields were plain integers still decode; a saved 0 in either fails to -/// decode instead of reaching quorum selection or chain lock verification. +/// bincode encodes the active signers and the window exactly as the `u16` and `u32` they hold, +/// the bytes of plain integer fields, and fails to decode a saved 0 in either, so a 0 cannot reach +/// quorum selection or chain lock verification. #[derive(Debug, Clone, Encode, Decode)] pub struct QuorumConfigForSavingV0 { quorum_type: QuorumType, diff --git a/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorums.rs b/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorums.rs index 6629a885104..94b9d5f5083 100644 --- a/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorums.rs +++ b/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorums.rs @@ -100,9 +100,8 @@ impl Quorums { quorum_config: &QuorumConfig, request_id: &[u8; 32], ) -> Option<(QuorumHash, &Q)> { - // binary (base-2) logarithm from active_signers: `31 - leading_zeros()` of the count as - // a u32, the same at every protocol version. At least one signer is active, so it cannot - // underflow. + // binary (base-2) logarithm of active_signers, rounded down; at least one signer is + // active, so it is always defined let n = quorum_config.active_signers.ilog2(); // Extract last 64 bits of request_id @@ -459,7 +458,7 @@ mod tests { } #[test] - fn choose_rotating_quorum_takes_signer_from_log2_of_active_signers_bits() { + fn should_take_rotating_quorum_signer_from_log2_of_active_signers_bits() { // The signer is the `log2(active_signers)` bits below the top bit of the request id's // last 8 bytes. All of them are set here, so the signer is the largest index those bits // can hold, from a single active signer (no bits) to u16::MAX of them (15 bits).