-
Notifications
You must be signed in to change notification settings - Fork 5
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
add block hint #42
add block hint #42
Conversation
d501fab
to
0fc876a
Compare
Co-authored-by: Clément Walter <[email protected]>
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 think that we'd like instead to implement From<SealedBlock>
for a KethBlock
, and such a KethBlock
would fit well with the already existing https://github.com/lambdaclass/cairo-vm/blob/main/vm/src/vm/vm_memory/memory_segments.rs#L132-L146 so that it's easier is we update the models
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.
KethBlock
would be a Vec<MaybeRelocatable>
?
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.
It'd have been better to import the test_block
function from test_os.cairo
but since #43 is supposed to remove this, I won't block the PR for this
-> HintExecutionResult { | ||
// This call will first add a new memory segment to the VM (the base) | ||
// Then we load the block into the VM starting from the base | ||
vm.gen_arg(&Into::<KethPayload>::into(block.clone()).0)?; |
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.
where do you actually assign the base to the block
cairo variable here?
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.
rust question: why do we need to clone?
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.
- For the base, this is done here no? https://github.com/lambdaclass/cairo-vm/blob/7d19956781901fb05c76f7c02fa82bd907f0f9a2/vm/src/vm/vm_memory/memory_segments.rs#L136 we add a new segment which is automatically set as the base for the block, is there anything else to set?
- For the clone, this is a closure question, for more explicitness, the exact error without cloning is
cannot move out of
block, a captured variable in an
Fnclosure move occurs because
blockhas type
SealedBlock, which does not implement the
Copytrait
impl From<Option<u64>> for KethPayload { | ||
fn from(value: Option<u64>) -> Self { | ||
MaybeRelocatable::from(Felt252::from(value.unwrap_or_default())).into() | ||
} | ||
} |
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.
not sure where we have Option<u64>
in blocks, but this would be converted into a model.Option
in cairo, meaning a struct with 2 values {is_some: 0, value: 0}
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.
blob_gas_used for example is an option, at the moment the model.Block is constructed like that so maybe let us keep as is and fine tuned this after modification on Cairo side no?
fn from(value: Bloom) -> Self { | ||
vec![ | ||
MaybeRelocatable::from(Felt252::from(value.len())), | ||
MaybeRelocatable::from(Felt252::from_bytes_be_slice(&value.0 .0)), |
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.
bloom is a 256 long bytes array, so a vec of felt
impl From<U256> for KethPayload { | ||
fn from(value: U256) -> Self { | ||
MaybeRelocatable::from(Felt252::from_bytes_be_slice( | ||
&value.to_be_bytes::<{ U256::BYTES }>(), | ||
)) | ||
.into() | ||
} | ||
} |
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.
the u256
is not trivial as it can be either Uint256*
(so a pointer to a new segment with two values) or directly inlined into the struct, and consequently split in _low
and _high
parts
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.
Here I put the case where this is always inlined into the struct
impl From<Bytes> for KethPayload { | ||
fn from(value: Bytes) -> Self { | ||
vec![ | ||
MaybeRelocatable::from(Felt252::from(value.len())), | ||
MaybeRelocatable::from(Felt252::from_bytes_be_slice(&value.0)), | ||
] | ||
.into() | ||
} | ||
} |
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.
bytes are an array of felt, not a single felt
fn from(value: Vec<u8>) -> Self { | ||
vec![ | ||
MaybeRelocatable::from(Felt252::from(value.len())), | ||
MaybeRelocatable::from(Felt252::from_bytes_be_slice(&value)), |
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.
same, needs to be an array
impl From<B256> for KethPayload { | ||
fn from(value: B256) -> Self { | ||
vec![ | ||
MaybeRelocatable::from(Felt252::from_bytes_be_slice(&value.0[16..])), | ||
MaybeRelocatable::from(Felt252::from_bytes_be_slice(&value.0[0..16])), | ||
] | ||
.into() | ||
} | ||
} |
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.
see comment, it depends on the target model
impl From<Option<B256>> for KethPayload { | ||
fn from(value: Option<B256>) -> Self { | ||
value.unwrap_or_default().into() | ||
} | ||
} |
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.
see above for Option
// The length of the signature payload | ||
MaybeRelocatable::from(Felt252::from(value.payload_len())), | ||
// The signature payload | ||
MaybeRelocatable::from(Felt252::from_bytes_be_slice(&value.to_bytes())), |
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.
see above, is this a vec
?
// Run the cairo program with the hint processor and the block | ||
// | ||
// We unwrap the result to ensure that the program ran successfully | ||
let _ = cairo_run(&program, &config, &mut hint_processor).unwrap(); |
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.
because the program does nothing, there is no chance to have this run failing. (you could remove the hint it'd be the same)
No description provided.