Piper/fix validation error if new head uncle is current head v2 (ethereum#1204)

* Fix issue 1185

* less fragile check for no-reorg

Author: pipermerriam

eth/chains/base.py

@@ -25,7 +25,10 @@
 
 from cytoolz import (
     assoc,
+    compose,
     groupby,
+    iterate,
+    take,
 )
 
 from eth_typing import (
@@ -35,7 +38,6 @@
 )
 
 from eth_utils import (
-    to_tuple,
     to_set,
     ValidationError,
 )
@@ -204,7 +206,7 @@ def get_score(self, block_hash):
     # Block API
     #
     @abstractmethod
-    def get_ancestors(self, limit: int, header: BlockHeader) -> Iterator[BaseBlock]:
+    def get_ancestors(self, limit: int, header: BlockHeader) -> Tuple[BaseBlock, ...]:
         raise NotImplementedError("Chain classes must implement this method")
 
     @abstractmethod
@@ -447,14 +449,27 @@ def ensure_header(self, header: BlockHeader=None) -> BlockHeader:
     #
     # Block API
     #
-    @to_tuple
-    def get_ancestors(self, limit: int, header: BlockHeader) -> Iterator[BaseBlock]:
+    def get_ancestors(self, limit: int, header: BlockHeader) -> Tuple[BaseBlock, ...]:
         """
         Return `limit` number of ancestor blocks from the current canonical head.
         """
-        lower_limit = max(header.block_number - limit, 0)
-        for n in reversed(range(lower_limit, header.block_number)):
-            yield self.get_canonical_block_by_number(BlockNumber(n))
+        ancestor_count = min(header.block_number, limit)
+
+        # We construct a temporary block object
+        vm_class = self.get_vm_class_for_block_number(header.block_number)
+        block_class = vm_class.get_block_class()
+        block = block_class(header=header, uncles=[])
+
+        ancestor_generator = iterate(compose(
+            self.get_block_by_hash,
+            operator.attrgetter('parent_hash'),
+            operator.attrgetter('header'),
+        ), block)
+        # we peel off the first element from the iterator which will be the
+        # temporary block object we constructed.
+        next(ancestor_generator)
+
+        return tuple(take(ancestor_count, ancestor_generator))
 
     def get_block(self) -> BaseBlock:
         """

eth/vm/base.py

@@ -562,7 +562,7 @@ def pack_block(self, block, *args, **kwargs):
         """
         if 'uncles' in kwargs:
             uncles = kwargs.pop('uncles')
-            kwargs.setdefault('uncles_hash', keccak(rlp.encode(block.uncles)))
+            kwargs.setdefault('uncles_hash', keccak(rlp.encode(uncles)))
         else:
             uncles = block.uncles
 
@@ -710,7 +710,7 @@ def validate_block(self, block):
                 " - header uncle_hash: {2}".format(
                     len(block.uncles),
                     local_uncle_hash,
-                    block.header.uncle_hash,
+                    block.header.uncles_hash,
                 )
             )
 

tests/core/chain-object/test_chain_get_ancestors.py

@@ -0,0 +1,129 @@
+import pytest
+
+from eth.chains.base import MiningChain
+from eth.db.backends.memory import MemoryDB
+
+
+@pytest.fixture
+def chain(chain_without_block_validation):
+    if not isinstance(chain_without_block_validation, MiningChain):
+        pytest.skip('only valid on mining chains')
+    return chain_without_block_validation
+
+
+@pytest.fixture
+def fork_chain(chain):
+    # 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)
+
+    return fork_chain
+
+
+@pytest.mark.parametrize(
+    'limit',
+    (0, 1, 2, 5),
+)
+def test_chain_get_ancestors_from_genesis_block(chain, limit):
+    header = chain.get_canonical_head()
+    assert header.block_number == 0
+
+    ancestors = chain.get_ancestors(limit, header)
+    assert ancestors == tuple()
+
+
+def test_chain_get_ancestors_from_block_1(chain):
+    genesis = chain.get_canonical_block_by_number(0)
+    block_1 = chain.mine_block()
+    header = block_1.header
+    assert header.block_number == 1
+
+    assert chain.get_ancestors(0, header) == tuple()
+    assert chain.get_ancestors(1, header) == (genesis,)
+    assert chain.get_ancestors(2, header) == (genesis,)
+    assert chain.get_ancestors(5, header) == (genesis,)
+
+
+def test_chain_get_ancestors_from_block_5(chain):
+    genesis = chain.get_canonical_block_by_number(0)
+    (
+        block_1,
+        block_2,
+        block_3,
+        block_4,
+        block_5,
+    ) = [chain.mine_block() for _ in range(5)]
+
+    header = block_5.header
+    assert header.block_number == 5
+
+    assert chain.get_ancestors(0, header) == tuple()
+    assert chain.get_ancestors(1, header) == (block_4,)
+    assert chain.get_ancestors(2, header) == (block_4, block_3)
+    assert chain.get_ancestors(3, header) == (block_4, block_3, block_2)
+    assert chain.get_ancestors(4, header) == (block_4, block_3, block_2, block_1)
+    assert chain.get_ancestors(5, header) == (block_4, block_3, block_2, block_1, genesis)
+    assert chain.get_ancestors(6, header) == (block_4, block_3, block_2, block_1, genesis)
+    assert chain.get_ancestors(10, header) == (block_4, block_3, block_2, block_1, genesis)
+
+
+def test_chain_get_ancestors_for_fork_chains(chain, fork_chain):
+    genesis = chain.get_canonical_block_by_number(0)
+    (
+        block_1,
+        block_2,
+        block_3,
+    ) = [chain.mine_block() for _ in range(3)]
+    (
+        f_block_1,
+        f_block_2,
+        f_block_3,
+    ) = [fork_chain.mine_block() for _ in range(3)]
+
+    assert block_1 == f_block_1
+    assert block_2 == f_block_2
+    assert block_3 == f_block_3
+
+    # force the fork chain to diverge
+    fork_chain.header = fork_chain.header.copy(extra_data=b'fork-it!')
+
+    # mine ahead a bit further on both chains.
+    (
+        block_4,
+        block_5,
+        block_6,
+    ) = [chain.mine_block() for _ in range(3)]
+    (
+        f_block_4,
+        f_block_5,
+        f_block_6,
+    ) = [fork_chain.mine_block() for _ in range(3)]
+
+    # import the fork blocks into the main chain (ensuring they don't cause a reorg)
+    _, new_chain, _ = chain.import_block(f_block_4)
+    assert new_chain == tuple()
+    _, new_chain, _ = chain.import_block(f_block_5)
+    assert new_chain == tuple()
+
+    # check with a block that has been imported
+    assert chain.get_ancestors(0, f_block_5.header) == tuple()
+    assert chain.get_ancestors(1, f_block_5.header) == (f_block_4,)
+    assert chain.get_ancestors(2, f_block_5.header) == (f_block_4, block_3)
+    assert chain.get_ancestors(3, f_block_5.header) == (f_block_4, block_3, block_2)
+    assert chain.get_ancestors(4, f_block_5.header) == (f_block_4, block_3, block_2, block_1)
+    assert chain.get_ancestors(5, f_block_5.header) == (f_block_4, block_3, block_2, block_1, genesis)  # noqa: E501
+    # check that when we hit genesis it self limits
+    assert chain.get_ancestors(6, f_block_5.header) == (f_block_4, block_3, block_2, block_1, genesis)  # noqa: E501
+    assert chain.get_ancestors(20, f_block_5.header) == (f_block_4, block_3, block_2, block_1, genesis)  # noqa: E501
+
+    # check with a block that has NOT been imported
+    assert chain.get_ancestors(0, f_block_6.header) == tuple()
+    assert chain.get_ancestors(1, f_block_6.header) == (f_block_5,)
+    assert chain.get_ancestors(2, f_block_6.header) == (f_block_5, f_block_4)
+    assert chain.get_ancestors(3, f_block_6.header) == (f_block_5, f_block_4, block_3)
+    assert chain.get_ancestors(4, f_block_6.header) == (f_block_5, f_block_4, block_3, block_2)
+    assert chain.get_ancestors(5, f_block_6.header) == (f_block_5, f_block_4, block_3, block_2, block_1)  # noqa: E501
+    assert chain.get_ancestors(6, f_block_6.header) == (f_block_5, f_block_4, block_3, block_2, block_1, genesis)  # noqa: E501
+    # check that when we hit genesis it self limits
+    assert chain.get_ancestors(7, f_block_6.header) == (f_block_5, f_block_4, block_3, block_2, block_1, genesis)  # noqa: E501
+    assert chain.get_ancestors(20, f_block_6.header) == (f_block_5, f_block_4, block_3, block_2, block_1, genesis)  # noqa: E501

tests/core/chain-object/test_chain_reorganization.py

@@ -14,7 +14,7 @@
 
 
 @pytest.fixture(params=VM_CLASSES)
-def chain(request, genesis_state):
+def base_chain(request, genesis_state):
     VMClass = request.param.configure(validate_seal=lambda block: None)
 
     class ChainForTest(MiningChain):
@@ -30,20 +30,23 @@ class ChainForTest(MiningChain):
     return chain
 
 
