Implementation of EIP-1153: Transient Storage using Disk Persistence and Lifecycle Management #1588

snissn · 2024-11-20T02:53:02Z

Description

This WIP PR introduces transient storage support in the Filecoin EVM, conceptually aligning with Ethereum's EIP-1153. Transient storage functionality is implemented with a tombstone mechanism for lifecycle tracking keyed by transaction identifiers (origin and nonce). While the core implementation is in place, work remains on validating the transient data's lifecycle and completing associated tests.

Current Progress

State Modifications:

Persistent Transient Data:
- A Cid variable has been added to represent transient storage.
```
pub transient_state: Cid
```

Lifecycle Tracking:

Introduced TransientDataLifespan to track lifecycle with origin ActorID and nonce.

pub struct TransientDataLifespan {
    pub origin: ActorID,
    pub nonce: u64,
}

New Operations:

TLOAD: Retrieves transient data while ensuring lifecycle validity.
TSTORE: Updates transient data and lifecycle.
Lifecycle validation (TODO): Compare transaction metadata with stored tombstone.

Remaining TODOs

Liveliness Check for Transient Data:
- Validate that transient data is only accessible within the valid transaction context by comparing the tombstone (stored origin and nonce) with the current transaction.
Code Location:
- get_transient_storage and set_transient_storage in actors/evm/src/interpreter/system.rs.
```
// TODO check tombstone for liveliness of data
```
Test Coverage:
- Write tests to verify:
  - Transient data lifecycle (e.g., expiration between transactions).
  - Liveliness checks during nested and delegate calls.
  - Behavior when accessing invalid or expired transient data.
- Existing transient storage operations (TLOAD, TSTORE) are partially tested.
Code Location:
- actors/evm/src/interpreter/instructions/storage.rs (Test module)
```
// TODO test transient storage lifecycle
```
Edge Case Handling:
- Ensure transient data is cleared appropriately in all scenarios, including contract self-destruct or re-creation.

Testing

Implemented Tests:

test_tload: Validates TLOAD operation when data exists.
test_tload_oob: Ensures out-of-bounds TLOAD returns zero.
test_tstore: Confirms TSTORE updates transient data correctly.

Pending Tests:

Liveliness and lifecycle tests (TODO in the test module).
System-wide validation tests for lifecycle management.

Tradeoffs and Considerations

Increased Storage Overhead:
- Adds transient_state and transient_data_lifespan to State.
Gas Inefficiency:
- Current approach persists transient data, diverging from EIP-1153's ephemeral storage design.

Checklist

Core implementation of TLOAD and TSTORE.
Integration with State and System.
Basic tests for TLOAD and TSTORE.
Lifecycle validation for transient data (TODO).
Comprehensive test coverage (TODO).

Next Steps

Provision work for go-state-types evm state upgrade and migration for next network upgrade started here: https://github.com/filecoin-project/go-state-types/compare/mikers/evm-transient-data-nv17?expand=1

References

EIP-1153: Transient Storage: Link to EIP-1153
Related Discussions: EIP-1153: Transient Storage FIPs#855 (reply in thread)

Review Feedback

Incorporates feedback from @Stebalien:

Minimize unnecessary writes when clearing transient data.
Keep State clean unless other updates occur.

…around comparing the liveliness of the transient data and validating that functionality in a test

actors/evm/src/state.rs

actors/evm/src/interpreter/system.rs

…bine transient_slots with transient_data_lifespan anyway.

…DO reinitialize does not properly clear state KAMT due to clone/reference issues that are being debugged

rvagg · 2024-11-22T03:44:11Z

actors/evm/src/interpreter/system.rs

+        // Reinitialize the transient_slots with a fresh KAMT
+        //let transient_store = self.rt.store().clone();
+        //self.transient_slots = StateKamt::new_with_config(transient_store, KAMT_CONFIG.clone());
+        // TODO XXX reinitialize does not currently work due to blockstore reference issues


I'd be interested to know what these errors are and how you end up diagnosing the problem, for my own educational purposes because your commented code looks like it should work.

It's because self.rt.store() doesn't implement clone, while &... does, so rust is helpfully creating a reference for you.

Note: the new function explicitly requires that the store be clonable.

Options are:

Lift that requirement up to the impl level.

Use self.transient_slots.into_store() to "take" the existing store.

@Stebalien I've tried a few different variations of the "taking" option that you are suggesting. I didn't quite get anything to compile however.

