[WIP] Add confidential prefix check utility #1059
Conversation
| return false; | ||
| } | ||
| let prefix = &data[..PREFIX_LEN]; | ||
| // u8 representation of "confidential" |
There was a problem hiding this comment.
we feel pretty good that this is otherwise going to be an invalid transaction?
There was a problem hiding this comment.
Nope. We need a better guarantee, preferably in the form of an EIP or some existing standard.
There was a problem hiding this comment.
there are known "reserve" or "invalid" evm byte code. can't we use those somehow creatively?
There was a problem hiding this comment.
Have a link I can read?
There was a problem hiding this comment.
namely:
For a single byte whose value is in the [0x00, 0x7f] range, that byte is its own RLP encoding.
There was a problem hiding this comment.
Wait, does the data blob include the CREATE/CALL instruction (e.g., 0xf0 or 0xf1) or is the create/call implicit and only the initcode/sighash+abi is in the data blob?
If it is the latter this is more complicated as we need a value which at the same time:
- Is unlikely to be a sighash (I assume a 32-byte hash, e.g. Keccak?).
- Is unlikely to be initcode (EVM bytecode).
There are some opcodes which are invalid (e.g., based on this it seems like e.g. 0x0c is an invalid opcode). But of course a future revision could add opcodes in there. We could use some combination which is unlikely, e.g. 0x000c0d636f...6c.
There was a problem hiding this comment.
Create/call is implicit based upon there existing a to address or not.
There was a problem hiding this comment.
Then something like some invalid bytecodes at the start and then some other specific value like Web3c-Confidential should be fine? For initcode the invalid opcodes should never appear and for a hash a specific constant is very very unlikely (but possible - can be made more unlikely by being longer).
There was a problem hiding this comment.
So it looks like parity uses the fact that WASM modules prepend the 4 byte prefix \0asm to know if it's dealing with WASM code. This prefix is assumed to be part of both the initcode and the bytecode of the contract. See https://github.com/oasislabs/parity/blob/ekiden/ethcore/src/factory.rs#L33.
For confidential contracts we can do something similar.
For CREATE, say, in the web3c.js client, we can prepend \0pri (or some other 4 byte prefix to be consistent with wasm's prefix) to the data field in the transaction.
During the CREATE, in the runtime, we can check for the prefix, and when we finally store the bytecode of the contract (which drops the initcode), we can re-prepend the \0pri to the stored byte code. On all subsequent CALLs, we can do the check for the prefix by looking at the account's stored code, and decrypt the data field if its spotted.
This scheme is mainly different from the previous above conversation because, for CALLs, the data field itself doesn't have a prefix and is just encrypted. As a result, we can remove the possibility of the sighash colliding with our prefix. Furthermore, the \0 byte represents the STOP opcode in the EVM, and so during CREATE, we don't have to worry about a collision with valid bytecode either, since that won't ever be the first instruction.
| } | ||
| let prefix = &data[..PREFIX_LEN]; | ||
| // u8 representation of "confidential" | ||
| let confidential_prefix = [99, 111, 110, 102, 105, 100, 101, 110, 116, 105, 97, 108]; |
There was a problem hiding this comment.
It would be better to make this a module-level constant.
| if data.len() < PREFIX_LEN { | ||
| return data; | ||
| } | ||
| data[PREFIX_LEN..].to_vec() |
There was a problem hiding this comment.
Shouldn't this check that data actually has the prefix, instead of assuming that the first PREFIX_LEN bytes are the prefix?
There was a problem hiding this comment.
It depends on the scheme we use. It should be fine to assume it’s the first couple of bytes. We just need to make sure the prefix chosen is not valid bytecode so that we don’t mistake a valid transaction for an encrypted one.
Is there a specific reason why we might want the prefix to be anywhere in the ‘data’ that I’m missing?
There was a problem hiding this comment.
I don't think @Yawning is saying that the prefix could be anywhere in the data, but that the function should maybe first check if data[..PREFIX_LEN] == CONFIDENTIAL_PREFIX and not just assume that it is just based on length as it is currently.
Could return the data unchanged otherwise or even return an Option in this case and return None in case the prefix is not there. In any case this behavior should be documented.
There was a problem hiding this comment.
Ah I see good point. I'll add the check. The assumption I should've stated was that is_encrypted returned true.
|
Putting back into [WIP] while I address comments and update the |
armaniferrante
changed the title
|
With the new scheme, putting the confidential prefix onto the contract's bytecode, instead of onto the the transaction's data field, I'll be pushing these changes down into parity and so will not need this PR. Closing. |
No description provided.