feat: enforce sorted and unique storage slots

CHANGELOG.md

@@ -4,8 +4,7 @@
 
 ### Features
 
-- [BREAKING] Refactor storage slots to be accessed by names instead of indices ([#1987](https://github.com/0xMiden/miden-base/pull/1987)).
-- [BREAKING] Refactor storage slots to be accessed by names instead of indices ([#1987](https://github.com/0xMiden/miden-base/pull/1987), [#2025](https://github.com/0xMiden/miden-base/pull/2025), [#2149](https://github.com/0xMiden/miden-base/pull/2149), [#2150](https://github.com/0xMiden/miden-base/pull/2150), [#2153](https://github.com/0xMiden/miden-base/pull/2153)).
+- [BREAKING] Refactor storage slots to be accessed by names instead of indices ([#1987](https://github.com/0xMiden/miden-base/pull/1987), [#2025](https://github.com/0xMiden/miden-base/pull/2025), [#2149](https://github.com/0xMiden/miden-base/pull/2149), [#2150](https://github.com/0xMiden/miden-base/pull/2150), [#2153](https://github.com/0xMiden/miden-base/pull/2153), [#2154](https://github.com/0xMiden/miden-base/pull/2154)).
 
 ### Changes
 

crates/miden-lib/asm/kernels/transaction/lib/account.masm

@@ -33,6 +33,8 @@ const.ERR_ACCOUNT_PROC_NOT_AUTH_PROC="account procedure is not the authenticatio
 # TODO(named_slots): Remove along with index APIs.
 const.ERR_ACCOUNT_STORAGE_SLOT_INDEX_OUT_OF_BOUNDS="provided storage slot index is out of bounds"
 
+const.ERR_ACCOUNT_STORAGE_SLOTS_MUST_BE_SORTED_AND_UNIQUE="slot IDs must be unique and sorted in ascending order"
+
 const.ERR_ACCOUNT_UNKNOWN_STORAGE_SLOT_NAME="storage slot with the provided name does not exist"
 
 const.ERR_FAUCET_INVALID_STORAGE_OFFSET="storage offset is invalid for a faucet account (0 is prohibited as it is the reserved data slot for faucets)"
@@ -1091,6 +1093,106 @@ export.validate_seed
     # => []
 end
 
+#! Validates that slot IDs are sorted in ascending order and that slot IDs are unique.
+#!
+#! Inputs:  []
+#! Outputs: []
+#!
+#! Pancis if:
+#! - each slot's ID is not strictly less than the next slot's ID.
+#!    - this ensures sorting and uniqueness among slot IDs.
+pub proc validate_storage
+    exec.memory::get_num_storage_slots
+    # => [num_slots]
+
+    # compute the number of slot ID comparisons we need to make
+    # generally, we need num_storage_slots - 1 comparisons, e.g., if we have 3 slots, we need 2
+    # comparisons: 2 with 1 and 1 with 0.
+    # we subtract 1 if any slots exist and 0 otherwise, notably:
+    # maps 1 storage slot -> 0 comparisons, since 1 slot is always sorted and unique
+    # maps 0 storage slots -> 0 comparisons
+    dup u32gt.0
+    # => [has_slots, num_slots]
+
+    sub
+    # => [num_comparisons]
+
+    # loop if we need to compare slots
+    dup neq.0
+    # => [should_loop, num_comparisons]
+
+    while.true
+        # first iteration: number of comparisons = current slot index
+        # => [curr_slot_idx]
+
+        dup exec.get_slot_id
+        # => [curr_slot_id_prefix, curr_slot_id_suffix, curr_slot_idx]
+
+        # we are guaranteed to not underflow because curr_slot_idx is at least 1 at the
+        # beginning of the loop
+        dup.2 sub.1
+        # => [prev_slot_idx, curr_slot_id_prefix, curr_slot_id_suffix, curr_slot_idx]
+
+        exec.get_slot_id
+        # => [prev_slot_id_prefix, prev_slot_id_suffix, curr_slot_id_prefix, curr_slot_id_suffix, curr_slot_idx]
+
+        # this effectively checks that slots are sorted _and_ unique, since duplicate slot IDs are
+        # not less than each other
+        exec.is_slot_id_lt
+        # => [is_prev_lt_curr, curr_slot_idx]
+
+        assert.err=ERR_ACCOUNT_STORAGE_SLOTS_MUST_BE_SORTED_AND_UNIQUE
+        # => [curr_slot_idx]
+
+        sub.1 dup neq.0
+        # => [should_continue, prev_slot_idx]
+    end
+    # => [prev_slot_idx]
+
+    drop
+    # => []
+end
+
+#! Returns 1 if the previous slot ID is smaller than the current slot ID, 0 otherwise.
+#!
+#! In the slot ID comparison, the prefix takes precedence over the suffix.
+#!
+#! This procedure is public so it can be tested.
+#!
+#! Inputs:  [prev_slot_id_prefix, prev_slot_id_suffix, curr_slot_id_prefix, curr_slot_id_suffix]
+#! Outputs: [is_prev_lt_curr]
+pub proc is_slot_id_lt
+    movup.2
+    # => [curr_slot_id_prefix, prev_slot_id_prefix, prev_slot_id_suffix, curr_slot_id_suffix]
+
+    # compute prev == curr for prefix
+    dup dup.2 eq
+    # => [is_prefix_eq, curr_slot_id_prefix, prev_slot_id_prefix, prev_slot_id_suffix, curr_slot_id_suffix]
+
+    movdn.4
+    # => [curr_slot_id_prefix, prev_slot_id_prefix, prev_slot_id_suffix, curr_slot_id_suffix, is_prefix_eq]
+
+    # compute prev < curr for prefix
+    lt
+    # => [is_prev_lt_curr_prefix, prev_slot_id_suffix, curr_slot_id_suffix, is_prefix_eq]
+
+    swap.2
+    # => [curr_slot_id_suffix, prev_slot_id_suffix, is_prev_lt_curr_prefix, is_prefix_eq]
+
+    # compute prev < curr for suffix
+    lt
+    # => [is_prev_lt_curr_suffix, is_prev_lt_curr_prefix, is_prefix_eq]
+
+    movup.2
+    # => [is_prefix_eq, is_prev_lt_curr_suffix, is_prev_lt_curr_prefix]
+
+    # compute result as is_prefix_lt || (is_suffix_lt && is_prefix_eq)
+    # is_suffix_lt only affects the result if the prefix was equal, otherwise the prefix
+    # determines the outcome
+    and or
+    # => [is_prev_lt_curr]
+end
+
 # DATA LOADERS
 # -------------------------------------------------------------------------------------------------
 

crates/miden-lib/asm/kernels/transaction/lib/prologue.masm

@@ -276,7 +276,6 @@ end
 # ACCOUNT DATA
 # =================================================================================================
 
-#! TODO(named_slots): Validate that slot names are unique.
 #! This should only be necessary whenever the slots of an account "change", which is essentially
 #! the case when it is created, or when slots are added or removed (currently unimplemented).
 #! So, it should be sufficient to do this here instead of in save_account_storage_data.
@@ -363,6 +362,11 @@ proc.validate_new_account
     exec.account::validate_seed
     # => []
 
+    # Assert the storage slots are sorted and unique.
+    # ---------------------------------------------------------------------------------------------
+    exec.account::validate_storage
+    # => []
+
     # Assert the provided procedures offsets and sizes satisfy storage requirements
     # ---------------------------------------------------------------------------------------------
     exec.account::validate_procedure_metadata

crates/miden-lib/src/errors/tx_kernel_errors.rs

@@ -56,6 +56,8 @@ pub const ERR_ACCOUNT_STACK_UNDERFLOW: MasmError = MasmError::from_static_str("f
 pub const ERR_ACCOUNT_STORAGE_COMMITMENT_MISMATCH: MasmError = MasmError::from_static_str("computed account storage commitment does not match recorded account storage commitment");
 /// Error Message: "storage map entries provided as advice inputs do not have the same storage map root as the root of the map the new account commits to"
 pub const ERR_ACCOUNT_STORAGE_MAP_ENTRIES_DO_NOT_MATCH_MAP_ROOT: MasmError = MasmError::from_static_str("storage map entries provided as advice inputs do not have the same storage map root as the root of the map the new account commits to");
+/// Error Message: "slot IDs must be unique and sorted in ascending order"
+pub const ERR_ACCOUNT_STORAGE_SLOTS_MUST_BE_SORTED_AND_UNIQUE: MasmError = MasmError::from_static_str("slot IDs must be unique and sorted in ascending order");
 /// Error Message: "provided storage slot index is out of bounds"
 pub const ERR_ACCOUNT_STORAGE_SLOT_INDEX_OUT_OF_BOUNDS: MasmError = MasmError::from_static_str("provided storage slot index is out of bounds");
 /// Error Message: "number of account procedures exceeds the maximum limit of 256"

crates/miden-objects/src/account/storage/header.rs

@@ -76,6 +76,7 @@ impl AccountStorageHeader {
     /// Returns an error if:
     /// - The number of provided slots is greater than [`AccountStorage::MAX_NUM_STORAGE_SLOTS`].
     /// - The slots are not sorted by [`StorageSlotId`].
+    /// - There are multiple storage slots with the same [`StorageSlotName`].
     pub fn new(slots: Vec<(StorageSlotName, StorageSlotType, Word)>) -> Result<Self, AccountError> {
         if slots.len() > AccountStorage::MAX_NUM_STORAGE_SLOTS {
             return Err(AccountError::StorageTooManySlots(slots.len() as u64));
@@ -85,6 +86,14 @@ impl AccountStorageHeader {
             return Err(AccountError::UnsortedStorageSlots);
         }
 
+        // Check for slot name uniqueness by checking each neighboring slot's IDs. This is
+        // sufficient because the slots are sorted.
+        for slots in slots.windows(2) {
+            if slots[0].0.id() == slots[1].0.id() {
+                return Err(AccountError::DuplicateStorageSlotName(slots[0].0.clone()));
+            }
+        }
+
         Ok(Self { slots })
     }
 

crates/miden-objects/src/account/storage/mod.rs

@@ -1,4 +1,3 @@
-use alloc::collections::BTreeSet;
 use alloc::string::ToString;
 use alloc::vec::Vec;
 
@@ -82,19 +81,17 @@ impl AccountStorage {
             return Err(AccountError::StorageTooManySlots(num_slots as u64));
         }
 
-        // TODO(named_slots): Optimization: If we keep slots sorted, iterate slots instead and
-        // compare adjacent elements.
-        let mut names = BTreeSet::new();
-        for slot in &slots {
-            if !names.insert(slot.name()) {
-                // TODO(named_slots): Add test for this new error.
-                return Err(AccountError::DuplicateStorageSlotName(slot.name().clone()));
-            }
-        }
-
         // Unstable sort is fine because we require all names to be unique.
         slots.sort_unstable();
 
+        // Check for slot name uniqueness by checking each neighboring slot's IDs. This is
+        // sufficient because the slots are sorted.
+        for slots in slots.windows(2) {
+            if slots[0].id() == slots[1].id() {
+                return Err(AccountError::DuplicateStorageSlotName(slots[0].name().clone()));
+            }
+        }
+
         Ok(Self { slots })
     }
 
@@ -400,8 +397,11 @@ impl Deserializable for AccountStorage {
 
 #[cfg(test)]
 mod tests {
+    use assert_matches::assert_matches;
+
     use super::{AccountStorage, Deserializable, Serializable};
-    use crate::account::{StorageSlot, StorageSlotName};
+    use crate::AccountError;
+    use crate::account::{AccountStorageHeader, StorageSlot, StorageSlotName};
 
     #[test]
     fn test_serde_account_storage() -> anyhow::Result<()> {
@@ -438,4 +438,41 @@ mod tests {
 
         Ok(())
     }
+
+    #[test]
+    fn test_account_storage_and_header_fail_on_duplicate_slot_name() -> anyhow::Result<()> {
+        let slot_name0 = StorageSlotName::mock(0);
+        let slot_name1 = StorageSlotName::mock(1);
+        let slot_name2 = StorageSlotName::mock(2);
+
+        let mut slots = vec![
+            StorageSlot::with_empty_value(slot_name0.clone()),
+            StorageSlot::with_empty_value(slot_name1.clone()),
+            StorageSlot::with_empty_map(slot_name0.clone()),
+            StorageSlot::with_empty_value(slot_name2.clone()),
+        ];
+
+        // Set up a test where the slots we pass are not already sorted
+        // This ensures the duplicate is correctly found
+        let err = AccountStorage::new(slots.clone()).unwrap_err();
+
+        assert_matches!(err, AccountError::DuplicateStorageSlotName(name) => {
+            assert_eq!(name, slot_name0);
+        });
+
+        slots.sort_unstable();
+        let err = AccountStorageHeader::new(
+            slots
+                .iter()
+                .map(|slot| (slot.name().clone(), slot.slot_type(), slot.value()))
+                .collect(),
+        )
+        .unwrap_err();
+
+        assert_matches!(err, AccountError::DuplicateStorageSlotName(name) => {
+            assert_eq!(name, slot_name0);
+        });
+
+        Ok(())
+    }
 }

crates/miden-testing/src/kernel_tests/tx/test_account.rs

@@ -28,6 +28,7 @@ use miden_objects::account::{
     StorageMap,
     StorageSlot,
     StorageSlotContent,
+    StorageSlotId,
     StorageSlotName,
     StorageSlotType,
 };
@@ -524,6 +525,68 @@ async fn test_get_storage_slot_type() -> miette::Result<()> {
     Ok(())
 }
 
+#[tokio::test]
+async fn test_is_slot_id_lt() -> miette::Result<()> {
+    // Note that the slot IDs derived from the names are essentially randomly sorted, so these cover
+    // "less than" and "greater than" outcomes.
+    let mut test_cases = (0..100)
+        .map(|i| {
+            let prev_slot = StorageSlotName::mock(i).id();
+            let curr_slot = StorageSlotName::mock(i + 1).id();
+            (prev_slot, curr_slot)
+        })
+        .collect::<Vec<_>>();
+
+    // Extend with special case where prefix matches and suffix determines the outcome.
+    let prefix = Felt::from(100u32);
+    test_cases.extend([
+        // prev_slot == curr_slot
+        (
+            StorageSlotId::new(Felt::from(50u32), prefix),
+            StorageSlotId::new(Felt::from(50u32), prefix),
+        ),
+        // prev_slot < curr_slot
+        (
+            StorageSlotId::new(Felt::from(50u32), prefix),
+            StorageSlotId::new(Felt::from(51u32), prefix),
+        ),
+        // prev_slot > curr_slot
+        (
+            StorageSlotId::new(Felt::from(51u32), prefix),
+            StorageSlotId::new(Felt::from(50u32), prefix),
+        ),
+    ]);
+
+    for (prev_slot, curr_slot) in test_cases {
+        let code = format!(
+            r#"
+            use.$kernel::account
+
+            begin
+                push.{curr_suffix}.{curr_prefix}.{prev_suffix}.{prev_prefix}
+                # => [prev_slot_id_prefix, prev_slot_id_suffix, curr_slot_id_prefix, curr_slot_id_suffix]
+
+                exec.account::is_slot_id_lt
+                # => [is_slot_id_lt]
+
+                push.{is_lt}
+                assert_eq.err="is_slot_id_lt was not {is_lt}"
+                # => []
+            end
+            "#,
+            prev_prefix = prev_slot.prefix(),
+            prev_suffix = prev_slot.suffix(),
+            curr_prefix = curr_slot.prefix(),
+            curr_suffix = curr_slot.suffix(),
+            is_lt = u8::from(prev_slot < curr_slot)
+        );
+
+        CodeExecutor::with_default_host().run(&code).await?;
+    }
+
+    Ok(())
+}
+
 #[tokio::test]
 async fn test_set_item() -> anyhow::Result<()> {
     let tx_context = TransactionContextBuilder::with_existing_mock_account().build().unwrap();

crates/miden-tx/src/host/storage_delta_tracker.rs

@@ -192,7 +192,7 @@ impl StorageDeltaTracker {
 
 /// Creates empty slots of the same slot types as the to-be-created account.
 fn empty_storage_header_from_account(account: &PartialAccount) -> AccountStorageHeader {
-    let mut slots: Vec<(StorageSlotName, StorageSlotType, Word)> = account
+    let slots: Vec<(StorageSlotName, StorageSlotType, Word)> = account
         .storage()
         .header()
         .slots()
@@ -202,9 +202,7 @@ fn empty_storage_header_from_account(account: &PartialAccount) -> AccountStorage
         })
         .collect();
 
-    slots.sort_by_key(|(slot_name, ..)| slot_name.id());
-
-    // SAFETY: We have sorted the slots and the max number of slots should not be exceeded as
-    // enforced by the storage header in partial storage.
+    // SAFETY: We are recreating a valid storage header with different values, which should not
+    // violate any constraints of the storage header.
     AccountStorageHeader::new(slots).expect("storage header should be valid")
 }