docs: Add EIP checklist templates #1327
Conversation
| - [ ] 31 bytes expansion | ||
| - [ ] 32 bytes expansion | ||
| - [ ] 33 bytes expansion | ||
| - [ ] 64 or more bytes expansion |
There was a problem hiding this comment.
- boundary test int_max +/- 1 bytes expansion. int64_max+/-1 bytes expansion.
such tests must OOG right away without trying to allocate the memory.
| - [ ] If the opcode pushes one or more items to the stack, and the opcode pushes more elements than it pops, verify that the opcode execution results in exeptional abort when pushing elements to the stack would result in the stack having more than 1024 elements. | ||
| - [ ] If the opcode pops one or more items to the stack, or it has a minimum stack height of one or more, verify that the opcode execution results in exeptional abort then stack has 1 less item than the minimum stack height expected. | ||
| - [ ] Execution context | ||
| - [ ] Normal call |
There was a problem hiding this comment.
- in create/create2 constructor.
- in transaction constructor.
There was a problem hiding this comment.
Thanks! This should be covered by lines 39-42.
| - [ ] Fork transition | ||
| - [ ] Verify that the opcode results in exeptional abort if executed before its activation fork. | ||
| - [ ] Verify that the opcode results in invalid EOF container if attempted to deploy before its activation fork. | ||
|
|
There was a problem hiding this comment.
I think we can automate some of this checks with something like test_scenarios. although I see it will become too complicated. So I suggest to make a vector of automatic checks.
lets say we have an opcode(x,y,z) we give it to a class that auto generates the tests for values of x,y,z and verifies the exceptions. not a fuzzing but like a verifiaction against static template of test inputs.
| - [ ] Fork transition | ||
| - [ ] Verify initial value of the field at the first block of the activation fork. | ||
| - [ ] Verify that a block containing the new header field before the activation of the fork is invalid. | ||
| - [ ] Verify that a block lacking the new header field at the activation of the fork is invalid. |
There was a problem hiding this comment.
if field is a list, verify the validity status of empty list, and max_allowed_size +/-1
| - [ ] Verify that the sender account of the new transaction type transaction has its nonce incremented by one after the transaction is included in a block | ||
| - [ ] Verify that the sender account of the new transaction type transaction has its balance reduced by the correct amount (gas consumed and value) after the transaction is included in a block | ||
| - [ ] Fork transition | ||
| - [ ] Verify that a block prior to fork activation where the new transaction type is introduced and containing the new transaction type is invalid. |
There was a problem hiding this comment.
same here if a new transaction type has a field which is a list, verify if it is allowed to have an empty_list, and if there is a limit on number of elements verify that it is rejected if limit is overflown
| - [ ] Fork transition | ||
| - [ ] Verify that calling the precompile before its activation fork results in a valid call even for inputs that are expected to be invalid for the precompile. | ||
| - [ ] Verify that calling the precompile before its activation fork with zero gas results in a valid call. | ||
|
|
There was a problem hiding this comment.
aha here then we need to verify that a new system contract can be executed by 7702 delegation. which I think shall be done automatically by our test framework
| - [ ] Execution context | ||
| - [ ] Normal call | ||
| - [ ] Static call | ||
| - [ ] Verify exeptional abort if the opcode attempts to modify the code, storage or balance of an account |
There was a problem hiding this comment.
In some cases opcodes are also banned if they do not do not modify code (for instance I think that PAY opcode is banned even if the value sent is 0). Another example is SSTORE where the current value is not modified (although this is more obvious that it should not happen in a STATIC context I think)
| - [ ] Normal call | ||
| - [ ] Static call | ||
| - [ ] Verify exeptional abort if the opcode attempts to modify the code, storage or balance of an account | ||
| - [ ] Delegate call |
There was a problem hiding this comment.
Also verify here that the STATICCALL context stacks. So STATIC -> DELEGATE should still run in static mode. (also applies to CALL/CALLCODE)
| - [ ] If the opcode modifies the storage of the account currently executing it, verify that only the account that is delegating execution is the one that has its storage modified. | ||
| - [ ] If the opcode modifies the balance of the account currently executing it, verify that only the account that is delegating execution is the one that has its balance modified. | ||
| - [ ] If the opcode modifies the code of the account currently executing it, verify that only the account that is delegating execution is the one that has its code modified. | ||
| - [ ] Code call |
| - [ ] Initcode | ||
| - [ ] Verify opcode behaves as expected when running during the initcode phase of contract creation | ||
| - [ ] Initcode of a contract creating transaction. | ||
| - [ ] Initcode of a contract creating opcode (including itself if opcode creates a contract). |
There was a problem hiding this comment.
So CREATE and CREATE2.
Also be aware of weird quirks like contract A doing CREATE2, then in this create context calling back into A, which then attempts to deploy exactly the same CREATE2 (so tries to deploy at same address. This is prevented because the nonce of the to-be-created address gets bumped to 1 prior to executing, and prior to trying to create a new address the nonce of the target address should be 0, so the second attempt will immediately fail).
| - [ ] EOF Container Context | ||
| - [ ] If opcode changes behavior depending on particular EOF container properties, test using multiple values for each property. | ||
| - [ ] Return data | ||
| - [ ] Verify proper return data buffer modification if the opcode is meant to interact with it, otherwise verify that the return data buffer is unnaffected |
There was a problem hiding this comment.
Nit: return data cannot be modified, it can only be overwritten
| - [ ] Verify the transaction (and the block it is included in) is valid by providing the exact intrinsic gas as `gas_limit` value to the transaction with all multiple combinations of values to the field. | ||
| - [ ] Verify the transaction (and the block it is included in) is invalid by providing the exact intrinsic gas minus one as `gas_limit` value to the transaction with all multiple combinations of values to the field. | ||
| - [ ] Encoding Tests (RLP, SSZ) | ||
| - [ ] Verify correct transaction rejection due to incorrect field sizes |
There was a problem hiding this comment.
Maybe to add: for a field, add leading 0s, this should invalidate the field. (In case of Address field this should either be 0/20 bytes (to: field of tx), so adding 0s will also invalidate it but now for a different reason, namely: length is wrong instead of leading zeros (or both, depends upon which order the client checks the errors))
| - [ ] Verify correct transaction rejection if the transaction type contains extra fields | ||
| - [ ] If the transaction contains fields with new serializable types, perform all previous tests on the new type/field | ||
| - [ ] RPC Tests | ||
| - [ ] * Verify `eth_estimateGas` behavior for different valid combinations of the new transaction type |
There was a problem hiding this comment.
How to test this? This depends upon the parent block it is being called on, and it will also depend upon what params are used in the block we are adding this tx (for instance: timestamp, could have a skipped block so this block is not T+12 but T+24)
| - [ ] Contract creation | ||
| - [ ] Verify that the transaction can create new contracts, or that it fails to do so if it's not allowed | ||
| - [ ] Sender account modifications | ||
| - [ ] Verify that the sender account of the new transaction type transaction has its nonce incremented by one after the transaction is included in a block |
There was a problem hiding this comment.
Nit: does not apply to 7702 if self-delegated since this bumps nonce by more than 1.
| - [ ] Verify that the transaction can create new contracts, or that it fails to do so if it's not allowed | ||
| - [ ] Sender account modifications | ||
| - [ ] Verify that the sender account of the new transaction type transaction has its nonce incremented by one after the transaction is included in a block | ||
| - [ ] Verify that the sender account of the new transaction type transaction has its balance reduced by the correct amount (gas consumed and value) after the transaction is included in a block |
There was a problem hiding this comment.
Also verify that the sender has enough balance to send the tx (intrinsic gas + value)
| - [ ] Add system contract address to relevant methods in the fork where the EIP is introduced in `src/ethereum_test_forks/forks/forks.py` | ||
| - [ ] Add system contract bytecode to the returned value of `pre_allocation_blockchain` in the fork where the EIP is introduced in `src/ethereum_test_forks/forks/forks.py` | ||
|
|
||
| ## New Transaction Type |
There was a problem hiding this comment.
Also tests regarding txs within the same block. For instance, the "warm addresses" and "warm slots" are cleared after a transaction (so does not stack to next transaction). Also things like Transient Storage (EIP 1153) should be cleared after each tx.
🗒️ Description
WIP
This PR aims to introduce EIP checklist templates that must be followed when implementing tests for a given EIP.
🔗 Related Issues
✅ Checklist
mkdocs servelocally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.