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(forge): account access cheatcode accounts for extcode* and balance opcodes #6545

Merged
merged 3 commits into from
Dec 10, 2023

Conversation

emo-eth
Copy link
Contributor

@emo-eth emo-eth commented Dec 8, 2023

Motivation

vm.startStateDiffRecording should track the BALANCE and EXTCODE* family of opcodes, since they affect account warmth

Closes #6544

Solution

Adds 4 more values to the AccountAccessKind enum and watches for those 4 opcodes as part of step.

Copy link
Member

@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.

this looks reasonable.

this mimics the existing opcode handling, tests look good.

pending @Evalir @DaniPopes

Copy link
Contributor

@Inphi Inphi left a comment

Choose a reason for hiding this comment

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

Typically, the AccountAccessKinds indicate an EVM context switch. The Extcode* opcodes breaks this expectation. What do you think about indicating them in the StorageAccess struct? This way, all extcode* accesses are grouped under the current context.

@emo-eth
Copy link
Contributor Author

emo-eth commented Dec 8, 2023

Typically, the AccountAccessKinds indicate an EVM context switch. The Extcode* opcodes breaks this expectation. What do you think about indicating them in the StorageAccess struct? This way, all extcode* accesses are grouped under the current context.

How would you modify StorageAccess to accommodate? Could add a new extcode enum to the end and rename it something like StorageOrExtcodeAccess, but imo that just makes the struct more confusing.

@Inphi
Copy link
Contributor

Inphi commented Dec 8, 2023

Typically, the AccountAccessKinds indicate an EVM context switch. The Extcode* opcodes breaks this expectation. What do you think about indicating them in the StorageAccess struct? This way, all extcode* accesses are grouped under the current context.

How would you modify StorageAccess to accommodate? Could add a new extcode enum to the end and rename it something like StorageOrExtcodeAccess, but imo that just makes the struct more confusing.

You're right, that would be more confusing. I guess this is fine as-is then. With the Resume access kind, we probably could even flatten both storage and account accesses in a single array. As it's now possible to link contexts.
@JuanCoRo care to chime in, as I think you may have a unique usecase for this cheat code.

@emo-eth emo-eth changed the title feat(forge): account access cheatcode accounts for ext* opcodes feat(forge): account access cheatcode accounts for extcode* and balance opcodes Dec 8, 2023
@emo-eth
Copy link
Contributor Author

emo-eth commented Dec 8, 2023

Update to include balance opcode as well, since that marks the target account as warm. I think that's all of them, for real this time – any I might be missing?

@JuanCoRo
Copy link

JuanCoRo commented Dec 9, 2023

@Inphi thanks for the ping! This looks good to me, I don't think it will be any trouble for what I'm using it 👌

Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

LGTM

@DaniPopes DaniPopes merged commit e972bf7 into foundry-rs:master Dec 10, 2023
19 checks passed
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.

vm.startStateDiffRecording should record EXTCODE* and BALANCE opcodes as account accesses
6 participants