Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
f2abeda to
eba600b
Compare
|
Couple things
|
get seccomp violation traces
refactor remove unused style fix typo main: use gpa
| inline for (@import("src/services.zon")) |service_name| { | ||
| const service_mod = b.createModule(.{ | ||
| .target = target, | ||
| .optimize = optimize, | ||
| .root_source_file = b.path("src/services").path(b, service_name ++ ".zig"), | ||
| .single_threaded = true, | ||
| .omit_frame_pointer = false, | ||
| }); | ||
| service_mod.addImport("common", common); | ||
| service_mod.addImport("start", start_service); | ||
| service_mod.addImport("tracy", tracy); | ||
|
|
||
| const lib_svc = b.addLibrary(.{ | ||
| .name = service_name, | ||
| .root_module = service_mod, | ||
| .use_llvm = true, | ||
| }); | ||
| sig_init.linkLibrary(lib_svc); | ||
|
|
||
| const service_tests = b.addTest(.{ .root_module = service_mod, .name = service_name }); | ||
| const service_tests_run = b.addRunArtifact(service_tests); | ||
| test_step.dependOn(&service_tests_run.step); | ||
| } | ||
|
|
||
| const validate_services_list_exe = b.addExecutable(.{ | ||
| .name = "validate_services_list", | ||
| .root_module = b.createModule(.{ | ||
| .root_source_file = b.path("scripts/validate_services_list.zig"), | ||
| .target = b.graph.host, | ||
| .optimize = .Debug, | ||
| .imports = &.{ | ||
| .{ | ||
| .name = "services", | ||
| .module = b.createModule(.{ .root_source_file = b.path("src/services.zon") }), | ||
| }, | ||
| }, | ||
| }), | ||
| }); | ||
| const validate_services_list_run = b.addRunArtifact(validate_services_list_exe); | ||
| validate_services_list_run.addDirectoryArg(b.path("src/services")); | ||
| b.getInstallStep().dependOn(&validate_services_list_run.step); | ||
| } |
There was a problem hiding this comment.
Could inline the service list here instead of the zon file. Would remove the need for the zon file + the validate_service_list script.
| const SIZE_OF_MERKLE_ROOT: usize = Hash.SIZE; | ||
|
|
||
| /// Analogous to [Shred](https://github.com/anza-xyz/agave/blob/8c5a33a81a0504fd25d0465bed35d153ff84819f/ledger/src/shred.rs#L245) | ||
| pub const Shred = union(ShredType) { |
There was a problem hiding this comment.
Does this need to be in common? There's a good chance this won't be used outside the shred service. I'd rather keep things scoped in services and only move them into common when it's clearly needed by multiple services.
There was a problem hiding this comment.
Keeping services lean and focused on the data structures and algorithms which facilitate their higher level logic is preferable. A specific data structure being used by only a single service is not a good justification for its implementation being defined within that service. For example we would not implement the zksdk inside the zk_elgamal program simply because that is the only place it is used at the moment. Instead the zk_elgamal program defines only its instruction type and execution function which together define its 'higher level logic' (i.e. what is it doing).
There was a problem hiding this comment.
Let's consider a hypothetical data structure that defines a domain-specific concept which is only meaningful in the one very narrow context that is fully implemented by a single service. The concept has no relevance in any other context outside that service, and it never well. It's not general purpose code that will likely to be useful in any other context. In that case, it makes no sense for this data structure to exist in a common library. Can we agree on this much? My claim is that Shred follows this pattern.
| test { | ||
| _ = std.testing.refAllDecls(@This()); | ||
| } |
There was a problem hiding this comment.
Is this going to be needed for every namespace that contains tests? I get that refAllDeclsRecursive isn't perfect, but I also don't like needing to add this to every file.
Also, I don't think you need to put this in a test {} block. This makes the test counts confusing because it adds another unit test for every instance of test {}. It's not a big problem but you could just put it in a comptime block instead to achieve the same thing. refAllDecls is a noop if it's not a test build.
| test { | |
| _ = std.testing.refAllDecls(@This()); | |
| } | |
| comptime { | |
| std.testing.refAllDecls(@This()); | |
| } |
| const common = @import("../../common.zig"); | ||
|
|
||
| const std = @import("std"); | ||
|
|
||
| const Slot = common.solana.Slot; | ||
| const Pubkey = common.solana.Pubkey; |
There was a problem hiding this comment.
| const common = @import("../../common.zig"); | |
| const std = @import("std"); | |
| const Slot = common.solana.Slot; | |
| const Pubkey = common.solana.Pubkey; | |
| const std = @import("std"); | |
| const solana = @import("../solana.zig"); | |
| const Slot = solana.Slot; | |
| const Pubkey = solana.Pubkey; |
The code is more modular if it only reaches out into the nearest parent scope that's necessary to get its dependencies. We shouldn't make every file depend on the overall structure of common if it's not necessary. It'll be easier to refactor the code if things are more tightly scoped, and I don't see a downside to it.
| defer diag.deinit(allocator); | ||
|
|
||
| break :cfg std.zon.parse.fromSlice(Config, allocator, cfg_str, &diag, .{}) catch |err| { | ||
| std.debug.print("{f}\n", .{diag}); |
| const shared_regions: []const services.SharedRegion = &.{ | ||
| .{ | ||
| .region = .{ .net_pair = .{ .port = config.shred_network.recv_port } }, | ||
| .shares = &.{ | ||
| .{ .instance = .{ .service = .shred_receiver }, .rw = true }, | ||
| .{ .instance = .{ .service = .net }, .rw = true }, | ||
| }, | ||
| }, | ||
| .{ | ||
| .region = .{ .leader_schedule = .{ .schedule_string = &reader.interface } }, | ||
| .shares = &.{ | ||
| .{ .instance = .{ .service = .shred_receiver } }, | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
It's not in scope right now but I'm curious on your thoughts. It seems like the types and numbers of expected shares per service could actually be partly inferred from each serviceMain's parameters and validated at comptime against these shares.
| var status: u32 = 0; | ||
| const exited_pid: i32 = pid: { | ||
| const ret: usize = linux.waitpid(-1, &status, 0); | ||
| std.debug.assert(ret != -1); |
There was a problem hiding this comment.
this is a tautology for usize.
is this more meaningful?
| std.debug.assert(ret != -1); | |
| std.debug.assert(ret != std.math.maxInt(usize)); |
or should we be checking e(ret) != .SUCCESS?
| pub var stderr: std.os.linux.fd_t = undefined; | ||
| pub var exit: *common.Exit = undefined; |
There was a problem hiding this comment.
is it safe to set these to undefined? what if something tries to use one of these before they are set?
| } | ||
|
|
||
| pub fn get(self: *const MerkleProofEntryList, index: usize) ?MerkleProofEntry { | ||
| if (index > self.len) return null; |
There was a problem hiding this comment.
pre-existing bug
| if (index > self.len) return null; | |
| if (index >= self.len) return null; |
| } | ||
|
|
||
| const SECCOMP = std.os.linux.SECCOMP; | ||
| const syscalls = std.os.linux.syscalls.X64; |
There was a problem hiding this comment.
| const syscalls = std.os.linux.syscalls.X64; | |
| const syscalls = std.os.linux.SYS; |
| if (std.os.linux.syscall3(.close_range, 0, @intCast(stderr - 1), 0) != 0) | ||
| std.debug.panic("close_range failed\n", .{}); | ||
| if (std.os.linux.syscall3(.close_range, @intCast(stderr + 1), max_fd, 0) != 0) | ||
| std.debug.panic("close_range failed\n", .{}); |
There was a problem hiding this comment.
not that i would expect stderr to actually reach numbers this high or low, but saturating is equally correct and theoretically safer.
| if (std.os.linux.syscall3(.close_range, 0, @intCast(stderr - 1), 0) != 0) | |
| std.debug.panic("close_range failed\n", .{}); | |
| if (std.os.linux.syscall3(.close_range, @intCast(stderr + 1), max_fd, 0) != 0) | |
| std.debug.panic("close_range failed\n", .{}); | |
| if (std.os.linux.syscall3(.close_range, 0, @intCast(stderr -| 1), 0) != 0) | |
| std.debug.panic("close_range failed\n", .{}); | |
| if (std.os.linux.syscall3(.close_range, @intCast(stderr +| 1), max_fd, 0) != 0) | |
| std.debug.panic("close_range failed\n", .{}); |
Working
Example output:
What a minimal service looks like: