Skip to content
This repository has been archived by the owner on Aug 21, 2024. It is now read-only.

Use BlockNumber in GetBlockHashRequest. #604

Merged

Conversation

ArniStarkware
Copy link
Contributor

@ArniStarkware ArniStarkware commented Jun 13, 2023

This change is Reviewable

@ArniStarkware ArniStarkware force-pushed the arni/starknet/get_block_syscall/skeleton branch from e9fd3b2 to 57aef41 Compare June 13, 2023 10:02
@ArniStarkware ArniStarkware changed the base branch from arni/starknet/get_block_syscall/skeleton to main-v0.12.0 June 15, 2023 06:52
@ArniStarkware ArniStarkware force-pushed the arni/starknet/get_block_syscall/use_block_number branch from 986807d to 8238804 Compare June 22, 2023 13:20
@ArniStarkware
Copy link
Contributor Author

crates/blockifier/src/execution/syscalls/mod.rs line 304 at r2 (raw file):

        let block_number_as_felt = stark_felt_from_ptr(vm, ptr)?;
        let block_number =
            BlockNumber(u64::try_from(usize::try_from(block_number_as_felt)?).map_err(|_| {

There is a correctness issue here.
Can be solved if the following PR is merged into starknet-api: https://reviewable.io/reviews/starkware-libs/starknet-api/87

Code quote:

u64::try_from(usize::try_from(block_number_as_felt)?

@ArniStarkware ArniStarkware requested review from dafnamatsry and noaov1 and removed request for Yoni-Starkware June 26, 2023 09:05
Copy link
Contributor

@dafnamatsry dafnamatsry left a comment

Choose a reason for hiding this comment

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

Reviewed 9 of 9 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noaov1)

dafnamatsry
dafnamatsry previously approved these changes Jun 26, 2023
Copy link
Contributor

@dafnamatsry dafnamatsry left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noaov1)

@ArniStarkware
Copy link
Contributor Author

crates/blockifier/src/execution/syscalls/mod.rs line 304 at r2 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

There is a correctness issue here.
Can be solved if the following PR is merged into starknet-api: https://reviewable.io/reviews/starkware-libs/starknet-api/87

Talked with @Yoni-Starkware . Let us merge this PR for now.
The only harm that this correctness issue would do is:

  1. Some valid requests will not be processed.
  2. The requests that will not be processed are for block_number >= 2^32. This will not be an issue in the following week.

Copy link
Collaborator

@noaov1 noaov1 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 9 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @dafnamatsry)


crates/blockifier/src/execution/syscalls/mod.rs line 306 at r3 (raw file):

                    string: block_number_as_felt.to_string(),
                })
            })?);

WDYT?

Suggestion:

    let block_number = vm.get_integer(*ptr)?;
    let block_number = felt.to_u64().ok_or_else(|| {
        SyscallExecutionError::StarknetApiError(StarknetApiError::OutOfRange {
            string: felt.to_string(),
        })
    })?;

@ArniStarkware
Copy link
Contributor Author

crates/blockifier/src/execution/syscalls/mod.rs line 306 at r3 (raw file):

Previously, noaov1 (Noa Oved) wrote…

WDYT?

Looks great!
See the solution in the following PR:

Copy link
Collaborator

@noaov1 noaov1 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 9 files reviewed, all discussions resolved (waiting on @dafnamatsry)

Copy link
Contributor

@dafnamatsry dafnamatsry 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 r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)

@ArniStarkware ArniStarkware merged commit 15023cd into main-v0.12.0 Jun 27, 2023
3 checks passed
@ArniStarkware
Copy link
Contributor Author

crates/blockifier/src/execution/syscalls/mod.rs line 306 at r3 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Looks great!
See the solution in the following PR:

See #668.

@ArniStarkware ArniStarkware deleted the arni/starknet/get_block_syscall/use_block_number branch July 5, 2023 07:57
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.

3 participants