feat: switch blob ID calculation from data offset to configurable offset #1593
Conversation
Salka1988
requested review from
digorithm,
hal3e,
MujkicA and
segfault-magnet
as code owners
# Conflicts: # packages/fuels-programs/src/executable.rs
hal3e
changed the title
…uels-rs into feat/support_for_verified_abi
packages/fuels-programs/src/assembly/script_and_predicate_loader.rs
Outdated
Show resolved
Hide resolved
packages/fuels-programs/src/assembly/script_and_predicate_loader.rs
Outdated
Show resolved
Hide resolved
packages/fuels-programs/src/assembly/script_and_predicate_loader.rs
Outdated
Show resolved
Hide resolved
packages/fuels-programs/src/assembly/script_and_predicate_loader.rs
Outdated
Show resolved
Hide resolved
packages/fuels-programs/src/assembly/script_and_predicate_loader.rs
Outdated
Show resolved
Hide resolved
packages/fuels-programs/src/assembly/script_and_predicate_loader.rs
Outdated
Show resolved
Hide resolved
packages/fuels-programs/src/assembly/script_and_predicate_loader.rs
Outdated
Show resolved
Hide resolved
…er.rs Co-authored-by: Ahmed Sagdati <[email protected]>
Co-authored-by: Ahmed Sagdati <[email protected]>
Co-authored-by: Ahmed Sagdati <[email protected]>
There was a problem hiding this comment.
LGTM. Great job, @Salka1988, and great suggestions, @segfault-magnet!
packages/fuels-programs/src/assembly/script_and_predicate_loader.rs
Outdated
Show resolved
Hide resolved
packages/fuels-programs/src/assembly/script_and_predicate_loader.rs
Outdated
Show resolved
Hide resolved
| pub fn has_configurables_section_offset(binary: &[u8]) -> Result<Option<bool>> { | ||
| if binary.len() < 8 { | ||
| return Err(fuels_core::error!( | ||
| Other, | ||
| "binary too short to check JMPF instruction, need at least 8 bytes but got: {}", | ||
| binary.len() | ||
| )); | ||
| } | ||
|
|
||
| let instruction = Instruction::try_from([binary[4], binary[5], binary[6], binary[7]]) | ||
| .map_err(|e| fuels_core::error!(Other, "Invalid instruction at byte 4: {:?}", e))?; | ||
|
|
||
| if let Instruction::JMPF(offset) = instruction { | ||
| let offset_val = offset.imm18().to_u32(); | ||
| let result = match offset_val { | ||
| 0x04 => Some(true), | ||
| 0x02 => Some(false), | ||
| _ => None, | ||
| }; | ||
| Ok(result) | ||
| } else { | ||
| Ok(None) | ||
| } | ||
| } |
There was a problem hiding this comment.
That Result<Option<bool>> combo feels kinda awkward. It’s not immediately clear what it’s trying to communicate.
I think it’s doing something like this:
Ok(Some(true | false)) → "I know for sure whether the section is there or not."
Ok(None) → "I have no clue if the section is there."
Err → "The binary is too short, or I couldn't read the instruction."
Feels like this should just be Result<bool>, and we should fail if we don’t detect what we expect. Otherwise, if sway changes how they handle jmpf in that spot, we might unknowingly start messing up the configurables.
Also, do we actually have confirmation from the sway team that the 0x04 and 0x02 offsets are stable? If not, we need to decide on a fallback so that if they do change, the SDK doesn’t just break.
If we have to pick a default for the case when we cannot reliably detect whether the configurables offset is present, I'd default to assuming that there is a configurables offset.
There was a problem hiding this comment.
We did this because of handwritten code. Previously it was Result<bool>. I'll revert it.
Co-authored-by: Ahmed Sagdati <[email protected]>
Co-authored-by: Ahmed Sagdati <[email protected]>
Co-authored-by: Ahmed Sagdati <[email protected]>
Release notes
In this release, we:
- Previously deployed blobs are no longer accessible due to changes in blob ID calculation. Redeployment is required.Summary
Updates blob ID calculation to use the configurable offset instead of the data offset, aligning with the updated design for scripts and predicates.
has_configurable_section_offsetfunction checks bytes 4 and 7 to distinguish binary versions. If byte 4 is 0x74 (JMPF), it considers byte 7: 0x02 means legacy, 0x04 means new, and any other value results in an error.Checklist