Adding transient storage maps with TLOAD/TSTORE implementation #2801

fmacleal · 2024-10-11T12:24:05Z

This PR adds the basic implementation of the TLOAD/TSTORE opcodes with some DSL contracts tests. Still work in progress and more validations need to be added.

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
Tests for the changes have been added (for bug fixes / features)
Requires Activation Code (Hard Fork)

Other information:

rmoreliovlabs · 2024-10-23T12:57:39Z

rskj-core/src/main/java/org/ethereum/db/MutableRepository.java

+
+    @Override
+    public void clearTransientStorage() {
+        this.transientTrie =  new MutableTrieImpl(new TrieStoreImpl(new HashMapDB()), new Trie());


how often does clearTransientStorage gets called in our app? If it’s pretty frequent, I wonder if constantly reinitializing transientTrie is the best approach performance-wise. Maybe we could find a way to clear entries without creating a new instance? let me know what you think!

Basically after every transaction execution. The thing is that this class it's initialized everytime a transaction starts anyway. Because this is used for the Storage during the transaction execution that might be rolled back and used by the storage. This clear will be called at end of a transaction. And it's extremely important that this data it's cleared. So, we would basically create a new MutableTrieStore anyway.

github-actions · 2024-10-25T15:28:01Z

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

Package	Version	Score	Details

Scanned Manifest Files

asoto-iov · 2024-10-23T11:04:06Z

rskj-core/src/main/java/org/ethereum/db/MutableRepository.java

+    @Nullable
+    @Override
+    public DataWord getTransientStorageValue(RskAddress addr, DataWord key) {
+        byte[] triekey = trieKeyMapper.getAccountStorageKey(addr, key);


Method below public byte[] getTransientStorageBytes(RskAddress addr, DataWord key) could be used here before wrap the result into the DataWord as they have the same logic to retrieve the value from the transient trie.

asoto-iov · 2024-10-23T11:58:32Z

rskj-core/src/main/java/org/ethereum/db/TransientStorageRepositoryCreator.java

+    private TransientStorageRepositoryCreator() {
+    }
+
+    public static Repository createNewTransientStorage() {


Where this method would be used? Why is needed? Are we going to need TransientStorage "alone"?

Not used anymore, should have been deleted and I forgot it here during some rebases. Good catch,

asoto-iov · 2024-10-23T11:59:52Z

rskj-core/src/main/java/org/ethereum/vm/VM.java

+        }
+
+        program.stackPush(val);
+        // key could be returned to the pool, but transientStorageLoad semantics should be checked
        // to make sure storageLoad always gets a copy, not a reference.
        program.step();
    }

    protected void doTSTORE(){


minor comment, It would be useful (for the reviewers) to add a //TODO for the speding gas logic and static call checks etc

asoto-iov · 2024-10-25T18:18:16Z

rskj-core/src/main/java/org/ethereum/vm/VM.java

+                if (!activations.isActive(RSKIP446)) {
+                    throw Program.ExceptionHelper.invalidOpCode(program);
+                }
+                doTLOAD();
                break;
            case OpCodes.OP_TSTORE: doTSTORE();


I think it should go after the activation rule check.

Yes, it should. good catch!

asoto-iov · 2024-10-25T18:27:22Z

rskj-core/src/test/java/co/rsk/vm/opcode/TransientStorageDslTest.java

+    }
+
+    @Test
+    void testTransientStorageOpcodesExecutionFailsWithRSKIPDeactivated() throws FileNotFoundException, DslProcessorException {


In the code the OP code call is done before the activation check, is the test working anyway?

It is working, because it fails for the other one that was fine. We can have a split test with a contract that does only TLOAD and other only TSTORE to catch these two situations, I will do.

asoto-iov · 2024-10-25T18:32:06Z

rskj-core/src/main/java/org/ethereum/vm/VM.java

        // to make sure storageLoad always gets a copy, not a reference.
        program.step();
    }

    protected void doTSTORE(){
-
+        DataWord address = program.stackPop();


About the naming of this variable, shouldn't it be "key" instead of address?

asoto-iov · 2024-10-25T18:33:02Z

rskj-core/src/main/java/org/ethereum/db/MutableRepository.java

+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.Optional;
+import java.util.Set;

 public class MutableRepository implements Repository {


I don't fully understand why the transient storage is implemented into the standard storage.

Because the standard mutable storage, it's also mutable and if a transaction is not rolled back it can be reverted. As you can see, in the lifecycle of the transaction, every time it starts we create a new instance of this MutableRepository. With that, we will have the snapshot storage that can be reverted or commited in the final storage at the end but we also will have this transient storage.

The transient storage it's also mutable and the lifecycle of it it's the same for the regular mutable storage. That's why it's implemented here, for the sake of simplicity and to be managed in the same way. It will get advantage of the whole lifecycle of the mutable storage that we already have.

ok, it is interesting point of view.

asoto-iov · 2024-10-25T18:34:08Z

rskj-core/src/main/java/org/ethereum/core/Repository.java

@@ -27,7 +27,7 @@

 import java.math.BigInteger;

-public interface Repository extends RepositorySnapshot {
+public interface Repository extends RepositorySnapshot, TransientRepository {


Why a repository is both standard repo and transient repo?

It's explained a bit in the other comment here, if still not clear, let me know and I will try to explain more. Maybe we can have a call to discuss.

asoto-iov · 2024-10-25T18:43:24Z

rskj-core/src/main/java/org/ethereum/core/TransientRepository.java

+
+    void addTransientStorageRow(RskAddress addr, DataWord key, DataWord value);
+
+    void addTransientStorageBytes(RskAddress addr, DataWord key, byte[] value);


is it needed these two methods? it seems that addTransientStorageBytes is only called from addTransientStorageRow and addTransientStorageRow it is only being used once in the code.

After follow your suggestion, is being used twice now. Let's keep it. :)

sonarcloud · 2024-10-28T14:09:11Z

Quality Gate failed

Failed conditions
1 New Blocker Issues (required ≤ 0)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

asoto-iov

Good work.
My only concern is about the implementation of both transient and standard storage in the same classes. I think it could have some disadvantages. I would like to talk about it in a call but I think it is not a blocker to not continue with it.

- Adding scratch from the DSL tests and contracts for tests - The tests are still failing, I am investigating to find the root cause - Adding more logic to the map structures - Finishing test validation for basic scenarios of TLOAD/TSTORE - Now we need to add more scenarios to validate that the memory isn't shared and erased in the end of the transaction.

- In order to get advantage from the whole flow of tracking, rollback and commits already present in the MutableRepository. We refactored a bit the logic of transient storage, now it's simpler. - The task isn't done yet, more tests needs to be added to validate that this is working properly.

* Adding the structure for transient storage opcodes - Finishing test validation for basic scenarios of TLOAD/TSTORE * Refactor from code to get advantage from the MutableRepository - In order to get advantage from the whole flow of tracking, rollback and commits already present in the MutableRepository. We refactored a bit the logic of transient storage, now it's simpler. * Adding more tests defined on the EIP-1153 spec * Adding tests for the different create contexts EIP1153 * Addressing comments from review * Adding more scenarios of test with dynamic execution context

fmacleal requested review from a team as code owners October 11, 2024 12:24

fmacleal force-pushed the fmacleal/adding_transient_storage_maps branch from 6142056 to 7149f88 Compare October 14, 2024 10:32

fmacleal force-pushed the fmacleal/addition_transient_storage_opcodes branch from d6dc804 to 8892bf3 Compare October 14, 2024 10:46

fmacleal force-pushed the fmacleal/adding_transient_storage_maps branch 3 times, most recently from 743e9e8 to 3ce63a0 Compare October 20, 2024 17:51

fmacleal force-pushed the fmacleal/adding_transient_storage_maps branch 2 times, most recently from 01c2a45 to 81cef20 Compare October 22, 2024 21:18

rmoreliovlabs reviewed Oct 23, 2024

View reviewed changes

fmacleal force-pushed the fmacleal/adding_transient_storage_maps branch from b887917 to 9d4530d Compare October 25, 2024 15:27

fmacleal force-pushed the fmacleal/adding_transient_storage_maps branch from 9d4530d to 7038543 Compare October 25, 2024 15:28

asoto-iov reviewed Oct 25, 2024

View reviewed changes

fmacleal force-pushed the fmacleal/adding_transient_storage_maps branch from ffa96a3 to bfd6026 Compare October 28, 2024 13:53

fmacleal force-pushed the fmacleal/adding_transient_storage_maps branch 2 times, most recently from 55b6728 to e9494b0 Compare October 29, 2024 13:58

asoto-iov approved these changes Oct 31, 2024

View reviewed changes

fmacleal added 7 commits November 4, 2024 14:00

Adding more tests defined on the EIP-1153 spec

fcf2111

Starting to add more context calls tests for the tload/tstore

b82b3d6

Adding tests for the different create contexts EIP1153

57c9f39

Addressing comments from review

d340722

Adding more scenarios of test with dynamic execution context

8bba650

fmacleal force-pushed the fmacleal/adding_transient_storage_maps branch from e9494b0 to 8bba650 Compare November 4, 2024 13:00

fmacleal merged commit f244dc2 into fmacleal/addition_transient_storage_opcodes Nov 4, 2024
5 of 7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding transient storage maps with TLOAD/TSTORE implementation #2801

Adding transient storage maps with TLOAD/TSTORE implementation #2801

fmacleal commented Oct 11, 2024

rmoreliovlabs Oct 23, 2024

fmacleal Oct 24, 2024 •

edited

Loading

github-actions bot commented Oct 25, 2024 •

edited

Loading

asoto-iov Oct 23, 2024

asoto-iov Oct 23, 2024

fmacleal Oct 28, 2024

asoto-iov Oct 23, 2024

asoto-iov Oct 25, 2024

fmacleal Oct 28, 2024

asoto-iov Oct 25, 2024

fmacleal Oct 28, 2024

asoto-iov Oct 25, 2024

asoto-iov Oct 25, 2024

fmacleal Oct 28, 2024 •

edited

Loading

asoto-iov Oct 31, 2024

asoto-iov Oct 25, 2024

fmacleal Oct 28, 2024 •

edited

Loading

asoto-iov Oct 25, 2024

fmacleal Oct 28, 2024

sonarcloud bot commented Oct 28, 2024

asoto-iov left a comment


		void addTransientStorageRow(RskAddress addr, DataWord key, DataWord value);

		void addTransientStorageBytes(RskAddress addr, DataWord key, byte[] value);

Adding transient storage maps with TLOAD/TSTORE implementation #2801

Adding transient storage maps with TLOAD/TSTORE implementation #2801

Conversation

fmacleal commented Oct 11, 2024

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Choose a reason for hiding this comment

fmacleal Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Oct 25, 2024 • edited Loading

Dependency Review

OpenSSF Scorecard

Scanned Manifest Files

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmacleal Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmacleal Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Oct 28, 2024

Quality Gate failed

asoto-iov left a comment

Choose a reason for hiding this comment

fmacleal Oct 24, 2024 •

edited

Loading

github-actions bot commented Oct 25, 2024 •

edited

Loading

fmacleal Oct 28, 2024 •

edited

Loading

fmacleal Oct 28, 2024 •

edited

Loading