feat(blockifier): add native execution engine #620
Conversation
There was a problem hiding this comment.
Reviewed 19 of 20 files at r1, all commit messages.
Reviewable status: 19 of 20 files reviewed, all discussions resolved
There was a problem hiding this comment.
Reviewable status: 19 of 20 files reviewed, 6 unresolved discussions (waiting on @meship-starkware and @varex83)
crates/blockifier/src/execution/call_info.rs line 27 at r1 (raw file):
} #[derive(Clone, Debug, Default, Eq, PartialEq, Serialize)]
Continuing this conversation:
How is your execute_inner_call implementation different from the existing one? Why is it needed to return the call info now?
Code quote:
#[derive(Clone, Debug, Default, Eq, PartialEq, Serialize)]crates/blockifier/src/execution/contract_class.rs line 613 at r1 (raw file):
} pub fn to_casm_contract_class(
When do you plan to delete it?
Code quote:
pub fn to_casm_contract_class(crates/blockifier/src/execution/contract_class.rs line 622 at r1 (raw file):
abi: None, sierra_program_debug_info: None, contract_class_version: String::default(),
See comment.
Code quote:
contract_class_version: String::default(),crates/blockifier/src/execution/contract_class.rs line 625 at r1 (raw file):
}; CasmContractClass::from_contract_class(sierra_contract_class, false, usize::MAX)
See comment
Code quote:
CasmContractClass::from_contract_class(sierra_contract_class, false, usize::MAX)crates/blockifier/src/execution/contract_class.rs line 629 at r1 (raw file):
/// Returns an entry point into the natively compiled contract. pub fn get_entry_point(&self, call: &CallEntryPoint) -> Result<&FunctionId, PreExecutionError> {
See comment.
crates/blockifier/src/execution/contract_class.rs line 657 at r1 (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
See comment.
Will be removed.
Code quote:
// Storing the raw sierra program and entry points to be able to fallback to the vm
There was a problem hiding this comment.
Reviewable status: 19 of 20 files reviewed, 6 unresolved discussions (waiting on @meship-starkware and @noaov1)
crates/blockifier/src/execution/call_info.rs line 27 at r1 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Continuing this conversation:
How is yourexecute_inner_callimplementation different from the existing one? Why is it needed to return the call info now?
Okay, I think we can just return retdata from the execute_inner_call, so that we can avoid using clone here.
But I'm just really curious why you don't like the idea of using Clone?
crates/blockifier/src/execution/contract_class.rs line 613 at r1 (raw file):
Previously, noaov1 (Noa Oved) wrote…
When do you plan to delete it?
Done.
crates/blockifier/src/execution/contract_class.rs line 622 at r1 (raw file):
Previously, noaov1 (Noa Oved) wrote…
See comment.
Removed the function, should be done.
crates/blockifier/src/execution/contract_class.rs line 625 at r1 (raw file):
Previously, noaov1 (Noa Oved) wrote…
See comment
Removed the function, should be done.
crates/blockifier/src/execution/contract_class.rs line 629 at r1 (raw file):
Previously, noaov1 (Noa Oved) wrote…
See comment.
duplicating: 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. )
There was a problem hiding this comment.
Reviewed 1 of 20 files at r1, 1 of 2 files at r3.
Reviewable status: 19 of 20 files reviewed, 6 unresolved discussions (waiting on @noaov1 and @varex83)
There was a problem hiding this comment.
Reviewed 1 of 2 files at r3.
Reviewable status: all files reviewed (commit messages unreviewed), 6 unresolved discussions (waiting on @noaov1 and @varex83)
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @noaov1 and @varex83)
There was a problem hiding this comment.
Reviewed 12 of 20 files at r1, 1 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @varex83)
crates/blockifier/src/execution/call_info.rs line 27 at r1 (raw file):
Previously, varex83 (Bohdan Ohorodnii) wrote…
Okay, I think we can just return
retdatafrom theexecute_inner_call, so that we can avoid using clone here.But I'm just really curious why you don't like the idea of using
Clone?
We prefer to avoid clone because cloning can have performance implications, especially if the data being cloned is large or complex, as Clone creates a deep copy of the object (and the call info can be large).
|
Benchmark movements: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #620 +/- ##
==========================================
- Coverage 74.18% 70.44% -3.75%
==========================================
Files 359 87 -272
Lines 36240 11512 -24728
Branches 36240 11512 -24728
==========================================
- Hits 26886 8110 -18776
+ Misses 7220 3033 -4187
+ Partials 2134 369 -1765
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Reviewable status: 0 of 38 files reviewed, 3 unresolved discussions (waiting on @meship-starkware and @noaov1)
crates/blockifier/src/execution/call_info.rs line 27 at r1 (raw file):
Previously, noaov1 (Noa Oved) wrote…
We prefer to avoid clone because cloning can have performance implications, especially if the data being cloned is large or complex, as
Clonecreates a deep copy of the object (and the call info can be large).
Done.
There was a problem hiding this comment.
Could you please fetch and rebase, we see all the CI changes in this PR.
Reviewable status: 0 of 38 files reviewed, 3 unresolved discussions (waiting on @noaov1 and @varex83)
There was a problem hiding this comment.
Done.
Reviewable status: 0 of 38 files reviewed, 3 unresolved discussions (waiting on @meship-starkware and @noaov1)
|
Benchmark movements: |
There was a problem hiding this comment.
Reviewed 1 of 37 files at r4, 9 of 15 files at r10, 1 of 1 files at r12, 22 of 23 files at r13, 1 of 1 files at r14, 1 of 3 files at r15, 2 of 2 files at r16, all commit messages.
Reviewable status: 37 of 38 files reviewed, 3 unresolved discussions (waiting on @noaov1 and @varex83)
There was a problem hiding this comment.
Please add the native class here as well.
Reviewed 10 of 15 files at r10, 1 of 1 files at r11, 1 of 1 files at r12, 1 of 23 files at r13, 1 of 1 files at r14, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @varex83)
crates/blockifier/src/execution/contract_class.rs line 11 at r13 (raw file):
use cairo_lang_starknet_classes::contract_class::{ ContractClass as SierraContractClass, ContractEntryPoint,
Suggestion:
ContractEntryPoint as SierraContractEntryPoint,crates/blockifier/src/execution/contract_class.rs line 606 at r13 (raw file):
&& call.entry_point_selector != selector_from_name(CONSTRUCTOR_ENTRY_POINT_NAME) { return Err(PreExecutionError::InvalidConstructorEntryPointName);
Can you please extract this into a method and call it from get_entry_point of ContractClassV1 as well?
Code quote:
if call.entry_point_type == EntryPointType::Constructor
&& call.entry_point_selector != selector_from_name(CONSTRUCTOR_ENTRY_POINT_NAME)
{
return Err(PreExecutionError::InvalidConstructorEntryPointName);crates/blockifier/src/execution/contract_class.rs line 632 at r13 (raw file):
// Storing the raw sierra program and entry points to be able to fallback to the vm sierra_program: Vec<BigUintAsHex>, }
Why not delete it in this PR?
Code quote:
// Storing the raw sierra program and entry points to be able to fallback to the vm
sierra_program: Vec<BigUintAsHex>,
}crates/blockifier/src/execution/contract_class.rs line 649 at r13 (raw file):
// entry point with FunctionIds from SierraProgram. let lookup_fid: LookupTable<'_> = HashMap::from_iter(sierra_program.funcs.iter().map(|fid| {
Suggestion:
HashMap::from_iter(sierra_program.funcs.iter().map(|func| {crates/blockifier/src/execution/contract_class.rs line 673 at r13 (raw file):
&& self.sierra_program == other.sierra_program } }
When is it used?
Code quote:
impl PartialEq for NativeContractClassV1Inner {
fn eq(&self, other: &Self) -> bool {
self.entry_points_by_type == other.entry_points_by_type
&& self.sierra_program == other.sierra_program
}
}crates/blockifier/src/execution/contract_class.rs line 689 at r13 (raw file):
/// [FunctionId] lookup table. /// /// On failure returns the first FunctionId that it couldn't find.
Where can I see it in the code?
Code quote:
/// On failure returns the first FunctionId that it couldn't find.crates/blockifier/src/execution/contract_class.rs line 692 at r13 (raw file):
fn try_from( lookup: &LookupTable<'_>, sierra_ep: &SierraContractEntryPoints,
Suggestion:
sierra_eps: &SierraContractEntryPoints,crates/blockifier/src/execution/contract_class.rs line 697 at r13 (raw file):
.constructor .iter() .map(|c| NativeEntryPoint::try_from(lookup, c))
Same below
Suggestion:
.map(|sierra_ep| NativeEntryPoint::try_from(lookup, c))crates/blockifier/src/execution/contract_class.rs line 738 at r13 (raw file):
fn try_from( lookup: &LookupTable<'_>, contract_ep: &ContractEntryPoint,
Suggestion:
sierra_ep: &ContractEntryPoint,crates/blockifier/src/execution/contract_class.rs line 741 at r13 (raw file):
) -> Result<NativeEntryPoint, NativeEntryPointError> { let &function_id = lookup .get(&contract_ep.function_idx)
I don't think that the function id (the key) has the same meaning as the function index
Code quote:
let &function_id = lookup
.get(&contract_ep.function_idx)crates/blockifier/src/execution/errors.rs line 101 at r13 (raw file):
#[source] source: NativeRunnerError, },
Please remove the added error that are not used in this PR.
Code quote:
#[error("Failed to convert Sierra to Casm: {0}")]
FailedToConvertSierraToCasm(String),
#[error("Internal error: {0}")]
InternalError(String),
#[error("Invalid input: {input_descriptor}; {info}")]
InvalidExecutionInput { input_descriptor: String, info: String },
#[error("Native execution error: {info}")]
NativeExecutionError { info: String },
#[error("Native Fallback Error: {info}")]
NativeFallbackError { info: Box<EntryPointExecutionError> },
#[error("Native unexpected error: {source}")]
NativeUnexpectedError {
#[source]
source: NativeRunnerError,
},crates/blockifier/src/transaction/transaction_utils.rs line 40 at r16 (raw file):
} ContractClass::V1Native(_) => todo!("Native verify contract class version."), }
Suggestion:
ContractClass::V1(_) | ContractClass::V1Native(_)=> {
if declare_version == TransactionVersion::TWO
|| declare_version == TransactionVersion::THREE
{
return Ok(());
}
Err(TransactionExecutionError::ContractClassVersionMismatch {
declare_version,
cairo_version: 1,
})
}
}
There was a problem hiding this comment.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @varex83)
crates/blockifier/src/execution/contract_class.rs line 741 at r13 (raw file):
Previously, noaov1 (Noa Oved) wrote…
I don't think that the function id (the key) has the same meaning as the function index
You should use the function_idx as the index in the function vector.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @noaov1)
crates/blockifier/src/execution/contract_class.rs line 11 at r13 (raw file):
use cairo_lang_starknet_classes::contract_class::{ ContractClass as SierraContractClass, ContractEntryPoint,
Done.
crates/blockifier/src/execution/contract_class.rs line 606 at r13 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Can you please extract this into a method and call it from
get_entry_pointofContractClassV1as well?
Done.
crates/blockifier/src/execution/contract_class.rs line 632 at r13 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Why not delete it in this PR?
We will need to add a few tests, and hashing for the sierra_program in order to compare them
crates/blockifier/src/execution/contract_class.rs line 673 at r13 (raw file):
Previously, noaov1 (Noa Oved) wrote…
When is it used?
ContractClass -> ContractClass::V1(NativeContractClassV1) -> NativeContractClassV1(pub Arc)
Since ContractClass needs to have PartialEq implemented for some tests (10 occurrences), we implement it for NativeContractClassV1Inner
crates/blockifier/src/execution/contract_class.rs line 689 at r13 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Where can I see it in the code?
NativeEntryPoint::try_from(..) returns the NativeEntryPointError::FunctionIdNotFound(<function_idx>), which is called and then unwrapped in the following code
Code snippet:
let constructor = sierra_eps
.constructor
.iter()
.map(|sierra_ep| NativeEntryPoint::try_from(lookup, sierra_ep))
.collect::<Result<_, _>>()?;crates/blockifier/src/execution/contract_class.rs line 692 at r13 (raw file):
fn try_from( lookup: &LookupTable<'_>, sierra_ep: &SierraContractEntryPoints,
Done.
crates/blockifier/src/execution/contract_class.rs line 697 at r13 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Same below
Done.
crates/blockifier/src/execution/contract_class.rs line 738 at r13 (raw file):
fn try_from( lookup: &LookupTable<'_>, contract_ep: &ContractEntryPoint,
Done.
crates/blockifier/src/execution/errors.rs line 101 at r13 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Please remove the added error that are not used in this PR.
Done.
|
Benchmark movements: |
There was a problem hiding this comment.
Reviewable status: 35 of 38 files reviewed, 10 unresolved discussions (waiting on @meship-starkware and @noaov1)
crates/blockifier/src/execution/contract_class.rs line 741 at r13 (raw file):
Previously, noaov1 (Noa Oved) wrote…
You should use the
function_idxas the index in the function vector.
Done.
|
Benchmark movements: |
There was a problem hiding this comment.
Reviewed 1 of 3 files at r17, all commit messages.
Reviewable status: 36 of 38 files reviewed, 12 unresolved discussions (waiting on @noaov1 and @varex83)
.github/workflows/blockifier_ci.yml line 59 at r18 (raw file):
steps: - uses: actions/checkout@v4
Revert this file changes.
.github/workflows/blockifier_post-merge.yml line 39 at r18 (raw file):
- run: | pip install -r crates/blockifier/tests/requirements.txt
Revert this file changes.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r16, all commit messages.
Reviewable status: 36 of 38 files reviewed, 8 unresolved discussions (waiting on @meship-starkware and @varex83)
crates/blockifier/src/execution/contract_class.rs line 632 at r13 (raw file):
Previously, varex83 (Bohdan Ohorodnii) wrote…
We will need to add a few tests, and hashing for the
sierra_programin order to compare them
Got it. WDYT about storing the casm directly?
i.e. have a field of
crates/blockifier/src/execution/contract_class.rs line 57 at r18 (raw file):
pub type LookupTable<'a> = HashMap<usize, &'a FunctionId>; fn assert_entry_point_not_constructor(call: &CallEntryPoint) -> Result<(), PreExecutionError> {
Consider defining it in the impl of CallEntryPoint.
Suggestion:
fn verify_constructor(call: &CallEntryPoint) -> Resultcrates/blockifier/src/execution/contract_class.rs line 652 at r18 (raw file):
// entry point with FunctionIds from SierraProgram. let lookup_fid: LookupTable<'_> = HashMap::from_iter( sierra_program.funcs.iter().enumerate().map(|(idx, func)| (idx, &func.id)),
Why not keep it as a vector of function ids??
Code quote:
let lookup_fid: LookupTable<'_> = HashMap::from_iter(
sierra_program.funcs.iter().enumerate().map(|(idx, func)| (idx, &func.id)),
There was a problem hiding this comment.
Reviewable status: 36 of 38 files reviewed, 8 unresolved discussions (waiting on @meship-starkware, @noaov1, and @varex83)
crates/blockifier/src/execution/contract_class.rs line 632 at r13 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Got it. WDYT about storing the casm directly?
i.e. have a field of
Oops, got half a comment it seems :). My 5 cents is that it won't matter too much, we are already working on eliminating it completely already
crates/blockifier/src/execution/contract_class.rs line 673 at r13 (raw file):
Previously, varex83 (Bohdan Ohorodnii) wrote…
ContractClass -> ContractClass::V1(NativeContractClassV1) -> NativeContractClassV1(pub Arc)
Since ContractClass needs to have
PartialEqimplemented for some tests (10 occurrences), we implement it for NativeContractClassV1Inner
As Bohdam said it is requirement. Since we are dumping the Sierra program and comparing the executors is not straightforward since they are reference pointers and not the actual binary.
The plan is to remove sierra_program and leave a unique identifier which is a obtained hashing all the inputs (open to ideas on this as well)
We have yet to determine if the mechanism also implies we don't need to compare entry_points_by_type either.
Finally, shouldn't the CASM and Native representation of the same File have the same comparison? We haven't accounted for this yet, but maybe it needs to be implemented
|
Benchmark movements: |
|
Benchmark movements: |
There was a problem hiding this comment.
Reviewable status: 36 of 39 files reviewed, 8 unresolved discussions (waiting on @meship-starkware, @noaov1, and @rodrigo-pino)
crates/blockifier/src/execution/contract_class.rs line 57 at r18 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Consider defining it in the
implofCallEntryPoint.
Done.
crates/blockifier/src/execution/contract_class.rs line 652 at r18 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Why not keep it as a vector of function ids??
Done.
|
Benchmark movements: |
There was a problem hiding this comment.
Reviewed 1 of 2 files at r20, 2 of 2 files at r21, all commit messages.
Reviewable status: 37 of 39 files reviewed, 7 unresolved discussions (waiting on @noaov1 and @varex83)
.github/workflows/blockifier_ci.yml line 59 at r18 (raw file):
Previously, meship-starkware (Meshi Peled) wrote…
Revert this file changes.
There is still this space
…ml and main.yml workflows
rodrigo-pino
force-pushed
the
native/add-native-execution-engine
branch
from
ceeeaa4 to
9a9e4bb
Compare
There was a problem hiding this comment.
Reviewed 13 of 13 files at r43, 1 of 1 files at r44, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @varex83)
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-18If installed with brew
If installed using Debian / Ubuntu repository
git clone https://github.com/lambdaclass/cairo_nativecd cairo_native/runtimecargo build --releasecd ..andexport CAIRO_NATIVE_RUNTIME_LIBRARY=$(pwd)/target/release/libcairo_native_runtime.aThis change is