Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
anorth opened this issue Jul 31, 2023 · 1 comment
Open

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

anorth opened this issue Jul 31, 2023 · 1 comment
Assignees
Labels
bug Something isn't working cleanup help wanted Extra attention is needed

Comments

@anorth
Copy link
Member

anorth commented Jul 31, 2023

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 anorth added bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed cleanup labels Jul 31, 2023
@anorth anorth self-assigned this Aug 1, 2023
Stebalien added a commit to filecoin-project/ref-fvm that referenced this issue Aug 2, 2023
1. We don't intentionally use these anywhere.
2. We _do_ unintentionally use these where we
shouldn't (filecoin-project/builtin-actors#1346).

fixes #1346
Stebalien added a commit to filecoin-project/ref-fvm that referenced this issue Aug 2, 2023
1. We don't intentionally use these anywhere.
2. We _do_ unintentionally use these where we
shouldn't (filecoin-project/builtin-actors#1346).

fixes #1828
Stebalien added a commit to filecoin-project/ref-fvm that referenced this issue Aug 3, 2023
1. We don't intentionally use these anywhere.
2. We _do_ unintentionally use these where we
shouldn't (filecoin-project/builtin-actors#1346).

fixes #1828
Stebalien added a commit that referenced this issue Aug 10, 2023
This _doesn't_ migrate to the new `Map2` interface everywhere, it just
avoids the use of `Hamt::load` and `Hamt::new`, which will be deprecated
in the next release of `fvm_ipld_hamt`.

part of #1346
Stebalien added a commit that referenced this issue Aug 11, 2023
This _doesn't_ migrate to the new `Map2` interface everywhere, it just
avoids the use of `Hamt::load` and `Hamt::new`, which will be deprecated
in the next release of `fvm_ipld_hamt`.

part of #1346
github-merge-queue bot pushed a commit that referenced this issue Aug 11, 2023
This _doesn't_ migrate to the new `Map2` interface everywhere, it just
avoids the use of `Hamt::load` and `Hamt::new`, which will be deprecated
in the next release of `fvm_ipld_hamt`.

part of #1346
@anorth
Copy link
Member Author

anorth commented Aug 15, 2023

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

@anorth anorth moved this from Todo to In Progress in 🍉 nv21 - FilDev Track Board Aug 23, 2023
@anorth anorth moved this to In Progress in nv22 - Track Board Jan 22, 2024
@rjan90 rjan90 added this to FilOz Jan 29, 2024
@rjan90 rjan90 moved this to ⌨️In Progress in FilOz Jan 29, 2024
@anorth anorth removed the good first issue Good for newcomers label Mar 7, 2024
@anorth anorth changed the title Map::load uses wrong bitwidth in state checks Map::load uses wrong bitwidth in state checks (use Map2 everywhere) May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cleanup help wanted Extra attention is needed
Projects
Status: ⌨️ In Progress
Status: In Progress
Status: In Progress
Development

No branches or pull requests

1 participant