cleanup from changes to import_block (#1200)

pipermerriam · web-flow · commit dab298e2945e · 2018-08-21T16:31:58.000-06:00
* cleanup from changes to import_block

* Rename orphaned to old
diff --git a/eth/chains/base.py b/eth/chains/base.py
@@ -269,7 +269,7 @@ def estimate_gas(
     def import_block(self,
                      block: BaseBlock,
                      perform_validation: bool=True,
-                     ) -> Tuple[Tuple[BaseBlock, ...], Tuple[BaseBlock, ...]]:
+                     ) -> Tuple[BaseBlock, Tuple[BaseBlock, ...], Tuple[BaseBlock, ...]]:
         raise NotImplementedError("Chain classes must implement this method")
 
     #
@@ -591,9 +591,13 @@ def estimate_gas(
     def import_block(self,
                      block: BaseBlock,
                      perform_validation: bool=True
-                     ) -> Tuple[Tuple[BaseBlock, ...], Tuple[BaseBlock, ...]]:
+                     ) -> Tuple[BaseBlock, Tuple[BaseBlock, ...], Tuple[BaseBlock, ...]]:
         """
-        Imports a complete block and returns new_blocks and orphaned_canonical_blocks.
+        Imports a complete block and returns a 3-tuple
+
+        - the imported block
+        - a tuple of blocks which are now part of the canonical chain.
+        - a tuple of blocks which are were canonical and now are no longer canonical.
         """
 
         try:
@@ -617,8 +621,8 @@ def import_block(self,
             self.validate_block(imported_block)
 
         (
-            new_canonical_header_hashes,
-            orphaned_header_hashes,
+            new_canonical_hashes,
+            old_canonical_hashes,
         ) = self.chaindb.persist_block(imported_block)
 
         self.logger.debug(
@@ -627,15 +631,18 @@ def import_block(self,
             encode_hex(imported_block.hash),
         )
 
-        new_blocks = tuple(
-            self.get_block_by_hash(header_hash) for header_hash in new_canonical_header_hashes)
-        orphaned_canonical_blocks = tuple(
-            self.get_block_by_hash(header_hash) for header_hash in orphaned_header_hashes)
-        # add imported block to new block when no chain re-org
-        if not new_blocks:
-            new_blocks = (imported_block, )
+        new_canonical_blocks = tuple(
+            self.get_block_by_hash(header_hash)
+            for header_hash
+            in new_canonical_hashes
+        )
+        old_canonical_blocks = tuple(
+            self.get_block_by_hash(header_hash)
+            for header_hash
+            in old_canonical_hashes
+        )
 
-        return new_blocks, orphaned_canonical_blocks
+        return imported_block, new_canonical_blocks, old_canonical_blocks
 
     #
     # Validation API
@@ -804,12 +811,12 @@ def apply_transaction(self, transaction):
     def import_block(self,
                      block: BaseBlock,
                      perform_validation: bool=True
-                     ) -> Tuple[Tuple[BaseBlock, ...], Tuple[BaseBlock, ...]]:
-        new_canonical_blocks, orphaned_canonical_blocks = super().import_block(
+                     ) -> Tuple[BaseBlock, Tuple[BaseBlock, ...], Tuple[BaseBlock, ...]]:
+        imported_block, new_canonical_blocks, old_canonical_blocks = super().import_block(
             block, perform_validation)
 
         self.header = self.ensure_header()
-        return new_canonical_blocks, orphaned_canonical_blocks
+        return imported_block, new_canonical_blocks, old_canonical_blocks
 
     def mine_block(self, *args: Any, **kwargs: Any) -> BaseBlock:
         """
@@ -834,10 +841,12 @@ def get_vm(self, at_header: BlockHeader=None) -> 'BaseVM':
 # This class is a work in progress; its main purpose is to define the API of an asyncio-compatible
 # Chain implementation.
 class AsyncChain(Chain):
+    # TODO: this really belongs in the `trinity` module.
 
     async def coro_import_block(self,
                                 block: BlockHeader,
-                                perform_validation: bool=True) -> BaseBlock:
+                                perform_validation: bool=True,
+                                ) -> Tuple[BaseBlock, Tuple[BaseBlock, ...], Tuple[BaseBlock, ...]]:
         raise NotImplementedError()
 
     async def coro_validate_chain(
diff --git a/eth/db/chain.py b/eth/db/chain.py
@@ -220,19 +220,19 @@ def persist_header(self,
         except CanonicalHeadNotFound:
             (
                 new_canonical_headers,
-                orphaned_canonical_headers
+                old_canonical_headers
             ) = self._set_as_canonical_chain_head(header)
         else:
             if score > head_score:
                 (
                     new_canonical_headers,
-                    orphaned_canonical_headers
+                    old_canonical_headers
                 ) = self._set_as_canonical_chain_head(header)
             else:
                 new_canonical_headers = tuple()
-                orphaned_canonical_headers = tuple()
+                old_canonical_headers = tuple()
 
-        return new_canonical_headers, orphaned_canonical_headers
+        return new_canonical_headers, old_canonical_headers
 
     # TODO: update this to take a `hash` rather than a full header object.
     def _set_as_canonical_chain_head(self,
@@ -248,7 +248,7 @@ def _set_as_canonical_chain_head(self,
                 header.hash))
 
         new_canonical_headers = tuple(reversed(self._find_new_ancestors(header)))
-        orphaned_canonical_headers = []
+        old_canonical_headers = []
         # remove transaction lookups for blocks that are no longer canonical
         for h in new_canonical_headers:
             try:
@@ -257,17 +257,17 @@ def _set_as_canonical_chain_head(self,
                 # no old block, and no more possible
                 break
             else:
-                orphaned_header = self.get_block_header_by_hash(old_hash)
-                orphaned_canonical_headers.append(orphaned_header)
-                for transaction_hash in self.get_block_transaction_hashes(orphaned_header):
+                old_header = self.get_block_header_by_hash(old_hash)
+                old_canonical_headers.append(old_header)
+                for transaction_hash in self.get_block_transaction_hashes(old_header):
                     self._remove_transaction_from_canonical_chain(transaction_hash)
 
         for h in new_canonical_headers:
             self._add_block_number_to_hash_lookup(h)
 
         self.db.set(SchemaV1.make_canonical_head_hash_lookup_key(), header.hash)
 
-        return new_canonical_headers, tuple(orphaned_canonical_headers)
+        return new_canonical_headers, tuple(old_canonical_headers)
 
     #
     # Block API
@@ -280,7 +280,7 @@ def persist_block(self,
 
         Assumes all block transactions have been persisted already.
         '''
-        new_canonical_headers, orphaned_canonical_headers = self.persist_header(block.header)
+        new_canonical_headers, old_canonical_headers = self.persist_header(block.header)
 
         for header in new_canonical_headers:
             if header.hash == block.hash:
@@ -302,11 +302,11 @@ def persist_block(self,
             raise ValidationError(
                 "Block's uncles_hash (%s) does not match actual uncles' hash (%s)",
                 block.header.uncles_hash, uncles_hash)
-        new_canonical_header_hashes = tuple(header.hash for header in new_canonical_headers)
-        orphaned_canonical_header_hashes = tuple(
-            header.hash for header in orphaned_canonical_headers)
+        new_canonical_hashes = tuple(header.hash for header in new_canonical_headers)
+        old_canonical_hashes = tuple(
+            header.hash for header in old_canonical_headers)
 
-        return new_canonical_header_hashes, orphaned_canonical_header_hashes
+        return new_canonical_hashes, old_canonical_hashes
 
     def persist_uncles(self, uncles: Tuple[BlockHeader]) -> Hash32:
         """
diff --git a/eth/db/header.py b/eth/db/header.py
@@ -178,15 +178,19 @@ def persist_header(self,
         try:
             head_score = self.get_score(self.get_canonical_head().hash)
         except CanonicalHeadNotFound:
-            new_canonical_headers, old_headers = self._set_as_canonical_chain_head(header.hash)
+            new_canonical_headers, old_canonical_headers = self._set_as_canonical_chain_head(
+                header.hash,
+            )
         else:
             if score > head_score:
-                new_canonical_headers, old_headers = self._set_as_canonical_chain_head(header.hash)
+                new_canonical_headers, old_canonical_headers = self._set_as_canonical_chain_head(
+                    header.hash
+                )
             else:
                 new_canonical_headers = tuple()
-                old_headers = tuple()
+                old_canonical_headers = tuple()
 
-        return new_canonical_headers, old_headers
+        return new_canonical_headers, old_canonical_headers
 
     def _set_as_canonical_chain_head(self, block_hash: Hash32
                                      ) -> Tuple[Tuple[BlockHeader, ...], Tuple[BlockHeader, ...]]:
@@ -204,24 +208,24 @@ def _set_as_canonical_chain_head(self, block_hash: Hash32
             )
 
         new_canonical_headers = tuple(reversed(self._find_new_ancestors(header)))
-        orphaned_headers = []
+        old_canonical_headers = []
 
         for h in new_canonical_headers:
             try:
-                orphaned_hash = self.get_canonical_block_hash(h.block_number)
+                old_canonical_hash = self.get_canonical_block_hash(h.block_number)
             except HeaderNotFound:
-                # no orphaned block, and no more possible
+                # no old_canonical block, and no more possible
                 break
             else:
-                orphaned_header = self.get_block_header_by_hash(orphaned_hash)
-                orphaned_headers.append(orphaned_header)
+                old_canonical_header = self.get_block_header_by_hash(old_canonical_hash)
+                old_canonical_headers.append(old_canonical_header)
 
         for h in new_canonical_headers:
             self._add_block_number_to_hash_lookup(h)
 
         self.db.set(SchemaV1.make_canonical_head_hash_lookup_key(), header.hash)
 
-        return new_canonical_headers, tuple(orphaned_headers)
+        return new_canonical_headers, tuple(old_canonical_headers)
 
     @to_tuple
     def _find_new_ancestors(self, header: BlockHeader) -> Iterable[BlockHeader]:
diff --git a/eth/tools/fixture_tests.py b/eth/tools/fixture_tests.py
@@ -641,8 +641,7 @@ def apply_fixture_block_to_chain(block_fixture, chain):
 
     block = rlp.decode(block_fixture['rlp'], sedes=block_class)
 
-    imported_blocks, _ = chain.import_block(block)
-    mined_block = imported_blocks[-1]
+    mined_block, _, _ = chain.import_block(block)
 
     rlp_encoded_mined_block = rlp.encode(mined_block, sedes=block_class)
 
diff --git a/scripts/benchmark/checks/import_empty_blocks.py b/scripts/benchmark/checks/import_empty_blocks.py
@@ -47,8 +47,7 @@ def import_empty_blocks(self, chain: Chain, number_blocks: int) -> int:
 
         total_gas_used = 0
         for _ in range(1, number_blocks + 1):
-            imported_blocks, _ = chain.import_block(chain.get_vm().block, False)
-            block = imported_blocks[-1]
+            block, _, _ = chain.import_block(chain.get_vm().block, False)
 
             total_gas_used = total_gas_used + block.header.gas_used
             logging.debug(format_block(block))
diff --git a/tests/core/chain-object/test_chain.py b/tests/core/chain-object/test_chain.py
@@ -41,8 +41,7 @@ def tx(chain, funded_address, funded_address_private_key):
 @pytest.mark.xfail(reason="modification to initial allocation made the block fixture invalid")
 def test_import_block_validation(valid_chain, funded_address, funded_address_initial_balance):
     block = rlp.decode(valid_block_rlp, sedes=FrontierBlock)
-    imported_blocks, _ = valid_chain.import_block(block)
-    imported_block = imported_blocks[-1]
+    imported_block, _, _ = valid_chain.import_block(block)
 
     assert len(imported_block.transactions) == 1
     tx = imported_block.transactions[0]
@@ -66,8 +65,7 @@ def test_import_block(chain, tx):
         new_block, receipts, computations = chain.build_block_with_transactions([tx])
         assert computations[0].is_success
 
-    imported_blocks, _ = chain.import_block(new_block)
-    block = imported_blocks[-1]
+    block, _, _ = chain.import_block(new_block)
 
     assert block.transactions == (tx,)
     assert chain.get_block_by_hash(block.hash) == block
diff --git a/tests/core/chain-object/test_chain_reorganization.py b/tests/core/chain-object/test_chain_reorganization.py
@@ -0,0 +1,114 @@
+import pytest
+
+from eth import constants
+from eth.chains.base import MiningChain
+from eth.chains.tester import MAINNET_VMS
+from eth.db.backends.memory import MemoryDB
+
+
+VM_CLASSES = tuple(
+    VMClass
+    for _, VMClass
+    in MAINNET_VMS.items()
+)
+
+
+@pytest.fixture(params=VM_CLASSES)
+def chain(request, genesis_state):
+    VMClass = request.param.configure(validate_seal=lambda block: None)
+
+    class ChainForTest(MiningChain):
+        vm_configuration = ((0, VMClass),)
+        network_id = 1337
+
+    genesis_params = {
+        'block_number': constants.GENESIS_BLOCK_NUMBER,
+        'difficulty': constants.GENESIS_DIFFICULTY,
+        'gas_limit': constants.GENESIS_GAS_LIMIT,
+    }
+    chain = ChainForTest.from_genesis(MemoryDB(), genesis_params, genesis_state)
+    return chain
+
+
+ZERO_ADDRESS = b'\x00' * 20
+
+
+def test_import_block_with_reorg(chain, funded_address_private_key):
+    # mine 3 "common blocks"
+    block_0 = chain.get_canonical_block_by_number(0)
+    assert block_0.number == 0
+    block_1 = chain.mine_block()
+    assert block_1.number == 1
+    block_2 = chain.mine_block()
+    assert block_2.number == 2
+    block_3 = chain.mine_block()
+    assert block_3.number == 3
+
+    # make a duplicate chain with no shared state
+    fork_db = MemoryDB(chain.chaindb.db.kv_store.copy())
+    fork_chain = type(chain)(fork_db, chain.header)
+
+    # sanity check to verify that the two chains are the same.
+    assert chain.header == fork_chain.header
+
+    assert chain.header.block_number == 4
+    assert fork_chain.header.block_number == 4
+
+    assert fork_chain.get_canonical_head() == chain.get_canonical_head()
+
+    assert fork_chain.get_canonical_block_by_number(0) == block_0
+    assert fork_chain.get_canonical_block_by_number(1) == block_1
+    assert fork_chain.get_canonical_block_by_number(2) == block_2
+    assert fork_chain.get_canonical_block_by_number(3) == block_3
+
+    # now cause the fork chain to diverge from the main chain
+    tx = fork_chain.create_unsigned_transaction(
+        nonce=0,
+        gas_price=1,
+        gas=21000,
+        to=ZERO_ADDRESS,
+        value=0,
+        data=b'',
+    )
+    fork_chain.apply_transaction(tx.as_signed_transaction(funded_address_private_key))
+
+    # Mine 3 blocks, ensuring that the difficulty of the main chain remains
+    # equal or greater than the fork chain
+    block_4 = chain.mine_block()
+    f_block_4 = fork_chain.mine_block()
+    assert f_block_4 != block_4
+    assert block_4.number == 4
+    assert f_block_4.number == 4
+    assert f_block_4.header.difficulty <= block_4.header.difficulty
+
+    f_block_5, block_5 = fork_chain.mine_block(), chain.mine_block()
+    assert f_block_5 != block_5
+    assert block_5.number == 5
+    assert f_block_5.number == 5
+    assert f_block_5.header.difficulty <= block_5.header.difficulty
+
+    f_block_6, block_6 = fork_chain.mine_block(), chain.mine_block()
+    assert f_block_6 != block_6
+    assert block_6.number == 6
+    assert f_block_6.number == 6
+    assert f_block_6.header.difficulty <= block_6.header.difficulty
+
+    # now mine the 7th block which will outpace the main chain difficulty.
+    f_block_7 = fork_chain.mine_block()
+
+    pre_fork_chain_head = chain.header
+
+    # now we proceed to import the blocks from the fork chain into the main
+    # chain.  Blocks 4, 5, and 6 should import resulting in no re-organization.
+    for block in (f_block_4, f_block_5, f_block_6):
+        _, new_canonical_blocks, old_canonical_blocks = chain.import_block(block)
+        assert not new_canonical_blocks
+        assert not old_canonical_blocks
+        assert chain.header == pre_fork_chain_head
+
+    # now we import block 7 from the fork chain.  This should cause a re-org.
+    _, new_canonical_blocks, old_canonical_blocks = chain.import_block(f_block_7)
+    assert new_canonical_blocks == (f_block_4, f_block_5, f_block_6, f_block_7)
+    assert old_canonical_blocks == (block_4, block_5, block_6)
+
+    assert chain.get_canonical_head() == f_block_7.header
diff --git a/tests/core/chain-object/test_import_block.py b/tests/core/chain-object/test_import_block.py
diff --git a/tests/core/vm/test_vm_state.py b/tests/core/vm/test_vm_state.py
diff --git a/trinity/sync/full/chain.py b/trinity/sync/full/chain.py
