sdk/monotree: Fix bug where root hash depended on insertion order for small numbers of key-value pairs

src/sdk/src/monotree/bits.rs

@@ -17,16 +17,16 @@
  * along with this program.  If not, see <https://www.gnu.org/licenses/>.
  */
 
-use std::{cmp::Ordering, ops::Range};
+use std::ops::Range;
 
 use super::{
-    utils::{bit, bytes_to_int, len_lcp, offsets},
+    utils::{bit, bytes_to_int, len_lcp, nbytes_across},
     BitsLen,
 };
 use crate::GenericResult;
 
+#[derive(Debug, Clone, PartialEq)]
 /// `BitVec` implementation based on bytes slice.
-#[derive(Debug, Clone)]
 pub struct Bits<'a> {
     pub path: &'a [u8],
     pub range: Range<BitsLen>,
@@ -34,7 +34,7 @@ pub struct Bits<'a> {
 
 impl<'a> Bits<'a> {
     pub fn new(bytes: &'a [u8]) -> Self {
-        Self { path: bytes, range: 0..(bytes.len() as BitsLen * 8) }
+        Bits { path: bytes, range: 0..(bytes.len() as BitsLen * 8) }
     }
 
     /// Construct `Bits` instance by deserializing bytes slice.
@@ -47,7 +47,21 @@ impl<'a> Bits<'a> {
 
     /// Serialize `Bits` into bytes.
     pub fn to_bytes(&self) -> GenericResult<Vec<u8>> {
-        Ok([&self.range.start.to_be_bytes(), &self.range.end.to_be_bytes(), self.path].concat())
+        let start = (self.range.start / 8) as usize;
+        let end = self.range.end.div_ceil(8) as usize;
+        let mut path = self.path[start..end].to_owned();
+        let r = (self.range.start % 8) as u8;
+        if r != 0 {
+            let mask = 0xffu8 >> r;
+            path[0] &= mask;
+        }
+        let r = (self.range.end % 8) as u8;
+        if r != 0 {
+            let mask = 0xffu8 << (8 - r);
+            let last = path.len() - 1;
+            path[last] &= mask;
+        }
+        Ok([&self.range.start.to_be_bytes(), &self.range.end.to_be_bytes(), &path[..]].concat())
     }
 
     /// Get the very first bit.
@@ -63,63 +77,24 @@ impl<'a> Bits<'a> {
         self.len() == 0 || self.path.len() == 0
     }
 
-    /// Get the resulting `Bits` when shifted with the given size.
-    pub fn shift(&self, n: BitsLen, tail: bool) -> Self {
-        let (q, range) = offsets(&self.range, n, tail);
-        if tail {
-            Self { path: &self.path[..q as usize], range }
-        } else {
-            Self { path: &self.path[q as usize..], range }
-        }
+    /// Get the first `n` bits.
+    pub fn take(&self, n: BitsLen) -> Self {
+        let x = self.range.start + n;
+        let q = nbytes_across(self.range.start, x);
+        let range = self.range.start..x;
+        Self { path: &self.path[..q as usize], range }
+    }
+
+    /// Skip the first `n` bits.
+    pub fn drop(&self, n: BitsLen) -> Self {
+        let x = self.range.start + n;
+        let q = x / 8;
+        let range = x % 8..self.range.end - 8 * (x / 8);
+        Self { path: &self.path[q as usize..], range }
     }
 
     /// Get length of the longest common prefix bits for the given two `Bits`.
     pub fn len_common_bits(a: &Self, b: &Self) -> BitsLen {
         len_lcp(a.path, &a.range, b.path, &b.range)
     }
-
-    /// Get the bit at position `i` within this Bits range
-    pub fn bit(&self, i: BitsLen) -> bool {
-        assert!(i < self.len(), "Bit index out of range");
-        bit(self.path, self.range.start + i)
-    }
-
-    /// Compare bits lexicographically (MSB to LSB)
-    pub fn lexical_cmp(&self, other: &Self) -> Ordering {
-        let min_len = std::cmp::min(self.len(), other.len());
-
-        // Compare bit by bit from start of range
-        for i in 0..min_len {
-            match (self.bit(i), other.bit(i)) {
-                (false, true) => return Ordering::Less,
-                (true, false) => return Ordering::Greater,
-                _ => continue,
-            }
-        }
-
-        // All compared bits equal, compare lengths
-        self.len().cmp(&other.len())
-    }
-}
-
-// Implement equality/ordering based on actual bit values
-impl PartialEq for Bits<'_> {
-    fn eq(&self, other: &Self) -> bool {
-        self.len() == other.len() && (0..self.len()).all(|i| self.bit(i) == other.bit(i))
-    }
-}
-
-impl Eq for Bits<'_> {}
-
-#[allow(clippy::non_canonical_partial_ord_impl)]
-impl PartialOrd for Bits<'_> {
-    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
-        Some(self.lexical_cmp(other))
-    }
-}
-
-impl Ord for Bits<'_> {
-    fn cmp(&self, other: &Self) -> Ordering {
-        self.lexical_cmp(other)
-    }
 }

