Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make contract creation work #26

Merged
merged 6 commits into from
Mar 28, 2024
Merged

Make contract creation work #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
39f9783
types: add receipt structs
jsign Mar 5, 2024
9f51d2f
blockchain: add receipt generation
jsign Mar 5, 2024
7e16741
state/types: in AccountState only free code if it was allocated
jsign Mar 5, 2024
b4c6e6a
blockchain: support contract creation
jsign Mar 5, 2024
763575f
tests: add contract creation custom test
jsign Mar 5, 2024
1b2c564
nits and comments
jsign Mar 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/blockchain/blockchain.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,7 @@ pub const Blockchain = struct {

// prepareMessage prepares an EVM message.
// The caller must call deinit() on the returned Message.
fn prepareMessage(
pub fn prepareMessage(
allocator: Allocator,
caller: Address,
target: ?Address,
Expand Down
27 changes: 21 additions & 6 deletions src/blockchain/vm.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,15 +70,15 @@ pub const VM = struct {
.gas = @intCast(msg.gas),
.recipient = toEVMCAddress(msg.current_target),
.sender = toEVMCAddress(msg.caller),
.input_data = msg.data.ptr,
.input_size = msg.data.len,
.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,
Comment on lines +73 to +74
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.value = blk: {
var tx_value: [32]u8 = undefined;
std.mem.writeIntSliceBig(u256, &tx_value, msg.value);
break :blk .{ .bytes = tx_value };
},
.create2_salt = undefined, // EVMC docs: field only mandatory for CREATE2 kind which doesn't apply at depth 0.
.code_address = toEVMCAddress(msg.code_address),
.code_address = toEVMCAddress(msg.target),
};

const result = EVMOneHost.call(@ptrCast(self), @ptrCast(&evmc_message));
Expand Down Expand Up @@ -402,8 +402,16 @@ const EVMOneHost = struct {
};
}

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"),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no support for STATICCALL ?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
image

};
Comment on lines -405 to +413
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


var result = vm.evm.*.execute.?(
vm.evm,
@ptrCast(&vm.host),
Expand All @@ -415,6 +423,12 @@ const EVMOneHost = struct {
);

if (result.status_code == evmc.EVMC_SUCCESS) {
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"),
};
}
Comment on lines +426 to +431
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Free the backup and indireclty commit to the changes that happened.
prev_statedb.deinit();

Expand All @@ -428,7 +442,8 @@ const EVMOneHost = struct {
vm.env.state.* = prev_statedb;
}

evmclog.debug("call() depth={d} ended", .{msg.*.depth});
evmclog.debug("call() end depth={d} status_code={} gas_left={}", .{ msg.*.depth, result.status_code, result.gas_left });

return result;
}
};
Expand Down
1 change: 1 addition & 0 deletions src/lib.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

std.testing.refAllDeclsRecursive(@import("state/state.zig"));
std.testing.refAllDeclsRecursive(@import("types/types.zig"));
}
19 changes: 18 additions & 1 deletion src/state/statedb.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ pub const StateDB = struct {
accessed_accounts: AddressSet,
accessed_storage_keys: AddressKeySet,

pub fn init(allocator: Allocator, accounts: []AccountState) !StateDB {
pub fn init(allocator: Allocator, accounts: []const AccountState) !StateDB {
var db = AccountDB.init(allocator);
try db.ensureTotalCapacity(@intCast(accounts.len));
for (accounts) |account| {
Expand All @@ -45,9 +45,15 @@ pub const StateDB = struct {
}

pub fn deinit(self: *StateDB) void {
var key_iterator = self.db.keyIterator();
while (key_iterator.next()) |addr| {
self.db.getPtr(addr.*).?.deinit();
}
Comment on lines +48 to +51
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

self.db.deinit();

self.accessed_accounts.deinit();
self.accessed_storage_keys.deinit();

if (self.original_db) |*original_db| {
original_db.deinit();
}
Expand Down Expand Up @@ -130,6 +136,17 @@ pub const StateDB = struct {
_ = self.db.remove(addr);
}

pub fn setContractCode(self: *StateDB, addr: Address, code: []const u8) !void {
var account = self.db.getPtr(addr);
if (account) |acc| {
if (acc.code.len > 0)
return error.AccountAlreadyHasCode;
acc.code = try acc.allocator.dupe(u8, code);
return;
}
try self.db.put(addr, try AccountState.init(self.allocator, addr, 0, 0, code));
}

pub fn accountExistsAndIsEmpty(self: *StateDB, addr: Address) bool {
const account = self.db.get(addr) orelse return false;
return account.nonce == 0 and account.balance == 0 and account.code.len == 0;
Expand Down
5 changes: 3 additions & 2 deletions src/state/types.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,14 @@ pub const AccountState = struct {
.addr = addr,
.nonce = nonce,
.balance = balance,
.code = try allocator.dupe(u8, code),
.code = if (code.len > 0) try allocator.dupe(u8, code) else code,
.storage = std.AutoHashMap(u256, Bytes32).init(allocator),
};
}

pub fn deinit(self: *AccountState) void {
self.storage.deinit();
self.allocator.free(self.code);
if (self.code.len > 0)
self.allocator.free(self.code);
}
};
98 changes: 98 additions & 0 deletions src/tests/custom_tests.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
const std = @import("std");
const config = @import("../config/config.zig");
const common = @import("../common/common.zig");
const vm = @import("../blockchain/vm.zig");
const state = @import("../state/state.zig");
const blockchain = @import("../blockchain/blockchain.zig");
const blockchain_types = @import("../blockchain/types.zig");
const Environment = blockchain_types.Environment;
const types = @import("../types/types.zig");
const Hash32 = types.Hash32;
const Address = types.Address;
const StateDB = state.StateDB;
const ChainID = config.ChainId;

test "create contract" {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var arena = std.heap.ArenaAllocator.init(std.testing.allocator);
defer arena.deinit();
const allocator = arena.allocator();

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();
Comment on lines +20 to +23
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Init a coinbase in our statedb.


// Configure an EVM execution enviroment for a block from this coinbase.
const env: Environment = .{
.block_hashes = [_]Hash32{std.mem.zeroes(Hash32)} ** 256,
.origin = coinbase,
.coinbase = coinbase,
.number = 100,
.base_fee_per_gas = 1,
.gas_limit = 15_000_000,
.gas_price = 100,
.time = 100,
.prev_randao = [_]u8{42} ** 32,
.state = &sdb,
.chain_id = ChainID.SpecTest,
};

var vmi = vm.VM.init(env);
defer vmi.deinit();

// 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;
};
Comment on lines +43 to +78
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Contract creation tx, execute and check that was successful.


// 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);
}
Comment on lines +80 to +97
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
Loading