Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Clique Consensus to handle initialization internally #1874

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 34 additions & 6 deletions eth/abc.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
JournalDBCheckpoint,
AccountState,
HeaderParams,
VMConfiguration,
)


Expand Down Expand Up @@ -1638,9 +1639,8 @@ def validate_receipt(self, receipt: ReceiptAPI) -> None:
def validate_block(self, block: BlockAPI) -> None:
...

@classmethod
@abstractmethod
def validate_header(cls,
def validate_header(self,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

header: BlockHeaderAPI,
parent_header: BlockHeaderAPI,
check_seal: bool = True
Expand All @@ -1661,9 +1661,8 @@ def validate_transaction_against_header(self,
"""
...

@classmethod
@abstractmethod
def validate_seal(cls, header: BlockHeaderAPI) -> None:
def validate_seal(self, header: BlockHeaderAPI) -> None:
...

@classmethod
Expand All @@ -1688,6 +1687,34 @@ def state_in_temp_block(self) -> ContextManager[StateAPI]:
...


class VirtualMachineModifierAPI(ABC):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"""
Amend a set of VMs for a chain. This allows modifying a chain for different consensus schemes.
"""

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

@classmethod
@abstractmethod
def amend_vm_configuration_for_chain_class(cls, vm_config: VMConfiguration) -> None:
"""
Make amendments to the ``vm_config`` that are independent of any instance state. These
changes are applied across all instances of the chain where this
``VirtualMachineModifierAPI`` is applied on.
"""
...

@abstractmethod
def amend_vm_for_chain_instance(self, vm: VirtualMachineAPI) -> None:
"""
Make amendments to ``vm`` that are only valid for a specific chain instance. This
includes any modifications that depend on stateful data.
"""
...


class HeaderChainAPI(ABC):
header: BlockHeaderAPI
chain_id: int
Expand Down Expand Up @@ -1748,6 +1775,8 @@ class ChainAPI(ConfigurableAPI):
vm_configuration: Tuple[Tuple[BlockNumber, Type[VirtualMachineAPI]], ...]
chain_id: int
chaindb: ChainDatabaseAPI
consensus_engine_class: Type[VirtualMachineModifierAPI]
consensus_engine: VirtualMachineModifierAPI

#
# Helpers
Expand Down Expand Up @@ -1923,10 +1952,9 @@ def validate_gaslimit(self, header: BlockHeaderAPI) -> None:
def validate_uncles(self, block: BlockAPI) -> None:
...

@classmethod
@abstractmethod
def validate_chain(
cls,
self,
root: BlockHeaderAPI,
descendants: Tuple[BlockHeaderAPI, ...],
seal_check_random_sample_rate: int = 1) -> None:
Expand Down
37 changes: 28 additions & 9 deletions eth/chains/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,16 @@
ChainAPI,
ChainDatabaseAPI,
VirtualMachineAPI,
VirtualMachineModifierAPI,
ReceiptAPI,
ComputationAPI,
StateAPI,
SignedTransactionAPI,
UnsignedTransactionAPI,
)
from eth.consensus.pow import (
PowConsensus,
)
from eth.constants import (
EMPTY_UNCLE_HASH,
MAX_UNCLE_DEPTH,
Expand Down Expand Up @@ -113,6 +117,8 @@ class BaseChain(Configurable, ChainAPI):
chaindb_class: Type[ChainDatabaseAPI] = None
vm_configuration: Tuple[Tuple[BlockNumber, Type[VirtualMachineAPI]], ...] = None
chain_id: int = None
consensus_engine: VirtualMachineModifierAPI = None
cburgdorf marked this conversation as resolved.
Show resolved Hide resolved
consensus_engine_class: Type[VirtualMachineModifierAPI] = PowConsensus

@classmethod
def get_vm_class_for_block_number(cls, block_number: BlockNumber) -> Type[VirtualMachineAPI]:
Expand All @@ -136,9 +142,8 @@ def get_vm_class(cls, header: BlockHeaderAPI) -> Type[VirtualMachineAPI]:
#
# Validation API
#
@classmethod
def validate_chain(
cls,
self,
root: BlockHeaderAPI,
descendants: Tuple[BlockHeaderAPI, ...],
seal_check_random_sample_rate: int = 1) -> None:
Expand Down Expand Up @@ -168,9 +173,9 @@ def validate_chain(
f" but expected {encode_hex(parent.hash)}"
)
should_check_seal = index in indices_to_check_seal
vm_class = cls.get_vm_class_for_block_number(child.block_number)
vm = self.get_vm(child)
try:
vm_class.validate_header(child, parent, check_seal=should_check_seal)
vm.validate_header(child, parent, check_seal=should_check_seal)
except ValidationError as exc:
raise ValidationError(
f"{child} is not a valid child of {parent}: {exc}"
Expand All @@ -197,11 +202,23 @@ def __init__(self, base_db: AtomicDatabaseAPI) -> None:
else:
validate_vm_configuration(self.vm_configuration)

if not self.consensus_engine_class:
raise ValueError(
"The Chain class cannot be instantiated without a `consensus_engine_class`"
)
else:
self.consensus_engine = self.consensus_engine_class(base_db)
self.apply_consensus_engine_to_vms(self.consensus_engine)

self.chaindb = self.get_chaindb_class()(base_db)
self.headerdb = HeaderDB(base_db)
if self.gas_estimator is None:
self.gas_estimator = get_gas_estimator()

@classmethod
def apply_consensus_engine_to_vms(cls, consensus_engine: VirtualMachineModifierAPI) -> None:
consensus_engine.amend_vm_configuration_for_chain_class(cls.vm_configuration)

#
# Helpers
#
Expand Down Expand Up @@ -272,7 +289,9 @@ def get_vm(self, at_header: BlockHeaderAPI = None) -> VirtualMachineAPI:
header = self.ensure_header(at_header)
vm_class = self.get_vm_class_for_block_number(header.block_number)
chain_context = ChainContext(self.chain_id)
return vm_class(header=header, chaindb=self.chaindb, chain_context=chain_context)
vm = vm_class(header=header, chaindb=self.chaindb, chain_context=chain_context)
self.consensus_engine.amend_vm_for_chain_instance(vm)
return vm

#
# Header API
Expand Down Expand Up @@ -578,18 +597,18 @@ def validate_block(self, block: BlockAPI) -> None:
"""
if block.is_genesis:
raise ValidationError("Cannot validate genesis block this way")
VM_class = self.get_vm_class_for_block_number(BlockNumber(block.number))
vm = self.get_vm(block.header)
parent_header = self.get_block_header_by_hash(block.header.parent_hash)
VM_class.validate_header(block.header, parent_header, check_seal=True)
vm.validate_header(block.header, parent_header, check_seal=True)
self.validate_uncles(block)
self.validate_gaslimit(block.header)

def validate_seal(self, header: BlockHeaderAPI) -> None:
"""
Validate the seal on the given header.
"""
VM_class = self.get_vm_class_for_block_number(BlockNumber(header.block_number))
VM_class.validate_seal(header)
vm = self.get_vm(header)
vm.validate_seal(header)

def validate_gaslimit(self, header: BlockHeaderAPI) -> None:
"""
Expand Down
9 changes: 4 additions & 5 deletions eth/chains/mainnet/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,26 +45,25 @@
class MainnetDAOValidatorVM(HomesteadVM):
"""Only on mainnet, TheDAO fork is accompanied by special extra data. Validate those headers"""

@classmethod
def validate_header(cls,
def validate_header(self,
header: BlockHeaderAPI,
previous_header: BlockHeaderAPI,
check_seal: bool=True) -> None:

super().validate_header(header, previous_header, check_seal)

# The special extra_data is set on the ten headers starting at the fork
dao_fork_at = cls.get_dao_fork_block_number()
dao_fork_at = self.get_dao_fork_block_number()
extra_data_block_nums = range(dao_fork_at, dao_fork_at + 10)

if header.block_number in extra_data_block_nums:
if cls.support_dao_fork and header.extra_data != DAO_FORK_MAINNET_EXTRA_DATA:
if self.support_dao_fork and header.extra_data != DAO_FORK_MAINNET_EXTRA_DATA:
raise ValidationError(
f"Block {header!r} must have extra data "
f"{encode_hex(DAO_FORK_MAINNET_EXTRA_DATA)} not "
f"{encode_hex(header.extra_data)} when supporting DAO fork"
)
elif not cls.support_dao_fork and header.extra_data == DAO_FORK_MAINNET_EXTRA_DATA:
elif not self.support_dao_fork and header.extra_data == DAO_FORK_MAINNET_EXTRA_DATA:
raise ValidationError(
f"Block {header!r} must not have extra data "
f"{encode_hex(DAO_FORK_MAINNET_EXTRA_DATA)} when declining the DAO fork"
Expand Down
4 changes: 2 additions & 2 deletions eth/chains/tester/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,8 @@ class MainnetTesterChain(BaseMainnetTesterChain):
It exposes one additional API `configure_forks` to allow for in-flight
configuration of fork rules.
"""
@classmethod
def validate_seal(cls, block: BlockAPI) -> None:

def validate_seal(self, block: BlockAPI) -> None:
"""
We don't validate the proof of work seal on the tester chain.
"""
Expand Down
3 changes: 3 additions & 0 deletions eth/consensus/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
from .clique.clique import CliqueConsensus # noqa: F401
from .noproof import NoProofConsensus # noqa: F401
from .pow import PowConsensus # noqa: F401
33 changes: 18 additions & 15 deletions eth/consensus/clique/clique.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
import logging
from typing import Sequence, Iterable
from typing import (
Iterable,
Sequence,
)

from eth.abc import (
AtomicDatabaseAPI,
BlockHeaderAPI,
VirtualMachineAPI,
VirtualMachineModifierAPI,
)
from eth.db.chain import ChainDB

Expand All @@ -14,14 +18,12 @@
)
from eth_utils import (
encode_hex,
to_tuple,
ValidationError,
)

from eth.typing import (
HeaderParams,
VMConfiguration,
VMFork,
)
from eth.vm.chain_context import ChainContext
from eth.vm.execution_context import (
Expand Down Expand Up @@ -65,7 +67,7 @@ def _construct_turn_error_message(expected_difficulty: int,
)


class CliqueConsensus:
class CliqueConsensus(VirtualMachineModifierAPI):
"""
This class is the entry point to operate a chain under the rules of Clique consensus which
is defined in EIP-225: https://eips.ethereum.org/EIPS/eip-225
Expand All @@ -85,22 +87,23 @@ def __init__(self, base_db: AtomicDatabaseAPI, epoch_length: int = EPOCH_LENGTH)
self._epoch_length,
)

@to_tuple
def amend_vm_configuration(self, config: VMConfiguration) -> Iterable[VMFork]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@classmethod
def amend_vm_configuration_for_chain_class(cls, config: VMConfiguration) -> None:
"""
Amend the given ``VMConfiguration`` to operate under the rules of Clique consensus.
"""
for pair in config:
block_number, vm = pair
vm_class = vm.configure(
extra_data_max_bytes=65535,
validate_seal=staticmethod(self.validate_seal),
create_execution_context=staticmethod(self.create_execution_context),
configure_header=configure_header,
_assign_block_rewards=lambda _, __: None,
)

yield block_number, vm_class
setattr(vm, 'extra_data_max_bytes', 65535)
setattr(vm, 'create_execution_context', staticmethod(cls.create_execution_context))
setattr(vm, 'configure_header', configure_header)
setattr(vm, '_assign_block_rewards', lambda *_: None)

def amend_vm_for_chain_instance(self, vm: VirtualMachineAPI) -> None:
# `validate_seal` is stateful. We would not want two different instances of GoerliChain
# to operate on the same instance of CliqueConsensus. Therefore, this modification needs
# to be done for a specic chain instance, not chain class.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

setattr(vm, 'validate_seal', self.validate_seal)

@staticmethod
def create_execution_context(header: BlockHeaderAPI,
Expand Down
29 changes: 29 additions & 0 deletions eth/consensus/noproof.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
from eth.abc import (
AtomicDatabaseAPI,
VirtualMachineAPI,
VirtualMachineModifierAPI,
)
from eth.typing import (
VMConfiguration,
)


class NoProofConsensus(VirtualMachineModifierAPI):
"""
Modify a set of VMs to accept blocks without any validation.
"""

def __init__(self, base_db: AtomicDatabaseAPI) -> None:
pass

@classmethod
def amend_vm_configuration_for_chain_class(cls, config: VMConfiguration) -> None:
"""
Amend the given ``VMConfiguration`` to operate under the default POW rules.
"""
for pair in config:
block_number, vm = pair
setattr(vm, 'validate_seal', lambda *_: None)

def amend_vm_for_chain_instance(self, vm: VirtualMachineAPI) -> None:
setattr(vm, 'validate_seal', lambda *_: None)
Loading