src/sdk/src/monotree/node.rs

@@ -59,12 +59,12 @@ pub struct Unit<'a> {
 /// By default `HashLen = 32`, `BitsLen = 2`.
 ///
 /// _SoftNode_ = `Cell` + `0x00`(1), where
-/// `Cell` = `hash`(`HASH_LEN`) + `path`(`< HASH_LEN`) + `range_start`(`BitsLen`) + `range_end`(`BitsLen`).
+/// `Cell` = `hash`(`HASH_LEN`) + `range_start`(`BitsLen`) + `range_end`(`BitsLen`) + `path`(`< HASH_LEN`).
 /// `0x00` is an indicator for soft node.
 ///
 /// _HardNode_ = `Cell_L` + `Cell_R` + `0x01`(1), where
-/// `Cell_L` = `hash_L`(`HASH_LEN`) + `path_L`(`< HASH_LEN`) + `range_L_start`(`BitsLen`) + `range_L_end`(`BitsLen`
-/// `Cell_R` = `path_R`(`< HASH_LEN`) _ `range_R_start`(`BitsLen`) + `range_R_end`(`BitsLen`) + `hash_R`(`HASH_LEN`).
+/// `Cell_L` = `hash_L`(`HASH_LEN`) + `range_L_start`(`BitsLen`) + `range_L_end`(`BitsLen`) + `path_L`(`< HASH_LEN`)
+/// `Cell_R` = `range_R_start`(`BitsLen`) + `range_R_end`(`BitsLen`) + path_R`(`< HASH_LEN`) + `hash_R`(`HASH_LEN`).
 /// `0x01` is an indicator for hard node.
 ///
 /// To make ***Merkle proof*** easier, we purposely placed the _hashes_ on outskirts of the serialized form.

src/sdk/src/monotree/tree.rs

