From 67abfc0c6355285e7c8335377e537768c09311ff Mon Sep 17 00:00:00 2001 From: nachog00 Date: Fri, 17 Oct 2025 01:20:47 -0300 Subject: [PATCH 1/3] Add unit tests proving Height type safety issues Adds 5 unit tests that prove critical safety bugs in Zaino's Height type: 1. test_underflow_wraps_to_max_in_release - Proves Height(0) - 1 wraps to Height(4,294,967,295) in release mode. Documents real-world impact on reorg detection in non_finalised_state.rs:390. 2. test_underflow_panics_in_debug - Proves same operation panics in debug mode with 'attempt to subtract with overflow'. 3. test_overflow_wraps_in_release - Proves Height(u32::MAX - 10) + 20 wraps to Height(9) instead of detecting overflow. 4. test_overflow_panics_in_debug - Proves addition overflow panics in debug mode. 5. test_arithmetic_bypasses_validation - Proves arithmetic operations can create Heights > MAX (2^31 - 1), bypassing TryFrom validation and breaking type invariants. These tests demonstrate that business logic must manually check for edge cases that should be enforced by the type system. Zebra's Height type provides safe arithmetic (returns Option) that forces handling at compile time, eliminating this class of bugs. All tests pass, proving these are real bugs in production code. --- .../src/chain_index/types/db/legacy.rs | 90 +++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/zaino-state/src/chain_index/types/db/legacy.rs b/zaino-state/src/chain_index/types/db/legacy.rs index 3ee7627ee..125448a86 100644 --- a/zaino-state/src/chain_index/types/db/legacy.rs +++ b/zaino-state/src/chain_index/types/db/legacy.rs @@ -2955,3 +2955,93 @@ pub mod serde_arrays { .map_err(|_| serde::de::Error::custom(format!("invalid length for [u8; {N}]"))) } } + +#[cfg(test)] +mod height_safety_tests { + use super::*; + + /// This test demonstrates the CRITICAL underflow bug in Height subtraction. + /// When subtracting from genesis (Height(0)), the result wraps to u32::MAX + /// in release mode, or panics in debug mode. + /// + /// Real-world impact: This breaks reorg detection at genesis in non_finalised_state.rs:394 + /// where the code does `let mut next_height_down = best_tip.height - 1;` without checking + /// if height is 0. The wrapped value causes silent failure instead of ReorgError::AtGenesis. + #[test] + #[cfg(not(debug_assertions))] // Only runs in release mode + fn test_underflow_wraps_to_max_in_release() { + let genesis = GENESIS_HEIGHT; // Height(0) + + // In release mode, this wraps to u32::MAX instead of detecting the error + let result = genesis - 1; + + // This assertion proves the bug: subtracting from 0 gives 4,294,967,295! + assert_eq!(result.0, u32::MAX); + assert_eq!(result.0, 4_294_967_295); + } + + /// This test demonstrates the underflow bug panics in debug mode. + /// Uncommenting this test in debug builds will cause a panic. + #[test] + #[should_panic] + #[cfg(debug_assertions)] // Only runs in debug mode + fn test_underflow_panics_in_debug() { + let genesis = GENESIS_HEIGHT; // Height(0) + + // In debug mode, this panics with "attempt to subtract with overflow" + let _result = genesis - 1; + } + + /// This test demonstrates the overflow bug in Height addition. + /// When adding to a height near u32::MAX, the result wraps around + /// in release mode, or panics in debug mode. + #[test] + #[cfg(not(debug_assertions))] // Only runs in release mode + fn test_overflow_wraps_in_release() { + let high_height = Height(u32::MAX - 10); + + // Adding 20 to (u32::MAX - 10) should overflow + // In release mode, this wraps around + let result = high_height + 20; + + // Proves wraparound: (u32::MAX - 10) + 20 = 9 + assert_eq!(result.0, 9); + + // This could happen if: + // 1. Corrupted database stores invalid height + // 2. Arithmetic on heights near the limit + // 3. Could cause incorrect block selection + } + + /// This test demonstrates the overflow bug panics in debug mode. + #[test] + #[should_panic] + #[cfg(debug_assertions)] // Only runs in debug mode + fn test_overflow_panics_in_debug() { + let high_height = Height(u32::MAX - 10); + + // In debug mode, this panics with "attempt to add with overflow" + let _result = high_height + 20; + } + + /// This test demonstrates field access allows bypassing validation. + /// Height can only be constructed via TryFrom (validated), but arithmetic + /// can produce invalid values. + #[test] + #[cfg(not(debug_assertions))] + fn test_arithmetic_bypasses_validation() { + // Construction is validated + let result = Height::try_from(u32::MAX); + assert!(result.is_err()); + assert_eq!(result.unwrap_err(), "height must be ≤ 2^31 - 1"); + + // But arithmetic can create invalid heights! + let valid_height = Height(zebra_chain::block::Height::MAX.0); // 2^31 - 1 + let invalid_height = valid_height + 1; // Now we have 2^31 (invalid!) + + // This height is > MAX but exists as a Height type + assert!(invalid_height.0 > zebra_chain::block::Height::MAX.0); + + // Type system can't prevent this - the invariant is broken + } +} From c31c3242e99dc419e1a0201f06f25fc8fb3ec71b Mon Sep 17 00:00:00 2001 From: nachog00 Date: Fri, 17 Oct 2025 01:26:08 -0300 Subject: [PATCH 2/3] Extract Height type to primitives/height.rs with safety tests Moves Height type and implementations from legacy.rs to dedicated module: - Creates types/db/primitives/height.rs - Adds 5 unit tests proving critical safety bugs in current implementation - Updates primitives.rs to export Height and GENESIS_HEIGHT Tests prove: 1. Underflow wraps to u32::MAX in release (Height(0) - 1 = Height(4294967295)) 2. Underflow panics in debug mode 3. Overflow wraps in release (Height(u32::MAX-10) + 20 = Height(9)) 4. Overflow panics in debug mode 5. Arithmetic bypasses TryFrom validation (can create Heights > MAX) Real-world impact documented: non_finalised_state.rs:390 reorg detection bug where unchecked subtraction causes silent failure instead of error. This extraction prepares for future migration to safe arithmetic that returns Option, matching Zebra's Height implementation. --- .../src/chain_index/types/db/legacy.rs | 204 +---------------- .../src/chain_index/types/db/primitives.rs | 4 + .../chain_index/types/db/primitives/height.rs | 210 ++++++++++++++++++ 3 files changed, 216 insertions(+), 202 deletions(-) create mode 100644 zaino-state/src/chain_index/types/db/primitives/height.rs diff --git a/zaino-state/src/chain_index/types/db/legacy.rs b/zaino-state/src/chain_index/types/db/legacy.rs index 125448a86..7942d50d8 100644 --- a/zaino-state/src/chain_index/types/db/legacy.rs +++ b/zaino-state/src/chain_index/types/db/legacy.rs @@ -282,118 +282,8 @@ impl FixedEncodedLen for TransactionHash { const ENCODED_LEN: usize = 32; } -/// Block height. -/// -/// NOTE: Encoded as 4-byte big-endian byte-string to ensure height ordering -/// for keys in Lexicographically sorted B-Tree. -#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)] -#[cfg_attr(test, derive(serde::Serialize, serde::Deserialize))] -pub struct Height(pub(crate) u32); - -/// The first block -pub const GENESIS_HEIGHT: Height = Height(0); - -impl TryFrom for Height { - type Error = &'static str; - - fn try_from(height: u32) -> Result { - // Zebra enforces Height <= 2^31 - 1 - if height <= zebra_chain::block::Height::MAX.0 { - Ok(Self(height)) - } else { - Err("height must be ≤ 2^31 - 1") - } - } -} - -impl From for u32 { - fn from(h: Height) -> Self { - h.0 - } -} - -impl std::ops::Add for Height { - type Output = Self; - - fn add(self, rhs: u32) -> Self::Output { - Height(self.0 + rhs) - } -} - -impl std::ops::Sub for Height { - type Output = Self; - - fn sub(self, rhs: u32) -> Self::Output { - Height(self.0 - rhs) - } -} - -impl std::fmt::Display for Height { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "{}", self.0) - } -} - -impl std::str::FromStr for Height { - type Err = &'static str; - - fn from_str(s: &str) -> Result { - let h = s.parse::().map_err(|_| "invalid u32")?; - Self::try_from(h) - } -} - -impl From for zebra_chain::block::Height { - fn from(h: Height) -> Self { - zebra_chain::block::Height(h.0) - } -} - -impl TryFrom for Height { - type Error = &'static str; - - fn try_from(h: zebra_chain::block::Height) -> Result { - Height::try_from(h.0) - } -} - -impl From for zcash_protocol::consensus::BlockHeight { - fn from(h: Height) -> Self { - zcash_protocol::consensus::BlockHeight::from(h.0) - } -} - -impl TryFrom for Height { - type Error = &'static str; - - fn try_from(h: zcash_protocol::consensus::BlockHeight) -> Result { - Height::try_from(u32::from(h)) - } -} - -impl ZainoVersionedSerialise for Height { - const VERSION: u8 = version::V1; - - fn encode_body(&self, w: &mut W) -> io::Result<()> { - // Height must sort lexicographically - write **big-endian** - write_u32_be(w, self.0) - } - - fn decode_latest(r: &mut R) -> io::Result { - Self::decode_v1(r) - } - - fn decode_v1(r: &mut R) -> io::Result { - let raw = read_u32_be(r)?; - Height::try_from(raw).map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e)) - } -} - -/// Height = 4-byte big-endian body. -impl FixedEncodedLen for Height { - /// 4 bytes, BE - const ENCODED_LEN: usize = 4; -} +// Height type has been moved to primitives/height.rs +pub use super::primitives::{Height, GENESIS_HEIGHT}; /// Numerical index of subtree / shard roots. /// @@ -2955,93 +2845,3 @@ pub mod serde_arrays { .map_err(|_| serde::de::Error::custom(format!("invalid length for [u8; {N}]"))) } } - -#[cfg(test)] -mod height_safety_tests { - use super::*; - - /// This test demonstrates the CRITICAL underflow bug in Height subtraction. - /// When subtracting from genesis (Height(0)), the result wraps to u32::MAX - /// in release mode, or panics in debug mode. - /// - /// Real-world impact: This breaks reorg detection at genesis in non_finalised_state.rs:394 - /// where the code does `let mut next_height_down = best_tip.height - 1;` without checking - /// if height is 0. The wrapped value causes silent failure instead of ReorgError::AtGenesis. - #[test] - #[cfg(not(debug_assertions))] // Only runs in release mode - fn test_underflow_wraps_to_max_in_release() { - let genesis = GENESIS_HEIGHT; // Height(0) - - // In release mode, this wraps to u32::MAX instead of detecting the error - let result = genesis - 1; - - // This assertion proves the bug: subtracting from 0 gives 4,294,967,295! - assert_eq!(result.0, u32::MAX); - assert_eq!(result.0, 4_294_967_295); - } - - /// This test demonstrates the underflow bug panics in debug mode. - /// Uncommenting this test in debug builds will cause a panic. - #[test] - #[should_panic] - #[cfg(debug_assertions)] // Only runs in debug mode - fn test_underflow_panics_in_debug() { - let genesis = GENESIS_HEIGHT; // Height(0) - - // In debug mode, this panics with "attempt to subtract with overflow" - let _result = genesis - 1; - } - - /// This test demonstrates the overflow bug in Height addition. - /// When adding to a height near u32::MAX, the result wraps around - /// in release mode, or panics in debug mode. - #[test] - #[cfg(not(debug_assertions))] // Only runs in release mode - fn test_overflow_wraps_in_release() { - let high_height = Height(u32::MAX - 10); - - // Adding 20 to (u32::MAX - 10) should overflow - // In release mode, this wraps around - let result = high_height + 20; - - // Proves wraparound: (u32::MAX - 10) + 20 = 9 - assert_eq!(result.0, 9); - - // This could happen if: - // 1. Corrupted database stores invalid height - // 2. Arithmetic on heights near the limit - // 3. Could cause incorrect block selection - } - - /// This test demonstrates the overflow bug panics in debug mode. - #[test] - #[should_panic] - #[cfg(debug_assertions)] // Only runs in debug mode - fn test_overflow_panics_in_debug() { - let high_height = Height(u32::MAX - 10); - - // In debug mode, this panics with "attempt to add with overflow" - let _result = high_height + 20; - } - - /// This test demonstrates field access allows bypassing validation. - /// Height can only be constructed via TryFrom (validated), but arithmetic - /// can produce invalid values. - #[test] - #[cfg(not(debug_assertions))] - fn test_arithmetic_bypasses_validation() { - // Construction is validated - let result = Height::try_from(u32::MAX); - assert!(result.is_err()); - assert_eq!(result.unwrap_err(), "height must be ≤ 2^31 - 1"); - - // But arithmetic can create invalid heights! - let valid_height = Height(zebra_chain::block::Height::MAX.0); // 2^31 - 1 - let invalid_height = valid_height + 1; // Now we have 2^31 (invalid!) - - // This height is > MAX but exists as a Height type - assert!(invalid_height.0 > zebra_chain::block::Height::MAX.0); - - // Type system can't prevent this - the invariant is broken - } -} diff --git a/zaino-state/src/chain_index/types/db/primitives.rs b/zaino-state/src/chain_index/types/db/primitives.rs index f40bf037a..36d1f9ffb 100644 --- a/zaino-state/src/chain_index/types/db/primitives.rs +++ b/zaino-state/src/chain_index/types/db/primitives.rs @@ -5,3 +5,7 @@ //! - ShardIndex //! - ScriptType //! - ShardRoot + +mod height; + +pub use height::{Height, GENESIS_HEIGHT}; diff --git a/zaino-state/src/chain_index/types/db/primitives/height.rs b/zaino-state/src/chain_index/types/db/primitives/height.rs new file mode 100644 index 000000000..433f37ad9 --- /dev/null +++ b/zaino-state/src/chain_index/types/db/primitives/height.rs @@ -0,0 +1,210 @@ +//! Block height type and utilities. + +use core2::io::{self, Read, Write}; + +use crate::chain_index::encoding::{ + read_u32_be, version, write_u32_be, FixedEncodedLen, ZainoVersionedSerialise, +}; + +/// Block height. +/// +/// NOTE: Encoded as 4-byte big-endian byte-string to ensure height ordering +/// for keys in Lexicographically sorted B-Tree. +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)] +#[cfg_attr(test, derive(serde::Serialize, serde::Deserialize))] +pub struct Height(pub(crate) u32); + +/// The first block +pub const GENESIS_HEIGHT: Height = Height(0); + +impl TryFrom for Height { + type Error = &'static str; + + fn try_from(height: u32) -> Result { + // Zebra enforces Height <= 2^31 - 1 + if height <= zebra_chain::block::Height::MAX.0 { + Ok(Self(height)) + } else { + Err("height must be ≤ 2^31 - 1") + } + } +} + +impl From for u32 { + fn from(h: Height) -> Self { + h.0 + } +} + +impl std::ops::Add for Height { + type Output = Self; + + fn add(self, rhs: u32) -> Self::Output { + Height(self.0 + rhs) + } +} + +impl std::ops::Sub for Height { + type Output = Self; + + fn sub(self, rhs: u32) -> Self::Output { + Height(self.0 - rhs) + } +} + +impl std::fmt::Display for Height { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.0) + } +} + +impl std::str::FromStr for Height { + type Err = &'static str; + + fn from_str(s: &str) -> Result { + let h = s.parse::().map_err(|_| "invalid u32")?; + Self::try_from(h) + } +} + +impl From for zebra_chain::block::Height { + fn from(h: Height) -> Self { + zebra_chain::block::Height(h.0) + } +} + +impl TryFrom for Height { + type Error = &'static str; + + fn try_from(h: zebra_chain::block::Height) -> Result { + Height::try_from(h.0) + } +} + +impl From for zcash_protocol::consensus::BlockHeight { + fn from(h: Height) -> Self { + zcash_protocol::consensus::BlockHeight::from(h.0) + } +} + +impl TryFrom for Height { + type Error = &'static str; + + fn try_from(h: zcash_protocol::consensus::BlockHeight) -> Result { + Height::try_from(u32::from(h)) + } +} + +impl ZainoVersionedSerialise for Height { + const VERSION: u8 = version::V1; + + fn encode_body(&self, w: &mut W) -> io::Result<()> { + // Height must sort lexicographically - write **big-endian** + write_u32_be(w, self.0) + } + + fn decode_latest(r: &mut R) -> io::Result { + Self::decode_v1(r) + } + + fn decode_v1(r: &mut R) -> io::Result { + let raw = read_u32_be(r)?; + Height::try_from(raw).map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e)) + } +} + +/// Height = 4-byte big-endian body. +impl FixedEncodedLen for Height { + /// 4 bytes, BE + const ENCODED_LEN: usize = 4; +} + +#[cfg(test)] +mod height_safety_tests { + use super::*; + + /// This test demonstrates the CRITICAL underflow bug in Height subtraction. + /// When subtracting from genesis (Height(0)), the result wraps to u32::MAX + /// in release mode, or panics in debug mode. + /// + /// Real-world impact: This breaks reorg detection at genesis in non_finalised_state.rs:390 + /// where the code does `let mut next_height_down = best_tip.height - 1;` without checking + /// if height is 0. The wrapped value causes silent failure instead of ReorgError::AtGenesis. + #[test] + #[cfg(not(debug_assertions))] // Only runs in release mode + fn test_underflow_wraps_to_max_in_release() { + let genesis = GENESIS_HEIGHT; // Height(0) + + // In release mode, this wraps to u32::MAX instead of detecting the error + let result = genesis - 1; + + // This assertion proves the bug: subtracting from 0 gives 4,294,967,295! + assert_eq!(result.0, u32::MAX); + assert_eq!(result.0, 4_294_967_295); + } + + /// This test demonstrates the underflow bug panics in debug mode. + /// Uncommenting this test in debug builds will cause a panic. + #[test] + #[should_panic] + #[cfg(debug_assertions)] // Only runs in debug mode + fn test_underflow_panics_in_debug() { + let genesis = GENESIS_HEIGHT; // Height(0) + + // In debug mode, this panics with "attempt to subtract with overflow" + let _result = genesis - 1; + } + + /// This test demonstrates the overflow bug in Height addition. + /// When adding to a height near u32::MAX, the result wraps around + /// in release mode, or panics in debug mode. + #[test] + #[cfg(not(debug_assertions))] // Only runs in release mode + fn test_overflow_wraps_in_release() { + let high_height = Height(u32::MAX - 10); + + // Adding 20 to (u32::MAX - 10) should overflow + // In release mode, this wraps around + let result = high_height + 20; + + // Proves wraparound: (u32::MAX - 10) + 20 = 9 + assert_eq!(result.0, 9); + + // This could happen if: + // 1. Corrupted database stores invalid height + // 2. Arithmetic on heights near the limit + // 3. Could cause incorrect block selection + } + + /// This test demonstrates the overflow bug panics in debug mode. + #[test] + #[should_panic] + #[cfg(debug_assertions)] // Only runs in debug mode + fn test_overflow_panics_in_debug() { + let high_height = Height(u32::MAX - 10); + + // In debug mode, this panics with "attempt to add with overflow" + let _result = high_height + 20; + } + + /// This test demonstrates field access allows bypassing validation. + /// Height can only be constructed via TryFrom (validated), but arithmetic + /// can produce invalid values. + #[test] + #[cfg(not(debug_assertions))] + fn test_arithmetic_bypasses_validation() { + // Construction is validated + let result = Height::try_from(u32::MAX); + assert!(result.is_err()); + assert_eq!(result.unwrap_err(), "height must be ≤ 2^31 - 1"); + + // But arithmetic can create invalid heights! + let valid_height = Height(zebra_chain::block::Height::MAX.0); // 2^31 - 1 + let invalid_height = valid_height + 1; // Now we have 2^31 (invalid!) + + // This height is > MAX but exists as a Height type + assert!(invalid_height.0 > zebra_chain::block::Height::MAX.0); + + // Type system can't prevent this - the invariant is broken + } +} From fdcdcc771f88d6abaeeb81143fc25adec0dc7d0c Mon Sep 17 00:00:00 2001 From: nachog00 Date: Fri, 17 Oct 2025 12:58:02 -0300 Subject: [PATCH 3/3] Remove ambiguous CRITICAL from tests description --- zaino-state/src/chain_index/types/db/primitives/height.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zaino-state/src/chain_index/types/db/primitives/height.rs b/zaino-state/src/chain_index/types/db/primitives/height.rs index 433f37ad9..12d483fa8 100644 --- a/zaino-state/src/chain_index/types/db/primitives/height.rs +++ b/zaino-state/src/chain_index/types/db/primitives/height.rs @@ -123,7 +123,7 @@ impl FixedEncodedLen for Height { mod height_safety_tests { use super::*; - /// This test demonstrates the CRITICAL underflow bug in Height subtraction. + /// This test demonstrates the underflow bug in Height subtraction. /// When subtracting from genesis (Height(0)), the result wraps to u32::MAX /// in release mode, or panics in debug mode. ///