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
There was a problem hiding this comment.
Added to "Call contexts" section, thanks!
| - [ ] 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 |
There was a problem hiding this comment.
Was trying to convey "call made using CALLCODE opcode", but I think it's not clear enough.
I've changed it to CALLCODE, and also the rest of the entries to use the opcode name.
| - [ ] 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).
There was a problem hiding this comment.
Added:
Verify recursive contract creation using the opcode: Factory contract uses the opcode, and initcode calls back to factory contract
To the `Contract creation section below.
| - [ ] 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)
There was a problem hiding this comment.
I think https://github.com/ethereum/execution-apis has the correct setup to make this more deterministic.
In this repository we could try to use execute to make these tests deterministic.
| - [ ] 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.
| - [ ] Verify interesting edge values given the precompile functionality. | ||
| - [ ] If precompile performs cryptographic operations, verify behavior on all inputs that have special cryptographic properties. | ||
| - [ ] Verify all zeros input. | ||
| - [ ] Verify 2^N-1 where N is a single or multiple valid bit-lengths. |
There was a problem hiding this comment.
Maybe explicitly stating invalid input combinations for precompiles?
| - [ ] Verify 2^N-1 where N is a single or multiple valid bit-lengths. | |
| - [ ] Verify 2^N-1 where N is a single or multiple valid bit-lengths. | |
| - [ ] Verify combinations of invalid inputs to the precompile. | |
| - [ ] Inputs that fail specific mathematical or cryptographic validity checks. | |
| - [ ] Inputs that are malformed/corrupted. | |
| - [ ] Verify combinations of valid inputs to the system contract | ||
| - [ ] Verify interesting edge values given the system contract functionality. | ||
| - [ ] Verify all zeros input. | ||
| - [ ] Verify 2^N-1 where N is a single or multiple valid bit-lengths. |
There was a problem hiding this comment.
Maybe explicitly stating invalid input combinatinos for system contracts?
| - [ ] Verify 2^N-1 where N is a single or multiple valid bit-lengths. | |
| - [ ] Verify 2^N-1 where N is a single or multiple valid bit-lengths. | |
| - [ ] Verify combinations of invalid inputs to the precompile. | |
| - [ ] Inputs that fail specific validity checks. | |
| - [ ] Inputs that are malformed/corrupted. |
| - [ ] Modify emitted logs | ||
| - [ ] Fork transition | ||
| - [ ] Verify calling the system contract before its activation fork results in correct behavior (depends on the system contract implementation). | ||
|
|
There was a problem hiding this comment.
Maybe we could also add value transfer tests to system contracts:
- [ ] Value Transfer
- [ ] Verify calls to the system contract that include a non-zero ETH value
...
| - [ ] 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.
We could add value transfer tests to precompiles:
- [ ] Value Transfer
- [ ] Verify calls to the precompile that include a non-zero ETH value
- ...
| - [ ] RPC Tests | ||
| - [ ] * Verify `eth_estimateGas` behavior for different valid combinations of the new transaction type | ||
| - [ ] Verify `eth_sendRawTransaction` using `execute` | ||
|
|
There was a problem hiding this comment.
What about the following within a single block. The first 2 assume we can hardcode the block gas limit in the environment.
- [ ] Tx Block Interactions
- [ ] Verify that a transaction of the new type is rejected if its gas limit exceeds the block gas limit.
- [ ] Verify that a transaction of the new type is accepted if the gas limit equals the block gas limit.
- [ ] Verify that a block with all transactions types including the new type is executed correctly.
There was a problem hiding this comment.
This is a great suggestion, it reminded me of an important case missing from this checklist, will be adding π
|
|
||
| ### Framework Changes | ||
|
|
||
| **TBD** |
There was a problem hiding this comment.
Adding changes to execution payload if required.
|
|
||
| ### Framework Changes | ||
|
|
||
| **TBD** |
There was a problem hiding this comment.
Adding changes to new payload if required.
| ## General | ||
|
|
||
| - [ ] Code coverage | ||
| - [ ] Run produced tests against [EELS](https://github.com/ethereum/execution-specs) and verify that line code coverage of new added lines for the EIP is 100%, with only exeptions being unreachable code lines. |
There was a problem hiding this comment.
| - [ ] Run produced tests against [EELS](https://github.com/ethereum/execution-specs) and verify that line code coverage of new added lines for the EIP is 100%, with only exeptions being unreachable code lines. | |
| - [ ] Run produced tests against [EELS](https://github.com/ethereum/execution-specs) and verify that line code coverage of new added lines for the EIP is 100%, with only exeptions being unreachable code lines. | |
| - [ ] Run coverage on the test code itself (as a basic logic sanity check), i.e., `uv run fill --cov tests`. |
.markdownlint.yaml
Outdated
| MD024: false # no-duplicate-heading - We use duplicate headings in the changelog. | ||
| MD033: false # no-inline-html - Too strict. | ||
| MD007: | ||
| indent: 4 |
This is a great idea, I'm going to push to have this finished today and merged. |
Co-authored-by: danceratopz <danceratopz@gmail.com> Co-authored-by: spencer-tb <spencer@spencertaylorbrown.uk> Co-authored-by: Jochem Brouwer <jochembrouwer96@gmail.com>
|
@danceratopz @spencer-tb I think this one is ready to merge. Let me know what you think. |
ποΈ 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.