RegularChainSyncer crashes when block importing fails

[gsalgado]

So, validate_rlp_equal() is raising a TypeError even though the diff between the imported block and the original one is empty and they seem to be instances of the same class, and that is causing RegularChainSyncer to crash:

   DEBUG  09-25 17:02:51           ByzantiumVM  BLOCK REWARD: 3000000000000000000 -> b'\x1b\xc1\xeet\x1b= GT?Yq\xadu4\x074\xc1D\x13'
   DEBUG  09-25 17:02:51    RegularChainSyncer  <coroutine object RegularChainSyncer._import_ready_blocks at 0x7fd3147e72b0> finished while <trinity.sync.full.chain.           RegularChainSyncer object at 0x7fd3179ac940> is still running, terminating as well
 WARNING  09-25 17:02:51    RegularChainSyncer  Task <coroutine object RegularChainSyncer._import_ready_blocks at 0x7fd3147e7308> finished unexpectedly: block                  (<ByzantiumBlock(#Block #4111023)>) != imported block (<ByzantiumBlock(#Block #4111023)>) but got an empty diff
   DEBUG  09-25 17:02:51    RegularChainSyncer  Task failure traceback
trinity.chains.RemoteTraceback:
"""
Traceback (most recent call last):
  File "/home/salgado/src/py-evm/trinity/chains/__init__.py", line 184, in wrapper
    return attr(*args, **kwargs)
  File "/home/salgado/src/py-evm/eth/chains/base.py", line 646, in import_block
    validate_imported_block_unchanged(imported_block, block)
  File "cytoolz/functoolz.pyx", line 236, in cytoolz.functoolz.curry.__call__
  File "cytoolz/functoolz.pyx", line 232, in cytoolz.functoolz.curry.__call__
  File "/home/salgado/src/py-evm/eth/utils/rlp.py", line 65, in validate_rlp_equal
    obj_b,
TypeError: block (<ByzantiumBlock(#Block #4111023)>) != imported block (<ByzantiumBlock(#Block #4111023)>) but got an empty diff
"""

[gsalgado]

Debug log: trinity-import-crash.log

[pipermerriam]

The __eq__ implementation of rlp.Serializable is:

def __eq__(self, other):
    return isinstance(other, Serizalizable) and hash(self) == hash(other)
def __hash__(self):
    return hash(tuple(self))

Since blocks have sequences of transactions and uncles on them, one possible thing that could be happening is that one of those somehow made it onto the object as a list instead of a tuple.

Beyond that, I think we need to do the extra work to update validate_rlp_equal to do a more robust introspection so that we can get helpful errors in these cases.

[gsalgado]

It's actually the headers that differ because one of them seems to have a stale ._hash_cache:

> /home/salgado/src/py-evm/eth/utils/rlp.py(61)validate_rlp_equal()
-> raise TypeError(
(Pdb) p obj_a.header._hash_cache
606040828753068555
(Pdb) p obj_b.header._hash_cache
1093354690931819732
(Pdb) p hash(tuple(obj_a.header))
606040828753068555
(Pdb) p hash(tuple(obj_b.header))
606040828753068555

[gsalgado]

And the above happens because the original block (the one we want to import) is pickled (with its ._hash_cache) and sent to a different process, but python's hash() builtin is not stable across processes.

This wasn't a problem in the past because it was only recently that validate_rlp_equal() was modified to raise a TypeError when the objects don't compare equally but the diff is empty

I can see a few alternatives to fix this:

1. Get rid of ._hash_cache from rlp.Serializable
2. Add a __getstate__() method to rlp.Serializable so we don't pickle the ._hash_cache attribute
3. Use a stable hashing implementation (e.g. hashlib) instead of the builtin hash()

I'd be in favour of 1 as I'm not sure caching the hash of rlp.Serializables provides a significant performance improvement anywhere

[carver]

This wasn't a problem in the past because it was only recently that validate_rlp_equal() was modified to raise a TypeError when the objects don't compare equally but the diff is empty

Nope, it's just a very infrequent bug. The very next line was failing when the diff is empty, because:

ValueError: max() arg is an empty sequence

Good deep dive on the hunting and solution!

[pipermerriam]

Just noting that there is no change needed for the validate_rlp_equal since the underlying issue here was beyond the scope of what the function could reasonable handle.

[pipermerriam]

This issue can be closed once the rlp dependency has been bumped to >=1.0.3