-
Notifications
You must be signed in to change notification settings - Fork 115
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
Tests & implementation of CallDataLoad and CodeCopy #67
Comments
https://github.com/status-im/nimbus/pull/65/commits/42fd80c00b1432bcce42f11fb755d46534ad3db9 should handle the edge cases. Tests needed: |
mratsim
added a commit
that referenced
this issue
Jul 5, 2018
mratsim
added a commit
that referenced
this issue
Jul 6, 2018
* move forks constants, rename errors * Move vm/utils to vm/interpreter/utils * initial opcodes refactoring * Add refactored Comparison & Bitwise Logic Operations * Add sha3 and address, simplify macro, support pop 0 * balance, origin, caller, callValue * fix gas copy opcodes gas costs, add callDataLoad/Size/Copy, CodeSize/Copy and gas price opcode * Update with 30s, 40s, 50s opcodes + impl of balance + stack improvement * add push, dup, swap, log, create and call operations * finish opcode implementation * Add the new dispatching logic * Pass the opcode test * Make test_vm_json compile * halt execution without exceptions for Return, Revert, selfdestruct (fix #62) * Properly catch and recover from EVM exceptions (stack underflow ...) * Fix byte op * Fix jump regressions * Update for latest devel, don't import old dispatch code as quasiBoolean macro is broken by latest devel * Fix sha3 regression on empty memory slice and until end of range slice * Fix padding / range error on expXY_success (gas computation left) * update logging procs * Add tracing - expXY_success is not a regression, sload stub was accidentally passing the test * Reuse the same stub as OO implementation * Delete previous opcode implementation * Delete object oriented fork code * Delete exceptions that were used as control flows * delete base.nim 🔥, yet another OO remnants * Delete opcode table * Enable omputed gotos and compile-time gas fees * Revert const gasCosts -> generates SIGSEGV * inline push, swap and dup opcodes * loggers are now template again, why does this pass new tests? * Trigger CI rebuild after rocksdb fix status-im/nim-rocksdb#5 * Address review comment on "push" + VMTests in debug mode (not release) * Address review comment: don't tag fork by default, make opcode impl grepable * Static compilation fixes after rebasing * fix the initialization of the VM database * add a missing import * Deactivate balance and sload test following #59 * Reactivate stack check (deactivated in #59, necessary to pass tests) * Merge remaining opcodes implementation from #59 * Merge callDataLoad and codeCopy fixes, todo simplify see #67
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Extracting discussion from https://github.com/status-im/nimbus/pull/59/files#r200313762
CallDataLoad
The current master implementation for calldataload is (from #59):
With a potential simpler solution (from #65)
CodeCopy
The current master implementation for codecopy is (from #59):
With a simpler solution (from #65):
Reference implementation
Here are the implementations in Py-EVM, Geth and Parity:
Py-EVM is doing something very complex, increasing a program counter in the "CodeStream" type. while Geth, Parity and the Yellow paper are just copying bytes?
Concerns
The current implementation is complex, with 2 nested ifs, and the proc "writePaddingBytes" that is redundant with memory.extend (which pads with 0 already)
The simpler codecopy has an off by one error (which see the simpler calldataload which does not suffer for this)
codePos (callDataLoad) and copyPos (codeCopy) can be out of bounds. For copyPos this is handled in memory.write:
but there is no handling for outofbounds read in codePos.
TODO
cc @zah
The text was updated successfully, but these errors were encountered: