Skip to content
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

Key not found in preimage #442

Merged
merged 10 commits into from
Dec 27, 2024
Merged

Key not found in preimage #442

merged 10 commits into from
Dec 27, 2024

Conversation

ftheirs
Copy link
Collaborator

@ftheirs ftheirs commented Nov 28, 2024

Problem: Execution from SNOS for the following blocks results in a key not found in preimage error:
237025, 237030, 237037, 237042, 237044, 237053, 237083, 237086, 235385, 235620
Better description from the issue can be found in #443

Solution:
Add the missing keys required to retrieve the correct storage proof for the contract address 0x1.

Closes #443

Type

  • feature
  • bugfix
  • dev (no functional changes, no API changes)
  • fmt (formatting, renaming)
  • build
  • docs
  • testing

Description

Breaking changes?

  • yes
  • no

@ftheirs ftheirs marked this pull request as ready for review November 29, 2024 19:52
Copy link
Collaborator

@whichqua whichqua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

) {
// A list of the contracts that accessed to the storage from 0x1 using `get_block_hash_syscall`
let special_addresses: Vec<ContractAddress> = vec![
contract_address!("0x01246c3031c5d0d1cf60a9370aac03a4717538f659e4a2bfb0f692e970e0c4b5"),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ftheirs Will these contract addresses apply on mainnet?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right! These addresses correspond to contracts deployed on Sepolia. For sure we'll have similar issues in mainnet

Copy link
Collaborator

@HermanObst HermanObst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there! Good work on this :)
I just added some questions as it seems the logic can be much simpler!

/// Inserts additional keys for retrieving storage proof from the block hash contract (address 0x1).
/// Certain contracts necessitate extra nodes from the contract 0x1. However, since Blockifier does not provide this information,
/// it is necessary to add some extra keys to ensure the inclusion of the required nodes.
/// This approach serves as a workaround. The ideal solutions would be to either retrieve the full tree or obtain information about the necessary nodes.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add:
obtain information about the accessed storage values (currently blockifier do not stores storage values accessed vie syscall: LINK to blockifier comment).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping on this

Comment on lines 133 to 136
100 * STORED_BLOCK_HASH_BUFFER
} else {
STORED_BLOCK_HASH_BUFFER
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

x100 multiplier fix everything (also Mainnet?)

// Include extra keys for contracts that trigger get_block_hash_syscall
insert_extra_storage_reads_keys(&storage_reads, old_block_number, &mut keys);

// Within the Starknet architecture, the address 0x1 is a special address that maps block numbers to their corresponding block hashes. As some contracts might access storage reads using `get_block_hash_syscall`, it is necessary to add some extra keys here.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As some contracts might access storage reads using get_block_hash_syscall and Blockifier currently do not stores these accessed values.

Comment on lines 170 to 175
let additional_storage_reads: Vec<Felt> = storage_reads
.values()
.flat_map(|vec| vec.iter().cloned().filter(|x| x <= &Felt252::from(block_number)))
.collect::<HashSet<Felt>>()
.into_iter()
.collect();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the rationale for this?
It seems that you are preventing to call storage values (of contract 0x1) that are also called in another contract tree.

Why just not get all the storage values that we will ask (we obtain this in insert_extra_storage_reads_keys, given if we need it or not) and just extend
keys.entry(block_hash_contract_address).or_default().extend(

It seems that we are adding, subtracting and doing some logic that could be much simpler.

Does it makes sense?

@ftheirs ftheirs force-pushed the ft/key_not_found_in_preimage branch 14 times, most recently from a81cac2 to 88d34bc Compare December 23, 2024 19:23
@ftheirs ftheirs force-pushed the ft/key_not_found_in_preimage branch from 88d34bc to 2cb5da5 Compare December 23, 2024 19:31
Copy link
Collaborator

@HermanObst HermanObst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@HermanObst HermanObst merged commit 3f5bbc6 into main Dec 27, 2024
5 checks passed
@HermanObst HermanObst deleted the ft/key_not_found_in_preimage branch December 27, 2024 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: Key not found in preimage
4 participants