pyevmasm, tests: Add support for multiple forks. #22
Conversation
|
Hi @dguido , @computerality before the review starts I would like to clarify that I introduced three instruction tables, one per each fork. It may look a bit redundant instead of patching pre-byzantium table, but might be error-prone in the future where there might be changes in gas costs, params etc. and I decided to implement this way. Feel free to modify it later if needed. |
pyevmasm/__main__.py
Outdated
| args = parser.parse_args(sys.argv[1:]) | ||
|
|
||
| accepted_forks = ['pre-byzantium', 'byzantium', 'constantinople'] | ||
| arg_fork = args.fork[0].lower() |
There was a problem hiding this comment.
| arg_fork = args.fork[0].lower() | |
| arg_fork = args.fork.lower() |
There was a problem hiding this comment.
Hi @disconnect3d that was my original implementation ;) When you set argparse required argument parameters to 1, it becomes an array.
pyevmasm/__main__.py
Outdated
| help='Input file, default=stdin') | ||
| parser.add_argument('-o', '--output', nargs='?', default=sys.stdout, type=argparse.FileType('w'), | ||
| help='Output file, default=stdout') | ||
| parser.add_argument('-f', '--fork', nargs=1, default=DEFAULT_FORK, type=str, |
There was a problem hiding this comment.
| parser.add_argument('-f', '--fork', nargs=1, default=DEFAULT_FORK, type=str, | |
| parser.add_argument('-f', '--fork', default=DEFAULT_FORK, type=str, |
There was a problem hiding this comment.
I assume there is no reason to specify a list of forks. When nargs=1 is not passed, args.fork will return a string instead of a single item list with a string.
There was a problem hiding this comment.
Ok thanks. I will change it back to '?' (see my comment above).
| 0x19: ('NOT', 0, 1, 1, 3, 'Bitwise NOT operation.'), | ||
| 0x1a: ('BYTE', 0, 2, 1, 3, 'Retrieve single byte from word.'), | ||
| 0x20: ('SHA3', 0, 2, 1, 30, 'Compute Keccak-256 hash.'), | ||
| 0x30: ('ADDRESS', 0, 0, 1, 2, 'Get address of currently executing account .'), |
There was a problem hiding this comment.
| 0x30: ('ADDRESS', 0, 0, 1, 2, 'Get address of currently executing account .'), | |
| 0x30: ('ADDRESS', 0, 0, 1, 2, 'Get address of currently executing account.'), |
| 0x3a: ('GASPRICE', 0, 0, 1, 2, 'Get price of gas in current environment.'), | ||
| 0x3b: ('EXTCODESIZE', 0, 1, 1, 20, "Get size of an account's code."), | ||
| 0x3c: ('EXTCODECOPY', 0, 4, 0, 20, "Copy an account's code to memory."), | ||
| 0x3d: ('RETURNDATASIZE', 0, 0, 1, 2, 'Get size of output data from the previous call from the current environment'), |
There was a problem hiding this comment.
| 0x3d: ('RETURNDATASIZE', 0, 0, 1, 2, 'Get size of output data from the previous call from the current environment'), | |
| 0x3d: ('RETURNDATASIZE', 0, 0, 1, 2, 'Get size of output data from the previous call from the current environment.'), |
| 0x3b: ('EXTCODESIZE', 0, 1, 1, 20, "Get size of an account's code."), | ||
| 0x3c: ('EXTCODECOPY', 0, 4, 0, 20, "Copy an account's code to memory."), | ||
| 0x3d: ('RETURNDATASIZE', 0, 0, 1, 2, 'Get size of output data from the previous call from the current environment'), | ||
| 0x3e: ('RETURNDATACOPY', 0, 3, 0, 3, 'Copy output data from the previous call to memory'), |
There was a problem hiding this comment.
| 0x3e: ('RETURNDATACOPY', 0, 3, 0, 3, 'Copy output data from the previous call to memory'), | |
| 0x3e: ('RETURNDATACOPY', 0, 3, 0, 3, 'Copy output data from the previous call to memory.'), |
There was a problem hiding this comment.
+3 / I am wondering where did it came from...
tests/test_EVMAssembler.py
Outdated
| insn = EVMAsm.disassemble_one(b'\x1d', fork='byzantium') | ||
| self.assertTrue(insn.mnemonic == 'INVALID') | ||
| insn = EVMAsm.disassemble_one(b'\xf5', fork='byzantium') | ||
| self.assertTrue(insn.mnemonic == 'INVALID') |
There was a problem hiding this comment.
Can we add comments like in # REVERT added in byzantium?
|
Hi @disconnect3d . Thank you for the detailed review. I will go back to the PR tomorrow morning. As for the whitespaces, I usually use PyCharm with 'show whitespaces' enabled, this time I used Visual Studio Code and apparently I did not have the option enabled. |
cryptomental
force-pushed
the
feature/add-forks-support
branch
6 times, most recently
from
a090512 to
7c04cb5
Compare
|
Hi @disconnect3d I addressed all findings, updated test cases as advised and amended the pull request. Tests still pass. ;) The pull request is ready for another round of the review. |
There was a problem hiding this comment.
Hey, can you also fix the occurrences of instruction_table in docstring/readme?
Those can be found with git grep instruction_table.
Apart from that, things seems to work fine, e.g.:
>>> list(disassemble_all('\x1c'))
[Instruction(0xfe, 'INVALID', 0, 0, 0, 0, 'Designated invalid instruction.', None, 0)]
>>> list(disassemble_all('\x1c', fork='constantinople'))
[Instruction(0x1c, 'SHR', 0, 2, 1, 3, 'Logical right shift.', None, 0)]EDIT: eh, those github lags :/
|
@disconnect3d I am on it asap! |
* separate instruction tables for pre-byzantium, byzantium and constantinopole.
* added new optional argument to CLI (--fork, -f) that accepts fork to be used.
* it is possible to select fork by name or by block number.
* introduced default fork selection by introducing DEFAULT_FORK constant
* added 3 new test cases, one per supported fork
* fixed test_ADD_1 test case
All functions and evmasm are fully backwards compatible. If no fork is selected, "byzantium" is chosen by default.
Example usage:
```
evmasm -t -f 0
evmasm -t -f 3700000
evmasm -t -f 4369999
evmasm -t -f pre-byzantium
evmasm -t
evmasm -t -f 4370000
evmasm -t -f 4370001
evmasm -t -f byzantium
evmasm -t -f 9999999 (block number to be updated after mainnet launch)
evmasm -t -f constantinople
```
Error handling:
```
$ evmasm -t -f a
Wrong fork name or block number. Please provide an integer or one of ['pre-byzantium', 'byzantium', 'constantinople'].
$ evmasm -t -f constantinoplee
Wrong fork name or block number. Please provide an integer or one of ['pre-byzantium', 'byzantium', 'constantinople'].
$ evmasm -t -f 1.2
Wrong fork name or block number. Please provide an integer or one of ['pre-byzantium', 'byzantium', 'constantinople'].
$ evmasm -t -f
usage: evmasm [-h] (-a | -d | -t) [-bi] [-bo] [-i [INPUT]] [-o [OUTPUT]]
[-f FORK]
evmasm: error: argument -f/--fork: expected 1 argument
```
Refs: crytic#7
cryptomental
force-pushed
the
feature/add-forks-support
branch
from
7c04cb5 to
4af7c00
Compare
@disconnect3d I corrected instruction tables import in readme, docstrings and added new command line option description to readme. I did not touch anything else. Thanks for noticing this! I guess I will start my next PRs with checking README's ;) |
disconnect3d
left a comment
disconnect3d
left a comment
There was a problem hiding this comment.
That's awesome, thanks!
4 participants
All functions and evmasm are fully backwards compatible. If no fork is selected, "byzantium" is chosen by default.
Example usage:
Error handling:
Refs: #7