feat(v2): move cryptography to its own library by hamza-syndica · Pull Request #1485 · Syndica/sig

hamza-syndica · 2026-05-27T09:25:28Z

Solves: #1388

As per discussion with @Sobeston: The reed-solomon part got a rewrite in replay branch, it's okay to ignore that for now in this PR.

codecov · 2026-05-27T10:23:07Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
see 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sobeston

This doesn't do what you expect it to do; it needs to be a static library (i.e. its own compilation unit) for the benefits

InKryption

I have more gripes, but the ones I outline here are a bit fundamental and will probably impact any other comments I may have.

InKryption · 2026-06-02T13:36:18Z

+    // Ignore consumer's optimize choice — always ReleaseFast for crypto.
+    _ = b.standardOptimizeOption(.{});


I don't quite understand the point of doing this? If we're going to always use ReleaseFast, it should just not be consumed.

InKryption · 2026-06-02T13:45:00Z

+// -- @extern declarations for cryptographic operations from sig-crypto -- //
+
+// Hash (cryptographic)
+const sig_crypto_hash_init = @extern(
+    *const fn ([*]const u8, usize, *Hash) callconv(.c) void,
+    .{ .name = "sig_crypto_hash_init" },
+);
+const sig_crypto_hash_init_many = @extern(
+    *const fn ([*]const [*]const u8, [*]const usize, usize, *Hash) callconv(.c) void,
+    .{ .name = "sig_crypto_hash_init_many" },
+);
+const sig_crypto_hash_extend = @extern(
+    *const fn (*const Hash, [*]const u8, usize, *Hash) callconv(.c) void,
+    .{ .name = "sig_crypto_hash_extend" },
+);
+const sig_crypto_hash_repeated = @extern(
+    *const fn (*const Hash, *Hash, usize) callconv(.c) void,
+    .{ .name = "sig_crypto_hash_repeated" },
+);
+
+// Signature (cryptographic)
+const sig_crypto_sig_verify = @extern(
+    *const fn (*const Signature, *const Pubkey, [*]const u8, usize) callconv(.c) bool,
+    .{ .name = "sig_crypto_sig_verify" },
+);
+
+// -- Types -- //
+
+pub const ed25519 = struct {
+    // ed25519 functions are called internally by Signature.verify,
+    // which lives in the static library. No direct @extern needed here.
+};


I think it would make sense to expose and move these extern decls into the ed25519 namespace. Also: since you're using @extern, there's no need for the actual decl name to match the extern symbol identifier. So can rename these as sig_crypto_sig_verify -> sigVerify and so on.

Side note: we could export these symbols using camelCase, there's no inherent reason to use snake_case for functions just because they're linked.

InKryption · 2026-06-02T13:46:15Z

+// -- Hash Exports (cryptographic) -- //
+
+fn hashInit(data_ptr: [*]const u8, data_len: usize, out: *Hash) callconv(.c) void {
+    out.* = Hash.init(data_ptr[0..data_len]);
+}
+comptime {
+    @export(&hashInit, .{ .name = "sig_crypto_hash_init" });
+}
+
+fn hashInitMany(
+    ptrs: [*]const [*]const u8,
+    lens: [*]const usize,
+    count: usize,
+    out: *Hash,
+) callconv(.c) void {
+    var slices: [8][]const u8 = undefined;
+    const n = @min(count, 8);
+    for (0..n) |i| {
+        slices[i] = ptrs[i][0..lens[i]];
+    }
+    out.* = Hash.initMany(slices[0..n]);
+}
+comptime {
+    @export(&hashInitMany, .{ .name = "sig_crypto_hash_init_many" });
+}
+
+fn hashExtend(
+    self: *const Hash,
+    data_ptr: [*]const u8,
+    data_len: usize,
+    out: *Hash,
+) callconv(.c) void {
+    out.* = self.extend(data_ptr[0..data_len]);
+}
+comptime {
+    @export(&hashExtend, .{ .name = "sig_crypto_hash_extend" });
+}
+
+fn hashRepeated(input: *const Hash, out: *Hash, count: usize) callconv(.c) void {
+    Hash.hashRepeated(input, out, count);
+}
+comptime {
+    @export(&hashRepeated, .{ .name = "sig_crypto_hash_repeated" });
+}
+
+// -- Signature Exports (cryptographic) -- //
+
+fn sigVerify(
+    self: *const Signature,
+    pubkey: *const Pubkey,
+    msg_ptr: [*]const u8,
+    msg_len: usize,
+) callconv(.c) bool {
+    self.verify(pubkey, msg_ptr[0..msg_len]) catch return false;
+    return true;
+}
+comptime {
+    @export(&sigVerify, .{ .name = "sig_crypto_sig_verify" });
+}


It would seem clearer to me to simply mark these all as export fn? And as stated in the other comment, linker symbols don't have to be snake case.

InKryption · 2026-06-02T13:50:20Z

+    pub fn initMany(data: []const []const u8) Hash {
+        var ptrs: [8][*]const u8 = undefined;
+        var lens: [8]usize = undefined;
+        const n = @min(data.len, 8);
+        for (0..n) |i| {
+            ptrs[i] = data[i].ptr;
+            lens[i] = data[i].len;
+        }
+        var out: Hash = undefined;
+        sig_crypto_hash_init_many(&ptrs, &lens, n, &out);
+        return out;
+    }


Doesn't this mean we're just ignoring data[8..]?
I think this is indicative of this API not making a huge amount of sense at this level. What we should probably do is expose the Sha state in the header, and have an update function equivalent to what we were using before. I imagine there are also other cases where we would want incremental hashing beyond just this.

