-
Notifications
You must be signed in to change notification settings - Fork 686
[WIP] add support for EIP-1559 #2005
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
Changes from 9 commits
8070f2d
c0e67ff
0b15249
bb0c3f7
51d353e
765636f
9da46a6
cf48c45
48ad8fb
3b9eb9a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -74,6 +74,7 @@ class MiningHeaderAPI(ABC): | |
gas_used: int | ||
timestamp: int | ||
extra_data: bytes | ||
base_fee_per_gas: int # EIP-1559 | ||
|
||
@property | ||
@abstractmethod | ||
|
@@ -326,14 +327,17 @@ class TransactionFieldsAPI(ABC): | |
""" | ||
A class to define all common transaction fields. | ||
""" | ||
max_fee_per_gas: int | ||
max_priority_fee_per_gas: int | ||
|
||
@property | ||
@abstractmethod | ||
def nonce(self) -> int: | ||
... | ||
|
||
@property | ||
@abstractmethod | ||
def gas_price(self) -> int: | ||
def gas_price(self) -> Optional[int]: | ||
... | ||
|
||
@property | ||
|
@@ -1684,6 +1688,14 @@ def chain_id(self) -> int: | |
""" | ||
... | ||
|
||
@property | ||
@abstractmethod | ||
def base_gas_fee(self) -> Optional[int]: | ||
""" | ||
Return the base gas fee of the block | ||
""" | ||
... | ||
|
||
|
||
class ComputationAPI(ContextManager['ComputationAPI'], StackManipulationAPI): | ||
""" | ||
|
@@ -2865,9 +2877,8 @@ def get_computation(self, | |
# | ||
# Transaction context | ||
# | ||
@classmethod | ||
@abstractmethod | ||
def get_transaction_context_class(cls) -> Type[TransactionContextAPI]: | ||
def get_transaction_context_class(self) -> Type[TransactionContextAPI]: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like this one could stay a |
||
""" | ||
Return the :class:`~eth.vm.transaction_context.BaseTransactionContext` class that the | ||
state class uses. | ||
|
@@ -2917,9 +2928,8 @@ def validate_transaction(self, transaction: SignedTransactionAPI) -> None: | |
""" | ||
... | ||
|
||
@classmethod | ||
@abstractmethod | ||
def get_transaction_context(cls, | ||
def get_transaction_context(self, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, bummer that this can't be a class method anymore. I can maybe imagine doing something where the transaction context no longer gives access to the active gas price without explicit access to the execution-context (like as a parameter to a new |
||
transaction: SignedTransactionAPI) -> TransactionContextAPI: | ||
""" | ||
Return the :class:`~eth.abc.TransactionContextAPI` for the given ``transaction`` | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,7 +56,13 @@ | |
validate_block_number, | ||
validate_word, | ||
) | ||
from eth.vm.forks.london.blocks import ( | ||
LondonBlockHeader | ||
) | ||
|
||
from rlp.exceptions import ( | ||
ObjectDeserializationError | ||
) | ||
|
||
class HeaderDB(HeaderDatabaseAPI): | ||
def __init__(self, db: AtomicDatabaseAPI) -> None: | ||
|
@@ -619,4 +625,8 @@ def _add_block_number_to_hash_lookup(db: DatabaseAPI, header: BlockHeaderAPI) -> | |
# be looking up recent blocks. | ||
@functools.lru_cache(128) | ||
def _decode_block_header(header_rlp: bytes) -> BlockHeaderAPI: | ||
return rlp.decode(header_rlp, sedes=BlockHeader) | ||
try: | ||
return rlp.decode(header_rlp, sedes=BlockHeader) | ||
except ObjectDeserializationError: | ||
# could be a new >=London block header | ||
return rlp.decode(header_rlp, sedes=LondonBlockHeader) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this look good, or should we find some way to check the header type first? |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,7 @@ | |
istanbul_at, | ||
muir_glacier_at, | ||
berlin_at, | ||
london_at, | ||
latest_mainnet_at, | ||
) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,3 +28,6 @@ | |
from .berlin import ( # noqa: F401 | ||
BerlinVM, | ||
) | ||
from .london import ( # noqa: F401 | ||
LondonVM | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels... unfortunate, but it might be necessary. Ideally, it would be nice if these fields didn't leak into all the previous VM APIs.... but doing so might introduce even more unfortunate complexity into our type definitions which might be worse...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree and will take a deeper look -- for now I can say a similar problem applies to transactions. Base
transaction.validate
methods all type-checkgas_price
which is no longer a thing in London transactions so they need to be duplicated almost entirely. We could keepgas_price
as an (internal, not exposed) alias for eithermax_fee_per_gas
ormax_priority_fee_per_gas
so that we could extend and call basic validation methods viasuper()
, but it sounds confusing :(There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After dealing with typed transactions, my general take on it was: the latest VM should have the cleanest implementation. If a kludge is called for, it should be on the old VMs (like v vs y-parity in the signature, where we started to prefer y-parity as the default/native field). At first glance, this change seems to follow that approach, so I'm 👌🏻 to keep it. I'll comment if I stumble on a better option while reading the rest.