Implement EIP-158 and EIP-2200 which allow to pass 7 extra spec tests #18

jsign · 2024-01-23T14:29:22Z

Note: This PR should be reviewed after #17, since it's building on top of it

This PR implements some remaining gaps:

EIP-158, which is related to state clearing.
EIP-2200, which was an important gap to charge SSTORE correctly.

Now from the EVMOne side the only gaps are emitting logs and self-destruct. We can do the former without much rush, and the latter might depend since we might want to implement directly on Cancun/Prague expectations.

This allows us to run all the rest of the exec-spec-tests of the file we imported from Shanghai. So we moved from passing 1 test there to now 8.

Moving forward, I could keep adding other files in the Shanghai dataset, and I'm pretty sure most of them will pass. Now I'm more interested in probably implementing other gaps at the "blockchain" level like generating receipts, and having a MPT impl to calculate tx/receipts root.

Signed-off-by: Ignacio Hagopian <[email protected]>

jsign · 2024-01-23T14:31:02Z

src/blockchain/blockchain.zig

@@ -171,6 +172,7 @@ pub const Blockchain = struct {
                .chain_id = chain.chain_id,
            };

+            try state.startTx();


This is a new method in statedb which does some tx-scope clearing for:

Account access lists

Storage slot accessed list.

and other tasks that are relevant whenever we start a new tx.

jsign · 2024-01-23T14:31:25Z

src/blockchain/blockchain.zig

+        for (env.state.touched_addresses.items) |address| {
+            if (env.state.isEmpty(address))
+                env.state.destroyAccount(address);
+        }


Now statedb tracks all touched addresses, so it can destroy them as explained in EIP-158.

jsign · 2024-01-23T14:32:29Z

src/blockchain/vm.zig

        const k = std.mem.readIntSlice(u256, &key.*.bytes, std.builtin.Endian.Big);
+        const storage_status: evmc.enum_evmc_storage_status = blk: {


This is the most important gap and change of this PR.
See the link mentioned in L163 and EIP-2200.

What we do now, is that stated has a reference to the state of the world when the tx started. This allows to do this logic of comparing the original vs current vs new value to charge the appropriate gas.

jsign · 2024-01-23T14:32:50Z

src/blockchain/vm.zig

-            const receipient_balance = vm.env.state.getAccount(fromEVMCAddress(msg.*.recipient)).balance;
-            vm.env.state.setBalance(sender, receipient_balance + value) catch |err| switch (err) {
+            const recipient_balance = vm.env.state.getAccount(recipient_addr).balance;


There was a typo here and a small refactoring.

jsign · 2024-01-23T14:33:51Z

src/blockchain/vm.zig

+            vm.env.state.* = prev_statedb;
+        }
+
+        evmclog.debug("call() depth={d} ended", .{msg.*.depth});


Added this log since it's useful to see also when we finished in a call scope.

jsign · 2024-01-23T14:34:09Z

src/engine_api/execution_payload.zig

-    std.log.info("newPayloadV2Handler: {any}", .{params});
+    // std.log.info("newPayloadV2Handler: {any}", .{params});

    var block = params.toBlock();
-    std.debug.print("block number={}\n", .{block.header.block_number});
+    _ = block;
+    // std.debug.print("block number={}\n", .{block.header.block_number});


For now, I commented this since it added some extra noise in the logs.

jsign · 2024-01-23T14:34:27Z

src/state/statedb.zig

+    // original_db contains the state of the world when the transaction starts.
+    // It assists in charging the right amount of gas for SSTORE.
+    original_db: ?AccountDB = null,


Useful for EIP-2200 impl.

ideally we would use some sort of journal, but let's not overcomplicate things at this point.

jsign · 2024-01-23T14:34:48Z

src/state/statedb.zig

+    // Tx-scoped lists.
+    touched_addresses: ArrayList(Address),
    accessed_accounts: AddressSet,
    accessed_storage_keys: AddressKeySet,


These are the tx-scoped fields for access lists and EIP-158 EIPs.

jsign · 2024-01-23T14:36:09Z

src/state/statedb.zig

+        if (std.mem.eql(u8, &value, &std.mem.zeroes(Bytes32))) {
+            _ = account.storage.remove(key);
+            return;
+        }


This was a bug, since writing the zero value should mean we remove the key/value pair from the state. (i.e: empty, not explicitly zero). Note that this is required for now in the "MPT world" for the current spec tests to pass, this will evolve with VKT.

gballet

left a general comment, no action needed. This can be merged as far as I'm concerned.

gballet · 2024-02-07T11:10:01Z

src/state/statedb.zig

+    // original_db contains the state of the world when the transaction starts.
+    // It assists in charging the right amount of gas for SSTORE.
+    original_db: ?AccountDB = null,


ideally we would use some sort of journal, but let's not overcomplicate things at this point.

jsign added 6 commits January 21, 2024 12:27

cleanups

1486191

Signed-off-by: Ignacio Hagopian <[email protected]>

blockchain/vm: fix typo and add log

5c546e1

Signed-off-by: Ignacio Hagopian <[email protected]>

blockchain: add statedb tx scoped variables handling

f9d24bd

Signed-off-by: Ignacio Hagopian <[email protected]>

blockchain/vm: implement EIP-220

c79f9b2

Signed-off-by: Ignacio Hagopian <[email protected]>

implement EIP-158

59c8407

Signed-off-by: Ignacio Hagopian <[email protected]>

test: run all tests from spec

7e44b23

Signed-off-by: Ignacio Hagopian <[email protected]>

jsign commented Jan 23, 2024

View reviewed changes

jsign changed the title ~~[Draft] Implement EIP-158 and EIP-2200 which allow to pass 7 extra spec tests~~ [on hold] Implement EIP-158 and EIP-2200 which allow to pass 7 extra spec tests Jan 23, 2024

jsign mentioned this pull request Jan 30, 2024

Rename Txn* to Tx* #19

Merged

jsign added the waiting in merge pipeline label Jan 30, 2024

jsign changed the title ~~[on hold] Implement EIP-158 and EIP-2200 which allow to pass 7 extra spec tests~~ Implement EIP-158 and EIP-2200 which allow to pass 7 extra spec tests Jan 30, 2024

Base automatically changed from jsign-vm2 to main February 6, 2024 23:19

jsign removed the waiting in merge pipeline label Feb 6, 2024

gballet approved these changes Feb 7, 2024

View reviewed changes

jsign merged commit 2a4bcb1 into main Feb 7, 2024
4 checks passed

jsign deleted the jsign-vm3 branch February 7, 2024 17:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement EIP-158 and EIP-2200 which allow to pass 7 extra spec tests #18

Implement EIP-158 and EIP-2200 which allow to pass 7 extra spec tests #18

jsign commented Jan 23, 2024 •

edited

Loading

jsign Jan 23, 2024

jsign Jan 23, 2024

jsign Jan 23, 2024

jsign Jan 23, 2024

jsign Jan 23, 2024

jsign Jan 23, 2024

jsign Jan 23, 2024

gballet Feb 7, 2024

jsign Jan 23, 2024

jsign Jan 23, 2024

gballet left a comment

gballet Feb 7, 2024

		const k = std.mem.readIntSlice(u256, &key.*.bytes, std.builtin.Endian.Big);
		const storage_status: evmc.enum_evmc_storage_status = blk: {

Implement EIP-158 and EIP-2200 which allow to pass 7 extra spec tests #18

Implement EIP-158 and EIP-2200 which allow to pass 7 extra spec tests #18

Conversation

jsign commented Jan 23, 2024 • edited Loading

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

gballet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsign commented Jan 23, 2024 •

edited

Loading