Merge #348: fix: fix value bit corruption in right_shift

b64e1b3 add replicating tests (stringhandler)
6c74085 fix value bit corruption (stringhandler)

Pull request description:

  Yet another option to solve #337 
  
  The tests are actually generated by Claude. The comments are a bit wordy, so happy to try clean it up if it bothers anyone. 
  
  The interesting thing to note is that the bug is not as a result of `from_padded_bits`, as apoelstra (and myself) suspected.  Upon adding the two regression tests from #339, `prune_regression_337_1` still fails, indicating another bug in `from_padded_bits`. 
  
  However, `prune_regression_337_2` passes on this PR without any changes to `from_padded_bits`. The tests added in this PR also reproduce the error without calling `from_padded_bits`.
  
  I have also tried to keep the optimization of not cloning the array if the bit is already equal to `new_bit` and added it to the true case as well.
  
  For those interested here are the before and after benchmarks. Nothing jumps out at me as a huge improvement or degradation.
  
  BEFORE (Commit f84b09d)
  ```
  test node::redeem::benches::decode_fixed_program          ... bench:      46,840.83 ns/iter (+/- 596.58)
  test value::benches::bench_value_create_512_256           ... bench:      60,676.67 ns/iter (+/- 285.67)
  test value::benches::bench_value_create_512_256_compact   ... bench:       3,226.49 ns/iter (+/- 73.52)
  test value::benches::bench_value_create_512_256_padded    ... bench:       3,277.02 ns/iter (+/- 108.64)
  test value::benches::bench_value_create_64k               ... bench:     669,047.50 ns/iter (+/- 4,790.50)
  test value::benches::bench_value_create_64k_compact       ... bench:      34,772.54 ns/iter (+/- 3,966.75)
  test value::benches::bench_value_create_64k_padded        ... bench:      33,474.00 ns/iter (+/- 3,886.75)
  test value::benches::bench_value_create_deep_some         ... bench:     402,400.62 ns/iter (+/- 3,995.75)
  test value::benches::bench_value_create_deep_some_compact ... bench:     145,885.00 ns/iter (+/- 1,446.33)
  test value::benches::bench_value_create_deep_some_padded  ... bench:       1,224.16 ns/iter (+/- 4.50)
  test value::benches::bench_value_create_u2048             ... bench:     469,603.75 ns/iter (+/- 6,750.75)
  test value::benches::bench_value_create_u2048_compact     ... bench:       2,117.00 ns/iter (+/- 7.90)
  test value::benches::bench_value_create_u2048_padded      ... bench:       2,063.22 ns/iter (+/- 11.30)
  test value::benches::bench_value_create_u64               ... bench:          22.49 ns/iter (+/- 0.32)
  test value::benches::bench_value_create_u64_compact       ... bench:         165.02 ns/iter (+/- 1.33)
  test value::benches::bench_value_create_u64_padded        ... bench:         114.52 ns/iter (+/- 0.88)
  test value::benches::bench_value_display_512_256          ... bench:      69,926.25 ns/iter (+/- 804.88)
  test value::benches::bench_value_display_64k              ... bench:     707,050.00 ns/iter (+/- 10,249.00)
  test value::benches::bench_value_display_deep_some        ... bench:      44,677.50 ns/iter (+/- 727.31)
  test value::benches::bench_value_display_u2024            ... bench:      44,629.52 ns/iter (+/- 341.24)
  test value::benches::bench_value_display_u64              ... bench:         351.55 ns/iter (+/- 2.40)
  ``` 
  
  AFTER (c70e3ed)
  ```
  test node::redeem::benches::decode_fixed_program          ... bench:      47,575.83 ns/iter (+/- 1,888.92)
  test value::benches::bench_value_create_512_256           ... bench:      62,040.00 ns/iter (+/- 684.18)
  test value::benches::bench_value_create_512_256_compact   ... bench:       3,091.83 ns/iter (+/- 19.26)
  test value::benches::bench_value_create_512_256_padded    ... bench:       3,034.38 ns/iter (+/- 17.38)
  test value::benches::bench_value_create_64k               ... bench:     671,748.75 ns/iter (+/- 4,494.75)
  test value::benches::bench_value_create_64k_compact       ... bench:      32,111.70 ns/iter (+/- 5,517.22)
  test value::benches::bench_value_create_64k_padded        ... bench:      32,177.62 ns/iter (+/- 7,199.71)
  test value::benches::bench_value_create_deep_some         ... bench:     401,901.88 ns/iter (+/- 4,945.75)
  test value::benches::bench_value_create_deep_some_compact ... bench:     145,748.00 ns/iter (+/- 833.20)
  test value::benches::bench_value_create_deep_some_padded  ... bench:       1,205.44 ns/iter (+/- 5.89)
  test value::benches::bench_value_create_u2048             ... bench:     482,537.50 ns/iter (+/- 8,726.38)
  test value::benches::bench_value_create_u2048_compact     ... bench:       2,169.61 ns/iter (+/- 7.86)
  test value::benches::bench_value_create_u2048_padded      ... bench:       2,075.53 ns/iter (+/- 16.34)
  test value::benches::bench_value_create_u64               ... bench:          32.03 ns/iter (+/- 0.91)
  test value::benches::bench_value_create_u64_compact       ... bench:         162.57 ns/iter (+/- 1.75)
  test value::benches::bench_value_create_u64_padded        ... bench:         113.74 ns/iter (+/- 0.67)
  test value::benches::bench_value_display_512_256          ... bench:      74,847.14 ns/iter (+/- 1,099.57)
  test value::benches::bench_value_display_64k              ... bench:     709,160.00 ns/iter (+/- 12,038.00)
  test value::benches::bench_value_display_deep_some        ... bench:      43,401.18 ns/iter (+/- 559.59)
  test value::benches::bench_value_display_u2024            ... bench:      44,913.00 ns/iter (+/- 477.15)
  test value::benches::bench_value_display_u64              ... bench:         357.24 ns/iter (+/- 7.69)
  ```


ACKs for top commit:
  apoelstra:
    ACK b64e1b3; successfully ran local tests


Tree-SHA512: 950a480de05ac94681ec5df80f1730520ffd2a071cffa1a6a492761956203dbf8daf01644de882e34ef12080eacc7b447588bcae8f1b08cb86a4216635d9bfcc

Author: apoelstra

src/value.rs

@@ -276,14 +276,19 @@ fn right_shift_1(inner: &Arc<[u8]>, bit_offset: usize, new_bit: bool) -> (Arc<[u
     // If the current bit offset is nonzero this is super easy: we just
     // lower the bit offset and call that a fix.
     if bit_offset > 0 {
-        if new_bit {
-            let new_bit_offset = bit_offset - 1;
+        let new_bit_offset = bit_offset - 1;
+        let mask = 1u8 << (7 - new_bit_offset % 8);
+        let current_bit = inner[new_bit_offset / 8] & mask != 0;
+        if current_bit == new_bit {
+            (Arc::clone(inner), new_bit_offset)
+        } else if new_bit {
             let mut bx: Box<[u8]> = inner.as_ref().into();
-            bx[new_bit_offset / 8] |= 1 << (7 - new_bit_offset % 8);
+            bx[new_bit_offset / 8] |= mask;
             (bx.into(), new_bit_offset)
         } else {
-            // ...and if we are inserting a 0 we don't even need to allocate a new [u8]
-            (Arc::clone(inner), bit_offset - 1)
+            let mut bx: Box<[u8]> = inner.as_ref().into();
+            bx[new_bit_offset / 8] &= !mask;
+            (bx.into(), new_bit_offset)
         }
     } else {
         // If the current bit offset is 0, we just shift everything right by 8
@@ -1262,6 +1267,117 @@ mod tests {
         let new_v = Value::from_padded_bits(&mut iter, &v.ty).unwrap();
         assert_eq!(v, new_v);
     }
+
+    // --- Bug regression tests for right_shift_1 when new_bit=false ---
+    //
+    // right_shift_1(inner, bit_offset > 0, new_bit=false) is supposed to insert
+    // a 0 (Left tag) at position bit_offset-1 by decrementing bit_offset.  The bug
+    // is that it clones the Arc without clearing that position, so any pre-existing
+    // 1-bit at bit_offset-1 corrupts the Left tag, causing the value to read as
+    // Right instead of Left.
+    //
+    // The dirty bit originates from product sub-value extraction: as_product()
+    // returns ValueRef children that share the parent buffer.  The right child has
+    // bit_offset = parent.bit_offset + left_bit_width, so every 1-bit encoded by
+    // the left component sits "before" the right child's logical start.  When that
+    // right child is later wrapped in Left, right_shift_1 may see a stale 1-bit.
+
+    /// Simplest direct construction: extract the right sub-value of a product
+    /// whose left component has 1-bits, then wrap the sub-value in Left.
+    ///
+    /// `product(Right(u1(1)), Right(u1(0)))` encodes as [0xE0] at bit_offset=0:
+    ///   bits 0..=1  = Right(u1(1))  = 1, 1   ← left component
+    ///   bits 2..=3  = Right(u1(0))  = 1, 0   ← right component (bit_offset=2)
+    ///
+    /// Wrapping the right sub-value (bit_offset=2 in [0xE0]) in Left calls
+    /// right_shift_1(false) which should clear bit 1.  The bug leaves bit 1 = 1,
+    /// so the result reads as Right instead of Left.
+    #[test]
+    fn left_tag_not_corrupted_by_dirty_buffer() {
+        let product_val = Value::product(
+            Value::right(Final::u1(), Value::u1(1)), // encodes bits 0,1 as 1,1
+            Value::right(Final::u1(), Value::u1(0)), // starts at bit_offset=2
+        );
+
+        let (_, right_sub) = product_val.as_product().unwrap();
+        // right_sub shares [0xE0]; bit at position 1 (before its start) is 1.
+        let right_sub_owned = right_sub.to_value();
+
+        let left_val = Value::left(right_sub_owned, Final::unit());
+
+        assert!(
+            left_val.as_left().is_some(),
+            "BUG: Left tag corrupted — right_shift_1(false) did not clear dirty bit; \
+             value reads as Right instead of Left"
+        );
+        assert!(
+            left_val.as_right().is_none(),
+            "BUG: Left tag corrupted — as_right() should return None for a Left value"
+        );
+    }
+
+    /// Verify the dirty-buffer bug at the bit level: the first padded bit of a
+    /// Left value must always be 0 (the Left discriminant).
+    #[test]
+    fn left_tag_first_padded_bit_is_zero() {
+        let product_val = Value::product(
+            Value::right(Final::u1(), Value::u1(1)),
+            Value::right(Final::u1(), Value::u1(0)),
+        );
+
+        let (_, right_sub) = product_val.as_product().unwrap();
+        let left_val = Value::left(right_sub.to_value(), Final::unit());
+
+        let first_bit = left_val.iter_padded().next();
+        assert_eq!(
+            first_bit,
+            Some(false),
+            "BUG: first padded bit of Left value is {:?}, expected Some(false)",
+            first_bit
+        );
+    }
+
+    /// End-to-end via prune: pruning can extract a sub-value via to_value() that
+    /// shares the parent buffer and has 1-bits before its logical start.  When
+    /// prune then calls Value::left() on it (Task::MakeLeft), the bug corrupts
+    /// the Left tag.
+    ///
+    /// Original:  Left(product(Right(u1(1)), Right(u1(0))))
+    ///            type: ((u1+u1) * (u1+u1)) + unit
+    ///
+    /// Pruned to: (unit * (u1+u1)) + unit
+    ///
+    /// The right component of the inner product keeps its type (u1+u1) unchanged,
+    /// so prune returns it via to_value() — sharing the same dirty buffer.
+    /// MakeLeft then wraps it, triggering the bug.
+    #[test]
+    fn prune_does_not_corrupt_left_tag_via_shared_buffer() {
+        let original = Value::left(
+            Value::product(
+                Value::right(Final::u1(), Value::u1(1)),
+                Value::right(Final::u1(), Value::u1(0)),
+            ),
+            Final::unit(),
+        );
+
+        // Shrink the left component: (u1+u1)*(u1+u1)  →  unit*(u1+u1).
+        // The right sub-value of the product keeps type u1+u1 (matches exactly),
+        // so prune returns it via to_value() with the dirty shared buffer intact.
+        let pruned_ty = Final::sum(
+            Final::product(Final::unit(), Final::sum(Final::u1(), Final::u1())),
+            Final::unit(),
+        );
+
+        let pruned = original
+            .prune(&pruned_ty)
+            .expect("prune returned None; the target type is compatible");
+
+        assert!(
+            pruned.as_left().is_some(),
+            "BUG: prune corrupted the Left tag — right_shift_1(false) did not clear \
+             a dirty bit from the shared product buffer; value reads as Right"
+        );
+    }
 }
 
 #[cfg(bench)]