implement deserialization of newPayloadV2 request #12

gballet · 2023-10-30T10:55:03Z

This PR adds an RPC server (based on a totally overkill library, but that's the only one I could find that supports modules and works). It will decode the execution data payload and turn it into a block.

I have left the execution witness out for the moment.

gballet · 2023-10-30T10:56:41Z

build.zig

+    const zap = b.dependency("zap", .{
+        .target = target,
+        .optimize = optimize,
+    });
+    const mod_zap = zap.module("zap");


zap is a full web framework. I looked at other, simpler libs, but they either didn't work or weren't offering module support.

gballet · 2023-10-30T10:57:42Z

src/engine_api/engine_api.zig

+fn prefixedhex2hash(dst: []u8, src: []const u8) !void {
+    if (src.len < 2 or src.len % 2 != 0) {
+        return error.InvalidInput;
+    }
+    var skip0x: usize = if (src[1] == 'X' or src[1] == 'x') 2 else 0;
+    if (src[skip0x..].len != 2 * dst.len) {
+        return error.InvalidOutputLength;
+    }
+    _ = try fmt.hexToBytes(dst[0..], src[skip0x..]);
+}
+
+fn prefixedhex2byteslice(allocator: Allocator, src: []const u8) ![]u8 {
+    // TODO catch the 0x0 corner case
+    if (src.len < 2 or src.len % 2 != 0) {
+        return error.InvalidInput;
+    }
+    var skip0x: usize = if (src[1] == 'X' or src[1] == 'x') 2 else 0;
+    var dst: []u8 = try allocator.alloc(u8, src[skip0x..].len / 2);
+
+    _ = try fmt.hexToBytes(dst[0..], src[skip0x..]);
+
+    return dst;
+}
+
+fn prefixedhex2u64(src: []const u8) !u64 {
+    // execution engine integers can be odd-length :facepalm:
+    if (src.len < 3) {
+        return error.InvalidInput;
+    }
+
+    var skip0x: usize = if (src[1] == 'X' or src[1] == 'x') 2 else 0;
+
+    return std.fmt.parseInt(u64, src[skip0x..], 16);
+}


I want to try to turn this into a single hex2bin function but that will be the role of a subsequent PR

Worth also consider if these kind of helper funcs are worth putting in a separate package, since sounds can be useful for other things.

gballet · 2023-10-30T10:59:09Z

src/engine_api/execution_payload.zig

+                .block_number = @intCast(self.blockNumber),
+                .gas_limit = @intCast(self.gasLimit),
+                .gas_used = self.gasUsed,
+                .timestamp = @intCast(self.timestamp),


for some reason, your Block type uses i64 for these fields. I assume that it's imposed by evmone, I worked around it for now.

https://github.com/jsign/phant/blob/main/src/vm/vm.zig#L158 :)

gballet · 2023-10-30T10:59:57Z

src/engine_api/execution_payload.zig

+pub fn newPayloadV2Handler(params: *const ExecutionPayload) !void {
+    // TODO reconstruct the proof from the (currently undefined) execution witness
+    // and verify it. Then execute the block and return the result.
+    // vm.run_block(params.to_block(), params.transactions);
+
+    // But so far, just print the content of the payload
+    std.log.info("newPayloadV2Handler: {any}", .{params});
+
+    _ = params.to_block();
+}


currently this simply checks that I can deserialize the block. I will expand that when I am done updating zig-verkle.

gballet · 2023-10-30T11:00:23Z

src/main.zig

+const engine_api = @import("engine_api/engine_api.zig");
+const json = std.json;
+
+var allocator: std.mem.Allocator = undefined;


I made the allocator public as I needed it in my handler.

Woah, OK, that's a bit ugly but I can't see any other workaround considering the forced handler signature.

At some point we can put these handler stuff in a separate file, and make the global allocator scoped there.

Still is a bit ugly that it's implicit that somebody has to initialize that allocator before using things.

gballet · 2023-10-30T11:01:01Z

src/main.zig

+    var listener = zap.SimpleHttpListener.init(.{
+        .port = 8551,
+        .on_request = engineAPIHandler,
+        .log = true,
+    });
+    try listener.listen();
+    std.log.info("Listening on 8551", .{});
+    zap.start(.{
+        .threads = 1,
+        .workers = 1,
+    });


I added that at the end of the main function, but I could also move it to a different binary if you prefer.

I think should be fine; we can spend some time in the future refactoring stuff after we introduce a bunch of features so things don't get out of control.

gballet · 2023-10-30T11:02:24Z

src/main.zig

+
+var allocator: std.mem.Allocator = undefined;
+
+fn engineAPIHandler(r: zap.SimpleRequest) void {


the return type here is void and not !void because you have to report errors as HTTP error codes. Not what I'm used to if you compare it to e.g. rails in dev mode, but why not.

Go is the same:
func handler(w http.ResponseWriter, r *http.Request){}

jsign

LGTM. I left some comments/questions for your consideration.

build.zig

jsign · 2023-10-30T11:12:58Z

build.zig.zon

+    .dependencies = .{ .@"zig-rlp" = .{
+        .url = "https://github.com/gballet/zig-rlp/archive/refs/tags/v0.0.1-beta-0.tar.gz",
+        .hash = "122000e0811d6cb4758f6122b1de2d384efa32b9b2714caec2236f6b34b0529d699c",
+    }, .zap = .{
+        .url = "https://github.com/zigzap/zap/archive/refs/tags/v0.1.14-pre.tar.gz",
+        .hash = "122067d12bc7abb93f7ce623f61b94cadfdb180cef12da6559d092e6b374202acda3",
+    } },


Formatting nit:

.{ .@"zig-rlp" = .{ .url = "https://github.com/gballet/zig-rlp/archive/refs/tags/v0.0.1-beta-0.tar.gz", .hash = "122000e0811d6cb4758f6122b1de2d384efa32b9b2714caec2236f6b34b0529d699c", }, .zap = .{ .url = "https://github.com/zigzap/zap/archive/refs/tags/v0.1.14-pre.tar.gz", .hash = "122067d12bc7abb93f7ce623f61b94cadfdb180cef12da6559d092e6b374202acda3", }, },

yeah my zig formatter is broken in visual studio for some reason, thankfully helix comes to the rescue but I have to manually save the file each time.

also the module name was plain wrong

jsign · 2023-10-30T11:19:12Z

src/engine_api/engine_api.zig

+    if (src[skip0x..].len != 2 * dst.len) {
+        return error.InvalidOutputLength;
+    }
+    _ = try fmt.hexToBytes(dst[0..], src[skip0x..]);


Nit: the dst[0..] is not needed, dst alone should be fine.

jsign · 2023-10-30T11:20:37Z

src/engine_api/engine_api.zig

+fn prefixedhex2hash(dst: []u8, src: []const u8) !void {
+    if (src.len < 2 or src.len % 2 != 0) {
+        return error.InvalidInput;
+    }
+    var skip0x: usize = if (src[1] == 'X' or src[1] == 'x') 2 else 0;
+    if (src[skip0x..].len != 2 * dst.len) {
+        return error.InvalidOutputLength;
+    }
+    _ = try fmt.hexToBytes(dst[0..], src[skip0x..]);
+}
+
+fn prefixedhex2byteslice(allocator: Allocator, src: []const u8) ![]u8 {
+    // TODO catch the 0x0 corner case
+    if (src.len < 2 or src.len % 2 != 0) {
+        return error.InvalidInput;
+    }
+    var skip0x: usize = if (src[1] == 'X' or src[1] == 'x') 2 else 0;
+    var dst: []u8 = try allocator.alloc(u8, src[skip0x..].len / 2);
+
+    _ = try fmt.hexToBytes(dst[0..], src[skip0x..]);
+
+    return dst;
+}
+
+fn prefixedhex2u64(src: []const u8) !u64 {
+    // execution engine integers can be odd-length :facepalm:
+    if (src.len < 3) {
+        return error.InvalidInput;
+    }
+
+    var skip0x: usize = if (src[1] == 'X' or src[1] == 'x') 2 else 0;
+
+    return std.fmt.parseInt(u64, src[skip0x..], 16);
+}


Worth also consider if these kind of helper funcs are worth putting in a separate package, since sounds can be useful for other things.

jsign · 2023-10-30T11:21:38Z

src/engine_api/engine_api.zig

+        return error.InvalidInput;
+    }
+    var skip0x: usize = if (src[1] == 'X' or src[1] == 'x') 2 else 0;
+    var dst: []u8 = try allocator.alloc(u8, src[skip0x..].len / 2);


I know that you're planning to refactor this, but this could do this allocation and call prefixedhex2hash.

added a TODO to remind me to do that, indeed I would rather do it when I refactor.

jsign · 2023-10-30T11:48:52Z

src/engine_api/execution_payload.zig

+                .block_number = @intCast(self.blockNumber),
+                .gas_limit = @intCast(self.gasLimit),
+                .gas_used = self.gasUsed,
+                .timestamp = @intCast(self.timestamp),


https://github.com/jsign/phant/blob/main/src/vm/vm.zig#L158 :)

jsign · 2023-10-30T11:51:36Z

src/main.zig

+    var listener = zap.SimpleHttpListener.init(.{
+        .port = 8551,
+        .on_request = engineAPIHandler,
+        .log = true,
+    });
+    try listener.listen();
+    std.log.info("Listening on 8551", .{});
+    zap.start(.{
+        .threads = 1,
+        .workers = 1,
+    });


I think should be fine; we can spend some time in the future refactoring stuff after we introduce a bunch of features so things don't get out of control.

jsign · 2023-10-30T11:53:22Z

src/main.zig

+const engine_api = @import("engine_api/engine_api.zig");
+const json = std.json;
+
+var allocator: std.mem.Allocator = undefined;


Woah, OK, that's a bit ugly but I can't see any other workaround considering the forced handler signature.

At some point we can put these handler stuff in a separate file, and make the global allocator scoped there.

Still is a bit ugly that it's implicit that somebody has to initialize that allocator before using things.

jsign · 2023-10-30T11:58:27Z

src/main.zig

+
+var allocator: std.mem.Allocator = undefined;
+
+fn engineAPIHandler(r: zap.SimpleRequest) void {


Go is the same:
func handler(w http.ResponseWriter, r *http.Request){}

jsign · 2023-10-30T11:59:59Z

src/main.zig

+        return;
+    }
+    const payload = json.parseFromSlice(engine_api.EngineAPIRequest, allocator, r.body.?, .{ .ignore_unknown_fields = true }) catch |err| {
+        std.log.err("error parsing json: {} (body={s})", .{ err, r.body.? });


I think at some point we have to work on some log package.

jsign · 2023-10-30T12:04:36Z

@gballet, there's a failing test. I suspect might be related to added fields in the Block.

gballet · 2023-11-01T09:46:32Z

right, it seems that zig-rlp doesn't support optional fields the way they should work.

jsign · 2023-11-20T20:18:35Z

src/engine_api/execution_payload.zig

    // TODO reconstruct the proof from the (currently undefined) execution witness
    // and verify it. Then execute the block and return the result.
    // vm.run_block(params.to_block(), params.transactions);

    // But so far, just print the content of the payload
    std.log.info("newPayloadV2Handler: {any}", .{params});

-    _ = params.to_block();
+    var block = params.to_block();


L63 and L64 look quite weird to me. How do you know that params.to_block() allocator is the same that we received in this method?

These kind of patterns should probably make block have the right allocator internally and make L64 be block.deinit().

This potential mismatch between the received allocation, and whatever params.to_block() allocator was used, is quite confusing and hard to know if it's doing the right thing.

There are several issues with this: first of all, the data that is freed has been allocated long before, with params - but a reference to the allocator could be copied from params in to_block. The other problem, however, is that the block can be serialized to RLP, which would make no sense if it holds an allocator.

well actually in ssz I offer the possibility to write a custom encoder, maybe I could do the same here, and so skip the allocator if it exists.

see https://github.com/gballet/phant/pull/1 for the current state

I think this confusion is mostly making it clear in the to_block() method comments who is the owner of params after that call.

It looks like block is now the owner of param after to_block() or something like that? But I'm not sure what it would mean if you do two to_block() calls -- wouldn't that mean that you risk doing a double-free?

To be sure of course you can do a copy in block of everything it needs so both things aren't tied together and solve the problem at the cost of a copy.

To be clear, I'm not sure what is the right solution.
But here's when ownership gets interesting and it's good to find good patterns to avoid remembering "who owns what" of if constructing more than one to_block() is safe or not.

See https://github.com/gballet/phant/pull/1#discussion_r1417657311 for some extra comment

gballet · 2023-12-08T11:14:27Z

Replaced by #2, closing.

gballet · 2023-12-08T11:18:35Z

That was the wrong PR to close 🤦‍♂️

gballet requested a review from jsign October 30, 2023 10:55

gballet commented Oct 30, 2023

View reviewed changes

jsign approved these changes Oct 30, 2023

View reviewed changes

jsign approved these changes Nov 20, 2023

View reviewed changes

jsign mentioned this pull request Dec 4, 2023

Support transaction signing and public key recovery #13

Merged

gballet closed this Dec 8, 2023

gballet reopened this Dec 8, 2023

gballet force-pushed the new-payload-v2 branch from 51ca3ac to d29ab6a Compare December 8, 2023 11:31

implement deserialization of newPayloadV2 request

761efd0

gballet force-pushed the new-payload-v2 branch from d29ab6a to 761efd0 Compare December 8, 2023 11:34

gballet merged commit 4938f47 into jsign:main Dec 8, 2023
4 checks passed


		var allocator: std.mem.Allocator = undefined;

		fn engineAPIHandler(r: zap.SimpleRequest) void {

implement deserialization of newPayloadV2 request #12

implement deserialization of newPayloadV2 request #12

Conversation

gballet commented Oct 30, 2023

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

jsign 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

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

jsign commented Oct 30, 2023

gballet commented Nov 1, 2023

jsign Nov 20, 2023 • 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

gballet commented Dec 8, 2023

gballet commented Dec 8, 2023

jsign Nov 20, 2023 •

edited

Loading