Skip to content

Commit

Permalink
Undo retdata hacks
Browse files Browse the repository at this point in the history
  • Loading branch information
@notlesh
notlesh committed Nov 4, 2024
1 parent 21eaa9a commit 8b91596
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 84 deletions.
51 changes: 2 additions & 49 deletions crates/starknet-os/src/execution/syscall_handler.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,6 @@ pub struct DeployHandler;
pub struct DeployResponse {
pub contract_address: Felt252,
pub constructor_retdata: ReadOnlySegment,
pub need_retdata_hack: bool,
}

impl<PCS> SyscallHandler<PCS> for DeployHandler
Expand Down Expand Up @@ -292,59 +291,13 @@ where
"No more deployed contracts available to replay".to_string().into_boxed_str(),
))?;

let need_retdata_hack = if let Some(os_input) = execution_helper.os_input.as_ref() {
let class_hash = os_input
.contract_address_to_class_hash
.get(&contract_address)
.expect("No class_hash for contract_address");
let num_constructors =
if let Some(compiled_class_hash) = os_input.class_hash_to_compiled_class_hash.get(class_hash) {
let casm = os_input.compiled_classes.get(compiled_class_hash).expect("No CASM");
let num_constructors = casm
.get_cairo_lang_contract_class()
.expect("couldn't get cairo lang class")
.entry_points_by_type
.constructor
.len();
num_constructors
} else {
let deprecated_cc = os_input.deprecated_compiled_classes.get(class_hash).expect("no deprecated CC");
let num_constructors = deprecated_cc
.get_starknet_api_contract_class()
.expect("couldn't get starknet api class")
.entry_points_by_type
.get(&starknet_api::deprecated_contract_class::EntryPointType::Constructor)
.expect("should have constructor list")
.len();
num_constructors
};

// we need the hack if there are no constructor entry points
num_constructors == 0
} else {
false
};

Ok(DeployResponse { contract_address, constructor_retdata, need_retdata_hack })
Ok(DeployResponse { contract_address, constructor_retdata })
}

fn write_response(response: Self::Response, vm: &mut VirtualMachine, ptr: &mut Relocatable) -> WriteResponseResult {
write_felt(vm, ptr, response.contract_address)?;
write_segment(vm, ptr, response.constructor_retdata)?;

// Bugfix:
// When retdata size is 0, the OS will return retdata as a Int(0) instead of a relocatable
// as a result of `cast(0, felt*)`. To work around this, we can write the same here so that
// later the OS will assert this value is the same as retdata; that is, both need to be the
// same value which is Int(0).
//
// retdata is cast here: https://github.com/starkware-libs/cairo-lang/blob/a86e92bfde9c171c0856d7b46580c66e004922f3/src/starkware/starknet/core/os/execution/execute_entry_point.cairo#L170
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)?;
}
Ok(())
}
}
Expand Down
43 changes: 8 additions & 35 deletions crates/starknet-os/src/hints/execution.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1122,23 +1122,10 @@ pub fn check_new_deploy_response(
) -> Result<(), HintError> {
let response_ptr = get_ptr_from_var_name(vars::ids::RESPONSE, vm, ids_data, ap_tracking)?;

// Bugfix:
// If constructor_retdata_start is a Int(0) instead of a Relocatable, we need to do nothing. This
// can happen sometimes when a constructor is called but not defined in the class'es entry
// points. Immediately following this, `add_relocation_rule()` will be called with src=0, dest=0
// and it will also no-op.
//
// If "retdata" is an Int instead of a Relocatable, then the vm will error when we try to
// request it as such. In this case, there is no retdata anyway, so there is no need to assert
// that the contents are equal.
let retdata_start_key: Relocatable =
(response_ptr + new_syscalls::DeployResponse::constructor_retdata_start_offset())?;
let maybe_retdata_start = vm
.get_maybe(&retdata_start_key)
.ok_or(HintError::VariableNotInScopeError("retdata".to_string().into_boxed_str()))?;
let zero = MaybeRelocatable::Int(Felt252::ZERO);
if maybe_retdata_start == zero {
log::warn!("retdata_start is 0, not a relocatable, ignoring add_relocation_rule()");
let retdata_size =
felt_to_usize(get_integer_from_var_name(vars::ids::RETDATA_SIZE, vm, ids_data, ap_tracking)?.as_ref())?;
if retdata_size == 0 {
log::warn!("Ignoring empty retdata");
return Ok(());
}

Expand All @@ -1149,8 +1136,6 @@ pub fn check_new_deploy_response(
let response_retdata_size = (constructor_retdata_end - constructor_retdata_start)?;

let retdata = get_ptr_from_var_name(vars::ids::RETDATA, vm, ids_data, ap_tracking)?;
let retdata_size =
felt_to_usize(get_integer_from_var_name(vars::ids::RETDATA_SIZE, vm, ids_data, ap_tracking)?.as_ref())?;

assert_memory_ranges_equal(vm, constructor_retdata_start, response_retdata_size, retdata, retdata_size)?;

Expand Down Expand Up @@ -1228,8 +1213,8 @@ pub fn check_response_return_value(
ap_tracking: &ApTracking,
_constants: &HashMap<String, Felt252>,
) -> Result<(), HintError> {
let retdata = get_ptr_from_var_name(vars::ids::RETDATA, vm, ids_data, ap_tracking)?;
let retdata_size = get_integer_from_var_name(vars::ids::RETDATA_SIZE, vm, ids_data, ap_tracking)?;
let retdata = get_ptr_from_var_name(vars::ids::RETDATA, vm, ids_data, ap_tracking)?;

let response = get_ptr_from_var_name(vars::ids::RESPONSE, vm, ids_data, ap_tracking)?;
let response_retdata_start =
Expand Down Expand Up @@ -1289,18 +1274,6 @@ pub fn add_relocation_rule(
ap_tracking: &ApTracking,
_constants: &HashMap<String, Felt252>,
) -> Result<(), HintError> {
// Bugfix:
// add_relocation_rule() can be called sometimes with Int(0) instead of Relocatables. This is
// a result of `cast(0, felt*)` being treated as Int(0). We can work around this here by
// ensuring that both src and dest end up being Int(0), and in that case we no-op here.
let src_ptr_maybe = get_maybe_relocatable_from_var_name(vars::ids::SRC_PTR, vm, ids_data, ap_tracking)?;
let dest_ptr_maybe = get_maybe_relocatable_from_var_name(vars::ids::DEST_PTR, vm, ids_data, ap_tracking)?;

let zero = MaybeRelocatable::Int(Felt252::ZERO);
if src_ptr_maybe == zero && dest_ptr_maybe == zero {
log::warn!("add_relocation_rule with Int(0) => Int(0), doing nothing");
return Ok(());
}

let src_ptr = get_ptr_from_var_name(vars::ids::SRC_PTR, vm, ids_data, ap_tracking)?;

Expand All @@ -1317,12 +1290,12 @@ pub fn add_relocation_rule(
let dest_ptr = if let Ok(infos) = get_ptr_from_var_name(vars::ids::INFOS, vm, ids_data, ap_tracking) {
let infos_0_end = vm.get_relocatable((infos + 1)?)?;
log::warn!("rewriting dest_ptr for segment_arena workaround");
(infos_0_end + 1u32)?
(infos_0_end + 1u32)?.into()
} else {
get_ptr_from_var_name(vars::ids::DEST_PTR, vm, ids_data, ap_tracking)?
get_maybe_relocatable_from_var_name(vars::ids::DEST_PTR, vm, ids_data, ap_tracking)?
};

vm.add_relocation_rule(src_ptr, dest_ptr)?;
vm.add_relocation_rule_maybe_relocatable(src_ptr, dest_ptr)?;

Ok(())
}
Expand Down

0 comments on commit 8b91596

Please sign in to comment.