Remove non-transactional mutation from KeyValueStore (#500)

Put and remove are currently available on the KeyValueStorage interface
as non-transactional calls.  These are used nowhere other than test
code, all our production write calls are to the transactional interface.

* Remove the non-transactional mutation APIs from the interface
* Update the tests to use transactional writes
* Update getStartTransaction to just be startTransaction since it is
  not a property but a method.

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/storage/keyvalue/KeyValueStoragePrefixedKeyBlockchainStorage.java

@@ -105,7 +105,7 @@ public class KeyValueStoragePrefixedKeyBlockchainStorage implements BlockchainSt
 
   @Override
   public Updater updater() {
-    return new Updater(storage.getStartTransaction());
+    return new Updater(storage.startTransaction());
   }
 
   private List<TransactionReceipt> rlpDecodeTransactionReceipts(final BytesValue bytes) {

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/storage/keyvalue/KeyValueStorageWorldStateStorage.java

@@ -45,7 +45,7 @@ public class KeyValueStorageWorldStateStorage implements WorldStateStorage {
 
   @Override
   public Updater updater() {
-    return new Updater(keyValueStorage.getStartTransaction());
+    return new Updater(keyValueStorage.startTransaction());
   }
 
   public static class Updater implements WorldStateStorage.Updater {

ethereum/trie/src/main/java/tech/pegasys/pantheon/ethereum/trie/KeyValueMerkleStorage.java

@@ -49,7 +49,7 @@ public class KeyValueMerkleStorage implements MerkleStorage {
       // Nothing to do
       return;
     }
-    final KeyValueStorage.Transaction kvTx = keyValueStorage.getStartTransaction();
+    final KeyValueStorage.Transaction kvTx = keyValueStorage.startTransaction();
     for (final Map.Entry<Bytes32, BytesValue> entry : pendingUpdates.entrySet()) {
       kvTx.put(entry.getKey(), entry.getValue());
     }

services/kvstore/src/main/java/tech/pegasys/pantheon/services/kvstore/InMemoryKeyValueStorage.java

@@ -42,29 +42,7 @@ public class InMemoryKeyValueStorage implements KeyValueStorage {
   }
 
   @Override
-  public void put(final BytesValue key, final BytesValue value) {
+  public Transaction startTransaction() {
-    final Lock lock = rwLock.writeLock();
-    lock.lock();
-    try {
-      hashValueStore.put(key, value);
-    } finally {
-      lock.unlock();
-    }
-  }
-
-  @Override
-  public void remove(final BytesValue key) throws StorageException {
-    final Lock lock = rwLock.writeLock();
-    lock.lock();
-    try {
-      hashValueStore.remove(key);
-    } finally {
-      lock.unlock();
-    }
-  }
-
-  @Override
-  public Transaction getStartTransaction() {
     return new InMemoryTransaction();
   }
 

services/kvstore/src/main/java/tech/pegasys/pantheon/services/kvstore/KeyValueStorage.java

@@ -30,25 +30,12 @@ public interface KeyValueStorage extends Closeable {
    */
   Optional<BytesValue> get(BytesValue key) throws StorageException;
 
-  /**
-   * @param key Index into persistent data repository.
-   * @param value The value persisted at the key index.
-   */
-  void put(BytesValue key, BytesValue value) throws StorageException;
-
-  /**
-   * Remove the data corresponding to the given key.
-   *
-   * @param key Index into persistent data repository.
-   */
-  void remove(BytesValue key) throws StorageException;
-
   /**
    * Begins a transaction. Returns a transaction object that can be updated and committed.
    *
    * @return An object representing the transaction.
    */
-  Transaction getStartTransaction() throws StorageException;
+  Transaction startTransaction() throws StorageException;
 
   /**
    * Stream all stored key-value pairs.

services/kvstore/src/main/java/tech/pegasys/pantheon/services/kvstore/RocksDbKeyValueStorage.java

@@ -73,27 +73,7 @@ public class RocksDbKeyValueStorage implements KeyValueStorage, Closeable {
   }
 
   @Override
-  public void put(final BytesValue key, final BytesValue value) throws StorageException {
+  public Transaction startTransaction() throws StorageException {
-    throwIfClosed();
-    try {
-      db.put(key.extractArray(), value.extractArray());
-    } catch (final RocksDBException e) {
-      throw new StorageException(e);
-    }
-  }
-
-  @Override
-  public void remove(final BytesValue key) throws StorageException {
-    throwIfClosed();
-    try {
-      db.delete(key.extractArray());
-    } catch (final RocksDBException e) {
-      throw new StorageException(e);
-    }
-  }
-
-  @Override
-  public Transaction getStartTransaction() throws StorageException {
     throwIfClosed();
     final WriteOptions options = new WriteOptions();
     return new RocksDbTransaction(db.beginTransaction(options), options);

services/kvstore/src/test/java/tech/pegasys/pantheon/services/kvstore/AbstractKeyValueStorageTest.java

@@ -42,7 +42,9 @@ public abstract class AbstractKeyValueStorageTest {
     final KeyValueStorage store1 = createStore();
     final KeyValueStorage store2 = createStore();
 
-    store1.put(BytesValue.fromHexString("0001"), BytesValue.fromHexString("0FFF"));
+    Transaction tx = store1.startTransaction();
+    tx.put(BytesValue.fromHexString("0001"), BytesValue.fromHexString("0FFF"));
+    tx.commit();
     final Optional<BytesValue> result = store2.get(BytesValue.fromHexString("0001"));
     assertEquals(Optional.empty(), result);
   }
@@ -51,11 +53,15 @@ public abstract class AbstractKeyValueStorageTest {
   public void put() throws Exception {
     final KeyValueStorage store = createStore();
 
-    store.put(BytesValue.fromHexString("0F"), BytesValue.fromHexString("0ABC"));
+    Transaction tx = store.startTransaction();
+    tx.put(BytesValue.fromHexString("0F"), BytesValue.fromHexString("0ABC"));
+    tx.commit();
     assertEquals(
         Optional.of(BytesValue.fromHexString("0ABC")), store.get(BytesValue.fromHexString("0F")));
 
-    store.put(BytesValue.fromHexString("0F"), BytesValue.fromHexString("0DEF"));
+    tx = store.startTransaction();
+    tx.put(BytesValue.fromHexString("0F"), BytesValue.fromHexString("0DEF"));
+    tx.commit();
     assertEquals(
         Optional.of(BytesValue.fromHexString("0DEF")), store.get(BytesValue.fromHexString("0F")));
   }
@@ -63,15 +69,31 @@ public abstract class AbstractKeyValueStorageTest {
   @Test
   public void removeExisting() throws Exception {
     final KeyValueStorage store = createStore();
-    store.put(BytesValue.fromHexString("0F"), BytesValue.fromHexString("0ABC"));
+    Transaction tx = store.startTransaction();
-    store.remove(BytesValue.fromHexString("0F"));
+    tx.put(BytesValue.fromHexString("0F"), BytesValue.fromHexString("0ABC"));
+    tx.commit();
+    tx = store.startTransaction();
+    tx.remove(BytesValue.fromHexString("0F"));
+    tx.commit();
+    assertEquals(Optional.empty(), store.get(BytesValue.fromHexString("0F")));
+  }
+
+  @Test
+  public void removeExistingSameTransaction() throws Exception {
+    final KeyValueStorage store = createStore();
+    Transaction tx = store.startTransaction();
+    tx.put(BytesValue.fromHexString("0F"), BytesValue.fromHexString("0ABC"));
+    tx.remove(BytesValue.fromHexString("0F"));
+    tx.commit();
     assertEquals(Optional.empty(), store.get(BytesValue.fromHexString("0F")));
   }
 
   @Test
   public void removeNonExistent() throws Exception {
     final KeyValueStorage store = createStore();
-    store.remove(BytesValue.fromHexString("0F"));
+    Transaction tx = store.startTransaction();
+    tx.remove(BytesValue.fromHexString("0F"));
+    tx.commit();
     assertEquals(Optional.empty(), store.get(BytesValue.fromHexString("0F")));
   }
 
@@ -83,9 +105,11 @@ public abstract class AbstractKeyValueStorageTest {
         Arrays.asList(
             Entry.create(BytesValue.fromHexString("01"), BytesValue.fromHexString("0ABC")),
             Entry.create(BytesValue.fromHexString("02"), BytesValue.fromHexString("0DEF")));
+    Transaction tx = store.startTransaction();
     for (final Entry testEntry : testEntries) {
-      store.put(testEntry.getKey(), testEntry.getValue());
+      tx.put(testEntry.getKey(), testEntry.getValue());
     }
+    tx.commit();
 
     final List<Entry> actualEntries = store.entries().collect(Collectors.toList());
     testEntries.sort(Comparator.comparing(Entry::getKey));
@@ -106,7 +130,9 @@ public abstract class AbstractKeyValueStorageTest {
                 () -> {
                   try {
                     for (int i = 0; i < keyCount; i++) {
-                      store.put(BytesValues.toMinimalBytes(i), value);
+                      Transaction tx = store.startTransaction();
+                      tx.put(BytesValues.toMinimalBytes(i), value);
+                      tx.commit();
                     }
                   } finally {
                     finishedLatch.countDown();
@@ -134,12 +160,14 @@ public abstract class AbstractKeyValueStorageTest {
   public void transactionCommit() throws Exception {
     final KeyValueStorage store = createStore();
     // Add some values
-    store.put(BytesValue.of(1), BytesValue.of(1));
+    Transaction tx = store.startTransaction();
-    store.put(BytesValue.of(2), BytesValue.of(2));
+    tx.put(BytesValue.of(1), BytesValue.of(1));
-    store.put(BytesValue.of(3), BytesValue.of(3));
+    tx.put(BytesValue.of(2), BytesValue.of(2));
+    tx.put(BytesValue.of(3), BytesValue.of(3));
+    tx.commit();
 
     // Start transaction that adds, modifies, and removes some values
-    final Transaction tx = store.getStartTransaction();
+    tx = store.startTransaction();
     tx.put(BytesValue.of(2), BytesValue.of(3));
     tx.put(BytesValue.of(2), BytesValue.of(4));
     tx.remove(BytesValue.of(3));
@@ -164,12 +192,14 @@ public abstract class AbstractKeyValueStorageTest {
   public void transactionRollback() throws Exception {
     final KeyValueStorage store = createStore();
     // Add some values
-    store.put(BytesValue.of(1), BytesValue.of(1));
+    Transaction tx = store.startTransaction();
-    store.put(BytesValue.of(2), BytesValue.of(2));
+    tx.put(BytesValue.of(1), BytesValue.of(1));
-    store.put(BytesValue.of(3), BytesValue.of(3));
+    tx.put(BytesValue.of(2), BytesValue.of(2));
+    tx.put(BytesValue.of(3), BytesValue.of(3));
+    tx.commit();
 
     // Start transaction that adds, modifies, and removes some values
-    final Transaction tx = store.getStartTransaction();
+    tx = store.startTransaction();
     tx.put(BytesValue.of(2), BytesValue.of(3));
     tx.put(BytesValue.of(2), BytesValue.of(4));
     tx.remove(BytesValue.of(3));
@@ -193,21 +223,21 @@ public abstract class AbstractKeyValueStorageTest {
   @Test
   public void transactionCommitEmpty() throws Exception {
     final KeyValueStorage store = createStore();
-    final Transaction tx = store.getStartTransaction();
+    final Transaction tx = store.startTransaction();
     tx.commit();
   }
 
   @Test
   public void transactionRollbackEmpty() throws Exception {
     final KeyValueStorage store = createStore();
-    final Transaction tx = store.getStartTransaction();
+    final Transaction tx = store.startTransaction();
     tx.rollback();
   }
 
   @Test(expected = IllegalStateException.class)
   public void transactionPutAfterCommit() throws Exception {
     final KeyValueStorage store = createStore();
-    final Transaction tx = store.getStartTransaction();
+    final Transaction tx = store.startTransaction();
     tx.commit();
     tx.put(BytesValue.of(1), BytesValue.of(1));
   }
@@ -215,7 +245,7 @@ public abstract class AbstractKeyValueStorageTest {
   @Test(expected = IllegalStateException.class)
   public void transactionRemoveAfterCommit() throws Exception {
     final KeyValueStorage store = createStore();
-    final Transaction tx = store.getStartTransaction();
+    final Transaction tx = store.startTransaction();
     tx.commit();
     tx.remove(BytesValue.of(1));
   }
@@ -223,7 +253,7 @@ public abstract class AbstractKeyValueStorageTest {
   @Test(expected = IllegalStateException.class)
   public void transactionPutAfterRollback() throws Exception {
     final KeyValueStorage store = createStore();
-    final Transaction tx = store.getStartTransaction();
+    final Transaction tx = store.startTransaction();
     tx.rollback();
     tx.put(BytesValue.of(1), BytesValue.of(1));
   }
@@ -231,7 +261,7 @@ public abstract class AbstractKeyValueStorageTest {
   @Test(expected = IllegalStateException.class)
   public void transactionRemoveAfterRollback() throws Exception {
     final KeyValueStorage store = createStore();
-    final Transaction tx = store.getStartTransaction();
+    final Transaction tx = store.startTransaction();
     tx.rollback();
     tx.remove(BytesValue.of(1));
   }
@@ -239,7 +269,7 @@ public abstract class AbstractKeyValueStorageTest {
   @Test(expected = IllegalStateException.class)
   public void transactionCommitAfterRollback() throws Exception {
     final KeyValueStorage store = createStore();
-    final Transaction tx = store.getStartTransaction();
+    final Transaction tx = store.startTransaction();
     tx.rollback();
     tx.commit();
   }
@@ -247,7 +277,7 @@ public abstract class AbstractKeyValueStorageTest {
   @Test(expected = IllegalStateException.class)
   public void transactionCommitTwice() throws Exception {
     final KeyValueStorage store = createStore();
-    final Transaction tx = store.getStartTransaction();
+    final Transaction tx = store.startTransaction();
     tx.commit();
     tx.commit();
   }
@@ -255,7 +285,7 @@ public abstract class AbstractKeyValueStorageTest {
   @Test(expected = IllegalStateException.class)
   public void transactionRollbackAfterCommit() throws Exception {
     final KeyValueStorage store = createStore();
-    final Transaction tx = store.getStartTransaction();
+    final Transaction tx = store.startTransaction();
     tx.commit();
     tx.rollback();
   }
@@ -263,7 +293,7 @@ public abstract class AbstractKeyValueStorageTest {
   @Test(expected = IllegalStateException.class)
   public void transactionRollbackTwice() throws Exception {
     final KeyValueStorage store = createStore();
-    final Transaction tx = store.getStartTransaction();
+    final Transaction tx = store.startTransaction();
     tx.rollback();
     tx.rollback();
   }
@@ -272,8 +302,8 @@ public abstract class AbstractKeyValueStorageTest {
   public void twoTransactions() throws Exception {
     final KeyValueStorage store = createStore();
 
-    final Transaction tx1 = store.getStartTransaction();
+    final Transaction tx1 = store.startTransaction();
-    final Transaction tx2 = store.getStartTransaction();
+    final Transaction tx2 = store.startTransaction();
 
     tx1.put(BytesValue.of(1), BytesValue.of(1));
     tx2.put(BytesValue.of(2), BytesValue.of(2));
@@ -295,7 +325,7 @@ public abstract class AbstractKeyValueStorageTest {
         (value) ->
             new Thread(
                 () -> {
-                  final Transaction tx = store.getStartTransaction();
+                  final Transaction tx = store.startTransaction();
                   for (int i = 0; i < keyCount; i++) {
                     tx.put(BytesValues.toMinimalBytes(i), value);
                   }