build(blockifier): track access to contract 0x1 #3875
Conversation
|
This change is |
aner-starkware
force-pushed
the
aner/track_contract_0x1
branch
5 times, most recently
from
5a80804 to
5f11877
Compare
There was a problem hiding this comment.
Reviewed 6 of 6 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @aner-starkware)
crates/blockifier/src/execution/call_info.rs line 152 at r1 (raw file):
pub read_block_hash_values: Vec<BlockHash>, pub accessed_blocks: HashSet<BlockNumber>, }
Now I would create a struct for this... non-blocking, and maybe @Yoni-Starkware won't like...
WDYT?
Suggestion:
#[cfg_attr(any(test, feature = "testing"), derive(Clone))]
#[cfg_attr(feature = "transaction_serde", derive(serde::Deserialize))]
#[derive(Debug, Default, Eq, PartialEq, Serialize)]
pub struct StorageAccess<K, V> {
pub accessed_keys: HashSet<K>,
pub ordered_values: Vec<V>,
}
#[cfg_attr(any(test, feature = "testing"), derive(Clone))]
#[cfg_attr(feature = "transaction_serde", derive(serde::Deserialize))]
#[derive(Debug, Default, Eq, PartialEq, Serialize)]
pub struct StorageAccessTracker {
pub storage_reads: StorageAccess<StorageKey, Felt>,
pub class_hash_reads: StorageAccess<ContractAddress, ClassHash>,
// TODO(Aner): add tests for storage tracking of contract 0x1
pub block_hash_reads: StorageAccess<BlockNumber, BlockHash>
}crates/starknet_consensus_orchestrator/src/cende/central_objects_test.rs line 496 at r1 (raw file):
accessed_contract_addresses: HashSet::from([contract_address!("0x1")]), read_block_hash_values: vec![BlockHash(felt!("0xdeafbee"))], accessed_blocks: HashSet::from([BlockNumber(100)]),
probably needs eyes - or at least to inform - this team
Code quote:
read_block_hash_values: vec![BlockHash(felt!("0xdeafbee"))],
accessed_blocks: HashSet::from([BlockNumber(100)]),crates/blockifier/src/execution/native/syscall_handler.rs line 237 at r1 (raw file):
impl StarknetSyscallHandler for &mut NativeSyscallHandler<'_> { fn get_block_hash( // HERE???
this function calls self.base.get_block_hash, so I don't think you need to touch anything here; right @Yoni-Starkware ?
Code quote:
// HERE???crates/blockifier/src/execution/syscalls/syscall_base.rs line 52 at r1 (raw file):
pub read_class_hash_values: Vec<ClassHash>, // Accessed addresses by the `get_class_hash_at` syscall. pub accessed_contract_addresses: HashSet<ContractAddress>,
these four should already be in the StorageAccessTracker struct, no?
- now you have 6 fields
Code quote:
pub read_values: Vec<Felt>,
pub accessed_keys: HashSet<StorageKey>,
pub read_class_hash_values: Vec<ClassHash>,
// Accessed addresses by the `get_class_hash_at` syscall.
pub accessed_contract_addresses: HashSet<ContractAddress>,crates/blockifier/src/execution/syscalls/syscall_base.rs line 144 at r1 (raw file):
self.read_block_hash_values.push(BlockHash(block_hash)); Ok(block_hash) }
yes, exactly :)
Code quote:
self.accessed_blocks.insert(BlockNumber(requested_block_number));
let key = StorageKey::try_from(Felt::from(requested_block_number))?;
let block_hash_contract_address = self
.context
.tx_context
.block_context
.versioned_constants
.os_constants
.os_contract_addresses
.block_hash_contract_address();
let block_hash = self.state.get_storage_at(block_hash_contract_address, key)?;
self.read_block_hash_values.push(BlockHash(block_hash));
Ok(block_hash)
}
|
Previously, dorimedini-starkware wrote…
I was thinking the same, but I guess there's a reason it was done this way? |
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @aner-starkware)
crates/blockifier/src/execution/call_info.rs line 152 at r1 (raw file):
Previously, aner-starkware wrote…
I was thinking the same, but I guess there's a reason it was done this way?
don't think so, @Yoni-Starkware ?
aner-starkware
force-pushed
the
aner/track_contract_0x1
branch
from
5f11877 to
7988824
Compare
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @aner-starkware)
aner-starkware
force-pushed
the
aner/track_contract_0x1
branch
from
7988824 to
6e533b7
Compare
There was a problem hiding this comment.
Reviewable status: 5 of 7 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware)
crates/blockifier/src/execution/syscalls/syscall_base.rs line 52 at r1 (raw file):
Previously, dorimedini-starkware wrote…
these four should already be in the StorageAccessTracker struct, no?
- now you have 6 fields
Done?
crates/starknet_consensus_orchestrator/src/cende/central_objects_test.rs line 496 at r1 (raw file):
Previously, dorimedini-starkware wrote…
probably needs eyes - or at least to inform - this team
Who would that be?
There was a problem hiding this comment.
Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aner-starkware)
crates/starknet_consensus_orchestrator/src/cende/central_objects_test.rs line 496 at r1 (raw file):
Previously, aner-starkware wrote…
Who would that be?
good question. maybe @ShahakShama ?
aner-starkware
force-pushed
the
aner/track_contract_0x1
branch
2 times, most recently
from
dffcf55 to
ffcd860
Compare
aner-starkware
force-pushed
the
aner/track_contract_0x1
branch
from
ffcd860 to
08a3cf5
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aner-starkware)
No description provided.