PR feedback

Author: cburgdorf

eth/abc.py

@@ -18,7 +18,8 @@
     Type,
     TypeVar,
     Union,
-    NamedTuple)
+    NamedTuple,
+)
 from uuid import UUID
 
 import rlp
@@ -38,10 +39,11 @@
 from eth.constants import (
     BLANK_ROOT_HASH,
 )
+
 from eth.exceptions import VMError
 from eth.typing import (
-    BlockRange,
     BytesOrView,
+    ChainGaps,
     JournalDBCheckpoint,
     AccountState,
     HeaderParams,
@@ -494,15 +496,16 @@ def __init__(self, db: AtomicDatabaseAPI) -> None:
         ...
 
     @abstractmethod
-    def get_header_chain_gaps(self) -> Tuple[BlockRange, ...]:
+    def get_header_chain_gaps(self) -> ChainGaps:
         """
-        Return an ordered sequence of block ranges describing the integrity of the chain of
-        headers. Each block range describes a missing segment in the chain and each range is defined
-        with inclusive boundaries, meaning the first value describes the first missing block of that
-        segment and the second value describes the last missing block of the segment.
+        Return information about gaps in the chain of headers. This consists of an ordered sequence
+        of block ranges describing the integrity of the chain. Each block range describes a missing
+        segment in the chain and each range is defined with inclusive boundaries, meaning the first
+        value describes the first missing block of that segment and the second value describes the
+        last missing block of the segment.
 
-        The last block range in the sequence is expected to have a block number of `-1` as the
-        right-hand value which is to say the gap is open-ended.
+        In addition to the sequences of block ranges a block number is included that indicates the
+        number of the first header that is known to be missing at the very tip of the chain.
         """
 
     #

eth/db/chain_gaps.py

@@ -4,48 +4,50 @@
 from eth_typing import BlockNumber
 
 from eth.exceptions import GapTrackingCorrupted
-from eth.typing import BlockRange
+from eth.typing import BlockRange, ChainGaps
 
 
 class GapChange(enum.Enum):
-
     NoChange = enum.auto()
     NewGap = enum.auto()
+    GapFill = enum.auto()
     GapSplit = enum.auto()
     GapShrink = enum.auto()
     TailWrite = enum.auto()
 
 
-GapInfo = Tuple[GapChange, Tuple[BlockRange, ...]]
+GAP_WRITES = (GapChange.GapFill, GapChange.GapSplit, GapChange.GapShrink)
+GENESIS_CHAIN_GAPS = ((), BlockNumber(1))
+
+GapInfo = Tuple[GapChange, ChainGaps]
 
 
-def calculate_gaps(newly_persisted: BlockNumber, base_gaps: Tuple[BlockRange, ...]) -> GapInfo:
+def calculate_gaps(newly_persisted: BlockNumber, base_gaps: ChainGaps) -> GapInfo:
 
-    # If we have a fresh chain, our highest missing number can only be 1
-    highest_missing_number = 1 if base_gaps == () else base_gaps[-1][0]
+    current_gaps, known_missing_tip = base_gaps
 
-    if newly_persisted == highest_missing_number:
+    if newly_persisted == known_missing_tip:
         # This is adding a consecutive header at the very tail
-        new_last_marker = (newly_persisted + 1, -1)
-        new_gaps = base_gaps[:-1] + (new_last_marker,)
+        new_gaps = (current_gaps, BlockNumber(newly_persisted + 1))
         gap_change = GapChange.TailWrite
-    elif newly_persisted > highest_missing_number:
+    elif newly_persisted > known_missing_tip:
         # We are creating a gap in the chain
-        gap_end = newly_persisted - 1
-        new_tail = ((highest_missing_number, gap_end), (newly_persisted + 1, -1),)
-        new_gaps = base_gaps[:-1] + new_tail
+        gap_end = BlockNumber(newly_persisted - 1)
+        new_gaps = (
+            current_gaps + ((known_missing_tip, gap_end),), BlockNumber(newly_persisted + 1)
+        )
         gap_change = GapChange.NewGap
-    elif newly_persisted < highest_missing_number:
+    elif newly_persisted < known_missing_tip:
         # We are patching a gap which may either shrink an existing gap or divide it
         matching_gaps = [
-            (index, pair) for index, pair in enumerate(base_gaps)
+            (index, pair) for index, pair in enumerate(current_gaps)
             if newly_persisted >= pair[0] and newly_persisted <= pair[1]
         ]
 
         if len(matching_gaps) > 1:
             raise GapTrackingCorrupted(
                 "Corrupted chain gap tracking",
-                f"No {newly_persisted} appears to be missing in multiple gaps",
+                f"No. {newly_persisted} appears to be missing in multiple gaps",
                 f"1st gap goes from {matching_gaps[0][1][0]} to {matching_gaps[0][1][1]}"
                 f"2nd gap goes from {matching_gaps[1][1][0]} to {matching_gaps[1][1][1]}"
             )
@@ -55,26 +57,28 @@ def calculate_gaps(newly_persisted: BlockNumber, base_gaps: Tuple[BlockRange, ..
         elif len(matching_gaps) == 1:
             gap_index, gap = matching_gaps[0]
             if newly_persisted == gap[0] and newly_persisted == gap[1]:
-                updated_center: Tuple[Tuple[int, int], ...] = ()
-                gap_change = GapChange.GapShrink
+                updated_center: Tuple[BlockRange, ...] = ()
+                gap_change = GapChange.GapFill
             elif newly_persisted == gap[0]:
                 # we are shrinking the gap at the start
-                updated_center = ((gap[0] + 1, gap[1],),)
+                updated_center = ((BlockNumber(gap[0] + 1), gap[1],),)
                 gap_change = GapChange.GapShrink
             elif newly_persisted == gap[1]:
                 # we are shrinking the gap at the tail
-                updated_center = ((gap[0], gap[1] - 1,),)
+                updated_center = ((gap[0], BlockNumber(gap[1] - 1),),)
                 gap_change = GapChange.GapShrink
-            else:
+            elif gap[0] < newly_persisted < gap[1]:
                 # we are dividing the gap
-                first_new_gap = (gap[0], newly_persisted - 1)
-                second_new_gap = (newly_persisted + 1, gap[1])
+                first_new_gap = (gap[0], BlockNumber(newly_persisted - 1))
+                second_new_gap = (BlockNumber(newly_persisted + 1), gap[1])
                 updated_center = (first_new_gap, second_new_gap,)
                 gap_change = GapChange.GapSplit
+            else:
+                raise Exception("Invariant")
 
-            before_gap = base_gaps[:gap_index]
-            after_gap = base_gaps[gap_index + 1:]
-            new_gaps = before_gap + updated_center + after_gap
+            before_gap = current_gaps[:gap_index]
+            after_gap = current_gaps[gap_index + 1:]
+            new_gaps = (before_gap + updated_center + after_gap, known_missing_tip)
 
     else:
         raise Exception("Invariant")

eth/db/header.py

@@ -33,6 +33,8 @@
     calculate_gaps,
     GapChange,
     GapInfo,
+    GAP_WRITES,
+    GENESIS_CHAIN_GAPS,
 )
 from eth.exceptions import (
     CanonicalHeadNotFound,
@@ -41,11 +43,8 @@
 )
 from eth.db.schema import SchemaV1
 from eth.rlp.headers import BlockHeader
-from eth.rlp.chain_gaps import (
-    decode_chain_gaps,
-    encode_chain_gaps,
-)
-from eth.typing import BlockRange
+from eth.rlp.sedes import chain_gaps
+from eth.typing import ChainGaps
 from eth.validation import (
     validate_block_number,
     validate_word,
@@ -56,24 +55,24 @@ class HeaderDB(HeaderDatabaseAPI):
     def __init__(self, db: AtomicDatabaseAPI) -> None:
         self.db = db
 
-    def get_header_chain_gaps(self) -> Tuple[BlockRange, ...]:
+    def get_header_chain_gaps(self) -> ChainGaps:
         return self._get_header_chain_gaps(self.db)
 
     @classmethod
-    def _get_header_chain_gaps(cls, db: DatabaseAPI) -> Tuple[BlockRange, ...]:
+    def _get_header_chain_gaps(cls, db: DatabaseAPI) -> ChainGaps:
         try:
             encoded_gaps = db[SchemaV1.make_header_chain_gaps_lookup_key()]
         except KeyError:
-            return ()
+            return GENESIS_CHAIN_GAPS
         else:
-            return decode_chain_gaps(encoded_gaps)
+            return rlp.decode(encoded_gaps, sedes=chain_gaps)
 
     @classmethod
     def _update_header_chain_gaps(
             cls,
             db: DatabaseAPI,
             persisted_header: BlockHeaderAPI,
-            base_gaps: Tuple[BlockRange, ...] = None) -> GapInfo:
+            base_gaps: ChainGaps = None) -> GapInfo:
 
             # If we make many updates in a row, we can avoid reloading the integrity info by
             # continuously caching it and providing it as a parameter to this API
@@ -85,7 +84,7 @@ def _update_header_chain_gaps(
             if gap_change is not GapChange.NoChange:
                 db.set(
                     SchemaV1.make_header_chain_gaps_lookup_key(),
-                    encode_chain_gaps(gaps)
+                    rlp.encode(gaps, sedes=chain_gaps)
                 )
 
             return gap_change, gaps
@@ -257,7 +256,14 @@ 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 is GapChange.GapShrink or gap_change is GapChange.GapSplit:
+        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)
 
         orig_headers_seq = concat([(first_header,), headers_iterator])
@@ -277,7 +283,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 is GapChange.GapShrink or gap_change is GapChange.GapSplit:
+            if gap_change in GAP_WRITES:
                 cls._add_block_number_to_hash_lookup(db, curr_chain_head)
         try:
             previous_canonical_head = cls._get_canonical_head_hash(db)

eth/exceptions.py

@@ -67,7 +67,7 @@ class CanonicalHeadNotFound(PyEVMError):
 
 class GapTrackingCorrupted(PyEVMError):
     """
-    Raised when the tracking of chain gaps appears to be corrupted
+    Raised when the tracking of chain gaps appears to be corrupted (e.g. overlapping gaps)
     """
     pass
 

eth/rlp/chain_gaps.py

@@ -1,3 +1,4 @@
+import rlp
 from rlp.sedes import (
     BigEndianInt,
     Binary,
@@ -9,3 +10,7 @@
 uint32 = BigEndianInt(32)
 uint256 = BigEndianInt(256)
 trie_root = Binary.fixed_length(32, allow_empty=True)
+chain_gaps = rlp.sedes.List((
+    rlp.sedes.CountableList(rlp.sedes.List((uint32, uint32))),
+    uint32,
+))

eth/rlp/sedes.py

@@ -41,7 +41,14 @@
 
 AccountDiff = Iterable[Tuple[Address, str, Union[int, bytes], Union[int, bytes]]]
 
-BlockRange = Tuple[int, int]
+BlockRange = Tuple[BlockNumber, BlockNumber]
+
+ChainGaps = Tuple[
+    # series of gaps before the chain head
+    Tuple[BlockRange, ...],
+    # the first missing block number at the tip of the chain
+    BlockNumber,
+]
 
 GeneralState = Union[
     AccountState,