-
Notifications
You must be signed in to change notification settings - Fork 170
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
Native contract executor #2156
Native contract executor #2156
Conversation
Corresponding sequencer PR: NethermindEth/sequencer#19 PR: #2156
778878f
to
1368f82
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ContractExecutor::new
does not do any caching to disk. The compile_and_load
, load_compiled_contract
and others needs to be reworked with ContractExecutor::save
and ContractExecutor::load
.
ContractExecutor's entrypoints are not public, so we must either:
|
Those don't have to be public, it's handled by the load/save of
The |
2e4d323
to
f0baf42
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two points to address:
- caching has no effect if it's always invalidated
- format the code with
cargo fmt
.
and the minimum supported Rust version is now 1.80.1.
vm/rust/src/juno_state_reader.rs
Outdated
let executor = ContractExecutor::load(&library_output_path).unwrap_or({ | ||
let executor = ContractExecutor::new(&sierra_program, OptLevel::Default)?; | ||
executor.save(&library_output_path)?; | ||
executor | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will always compile and save the contract as the argument to unwrap_or
is eagerly evaluated. Use unwrap_or_else
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Furthermore, load
and save
needlessly borrow the library_output_path
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will always compile and save the contract as the argument to
unwrap_or
is eagerly evaluated. Useunwrap_or_else
instead.
Changed to or_else
so I could still use "?" operator. Had to specify the type though since it couldn't figure it out.
Furthermore,
load
andsave
needlessly borrow thelibrary_output_path
.
Fixed.
17015c0
to
7de1090
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change itself looks good and I've approved it. But before merging look into the following regression first.
Block 633333 fails with:
execute transaction #289: failed txn 1132083247661400189175640804679018936217786098286704013683829309015708481258 reason: L1 gas bounds (max amount: 1038, max price: 67469438047820) exceed balance (44664767982987702).
Corresponding sequencer PR: NethermindEth/sequencer#19 PR: #2156
12c6a0d
to
f5e1828
Compare
Noted. I think it's fine for now as the gas changes will break things until we have all the starkware changes in our repo. |
Corresponding sequencer PR: NethermindEth/sequencer#19
PR: #2156