-ZERO_ADDRESS = b'\x00' * 20
-
-
-def test_import_block_with_reorg(chain, funded_address_private_key):
+@pytest.fixture
+def chain(base_chain):
     # mine 3 "common blocks"
-    block_0 = chain.get_canonical_block_by_number(0)
+    block_0 = base_chain.get_canonical_block_by_number(0)
     assert block_0.number == 0
-    block_1 = chain.mine_block()
+    block_1 = base_chain.mine_block()
     assert block_1.number == 1
-    block_2 = chain.mine_block()
+    block_2 = base_chain.mine_block()
     assert block_2.number == 2
-    block_3 = chain.mine_block()
+    block_3 = base_chain.mine_block()
     assert block_3.number == 3
 
+    return base_chain
+
+
+@pytest.fixture
+def fork_chain(chain):
     # 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)
@@ -56,12 +59,26 @@ def test_import_block_with_reorg(chain, funded_address_private_key):
 
     assert fork_chain.get_canonical_head() == chain.get_canonical_head()
 
+    block_0 = chain.get_canonical_block_by_number(0)
     assert fork_chain.get_canonical_block_by_number(0) == block_0
+
+    block_1 = chain.get_canonical_block_by_number(1)
     assert fork_chain.get_canonical_block_by_number(1) == block_1
+
+    block_2 = chain.get_canonical_block_by_number(2)
     assert fork_chain.get_canonical_block_by_number(2) == block_2
+
+    block_3 = chain.get_canonical_block_by_number(3)
     assert fork_chain.get_canonical_block_by_number(3) == block_3
 
-    # now cause the fork chain to diverge from the main chain
+    return fork_chain
+
+
+ZERO_ADDRESS = b'\x00' * 20
+
+
+def test_import_block_with_reorg(chain, fork_chain, funded_address_private_key):
+    # cause the fork chain to diverge from the main chain
     tx = fork_chain.create_unsigned_transaction(
         nonce=0,
         gas_price=1,
@@ -72,7 +89,7 @@ def test_import_block_with_reorg(chain, funded_address_private_key):
     )
     fork_chain.apply_transaction(tx.as_signed_transaction(funded_address_private_key))
 
-    # Mine 3 blocks, ensuring that the difficulty of the main chain remains
+    # Mine 2 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()
@@ -87,28 +104,51 @@ def test_import_block_with_reorg(chain, funded_address_private_key):
     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()
+    # now mine the 6th block which will outpace the main chain difficulty.
+    f_block_6 = 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):
+    # chain.  Blocks 4 and 5 should import resulting in no re-organization.
+    for block in (f_block_4, f_block_5):
         _, 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)
+    # now we import block 6 from the fork chain.  This should cause a re-org.
+    _, new_canonical_blocks, old_canonical_blocks = chain.import_block(f_block_6)
+    assert new_canonical_blocks == (f_block_4, f_block_5, f_block_6)
+    assert old_canonical_blocks == (block_4, block_5)
+
+    assert chain.get_canonical_head() == f_block_6.header
+
+
+def test_import_block_with_reorg_with_current_head_as_uncle(
+        chain,
+        fork_chain,
+        funded_address_private_key):
+    """
+    https://github.com/ethereum/py-evm/issues/1185
+    """
+    # mine a block on the main chain which will eventually become an uncle on
+    # the main chain after a reorg.
+    block = chain.mine_block()
+
+    # Force the fork_chain to diverge from the main chain
+    fork_chain.header = fork_chain.header.copy(extra_data=b'fork-it!')
+    f_block_a = fork_chain.mine_block()
+
+    # now mine a block which has the current chain head as an uncle.
+    assert f_block_a != block
+    f_block_b = fork_chain.mine_block(uncles=(block.header,))
+
+    # ensure that we don't cause a re-org with our first import.
+    _, new_chain, _ = chain.import_block(f_block_a)
+    assert new_chain == tuple()
+
+    # import the block with the uncle, ensure that the chain did indeed re-org
+    chain.import_block(f_block_b)
 
-    assert chain.get_canonical_head() == f_block_7.header
+    assert chain.get_canonical_head() == f_block_b.header

trinity/chains/light.py

@@ -128,7 +128,7 @@ def get_score(self, block_hash: Hash32) -> int:
     #
     # Block API
     #
-    def get_ancestors(self, limit: int, header: BlockHeader=None) -> Iterator[BaseBlock]:
+    def get_ancestors(self, limit: int, header: BlockHeader) -> Tuple[BaseBlock, ...]:
         raise NotImplementedError("Chain classes must implement " + inspect.stack()[0][3])
 
     def get_block(self) -> BaseBlock: