fix: Identifier retdata is not relocatable
#393
Conversation
* point to snos_requirements branch * fix inconsistent hash * add blocks to CI
The reference PIE was created with `full_output` enabled, so `output.rs` was updated to support that. The code was refactored to better mirror the Cairo serialization code. Co-authored-by: Herman Obst Demaestri <hodemaestri@gmail.com>
…ion (#391) * debug : prints added for blob * debug : prints added for blob * debug : prints added for blob * debug : prints added for blob * fix: fixed split commitment function * fix: fixed split commitment function * fix: fixed split commitment function * feat : removed logs * fix : lint and clippy * test : added a test for split commitment function * feat : requested changes done * chore : added constants for hardcoded values --------- Co-authored-by: Herman Obst Demaestri <70286869+HermanObst@users.noreply.github.com>
| @@ -28,6 +29,7 @@ where | |||
| PCS: PerContractStorage, | |||
| { | |||
| pub _prev_block_context: Option<BlockContext>, | |||
| pub os_input: Option<Rc<StarknetOsInput>>, | |||
There was a problem hiding this comment.
@notlesh why do you need this to be a Rc?
There was a problem hiding this comment.
That avoids cloning it, which is the part of this PR that improves performance
There was a problem hiding this comment.
To elaborate: the OsInput is now used in both ExecutionHelper and in run_os() so this avoids having to have two copies of it in those cases
| if response.need_retdata_hack { | ||
| log::warn!("Writing Felt::ZERO instead of pointer since retdata size is 0"); | ||
| write_felt(vm, ptr, Felt252::ZERO)?; // casted pointer | ||
| write_felt(vm, ptr, Felt252::ZERO)?; // size | ||
| } else { | ||
| write_segment(vm, ptr, response.constructor_retdata)?; | ||
| } |
There was a problem hiding this comment.
@notlesh could this be handled in cairo-vm?
There was a problem hiding this comment.
Which part? The syscall is specific to the OS.
If you mean the underlying problem (which is that cast(0, felt*) is represented by cairo-vm as an Int(0) then yes, I think that could be an upstream bug, but I'm not sure. It's worth asking about...
| @@ -1119,6 +1121,21 @@ pub fn check_new_deploy_response( | |||
| _constants: &HashMap<String, Felt252>, | |||
| ) -> Result<(), HintError> { | |||
| let response_ptr = get_ptr_from_var_name(vars::ids::RESPONSE, vm, ids_data, ap_tracking)?; | |||
There was a problem hiding this comment.
@notlesh cant we use the "need_retdata_hack" here?
There was a problem hiding this comment.
I don't think so. The need_retdata_hack is part of the syscall struct, which has disappeared by this point. It's possible that we could write something to the vm to use later, but I think that's a bad idea.
Problem:
This issue occurs with the following conditions
deploy_contractis called inside an invoke tx.Solution:
Rc::new(os_input)
Note that this PR also wraps the os_input in a Rc. This was necessary(-ish) so that the deploy syscall could inspect the class info, but it should also have a big improvement to performance. See 3ddd5ef (and 8562944).
Issue Number: #392
Type
Description
Breaking changes?