PR feedback

Author: cburgdorf

eth/db/chain_gaps.py

@@ -24,20 +24,20 @@ class GapChange(enum.Enum):
 
 def calculate_gaps(newly_persisted: BlockNumber, base_gaps: ChainGaps) -> GapInfo:
 
-    current_gaps, known_missing_tip = base_gaps
+    current_gaps, tip_child = base_gaps
 
-    if newly_persisted == known_missing_tip:
+    if newly_persisted == tip_child:
         # This is adding a consecutive header at the very tail
         new_gaps = (current_gaps, BlockNumber(newly_persisted + 1))
         gap_change = GapChange.TailWrite
-    elif newly_persisted > known_missing_tip:
+    elif newly_persisted > tip_child:
         # We are creating a gap in the chain
         gap_end = BlockNumber(newly_persisted - 1)
         new_gaps = (
-            current_gaps + ((known_missing_tip, gap_end),), BlockNumber(newly_persisted + 1)
+            current_gaps + ((tip_child, gap_end),), BlockNumber(newly_persisted + 1)
         )
         gap_change = GapChange.NewGap
-    elif newly_persisted < known_missing_tip:
+    elif newly_persisted < tip_child:
         # We are patching a gap which may either shrink an existing gap or divide it
         matching_gaps = [
             (index, pair) for index, pair in enumerate(current_gaps)
@@ -78,7 +78,7 @@ def calculate_gaps(newly_persisted: BlockNumber, base_gaps: ChainGaps) -> GapInf
 
             before_gap = current_gaps[:gap_index]
             after_gap = current_gaps[gap_index + 1:]
-            new_gaps = (before_gap + updated_center + after_gap, known_missing_tip)
+            new_gaps = (before_gap + updated_center + after_gap, tip_child)
 
     else:
         raise Exception("Invariant")

eth/db/header.py

@@ -256,15 +256,7 @@ def _persist_header_chain(
         )
         score = cls._set_hash_scores_to_db(db, curr_chain_head, score)
         gap_change, gaps = cls._update_header_chain_gaps(db, curr_chain_head)
-        if gap_change in GAP_WRITES:
-            # The caller implicitly asserts that persisted headers are canonical here.
-            # This assertion is made when persisting headers that are known to be part of a gap
-            # in the canonical chain. While we do validate that the headers are valid, we can not
-            # be 100 % sure that they will eventually lead to the expected child that fills the gap.
-            # E.g. there is nothing preventing one from persisting an uncle as the final header to
-            # close the gap, even if that will not be the parent of the next consecutive header.
-            # Py-EVM makes the assumption that client code will check and prevent such a scenario.
-            cls._add_block_number_to_hash_lookup(db, curr_chain_head)
+        cls._handle_gap_change(db, gap_change, curr_chain_head, genesis_parent_hash)
 
         orig_headers_seq = concat([(first_header,), headers_iterator])
         for parent, child in sliding_window(2, orig_headers_seq):
@@ -283,8 +275,7 @@ def _persist_header_chain(
 
             score = cls._set_hash_scores_to_db(db, curr_chain_head, score)
             gap_change, gaps = cls._update_header_chain_gaps(db, curr_chain_head, gaps)
-            if gap_change in GAP_WRITES:
-                cls._add_block_number_to_hash_lookup(db, curr_chain_head)
+            cls._handle_gap_change(db, gap_change, curr_chain_head, genesis_parent_hash)
         try:
             previous_canonical_head = cls._get_canonical_head_hash(db)
             head_score = cls._get_score(db, previous_canonical_head)
@@ -296,6 +287,47 @@ def _persist_header_chain(
 
         return tuple(), tuple()
 
+    @classmethod
+    def _handle_gap_change(cls,
+                           db: DatabaseAPI,
+                           gap_change: GapChange,
+                           header: BlockHeaderAPI,
+                           genesis_parent_hash: Hash32) -> None:
+
+        if gap_change not in GAP_WRITES:
+            return
+
+        if gap_change is GapChange.GapFill:
+            expected_child = cls._get_canonical_head(db)
+            if header.hash != expected_child.parent_hash:
+                # We are trying to close a gap with an uncle. Reject!
+                raise ValidationError(f"{header} is not the parent of {expected_child}")
+
+        # We implicitly assert that persisted headers are canonical here.
+        # This assertion is made when persisting headers that are known to be part of a gap
+        # in the canonical chain.
+        # What we don't know is if this header will eventually lead up to the upper end of the
+        # gap (the checkpoint header) or if this is an alternative history. But we do ensure the
+        # integrity eventually. For one, we check the final header that would close the gap and if
+        # it does not match our expected child (the checkpoint header), we will reject it.
+        # Additionally, if we have persisted a chain of alternative history under the wrong
+        # assumption that it would be the canonical chain, but then at a later point it turns out it
+        # isn't and we overwrite it with the correct canonical chain, we make sure to
+        # recanonicalize all affected headers.
+        # IMPORTANT: Not only do we have to take this into account when we fill a gap, but also
+        # every time we write into a gap.
+        # Suppose we have a gap of 10 headers, then we fill 1 -5 with uncles from chain B.
+        # Now suppose we find the original chain A and attempt to write headers 1 - 10. At that
+        # point, we are under the assumption our current gap spans from just 6 - 10 because we have
+        # previously written headers 1 - 5 from chain B *believing* they are canonical which they
+        # turn out not to be. That means by the time we write headers 1 -5 again (but from chain A)
+        # we do not recognize it as writing to a known gap. Therefore by the time we write header 6,
+        # when we finally realize that we are attempting to fill in a gap, we have to backtrack
+        # the ancestors to add them to the canonical chain.
+
+        for ancestor in cls._find_new_ancestors(db, header, genesis_parent_hash):
+            cls._add_block_number_to_hash_lookup(db, ancestor)
+
     @classmethod
     def _set_as_canonical_chain_head(
         cls,

tests/database/test_header_db.py

@@ -142,6 +142,44 @@ def test_can_patch_holes(headerdb, genesis_header):
     assert_headers_eq(headerdb.get_canonical_head(), pseudo_genesis)
 
 
+def test_gap_filling_handles_uncles_correctly(headerdb, genesis_header):
+    headerdb.persist_header(genesis_header)
+    chain_a = mk_header_chain(genesis_header, length=10)
+    chain_b = mk_header_chain(genesis_header, length=10)
+
+    assert headerdb.get_header_chain_gaps() == GENESIS_CHAIN_GAPS
+    # Persist the checkpoint header with a trusted score
+    # chain_a[7] has block number 8 because `chain_a` doesn't include the genesis
+    pseudo_genesis = chain_a[7]
+    headerdb.persist_checkpoint_header(pseudo_genesis, 154618822656)
+    assert headerdb.get_header_chain_gaps() == (((1, 7),), 9)
+    assert_headers_eq(headerdb.get_canonical_head(), pseudo_genesis)
+
+    # Start filling the gap with headers from `chain_b`. At this point, we do not yet know this is
+    # an alternative history not leading up to our expected checkpoint header.
+    headerdb.persist_header_chain(chain_b[:3])
+    assert headerdb.get_header_chain_gaps() == (((4, 7),), 9)
+
+    with pytest.raises(ValidationError):
+        headerdb.persist_header_chain(chain_b[3:7])
+
+    # That last persist did not write any headers
+    assert headerdb.get_header_chain_gaps() == (((4, 7),), 9)
+
+    for number in range(1, 4):
+        # Make sure we can lookup the headers by block number
+        header_by_number = headerdb.get_canonical_block_header_by_number(number)
+        assert header_by_number == chain_b[number - 1]
+
+    # Make sure patching the hole does not affect what our current head is
+    assert_headers_eq(headerdb.get_canonical_head(), pseudo_genesis)
+
+    # Now we fill the gap with the actual correct chain that does lead up to our checkpoint header
+    headerdb.persist_header_chain(chain_a)
+
+    assert_is_canonical_chain(headerdb, chain_a)
+
+
 def test_write_batch_that_patches_gap_and_adds_new_at_the_tip(headerdb, genesis_header):
     headerdb.persist_header(genesis_header)
     headers = mk_header_chain(genesis_header, length=10)