From d46362a561e70f7116235c877271e182a856e8ff Mon Sep 17 00:00:00 2001 From: dignifiedquire <me@dignifiedquire.com> Date: Wed, 6 Apr 2022 13:17:57 +0200 Subject: [PATCH 1/7] start patching through errors in ipld --- ipld/blockstore/Cargo.toml | 1 - ipld/blockstore/src/lib.rs | 45 ++++++++++++++++++--------------- ipld/blockstore/src/memory.rs | 10 +++++--- ipld/blockstore/src/tracking.rs | 16 +++++++----- ipld/encoding/Cargo.toml | 3 +-- ipld/encoding/src/cbor_store.rs | 15 ++++++++--- ipld/encoding/src/lib.rs | 2 +- ipld/hamt/Cargo.toml | 1 - ipld/hamt/src/error.rs | 44 +++++--------------------------- 9 files changed, 61 insertions(+), 76 deletions(-) diff --git a/ipld/blockstore/Cargo.toml b/ipld/blockstore/Cargo.toml index 00681469a..f125bd56f 100644 --- a/ipld/blockstore/Cargo.toml +++ b/ipld/blockstore/Cargo.toml @@ -9,7 +9,6 @@ repository = "https://github.com/filecoin-project/ref-fvm" [dependencies] cid = { version = "0.8.2", default-features = false, features = ["serde-codec", "std"] } -anyhow = "1.0.51" [dev-dependencies] diff --git a/ipld/blockstore/src/lib.rs b/ipld/blockstore/src/lib.rs index 15a720d89..3e3bc292a 100644 --- a/ipld/blockstore/src/lib.rs +++ b/ipld/blockstore/src/lib.rs @@ -1,6 +1,5 @@ use std::rc::Rc; -use anyhow::Result; use cid::{multihash, Cid}; pub mod tracking; @@ -15,8 +14,10 @@ pub use block::*; /// /// The cgo blockstore adapter implements this trait. pub trait Blockstore { + type Error: std::error::Error + std::fmt::Debug; + /// Gets the block from the blockstore. - fn get(&self, k: &Cid) -> Result<Option<Vec<u8>>>; + fn get(&self, k: &Cid) -> Result<Option<Vec<u8>>, Self::Error>; /// Put a block with a pre-computed cid. /// @@ -24,17 +25,17 @@ pub trait Blockstore { /// even if you provide it. /// /// If you _do_ already know the CID, use this method as some blockstores _won't_ recompute it. - fn put_keyed(&self, k: &Cid, block: &[u8]) -> Result<()>; + fn put_keyed(&self, k: &Cid, block: &[u8]) -> Result<(), Self::Error>; /// Checks if the blockstore has the specified block. - fn has(&self, k: &Cid) -> Result<bool> { + fn has(&self, k: &Cid) -> Result<bool, Self::Error> { Ok(self.get(k)?.is_some()) } /// Puts the block into the blockstore, computing the hash with the specified multicodec. /// /// By default, this defers to put. - fn put<D>(&self, mh_code: multihash::Code, block: &Block<D>) -> Result<Cid> + fn put<D>(&self, mh_code: multihash::Code, block: &Block<D>) -> Result<Cid, Self::Error> where Self: Sized, D: AsRef<[u8]>, @@ -55,7 +56,7 @@ pub trait Blockstore { /// let blocks = vec![Block::new(0x55, vec![0, 1, 2])]; /// bs.put_many(blocks.iter().map(|b| (Blake2b256, b.into()))).unwrap(); /// ``` - fn put_many<D, I>(&self, blocks: I) -> Result<()> + fn put_many<D, I>(&self, blocks: I) -> Result<(), Self::Error> where Self: Sized, D: AsRef<[u8]>, @@ -68,7 +69,7 @@ pub trait Blockstore { /// Bulk-put pre-keyed blocks into the blockstore. /// /// By default, this defers to put_keyed. - fn put_many_keyed<D, I>(&self, blocks: I) -> Result<()> + fn put_many_keyed<D, I>(&self, blocks: I) -> Result<(), Self::Error> where Self: Sized, D: AsRef<[u8]>, @@ -82,26 +83,28 @@ pub trait Blockstore { } pub trait Buffered: Blockstore { - fn flush(&self, root: &Cid) -> Result<()>; + fn flush(&self, root: &Cid) -> Result<(), Self::Error>; } impl<BS> Blockstore for &BS where BS: Blockstore, { - fn get(&self, k: &Cid) -> Result<Option<Vec<u8>>> { + type Error = BS::Error; + + fn get(&self, k: &Cid) -> Result<Option<Vec<u8>>, Self::Error> { (*self).get(k) } - fn put_keyed(&self, k: &Cid, block: &[u8]) -> Result<()> { + fn put_keyed(&self, k: &Cid, block: &[u8]) -> Result<(), Self::Error> { (*self).put_keyed(k, block) } - fn has(&self, k: &Cid) -> Result<bool> { + fn has(&self, k: &Cid) -> Result<bool, Self::Error> { (*self).has(k) } - fn put<D>(&self, mh_code: multihash::Code, block: &Block<D>) -> Result<Cid> + fn put<D>(&self, mh_code: multihash::Code, block: &Block<D>) -> Result<Cid, Self::Error> where Self: Sized, D: AsRef<[u8]>, @@ -109,7 +112,7 @@ where (*self).put(mh_code, block) } - fn put_many<D, I>(&self, blocks: I) -> Result<()> + fn put_many<D, I>(&self, blocks: I) -> Result<(), Self::Error> where Self: Sized, D: AsRef<[u8]>, @@ -118,7 +121,7 @@ where (*self).put_many(blocks) } - fn put_many_keyed<D, I>(&self, blocks: I) -> Result<()> + fn put_many_keyed<D, I>(&self, blocks: I) -> Result<(), Self::Error> where Self: Sized, D: AsRef<[u8]>, @@ -132,19 +135,21 @@ impl<BS> Blockstore for Rc<BS> where BS: Blockstore, { - fn get(&self, k: &Cid) -> Result<Option<Vec<u8>>> { + type Error = BS::Error; + + fn get(&self, k: &Cid) -> Result<Option<Vec<u8>>, Self::Error> { (**self).get(k) } - fn put_keyed(&self, k: &Cid, block: &[u8]) -> Result<()> { + fn put_keyed(&self, k: &Cid, block: &[u8]) -> Result<(), Self::Error> { (**self).put_keyed(k, block) } - fn has(&self, k: &Cid) -> Result<bool> { + fn has(&self, k: &Cid) -> Result<bool, Self::Error> { (**self).has(k) } - fn put<D>(&self, mh_code: multihash::Code, block: &Block<D>) -> Result<Cid> + fn put<D>(&self, mh_code: multihash::Code, block: &Block<D>) -> Result<Cid, Self::Error> where Self: Sized, D: AsRef<[u8]>, @@ -152,7 +157,7 @@ where (**self).put(mh_code, block) } - fn put_many<D, I>(&self, blocks: I) -> Result<()> + fn put_many<D, I>(&self, blocks: I) -> Result<(), Self::Error> where Self: Sized, D: AsRef<[u8]>, @@ -161,7 +166,7 @@ where (**self).put_many(blocks) } - fn put_many_keyed<D, I>(&self, blocks: I) -> Result<()> + fn put_many_keyed<D, I>(&self, blocks: I) -> Result<(), Self::Error> where Self: Sized, D: AsRef<[u8]>, diff --git a/ipld/blockstore/src/memory.rs b/ipld/blockstore/src/memory.rs index 54a3a306a..7bec1a8ed 100644 --- a/ipld/blockstore/src/memory.rs +++ b/ipld/blockstore/src/memory.rs @@ -1,7 +1,7 @@ use std::cell::RefCell; use std::collections::HashMap; +use std::convert::Infallible; -use anyhow::Result; use cid::Cid; use super::Blockstore; @@ -18,15 +18,17 @@ impl MemoryBlockstore { } impl Blockstore for MemoryBlockstore { - fn has(&self, k: &Cid) -> Result<bool> { + type Error = Infallible; + + fn has(&self, k: &Cid) -> Result<bool, Self::Error> { Ok(self.blocks.borrow().contains_key(k)) } - fn get(&self, k: &Cid) -> Result<Option<Vec<u8>>> { + fn get(&self, k: &Cid) -> Result<Option<Vec<u8>>, Self::Error> { Ok(self.blocks.borrow().get(k).cloned()) } - fn put_keyed(&self, k: &Cid, block: &[u8]) -> Result<()> { + fn put_keyed(&self, k: &Cid, block: &[u8]) -> Result<(), Self::Error> { self.blocks.borrow_mut().insert(*k, block.into()); Ok(()) } diff --git a/ipld/blockstore/src/tracking.rs b/ipld/blockstore/src/tracking.rs index 244028162..7ca01fdf5 100644 --- a/ipld/blockstore/src/tracking.rs +++ b/ipld/blockstore/src/tracking.rs @@ -2,7 +2,6 @@ use std::cell::RefCell; -use anyhow::Result; use cid::multihash::{self, Code}; use cid::Cid; @@ -46,7 +45,9 @@ impl<BS> Blockstore for TrackingBlockstore<BS> where BS: Blockstore, { - fn get(&self, cid: &Cid) -> Result<Option<Vec<u8>>> { + type Error = BS::Error; + + fn get(&self, cid: &Cid) -> Result<Option<Vec<u8>>, Self::Error> { let mut stats = self.stats.borrow_mut(); stats.r += 1; let bytes = self.base.get(cid)?; @@ -55,12 +56,13 @@ where } Ok(bytes) } - fn has(&self, cid: &Cid) -> Result<bool> { + + fn has(&self, cid: &Cid) -> Result<bool, Self::Error> { self.stats.borrow_mut().r += 1; self.base.has(cid) } - fn put<D>(&self, code: Code, block: &Block<D>) -> Result<Cid> + fn put<D>(&self, code: Code, block: &Block<D>) -> Result<Cid, Self::Error> where D: AsRef<[u8]>, { @@ -70,14 +72,14 @@ where self.base.put(code, block) } - fn put_keyed(&self, k: &Cid, block: &[u8]) -> Result<()> { + fn put_keyed(&self, k: &Cid, block: &[u8]) -> Result<(), Self::Error> { let mut stats = self.stats.borrow_mut(); stats.w += 1; stats.bw += block.len(); self.base.put_keyed(k, block) } - fn put_many<D, I>(&self, blocks: I) -> Result<()> + fn put_many<D, I>(&self, blocks: I) -> Result<(), Self::Error> where Self: Sized, D: AsRef<[u8]>, @@ -91,7 +93,7 @@ where Ok(()) } - fn put_many_keyed<D, I>(&self, blocks: I) -> Result<()> + fn put_many_keyed<D, I>(&self, blocks: I) -> Result<(), Self::Error> where Self: Sized, D: AsRef<[u8]>, diff --git a/ipld/encoding/Cargo.toml b/ipld/encoding/Cargo.toml index d98d1a1ee..bf875f76d 100644 --- a/ipld/encoding/Cargo.toml +++ b/ipld/encoding/Cargo.toml @@ -15,8 +15,7 @@ serde_ipld_dagcbor = "0.1.2" serde_tuple = "0.5" serde_repr = "0.1" cid = { version = "0.8.2", default-features = false, features = ["serde-codec", "std"] } -thiserror = "1.0" -anyhow = "1.0.56" +thiserror = "1.0.30" fvm_ipld_blockstore = { version = "0.1", path = "../blockstore" } [features] diff --git a/ipld/encoding/src/cbor_store.rs b/ipld/encoding/src/cbor_store.rs index bb2a1a82e..b2427530b 100644 --- a/ipld/encoding/src/cbor_store.rs +++ b/ipld/encoding/src/cbor_store.rs @@ -4,14 +4,22 @@ use serde::{de, ser}; use crate::DAG_CBOR; +#[derive(thiserror::Error, Debug)] +pub enum Error<BS: Blockstore> { + #[error("blockstore: {0}")] + Blockstore(BS::Error), + #[error("encoding: {0}")] + Encoding(#[from] crate::errors::Error), +} + /// Wrapper for database to handle inserting and retrieving ipld data with Cids pub trait CborStore: Blockstore + Sized { /// Get typed object from block store by Cid. - fn get_cbor<T>(&self, cid: &Cid) -> anyhow::Result<Option<T>> + fn get_cbor<T>(&self, cid: &Cid) -> Result<Option<T>, Error<Self>> where T: de::DeserializeOwned, { - match self.get(cid)? { + match self.get(cid).map_err(Error::Blockstore)? { Some(bz) => { let res = crate::from_slice(&bz)?; Ok(Some(res)) @@ -21,7 +29,7 @@ pub trait CborStore: Blockstore + Sized { } /// Put an object in the block store and return the Cid identifier. - fn put_cbor<S>(&self, obj: &S, code: multihash::Code) -> anyhow::Result<Cid> + fn put_cbor<S>(&self, obj: &S, code: multihash::Code) -> Result<Cid, Error<Self>> where S: ser::Serialize, { @@ -33,6 +41,7 @@ pub trait CborStore: Blockstore + Sized { data: &bytes, }, ) + .map_err(Error::Blockstore) } } diff --git a/ipld/encoding/src/lib.rs b/ipld/encoding/src/lib.rs index d3eb0cb31..876ab1581 100644 --- a/ipld/encoding/src/lib.rs +++ b/ipld/encoding/src/lib.rs @@ -13,7 +13,7 @@ pub use serde_bytes; pub use self::bytes::*; pub use self::cbor::*; -pub use self::cbor_store::CborStore; +pub use self::cbor_store::{CborStore, Error as CborStoreError}; pub use self::errors::*; pub use self::vec::*; diff --git a/ipld/hamt/Cargo.toml b/ipld/hamt/Cargo.toml index 672eec161..08a6f41e1 100644 --- a/ipld/hamt/Cargo.toml +++ b/ipld/hamt/Cargo.toml @@ -17,7 +17,6 @@ thiserror = "1.0" sha2 = "0.10" once_cell = "1.5" forest_hash_utils = "0.1" -anyhow = "1.0.51" libipld-core = { version = "0.13.1", features = ["serde-codec"] } fvm_ipld_encoding = { version = "0.1", path = "../encoding" } fvm_ipld_blockstore = { version = "0.1", path = "../blockstore" } diff --git a/ipld/hamt/src/error.rs b/ipld/hamt/src/error.rs index 0937e5824..8ec9a96db 100644 --- a/ipld/hamt/src/error.rs +++ b/ipld/hamt/src/error.rs @@ -3,12 +3,13 @@ use std::error::Error as StdError; -use fvm_ipld_encoding::Error as EncodingError; +use fvm_ipld_blockstore::Blockstore; +use fvm_ipld_encoding::{CborStore, CborStoreError, Error as EncodingError}; use thiserror::Error; /// HAMT Error #[derive(Debug, Error)] -pub enum Error { +pub enum Error<BS: Blockstore> { /// Maximum depth error #[error("Maximum depth reached")] MaxDepth, @@ -21,39 +22,8 @@ pub enum Error { /// Cid not found in store error #[error("Cid ({0}) did not match any in database")] CidNotFound(String), - // TODO: This should be something like "internal" or "io". And we shouldn't have both this and - // "other"; they serve the same purpose. - /// Dynamic error for when the error needs to be forwarded as is. - #[error("{0}")] - Dynamic(anyhow::Error), -} - -impl From<String> for Error { - fn from(e: String) -> Self { - Self::Dynamic(anyhow::anyhow!(e)) - } -} - -impl From<&'static str> for Error { - fn from(e: &'static str) -> Self { - Self::Dynamic(anyhow::anyhow!(e)) - } -} - -impl From<anyhow::Error> for Error { - fn from(e: anyhow::Error) -> Self { - e.downcast::<Error>().unwrap_or_else(Self::Dynamic) - } -} - -impl From<EncodingError> for Error { - fn from(e: EncodingError) -> Self { - Self::Dynamic(anyhow::anyhow!(e)) - } -} - -impl From<Box<dyn StdError + Send + Sync>> for Error { - fn from(e: Box<dyn StdError + Send + Sync>) -> Self { - Self::Dynamic(anyhow::anyhow!(e)) - } + #[error("blockstore {0}")] + Blockstore(BS::Error), + #[error("encoding error {0}")] + Encoding(#[from] EncodingError), } From bcc4fbe064cbb95dc62b68c985c16116efac97f1 Mon Sep 17 00:00:00 2001 From: dignifiedquire <me@dignifiedquire.com> Date: Wed, 6 Apr 2022 15:01:30 +0200 Subject: [PATCH 2/7] convert ipld/hamt --- ipld/hamt/src/error.rs | 45 ++++++++++++++++++++++++++++------- ipld/hamt/src/hamt.rs | 25 +++++++++---------- ipld/hamt/src/hash_bits.rs | 12 +++++----- ipld/hamt/src/lib.rs | 2 +- ipld/hamt/src/node.rs | 25 +++++++++---------- ipld/hamt/src/pointer.rs | 3 ++- ipld/hamt/tests/hamt_tests.rs | 6 ++--- 7 files changed, 74 insertions(+), 44 deletions(-) diff --git a/ipld/hamt/src/error.rs b/ipld/hamt/src/error.rs index 8ec9a96db..caf5c778d 100644 --- a/ipld/hamt/src/error.rs +++ b/ipld/hamt/src/error.rs @@ -1,21 +1,15 @@ // Copyright 2019-2022 ChainSafe Systems // SPDX-License-Identifier: Apache-2.0, MIT -use std::error::Error as StdError; - use fvm_ipld_blockstore::Blockstore; -use fvm_ipld_encoding::{CborStore, CborStoreError, Error as EncodingError}; +use fvm_ipld_encoding::{CborStoreError, Error as EncodingError}; use thiserror::Error; /// HAMT Error #[derive(Debug, Error)] pub enum Error<BS: Blockstore> { - /// Maximum depth error - #[error("Maximum depth reached")] - MaxDepth, - /// Hash bits does not support greater than 8 bit width - #[error("HashBits does not support retrieving more than 8 bits")] - InvalidHashBitLen, + #[error("hashbits: {0}")] + HashBits(#[from] HashBitsError), /// This should be treated as a fatal error, must have at least one pointer in node #[error("Invalid HAMT format, node cannot have 0 pointers")] ZeroPointers, @@ -27,3 +21,36 @@ pub enum Error<BS: Blockstore> { #[error("encoding error {0}")] Encoding(#[from] EncodingError), } + +impl<BS: Blockstore> From<CborStoreError<BS>> for Error<BS> { + fn from(err: CborStoreError<BS>) -> Self { + match err { + CborStoreError::Blockstore(err) => Error::Blockstore(err), + CborStoreError::Encoding(err) => Error::Encoding(err), + } + } +} + +#[derive(Debug, Error)] +pub enum EitherError<U, BS: Blockstore> { + #[error("user: {0}")] + User(U), + #[error("hamt: {0}")] + Hamt(#[from] Error<BS>), +} + +impl<U, BS: Blockstore> From<CborStoreError<BS>> for EitherError<U, BS> { + fn from(err: CborStoreError<BS>) -> Self { + EitherError::Hamt(err.into()) + } +} + +#[derive(Error, Debug)] +pub enum HashBitsError { + /// Maximum depth error + #[error("Maximum depth reached")] + MaxDepth, + /// Hash bits does not support greater than 8 bit width + #[error("HashBits does not support retrieving more than 8 bits")] + InvalidLen, +} diff --git a/ipld/hamt/src/hamt.rs b/ipld/hamt/src/hamt.rs index 48f91d653..2a3c626d6 100644 --- a/ipld/hamt/src/hamt.rs +++ b/ipld/hamt/src/hamt.rs @@ -12,6 +12,7 @@ use multihash::Code; use serde::de::DeserializeOwned; use serde::{Serialize, Serializer}; +use crate::error::EitherError; use crate::node::Node; use crate::{Error, Hash, HashAlgorithm, Sha256, DEFAULT_BIT_WIDTH}; @@ -82,12 +83,12 @@ where } /// Lazily instantiate a hamt from this root Cid. - pub fn load(cid: &Cid, store: BS) -> Result<Self, Error> { + pub fn load(cid: &Cid, store: BS) -> Result<Self, Error<BS>> { Self::load_with_bit_width(cid, store, DEFAULT_BIT_WIDTH) } /// Lazily instantiate a hamt from this root Cid with a specified bit width. - pub fn load_with_bit_width(cid: &Cid, store: BS, bit_width: u32) -> Result<Self, Error> { + pub fn load_with_bit_width(cid: &Cid, store: BS, bit_width: u32) -> Result<Self, Error<BS>> { match store.get_cbor(cid)? { Some(root) => Ok(Self { root, @@ -100,7 +101,7 @@ where } /// Sets the root based on the Cid of the root node using the Hamt store - pub fn set_root(&mut self, cid: &Cid) -> Result<(), Error> { + pub fn set_root(&mut self, cid: &Cid) -> Result<(), Error<BS>> { match self.store.get_cbor(cid)? { Some(root) => self.root = root, None => return Err(Error::CidNotFound(cid.to_string())), @@ -136,7 +137,7 @@ where /// map.set(37, "b".to_string()).unwrap(); /// map.set(37, "c".to_string()).unwrap(); /// ``` - pub fn set(&mut self, key: K, value: V) -> Result<Option<V>, Error> + pub fn set(&mut self, key: K, value: V) -> Result<Option<V>, Error<BS>> where V: PartialEq, { @@ -171,7 +172,7 @@ where /// let c = map.set_if_absent(30, "c".to_string()).unwrap(); /// assert_eq!(c, true); /// ``` - pub fn set_if_absent(&mut self, key: K, value: V) -> Result<bool, Error> + pub fn set_if_absent(&mut self, key: K, value: V) -> Result<bool, Error<BS>> where V: PartialEq, { @@ -200,7 +201,7 @@ where /// assert_eq!(map.get(&2).unwrap(), None); /// ``` #[inline] - pub fn get<Q: ?Sized>(&self, k: &Q) -> Result<Option<&V>, Error> + pub fn get<Q: ?Sized>(&self, k: &Q) -> Result<Option<&V>, Error<BS>> where K: Borrow<Q>, Q: Hash + Eq, @@ -232,7 +233,7 @@ where /// assert_eq!(map.contains_key(&2).unwrap(), false); /// ``` #[inline] - pub fn contains_key<Q: ?Sized>(&self, k: &Q) -> Result<bool, Error> + pub fn contains_key<Q: ?Sized>(&self, k: &Q) -> Result<bool, Error<BS>> where K: Borrow<Q>, Q: Hash + Eq, @@ -263,7 +264,7 @@ where /// assert_eq!(map.delete(&1).unwrap(), Some((1, "a".to_string()))); /// assert_eq!(map.delete(&1).unwrap(), None); /// ``` - pub fn delete<Q: ?Sized>(&mut self, k: &Q) -> Result<Option<(K, V)>, Error> + pub fn delete<Q: ?Sized>(&mut self, k: &Q) -> Result<Option<(K, V)>, Error<BS>> where K: Borrow<Q>, Q: Hash + Eq, @@ -273,7 +274,7 @@ where } /// Flush root and return Cid for hamt - pub fn flush(&mut self) -> Result<Cid, Error> { + pub fn flush(&mut self) -> Result<Cid, Error<BS>> { self.root.flush(self.store.borrow())?; Ok(self.store.put_cbor(&self.root, Code::Blake2b256)?) } @@ -301,15 +302,15 @@ where /// let mut total = 0; /// map.for_each(|_, v: &u64| { /// total += v; - /// Ok(()) + /// Ok::<(), ()>(()) /// }).unwrap(); /// assert_eq!(total, 3); /// ``` #[inline] - pub fn for_each<F>(&self, mut f: F) -> Result<(), Error> + pub fn for_each<F, U>(&self, mut f: F) -> Result<(), EitherError<U, BS>> where V: DeserializeOwned, - F: FnMut(&K, &V) -> anyhow::Result<()>, + F: FnMut(&K, &V) -> Result<(), U>, { self.root.for_each(self.store.borrow(), &mut f) } diff --git a/ipld/hamt/src/hash_bits.rs b/ipld/hamt/src/hash_bits.rs index c688270e2..3309d7001 100644 --- a/ipld/hamt/src/hash_bits.rs +++ b/ipld/hamt/src/hash_bits.rs @@ -3,7 +3,7 @@ use std::cmp::Ordering; -use crate::{Error, HashedKey}; +use crate::{HashBitsError, HashedKey}; /// Helper struct which indexes and allows returning bits from a hashed key #[derive(Debug, Clone, Copy)] @@ -32,12 +32,12 @@ impl<'a> HashBits<'a> { /// Returns next `i` bits of the hash and returns the value as an integer and returns /// Error when maximum depth is reached - pub fn next(&mut self, i: u32) -> Result<u32, Error> { + pub fn next(&mut self, i: u32) -> Result<u32, HashBitsError> { if i > 8 { - return Err(Error::InvalidHashBitLen); + return Err(HashBitsError::InvalidLen); } if (self.consumed + i) as usize > self.b.len() * 8 { - return Err(Error::MaxDepth); + return Err(HashBitsError::MaxDepth); } Ok(self.next_bits(i)) } @@ -94,11 +94,11 @@ mod tests { assert_eq!(hb.next(5).unwrap(), 0b01010); assert_eq!(hb.next(6).unwrap(), 0b111111); assert_eq!(hb.next(8).unwrap(), 0b11111111); - assert!(matches!(hb.next(9), Err(Error::InvalidHashBitLen))); + assert!(matches!(hb.next(9), Err(HashBitsError::InvalidLen))); for _ in 0..28 { // Iterate through rest of key to test depth hb.next(8).unwrap(); } - assert!(matches!(hb.next(1), Err(Error::MaxDepth))); + assert!(matches!(hb.next(1), Err(HashBitsError::MaxDepth))); } } diff --git a/ipld/hamt/src/lib.rs b/ipld/hamt/src/lib.rs index c8de66b44..9cfa7abf2 100644 --- a/ipld/hamt/src/lib.rs +++ b/ipld/hamt/src/lib.rs @@ -21,7 +21,7 @@ mod pointer; pub use forest_hash_utils::{BytesKey, Hash}; use serde::{Deserialize, Serialize}; -pub use self::error::Error; +pub use self::error::*; pub use self::hamt::Hamt; pub use self::hash::*; pub use self::hash_algorithm::*; diff --git a/ipld/hamt/src/node.rs b/ipld/hamt/src/node.rs index 03ee347ba..c5891c886 100644 --- a/ipld/hamt/src/node.rs +++ b/ipld/hamt/src/node.rs @@ -16,6 +16,7 @@ use super::bitfield::Bitfield; use super::hash_bits::HashBits; use super::pointer::Pointer; use super::{Error, Hash, HashAlgorithm, KeyValuePair, MAX_ARRAY_WIDTH}; +use crate::error::EitherError; /// Node in Hamt tree which contains bitfield of set indexes and pointers to nodes #[derive(Debug)] @@ -85,7 +86,7 @@ where store: &S, bit_width: u32, overwrite: bool, - ) -> Result<(Option<V>, bool), Error> + ) -> Result<(Option<V>, bool), Error<S>> where V: PartialEq, { @@ -107,7 +108,7 @@ where k: &Q, store: &S, bit_width: u32, - ) -> Result<Option<&V>, Error> + ) -> Result<Option<&V>, Error<S>> where K: Borrow<Q>, Q: Eq + Hash, @@ -121,7 +122,7 @@ where k: &Q, store: &S, bit_width: u32, - ) -> Result<Option<(K, V)>, Error> + ) -> Result<Option<(K, V)>, Error<S>> where K: Borrow<Q>, Q: Eq + Hash, @@ -135,9 +136,9 @@ where self.pointers.is_empty() } - pub(crate) fn for_each<S, F>(&self, store: &S, f: &mut F) -> Result<(), Error> + pub(crate) fn for_each<S, F, U>(&self, store: &S, f: &mut F) -> Result<(), EitherError<U, S>> where - F: FnMut(&K, &V) -> anyhow::Result<()>, + F: FnMut(&K, &V) -> Result<(), U>, S: Blockstore, { for p in &self.pointers { @@ -150,7 +151,7 @@ where node } else { #[cfg(not(feature = "ignore-dead-links"))] - return Err(Error::CidNotFound(cid.to_string())); + return Err(Error::CidNotFound(cid.to_string()).into()); #[cfg(feature = "ignore-dead-links")] continue; @@ -164,7 +165,7 @@ where Pointer::Dirty(n) => n.for_each(store, f)?, Pointer::Values(kvs) => { for kv in kvs { - f(kv.0.borrow(), kv.1.borrow())?; + f(kv.0.borrow(), kv.1.borrow()).map_err(EitherError::User)?; } } } @@ -178,7 +179,7 @@ where q: &Q, store: &S, bit_width: u32, - ) -> Result<Option<&KeyValuePair<K, V>>, Error> + ) -> Result<Option<&KeyValuePair<K, V>>, Error<S>> where K: Borrow<Q>, Q: Eq + Hash, @@ -194,7 +195,7 @@ where depth: u64, key: &Q, store: &S, - ) -> Result<Option<&KeyValuePair<K, V>>, Error> + ) -> Result<Option<&KeyValuePair<K, V>>, Error<S>> where K: Borrow<Q>, Q: Eq + Hash, @@ -244,7 +245,7 @@ where value: V, store: &S, overwrite: bool, - ) -> Result<(Option<V>, bool), Error> + ) -> Result<(Option<V>, bool), Error<S>> where V: PartialEq, { @@ -364,7 +365,7 @@ where depth: u64, key: &Q, store: &S, - ) -> Result<Option<(K, V)>, Error> + ) -> Result<Option<(K, V)>, Error<S>> where K: Borrow<Q>, Q: Hash + Eq, @@ -427,7 +428,7 @@ where } } - pub fn flush<S: Blockstore>(&mut self, store: &S) -> Result<(), Error> { + pub fn flush<S: Blockstore>(&mut self, store: &S) -> Result<(), Error<S>> { for pointer in &mut self.pointers { if let Pointer::Dirty(node) = pointer { // Flush cached sub node to clear it's cache diff --git a/ipld/hamt/src/pointer.rs b/ipld/hamt/src/pointer.rs index e024cefb1..999278dff 100644 --- a/ipld/hamt/src/pointer.rs +++ b/ipld/hamt/src/pointer.rs @@ -5,6 +5,7 @@ use std::cmp::Ordering; use std::convert::{TryFrom, TryInto}; use cid::Cid; +use fvm_ipld_blockstore::Blockstore; use libipld_core::ipld::Ipld; use once_cell::unsync::OnceCell; use serde::de::{self, DeserializeOwned}; @@ -111,7 +112,7 @@ where /// Internal method to cleanup children, to ensure consistent tree representation /// after deletes. - pub(crate) fn clean(&mut self) -> Result<(), Error> { + pub(crate) fn clean<BS: Blockstore>(&mut self) -> Result<(), Error<BS>> { match self { Pointer::Dirty(n) => match n.pointers.len() { 0 => Err(Error::ZeroPointers), diff --git a/ipld/hamt/tests/hamt_tests.rs b/ipld/hamt/tests/hamt_tests.rs index 4961663a2..b67c70ff2 100644 --- a/ipld/hamt/tests/hamt_tests.rs +++ b/ipld/hamt/tests/hamt_tests.rs @@ -273,7 +273,7 @@ fn for_each() { hamt.for_each(|k, v| { assert_eq!(k, v); count += 1; - Ok(()) + Ok::<(), ()>(()) }) .unwrap(); assert_eq!(count, 200); @@ -291,7 +291,7 @@ fn for_each() { hamt.for_each(|k, v| { assert_eq!(k, v); count += 1; - Ok(()) + Ok::<(), ()>(()) }) .unwrap(); assert_eq!(count, 200); @@ -301,7 +301,7 @@ fn for_each() { hamt.for_each(|k, v| { assert_eq!(k, v); count += 1; - Ok(()) + Ok::<(), ()>(()) }) .unwrap(); assert_eq!(count, 200); From 6a0a7508faf1be206457fa5ed4d80157dbd8148f Mon Sep 17 00:00:00 2001 From: dignifiedquire <me@dignifiedquire.com> Date: Wed, 6 Apr 2022 15:24:26 +0200 Subject: [PATCH 3/7] convert ipld/amt --- ipld/amt/Cargo.toml | 1 - ipld/amt/src/amt.rs | 41 +++++++------ ipld/amt/src/error.rs | 76 ++++++++++++++---------- ipld/amt/src/node.rs | 112 ++++++++++++++++++++---------------- ipld/amt/tests/amt_tests.rs | 8 +-- 5 files changed, 132 insertions(+), 106 deletions(-) diff --git a/ipld/amt/Cargo.toml b/ipld/amt/Cargo.toml index 0e6a72355..20bf742cd 100644 --- a/ipld/amt/Cargo.toml +++ b/ipld/amt/Cargo.toml @@ -14,7 +14,6 @@ thiserror = "1.0" once_cell = "1.5" ahash = { version = "0.7", optional = true } itertools = "0.10" -anyhow = "1.0.51" fvm_ipld_blockstore = { version = "0.1", path = "../blockstore" } fvm_ipld_encoding = { version = "0.1", path = "../encoding" } diff --git a/ipld/amt/src/amt.rs b/ipld/amt/src/amt.rs index 0a0b83c2a..8dc4ffa0b 100644 --- a/ipld/amt/src/amt.rs +++ b/ipld/amt/src/amt.rs @@ -1,7 +1,6 @@ // Copyright 2019-2022 ChainSafe Systems // SPDX-License-Identifier: Apache-2.0, MIT -use anyhow::anyhow; use cid::multihash::Code; use cid::Cid; use fvm_ipld_blockstore::Blockstore; @@ -11,6 +10,7 @@ use fvm_ipld_encoding::CborStore; use itertools::sorted; use super::ValueMut; +use crate::error::EitherError; use crate::node::{CollapsedNode, Link}; use crate::{ init_sized_vec, nodes_for_height, Error, Node, Root, DEFAULT_BIT_WIDTH, MAX_HEIGHT, MAX_INDEX, @@ -72,7 +72,7 @@ where } /// Constructs an AMT with a blockstore and a Cid of the root of the AMT - pub fn load(cid: &Cid, block_store: BS) -> Result<Self, Error> { + pub fn load(cid: &Cid, block_store: BS) -> Result<Self, Error<BS>> { // Load root bytes from database let root: Root<V> = block_store .get_cbor(cid)? @@ -97,7 +97,10 @@ where } /// Generates an AMT with block store and array of cbor marshallable objects and returns Cid - pub fn new_from_iter(block_store: BS, vals: impl IntoIterator<Item = V>) -> Result<Cid, Error> { + pub fn new_from_iter( + block_store: BS, + vals: impl IntoIterator<Item = V>, + ) -> Result<Cid, Error<BS>> { let mut t = Self::new(block_store); t.batch_set(vals)?; @@ -106,7 +109,7 @@ where } /// Get value at index of AMT - pub fn get(&self, i: u64) -> Result<Option<&V>, Error> { + pub fn get(&self, i: u64) -> Result<Option<&V>, Error<BS>> { if i > MAX_INDEX { return Err(Error::OutOfRange(i)); } @@ -121,7 +124,7 @@ where } /// Set value at index - pub fn set(&mut self, i: u64, val: V) -> Result<(), Error> { + pub fn set(&mut self, i: u64, val: V) -> Result<(), Error<BS>> { if i > MAX_INDEX { return Err(Error::OutOfRange(i)); } @@ -163,7 +166,7 @@ where /// Batch set (naive for now) // TODO Implement more efficient batch set to not have to traverse tree and keep cache for each - pub fn batch_set(&mut self, vals: impl IntoIterator<Item = V>) -> Result<(), Error> { + pub fn batch_set(&mut self, vals: impl IntoIterator<Item = V>) -> Result<(), Error<BS>> { for (i, val) in (0u64..).zip(vals) { self.set(i, val)?; } @@ -172,7 +175,7 @@ where } /// Delete item from AMT at index - pub fn delete(&mut self, i: u64) -> Result<Option<V>, Error> { + pub fn delete(&mut self, i: u64) -> Result<Option<V>, Error<BS>> { if i > MAX_INDEX { return Err(Error::OutOfRange(i)); } @@ -243,7 +246,7 @@ where &mut self, iter: impl IntoIterator<Item = u64>, strict: bool, - ) -> Result<bool, Error> { + ) -> Result<bool, Error<BS>> { // TODO: optimize this let mut modified = false; @@ -251,7 +254,7 @@ where for i in sorted(iter) { let found = self.delete(i)?.is_none(); if strict && found { - return Err(anyhow!("no such index {} in Amt for batch delete", i).into()); + return Err(Error::BatchDelteNotFound(i)); } modified |= found; } @@ -259,7 +262,7 @@ where } /// flush root and return Cid used as key in block store - pub fn flush(&mut self) -> Result<Cid, Error> { + pub fn flush(&mut self) -> Result<Cid, Error<BS>> { self.root.node.flush(&self.block_store)?; Ok(self.block_store.put_cbor(&self.root, Code::Blake2b256)?) } @@ -283,14 +286,14 @@ where /// let mut values: Vec<(u64, String)> = Vec::new(); /// map.for_each(|i, v| { /// values.push((i, v.clone())); - /// Ok(()) + /// Ok::<_, ()>(()) /// }).unwrap(); /// assert_eq!(&values, &[(1, "One".to_owned()), (4, "Four".to_owned())]); /// ``` #[inline] - pub fn for_each<F>(&self, mut f: F) -> Result<(), Error> + pub fn for_each<F, U>(&self, mut f: F) -> Result<(), EitherError<U, BS>> where - F: FnMut(u64, &V) -> anyhow::Result<()>, + F: FnMut(u64, &V) -> Result<(), U>, { self.for_each_while(|i, x| { f(i, x)?; @@ -300,9 +303,9 @@ where /// Iterates over each value in the Amt and runs a function on the values, for as long as that /// function keeps returning `true`. - pub fn for_each_while<F>(&self, mut f: F) -> Result<(), Error> + pub fn for_each_while<F, U>(&self, mut f: F) -> Result<(), EitherError<U, BS>> where - F: FnMut(u64, &V) -> anyhow::Result<bool>, + F: FnMut(u64, &V) -> Result<bool, U>, { self.root .node @@ -318,10 +321,10 @@ where /// Iterates over each value in the Amt and runs a function on the values that allows modifying /// each value. - pub fn for_each_mut<F>(&mut self, mut f: F) -> Result<(), Error> + pub fn for_each_mut<F, U>(&mut self, mut f: F) -> Result<(), EitherError<U, BS>> where V: Clone, - F: FnMut(u64, &mut ValueMut<'_, V>) -> anyhow::Result<()>, + F: FnMut(u64, &mut ValueMut<'_, V>) -> Result<(), U>, { self.for_each_while_mut(|i, x| { f(i, x)?; @@ -331,12 +334,12 @@ where /// Iterates over each value in the Amt and runs a function on the values that allows modifying /// each value, for as long as that function keeps returning `true`. - pub fn for_each_while_mut<F>(&mut self, mut f: F) -> Result<(), Error> + pub fn for_each_while_mut<F, U>(&mut self, mut f: F) -> Result<(), EitherError<U, BS>> where // TODO remove clone bound when go-interop doesn't require it. // (If needed without, this bound can be removed by duplicating function signatures) V: Clone, - F: FnMut(u64, &mut ValueMut<'_, V>) -> anyhow::Result<bool>, + F: FnMut(u64, &mut ValueMut<'_, V>) -> Result<bool, U>, { #[cfg(not(feature = "go-interop"))] { diff --git a/ipld/amt/src/error.rs b/ipld/amt/src/error.rs index 80d5ebd51..35231e8e3 100644 --- a/ipld/amt/src/error.rs +++ b/ipld/amt/src/error.rs @@ -1,16 +1,14 @@ // Copyright 2019-2022 ChainSafe Systems // SPDX-License-Identifier: Apache-2.0, MIT -use std::error::Error as StdError; - -use anyhow::anyhow; use cid::Error as CidError; -use fvm_ipld_encoding::Error as EncodingError; +use fvm_ipld_blockstore::Blockstore; +use fvm_ipld_encoding::{CborStoreError, Error as EncodingError}; use thiserror::Error; /// AMT Error #[derive(Debug, Error)] -pub enum Error { +pub enum Error<BS: Blockstore> { /// Index referenced it above arbitrary max set #[error("index {0} out of range for the amt")] OutOfRange(u64), @@ -20,49 +18,65 @@ pub enum Error { /// Error generating a Cid for data #[error(transparent)] Cid(#[from] CidError), - /// Error when trying to serialize an AMT without a flushed cache - #[error("Tried to serialize without saving cache, run flush() on Amt before serializing")] - Cached, /// Serialized vector less than number of bits set #[error("Vector length does not match bitmap")] InvalidVecLength, - /// Invalid formatted serialized node. - #[error("Serialized node cannot contain both links and values")] - LinksAndValues, /// Cid not found in store error #[error("Cid ({0}) did not match any in database")] CidNotFound(String), - /// Dynamic error for when the error needs to be forwarded as is. #[error("{0}")] - Dynamic(anyhow::Error), + CollapsedNode(#[from] CollapsedNodeError), + #[error("no such index {0} in Amt for batch delete")] + BatchDelteNotFound(u64), + #[error("blockstore {0}")] + Blockstore(BS::Error), + #[error("encoding error {0}")] + Encoding(#[from] EncodingError), } -impl From<String> for Error { - fn from(e: String) -> Self { - Self::Dynamic(anyhow::anyhow!(e)) +impl<BS: Blockstore> From<CborStoreError<BS>> for Error<BS> { + fn from(err: CborStoreError<BS>) -> Self { + match err { + CborStoreError::Blockstore(err) => Error::Blockstore(err), + CborStoreError::Encoding(err) => Error::Encoding(err), + } } } -impl From<&'static str> for Error { - fn from(e: &'static str) -> Self { - Self::Dynamic(anyhow::anyhow!(e)) - } +#[derive(Debug, Error)] +pub enum EitherError<U, BS: Blockstore> { + #[error("user: {0}")] + User(U), + #[error("hamt: {0}")] + Hamt(#[from] Error<BS>), } -impl From<anyhow::Error> for Error { - fn from(e: anyhow::Error) -> Self { - e.downcast::<Error>().unwrap_or_else(Self::Dynamic) +impl<U, BS: Blockstore> From<CborStoreError<BS>> for EitherError<U, BS> { + fn from(err: CborStoreError<BS>) -> Self { + EitherError::Hamt(err.into()) } } -impl From<EncodingError> for Error { - fn from(e: EncodingError) -> Self { - Self::Dynamic(anyhow!(e)) - } +#[derive(Debug, Error)] +pub enum CollapsedNodeError { + #[error("expected bitfield of length {0}, found bitfield with length {1}")] + LengthMissmatch(usize, usize), + #[error("Bitmap contained more set bits than links provided")] + MoreBitsThanLinks, + #[error("Bitmap contained less set bits than links provided")] + LessBitsThanLinks, + #[error("Bitmap contained more set bits than values provided")] + MoreBitsThanValues, + #[error("Bitmap contained less set bits than values provided")] + LessBitsThanValues, + /// Invalid formatted serialized node. + #[error("Serialized node cannot contain both links and values")] + LinksAndValues, } -impl From<Box<dyn StdError + Send + Sync>> for Error { - fn from(e: Box<dyn StdError + Send + Sync>) -> Self { - Self::Dynamic(anyhow!(e)) - } +#[derive(Debug, Error)] +pub enum SerdeError { + /// Error when trying to serialize an AMT without a flushed cache + #[error("Tried to serialize without saving cache, run flush() on Amt before serializing")] + Cached, } diff --git a/ipld/amt/src/node.rs b/ipld/amt/src/node.rs index d57199de4..f63586ce3 100644 --- a/ipld/amt/src/node.rs +++ b/ipld/amt/src/node.rs @@ -3,7 +3,6 @@ use std::convert::{TryFrom, TryInto}; -use anyhow::anyhow; use cid::multihash::Code; use cid::Cid; use fvm_ipld_blockstore::Blockstore; @@ -13,6 +12,7 @@ use serde::de::{self, DeserializeOwned}; use serde::{ser, Deserialize, Serialize}; use super::ValueMut; +use crate::error::{CollapsedNodeError, EitherError, SerdeError}; use crate::{bmap_bytes, init_sized_vec, nodes_for_height, Error}; /// This represents a link to another Node @@ -106,7 +106,7 @@ where collapsed.push(cid); bmap[i / 8] |= 1 << (i % 8); } else { - return Err(ser::Error::custom(Error::Cached)); + return Err(ser::Error::custom(SerdeError::Cached)); } } } @@ -120,19 +120,17 @@ where pub(crate) struct CollapsedNode<V>(#[serde(with = "serde_bytes")] Vec<u8>, Vec<Cid>, Vec<V>); impl<V> CollapsedNode<V> { - pub(crate) fn expand(self, bit_width: u32) -> Result<Node<V>, Error> { + pub(crate) fn expand(self, bit_width: u32) -> Result<Node<V>, CollapsedNodeError> { let CollapsedNode(bmap, links, values) = self; if !links.is_empty() && !values.is_empty() { - return Err(Error::LinksAndValues); + return Err(CollapsedNodeError::LinksAndValues); } if bmap_bytes(bit_width) != bmap.len() { - return Err(anyhow!( - "expected bitfield of length {}, found bitfield with length {}", + return Err(CollapsedNodeError::LengthMissmatch( bmap_bytes(bit_width), - bmap.len() - ) - .into()); + bmap.len(), + )); } if !links.is_empty() { @@ -140,13 +138,15 @@ impl<V> CollapsedNode<V> { let mut links = init_sized_vec::<Link<V>>(bit_width); for (i, v) in links.iter_mut().enumerate() { if bmap[i / 8] & (1 << (i % 8)) != 0 { - *v = Some(Link::from(links_iter.next().ok_or_else(|| { - anyhow!("Bitmap contained more set bits than links provided",) - })?)) + *v = Some(Link::from( + links_iter + .next() + .ok_or_else(|| CollapsedNodeError::MoreBitsThanLinks)?, + )) } } if links_iter.next().is_some() { - return Err(anyhow!("Bitmap contained less set bits than links provided",).into()); + return Err(CollapsedNodeError::LessBitsThanLinks); } Ok(Node::Link { links }) } else { @@ -154,13 +154,15 @@ impl<V> CollapsedNode<V> { let mut vals = init_sized_vec::<V>(bit_width); for (i, v) in vals.iter_mut().enumerate() { if bmap[i / 8] & (1 << (i % 8)) != 0 { - *v = Some(val_iter.next().ok_or_else(|| { - anyhow!("Bitmap contained more set bits than values provided") - })?) + *v = Some( + val_iter + .next() + .ok_or_else(|| CollapsedNodeError::MoreBitsThanValues)?, + ) } } if val_iter.next().is_some() { - return Err(anyhow!("Bitmap contained less set bits than values provided").into()); + return Err(CollapsedNodeError::LessBitsThanValues); } Ok(Node::Leaf { vals }) } @@ -180,7 +182,7 @@ where } /// Flushes cache for node, replacing any cached values with a Cid variant - pub(super) fn flush<DB: Blockstore>(&mut self, bs: &DB) -> Result<(), Error> { + pub(super) fn flush<DB: Blockstore>(&mut self, bs: &DB) -> Result<(), Error<DB>> { if let Node::Link { links } = self { for link in links.iter_mut().flatten() { // links should only be flushed if the bitmap is set. @@ -235,20 +237,22 @@ where height: u32, bit_width: u32, i: u64, - ) -> Result<Option<&V>, Error> { + ) -> Result<Option<&V>, Error<DB>> { match self { Node::Leaf { vals, .. } => Ok(vals.get(i as usize).and_then(|v| v.as_ref())), Node::Link { links, .. } => { let sub_i: usize = (i / nodes_for_height(bit_width, height)) .try_into() .unwrap(); - match links.get(sub_i).and_then(|v| v.as_ref()) { - Some(Link::Cid { cid, cache }) => { - let cached_node = cache.get_or_try_init(|| { - bs.get_cbor::<CollapsedNode<V>>(cid)? + + match links.get(sub_i) { + Some(Some(Link::Cid { cid, cache })) => { + let cached_node = cache.get_or_try_init(|| -> Result<_, Error<DB>> { + let node = bs + .get_cbor::<CollapsedNode<V>>(cid)? .ok_or_else(|| Error::CidNotFound(cid.to_string()))? - .expand(bit_width) - .map(Box::new) + .expand(bit_width)?; + Ok(Box::new(node)) })?; cached_node.get( @@ -258,13 +262,13 @@ where i % nodes_for_height(bit_width, height), ) } - Some(Link::Dirty(n)) => n.get( + Some(Some(Link::Dirty(n))) => n.get( bs, height - 1, bit_width, i % nodes_for_height(bit_width, height), ), - None => Ok(None), + Some(None) | None => Ok(None), } } } @@ -278,7 +282,7 @@ where bit_width: u32, i: u64, val: V, - ) -> Result<Option<V>, Error> { + ) -> Result<Option<V>, Error<DB>> { if height == 0 { return Ok(self.set_leaf(i, val)); } @@ -350,7 +354,7 @@ where height: u32, bit_width: u32, i: u64, - ) -> Result<Option<V>, Error> { + ) -> Result<Option<V>, Error<DB>> { match self { Self::Leaf { vals } => Ok(vals .get_mut(usize::try_from(i).unwrap()) @@ -381,11 +385,13 @@ where } Some(Link::Cid { cid, cache }) => { // Take cache, will be replaced if no nodes deleted - cache.get_or_try_init(|| { - bs.get_cbor::<CollapsedNode<V>>(cid)? + cache.get_or_try_init(|| -> Result<_, Error<DB>> { + let node = bs + .get_cbor::<CollapsedNode<V>>(cid)? .ok_or_else(|| Error::CidNotFound(cid.to_string()))? - .expand(bit_width) - .map(Box::new) + .expand(bit_width)?; + + Ok(Box::new(node)) })?; let sub_node = cache.get_mut().expect("filled line above"); let deleted = sub_node.delete( @@ -419,23 +425,23 @@ where } } - pub(super) fn for_each_while<S, F>( + pub(super) fn for_each_while<S, F, U>( &self, bs: &S, height: u32, bit_width: u32, offset: u64, f: &mut F, - ) -> Result<bool, Error> + ) -> Result<bool, EitherError<U, S>> where - F: FnMut(u64, &V) -> anyhow::Result<bool>, + F: FnMut(u64, &V) -> Result<bool, U>, S: Blockstore, { match self { Node::Leaf { vals } => { for (i, v) in (0..).zip(vals.iter()) { if let Some(v) = v { - let keep_going = f(offset + i, v)?; + let keep_going = f(offset + i, v).map_err(EitherError::User)?; if !keep_going { return Ok(false); @@ -452,12 +458,14 @@ where sub.for_each_while(bs, height - 1, bit_width, offs, f)? } Link::Cid { cid, cache } => { - let cached_node = cache.get_or_try_init(|| { - bs.get_cbor::<CollapsedNode<V>>(cid)? - .ok_or_else(|| Error::CidNotFound(cid.to_string()))? - .expand(bit_width) - .map(Box::new) - })?; + let cached_node = + cache.get_or_try_init(|| -> Result<_, Error<S>> { + let node = bs + .get_cbor::<CollapsedNode<V>>(cid)? + .ok_or_else(|| Error::CidNotFound(cid.to_string()))? + .expand(bit_width)?; + Ok(Box::new(node)) + })?; cached_node.for_each_while(bs, height - 1, bit_width, offs, f)? } @@ -478,16 +486,16 @@ where /// a closure call returned `Ok(false)`, indicating that a `break` has happened. /// `did_mutate` will be `true` iff any of the values in the node was actually /// mutated inside the closure, requiring the node to be cached. - pub(super) fn for_each_while_mut<S, F>( + pub(super) fn for_each_while_mut<S, F, U>( &mut self, bs: &S, height: u32, bit_width: u32, offset: u64, f: &mut F, - ) -> Result<(bool, bool), Error> + ) -> Result<(bool, bool), EitherError<U, S>> where - F: FnMut(u64, &mut ValueMut<'_, V>) -> anyhow::Result<bool>, + F: FnMut(u64, &mut ValueMut<'_, V>) -> Result<bool, U>, S: Blockstore, { let mut did_mutate = false; @@ -498,7 +506,8 @@ where if let Some(v) = v { let mut value_mut = ValueMut::new(v); - let keep_going = f(offset + i, &mut value_mut)?; + let keep_going = + f(offset + i, &mut value_mut).map_err(EitherError::User)?; did_mutate |= value_mut.value_changed(); if !keep_going { @@ -516,11 +525,12 @@ where sub.for_each_while_mut(bs, height - 1, bit_width, offs, f)? } Link::Cid { cid, cache } => { - cache.get_or_try_init(|| { - bs.get_cbor::<CollapsedNode<V>>(cid)? + cache.get_or_try_init(|| -> Result<_, Error<S>> { + let node = bs + .get_cbor::<CollapsedNode<V>>(cid)? .ok_or_else(|| Error::CidNotFound(cid.to_string()))? - .expand(bit_width) - .map(Box::new) + .expand(bit_width)?; + Ok(Box::new(node)) })?; let node = cache.get_mut().expect("cache filled on line above"); diff --git a/ipld/amt/tests/amt_tests.rs b/ipld/amt/tests/amt_tests.rs index a5263c1f5..4eb82c8a8 100644 --- a/ipld/amt/tests/amt_tests.rs +++ b/ipld/amt/tests/amt_tests.rs @@ -322,7 +322,7 @@ fn for_each() { let mut x = 0; a.for_each(|_, _: &BytesDe| { x += 1; - Ok(()) + Ok::<(), ()>(()) }) .unwrap(); @@ -343,13 +343,13 @@ fn for_each() { ); } x += 1; - Ok(()) + Ok::<(), ()>(()) }) .unwrap(); assert_eq!(x, indexes.len()); // Iteration again will be read diff with go-interop, since they do not cache - new_amt.for_each(|_, _: &BytesDe| Ok(())).unwrap(); + new_amt.for_each(|_, _: &BytesDe| Ok::<(), ()>(())).unwrap(); assert_eq!( c.to_string().as_str(), @@ -385,7 +385,7 @@ fn for_each_mutate() { // Value it's set to doesn't matter, just cloning for expedience **v = v.clone(); } - Ok(()) + Ok::<(), ()>(()) }) .unwrap(); From 4480da3911d0730bce34dc1ebae7da77930d192f Mon Sep 17 00:00:00 2001 From: dignifiedquire <me@dignifiedquire.com> Date: Wed, 6 Apr 2022 15:32:28 +0200 Subject: [PATCH 4/7] convert shared --- ipld/blockstore/src/lib.rs | 2 +- shared/Cargo.toml | 1 - shared/src/actor/builtin.rs | 39 ++++++++++++++++++++++++++++--------- shared/src/message.rs | 15 ++++++++++---- 4 files changed, 42 insertions(+), 15 deletions(-) diff --git a/ipld/blockstore/src/lib.rs b/ipld/blockstore/src/lib.rs index 3e3bc292a..b77872f89 100644 --- a/ipld/blockstore/src/lib.rs +++ b/ipld/blockstore/src/lib.rs @@ -13,7 +13,7 @@ pub use block::*; /// An IPLD blockstore suitable for injection into the FVM. /// /// The cgo blockstore adapter implements this trait. -pub trait Blockstore { +pub trait Blockstore: std::fmt::Debug { type Error: std::error::Error + std::fmt::Debug; /// Gets the block from the blockstore. diff --git a/shared/Cargo.toml b/shared/Cargo.toml index 3e900bb82..6ef5acfe0 100644 --- a/shared/Cargo.toml +++ b/shared/Cargo.toml @@ -23,7 +23,6 @@ log = "0.4.8" cid = { version = "0.8.2", default-features = false, features = ["serde-codec", "std"] } multihash = { version = "0.16.1", default-features = false, features = ["blake2b", "multihash-impl"] } unsigned-varint = "0.7.1" -anyhow = "1.0.51" bimap = { version = "0.6.2", features = ["serde"] } fvm_ipld_blockstore = { version = "0.1", path = "../ipld/blockstore" } fvm_ipld_encoding = { version = "0.1", path = "../ipld/encoding" } diff --git a/shared/src/actor/builtin.rs b/shared/src/actor/builtin.rs index bf4260281..e58cd80a4 100644 --- a/shared/src/actor/builtin.rs +++ b/shared/src/actor/builtin.rs @@ -1,10 +1,9 @@ use std::fmt::{Debug, Display, Formatter}; -use anyhow::anyhow; use bimap::BiBTreeMap; use cid::Cid; use fvm_ipld_blockstore::Blockstore; -use fvm_ipld_encoding::CborStore; +use fvm_ipld_encoding::{CborStore, CborStoreError}; use num_derive::FromPrimitive; use serde_repr::{Deserialize_repr, Serialize_repr}; @@ -115,26 +114,36 @@ impl Display for Type { /// A mapping of builtin actor CIDs to their respective types. pub type Manifest = BiBTreeMap<Cid, Type>; -pub fn load_manifest<B: Blockstore>(bs: &B, root_cid: &Cid, ver: u32) -> anyhow::Result<Manifest> { +pub fn load_manifest<B: Blockstore>( + bs: &B, + root_cid: &Cid, + ver: u32, +) -> Result<Manifest, ManifestError<B>> { match ver { 0 => load_manifest_v0(bs, root_cid), 1 => load_manifest_v1(bs, root_cid), - _ => Err(anyhow!("unknown manifest version {}", ver)), + _ => Err(ManifestError::UnknownVersion(ver)), } } -pub fn load_manifest_v0<B: Blockstore>(bs: &B, root_cid: &Cid) -> anyhow::Result<Manifest> { +pub fn load_manifest_v0<B: Blockstore>( + bs: &B, + root_cid: &Cid, +) -> Result<Manifest, ManifestError<B>> { match bs.get_cbor::<Manifest>(root_cid)? { Some(mf) => Ok(mf), - None => Err(anyhow!("cannot find manifest root cid {}", root_cid)), + None => Err(ManifestError::MissingRootCid(*root_cid)), } } -pub fn load_manifest_v1<B: Blockstore>(bs: &B, root_cid: &Cid) -> anyhow::Result<Manifest> { +pub fn load_manifest_v1<B: Blockstore>( + bs: &B, + root_cid: &Cid, +) -> Result<Manifest, ManifestError<B>> { let vec: Vec<(String, Cid)> = match bs.get_cbor(root_cid)? { Some(vec) => vec, None => { - return Err(anyhow!("cannot find manifest root cid {}", root_cid)); + return Err(ManifestError::MissingRootCid(*root_cid)); } }; let mut manifest = Manifest::new(); @@ -145,9 +154,21 @@ pub fn load_manifest_v1<B: Blockstore>(bs: &B, root_cid: &Cid) -> anyhow::Result manifest.insert(code_cid, t); } Err(what) => { - return Err(anyhow!("bad builtin actor name: {}: {} ", name, what)); + return Err(ManifestError::BadBuiltinActor(name, what)); } } } Ok(manifest) } + +#[derive(thiserror::Error, Debug)] +pub enum ManifestError<BS: Blockstore> { + #[error("unknown manifest version {0}")] + UnknownVersion(u32), + #[error("cannot find manifest root cid {0}")] + MissingRootCid(Cid), + #[error("bad builtin actor name: {0}: {1}")] + BadBuiltinActor(String, String), + #[error("encoding {0}")] + Encoding(#[from] CborStoreError<BS>), +} diff --git a/shared/src/message.rs b/shared/src/message.rs index f07333334..3dd2cded7 100644 --- a/shared/src/message.rs +++ b/shared/src/message.rs @@ -1,7 +1,6 @@ // Copyright 2019-2022 ChainSafe Systems // SPDX-License-Identifier: Apache-2.0, MIT -use anyhow::anyhow; use fvm_ipld_encoding::de::{Deserialize, Deserializer}; use fvm_ipld_encoding::ser::{Serialize, Serializer}; use fvm_ipld_encoding::{Cbor, RawBytes}; @@ -38,17 +37,25 @@ impl Message { } /// Does some basic checks on the Message to see if the fields are valid. - pub fn check(self: &Message) -> anyhow::Result<()> { + pub fn check(self: &Message) -> Result<(), MessageError> { if self.gas_limit == 0 { - return Err(anyhow!("Message has no gas limit set")); + return Err(MessageError::MissingGasLimit); } if self.gas_limit < 0 { - return Err(anyhow!("Message has negative gas limit")); + return Err(MessageError::NegativeGasLimit); } Ok(()) } } +#[derive(thiserror::Error, Debug)] +pub enum MessageError { + #[error("Message has no gas limit set")] + MissingGasLimit, + #[error("Message has negative gas limit")] + NegativeGasLimit, +} + impl Serialize for Message { fn serialize<S>(&self, s: S) -> std::result::Result<S::Ok, S::Error> where From 8b9dc8f73a2bf10f966e07da2683bc1ad58f43f2 Mon Sep 17 00:00:00 2001 From: dignifiedquire <me@dignifiedquire.com> Date: Wed, 6 Apr 2022 16:06:46 +0200 Subject: [PATCH 5/7] convert fvm and update type bounds accordingly --- fvm/src/blockstore/buffered.rs | 94 +++++++++++++++++++---------- ipld/amt/benches/amt_benchmark.rs | 4 +- ipld/amt/src/amt.rs | 24 ++++---- ipld/amt/src/error.rs | 17 +++--- ipld/amt/src/node.rs | 37 ++++++------ ipld/blockstore/src/lib.rs | 4 +- ipld/encoding/src/cbor_store.rs | 8 +-- ipld/hamt/benches/hamt_benchmark.rs | 4 +- ipld/hamt/src/error.rs | 17 +++--- ipld/hamt/src/hamt.rs | 24 +++++--- ipld/hamt/src/node.rs | 26 ++++---- ipld/hamt/src/pointer.rs | 2 +- shared/src/actor/builtin.rs | 10 +-- 13 files changed, 156 insertions(+), 115 deletions(-) diff --git a/fvm/src/blockstore/buffered.rs b/fvm/src/blockstore/buffered.rs index 29049ac0c..a80b112ac 100644 --- a/fvm/src/blockstore/buffered.rs +++ b/fvm/src/blockstore/buffered.rs @@ -1,7 +1,6 @@ // Copyright 2019-2022 ChainSafe Systems // SPDX-License-Identifier: Apache-2.0, MIT -use anyhow::{anyhow, Result}; use byteorder::{BigEndian, ByteOrder, ReadBytesExt}; use cid::Cid; use fvm_ipld_blockstore::{Blockstore, Buffered}; @@ -38,6 +37,14 @@ where } } +#[derive(thiserror::Error, Debug)] +pub enum Error<E> { + #[error("flush: {0}")] + Flush(#[from] FlushError), + #[error("blockstore: {0}")] + Blockstore(E), +} + impl<BS> Buffered for BufferedBlockstore<BS> where BS: Blockstore, @@ -45,18 +52,46 @@ where /// Flushes the buffered cache based on the root node. /// This will recursively traverse the cache and write all data connected by links to this /// root Cid. - fn flush(&self, root: &Cid) -> Result<()> { + fn flush(&self, root: &Cid) -> Result<(), Error<BS::Error>> { let mut buffer = Vec::new(); let mut s = self.write.borrow_mut(); copy_rec(&s, *root, &mut buffer)?; - self.base.put_many_keyed(buffer)?; + self.base + .put_many_keyed(buffer) + .map_err(Error::Blockstore)?; *s = Default::default(); Ok(()) } } +#[derive(thiserror::Error, Debug)] +pub enum FlushError { + #[error("io: {0}")] + Io(#[from] std::io::Error), + #[error("cid: {0}")] + Cid(#[from] cid::Error), + #[error("cbor input was not canonical (lval 24 with value < 24)")] + HeaderLval24, + #[error("cbor input was not canonical (lval 25 with value <= MaxUint8)")] + HeaderLval25, + #[error("cbor input was not canonical (lval 26 with value <= MaxUint16)")] + HeaderLval26, + #[error("cbor input was not canonical (lval 27 with value <= MaxUint32)")] + HeaderLval27, + #[error("invalid header cbor_read_header_buf")] + HeaderInvalid, + #[error("expected cbor type byte string in input")] + UnexpectedByteString, + #[error("string in cbor input too long")] + StringTooLong, + #[error("Invalid link ({0}) in flushing buffered store")] + InvalidLink(Cid), + #[error("unhandled cbor type: {0}")] + UnhandledCborType(u8), +} + /// Given a CBOR encoded Buffer, returns a tuple of: /// the type of the CBOR object along with extra /// elements we expect to read. More info on this can be found in @@ -64,7 +99,10 @@ where /// This was implemented because the CBOR library we use does not expose low /// methods like this, requiring us to deserialize the whole CBOR payload, which /// is unnecessary and quite inefficient for our usecase here. -fn cbor_read_header_buf<B: Read>(br: &mut B, scratch: &mut [u8]) -> anyhow::Result<(u8, usize)> { +fn cbor_read_header_buf<B: Read>( + br: &mut B, + scratch: &mut [u8], +) -> Result<(u8, usize), FlushError> { let first = br.read_u8()?; let maj = (first & 0xe0) >> 5; let low = first & 0x1f; @@ -74,49 +112,41 @@ fn cbor_read_header_buf<B: Read>(br: &mut B, scratch: &mut [u8]) -> anyhow::Resu } else if low == 24 { let val = br.read_u8()?; if val < 24 { - return Err(anyhow!( - "cbor input was not canonical (lval 24 with value < 24)" - )); + return Err(FlushError::HeaderLval24); } Ok((maj, val as usize)) } else if low == 25 { br.read_exact(&mut scratch[..2])?; let val = BigEndian::read_u16(&scratch[..2]); if val <= u8::MAX as u16 { - return Err(anyhow!( - "cbor input was not canonical (lval 25 with value <= MaxUint8)" - )); + return Err(FlushError::HeaderLval25); } Ok((maj, val as usize)) } else if low == 26 { br.read_exact(&mut scratch[..4])?; let val = BigEndian::read_u32(&scratch[..4]); if val <= u16::MAX as u32 { - return Err(anyhow!( - "cbor input was not canonical (lval 26 with value <= MaxUint16)" - )); + return Err(FlushError::HeaderLval26); } Ok((maj, val as usize)) } else if low == 27 { br.read_exact(&mut scratch[..8])?; let val = BigEndian::read_u64(&scratch[..8]); if val <= u32::MAX as u64 { - return Err(anyhow!( - "cbor input was not canonical (lval 27 with value <= MaxUint32)" - )); + return Err(FlushError::HeaderLval27); } Ok((maj, val as usize)) } else { - Err(anyhow!("invalid header cbor_read_header_buf")) + Err(FlushError::HeaderInvalid) } } /// Given a CBOR serialized IPLD buffer, read through all of it and return all the Links. /// This function is useful because it is quite a bit more fast than doing this recursively on a /// deserialized IPLD object. -fn scan_for_links<B: Read + Seek, F>(buf: &mut B, mut callback: F) -> Result<()> +fn scan_for_links<B: Read + Seek, F>(buf: &mut B, mut callback: F) -> Result<(), FlushError> where - F: FnMut(Cid) -> anyhow::Result<()>, + F: FnMut(Cid) -> Result<(), FlushError>, { let mut scratch: [u8; 100] = [0; 100]; let mut remaining = 1; @@ -136,10 +166,10 @@ where let (maj, extra) = cbor_read_header_buf(buf, &mut scratch)?; // The actual CID is expected to be a byte string if maj != 2 { - return Err(anyhow!("expected cbor type byte string in input")); + return Err(FlushError::UnexpectedByteString); } if extra > 100 { - return Err(anyhow!("string in cbor input too long")); + return Err(FlushError::StringTooLong); } buf.read_exact(&mut scratch[..extra])?; let c = Cid::try_from(&scratch[1..extra])?; @@ -157,7 +187,7 @@ where remaining += extra * 2; } _ => { - return Err(anyhow!("unhandled cbor type: {}", maj)); + return Err(FlushError::UnhandledCborType(maj)); } } remaining -= 1; @@ -170,16 +200,14 @@ fn copy_rec<'a>( cache: &'a HashMap<Cid, Vec<u8>>, root: Cid, buffer: &mut Vec<(Cid, &'a [u8])>, -) -> Result<()> { +) -> Result<(), FlushError> { // TODO: Make this non-recursive. // Skip identity and Filecoin commitment Cids if root.codec() != DAG_CBOR { return Ok(()); } - let block = &*cache - .get(&root) - .ok_or_else(|| anyhow!("Invalid link ({}) in flushing buffered store", root))?; + let block = &*cache.get(&root).ok_or(FlushError::InvalidLink(root))?; scan_for_links(&mut Cursor::new(block), |link| { if link.codec() != DAG_CBOR { @@ -205,28 +233,30 @@ impl<BS> Blockstore for BufferedBlockstore<BS> where BS: Blockstore, { - fn get(&self, cid: &Cid) -> Result<Option<Vec<u8>>> { + type Error = Error<BS::Error>; + + fn get(&self, cid: &Cid) -> Result<Option<Vec<u8>>, Self::Error> { Ok(if let Some(data) = self.write.borrow().get(cid) { Some(data.clone()) } else { - self.base.get(cid)? + self.base.get(cid).map_err(Error::Blockstore)? }) } - fn put_keyed(&self, cid: &Cid, buf: &[u8]) -> Result<()> { + fn put_keyed(&self, cid: &Cid, buf: &[u8]) -> Result<(), Self::Error> { self.write.borrow_mut().insert(*cid, Vec::from(buf)); Ok(()) } - fn has(&self, k: &Cid) -> Result<bool> { + fn has(&self, k: &Cid) -> Result<bool, Self::Error> { if self.write.borrow().contains_key(k) { Ok(true) } else { - Ok(self.base.has(k)?) + Ok(self.base.has(k).map_err(Error::Blockstore)?) } } - fn put_many_keyed<D, I>(&self, blocks: I) -> Result<()> + fn put_many_keyed<D, I>(&self, blocks: I) -> Result<(), Self::Error> where Self: Sized, D: AsRef<[u8]>, diff --git a/ipld/amt/benches/amt_benchmark.rs b/ipld/amt/benches/amt_benchmark.rs index c3827b722..4e6b27e6d 100644 --- a/ipld/amt/benches/amt_benchmark.rs +++ b/ipld/amt/benches/amt_benchmark.rs @@ -117,7 +117,9 @@ fn for_each(c: &mut Criterion) { c.bench_function("AMT for_each function", |b| { b.iter(|| { let a = Amt::load(&cid, &db).unwrap(); - black_box(a).for_each(|_, _v: &u64| Ok(())).unwrap(); + black_box(a) + .for_each(|_, _v: &u64| Ok::<_, ()>(())) + .unwrap(); }) }); } diff --git a/ipld/amt/src/amt.rs b/ipld/amt/src/amt.rs index 8dc4ffa0b..195f5b615 100644 --- a/ipld/amt/src/amt.rs +++ b/ipld/amt/src/amt.rs @@ -72,7 +72,7 @@ where } /// Constructs an AMT with a blockstore and a Cid of the root of the AMT - pub fn load(cid: &Cid, block_store: BS) -> Result<Self, Error<BS>> { + pub fn load(cid: &Cid, block_store: BS) -> Result<Self, Error<BS::Error>> { // Load root bytes from database let root: Root<V> = block_store .get_cbor(cid)? @@ -100,7 +100,7 @@ where pub fn new_from_iter( block_store: BS, vals: impl IntoIterator<Item = V>, - ) -> Result<Cid, Error<BS>> { + ) -> Result<Cid, Error<BS::Error>> { let mut t = Self::new(block_store); t.batch_set(vals)?; @@ -109,7 +109,7 @@ where } /// Get value at index of AMT - pub fn get(&self, i: u64) -> Result<Option<&V>, Error<BS>> { + pub fn get(&self, i: u64) -> Result<Option<&V>, Error<BS::Error>> { if i > MAX_INDEX { return Err(Error::OutOfRange(i)); } @@ -124,7 +124,7 @@ where } /// Set value at index - pub fn set(&mut self, i: u64, val: V) -> Result<(), Error<BS>> { + pub fn set(&mut self, i: u64, val: V) -> Result<(), Error<BS::Error>> { if i > MAX_INDEX { return Err(Error::OutOfRange(i)); } @@ -166,7 +166,7 @@ where /// Batch set (naive for now) // TODO Implement more efficient batch set to not have to traverse tree and keep cache for each - pub fn batch_set(&mut self, vals: impl IntoIterator<Item = V>) -> Result<(), Error<BS>> { + pub fn batch_set(&mut self, vals: impl IntoIterator<Item = V>) -> Result<(), Error<BS::Error>> { for (i, val) in (0u64..).zip(vals) { self.set(i, val)?; } @@ -175,7 +175,7 @@ where } /// Delete item from AMT at index - pub fn delete(&mut self, i: u64) -> Result<Option<V>, Error<BS>> { + pub fn delete(&mut self, i: u64) -> Result<Option<V>, Error<BS::Error>> { if i > MAX_INDEX { return Err(Error::OutOfRange(i)); } @@ -246,7 +246,7 @@ where &mut self, iter: impl IntoIterator<Item = u64>, strict: bool, - ) -> Result<bool, Error<BS>> { + ) -> Result<bool, Error<BS::Error>> { // TODO: optimize this let mut modified = false; @@ -262,7 +262,7 @@ where } /// flush root and return Cid used as key in block store - pub fn flush(&mut self) -> Result<Cid, Error<BS>> { + pub fn flush(&mut self) -> Result<Cid, Error<BS::Error>> { self.root.node.flush(&self.block_store)?; Ok(self.block_store.put_cbor(&self.root, Code::Blake2b256)?) } @@ -291,7 +291,7 @@ where /// assert_eq!(&values, &[(1, "One".to_owned()), (4, "Four".to_owned())]); /// ``` #[inline] - pub fn for_each<F, U>(&self, mut f: F) -> Result<(), EitherError<U, BS>> + pub fn for_each<F, U>(&self, mut f: F) -> Result<(), EitherError<U, BS::Error>> where F: FnMut(u64, &V) -> Result<(), U>, { @@ -303,7 +303,7 @@ where /// Iterates over each value in the Amt and runs a function on the values, for as long as that /// function keeps returning `true`. - pub fn for_each_while<F, U>(&self, mut f: F) -> Result<(), EitherError<U, BS>> + pub fn for_each_while<F, U>(&self, mut f: F) -> Result<(), EitherError<U, BS::Error>> where F: FnMut(u64, &V) -> Result<bool, U>, { @@ -321,7 +321,7 @@ where /// Iterates over each value in the Amt and runs a function on the values that allows modifying /// each value. - pub fn for_each_mut<F, U>(&mut self, mut f: F) -> Result<(), EitherError<U, BS>> + pub fn for_each_mut<F, U>(&mut self, mut f: F) -> Result<(), EitherError<U, BS::Error>> where V: Clone, F: FnMut(u64, &mut ValueMut<'_, V>) -> Result<(), U>, @@ -334,7 +334,7 @@ where /// Iterates over each value in the Amt and runs a function on the values that allows modifying /// each value, for as long as that function keeps returning `true`. - pub fn for_each_while_mut<F, U>(&mut self, mut f: F) -> Result<(), EitherError<U, BS>> + pub fn for_each_while_mut<F, U>(&mut self, mut f: F) -> Result<(), EitherError<U, BS::Error>> where // TODO remove clone bound when go-interop doesn't require it. // (If needed without, this bound can be removed by duplicating function signatures) diff --git a/ipld/amt/src/error.rs b/ipld/amt/src/error.rs index 35231e8e3..13a2dfb0b 100644 --- a/ipld/amt/src/error.rs +++ b/ipld/amt/src/error.rs @@ -2,13 +2,12 @@ // SPDX-License-Identifier: Apache-2.0, MIT use cid::Error as CidError; -use fvm_ipld_blockstore::Blockstore; use fvm_ipld_encoding::{CborStoreError, Error as EncodingError}; use thiserror::Error; /// AMT Error #[derive(Debug, Error)] -pub enum Error<BS: Blockstore> { +pub enum Error<E> { /// Index referenced it above arbitrary max set #[error("index {0} out of range for the amt")] OutOfRange(u64), @@ -29,13 +28,13 @@ pub enum Error<BS: Blockstore> { #[error("no such index {0} in Amt for batch delete")] BatchDelteNotFound(u64), #[error("blockstore {0}")] - Blockstore(BS::Error), + Blockstore(E), #[error("encoding error {0}")] Encoding(#[from] EncodingError), } -impl<BS: Blockstore> From<CborStoreError<BS>> for Error<BS> { - fn from(err: CborStoreError<BS>) -> Self { +impl<E> From<CborStoreError<E>> for Error<E> { + fn from(err: CborStoreError<E>) -> Self { match err { CborStoreError::Blockstore(err) => Error::Blockstore(err), CborStoreError::Encoding(err) => Error::Encoding(err), @@ -44,15 +43,15 @@ impl<BS: Blockstore> From<CborStoreError<BS>> for Error<BS> { } #[derive(Debug, Error)] -pub enum EitherError<U, BS: Blockstore> { +pub enum EitherError<U, E> { #[error("user: {0}")] User(U), #[error("hamt: {0}")] - Hamt(#[from] Error<BS>), + Hamt(#[from] Error<E>), } -impl<U, BS: Blockstore> From<CborStoreError<BS>> for EitherError<U, BS> { - fn from(err: CborStoreError<BS>) -> Self { +impl<U, E> From<CborStoreError<E>> for EitherError<U, E> { + fn from(err: CborStoreError<E>) -> Self { EitherError::Hamt(err.into()) } } diff --git a/ipld/amt/src/node.rs b/ipld/amt/src/node.rs index f63586ce3..44d1f54ba 100644 --- a/ipld/amt/src/node.rs +++ b/ipld/amt/src/node.rs @@ -141,7 +141,7 @@ impl<V> CollapsedNode<V> { *v = Some(Link::from( links_iter .next() - .ok_or_else(|| CollapsedNodeError::MoreBitsThanLinks)?, + .ok_or(CollapsedNodeError::MoreBitsThanLinks)?, )) } } @@ -157,7 +157,7 @@ impl<V> CollapsedNode<V> { *v = Some( val_iter .next() - .ok_or_else(|| CollapsedNodeError::MoreBitsThanValues)?, + .ok_or(CollapsedNodeError::MoreBitsThanValues)?, ) } } @@ -182,7 +182,7 @@ where } /// Flushes cache for node, replacing any cached values with a Cid variant - pub(super) fn flush<DB: Blockstore>(&mut self, bs: &DB) -> Result<(), Error<DB>> { + pub(super) fn flush<DB: Blockstore>(&mut self, bs: &DB) -> Result<(), Error<DB::Error>> { if let Node::Link { links } = self { for link in links.iter_mut().flatten() { // links should only be flushed if the bitmap is set. @@ -237,7 +237,7 @@ where height: u32, bit_width: u32, i: u64, - ) -> Result<Option<&V>, Error<DB>> { + ) -> Result<Option<&V>, Error<DB::Error>> { match self { Node::Leaf { vals, .. } => Ok(vals.get(i as usize).and_then(|v| v.as_ref())), Node::Link { links, .. } => { @@ -247,13 +247,14 @@ where match links.get(sub_i) { Some(Some(Link::Cid { cid, cache })) => { - let cached_node = cache.get_or_try_init(|| -> Result<_, Error<DB>> { - let node = bs - .get_cbor::<CollapsedNode<V>>(cid)? - .ok_or_else(|| Error::CidNotFound(cid.to_string()))? - .expand(bit_width)?; - Ok(Box::new(node)) - })?; + let cached_node = + cache.get_or_try_init(|| -> Result<_, Error<DB::Error>> { + let node = bs + .get_cbor::<CollapsedNode<V>>(cid)? + .ok_or_else(|| Error::CidNotFound(cid.to_string()))? + .expand(bit_width)?; + Ok(Box::new(node)) + })?; cached_node.get( bs, @@ -282,7 +283,7 @@ where bit_width: u32, i: u64, val: V, - ) -> Result<Option<V>, Error<DB>> { + ) -> Result<Option<V>, Error<DB::Error>> { if height == 0 { return Ok(self.set_leaf(i, val)); } @@ -354,7 +355,7 @@ where height: u32, bit_width: u32, i: u64, - ) -> Result<Option<V>, Error<DB>> { + ) -> Result<Option<V>, Error<DB::Error>> { match self { Self::Leaf { vals } => Ok(vals .get_mut(usize::try_from(i).unwrap()) @@ -385,7 +386,7 @@ where } Some(Link::Cid { cid, cache }) => { // Take cache, will be replaced if no nodes deleted - cache.get_or_try_init(|| -> Result<_, Error<DB>> { + cache.get_or_try_init(|| -> Result<_, Error<DB::Error>> { let node = bs .get_cbor::<CollapsedNode<V>>(cid)? .ok_or_else(|| Error::CidNotFound(cid.to_string()))? @@ -432,7 +433,7 @@ where bit_width: u32, offset: u64, f: &mut F, - ) -> Result<bool, EitherError<U, S>> + ) -> Result<bool, EitherError<U, S::Error>> where F: FnMut(u64, &V) -> Result<bool, U>, S: Blockstore, @@ -459,7 +460,7 @@ where } Link::Cid { cid, cache } => { let cached_node = - cache.get_or_try_init(|| -> Result<_, Error<S>> { + cache.get_or_try_init(|| -> Result<_, Error<S::Error>> { let node = bs .get_cbor::<CollapsedNode<V>>(cid)? .ok_or_else(|| Error::CidNotFound(cid.to_string()))? @@ -493,7 +494,7 @@ where bit_width: u32, offset: u64, f: &mut F, - ) -> Result<(bool, bool), EitherError<U, S>> + ) -> Result<(bool, bool), EitherError<U, S::Error>> where F: FnMut(u64, &mut ValueMut<'_, V>) -> Result<bool, U>, S: Blockstore, @@ -525,7 +526,7 @@ where sub.for_each_while_mut(bs, height - 1, bit_width, offs, f)? } Link::Cid { cid, cache } => { - cache.get_or_try_init(|| -> Result<_, Error<S>> { + cache.get_or_try_init(|| -> Result<_, Error<S::Error>> { let node = bs .get_cbor::<CollapsedNode<V>>(cid)? .ok_or_else(|| Error::CidNotFound(cid.to_string()))? diff --git a/ipld/blockstore/src/lib.rs b/ipld/blockstore/src/lib.rs index b77872f89..06c61729a 100644 --- a/ipld/blockstore/src/lib.rs +++ b/ipld/blockstore/src/lib.rs @@ -13,8 +13,8 @@ pub use block::*; /// An IPLD blockstore suitable for injection into the FVM. /// /// The cgo blockstore adapter implements this trait. -pub trait Blockstore: std::fmt::Debug { - type Error: std::error::Error + std::fmt::Debug; +pub trait Blockstore { + type Error: std::error::Error + std::fmt::Debug + Send + Sync + 'static; /// Gets the block from the blockstore. fn get(&self, k: &Cid) -> Result<Option<Vec<u8>>, Self::Error>; diff --git a/ipld/encoding/src/cbor_store.rs b/ipld/encoding/src/cbor_store.rs index b2427530b..02941d020 100644 --- a/ipld/encoding/src/cbor_store.rs +++ b/ipld/encoding/src/cbor_store.rs @@ -5,9 +5,9 @@ use serde::{de, ser}; use crate::DAG_CBOR; #[derive(thiserror::Error, Debug)] -pub enum Error<BS: Blockstore> { +pub enum Error<E> { #[error("blockstore: {0}")] - Blockstore(BS::Error), + Blockstore(E), #[error("encoding: {0}")] Encoding(#[from] crate::errors::Error), } @@ -15,7 +15,7 @@ pub enum Error<BS: Blockstore> { /// Wrapper for database to handle inserting and retrieving ipld data with Cids pub trait CborStore: Blockstore + Sized { /// Get typed object from block store by Cid. - fn get_cbor<T>(&self, cid: &Cid) -> Result<Option<T>, Error<Self>> + fn get_cbor<T>(&self, cid: &Cid) -> Result<Option<T>, Error<Self::Error>> where T: de::DeserializeOwned, { @@ -29,7 +29,7 @@ pub trait CborStore: Blockstore + Sized { } /// Put an object in the block store and return the Cid identifier. - fn put_cbor<S>(&self, obj: &S, code: multihash::Code) -> Result<Cid, Error<Self>> + fn put_cbor<S>(&self, obj: &S, code: multihash::Code) -> Result<Cid, Error<Self::Error>> where S: ser::Serialize, { diff --git a/ipld/hamt/benches/hamt_benchmark.rs b/ipld/hamt/benches/hamt_benchmark.rs index 5e071e4ad..b7d8505ea 100644 --- a/ipld/hamt/benches/hamt_benchmark.rs +++ b/ipld/hamt/benches/hamt_benchmark.rs @@ -94,7 +94,9 @@ fn for_each(c: &mut Criterion) { c.bench_function("HAMT for_each function", |b| { b.iter(|| { let a = Hamt::<_, _>::load(&cid, &db).unwrap(); - black_box(a).for_each(|_k, _v: &BenchData| Ok(())).unwrap(); + black_box(a) + .for_each(|_k, _v: &BenchData| Ok::<_, ()>(())) + .unwrap(); }) }); } diff --git a/ipld/hamt/src/error.rs b/ipld/hamt/src/error.rs index caf5c778d..cae5e92f8 100644 --- a/ipld/hamt/src/error.rs +++ b/ipld/hamt/src/error.rs @@ -1,13 +1,12 @@ // Copyright 2019-2022 ChainSafe Systems // SPDX-License-Identifier: Apache-2.0, MIT -use fvm_ipld_blockstore::Blockstore; use fvm_ipld_encoding::{CborStoreError, Error as EncodingError}; use thiserror::Error; /// HAMT Error #[derive(Debug, Error)] -pub enum Error<BS: Blockstore> { +pub enum Error<E> { #[error("hashbits: {0}")] HashBits(#[from] HashBitsError), /// This should be treated as a fatal error, must have at least one pointer in node @@ -17,13 +16,13 @@ pub enum Error<BS: Blockstore> { #[error("Cid ({0}) did not match any in database")] CidNotFound(String), #[error("blockstore {0}")] - Blockstore(BS::Error), + Blockstore(E), #[error("encoding error {0}")] Encoding(#[from] EncodingError), } -impl<BS: Blockstore> From<CborStoreError<BS>> for Error<BS> { - fn from(err: CborStoreError<BS>) -> Self { +impl<E> From<CborStoreError<E>> for Error<E> { + fn from(err: CborStoreError<E>) -> Self { match err { CborStoreError::Blockstore(err) => Error::Blockstore(err), CborStoreError::Encoding(err) => Error::Encoding(err), @@ -32,15 +31,15 @@ impl<BS: Blockstore> From<CborStoreError<BS>> for Error<BS> { } #[derive(Debug, Error)] -pub enum EitherError<U, BS: Blockstore> { +pub enum EitherError<U, E> { #[error("user: {0}")] User(U), #[error("hamt: {0}")] - Hamt(#[from] Error<BS>), + Hamt(#[from] Error<E>), } -impl<U, BS: Blockstore> From<CborStoreError<BS>> for EitherError<U, BS> { - fn from(err: CborStoreError<BS>) -> Self { +impl<U, E> From<CborStoreError<E>> for EitherError<U, E> { + fn from(err: CborStoreError<E>) -> Self { EitherError::Hamt(err.into()) } } diff --git a/ipld/hamt/src/hamt.rs b/ipld/hamt/src/hamt.rs index 2a3c626d6..201b78fc5 100644 --- a/ipld/hamt/src/hamt.rs +++ b/ipld/hamt/src/hamt.rs @@ -83,12 +83,16 @@ where } /// Lazily instantiate a hamt from this root Cid. - pub fn load(cid: &Cid, store: BS) -> Result<Self, Error<BS>> { + pub fn load(cid: &Cid, store: BS) -> Result<Self, Error<BS::Error>> { Self::load_with_bit_width(cid, store, DEFAULT_BIT_WIDTH) } /// Lazily instantiate a hamt from this root Cid with a specified bit width. - pub fn load_with_bit_width(cid: &Cid, store: BS, bit_width: u32) -> Result<Self, Error<BS>> { + pub fn load_with_bit_width( + cid: &Cid, + store: BS, + bit_width: u32, + ) -> Result<Self, Error<BS::Error>> { match store.get_cbor(cid)? { Some(root) => Ok(Self { root, @@ -101,7 +105,7 @@ where } /// Sets the root based on the Cid of the root node using the Hamt store - pub fn set_root(&mut self, cid: &Cid) -> Result<(), Error<BS>> { + pub fn set_root(&mut self, cid: &Cid) -> Result<(), Error<BS::Error>> { match self.store.get_cbor(cid)? { Some(root) => self.root = root, None => return Err(Error::CidNotFound(cid.to_string())), @@ -137,7 +141,7 @@ where /// map.set(37, "b".to_string()).unwrap(); /// map.set(37, "c".to_string()).unwrap(); /// ``` - pub fn set(&mut self, key: K, value: V) -> Result<Option<V>, Error<BS>> + pub fn set(&mut self, key: K, value: V) -> Result<Option<V>, Error<BS::Error>> where V: PartialEq, { @@ -172,7 +176,7 @@ where /// let c = map.set_if_absent(30, "c".to_string()).unwrap(); /// assert_eq!(c, true); /// ``` - pub fn set_if_absent(&mut self, key: K, value: V) -> Result<bool, Error<BS>> + pub fn set_if_absent(&mut self, key: K, value: V) -> Result<bool, Error<BS::Error>> where V: PartialEq, { @@ -201,7 +205,7 @@ where /// assert_eq!(map.get(&2).unwrap(), None); /// ``` #[inline] - pub fn get<Q: ?Sized>(&self, k: &Q) -> Result<Option<&V>, Error<BS>> + pub fn get<Q: ?Sized>(&self, k: &Q) -> Result<Option<&V>, Error<BS::Error>> where K: Borrow<Q>, Q: Hash + Eq, @@ -233,7 +237,7 @@ where /// assert_eq!(map.contains_key(&2).unwrap(), false); /// ``` #[inline] - pub fn contains_key<Q: ?Sized>(&self, k: &Q) -> Result<bool, Error<BS>> + pub fn contains_key<Q: ?Sized>(&self, k: &Q) -> Result<bool, Error<BS::Error>> where K: Borrow<Q>, Q: Hash + Eq, @@ -264,7 +268,7 @@ where /// assert_eq!(map.delete(&1).unwrap(), Some((1, "a".to_string()))); /// assert_eq!(map.delete(&1).unwrap(), None); /// ``` - pub fn delete<Q: ?Sized>(&mut self, k: &Q) -> Result<Option<(K, V)>, Error<BS>> + pub fn delete<Q: ?Sized>(&mut self, k: &Q) -> Result<Option<(K, V)>, Error<BS::Error>> where K: Borrow<Q>, Q: Hash + Eq, @@ -274,7 +278,7 @@ where } /// Flush root and return Cid for hamt - pub fn flush(&mut self) -> Result<Cid, Error<BS>> { + pub fn flush(&mut self) -> Result<Cid, Error<BS::Error>> { self.root.flush(self.store.borrow())?; Ok(self.store.put_cbor(&self.root, Code::Blake2b256)?) } @@ -307,7 +311,7 @@ where /// assert_eq!(total, 3); /// ``` #[inline] - pub fn for_each<F, U>(&self, mut f: F) -> Result<(), EitherError<U, BS>> + pub fn for_each<F, U>(&self, mut f: F) -> Result<(), EitherError<U, BS::Error>> where V: DeserializeOwned, F: FnMut(&K, &V) -> Result<(), U>, diff --git a/ipld/hamt/src/node.rs b/ipld/hamt/src/node.rs index c5891c886..ba135f56d 100644 --- a/ipld/hamt/src/node.rs +++ b/ipld/hamt/src/node.rs @@ -86,7 +86,7 @@ where store: &S, bit_width: u32, overwrite: bool, - ) -> Result<(Option<V>, bool), Error<S>> + ) -> Result<(Option<V>, bool), Error<S::Error>> where V: PartialEq, { @@ -108,7 +108,7 @@ where k: &Q, store: &S, bit_width: u32, - ) -> Result<Option<&V>, Error<S>> + ) -> Result<Option<&V>, Error<S::Error>> where K: Borrow<Q>, Q: Eq + Hash, @@ -122,7 +122,7 @@ where k: &Q, store: &S, bit_width: u32, - ) -> Result<Option<(K, V)>, Error<S>> + ) -> Result<Option<(K, V)>, Error<S::Error>> where K: Borrow<Q>, Q: Eq + Hash, @@ -136,7 +136,11 @@ where self.pointers.is_empty() } - pub(crate) fn for_each<S, F, U>(&self, store: &S, f: &mut F) -> Result<(), EitherError<U, S>> + pub(crate) fn for_each<S, F, U>( + &self, + store: &S, + f: &mut F, + ) -> Result<(), EitherError<U, S::Error>> where F: FnMut(&K, &V) -> Result<(), U>, S: Blockstore, @@ -179,7 +183,7 @@ where q: &Q, store: &S, bit_width: u32, - ) -> Result<Option<&KeyValuePair<K, V>>, Error<S>> + ) -> Result<Option<&KeyValuePair<K, V>>, Error<S::Error>> where K: Borrow<Q>, Q: Eq + Hash, @@ -195,7 +199,7 @@ where depth: u64, key: &Q, store: &S, - ) -> Result<Option<&KeyValuePair<K, V>>, Error<S>> + ) -> Result<Option<&KeyValuePair<K, V>>, Error<S::Error>> where K: Borrow<Q>, Q: Eq + Hash, @@ -245,7 +249,7 @@ where value: V, store: &S, overwrite: bool, - ) -> Result<(Option<V>, bool), Error<S>> + ) -> Result<(Option<V>, bool), Error<S::Error>> where V: PartialEq, { @@ -365,7 +369,7 @@ where depth: u64, key: &Q, store: &S, - ) -> Result<Option<(K, V)>, Error<S>> + ) -> Result<Option<(K, V)>, Error<S::Error>> where K: Borrow<Q>, Q: Hash + Eq, @@ -393,7 +397,7 @@ where if deleted.is_some() { *child = Pointer::Dirty(std::mem::take(child_node)); // Clean to retrieve canonical form - child.clean()?; + child.clean::<S>()?; } Ok(deleted) @@ -403,7 +407,7 @@ where let deleted = n.rm_value(hashed_key, bit_width, depth + 1, key, store)?; // Clean to ensure canonical form - child.clean()?; + child.clean::<S>()?; Ok(deleted) } Pointer::Values(vals) => { @@ -428,7 +432,7 @@ where } } - pub fn flush<S: Blockstore>(&mut self, store: &S) -> Result<(), Error<S>> { + pub fn flush<S: Blockstore>(&mut self, store: &S) -> Result<(), Error<S::Error>> { for pointer in &mut self.pointers { if let Pointer::Dirty(node) = pointer { // Flush cached sub node to clear it's cache diff --git a/ipld/hamt/src/pointer.rs b/ipld/hamt/src/pointer.rs index 999278dff..ff001e4c4 100644 --- a/ipld/hamt/src/pointer.rs +++ b/ipld/hamt/src/pointer.rs @@ -112,7 +112,7 @@ where /// Internal method to cleanup children, to ensure consistent tree representation /// after deletes. - pub(crate) fn clean<BS: Blockstore>(&mut self) -> Result<(), Error<BS>> { + pub(crate) fn clean<BS: Blockstore>(&mut self) -> Result<(), Error<BS::Error>> { match self { Pointer::Dirty(n) => match n.pointers.len() { 0 => Err(Error::ZeroPointers), diff --git a/shared/src/actor/builtin.rs b/shared/src/actor/builtin.rs index e58cd80a4..e4b2f9a2f 100644 --- a/shared/src/actor/builtin.rs +++ b/shared/src/actor/builtin.rs @@ -118,7 +118,7 @@ pub fn load_manifest<B: Blockstore>( bs: &B, root_cid: &Cid, ver: u32, -) -> Result<Manifest, ManifestError<B>> { +) -> Result<Manifest, ManifestError<B::Error>> { match ver { 0 => load_manifest_v0(bs, root_cid), 1 => load_manifest_v1(bs, root_cid), @@ -129,7 +129,7 @@ pub fn load_manifest<B: Blockstore>( pub fn load_manifest_v0<B: Blockstore>( bs: &B, root_cid: &Cid, -) -> Result<Manifest, ManifestError<B>> { +) -> Result<Manifest, ManifestError<B::Error>> { match bs.get_cbor::<Manifest>(root_cid)? { Some(mf) => Ok(mf), None => Err(ManifestError::MissingRootCid(*root_cid)), @@ -139,7 +139,7 @@ pub fn load_manifest_v0<B: Blockstore>( pub fn load_manifest_v1<B: Blockstore>( bs: &B, root_cid: &Cid, -) -> Result<Manifest, ManifestError<B>> { +) -> Result<Manifest, ManifestError<B::Error>> { let vec: Vec<(String, Cid)> = match bs.get_cbor(root_cid)? { Some(vec) => vec, None => { @@ -162,7 +162,7 @@ pub fn load_manifest_v1<B: Blockstore>( } #[derive(thiserror::Error, Debug)] -pub enum ManifestError<BS: Blockstore> { +pub enum ManifestError<E> { #[error("unknown manifest version {0}")] UnknownVersion(u32), #[error("cannot find manifest root cid {0}")] @@ -170,5 +170,5 @@ pub enum ManifestError<BS: Blockstore> { #[error("bad builtin actor name: {0}: {1}")] BadBuiltinActor(String, String), #[error("encoding {0}")] - Encoding(#[from] CborStoreError<BS>), + Encoding(#[from] CborStoreError<E>), } From fd9f106b6204d197cd3c91bc22ab892c5fd04e65 Mon Sep 17 00:00:00 2001 From: dignifiedquire <me@dignifiedquire.com> Date: Thu, 7 Apr 2022 11:54:18 +0200 Subject: [PATCH 6/7] apply cr --- fvm/src/blockstore/buffered.rs | 18 ++++++------------ ipld/amt/src/amt.rs | 2 +- ipld/amt/src/error.rs | 10 ++++++---- ipld/hamt/src/error.rs | 2 ++ 4 files changed, 15 insertions(+), 17 deletions(-) diff --git a/fvm/src/blockstore/buffered.rs b/fvm/src/blockstore/buffered.rs index a80b112ac..065f934ee 100644 --- a/fvm/src/blockstore/buffered.rs +++ b/fvm/src/blockstore/buffered.rs @@ -72,14 +72,8 @@ pub enum FlushError { Io(#[from] std::io::Error), #[error("cid: {0}")] Cid(#[from] cid::Error), - #[error("cbor input was not canonical (lval 24 with value < 24)")] - HeaderLval24, - #[error("cbor input was not canonical (lval 25 with value <= MaxUint8)")] - HeaderLval25, - #[error("cbor input was not canonical (lval 26 with value <= MaxUint16)")] - HeaderLval26, - #[error("cbor input was not canonical (lval 27 with value <= MaxUint32)")] - HeaderLval27, + #[error("cbor input was not canonical (lval {0} with value < {1})")] + HeaderNotCanonical(usize, &'static str), #[error("invalid header cbor_read_header_buf")] HeaderInvalid, #[error("expected cbor type byte string in input")] @@ -112,28 +106,28 @@ fn cbor_read_header_buf<B: Read>( } else if low == 24 { let val = br.read_u8()?; if val < 24 { - return Err(FlushError::HeaderLval24); + return Err(FlushError::HeaderNotCanonical(24, "24")); } Ok((maj, val as usize)) } else if low == 25 { br.read_exact(&mut scratch[..2])?; let val = BigEndian::read_u16(&scratch[..2]); if val <= u8::MAX as u16 { - return Err(FlushError::HeaderLval25); + return Err(FlushError::HeaderNotCanonical(25, "MaxUint8")); } Ok((maj, val as usize)) } else if low == 26 { br.read_exact(&mut scratch[..4])?; let val = BigEndian::read_u32(&scratch[..4]); if val <= u16::MAX as u32 { - return Err(FlushError::HeaderLval26); + return Err(FlushError::HeaderNotCanonical(26, "MaxUint16")); } Ok((maj, val as usize)) } else if low == 27 { br.read_exact(&mut scratch[..8])?; let val = BigEndian::read_u64(&scratch[..8]); if val <= u32::MAX as u64 { - return Err(FlushError::HeaderLval27); + return Err(FlushError::HeaderNotCanonical(27, "MaxUint32")); } Ok((maj, val as usize)) } else { diff --git a/ipld/amt/src/amt.rs b/ipld/amt/src/amt.rs index 195f5b615..718dd3dca 100644 --- a/ipld/amt/src/amt.rs +++ b/ipld/amt/src/amt.rs @@ -254,7 +254,7 @@ where for i in sorted(iter) { let found = self.delete(i)?.is_none(); if strict && found { - return Err(Error::BatchDelteNotFound(i)); + return Err(Error::BatchDeleteNotFound(i)); } modified |= found; } diff --git a/ipld/amt/src/error.rs b/ipld/amt/src/error.rs index 13a2dfb0b..c29cc1d86 100644 --- a/ipld/amt/src/error.rs +++ b/ipld/amt/src/error.rs @@ -26,7 +26,7 @@ pub enum Error<E> { #[error("{0}")] CollapsedNode(#[from] CollapsedNodeError), #[error("no such index {0} in Amt for batch delete")] - BatchDelteNotFound(u64), + BatchDeleteNotFound(u64), #[error("blockstore {0}")] Blockstore(E), #[error("encoding error {0}")] @@ -42,17 +42,19 @@ impl<E> From<CborStoreError<E>> for Error<E> { } } +/// This error wraps around around two different errors, either the native `Error` from `amt`, or +/// a custom user error, returned from executing a user defined function. #[derive(Debug, Error)] pub enum EitherError<U, E> { #[error("user: {0}")] User(U), - #[error("hamt: {0}")] - Hamt(#[from] Error<E>), + #[error("amt: {0}")] + Amt(#[from] Error<E>), } impl<U, E> From<CborStoreError<E>> for EitherError<U, E> { fn from(err: CborStoreError<E>) -> Self { - EitherError::Hamt(err.into()) + EitherError::Amt(err.into()) } } diff --git a/ipld/hamt/src/error.rs b/ipld/hamt/src/error.rs index cae5e92f8..2b7ad2193 100644 --- a/ipld/hamt/src/error.rs +++ b/ipld/hamt/src/error.rs @@ -30,6 +30,8 @@ impl<E> From<CborStoreError<E>> for Error<E> { } } +/// This error wraps around around two different errors, either the native `Error` from `hamt`, or +/// a custom user error, returned from executing a user defined function. #[derive(Debug, Error)] pub enum EitherError<U, E> { #[error("user: {0}")] From 4b20a04ebbc5c947ce59792a3e091ffd8a058e8c Mon Sep 17 00:00:00 2001 From: dignifiedquire <me@dignifiedquire.com> Date: Tue, 12 Apr 2022 18:21:44 +0200 Subject: [PATCH 7/7] additions based on integration into builtin-actors --- fvm/src/state_tree.rs | 2 +- ipld/amt/benches/amt_benchmark.rs | 2 +- ipld/amt/src/amt.rs | 75 ++++++++++++++++++++++++++--- ipld/amt/src/lib.rs | 2 +- ipld/amt/tests/amt_tests.rs | 5 +- ipld/blockstore/src/lib.rs | 2 +- ipld/hamt/benches/hamt_benchmark.rs | 2 +- ipld/hamt/src/hamt.rs | 20 +++++++- ipld/hamt/tests/hamt_tests.rs | 3 -- 9 files changed, 92 insertions(+), 21 deletions(-) diff --git a/fvm/src/state_tree.rs b/fvm/src/state_tree.rs index bb9784661..da65816ba 100644 --- a/fvm/src/state_tree.rs +++ b/fvm/src/state_tree.rs @@ -504,7 +504,7 @@ where where F: FnMut(Address, &ActorState) -> anyhow::Result<()>, { - self.hamt.for_each(|k, v| { + self.hamt.try_for_each(|k, v| { let addr = Address::from_bytes(&k.0)?; f(addr, v) })?; diff --git a/ipld/amt/benches/amt_benchmark.rs b/ipld/amt/benches/amt_benchmark.rs index 4e6b27e6d..c0ce676fb 100644 --- a/ipld/amt/benches/amt_benchmark.rs +++ b/ipld/amt/benches/amt_benchmark.rs @@ -118,7 +118,7 @@ fn for_each(c: &mut Criterion) { b.iter(|| { let a = Amt::load(&cid, &db).unwrap(); black_box(a) - .for_each(|_, _v: &u64| Ok::<_, ()>(())) + .try_for_each(|_, _v: &u64| Ok::<_, ()>(())) .unwrap(); }) }); diff --git a/ipld/amt/src/amt.rs b/ipld/amt/src/amt.rs index 718dd3dca..b30188ee1 100644 --- a/ipld/amt/src/amt.rs +++ b/ipld/amt/src/amt.rs @@ -284,26 +284,42 @@ where /// map.set(4, "Four".to_owned()).unwrap(); /// /// let mut values: Vec<(u64, String)> = Vec::new(); - /// map.for_each(|i, v| { + /// map.try_for_each(|i, v| { /// values.push((i, v.clone())); /// Ok::<_, ()>(()) /// }).unwrap(); /// assert_eq!(&values, &[(1, "One".to_owned()), (4, "Four".to_owned())]); /// ``` #[inline] - pub fn for_each<F, U>(&self, mut f: F) -> Result<(), EitherError<U, BS::Error>> + pub fn try_for_each<F, U>(&self, mut f: F) -> Result<(), EitherError<U, BS::Error>> where F: FnMut(u64, &V) -> Result<(), U>, { - self.for_each_while(|i, x| { + self.try_for_each_while(|i, x| { f(i, x)?; Ok(true) }) } + #[inline] + pub fn for_each<F>(&self, mut f: F) -> Result<(), Error<BS::Error>> + where + V: DeserializeOwned, + F: FnMut(u64, &V), + { + self.try_for_each(|k, v| { + f(k, v); + Ok(()) + }) + .map_err(|err| match err { + EitherError::User(()) => unreachable!(), + EitherError::Amt(e) => e, + }) + } + /// Iterates over each value in the Amt and runs a function on the values, for as long as that /// function keeps returning `true`. - pub fn for_each_while<F, U>(&self, mut f: F) -> Result<(), EitherError<U, BS::Error>> + pub fn try_for_each_while<F, U>(&self, mut f: F) -> Result<(), EitherError<U, BS::Error>> where F: FnMut(u64, &V) -> Result<bool, U>, { @@ -319,22 +335,51 @@ where .map(|_| ()) } + /// Iterates over each value in the Amt and runs a function on the values, for as long as that + /// function keeps returning `true`. + pub fn for_each_while<F>(&self, mut f: F) -> Result<(), Error<BS::Error>> + where + F: FnMut(u64, &V) -> bool, + { + self.try_for_each_while::<_, ()>(|key, value| Ok(f(key, value))) + .map_err(|err| match err { + EitherError::User(()) => unreachable!(), + EitherError::Amt(e) => e, + }) + } + /// Iterates over each value in the Amt and runs a function on the values that allows modifying /// each value. - pub fn for_each_mut<F, U>(&mut self, mut f: F) -> Result<(), EitherError<U, BS::Error>> + pub fn try_for_each_mut<F, U>(&mut self, mut f: F) -> Result<(), EitherError<U, BS::Error>> where V: Clone, F: FnMut(u64, &mut ValueMut<'_, V>) -> Result<(), U>, { - self.for_each_while_mut(|i, x| { + self.try_for_each_while_mut(|i, x| { f(i, x)?; Ok(true) }) } + /// Iterates over each value in the Amt and runs a function on the values that allows modifying + /// each value. + pub fn for_each_mut<F>(&mut self, mut f: F) -> Result<(), Error<BS::Error>> + where + V: Clone, + F: FnMut(u64, &mut ValueMut<'_, V>), + { + self.for_each_while_mut(|i, x| { + f(i, x); + true + }) + } + /// Iterates over each value in the Amt and runs a function on the values that allows modifying /// each value, for as long as that function keeps returning `true`. - pub fn for_each_while_mut<F, U>(&mut self, mut f: F) -> Result<(), EitherError<U, BS::Error>> + pub fn try_for_each_while_mut<F, U>( + &mut self, + mut f: F, + ) -> Result<(), EitherError<U, BS::Error>> where // TODO remove clone bound when go-interop doesn't require it. // (If needed without, this bound can be removed by duplicating function signatures) @@ -392,4 +437,20 @@ where Ok(()) } } + + /// Iterates over each value in the Amt and runs a function on the values that allows modifying + /// each value, for as long as that function keeps returning `true`. + pub fn for_each_while_mut<F>(&mut self, mut f: F) -> Result<(), Error<BS::Error>> + where + // TODO remove clone bound when go-interop doesn't require it. + // (If needed without, this bound can be removed by duplicating function signatures) + V: Clone, + F: FnMut(u64, &mut ValueMut<'_, V>) -> bool, + { + self.try_for_each_while_mut::<_, ()>(|key, value| Ok(f(key, value))) + .map_err(|err| match err { + EitherError::User(()) => unreachable!(), + EitherError::Amt(e) => e, + }) + } } diff --git a/ipld/amt/src/lib.rs b/ipld/amt/src/lib.rs index 72c7ed1f9..e70f57fbf 100644 --- a/ipld/amt/src/lib.rs +++ b/ipld/amt/src/lib.rs @@ -13,7 +13,7 @@ mod root; mod value_mut; pub use self::amt::Amt; -pub use self::error::Error; +pub use self::error::{EitherError, Error}; pub(crate) use self::node::Node; pub(crate) use self::root::Root; pub use self::value_mut::ValueMut; diff --git a/ipld/amt/tests/amt_tests.rs b/ipld/amt/tests/amt_tests.rs index 4eb82c8a8..04b86b0f7 100644 --- a/ipld/amt/tests/amt_tests.rs +++ b/ipld/amt/tests/amt_tests.rs @@ -322,7 +322,6 @@ fn for_each() { let mut x = 0; a.for_each(|_, _: &BytesDe| { x += 1; - Ok::<(), ()>(()) }) .unwrap(); @@ -343,13 +342,12 @@ fn for_each() { ); } x += 1; - Ok::<(), ()>(()) }) .unwrap(); assert_eq!(x, indexes.len()); // Iteration again will be read diff with go-interop, since they do not cache - new_amt.for_each(|_, _: &BytesDe| Ok::<(), ()>(())).unwrap(); + new_amt.for_each(|_, _: &BytesDe| {}).unwrap(); assert_eq!( c.to_string().as_str(), @@ -385,7 +383,6 @@ fn for_each_mutate() { // Value it's set to doesn't matter, just cloning for expedience **v = v.clone(); } - Ok::<(), ()>(()) }) .unwrap(); diff --git a/ipld/blockstore/src/lib.rs b/ipld/blockstore/src/lib.rs index 06c61729a..8e1c1eee0 100644 --- a/ipld/blockstore/src/lib.rs +++ b/ipld/blockstore/src/lib.rs @@ -14,7 +14,7 @@ pub use block::*; /// /// The cgo blockstore adapter implements this trait. pub trait Blockstore { - type Error: std::error::Error + std::fmt::Debug + Send + Sync + 'static; + type Error: std::error::Error + Send + Sync + 'static; /// Gets the block from the blockstore. fn get(&self, k: &Cid) -> Result<Option<Vec<u8>>, Self::Error>; diff --git a/ipld/hamt/benches/hamt_benchmark.rs b/ipld/hamt/benches/hamt_benchmark.rs index b7d8505ea..cf4ffde56 100644 --- a/ipld/hamt/benches/hamt_benchmark.rs +++ b/ipld/hamt/benches/hamt_benchmark.rs @@ -95,7 +95,7 @@ fn for_each(c: &mut Criterion) { b.iter(|| { let a = Hamt::<_, _>::load(&cid, &db).unwrap(); black_box(a) - .for_each(|_k, _v: &BenchData| Ok::<_, ()>(())) + .try_for_each(|_k, _v: &BenchData| Ok::<_, ()>(())) .unwrap(); }) }); diff --git a/ipld/hamt/src/hamt.rs b/ipld/hamt/src/hamt.rs index 201b78fc5..c1129169f 100644 --- a/ipld/hamt/src/hamt.rs +++ b/ipld/hamt/src/hamt.rs @@ -304,14 +304,14 @@ where /// map.set(4, 2).unwrap(); /// /// let mut total = 0; - /// map.for_each(|_, v: &u64| { + /// map.try_for_each(|_, v: &u64| { /// total += v; /// Ok::<(), ()>(()) /// }).unwrap(); /// assert_eq!(total, 3); /// ``` #[inline] - pub fn for_each<F, U>(&self, mut f: F) -> Result<(), EitherError<U, BS::Error>> + pub fn try_for_each<F, U>(&self, mut f: F) -> Result<(), EitherError<U, BS::Error>> where V: DeserializeOwned, F: FnMut(&K, &V) -> Result<(), U>, @@ -319,6 +319,22 @@ where self.root.for_each(self.store.borrow(), &mut f) } + #[inline] + pub fn for_each<F>(&self, mut f: F) -> Result<(), Error<BS::Error>> + where + V: DeserializeOwned, + F: FnMut(&K, &V), + { + self.try_for_each(|k, v| { + f(k, v); + Ok(()) + }) + .map_err(|err| match err { + EitherError::User(()) => unreachable!(), + EitherError::Hamt(e) => e, + }) + } + /// Consumes this HAMT and returns the Blockstore it owns. pub fn into_store(self) -> BS { self.store diff --git a/ipld/hamt/tests/hamt_tests.rs b/ipld/hamt/tests/hamt_tests.rs index b67c70ff2..37581dc42 100644 --- a/ipld/hamt/tests/hamt_tests.rs +++ b/ipld/hamt/tests/hamt_tests.rs @@ -273,7 +273,6 @@ fn for_each() { hamt.for_each(|k, v| { assert_eq!(k, v); count += 1; - Ok::<(), ()>(()) }) .unwrap(); assert_eq!(count, 200); @@ -291,7 +290,6 @@ fn for_each() { hamt.for_each(|k, v| { assert_eq!(k, v); count += 1; - Ok::<(), ()>(()) }) .unwrap(); assert_eq!(count, 200); @@ -301,7 +299,6 @@ fn for_each() { hamt.for_each(|k, v| { assert_eq!(k, v); count += 1; - Ok::<(), ()>(()) }) .unwrap(); assert_eq!(count, 200);