Add get_block_hash_syscall skeleton to syscalls and libfuncs.

[ArniStarkware]

This change is 

[ArniStarkware]

crates/cairo-lang-runner/src/casm_run/mod.rs line 714 at r1 (raw file):

        block_number: u64,
    ) -> Result<SyscallResult, HintError> {
        // TODO(Arni, 28/5/2023): Replace the temporary return value with the required value.

Here is the point of discussion from Monday - 22/5/2023.

[orizi]

Reviewed 4 of 9 files at r1, all commit messages.
Reviewable status: 4 of 9 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware)

---

crates/cairo-lang-runner/src/casm_run/mod.rs line 714 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Here is the point of discussion from Monday - 22/5/2023.

if it is some storage read - and no actual OS support - this should still be a contract call.

aside from this - the pr structure is fine.

---

crates/cairo-lang-sierra-gas/src/starknet_libfunc_cost_base.rs line 42 at r1 (raw file):

        // TODO(Arni, 28/5/2023): Fix to the correct cost. For now aligned with the cost of
        // StorageRead.
        StarkNetConcreteLibfunc::GetBlockHash(_) => syscall_cost(2),

this is validated - cost is calculated by arg count only.

Suggestion:

        StarkNetConcreteLibfunc::GetBlockHash(_) => syscall_cost(1),

[ArniStarkware]

crates/cairo-lang-sierra-gas/src/starknet_libfunc_cost_base.rs line 42 at r1 (raw file):

Previously, orizi wrote…

this is validated - cost is calculated by arg count only.

Done.

[orizi]

Reviewed 1 of 1 files at r2.
Reviewable status: 4 of 9 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware)

---

tests/e2e_test_data/libfuncs/starknet/syscalls line 83 at r2 (raw file):

test::foo@0([0]: GasBuiltin, [1]: System, [2]: core::array::Span::<core::felt252>, [3]: core::array::Span::<core::felt252>) -> (GasBuiltin, System, core::result::Result::<(), core::array::Array::<core::felt252>>);

// TODO(Arni, 28/5/2023): Add test for get_block_hash_syscall.

adding the test is about as easy as adding the todo :)

Code quote:

// TODO(Arni, 28/5/2023): Add test for get_block_hash_syscall.

[orizi]

Reviewed 1 of 9 files at r1, all commit messages.
Reviewable status: 5 of 9 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware)

---

crates/cairo-lang-runner/src/casm_run/mod.rs line 718 at r3 (raw file):

        //      One design suggestion - to preform a storage read. Have an arbitrary, hardcoded
        //      (For example, addr=1) contain the mapping from block number to block hash.
        Ok(SyscallResult::Success(vec![0.into()]))

can you add a better placeholder impl?
either some random value based on the block number or just:
fail_syscall!(b"GET_BLOCK_HASH_UNIMPLEMENTED");

Code quote:

        // TODO(Arni, 28/5/2023): Replace the temporary return value with the required value.
        //      One design suggestion - to preform a storage read. Have an arbitrary, hardcoded
        //      (For example, addr=1) contain the mapping from block number to block hash.
        Ok(SyscallResult::Success(vec![0.into()]))

[orizi]

Reviewed 1 of 9 files at r1, 2 of 4 files at r4, all commit messages.
Reviewable status: 7 of 11 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware)

---

crates/cairo-lang-sierra/src/extensions/modules/starknet/getter.rs line 172 at r4 (raw file):

#[derive(Default)]
pub struct GetBlockHashTrait {}
impl GetterTraitsEx for GetBlockHashTrait {

this isn't a getter - this should be implemented more like storage_read.

Code quote:

impl GetterTraitsEx for GetBlockHashTrait

[ArniStarkware]

crates/cairo-lang-runner/src/casm_run/mod.rs line 718 at r3 (raw file):

Previously, orizi wrote…

can you add a better placeholder impl?
either some random value based on the block number or just:
fail_syscall!(b"GET_BLOCK_HASH_UNIMPLEMENTED");

Done.

[orizi]

Reviewable status: 7 of 11 files reviewed, 4 unresolved discussions (waiting on @ArniStarkware)

a discussion (no related file):
also - add a test with

#[should_panic(expected: ('Out of gas', 'GET_BLOCK_HASH_UNIMPLEMENTED', ))]

in the cairo_level_tests library.

---

---

tests/e2e_test_data/libfuncs/starknet/syscalls line 83 at r2 (raw file):

Previously, orizi wrote…

adding the test is about as easy as adding the todo :)

this test is definitely not valid yet (probably because of the other requested changes)

[orizi]

Reviewed 1 of 9 files at r1, 1 of 2 files at r3, 2 of 4 files at r4.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ArniStarkware)

[ArniStarkware]

crates/cairo-lang-sierra/src/extensions/modules/starknet/getter.rs line 172 at r4 (raw file):

Previously, orizi wrote…

this isn't a getter - this should be implemented more like storage_read.

Done. Removed.

[orizi]

Reviewed 6 of 6 files at r5, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ArniStarkware)

---

crates/cairo-lang-sierra/src/extensions/modules/starknet/storage.rs line 233 at r5 (raw file):

        context: &dyn SignatureSpecializationContext,
    ) -> Result<Vec<crate::ids::ConcreteTypeId>, SpecializationError> {
        Ok(vec![

The input is u64, the output is felt252, fix all.

---

crates/cairo-lang-starknet/cairo_level_tests/interoperability.cairo line 169 at r5 (raw file):

// #[test]
// #[should_panic(expected: ('Out of gas', 'GET_BLOCK_HASH_UNIMPLEMENTED', ))]

Uncomment

[orizi]

Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArniStarkware)

[ArniStarkware]

tests/e2e_test_data/libfuncs/starknet/syscalls line 83 at r2 (raw file):

Previously, orizi wrote…

this test is definitely not valid yet (probably because of the other requested changes)

Done! Finally

[ArniStarkware]

crates/cairo-lang-sierra/src/extensions/modules/starknet/storage.rs line 233 at r5 (raw file):

Previously, orizi wrote…

The input is u64, the output is felt252, fix all.

Done.

[ArniStarkware]

Reviewable status: 12 of 13 files reviewed, 1 unresolved discussion (waiting on @orizi)

a discussion (no related file):

Previously, orizi wrote…

also - add a test with

#[should_panic(expected: ('Out of gas', 'GET_BLOCK_HASH_UNIMPLEMENTED', ))]

in the cairo_level_tests library.

Done.

---

---

crates/cairo-lang-starknet/cairo_level_tests/interoperability.cairo line 169 at r5 (raw file):

Previously, orizi wrote…

Uncomment

Done.

[orizi]

Reviewed all commit messages.
Reviewable status: 12 of 13 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware)

---

crates/cairo-lang-starknet/cairo_level_tests/interoperability.cairo line 169 at r5 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Done.

I think unwrapping and 'should_panic' would be nicer for when you actually add an implementation

[ArniStarkware]

crates/cairo-lang-starknet/cairo_level_tests/interoperability.cairo line 169 at r5 (raw file):

Previously, orizi wrote…

I think unwrapping and 'should_panic' would be nicer for when you actually add an implementation

Now with nicer should_panic.

[orizi]

Reviewed 2 of 2 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)

[ilyalesokhin-starkware]

Reviewed 3 of 9 files at r1, 1 of 1 files at r2, 2 of 4 files at r4, 3 of 6 files at r5, 2 of 3 files at r6, 2 of 2 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)