PR feedback

Author: lithp

eth/db/chain.py

@@ -35,7 +35,6 @@
     GENESIS_PARENT_HASH,
 )
 from eth.exceptions import (
-    CanonicalHeadNotFound,
     HeaderNotFound,
     ReceiptNotFound,
     TransactionNotFound,
@@ -91,24 +90,11 @@ def get_block_uncles(self, uncles_hash: Hash32) -> Tuple[BlockHeaderAPI, ...]:
             return tuple(rlp.decode(encoded_uncles, sedes=rlp.sedes.CountableList(BlockHeader)))
 
     @classmethod
-    def _set_as_canonical_chain_head(
-            cls,
-            db: DatabaseAPI,
-            header: BlockHeaderAPI,
-            genesis_parent_hash: Hash32,
-    ) -> Tuple[Tuple[BlockHeaderAPI, ...], Tuple[BlockHeaderAPI, ...]]:
-        # performance optimization, short circuit if we're just adding a single header
-        try:
-            current_canonical_head = cls._get_canonical_head_hash(db)
-        except CanonicalHeadNotFound:
-            current_canonical_head = None
-        if current_canonical_head and header.parent_hash == current_canonical_head:
-            cls._add_block_number_to_hash_lookup(db, header)
-            db.set(SchemaV1.make_canonical_head_hash_lookup_key(), header.hash)
-            return (header,), tuple()
-
-        new_canonical_headers = tuple(reversed(
-            cls._find_new_ancestors(db, header, genesis_parent_hash)))
+    def _decanonicalize_old_headers(
+        cls,
+        db: DatabaseAPI,
+        new_canonical_headers: Tuple[BlockHeaderAPI, ...]
+    ) -> Tuple[BlockHeaderAPI, ...]:
         old_canonical_headers = []
 
         # remove transaction lookups for blocks that are no longer canonical
@@ -122,19 +108,16 @@ def _set_as_canonical_chain_head(
                 old_header = cls._get_block_header_by_hash(db, old_hash)
                 old_canonical_headers.append(old_header)
                 try:
-                    for transaction_hash in cls._get_block_transaction_hashes(db, old_header):
+                    transaction_hashes = cls._get_block_transaction_hashes(db, old_header)
+                    for transaction_hash in transaction_hashes:
                         cls._remove_transaction_from_canonical_chain(db, transaction_hash)
                 except MissingTrieNode:
-                    # If the transactions were never stored for the (now) non-canonical chain,
-                    #   then you don't need to remove them from the canonical chain lookup.
+                    # If the transactions were never stored for the (now) non-canonical
+                    # chain, then you don't need to remove them from the canonical chain
+                    # lookup.
                     pass
 
-        for h in new_canonical_headers:
-            cls._add_block_number_to_hash_lookup(db, h)
-
-        db.set(SchemaV1.make_canonical_head_hash_lookup_key(), header.hash)
-
-        return new_canonical_headers, tuple(old_canonical_headers)
+        return tuple(old_canonical_headers)
 
     #
     # Block API

eth/db/header.py

@@ -254,18 +254,42 @@ def _set_as_canonical_chain_head(
         :return: a tuple of the headers that are newly in the canonical chain, and the headers that
             are no longer in the canonical chain
         """
-        # performance optimization, short circuit if we're just adding a single header
         try:
             current_canonical_head = cls._get_canonical_head_hash(db)
         except CanonicalHeadNotFound:
             current_canonical_head = None
+
+        new_canonical_headers: Tuple[BlockHeaderAPI, ...]
+        old_canonical_headers: Tuple[BlockHeaderAPI, ...]
+
         if current_canonical_head and header.parent_hash == current_canonical_head:
-            cls._add_block_number_to_hash_lookup(db, header)
-            db.set(SchemaV1.make_canonical_head_hash_lookup_key(), header.hash)
-            return (header,), tuple()
+            # the calls to _find_new_ancestors and _decanonicalize_old_headers are
+            # relatively expensive, it's better to skip them in this case, where we're
+            # extending the canonical chain by a header
+            new_canonical_headers = (header,)
+            old_canonical_headers = ()
+        else:
+            new_canonical_headers = cast(
+                Tuple[BlockHeaderAPI, ...],
+                tuple(reversed(cls._find_new_ancestors(db, header, genesis_parent_hash)))
+            )
+            old_canonical_headers = cls._decanonicalize_old_headers(
+                db, new_canonical_headers
+            )
+
+        for h in new_canonical_headers:
+            cls._add_block_number_to_hash_lookup(db, h)
+
+        db.set(SchemaV1.make_canonical_head_hash_lookup_key(), header.hash)
 
-        new_canonical_headers = tuple(reversed(
-            cls._find_new_ancestors(db, header, genesis_parent_hash)))
+        return new_canonical_headers, old_canonical_headers
+
+    @classmethod
+    def _decanonicalize_old_headers(
+        cls,
+        db: DatabaseAPI,
+        new_canonical_headers: Tuple[BlockHeaderAPI, ...]
+    ) -> Tuple[BlockHeaderAPI, ...]:
         old_canonical_headers = []
 
         for h in new_canonical_headers:
@@ -278,12 +302,7 @@ def _set_as_canonical_chain_head(
                 old_canonical_header = cls._get_block_header_by_hash(db, old_canonical_hash)
                 old_canonical_headers.append(old_canonical_header)
 
-        for h in new_canonical_headers:
-            cls._add_block_number_to_hash_lookup(db, h)
-
-        db.set(SchemaV1.make_canonical_head_hash_lookup_key(), header.hash)
-
-        return new_canonical_headers, tuple(old_canonical_headers)
+        return tuple(old_canonical_headers)
 
     @classmethod
     @to_tuple