From 05cf399a936f075f9e0b5f1c6f2b6b35efc5eccd Mon Sep 17 00:00:00 2001 From: deepld Date: Tue, 28 Apr 2026 05:52:55 -0400 Subject: [PATCH 1/4] feat: add plain_value_and_slot accessor and align SaltValue to 8 bytes - PlainStateProvider::plain_value_and_slot() returns the (SlotId, SaltValue) pair without the Vec copy that plain_value performs; plain_value now delegates to it. - Align SaltValue to 8 bytes so hot-path eq/clone can use aligned u64 loads (adds 2 bytes of trailing padding, 94 -> 96). - Cover plain_value_and_slot with a direct test asserting both the returned SlotId and the SaltValue match the underlying store entry. --- salt/src/state/state.rs | 68 ++++++++++++++++++++++++++++++++++++++--- salt/src/types.rs | 19 ++++++++---- 2 files changed, 76 insertions(+), 11 deletions(-) diff --git a/salt/src/state/state.rs b/salt/src/state/state.rs index a29ed45b..6a1e0658 100644 --- a/salt/src/state/state.rs +++ b/salt/src/state/state.rs @@ -902,16 +902,38 @@ impl<'a, S: StateReader> PlainStateProvider<'a, S> { plain_key: &[u8], hint: Option, ) -> Result>, S::Error> { + Ok(self + .plain_value_and_slot(plain_key, hint)? + .map(|(_, salt_val)| salt_val.value().to_vec())) + } + + /// Retrieves a plain value together with the slot it occupies. + /// + /// Same lookup as [`Self::plain_value`], but exposes the underlying + /// [`SlotId`] and full [`SaltValue`] for callers that need them + /// (e.g. to read the original encoded record or perform follow-up + /// slot-addressed operations) without paying for the `Vec` copy. + /// + /// # Arguments + /// * `plain_key` - The plain key to look up + /// * `hint` - Optional bucket_id hint; if provided only that bucket is searched + /// + /// # Returns + /// * `Ok(Some((slot_id, salt_value)))` - Slot and encoded value when the key exists + /// * `Ok(None)` - If the key does not exist + /// * `Err(error)` - On underlying storage errors + pub fn plain_value_and_slot( + &self, + plain_key: &[u8], + hint: Option, + ) -> Result, S::Error> { // Use the hint if provided, otherwise compute the bucket_id let bucket_id = hint.unwrap_or_else(|| hasher::bucket_id(plain_key)); let meta = self.store.metadata(bucket_id)?; - match shi_search(bucket_id, meta.nonce, meta.capacity, plain_key, |key| { + shi_search(bucket_id, meta.nonce, meta.capacity, plain_key, |key| { self.store.value(key) - })? { - Some((_, salt_val)) => Ok(Some(salt_val.value().to_vec())), - None => Ok(None), - } + }) } } @@ -1494,6 +1516,42 @@ mod tests { assert_eq!(provider.plain_value(b"missing", None).unwrap(), None); } + #[test] + fn test_plain_state_provider_plain_value_and_slot() { + let store = MemStore::new(); + let kvs = create_same_bucket_test_data(1); + let updates = EphemeralSaltState::new(&store) + .update_fin(kvs.iter().map(|(k, v)| (k, v))) + .unwrap(); + store.update_state(updates); + + let provider = PlainStateProvider::new(&store); + let (key, value) = &kvs[0]; + let bucket_id = hasher::bucket_id(key); + + // Successful lookup returns the slot and a SaltValue whose value() matches. + let (slot, salt_val) = provider.plain_value_and_slot(key, None).unwrap().unwrap(); + assert_eq!(salt_val.value(), value.as_deref().unwrap()); + + // Returned slot must be the slot the entry was actually stored at. + let stored = store.value((bucket_id, slot).into()).unwrap().unwrap(); + assert_eq!(stored, salt_val); + + // Wrong hint short-circuits to None without touching the right bucket. + assert_eq!( + provider + .plain_value_and_slot(key, Some(bucket_id + 1)) + .unwrap(), + None + ); + + // Non-existent key returns None. + assert_eq!( + provider.plain_value_and_slot(b"missing", None).unwrap(), + None + ); + } + /// Tests cache hit and miss behavior in the value() method. /// /// Validates that the caching logic correctly handles: diff --git a/salt/src/types.rs b/salt/src/types.rs index b9c07713..bf96dfab 100644 --- a/salt/src/types.rs +++ b/salt/src/types.rs @@ -270,7 +270,12 @@ pub const MAX_SALT_VALUE_BYTES: usize = 94; /// /// Format: `key_len` (1 byte) | `value_len` (1 byte) | `key` | `value` /// Supports Account, Storage, and BucketMeta types. +/// +/// `repr(align(8))` so hot paths (PartialEq / clone) can access `data` +/// as u64 chunks without unaligned-load fixups. Costs 2 bytes of trailing +/// padding per value (size 94 → 96). #[derive(Clone, Debug, Deref, DerefMut, PartialEq, Eq, Serialize, Deserialize)] +#[repr(align(8))] pub struct SaltValue { /// Fixed-size array accommodating the largest possible encoded data (94 bytes). #[deref] @@ -287,13 +292,15 @@ impl SaltValue { let key_len = key.len(); let value_len = value.len(); - let mut data = [0u8; MAX_SALT_VALUE_BYTES]; - data[0] = key_len as u8; - data[1] = value_len as u8; - data[2..2 + key_len].copy_from_slice(key); - data[2 + key_len..2 + key_len + value_len].copy_from_slice(value); + let mut result = Self { + data: [0u8; MAX_SALT_VALUE_BYTES], + }; + result.data[0] = key_len as u8; + result.data[1] = value_len as u8; + result.data[2..2 + key_len].copy_from_slice(key); + result.data[2 + key_len..2 + key_len + value_len].copy_from_slice(value); - Self { data } + result } /// Extract the key portion from the encoded data. From 14f94308acc82d6b3f7298cf20c8756486d53580 Mon Sep 17 00:00:00 2001 From: deepld Date: Tue, 28 Apr 2026 06:28:42 -0400 Subject: [PATCH 2/4] revert: drop repr(align(8)) on SaltValue The align(8) change introduced in the previous commit caused a perf regression on the update path: -12% on 1-thread, scaling down to -3% at 16-thread `update 10000 KVs` benchmark. Root cause: SaltValue is already 8-aligned in most containers (Vec, Box, BTreeMap) due to surrounding alignment. The one place align(8) actually changes layout is inside Option -- where it forces 7 bytes of padding after the discriminant plus 2 bytes of trailing padding inside SaltValue itself, growing Option from 95 to 104 bytes (+9.5%). Both hot-path containers carry Option: - EphemeralSaltState::cache: HashMap> - StateUpdates::data: BTreeMap, Option)> So the alignment penalty hits exactly the working set of `update 10000 KVs`, inflating it by ~10% and pushing it past L1 on single-thread. The "aligned u64 load" win is also negligible on modern x86 (unaligned MOVQ is zero-cost since Sandy Bridge). Keeps plain_value_and_slot from the previous commit -- only reverts the alignment change and the cosmetic SaltValue::new rewrite that was bundled with it. --- salt/src/types.rs | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/salt/src/types.rs b/salt/src/types.rs index bf96dfab..b9c07713 100644 --- a/salt/src/types.rs +++ b/salt/src/types.rs @@ -270,12 +270,7 @@ pub const MAX_SALT_VALUE_BYTES: usize = 94; /// /// Format: `key_len` (1 byte) | `value_len` (1 byte) | `key` | `value` /// Supports Account, Storage, and BucketMeta types. -/// -/// `repr(align(8))` so hot paths (PartialEq / clone) can access `data` -/// as u64 chunks without unaligned-load fixups. Costs 2 bytes of trailing -/// padding per value (size 94 → 96). #[derive(Clone, Debug, Deref, DerefMut, PartialEq, Eq, Serialize, Deserialize)] -#[repr(align(8))] pub struct SaltValue { /// Fixed-size array accommodating the largest possible encoded data (94 bytes). #[deref] @@ -292,15 +287,13 @@ impl SaltValue { let key_len = key.len(); let value_len = value.len(); - let mut result = Self { - data: [0u8; MAX_SALT_VALUE_BYTES], - }; - result.data[0] = key_len as u8; - result.data[1] = value_len as u8; - result.data[2..2 + key_len].copy_from_slice(key); - result.data[2 + key_len..2 + key_len + value_len].copy_from_slice(value); + let mut data = [0u8; MAX_SALT_VALUE_BYTES]; + data[0] = key_len as u8; + data[1] = value_len as u8; + data[2..2 + key_len].copy_from_slice(key); + data[2 + key_len..2 + key_len + value_len].copy_from_slice(value); - result + Self { data } } /// Extract the key portion from the encoded data. From 459c28e4337804288f61ee61abc48c046d4e8ef3 Mon Sep 17 00:00:00 2001 From: deepld Date: Tue, 28 Apr 2026 09:08:39 -0400 Subject: [PATCH 3/4] fix: drop hashbrown/serde feature to unblock downstream alloy-tx-macros Hand-write Serialize/Deserialize for SaltProof::levels so the salt and ipa-multipoint crates can drop the hashbrown/serde feature flag, which transitively pulled in serde_core 1.0.221+ and broke downstream alloy-tx-macros 1.0.23. Entries are emitted in ascending key order so proof bytes stay deterministic across provers. --- Cargo.lock | 2 - ipa-multipoint/Cargo.toml | 2 +- salt/Cargo.toml | 2 +- salt/src/proof/prover.rs | 138 +++++++++++++++++++++++++++++++++++++- 4 files changed, 139 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 64a3b197..6b7625fe 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -634,8 +634,6 @@ dependencies = [ "equivalent", "foldhash", "rayon", - "serde", - "serde_core", ] [[package]] diff --git a/ipa-multipoint/Cargo.toml b/ipa-multipoint/Cargo.toml index 424eb633..dd99b08d 100644 --- a/ipa-multipoint/Cargo.toml +++ b/ipa-multipoint/Cargo.toml @@ -12,7 +12,7 @@ required-features = ["std"] [dependencies] banderwagon = { path = "../banderwagon", default-features = false } -hashbrown = { version = "0.16", features = ["serde"] } +hashbrown = "0.16" hex = { version = "0.4.3", default-features = false, features = ["alloc"] } itertools = { version = "0.10.1", default-features = false, features = ["use_alloc"] } rayon = { version = "*", optional = true } diff --git a/salt/Cargo.toml b/salt/Cargo.toml index 7eef6f64..5b46a30f 100644 --- a/salt/Cargo.toml +++ b/salt/Cargo.toml @@ -12,7 +12,7 @@ salt-macros = { path = "../salt-macros", default-features = false } auto_impl = { version = "1", default-features = false } blake3 = { version = "1.8", default-features = false } derive_more = { version = "2", default-features = false, features = ["full"] } -hashbrown = { version = "0.16", features = ["serde"] } +hashbrown = "0.16" hex = { version = "0.4", default-features = false, features = ["alloc"] } rayon = { version = "*", optional = true } rustc-hash = { version = "2.0", default-features = false } diff --git a/salt/src/proof/prover.rs b/salt/src/proof/prover.rs index 3cc6246f..cf6055bb 100644 --- a/salt/src/proof/prover.rs +++ b/salt/src/proof/prover.rs @@ -24,11 +24,16 @@ use ipa_multipoint::{ use salt_macros::prelude::*; use salt_macros::{chunks, iter, num_threads, sort_unstable}; -use serde::{Deserialize, Deserializer, Serialize, Serializer}; +use serde::{ + de::{MapAccess, Visitor}, + ser::SerializeMap, + Deserialize, Deserializer, Serialize, Serializer, +}; use spin::Lazy; use std::collections::{BTreeMap, BTreeSet}; use std::{format, string::ToString, vec::Vec}; +use core::fmt; use hashbrown::HashMap; use rustc_hash::FxBuildHasher; type FxHashMap = HashMap; @@ -109,9 +114,58 @@ pub struct SaltProof { /// the level of the buckets trie /// used to let verifier determine the bucket trie level + #[serde(with = "fx_hashmap_serde")] pub levels: FxHashMap, } +// Hand-written (de)serialization for `levels` so the salt crate doesn't need +// to enable `hashbrown/serde` (which transitively pulls in serde_core 1.0.221+ +// and breaks downstream alloy-tx-macros 1.0.23). Entries are emitted in +// ascending key order to keep proof bytes deterministic across provers. +mod fx_hashmap_serde { + use super::*; + + pub fn serialize( + map: &FxHashMap, + s: S, + ) -> Result { + let mut entries: Vec<(&BucketId, &u8)> = map.iter().collect(); + entries.sort_unstable_by_key(|(k, _)| **k); + let mut m = s.serialize_map(Some(entries.len()))?; + for (k, v) in entries { + m.serialize_entry(k, v)?; + } + m.end() + } + + pub fn deserialize<'de, D: Deserializer<'de>>( + d: D, + ) -> Result, D::Error> { + struct LevelsVisitor; + + impl<'de> Visitor<'de> for LevelsVisitor { + type Value = FxHashMap; + + fn expecting(&self, f: &mut fmt::Formatter) -> fmt::Result { + f.write_str("a map of BucketId to u8") + } + + fn visit_map>(self, mut access: A) -> Result { + let mut map = HashMap::with_capacity_and_hasher( + access.size_hint().unwrap_or(0), + FxBuildHasher, + ); + while let Some((k, v)) = access.next_entry::()? { + map.insert(k, v); + } + Ok(map) + } + } + + d.deserialize_map(LevelsVisitor) + } +} + /// Converts a bucket slot entry into a field element for IPA polynomial commitments. /// /// This function maps key-value pairs (or empty slots) to field elements that serve @@ -1213,6 +1267,88 @@ mod tests { assert_eq!(queries[1].point, Fr::from(5u64)); } + /// Round-trip + determinism tests for the hand-rolled `fx_hashmap_serde` + /// module. The module exists so we can drop the `hashbrown/serde` feature + /// (which transitively pulls in `serde_core >= 1.0.221` and breaks + /// downstream `alloy-tx-macros 1.0.23`); the sort-on-serialize is what + /// keeps proof bytes identical across provers despite `FxHashMap`'s + /// non-deterministic iteration order. + mod fx_hashmap_serde_tests { + use super::*; + + #[derive(Debug, PartialEq, Serialize, Deserialize)] + struct LevelsWrapper(#[serde(with = "fx_hashmap_serde")] FxHashMap); + + fn levels_wrapper>(entries: I) -> LevelsWrapper { + let mut map: FxHashMap = HashMap::with_hasher(FxBuildHasher); + for (k, v) in entries { + map.insert(k, v); + } + LevelsWrapper(map) + } + + /// Serializing then deserializing must yield an equivalent map. + #[test] + fn round_trip_preserves_entries() { + let original = + levels_wrapper([(0u32, 0u8), (42, 3), (1_000_000, 7), (BucketId::MAX, 255)]); + + let bytes = + bincode::serde::encode_to_vec(&original, bincode::config::legacy()).unwrap(); + let (decoded, _): (LevelsWrapper, _) = + bincode::serde::decode_from_slice(&bytes, bincode::config::legacy()).unwrap(); + + assert_eq!(original, decoded); + } + + /// Empty map round-trips and serializes to a stable, non-empty map header. + #[test] + fn round_trip_empty() { + let original = levels_wrapper([]); + let bytes = + bincode::serde::encode_to_vec(&original, bincode::config::legacy()).unwrap(); + let (decoded, _): (LevelsWrapper, _) = + bincode::serde::decode_from_slice(&bytes, bincode::config::legacy()).unwrap(); + assert_eq!(original, decoded); + assert!(decoded.0.is_empty()); + } + + /// Two maps with identical entries inserted in different orders must + /// serialize to identical bytes. Without the ascending-key sort in + /// `fx_hashmap_serde::serialize`, `FxHashMap` iteration order would + /// leak into proof bytes and silently diverge across provers. + #[test] + fn serialization_is_order_independent() { + let entries: [(BucketId, u8); 6] = [ + (3, 1), + (1, 2), + (4, 3), + (1_000_000, 4), + (5, 5), + (BucketId::MAX, 6), + ]; + + let forward = levels_wrapper(entries); + // Reverse insertion order to perturb the FxHashMap's internal layout. + let reverse = { + let mut rev = entries; + rev.reverse(); + levels_wrapper(rev) + }; + + // Sanity: the underlying maps are equal as maps... + assert_eq!(forward, reverse); + + let forward_bytes = + bincode::serde::encode_to_vec(&forward, bincode::config::legacy()).unwrap(); + let reverse_bytes = + bincode::serde::encode_to_vec(&reverse, bincode::config::legacy()).unwrap(); + + // ...and serialize to identical bytes regardless of insertion order. + assert_eq!(forward_bytes, reverse_bytes); + } + } + /// Tests create_leaf_node_queries with missing key-value data. #[test] fn test_create_leaf_node_queries_missing_data() { From 950d81b5662815226df5081c8b3ba741e5870675 Mon Sep 17 00:00:00 2001 From: deepld Date: Thu, 30 Apr 2026 03:21:10 -0400 Subject: [PATCH 4/4] fix comment --- salt/src/proof/prover.rs | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/salt/src/proof/prover.rs b/salt/src/proof/prover.rs index cf6055bb..d206cce9 100644 --- a/salt/src/proof/prover.rs +++ b/salt/src/proof/prover.rs @@ -25,7 +25,7 @@ use ipa_multipoint::{ use salt_macros::prelude::*; use salt_macros::{chunks, iter, num_threads, sort_unstable}; use serde::{ - de::{MapAccess, Visitor}, + de::{Error as _, MapAccess, Visitor}, ser::SerializeMap, Deserialize, Deserializer, Serialize, Serializer, }; @@ -151,12 +151,11 @@ mod fx_hashmap_serde { } fn visit_map>(self, mut access: A) -> Result { - let mut map = HashMap::with_capacity_and_hasher( - access.size_hint().unwrap_or(0), - FxBuildHasher, - ); + let mut map: FxHashMap = FxHashMap::default(); while let Some((k, v)) = access.next_entry::()? { - map.insert(k, v); + if map.insert(k, v).is_some() { + return Err(A::Error::custom("duplicate BucketId in levels")); + } } Ok(map) } @@ -1347,6 +1346,20 @@ mod tests { // ...and serialize to identical bytes regardless of insertion order. assert_eq!(forward_bytes, reverse_bytes); } + + #[test] + fn rejects_duplicate_bucket_id() { + let mut bytes = Vec::new(); + bytes.extend_from_slice(&2u64.to_le_bytes()); + bytes.extend_from_slice(&7u32.to_le_bytes()); + bytes.push(1u8); + bytes.extend_from_slice(&7u32.to_le_bytes()); + bytes.push(2u8); + + let result: Result<(LevelsWrapper, _), _> = + bincode::serde::decode_from_slice(&bytes, bincode::config::legacy()); + assert!(result.is_err(), "duplicate BucketId must be rejected"); + } } /// Tests create_leaf_node_queries with missing key-value data.