Add MerkleStorage trait to data

[vlopes11]

The merkle storage trait will extend the storage functionality, adding
the ability to calculate the merkle root.

The contract code root assumes a small in-memory merkle tree is used as
backend to calculate the contract id.

This commit also included the improvement over the instructions
execution that the opcodes are enforced by design to have consistent
register references (e.g. they only stored in 6 bits - hence they can't
overflow the 64 available registers). This means that range check for
read-only registers is not required.

[vlopes11]

For the merkle storage trait, we can either expect a single structure to be the wrapper to all the underlying behavior such as merkle tree, sparse merkle tree, traditional KVS, etc; or we can have one generic type for every expected implementation (example: a dedicated type for the contract ID merkle root calculation).

I prefer the first (a single wrapper type) because this way the code is more readable and if we want to swap the implementor of one of the aspects, we just update the wrapper type. The syntax would be Interpreter<MemoryStorage> or Interpreter<FooStorage> and both MemoryStorage/FooStorage will be responsible to implement the internal strategies for every trait requirement of the VM. Example: the expected Storage<K, V> + MerkleStorage<K, V>, for (K, V) = (Word, [u8; 8]), is the implementation that is going to be used as code root merkle tree -> contract id.

The alternative is to use generic types for every of the specific implementations (example: code root merkle tree). This undermines the code readability and may lead to huge/coupled types. Example: Interpreter<MemoryStorage, CodeRootMerkleProvider, FooMerkleProvider>. The advantage of this approach is we discard the need of a wrapper that will need to have some knowledge of the specifics of the VM (example: the wrapper need to know that MerkleStorage<Word, [u8; 8> is used to calculate the code root).

[adlerjohn]

I don't have strong opinions one way or the other on the interface. Note two things:

1. The Merkle storages should return a hash, not a contract ID, on writes.
2. Computing the contract ID from bytecode is actually not a storage op, it's a pure operation. So it should probably be done entirely in the VM, and not be present in the storage trait.

[Voxelot]

Take it or leave it, here's one way the alternative proposal could make the Interpreter less painful to work with so you don't need to specify a dozen generic params each time it's used. It's got some of it's own boilerplate, but provides flexibility to separate out the data interfaces however is needed. This may feel more verbose and complicated in other ways, so I'm fine with sticking to the original approach for now.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=a75a1e07d407068356b0ca99e9fd313d

[vlopes11]

Take it or leave it, here's one way the alternative proposal could make the Interpreter less painful to work with so you don't need to specify a dozen generic params each time it's used. It's got some of it's own boilerplate, but provides flexibility to separate out the data interfaces however is needed. This may feel more verbose and complicated in other ways, so I'm fine with sticking to the original approach for now.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=a75a1e07d407068356b0ca99e9fd313d

I like this approach you suggested.

The main advantage of the current approach is that we can have a single type with different trait implementations that will map the behavior of the storage according to the VM functionality. This prevents us from having duplicate concrete types that will behave the same way but for different underlying types.

Example:
impl Storage<ContractId, Contract> for FooStorage {..}
impl Storage<ContractId, ContractState> for FooStorage {..}

or

impl Storage<ContractId, Contract> for ContractCodeProvider {..}
impl Storage<ContractId, ContractState> for ContractStateProvider {..}

struct BarStorageProvider {
  type CodeProvider = ContractCodeProvider;
  type StateProvider = ContractStateProvider;
}

I'll make some tests and compare the differences

[Voxelot]

You don't have to use different structs for each type, so you could do it like this:

type ContractState = Bytes32
type MerkleRoot = Bytes32

impl Storage<ContractId, Contract> for MemoryStore {..}
impl Storage<ContractId, ContractState> for MemoryStore {..}
impl Storage<ContractId, MerkleRoot> for MerkleRootProvider {..}

struct BarStorageProvider {
  type CodeProvider = MemoryStore;
  type StateProvider = MemoryStore;
  type ContractMerkleRoot = MerkleRootProvider;
}

The main difference from what we have now is that the intent around each provider is a little more clear and we can disambiguate between Storage<K, V>'s with similar primitive values.

If you want to keep all the providers independent, they could just be newtype-wrappers around a generic MemoryStore type.

struct MemoryStore<K, V> { .. }
impl<K, V> Storage for MemoryStore<K, V> { }

struct BalanceProvider(MemoryStore<([u8;32], [u8;32]), [u8;32]>);
impl Storage<([u8;32], [u8;32]), [u8;32]> for BalanceProvider { 
    fn get(&self, key: &K) -> Option<&V> {
        self.0.get(key)
    }
}

While this cuts down some redundant impl, there is still a lot of wrapper to implement. Here is an admittedly lazy way of doing it if you really don't want to manually implement a impl Storage<K, V> for Provider for each provider wrapper type that just does a passthrough to a generic MemoryStore. Although this type of thing could also be automated with a macro.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=bbd13bc662df38578c077dc3f9252f92

[vlopes11]

@Voxelot I liked the approach you suggested. But we have an open question.

Considering this trait:

pub trait InterpreterStorage:
{
    type ContractStateProvider: KeyedMerkleStorage<ContractId, ContractData, (), ContractState>;
    type ColorBalanceProvider: KeyedMerkleStorage<ContractId, (), Color, Word>;
    type ContractMapProvider: KeyedMerkleStorage<ContractId, (), Bytes32, Bytes32>;

    fn block_height(&self) -> Result<u32, DataError>;
    fn coinbase(&self) -> Result<Address, DataError>;
    fn block_data(&self, block_height: u32) -> Result<BlockData, DataError>;
}

It forces us to have multiple attributes of the interpreter as the storage. Example:

struct Interpreter {
    ...,
    contract_state_provider: S::ContractStateProvider,
    color_balance_provider: S::ColorBalanceProvider,
    contract_map_provider: S::ContractMapProvider,
}

Instead of

struct Interpreter {
    ...,
    storage: S
}

We cannot expect the Interpreter::with_storage to take multiple arguments for each of these storages because it would create a hard coupling between the consumers of the interpreter and its internal storage requirements.

Example: If someday we decide to get rid of the contract_map_provider, we might produce an API breaking change for all Interpreter::with_storage(storage1, storage2, ...) calls (probably everywhere; from forc to graphql). However, if we only have storage: S, we just update the storage implementations + trait and Interpreter::with_storage(foo_storage) will still work as long as foo_storage meet the requirements.

As alternative, we can require InterpreterStorage to provide fn contract_state_provider(&self) -> &S::ContractStateProvider, but that kinda kills the purpose of having the dynamic types since the wrapper itself will still have to hold references to the providers (and that probably would be just &Self).

I think its best we just leave it as is for now until we come with a solution for the dynamic types that allows us to create an interpreter instance with only Interpreter::with_storage(foo_storage)

[vlopes11]

@adlerjohn do you agree

type ContractStateProvider: KeyedMerkleStorage<ContractId, ContractData, (), ContractState>;

supersede

type ContractMapProvider: KeyedMerkleStorage<ContractId, (), Bytes32, Bytes32>;
?

We could keep the map provider as helper to the contract users, but maybe they can implement it themselves using the state storage. That looks like more of a responsibility of the high level language.

A TODO feature of the contract state is to return memory references instead of owned bulks in order to support huge contract states.

Resolved

[Voxelot]

Not going to block, but I don't think this justification is sufficient. It should explain what about the interpreter allows us to do unsafe buffer access in this particular scenario.

[Voxelot]

One of my main concerns is rushing in the extensive use of unsafe code. The risk here is big, since as a distributed system everyone will need to be able to run arbitrary code put into their VM.

However, so far I haven't seen any lifetime rules broken or anything like that. But in the future I think we shouldn't be so eager to use unsafe code without more thorough justifications in the code comments.