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.

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: support catching EntryPointNotFound #1248

Merged
merged 1 commit into from
Oct 21, 2024

Conversation

ilyalesokhin-starkware
Copy link
Contributor

@ilyalesokhin-starkware ilyalesokhin-starkware commented Oct 8, 2024

This change is Reviewable

Copy link
Contributor Author

@ilyalesokhin-starkware ilyalesokhin-starkware left a 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 7 files reviewed, 1 unresolved discussion (waiting on @Yoni-Starkware)


crates/blockifier/src/execution/entry_point.rs line 170 at r1 (raw file):

                resources: Default::default(),
                inner_calls: vec![],
                tracked_resource: TrackedResource::SierraGas,

what should I put here?

Code quote:

tracked_resource: TrackedResource::SierraGas

Copy link

codecov bot commented Oct 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.19%. Comparing base (b0cfe82) to head (02c1b7d).
Report is 465 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (b0cfe82) and HEAD (02c1b7d). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (b0cfe82) HEAD (02c1b7d)
3 1
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1248      +/-   ##
==========================================
- Coverage   74.18%   65.19%   -8.99%     
==========================================
  Files         359      163     -196     
  Lines       36240    22051   -14189     
  Branches    36240    22051   -14189     
==========================================
- Hits        26886    14377   -12509     
+ Misses       7220     6537     -683     
+ Partials     2134     1137     -997     
Flag Coverage Δ
?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ilyalesokhin-starkware ilyalesokhin-starkware force-pushed the ilya/selector_not_found branch 2 times, most recently from 9b7a672 to 5aa6bfe Compare October 8, 2024 11:03
@ilyalesokhin-starkware ilyalesokhin-starkware changed the title Support catching EntryPointNotFound. feat: support catching EntryPointNotFound Oct 8, 2024
Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 7 files at r1, 1 of 4 files at r2.
Reviewable status: 3 of 9 files reviewed, 4 unresolved discussions (waiting on @ilyalesokhin-starkware)


crates/blockifier/src/execution/entry_point.rs line 170 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

what should I put here?

Move this code inside execute_entry_point_call_wrapper and take context.tracked_resource_stack.last()


crates/blockifier/src/execution/entry_point.rs line 174 at r2 (raw file):

                accessed_storage_keys: Default::default(),
            }),
            res => res,

Suggestion:

        let orig_call = self.clone();
        match execute_entry_point_call_wrapper(self, contract_class, state, resources, context) {
            Err(EntryPointExecutionError::PreExecutionError(
                PreExecutionError::EntryPointNotFound(_),
            )) if context.versioned_constants().enable_reverts => Ok(CallInfo {
                call: self.clone(),
                execution: CallExecution {
                    retdata: Retdata(vec![Felt::from_hex(ENTRYPOINT_NOT_FOUND_ERROR).unwrap()]),
                    failed: true,
                    ..CallExecution::default()
                },
                tracked_resource: TrackedResource::SierraGas,
                ..CallInfo::default()
            }),
            res => res,

crates/blockifier/src/execution/entry_point_test.rs line 168 at r2 (raw file):

        CallEntryPoint { entry_point_selector, ..trivial_external_entry_point_new(test_contract) };
    let call_info = entry_point_call.execute_directly(&mut state).unwrap();
    assert!(call_info.execution.failed);

Can you check the retdata format?

Code quote:

assert!(call_info.execution.failed);

crates/papyrus_rpc/src/v0_8/execution_test.rs line 293 at r2 (raw file):

    let entry_point_not_found_error =
        felt!("0x000000000000000000000000454e545259504f494e545f4e4f545f464f554e44");
    assert_eq!(call, [entry_point_not_found_error]);

Not sure this is the right behavior here. A reverted call should result in an error.

Code quote:

    // Calling a non-existent entry point.
    let call = module
        .call::<_, Vec<Felt>>(
            "starknet_V0_8_call",
            (
                CallRequest {
                    contract_address: *DEPRECATED_CONTRACT_ADDRESS,
                    entry_point_selector: selector_from_name("aaa"),
                    calldata: calldata![key, value],
                },
                BlockId::HashOrNumber(BlockHashOrNumber::Number(BlockNumber(0))),
            ),
        )
        .await
        .unwrap();

    let entry_point_not_found_error =
        felt!("0x000000000000000000000000454e545259504f494e545f4e4f545f464f554e44");
    assert_eq!(call, [entry_point_not_found_error]);

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

What if the missing selector is coming from Cairo 0?

Reviewable status: 3 of 9 files reviewed, 4 unresolved discussions (waiting on @ilyalesokhin-starkware)

@ilyalesokhin-starkware
Copy link
Contributor Author

crates/blockifier/src/execution/entry_point.rs line 174 at r2 (raw file):

 gas_consumed: 0,

this is also the default, is there a reason you left it?

@ilyalesokhin-starkware
Copy link
Contributor Author

crates/blockifier/src/execution/entry_point.rs line 174 at r2 (raw file):

self.clone(),

self was consume by execute_entry_point_call_wrapper

Copy link
Contributor Author

@ilyalesokhin-starkware ilyalesokhin-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 9 files reviewed, 3 unresolved discussions (waiting on @Yoni-Starkware)


crates/blockifier/src/execution/entry_point_test.rs line 168 at r2 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Can you check the retdata format?

Done.

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 8 files at r3, all commit messages.
Reviewable status: 5 of 10 files reviewed, 4 unresolved discussions (waiting on @ilyalesokhin-starkware)


crates/blockifier/src/execution/execution_utils.rs line 60 at r3 (raw file):

    context: &mut EntryPointExecutionContext,
) -> EntryPointExecutionResult<CallInfo> {
    let tracked_resource = contract_class

Suggestion:

contract_tracked_resource = contract_class

crates/blockifier/src/execution/execution_utils.rs line 91 at r3 (raw file):

            res
        }
    }

To be on the safe side - you might add more cases here and forget to pop it.

Suggestion:

    let orig_call = call.clone();
    let res = execute_entry_point_call(call, contract_class, state, resources, context);
    let current_tracked_resource = context.tracked_resource_stack.pop().expect("Unexpected empty tracked resource.")
    match res {
        Err(EntryPointExecutionError::PreExecutionError(
            PreExecutionError::EntryPointNotFound(_),
        )) if context.versioned_constants().enable_reverts => Ok(CallInfo {
            call: orig_call,
            execution: CallExecution {
                retdata: Retdata(vec![Felt::from_hex(ENTRYPOINT_NOT_FOUND_ERROR).unwrap()]),
                failed: true,
                gas_consumed: 0,
                ..CallExecution::default()
            },
            tracked_resource: current_tracked_resource,
            ..CallInfo::default()
        }),
        res => res;
    }

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 5 of 10 files reviewed, 4 unresolved discussions (waiting on @ilyalesokhin-starkware)


crates/blockifier/src/execution/entry_point.rs line 174 at r2 (raw file):

this is also the default, is there a reason you left it?
I deleted it

Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.657 ms 34.721 ms 34.785 ms]
change: [-5.1593% -3.1591% -1.3301%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
3 (3.00%) high mild

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 7 files at r4, all commit messages.
Reviewable status: 5 of 14 files reviewed, 4 unresolved discussions (waiting on @ilyalesokhin-starkware)

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: 5 of 14 files reviewed, 4 unresolved discussions (waiting on @ilyalesokhin-starkware)

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 10 files at r6, all commit messages.
Reviewable status: 9 of 14 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)

@ilyalesokhin-starkware
Copy link
Contributor Author

crates/papyrus_rpc/src/v0_8/execution_test.rs line 293 at r2 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Not sure this is the right behavior here. A reverted call should result in an error.

fixed

Copy link
Contributor Author

@ilyalesokhin-starkware ilyalesokhin-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 7 of 14 files reviewed, 1 unresolved discussion (waiting on @Yoni-Starkware)


crates/blockifier/src/execution/execution_utils.rs line 91 at r3 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

To be on the safe side - you might add more cases here and forget to pop it.

Done.

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 7 files at r4, 2 of 10 files at r6, 3 of 3 files at r7, all commit messages.
Reviewable status: 13 of 14 files reviewed, all discussions resolved

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r8, all commit messages.
Reviewable status: 13 of 14 files reviewed, all discussions resolved

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 7 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware)

@ilyalesokhin-starkware ilyalesokhin-starkware force-pushed the ilya/selector_not_found branch 2 times, most recently from eefa1b2 to 7b1f7bd Compare October 20, 2024 11:02
@ilyalesokhin-starkware
Copy link
Contributor Author

crates/blockifier/src/execution/execution_utils.rs line 60 at r3 (raw file):

    context: &mut EntryPointExecutionContext,
) -> EntryPointExecutionResult<CallInfo> {
    let tracked_resource = contract_class

done

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware)

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware)

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r10, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware)

@ilyalesokhin-starkware ilyalesokhin-starkware merged commit e8f83df into main Oct 21, 2024
20 checks passed
@ilyalesokhin-starkware ilyalesokhin-starkware deleted the ilya/selector_not_found branch October 21, 2024 16:06
@github-actions github-actions bot locked and limited conversation to collaborators Oct 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants