builtin-actors icon indicating copy to clipboard operation
builtin-actors copied to clipboard

Map::load uses wrong bitwidth in state checks (use Map2 everywhere)

Open anorth opened this issue 1 year ago • 1 comments

The Map type is defined as an alias to Hamt, which is imported from a FVM repos, so we don't control its interface. The Hamt type includes a load(Cid, Store) method which internally defaults to a bitwidth parameter of DEFAULT_BIT_WIDTH=8. This is not the bitwidth of any HAMTs in Filecoin state, which uses DEFAULT_BIT_WIDTH=5 for many, and specifically tuned widths in some places. Using Map::load() is thus always an error. Because the HAMT implementation's root node is not self-describing and does not include the bitwidth with which it should be interpreted, this error goes undetected at runtime. The observed effect is that keys that should be in the map aren't found.

Luckily, the only significant uses of Map::load() are in the state invariant checks. Some of these checks are thus ineffective, but for historical reasons these aren't the checks that we actually use to verify state and migrations. The checks are duplicated in the go-state-types repo and those checks don't have this error. (That duplication is a whole other issue, but not one that we can solve in this repo). So the error here most likely has had no effect.

As well as fixing the places that use Map::load() already, we should fix the types so that this can't happen again. Because we don't control the Hamt interface, I think we should change Map from an alias to a newtype wrapping Hamt, forwarding the appropriate methods. There may be an opportunity to improve the interface here too, such as by using Into<BytesKey> for key parameters and providing concise but hard-to-misuse constructor functions.

anorth avatar Jul 31 '23 21:07 anorth

Keeping this issue open until the Map type is gone, replaced with what's currently called Map2.

anorth avatar Aug 15 '23 22:08 anorth