Return header instead of block from vm.apply_txn #625
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pipermerriam
approved these changes
May 2, 2018
evm/chains/base.py
Outdated
| @@ -487,20 +487,29 @@ def apply_transaction(self, transaction): | |||
| heavy and incurs significant perferomance overhead. | |||
| """ | |||
| vm = self.get_vm() | |||
| block, receipt, computation = vm.apply_transaction(transaction) | |||
| receipts = block.get_receipts(self.chaindb) + [receipt] | |||
| old_block = vm.block | |||
There was a problem hiding this comment.
base_blockprevious_block
If you like either better.
Temporarily in VM, but eventually in Chain, once the state tests get refactored.
pipermerriam
approved these changes
May 2, 2018
evm/vm/base.py
Outdated
|
|
||
| return self.mine_block() | ||
|
|
||
| def seal_block(self, base_block, new_header, transactions, receipts): |
There was a problem hiding this comment.
I don't like the term seal as for me at least, it implies mining like setting the mix_hash and nonce.
What about just add_transaction_to_block or something?
There was a problem hiding this comment.
How about set_transactions_in_block?
- pro: transactions is plural, old transactions in block are replaced, and -- for now -- the trie data is persisted
- con: implies block mutation, when it's really building a block copy
Other alternatives:
place_transactions_in_blockcommit_transactions_to_blockget_block_with_transactions(problem: get implies no side-effects)add_transactions_to_block(problem: add implies that previous transactions are left in place)
There was a problem hiding this comment.
set_block_transactions(keeps withget_,set_convention...)update_block_transactionsjust an alternate prefix.
any of of those work for you?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What was wrong?
Related to #599 & #507 - VM returns partially-completed block objects as a result of
apply_transaction()How was it fixed?
Return the header instead, and let
chaindeal with building the block (at the appropriate time). As a bonus, we build fewer block objects duringvm.import_block().TODO in follow-up PR: decide how to deal with the exact some problem with
headerwhich is only partially formed (no state/receipt root) -- maybe this is where aTransactionDiffcomes in`Cute Animal Picture