-
Notifications
You must be signed in to change notification settings - Fork 164
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
implement Blake2s opcode in runner #1927
base: main
Are you sure you want to change the base?
Conversation
b3b5246
to
4f6a596
Compare
|
4f6a596
to
13d5632
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1927 +/- ##
==========================================
+ Coverage 96.37% 96.39% +0.02%
==========================================
Files 102 102
Lines 41245 41483 +238
==========================================
+ Hits 39749 39987 +238
Misses 1496 1496 ☔ View full report in Codecov by Sentry. |
13d5632
to
6738a48
Compare
Benchmark Results for unmodified programs 🚀
|
5d273fc
to
912b050
Compare
|
||
// Tests the Blake2s opcode runner using a preexisting implementation within the repo as reference. | ||
// The initial state, a random message of 68 bytes and counter are used as input. | ||
// Both the opcode and the reference implementation are run on said inputs and outputs are compared. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this sentence
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you mean said
-> same
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
// An instruction encoding is built from offsets -5, -4, -3 and flags which are all 0 except for | ||
// those denoting uses of fp as the base for operand addresses and flag_opcode_blake (16th flag). | ||
// The instruction is then written to [pc] and the runner is forced to execute Blake2s. | ||
func force_blake2s_non_last_block_opcode( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just opcode_blake2s
or execute_blake2s
or blake2s
or run_blake2s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
d25cef1
to
1c031e4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 11 files reviewed, 9 unresolved discussions (waiting on @DavidLevitGurevich, @fmoletta, @gabrielbosio, @igaray, @juanbono, @ohad-nir-starkware, @Oppen, @pefontana, and @YairVaknin-starkware)
vm/src/vm/vm_core.rs
line 1007 at r2 (raw file):
for (i, x) in res_cow.iter().enumerate() { let le_digits = x.clone().into_owned().to_le_digits(); if le_digits[0] >= (1 << 32)
This conversion to u32
can be moved to a function and also called from handle_blake_2s
cairo_programs/stwo_exclusive_programs/blake2s_opcode_test.cairo
line 46 at r2 (raw file):
assert blake2s_ptr[7] = 0x5BE0CD19; static_assert STATE_SIZE_FELTS == 8; let blake2s_ptr = blake2s_ptr + STATE_SIZE_FELTS;
Another variable with the same name on purpose?
cairo_programs/stwo_exclusive_programs/blake2s_opcode_test.cairo
line 50 at r2 (raw file):
let (cairo_output) = blake2s_inner{range_check_ptr=range_check_ptr, blake2s_ptr=blake2s_ptr}(data=random_message, n_bytes=INPUT_BLOCK_BYTES+4, counter=COUNTER); let relevant_output_start = blake2s_ptr_start+INPUT_BLOCK_FELTS+2+STATE_SIZE_FELTS;
2?
cairo_programs/stwo_exclusive_programs/blake2s_opcode_test.cairo
line 128 at r2 (raw file):
let vm_output_start = cast([ap], felt*); return vm_output_start; }
add new line
vm/src/vm/decoding/decoder.rs
line 8 at r2 (raw file):
}; // opcode|ap_update|pc_update|res_logic|op1_src|op0_reg|dst_reg
opcode_extension | opcode |
16 15
vm/src/vm/decoding/decoder.rs
line 101 at r2 (raw file):
let opcode_extension = match opcode_extension_num { 0 => OpcodeExtension::Stone, _ => {
1 ?
vm/src/vm/decoding/decoder.rs
line 102 at r2 (raw file):
0 => OpcodeExtension::Stone, _ => { if opcode != Opcode::NOp {
another assertion:
-> op1 src can be only 2/4
res -> op1
pc_update-> regular
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 3 files at r2.
Reviewable status: 1 of 11 files reviewed, 14 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @ohad-nir-starkware, @Oppen, @pefontana, @Stavbe, and @YairVaknin-starkware)
cairo_programs/stwo_exclusive_programs/blake2s_opcode_test.cairo
line 46 at r2 (raw file):
Previously, Stavbe wrote…
Another variable with the same name on purpose?
this is a standard pattern
cairo_programs/stwo_exclusive_programs/blake2s_opcode_test.cairo
line 9 at r2 (raw file):
// Tests the Blake2s opcode runner using a preexisting implementation within the repo as reference. // The initial state, a random message of 68 bytes and counter are used as input.
why are you using 68 bytes (more than a single block) if you are testing only one block?
cairo_programs/stwo_exclusive_programs/blake2s_opcode_test.cairo
line 48 at r2 (raw file):
let blake2s_ptr = blake2s_ptr + STATE_SIZE_FELTS; let (cairo_output) = blake2s_inner{range_check_ptr=range_check_ptr, blake2s_ptr=blake2s_ptr}(data=random_message, n_bytes=INPUT_BLOCK_BYTES+4, counter=COUNTER);
why do you compare to blake2s_inner
and not to blake2s_compress
?
cairo_programs/stwo_exclusive_programs/blake2s_opcode_test.cairo
line 58 at r2 (raw file):
); tempvar check_nonempty = vm_output_start[0];
does it really check that it's nonempty? if you don't initialize it on porpuse does it shout?
cairo_programs/stwo_exclusive_programs/blake2s_opcode_test.cairo
line 67 at r2 (raw file):
tempvar check_nonempty = vm_output_start[7]; assert vm_output_start[0] = relevant_output_start[0];
you can avoid this by passing the vm_output_start
as input to run_blake2s
cairo_programs/stwo_exclusive_programs/blake2s_opcode_test.cairo
line 88 at r2 (raw file):
// those denoting uses of fp as the base for operand addresses and flag_opcode_blake (16th flag). // The instruction is then written to [pc] and the runner is forced to execute Blake2s. func run_blake2s(
you can have the is_last_block
flag as input, place 2 dw
s and jump to the correct one
1c031e4
to
de91ec1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 11 files reviewed, 13 unresolved discussions (waiting on @DavidLevitGurevich, @fmoletta, @gabrielbosio, @igaray, @juanbono, @Oppen, @pefontana, @Stavbe, and @YairVaknin-starkware)
cairo_programs/stwo_exclusive_programs/blake2s_opcode_test.cairo
line 50 at r2 (raw file):
Previously, Stavbe wrote…
2?
blake2s_inner writes t0 and f0 into the blake_ptr segment
cairo_programs/stwo_exclusive_programs/blake2s_opcode_test.cairo
line 58 at r2 (raw file):
Previously, DavidLevitGurevich wrote…
does it really check that it's nonempty? if you don't initialize it on porpuse does it shout?
yes.
cairo_programs/stwo_exclusive_programs/blake2s_opcode_test.cairo
line 67 at r2 (raw file):
Previously, DavidLevitGurevich wrote…
you can avoid this by passing the
vm_output_start
as input to run_blake2s
avoid what?
cairo_programs/stwo_exclusive_programs/blake2s_opcode_test.cairo
line 88 at r2 (raw file):
Previously, DavidLevitGurevich wrote…
you can have the
is_last_block
flag as input, place 2dw
s and jump to the correct one
thanks, maybe that's a good idea in the blake_last_block PR (#1932)
does that mean you want both tests to be done in one cairo file?
cairo_programs/stwo_exclusive_programs/blake2s_opcode_test.cairo
line 128 at r2 (raw file):
Previously, Stavbe wrote…
add new line
Done.
vm/src/vm/decoding/decoder.rs
line 8 at r2 (raw file):
Previously, Stavbe wrote…
opcode_extension | opcode |
16 15
Done.
vm/src/vm/decoding/decoder.rs
line 101 at r2 (raw file):
Previously, Stavbe wrote…
1 ?
Done.
vm/src/vm/decoding/decoder.rs
line 102 at r2 (raw file):
Previously, Stavbe wrote…
another assertion:
-> op1 src can be only 2/4
res -> op1
pc_update-> regular
Done.
de91ec1
to
66999d2
Compare
b304359
to
e3049ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 4 files at r9, 9 of 9 files at r10, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @Oppen, @pefontana, @Stavbe, and @YairVaknin-starkware)
vm/src/vm/decoding/decoder.rs
line 426 at r10 (raw file):
fn decode_opcode_extension_clash() { // opcode_extension| opcode|ap_update|pc_update|res_logic|op1_src|op0_reg|dst_reg // 31 ... 17 16 15| 14 13 12| 11 10| 9 8 7| 6 5|4 3 2| 1| 0
Why did you start from 31?
1bec5ba
to
afcbcea
Compare
e3049ac
to
989f630
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @DavidLevitGurevich, @fmoletta, @gabrielbosio, @igaray, @juanbono, @Oppen, @pefontana, and @YairVaknin-starkware)
vm/src/vm/decoding/decoder.rs
line 426 at r10 (raw file):
Previously, DavidLevitGurevich wrote…
Why did you start from 31?
Done.
989f630
to
a645645
Compare
a77d76e
to
6f060e8
Compare
a645645
to
4a7be69
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ohad-nir-starkware! I left you some test related comments.
4a7be69
to
62c28ae
Compare
6f060e8
to
ee6ba48
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 4 files at r11, 3 of 4 files at r12, 3 of 10 files at r13, 8 of 8 files at r14, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @ohad-nir-starkware, @Oppen, @pefontana, and @YairVaknin-starkware)
vm/src/vm/decoding/decoder.rs
line 426 at r10 (raw file):
Previously, ohad-nir-starkware (Ohad Nir) wrote…
Done.
this is better. Still not sure why it had to be changed at all, in particular in this PR, but non-blocking
62c28ae
to
4f2ec51
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 6 of 13 files reviewed, 4 unresolved discussions (waiting on @DavidLevitGurevich, @fmoletta, @gabrielbosio, @igaray, @juanbono, @ohad-nir-starkware, @Oppen, @pefontana, and @YairVaknin-starkware)
cairo_programs/stwo_exclusive_programs/blake2s_opcode_test.cairo
line 8 at r15 (raw file):
from starkware.cairo.common.cairo_builtins import BitwiseBuiltin const COUNTER = 128;
I think it is better to use the correct value (64)
4f2ec51
to
261b2f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 6 of 13 files reviewed, 4 unresolved discussions (waiting on @DavidLevitGurevich, @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, @Stavbe, and @YairVaknin-starkware)
cairo_programs/stwo_exclusive_programs/blake2s_opcode_test.cairo
line 8 at r15 (raw file):
Previously, Stavbe wrote…
I think it is better to use the correct value (64)
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 4 files at r16.
Reviewable status: 5 of 13 files reviewed, 4 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, @Stavbe, and @YairVaknin-starkware)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 5 of 13 files reviewed, 4 unresolved discussions (waiting on @DavidLevitGurevich, @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, @Stavbe, and @YairVaknin-starkware)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 5 of 13 files reviewed, 3 unresolved discussions (waiting on @DavidLevitGurevich, @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, and @YairVaknin-starkware)
261b2f7
to
c8f6543
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 7 files at r15, 1 of 4 files at r16, 1 of 8 files at r17.
Reviewable status: 6 of 13 files reviewed, 3 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, and @YairVaknin-starkware)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 8 files at r17.
Reviewable status: 7 of 13 files reviewed, 3 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, and @YairVaknin-starkware)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 8 files at r17, all commit messages.
Reviewable status: 8 of 13 files reviewed, 3 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, and @YairVaknin-starkware)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 8 files at r17.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @JulianGCalderon, @Oppen, @pefontana, and @YairVaknin-starkware)
c8f6543
to
4c2fdad
Compare
Blake2s opcode runner
Description
Adding the opcode Blake2s to the VM.
Expects op0 to be a pointer to a sequence of 8 felts and and op1 to be a pointer to a sequence of 16 felts.
Said felts should represent u32 integers, i.e. have value of at most 2**32-1.
The 8 felts of op0 represent a state and the 16 felts of op1 represent a message.
dst should hold the value of the counter.
The "output" consists of 8 felts representing u32 numbers of the output of the (non last block) Blake2s compression.
ap should store a pointer, it points to a sequence of 8 cells which each should either be uninitialised or already contain a value matching that of the output at the same index.
The opcode inserts the aforementioned output into the 8 cells [[ap]], [[ap]+1], ... [[ap]+7] (and yields an error if one of said cells already contains a value differing from the output).
Currently Blake2s has opcode_num 8, meaning encoded_instr can use all 64 bits and InstructionNonZeroHighBits is no longer needed as an error.
The motivation is that it has been decided at Starkware that Blake2s is to be implemented at an opcode, thus it needs to be supported by the runner.
Checklist
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)