Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Ok also added extensive tests here, so I am pretty sure that we are correct here as well 😄 |
holgerd77
left a comment
holgerd77
left a comment
There was a problem hiding this comment.
Hi Jochem,
had a first look here, thanks a lot for taking on these two additional EIPs as well, you are really our hero atm! 😄 Have some comments, generally the whole PR looks great though with these extensive tests and everything.
| } | ||
| } | ||
|
|
||
| export function EIP3541ViolationResult(gasLimit: BN): ExecResult { |
There was a problem hiding this comment.
Maybe I am missing something, but where is this function actually called? 😜
| INVALID_BEGINSUB = 'invalid BEGINSUB', | ||
| INVALID_RETURNSUB = 'invalid RETURNSUB', | ||
| INVALID_JUMPSUB = 'invalid JUMPSUB', | ||
| EIP3541_VIOLATION = 'eip3541 violation: cannot put contracts starting with 0xEF bytecode', |
There was a problem hiding this comment.
Maybe we can use something more semantically expressive here - a bit analog to the last discussions we had coming to the conclusion to rather avoid EIP numbers for names? 🤔
Suggestion (not sure yet though):
RESTRICTED_STARTING_BYTECODE
Or similar.
There was a problem hiding this comment.
(same a bit for the function name from above)
There was a problem hiding this comment.
In some place seen INVALID_CODE (geth) or INVALID_BYTECODE, and in EVMC we also had CONTRACT_VALIDATION_FAILED from earlier, that may be also a good one in the future.
There was a problem hiding this comment.
Ah yeah, that's better, my suggestion is likely still to specific, I like INVALID_BYTECODE. 😄
There was a problem hiding this comment.
But CONTRACT_VALIDATION_FAILED might be a bit better targeted, so eventually more suited.
There was a problem hiding this comment.
Probably INVALID_CODE/INVALID_BYTECODE seems to be the best for now, and if any "real validation" is rolled out later (such as EIP-3540), then the CONTRACT_VALIDATION_FAILED error could be introduced in addition.
There was a problem hiding this comment.
I personally like INVALID_BYTECODE.
| st.end() | ||
| }) | ||
|
|
||
| t.test('should not deploy contracts starting with 0xEF using CREATE', async (st) => { |
There was a problem hiding this comment.
These header are not matching the test cases applied with both the success and failure cases, maybe just drop the "should not" here (and other occurrences where it applies)
|
Updated and rebased. Ready for re-review @holgerd77 @ryanio |
holgerd77
left a comment
holgerd77
left a comment
There was a problem hiding this comment.
Yeah, really nice, looks good to me. 🙂
|
|
||
| code = await vm.stateManager.getContractCode(created!) | ||
|
|
||
| st.ok(code.length > 0, 'did deposit code') |
| st.ok(code.length > 0, 'did deposit code') | ||
|
|
||
| st.end() | ||
| }) |
There was a problem hiding this comment.
Important counter case (with all the last 1559 experiences 😛 ).
|
Could you run ethereumjs no this PR by any chance to confirm everything is in order: ethereum/tests#835 ? |
|
@axic Yup will do 😄 |
|
@jochem-brouwer @axic I am on it, see #1247 |
|
@axic @jochem-brouwer Ok, I am through (see comments on linked PR), two test runs which might need some further investigation:
This might either hint to the test cases being non-testing things but alternatively also to an error in our implementation (we might apply the EIP-3541 condition to a pre-EIP-3541 state). Likely need to stop for now and pass this on. |
3 participants
Implements EIP3541.
Verify that if we deploy 0xEF on 3541 activated commons we should consume all gas (this is most likely the case).(confirmed on ACD)