Daohub-io / cap9

Capability-based security protocol for smart contracts
Apache License 2.0
22 stars 10 forks source link

New procedure registration process #70

Closed JakeOShannessy closed 6 years ago

JakeOShannessy commented 6 years ago

Previously a procedure was added to the kernel by passing the bytecode to the kernel and letting it validate and instantiate it. This has a few problems, the most pressing of which is that the constructor of any contract must be run before we can be sure what we are validating, therefore it must always be deployed first. Rather than relying on the kernel to do this, initial deployment of the contract is simply done by the user/developer and the contract address (along with a name and capabilities) is passed to the kernel. The kernel will then validate the procedure and add it to its procedure table (or reject it accordingly).

This is still done via a direct registerProcedure ABI call to the kernel.

Latrasis commented 6 years ago

Looks good so far. Please update the test descriptions in /withentryproc/*.js so that we could differentiate which tests are which in output.

Latrasis commented 6 years ago

Looks like i'm getting massive delays in tests due to the whitelisting:

it s slow

JakeOShannessy commented 6 years ago

What;s the difference from the previous times?

Latrasis commented 6 years ago

Here's the speedup:

capture

I replaced the verification with this:

if (ins == 0x54) {return 1;} // SLOAD
if (ins == 0x55) {return 2;} // SSTORE

if (ins == 0xa0) {return 3;} // LOG0
if (ins == 0xa1) {return 4;} // LOG1
if (ins == 0xa2) {return 5;} // LOG2
if (ins == 0xa3) {return 6;} // LOG3
if (ins == 0xa4) {return 7;} // LOG4

if (ins == 0xf0) {return 8;} // CREATE
if (ins == 0xf1) {return 9;} // CALL
if (ins == 0xf2) {return 10;} // CALLCODE
if (ins == 0xf4) {return 11;} // DELEGATECALL
if (ins == 0xf5) {return 12;} // CREATE2
if (ins == 0xff) {return 13;} // SELFDESTRUCT

if (ins >= 0x60 && ins <= 0x7f) {
    i += ins - 95;
}
Latrasis commented 6 years ago

What;s the difference from the previous times?

So it's about 3-5x slowdown

JakeOShannessy commented 6 years ago

Let's wait until I fixed the previous bug then benchmark it again.

Latrasis commented 6 years ago

@JakeOShannessy: Here's a blacklist that checks for unknown opcodes:

if (ins == 0x54) {return 1;} // SLOAD
if (ins == 0x55) {return 2;} // SSTORE

if (ins == 0xa0) {return 3;} // LOG0
if (ins == 0xa1) {return 4;} // LOG1
if (ins == 0xa2) {return 5;} // LOG2
if (ins == 0xa3) {return 6;} // LOG3
if (ins == 0xa4) {return 7;} // LOG4

if (ins == 0xf0) {return 8;} // CREATE
if (ins == 0xf1) {return 9;} // CALL
if (ins == 0xf2) {return 10;} // CALLCODE
if (ins == 0xf4) {return 11;} // DELEGATECALL
if (ins == 0xf5) {return 12;} // CREATE2
if (ins == 0xff) {return 13;} // SELFDESTRUCT

if(
    (ins > 0x0b && ins < 0x10) ||
    (ins > 0x1d && ins < 0x20) ||
    (ins > 0x20 && ins < 0x30) ||
    (ins > 0x3f && ins < 0x40) ||
    (ins > 0x40 && ins < 0x50) ||
    (ins > 0x5b && ins < 0x60) ||
    (ins > 0xa4 && ins < 0xf0) ||
    (ins > 0xf5 && ins < 0xfa) ||
    (ins > 0xfa && ins < 0xfd)
) {
    return 100;
} // UNKOWN OPCODE

if (ins >= 0x60 && ins <= 0x7f) {
    i += ins - 95;
}
JakeOShannessy commented 6 years ago

Yeah that's what I meant by using ranges. But let's get everything working first before we worry about efficiency.

Latrasis commented 6 years ago

Yeah that's what I meant by using ranges. But let's get everything working first before we worry about efficiency.

Not against that, but it slows tests by a large margin. I don't see an issue to simply use the blacklist at this time.

Latrasis commented 6 years ago

Propose to delay whitelist with #71

JakeOShannessy commented 6 years ago

Because the blacklist doesn't work either.

JakeOShannessy commented 6 years ago

Ok, the tests are now passing properly, and validation is being properly tested, with the exception of SLOAD calls, which are still used in a couple of places for testing (we don't currently have a read capability).

Latrasis commented 6 years ago

Is this ready for review?

JakeOShannessy commented 6 years ago

yes

Latrasis commented 6 years ago

Please change test names to differentiate tests with entry proc and without. Otherwise good to merge.