Bonsai trie : clone updater during persist (#2261)

* clone updater to avoid issue

Signed-off-by: Karim TAAM <karim.t2am@gmail.com>

* clean code

Signed-off-by: Karim TAAM <karim.t2am@gmail.com>

* fix roothash calculation

Signed-off-by: Karim TAAM <karim.t2am@gmail.com>

* fix invalid slot value verification during rollback

Signed-off-by: Karim TAAM <karim.t2am@gmail.com>

Co-authored-by: Sally MacFarlane <sally.macfarlane@consensys.net>
Co-authored-by: garyschulte <garyschulte@gmail.com>

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/BonsaiPersistedWorldState.java

@@ -100,8 +100,14 @@ public class BonsaiPersistedWorldState implements MutableWorldState, BonsaiWorld
   }
 
   protected Hash calculateRootHash(final BonsaiWorldStateKeyValueStorage.Updater stateUpdater) {
+    return calculateRootHash(stateUpdater, updater.copy());
+  }
+
+  protected Hash calculateRootHash(
+      final BonsaiWorldStateKeyValueStorage.Updater stateUpdater,
+      final BonsaiWorldStateUpdater worldStateUpdater) {
     // first clear storage
-    for (final Address address : updater.getStorageToClear()) {
+    for (final Address address : worldStateUpdater.getStorageToClear()) {
       // because we are clearing persisted values we need the account root as persisted
       final BonsaiAccount oldAccount =
           worldStateStorage
@@ -138,12 +144,12 @@ public class BonsaiPersistedWorldState implements MutableWorldState, BonsaiWorld
     // second update account storage state.  This must be done before updating the accounts so
     // that we can get the storage state hash
     for (final Map.Entry<Address, Map<Hash, BonsaiValue<UInt256>>> storageAccountUpdate :
-        updater.getStorageToUpdate().entrySet()) {
+        worldStateUpdater.getStorageToUpdate().entrySet()) {
       final Address updatedAddress = storageAccountUpdate.getKey();
       final Hash updatedAddressHash = Hash.hash(updatedAddress);
-      if (updater.getAccountsToUpdate().containsKey(updatedAddress)) {
+      if (worldStateUpdater.getAccountsToUpdate().containsKey(updatedAddress)) {
         final BonsaiValue<BonsaiAccount> accountValue =
-            updater.getAccountsToUpdate().get(updatedAddress);
+            worldStateUpdater.getAccountsToUpdate().get(updatedAddress);
         final BonsaiAccount accountOriginal = accountValue.getPrior();
         final Hash storageRoot =
             (accountOriginal == null) ? Hash.EMPTY_TRIE_HASH : accountOriginal.getStorageRoot();
@@ -185,7 +191,7 @@ public class BonsaiPersistedWorldState implements MutableWorldState, BonsaiWorld
 
     // Third update the code.  This has the side effect of ensuring a code hash is calculated.
     for (final Map.Entry<Address, BonsaiValue<Bytes>> codeUpdate :
-        updater.getCodeToUpdate().entrySet()) {
+        worldStateUpdater.getCodeToUpdate().entrySet()) {
       final Bytes updatedCode = codeUpdate.getValue().getUpdated();
       final Hash accountHash = Hash.hash(codeUpdate.getKey());
       if (updatedCode == null || updatedCode.size() == 0) {
@@ -207,7 +213,7 @@ public class BonsaiPersistedWorldState implements MutableWorldState, BonsaiWorld
 
     // now add the accounts
     for (final Map.Entry<Address, BonsaiValue<BonsaiAccount>> accountUpdate :
-        updater.getAccountsToUpdate().entrySet()) {
+        worldStateUpdater.getAccountsToUpdate().entrySet()) {
       final Bytes accountKey = accountUpdate.getKey();
       final BonsaiValue<BonsaiAccount> bonsaiValue = accountUpdate.getValue();
       final BonsaiAccount updatedAccount = bonsaiValue.getUpdated();
@@ -235,12 +241,15 @@ public class BonsaiPersistedWorldState implements MutableWorldState, BonsaiWorld
   @Override
   public void persist(final BlockHeader blockHeader) {
     boolean success = false;
+
+    final BonsaiWorldStateUpdater localUpdater = updater.copy();
+
     final Hash originalBlockHash = worldStateBlockHash;
     final Hash originalRootHash = worldStateRootHash;
     final BonsaiWorldStateKeyValueStorage.Updater stateUpdater = worldStateStorage.updater();
 
     try {
-      worldStateRootHash = calculateRootHash(stateUpdater);
+      worldStateRootHash = calculateRootHash(stateUpdater, localUpdater);
       stateUpdater
           .getTrieBranchStorageTransaction()
           .put(WORLD_ROOT_HASH_KEY, worldStateRootHash.toArrayUnsafe());
@@ -262,7 +271,7 @@ public class BonsaiPersistedWorldState implements MutableWorldState, BonsaiWorld
             .put(WORLD_BLOCK_HASH_KEY, worldStateBlockHash.toArrayUnsafe());
         if (originalBlockHash.equals(blockHeader.getParentHash())) {
           LOG.debug("Writing Trie Log for {}", worldStateBlockHash);
-          final TrieLogLayer trieLog = updater.generateTrieLog(worldStateBlockHash);
+          final TrieLogLayer trieLog = localUpdater.generateTrieLog(worldStateBlockHash);
           trieLog.freeze();
           archive.addLayeredWorldState(this, blockHeader, worldStateRootHash, trieLog);
           final BytesValueRLPOutput rlpLog = new BytesValueRLPOutput();
@@ -331,7 +340,8 @@ public class BonsaiPersistedWorldState implements MutableWorldState, BonsaiWorld
   @Override
   public Hash frontierRootHash() {
     return calculateRootHash(
-        new BonsaiWorldStateKeyValueStorage.Updater(noOpTx, noOpTx, noOpTx, noOpTx, noOpTx));
+        new BonsaiWorldStateKeyValueStorage.Updater(noOpTx, noOpTx, noOpTx, noOpTx, noOpTx),
+        updater.copy());
   }
 
   public Hash blockHash() {

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/BonsaiWorldStateUpdater.java

@@ -47,20 +47,30 @@ import org.apache.tuweni.units.bigints.UInt256;
 public class BonsaiWorldStateUpdater extends AbstractWorldUpdater<BonsaiWorldView, BonsaiAccount>
     implements BonsaiWorldView {
 
-  private final Map<Address, BonsaiValue<BonsaiAccount>> accountsToUpdate = new HashMap<>();
-  private final Map<Address, BonsaiValue<Bytes>> codeToUpdate = new HashMap<>();
-  private final Set<Address> storageToClear = new HashSet<>();
+  private Map<Address, BonsaiValue<BonsaiAccount>> accountsToUpdate = new HashMap<>();
+  private Map<Address, BonsaiValue<Bytes>> codeToUpdate = new HashMap<>();
+  private Set<Address> storageToClear = new HashSet<>();
 
   // storage sub mapped by _hashed_ key.  This is because in self_destruct calls we need to
   // enumerate the old storage and delete it.  Those are trie stored by hashed key by spec and the
   // alternative was to keep a giant pre-image cache of the entire trie.
-  private final Map<Address, Map<Hash, BonsaiValue<UInt256>>> storageToUpdate =
-      new ConcurrentHashMap<>();
+  private Map<Address, Map<Hash, BonsaiValue<UInt256>>> storageToUpdate = new ConcurrentHashMap<>();
 
   BonsaiWorldStateUpdater(final BonsaiWorldView world) {
     super(world);
   }
 
+  public BonsaiWorldStateUpdater copy() {
+    final BonsaiWorldStateUpdater copy = new BonsaiWorldStateUpdater(wrappedWorldView());
+    copy.accountsToUpdate = new HashMap<>(accountsToUpdate);
+    copy.codeToUpdate = new HashMap<>(codeToUpdate);
+    copy.storageToClear = new HashSet<>(storageToClear);
+    copy.storageToUpdate = new ConcurrentHashMap<>(storageToUpdate);
+    copy.updatedAccounts = new HashMap<>(updatedAccounts);
+    copy.deletedAccounts = new HashSet<>(deletedAccounts);
+    return copy;
+  }
+
   @Override
   public Account get(final Address address) {
     return super.get(address);
@@ -617,7 +627,7 @@ public class BonsaiWorldStateUpdater extends AbstractWorldUpdater<BonsaiWorldVie
                 "Expected to create slot, but the slot exists. Account=%s SlotHash=%s expectedValue=%s existingValue=%s",
                 address, slotHash, expectedValue, existingSlotValue));
       }
-      if (!Objects.equals(expectedValue, existingSlotValue)) {
+      if (!isSlotEquals(expectedValue, existingSlotValue)) {
         throw new IllegalStateException(
             String.format(
                 "Old value of slot does not match expected value. Account=%s SlotHash=%s Expected=%s Actual=%s",
@@ -639,6 +649,13 @@ public class BonsaiWorldStateUpdater extends AbstractWorldUpdater<BonsaiWorldVie
     }
   }
 
+  private boolean isSlotEquals(final UInt256 expectedValue, final UInt256 existingSlotValue) {
+    final UInt256 sanitizedExpectedValue = (expectedValue == null) ? UInt256.ZERO : expectedValue;
+    final UInt256 sanitizedExistingSlotValue =
+        (existingSlotValue == null) ? UInt256.ZERO : existingSlotValue;
+    return Objects.equals(sanitizedExpectedValue, sanitizedExistingSlotValue);
+  }
+
   @Override
   public void reset() {
     storageToClear.clear();

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/AbstractWorldUpdater.java

@@ -33,8 +33,8 @@ public abstract class AbstractWorldUpdater<W extends WorldView, A extends Accoun
 
   private final W world;
 
-  private final Map<Address, UpdateTrackingAccount<A>> updatedAccounts = new HashMap<>();
-  private final Set<Address> deletedAccounts = new HashSet<>();
+  protected Map<Address, UpdateTrackingAccount<A>> updatedAccounts = new HashMap<>();
+  protected Set<Address> deletedAccounts = new HashSet<>();
 
   protected AbstractWorldUpdater(final W world) {
     this.world = world;