CosmWasm / CosmWasm/cw-storage-plus

SnapshotMap height loading discrepancy

Open
#90 1 comment 1 reaction 0 assignees View on GitHub
Dominant language
Rust
Stars
51
Forks
34
PR merge metrics
No merged PRs in 30d

Description

We use `SnapshotMap` a lot over at DAO DAO, and there's one aspect of it that has always bothered me. I think it leads to pretty confusing behavior and is likely to cause bugs, potentially security issues, if not deeply understood by the developers using it.

In my understanding, when a new value `V_new` is saved at height `n`, a snapshot is created at height `n` that contains the current value `V_old` in a field called `old`, and the value in the `primary` map is overridden with `V_new`. `load` and `may_load` read directly from the `primary` map, finding `V_new`, behaving as expected. But `may_load_at_height` actually returns `V_old` when called with the height at which `V_new` was saved.

Thus, `may_load` differs from `may_load_at_height` if used in the same block right after a value is stored. IMO, one would assume that calling `save` at the current height followed by `may_load_at_height` at the current height would retrieve the value just saved, but that is not the case.

The only justification I could see for this behavior is that `SnapshotMap` intends to be deterministic for the whole block, always returning the value that existed at the very beginning of the block. Thus, if another contract queries a contract that uses a `SnapshotMap` during a block where the map is being updated, it will always get the same value, regardless of the ordering of transactions in the block. Though I'm not totally convinced by this argument as one already cannot expect deterministic ordering when interacting with other contracts, and other state storage exists besides `SnapshotMap`. And I'm not sure if this benefit is worth the tradeoff of less intuitive security critical code.

Is there another reason it was designed this way? Please convince me this is necessary 😁

There are a few ways I think this could be fixed pretty easily, though I don't have full knowledge of this monorepo, and maybe these would break other things. Let me know what you think.
1. In `write_change` https://github.com/CosmWasm/cw-storage-plus/blob/cac9687e29579c61eeacffafc131614c9f43baaa/src/snapshot/map.rs#L117-L126 use `height - 1` instead of `height`, since that is the last block at which the `old` value is accurate.
2. In `may_load_at_height` https://github.com/CosmWasm/cw-storage-plus/blob/cac9687e29579c61eeacffafc131614c9f43baaa/src/snapshot/map.rs#L154-L170 use `height + 1` when calling `self.snapshots.may_load_at_height`, since it will try to find the next change after the requested height, whose `old` value would be the correct value for `height`.
3. In `Snapshot` https://github.com/CosmWasm/cw-storage-plus/blob/cac9687e29579c61eeacffafc131614c9f43baaa/src/snapshot/mod.rs#L151-L180 change the starting lower bound from `inclusive` to `exclusive` when looking in the changelog for the `old` value.

As far as I can tell, 2 and 3 above are identical, except 3 may fix the problem in other places if `Snapshot` is used the same way.

Contributor guide

No contributing guide indexed for this repository

Research direction

Read the SnapshotMap logic in src/snapshot/map.rs, especially write_change and may_load_at_height, then inspect Snapshot in src/snapshot/mod.rs. Compare the three proposed height and range changes and establish the intended semantics for reads at the current height. Done means the behavior is decided and the relevant snapshot-loading paths consistently implement that decision.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
database
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.