Refactor Clique Consensus to handle initialization internally

[cburgdorf]

What was wrong?

The current clique implementation requires attaching a DB to the chain class (not chain instance). This is problematic (see discussion in Trinity)

It is also flawed as it is leaking a stateful consensus engine instance on different chain instances that should be independent.

For instance:

chain = GoerliChain()
second_chain = GoerliChain()

# this may add a signer which is now leaking to the second_chain instance
chain.validate_seal(...)  

How was it fixed?

1. This introduces a VirtualMachineModifierAPI interface defined as

class VirtualMachineModifierAPI(ABC):

    @abstractmethod
    def __init__(self, base_db: AtomicDatabaseAPI) -> None:
        ...

    @classmethod
    @abstractmethod
    def amend_vm_configuration_for_chain_class(
            cls,
            config: VMConfiguration) -> Tuple[Tuple[BlockNumber, Type[VirtualMachineAPI]], ...]:
        """
        Make amendments to the ``VMConfiguration`` such as redefining its methods for different
        consensus schemes.
        """
        ...

    @abstractmethod
    def amend_vm_class_for_chain_instance(
            self,
            vm_class: Type[VirtualMachineAPI]) -> Type[VirtualMachineAPI]:
        """
        Make amendments to a VM class that are not supposed to be shared across different chain
        instances.
        """
        ...

The amend_vm_configuration_for_chain_class defines a classmethod which can perform all kind of class relevant actions on the VMConfiguration. This includes things like setting extra_data_max_bytes to 65535 which is safe to share across all GoerliChain instances.

But it also defines an instance method amend_vm_class_for_chain_instance which performs modifications that are not safe to share across different chain instances. Such as replacing validate_seal with an implementation that is stateful.

2. It also means that validate_seal on the VM had to become an instance method which also means that validate_chain on the VM and validate_chain on the Chain had to become instance methods. A fact that @carver had also pointed out in the linked Trintiy thread.

3. This also adds a PowConsensus and a NoProofConsensus engine and refactors some test helpers.

4. There's a test that proves that stateful consensus engines do in fact not leak across different chain instances.

To-Do

• Add entry to the release notes

Cute Animal Picture

[pipermerriam]

I like this direction (and I like the formalization of the ConsensysEngine classes)

[cburgdorf]

I've been wondering if this should be named something like ConsensusEngineAPI. That would be more aligned with how it is used but on the other hand, I'm usually a fan of small focused and therefore very generic interfaces which may be sticked together to create more specialized interfaces. And with that in mind, there's no reason why this should be called ConsensusEngineAPI when in reality there's nothing consensus engine specific about it.

[carver]

Direction looks solid!

[cburgdorf]

@carver Alright, this has taken more time than I anticipated. I think this is ready for review. There's also a draft PR of the Görli integration that uses this branch: ethereum/trinity#1196

[carver]

Thoughts on moving the consensus engine to the VM? It feels like we're patching patches of patches right now, so I think we need to step back.

[carver]

I still kind of like the idea of validate_header dropping the seal check, so it can stay a classmethod. But... I'm okay saving that for a different PR, since it probably causes a decent amount of churn, where places need to remove the check_seal and add a validate_seal.

[carver]

I liked this old approach of not mutating the original VM class. If this was just to make it easier to write the test, I think we can find another way.

[cburgdorf]

I agree but I don't have a good answer. With the old approach you always end up having different classes then those defined in cls.vm_configuration. We have tests that assert things like assert chain.vm_configuration == RopstenChain.vm_configuration that break because of that. Is there an alternative (apart from formalizing every method that needs to be replaced in an interface and delegating to it)

[carver]

Getting backed into a corner here, where this seems like the best solution, makes me think we're doing something else wrong at an architectural level.

One avenue to explore is the location of the consensus class. I'm thinking this might all get a lot easier if the consensus_class were a feature of the VM instead of the chain. Arguably, that better represents the real world anyway, as the consensus rules might change at a fork. Then the class gets initialized inside the VM. VM.validate_seal() could just forward to VM.consensus_engine.validate_seal(), etc.

[cburgdorf]

I was thinking the same especially because validate_seal is always going to be overwritten. The thing that I'm unsure about though: There's no clear picture about which other methods expect to be overwritten. Some consensus engine may fiddle with block rewards another might do funky things with uncles. If we take Clique as a driver then we end up formalizing validate_seal extra_data_max_bytes, create_execution_context, configure_header, _assign_block_rewardsfor the consensus engine whereasno_proofandpowonly needvalidate_seal`.

Arguably, that better represents the real world anyway, as the consensus rules might change at a fork

But it also becomes really blurry where consensus rules go. I mean, every new VM is a consensus change and that's what VMs are for. Other consensus engines are a weird and barely formalized way of somehow hacking and bending the system (e.g. how extra_data, nonce, coinbase get totally different meaning in Clique) to somehow work. Something tells me that deserves a weird patchy answer and the current APIs at least break it down to a very generic level of hacking things on the class or the instance level.

[cburgdorf]

Replaced by #1899