Builtin SHA256 hashing

[MatthewJohnHeath]

Expose the functionality of Zig's std.crypto.hash.sha2.Sha256 through a pure-functional interface to Roc.

[lukewilliamboswell]

@MatthewJohnHeath I tried to push to your branch but I got permission denied. Made a PR in case that helps.

[MatthewJohnHeath]

Hi Luke, Sorry I'm slow to respond. I've been travelling. There are definitely some problems on the branch; it builds but then in the REPL the functions seem to be missing (although triggering a different error to *actually* non-existent function names). I created the initial PR mostly in order to get feedback on what might be wring (I was also expecting to be able to get more time online these couple of weeks). I think I am going to need some help to get it working.

…

On Wed, 14 Aug 2024 at 02:26, Luke Boswell ***@***.***> wrote: @MatthewJohnHeath <https://github.com/MatthewJohnHeath> I tried to push to your branch but I got permission denied. Made a PR in case that helps. — Reply to this email directly, view it on GitHub <#6977 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEZKVQQ7LL7GIFEDIJSQHBTZRKW3PAVCNFSM6AAAAABMI4AGLKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOBXGY2TMNZVHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Matthew Heath <http://mattheath.wordpress.com>

[smores56]

The shape of this PR is a good start, but I'm seeing a potential issue if we add other cryptographic code to this module, which would be quite unsurprising. You have emptySha256, addBytes, and digest. The first is named based on the hash, but the second and third are nominally agnostic to the hash, but only work for SHA-256. Could you rename them to addBytesSha256 or something similar?

[smores56]

Otherwise, I'm attempting to get things running

[smores56]

Okay, I'm almost done getting this working, just need to coordinate the Zig and Roc builtin types.

[smores56]

The REPL worked for me! We'll still need some tests, but it should be good to go, above comments notwithstanding.

[smores56]

Also, there's no code currently to extract state from the new Digest256 opaque type. Could you add a function or something to do that? Maybe Crypt.digest256ToBytes : Digest256 -> List U8?

[MatthewJohnHeath]

Also, there's no code currently to extract state from the new Digest256 opaque type. Could you add a function or something to do that? Maybe Crypt.digest256ToBytes : Digest256 -> List U8?

I was thinking of (only) having the type implement Eq. Can you think of use cases where the individual bytes would be needed?

[smores56]

I was thinking of (only) having the type implement Eq. Can you think of use cases where the individual bytes would be needed?

If I want to store the hash for future comparison. Not that using SHA-256 would be recommended over Argon2, but the idea holds. There are a few algorithms out there that use SHA-256 digest bytes for computation (Wikipedia), at least UUID v5 uses SHA-1 in that way.

[smores56]

Tests are currently failing. @MatthewJohnHeath do you have time to implement the above fixes any time soon? If not, I can help, but it's your PR, so I'll defer to you.

[MatthewJohnHeath]

Tests are currently failing. @MatthewJohnHeath do you have time to implement the above fixes any time soon? If not, I can help, but it's your PR, so I'll defer to you.

@smores56 It's not really clear to me why the build is hanging (which seems to be what is happening). I will try a few things, but might need to ask for help

[MatthewJohnHeath]

@smores56 It's building now. Hopefully I can do those last 2 renamings of functions you asked for tomorrow.

[MatthewJohnHeath]

If this gets through the tests, would it be to merge and then do docs and tests? Or do those need to be there first?

[smores56]

It's always preferable to do at least testing, but ideally also docs, in the same PR. We'd usually avoid them if they're going to make the PR too big, or if they're blocking us merging an important change/bug fix. Since it's neither of those, I'd prefer if we can get them in this change. I'm happy to help with either the tests or docs!

[smores56]

nitpick: Digest256(space)implements ...

also, consider adding an empty doc comment between these two doc comments, that'll follow the "one-line summary on top, context beneath" structure for docs.

[smores56]

I'd recommend "An empty SHA-256 digest."

And it's "to which no data has been added." https://www.thesaurus.com/e/grammar/data-is-or-data-are/

[MatthewJohnHeath]

The word shouldn't be "digest", I think. We should save that for the Digest256 returned after "finalize" has been called on the zig object.
I am struggling to find a good term to describe what the Sha256 object does. My mental model of it is as the state of the algorithm, but that doesn't seem useful wording here. Something like "An empty SHA-256 hasher", maybe?

I agree that data (EDIT)is in this context; I've had too much time in settings where I get corrected the other way.

[smores56]

nitpick: it being an opaque type is implied, I don't think we need to explicitly mention that here.

[smores56]

So I made progress on running things. Tests are running for the builtins, and there is some LLVM issue I'll have to investigate tomorrow:

smores@smortress:~/dev/roc$ cargo run --release --bin roc -- test crates/compiler/builtins/roc/Crypt.roc
    Finished release [optimized] target(s) in 0.16s
     Running `target/release/roc test crates/compiler/builtins/roc/Crypt.roc`
Call parameter type does not match function signature!
%list.RocList %"#arg2"
 ptr  %call_builtin = call i64 @roc_builtins.crypt.sha256AddBytes(i64 %"#arg1", %list.RocList nocapture nonnull readonly byval(%list.RocList) %"#arg2"), !dbg !1635


Function "Crypt_sha256AddBytes_ade2dc9e385e74c61c8d36210907131d7823a7fe8d06c7bd978c1f46a9d3830" failed LLVM verification in NON-OPTIMIZED build. Its content was:

define internal fastcc i64 @Crypt_sha256AddBytes_ade2dc9e385e74c61c8d36210907131d7823a7fe8d06c7bd978c1f46a9d3830(i64 %"#arg1", %list.RocList %"#arg2") !dbg !1634 {
entry:
  %call_builtin = call i64 @roc_builtins.crypt.sha256AddBytes(i64 %"#arg1", %list.RocList nocapture nonnull readonly byval(%list.RocList) %"#arg2"), !dbg !1635
  ret i64 %call_builtin, !dbg !1635
}
thread 'main' panicked at crates/compiler/gen_llvm/src/llvm/build.rs:5810:21:
😱 LLVM errors when defining function "Crypt_sha256AddBytes_ade2dc9e385e74c61c8d36210907131d7823a7fe8d06c7bd978c1f46a9d3830"; I wrote the full LLVM IR to "/tmp/nix-shell.7F740v/test.ll"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[smores56]

I've been distracted, but I'll be looking at this again today. I might have to ask for help from @bhansconnect since I'm not much of an LLVM guy, which seems to be the last issue here. Needless to say, @MatthewJohnHeath there's nothing left that you need to do on this PR. If you have any changes you want to make, feel free, but otherwise I'm planning on merging as soon as I fix the LLVM issue.