-
Notifications
You must be signed in to change notification settings - Fork 26
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.
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
feat(blockifier): add native execution engine #429
feat(blockifier): add native execution engine #429
Conversation
# Conflicts: # crates/blockifier/src/execution/syscalls/syscalls_test.rs
…-engine # Conflicts: # Cargo.lock
…cution-engine # Conflicts: # Cargo.lock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 36 files reviewed, 1 unresolved discussion (waiting on @varex83)
a discussion (no related file):
Could you please rebase the PR. Some of the changes are already merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 36 files reviewed, 2 unresolved discussions (waiting on @varex83)
crates/blockifier/src/test_utils/contracts.rs
line 221 at r18 (raw file):
CairoVersion::Cairo0 => ERC20_CAIRO0_CONTRACT_SOURCE_PATH, CairoVersion::Cairo1 => ERC20_CAIRO1_CONTRACT_SOURCE_PATH, }
There is a new version of this function that might resolve this change. Look at this code:
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 19 files at r1, 1 of 11 files at r3, 11 of 17 files at r15, 16 of 20 files at r17.
Reviewable status: 28 of 36 files reviewed, 3 unresolved discussions (waiting on @varex83)
crates/blockifier/src/execution/call_info.rs
line 27 at r18 (raw file):
} #[derive(Clone, Debug, Default, Eq, PartialEq, Serialize)]
Why do you need Clone outside of the test scope?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 20 files at r17, all commit messages.
Reviewable status: 29 of 36 files reviewed, 3 unresolved discussions (waiting on @varex83)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 29 of 36 files reviewed, 4 unresolved discussions (waiting on @varex83)
-- commits
line 21 at r18:
Why are the commit messages of the already merged PR still in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 17 files at r15, 1 of 20 files at r17, 1 of 1 files at r18.
Reviewable status: 30 of 36 files reviewed, 8 unresolved discussions (waiting on @meship-starkware and @varex83)
crates/blockifier/src/execution/contract_class.rs
line 46 at r18 (raw file):
use super::entry_point::EntryPointExecutionResult; use super::errors::EntryPointExecutionError;
Please avoid using super
Code quote:
use super::entry_point::EntryPointExecutionResult;
use super::errors::EntryPointExecutionError;
crates/blockifier/src/execution/contract_class.rs
line 636 at r18 (raw file):
entry_point_type: EntryPointType, entrypoint_selector: EntryPointSelector, ) -> EntryPointExecutionResult<&FunctionId> {
WDYT?
Suggestion:
pub fn get_entrypoint(
&self,
call: &CallEntryPoint,
) -> EntryPointExecutionResult<&FunctionId> {
crates/blockifier/src/execution/contract_class.rs
line 637 at r18 (raw file):
entrypoint_selector: EntryPointSelector, ) -> EntryPointExecutionResult<&FunctionId> { let entrypoints = &self.entry_points_by_type[entry_point_type];
I saw that in the casm we verify that if the entry point type is constructor than the entry point selector must be equal to "constructor". Don't we need it here as well?
crates/blockifier/src/execution/contract_class.rs
line 641 at r18 (raw file):
entrypoints .iter() .find(|entrypoint| entrypoint.selector == entrypoint_selector)
What if there is more than one matching entry point? We want to return an error in this case
Code quote:
.find(|entrypoint| entrypoint.selector == entrypoint_selector)
…cution-engine # Conflicts: # Cargo.lock # crates/blockifier/src/execution/contract_class.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 28 of 36 files reviewed, 8 unresolved discussions (waiting on @meship-starkware and @noaov1)
Previously, meship-starkware (Meshi Peled) wrote…
Why are the commit messages of the already merged PR still in this PR?
If we re-make the PR it will show only new commits, I tried to do something with that, without any results
crates/blockifier/src/execution/call_info.rs
line 27 at r18 (raw file):
Previously, meship-starkware (Meshi Peled) wrote…
Why do you need Clone outside of the test scope?
Because in the execute_inner_call
function, we need to return call_info
and add it to the vector, so Clone is necessary here
Code snippet:
pub fn execute_inner_call(
&mut self,
entry_point: CallEntryPoint,
remaining_gas: &mut u128,
) -> SyscallResult<CallInfo> {
let call_info = entry_point
.execute(self.state, self.execution_resources, self.execution_context)
.map_err(|e| encode_str_as_felts(&e.to_string()))?;
let retdata = call_info.execution.retdata.0.clone();
if call_info.execution.failed {
// In VM it's wrapped into `SyscallExecutionError::SyscallError`.
return Err(retdata);
}
self.update_remaining_gas(remaining_gas, &call_info);
self.inner_calls.push(call_info.clone());
Ok(call_info)
}
crates/blockifier/src/execution/contract_class.rs
line 636 at r18 (raw file):
Previously, noaov1 (Noa Oved) wrote…
WDYT?
Good catch!
crates/blockifier/src/execution/contract_class.rs
line 637 at r18 (raw file):
Previously, noaov1 (Noa Oved) wrote…
I saw that in the casm we verify that if the entry point type is constructor than the entry point selector must be equal to "constructor". Don't we need it here as well?
Refactored
crates/blockifier/src/execution/contract_class.rs
line 641 at r18 (raw file):
Previously, noaov1 (Noa Oved) wrote…
What if there is more than one matching entry point? We want to return an error in this case
Refactored
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 3 files at r21, 5 of 6 files at r22, 1 of 1 files at r23, 2 of 8 files at r24, 1 of 1 files at r25, all commit messages.
Reviewable status: 34 of 42 files reviewed, 6 unresolved discussions (waiting on @noaov1 and @varex83)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 6 of 8 files at r24.
Reviewable status: 40 of 42 files reviewed, 6 unresolved discussions (waiting on @noaov1 and @varex83)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 40 of 42 files reviewed, 7 unresolved discussions (waiting on @noaov1 and @varex83)
crates/blockifier/src/execution/contract_class.rs
line 681 at r25 (raw file):
// function name is what is used by Cairo Native to lookup the function. // Therefore it's not enough to know the function index and we need enrich the contract // entry point with FunctionIds from SierraProgram.
// Therefore, it's not enough to know the function index, and we need to enrich the contract
// entry point with FunctionIds from SierraProgram.
Code quote:
// Therefore it's not enough to know the function index and we need enrich the contract
// entry point with FunctionIds from SierraProgram.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 40 of 42 files reviewed, 10 unresolved discussions (waiting on @noaov1 and @varex83)
crates/blockifier/src/execution/contract_class.rs
line 676 at r25 (raw file):
let sierra_program = sierra_contract_class.extract_sierra_program().expect("can't extract sierra program"); // Note [Cairo Native ABI]
What do the squared brackets mean? Is it the name of the note or files it is related to?
I am not familiar with this notion.
crates/blockifier/src/execution/contract_class.rs
line 684 at r25 (raw file):
let lookup_fid: HashMap<usize, &FunctionId> = HashMap::from_iter(sierra_program.funcs.iter().map(|fid| { // This exception should never occur as the id is also in [SierraContractClass]
I am not sure I understand this comment, we expect here only if the id cannot be turned into a usize.
Do you mean that we have already defined it as a usize in the SierraContractClass?
crates/blockifier/src/execution/contract_class.rs
line 762 at r25 (raw file):
#[derive(Debug, PartialEq)] /// Provides a relation between a function in a contract and a compiled contract.
Is it supposed to provide a relation between any contract and any compiled contract, or is it supposed to provide a relation between a contract and its compiled contract? If it is the latter, please consider the following change
/// Provides a relation between a function in a contract and its compiled contract.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 3 files at r21.
Reviewable status: 41 of 42 files reviewed, 10 unresolved discussions (waiting on @noaov1 and @varex83)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please update according to the new content?
Reviewed 1 of 3 files at r21, 1 of 1 files at r25.
Reviewable status: 41 of 42 files reviewed, 12 unresolved discussions (waiting on @meship-starkware and @varex83)
crates/blockifier/src/execution/call_info.rs
line 27 at r18 (raw file):
Previously, varex83 (Bohdan Ohorodnii) wrote…
Because in the
execute_inner_call
function, we need to returncall_info
and add it to the vector, so Clone is necessary here
Why do you need to return the callinfo?
crates/blockifier/src/execution/contract_class.rs
line 599 at r25 (raw file):
#[error("FunctionId {0} not found")] FunctionIdNotFound(usize), }
Can you please move it to the file: crates/blockifier/src/execution/errors.rs
?
Code quote:
#[derive(Debug, thiserror::Error)]
pub enum NativeEntryPointError {
#[error("FunctionId {0} not found")]
FunctionIdNotFound(usize),
}
crates/blockifier/src/execution/contract_class.rs
line 603 at r25 (raw file):
impl NativeContractClassV1 { fn constructor_selector(&self) -> Option<EntryPointSelector> { self.entry_points_by_type.constructor.first().map(|ep| ep.selector)
Suggestion:
self.entry_points_by_type.constructor.first()?.selector
crates/blockifier/src/execution/contract_class.rs
line 628 at r25 (raw file):
abi: None, sierra_program_debug_info: None, contract_class_version: String::default(),
Why?
Code quote:
contract_class_version: String::default(),
crates/blockifier/src/execution/contract_class.rs
line 631 at r25 (raw file):
}; CasmContractClass::from_contract_class(sierra_contract_class, false, usize::MAX)
Can you please use the compile
method of the CairoLangSierraToCasmCompiler
?
Code quote:
CasmContractClass::from_contract_class(sierra_contract_class, false, usize::MAX)
crates/blockifier/src/execution/contract_class.rs
line 635 at r25 (raw file):
/// Returns an entry point into the natively compiled contract. pub fn get_entry_point(&self, call: &CallEntryPoint) -> Result<&FunctionId, PreExecutionError> {
Consider sharing code with get_entry_point
of ContractClassV1
struct.
Code quote:
pub fn get_entry_point(&self, call: &CallEntryPoint) -> Result<&FunctionId, PreExecutionError> {
crates/blockifier/src/execution/contract_class.rs
line 664 at r25 (raw file):
entry_points_by_type: NativeContractEntryPoints, // Storing the raw sierra program and entry points to be able to fallback to the vm sierra_program_raw: Vec<BigUintAsHex>,
WDYT?
Suggestion:
sierra_program: Vec<BigUintAsHex>,
crates/blockifier/src/execution/contract_class.rs
line 749 at r25 (raw file):
} impl Index<EntryPointType> for NativeContractEntryPoints {
Nice!
Code quote:
impl Index<EntryPointType> for NativeContractEntryPoints {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 41 of 42 files reviewed, 21 unresolved discussions (waiting on @meship-starkware and @varex83)
crates/blockifier/src/execution/contract_class.rs
line 651 at r18 (raw file):
#[derive(Debug)] pub struct NativeContractClassV1Inner { pub executor: AotNativeExecutor,
Why not storing the output of the native compiler and create the executor before running the entry point? 🤔
Suggestion:
pub native: NativeModule,
crates/blockifier/src/execution/contract_class.rs
line 676 at r18 (raw file):
// This exception should never occur as the id is also in [SierraContractClass] let id: usize = fid.id.id.try_into().expect("function id exceeds usize"); (id, &fid.id)
What is the difference between the two fields?
Code quote:
(id, &fid.id)
crates/blockifier/src/execution/contract_class.rs
line 684 at r18 (raw file):
&lookup_fid, &sierra_contract_class.entry_points_by_type, )?,
Can it be extracted from the compilation output?
Code quote:
entry_points_by_type: NativeContractEntryPoints::try_from(
&lookup_fid,
&sierra_contract_class.entry_points_by_type,
)?,
crates/blockifier/src/execution/contract_class.rs
line 716 at r18 (raw file):
/// On failure returns the first FunctionId that it couldn't find. fn try_from( lookup: &HashMap<usize, &FunctionId>,
Consider defining a type.
Code quote:
lookup: &HashMap<usize, &FunctionId>,
crates/blockifier/src/execution/contract_class.rs
line 717 at r18 (raw file):
fn try_from( lookup: &HashMap<usize, &FunctionId>, sep: &SierraContractEntryPoints,
Suggestion:
sierra_ep: &SierraContractEntryPoints,
crates/blockifier/src/execution/contract_class.rs
line 763 at r18 (raw file):
fn try_from( lookup: &HashMap<usize, &FunctionId>, cep: &ContractEntryPoint,
Suggestion:
contract_ep: &ContractEntryPoint,
crates/blockifier/src/execution/contract_class.rs
line 631 at r25 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Can you please use the
compile
method of theCairoLangSierraToCasmCompiler
?
I see that the compile
method adds pythonic hints which are not needed in this case. Consider adding it as a parameter.
crates/blockifier/src/execution/contract_class.rs
line 663 at r25 (raw file):
pub executor: AotNativeExecutor, entry_points_by_type: NativeContractEntryPoints, // Storing the raw sierra program and entry points to be able to fallback to the vm
Why?
Can we extract it from the state?
Code quote:
// Storing the raw sierra program and entry points to be able to fallback to the vm
crates/blockifier/src/execution/native/utils.rs
line 38 at r18 (raw file):
) -> EntryPointSelector { let selector_felt = Felt::from_bytes_be_slice(&entrypoint.selector.to_be_bytes()); EntryPointSelector(selector_felt)
Consider using the From<&BigUint> for Felt
method.
I see that the method uses little endian. Does it matter or it just need to be consistent?
Code quote:
let selector_felt = Felt::from_bytes_be_slice(&entrypoint.selector.to_be_bytes());
EntryPointSelector(selector_felt)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 20 files at r17.
Reviewable status: 41 of 42 files reviewed, 21 unresolved discussions (waiting on @varex83)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 20 files at r17.
Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @varex83)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 20 files at r17.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @varex83)
Previously, noaov1 (Noa Oved) wrote…
Because |
Previously, meship-starkware (Meshi Peled) wrote…
It indicates that the comment is referenced from multiple places. In this case the comment is also relevant for |
Previously, noaov1 (Noa Oved) wrote…
This information is available during the compilation phase so it could be stored alongside with the rest of the metadata we would like to store, but as of now it's not part of the compilation output. |
Previously, noaov1 (Noa Oved) wrote…
A To execute a function in a contract we need to go from a selector to a function name in the compiled contract / shared object. We get there by |
Previously, meship-starkware (Meshi Peled) wrote…
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 39 of 42 files reviewed, 15 unresolved discussions (waiting on @meship-starkware and @noaov1)
crates/blockifier/src/execution/call_info.rs
line 27 at r18 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Why do you need to return the callinfo?
We will reuse it in the future in the syscall's implementations. Is adding "Clone" causing some problems?
crates/blockifier/src/execution/contract_class.rs
line 717 at r18 (raw file):
fn try_from( lookup: &HashMap<usize, &FunctionId>, sep: &SierraContractEntryPoints,
Done.
crates/blockifier/src/execution/contract_class.rs
line 599 at r25 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Can you please move it to the file:
crates/blockifier/src/execution/errors.rs
?
Done.
crates/blockifier/src/execution/contract_class.rs
line 603 at r25 (raw file):
impl NativeContractClassV1 { fn constructor_selector(&self) -> Option<EntryPointSelector> { self.entry_points_by_type.constructor.first().map(|ep| ep.selector)
In this case, we need to wrap it into Some(..)
, so I would like to leave it as it is
crates/blockifier/src/execution/contract_class.rs
line 664 at r25 (raw file):
Previously, noaov1 (Noa Oved) wrote…
WDYT?
Done.
crates/blockifier/src/execution/native/utils.rs
line 38 at r18 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Consider using the
From<&BigUint> for Felt
method.
I see that the method uses little endian. Does it matter or it just need to be consistent?
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 39 of 42 files reviewed, 12 unresolved discussions (waiting on @meship-starkware, @noaov1, and @varex83)
crates/blockifier/src/execution/call_info.rs
line 27 at r18 (raw file):
Why do you need to return the callinfo?
We use the retdata from the call info. We can modify it to just return that. Personally, it makes sense to return the call info, but we can specialize it.
crates/blockifier/src/execution/contract_class.rs
line 628 at r25 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Why?
This is only used for the Fallback, I'll publish a commit removing this
crates/blockifier/src/execution/contract_class.rs
line 631 at r25 (raw file):
Previously, noaov1 (Noa Oved) wrote…
I see that the
compile
method adds pythonic hints which are not needed in this case. Consider adding it as a parameter.
Will be deleted as well.
crates/blockifier/src/execution/contract_class.rs
line 663 at r25 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Why?
Can we extract it from the state?
Remember from our past conversation, specifically asking for CASM would require a change in the State API. Nonetheless, we will be remove this accordingly since it is specifically for the Fallback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 39 of 42 files reviewed, 10 unresolved discussions (waiting on @meship-starkware and @noaov1)
crates/blockifier/src/execution/contract_class.rs
line 635 at r25 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Consider sharing code with
get_entry_point
ofContractClassV1
struct.
I see your point, but these methods work with different types, so that will be challenging to make it more generic (e.g. implementing shared traits, etc. )
Solved in #620 |
This PR bring the boilerplate for the cairo native. It contains some functions that will be used in the future, and put here to parallelize creating of the following PRs.
To test this changes locally you need to set up native dependencies, which are described in cairo native repository:
brew install llvm-18
If installed with brew
If installed using Debian / Ubuntu repository
git clone https://github.com/lambdaclass/cairo_native
cd cairo_native/runtime
cargo build --release
cd ..
andexport CAIRO_NATIVE_RUNTIME_LIBRARY=$(pwd)/target/release/libcairo_native_runtime.a
This change is