InKryption · 2026-06-02T13:59:20Z

+fn hashInitMany(
+    ptrs: [*]const [*]const u8,
+    lens: [*]const usize,
+    count: usize,
+    out: *Hash,
+) callconv(.c) void {
+    var slices: [8][]const u8 = undefined;
+    const n = @min(count, 8);
+    for (0..n) |i| {
+        slices[i] = ptrs[i][0..lens[i]];
+    }
+    out.* = Hash.initMany(slices[0..n]);
+}


Re-doubling on my comment about ignoring data[8..].

InKryption · 2026-06-02T14:12:30Z

+            .{ .name = "tracy", .module = tracy_mod },
+            .{ .name = "build-options", .module = build_options_mod },
+        },
+    });


I'm not sure where to put this comment, but I'll put it here:
I was confused by the structure of this library, initially thinking exports.zig had a circular definition with header.zig. This was because I didn't realize that the types are simply defined twice, once for the impl, and once for the header.

I think it would be preferable to have two directories that cleanly divide these: the implementation source, and the exposed header source.

I also think it would be good if we could avoid needing to define the types twice; I will describe one possible approach to doing this:

Have one module, which is just the base ed25519 library.

Define api.zig (can bikeshed name), which defines the fundamental APIs in terms of arrays (no reference to any of the type wrappers like Hash), all with callconv(.c), and all marked pub. Then there should be a comptime block which conditionally exports them. An easy condition for this would be @This() == @import("root"), so that the exports will only be evaluated if we're compiling it directly as the root of a compilation (ie, as a library). We could also give it a dedicated build-options module to import if we want to be more explicit.

Define the header, which will import the api module in order to implement the methods of the exposed types; this is what we expose to consumers.

InKryption · 2026-06-02T14:14:51Z

+    const crypto_lib = b.addLibrary(.{
+        .name = "sig-crypto",
+        .linkage = .static,
+        .root_module = crypto_mod,
+    });
+    b.installArtifact(crypto_lib);


We don't want to install the library; it can't meaningfully be used without its "header", which is the module, so it should only really be used through that.

It could make sense to install it if it had an associated C header file that could be used by another language for FFI, but we aren't doing that, and probably aren't going to do that in any near future, and certainly not if it remains an exclusive subsidiary of the sig repo.

InKryption · 2026-06-02T14:15:41Z

+    _ = api_mod;
+
+    // Test step
+    const test_step = b.step("test", "Run crypto unit tests");


Would prefer to keep steps defined at the top of the build.zig, as is precedent and convention - maybe we should add that to our style/conventions document.

InKryption · 2026-06-02T14:17:11Z

+    .paths = .{
+        "build.zig.zon",
+        "build.zig",
+        "lib.zig",
+        "ed25519.zig",
+        "ed25519",
+        "pubkey.zig",
+        "signature.zig",
+        "hash.zig",
+    },


This seems like maybe a good reason to organize these into dedicated directories (this also relates to my previous comment about organizing the library into 3 parts).

InKryption · 2026-06-02T14:19:04Z

+    if (!@import("builtin").is_test)
+        std.debug.assert(@import("builtin").mode == .ReleaseFast);
+}


Is there any reason for the is_test guard here, considering that we always compile the module with ReleaseFast, even for the test?

hamza-syndica · 2026-06-15T08:56:07Z

Initially felt that, it would be just easy renames with few exports and just using those. But, i think these need some fundamental raw changes (or just redo from scratch) from v2 guys maybe.

separated crypto codes as lib

f15b21a

github-project-automation Bot added this to Sig May 27, 2026

github-project-automation Bot moved this to 🏗 In progress in Sig May 27, 2026

hamza-syndica added 2 commits May 27, 2026 16:00

minor changes

1d3171b

ci build fix

f9ec6dd

hamza-syndica marked this pull request as ready for review May 27, 2026 13:06

hamza-syndica requested a review from Sobeston May 27, 2026 13:07

hamza-syndica marked this pull request as draft May 27, 2026 13:10

hamza-syndica removed the request for review from Sobeston May 27, 2026 13:12

Merge branch 'main' into hamza/crypto-as-lib

3b8ebfc

hamza-syndica marked this pull request as ready for review May 29, 2026 07:54

Sobeston requested changes May 29, 2026

View reviewed changes

github-project-automation Bot moved this from 🏗 In progress to 👀 In review in Sig May 29, 2026

hamza-syndica enabled auto-merge May 29, 2026 08:07

dnut linked an issue May 29, 2026 that may be closed by this pull request

feat(v2): move cryptography to its own library #1388

Open

hamza-syndica added 2 commits June 1, 2026 18:24

updated crypto lib with static build & header.zig

1ced2c6

format fixes

ee53838

hamza-syndica requested a review from Sobeston June 1, 2026 14:07

dnut requested a review from InKryption June 2, 2026 12:54

InKryption requested changes Jun 2, 2026

View reviewed changes

hamza-syndica marked this pull request as draft June 5, 2026 04:52

auto-merge was automatically disabled June 5, 2026 04:52
Pull request was converted to draft

hamza-syndica closed this Jun 15, 2026

github-project-automation Bot moved this from 👀 In review to ✅ Done in Sig Jun 15, 2026

		// Ignore consumer's optimize choice — always ReleaseFast for crypto.
		_ = b.standardOptimizeOption(.{});

Conversation

hamza-syndica commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Sobeston left a comment

Choose a reason for hiding this comment

Uh oh!

InKryption left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hamza-syndica commented Jun 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hamza-syndica commented May 27, 2026 •

edited

Loading

codecov Bot commented May 27, 2026 •

edited

Loading