Skip to content

Conversation

@jsign
Copy link
Collaborator

@jsign jsign commented Oct 16, 2024

This PR is solving spec bugs found when @tanishqjasoria is running tentative v0.0.5 fixture set.

Comment on lines +355 to +356
isSystemContract := interpreter.evm.isSystemContract(address)
if _, isPrecompile := interpreter.evm.precompile(address); isPrecompile || isSystemContract {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image

Comment on lines -410 to +412
if _, isPrecompile := interpreter.evm.precompile(addr); isPrecompile {
isSystemContract := interpreter.evm.isSystemContract(addr)
if _, isPrecompile := interpreter.evm.precompile(addr); isPrecompile || isSystemContract {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ditto.

@jsign jsign requested a review from gballet October 16, 2024 13:16
@jsign jsign marked this pull request as ready for review October 16, 2024 13:16
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.

most of it is fine, but I believe that the write-touching of the code hash should be distinct from whether or not there is an account at the location.

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.

As I said in dm, we should rename and always set the write mode to true since there is no gas difference. This way, it'll be covered by the 21000. Just please add a comment so that we don't "fix" it later when rebasing.

Signed-off-by: Ignacio Hagopian <[email protected]>
@jsign jsign merged commit 1ec805c into jsign-witness-fix Oct 16, 2024
3 of 6 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.

4 participants