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.

Sign up for GitHub

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

Jump to bottom

Refactor evm recursion lock #2433

Merged
merged 1 commit into from
Aug 24, 2023
Merged

Refactor evm recursion lock #2433

merged 1 commit into from
Aug 24, 2023

Conversation

librelois
Copy link
Collaborator

What does it do?

Refactor evm recursion lock to avoid race conditions (may happen only in native mode, but ti's problematic for tests)

What important points reviewers should know?

Is there something left for follow-up PRs?

What alternative implementations were considered?

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

What value does it bring to the blockchain users?

@librelois librelois added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. not-breaking Does not need to be mentioned in breaking changes labels Aug 14, 2023
@github-actions
Copy link
Contributor

Coverage generated "Mon Aug 14 14:56:41 UTC 2023":
https://d3ifz9vhxc2wtb.cloudfront.net/pulls/2433/html/index.html

Master coverage: 87.39%
Pull coverage:

@notlesh
Copy link
Contributor

notlesh commented Aug 14, 2023

What was the point of the refactor?

Changes, FYI: moonbeam-foundation/frontier@ee898e9

@librelois
Copy link
Collaborator Author

What was the point of the refactor?

Fix a race condition bug that occur randomly in ts tests (can occur only in native mode), for more details, see MOON-2209

@librelois librelois marked this pull request as ready for review August 21, 2023 13:22
@librelois librelois requested a review from notlesh August 21, 2023 14:51
crystalin
crystalin approved these changes Aug 21, 2023
View reviewed changes
Copy link
Collaborator

@crystalin crystalin left a comment

Choose a reason for hiding this comment

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

I checked the frontier commit moonbeam-foundation/frontier@ee898e9

@notlesh
Copy link
Contributor

notlesh commented Aug 21, 2023 •
edited
Loading

The main change here seems to be deferring the *in_evm = false via sp_core::defer!. Is it possible that this deferral could be triggered at an incorrect time and allow for recursion when it shouldn't?

Edit: It seems like it shouldn't, this is just using a Drop impl.

notlesh
notlesh approved these changes Aug 21, 2023
View reviewed changes
Copy link
Contributor

@notlesh notlesh left a comment

Choose a reason for hiding this comment

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

Approved. The new code seems good to me, but I'm not sure about the old code -- do you know why it was previously failing sometimes?

@noandrea
Copy link
Collaborator

@librelois can you merge this one?

@librelois librelois merged commit 52ad62c into master Aug 24, 2023
32 checks passed
@librelois librelois deleted the elois-forbid-evm-reentrancy-fix branch August 24, 2023 13:34
grw-ms pushed a commit that referenced this pull request Aug 28, 2023
@librelois @grw-ms
update frontier pin (#2433)
80d226b
@noandrea noandrea changed the title refactor evm recursion lock Refactor evm recursion lock Aug 31, 2023
@notlesh notlesh added D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@notlesh notlesh notlesh approved these changes

@crystalin crystalin crystalin approved these changes

Assignees
No one assigned
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited not-breaking Does not need to be mentioned in breaking changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@librelois @notlesh @noandrea @crystalin