Clean up bulk_update semantics

src/error.rs

@@ -29,6 +29,7 @@ pub enum Error {
     BuilderStackLeftover,
     BuilderFull,
     BulkUpdateUnclean,
+    UpdateMapMissingMaxIndex,
     CowMissingEntry,
     LevelIterPendingUpdates,
     IntraRebaseZeroHash,

src/interface.rs

@@ -26,6 +26,7 @@ pub trait ImmList<T: Value> {
 
 pub trait MutList<T: Value>: ImmList<T> {
     fn validate_push(current_len: usize) -> Result<(), Error>;
+    fn validate_bulk_update<U: UpdateMap<T>>(&self, updates: &U) -> Result<(), Error>;
     fn replace(&mut self, index: usize, value: T) -> Result<(), Error>;
     fn update<U: UpdateMap<T>>(
         &mut self,
@@ -137,10 +138,21 @@ where
         self.len() == 0
     }
 
+    /// Stage a new set of `updates`, while flushing existing ones.
     pub fn bulk_update(&mut self, updates: U) -> Result<(), Error> {
-        if !self.updates.is_empty() {
-            return Err(Error::BulkUpdateUnclean);
+        // Flush existing updates.
+        //
+        // This is required prior to `validate_bulk_update` to update the backing's internal
+        // length. It does mean that an invalid bulk update will mutate `self` by flushing the
+        // existing updates.
+        self.apply_updates()?;
+
+        // Validate new updates (check against List/Vector capacity and ensure contiguous range).
+        if !updates.is_empty() {
+            self.backing.validate_bulk_update(&updates)?;
         }
+
+        // Stage new updates.
         self.updates = updates;
         Ok(())
     }

src/list.rs

@@ -16,9 +16,11 @@ use serde::{Deserialize, Deserializer, Serialize, Serializer, ser::SerializeSeq}
 use ssz::{BYTES_PER_LENGTH_OFFSET, Decode, Encode, SszEncoder, TryFromIter};
 use std::collections::{BTreeMap, HashMap};
 use std::marker::PhantomData;
+use std::ops::ControlFlow;
 use tree_hash::{Hash256, PackedEncoding, TreeHash};
 use typenum::Unsigned;
 use vec_map::VecMap;
+
 #[derive(Debug, Clone, Educe)]
 #[educe(PartialEq(bound(T: Value, N: Unsigned, U: UpdateMap<T> + PartialEq)))]
 #[cfg_attr(
@@ -195,6 +197,14 @@ impl<T: Value, N: Unsigned, U: UpdateMap<T>> List<T, N, U> {
         self.interface.apply_updates()
     }
 
+    /// Replace the `update_map` for the list with the given `updates`.
+    ///
+    /// Any existing updates will be flushed to the underlying tree before the replacement occurs.
+    /// The new updates WILL NOT be flushed, unless `self.apply_updates` is called immediately
+    /// afterwards.
+    ///
+    /// Errors if the updates would create a gap (all updates at indices >= current length must be
+    /// contiguous without gaps).
     pub fn bulk_update(&mut self, updates: U) -> Result<(), Error> {
         self.interface.bulk_update(updates)
     }
@@ -292,6 +302,38 @@ where
         }
     }
 
+    fn validate_bulk_update<U: UpdateMap<T>>(&self, updates: &U) -> Result<(), Error> {
+        // For a list, we need to check that all updates at indices >= the current length are
+        // contiguous.
+        let max_index = updates.max_index().ok_or(Error::UpdateMapMissingMaxIndex)?;
+
+        if max_index >= N::to_usize() {
+            return Err(Error::OutOfBoundsUpdate {
+                index: max_index,
+                len: N::to_usize(),
+            });
+        }
+
+        let start = self.length.as_usize();
+        let end = max_index.saturating_add(1);
+
+        let mut previous_index = self.length.as_usize().checked_sub(1);
+
+        updates.for_each_range(start, end, |index, _| {
+            // This neatly handles the length=0 case, by ensuring that we only accept `index == 0`,
+            // which yields `None` when `.checked_sub(1)` is applied.
+            if index.checked_sub(1) != previous_index {
+                ControlFlow::Continue(Err(Error::OutOfBoundsUpdate {
+                    index,
+                    len: self.length.as_usize(),
+                }))
+            } else {
+                previous_index = Some(index);
+                ControlFlow::Continue(Ok(()))
+            }
+        })
+    }
+
     fn replace(&mut self, index: usize, value: T) -> Result<(), Error> {
         if index > self.len().as_usize() {
             return Err(Error::OutOfBoundsUpdate {

src/tests/bulk_update.rs

@@ -0,0 +1,48 @@
+use crate::List;
+use std::collections::BTreeMap;
+use typenum::U8;
+
+#[test]
+fn bulk_update_push_empty() {
+    let mut list: List<u64, U8, BTreeMap<usize, u64>> = List::empty();
+    let updates = BTreeMap::from([(0, 0)]);
+    list.bulk_update(updates).unwrap();
+    assert_eq!(list.len(), 1);
+    assert_eq!(*list.get(0).unwrap(), 0);
+}
+
+#[test]
+fn bulk_update_push_empty_fail() {
+    let mut list: List<u64, U8, BTreeMap<usize, u64>> = List::empty();
+    let updates = BTreeMap::from([(1, 0)]);
+    list.bulk_update(updates).unwrap_err();
+    assert_eq!(list.len(), 0);
+}
+
+#[test]
+fn bulk_update_push_one() {
+    let mut list: List<u64, U8, BTreeMap<usize, u64>> = List::new(vec![0, 1, 2, 3, 4]).unwrap();
+    let updates = BTreeMap::from([(5, 5)]);
+    list.bulk_update(updates).unwrap();
+    assert_eq!(list.len(), 6);
+    assert_eq!(*list.get(5).unwrap(), 5);
+}
+
+#[test]
+fn bulk_update_push_two_btreemap() {
+    let mut list: List<u64, U8, BTreeMap<usize, u64>> = List::new(vec![0, 1, 2, 3, 4]).unwrap();
+    let updates = BTreeMap::from([(5, 5), (6, 6)]);
+    list.bulk_update(updates).unwrap();
+    assert_eq!(list.len(), 7);
+    assert_eq!(*list.get(5).unwrap(), 5);
+    assert_eq!(*list.get(6).unwrap(), 6);
+}
+
+#[test]
+fn bulk_update_with_gap() {
+    let mut list: List<u64, U8, BTreeMap<usize, u64>> = List::new(vec![0, 1, 2, 3, 4]).unwrap();
+    let updates = BTreeMap::from([(6, 6)]);
+    list.bulk_update(updates).unwrap_err();
+    list.apply_updates().unwrap();
+    assert_eq!(list.len(), 5);
+}

src/tests/mod.rs

@@ -1,6 +1,7 @@
 #![cfg(test)]
 
 mod builder;
+mod bulk_update;
 mod iterator;
 mod mem;
 mod packed;

src/tests/proptest/operations.rs

@@ -1,7 +1,8 @@
 use super::{Large, arb_hash256, arb_index, arb_large, arb_list, arb_vect};
-use crate::{Error, List, Value, Vector};
+use crate::{Error, List, UpdateMap, Value, Vector, update_map::MaxMap};
 use proptest::prelude::*;
 use ssz::{Decode, Encode};
+use std::cmp::Ordering;
 use std::fmt::Debug;
 use std::marker::PhantomData;
 use std::ops::Deref;
@@ -90,6 +91,29 @@ impl<T: Value, N: Unsigned> Spec<T, N> {
             Err(Error::PushNotSupported)
         }
     }
+
+    pub fn bulk_update(&mut self, mut updates: Vec<(usize, T)>) -> Result<(), Error> {
+        let mut new_list = self.clone();
+        updates.sort_by_key(|(index, _)| *index);
+        for (index, value) in updates {
+            match index.cmp(&new_list.values.len()) {
+                Ordering::Less => {
+                    new_list.values[index] = value;
+                }
+                Ordering::Equal if self.allow_push => {
+                    new_list.push(value)?;
+                }
+                _ => {
+                    return Err(Error::OutOfBoundsUpdate {
+                        index,
+                        len: new_list.values.len(),
+                    });
+                }
+            }
+        }
+        *self = new_list;
+        Ok(())
+    }
 }
 
 #[derive(Debug, Clone)]
@@ -128,6 +152,8 @@ pub enum Op<T> {
     FromIntoRoundtrip,
     /// Rebase the list on itself to exploit self-sharing.
     IntraRebase,
+    /// Apply a bulk update to the list/vector.
+    BulkUpdate(Vec<(usize, T)>),
 }
 
 fn arb_op<'a, T, S>(strategy: &'a S, n: usize) -> impl Strategy<Value = Op<T>> + 'a
@@ -158,10 +184,11 @@ where
         Just(Op::Debase),
         Just(Op::FromIntoRoundtrip),
         Just(Op::IntraRebase),
+        proptest::collection::vec((arb_index(n), strategy), 0..n).prop_map(Op::BulkUpdate),
     ];
     prop_oneof![
         10 => a_block,
-        7 => b_block
+        8 => b_block
     ]
 }
 
@@ -285,6 +312,21 @@ where
                 assert_eq!(new_list, *list);
                 *list = new_list;
             }
+            Op::BulkUpdate(updates) => {
+                let mut update_map = MaxMap::<vec_map::VecMap<T>>::default();
+                for &(index, ref value) in &updates {
+                    update_map.insert(index, value.clone());
+                }
+
+                match spec.bulk_update(updates) {
+                    Ok(()) => {
+                        list.bulk_update(update_map).unwrap();
+                    }
+                    Err(_) => {
+                        list.bulk_update(update_map).unwrap_err();
+                    }
+                }
+            }
         }
     }
 }
@@ -389,6 +431,21 @@ where
                 assert_eq!(new_vect, *vect);
                 *vect = new_vect;
             }
+            Op::BulkUpdate(updates) => {
+                let mut update_map = MaxMap::<vec_map::VecMap<T>>::default();
+                for &(index, ref value) in &updates {
+                    update_map.insert(index, value.clone());
+                }
+
+                match spec.bulk_update(updates) {
+                    Ok(()) => {
+                        vect.bulk_update(update_map).unwrap();
+                    }
+                    Err(_) => {
+                        vect.bulk_update(update_map).unwrap_err();
+                    }
+                }
+            }
         }
     }
 }

src/update_map.rs

@@ -20,6 +20,7 @@ pub trait UpdateMap<T>: Default + Clone {
 
     fn insert(&mut self, k: usize, value: T) -> Option<T>;
 
+    /// Iterate between `start` (inclusive) and `end` (exclusive).
     fn for_each_range<F, E>(&self, start: usize, end: usize, f: F) -> Result<(), E>
     where
         F: FnMut(usize, &T) -> ControlFlow<(), Result<(), E>>;

src/vector.rs

@@ -130,6 +130,10 @@ impl<T: Value, N: Unsigned, U: UpdateMap<T>> Vector<T, N, U> {
     pub fn apply_updates(&mut self) -> Result<(), Error> {
         self.interface.apply_updates()
     }
+
+    pub fn bulk_update(&mut self, updates: U) -> Result<(), Error> {
+        self.interface.bulk_update(updates)
+    }
 }
 
 impl<T: Value, N: Unsigned, U: UpdateMap<T>> TryFrom<List<T, N, U>> for Vector<T, N, U> {
@@ -249,6 +253,20 @@ where
         Err(Error::PushNotSupported)
     }
 
+    fn validate_bulk_update<U: UpdateMap<T>>(&self, updates: &U) -> Result<(), Error> {
+        // For a vector there is no possibility of pushing in a bulk update so it is sufficient to
+        // check the `max_index`.
+        let max_index = updates.max_index().ok_or(Error::UpdateMapMissingMaxIndex)?;
+        if max_index >= N::to_usize() {
+            Err(Error::OutOfBoundsUpdate {
+                index: max_index,
+                len: N::to_usize(),
+            })
+        } else {
+            Ok(())
+        }
+    }
+
     fn replace(&mut self, index: usize, value: T) -> Result<(), Error> {
         if index >= self.len().as_usize() {
             return Err(Error::OutOfBoundsUpdate {