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

feat: added special address with block number and block hash #347

Merged
merged 12 commits into from
Oct 18, 2024

Conversation

Mohiiit
Copy link
Contributor

@Mohiiit Mohiiit commented Oct 15, 2024

Small fix for SNOS to run, it requires special address in the state diff

Pull Request type

Please add the labels corresponding to the type of changes your PR introduces:

  • Feature

What is the current behavior?

Currently we were not adding the block hash and block number of previous block with special address which was breaking the SNOS

Resolves: #NA

What is the new behavior?

  • The above mention issue is resolved

Does this introduce a breaking change?

No

@Mohiiit Mohiiit marked this pull request as ready for review October 15, 2024 18:50
}
Ok(None) => {
// this shouldn't happen, ideally should panic
log::error!("No block hash found for block number {}", prev_block_number);
Copy link
Contributor

Choose a reason for hiding this comment

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

let's panic ig?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

if block_n >= 10 {
let prev_block_number = block_n - 10;
match self.backend.get_block_hash(&BlockId::Number(prev_block_number)) {
Ok(Some(prev_block_hash)) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's add a comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

Comment on lines 266 to 267
block_time: Duration::from_secs(4),
pending_block_update_time: Duration::from_secs(1),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
block_time: Duration::from_secs(4),
pending_block_update_time: Duration::from_secs(1),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

@Mohiiit Mohiiit requested a review from apoorvsadana October 16, 2024 04:09
@antiyro antiyro requested a review from cchudant October 16, 2024 08:59
@jbcaron
Copy link
Member

jbcaron commented Oct 17, 2024

This logic for block hashes is already implemented for blockifier with the StateAdapter, it should be changed accordingly while keeping a compatibility of re-execution of the mainnet.

if *contract_address.key() == Felt::ONE {
let requested_block_number = felt_to_u64(key.0.key()).map_err(|_| StateError::OldBlockHashNotProvided)?;
// Not found if in the last 10 blocks.
if !block_hash_storage_check_range(
&self.backend.chain_config().chain_id,
self.block_number,
requested_block_number,
) {
return Ok(Felt::ZERO);
}
return self
.backend
.get_block_hash(&BlockId::Number(requested_block_number))
.map_err(|err| {
log::warn!("Failed to retrieve block hash for block number {requested_block_number}: {err:#}");
StateError::StateReadError(format!(
"Failed to retrieve block hash for block number {requested_block_number}",
))
})?
.ok_or(StateError::OldBlockHashNotProvided);
}

@apoorvsadana
Copy link
Contributor

@jbcaron, I am mostly missing something but do we need this custom logic in the reader? The feeder gateway does return state diffs where it says 0x1 has the following storage keys and values. In full node mode, if we are applying these state diffs in the DB, won't normal fetching from DB work same as this custom logic we're using here?

@jbcaron
Copy link
Member

jbcaron commented Oct 17, 2024

we should just delete this code in the same PR but be sure that we can re-execute the mainnet blocks (I don't know from when this feature was introduced)
I checked on sepolia, there is no issue

@apoorvsadana
Copy link
Contributor

apoorvsadana commented Oct 17, 2024

we should just delete this code in the same PR but be sure that we can re-execute the mainnet blocks

makes sense, we can delete and see if the test cases pass + that we can resync a few hundred blocks on mainnet + get_storage_at on 0x1 address works as expected

EDIT: @Mohiiit has also already informed the visoft team for e2e cases for this specific case

Trantorian1
Trantorian1 previously approved these changes Oct 17, 2024
@Trantorian1 Trantorian1 self-requested a review October 17, 2024 06:44
@Trantorian1 Trantorian1 dismissed their stale review October 17, 2024 06:45

Cannot merge now

// at address 0x1 with the block number as the key
if block_n >= 10 {
let prev_block_number = block_n - 10;
match self.backend.get_block_hash(&BlockId::Number(prev_block_number)) {
Copy link
Member

Choose a reason for hiding this comment

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

this match can be simplified to

let prev_block_hash = self.backend.get_block_hash(&BlockId::Number(prev_block_number))
  .map_err(|err| Error::Unexpected(
      format!("Error fetching block hash for block {prev_block_number}: {err:#}").into()
  ))?
  .ok_or_else(|| Error::Unexpected(
      format!("No block hash found for block number {prev_block_number}").into()
  ))?;
let address = Felt::ONE;
new_state_diff.storage_diffs.push(ContractStorageDiffItem {
    address,
    storage_entries: vec![StorageEntry { key: Felt::from(block_n), value: prev_block_hash }],
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

.backend
.get_contract_class_hash_at(&block_id, &contract_address)
.or_internal_server_error("Failed to check if contract is deployed")?
.ok_or(StarknetRpcApiError::ContractNotFound)?;
Copy link
Member

Choose a reason for hiding this comment

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

I think we should only return 0x0 in this case
we can therefore remove this check
this is already the implementation in the state adapter but maybe this logic should be in the db
@cchudant what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked with the juno rpc call and it has this behaviour hence implemented this

Copy link
Member

Choose a reason for hiding this comment

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

apparently you have to return ContractNotFound
https://github.com/starkware-libs/starknet-specs/blob/master/api/starknet_api_openrpc.json#L216

but we need to bring this up into full node collab channel - getNonce also returns ContractNotFound but I think it's possible to increment the nonce of an undeclared contract

@antiyro antiyro merged commit ab98e51 into main Oct 18, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants