Conversation
anshalshukla
force-pushed
the
anshalshukla/zig-0.15.2-upgrade
branch
from
21a5a56 to
4c47532
Compare
anshalshukla
force-pushed
the
anshalshukla/zig-0.15.2-upgrade
branch
from
4c47532 to
0b88820
Compare
build/risc0.zig
Outdated
| // user data length + data | ||
| try writer.writeInt(u32, @truncate(bindata.len), .little); | ||
| _ = try writer.write(bindata); | ||
| try writer.interface.writeAll(&std.mem.toBytes(@as(u32, @truncate(bindata.len)))); |
There was a problem hiding this comment.
what if this runs on a big endian platform?
There was a problem hiding this comment.
fixed it, it will now convert it into little endian
pkgs/api/src/event_broadcaster.zig
Outdated
| defer self.mutex.unlock(); | ||
|
|
||
| try self.stream.writeAll(event_json); | ||
| var write_buf: [4096]u8 = undefined; |
There was a problem hiding this comment.
allocate (and free) memory, there's an allocator available in self.
There was a problem hiding this comment.
I've changed it to allocate, 512 bytes instead of 4096 since it is for sse events, I think it is better to do a stack allocation here instead of heap allocation. I've also added testcases to make sure that the write_buf is sufficiently large to handle the event json even if new events are added.
pkgs/cli/src/api_server.zig
Outdated
| var read_buffer: [4096]u8 = undefined; | ||
| var write_buffer: [4096]u8 = undefined; |
There was a problem hiding this comment.
same here, use an allocator
There was a problem hiding this comment.
Just to be clear, this is required so the stack mem is not over utilized to serve API requests? Made the change
pkgs/cli/src/api_server.zig
Outdated
| defer metrics_output.deinit(); | ||
| fn handleMetrics(_: *const Self, request: *std.http.Server.Request) void { | ||
| // 64KiB buffer for metrics output: ipv4 max limit | ||
| var buffer: [65536]u8 = undefined; |
There was a problem hiding this comment.
definitely use an allocator here, this is guaranteed to overflow under stress
pkgs/database/src/interface.zig
Outdated
| /// Helper function to format state keys consistently | ||
| pub fn formatStateKey(allocator: Allocator, state_root: types.Root) ![]const u8 { | ||
| return std.fmt.allocPrint(allocator, "state:{any}", .{std.fmt.fmtSliceHexLower(&state_root)}); | ||
| return std.fmt.allocPrint(allocator, "state:{x}", .{&state_root}); |
There was a problem hiding this comment.
sometimes you pass by value and sometimes by pointer. Try to pass by pointer if you can.
| - Both [risc0](https://github.com/risc0/risc0) (v3.0.3) and OpenVM provers are supported: | ||
| - risc0 requires the toolchain to be [installed](https://dev.risczero.com/api/zkvm/install): `rzup install r0vm 3.0.3` | ||
| - OpenVM is self-contained (no external toolchain needed) | ||
| - Zeam requires zig version 0.15.2 to build. |
There was a problem hiding this comment.
this is the only thing that needs to change in this PR, the rest isn't related to this change and should be reverted
build/risc0.zig
Outdated
| // magic + binary format | ||
| _ = try writer.write(magic); | ||
| try writer.writeInt(u32, BinaryFormatVersion, .little); | ||
| var write_buf: [4096]u8 = undefined; |
There was a problem hiding this comment.
that doesn't look correct, I don't think that the whole binary file can fit in 4K
There was a problem hiding this comment.
@copilot suggest the proper number of bytes to allocate here.
There was a problem hiding this comment.
this change is frankly weird, was it needed or done by AI?
There was a problem hiding this comment.
It's needed because of the change in writer interface. I'll use an allocating writer here instead.
anshalshukla
force-pushed
the
anshalshukla/zig-0.15.2-upgrade
branch
2 times, most recently
from
f6a95a9 to
04c180c
Compare
gballet
left a comment
gballet
left a comment
There was a problem hiding this comment.
LGTM. We agreed with Anshal that this was breaking the risc0 prover, but I will investigate the topic as I am doing heavy editing for provers anyway.
No description provided.