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

fix: missing code read in state write #699

Merged
merged 4 commits into from
Oct 4, 2024
Merged

fix: missing code read in state write #699

merged 4 commits into from
Oct 4, 2024

Conversation

Nashtare
Copy link
Collaborator

@Nashtare Nashtare commented Oct 4, 2024

We're currently erroring when missing an entry in the Hash2Code map. The access being done upon state_read in a state write, I'm wondering whether we should always expect the client to provide necessary information, or if we should allow missing entries.

In the current state of things, block 1034, txn 0 (0xcfaf63b718a548069b856e8ff6b27ad69c3e8ca84ae2ab6ae67c545ecf0b642c) is failing to parse at the decoder stage because we're missing contract bytecode associated to this txn trace:

            "0xd18e94fa1353e8b3b72e64ffc4a64d28a98cdee1": {
              "code_usage": {
                "read": "0x4ab15219b84093c7fd1c51566743820f9383a50b34c3f302b5331f26a89ddc73"
              }
            },

However, the prover does not need it and processes the txn payload fine with the change on this branch. I think Jerigon here knows we can skip the actual bytecode, and hence isn't passing it to the compact witness (@cffls to confirm). In which case we should not bail! when failing to find the mapping bytecode.

Opening as discussion because I'm not clear yet as to what the assumption should be / who's responsability it should be.
Passing the block payload here (note that it's failing to run further ahead)
b1034_dev.json

3.5/N @praetoriansentry

@Nashtare Nashtare self-assigned this Oct 4, 2024
@github-actions github-actions bot added the crate: trace_decoder Anything related to the trace_decoder crate. label Oct 4, 2024
@Nashtare Nashtare marked this pull request as ready for review October 4, 2024 00:24
Copy link
Contributor

@0xaatif 0xaatif left a comment

Choose a reason for hiding this comment

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

I think allow_missing_code should be a parameter to middle, not a property of Hash2Code.

But I don't think we want it at all:

// TODO(Nashtare): https://github.com/0xPolygonZero/zk_evm/issues/...
//                 This is a bug in the zero tracer, which shouldn't be giving us this read at all.
//                 Workaround for now.
if let Some(code) = code.get(hash) {
    batch_contract_code.insert(code)
}

@Nashtare
Copy link
Collaborator Author

Nashtare commented Oct 4, 2024

But I don't think we want it at all

Don't we want to at least disable this edge case for the native tracer?

@0xaatif
Copy link
Contributor

0xaatif commented Oct 4, 2024

Don't we want to at least disable this edge case for the native tracer?

Missed this from our Slack discussion :D

Makes sense, let's do, and document this our choice at the call site.

pub struct FatalMissingCode(pub bool);

fn middle(..., fatal_missing_code: FatalMissingCode) {..}
// TODO(Nashtare): https://github.com/0xPolygonZero/zk_evm/issues/...
//                 This is a bug in the zero tracer, which shouldn't be giving us this read at all.
//                 Workaround for now.
match (fatal_missing_code, code.get(hash)) {
    (FatalMissingCode(true), None) => bail!("no code for hash {hash}"),
    (_, Some(it)) => batch_contract_code.insert(code),
    (_, None) => warn!(%hash, "no code for hash"),
}

This reverts commit 3a750e9.
Co-authored-by: 0xaatif <aatifsyedyp+polygon@gmail.com>
Copy link
Contributor

@0xaatif 0xaatif left a comment

Choose a reason for hiding this comment

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

love it!

Comment on lines +44 to +47
let fatal_missing_code = match trie_pre_images {
BlockTraceTriePreImages::Separate(_) => FatalMissingCode(true),
BlockTraceTriePreImages::Combined(_) => FatalMissingCode(false),
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This is my first time realising that native = separate, and compact = jerigon

@Nashtare Nashtare changed the title discuss: missing code read in state write fix: missing code read in state write Oct 4, 2024
@Nashtare Nashtare merged commit 038457a into develop Oct 4, 2024
20 checks passed
@Nashtare Nashtare deleted the fix/code_read branch October 4, 2024 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate: trace_decoder Anything related to the trace_decoder crate.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants