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

statedb: avoid getting code from cache #480

Merged
merged 1 commit into from
Sep 2, 2024
Merged

Conversation

jsign
Copy link
Collaborator

@jsign jsign commented Sep 2, 2024

This PR fixes a bug discovered while debugging a test fixture.

if err := s.trie.UpdateAccount(addr, &obj.data, len(obj.code)); err != nil {
if err := s.trie.UpdateAccount(addr, &obj.data, len(s.GetCode(addr))); err != nil {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem appeared in the dumped state for the 2935 contract.
In the post-state, the codeLen encoded in basicData was 0 which made no sense.

After looking, the problem is that obj.code was nil since technically the EVM never called GetCode(...). GetCode(...) actually fills obj.code as a cache.

Since we were accessing the cache directly, we were re-encoding basicdata with length 0 which was wrong.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Normal contract would have already call GetCode(...) in the EVM, so this extra call will simply return it from the cache.

But this will cover for system contracts (like 2935) which update from storage directly as an optimization.

@jsign jsign marked this pull request as ready for review September 2, 2024 15:54
@jsign jsign requested a review from gballet September 2, 2024 15:55
Copy link
Owner

@gballet gballet left a comment

Choose a reason for hiding this comment

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

Yeah, I had a feeling this was going to be an issue, which is why I went the route of setting it in update contract code. Should be fixed now, so hopefully that's the end of that story

@jsign
Copy link
Collaborator Author

jsign commented Sep 2, 2024

Note that the consumption of the stable fixtures fails as expected. Remember the reason the job is green is because the consume direct doesn't return -1 yet. It will be fixed in the testing library next release.

@jsign jsign merged commit 47ef45f into kaustinen-with-shapella Sep 2, 2024
12 of 13 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.

2 participants