Contract storage enhancement for use of object graph #866

AlexandraRoatis · 2019-04-04T19:19:47Z

Description

The major change of this PR is allowing a different interpretation of the storageRoot for FVM contracts (same as before) and AVM contracts.

In the case of AVM contracts the storage root is a pointer to a database value that retrieves the long term storage root and the hash of the object graph. The reason for overwriting the meaning of the storageRoot is to maintain the same number of elements of an account involved in consensus.

To achieve this change the following classes were updated in this PR:

KernelInterfaceForAVM.jave implements the put/get object graph methods by delegating the call to the repository.
KernelInterfaceForFastVM.java does not support the object graph and therefore throws an exception when either method is called.
BulkExecutor.java can determine the VM type by either calling the contract through a copy of the repository or by making a direct DB call as before.
AionBlockchainImpl.java also save the VM type inside the contract details object.
CfgDb.java, AionRepositoryImpl.java, AbstractRepository.java, AionRepositoryCache.java and DetailsDataStore.java have a new database for storing the object graph and new method for setting and retrieving the VM type and object graph.
AbstractContractDetails.java, AionContractDetailsImpl.java, ContractDetailsCacheImpl.java include tracking of the VM type and to different degrees the object graph. Because the AVM implementation is not yet updated to use the object graph a boolean flag enableObjectGraph disables some of the functionality. The encoding of the contract details now contains also the VM type. When the contract details are created as a snapshot to a specific storage hash, they must search the database for the actual storage root and object graph hash.

Type of change

Insert x into the following checkboxes to confirm (eg. [x]):

Testing

Please describe the tests you used to validate this pull request. Provide any relevant details for test configurations as well as any instructions to reproduce these results.

existing tests

Verification

Insert x into the following checkboxes to confirm (eg. [x]):

I have self-reviewed my own code and conformed to the style guidelines of this project.
New and existing tests pass locally with my changes.
I have added tests for my fix or feature.
I have made appropriate changes to the corresponding documentation.
My code generates no new warnings.
Any dependent changes have been made.

- new database for storing the object graph - functionality for saving the correct VM type into contract details - new encoding of contract details - interpret storage root based on the VM used

modAionImpl/src/org/aion/zero/impl/AionBlockchainImpl.java

AionJayT · 2019-04-08T20:42:40Z

modAionImpl/src/org/aion/zero/impl/db/AionContractDetailsImpl.java

+    }
+
+    // TODO: use until the AVM impl is added
+    private final boolean enableObjectGraph = false;


eventually, the AVM will use the ObjectGraph. I think the flag can be removed.

AionJayT · 2019-04-08T20:49:17Z

modVM/src/org/aion/vm/KernelInterfaceForAVM.java

    }

    @Override
    public void putStorage(Address address, byte[] key, byte[] value) {
        ByteArrayWrapper storageKey = new ByteArrayWrapper(key);
        ByteArrayWrapper storageValue = new ByteArrayWrapper(value);
        this.repositoryCache.addStorageRow(address, storageKey, storageValue);
+        if (this.repositoryCache.getVmType(address) != AVM_CREATE_CODE) {


the check looks weird (also the L106) What's the reason for forcing the storage change the vm type?

Each kernel interface is called only if the contract fit its definition. Whenever the call fits a contract behavior like adding storage, code or the object graph I check if the VM code was not set yet. Otherwise, we cannot ensure that the correct code is set until the blockchain checks for contract transactions. Without this setting, we cannot guarantee that a contract can be correctly deployed and called in the same block.

Is it possible to have only one method set the vm type, and to ensure we hit this method before any others? We may not have good control over the ordering of these events, so perhaps it is not possible, but I feel it may be? 'lazy' updates that have to be scattered across every method scare me just because it increases the vulnerabilities and can be easy to forget about a certain code path that can get you into a corrupt state.

I don't like it either, but so far there is no such method. We need to add a createContract to the interfaces that would have to be called whenever a contract is created. This way we can also avoid instantiating and managing contract details objects for all the created accounts that most often are not actual contracts.

aionick · 2019-04-09T13:34:24Z

modAionImpl/src/org/aion/zero/impl/db/AionContractDetailsImpl.java

            this.rlpEncoded = rlpCode;
        }
+
+        if (!fastCheck || externalStorage || !keepStorageInMem) { // it was not a fast check


Could you add a comment explaining this conditional here? Will help someone coming along in the future, and will definitely help review this part of the code, because I'm not quite sure what each of the possible permutations here would mean.

aionick · 2019-04-09T13:44:22Z

modVM/src/org/aion/vm/KernelInterfaceForAVM.java

    }

    @Override
    public void putStorage(Address address, byte[] key, byte[] value) {
        ByteArrayWrapper storageKey = new ByteArrayWrapper(key);
        ByteArrayWrapper storageValue = new ByteArrayWrapper(value);
        this.repositoryCache.addStorageRow(address, storageKey, storageValue);
+        if (this.repositoryCache.getVmType(address) != AVM_CREATE_CODE) {


Is it possible to have only one method set the vm type, and to ensure we hit this method before any others? We may not have good control over the ordering of these events, so perhaps it is not possible, but I feel it may be? 'lazy' updates that have to be scattered across every method scare me just because it increases the vulnerabilities and can be easy to forget about a certain code path that can get you into a corrupt state.

aionick · 2019-04-09T13:47:36Z

modVM/src/org/aion/vm/KernelInterfaceForAVM.java

+        byte storedVmType = repositoryCache.getVMUsed(destination);
+
+        // DEFAULT is returned when there was no contract information stored
+        if (storedVmType == TransactionTypes.DEFAULT) {


Is this ever a correct state? Shouldn't this value always be set, and we should be throwing an error here instead?
The comment inside this if-block says 'will load contract into memory otherwise leading to consensus issues' which seems to say 'doing this will break things' - or am I misreading this?

The problem is that contract details objects get created in the case of simple accounts that are not contracts as well. This requires a lot of refactoring to clean up the implementation. The refactoring is delegated to 0.4.1 release though.

aionick · 2019-04-09T13:50:17Z

modAionImpl/src/org/aion/zero/impl/AionBlockchainImpl.java

+                if (tx.isContractCreationTransaction() && receipt.isSuccessful()) {
+                    track.saveVmType(
+                            tx.getContractAddress(),
+                            TransactionTypeRule.isValidAVMContractDeployment(tx.getTargetVM())


This condition seems a little misleading to me: 'isValidAVMContractDeployment'. I'm wondering if the meaning of this will ever diverge from what we want here? Possibly not... but I'm bringing it up because you are holding the tx.getTargetVM() already, and so can't we just test against that directly and get a more straightforward answer?

Unfortunately, there are contracts deployed on the mastery network with the incorrect VM type, so we need the TransactionTypeRule to validate AVM/FVM contracts differently depending on the fork parameter.

aionick · 2019-04-09T13:51:59Z

modAionImpl/src/org/aion/zero/impl/db/AbstractContractDetails.java

+    protected byte[] objectGraph = null;
+
+    // using the default transaction type to specify undefined VM
+    protected byte vmType = TransactionTypes.DEFAULT;


'default' and 'undefined' do mean two very different things. Perhaps it's better to explicitly create an UNDEFINED enum for these cases? Or else to rename 'default' if this has become its entire job.

This is not the only purpose. This value will also be used by transactions that are not contract deployments.

aionick · 2019-04-09T13:54:06Z

modAionImpl/src/org/aion/zero/impl/db/AionContractDetailsImpl.java

+            } else {
+                // note: the enforced use of optional is rather cumbersome here
+                Optional<byte[]> dbVal = getContractObjectGraphSource().get(objectGraphHash);
+                objectGraph = dbVal.isPresent() ? dbVal.get() : null;


We seem to have 3 classes of return values here (the object graph, an empty byte array, and null). Wondering if there is any meaningful difference between an empty byte array and null? If so, we should probably document it, and if not we should probably remove the null in favour of the empty array.

null means undefined, while empty byte array can be a valid setting; when the value is null I may need to check for the parent's object graph and load it

aionick · 2019-04-09T14:04:18Z

modAionImpl/src/org/aion/zero/impl/db/AionRepositoryImpl.java

@@ -376,6 +376,18 @@ public ByteArrayWrapper getStorageValue(Address address, ByteArrayWrapper key) {
        return (details == null) ? EMPTY_BYTE_ARRAY : details.getCode(codeHash);
    }

+    @Override
+    public byte getVmType(Address contract) {


Is it possible to return TransactionTypes instead here? Just so that the caller gets a more meaningful object in their hands?

Because of the way the interfaces are set up this is a very difficult refactoring. TransactionTypes are known only to the kernel, while the interfaces would need a compatible data type. We can leave this as an optimization for when the repository interfaces are extracted from the vm_api project.

aionick · 2019-04-09T14:06:13Z

modVM/src/org/aion/vm/BulkExecutor.java

+            byte storedVmType = repositoryChild.getVMUsed(destination);
+
+            // DEFAULT is returned when there was no contract information stored
+            if (storedVmType == TransactionTypes.DEFAULT) {


Is this supposed to be an error?

This value will be returned for contracts that have not been stored, resulting from databases that have been migrated to the new build without re-importing all blocks. In that case, we check for the vm type stored in the contract details object.

…ntracts deployed without code or storage

AlexandraRoatis added bug Something isn't working enhancement New feature or request labels Apr 4, 2019

AlexandraRoatis added this to the 0.3.4 milestone Apr 4, 2019

AlexandraRoatis self-assigned this Apr 4, 2019

AlexandraRoatis requested review from AionJayT, aionick and aion-kelvin April 4, 2019 19:19

AlexandraRoatis force-pushed the object-graph branch 2 times, most recently from e9396fa to dcc6730 Compare April 4, 2019 19:41

AlexandraRoatis modified the milestones: 0.3.4, 0.4.0 Apr 4, 2019

AlexandraRoatis added the wip Indicates that a PR or issue is Work In Progress, issuer should be notified before any actions label Apr 4, 2019

AlexandraRoatis mentioned this pull request Apr 4, 2019

FVM contract encoding update #867

Merged

12 tasks

AlexandraRoatis force-pushed the object-graph branch 7 times, most recently from 9732859 to 6798ea9 Compare April 8, 2019 19:48

AlexandraRoatis added 2 commits April 8, 2019 16:02

object graph & vm type added to kernel, repository & contract details

30da218

- new database for storing the object graph - functionality for saving the correct VM type into contract details - new encoding of contract details - interpret storage root based on the VM used

removing the auto-setting to dirty for contract details

da31446

AlexandraRoatis force-pushed the object-graph branch from 8718c99 to 30da218 Compare April 8, 2019 20:03

AlexandraRoatis removed the wip Indicates that a PR or issue is Work In Progress, issuer should be notified before any actions label Apr 8, 2019

AionJayT reviewed Apr 8, 2019

View reviewed changes

modAionImpl/src/org/aion/zero/impl/AionBlockchainImpl.java Outdated Show resolved Hide resolved

AionJayT reviewed Apr 8, 2019

View reviewed changes

correcting unit tests to save the VM type

0c51ca0

AlexandraRoatis force-pushed the object-graph branch from a70a714 to 0c51ca0 Compare April 8, 2019 21:13

saving the vm type when the object graph is stored

a016ec9

AlexandraRoatis force-pushed the object-graph branch from e36fce9 to a016ec9 Compare April 8, 2019 21:21

aionick suggested changes Apr 9, 2019

View reviewed changes

AlexandraRoatis force-pushed the object-graph branch from 98363ff to 9ef5b41 Compare April 9, 2019 15:43

bug fix: removing the dirty setting cause a break in consensus for co…

1a79fb6

…ntracts deployed without code or storage

AlexandraRoatis force-pushed the object-graph branch from 9ef5b41 to 1a79fb6 Compare April 9, 2019 16:07

set vm type for empty contracts to avoid consensus issues

c04fb84

AlexandraRoatis mentioned this pull request Apr 9, 2019

Object graph changes for AVM testnet reset #873

Merged

AlexandraRoatis closed this Apr 10, 2019

AlexandraRoatis deleted the object-graph branch June 3, 2019 18:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Contract storage enhancement for use of object graph #866

Contract storage enhancement for use of object graph #866

AlexandraRoatis commented Apr 4, 2019 •

edited

Loading

AionJayT Apr 8, 2019

AionJayT Apr 8, 2019

AlexandraRoatis Apr 8, 2019

aionick Apr 9, 2019

AlexandraRoatis Apr 10, 2019

aionick Apr 9, 2019

aionick Apr 9, 2019

aionick Apr 9, 2019

AlexandraRoatis Apr 10, 2019

aionick Apr 9, 2019

AlexandraRoatis Apr 9, 2019

aionick Apr 9, 2019

AlexandraRoatis Apr 9, 2019

aionick Apr 9, 2019

AlexandraRoatis Apr 9, 2019

aionick Apr 9, 2019

AlexandraRoatis Apr 9, 2019 •

edited

Loading

aionick Apr 9, 2019 •

edited

Loading

AlexandraRoatis Apr 10, 2019

Contract storage enhancement for use of object graph #866

Contract storage enhancement for use of object graph #866

Conversation

AlexandraRoatis commented Apr 4, 2019 • edited Loading

Description

Type of change

Testing

Verification

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexandraRoatis Apr 9, 2019 • edited Loading

Choose a reason for hiding this comment

aionick Apr 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexandraRoatis commented Apr 4, 2019 •

edited

Loading

AlexandraRoatis Apr 9, 2019 •

edited

Loading

aionick Apr 9, 2019 •

edited

Loading