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

refac: Use offset from SDK for executable loaders #6693

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

refac: Use offset from SDK for executable loaders #6693

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
93ad238
refac: Use offset value from SDK
Abeeujah Nov 5, 2024
c42c07b
fix cargo fmt
Abeeujah 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
145 changes: 21 additions & 124 deletions forc-plugins/forc-client/src/op/deploy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,17 +23,14 @@ use fuel_core_client::client::types::{ChainInfo, TransactionStatus};
use fuel_core_client::client::FuelClient;
use fuel_crypto::fuel_types::ChainId;
use fuel_tx::{Salt, Transaction};
use fuel_vm::{consts::WORD_SIZE, fuel_asm::op, prelude::*};
use fuel_vm::prelude::*;
use fuels::{
macros::abigen,
programs::{
contract::{LoadConfiguration, StorageConfiguration},
executable::Executable,
},
types::{
bech32::Bech32ContractId,
transaction_builders::{Blob, BlobId},
},
types::{bech32::Bech32ContractId, transaction_builders::Blob},
};
use fuels_accounts::{provider::Provider, Account, ViewOnlyAccount};
use fuels_core::types::{transaction::TxPolicies, transaction_builders::CreateTransactionBuilder};
Expand Down Expand Up @@ -354,103 +351,6 @@ pub async fn deploy(command: cmd::Deploy) -> Result<Vec<DeployedPackage>> {
Ok(deployed_packages)
}

/// Calculates the loader data offset. Returns a `None` if the original `binary`
/// does not have a data section (no configurables and no args). Otherwise
/// returns the new offset of the data section.
fn loader_data_offset(binary: &[u8], blob_id: &BlobId) -> Result<Option<usize>> {
// The following code is taken from SDK, and once they expose the offsets
// we will no longer need to maintain this duplicate version here.

// The final code is going to have this structure (if the data section is non-empty):
// 1. loader instructions
// 2. blob id
// 3. length_of_data_section
// 4. the data_section (updated with configurables as needed)
const BLOB_ID_SIZE: u16 = 32;
const REG_ADDRESS_OF_DATA_AFTER_CODE: u8 = 0x10;
const REG_START_OF_LOADED_CODE: u8 = 0x11;
const REG_GENERAL_USE: u8 = 0x12;
let get_instructions = |num_of_instructions| {
// There are 3 main steps:
// 1. Load the blob content into memory
// 2. Load the data section right after the blob
// 3. Jump to the beginning of the memory where the blob was loaded
[
// 1. Load the blob content into memory
// Find the start of the hardcoded blob ID, which is located after the loader code ends.
op::move_(REG_ADDRESS_OF_DATA_AFTER_CODE, RegId::PC),
// hold the address of the blob ID.
op::addi(
REG_ADDRESS_OF_DATA_AFTER_CODE,
REG_ADDRESS_OF_DATA_AFTER_CODE,
num_of_instructions * Instruction::SIZE as u16,
),
// The code is going to be loaded from the current value of SP onwards, save
// the location into REG_START_OF_LOADED_CODE so we can jump into it at the end.
op::move_(REG_START_OF_LOADED_CODE, RegId::SP),
// REG_GENERAL_USE to hold the size of the blob.
op::bsiz(REG_GENERAL_USE, REG_ADDRESS_OF_DATA_AFTER_CODE),
// Push the blob contents onto the stack.
op::ldc(REG_ADDRESS_OF_DATA_AFTER_CODE, 0, REG_GENERAL_USE, 1),
// Move on to the data section length
op::addi(
REG_ADDRESS_OF_DATA_AFTER_CODE,
REG_ADDRESS_OF_DATA_AFTER_CODE,
BLOB_ID_SIZE,
),
// load the size of the data section into REG_GENERAL_USE
op::lw(REG_GENERAL_USE, REG_ADDRESS_OF_DATA_AFTER_CODE, 0),
// after we have read the length of the data section, we move the pointer to the actual
// data by skipping WORD_SIZE B.
op::addi(
REG_ADDRESS_OF_DATA_AFTER_CODE,
REG_ADDRESS_OF_DATA_AFTER_CODE,
WORD_SIZE as u16,
),
// load the data section of the executable
op::ldc(REG_ADDRESS_OF_DATA_AFTER_CODE, 0, REG_GENERAL_USE, 2),
// Jump into the memory where the contract is loaded.
// What follows is called _jmp_mem by the sway compiler.
// Subtract the address contained in IS because jmp will add it back.
op::sub(
REG_START_OF_LOADED_CODE,
REG_START_OF_LOADED_CODE,
RegId::IS,
),
// jmp will multiply by 4, so we need to divide to cancel that out.
op::divi(REG_START_OF_LOADED_CODE, REG_START_OF_LOADED_CODE, 4),
// Jump to the start of the contract we loaded.
op::jmp(REG_START_OF_LOADED_CODE),
]
};

let offset = extract_data_offset(binary)?;

if binary.len() < offset {
anyhow::bail!("data sectio offset is out of bounds");
}

let data_section = binary[offset..].to_vec();

if !data_section.is_empty() {
let num_of_instructions = u16::try_from(get_instructions(0).len())
.expect("to never have more than u16::MAX instructions");

let instruction_bytes = get_instructions(num_of_instructions)
.into_iter()
.flat_map(|instruction| instruction.to_bytes());

let blob_bytes = blob_id.iter().copied();

let loader_offset =
instruction_bytes.count() + blob_bytes.count() + data_section.len().to_be_bytes().len();

Ok(Some(loader_offset))
} else {
Ok(None)
}
}

/// Builds and deploys executable (script and predicate) package(s) as blobs,
/// and generates a loader for each of them.
pub async fn deploy_executables(
Expand Down Expand Up @@ -482,28 +382,25 @@ pub async fn deploy_executables(
"Saved",
&format!("loader bytecode at {}", bin_path.display()),
);
if let Some(loader_data_section_offset) =
loader_data_offset(&pkg.bytecode.bytes, &BlobId::default())?
{
if let ProgramABI::Fuel(mut fuel_abi) = pkg.program_abi.clone() {
println_action_green("Generating", "loader abi for the uploaded executable.");
let json_abi_path = out_dir.join(format!("{pkg_name}-loader-abi.json"));
let original_data_section = extract_data_offset(&pkg.bytecode.bytes).unwrap();
let offset_shift = original_data_section - loader_data_section_offset;
// if there are configurables in the abi we need to shift them by `offset_shift`.
let configurables = fuel_abi.configurables.clone().map(|configs| {
configs
.into_iter()
.map(|config| Configurable {
offset: config.offset - offset_shift as u64,
..config.clone()
})
.collect()
});
fuel_abi.configurables = configurables;
let json_string = serde_json::to_string_pretty(&fuel_abi)?;
std::fs::write(json_abi_path, json_string)?;
}
let loader_data_section_offset = loader.data_offset_in_code();
if let ProgramABI::Fuel(mut fuel_abi) = pkg.program_abi.clone() {
println_action_green("Generating", "loader abi for the uploaded executable.");
let json_abi_path = out_dir.join(format!("{pkg_name}-loader-abi.json"));
let original_data_section = extract_data_offset(&pkg.bytecode.bytes).unwrap();
let offset_shift = original_data_section - loader_data_section_offset;
// if there are configurables in the abi we need to shift them by `offset_shift`.
let configurables = fuel_abi.configurables.clone().map(|configs| {
configs
.into_iter()
.map(|config| Configurable {
offset: config.offset - offset_shift as u64,
..config.clone()
})
.collect()
});
fuel_abi.configurables = configurables;
let json_string = serde_json::to_string_pretty(&fuel_abi)?;
std::fs::write(json_abi_path, json_string)?;
Comment on lines +385 to +403
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but I believe we need a test for this. Especially because we are not sure what is going to happen if the binary does not have a data section. In earlier implementation it was obvious that the function to calculate offset returns None in that case but now I am not sure what is going to happen.

So if you are up for it, let's add a test which get's its oracle values (like the calculated shifted values with the previous implementation) and in a next commit let's apply this change and see if the calculated values are same.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @kayagokalp I did look at the SDK before I implemented this change, the function implementation is guaranteed to always return an offset, and cases where it wouldn't the program crashes here's a snippet from Executable<Loader> impl block

pub fn data_offset_in_code(&self) -> usize {
        self.code_with_offset().1
    }

    fn code_with_offset(&self) -> (Vec<u8>, usize) {
        let mut code = self.state.code.clone();

        self.state.configurables.update_constants_in(&mut code);

        let blob_id = self.blob().id();

        transform_into_configurable_loader(code, &blob_id)
            .expect("checked before turning into a Executable<Loader>")
    }

But if you'd want a test after looking at this, what should I be testing for? that the value isn't zero? within a specific range? I'd like to hear your POV

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey again, it is possible that a binary does not contain any data section, that happens if the sway code does not have any constants and no configurable section. So if SDK is expecting this to be checked before calling the function, we might need to check this explicitly.

For testing we can use a example test script or predicate's binary from /test/data. And (possibly) extract the shift calculation to a separate function so that we can write tests against it.

Basically call this new calculate shift function and check if the calculated shift is being calculated correctly for two types of sway scripts:

  1. A script that has a single main function and nothing else.
  2. The script like https://github.com/FuelLabs/sway/blob/master/forc-plugins/forc-client/test/data/deployed_script/src/main.sw which needs this shift.

I can help with testing if you need it feel free to ping me

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'd need some help with testing it, How do I ping you? I tried on twitter, 2 days ago and you haven't responded

Copy link
Member

@kayagokalp kayagokalp Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will be adding the tests to this PR directly to better streamline the process, possibly tomorrow. Other than the tests this looks good to me, after the test I'll approve and hunt for your second approval :) Thanks again for the contribution.

}
// If the executable is a predicate, we also want to display and save the predicate root.
if pkg
Expand Down
Loading