feat(blockifier): add read and write cairo native syscalls

[rodrigo-pino]

Add the read and write syscalls to the NativeSyscallHandler.

Open for you to see and react. Notice that Cairo Native dependency has changed, this is because I had to update that relative paths are solved in order for it to detect the Runtime based on the .cargo/config.toml file.

Looking forward to your feedback.

---

This change is 

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.362 ms 34.446 ms 34.576 ms]
change: [-5.0542% -3.3553% -1.7909%] (p = 0.00 < 0.05)
Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
3 (3.00%) high mild
5 (5.00%) high severe

[meship-starkware]

Reviewed 3 of 3 files at r1, 1 of 2 files at r2, 2 of 2 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: 8 of 9 files reviewed, 1 unresolved discussion (waiting on @rodrigo-pino)

---

scripts/dependencies.sh line 79 at r4 (raw file):

function main() {
    # [ "$(uname)" = "Linux" ] && install_essential_deps_linux
    # setup_llvm_deps

Why?

Code quote:

    # [ "$(uname)" = "Linux" ] && install_essential_deps_linux
    # setup_llvm_deps

[avi-starkware]

Reviewed 3 of 3 files at r1, 1 of 2 files at r2, 2 of 2 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: 8 of 9 files reviewed, 5 unresolved discussions (waiting on @meship-starkware and @rodrigo-pino)

---

crates/blockifier/src/execution/native/syscall_handler.rs line 187 at r4 (raw file):

        let key = StorageKey(
            PatriciaKey::try_from(address).map_err(|e| encode_str_as_felts(&e.to_string()))?,
        );

Why not?

Suggestion:

        let key = StorageKey::try_from(address).map_err(|e| encode_str_as_felts(&e.to_string()))?;

---

crates/blockifier/src/execution/native/syscall_handler.rs line 190 at r4 (raw file):

        let read_result = self.state.get_storage_at(self.contract_address, key);
        let value = read_result.map_err(|e| encode_str_as_felts(&e.to_string()))?;

We don't do this error encoding in our current syscall handler. Are you sure we can't just use ? here?

Suggestion:

        self.state.get_storage_at(self.contract_address, key)?;

---

crates/blockifier/src/execution/native/syscall_handler.rs line 212 at r4 (raw file):

        let key = StorageKey(
            PatriciaKey::try_from(address).map_err(|e| encode_str_as_felts(&e.to_string()))?,
        );

Suggestion:

        let key = StorageKey::try_from(address).map_err(|e| encode_str_as_felts(&e.to_string()))?;

---

crates/blockifier/src/execution/native/syscall_handler.rs line 216 at r4 (raw file):

        let write_result = self.state.set_storage_at(self.contract_address, key, value);
        write_result.map_err(|e| encode_str_as_felts(&e.to_string()))?;

as suggested above
can this work?

Suggestion:

        self.state.set_storage_at(self.contract_address, key, value)?;

[avi-starkware]

Reviewable status: 8 of 9 files reviewed, 7 unresolved discussions (waiting on @meship-starkware and @rodrigo-pino)

---

Cargo.toml line 100 at r4 (raw file):

cairo-lang-utils = "2.8.2"
# cairo-native = "0.2.0-alpha.2"
cairo-native = { git = "https://github.com/NethermindEth/cairo_native", branch = "rdr/resolve-runtime-library-path-2.8.2" }

Why?

Code quote:

# cairo-native = "0.2.0-alpha.2"
cairo-native = { git = "https://github.com/NethermindEth/cairo_native", branch = "rdr/resolve-runtime-library-path-2.8.2" }

---

.cargo/config.toml line 2 at r4 (raw file):

[env]
CAIRO_NATIVE_RUNTIME_LIBRARY = "libcairo_native_runtime.a"

Please make this change in #1242

Code quote:

CAIRO_NATIVE_RUNTIME_LIBRARY = "libcairo_native_runtime.a"

[rodrigo-pino]

Reviewable status: 2 of 9 files reviewed, 7 unresolved discussions (waiting on @avi-starkware and @meship-starkware)

---

Cargo.toml line 100 at r4 (raw file):

Previously, avi-starkware wrote…

Why?

Required to correctly solve CAIRO_NATIVE_RUNTIME_LIBRARY otherwise we cannot use the .cargo/config.toml solution.

---

.cargo/config.toml line 2 at r4 (raw file):

Previously, avi-starkware wrote…

Please make this change in #1242

Done!

---

crates/blockifier/src/execution/native/syscall_handler.rs line 187 at r4 (raw file):

Previously, avi-starkware wrote…

Why not?

Done! Thanks!

---

crates/blockifier/src/execution/native/syscall_handler.rs line 190 at r4 (raw file):

Previously, avi-starkware wrote…

We don't do this error encoding in our current syscall handler. Are you sure we can't just use ? here?

You return a StateError which Native doesn't understand.

Cairo 1 (through Native) is expecting a SyscallResult, Cairo1/Sierra doesn't understand what a StateError is. What we do is that we propagate the error back by encoding the StateError message as a bunch of Felts and return control to the Contract.

Take into account that each time we are executing a contract we treat it as a black box. Each syscall is a request to the blockifier using the communication protocol established by the StarknetSyscallHandler trait.

---

crates/blockifier/src/execution/native/syscall_handler.rs line 212 at r4 (raw file):

        let key = StorageKey(
            PatriciaKey::try_from(address).map_err(|e| encode_str_as_felts(&e.to_string()))?,
        );

Done!

---

scripts/dependencies.sh line 79 at r4 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Why?

It wasn't intentional

[github-actions]

Benchmark movements:
full_committer_flow performance improved 😺
full_committer_flow time: [29.025 ms 29.061 ms 29.099 ms]
change: [-2.3279% -1.6504% -1.1666%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
3 (3.00%) high mild

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.61%. Comparing base (e3165c4) to head (8313167).
Report is 309 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1305       +/-   ##
===========================================
+ Coverage   40.10%   62.61%   +22.51%     
===========================================
  Files          26      361      +335     
  Lines        1895    39045    +37150     
  Branches     1895    39045    +37150     
===========================================
+ Hits          760    24449    +23689     
- Misses       1100    12567    +11467     
- Partials       35     2029     +1994     

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

[meship-starkware]

Reviewed 2 of 6 files at r5, 2 of 2 files at r6, 3 of 3 files at r7, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @avi-starkware and @rodrigo-pino)

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.290 ms 34.348 ms 34.414 ms]
change: [-2.8674% -1.8834% -1.1030%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
1 (1.00%) high mild
2 (2.00%) high severe

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.179 ms 34.228 ms 34.283 ms]
change: [-5.5778% -4.1527% -2.9184%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) high mild
1 (1.00%) high severe

