Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Preview upstream retdata fix #419

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Preview upstream retdata fix #419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
21eaa9a
Bump cairo-vm
notlesh Nov 4, 2024
8b91596
Undo retdata hacks
notlesh Nov 4, 2024
cc6f41f
fmt
notlesh Nov 4, 2024
33e1150
Commit Cargo.lock
notlesh Nov 4, 2024
98bb6a3
Merge branch 'main' into notlesh/upstream-retdata-preview
HermanObst Nov 5, 2024
5ee7ef4
Merge branch 'main' into notlesh/upstream-retdata-preview
HermanObst Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ cairo-lang-starknet-classes = { version = "=2.8.2" }
cairo-lang-utils = { version = "=2.8.2" }
cairo-lang-casm = { version = "=2.8.2" }
cairo-type-derive = { version = "0.1.0", path = "crates/cairo-type-derive" }
cairo-vm = { git = "https://github.com/Moonsong-Labs/cairo-vm", branch = "herman/fix-pie-serialization", features = ["cairo-1-hints", "extensive_hints", "mod_builtin"] }
cairo-vm = { git = "https://github.com/Moonsong-Labs/cairo-vm", branch = "notlesh/snos-2024-11-04", features = ["cairo-1-hints", "extensive_hints", "mod_builtin"] }
clap = { version = "4.5.4", features = ["derive"] }
c-kzg = { version = "1.0.3" }
env_logger = "0.11.3"
Expand Down
51 changes: 2 additions & 49 deletions crates/starknet-os/src/execution/syscall_handler.rs
View file
Open in desktop
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
44 changes: 8 additions & 36 deletions crates/starknet-os/src/hints/execution.rs
View file
Open in desktop
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,19 +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)?;

assert_eq!(src_ptr.offset, 0, "src_ptr offset should be 0");
Expand All @@ -1317,12 +1289,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
Loading