Could you share a deeper explanation on the two options to either lift the requirement to the impl level (I'm not sure what you mean by this; which requirement? which impl?) and also if you can help me with the second option about taking the ownership of the datastore in order to satisfy the requirements of new_with_config. I'm still learning the ins and outs of this in rust, and would be very grateful. I would definitely not mind a code commit either if it's a simple lift for you. Thanks in advanced!

Ah, so, this is going to be a bit of a pain. The issue is that you can't leave transient_slots uninitialized so you need something to put there while you construct the new Kamt. E.g., you can have Option<StateKamt<...>> and put none there while you replace it.

But... doing that will also help facilitate lazy loading (you can leave it as None until the user first tries to load a transient slot). So it's not the end of the world.

If you're stuck, I'd address the rest of the feedback first. Then I'll see what I can do to fix this part.

Thanks! I'm now returning from the thanksgiving break and will look into everything this week.

I have learned many things since starting this :) I see that my previous attempt was very much against the RAII ideas that rust enforces. I didn't realize that was the issue. We don't need to destroy and create a new KAMT object but simply need to clear the one we have.

As a placeholder I have implemented a loop over keys and delete implementation of clear for now until an efficient clear() can be used. I opened a PR filecoin-project/ref-fvm#2092 for an efficient implementation of clear in KAMT that I hope can be approved and it will be easy for me from there to upgrade kamt in builtin-actors to the future version that supports clear.

…ent_data_state and transient_data_lifespan

…date kamt lirbary to support a clear method

snissn · 2024-12-04T07:09:27Z

Remaining issues:

Upgrade KAMT to support a clear() method that supports efficient resetting of KAMT object
Comprehensive test coverage of reentry and nested contracts. Confirm if ref fvm MockRuntime tests can support delegate calls or if that test will only be able to be done at a higher abstraction like Lotus
Ensure lazy load on first use

BigLep · 2024-12-04T22:34:09Z

2024-12-04 conversation between FilB and FilOz:

For testing, should be able to do recursive calls.
Look at the fevm-test-kit for some inspiration.
Test this in builtin-actors (better testing framework) (look at integration-tests .. evm-tests)
- @snissn found an example test to hook on to.
Test this in lotus as well.
migrations
- a migration does need to get written but it is straightforward w
discussion on clear and whether it's needed here

rvagg · 2024-12-04T23:07:30Z

I did a quick look for a "simple" state migration that might be similar to this; I haven't found a great example yet but part of the v9 migration did introduce a couple of fields for FIP-0029 to the miner actor, BeneficiaryTerm and PendingBeneficiaryTerm onto a miner's Info, which were initialised with empty values. Unfortunately it's tangled up with a bunch of other complicated migration things, but I think the idea is present there:

top.go needed a call to newMinerMigrator which did this work
among other things, that migrator took the miner8.State object, copied it to a miner9.State along with the empty fields, and saved that

snissn · 2024-12-05T00:24:32Z

following up from a sync with @rvagg @BigLep and @Stebalien with my own copy of notes:

I will look into integration_tests/src/tests/evm_test.rs for remaining testing needs
clear() isn't needed and the best way to implement this

Instead the determination of transient data validity should happen when the actor state is loaded

State - should contain the (lifespan and kamt root) or be null

System - only needs a valid KAMT root or null

tload checks for null and returns 0 early if null. otherwise is reads from KAMT without a need to check any lifecycle info
tstore checks null, and if null initializes a new empty KAMT. from there it saves and sets the saved_state_root to null

a migration will be needed but it's a simple one

…ut still issues with reload

…builtin-actors into feat/transient_storage

actors/evm/src/interpreter/precompiles/evm.rs

Stebalien · 2024-12-12T19:14:43Z

actors/evm/src/interpreter/system.rs

        if crate::is_dead(rt, &state) {
            // If we're "dead", return an empty read-only contract. The code will be empty, so
            // nothing can happen anyways.
-            return Ok(Self::new(rt, true));
+            return Self::new(rt, true);
        }

        let read_only = rt.read_only();
+        let current_transient_data_lifespan = get_current_transient_data_lifespan(rt)?;


nit: crate::is_dead has similar logic to get the current lifetime. It would be nice to unify this and to have a single concept of "current transaction". On the other hand, I'm happy to do this in a followup PR.

I would feel very good about landing this and then collaborating with you or allowing you to do this in a followup PR.

actors/evm/src/interpreter/system.rs

actors/evm/src/interpreter/test_util.rs

integration_tests/src/tests/evm_test.rs

…ap because we have already validated the existance of an id

Stebalien

LGTM from a code perspective!

actors/evm/src/interpreter/system.rs

Co-authored-by: Steven Allen <steven@stebalien.com>

rvagg · 2024-12-13T07:36:45Z

https://github.com/filecoin-project/go-state-types/compare/mikers/evm-transient-data-nv17?expand=1#diff-61808733785f19d73d06ebbd0b984828297ac80b9443c2a9071d88f1fd521b89R31-R32 Nonce and TransientData will need to be flipped in order in your go-state-types representation of this state (order matters because it's encoded as an array). But otherwise that migration looks great.

actors/evm/tests/basic.rs

actors/evm/tests/contracts/TransientStorageTest.sol

rvagg · 2024-12-13T07:43:42Z

actors/evm/tests/contracts/TransientStorageTest.sol

+        require(retrievedValue == value, "TLOAD did not retrieve stored value within transaction");
+    }
+
+    function testLifecycleValidationSubsequentTransaction() public {


wouldn't this one be // Test 2.2?

actors/evm/tests/contracts/TransientStorageTest.sol

rvagg

well done, particularly on the epic test to actual-changes ratio; my suggestions are simply suggestions, up to you whether you bother

Co-authored-by: Rod Vagg <rod@vagg.org>

snissn · 2024-12-13T08:04:33Z

LGTM from a code perspective!
That's great I think

https://github.com/filecoin-project/go-state-types/compare/mikers/evm-transient-data-nv17?expand=1#diff-61808733785f19d73d06ebbd0b984828297ac80b9443c2a9071d88f1fd521b89R31-R32 Nonce and TransientData will need to be flipped in order in your go-state-types representation of this state (order matters because it's encoded as an array). But otherwise that migration looks great.

Thank you!! That could have been so frustrating to catch later on!

Co-authored-by: Rod Vagg <rod@vagg.org>

…efactored into TransientStorageTest

…builtin-actors into feat/transient_storage

snissn · 2024-12-14T00:45:02Z

@rvagg thanks for the feedback on the solidity tests! I ran forge fmt, updated the comments and also removed no longer used solidity test files that I wrapped back into the TranisentStorageTest file at some point during development!

initial implementation of EIP-1153: Transient Storage - TODOs remain …

801dc0c

…around comparing the liveliness of the transient data and validating that functionality in a test

snissn requested a review from Stebalien November 20, 2024 02:53

snissn self-assigned this Nov 20, 2024

Stebalien reviewed Nov 21, 2024

View reviewed changes

actors/evm/src/state.rs Outdated Show resolved Hide resolved

actors/evm/src/interpreter/system.rs Outdated Show resolved Hide resolved

actors/evm/src/interpreter/system.rs Outdated Show resolved Hide resolved

actors/evm/src/interpreter/system.rs Outdated Show resolved Hide resolved

snissn added 3 commits November 21, 2024 10:57

fix transient state error message

ef679de

remove pub visibility from transient_data_lifespan. TODO still is com…

a98c058

…bine transient_slots with transient_data_lifespan anyway.

further implementation of lifecycle management for transient data, TO…

9f3e90d

…DO reinitialize does not properly clear state KAMT due to clone/reference issues that are being debugged

rvagg reviewed Nov 22, 2024

View reviewed changes

snissn added 10 commits December 2, 2024 15:00

update evm contract State to use transient_data a composite of transi…

2ee1d9d

…ent_data_state and transient_data_lifespan

cargo fmt

31180a1

update comments on tests

56d2458

add tstorage test contract files

312299f

use a non optimized delete method to clear transient storage. TODO up…

757131f

…date kamt lirbary to support a clear method

failing tests that need to be investigated

eb894ae

add test confirming transient storage is cleared

3dfe377

more test debugging

ab808a2

cargo fmt

20aefb5

fix cargo check nit

99c1e6b

snissn added 8 commits December 6, 2024 16:13

simplify transient test

2b811de

support nested and reentry in transient storage in integration tests

89311f9

progress on fixing removing tranisent storage lifespan from system, b…

af6bb99

…ut still issues with reload

Merge branch 'master' into feat/transient_storage

661755a

use clear instead of loop and delete

d93167b

Merge branch 'master' into mikers/feat-tstore-reload-stuck2

0ceb2d8

use kamt clear in reload

250d133

merge

3052b34

snissn added 2 commits December 11, 2024 21:56

remove unused import

074e16f

Merge branch 'feat/transient_storage' of github.com:filecoin-project/…

7a99e10

…builtin-actors into feat/transient_storage

Stebalien reviewed Dec 12, 2024

View reviewed changes

snissn added 6 commits December 12, 2024 16:49

remove duplicate comment

fb8025d

use self.current_transient_data_lifespan

2c8febf

get_current_transient_data_lifespan does not need to be public

f1ac779

remove match from get_current_transient_data_lifespan and use an unwr…

7c6ba37

…ap because we have already validated the existance of an id

remove error case from new

a005cf7

remove allow non snake case from transient storage tests

8eed0c9

Stebalien approved these changes Dec 13, 2024

View reviewed changes

actors/evm/src/interpreter/system.rs Outdated Show resolved Hide resolved

Update actors/evm/src/interpreter/system.rs

133f7b5

Co-authored-by: Steven Allen <steven@stebalien.com>

Stebalien approved these changes Dec 13, 2024

View reviewed changes

rvagg reviewed Dec 13, 2024

View reviewed changes

actors/evm/tests/basic.rs Outdated Show resolved Hide resolved

rvagg reviewed Dec 13, 2024

View reviewed changes

actors/evm/tests/contracts/TransientStorageTest.sol Outdated Show resolved Hide resolved

rvagg reviewed Dec 13, 2024

View reviewed changes

actors/evm/tests/contracts/TransientStorageTest.sol Outdated Show resolved Hide resolved

rvagg reviewed Dec 13, 2024

View reviewed changes

actors/evm/tests/contracts/TransientStorageTest.sol Outdated Show resolved Hide resolved

rvagg approved these changes Dec 13, 2024

View reviewed changes

Update actors/evm/tests/basic.rs

1b57940

Co-authored-by: Rod Vagg <rod@vagg.org>

snissn and others added 3 commits December 12, 2024 22:05

Update actors/evm/tests/contracts/TransientStorageTest.sol

685fd63

Co-authored-by: Rod Vagg <rod@vagg.org>

Update actors/evm/tests/contracts/TransientStorageTest.sol

ff09cfa

Co-authored-by: Rod Vagg <rod@vagg.org>

Update actors/evm/tests/contracts/TransientStorageTest.sol

b96dbe5

Co-authored-by: Rod Vagg <rod@vagg.org>

snissn changed the title ~~[WIP] Implementation of EIP-1153: Transient Storage using Disk Persistence and Lifecycle Management~~ Implementation of EIP-1153: Transient Storage using Disk Persistence and Lifecycle Management Dec 14, 2024

snissn added 4 commits December 13, 2024 14:29

remove now unused solidity test contracts that have previously been r…

b7ac7ba

…efactored into TransientStorageTest

Merge branch 'feat/transient_storage' of github.com:filecoin-project/…

a43f34a

…builtin-actors into feat/transient_storage

forge fmt and solc recompile

260f401

add comments to transient storage test file with test information

a2e4b37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of EIP-1153: Transient Storage using Disk Persistence and Lifecycle Management #1588

Implementation of EIP-1153: Transient Storage using Disk Persistence and Lifecycle Management #1588

snissn commented Nov 20, 2024 •

edited

Loading

rvagg Nov 22, 2024

Stebalien Nov 22, 2024

snissn Nov 25, 2024

Stebalien Nov 25, 2024

Stebalien Nov 25, 2024

snissn Dec 2, 2024

snissn Dec 4, 2024

snissn commented Dec 4, 2024 •

edited

Loading

BigLep commented Dec 4, 2024

rvagg commented Dec 4, 2024

snissn commented Dec 5, 2024

Stebalien Dec 12, 2024

snissn Dec 13, 2024

Stebalien left a comment

rvagg commented Dec 13, 2024

rvagg Dec 13, 2024

rvagg left a comment •

edited

Loading

snissn commented Dec 13, 2024

snissn commented Dec 14, 2024

Implementation of EIP-1153: Transient Storage using Disk Persistence and Lifecycle Management #1588

Are you sure you want to change the base?

Implementation of EIP-1153: Transient Storage using Disk Persistence and Lifecycle Management #1588

Conversation

snissn commented Nov 20, 2024 • edited Loading

Description

Current Progress

Remaining TODOs

Testing

Tradeoffs and Considerations

Checklist

Next Steps

References

Review Feedback

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snissn commented Dec 4, 2024 • edited Loading

BigLep commented Dec 4, 2024

rvagg commented Dec 4, 2024

snissn commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien left a comment

Choose a reason for hiding this comment

rvagg commented Dec 13, 2024

Choose a reason for hiding this comment

rvagg left a comment • edited Loading

Choose a reason for hiding this comment

snissn commented Dec 13, 2024

snissn commented Dec 14, 2024

snissn commented Nov 20, 2024 •

edited

Loading

snissn commented Dec 4, 2024 •

edited

Loading

rvagg left a comment •

edited

Loading