full_committer_flow performance improved 😺
full_committer_flow time: [29.190 ms 29.249 ms 29.310 ms]
change: [-2.7350% -2.1228% -1.6355%] (p = 0.00 < 0.05)
Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mild

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.129 ms 34.191 ms 34.273 ms]
change: [-5.9819% -3.9751% -2.1340%] (p = 0.00 < 0.05)
Performance has improved.
Found 14 outliers among 100 measurements (14.00%)
7 (7.00%) high mild
7 (7.00%) high severe

[github-actions]

Artifacts upload triggered. View details here

[meship-starkware]

Reviewed 1 of 1 files at r21, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware and @noaov1)

[noaov1]

Reviewed 1 of 1 files at r21, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware)

[github-actions]

Artifacts upload triggered. View details here

[meship-starkware]

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware)

[noaov1]

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware)

[noaov1]

Dismissed @ilyalesokhin-starkware from a discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avi-starkware and @ilyalesokhin-starkware)

[noaov1]

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

[meship-starkware]

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

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[ilyalesokhin-starkware]

crates/blockifier/src/execution/native/syscall_handler.rs line 202 at r22 (raw file):

            let address_domain = Felt::from(address_domain);
            let err = SyscallExecutionError::InvalidAddressDomain { address_domain };
            return Err(encode_str_as_felts(&err.to_string()));

Can you add a wrapper for unprovable errors?

Suggestion:

handle_unprovable_error(err)

[meship-starkware]

Reviewed 5 of 5 files at r24, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware, @noaov1, and @rodrigo-pino)

---

-- commits line 5 at r24:
Can you squash these two commits into one

Code quote:

New commits in r22 on 11/11/2024 at 00:24, probably rebased from r21:
- 2fe81ff: feat(blockifier): add read & write syscalls

New commits in r24 (same as r22) on 11/11/2024 at 12:19:
- 2fe81ff: feat(blockifier): add read & write syscalls

[github-actions]

Artifacts upload triggered. View details here

[rodrigo-pino]

Reviewable status: 30 of 31 files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware, @meship-starkware, and @noaov1)

---

-- commits line 5 at r24:

Previously, meship-starkware (Meshi Peled) wrote…

Can you squash these two commits into one

It is weird, in my log I just have one commit 🤔 , not sure how to fix this.

---

crates/blockifier/src/execution/native/syscall_handler.rs line 202 at r22 (raw file):

Previously, ilyalesokhin-starkware wrote…

Can you add a wrapper for unprovable errors?

Done

---

crates/blockifier/src/execution/native/syscall_handler.rs line 101 at r25 (raw file):

    }

    fn handle_error(

Defined inside the impl intentionally (self will be used in the error handling)

[meship-starkware]

Reviewable status: 30 of 31 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware and @noaov1)

---

-- commits line 5 at r24:

Previously, rodrigo-pino (Rodrigo) wrote…

It is weird, in my log I just have one commit 🤔 , not sure how to fix this.

Then never mind, it will be squashed when merging

[ilyalesokhin-starkware]

Reviewed 1 of 2 files at r19, 1 of 1 files at r25.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @noaov1)

[noaov1]

Reviewed 1 of 1 files at r25, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avi-starkware and @meship-starkware)

[meship-starkware]

Reviewed 1 of 1 files at r25, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avi-starkware, @ilyalesokhin-starkware, and @noaov1)

[meship-starkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @rodrigo-pino)