Make contract creation work #26

jsign · 2024-03-05T22:30:20Z

This PR makes some fixes to make contract creation work.

Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>

jsign · 2024-03-05T22:30:54Z

src/blockchain/vm.zig

+            .input_data = if (msg.target != null) msg.data.ptr else msg.code.ptr,
+            .input_size = if (msg.target != null) msg.data.len else msg.code.len,


Depending if the msg.target is null or not, we use the message calldata or code which his specially prepared with the corresponding code from the contract.

oh my, that you have to distinguish data from code is terrible ux :( anyhow, I was wondering if it was somehow possible to cast the pointer to a slice, so that input_size is no longer needed?

I was wondering if it was somehow possible to cast the pointer to a slice, so that input_size is no longer needed?

This input_size is coming mostly from the library having a C interface. So in that case the concept of "slice" doesn't exist, and require the usual ptr+length API.

jsign · 2024-03-05T22:41:06Z

src/blockchain/vm.zig

-        const code_address = fromEVMCAddress(msg.*.code_address);
-        const code = vm.env.state.getAccount(code_address).code;
+        const code = switch (msg.*.kind) {
+            evmc.EVMC_CALL, evmc.EVMC_DELEGATECALL, evmc.EVMC_CALLCODE => blk: {
+                const code_address = fromEVMCAddress(msg.*.code_address);
+                const code = vm.env.state.getAccount(code_address).code;
+                break :blk code;
+            },
+            evmc.EVMC_CREATE, evmc.EVMC_CREATE2 => msg.*.input_data[0..msg.*.input_size],
+            else => @panic("unkown message kind"),
+        };


Depending of the call kind we take the code from the state or the message input.

jsign · 2024-03-05T22:41:51Z

src/blockchain/vm.zig

+            if (msg.*.kind == evmc.EVMC_CREATE or msg.*.kind == evmc.EVMC_CREATE2) {
+                vm.env.state.setContractCode(recipient_addr, result.output_data[0..result.output_size]) catch |err| switch (err) {
+                    error.OutOfMemory => @panic("OOO"),
+                    error.AccountAlreadyHasCode => @panic("account already has code"),
+                };
+            }


After the EVM execution, we take the result and save it as contract code for the corresponding address. This is saving the code for future calls for this addr.

jsign · 2024-03-05T22:43:00Z

src/lib.zig

@@ -8,6 +8,7 @@ test "tests" {
    std.testing.refAllDeclsRecursive(@import("crypto/crypto.zig"));
    std.testing.refAllDeclsRecursive(@import("engine_api/engine_api.zig"));
    std.testing.refAllDeclsRecursive(@import("tests/spec_tests.zig"));
+    std.testing.refAllDeclsRecursive(@import("tests/custom_tests.zig"));


I created this new tests/custom_tests.zig to put customized tests, since the other tests in tests are mostly spec related ones.

For this PR, I made a custom test (that you'll see later) that creates a contract, and then calls it since none of the current spec tests do contract creation (yet).

jsign · 2024-03-05T22:43:40Z

src/state/statedb.zig

+        var key_iterator = self.db.keyIterator();
+        while (key_iterator.next()) |addr| {
+            self.db.getPtr(addr.*).?.deinit();
+        }


We were missing deiniting stuff correctly, which prob wasn't detected before since we use arenas anyway.

and, from a performance point of view, would it not make sense not to deinit anything since that would be running some code that isn't necessary? I agree that, in terms of correctness, it's the wrong thing to do... and it might cause a lot of issues with tests... but we could require at the API level that what we are passed is an area allocator.

The rationale for this, is that this client isn't required to mine blocks, so all allocations are performed in a single "block execution" context. I think we can squeeze some performance and readability out of this.

For a stateless client usual workflow, that's correct and what we actually do today. (Use an arena per block, that we reset to the next).

Indeed all this work is a noop if an arena allocator was passed, so I agree I'd like to remove all this if really, despite technically correct, it's somewhat code that we don't need.

Now... the situation here is that for spec tests we're expected to provide an existing StateDB before any block is executed. So if more than one block is executed in the test, StateDB memory must survive so we can't really nuke the StateDB allocations after every block (i.e: the next block would have missing data, since spec tests don't have a witness to reconstruct what it needs).

So the situation here is that for spec tests, we can't let the StateDB allocations live in the same arena as the block... since that will corrupt the statedb information when the arena for the executed block is reset. This isn't a problem for a stateless client since we'll build a statedb on each block from the witness -- but that isn't the case for most tests.

To be clear since my message maybe is a bit messy. We could remove the deinitis, but that would require all tests to always use arenas so the spec-tests can work, since:

In spec tests, the arena to be used by StateDB and the rest of block exec can't be the same.

In stateless block exec, we can use the same arena for both StateDB and the rest of block exec.

Final comment: I'll take some note to change all tests to use arenas (if that isn't the case already), and we can simply remove the deinit stuff. Not the ideal thing to use arenas in test for everything since we won't be detecting any leak, but considering "the main case" will use arenas probably it's fine.

jsign · 2024-03-05T22:44:15Z

src/tests/custom_tests.zig

+const StateDB = state.StateDB;
+const ChainID = config.ChainId;
+
+test "create contract" {


A test that checks that contract creation and later execution works correctly.

jsign · 2024-03-05T22:44:35Z

src/tests/custom_tests.zig

+    const coinbase = common.hexToAddress("0x1000000000000000000000000000000000000001");
+    var coinbase_state = try state.AccountState.init(allocator, coinbase, 1, 0, &[_]u8{});
+    var sdb = try StateDB.init(allocator, &[_]state.AccountState{coinbase_state});
+    defer sdb.deinit();


Init a coinbase in our statedb.

jsign · 2024-03-05T22:45:05Z

src/tests/custom_tests.zig

+    // Create contract.
+    var contract_addr: Address = blk: {
+        const msg = try blockchain.Blockchain.prepareMessage(
+            allocator,
+            coinbase,
+            null,
+            0,
+            &[_]u8{
+                // Init
+                0x60, 0x8, // PUSH1 8
+                0x60, 0x0C, // PUSH 12
+                0x60, 0x00, // PUSH1 0
+                0x39, // CODECOPY
+                0x60, 0x8, // PUSH1 8
+                0x60, 0x00, // PUSH1 0
+                0xF3, // Return
+
+                // Runtime code
+                0x60, 0x01, // PUSH1 2 - Push 2 on the stack
+                0x60, 0x02, // PUSH1 4 - Push 4 on the stack
+                0x01, // ADD - Add stack[0] to stack[1]
+                0x60, 0x00, // PUSH1 0
+                0x55, // SSTORE
+            },
+            10_000,
+            env,
+        );
+
+        try sdb.startTx();
+        var out = try vmi.processMessageCall(msg);
+
+        // Check the contract creation execution was successful.
+        try std.testing.expect(out.success);
+
+        break :blk msg.current_target;
+    };


Contract creation tx, execute and check that was successful.

jsign · 2024-03-05T22:45:26Z

src/tests/custom_tests.zig

+    // Run it.
+    {
+        const msg = try blockchain.Blockchain.prepareMessage(
+            allocator,
+            coinbase,
+            contract_addr,
+            0,
+            &[_]u8{},
+            100_000,
+            env,
+        );
+
+        try sdb.startTx();
+        var out = try vmi.processMessageCall(msg);
+
+        // Check that the execution didn't fail, thus the contract was found and executed correctly.
+        try std.testing.expect(out.success);
+    }


Technically, the resulted bytecode of the contract creation was saved in the state. To double check, we run another tx calling that created code.

gballet

LGTM overall, just left a few questions/suggestions here and there, but feel free to merge if you want.

gballet · 2024-03-25T05:44:25Z

src/blockchain/vm.zig

+                break :blk code;
+            },
+            evmc.EVMC_CREATE, evmc.EVMC_CREATE2 => msg.*.input_data[0..msg.*.input_size],
+            else => @panic("unkown message kind"),


no support for STATICCALL ?

STATICCALL is modeled as a flag, it's not a kind.
kinds are:

pub const EVMC_CALL: c_int = 0; pub const EVMC_DELEGATECALL: c_int = 1; pub const EVMC_CALLCODE: c_int = 2; pub const EVMC_CREATE: c_int = 3; pub const EVMC_CREATE2: c_int = 4;

There's a flags field:

gballet · 2024-03-25T05:53:39Z

src/state/statedb.zig

+        var key_iterator = self.db.keyIterator();
+        while (key_iterator.next()) |addr| {
+            self.db.getPtr(addr.*).?.deinit();
+        }


and, from a performance point of view, would it not make sense not to deinit anything since that would be running some code that isn't necessary? I agree that, in terms of correctness, it's the wrong thing to do... and it might cause a lot of issues with tests... but we could require at the API level that what we are passed is an area allocator.

gballet · 2024-03-25T05:55:10Z

src/state/statedb.zig

+        var key_iterator = self.db.keyIterator();
+        while (key_iterator.next()) |addr| {
+            self.db.getPtr(addr.*).?.deinit();
+        }


The rationale for this, is that this client isn't required to mine blocks, so all allocations are performed in a single "block execution" context. I think we can squeeze some performance and readability out of this.

gballet · 2024-03-25T08:35:18Z

src/blockchain/vm.zig

+            .input_data = if (msg.target != null) msg.data.ptr else msg.code.ptr,
+            .input_size = if (msg.target != null) msg.data.len else msg.code.len,


oh my, that you have to distinguish data from code is terrible ux :( anyhow, I was wondering if it was somehow possible to cast the pointer to a slice, so that input_size is no longer needed?

jsign · 2024-03-28T00:12:26Z

@gballet merged, but remember to read my replies since you made good questions/comments!

jsign added 6 commits March 5, 2024 19:29

types: add receipt structs

39f9783

Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>

blockchain: add receipt generation

9f51d2f

Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>

state/types: in AccountState only free code if it was allocated

7e16741

blockchain: support contract creation

b4c6e6a

tests: add contract creation custom test

763575f

Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>

nits and comments

1b2c564

Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>

jsign commented Mar 5, 2024

View reviewed changes

jsign requested a review from gballet March 23, 2024 16:17

gballet approved these changes Mar 25, 2024

View reviewed changes

jsign merged commit 70a1231 into main Mar 28, 2024
4 checks passed

jsign deleted the jsign-create-contract-2 branch March 28, 2024 00:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make contract creation work #26

Make contract creation work #26

jsign commented Mar 5, 2024

jsign Mar 5, 2024

gballet Mar 25, 2024

jsign Mar 27, 2024

jsign Mar 5, 2024

jsign Mar 5, 2024

jsign Mar 5, 2024

jsign Mar 5, 2024

gballet Mar 25, 2024

gballet Mar 25, 2024

jsign Mar 27, 2024

jsign Mar 27, 2024

jsign Mar 28, 2024

jsign Mar 5, 2024

jsign Mar 5, 2024

jsign Mar 5, 2024

jsign Mar 5, 2024

gballet left a comment

gballet Mar 25, 2024

jsign Mar 27, 2024

gballet Mar 25, 2024

gballet Mar 25, 2024

gballet Mar 25, 2024

jsign commented Mar 28, 2024 •

edited

Loading

		.input_data = if (msg.target != null) msg.data.ptr else msg.code.ptr,
		.input_size = if (msg.target != null) msg.data.len else msg.code.len,

Make contract creation work #26

Make contract creation work #26

Conversation

jsign commented Mar 5, 2024

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

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

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

jsign commented Mar 28, 2024 • edited Loading

jsign commented Mar 28, 2024 •

edited

Loading