Skip to content

Update KAMT to update the cached root on set_root#1667

Merged
rvagg merged 2 commits intomasterfrom
steb/update-kamt
Apr 4, 2025
Merged

Update KAMT to update the cached root on set_root#1667
rvagg merged 2 commits intomasterfrom
steb/update-kamt

Conversation

@Stebalien
Copy link
Copy Markdown
Member

@Stebalien Stebalien commented Apr 4, 2025

Fix KAMT set_root CID caching

Issue

This PR fixes an issue reported in recallnet/contracts#98 where the Filecoin EVM was incorrectly handling contract state after a delegate call from a constructor.

Root Cause

The KAMT data structure, which Filecoin's EVM uses to store contract storage slots, has a performance optimization that caches the last flushed root CID. This prevents unnecessary recomputation when no changes occur.

In fvm_ipld_kamt@v0.4.0, a bug was introduced in the Kamt::set_root() method: when setting a new root, it correctly updated the root node but failed to update the cached root CID. This meant that subsequent flush() calls without intermediate modifications (which would clear the cached root) would return the old cached CID instead of the new root CID.

Reproduction Scenario

The bug manifests in this sequence:

  1. Flush a KAMT, producing cid_a.
  2. Call kamt.set_root(cid_b).
  3. Call kamt.flush() without modifying any storage slots.

Expected: flush() returns cid_b.
Actual: flush() incorrectly returns cid_a.

EVM Impact

This bug mostly affects delegate calls within contract constructors:

  1. When a contract constructor delegate-calls another contract that modifies state.
  2. The delegate call returns, and contract_state.set_root() is called to update state, but the cached CID isn't updated.
  3. When the constructor finishes, the contract bytecode is installed and the entire contract state is flushed, using the wrong root cid for contract_state.

Most contracts don't trigger this issue because usually the only way to modify the contract's state is to update a storage slot. However, during construction, the last operation is to install the contract's bytecode and flush the contract state (including that new bytecode) whether or not the contract's storage slots have been modified.

There are a few other ways to trigger this bug including re-entrency in a self-destructed contract and, e.g., some forms of re-entrency when constructing new contracts with the CREATE instruction (updating the contract's nonce will cause the state to be flushed).

Fix

The fix is simple: update set_root() to also set self.flushed_cid = Some(*cid) when loading a new root. This ensures the cached CID correctly reflects the current root.

@github-project-automation github-project-automation Bot moved this to 📌 Triage in FilOz Apr 4, 2025
@github-project-automation github-project-automation Bot moved this from 📌 Triage to ✔️ Approved by reviewer in FilOz Apr 4, 2025
@rvagg rvagg added this pull request to the merge queue Apr 4, 2025
Merged via the queue into master with commit 9ce5b29 Apr 4, 2025
17 checks passed
@rvagg rvagg deleted the steb/update-kamt branch April 4, 2025 05:26
@github-project-automation github-project-automation Bot moved this from ✔️ Approved by reviewer to 🎉 Done in FilOz Apr 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🎉 Done

Development

Successfully merging this pull request may close these issues.

2 participants