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.

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

More progress integrating EVMOne #2

Merged
merged 7 commits into from
Aug 5, 2023
Merged

More progress integrating EVMOne #2

merged 7 commits into from
Aug 5, 2023

Conversation

jsign
Copy link
Owner

@jsign jsign commented Aug 5, 2023

This PR:

  • Made progress in vm/vm.zig, which integrates better with EVMOne. I like how it looks; I still have to make more progress, but that will be next. I'd invite skim again vm/vm.zig since it might be worth it.
  • Rewrote src/main.zig to now use the VM abstraction instead of hacky direct usage of EVMOne. Now it's using something closer to the real APIs.
  • Other overall improvements.

I'd suggest seeing the code directly in the merged version, since looking at diffs or doing reviews now can confuse these kinds of "big changes". Maybe it's worth doing it when things settle down a bit or if the changes are more self-contained:

I left some comments that might be interesting for readers.

cc @gballet

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

Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
@@ -1,72 +1,54 @@
const std = @import("std");
Copy link
Owner Author

Choose a reason for hiding this comment

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

Much nicer to see main.zig now to play with things.
It still has limited usage, but it will keep getting more complete.

.kind = evmc.EVMC_CALL, // TODO(jsign): generalize.
.flags = evmc.EVMC_STATIC,
.depth = 0,
.gas = @intCast(txn.gas_limit), // TODO(jsign): why evmc expects a i64 for gas instead of u64?
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'm unsure why EVMC defines .gas as a signed integer instead of unsigned; maybe I'm missing something.

.create2_salt = .{
.bytes = [_]u8{0} ** 32, // TODO: fix this.
},
.code_address = util.to_evmc_address(txn.to), // TODO: fix this when .kind is generalized.
Copy link
Owner Author

Choose a reason for hiding this comment

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

const addr_typeinfo = @typeInfo(@TypeOf(address));
if (@TypeOf(address) != Address and addr_typeinfo.Optional.child != Address) {
@compileError("address must be of type Address or ?Address");
// ### EVMC Host Interface ###
Copy link
Owner Author

Choose a reason for hiding this comment

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

So now my next steps is mainly to fix these @panic("TODO"). At least, at a minimum, to run this spec-test successfully.
Then keep adding more spec-tests and keep completing the implementation.

Comment on lines +229 to +253
fn call(ctx: ?*evmc.struct_evmc_host_context, msg: [*c]const evmc.struct_evmc_message) callconv(.C) evmc.struct_evmc_result {
const vm: *VM = @as(*VM, @alignCast(@ptrCast(ctx.?)));
log.debug("call()", .{}); // TODO(jsign): explore creating custom formatter?

// Persist the current context. We'll restore it after the call return.
const current_context = vm.*.exec_context.?;
_ = current_context;

// Create the new context to be used to do the call.
vm.exec_context = ExecutionContext{ .address = util.from_evmc_address(msg.*.recipient) };

var recipient_code: Bytecode = &[_]u8{};
const recipient_account = vm.statedb.get(util.from_evmc_address(msg.*.code_address));
if (recipient_account) |account| {
recipient_code = account.code;
} else unreachable;

if (vm.evm.*.execute) |exec| {
// TODO(jsign): EVMC_SHANGHAI should be configurable at runtime.
// TODO(jsign): remove ptrCast
var result = exec(vm.evm, @ptrCast(&vm.host), @ptrCast(vm), evmc.EVMC_SHANGHAI, @ptrCast(msg), recipient_code.ptr, recipient_code.len);
log.debug("execution result: status_code={}, gas_left={}", .{ result.status_code, result.gas_left });
} else unreachable;

@panic("TODO");
Copy link
Owner Author

Choose a reason for hiding this comment

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

Already made some progress for call(...) which is something I need for the existing spec-test to run, since it's a contract call that calls another contract.

Here we see how we keep the current_context, and create a new context to do the call to the other contract.
This is still incomplete, but shows how ExecContext in vm.exec_context is used.

}

fn call(ctx: ?*evmc.struct_evmc_host_context, msg: [*c]const evmc.struct_evmc_message) callconv(.C) evmc.struct_evmc_result {
const vm: *VM = @as(*VM, @alignCast(@ptrCast(ctx.?)));
Copy link
Owner Author

Choose a reason for hiding this comment

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

Note how EVMC sends us an ctx opaque{} via params that we know is our VM.

This opaque{} is something our VM can tell EVMOne to always send in every EVMC interface call, so I choose that to be a reference to our VM instance. Having the *VM means we can access the current execution context and useful fields (e.g: statedb, etc)..

This line will be included in all the other EVMC interfaces that have @panic("TODO").

@jsign
Copy link
Owner Author

jsign commented Aug 5, 2023

Both zig build run and zig build test run correctly, so merging.

@jsign jsign merged commit 88757bf into main Aug 5, 2023
@jsign jsign deleted the jsign-vm-execution branch August 5, 2023 23:19
@jsign jsign mentioned this pull request Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant