From 410a9557f0637a3cbd3ca52cfffbb4604948978a Mon Sep 17 00:00:00 2001
From: austinabell <austinabell8@gmail.com>
Date: Fri, 2 Oct 2020 15:41:44 -0400
Subject: [PATCH 1/8] wip transfering over test vectors

---
 ipld/hamt/Cargo.toml          |  1 +
 ipld/hamt/tests/hamt_tests.rs | 57 +++++++++++++++++------------------
 2 files changed, 28 insertions(+), 30 deletions(-)

diff --git a/ipld/hamt/Cargo.toml b/ipld/hamt/Cargo.toml
index 6b3155ed8878..06b3240f6f63 100644
--- a/ipld/hamt/Cargo.toml
+++ b/ipld/hamt/Cargo.toml
@@ -30,6 +30,7 @@ murmur = ["murmur3"]
 [dev-dependencies]
 hex = "0.4.2"
 criterion = "0.3.3"
+ipld_blockstore = { path = "../blockstore", features = ["tracking"] }
 
 [[bench]]
 name = "hamt_beckmark"
diff --git a/ipld/hamt/tests/hamt_tests.rs b/ipld/hamt/tests/hamt_tests.rs
index 36a5fb0de21d..51de431af93c 100644
--- a/ipld/hamt/tests/hamt_tests.rs
+++ b/ipld/hamt/tests/hamt_tests.rs
@@ -3,13 +3,8 @@
 
 use ipld_hamt::{BytesKey, Hamt};
 
-#[cfg(feature = "murmur")]
 use cid::multihash::Blake2b256;
-#[cfg(feature = "murmur")]
-use ipld_blockstore::BlockStore;
-#[cfg(feature = "murmur")]
-use ipld_hamt::Murmur3;
-#[cfg(feature = "murmur")]
+use ipld_blockstore::{BSStats, BlockStore, TrackingBlockStore};
 use serde_bytes::ByteBuf;
 
 #[cfg(feature = "identity")]
@@ -66,12 +61,11 @@ fn test_load() {
 }
 
 #[test]
-#[cfg(feature = "murmur")]
 fn delete() {
-    let store = db::MemoryDB::default();
+    let mem = db::MemoryDB::default();
+    let store = TrackingBlockStore::new(&mem);
 
-    // ! Note that bytes must be specifically indicated serde_bytes type
-    let mut hamt: Hamt<_, _, BytesKey, Murmur3> = Hamt::new(&store);
+    let mut hamt: Hamt<_, _> = Hamt::new(&store);
     let (v1, v2, v3): (&[u8], &[u8], &[u8]) = (
         b"cat dog bear".as_ref(),
         b"cat dog".as_ref(),
@@ -83,47 +77,49 @@ fn delete() {
 
     let c = hamt.flush().unwrap();
     assert_eq!(
-        hex::encode(c.to_bytes()),
-        "0171a0e402204c4cec750f4e5fc0df61e5a6b6f430d45e6d42108824492658ccd480a4f86aef"
+        c.to_string().as_str(),
+        "bafy2bzacebhjoag2qmyibmvvzq372pg2evlkchovqdksmna4hm7py5itnrlhg"
     );
 
-    let mut h2 = Hamt::<_, ByteBuf, BytesKey, Murmur3>::load(&c, &store).unwrap();
+    let mut h2 = Hamt::<_, ByteBuf>::load(&c, &store).unwrap();
     assert_eq!(h2.delete(&b"foo".to_vec()).unwrap(), true);
     assert_eq!(h2.get(&b"foo".to_vec()).unwrap(), None);
 
-    // Assert previous hamt still has access
-    assert_eq!(hamt.get(&b"foo".to_vec()).unwrap(), Some(ByteBuf::from(v1)));
-
     let c2 = h2.flush().unwrap();
     assert_eq!(
-        hex::encode(c2.to_bytes()),
-        "0171a0e40220f8889d65614928ee8fd0a1fc27fb94357751ce95e99260b16b8789455eb7d212"
+        c2.to_string().as_str(),
+        "bafy2bzaceczehhtzfhg4ijrkv2omajt5ygwbd6srqhhtkxgd2hjttpihxs5ky"
     );
+    #[rustfmt::skip]
+    assert_eq!(*store.stats.borrow(), BSStats {r: 1, w: 2, br: 88, bw: 154});
 }
 
 #[test]
-#[cfg(feature = "murmur")]
 fn reload_empty() {
-    let store = db::MemoryDB::default();
+    let mem = db::MemoryDB::default();
+    let store = TrackingBlockStore::new(&mem);
 
-    let hamt: Hamt<_, (), BytesKey, Murmur3> = Hamt::new(&store);
+    let hamt: Hamt<_, ()> = Hamt::new(&store);
     let c = store.put(&hamt, Blake2b256).unwrap();
-    assert_eq!(
-        hex::encode(c.to_bytes()),
-        "0171a0e4022018fe6acc61a3a36b0c373c4a3a8ea64b812bf2ca9b528050909c78d408558a0c"
-    );
-    let h2 = Hamt::<_, (), BytesKey, Murmur3>::load(&c, &store).unwrap();
+
+    let h2 = Hamt::<_, ()>::load(&c, &store).unwrap();
     let c2 = store.put(&h2, Blake2b256).unwrap();
     assert_eq!(c, c2);
+    assert_eq!(
+        c.to_string().as_str(),
+        "bafy2bzaceamp42wmmgr2g2ymg46euououzfyck7szknvfacqscohrvaikwfay"
+    );
+    #[rustfmt::skip]
+    assert_eq!(*store.stats.borrow(), BSStats {r: 1, w: 2, br: 3, bw: 6});
 }
 
 #[test]
-#[cfg(feature = "murmur")]
 fn set_delete_many() {
-    let store = db::MemoryDB::default();
+    let mem = db::MemoryDB::default();
+    let store = TrackingBlockStore::new(&mem);
 
     // Test vectors setup specifically for bit width of 5
-    let mut hamt: Hamt<_, _, BytesKey, Murmur3> = Hamt::new_with_bit_width(&store, 5);
+    let mut hamt: Hamt<_, _> = Hamt::new_with_bit_width(&store, 5);
 
     for i in 0..200 {
         hamt.set(format!("{}", i).into_bytes().into(), i).unwrap();
@@ -174,7 +170,8 @@ fn add_and_remove_keys(
         .map(|(i, k)| (k.to_vec().into(), i as u8))
         .collect();
 
-    let store = db::MemoryDB::default();
+    let mem = db::MemoryDB::default();
+    let store = TrackingBlockStore::new(&mem);
 
     let mut hamt: Hamt<_, _, _, Identity> = Hamt::new_with_bit_width(&store, bit_width);
 

From 054b10b147be11d4ad795cced6344e928dadf252 Mon Sep 17 00:00:00 2001
From: austinabell <austinabell8@gmail.com>
Date: Mon, 5 Oct 2020 09:39:08 -0400
Subject: [PATCH 2/8] Updated current test cases

---
 ipld/hamt/tests/hamt_tests.rs | 71 ++++++++++++++++++++++++++---------
 1 file changed, 54 insertions(+), 17 deletions(-)

diff --git a/ipld/hamt/tests/hamt_tests.rs b/ipld/hamt/tests/hamt_tests.rs
index 51de431af93c..50b58a1b48aa 100644
--- a/ipld/hamt/tests/hamt_tests.rs
+++ b/ipld/hamt/tests/hamt_tests.rs
@@ -127,8 +127,8 @@ fn set_delete_many() {
 
     let c1 = hamt.flush().unwrap();
     assert_eq!(
-        hex::encode(c1.to_bytes()),
-        "0171a0e402207c660382de99c174ce39517bdbd28f3967801aebbd9795f0591e226d93e2f010"
+        c1.to_string().as_str(),
+        "bafy2bzaceaneyzybb37pn4rtg2mvn2qxb43rhgmqoojgtz7avdfjw2lhz4dge"
     );
 
     for i in 200..400 {
@@ -137,8 +137,8 @@ fn set_delete_many() {
 
     let cid_all = hamt.flush().unwrap();
     assert_eq!(
-        hex::encode(cid_all.to_bytes()),
-        "0171a0e40220dba161623db24093bd90e00c3d185bae8468f8d3e81f01f112b3afe47e603fd1"
+        cid_all.to_string().as_str(),
+        "bafy2bzaceaqmub32nf33s3joo6x2l3schxreuow7jkla7a27l7qcrsb2elzay"
     );
 
     for i in 200..400 {
@@ -151,9 +151,11 @@ fn set_delete_many() {
 
     let cid_d = hamt.flush().unwrap();
     assert_eq!(
-        hex::encode(cid_d.to_bytes()),
-        "0171a0e402207c660382de99c174ce39517bdbd28f3967801aebbd9795f0591e226d93e2f010"
+        cid_d.to_string().as_str(),
+        "bafy2bzaceaneyzybb37pn4rtg2mvn2qxb43rhgmqoojgtz7avdfjw2lhz4dge"
     );
+    #[rustfmt::skip]
+    assert_eq!(*store.stats.borrow(), BSStats {r: 87, w: 119, br: 7671, bw: 14042});
 }
 
 #[cfg(feature = "identity")]
@@ -162,6 +164,7 @@ fn add_and_remove_keys(
     keys: &[&[u8]],
     extra_keys: &[&[u8]],
     expected: &'static str,
+    stats: BSStats,
 ) {
     let all: Vec<(BytesKey, u8)> = keys
         .iter()
@@ -201,47 +204,81 @@ fn add_and_remove_keys(
     let cid1 = h1.flush().unwrap();
     let cid2 = h2.flush().unwrap();
     assert_eq!(cid1, cid2);
-    assert_eq!(hex::encode(cid1.to_bytes()), expected);
+    assert_eq!(cid1.to_string().as_str(), expected);
+    assert_eq!(*store.stats.borrow(), stats);
 }
 
 #[test]
 #[cfg(feature = "identity")]
 fn canonical_structure() {
     // Champ mutation semantics test
+    #[rustfmt::skip]
     add_and_remove_keys(
         DEFAULT_BIT_WIDTH,
         &[b"K"],
         &[b"B"],
-        "0171a0e402208683c5cd09bc6c1df93d100bee677d7a6bbe8db0b340361866e3fb20fb0a981e",
+        "bafy2bzacecdihronbg6gyhpzhuiax3thpv5gxpunwczuanqym3r7wih3bkmb4",
+        BSStats {r: 2, w: 5, br: 42, bw: 105},
     );
+    #[rustfmt::skip]
     add_and_remove_keys(
         DEFAULT_BIT_WIDTH,
         &[b"K0", b"K1", b"KAA1", b"KAA2", b"KAA3"],
         &[b"KAA4"],
-        "0171a0e40220e2a9e53c77d146010b60f2be9b3ba423c0db4efea06e66bd87e072671c8ef411",
+        "bafy2bzaceaxfdngr56h3kj5hplwslhyxauizcpwx3agwjqns6gjhroepgnfkm",
+        BSStats {r: 2, w: 5, br: 168, bw: 420},
     );
 }
 
 #[test]
 #[cfg(feature = "identity")]
 fn canonical_structure_alt_bit_width() {
+    #[rustfmt::skip]
     let kb_cases = [
-        "0171a0e402209a00d457b7d5d398a225fa837125db401a5eabdf4833352aed48dd28dc6eca56",
-        "0171a0e40220b45f48552b1b802fafcb79b417c4d2972ea42cd24600eaf9a0d1314c7d46c214",
-        "0171a0e40220c4ac32c9bb0dbec96b290d68b1b1fc6e1ddfe33f99420b4b46a078255d997db8",
+        (
+            "bafy2bzacedckymwjxmg35sllfegwrmnr7rxb3x7dh6muec2li2qhqjk5tf63q",
+            BSStats {r: 2, w: 5, br: 32, bw: 80},
+        ),
+        (
+            "bafy2bzacec2f6scvfmnyal5pzn43if6e2kls5jbm2jdab2xzuditctd5i3bbi",
+            BSStats {r: 2, w: 5, br: 28, bw: 70},
+        ),
+        (
+            "bafy2bzacecnabvcxw7k5hgfcex5ig4jf3nabuxvl35edgnjk5ven2kg4n3ffm",
+            BSStats {r: 2, w: 5, br: 26, bw: 65},
+        ),
     ];
+    #[rustfmt::skip]
     let other_cases = [
-        "0171a0e40220c5f39f53c67de67dbf8a058b699fb1e4673d78a5f6a0dc59583f9a175db234e3",
-        "0171a0e40220c84814bb7fdbb71a17ac24b0eb110a38e4e79c93fccaa6d87fa9e5aa771bb453",
-        "0171a0e4022094833c20da84ad6e18a603a47aa143e3393171d45786eddc5b182ae647dafd64",
+        (
+            "bafy2bzaceckigpba3kck23qyuyb2i6vbiprtsmlr2rlyn3o4lmmcvzsh3l6wi",
+            BSStats {r: 8, w: 13, br: 419, bw: 687},
+        ),
+        (
+            "bafy2bzacedeeqff3p7n3ogqxvqslb2yrbi4ojz44sp6mvjwyp6u6lktxdo2fg",
+            BSStats {r: 8, w: 13, br: 385, bw: 639},
+        ),
+        (
+            "bafy2bzacedc7hh2tyz66m7n7ricyw2m7whsgoplyux3kbxczla7zuf25wi2og",
+            BSStats {r: 9, w: 14, br: 420, bw: 661},
+        ),
     ];
     for i in 5..8 {
-        add_and_remove_keys(i, &[b"K"], &[b"B"], kb_cases[(i - 5) as usize]);
+        #[rustfmt::skip]
+        add_and_remove_keys(
+            i,
+            &[b"K"],
+            &[b"B"],
+            kb_cases[(i - 5) as usize].0,
+            kb_cases[(i - 5) as usize].1,
+        );
+        #[rustfmt::skip]
         add_and_remove_keys(
             i,
             &[b"K0", b"K1", b"KAA1", b"KAA2", b"KAA3"],
             &[b"KAA4"],
-            other_cases[(i - 5) as usize],
+            other_cases[(i - 5) as usize].0,
+            other_cases[(i - 5) as usize].1,
         );
     }
 }

From c4c987d7004d6e765f37e09b6136a753c17709e5 Mon Sep 17 00:00:00 2001
From: austinabell <austinabell8@gmail.com>
Date: Mon, 5 Oct 2020 15:23:50 -0400
Subject: [PATCH 3/8] Update tests and fix child reordering edge case

---
 Cargo.lock                    |  1 +
 ipld/hamt/Cargo.toml          |  1 +
 ipld/hamt/src/node.rs         |  2 +-
 ipld/hamt/src/pointer.rs      | 24 ++++++++++----
 ipld/hamt/tests/hamt_tests.rs | 61 ++++++++++++++++++++++++++++-------
 5 files changed, 70 insertions(+), 19 deletions(-)

diff --git a/Cargo.lock b/Cargo.lock
index 734e7d6c2b27..489841c7dfd9 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -2976,6 +2976,7 @@ dependencies = [
  "serde_bytes",
  "sha2 0.9.1",
  "thiserror",
+ "unsigned-varint 0.5.1",
 ]
 
 [[package]]
diff --git a/ipld/hamt/Cargo.toml b/ipld/hamt/Cargo.toml
index 06b3240f6f63..882db5e19cdd 100644
--- a/ipld/hamt/Cargo.toml
+++ b/ipld/hamt/Cargo.toml
@@ -31,6 +31,7 @@ murmur = ["murmur3"]
 hex = "0.4.2"
 criterion = "0.3.3"
 ipld_blockstore = { path = "../blockstore", features = ["tracking"] }
+unsigned-varint = "0.5"
 
 [[bench]]
 name = "hamt_beckmark"
diff --git a/ipld/hamt/src/node.rs b/ipld/hamt/src/node.rs
index b5a984b2340d..fb858e9af661 100644
--- a/ipld/hamt/src/node.rs
+++ b/ipld/hamt/src/node.rs
@@ -235,7 +235,7 @@ where
                     let mut sub = Node::default();
                     let consumed = hashed_key.consumed;
                     sub.modify_value(hashed_key, bit_width, depth + 1, key, value, store)?;
-                    let kvs = std::mem::replace(vals, Vec::new());
+                    let kvs = std::mem::take(vals);
                     for p in kvs.into_iter() {
                         let hash = H::hash(p.key());
                         sub.modify_value(
diff --git a/ipld/hamt/src/pointer.rs b/ipld/hamt/src/pointer.rs
index 43e84018d891..14b02edb53c0 100644
--- a/ipld/hamt/src/pointer.rs
+++ b/ipld/hamt/src/pointer.rs
@@ -2,11 +2,12 @@
 // SPDX-License-Identifier: Apache-2.0, MIT
 
 use super::node::Node;
-use super::{Error, KeyValuePair, MAX_ARRAY_WIDTH};
+use super::{Error, Hash, HashAlgorithm, KeyValuePair, MAX_ARRAY_WIDTH};
 use cid::Cid;
 use serde::de::{self, DeserializeOwned};
 use serde::ser;
 use serde::{Deserialize, Deserializer, Serialize, Serializer};
+use std::cmp::Ordering;
 
 /// Pointer to index values or a link to another child node.
 #[derive(Debug, Clone)]
@@ -92,8 +93,9 @@ impl<K, V, H> Default for Pointer<K, V, H> {
 
 impl<K, V, H> Pointer<K, V, H>
 where
-    K: Serialize + DeserializeOwned + Clone,
+    K: Serialize + DeserializeOwned + Clone + Hash + PartialOrd,
     V: Serialize + DeserializeOwned + Clone,
+    H: HashAlgorithm,
 {
     pub(crate) fn from_key_value(key: K, value: V) -> Self {
         Pointer::Values(vec![KeyValuePair::new(key, value)])
@@ -107,11 +109,12 @@ where
                 0 => Err(Error::ZeroPointers),
                 1 => {
                     // Node has only one pointer, swap with parent node
-                    if let p @ Pointer::Values(_) = &mut n.pointers[0] {
-                        // Only creating temp value to get around borrowing self mutably twice
-                        let mut move_pointer = Pointer::Values(Default::default());
-                        std::mem::swap(&mut move_pointer, p);
-                        *self = move_pointer
+                    if let Pointer::Values(vals) = &mut n.pointers[0] {
+                        // Take child values and sort, to ensure canonical ordering
+                        let values = std::mem::take(vals);
+
+                        // move parent node up
+                        *self = Pointer::Values(values)
                     }
                     Ok(())
                 }
@@ -127,12 +130,19 @@ where
                                     // Child values cannot be fit into parent node, keep as is
                                     return Ok(());
                                 }
+                                // TODO avoid clone by checking length before removing
                                 child_vals.push(kv.clone());
                             }
                         } else {
                             return Ok(());
                         }
                     }
+                    // Sorting by key, values are inserted based on the ordering of the key itself,
+                    // so when collapsed, it needs to be ensured that this order is equal.
+                    child_vals.sort_unstable_by(|a, b| {
+                        a.key().partial_cmp(b.key()).unwrap_or(Ordering::Equal)
+                    });
+
                     // Replace link node with child values
                     *self = Pointer::Values(child_vals);
                     Ok(())
diff --git a/ipld/hamt/tests/hamt_tests.rs b/ipld/hamt/tests/hamt_tests.rs
index 50b58a1b48aa..67651e17da54 100644
--- a/ipld/hamt/tests/hamt_tests.rs
+++ b/ipld/hamt/tests/hamt_tests.rs
@@ -218,15 +218,15 @@ fn canonical_structure() {
         &[b"K"],
         &[b"B"],
         "bafy2bzacecdihronbg6gyhpzhuiax3thpv5gxpunwczuanqym3r7wih3bkmb4",
-        BSStats {r: 2, w: 5, br: 42, bw: 105},
+        BSStats {r: 2, w: 4, br: 42, bw: 84},
     );
     #[rustfmt::skip]
     add_and_remove_keys(
         DEFAULT_BIT_WIDTH,
         &[b"K0", b"K1", b"KAA1", b"KAA2", b"KAA3"],
         &[b"KAA4"],
-        "bafy2bzaceaxfdngr56h3kj5hplwslhyxauizcpwx3agwjqns6gjhroepgnfkm",
-        BSStats {r: 2, w: 5, br: 168, bw: 420},
+        "bafy2bzacedrktzj4o7iumailmdzl5gz3uqr4bw2o72qg4zv5q7qhezy4r32bc",
+        BSStats {r: 7, w: 10, br: 388, bw: 561},
     );
 }
 
@@ -236,31 +236,31 @@ fn canonical_structure_alt_bit_width() {
     #[rustfmt::skip]
     let kb_cases = [
         (
-            "bafy2bzacedckymwjxmg35sllfegwrmnr7rxb3x7dh6muec2li2qhqjk5tf63q",
-            BSStats {r: 2, w: 5, br: 32, bw: 80},
+            "bafy2bzacecnabvcxw7k5hgfcex5ig4jf3nabuxvl35edgnjk5ven2kg4n3ffm",
+            BSStats {r: 2, w: 5, br: 26, bw: 65},
         ),
         (
             "bafy2bzacec2f6scvfmnyal5pzn43if6e2kls5jbm2jdab2xzuditctd5i3bbi",
             BSStats {r: 2, w: 5, br: 28, bw: 70},
         ),
         (
-            "bafy2bzacecnabvcxw7k5hgfcex5ig4jf3nabuxvl35edgnjk5ven2kg4n3ffm",
-            BSStats {r: 2, w: 5, br: 26, bw: 65},
+            "bafy2bzacedckymwjxmg35sllfegwrmnr7rxb3x7dh6muec2li2qhqjk5tf63q",
+            BSStats {r: 2, w: 5, br: 32, bw: 80},
         ),
     ];
     #[rustfmt::skip]
     let other_cases = [
         (
-            "bafy2bzaceckigpba3kck23qyuyb2i6vbiprtsmlr2rlyn3o4lmmcvzsh3l6wi",
-            BSStats {r: 8, w: 13, br: 419, bw: 687},
+            "bafy2bzacedc7hh2tyz66m7n7ricyw2m7whsgoplyux3kbxczla7zuf25wi2og",
+            BSStats {r: 9, w: 14, br: 420, bw: 661},
         ),
         (
             "bafy2bzacedeeqff3p7n3ogqxvqslb2yrbi4ojz44sp6mvjwyp6u6lktxdo2fg",
             BSStats {r: 8, w: 13, br: 385, bw: 639},
         ),
         (
-            "bafy2bzacedc7hh2tyz66m7n7ricyw2m7whsgoplyux3kbxczla7zuf25wi2og",
-            BSStats {r: 9, w: 14, br: 420, bw: 661},
+            "bafy2bzaceckigpba3kck23qyuyb2i6vbiprtsmlr2rlyn3o4lmmcvzsh3l6wi",
+            BSStats {r: 8, w: 13, br: 419, bw: 687},
         ),
     ];
     for i in 5..8 {
@@ -282,3 +282,42 @@ fn canonical_structure_alt_bit_width() {
         );
     }
 }
+
+#[test]
+fn clean_child_ordering() {
+    let make_key = |i: u64| -> BytesKey {
+        let mut key = unsigned_varint::encode::u64_buffer();
+        let n = unsigned_varint::encode::u64(i, &mut key);
+        n.to_vec().into()
+    };
+
+    let dummy_value = BytesKey(vec![0xaa, 0xbb, 0xcc, 0xdd]);
+
+    let mem = db::MemoryDB::default();
+    let store = TrackingBlockStore::new(&mem);
+
+    let mut h: Hamt<_, _> = Hamt::new_with_bit_width(&store, 5);
+
+    for i in 100..195 {
+        h.set(make_key(i), dummy_value.clone()).unwrap();
+    }
+
+    let root = h.flush().unwrap();
+    assert_eq!(
+        root.to_string().as_str(),
+        "bafy2bzaced2mfx4zquihmrbqei2ghtbsf7bvupjzaiwkkgfmvpfrbud25gfli"
+    );
+    let mut h = Hamt::<_, BytesKey>::load_with_bit_width(&root, &store, 5).unwrap();
+
+    h.delete(&make_key(104)).unwrap();
+    h.delete(&make_key(108)).unwrap();
+    let root = h.flush().unwrap();
+    Hamt::<_, BytesKey>::load_with_bit_width(&root, &store, 5).unwrap();
+
+    assert_eq!(
+        root.to_string().as_str(),
+        "bafy2bzacec6ro3q36okye22evifu6h7kwdkjlb4keq6ogpfqivka6myk6wkjo"
+    );
+    #[rustfmt::skip]
+    assert_eq!(*store.stats.borrow(), BSStats {r: 9, w: 17, br: 2327, bw: 2845});
+}

From e5db3f0bb864777cb551886bc4d7e6998432c5ce Mon Sep 17 00:00:00 2001
From: austinabell <austinabell8@gmail.com>
Date: Mon, 5 Oct 2020 17:23:37 -0400
Subject: [PATCH 4/8] Add caching to hamt, update interface and usage

---
 Cargo.lock                           |  1 +
 blockchain/state_manager/src/lib.rs  | 11 ++--
 ipld/hamt/Cargo.toml                 |  1 +
 ipld/hamt/src/bitfield.rs            |  2 +-
 ipld/hamt/src/hamt.rs                | 13 ++---
 ipld/hamt/src/hash_bits.rs           |  2 +-
 ipld/hamt/src/lib.rs                 |  5 +-
 ipld/hamt/src/node.rs                | 63 ++++++++++++++---------
 ipld/hamt/src/pointer.rs             | 77 +++++++++++++++++-----------
 ipld/hamt/tests/hamt_tests.rs        | 19 ++++---
 vm/actor/src/builtin/init/state.rs   |  2 +-
 vm/actor/src/builtin/market/mod.rs   | 26 +++++-----
 vm/actor/src/builtin/market/state.rs |  2 +-
 vm/actor/src/builtin/miner/state.rs  |  9 ++--
 vm/actor/src/builtin/multisig/mod.rs | 68 +++++++++++++-----------
 vm/actor/src/builtin/power/mod.rs    | 55 +++++++++++---------
 vm/actor/src/builtin/power/state.rs  |  6 +--
 vm/actor/src/builtin/verifreg/mod.rs | 33 ++++++------
 vm/actor/src/util/balance_table.rs   | 23 +++------
 vm/actor/tests/balance_table_test.rs | 24 +++------
 vm/actor/tests/market_actor_test.rs  |  2 +-
 vm/state_tree/src/lib.rs             |  6 +--
 22 files changed, 245 insertions(+), 205 deletions(-)

diff --git a/Cargo.lock b/Cargo.lock
index 489841c7dfd9..0f0364eb90e5 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -2971,6 +2971,7 @@ dependencies = [
  "forest_ipld",
  "hex",
  "ipld_blockstore",
+ "lazycell",
  "murmur3",
  "serde",
  "serde_bytes",
diff --git a/blockchain/state_manager/src/lib.rs b/blockchain/state_manager/src/lib.rs
index e8e2e052d268..8ff9df8fe31a 100644
--- a/blockchain/state_manager/src/lib.rs
+++ b/blockchain/state_manager/src/lib.rs
@@ -154,14 +154,15 @@ where
     ) -> Result<(power::Claim, power::Claim), Error> {
         let ps: power::State = self.load_actor_state(&*STORAGE_POWER_ACTOR_ADDR, state_cid)?;
 
-        let cm = make_map_with_root(&ps.claims, self.bs.as_ref())
+        let cm = make_map_with_root::<_, power::Claim>(&ps.claims, self.bs.as_ref())
             .map_err(|e| Error::State(e.to_string()))?;
-        let claim: power::Claim = cm
+        let claim = cm
             .get(&addr.to_bytes())
             .map_err(|e| Error::State(e.to_string()))?
             .ok_or_else(|| {
                 Error::State("Failed to retrieve claimed power from actor state".to_owned())
-            })?;
+            })?
+            .clone();
         Ok((
             claim,
             power::Claim {
@@ -800,12 +801,12 @@ where
             escrow: {
                 let et = BalanceTable::from_root(self.bs.as_ref(), &market_state.escrow_table)
                     .map_err(|_x| Error::State("Failed to build Escrow Table".to_string()))?;
-                et.get(&new_addr).unwrap_or_default()
+                et.get(&new_addr).map(Clone::clone).unwrap_or_default()
             },
             locked: {
                 let lt = BalanceTable::from_root(self.bs.as_ref(), &market_state.locked_table)
                     .map_err(|_x| Error::State("Failed to build Locked Table".to_string()))?;
-                lt.get(&new_addr).unwrap_or_default()
+                lt.get(&new_addr).map(Clone::clone).unwrap_or_default()
             },
         };
 
diff --git a/ipld/hamt/Cargo.toml b/ipld/hamt/Cargo.toml
index 882db5e19cdd..b252f072bacb 100644
--- a/ipld/hamt/Cargo.toml
+++ b/ipld/hamt/Cargo.toml
@@ -15,6 +15,7 @@ forest_ipld = { path = "../" }
 serde_bytes = "0.11.3"
 thiserror = "1.0"
 sha2 = "0.9.1"
+lazycell = "1.2.1"
 
 # TODO remove murmur3 support when finalized in Filecoin
 [dependencies.murmur3]
diff --git a/ipld/hamt/src/bitfield.rs b/ipld/hamt/src/bitfield.rs
index 81a6c5bb9655..fac99c10be31 100644
--- a/ipld/hamt/src/bitfield.rs
+++ b/ipld/hamt/src/bitfield.rs
@@ -9,7 +9,7 @@ use forest_encoding::{
 };
 use std::u64;
 
-#[derive(Debug, Clone, PartialEq, Eq)]
+#[derive(Debug, Clone, PartialEq, Eq, Copy)]
 pub struct Bitfield([u64; 4]);
 
 impl Serialize for Bitfield {
diff --git a/ipld/hamt/src/hamt.rs b/ipld/hamt/src/hamt.rs
index e33b69f90b90..39b7ef376d87 100644
--- a/ipld/hamt/src/hamt.rs
+++ b/ipld/hamt/src/hamt.rs
@@ -60,8 +60,8 @@ impl<'a, K: PartialEq, V: PartialEq, S: BlockStore, H: HashAlgorithm> PartialEq
 
 impl<'a, BS, V, K, H> Hamt<'a, BS, V, K, H>
 where
-    K: Hash + Eq + PartialOrd + Serialize + DeserializeOwned + Clone,
-    V: Serialize + DeserializeOwned + Clone,
+    K: Hash + Eq + PartialOrd + Serialize + DeserializeOwned,
+    V: Serialize + DeserializeOwned,
     BS: BlockStore,
     H: HashAlgorithm,
 {
@@ -156,7 +156,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>
     where
         K: Borrow<Q>,
         Q: Hash + Eq,
@@ -214,15 +214,12 @@ where
     /// assert_eq!(map.delete(&1).unwrap(), true);
     /// assert_eq!(map.delete(&1).unwrap(), false);
     /// ```
-    pub fn delete<Q: ?Sized>(&mut self, k: &Q) -> Result<bool, Error>
+    pub fn delete<Q: ?Sized>(&mut self, k: &Q) -> Result<Option<(K, V)>, Error>
     where
         K: Borrow<Q>,
         Q: Hash + Eq,
     {
-        match self.root.remove_entry(k, self.store, self.bit_width)? {
-            Some(_) => Ok(true),
-            None => Ok(false),
-        }
+        self.root.remove_entry(k, self.store, self.bit_width)
     }
 
     /// Flush root and return Cid for hamt
diff --git a/ipld/hamt/src/hash_bits.rs b/ipld/hamt/src/hash_bits.rs
index b7c762f79593..7dbcf038b1cb 100644
--- a/ipld/hamt/src/hash_bits.rs
+++ b/ipld/hamt/src/hash_bits.rs
@@ -5,7 +5,7 @@ use crate::{Error, HashedKey};
 use std::cmp::Ordering;
 
 /// Helper struct which indexes and allows returning bits from a hashed key
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, Copy)]
 pub struct HashBits<'a> {
     b: &'a HashedKey,
     pub consumed: u32,
diff --git a/ipld/hamt/src/lib.rs b/ipld/hamt/src/lib.rs
index d0a7b527025a..a3cf341cc0ec 100644
--- a/ipld/hamt/src/lib.rs
+++ b/ipld/hamt/src/lib.rs
@@ -35,13 +35,16 @@ const DEFAULT_BIT_WIDTH: u32 = 8;
 
 type HashedKey = [u8; 32];
 
-#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)]
+#[derive(Debug, Serialize, Deserialize, PartialEq)]
 struct KeyValuePair<K, V>(K, V);
 
 impl<K, V> KeyValuePair<K, V> {
     pub fn key(&self) -> &K {
         &self.0
     }
+    pub fn value(&self) -> &V {
+        &self.1
+    }
 }
 
 impl<K, V> KeyValuePair<K, V> {
diff --git a/ipld/hamt/src/node.rs b/ipld/hamt/src/node.rs
index fb858e9af661..2beee6ce61bf 100644
--- a/ipld/hamt/src/node.rs
+++ b/ipld/hamt/src/node.rs
@@ -15,7 +15,7 @@ use std::fmt::Debug;
 use std::marker::PhantomData;
 
 /// Node in Hamt tree which contains bitfield of set indexes and pointers to nodes
-#[derive(Debug, Clone)]
+#[derive(Debug)]
 pub(crate) struct Node<K, V, H> {
     pub(crate) bitfield: Bitfield,
     pub(crate) pointers: Vec<Pointer<K, V, H>>,
@@ -71,9 +71,9 @@ impl<K, V, H> Default for Node<K, V, H> {
 
 impl<K, V, H> Node<K, V, H>
 where
-    K: Hash + Eq + PartialOrd + Serialize + DeserializeOwned + Clone,
+    K: Hash + Eq + PartialOrd + Serialize + DeserializeOwned,
     H: HashAlgorithm,
-    V: Serialize + DeserializeOwned + Clone,
+    V: Serialize + DeserializeOwned,
 {
     pub fn set<S: BlockStore>(
         &mut self,
@@ -92,12 +92,12 @@ where
         k: &Q,
         store: &S,
         bit_width: u32,
-    ) -> Result<Option<V>, Error>
+    ) -> Result<Option<&V>, Error>
     where
         K: Borrow<Q>,
         Q: Eq + Hash,
     {
-        Ok(self.search(k, store, bit_width)?.map(|kv| kv.1))
+        Ok(self.search(k, store, bit_width)?.map(|kv| kv.value()))
     }
 
     #[inline]
@@ -127,13 +127,13 @@ where
     {
         for p in &self.pointers {
             match p {
-                Pointer::Link(cid) => {
+                Pointer::Link { cid, .. } => {
                     match store.get::<Node<K, V, H>>(cid).map_err(|e| e.to_string())? {
                         Some(node) => node.for_each(store, f)?,
                         None => return Err(format!("Node with cid {} not found", cid).into()),
                     }
                 }
-                Pointer::Cache(n) => n.for_each(store, f)?,
+                Pointer::Dirty(n) => n.for_each(store, f)?,
                 Pointer::Values(kvs) => {
                     for kv in kvs {
                         f(kv.0.borrow(), kv.1.borrow())?;
@@ -150,7 +150,7 @@ where
         q: &Q,
         store: &S,
         bit_width: u32,
-    ) -> Result<Option<KeyValuePair<K, V>>, Error>
+    ) -> Result<Option<&KeyValuePair<K, V>>, Error>
     where
         K: Borrow<Q>,
         Q: Eq + Hash,
@@ -166,7 +166,7 @@ where
         depth: usize,
         key: &Q,
         store: &S,
-    ) -> Result<Option<KeyValuePair<K, V>>, Error>
+    ) -> Result<Option<&KeyValuePair<K, V>>, Error>
     where
         K: Borrow<Q>,
         Q: Eq + Hash,
@@ -180,12 +180,24 @@ where
         let cindex = self.index_for_bit_pos(idx);
         let child = self.get_child(cindex);
         match child {
-            Pointer::Link(cid) => match store.get::<Node<K, V, H>>(cid)? {
-                Some(node) => Ok(node.get_value(hashed_key, bit_width, depth + 1, key, store)?),
-                None => Err(Error::CidNotFound(cid.to_string())),
-            },
-            Pointer::Cache(n) => n.get_value(hashed_key, bit_width, depth + 1, key, store),
-            Pointer::Values(vals) => Ok(vals.iter().find(|kv| key.eq(kv.key().borrow())).cloned()),
+            Pointer::Link { cid, cache } => {
+                if let Some(cached_node) = cache.borrow() {
+                    // Link node is cached
+                    cached_node.get_value(hashed_key, bit_width, depth + 1, key, store)
+                } else {
+                    // Link is not cached, need to load and fill cache, then traverse for value.
+                    let node = store
+                        .get::<Box<Node<K, V, H>>>(cid)?
+                        .ok_or_else(|| Error::CidNotFound(cid.to_string()))?;
+
+                    // Intentionally ignoring error, cache will always be the same.
+                    let _ = cache.fill(node);
+                    let cache_node = cache.borrow().expect("cache filled on line above");
+                    cache_node.get_value(hashed_key, bit_width, depth + 1, key, store)
+                }
+            }
+            Pointer::Dirty(n) => n.get_value(hashed_key, bit_width, depth + 1, key, store),
+            Pointer::Values(vals) => Ok(vals.iter().find(|kv| key.eq(kv.key().borrow()))),
         }
     }
 
@@ -211,16 +223,16 @@ where
         let child = self.get_child_mut(cindex);
 
         match child {
-            Pointer::Link(cid) => match store.get::<Node<K, V, H>>(cid)? {
+            Pointer::Link { cid, .. } => match store.get::<Node<K, V, H>>(cid)? {
                 Some(mut node) => {
                     // Pull value from store and update to cached node
                     node.modify_value(hashed_key, bit_width, depth + 1, key, value, store)?;
-                    *child = Pointer::Cache(Box::new(node));
+                    *child = Pointer::Dirty(Box::new(node));
                     Ok(())
                 }
                 None => Err(Error::CidNotFound(cid.to_string())),
             },
-            Pointer::Cache(n) => {
+            Pointer::Dirty(n) => {
                 Ok(n.modify_value(hashed_key, bit_width, depth + 1, key, value, store)?)
             }
             Pointer::Values(vals) => {
@@ -248,7 +260,7 @@ where
                         )?;
                     }
 
-                    *child = Pointer::Cache(Box::new(sub));
+                    *child = Pointer::Dirty(Box::new(sub));
                     return Ok(());
                 }
 
@@ -291,11 +303,11 @@ where
         let child = self.get_child_mut(cindex);
 
         match child {
-            Pointer::Link(cid) => match store.get::<Node<K, V, H>>(cid)? {
+            Pointer::Link { cid, .. } => match store.get::<Node<K, V, H>>(cid)? {
                 Some(mut node) => {
                     // Pull value from store and update to cached node
                     let del = node.rm_value(hashed_key, bit_width, depth + 1, key, store)?;
-                    *child = Pointer::Cache(Box::new(node));
+                    *child = Pointer::Dirty(Box::new(node));
 
                     // Clean to retrieve canonical form
                     child.clean()?;
@@ -303,7 +315,7 @@ where
                 }
                 None => Err(Error::CidNotFound(cid.to_string())),
             },
-            Pointer::Cache(n) => {
+            Pointer::Dirty(n) => {
                 // Delete value and return deleted value
                 let deleted = n.rm_value(hashed_key, bit_width, depth + 1, key, store)?;
 
@@ -335,7 +347,7 @@ where
 
     pub fn flush<S: BlockStore>(&mut self, store: &S) -> Result<(), Error> {
         for pointer in &mut self.pointers {
-            if let Pointer::Cache(node) = pointer {
+            if let Pointer::Dirty(node) = pointer {
                 // Flush cached sub node to clear it's cache
                 node.flush(store)?;
 
@@ -343,7 +355,10 @@ where
                 let cid = store.put(node, Blake2b256)?;
 
                 // Replace cached node with Cid link
-                *pointer = Pointer::Link(cid);
+                *pointer = Pointer::Link {
+                    cid,
+                    cache: Default::default(),
+                };
             }
         }
 
diff --git a/ipld/hamt/src/pointer.rs b/ipld/hamt/src/pointer.rs
index 14b02edb53c0..d5a21ff134ad 100644
--- a/ipld/hamt/src/pointer.rs
+++ b/ipld/hamt/src/pointer.rs
@@ -4,25 +4,29 @@
 use super::node::Node;
 use super::{Error, Hash, HashAlgorithm, KeyValuePair, MAX_ARRAY_WIDTH};
 use cid::Cid;
+use lazycell::LazyCell;
 use serde::de::{self, DeserializeOwned};
 use serde::ser;
 use serde::{Deserialize, Deserializer, Serialize, Serializer};
 use std::cmp::Ordering;
 
 /// Pointer to index values or a link to another child node.
-#[derive(Debug, Clone)]
+#[derive(Debug)]
 pub(crate) enum Pointer<K, V, H> {
     Values(Vec<KeyValuePair<K, V>>),
-    Link(Cid),
-    Cache(Box<Node<K, V, H>>),
+    Link {
+        cid: Cid,
+        cache: LazyCell<Box<Node<K, V, H>>>,
+    },
+    Dirty(Box<Node<K, V, H>>),
 }
 
 impl<K: PartialEq, V: PartialEq, H> PartialEq for Pointer<K, V, H> {
     fn eq(&self, other: &Self) -> bool {
         match (self, other) {
             (&Pointer::Values(ref a), &Pointer::Values(ref b)) => a == b,
-            (&Pointer::Link(ref a), &Pointer::Link(ref b)) => a == b,
-            (&Pointer::Cache(ref a), &Pointer::Cache(ref b)) => a == b,
+            (&Pointer::Link { cid: ref a, .. }, &Pointer::Link { cid: ref b, .. }) => a == b,
+            (&Pointer::Dirty(ref a), &Pointer::Dirty(ref b)) => a == b,
             _ => false,
         }
     }
@@ -46,7 +50,7 @@ where
                 };
                 ValsSer { vals }.serialize(serializer)
             }
-            Pointer::Link(cid) => {
+            Pointer::Link { cid, .. } => {
                 #[derive(Serialize)]
                 struct LinkSer<'a> {
                     #[serde(rename = "0")]
@@ -54,7 +58,7 @@ where
                 };
                 LinkSer { cid }.serialize(serializer)
             }
-            Pointer::Cache(_) => Err(ser::Error::custom("Cannot serialize cached values")),
+            Pointer::Dirty(_) => Err(ser::Error::custom("Cannot serialize cached values")),
         }
     }
 }
@@ -79,7 +83,10 @@ where
         let pointer_map = PointerDeser::deserialize(deserializer)?;
         match pointer_map {
             PointerDeser { vals: Some(v), .. } => Ok(Pointer::Values(v)),
-            PointerDeser { cid: Some(cid), .. } => Ok(Pointer::Link(cid)),
+            PointerDeser { cid: Some(cid), .. } => Ok(Pointer::Link {
+                cid,
+                cache: Default::default(),
+            }),
             _ => Err(de::Error::custom("Unexpected pointer serialization")),
         }
     }
@@ -93,8 +100,8 @@ impl<K, V, H> Default for Pointer<K, V, H> {
 
 impl<K, V, H> Pointer<K, V, H>
 where
-    K: Serialize + DeserializeOwned + Clone + Hash + PartialOrd,
-    V: Serialize + DeserializeOwned + Clone,
+    K: Serialize + DeserializeOwned + Hash + PartialOrd,
+    V: Serialize + DeserializeOwned,
     H: HashAlgorithm,
 {
     pub(crate) fn from_key_value(key: K, value: V) -> Self {
@@ -105,12 +112,12 @@ where
     /// after deletes.
     pub(crate) fn clean(&mut self) -> Result<(), Error> {
         match self {
-            Pointer::Cache(n) => match n.pointers.len() {
+            Pointer::Dirty(n) => match n.pointers.len() {
                 0 => Err(Error::ZeroPointers),
                 1 => {
                     // Node has only one pointer, swap with parent node
                     if let Pointer::Values(vals) = &mut n.pointers[0] {
-                        // Take child values and sort, to ensure canonical ordering
+                        // Take child values, to ensure canonical ordering
                         let values = std::mem::take(vals);
 
                         // move parent node up
@@ -119,24 +126,36 @@ where
                     Ok(())
                 }
                 2..=MAX_ARRAY_WIDTH => {
-                    // Iterate over all pointers in cached node to see if it can fit all within
-                    // one values node
-                    let mut child_vals: Vec<KeyValuePair<K, V>> =
-                        Vec::with_capacity(MAX_ARRAY_WIDTH);
-                    for pointer in n.pointers.iter() {
-                        if let Pointer::Values(kvs) = pointer {
-                            for kv in kvs.iter() {
-                                if child_vals.len() == MAX_ARRAY_WIDTH {
-                                    // Child values cannot be fit into parent node, keep as is
-                                    return Ok(());
-                                }
-                                // TODO avoid clone by checking length before removing
-                                child_vals.push(kv.clone());
+                    // If more child values than max width, nothing to change.
+                    let children_len: usize = n
+                        .pointers
+                        .iter()
+                        .filter_map(|p| {
+                            if let Pointer::Values(vals) = p {
+                                Some(vals.len())
+                            } else {
+                                None
                             }
-                        } else {
-                            return Ok(());
-                        }
+                        })
+                        .sum();
+                    if children_len > MAX_ARRAY_WIDTH {
+                        return Ok(());
                     }
+
+                    // Collect values from child nodes to collapse.
+                    let mut child_vals: Vec<KeyValuePair<K, V>> = n
+                        .pointers
+                        .iter_mut()
+                        .filter_map(|p| {
+                            if let Pointer::Values(kvs) = p {
+                                Some(std::mem::take(kvs))
+                            } else {
+                                None
+                            }
+                        })
+                        .flatten()
+                        .collect();
+
                     // Sorting by key, values are inserted based on the ordering of the key itself,
                     // so when collapsed, it needs to be ensured that this order is equal.
                     child_vals.sort_unstable_by(|a, b| {
@@ -149,7 +168,7 @@ where
                 }
                 _ => Ok(()),
             },
-            _ => unreachable!("clean is only called on cached pointer"),
+            _ => unreachable!("clean is only called on dirty pointer"),
         }
     }
 }
diff --git a/ipld/hamt/tests/hamt_tests.rs b/ipld/hamt/tests/hamt_tests.rs
index 67651e17da54..3436d5632627 100644
--- a/ipld/hamt/tests/hamt_tests.rs
+++ b/ipld/hamt/tests/hamt_tests.rs
@@ -19,9 +19,9 @@ fn test_basics() {
     let mut hamt = Hamt::<_, String, _>::new(&store);
     hamt.set(1, "world".to_string()).unwrap();
 
-    assert_eq!(hamt.get(&1).unwrap(), Some("world".to_string()));
+    assert_eq!(hamt.get(&1).unwrap(), Some(&"world".to_string()));
     hamt.set(1, "world2".to_string()).unwrap();
-    assert_eq!(hamt.get(&1).unwrap(), Some("world2".to_string()));
+    assert_eq!(hamt.get(&1).unwrap(), Some(&"world2".to_string()));
 }
 
 #[test]
@@ -31,9 +31,9 @@ fn test_load() {
     let mut hamt: Hamt<_, _, usize> = Hamt::new(&store);
     hamt.set(1, "world".to_string()).unwrap();
 
-    assert_eq!(hamt.get(&1).unwrap(), Some("world".to_string()));
+    assert_eq!(hamt.get(&1).unwrap(), Some(&"world".to_string()));
     hamt.set(1, "world2".to_string()).unwrap();
-    assert_eq!(hamt.get(&1).unwrap(), Some("world2".to_string()));
+    assert_eq!(hamt.get(&1).unwrap(), Some(&"world2".to_string()));
     let c = hamt.flush().unwrap();
 
     let new_hamt = Hamt::load(&c, &store).unwrap();
@@ -82,7 +82,7 @@ fn delete() {
     );
 
     let mut h2 = Hamt::<_, ByteBuf>::load(&c, &store).unwrap();
-    assert_eq!(h2.delete(&b"foo".to_vec()).unwrap(), true);
+    assert!(h2.delete(&b"foo".to_vec()).unwrap().is_some());
     assert_eq!(h2.get(&b"foo".to_vec()).unwrap(), None);
 
     let c2 = h2.flush().unwrap();
@@ -142,11 +142,14 @@ fn set_delete_many() {
     );
 
     for i in 200..400 {
-        assert_eq!(hamt.delete(&format!("{}", i).into_bytes()).unwrap(), true);
+        assert!(hamt
+            .delete(&format!("{}", i).into_bytes())
+            .unwrap()
+            .is_some());
     }
     // Ensure first 200 keys still exist
     for i in 0..200 {
-        assert_eq!(hamt.get(&format!("{}", i).into_bytes()).unwrap(), Some(i));
+        assert_eq!(hamt.get(&format!("{}", i).into_bytes()).unwrap(), Some(&i));
     }
 
     let cid_d = hamt.flush().unwrap();
@@ -187,7 +190,7 @@ fn add_and_remove_keys(
         Hamt::load_with_bit_width(&cid, &store, bit_width).unwrap();
 
     for (k, v) in all {
-        assert_eq!(Some(v), h1.get(&k).unwrap());
+        assert_eq!(Some(&v), h1.get(&k).unwrap());
     }
 
     // Set and delete extra keys
diff --git a/vm/actor/src/builtin/init/state.rs b/vm/actor/src/builtin/init/state.rs
index 25980131b3f0..83ed13b9ce7c 100644
--- a/vm/actor/src/builtin/init/state.rs
+++ b/vm/actor/src/builtin/init/state.rs
@@ -66,7 +66,7 @@ impl State {
 
         let map = make_map_with_root(&self.address_map, store)?;
 
-        Ok(map.get(&addr.to_bytes())?.map(Address::new_id))
+        Ok(map.get(&addr.to_bytes())?.copied().map(Address::new_id))
     }
 }
 
diff --git a/vm/actor/src/builtin/market/mod.rs b/vm/actor/src/builtin/market/mod.rs
index 4d51c03fdc6d..e9ff86a66c0c 100644
--- a/vm/actor/src/builtin/market/mod.rs
+++ b/vm/actor/src/builtin/market/mod.rs
@@ -770,8 +770,7 @@ impl Actor {
                             return Err(actor_error!(ErrIllegalState;
                                         "failed to delete deal proposal: does not exist"));
                         }
-                        let deleted = msm
-                            .pending_deals
+                        msm.pending_deals
                             .as_mut()
                             .unwrap()
                             .delete(&dcid.to_bytes())
@@ -780,17 +779,18 @@ impl Actor {
                                     ExitCode::ErrIllegalState,
                                     "failed to delete pending proposal",
                                 )
+                            })?
+                            .ok_or_else(|| {
+                                actor_error!(
+                                    ErrIllegalState,
+                                    "failed to delete pending proposal: does not exist"
+                                )
                             })?;
-                        if !deleted {
-                            return Err(actor_error!(ErrIllegalState;
-                                            "failed to delete pending proposal: does not exist"));
-                        }
                     }
                     let mut state = state.unwrap();
 
                     if state.last_updated_epoch == EPOCH_UNDEFINED {
-                        let deleted = msm
-                            .pending_deals
+                        msm.pending_deals
                             .as_mut()
                             .unwrap()
                             .delete(&dcid.to_bytes())
@@ -799,11 +799,13 @@ impl Actor {
                                     ExitCode::ErrIllegalState,
                                     "failed to delete pending proposal",
                                 )
+                            })?
+                            .ok_or_else(|| {
+                                actor_error!(
+                                    ErrIllegalState,
+                                    "failed to delete pending proposal: does not exist"
+                                )
                             })?;
-                        if !deleted {
-                            return Err(actor_error!(ErrIllegalState;
-                                    "failed to delete pending proposal: does not exist"));
-                        }
                     }
 
                     let (slash_amount, next_epoch, remove_deal) =
diff --git a/vm/actor/src/builtin/market/state.rs b/vm/actor/src/builtin/market/state.rs
index 0a07408d403d..361dab5ca53e 100644
--- a/vm/actor/src/builtin/market/state.rs
+++ b/vm/actor/src/builtin/market/state.rs
@@ -511,7 +511,7 @@ where
             e.downcast_default(ExitCode::ErrIllegalState, "failed to get escrow balance")
         })?;
 
-        if &prev_locked + amount > escrow_balance {
+        if prev_locked + amount > *escrow_balance {
             return Err(actor_error!(ErrInsufficientFunds;
                     "not enough balance to lock for addr{}: \
                     escrow balance {} < prev locked {} + amount {}", 
diff --git a/vm/actor/src/builtin/miner/state.rs b/vm/actor/src/builtin/miner/state.rs
index 9b6f45d623b1..53da4df5ab6b 100644
--- a/vm/actor/src/builtin/miner/state.rs
+++ b/vm/actor/src/builtin/miner/state.rs
@@ -254,7 +254,7 @@ impl State {
         sector_num: SectorNumber,
     ) -> Result<Option<SectorPreCommitOnChainInfo>, HamtError> {
         let precommitted = make_map_with_root(&self.pre_committed_sectors, store)?;
-        precommitted.get(&u64_key(sector_num))
+        Ok(precommitted.get(&u64_key(sector_num))?.cloned())
     }
 
     /// Gets and returns the requested pre-committed sectors, skipping missing sectors.
@@ -263,7 +263,10 @@ impl State {
         store: &BS,
         sector_numbers: &[SectorNumber],
     ) -> Result<Vec<SectorPreCommitOnChainInfo>, Box<dyn StdError>> {
-        let precommitted = make_map_with_root(&self.pre_committed_sectors, store)?;
+        let precommitted = make_map_with_root::<_, SectorPreCommitOnChainInfo>(
+            &self.pre_committed_sectors,
+            store,
+        )?;
         let mut result = Vec::with_capacity(sector_numbers.len());
 
         for &sector_number in sector_numbers {
@@ -273,7 +276,7 @@ impl State {
                     sector_number
                 ))
             })? {
-                Some(info) => info,
+                Some(info) => info.clone(),
                 None => continue,
             };
 
diff --git a/vm/actor/src/builtin/multisig/mod.rs b/vm/actor/src/builtin/multisig/mod.rs
index 56bfb3681be3..9d70a5fc6062 100644
--- a/vm/actor/src/builtin/multisig/mod.rs
+++ b/vm/actor/src/builtin/multisig/mod.rs
@@ -202,10 +202,10 @@ impl Actor {
 
             // Go implementation holds reference to state after transaction so state must be cloned
             // to match to handle possible exit code inconsistency
-            Ok((st.clone(), txn))
+            Ok((st.clone(), txn.clone()))
         })?;
 
-        let (applied, ret, code) = execute_transaction_if_approved(rt, &st, id, txn.clone())?;
+        let (applied, ret, code) = execute_transaction_if_approved(rt, &st, id, &txn)?;
         if !applied {
             // if the transaction hasn't already been approved, "process" the approval
             // and see if the transaction can be executed
@@ -263,16 +263,19 @@ impl Actor {
                 return Err(actor_error!(ErrIllegalState; "hash does not match proposal params"));
             }
 
-            let deleted = ptx.delete(&params.id.key()).map_err(|e| {
-                e.downcast_default(
-                    ExitCode::ErrIllegalState,
-                    "failed to delete pending transaction",
-                )
-            })?;
-            if !deleted {
-                return Err(actor_error!(ErrIllegalState;
-                    "failed to delete pending transaction: does not exist"));
-            }
+            ptx.delete(&params.id.key())
+                .map_err(|e| {
+                    e.downcast_default(
+                        ExitCode::ErrIllegalState,
+                        "failed to delete pending transaction",
+                    )
+                })?
+                .ok_or_else(|| {
+                    actor_error!(
+                        ErrIllegalState,
+                        "failed to delete pending transaction: does not exist"
+                    )
+                })?;
 
             st.pending_txs = ptx.flush().map_err(|e| {
                 e.downcast_default(
@@ -475,7 +478,7 @@ impl Actor {
             Ok(st.clone())
         })?;
 
-        execute_transaction_if_approved(rt, &st, tx_id, txn)
+        execute_transaction_if_approved(rt, &st, tx_id, &txn)
     }
 }
 
@@ -483,7 +486,7 @@ fn execute_transaction_if_approved<BS, RT>(
     rt: &mut RT,
     st: &State,
     txn_id: TxnID,
-    txn: Transaction,
+    txn: &Transaction,
 ) -> Result<(bool, Serialized, ExitCode), ActorError>
 where
     BS: BlockStore,
@@ -499,7 +502,7 @@ where
                 |e| actor_error!(ErrInsufficientFunds; "insufficient funds unlocked: {}", e),
             )?;
 
-        match rt.send(txn.to, txn.method, txn.params, txn.value) {
+        match rt.send(txn.to, txn.method, txn.params.clone(), txn.value.clone()) {
             Ok(ser) => {
                 out = ser;
             }
@@ -518,16 +521,19 @@ where
                     )
                 })?;
 
-            let deleted = ptx.delete(&txn_id.key()).map_err(|e| {
-                e.downcast_default(
-                    ExitCode::ErrIllegalState,
-                    "failed to delete transaction for cleanup",
-                )
-            })?;
-            if !deleted {
-                return Err(actor_error!(ErrIllegalState;
-                    "failed to delete transaction for cleanup: does not exist"));
-            }
+            ptx.delete(&txn_id.key())
+                .map_err(|e| {
+                    e.downcast_default(
+                        ExitCode::ErrIllegalState,
+                        "failed to delete transaction for cleanup",
+                    )
+                })?
+                .ok_or_else(|| {
+                    actor_error!(
+                        ErrIllegalState,
+                        "failed to delete transaction for cleanup: does not exist"
+                    )
+                })?;
 
             st.pending_txs = ptx.flush().map_err(|e| {
                 e.downcast_default(
@@ -542,23 +548,23 @@ where
     Ok((applied, out, code))
 }
 
-fn get_pending_transaction<'bs, BS: BlockStore>(
-    ptx: &Map<'bs, BS, Transaction>,
+fn get_pending_transaction<'bs, 'm, BS: BlockStore>(
+    ptx: &'m Map<'bs, BS, Transaction>,
     txn_id: TxnID,
-) -> Result<Transaction, Box<dyn StdError>> {
+) -> Result<&'m Transaction, Box<dyn StdError>> {
     Ok(ptx
         .get(&txn_id.key())
         .map_err(|e| e.downcast_wrap("failed to read transaction"))?
         .ok_or_else(|| actor_error!(ErrNotFound, "failed to find transaction: {}", txn_id.0))?)
 }
 
-fn get_transaction<'bs, BS, RT>(
+fn get_transaction<'bs, 'm, BS, RT>(
     rt: &RT,
-    ptx: &Map<'bs, BS, Transaction>,
+    ptx: &'m Map<'bs, BS, Transaction>,
     txn_id: TxnID,
     proposal_hash: Vec<u8>,
     check_hash: bool,
-) -> Result<Transaction, ActorError>
+) -> Result<&'m Transaction, ActorError>
 where
     BS: BlockStore,
     RT: Runtime<BS>,
diff --git a/vm/actor/src/builtin/power/mod.rs b/vm/actor/src/builtin/power/mod.rs
index f1ffb99eff67..d57ad31dea6f 100644
--- a/vm/actor/src/builtin/power/mod.rs
+++ b/vm/actor/src/builtin/power/mod.rs
@@ -277,25 +277,31 @@ impl Actor {
                 e.downcast_default(ExitCode::ErrIllegalState, "failed to load claims")
             })?;
 
-            let claim = get_claim(&claims, &miner_addr)
+            let (raw_byte_power, quality_adj_power) = get_claim(&claims, &miner_addr)
                 .map_err(|e| {
                     e.downcast_default(
                         ExitCode::ErrIllegalState,
                         "failed to read claimed power for fault",
                     )
                 })?
+                .map(|claim| {
+                    (
+                        claim.raw_byte_power.clone(),
+                        claim.quality_adj_power.clone(),
+                    )
+                })
                 .ok_or_else(|| {
                     actor_error!(ErrNotFound;
                         "miner {} not registered (already slashed?)", miner_addr)
                 })?;
-            assert_ne!(claim.raw_byte_power.sign(), Sign::Minus);
-            assert_ne!(claim.quality_adj_power.sign(), Sign::Minus);
+            assert_ne!(raw_byte_power.sign(), Sign::Minus);
+            assert_ne!(quality_adj_power.sign(), Sign::Minus);
 
             st.add_to_claim(
                 &mut claims,
                 &miner_addr,
-                &claim.raw_byte_power.neg(),
-                &claim.quality_adj_power.neg(),
+                &raw_byte_power.neg(),
+                &quality_adj_power.neg(),
             )
             .map_err(|e| {
                 e.downcast_default(
@@ -307,16 +313,21 @@ impl Actor {
             st.add_pledge_total(pledge_delta.neg());
 
             // delete miner actor claims
-            let deleted = claims.delete(&miner_addr.to_bytes()).map_err(|e| {
-                e.downcast_default(
-                    ExitCode::ErrIllegalState,
-                    format!("failed to remove miner {}", miner_addr),
-                )
-            })?;
-            if !deleted {
-                return Err(actor_error!(ErrIllegalState;
-                    "failed to remove miner {}: does not exist", miner_addr));
-            }
+            claims
+                .delete(&miner_addr.to_bytes())
+                .map_err(|e| {
+                    e.downcast_default(
+                        ExitCode::ErrIllegalState,
+                        format!("failed to remove miner {}", miner_addr),
+                    )
+                })?
+                .ok_or_else(|| {
+                    actor_error!(
+                        ErrIllegalState,
+                        "failed to remove miner {}: does not exist",
+                        miner_addr
+                    )
+                })?;
 
             st.miner_count -= 1;
 
@@ -567,7 +578,7 @@ impl Actor {
 
             // Remove power and leave miner frozen
             for miner_addr in failed_miner_crons {
-                let claim = match get_claim(&claims, &miner_addr) {
+                let (rbp, qap) = match get_claim(&claims, &miner_addr) {
                     Err(e) => {
                         log::error!(
                             "failed to get claim for miner {} after \
@@ -584,16 +595,14 @@ impl Actor {
                         );
                         continue;
                     }
-                    Ok(Some(claim)) => claim,
+                    Ok(Some(claim)) => (
+                        claim.raw_byte_power.clone(),
+                        claim.quality_adj_power.clone(),
+                    ),
                 };
 
                 // zero out miner power
-                let res = st.add_to_claim(
-                    &mut claims,
-                    &miner_addr,
-                    &claim.raw_byte_power.neg(),
-                    &claim.quality_adj_power.neg(),
-                );
+                let res = st.add_to_claim(&mut claims, &miner_addr, &rbp.neg(), &qap.neg());
                 if let Err(e) = res {
                     log::warn!(
                         "failed to remove power for miner {} after to failed cron: {}",
diff --git a/vm/actor/src/builtin/power/state.rs b/vm/actor/src/builtin/power/state.rs
index ad79df9acd79..6715609a3efa 100644
--- a/vm/actor/src/builtin/power/state.rs
+++ b/vm/actor/src/builtin/power/state.rs
@@ -232,10 +232,10 @@ pub(super) fn load_cron_events<BS: BlockStore>(
 }
 
 /// Gets claim from claims map by address
-pub fn get_claim<BS: BlockStore>(
-    claims: &Map<BS, Claim>,
+pub fn get_claim<'m, BS: BlockStore>(
+    claims: &'m Map<BS, Claim>,
     a: &Address,
-) -> Result<Option<Claim>, Box<dyn StdError>> {
+) -> Result<Option<&'m Claim>, Box<dyn StdError>> {
     Ok(claims
         .get(&a.to_bytes())
         .map_err(|e| e.downcast_wrap(format!("failed to get claim for address {}", a)))?)
diff --git a/vm/actor/src/builtin/verifreg/mod.rs b/vm/actor/src/builtin/verifreg/mod.rs
index 9bfd9c82f25a..f97251d3da43 100644
--- a/vm/actor/src/builtin/verifreg/mod.rs
+++ b/vm/actor/src/builtin/verifreg/mod.rs
@@ -130,12 +130,14 @@ impl Actor {
                 .map_err(|e| {
                     e.downcast_default(ExitCode::ErrIllegalState, "failed to load verified clients")
                 })?;
-            let deleted = verifiers.delete(&verifier_addr.to_bytes()).map_err(|e| {
-                e.downcast_default(ExitCode::ErrIllegalState, "failed to remove verifier")
-            })?;
-            if !deleted {
-                return Err(actor_error!(ErrIllegalState; "failed to remove verifier: not found"));
-            }
+            verifiers
+                .delete(&verifier_addr.to_bytes())
+                .map_err(|e| {
+                    e.downcast_default(ExitCode::ErrIllegalState, "failed to remove verifier")
+                })?
+                .ok_or_else(
+                    || actor_error!(ErrIllegalState; "failed to remove verifier: not found"),
+                )?;
 
             st.verifiers = verifiers.flush().map_err(|e| {
                 e.downcast_default(ExitCode::ErrIllegalState, "failed to flush verifiers")
@@ -206,7 +208,7 @@ impl Actor {
             }
 
             // Compute new verifier cap and update.
-            if verifier_cap < params.allowance {
+            if verifier_cap < &params.allowance {
                 return Err(actor_error!(ErrIllegalArgument;
                         "Add more DataCap {} for VerifiedClient than allocated {}",
                         params.allowance, verifier_cap
@@ -306,7 +308,7 @@ impl Actor {
                 )?;
             assert_ne!(vc_cap.sign(), Sign::Minus);
 
-            if params.deal_size > vc_cap {
+            if &params.deal_size > vc_cap {
                 return Err(actor_error!(ErrIllegalArgument;
                         "Deal size of {} is greater than verifier_cap {} for verified client {}",
                         params.deal_size, vc_cap, params.address
@@ -317,20 +319,20 @@ impl Actor {
             if new_vc_cap < *MINIMUM_VERIFIED_DEAL_SIZE {
                 // Delete entry if remaining DataCap is less than MinVerifiedDealSize.
                 // Will be restored later if the deal did not get activated with a ProvenSector.
-                let deleted = verified_clients
+                verified_clients
                     .delete(&params.address.to_bytes())
                     .map_err(|e| {
                         e.downcast_default(
                             ExitCode::ErrIllegalState,
                             format!("Failed to delete verified client {}", params.address),
                         )
+                    })?
+                    .ok_or_else(|| {
+                        actor_error!(ErrIllegalState;
+                            "Failed to delete verified client {}: not found",
+                            params.address
+                        )
                     })?;
-                if !deleted {
-                    return Err(actor_error!(ErrIllegalState;
-                        "Failed to delete verified client {}: not found",
-                        params.address
-                    ));
-                }
             } else {
                 verified_clients
                     .set(params.address.to_bytes().into(), BigIntDe(new_vc_cap))
@@ -405,6 +407,7 @@ impl Actor {
                         format!("failed to get verified client {}", &params.address),
                     )
                 })?
+                .cloned()
                 .unwrap_or_default();
 
             // Update to new cap
diff --git a/vm/actor/src/util/balance_table.rs b/vm/actor/src/util/balance_table.rs
index 5883604af6ee..38fe3efff68a 100644
--- a/vm/actor/src/util/balance_table.rs
+++ b/vm/actor/src/util/balance_table.rs
@@ -34,8 +34,8 @@ where
 
     /// Gets token amount for given address in balance table
     #[inline]
-    pub fn get(&self, key: &Address) -> Result<TokenAmount, Box<dyn StdError>> {
-        Ok(self
+    pub fn get(&self, key: &Address) -> Result<&TokenAmount, Box<dyn StdError>> {
+        Ok(&self
             .0
             .get(&key.to_bytes())?
             .ok_or(format!("no key {} in map root", key))?
@@ -59,8 +59,8 @@ where
 
     /// Adds token amount to previously initialized account.
     pub fn add(&mut self, key: &Address, value: &TokenAmount) -> Result<(), Box<dyn StdError>> {
-        let prev = self.get(key)?;
-        Ok(self.0.set(key.to_bytes().into(), BigIntDe(prev + value))?)
+        let new_value = { self.get(key)? + value };
+        Ok(self.0.set(key.to_bytes().into(), BigIntDe(new_value))?)
     }
 
     /// Adds an amount to a balance. Creates entry if not exists
@@ -70,7 +70,7 @@ where
         value: TokenAmount,
     ) -> Result<(), Box<dyn StdError>> {
         let new_val = match self.0.get(&key.to_bytes())? {
-            Some(v) => v.0 + value,
+            Some(v) => &v.0 + value,
             None => value,
         };
         Ok(self.0.set(key.to_bytes().into(), BigIntDe(new_val))?)
@@ -85,7 +85,7 @@ where
         req: &TokenAmount,
         floor: &TokenAmount,
     ) -> Result<TokenAmount, Box<dyn StdError>> {
-        let prev = self.get(key)?;
+        let prev = self.get(key)?.clone();
         let res = prev
             .checked_sub(req)
             .unwrap_or_else(|| TokenAmount::from(0u8));
@@ -120,17 +120,6 @@ where
         Ok(())
     }
 
-    /// Removes an entry from the table, returning the prior value. The entry must have been previously initialized.
-    pub fn remove(&mut self, key: &Address) -> Result<TokenAmount, Box<dyn StdError>> {
-        // Ensure entry exists and get previous value
-        let prev = self.get(key)?;
-
-        // Remove entry from table
-        self.0.delete(&key.to_bytes())?;
-
-        Ok(prev)
-    }
-
     /// Returns total balance held by this balance table
     pub fn total(&self) -> Result<TokenAmount, Box<dyn StdError>> {
         let mut total = TokenAmount::default();
diff --git a/vm/actor/tests/balance_table_test.rs b/vm/actor/tests/balance_table_test.rs
index 34908e79df4d..ad0c99c4ef8a 100644
--- a/vm/actor/tests/balance_table_test.rs
+++ b/vm/actor/tests/balance_table_test.rs
@@ -15,10 +15,10 @@ fn add_create() {
     assert_eq!(bt.has(&addr).unwrap(), false);
 
     bt.add_create(&addr, TokenAmount::from(10u8)).unwrap();
-    assert_eq!(bt.get(&addr).unwrap(), TokenAmount::from(10u8));
+    assert_eq!(bt.get(&addr).unwrap(), &TokenAmount::from(10u8));
 
     bt.add_create(&addr, TokenAmount::from(20u8)).unwrap();
-    assert_eq!(bt.get(&addr).unwrap(), TokenAmount::from(30u8));
+    assert_eq!(bt.get(&addr).unwrap(), &TokenAmount::from(30u8));
 }
 
 // Ported test from specs-actors
@@ -73,14 +73,14 @@ fn balance_subtracts() {
     let mut bt = BalanceTable::new(&store);
 
     bt.set(&addr, TokenAmount::from(80u8)).unwrap();
-    assert_eq!(bt.get(&addr).unwrap(), TokenAmount::from(80u8));
+    assert_eq!(bt.get(&addr).unwrap(), &TokenAmount::from(80u8));
     // Test subtracting past minimum only subtracts correct amount
     assert_eq!(
         bt.subtract_with_minimum(&addr, &TokenAmount::from(20u8), &TokenAmount::from(70u8))
             .unwrap(),
         TokenAmount::from(10u8)
     );
-    assert_eq!(bt.get(&addr).unwrap(), TokenAmount::from(70u8));
+    assert_eq!(bt.get(&addr).unwrap(), &TokenAmount::from(70u8));
 
     // Test subtracting to limit
     assert_eq!(
@@ -88,24 +88,12 @@ fn balance_subtracts() {
             .unwrap(),
         TokenAmount::from(10u8)
     );
-    assert_eq!(bt.get(&addr).unwrap(), TokenAmount::from(60u8));
+    assert_eq!(bt.get(&addr).unwrap(), &TokenAmount::from(60u8));
 
     // Test must subtract success
     bt.must_subtract(&addr, &TokenAmount::from(10u8)).unwrap();
-    assert_eq!(bt.get(&addr).unwrap(), TokenAmount::from(50u8));
+    assert_eq!(bt.get(&addr).unwrap(), &TokenAmount::from(50u8));
 
     // Test subtracting more than available
     assert!(bt.must_subtract(&addr, &TokenAmount::from(100u8)).is_err());
 }
-
-#[test]
-fn remove() {
-    let addr = Address::new_id(100);
-    let store = db::MemoryDB::default();
-    let mut bt = BalanceTable::new(&store);
-
-    bt.set(&addr, TokenAmount::from(1u8)).unwrap();
-    assert_eq!(bt.get(&addr).unwrap(), TokenAmount::from(1u8));
-    bt.remove(&addr).unwrap();
-    assert!(bt.get(&addr).is_err());
-}
diff --git a/vm/actor/tests/market_actor_test.rs b/vm/actor/tests/market_actor_test.rs
index e617106ade34..d39053b5f241 100644
--- a/vm/actor/tests/market_actor_test.rs
+++ b/vm/actor/tests/market_actor_test.rs
@@ -47,7 +47,7 @@ fn get_escrow_balance(rt: &MockRuntime, addr: &Address) -> Result<TokenAmount, A
 
     let et = BalanceTable::from_root(rt.store(), &st.escrow_table).unwrap();
 
-    Ok(et.get(addr).unwrap())
+    Ok(et.get(addr).unwrap().clone())
 }
 
 // TODO add array stuff
diff --git a/vm/state_tree/src/lib.rs b/vm/state_tree/src/lib.rs
index 68e9414269d4..38a3a5d6974a 100644
--- a/vm/state_tree/src/lib.rs
+++ b/vm/state_tree/src/lib.rs
@@ -130,8 +130,8 @@ impl StateSnapshots {
 
     fn get_actor(&self, addr: &Address) -> Option<ActorState> {
         for layer in self.layers.iter().rev() {
-            if let Some(state) = layer.actors.read().get(addr).cloned() {
-                return state;
+            if let Some(state) = layer.actors.read().get(addr) {
+                return state.clone();
             }
         }
 
@@ -209,7 +209,7 @@ where
         }
 
         // if state doesn't exist, find using hamt
-        let act = self.hamt.get(&addr.to_bytes())?;
+        let act = self.hamt.get(&addr.to_bytes())?.cloned();
 
         // Update cache if state was found
         if let Some(act_s) = &act {

From 97d93c01bb4ec377f2a9b60ee63a32ef5f3a539a Mon Sep 17 00:00:00 2001
From: austinabell <austinabell8@gmail.com>
Date: Mon, 5 Oct 2020 17:29:25 -0400
Subject: [PATCH 5/8] update tracking to our values, add TODOs

---
 ipld/hamt/tests/hamt_tests.rs | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/ipld/hamt/tests/hamt_tests.rs b/ipld/hamt/tests/hamt_tests.rs
index 3436d5632627..b4b03dd6dee3 100644
--- a/ipld/hamt/tests/hamt_tests.rs
+++ b/ipld/hamt/tests/hamt_tests.rs
@@ -158,7 +158,8 @@ fn set_delete_many() {
         "bafy2bzaceaneyzybb37pn4rtg2mvn2qxb43rhgmqoojgtz7avdfjw2lhz4dge"
     );
     #[rustfmt::skip]
-    assert_eq!(*store.stats.borrow(), BSStats {r: 87, w: 119, br: 7671, bw: 14042});
+    // TODO https://github.com/ChainSafe/forest/issues/729
+    assert_eq!(*store.stats.borrow(), BSStats { r: 61, w: 93, br: 6478, bw: 12849 });
 }
 
 #[cfg(feature = "identity")]
@@ -229,7 +230,9 @@ fn canonical_structure() {
         &[b"K0", b"K1", b"KAA1", b"KAA2", b"KAA3"],
         &[b"KAA4"],
         "bafy2bzacedrktzj4o7iumailmdzl5gz3uqr4bw2o72qg4zv5q7qhezy4r32bc",
-        BSStats {r: 7, w: 10, br: 388, bw: 561},
+        // TODO we have a LOT less reads and writes, match go with 
+        // https://github.com/ChainSafe/forest/issues/729
+        BSStats { r: 4, w: 6, br: 228, bw: 346 },
     );
 }
 
@@ -240,30 +243,36 @@ fn canonical_structure_alt_bit_width() {
     let kb_cases = [
         (
             "bafy2bzacecnabvcxw7k5hgfcex5ig4jf3nabuxvl35edgnjk5ven2kg4n3ffm",
-            BSStats {r: 2, w: 5, br: 26, bw: 65},
+            // TODO https://github.com/ChainSafe/forest/issues/729
+            BSStats { r: 2, w: 4, br: 26, bw: 52 },
         ),
         (
             "bafy2bzacec2f6scvfmnyal5pzn43if6e2kls5jbm2jdab2xzuditctd5i3bbi",
-            BSStats {r: 2, w: 5, br: 28, bw: 70},
+            // TODO https://github.com/ChainSafe/forest/issues/729
+            BSStats { r: 2, w: 4, br: 28, bw: 56 },
         ),
         (
             "bafy2bzacedckymwjxmg35sllfegwrmnr7rxb3x7dh6muec2li2qhqjk5tf63q",
-            BSStats {r: 2, w: 5, br: 32, bw: 80},
+            // TODO https://github.com/ChainSafe/forest/issues/729
+            BSStats { r: 2, w: 4, br: 32, bw: 64 },
         ),
     ];
     #[rustfmt::skip]
     let other_cases = [
         (
             "bafy2bzacedc7hh2tyz66m7n7ricyw2m7whsgoplyux3kbxczla7zuf25wi2og",
-            BSStats {r: 9, w: 14, br: 420, bw: 661},
+            // TODO https://github.com/ChainSafe/forest/issues/729
+            BSStats { r: 4, w: 6, br: 190, bw: 292 },
         ),
         (
             "bafy2bzacedeeqff3p7n3ogqxvqslb2yrbi4ojz44sp6mvjwyp6u6lktxdo2fg",
-            BSStats {r: 8, w: 13, br: 385, bw: 639},
+            // TODO https://github.com/ChainSafe/forest/issues/729
+            BSStats { r: 4, w: 6, br: 202, bw: 306 },
         ),
         (
             "bafy2bzaceckigpba3kck23qyuyb2i6vbiprtsmlr2rlyn3o4lmmcvzsh3l6wi",
-            BSStats {r: 8, w: 13, br: 419, bw: 687},
+            // TODO https://github.com/ChainSafe/forest/issues/729
+            BSStats { r: 4, w: 6, br: 214, bw: 322 },
         ),
     ];
     for i in 5..8 {
@@ -322,5 +331,6 @@ fn clean_child_ordering() {
         "bafy2bzacec6ro3q36okye22evifu6h7kwdkjlb4keq6ogpfqivka6myk6wkjo"
     );
     #[rustfmt::skip]
-    assert_eq!(*store.stats.borrow(), BSStats {r: 9, w: 17, br: 2327, bw: 2845});
+    // TODO https://github.com/ChainSafe/forest/issues/729
+    assert_eq!(*store.stats.borrow(), BSStats { r: 3, w: 11, br: 1992, bw: 2510 });
 }

From 659b0c1ff8d939fb09c572cb87695216dcfe0139 Mon Sep 17 00:00:00 2001
From: austinabell <austinabell8@gmail.com>
Date: Mon, 5 Oct 2020 17:37:35 -0400
Subject: [PATCH 6/8] Fix doc tests

---
 ipld/hamt/src/hamt.rs | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/ipld/hamt/src/hamt.rs b/ipld/hamt/src/hamt.rs
index 39b7ef376d87..5d98d9e46d6d 100644
--- a/ipld/hamt/src/hamt.rs
+++ b/ipld/hamt/src/hamt.rs
@@ -22,8 +22,8 @@ use std::marker::PhantomData;
 ///
 /// let mut map: Hamt<_, _, usize> = Hamt::new(&store);
 /// map.set(1, "a".to_string()).unwrap();
-/// assert_eq!(map.get(&1).unwrap(), Some("a".to_string()));
-/// assert_eq!(map.delete(&1).unwrap(), true);
+/// assert_eq!(map.get(&1).unwrap(), Some(&"a".to_string()));
+/// assert_eq!(map.delete(&1).unwrap(), Some((1, "a".to_string())));
 /// assert_eq!(map.get::<_>(&1).unwrap(), None);
 /// let cid = map.flush().unwrap();
 /// ```
@@ -152,7 +152,7 @@ where
     ///
     /// let mut map: Hamt<_, _, usize> = Hamt::new(&store);
     /// map.set(1, "a".to_string()).unwrap();
-    /// assert_eq!(map.get(&1).unwrap(), Some("a".to_string()));
+    /// assert_eq!(map.get(&1).unwrap(), Some(&"a".to_string()));
     /// assert_eq!(map.get(&2).unwrap(), None);
     /// ```
     #[inline]
@@ -211,8 +211,8 @@ where
     ///
     /// let mut map: Hamt<_, _, usize> = Hamt::new(&store);
     /// map.set(1, "a".to_string()).unwrap();
-    /// assert_eq!(map.delete(&1).unwrap(), true);
-    /// assert_eq!(map.delete(&1).unwrap(), false);
+    /// 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>
     where

From b52b640cf8bf21ae54ec09b4d9057f7af2117fbb Mon Sep 17 00:00:00 2001
From: austinabell <austinabell8@gmail.com>
Date: Mon, 5 Oct 2020 17:51:53 -0400
Subject: [PATCH 7/8] Use cache for other operations

---
 ipld/hamt/src/node.rs | 68 +++++++++++++++++++++++++++----------------
 1 file changed, 43 insertions(+), 25 deletions(-)

diff --git a/ipld/hamt/src/node.rs b/ipld/hamt/src/node.rs
index 2beee6ce61bf..038aeebf5a7a 100644
--- a/ipld/hamt/src/node.rs
+++ b/ipld/hamt/src/node.rs
@@ -127,10 +127,18 @@ where
     {
         for p in &self.pointers {
             match p {
-                Pointer::Link { cid, .. } => {
-                    match store.get::<Node<K, V, H>>(cid).map_err(|e| e.to_string())? {
-                        Some(node) => node.for_each(store, f)?,
-                        None => return Err(format!("Node with cid {} not found", cid).into()),
+                Pointer::Link { cid, cache } => {
+                    if let Some(cached_node) = cache.borrow() {
+                        cached_node.for_each(store, f)?
+                    } else {
+                        let node = store
+                            .get(cid)?
+                            .ok_or_else(|| format!("Node with cid {} not found", cid))?;
+
+                        // Ignore error intentionally, the cache value will always be the same
+                        let _ = cache.fill(node);
+                        let cache_node = cache.borrow().expect("cache filled on line above");
+                        cache_node.for_each(store, f)?
                     }
                 }
                 Pointer::Dirty(n) => n.for_each(store, f)?,
@@ -223,15 +231,20 @@ where
         let child = self.get_child_mut(cindex);
 
         match child {
-            Pointer::Link { cid, .. } => match store.get::<Node<K, V, H>>(cid)? {
-                Some(mut node) => {
-                    // Pull value from store and update to cached node
-                    node.modify_value(hashed_key, bit_width, depth + 1, key, value, store)?;
-                    *child = Pointer::Dirty(Box::new(node));
-                    Ok(())
-                }
-                None => Err(Error::CidNotFound(cid.to_string())),
-            },
+            Pointer::Link { cid, cache } => {
+                let cached_node = std::mem::take(cache);
+                let mut child_node = if let Some(sn) = cached_node.into_inner() {
+                    sn
+                } else {
+                    store
+                        .get(cid)?
+                        .ok_or_else(|| Error::CidNotFound(cid.to_string()))?
+                };
+
+                child_node.modify_value(hashed_key, bit_width, depth + 1, key, value, store)?;
+                *child = Pointer::Dirty(child_node);
+                Ok(())
+            }
             Pointer::Dirty(n) => {
                 Ok(n.modify_value(hashed_key, bit_width, depth + 1, key, value, store)?)
             }
@@ -303,18 +316,23 @@ where
         let child = self.get_child_mut(cindex);
 
         match child {
-            Pointer::Link { cid, .. } => match store.get::<Node<K, V, H>>(cid)? {
-                Some(mut node) => {
-                    // Pull value from store and update to cached node
-                    let del = node.rm_value(hashed_key, bit_width, depth + 1, key, store)?;
-                    *child = Pointer::Dirty(Box::new(node));
-
-                    // Clean to retrieve canonical form
-                    child.clean()?;
-                    Ok(del)
-                }
-                None => Err(Error::CidNotFound(cid.to_string())),
-            },
+            Pointer::Link { cid, cache } => {
+                let cached_node = std::mem::take(cache);
+                let mut child_node = if let Some(sn) = cached_node.into_inner() {
+                    sn
+                } else {
+                    store
+                        .get(cid)?
+                        .ok_or_else(|| Error::CidNotFound(cid.to_string()))?
+                };
+
+                let deleted = child_node.rm_value(hashed_key, bit_width, depth + 1, key, store)?;
+                *child = Pointer::Dirty(child_node);
+
+                // Clean to retrieve canonical form
+                child.clean()?;
+                Ok(deleted)
+            }
             Pointer::Dirty(n) => {
                 // Delete value and return deleted value
                 let deleted = n.rm_value(hashed_key, bit_width, depth + 1, key, store)?;

From e9d6499689e5855f5a98821e11c0861392b4ccd3 Mon Sep 17 00:00:00 2001
From: austinabell <austinabell8@gmail.com>
Date: Mon, 5 Oct 2020 17:53:32 -0400
Subject: [PATCH 8/8] Remove murmur hash from Hamt

---
 Cargo.lock                      |  9 ---------
 ipld/hamt/Cargo.toml            |  9 +--------
 ipld/hamt/src/hash_algorithm.rs | 22 ----------------------
 3 files changed, 1 insertion(+), 39 deletions(-)

diff --git a/Cargo.lock b/Cargo.lock
index 0f0364eb90e5..8424a73bcafd 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -2972,7 +2972,6 @@ dependencies = [
  "hex",
  "ipld_blockstore",
  "lazycell",
- "murmur3",
  "serde",
  "serde_bytes",
  "sha2 0.9.1",
@@ -3818,14 +3817,6 @@ dependencies = [
  "unsigned-varint 0.4.0",
 ]
 
-[[package]]
-name = "murmur3"
-version = "0.4.1"
-source = "git+https://github.com/dignifiedquire/murmur3?branch=nicer-hashing#c30ee28caae87847ba66aaa10fcaab783033f5f7"
-dependencies = [
- "byteorder 1.3.4",
-]
-
 [[package]]
 name = "native-tls"
 version = "0.2.4"
diff --git a/ipld/hamt/Cargo.toml b/ipld/hamt/Cargo.toml
index b252f072bacb..843df0fb44db 100644
--- a/ipld/hamt/Cargo.toml
+++ b/ipld/hamt/Cargo.toml
@@ -17,16 +17,9 @@ thiserror = "1.0"
 sha2 = "0.9.1"
 lazycell = "1.2.1"
 
-# TODO remove murmur3 support when finalized in Filecoin
-[dependencies.murmur3]
-git = "https://github.com/dignifiedquire/murmur3"
-branch = "nicer-hashing"
-optional = true
-
 [features]
-default = ["identity", "murmur3"]
+default = ["identity"]
 identity = []
-murmur = ["murmur3"]
 
 [dev-dependencies]
 hex = "0.4.2"
diff --git a/ipld/hamt/src/hash_algorithm.rs b/ipld/hamt/src/hash_algorithm.rs
index e6de66e0e5a6..1d194be4f199 100644
--- a/ipld/hamt/src/hash_algorithm.rs
+++ b/ipld/hamt/src/hash_algorithm.rs
@@ -4,9 +4,6 @@
 use crate::{Hash, HashedKey};
 use sha2::{Digest, Sha256 as Sha256Hasher};
 
-#[cfg(feature = "murmur")]
-use murmur3::murmur3_x64_128::MurmurHasher;
-
 /// Algorithm used as the hasher for the Hamt.
 pub trait HashAlgorithm {
     fn hash<X: ?Sized>(key: &X) -> HashedKey
@@ -43,25 +40,6 @@ impl HashAlgorithm for Sha256 {
     }
 }
 
-#[cfg(feature = "murmur")]
-#[derive(Debug)]
-pub enum Murmur3 {}
-
-#[cfg(feature = "murmur")]
-impl HashAlgorithm for Murmur3 {
-    fn hash<X: ?Sized>(key: &X) -> HashedKey
-    where
-        X: Hash,
-    {
-        let mut hasher = MurmurHasher::default();
-        key.hash(&mut hasher);
-        let mut bz: [u8; 32] = Default::default();
-        let hash: [u8; 16] = hasher.finalize().into();
-        bz[0..16].copy_from_slice(&hash);
-        bz
-    }
-}
-
 #[cfg(feature = "identity")]
 use std::hash::Hasher;