-
Notifications
You must be signed in to change notification settings - Fork 152
core, params: Increase Stylus smart contract size limit via merge-on-activate #605
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
Conversation
bbb6e35 to
a95d471
Compare
|
@pmikolajczyk41 ready for another look 🫡 |
pmikolajczyk41
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good stuff 👌
core/state/statedb_arbitrum.go
Outdated
| } | ||
|
|
||
| // checks if a valid Stylus prefix is present | ||
| func IsStylusProgram(b []byte) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the IsStylusProgram function and replace it with something with a different name, that way, anywhere that called IsStylusProgram in the previous code will pop out and we can see what needs to be changed.
specifically - in go-ethereum/core/vm/instructions.go: opSelfdestruct6780 - we must not allow self-destruct of any stylus componenet (program / fragment / root)
|
@tsahee I believe I resolved your concern, ready for another look 🧐 |
core/vm/evm.go
Outdated
| if len(ret) >= 1 && ret[0] == 0xEF && evm.chainRules.IsLondon { | ||
| // Arbitrum: retain Stylus programs and instead store them in the DB alongside normal EVM bytecode. | ||
| if !(evm.chainRules.IsStylus && state.IsStylusProgram(ret)) { | ||
| if !(evm.chainRules.IsStylus && state.IsStylusComponentPrefix(ret)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we should actually have different behavior per arbos version.
possibly, the best way is to add arbosVersion to IsStylusComponentPrefix function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: I think this is the only place where arbos-version dependency is a must. Other places only affect deployed code, which will not be possible for a stylus program not supported by current arbos.. but if not too complicated it's probably more elegant to add arbos-version dependency as well
|
@tsahee ready for another look, I believe I resolved your concern, and I also added some regression tests for safe keeping |
|
|
||
| // IsStylusDeployableProgramPrefix reports whether the bytecode starts with a | ||
| // deployable Stylus program prefix (classic program or root program). | ||
| func IsStylusDeployableProgramPrefix(b []byte) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for completeness, I think this one should also accept arbos version
Pulled in by OffchainLabs/nitro#4193