[WIP] add support for EIP-1559 #2005
Conversation
|
I think yes, you can keep going. I only gave this a shallow review but the direction and structure looks good. I didn't dig into the actual method implementations yet. My main recommendation would be to extract the "stub" for London from this PR to reduce the size of the diff. See #1969 for how this was done for Berlin. |
| @@ -74,6 +74,8 @@ class MiningHeaderAPI(ABC): | |||
| gas_used: int | |||
| timestamp: int | |||
| extra_data: bytes | |||
| gas_target: int # EIP-1559 | |||
| base_fee_per_gas: int # EIP-1559 | |||
There was a problem hiding this comment.
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.
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-check gas_price which is no longer a thing in London transactions so they need to be duplicated almost entirely. We could keep gas_price as an (internal, not exposed) alias for either max_fee_per_gas or max_priority_fee_per_gas so that we could extend and call basic validation methods via super(), but it sounds confusing :(
There was a problem hiding this comment.
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.
|
|
||
| def normalize_transaction( | ||
| transaction: SignedTransactionAPI | ||
| ) -> SignedTransactionAPI: |
There was a problem hiding this comment.
This function modifies the transaction object rather than creating an instance of a possible new LondonTypedTransaction class so that both new (London-specific) and existing validation code can work on all transactions. However this means that max_priority_fee_per_gas and max_fee_per_gas cannot be read-only properties, and I'm wondering how should we handle users trying to access the new properties in pre-London transactions?
| 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.
Does this look good, or should we find some way to check the header type first?
| @property | ||
| def gas_price(self) -> None: | ||
| # maybe add a warning, or raise an exception instead? | ||
| return None |
There was a problem hiding this comment.
How should we handle this? gas_price is no longer a transaction attribute. It could still be interpreted as "effective gas price", but that requires knowing the base gas fee of the block so it would not a be "transaction-only" attribute anymore
|
@pipermerriam would you take another look? Sorry it took me a while. Two main things I'd like to get some feedback on:
Things that still have to be done are here + linting and style -- please let me know if this is too large/hard to read and you'd like me to better separate commits before taking a look. Thanks |
| @@ -74,6 +74,8 @@ class MiningHeaderAPI(ABC): | |||
| gas_used: int | |||
| timestamp: int | |||
| extra_data: bytes | |||
| gas_target: int # EIP-1559 | |||
| base_fee_per_gas: int # EIP-1559 | |||
There was a problem hiding this comment.
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.
| @abstractmethod | ||
| def get_transaction_context_class(cls) -> Type[TransactionContextAPI]: | ||
| def get_transaction_context_class(self) -> Type[TransactionContextAPI]: |
There was a problem hiding this comment.
Looks like this one could stay a classmethod
| if isinstance(transaction, LondonTypedTransaction): # TODO probably do this somewhere else | ||
| priority_fee_per_gas = min( | ||
| transaction.max_priority_fee_per_gas, | ||
| transaction.max_fee_per_gas - self.execution_context.base_gas_fee | ||
| ) | ||
| else: | ||
| priority_fee_per_gas = min( | ||
| transaction.gas_price, | ||
| transaction.gas_price - self.execution_context.base_gas_fee | ||
| ) |
There was a problem hiding this comment.
Yeah, my instinct is to put this in the transaction class, just have everything implement a priority_fee_per_gas attribute. Then we get to skip the instance check.
| @abstractmethod | ||
| def get_transaction_context(cls, | ||
| def get_transaction_context(self, |
There was a problem hiding this comment.
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 get_gas_price(execution_context) method). Or something along those lines. I am okay with this solution though. 👍🏻
|
Thanks for getting this started! The commits from this PR are integrated to #2013 |
Hi!
I tried to add new block headers and transactions, and a bit of header validation. Please take a look at those -- do they make sense? Should those changes to blocks/transactions be applied somewhere else? Should I keep going?
Thanks :)