Merge pull request #199 from harmony-dev/fix/fork-choice-issues

Fix/fork choice issues
harmony-dev · Sep 23, 2019 · 8d653af · 8d653af
2 parents 07d2003 + cf24e15
commit 8d653af
Show file tree

Hide file tree

Showing 4 changed files with 70 additions and 16 deletions.
diff --git a/chain/src/main/java/org/ethereum/beacon/chain/DefaultBeaconChain.java b/chain/src/main/java/org/ethereum/beacon/chain/DefaultBeaconChain.java
@@ -126,7 +126,7 @@ public synchronized ImportResult insert(BeaconBlock block) {
 
     BeaconTuple newTuple = BeaconTuple.of(block, postBlockState);
     tupleStorage.put(newTuple);
-    updateFinality(parentState, postBlockState);
+    updateFinality(postBlockState);
 
     chainStorage.commit();
 
@@ -153,21 +153,32 @@ public BeaconTuple getRecentlyProcessed() {
     return recentlyProcessed;
   }
 
-  private void updateFinality(BeaconState previous, BeaconState current) {
-    if (!previous.getFinalizedCheckpoint().equals(current.getFinalizedCheckpoint())) {
+  private void updateFinality(BeaconState current) {
+    boolean finalizedStorageUpdated = false;
+    boolean justifiedStorageUpdated = false;
+    if (current
+        .getFinalizedCheckpoint()
+        .getEpoch()
+        .greater(fetchFinalizedCheckpoint().getEpoch())) {
       chainStorage.getFinalizedStorage().set(current.getFinalizedCheckpoint());
+      finalizedStorageUpdated = true;
+    }
+    if (current
+        .getCurrentJustifiedCheckpoint()
+        .getEpoch()
+        .greater(fetchJustifiedCheckpoint().getEpoch())) {
+      chainStorage.getJustifiedStorage().set(current.getCurrentJustifiedCheckpoint());
+      justifiedStorageUpdated = true;
+    }
+    // publish updates after both storages have been updated
+    // the order can be important if a finalizedCheckpointStream subscriber will look
+    // into justified storage
+    // in general, it may be important to publish after commit has succeeded
+    if (finalizedStorageUpdated) {
       finalizedCheckpointStream.onNext(current.getFinalizedCheckpoint());
     }
-    if (!previous.getCurrentJustifiedCheckpoint().equals(current.getCurrentJustifiedCheckpoint())) {
-      // store new justified checkpoint if its epoch greater than previous one
-      if (current
-          .getCurrentJustifiedCheckpoint()
-          .getEpoch()
-          .greater(fetchJustifiedCheckpoint().getEpoch())) {
-        chainStorage.getJustifiedStorage().set(current.getCurrentJustifiedCheckpoint());
-      }
-
-      justifiedCheckpointStream.onNext(current.getFinalizedCheckpoint());
+    if (justifiedStorageUpdated) {
+      justifiedCheckpointStream.onNext(current.getCurrentJustifiedCheckpoint());
     }
   }
 

diff --git a/consensus/src/main/java/org/ethereum/beacon/consensus/spec/EpochProcessing.java b/consensus/src/main/java/org/ethereum/beacon/consensus/spec/EpochProcessing.java
@@ -131,7 +131,7 @@ default Pair<Crosslink, List<ValidatorIndex>> get_winning_crosslink_and_attestin
           Gwei b2 = get_attesting_balance(state,
               attestations.stream().filter(a -> a.getData().getCrosslink().equals(c2)).collect(toList()));
           if (b1.equals(b2)) {
-            return c1.getDataRoot().toString().compareTo(c2.getDataRoot().toString());
+            return c1.getDataRoot().compareTo(c2.getDataRoot());
           } else {
             return b1.compareTo(b2);
           }

diff --git a/consensus/src/main/java/org/ethereum/beacon/consensus/spec/ForkChoice.java b/consensus/src/main/java/org/ethereum/beacon/consensus/spec/ForkChoice.java
@@ -10,6 +10,7 @@
 import org.ethereum.beacon.core.types.Gwei;
 import org.ethereum.beacon.core.types.SlotNumber;
 import org.ethereum.beacon.core.types.ValidatorIndex;
+import org.javatuples.Pair;
 import tech.pegasys.artemis.ethereum.core.Hash32;
 
 /**
@@ -109,7 +110,7 @@ default Hash32 get_head(Store store) {
       }
 
       head = children.stream()
-          .max(Comparator.comparing(root -> get_latest_attesting_balance(store, root)))
+          .max(Comparator.comparing(root -> Pair.with(get_latest_attesting_balance(store, root), root)))
           .get();
     }
   }

diff --git a/types/src/test/java/tech/pegasys/artemis/util/bytes/BytesValueTest.java b/types/src/test/java/tech/pegasys/artemis/util/bytes/BytesValueTest.java
@@ -16,7 +16,6 @@
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.Unpooled;
 import io.vertx.core.buffer.Buffer;
-import net.consensys.cava.bytes.MutableBytes;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
@@ -857,6 +856,49 @@ public void testBytesValuesComparatorReturnsMatchUnsignedValueByteValue() {
     assertThat(small.compareTo(otherSmall)).isEqualTo(0);
   }
 
+  @Test
+  public void testBytesValueComparator_lexicographical_order() {
+    checkOrder(h("0x00"), h("0x01"));
+    checkOrder(h("0x01"), h("0x7f"));
+    checkOrder(h("0x7f"), h("0x80"));
+    checkOrder(h("0x80"), h("0xff"));
+
+    checkOrder(h("0x0000"), h("0x0001"));
+    checkOrder(h("0x0001"), h("0x007f"));
+    checkOrder(h("0x007f"), h("0x0080"));
+    checkOrder(h("0x0080"), h("0x00ff"));
+
+    checkOrder(h("0x0000"), h("0x0100"));
+    checkOrder(h("0x0100"), h("0x7f00"));
+    checkOrder(h("0x7f00"), h("0x8000"));
+    checkOrder(h("0x8000"), h("0xff00"));
+
+    checkOrder(h("0x00"), h("0x0100"));
+    checkOrder(h("0x01"), h("0x7f00"));
+    checkOrder(h("0x7f"), h("0x8000"));
+    checkOrder(h("0x80"), h("0xff00"));
+
+    checkOrder(h("0x00"), h("0x01ff"));
+    checkOrder(h("0x01"), h("0x7fff"));
+    checkOrder(h("0x7f"), h("0x80ff"));
+    checkOrder(h("0x80"), h("0xffff"));
+
+    checkOrder(h("0x0001"), h("0x0100"));
+    checkOrder(h("0x007f"), h("0x7f00"));
+    checkOrder(h("0x0080"), h("0x8000"));
+    checkOrder(h("0x00ff"), h("0xff00"));
+
+    checkOrder(h("0x000001"), h("0x010000"));
+    checkOrder(h("0x00007f"), h("0x7f0000"));
+    checkOrder(h("0x000080"), h("0x800000"));
+    checkOrder(h("0x0000ff"), h("0xff0000"));
+  }
+
+  static void checkOrder(BytesValue lesser, BytesValue greater) {
+    assertThat(lesser).isLessThan(greater);
+    assertThat(greater).isGreaterThan(lesser);
+  }
+
   @Test
   public void testGetSetBit() {
     MutableBytesValue bytes = MutableBytesValue.create(4);