@@ -223,35 +223,31 @@ impl Monotree {
     ///   Immediately split node into two with the longest common prefix,
     ///   then wind the recursive stack from there returning resulting hashes.
     fn put(&mut self, root: &[u8], bits: Bits, leaf: &[u8]) -> GenericResult<Option<Hash>> {
-        let bytes = self.db.get(root)?.expect("bytes");
-        let (lc, rc) = Node::cells_from_bytes(&bytes, bits.first())?;
-        let unit = lc.as_ref().expect("put(): left-unit");
+        let bytes = self.db.get(root)?.expect("put(): bytes");
+        let (left, right) = Node::cells_from_bytes(&bytes, bits.first())?;
+        let unit = left.as_ref().expect("put(): left-unit");
         let n = Bits::len_common_bits(&unit.bits, &bits);
 
         match n {
-            0 => self.put_node(Node::new(lc, Some(Unit { hash: leaf, bits }))),
-            n if n == bits.len() => self.put_node(Node::new(Some(Unit { hash: leaf, bits }), rc)),
+            0 => self.put_node(Node::new(left, Some(Unit { hash: leaf, bits }))),
+            n if n == bits.len() => {
+                self.put_node(Node::new(Some(Unit { hash: leaf, bits }), right))
+            }
             n if n == unit.bits.len() => {
-                let hash = &self.put(unit.hash, bits.shift(n, false), leaf)?.expect("put(): hash");
+                let hash =
+                    &self.put(unit.hash, bits.drop(n), leaf)?.expect("put(): consume & pass-over");
 
-                let unit = unit.to_owned();
-                self.put_node(Node::new(Some(Unit { hash, ..unit }), rc))
+                self.put_node(Node::new(Some(Unit { hash, bits: unit.bits.to_owned() }), right))
             }
             _ => {
-                let bits = bits.shift(n, false);
-                let ru = Unit { hash: leaf, bits };
-
-                let (cloned, unit) = (unit.bits.clone(), unit.to_owned());
-                let (hash, bits) = (unit.hash, unit.bits.shift(n, false));
-                let lu = Unit { hash, bits };
-
-                // ENFORCE DETERMINISTIC ORDERING
-                let (left, right) = if lu.bits < ru.bits { (lu, ru) } else { (ru, lu) };
-
-                let hash =
-                    &self.put_node(Node::new(Some(left), Some(right)))?.expect("put(): hash");
-                let bits = cloned.shift(n, true);
-                self.put_node(Node::new(Some(Unit { hash, bits }), rc))
+                let hash = &self
+                    .put_node(Node::new(
+                        Some(Unit { hash: unit.hash, bits: unit.bits.drop(n) }),
+                        Some(Unit { hash: leaf, bits: bits.drop(n) }),
+                    ))?
+                    .expect("put(): split-node");
+
+                self.put_node(Node::new(Some(Unit { hash, bits: unit.bits.take(n) }), right))
             }
         }
     }
@@ -265,13 +261,13 @@ impl Monotree {
     }
 
     fn find_key(&mut self, root: &[u8], bits: Bits) -> GenericResult<Option<Hash>> {
-        let bytes = self.db.get(root)?.expect("bytes");
+        let bytes = self.db.get(root)?.expect("find_key(): bytes");
         let (cell, _) = Node::cells_from_bytes(&bytes, bits.first())?;
         let unit = cell.as_ref().expect("find_key(): left-unit");
         let n = Bits::len_common_bits(&unit.bits, &bits);
         match n {
             n if n == bits.len() => Ok(Some(slice_to_hash(unit.hash))),
-            n if n == unit.bits.len() => self.find_key(unit.hash, bits.shift(n, false)),
+            n if n == unit.bits.len() => self.find_key(unit.hash, bits.drop(n)),
             _ => Ok(None),
         }
     }
@@ -285,25 +281,25 @@ impl Monotree {
     }
 
     fn delete_key(&mut self, root: &[u8], bits: Bits) -> GenericResult<Option<Hash>> {
-        let bytes = self.db.get(root)?.expect("bytes");
-        let (lc, rc) = Node::cells_from_bytes(&bytes, bits.first())?;
-        let unit = lc.as_ref().expect("delete_key(): left-unit");
+        let bytes = self.db.get(root)?.expect("delete_key(): bytes");
+        let (left, right) = Node::cells_from_bytes(&bytes, bits.first())?;
+        let unit = left.as_ref().expect("delete_key(): left-unit");
         let n = Bits::len_common_bits(&unit.bits, &bits);
 
         match n {
-            n if n == bits.len() => match rc {
-                Some(_) => self.put_node(Node::new(None, rc)),
+            n if n == bits.len() => match right {
+                Some(_) => self.put_node(Node::new(None, right)),
                 None => Ok(None),
             },
             n if n == unit.bits.len() => {
-                let hash = self.delete_key(unit.hash, bits.shift(n, false))?;
-                match (hash, &rc) {
+                let hash = self.delete_key(unit.hash, bits.drop(n))?;
+                match (hash, &right) {
                     (None, None) => Ok(None),
-                    (None, Some(_)) => self.put_node(Node::new(None, rc)),
+                    (None, Some(_)) => self.put_node(Node::new(None, right)),
                     (Some(ref hash), _) => {
                         let unit = unit.to_owned();
-                        let lc = Some(Unit { hash, ..unit });
-                        self.put_node(Node::new(lc, rc))
+                        let left = Some(Unit { hash, ..unit });
+                        self.put_node(Node::new(left, right))
                     }
                 }
             }
@@ -374,7 +370,7 @@ impl Monotree {
         bits: Bits,
         proof: &mut Proof,
     ) -> GenericResult<Option<Proof>> {
-        let bytes = self.db.get(root)?.expect("bytes");
+        let bytes = self.db.get(root)?.expect("gen_proof(): bytes");
         let (cell, _) = Node::cells_from_bytes(&bytes, bits.first())?;
         let unit = cell.as_ref().expect("gen_proof(): left-unit");
         let n = Bits::len_common_bits(&unit.bits, &bits);
@@ -386,7 +382,7 @@ impl Monotree {
             }
             n if n == unit.bits.len() => {
                 proof.push(self.encode_proof(&bytes, bits.first())?);
-                self.gen_proof(unit.hash, bits.shift(n, false), proof)
+                self.gen_proof(unit.hash, bits.drop(n), proof)
             }
             _ => Ok(None),
         }

src/sdk/src/monotree/utils.rs

@@ -108,34 +108,25 @@ where
     cast(count)
 }
 
+static BIT_MASKS: [u8; 8] = [0x80, 0x40, 0x20, 0x10, 0x08, 0x04, 0x02, 0x01];
+
 /// Get `i`-th bit from bytes slice. Index `i` starts from 0.
+#[inline]
 pub fn bit<T: PrimInt + NumCast>(bytes: &[u8], i: T) -> bool {
-    let q = i.to_usize().expect("bit(): usize") / 8;
-    let r = i.to_u8().expect("bit(): u8") % 8;
-    (bytes[q] >> (7 - r)) & 0x01 == 0x01
+    let i_usize = i.to_usize().unwrap();
+    let q = i_usize >> 3;
+    let r = i_usize & 7;
+    if q >= bytes.len() {
+        return false;
+    }
+    bytes[q] & BIT_MASKS[r] != 0
 }
 
 /// Get the required length of bytes from a `Range`, bits indices across the bytes.
 pub fn nbytes_across<T: PrimInt + NumCast>(start: T, end: T) -> T {
-    let n = (end - (start - start % cast(8))) / cast(8);
-
-    if end % cast(8) == cast(0) {
-        n
-    } else {
-        n + cast(1)
-    }
-}
-
-/// Adjust the bytes representation for `Bits` when shifted.
-/// Returns a bytes shift, `n` and thereby resulting shifted range, `R`.
-pub fn offsets<T: PrimInt + NumCast>(range: &Range<T>, n: T, tail: bool) -> (T, Range<T>) {
-    let x = range.start + n;
-    let e: T = cast(8);
-    if tail {
-        (nbytes_across(range.start, x), range.start..x)
-    } else {
-        (x / e, x % e..range.end - e * (x / e))
-    }
+    let eight = cast(8);
+    let bits = end - (start - start % eight);
+    (bits + eight - T::one()) / eight
 }
 
 /// Convert big-endian bytes into base10 or decimal number.
@@ -177,3 +168,76 @@ where
 pub fn bits_to_bytes(bits: &[bool]) -> Vec<u8> {
     bits.rchunks(8).rev().map(|v| bits_to_usize(v) as u8).collect()
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    #[test]
+    fn test_bit() {
+        let bytes = [0x73, 0x6f, 0x66, 0x69, 0x61];
+        assert_eq!(bit(&bytes, 10), true);
+        assert_eq!(bit(&bytes, 20), false);
+        assert_eq!(bit(&bytes, 30), false);
+    }
+
+    #[test]
+    fn test_nbyte_across() {
+        assert_eq!(nbytes_across(0, 8), 1);
+        assert_eq!(nbytes_across(1, 7), 1);
+        assert_eq!(nbytes_across(5, 9), 2);
+        assert_eq!(nbytes_across(9, 16), 1);
+        assert_eq!(nbytes_across(7, 19), 3);
+    }
+
+    #[test]
+    fn test_bytes_to_int() {
+        let number: usize = bytes_to_int(&[0x73, 0x6f, 0x66, 0x69, 0x61]);
+        assert_eq!(number, 495790221665usize);
+    }
+
+    #[test]
+    fn test_usize_to_bytes() {
+        assert_eq!(int_to_bytes(495790221665u64), [0x73, 0x6f, 0x66, 0x69, 0x61]);
+    }
+
+    #[test]
+    fn test_bytes_to_bits() {
+        assert_eq!(
+            bytes_to_bits(&[0x33, 0x33]),
+            [
+                false, false, true, true, false, false, true, true, false, false, true, true,
+                false, false, true, true,
+            ]
+        );
+    }
+
+    #[test]
+    fn test_bits_to_bytes() {
+        let bits = [
+            false, false, true, true, false, false, true, true, false, false, true, true, false,
+            false, true, true,
+        ];
+        assert_eq!(bits_to_bytes(&bits), [0x33, 0x33]);
+    }
+
+    #[test]
+    fn test_bits_to_usize() {
+        assert_eq!(
+            bits_to_usize(&[
+                false, false, true, true, false, false, true, true, false, false, true, true,
+                false, false, true, true,
+            ]),
+            13107usize
+        );
+    }
+
+    #[test]
+    fn test_len_lcp() {
+        let sofia = [0x73, 0x6f, 0x66, 0x69, 0x61];
+        let maria = [0x6d, 0x61, 0x72, 0x69, 0x61];
+        assert_eq!(len_lcp(&sofia, &(0..3), &maria, &(0..3)), 3);
+        assert_eq!(len_lcp(&sofia, &(0..3), &maria, &(5..9)), 0);
+        assert_eq!(len_lcp(&sofia, &(2..9), &maria, &(18..30)), 5);
+        assert_eq!(len_lcp(&sofia, &(20..30), &maria, &(3..15)), 4);
+    }
+}