-
Notifications
You must be signed in to change notification settings - Fork 14
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
CT: no code stop effect #958
base: main
Are you sure you want to change the base?
Conversation
go/ct/spc/specification.go
Outdated
@@ -126,7 +130,6 @@ func getAllRules() []Rule { | |||
), | |||
Effect: Change(func(s *st.State) { | |||
s.Status = st.Stopped | |||
s.ReturnData = Bytes{} | |||
s.Pc++ |
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.
This is surprising - when running into a "STOP" instruction -- explicit or implicit -- the return data should be empty.
I think what is happening is this:
- in all implementations the (implicit) return data slice is always empty until a
RETURN
or aREVERT
is reached. Only those two instructions are setting the return data to something that is not empty. - in all implementations, processing a
STOP
thus does not need to explicitly set the return data to the empty data, since it is empty throughout the contract execution - when we create a test state with the CT we will the return data with random values; those are now preserved by all implementations when running the
STOP
instruction, and it looks likeSTOP
is not supposed to have an effect on theReturnData
-- while it should, by making sure the return data is empty; codes terminated by an explicit or implicitSTOP
must return no return data
So it looks like the root of the problem is actually the fact that we produce states with random return data. This change is "covering" this up, by -- what looks like -- an invalid specification. Instead I think the state generation should be updated to only produce empty return data. Wdyt?
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.
from https://www.evm.codes/:
STOP: When a call is executed on an address with no code and the EVM tries to read the code data, the default value is returned, 0, which corresponds to this instruction and halts the execution.
I think this is exactly what Simon describes: a call into a no-code address.
can we interpret here that 0 needs to be returned?
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.
I also think the return data should not be set in the state generation as it is never read and only set by RETURN/REVERT opcodes.
The random return data is removed from the state generation and the regression input.
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.
@LuisPH3 just pointed out that this makes sense for the RETURN/REVERT instructions but does not make sense for the RETURNDATACOPY/RETURNDATASIZE instructions. A possible solution would be a special case in the state generation but I don't think this is the most elegant solution.
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.
Looks like this PR contains changes also covered by other PRs. Will approve once those are merged.
81b0e90
to
840a34c
Compare
840a34c
to
fbb4921
Compare
fbb4921
to
f56ec55
Compare
if resultStatus == st.Stopped || resultStatus == st.Reverted { | ||
resultReturnData = RandomBytes(rnd, st.MaxDataSize) | ||
} | ||
|
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.
As discussed, I have a concern about removing the generation of resutlReturnData.
The returned data is used in two different contexes:
- As write buffer for a "callee" contract, used by the RETURN op. This buffer cannot be accessed by the contract again, as the execution terminates right away. Regarding this OP, we can remove the buffer generation
- As read buffer for the "caller" contract, accessed via RETURNDATASIZE and RETURNDATACOPY.
The later is a state that would require the generation of a returned data buffer. Right?
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.
In the specification, the data returned by a call and the data returned by the current contract execution are stored in different fields of the state (see here).
For instance, if you look into the specification of the RETURNDATASIZE here, it is reading the size of the state's LastCallReturnData
field.
The fact that all implementations use the same field for the last call return data and the return data of the current call is a (valid) optimization, but not reflected in the specification.
golang.org/x/sys v0.20.0 // indirect | ||
golang.org/x/text v0.14.0 // indirect | ||
google.golang.org/protobuf v1.34.2 // indirect | ||
rsc.io/tmplfunc v0.0.3 // indirect | ||
) | ||
|
||
replace github.com/ethereum/go-ethereum => github.com/Fantom-foundation/go-ethereum-sonic v0.0.0-20240916105249-e7951db0c00b | ||
replace github.com/ethereum/go-ethereum => github.com/Fantom-foundation/go-ethereum-sonic v0.0.0-20241210083612-373029913b2e |
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.
Just to be sure: please update this once the corresponding go-ethereum-sonic change is merged to make sure the version number points to a commit on a non-development branch of go-ethereum-sonic
.
A CT full mode run failed with an empty code and return data. After some discussion, the geth integration has been changed to STOP if empty code is submitted in the stepN function, see #13.
The failed state has non empty return data which lead to the CT still failing. The stop rule's effect resets the return data, this is not the case for any of the interpreters, therefore it has been removed in this PR.