SnapshotMap height loading discrepancy

[NoahSaso]

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

   cw-storage-plus/src/snapshot/map.rs

   Lines 117 to 126 in cac9687

   	/// load old value and store changelog
   	fn write_change(&self, store: &mut dyn Storage, k: K, height: u64) -> StdResult<()> {
   	// if there is already data in the changelog for this key and block, do not write more
   	if self.snapshots.has_changelog(store, k.clone(), height)? {
   	return Ok(());
   	}
   	// otherwise, store the previous value
   	let old = self.primary.may_load(store, k.clone())?;
   	self.snapshots.write_changelog(store, k, height, old)
   	}

   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

   cw-storage-plus/src/snapshot/map.rs

   Lines 154 to 170 in cac9687

   	pub fn may_load_at_height(
   	&self,
   	store: &dyn Storage,
   	k: K,
   	height: u64,
   	) -> StdResult<Option<T>> {
   	let snapshot = self
   	.snapshots
   	.may_load_at_height(store, k.clone(), height)?;
   	if let Some(r) = snapshot {
   	Ok(r)
   	} else {
   	// otherwise, return current value
   	self.may_load(store, k)
   	}
   	}

   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

   cw-storage-plus/src/snapshot/mod.rs

   Lines 151 to 180 in cac9687

   	// may_load_at_height reads historical data from given checkpoints.
   	// Returns StdError::NotFound if we have no checkpoint, and can give no data.
   	// Returns Ok(None) if there is a checkpoint, but no cached data (no changes since the
   	// checkpoint. Caller should query current state).
   	// Return Ok(Some(x)) if there is a checkpoint and data written to changelog, returning the state at that time
   	pub fn may_load_at_height(
   	&self,
   	store: &dyn Storage,
   	key: K,
   	height: u64,
   	) -> StdResult<Option<Option<T>>> {
   	self.assert_checkpointed(store, height)?;
   	// this will look for the first snapshot of height >= given height
   	// If None, there is no snapshot since that time.
   	let start = Bound::inclusive(height);
   	let first = self
   	.changelog
   	.prefix(key)
   	.range_raw(store, Some(start), None, Order::Ascending)
   	.next();
   	if let Some(r) = first {
   	// if we found a match, return this last one
   	r.map(|(_, v)| Some(v.old))
   	} else {
   	Ok(None)
   	}
   	}
   	}

   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.

[NoahSaso]

here's a little writeup i did in the context of DAO DAO's vote delegation system, which may help clarify the problem:

https://github.com/DA0-DA0/dao-contracts/blob/f886f6761d75a02eebe61317b1d4966e68d8057c/contracts/delegation/dao-vote-delegation/README.md#implementation-notes