Height type has safety bugs in arithmetic operations

[nachog00]

Problem

Zaino's Height type has safety bugs in its arithmetic operations that can cause:

• Underflow wrapping to u32::MAX in release mode
• Panics in debug mode
• Silent failures in production code (e.g., reorg detection at genesis)

Real-World Bug

Location: non_finalised_state.rs:390

fn handle_reorg(...) -> Result<BestTip, SyncError> {
    let mut next_height_down = current_tip.height - 1;  // UNSAFE!

    let prev_hash = loop {
        if next_height_down == Height(0) {  // Check AFTER subtraction
            return Err(SyncError::ReorgFailure("attempted to reorg below chain genesis"));
        }
        // ...
        None => next_height_down = next_height_down - 1,  // ANOTHER unsafe subtraction!
    };
}

Problem: If current_tip.height is Height(0):

• Release mode: Wraps to Height(4,294,967,295), check never triggers → silent failure
• Debug mode: Panics with "attempt to subtract with overflow"
• Expected: Should return ReorgError::AtGenesis on first subtraction

Root Cause: Reimplementing Zebra's Battle-Tested Types

Zaino has its own Height type with faulty arithmetic implementations, when Zebra already provides safe, well-tested implementations we could leverage:

// Zaino (unsafe)
impl std::ops::Sub<u32> for Height {
    type Output = Self;
    fn sub(self, rhs: u32) -> Self::Output {
        Height(self.0 - rhs)  // Can underflow! No error handling
    }
}

// Zebra (safe)
impl Sub<HeightDiff> for Height {
    type Output = Option<Height>;  // Returns Option, not Height

    fn sub(self, rhs: HeightDiff) -> Option<Height> {
        let lhs = i64::from(self.0);
        let res = lhs - rhs;
        let res = u32::try_from(res).ok()?;  // Returns None on underflow
        Height::try_from(res).ok()
    }
}

// Usage (forced by type system)
let prev = height.checked_sub(1)
    .ok_or(ReorgError::AtGenesis)?;  // Compiler forces error handling

Zebra also provides helper methods:

• height.next() - Returns Result<Height, HeightError>
• height.previous() - Returns Result<Height, HeightError>
• height.is_min() - Check if at genesis

The problem: We're forced to maintain our own Height type because it's tightly coupled to our DB serialization layer via ZainoVersionedSerialise, preventing us from using Zebra's type directly.

Proposed Solutions

Three architectural options for fixing:

Option 1: Fix Our Arithmetic to Match Zebra's Safety

Add checked arithmetic to Zaino's Height:

impl std::ops::Sub<u32> for Height {
    type Output = Option<Self>;
    fn sub(self, rhs: u32) -> Option<Self> {
        self.0.checked_sub(rhs).map(Height)
    }
}

Pros: Keep our type, add safety
Cons: Reimplementing Zebra, need to update ~50+ call sites, ongoing maintenance burden

Option 2: Use Zebra's Height Directly

pub use zebra_chain::block::Height;

impl ZainoVersionedSerialise for zebra_chain::block::Height {
    // Only add DB serialization to external type
}

Pros: Inherit battle-tested safety, less code, no reimplementation
Cons: Trait implementation on external type (but standard Rust practice). May break strict DB versioned serialisation logic.

Option 3: Separate DB and Domain Types

// DB layer - stays in db/primitives/
mod db {
    struct HeightDb(u32);  // Simple, efficient
    impl ZainoVersionedSerialise for HeightDb { /* ... */ }
}

// Domain layer - use Zebra's safe type
pub use zebra_chain::block::Height;

// Repository layer - convert at boundaries
impl From<Height> for HeightDb { /* ... */ }
impl TryFrom<HeightDb> for Height { /* ... */ }  // Validates on read!

Pros: Clean separation of concerns, leverage Zebra's safety
Cons: Other DB types have Height fields that would need careful handling

Related

• PR height unit tests #605 adds unit tests proving these bugs exist