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(op): parse l1 block info from l2 tx input #10664

Merged
merged 8 commits into from
Sep 4, 2024
Merged

Conversation

emhane
Copy link
Member

@emhane emhane commented Sep 2, 2024

Closes #10462

Fixes offsets for parsing l1 base fee scalar and l1 blob base fee scalar

@emhane emhane added C-bug An unexpected or incorrect behavior A-rpc Related to the RPC implementation A-op-reth Related to Optimism and op-reth labels Sep 2, 2024
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

the offsets were wrong because of the selector is already removed here?
but doesn't this ignore the block sequencer number as described in the function docs

@clabby mind taking a look at this?

@mattsse
Copy link
Collaborator

mattsse commented Sep 2, 2024

@emhane
Copy link
Member Author

emhane commented Sep 2, 2024

the offsets were wrong because of the selector is already removed here?

no they were wrong despite that, base fee scalar was before taken at offset 12..16, when actually it's at offset 0..4. the selector is only 4 bytes.

got the correct order from op-geth code, as linked in the code comment https://github.com/ethereum-optimism/op-geth/blob/60038121c7571a59875ff9ed7679c48c9f73405d/core/types/rollup_cost.go#L317-L328

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

okay, this seems correct, can we update the function docs, because the sequence number is incorrect there

doc nits, pending sanity check @clabby or @refcell

crates/optimism/evm/src/l1.rs Show resolved Hide resolved
@tynes
Copy link

tynes commented Sep 3, 2024

This looks good to me given the above comments are addressed

Copy link
Collaborator

@clabby clabby left a comment

Choose a reason for hiding this comment

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

LGTM - looks like the old test for Ecotone also needs to be changed.

@emhane emhane requested a review from mattsse September 4, 2024 13:20
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

the function docs need to be updated

L111 is incorrect and should be removed

@emhane emhane enabled auto-merge September 4, 2024 14:01
@emhane emhane added this pull request to the merge queue Sep 4, 2024
Merged via the queue into main with commit 938227a Sep 4, 2024
34 checks passed
@emhane emhane deleted the emhane/op-l1-block-info branch September 4, 2024 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-op-reth Related to Optimism and op-reth A-rpc Related to the RPC implementation C-bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

op-reth outputs bad data for "eth_getTransactionReceipt" method
4 participants