Skip to content

Halve gas used by SafetyChecker #210

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.

Sign up for GitHub

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

Merged
K-Ho merged 1 commit into from
Aug 11, 2020
Merged

Halve gas used by SafetyChecker #210

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 58 additions & 58 deletions packages/contracts/contracts/optimistic-ethereum/ovm/SafetyChecker.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,7 @@ contract SafetyChecker is ContractResolver {
{
bool seenJUMP = false;
bool insideUnreachableCode = false;
uint256[] memory ops = new uint256[](_bytecode.length);
uint256 opIndex = 0;
uint256 ops = 0;
for (uint256 pc = 0; pc < _bytecode.length; pc++) {
// current opcode: 0x00...0xff
uint256 op = uint8(_bytecode[pc]);
Expand All @@ -85,72 +84,73 @@ contract SafetyChecker is ContractResolver {
}
} else {
// check that opcode is whitelisted (using the whitelist bit mask)
uint256 opBit = 2 ** op;
uint256 opBit = 1 << op;
if (opcodeWhitelistMask & opBit != opBit) {
// encountered a non-whitelisted opcode!
return false;
}
// append this opcode to a list of ops
ops[opIndex] = op;
// JUMPI
if (op == 0x57) {
// We can now reach all JUMPDESTs
seenJUMP = true;
// JUMP
} else if (op == 0x56) {
// We can now reach all JUMPDESTs
seenJUMP = true;
// we are now inside unreachable code until we hit a JUMPDEST!
insideUnreachableCode = true;
// STOP or REVERT or INVALID or RETURN (see safety checker docs in wiki for more info)
} else if (op == 0x00 || op == 0xfd || op == 0xfe || op == 0xf3) {
// If we can't jump to JUMPDESTs, then all remaining bytecode is unreachable
if (!seenJUMP) {
return true;
}
// We are now inside unreachable code until we hit a JUMPDEST!
insideUnreachableCode = true;
// CALL
} else if (op == 0xf1) {
// Minimum 4 total ops:
// 1. PUSH1 value
// 2. PUSH20 execution manager address
// 3. PUSH or DUP gas
// 4. CALL

if (opIndex < 3) {
return false;
}
uint256 gasOp = ops[opIndex - 1];
uint256 addressOp = ops[opIndex - 2];
uint256 valueOp = ops[opIndex - 3];
if (
gasOp < 0x60 || // PUSHes are 0x60...0x7f
gasOp > 0x8f || // DUPs are 0x80...0x8f
addressOp != 0x73 || // address must be set with a PUSH20
valueOp != 0x60 // value must be set with a PUSH1
) {
return false;
} else {
uint256 pushedBytes;
// gas is set with a PUSH##
if (gasOp >= 0x60 && gasOp <= 0x7f) {
pushedBytes = gasOp - 0x5f;
ops <<= 8;
ops |= op;
// [0x57, 0x56, 0x00, 0xfd, 0xfe, 0xf3, 0xf1] all have handlers
if (opBit & 0x600a00000000000000000000000000000000000000c000000000000000000001 != 0) {
// JUMPI
if (op == 0x57) {
// We can now reach all JUMPDESTs
seenJUMP = true;
// JUMP
} else if (op == 0x56) {
// We can now reach all JUMPDESTs
seenJUMP = true;
Copy link
Contributor

@K-Ho K-Ho Aug 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now that I think about it, we can just assume that we've seen a JUMP. I doubt we'll ever have a case where we reach a halting opcode before seeing a JUMP (no need to update this in this PR, but curious about your thoughts)

Copy link
Contributor Author

@geohot geohot Aug 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meh, personally I'd remove this logic. It's safer the other way, never allow something you don't expect.

// we are now inside unreachable code until we hit a JUMPDEST!
insideUnreachableCode = true;
// STOP or REVERT or INVALID or RETURN (see safety checker docs in wiki for more info)
} else if (op == 0x00 || op == 0xfd || op == 0xfe || op == 0xf3) {
Copy link
Contributor

@K-Ho K-Ho Aug 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be cheaper to also do an opBit & mask != 0 here?

Copy link
Contributor Author

@geohot geohot Aug 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but it's diminishing returns since it's already gated outside.

// If we can't jump to JUMPDESTs, then all remaining bytecode is unreachable
if (!seenJUMP) {
return true;
Comment on lines +110 to +111
Copy link
Contributor

@K-Ho K-Ho Aug 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See JUMP comment above

}

// 23 is from 1 + PUSH20 + 20 bytes of address + PUSH or DUP gas
byte callValue = _bytecode[pc - (23 + pushedBytes)];

// 21 is from 1 + 19 bytes of address + PUSH or DUP gas
address callAddress = toAddress(_bytecode, (pc - (21 + pushedBytes)));

// CALL is made to the execution manager with msg.value of 0 ETH
if (callAddress != address(resolveExecutionManager()) || callValue != 0 ) {
// We are now inside unreachable code until we hit a JUMPDEST!
insideUnreachableCode = true;
// CALL
} else if (op == 0xf1) {
// Minimum 4 total ops:
// 1. PUSH1 value
// 2. PUSH20 execution manager address
// 3. PUSH or DUP gas
// 4. CALL

// if opIndex < 3, there would be 0s here, so we don't need the check
uint256 gasOp = (ops >> 8) & 0xFF;
uint256 addressOp = (ops >> 16) & 0xFF;
uint256 valueOp = (ops >> 24) & 0xFF;
if (
gasOp < 0x60 || // PUSHes are 0x60...0x7f
gasOp > 0x8f || // DUPs are 0x80...0x8f
addressOp != 0x73 || // address must be set with a PUSH20
valueOp != 0x60 // value must be set with a PUSH1
) {
return false;
} else {
uint256 pushedBytes;
// gas is set with a PUSH##
if (gasOp >= 0x60 && gasOp <= 0x7f) {
pushedBytes = gasOp - 0x5f;
}

// 23 is from 1 + PUSH20 + 20 bytes of address + PUSH or DUP gas
byte callValue = _bytecode[pc - (23 + pushedBytes)];

// 21 is from 1 + 19 bytes of address + PUSH or DUP gas
address callAddress = toAddress(_bytecode, (pc - (21 + pushedBytes)));

// CALL is made to the execution manager with msg.value of 0 ETH
if (callAddress != address(resolveExecutionManager()) || callValue != 0 ) {
return false;
}
}
}
}
opIndex++;
}
}
return true;
Expand Down