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(cancun): correct search loop in transient storage #257

Merged
merged 2 commits into from
Jun 3, 2024

Conversation

Nashtare
Copy link
Collaborator

@Nashtare Nashtare commented Jun 2, 2024

The condition If pos'' > @GLOBAL_METADATA_TRANSIENT_STORAGE_LEN we need to update the length was always satisfied, because we were comparing the scaled pos with the unscaled len.

This PR also changes the length to be stored to be always scaled to reduce overhead in the loop, as well as fixes some stack comments / adds some more.

@Nashtare Nashtare added the bug Something isn't working label Jun 2, 2024
@Nashtare Nashtare added this to the Cancun - Q2 2024 milestone Jun 2, 2024
@Nashtare Nashtare requested a review from 4l0n50 June 2, 2024 17:48
@Nashtare Nashtare self-assigned this Jun 2, 2024
@github-actions github-actions bot added the crate: evm_arithmetization Anything related to the evm_arithmetization crate. label Jun 2, 2024
Copy link
Contributor

@4l0n50 4l0n50 left a comment

Choose a reason for hiding this comment

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

LGTM!

PUSH 1
SUB // 1 - seg
ADD // new_len = (addr - seg) + 1
// stack: pos, addr, original_value, slot, value, kexit_info
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to see if I understood, this part of the code was not correct (because pos was not considered part of the stack) but it never got executed because pos'' > @GLOBAL_METADATA_TRANSIENT_STORAGE_LEN was always false?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No the stack comment was wrong, but we were indeed using pos (as we should have) to update the length.
The stack before the conditional jump to this section is pos'' > len, pos'', addr, original_value, slot, value, kexit_info, effectively consuming the boolean on top and leaving pos'' as top of the stack.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I meant that the top of the stack was pos'' and not addr.

@Nashtare Nashtare merged commit 275c2dd into feat/cancun Jun 3, 2024
6 checks passed
@Nashtare Nashtare deleted the cancun/fix/transient branch June 3, 2024 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working crate: evm_arithmetization Anything related to the evm_arithmetization crate.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants