-
Notifications
You must be signed in to change notification settings - Fork 390
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
selfdestruct
in initcode should destroy
#5913
selfdestruct
in initcode should destroy
#5913
Conversation
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.
can you elaborate what selfdestruct
in initcode should destroy? and in this context do we only mean initcode as the argument to CREATE/CREATE2 or CreateTx as well ?
@Demuirgos so basically, selfdestruct in the initcode should act how it acted before the EIP. so adding the contract for destruction |
edc59f4
to
3505d6f
Compare
@Demuirgos for create transaction no need to worry about destroy, it results in the same output (no contract created) |
selfdestruct
in initcode should destoryselfdestruct
in initcode should destroy
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.
Added a few comments
[Parallelizable(ParallelScope.All)] | ||
public class Eip6780Tests : BlockchainTestBase | ||
{ | ||
//[TestCaseSource(nameof(LoadTests))] |
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.
Why commented?
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.
Tests are not merged yet.
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.
Also re-reviewing original PR made me find this which is weird: https://github.com/NethermindEth/nethermind/pull/4704/files#r1258001674
|
||
private void AssertNotDestroyed() | ||
{ | ||
AssertCodeHash(_contractAddress, Keccak.Compute(_selfDestructCode.AsSpan())); |
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.
why doesn't this do the opposite of AssertDestroyed
?
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.
Because with this call, i can make sure the account has the keccak of the deployed code. but I am not sure how useful that is.
Fixes Ethereum tests
Changes
selfdestruct
in initcode should destoryTypes of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Documentation
Requires documentation update
Requires explanation in Release Notes