-
Notifications
You must be signed in to change notification settings - Fork 26
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(blockifier): disallow contract addresses less than 16 #2252
Conversation
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.
python side PR
Reviewable status: 0 of 8 files reviewed, all discussions resolved (waiting on @dorimedini-starkware and @Yoni-Starkware)
Artifacts upload workflows: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2252 +/- ##
===========================================
+ Coverage 40.10% 65.44% +25.34%
===========================================
Files 26 152 +126
Lines 1895 18292 +16397
Branches 1895 18292 +16397
===========================================
+ Hits 760 11972 +11212
- Misses 1100 5569 +4469
- Partials 35 751 +716 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
78e37cf
to
43443da
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.
Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nimrod-starkware and @Yoni-Starkware)
crates/blockifier/src/execution/syscalls/syscall_tests/get_block_hash.rs
line 10 at r1 (raw file):
use test_case::test_case; use crate::abi::constants::{self, BLOCK_HASH_CONTRACT_ADDRESS};
delete (if no one else uses constants::<something>
in this file), or revert and write constants::BLOCK_HASH_CONTRACT_ADDRESS
instead of BLOCK_HASH_CONTRACT_ADDRESS
Code quote:
self
crates/starknet_api/src/core.rs
line 162 at r1 (raw file):
Ok(Self(PatriciaKey::try_from(hash)?)) } }
not a good place to check: 0x1
is a valid contract address (block hash contract), it's just not deployable.
I think this needs to be checked explicitly during execution of deployment (syscall, or DEPLOY_ACCOUNT tx).
@Yoni-Starkware thoughts?
Code quote:
impl TryFrom<StarkHash> for ContractAddress {
type Error = StarknetApiError;
fn try_from(hash: StarkHash) -> Result<Self, Self::Error> {
if hash <= MAX_RESERVED_CONTRACT_ADDRESS.into() {
return Err(Self::Error::ReservedContractAddress(hash));
}
Ok(Self(PatriciaKey::try_from(hash)?))
}
}
crates/blockifier/resources/versioned_constants_0_13_4.json
line 242 at r1 (raw file):
"pedersen_builtin": 7, "range_check_builtin": 22 },
where do these numbers come from? how are they generated?
also, what about DEPLOY_ACCOUNT? it doesn't use the syscall?
Code quote:
"n_steps": 1158,
"builtin_instance_counter": {
"pedersen_builtin": 7,
"range_check_builtin": 22
},
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.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @nimrod-starkware)
crates/starknet_api/src/core.rs
line 162 at r1 (raw file):
Previously, dorimedini-starkware wrote…
not a good place to check:
0x1
is a valid contract address (block hash contract), it's just not deployable.
I think this needs to be checked explicitly during execution of deployment (syscall, or DEPLOY_ACCOUNT tx).
@Yoni-Starkware thoughts?
Right.
No description provided.