-
Notifications
You must be signed in to change notification settings - Fork 680
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
Delete gas used from state #630
Changes from all commits
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 |
---|---|---|
|
@@ -75,7 +75,6 @@ def __init__(self, header, chaindb): | |
db=self.chaindb.db, | ||
execution_context=self.block.header.create_execution_context(self.previous_hashes), | ||
state_root=self.block.header.state_root, | ||
gas_used=self.block.header.gas_used, | ||
) | ||
|
||
# | ||
|
@@ -131,30 +130,64 @@ def execute_bytecode(self, | |
) | ||
|
||
@abstractmethod | ||
def make_receipt(self, transaction, computation, state): | ||
def make_receipt(self, base_header, transaction, computation, state): | ||
""" | ||
Make receipt. | ||
Generate the receipt resulting from applying the transaction. | ||
|
||
:param base_header: the header of the block before the transaction was applied. | ||
:param transaction: the transaction used to generate the receipt | ||
:param computation: the result of running the transaction computation | ||
:param state: the resulting state, after executing the computation | ||
|
||
:return: receipt | ||
""" | ||
raise NotImplementedError("Must be implemented by subclasses") | ||
|
||
def apply_transaction(self, transaction): | ||
@abstractmethod | ||
def validate_transaction_against_header(self, base_header, transaction): | ||
""" | ||
Validate that the given transaction is valid to apply to the given header. | ||
|
||
:param base_header: header before applying the transaction | ||
:param transaction: the transaction to validate | ||
|
||
:raises: ValidationError if the transaction is not valid to apply | ||
""" | ||
raise NotImplementedError("Must be implemented by subclasses") | ||
|
||
def apply_transaction(self, header, transaction): | ||
""" | ||
Apply the transaction to the current block. This is a wrapper around | ||
:func:`~evm.vm.state.State.apply_transaction` with some extra orchestration logic. | ||
|
||
:param header: header of the block before application | ||
:param transaction: to apply | ||
""" | ||
self.validate_transaction_against_header(header, transaction) | ||
state_root, computation = self.state.apply_transaction(transaction) | ||
receipt = self.make_receipt(transaction, computation, self.state) | ||
# TODO: remove this mutation. | ||
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. This was the inspiration for most of the PR. |
||
self.state.gas_used = receipt.gas_used | ||
receipt = self.make_receipt(header, transaction, computation, self.state) | ||
|
||
new_header = self.block.header.copy( | ||
bloom=int(BloomFilter(self.block.header.bloom) | receipt.bloom), | ||
new_header = header.copy( | ||
bloom=int(BloomFilter(header.bloom) | receipt.bloom), | ||
gas_used=receipt.gas_used, | ||
state_root=state_root, | ||
) | ||
|
||
return new_header, receipt, computation | ||
|
||
def _apply_all_transactions(self, transactions, base_header): | ||
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. How do you feel about sneaking in type definitions for new / changed functions? I do realize it (temporary) reduces conformity for that file in question when only few defs are typed while others are not but on the other hand it helps to work towards our goal to have full coverage and eventually enable stricter type checks to disallow untyped defs altogether. 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. ⚖️ I'm a little on the fence. These APIs will continue to change a lot. I guess I'm slightly 👎 at the moment, but when I feel like things start to settle, I'll be a lot more keen. |
||
receipts = [] | ||
previous_header = base_header | ||
result_header = base_header | ||
|
||
for transaction in transactions: | ||
result_header, receipt, _ = self.apply_transaction(previous_header, transaction) | ||
|
||
previous_header = result_header | ||
receipts.append(receipt) | ||
|
||
return result_header, receipts | ||
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. It was nice to pull this chunk of logic out, but it was awkward to both:
A small version involved a lot of variable clobbering, like: def _apply_all_transactions(self, transactions, header):
receipts = []
for transaction in transactions:
header, receipt, _ = self.apply_transaction(header, transaction)
receipts.append(receipt)
return header, receipts I'm happy to hear suggestions about a way to do chaining & accumulation at the same time, in a more elegant way. 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. Hammered out this accumulation based zero mutation way to do this.
Requires using the 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. Thanks! It got me thinking of some other ways to look at it. Right now, to my eyes, that particular implementation is a drop in readability, compared to having some variable reuse in a short function. I have an idea for an improvement to get rid of it, but I don't want to throw this PR off track, so I'll put it in the next one. 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. I think in implementation it will look like this. # top level helper function.
def _accumulate_apply_txn_fn(prev_result, apply_transaction_fn):
return apply_transaction_fn(prev_result[0])
# in VM class
def import_block(self, block):
...
if block.transactions:
apply_transaction_fns = tuple(
functools.partial(self.apply_transaction, transaction=transaction)
for transaction in block.transactions
)
headers, receipts = zip(*tuple(accumulate(
_accumulate_apply_txn_fn,
apply_transaction_fns,
(self.block.header, None),
))[1:])
last_header = headers[-1]
else:
last_header = self.block.header
receipts = tuple()
... I think I generally agree about the readability, but I'm also inclined to cut the extra API from the Dunno, your call. Just wanted to see it all written out mostly. |
||
|
||
# | ||
# Mining | ||
# | ||
|
@@ -179,25 +212,14 @@ def import_block(self, block): | |
db=self.chaindb.db, | ||
execution_context=self.block.header.create_execution_context(self.previous_hashes), | ||
state_root=self.block.header.state_root, | ||
gas_used=self.block.header.gas_used, | ||
) | ||
|
||
# run all of the transactions. | ||
execution_data = [ | ||
self.apply_transaction(transaction) | ||
for transaction | ||
in block.transactions | ||
] | ||
if execution_data: | ||
headers, receipts, _ = zip(*execution_data) | ||
header_with_txns = headers[-1] | ||
else: | ||
receipts = tuple() | ||
header_with_txns = self.block.header | ||
last_header, receipts = self._apply_all_transactions(block.transactions, self.block.header) | ||
|
||
self.block = self.set_block_transactions( | ||
self.block, | ||
header_with_txns, | ||
last_header, | ||
block.transactions, | ||
receipts, | ||
) | ||
|
@@ -283,7 +305,6 @@ def state_in_temp_block(self): | |
db=self.chaindb.db, | ||
execution_context=temp_block.header.create_execution_context(prev_hashes), | ||
state_root=temp_block.header.state_root, | ||
gas_used=0, | ||
) | ||
|
||
snapshot = state.snapshot() | ||
|
@@ -315,6 +336,13 @@ def validate_block(self, block): | |
""" | ||
Validate the the given block. | ||
""" | ||
if not isinstance(block, self.get_block_class()): | ||
raise ValidationError( | ||
"This vm ({0!r}) is not equipped to validate a block of type {1!r}".format( | ||
self, | ||
block, | ||
) | ||
) | ||
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. I guess this check will be moot, when this method moves up to Chain. But we are a couple days from moving this one, I think. |
||
if not block.is_genesis: | ||
parent_header = get_parent_header(block.header, self.chaindb) | ||
|
||
|
@@ -435,13 +463,6 @@ def get_block_class(cls): | |
""" | ||
return cls.get_state_class().get_block_class() | ||
|
||
@classmethod | ||
def get_block_by_header(cls, block_header, db): | ||
""" | ||
Lookup and return the block for the given header. | ||
""" | ||
return cls.get_block_class().from_header(block_header, db) | ||
|
||
@classmethod | ||
@functools.lru_cache(maxsize=32) | ||
@to_tuple | ||
|
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.
Great to see you're taking the chance to directly improve the documentation ❤️