Lowered Safety Checker Gas #218
Conversation
…hout requires, all pass
|
@K-Ho looks like a bunch of json files are committed? |
It's for the Safety Checker tests to verify that every compiled Synthetix contract is safe. There could be better ways to access the JSON files than just committing them to the main repo though. |
packages/contracts/contracts/optimistic-ethereum/ovm/SafetyChecker.sol
Outdated
Show resolved
Hide resolved
| // current opcode: 0x00...0xff | ||
| uint256 op = uint8(_bytecode[pc]); | ||
| uint256 op; |
| uint256 _opcodeProcMask = ~uint256(0xffffffffffffffffffffffe000000000fffffffff070ffff9c0ffffec000f001); | ||
| // Halting opcodes | ||
| uint256 _opcodeStopMask = ~uint256(0x6008000000000000000000000000000000000000004000000000000000000001); | ||
| // PUSH opcodes | ||
| uint256 _opcodePushMask = ~uint256(0xffffffff000000000000000000000000); | ||
|
|
||
| uint256 codeLength; | ||
| uint256 _pc; | ||
| assembly { | ||
| _pc := add(_bytecode, 0x20) | ||
| } | ||
| codeLength = _pc + _bytecode.length; |
There was a problem hiding this comment.
Any reason why these local variables are not consistently named (_varName vs name)?
There was a problem hiding this comment.
good point! Fixed all the other variable names, but I just tried replacing _pc with pc and this does not work in assembly, where pc refers to the PC opcode
| // 4. CALL | ||
| // inline assembly removes the extra add + bounds check | ||
| assembly { | ||
| let tmp := mload(_pc) |
There was a problem hiding this comment.
Would rename tmp to bytecodeStart or something that indicates what it is if that's not it.
| if (gasOp >= 0x60 && gasOp <= 0x7f) { | ||
| pushedBytes = gasOp - 0x5f; | ||
| } | ||
| let mpc := byte(0, mload(add(skip, byte(0, tmp)))) |
There was a problem hiding this comment.
mpc seems like it stands for something. Would rename it to be something that indicates what that is.
| if (opBit & _opcodeProcMask == 0) { | ||
| if (opBit & _opcodePushMask == 0) { | ||
| // subsequent bytes are not opcodes. Skip them. | ||
| _pc += (op - 0x5e); |
There was a problem hiding this comment.
might create a const for 0x5e. I assume that's the opcode before PUSH1? Maybe named pushStart?
| uint256[8] memory skip = [ | ||
| uint256(0x0001010101010101010101010000000001010101010101010101010101010000), | ||
| uint256(0x0100000000000000000000000000000000000000010101010101000000010100), | ||
| uint256(0x0000000000000000000000000000000001010101000000010101010100000000), | ||
| uint256(0x0203040500000000000000000000000000000000000000000000000000000000), | ||
| uint256(0x0101010101010101010101010101010101010101010101010101010101010101), | ||
| uint256(0x0101010101000000000000000000000000000000000000000000000000000000), | ||
| uint256(0x0000000000000000000000000000000000000000000000000000000000000000), | ||
| uint256(0x0000000000000000000000000000000000000000000000000000000000000000)]; |
There was a problem hiding this comment.
Any reason this is a uint256[8] instead of just bytes? I think it can just be a bytes literal: ethereum/solidity#832
There was a problem hiding this comment.
Didn't work with the way he's operating on it, unfortunately :(
| let mpc := byte(0, mload(add(skip, byte(0, tmp)))) | ||
| mpc := add(mpc, byte(0, mload(add(skip, byte(mpc, tmp))))) | ||
| mpc := add(mpc, byte(0, mload(add(skip, byte(mpc, tmp))))) | ||
| mpc := add(mpc, byte(0, mload(add(skip, byte(mpc, tmp))))) | ||
| mpc := add(mpc, byte(0, mload(add(skip, byte(mpc, tmp))))) | ||
| mpc := add(mpc, byte(0, mload(add(skip, byte(mpc, tmp))))) |
There was a problem hiding this comment.
It's very unclear what this is supposed to do. A few comments here and/or better naming would be useful.
| // encountered a JUMPDEST | ||
| if (op == 0x5b) break; | ||
| // skip PUSHed bytes | ||
| if ((1 << op) & _opcodePushMask == 0) _pc += (op - 0x5f); |
There was a problem hiding this comment.
Can re-use pushStart const here if created (see comment above)
There was a problem hiding this comment.
Tried this out, unfortunately, increases gas cost, so I added a comment instead
| if ((1 << op) & _opcodePushMask == 0) _pc += (op - 0x5f); | ||
| } while (_pc < codeLength); | ||
| // op is 0x5b, so we don't continue here since the _pc++ is fine | ||
| } else if (op == 0x33) { |
There was a problem hiding this comment.
| } else if (op == 0x33) { | |
| } else if (op == 0x33) { // CALLER opcode |
| for (uint256 pc = 0; pc < _bytecode.length; pc++) { | ||
| // autogenerated by gen_safety_checker_constants.py | ||
| // number of bytes to skip for each opcode | ||
| uint256[8] memory skip = [ |
There was a problem hiding this comment.
Would rename to something better than skip. Maybe opcodeBytesConsumed?
We don't have private dependencies anymore.
* Init * Update babylon-private reference * Revert "CI: Remove redundant SSH key logic (ethereum-optimism#218)"
Description
All credit goes to @geohot! #214
Lower Safety Checker gas to <1.5m gas for 24KB contracts.
This just extends the above PR with fixed tests and comments.
Contributing Agreement