feat(blockifier): add support for keccak syscall #1546
Conversation
varex83
added
the
native integration
|
This change is |
|
Artifacts upload triggered. View details here |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1546 +/- ##
===========================================
+ Coverage 40.10% 67.67% +27.56%
===========================================
Files 26 103 +77
Lines 1895 13827 +11932
Branches 1895 13827 +11932
===========================================
+ Hits 760 9357 +8597
- Misses 1100 4070 +2970
- Partials 35 400 +365 ☔ View full report in Codecov by Sentry. |
rodrigo-pino
force-pushed
the
rdr/update-testing-suite
branch
2 times, most recently
from
c728762 to
4cf52c8
Compare
varex83
changed the title
varex83
force-pushed
the
bohdan/add-keccak-syscall
branch
from
cb606a0 to
ada76ce
Compare
|
Artifacts upload triggered. View details here |
|
Artifacts upload triggered. View details here |
|
Artifacts upload triggered. View details here |
|
Artifacts upload triggered. View details here |
rodrigo-pino
force-pushed
the
rdr/update-testing-suite
branch
2 times, most recently
from
a00994e to
e2c9fe8
Compare
PearsonWhite
changed the base branch from
rdr/update-testing-suite
to
rdr/add-syscall-counting
|
Artifacts upload triggered. View details here |
rodrigo-pino
force-pushed
the
rdr/add-syscall-counting
branch
2 times, most recently
from
147c1d3 to
ded98cf
Compare
varex83
force-pushed
the
bohdan/add-keccak-syscall
branch
from
a63f4d1 to
59867fc
Compare
|
Artifacts upload triggered. View details here |
There was a problem hiding this comment.
Reviewed 1 of 3 files at r2, 1 of 2 files at r9, 1 of 1 files at r10.
Reviewable status: 3 of 6 files reviewed, 1 unresolved discussion (waiting on @noaov1 and @varex83)
crates/blockifier/src/execution/native/syscall_handler.rs line 233 at r10 (raw file):
)?; const KECCAK_FULL_RATE_IN_WORDS: usize = 17;
Where did this number come from?
Code quote:
const KECCAK_FULL_RATE_IN_WORDS: usize = 17;
There was a problem hiding this comment.
Reviewable status: 3 of 6 files reviewed, 1 unresolved discussion (waiting on @meship-starkware and @noaov1)
crates/blockifier/src/execution/native/syscall_handler.rs line 233 at r10 (raw file):
Previously, meship-starkware (Meshi Peled) wrote…
Where did this number come from?
The implementation of keccak is fully done by rewriting the original VM implementation, so this const is from here, please take a look here: https://github.com/starkware-libs/sequencer/blob/main/crates/blockifier/src/execution/syscalls/mod.rs#L677
rodrigo-pino
force-pushed
the
rdr/add-syscall-counting
branch
5 times, most recently
from
6e88aa9 to
55834b2
Compare
rodrigo-pino
force-pushed
the
rdr/add-syscall-counting
branch
from
139d79d to
3d4ab3d
Compare
varex83
force-pushed
the
bohdan/add-keccak-syscall
branch
from
c9d0ba1 to
721eb95
Compare
|
Artifacts upload triggered. View details here |
There was a problem hiding this comment.
Reviewed 5 of 5 files at r12, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @varex83)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @varex83)
varex83
force-pushed
the
bohdan/add-keccak-syscall
branch
from
721eb95 to
e0191eb
Compare
|
Artifacts upload triggered. View details here |
varex83
force-pushed
the
bohdan/add-keccak-syscall
branch
from
e0191eb to
d8cb878
Compare
|
Artifacts upload triggered. View details here |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r13, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @varex83)
There was a problem hiding this comment.
You have some conflicts please rebase
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @varex83)
varex83
force-pushed
the
bohdan/add-keccak-syscall
branch
from
d8cb878 to
6479b44
Compare
|
Artifacts upload triggered. View details here |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r14, all commit messages.
Dismissed @graphite-app[bot] from a discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @varex83 and @Yoni-Starkware)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noaov1, @varex83, and @Yoni-Starkware)
crates/blockifier/src/execution/native/syscall_handler.rs line 226 at r11 (raw file):
Previously, noaov1 (Noa Oved) wrote…
In the CASM syscall implementation, we receive input as a vector of felts. If any of the felts don't fit into a
u64, we return an (unprovable) error. I'm curious about how Cairo native would handle this situation (is it even possible?).
We have no control over it, we would have to ask the Native team for certainty but my guess is that it would throw an overflow error (if it was possible for it to happen at first). Since one of the pros of Sierra is that there is type information present, and if execution was correct until this point (no overflow errors whatsoever) my guess is that those values won't be incorrect.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @varex83 and @Yoni-Starkware)
There was a problem hiding this comment.
Reviewed 1 of 2 files at r13, 2 of 2 files at r14, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware and @Yoni-Starkware)
| let mut state = [0u64; 25]; | ||
| for chunk in input.chunks(KECCAK_FULL_RATE_IN_WORDS) { | ||
| for (i, val) in chunk.iter().enumerate() { | ||
| state[i] ^= val; | ||
| } | ||
| keccak::f1600(&mut state) | ||
| } |
There was a problem hiding this comment.
@ilyalesokhin-starkware
Is this comment correct?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware and @Yoni-Starkware)
There was a problem hiding this comment.
Reviewed 1 of 2 files at r13, 2 of 2 files at r14, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware)
| let mut state = [0u64; 25]; | ||
| for chunk in input.chunks(KECCAK_FULL_RATE_IN_WORDS) { | ||
| for (i, val) in chunk.iter().enumerate() { | ||
| state[i] ^= val; | ||
| } | ||
| keccak::f1600(&mut state) | ||
| } |
There was a problem hiding this comment.
The syscall computes a keccak round; the caller is responsible for the padding.
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @varex83)
| let mut state = [0u64; 25]; | ||
| for chunk in input.chunks(KECCAK_FULL_RATE_IN_WORDS) { | ||
| for (i, val) in chunk.iter().enumerate() { | ||
| state[i] ^= val; | ||
| } | ||
| keccak::f1600(&mut state) | ||
| } |
There was a problem hiding this comment.
Thanks @ilyalesokhin-starkware
|
Previously, Yoni-Starkware (Yoni) wrote…
|
This PR adds support for keccak syscall for the native syscall handler.