ContractDetails refactoring and testing

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

@@ -34,11 +34,9 @@ public class AionContractDetailsImpl implements ContractDetails {
     private boolean dirty = false;
     private boolean deleted = false;
 
-    // a value > 0 indicates that prune should be for that many blocks.
-    private int prune = 0;
-    // indicates the maximum storage size before shifting to the storage database
-    // NOTE: updating this value can lead to incompatible data storage
-    private int detailsInMemoryStorageLimit = 64 * 1024;
+    /** Indicates the maximum storage size before shifting to the storage database. */
+    @VisibleForTesting
+    static int detailsInMemoryStorageLimit = 64 * 1024;
 
     private Map<ByteArrayWrapper, byte[]> codes = new HashMap<>();
     // classes extending this rely on this value starting off as null
@@ -50,28 +48,17 @@ public class AionContractDetailsImpl implements ContractDetails {
     private ByteArrayKeyValueStore dataSource;
     private ByteArrayKeyValueStore objectGraphSource = null;
 
-    private byte[] rlpEncoded;
-
     private AionAddress address;
 
     private SecureTrie storageTrie = new SecureTrie(null);
 
-    public boolean externalStorage;
-    private ByteArrayKeyValueStore externalStorageDataSource;
-    private ByteArrayKeyValueStore contractObjectGraphSource = null;
+    private boolean externalStorage;
 
     private byte[] objectGraphHash = EMPTY_DATA_HASH;
     private byte[] concatenatedStorageHash = EMPTY_DATA_HASH;
 
     public AionContractDetailsImpl() {}
 
-    @VisibleForTesting
-    AionContractDetailsImpl(int prune, int memStorageLimit) {
-        this.prune = prune;
-        // NOTE: updating this value can lead to incompatible data storage
-        this.detailsInMemoryStorageLimit = memStorageLimit;
-    }
-
     /**
      * Creates a object with attached database access for the storage and object graph.
      *
@@ -102,6 +89,11 @@ public AionContractDetailsImpl(byte[] code) throws Exception {
         decode(code);
     }
 
+    @VisibleForTesting
+    public boolean isExternalStorage() {
+        return externalStorage;
+    }
+
     @Override
     public byte[] getCode(byte[] codeHash) {
         if (java.util.Arrays.equals(codeHash, EMPTY_DATA_HASH)) {
@@ -122,12 +114,15 @@ private void setCodes(Map<ByteArrayWrapper, byte[]> codes) {
 
     @Override
     public void appendCodes(Map<ByteArrayWrapper, byte[]> codes) {
+        if (!this.codes.keySet().containsAll(codes.keySet())) {
+            this.dirty = true;
+        }
         this.codes.putAll(codes);
     }
 
     @Override
-    public void setDirty(boolean dirty) {
-        this.dirty = dirty;
+    public void markAsDirty() {
+        this.dirty = true;
     }
 
     @Override
@@ -136,6 +131,7 @@ public boolean isDirty() {
     }
 
     public void delete() {
+        // TODO: should we set dirty=true?
         this.deleted = true;
     }
 
@@ -185,8 +181,7 @@ public void put(ByteArrayWrapper key, ByteArrayWrapper value) {
         byte[] data = RLP.encodeElement(value.toBytes());
         storageTrie.update(key.toBytes(), data);
 
-        setDirty(true);
-        rlpEncoded = null;
+        dirty = true;
     }
 
     @Override
@@ -195,8 +190,7 @@ public void delete(ByteArrayWrapper key) {
 
         storageTrie.delete(key.toBytes());
 
-        setDirty(true);
-        rlpEncoded = null;
+        dirty = true;
     }
 
     /**
@@ -237,8 +231,7 @@ public void setCode(byte[] code) {
             e.printStackTrace();
             return;
         }
-        setDirty(true);
-        rlpEncoded = null;
+        dirty = true;
     }
 
     @Override
@@ -264,8 +257,7 @@ public void setObjectGraph(byte[] graph) {
         this.objectGraph = graph;
         this.objectGraphHash = h256(objectGraph);
 
-        this.setDirty(true);
-        this.rlpEncoded = null;
+        dirty = true;
     }
 
     /**
@@ -305,39 +297,23 @@ private byte[] computeAvmStorageHash() {
      */
     public void decode(byte[] rlpCode) {
         // TODO: remove vm type requirement when refactoring into separate AVM & FVM implementations
-        decode(rlpCode, false);
-    }
-
-    /**
-     * Decodes an AionContractDetailsImpl object from the RLP encoding rlpCode. The fast check flag
-     * indicates whether the contractDetails needs to sync with external storage.
-     *
-     * @param rlpCode The encoding to decode.
-     * @param fastCheck indicates whether the contractDetails needs to sync with external storage.
-     */
-    public void decode(byte[] rlpCode, boolean fastCheck) {
         RLPList data = RLP.decode2(rlpCode);
 
         RLPList rlpList = (RLPList) data.get(0);
 
         // partial decode either encoding
-        boolean keepStorageInMem = decodeEncodingWithoutVmType(rlpList, fastCheck);
+        boolean keepStorageInMem = decodeEncodingWithoutVmType(rlpList);
 
         if (rlpList.size() != 5) {
             // revert back from storing the VM type in details
             // force a save with new encoding
             throw new IllegalStateException(
                     "Incompatible data storage. Please shutdown the kernel and perform database migration to version 1.0 (Denali) of the kernel as instructed in the release.");
-        } else {
-            // keep encoding when compatible with new style
-            this.rlpEncoded = rlpCode;
         }
 
-        if (!fastCheck || externalStorage || !keepStorageInMem) { // it was not a fast check
-            // NOTE: under normal circumstances the VM type is set by the details data store
-            // Do not forget to set the vmType value externally during tests!!!
-            decodeStorage(rlpList.get(2), rlpList.get(3), keepStorageInMem);
-        }
+        // NOTE: under normal circumstances the VM type is set by the details data store
+        // Do not forget to set the vmType value externally during tests!!!
+        decodeStorage(rlpList.get(2), rlpList.get(3), keepStorageInMem);
     }
 
     /**
@@ -351,17 +327,12 @@ public void decode(byte[] rlpCode, boolean fastCheck) {
      * @return {@code true} if the storage must continue to be kept in memory, {@code false}
      *     otherwise
      */
-    public boolean decodeEncodingWithoutVmType(RLPList rlpList, boolean fastCheck) {
+    public boolean decodeEncodingWithoutVmType(RLPList rlpList) {
         RLPItem isExternalStorage = (RLPItem) rlpList.get(1);
         RLPItem storage = (RLPItem) rlpList.get(3);
         this.externalStorage = isExternalStorage.getRLPData().length > 0;
         boolean keepStorageInMem = storage.getRLPData().length <= detailsInMemoryStorageLimit;
 
-        // No externalStorage require.
-        if (fastCheck && !externalStorage && keepStorageInMem) {
-            return keepStorageInMem;
-        }
-
         RLPItem address = (RLPItem) rlpList.get(0);
         RLPElement code = rlpList.get(4);
 
@@ -423,7 +394,6 @@ public void decodeStorage(RLPElement root, RLPElement storage, boolean keepStora
         } else {
             storageTrie.deserialize(storage.getRLPData());
         }
-        storageTrie.withPruningEnabled(prune > 0);
 
         // switch from in-memory to external storage
         if (!externalStorage && !keepStorageInMem) {
@@ -441,34 +411,24 @@ public void decodeStorage(RLPElement root, RLPElement storage, boolean keepStora
      * @return an rlp encoding of this.
      */
     public byte[] getEncoded() {
-        if (rlpEncoded == null) {
-
-            byte[] rlpAddress = RLP.encodeElement(address.toByteArray());
-            byte[] rlpIsExternalStorage = RLP.encodeByte((byte) (externalStorage ? 1 : 0));
-            byte[] rlpStorageRoot;
-            // encoding for AVM
-            if (vmType == InternalVmType.AVM) {
-                rlpStorageRoot = RLP.encodeElement(computeAvmStorageHash());
-            } else {
-                rlpStorageRoot =
-                        RLP.encodeElement(
-                                externalStorage ? storageTrie.getRootHash() : EMPTY_BYTE_ARRAY);
-            }
-            byte[] rlpStorage =
-                    RLP.encodeElement(externalStorage ? EMPTY_BYTE_ARRAY : storageTrie.serialize());
-            byte[][] codes = new byte[getCodes().size()][];
-            int i = 0;
-            for (byte[] bytes : this.getCodes().values()) {
-                codes[i++] = RLP.encodeElement(bytes);
-            }
-            byte[] rlpCode = RLP.encodeList(codes);
-
-            this.rlpEncoded =
-                    RLP.encodeList(
-                            rlpAddress, rlpIsExternalStorage, rlpStorageRoot, rlpStorage, rlpCode);
+        byte[] rlpAddress = RLP.encodeElement(address.toByteArray());
+        byte[] rlpIsExternalStorage = RLP.encodeByte((byte) (externalStorage ? 1 : 0));
+        byte[] rlpStorageRoot;
+        // encoding for AVM
+        if (vmType == InternalVmType.AVM) {
+            rlpStorageRoot = RLP.encodeElement(computeAvmStorageHash());
+        } else {
+            rlpStorageRoot = RLP.encodeElement(externalStorage ? storageTrie.getRootHash() : EMPTY_BYTE_ARRAY);
         }
+        byte[] rlpStorage = RLP.encodeElement(externalStorage ? EMPTY_BYTE_ARRAY : storageTrie.serialize());
+        byte[][] codes = new byte[getCodes().size()][];
+        int i = 0;
+        for (byte[] bytes : this.getCodes().values()) {
+            codes[i++] = RLP.encodeElement(bytes);
+        }
+        byte[] rlpCode = RLP.encodeList(codes);
 
-        return rlpEncoded;
+        return RLP.encodeList(rlpAddress, rlpIsExternalStorage, rlpStorageRoot, rlpStorage, rlpCode);
     }
 
     /**
@@ -495,7 +455,6 @@ public void setAddress(AionAddress address) {
         if (ContractInfo.isPrecompiledContract(address)) {
             setVmType(InternalVmType.FVM);
         }
-        this.rlpEncoded = null;
     }
 
     /** Syncs the storage trie. */
@@ -529,15 +488,6 @@ public void syncStorage() {
         }
     }
 
-    /**
-     * Sets the storage data source.
-     *
-     * @param dataSource The new dataSource.
-     */
-    public void setDataSource(ByteArrayKeyValueStore dataSource) {
-        this.dataSource = dataSource;
-    }
-
     /**
      * Sets the data source for storing the AVM object graph.
      *
@@ -553,12 +503,7 @@ public void setObjectGraphSource(ByteArrayKeyValueStore objectGraphSource) {
      * @return the external storage data source.
      */
     private ByteArrayKeyValueStore getExternalStorageDataSource() {
-        if (externalStorageDataSource == null) {
-            externalStorageDataSource =
-                    new XorDataSource(
-                            dataSource, h256(("details-storage/" + address.toString()).getBytes()));
-        }
-        return externalStorageDataSource;
+        return new XorDataSource(dataSource, h256(("details-storage/" + address.toString()).getBytes()));
     }
 
     /**
@@ -567,31 +512,18 @@ private ByteArrayKeyValueStore getExternalStorageDataSource() {
      * @return the data source specific to the current contract.
      */
     private ByteArrayKeyValueStore getContractObjectGraphSource() {
-        if (contractObjectGraphSource == null) {
-            if (objectGraphSource == null) {
-                throw new NullPointerException(
-                        "The contract object graph source was not initialized.");
-            } else {
-                contractObjectGraphSource =
-                        new XorDataSource(
-                                objectGraphSource,
-                                h256(("details-graph/" + address.toString()).getBytes()));
-            }
+        if (objectGraphSource == null) {
+            throw new NullPointerException("The contract object graph source was not initialized.");
+        } else {
+            return new XorDataSource(objectGraphSource, h256(("details-graph/" + address.toString()).getBytes()));
         }
-        return contractObjectGraphSource;
     }
 
     /**
      * Sets the external storage data source to dataSource.
-     *
-     * @param dataSource The new data source.
-     * @implNote The tests are taking a shortcut here in bypassing the XorDataSource created by
-     *     {@link #getExternalStorageDataSource()}. Do not use this method in production.
      */
     @VisibleForTesting
-    void setExternalStorageDataSource(ByteArrayKeyValueStore dataSource) {
-        // TODO: regarding the node above: the tests should be updated and the method removed
-        this.externalStorageDataSource = dataSource;
+    void initializeExternalStorageTrieForTest() {
         this.externalStorage = true;
         this.storageTrie = new SecureTrie(getExternalStorageDataSource());
     }
@@ -663,7 +595,6 @@ public AionContractDetailsImpl getSnapshotTo(byte[] hash, InternalVmType vm) {
 
         // storage information
         details.externalStorage = this.externalStorage;
-        details.externalStorageDataSource = this.externalStorageDataSource;
         details.dataSource = dataSource;
 
         return details;
@@ -692,12 +623,10 @@ public AionContractDetailsImpl copy() {
 
         // storage information
         aionContractDetailsCopy.dataSource = this.dataSource;
-        aionContractDetailsCopy.externalStorageDataSource = this.externalStorageDataSource;
         aionContractDetailsCopy.externalStorage = this.externalStorage;
 
         // object graph information
         aionContractDetailsCopy.objectGraphSource = this.objectGraphSource;
-        aionContractDetailsCopy.contractObjectGraphSource = this.contractObjectGraphSource;
         aionContractDetailsCopy.objectGraph =
                 objectGraph == null
                         ? null
@@ -714,16 +643,10 @@ public AionContractDetailsImpl copy() {
                         : Arrays.copyOf(
                                 this.concatenatedStorageHash, this.concatenatedStorageHash.length);
 
-        aionContractDetailsCopy.prune = this.prune;
-        aionContractDetailsCopy.detailsInMemoryStorageLimit = this.detailsInMemoryStorageLimit;
         aionContractDetailsCopy.setCodes(getDeepCopyOfCodes());
-        aionContractDetailsCopy.setDirty(this.isDirty());
+        aionContractDetailsCopy.dirty = this.dirty;
         aionContractDetailsCopy.deleted = this.deleted;
         aionContractDetailsCopy.address = this.address;
-        aionContractDetailsCopy.rlpEncoded =
-                (this.rlpEncoded == null)
-                        ? null
-                        : Arrays.copyOf(this.rlpEncoded, this.rlpEncoded.length);
         aionContractDetailsCopy.storageTrie =
                 (this.storageTrie == null) ? null : this.storageTrie.copy();
         return aionContractDetailsCopy;

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

@@ -63,8 +63,7 @@ public void createAccount(AionAddress address) {
 
             // TODO: unify contract details initialization from Impl and Track
             ContractDetails contractDetails = new ContractDetailsCacheImpl(null);
-            // TODO: refactor to use makeDirty() from AbstractState
-            contractDetails.setDirty(true);
+            contractDetails.markAsDirty();
             cachedDetails.put(address, contractDetails);
         } finally {
             lock.unlock();
@@ -278,8 +277,6 @@ public void saveCode(AionAddress address, byte[] code) {
             // preexisting code!
             ContractDetails contractDetails = getContractDetails(address);
             contractDetails.setCode(code);
-            // TODO: ensure that setDirty is done by the class itself
-            contractDetails.setDirty(true);
 
             // update the code hash
             getAccountState(address).setCodeHash(h256(code));