Keccak Rust Wrappers #1337

qwang98 · 2024-04-30T21:40:38Z

Input for syscall is one memory pointer to the state array of 25 gl fe. Output is calculated in place.

All functions outside of keccakf are executed in Rust. Might need to delete everything except keccakf from keccakf.asm (including the padding, updating, and byte <-> u64 conversions).

Not tested. Should I wait till all infrastructure needed are merged? Technically can also do the following for the machine and I think I can test it already?

pol commit input_state[25];
pol commit output_state[25];
operation keccakf_permutation<0> input_state[0], input_state[1], ..., input_state[24] -> output_state[0], output_state[1], ..., output_state[24]
array::zip(input_state, output_state, |input_state, output_state| (keccakf(input_state) - output_state = 0);

riscv/src/runtime.rs

test_data/asm/keccakf.asm

riscv/src/runtime.rs

test_data/asm/keccakf.asm

riscv/src/runtime.rs

riscv-runtime/src/hash.rs

riscv/src/runtime.rs

qwang98 · 2024-05-06T16:37:50Z

Ready for review again.

riscv-runtime/src/hash.rs

riscv/src/runtime.rs

std/machines/hash/keccakf.asm

pacheco · 2024-05-06T22:31:09Z

test_data/asm/keccakf.asm

    let x;
+    let y;
+    operation keccakf x -> y;


georgwiese

Very cool! :)

riscv-runtime/src/hash.rs

std/machines/hash/keccakf.asm

riscv-runtime/src/hash.rs

riscv/src/runtime.rs

qwang98 · 2024-05-14T17:58:52Z

Implement all comments. Ready for a final review.

Changes:

Rust wrapper takes two memory pointers and uses reinterpret casting.
Rust wrapper tested correct using tiny keccak instead of unfinished syscall, which can't be done yet.

Tested using: cargo run -r --bin powdr-rs compile riscv/tests/riscv_data/keccak -o output && RUST_BACKTRACE=full cargo test --release --package powdr-riscv --test riscv -- keccak --exact --show-output --ignored

riscv/src/runtime.rs

std/machines/hash/keccakf.asm

test_data/asm/keccakf.asm

riscv/src/runtime.rs

std/machines/hash/keccakf.asm

test_data/asm/keccakf.asm

riscv/tests/riscv_data/keccak/src/lib.rs

pacheco · 2024-05-15T02:21:46Z

riscv-runtime/Cargo.toml

@@ -11,6 +11,7 @@ repository = "https://github.com/powdr-labs/powdr"
 serde = { version = "1.0", default-features = false, features = ["alloc", "derive", "rc"] }
 serde_cbor = { version = "0.11.2", default-features = false, features = ["alloc"] }
 powdr-riscv-syscalls = { path = "../riscv-syscalls" }
+tiny-keccak = { version = "2.0.2", features = ["keccak"] }


not sure we want this here... i think the suggestion for tiny-keccak was mostly so it was possible to check things were proper without having the asm side. @leonardoalt?

Exactly. Still prefer to have it here or we need to remove the test coverage in riscv.rs entirely.

I don't think this should be here. Why do we need this dependency here for tests?

This is because currently the keccak.asm isn't ready and depends on Chris's circuit compiler #1218 to be merged first, so we put a placeholder keccak.asm. To test the Rust wrapper, we instead use the tiny-keccak.

I'm removing this now and leaving the keccakf Rust wrapper as unimplemented.

pacheco · 2024-05-15T02:24:25Z

riscv-runtime/src/hash.rs

+        // asm!("ecall", in("a0") input, in("a1") output, in("t0") u32::from(Syscall::KeccakF));
+    // }
+
+    // TODO: delete the following testing only chunk which uses tiny_keccak once syscall implemented


and this would be just unimplemented for now

and this would be just unimplemented for now

I think the tiny keccak part is still needed just for the test in riscv.rs to pass.

which test is that? The keccak test already imports tiny keccak itself, we shouldn't ship tiny keccak with the riscv-runtime crate

It's the test using our Rust wrapper (rather than tiny keccak) here https://github.com/powdr-labs/powdr/blob/main/riscv/tests/riscv_data/keccak/src/lib.rs, which I'm now moving to a separate draft PR. I'm also leaving this as unimplemented! for now, and the contents here are also moved to the same draft PR.

riscv-runtime/src/hash.rs

qwang98 · 2024-05-15T15:13:03Z

All comments addressed and ready for review again. @pacheco

riscv/src/runtime.rs

.gitignore

leonardoalt · 2024-05-28T15:49:13Z

riscv-runtime/Cargo.toml

@@ -11,6 +11,7 @@ repository = "https://github.com/powdr-labs/powdr"
 serde = { version = "1.0", default-features = false, features = ["alloc", "derive", "rc"] }
 serde_cbor = { version = "0.11.2", default-features = false, features = ["alloc"] }
 powdr-riscv-syscalls = { path = "../riscv-syscalls" }
+tiny-keccak = { version = "2.0.2", features = ["keccak"] }


I don't think this should be here. Why do we need this dependency here for tests?

riscv-runtime/src/hash.rs

leonardoalt · 2024-05-28T15:50:36Z

riscv-runtime/src/hash.rs

+        // asm!("ecall", in("a0") input, in("a1") output, in("t0") u32::from(Syscall::KeccakF));
+    // }
+
+    // TODO: delete the following testing only chunk which uses tiny_keccak once syscall implemented


which test is that? The keccak test already imports tiny keccak itself, we shouldn't ship tiny keccak with the riscv-runtime crate

riscv-runtime/src/hash.rs

riscv/src/runtime.rs

leonardoalt · 2024-05-28T15:53:20Z

riscv/tests/riscv_data/keccak/src/lib.rs

@@ -1,19 +1,32 @@
 #![no_std]

-use tiny_keccak::{Hasher, Keccak};
+extern crate alloc;


I don't think this test should be modified like this, I think we should add a new test instead. It's still good to have a test that uses native tiny keccak.

Alrighty. I'm restoring this file entirely and removed all tests using our Rust wrapper because it still depends on keccak.asm which is road blocked by Chris' circuit compiler.

I'm moving the tests for Rust wrapper to another draft PR, so that we can merge after all road blocks are cleared.

…keccakf to a separate draft pr

qwang98 · 2024-05-29T23:14:54Z

Split roadblocked contents to #1408. This PR is ready for a final review. @leonardoalt

leonardoalt · 2024-05-31T14:53:25Z

riscv/tests/riscv_data/keccak/Cargo.toml

@@ -8,5 +8,6 @@ edition = "2021"
 [dependencies]
 tiny-keccak = { version = "2.0.2", features = ["keccak"] }
 powdr-riscv-runtime = { path = "../../../../riscv-runtime" }
+hex-literal = "0.3.1"


I think this can also go?

leonardoalt

Sorry for being pedantic, but I think we could still add another riscv test (like a sibling of riscv/tests/riscv_data/keccak) that does call the new syscall but has a dummy assert, just so that's "ready to go"

qwang98 added 3 commits April 30, 2024 10:18

work

1755036

runtime syscall and submachine wrapper

223c994

removed unwanted files

ae69087

qwang98 requested a review from chriseth April 30, 2024 21:40

init call

ea306ec

qwang98 commented Apr 30, 2024

View reviewed changes

riscv/src/runtime.rs Outdated Show resolved Hide resolved

qwang98 commented Apr 30, 2024

View reviewed changes

test_data/asm/keccakf.asm Outdated Show resolved Hide resolved

fmt

9a39c37

qwang98 requested a review from leonardoalt April 30, 2024 22:12

leonardoalt reviewed May 1, 2024

View reviewed changes

pacheco reviewed May 2, 2024

View reviewed changes

riscv/src/runtime.rs Outdated Show resolved Hide resolved

qwang98 added 3 commits May 6, 2024 10:53

work

b2b20cf

merge main

1eb6d92

addressed comments

476e3de

qwang98 requested review from pacheco and leonardoalt May 6, 2024 16:38

qwang98 added 3 commits May 6, 2024 12:41

fmt

7e235f7

fix ci

49bf3b7

fmt

43db72e

pacheco reviewed May 6, 2024

View reviewed changes

georgwiese reviewed May 7, 2024

View reviewed changes

chriseth reviewed May 7, 2024

View reviewed changes

riscv-runtime/src/hash.rs Show resolved Hide resolved

chriseth reviewed May 7, 2024

View reviewed changes

riscv-runtime/src/hash.rs Outdated Show resolved Hide resolved

chriseth reviewed May 7, 2024

View reviewed changes

riscv/src/runtime.rs Show resolved Hide resolved

qwang98 added 3 commits May 14, 2024 09:08

updated wrappers

a6862cd

fixed signature

9f71f09

test configuration

507320b

qwang98 force-pushed the keccak-machine branch from 096ea97 to 507320b Compare May 14, 2024 18:02

qwang98 added 2 commits May 14, 2024 14:02

Merge branch 'main' into keccak-machine

3fc9ff3

testing only

adb4305

qwang98 requested review from pacheco and georgwiese May 14, 2024 18:06

qwang98 commented May 14, 2024

View reviewed changes

riscv/src/runtime.rs Show resolved Hide resolved

qwang98 commented May 14, 2024

View reviewed changes

std/machines/hash/keccakf.asm Outdated Show resolved Hide resolved

qwang98 commented May 14, 2024

View reviewed changes

test_data/asm/keccakf.asm Outdated Show resolved Hide resolved

fmt touches

38e27c9

pacheco reviewed May 15, 2024

View reviewed changes

qwang98 added 2 commits May 15, 2024 10:51

addressed comments

82956ba

update test

5ab7d46

qwang98 requested a review from pacheco May 15, 2024 15:13

chriseth reviewed May 21, 2024

View reviewed changes

riscv/src/runtime.rs Show resolved Hide resolved

leonardoalt reviewed May 28, 2024

View reviewed changes

qwang98 added 3 commits May 29, 2024 18:30

work

8af4bbf

restored tiny keccak test and moving rust wrapper implementation for …

b2de0ed

…keccakf to a separate draft pr

work

6d8843f

qwang98 mentioned this pull request May 29, 2024

Keccak Rust Wrapper Finalize #1408

Draft

qwang98 added 3 commits May 29, 2024 19:17

fix CI

19e7237

rerun CI

0b1cad5

rerun CI

9b70505

qwang98 requested a review from leonardoalt May 30, 2024 00:38

leonardoalt reviewed May 31, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keccak Rust Wrappers #1337

Keccak Rust Wrappers #1337

qwang98 commented Apr 30, 2024 •

edited

qwang98 commented May 6, 2024

pacheco May 6, 2024

qwang98 May 7, 2024

georgwiese left a comment

qwang98 commented May 14, 2024

pacheco May 15, 2024

qwang98 May 15, 2024 •

edited

leonardoalt May 28, 2024

qwang98 May 29, 2024

qwang98 May 29, 2024

pacheco May 15, 2024

qwang98 May 15, 2024

leonardoalt May 28, 2024

qwang98 May 29, 2024 •

edited

qwang98 commented May 15, 2024

leonardoalt May 28, 2024

leonardoalt May 28, 2024

leonardoalt May 28, 2024

qwang98 May 29, 2024

qwang98 commented May 29, 2024

leonardoalt May 31, 2024

leonardoalt left a comment

Keccak Rust Wrappers #1337

Are you sure you want to change the base?

Keccak Rust Wrappers #1337

Conversation

qwang98 commented Apr 30, 2024 • edited

qwang98 commented May 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgwiese left a comment

Choose a reason for hiding this comment

qwang98 commented May 14, 2024

Choose a reason for hiding this comment

qwang98 May 15, 2024 • edited

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

qwang98 May 29, 2024 • edited

Choose a reason for hiding this comment

qwang98 commented May 15, 2024

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

qwang98 commented May 29, 2024

Choose a reason for hiding this comment

leonardoalt left a comment

Choose a reason for hiding this comment

qwang98 commented Apr 30, 2024 •

edited

qwang98 May 15, 2024 •

edited

qwang98 May 29, 2024 •

edited