From 88828528e00a4a9601f180fc4ece64d7131492af Mon Sep 17 00:00:00 2001 From: Alex North <445306+anorth@users.noreply.github.com> Date: Wed, 9 Aug 2023 09:12:37 +1000 Subject: [PATCH] Initial implementation of new, more convenient Map wrapping HAMT (#1349) * Initial implementation of new, more convenient Map wrapping HAMT * MapKey trait --- Cargo.lock | 1 + actors/init/src/state.rs | 47 +++------ actors/init/src/testing.rs | 31 +++--- runtime/Cargo.toml | 41 ++++---- runtime/src/lib.rs | 25 ++--- runtime/src/util/map.rs | 201 +++++++++++++++++++++++++++++++++++++ runtime/src/util/mod.rs | 2 + 7 files changed, 262 insertions(+), 86 deletions(-) create mode 100644 runtime/src/util/map.rs diff --git a/Cargo.lock b/Cargo.lock index 8050e9227..a34121525 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1806,6 +1806,7 @@ dependencies = [ "fvm_sdk", "fvm_shared", "hex", + "integer-encoding", "itertools 0.10.5", "lazy_static", "libsecp256k1", diff --git a/actors/init/src/state.rs b/actors/init/src/state.rs index 49ac00518..c70dbee26 100644 --- a/actors/init/src/state.rs +++ b/actors/init/src/state.rs @@ -2,30 +2,27 @@ // SPDX-License-Identifier: Apache-2.0, MIT use cid::Cid; -use fil_actors_runtime::{ - actor_error, make_empty_map, make_map_with_root_and_bitwidth, ActorError, AsActorError, - FIRST_NON_SINGLETON_ADDR, -}; use fvm_ipld_blockstore::Blockstore; use fvm_ipld_encoding::tuple::*; use fvm_shared::address::{Address, Protocol}; -use fvm_shared::error::ExitCode; -use fvm_shared::{ActorID, HAMT_BIT_WIDTH}; +use fvm_shared::ActorID; + +use fil_actors_runtime::{actor_error, ActorError, Map2, DEFAULT_CONF, FIRST_NON_SINGLETON_ADDR}; -/// State is reponsible for creating #[derive(Serialize_tuple, Deserialize_tuple, Clone, Debug)] pub struct State { + /// HAMT[Address]ActorID pub address_map: Cid, pub next_id: ActorID, pub network_name: String, } +pub type AddressMap = Map2; + impl State { pub fn new(store: &BS, network_name: String) -> Result { - let empty_map = make_empty_map::<_, ()>(store, HAMT_BIT_WIDTH) - .flush() - .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to create empty map")?; - Ok(Self { address_map: empty_map, next_id: FIRST_NON_SINGLETON_ADDR, network_name }) + let empty = AddressMap::flush_empty(store, DEFAULT_CONF)?; + Ok(Self { address_map: empty, next_id: FIRST_NON_SINGLETON_ADDR, network_name }) } /// Maps argument addresses to to a new or existing actor ID. @@ -40,22 +37,16 @@ impl State { robust_addr: &Address, delegated_addr: Option<&Address>, ) -> Result<(ActorID, bool), ActorError> { - let mut map = make_map_with_root_and_bitwidth(&self.address_map, store, HAMT_BIT_WIDTH) - .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to load address map")?; + let mut map = AddressMap::load(store, &self.address_map, DEFAULT_CONF, "addresses")?; let (id, existing) = if let Some(delegated_addr) = delegated_addr { // If there's a delegated address, either recall the already-mapped actor ID or // create and map a new one. - let delegated_key = delegated_addr.to_bytes().into(); - if let Some(existing_id) = map - .get(&delegated_key) - .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to lookup delegated address")? - { + if let Some(existing_id) = map.get(delegated_addr)? { (*existing_id, true) } else { let new_id = self.next_id; self.next_id += 1; - map.set(delegated_key, new_id) - .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to map delegated address")?; + map.set(delegated_addr, new_id)?; (new_id, false) } } else { @@ -66,9 +57,7 @@ impl State { }; // Map the robust address to the ID, failing if it's already mapped to anything. - let is_new = map - .set_if_absent(robust_addr.to_bytes().into(), id) - .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to map robust address")?; + let is_new = map.set_if_absent(robust_addr, id)?; if !is_new { return Err(actor_error!( forbidden, @@ -76,8 +65,7 @@ impl State { robust_addr )); } - self.address_map = - map.flush().context_code(ExitCode::USR_ILLEGAL_STATE, "failed to store address map")?; + self.address_map = map.flush()?; Ok((id, existing)) } @@ -99,13 +87,8 @@ impl State { if addr.protocol() == Protocol::ID { return Ok(Some(*addr)); } - - let map = make_map_with_root_and_bitwidth(&self.address_map, store, HAMT_BIT_WIDTH) - .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to load address map")?; - - let found = map - .get(&addr.to_bytes()) - .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to get address entry")?; + let map = AddressMap::load(store, &self.address_map, DEFAULT_CONF, "addresses")?; + let found = map.get(addr)?; Ok(found.copied().map(Address::new_id)) } } diff --git a/actors/init/src/testing.rs b/actors/init/src/testing.rs index 9fc069c6a..10671b6d4 100644 --- a/actors/init/src/testing.rs +++ b/actors/init/src/testing.rs @@ -1,12 +1,14 @@ use std::collections::HashMap; -use fil_actors_runtime::{Map, MessageAccumulator, FIRST_NON_SINGLETON_ADDR}; use fvm_ipld_blockstore::Blockstore; use fvm_shared::{ address::{Address, Protocol}, ActorID, }; +use fil_actors_runtime::{MessageAccumulator, DEFAULT_CONF, FIRST_NON_SINGLETON_ADDR}; + +use crate::state::AddressMap; use crate::State; pub struct StateSummary { @@ -31,44 +33,37 @@ pub fn check_state_invariants( let mut stable_address_by_id = HashMap::::new(); let mut delegated_address_by_id = HashMap::::new(); - match Map::<_, ActorID>::load(&state.address_map, store) { + + match AddressMap::load(store, &state.address_map, DEFAULT_CONF, "addresses") { Ok(address_map) => { let ret = address_map.for_each(|key, actor_id| { - let key_address = Address::from_bytes(key)?; - - acc.require( - key_address.protocol() != Protocol::ID, - format!("key {key_address} is an ID address"), - ); + acc.require(key.protocol() != Protocol::ID, format!("key {key} is an ID address")); acc.require( actor_id >= &FIRST_NON_SINGLETON_ADDR, format!("unexpected singleton ID value {actor_id}"), ); - match key_address.protocol() { + match key.protocol() { Protocol::ID => { - acc.add(format!("key {key_address} is an ID address")); + acc.add(format!("key {key} is an ID address")); } Protocol::Delegated => { - if let Some(duplicate) = - delegated_address_by_id.insert(*actor_id, key_address) - { + if let Some(duplicate) = delegated_address_by_id.insert(*actor_id, key) { acc.add(format!( - "duplicate mapping to ID {actor_id}: {key_address} {duplicate}" + "duplicate mapping to ID {actor_id}: {key} {duplicate}" )); } } _ => { - if let Some(duplicate) = stable_address_by_id.insert(*actor_id, key_address) - { + if let Some(duplicate) = stable_address_by_id.insert(*actor_id, key) { acc.add(format!( - "duplicate mapping to ID {actor_id}: {key_address} {duplicate}" + "duplicate mapping to ID {actor_id}: {key} {duplicate}" )); } } } - init_summary.ids_by_address.insert(key_address, *actor_id); + init_summary.ids_by_address.insert(key, *actor_id); Ok(()) }); diff --git a/runtime/Cargo.toml b/runtime/Cargo.toml index 673849cb7..b7426cca7 100644 --- a/runtime/Cargo.toml +++ b/runtime/Cargo.toml @@ -8,40 +8,41 @@ edition = "2021" repository = "https://github.com/filecoin-project/builtin-actors" [dependencies] -fvm_ipld_hamt = { workspace = true } -fvm_ipld_amt = { workspace = true } -fvm_shared = { workspace = true } -num = { workspace = true } -num-traits = { workspace = true } -num-derive = { workspace = true } -serde = { workspace = true } -lazy_static = { workspace = true, optional = true } -unsigned-varint = { workspace = true } +anyhow = { workspace = true } byteorder = { workspace = true } +castaway = { workspace = true } cid = { workspace = true } -log = { workspace = true } -thiserror = { workspace = true } -anyhow = { workspace = true } -fvm_sdk = { workspace = true, optional = true } +fvm_ipld_amt = { workspace = true } +fvm_ipld_bitfield = { workspace = true } fvm_ipld_blockstore = { workspace = true } fvm_ipld_encoding = { workspace = true } -fvm_ipld_bitfield = { workspace = true } -multihash = { workspace = true } -serde_repr = { workspace = true } -regex = { workspace = true } +fvm_ipld_hamt = { workspace = true } +fvm_sdk = { workspace = true, optional = true } +fvm_shared = { workspace = true } +integer-encoding = { workspace = true } itertools = { workspace = true } +lazy_static = { workspace = true, optional = true } +log = { workspace = true } +multihash = { workspace = true } +num = { workspace = true } +num-derive = { workspace = true } +num-traits = { workspace = true } paste = { workspace = true } -castaway = { workspace = true } +regex = { workspace = true } +serde = { workspace = true } +serde_repr = { workspace = true } +thiserror = { workspace = true } +unsigned-varint = { workspace = true } # A fake-proofs dependency but... we can't select on that feature here because we enable it from # build.rs. sha2 = { workspace = true } # test_util -rand = { workspace = true, optional = true } -hex = { workspace = true, optional = true } blake2b_simd = { workspace = true, optional = true } +hex = { workspace = true, optional = true } pretty_env_logger = { workspace = true, optional = true } +rand = { workspace = true, optional = true } [dependencies.libsecp256k1] workspace = true diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index 82565f170..f09899a41 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -1,29 +1,29 @@ // Copyright 2019-2022 ChainSafe Systems // SPDX-License-Identifier: Apache-2.0, MIT -use builtin::HAMT_BIT_WIDTH; use cid::Cid; use fvm_ipld_amt::Amt; use fvm_ipld_blockstore::Blockstore; +#[cfg(not(feature = "fil-actor"))] +use fvm_ipld_hamt::Sha256; use fvm_ipld_hamt::{BytesKey, Error as HamtError, Hamt}; -use fvm_shared::address::Address; use fvm_shared::bigint::BigInt; pub use fvm_shared::BLOCKS_PER_EPOCH as EXPECTED_LEADERS_PER_EPOCH; use serde::de::DeserializeOwned; use serde::Serialize; use unsigned_varint::decode::Error as UVarintError; -pub use {fvm_ipld_amt, fvm_ipld_hamt}; -pub use self::actor_error::*; -pub use self::builtin::*; -pub use self::util::*; -use crate::runtime::Runtime; +use builtin::HAMT_BIT_WIDTH; +pub use dispatch::{dispatch, dispatch_default}; +pub use {fvm_ipld_amt, fvm_ipld_hamt}; #[cfg(feature = "fil-actor")] use crate::runtime::hash_algorithm::FvmHashSha256; +use crate::runtime::Runtime; -#[cfg(not(feature = "fil-actor"))] -use fvm_ipld_hamt::Sha256; +pub use self::actor_error::*; +pub use self::builtin::*; +pub use self::util::*; pub mod actor_error; pub mod builtin; @@ -31,7 +31,6 @@ pub mod runtime; pub mod util; mod dispatch; -pub use dispatch::{dispatch, dispatch_default}; #[cfg(feature = "test_utils")] pub mod test_utils; @@ -112,12 +111,6 @@ pub trait Keyer { fn key(&self) -> BytesKey; } -impl Keyer for Address { - fn key(&self) -> BytesKey { - self.to_bytes().into() - } -} - impl Keyer for u64 { fn key(&self) -> BytesKey { u64_key(*self) diff --git a/runtime/src/util/map.rs b/runtime/src/util/map.rs new file mode 100644 index 000000000..079a099d7 --- /dev/null +++ b/runtime/src/util/map.rs @@ -0,0 +1,201 @@ +use crate::builtin::HAMT_BIT_WIDTH; +use crate::{ActorError, AsActorError, Hasher}; +use anyhow::anyhow; +use cid::Cid; +use fvm_ipld_blockstore::Blockstore; +use fvm_ipld_hamt as hamt; +use fvm_shared::address::Address; +use fvm_shared::error::ExitCode; +use serde::de::DeserializeOwned; +use serde::Serialize; +use std::marker::PhantomData; + +/// Wraps a HAMT to provide a convenient map API. +/// Any errors are returned with exit code indicating illegal state. +/// The name is not persisted in state, but adorns any error messages. +pub struct Map2 +where + BS: Blockstore, + K: MapKey, + V: DeserializeOwned + Serialize, +{ + hamt: hamt::Hamt, + name: &'static str, + key_type: PhantomData, +} + +pub trait MapKey: Sized { + fn from_bytes(b: &[u8]) -> Result; + fn to_bytes(&self) -> Result, String>; +} + +pub type Config = hamt::Config; + +pub const DEFAULT_CONF: Config = + Config { bit_width: HAMT_BIT_WIDTH, min_data_depth: 0, max_array_width: 3 }; + +impl Map2 +where + BS: Blockstore, + K: MapKey, + V: DeserializeOwned + Serialize, +{ + /// Creates a new, empty map. + pub fn empty(store: BS, config: Config, name: &'static str) -> Self { + Self { + hamt: hamt::Hamt::new_with_config(store, config), + name, + key_type: Default::default(), + } + } + + /// Creates a new empty map and flushes it to the store. + /// Returns the CID of the empty map root. + pub fn flush_empty(store: BS, config: Config) -> Result { + // This CID is constant regardless of the HAMT's configuration, so as an optimisation + // we could hard-code it and merely check it is already stored. + Self::empty(store, config, "empty").flush() + } + + /// Loads a map from the store. + // There is no version of this method that doesn't take an explicit config parameter. + // The caller must know the configuration to interpret the HAMT correctly. + // Forcing them to provide it makes it harder to accidentally use an incorrect default. + pub fn load( + store: BS, + root: &Cid, + config: Config, + name: &'static str, + ) -> Result { + Ok(Self { + hamt: hamt::Hamt::load_with_config(root, store, config) + .with_context_code(ExitCode::USR_ILLEGAL_STATE, || { + format!("failed to load HAMT '{}'", name) + })?, + name, + key_type: Default::default(), + }) + } + + /// Flushes the map's contents to the store. + /// Returns the root node CID. + pub fn flush(&mut self) -> Result { + self.hamt.flush().with_context_code(ExitCode::USR_ILLEGAL_STATE, || { + format!("failed to flush HAMT '{}'", self.name) + }) + } + + /// Returns a reference to the value associated with a key, if present. + pub fn get(&self, key: &K) -> Result, ActorError> { + let k = key.to_bytes().context_code(ExitCode::USR_ASSERTION_FAILED, "invalid key")?; + self.hamt.get(&k).with_context_code(ExitCode::USR_ILLEGAL_STATE, || { + format!("failed to get from HAMT '{}'", self.name) + }) + } + + /// Inserts a key-value pair into the map. + /// Returns any value previously associated with the key. + pub fn set(&mut self, key: &K, value: V) -> Result, ActorError> + where + V: PartialEq, + { + let k = key.to_bytes().context_code(ExitCode::USR_ASSERTION_FAILED, "invalid key")?; + self.hamt.set(k.into(), value).with_context_code(ExitCode::USR_ILLEGAL_STATE, || { + format!("failed to set in HAMT '{}'", self.name) + }) + } + + /// Inserts a key-value pair only if the key does not already exist. + /// Returns whether the map was modified (i.e. key was absent). + pub fn set_if_absent(&mut self, key: &K, value: V) -> Result + where + V: PartialEq, + { + let k = key.to_bytes().context_code(ExitCode::USR_ASSERTION_FAILED, "invalid key")?; + self.hamt + .set_if_absent(k.into(), value) + .with_context_code(ExitCode::USR_ILLEGAL_STATE, || { + format!("failed to set in HAMT '{}'", self.name) + }) + } + + /// Iterates over all key-value pairs in the map. + pub fn for_each(&self, mut f: F) -> Result<(), ActorError> + where + // Note the result type of F uses ActorError. + // The implementation will extract and propagate any ActorError + // wrapped in a hamt::Error::Dynamic. + F: FnMut(K, &V) -> Result<(), ActorError>, + { + match self.hamt.for_each(|k, v| { + let key = K::from_bytes(k).context_code(ExitCode::USR_ILLEGAL_STATE, "invalid key")?; + f(key, v).map_err(|e| anyhow!(e)) + }) { + Ok(_) => Ok(()), + Err(hamt_err) => match hamt_err { + hamt::Error::Dynamic(e) => match e.downcast::() { + Ok(ae) => Err(ae), + Err(e) => Err(ActorError::illegal_state(format!( + "error traversing HAMT {}: {}", + self.name, e + ))), + }, + e => Err(ActorError::illegal_state(format!( + "error traversing HAMT {}: {}", + self.name, e + ))), + }, + } + } +} + +impl MapKey for u64 { + fn from_bytes(b: &[u8]) -> Result { + let (v, rem) = unsigned_varint::decode::u64(b).map_err(|e| e.to_string())?; + if !rem.is_empty() { + return Err(format!("trailing bytes after varint: {:?}", rem)); + } + Ok(v) + } + + fn to_bytes(&self) -> Result, String> { + let mut bz = unsigned_varint::encode::u64_buffer(); + let slice = unsigned_varint::encode::u64(*self, &mut bz); + Ok(slice.into()) + } +} + +impl MapKey for Address { + fn from_bytes(b: &[u8]) -> Result { + Address::from_bytes(b).map_err(|e| e.to_string()) + } + + fn to_bytes(&self) -> Result, String> { + Ok(Address::to_bytes(*self)) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use fvm_ipld_blockstore::MemoryBlockstore; + + #[test] + fn basic_put_get() { + let bs = MemoryBlockstore::new(); + let mut m = Map2::<_, u64, String>::empty(bs, DEFAULT_CONF, "empty"); + m.set(&1234, "1234".to_string()).unwrap(); + assert!(m.get(&2222).unwrap().is_none()); + assert_eq!(&"1234".to_string(), m.get(&1234).unwrap().unwrap()); + } + + #[test] + fn for_each_callback_exitcode_propagates() { + let bs = MemoryBlockstore::new(); + let mut m = Map2::<_, u64, String>::empty(bs, DEFAULT_CONF, "empty"); + m.set(&1234, "1234".to_string()).unwrap(); + let res = m.for_each(|_, _| Err(ActorError::forbidden("test".to_string()))); + assert!(res.is_err()); + assert_eq!(res.unwrap_err(), ActorError::forbidden("test".to_string())); + } +} diff --git a/runtime/src/util/mod.rs b/runtime/src/util/mod.rs index d2dbd22fe..20f96dfac 100644 --- a/runtime/src/util/mod.rs +++ b/runtime/src/util/mod.rs @@ -5,6 +5,7 @@ pub use self::batch_return::BatchReturn; pub use self::batch_return::BatchReturnGen; pub use self::batch_return::FailCode; pub use self::downcast::*; +pub use self::map::*; pub use self::mapmap::MapMap; pub use self::message_accumulator::MessageAccumulator; pub use self::multimap::*; @@ -14,6 +15,7 @@ pub use self::set_multimap::SetMultimap; mod batch_return; pub mod cbor; mod downcast; +mod map; mod mapmap; mod message_accumulator; mod multimap;