From ac7160208689aa82e8766dcda1f60fa89fe99906 Mon Sep 17 00:00:00 2001
From: Ruzzgar <ruzzgarcn@gmail.com>
Date: Sun, 13 Sep 2026 04:06:56 +0300
Subject: [PATCH] fix(types): return None from Height::decrement_by on
 underflow

The malachitebft Height trait documents decrement_by as returning None when
the height would go below its minimum. Arc's impl wraps saturating_sub in
Some, so it never returns None: decrementing below zero yields Some(Height(0)).

This disagrees with the inherent decrement() (checked_sub) and increment_by
(checked_add), and since the trait's default decrement() delegates to
decrement_by, the trait and inherent decrement() diverge at the zero boundary.

Use checked_sub. An existing test asserted the saturating result (Some(0) for
an underflow); it now asserts None.
---
 crates/types/src/height.rs | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/crates/types/src/height.rs b/crates/types/src/height.rs
index 104ff949..bf9b7c99 100644
--- a/crates/types/src/height.rs
+++ b/crates/types/src/height.rs
@@ -77,7 +77,7 @@ impl malachitebft_core_types::Height for Height {
     }
 
     fn decrement_by(&self, n: u64) -> Option<Self> {
-        Some(Self(self.0.saturating_sub(n)))
+        self.0.checked_sub(n).map(Self)
     }
 
     fn as_u64(&self) -> u64 {
@@ -219,9 +219,10 @@ mod tests {
         let decremented = height.decrement_by(10);
         assert_eq!(decremented.unwrap().as_u64(), 40);
 
-        // Test decrement_by with overflow
-        let decremented_overflow = height.decrement_by(100);
-        assert_eq!(decremented_overflow.unwrap().as_u64(), 0);
+        // decrement_by returns None when the result would go below the
+        // minimum, per the Height trait contract, rather than saturating to
+        // Some(0).
+        assert_eq!(height.decrement_by(100), None);
     }
 
     #[test]
@@ -257,4 +258,22 @@ mod tests {
         let decremented = max_height.decrement().unwrap();
         assert_eq!(decremented.as_u64(), u64::MAX - 1);
     }
+
+    #[test]
+    fn test_decrement_by_returns_none_on_underflow() {
+        // The Height trait documents decrement_by as returning None when the
+        // result would go below the minimum, and its default decrement()
+        // delegates here — so it must agree with the inherent decrement().
+        use malachitebft_core_types::Height as _;
+
+        assert_eq!(Height::new(5).decrement_by(5).map(|h| h.as_u64()), Some(0));
+        assert_eq!(Height::new(5).decrement_by(6), None);
+        assert_eq!(Height::ZERO.decrement_by(1), None);
+
+        // Trait decrement() (via decrement_by) must match inherent decrement().
+        assert_eq!(
+            <Height as malachitebft_core_types::Height>::decrement(&Height::ZERO),
+            Height::ZERO.decrement(),
+        );
+    }
 }