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.

Sign up for GitHub

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

Redesign receipt/tx trie generation on block imports #565

Merged
merged 3 commits into from
Apr 19, 2018
Merged

Redesign receipt/tx trie generation on block imports #565

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eb9b34e
Redesign receipt/tx trie generation on block imports
gsalgado Apr 17, 2018
21fbcef
Get rid of BaseState.receipts
gsalgado Apr 17, 2018
acfd862
Drop Chain.apply_transaction()
gsalgado Apr 18, 2018
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
19 changes: 0 additions & 19 deletions evm/chains/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,13 +209,6 @@ def get_vm(self, header=None):
#
# Execution API
#
@abstractmethod
def apply_transaction(self, transaction):
"""
Applies the transaction to the current head block of the Chain.
"""
raise NotImplementedError("Chain classes must implement this method")

@abstractmethod
def estimate_gas(self, transaction, at_header=None):
"""
Expand Down Expand Up @@ -487,18 +480,6 @@ def from_genesis_header(cls, chaindb, genesis_header):
#
# Mining and Execution API
#
def apply_transaction(self, transaction):
"""
Applies the transaction to the current head block of the Chain.
"""
vm = self.get_vm()
computation, block = vm.apply_transaction(transaction)

# Update header
self.header = block.header

return computation

def estimate_gas(self, transaction, at_header=None):
if at_header is None:
at_header = self.get_canonical_head()
Expand Down
40 changes: 28 additions & 12 deletions evm/vm/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@
ABCMeta,
abstractmethod
)

import contextlib
import functools
import logging
import rlp
from typing import List # noqa: F401

import functools
import rlp

from eth_utils import (
keccak,
Expand All @@ -28,6 +28,7 @@
from evm.rlp.headers import (
BlockHeader,
)
from evm.rlp.receipts import Receipt # noqa: F401
from evm.utils.datatypes import (
Configurable,
)
Expand Down Expand Up @@ -62,6 +63,7 @@ def __init__(self, header, chaindb):
self.chaindb = chaindb
block_class = self.get_block_class()
self.block = block_class.from_header(header=header, chaindb=self.chaindb)
self.receipts = [] # type: List[Receipt]

#
# Logging
Expand All @@ -77,14 +79,12 @@ def apply_transaction(self, transaction):
"""
Apply the transaction to the vm in the current block.
"""
computation, block, trie_data_dict = self.state.apply_transaction(
computation, block, receipt = self.state.apply_transaction(
Copy link
Contributor

@hwwhww hwwhww Apr 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Reminder: the reason why apply_transaction returned trie_data_dict is for the stateless client to generate witness, which was discussed here: #247 (comment)

But since the sharding roadmap was (largely) changed, now the stateless client implementation won't happen in couple months, so I don't have a strong opinion about the change. Hope that we will find a solution to solve in both stateless/non-stateless clients when we reimplement stateless client. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could have the VM incrementally generate the trie data, I suppose? The problem was that it was re-generating it from scratch for every transaction applied

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Except it wouldn't make the VM class stateless, so I guess that wouldn't really buy us anything

Copy link
Member

Choose a reason for hiding this comment

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

@hwwhww we'll continue to move towards stateless clients but for now going to prioritze full sync and then we'll figure out how to make it work with stateless clients also.

transaction,
self.block,
)
self.block = block

# Persist changed transaction and receipt key-values to self.chaindb.
self.chaindb.persist_trie_data_dict(trie_data_dict)
self.receipts.append(receipt)

self.clear_journal()

Expand Down Expand Up @@ -132,6 +132,7 @@ def execute_bytecode(self,
# Mining
#
def import_block(self, block):
self.receipts = []
Copy link
Member

Choose a reason for hiding this comment

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

This stood out to me. What if instead of tracking this state we instead returned the receipt from apply_transaction and accumulated the receipts in the loop here

It looks like this change goes well with the changes within the VMState class. It looks like we could remove the need for that class to be passed the receipts since I think this changes things so that the only thing it needs to know is the gas_used value from the latest receipt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, but should we then pass gas_used as an argument to __init__() and keep updating it in add_transaction()? Or did you have something else in mind?

Copy link
Member

Choose a reason for hiding this comment

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

Honestly not sure. Nothing specific in mind for exact implementation but starting with gas_used being passed into __init__ seems like a good start. It'd be nice if it didn't need to be mutated but i'm more ok with mutating that value than the receipt storage on the VM instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, we need gas_used in BaseState so I'm passing it via __init__() and updating it on BaseState.add_transaction(), but in BaseVM we also need that value in the .state @Property, which is currently looking that up from BaseVM.receipts but we could instead have a BaseVM.gas_used and update that on BaseVM.apply_transaction(). Would you prefer that?

Copy link
Member

Choose a reason for hiding this comment

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

That sounds viable. We can continue to iterate on this to I'd recommend just picking something that seems good enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We actually need to keep the receipts somewhere in order to generate the receipt trie

self.configure_header(
coinbase=block.header.coinbase,
gas_limit=block.header.gas_limit,
Expand All @@ -156,6 +157,13 @@ def mine_block(self, *args, **kwargs):
Mine the current block. Proxies to self.pack_block method.
"""
block = self.block
tx_root_hash, tx_kv_nodes = make_trie_root_and_nodes(block.transactions)
receipt_root_hash, receipt_kv_nodes = make_trie_root_and_nodes(self.receipts)
self.chaindb.persist_trie_data_dict(tx_kv_nodes)
self.chaindb.persist_trie_data_dict(receipt_kv_nodes)
self.block.header.transaction_root = tx_root_hash
self.block.header.receipt_root = receipt_root_hash

self.pack_block(block, *args, **kwargs)

if block.number == 0:
Expand Down Expand Up @@ -211,7 +219,10 @@ def state_in_temp_block(self):
header = self.block.header
temp_block = self.generate_block_from_parent_header_and_coinbase(header, header.coinbase)
prev_hashes = (header.hash, ) + self.previous_hashes
state = self.get_state(self.chaindb, temp_block, prev_hashes)
gas_used = 0
if self.receipts:
gas_used = self.receipts[-1].gas_used
state = self.get_state(self.chaindb, temp_block, prev_hashes, gas_used)
assert state.gas_used == 0, "There must not be any gas used in a fresh temporary block"

snapshot = state.snapshot()
Expand Down Expand Up @@ -266,6 +277,8 @@ def validate_block(self, block):
)
)

# FIXME: make_trie_root_and_nodes() is rather expensive, and we already run that once in
# import_block(), so should refactor some of this code to avoid re-generating it here.
Copy link
Member

Choose a reason for hiding this comment

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

This seems like we could fix this with some caching on the make_trie_root_and_nodes function (or maybe a wrapper around that function which implements a cache).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I guess #573 might still be useful!

tx_root_hash, _ = make_trie_root_and_nodes(block.transactions)
if tx_root_hash != block.header.transaction_root:
raise ValidationError(
Expand Down Expand Up @@ -435,7 +448,7 @@ def get_state_class(cls):
return cls._state_class

@classmethod
def get_state(cls, chaindb, block, prev_hashes):
def get_state(cls, chaindb, block, prev_hashes, gas_used):
"""
Return state object

Expand All @@ -445,21 +458,24 @@ def get_state(cls, chaindb, block, prev_hashes):
:return: state with root defined in block
"""
execution_context = block.header.create_execution_context(prev_hashes)
receipts = block.get_receipts(chaindb)

return cls.get_state_class()(
chaindb,
execution_context=execution_context,
state_root=block.header.state_root,
receipts=receipts,
gas_used=gas_used,
)

@property
def state(self):
"""Return current state property
"""
gas_used = 0
if self.receipts:
gas_used = self.receipts[-1].gas_used
return self.get_state(
chaindb=self.chaindb,
block=self.block,
prev_hashes=self.previous_hashes
prev_hashes=self.previous_hashes,
gas_used=gas_used,
)
49 changes: 7 additions & 42 deletions evm/vm/state.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,6 @@
TYPE_CHECKING
)

from cytoolz import (
merge,
)

from eth_utils import (
to_bytes
)
Expand All @@ -25,9 +21,6 @@
MAX_PREV_HEADER_DEPTH,
UINT_256_MAX,
)
from evm.db.trie import (
make_trie_root_and_nodes,
)
from evm.utils.datatypes import (
Configurable,
)
Expand All @@ -51,19 +44,18 @@ class BaseState(Configurable, metaclass=ABCMeta):
_chaindb = None
execution_context = None
state_root = None
receipts = None
access_logs = None
gas_used = None

block_class = None # type: Type[BaseBlock]
computation_class = None # type: Type[BaseComputation]
trie_class = None
transaction_context_class = None # type: Type[BaseTransactionContext]

def __init__(self, chaindb, execution_context, state_root, receipts=[]):
def __init__(self, chaindb, execution_context, state_root, gas_used):
self._chaindb = chaindb
self.execution_context = execution_context
self.state_root = state_root
self.receipts = receipts
self.gas_used = gas_used

#
# Logging
Expand Down Expand Up @@ -96,16 +88,6 @@ def difficulty(self):
def gas_limit(self):
return self.execution_context.gas_limit

#
# Helpers
#
@property
def gas_used(self):
if self.receipts:
return self.receipts[-1].gas_used
else:
return 0

#
# read only state_db
#
Expand Down Expand Up @@ -299,9 +281,9 @@ def apply_transaction(
computation = self.execute_transaction(transaction)

# Set block.
block, trie_data_dict = self.add_transaction(transaction, computation, block)
block, receipt = self.add_transaction(transaction, computation, block)
block.header.state_root = self.state_root
return computation, block, trie_data_dict
return computation, block, receipt

def add_transaction(self, transaction, computation, block):
"""
Expand All @@ -322,7 +304,7 @@ def add_transaction(self, transaction, computation, block):
:rtype: (Block, dict[bytes, bytes])
"""
receipt = self.make_receipt(transaction, computation)
self.add_receipt(receipt)
self.gas_used = receipt.gas_used

# Create a new Block object
block_header = block.header.clone()
Expand All @@ -331,29 +313,12 @@ def add_transaction(self, transaction, computation, block):

block.transactions.append(transaction)

# Get trie roots and changed key-values.
tx_root_hash, tx_kv_nodes = make_trie_root_and_nodes(
block.transactions,
self.trie_class,
)
receipt_root_hash, receipt_kv_nodes = make_trie_root_and_nodes(
self.receipts,
self.trie_class,
)

trie_data = merge(tx_kv_nodes, receipt_kv_nodes)

block.bloom_filter |= receipt.bloom

block.header.transaction_root = tx_root_hash
block.header.receipt_root = receipt_root_hash
block.header.bloom = int(block.bloom_filter)
block.header.gas_used = receipt.gas_used

return block, trie_data

def add_receipt(self, receipt):
self.receipts.append(receipt)
return block, receipt

@abstractmethod
def make_receipt(self, transaction, computation):
Expand Down
11 changes: 0 additions & 11 deletions tests/core/chain-object/test_chain.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,17 +34,6 @@ def tx(chain, funded_address, funded_address_private_key):
return new_transaction(vm, from_, recipient, amount, funded_address_private_key)


def test_apply_transaction(chain, tx):
vm = chain.get_vm()

computation = chain.apply_transaction(tx)

# Check if the state is updated.
vm = chain.get_vm()
assert vm.state.state_root == computation.state.state_root
assert vm.state.read_only_state_db.get_balance(tx.to) == tx.value


def test_import_block_validation(valid_chain, funded_address, funded_address_initial_balance):
block = rlp.decode(valid_block_rlp, sedes=FrontierBlock)
imported_block = valid_chain.import_block(block)
Expand Down
8 changes: 4 additions & 4 deletions tests/core/helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,17 +35,17 @@ def fill_block(chain, from_, key, gas, data):
recipient = decode_hex('0xa94f5374fce5edbc8e2a8697c15331677e6ebf0c')
amount = 100

assert chain.get_vm().state.gas_used == 0
vm = chain.get_vm()
assert vm.state.gas_used == 0

while True:
vm = chain.get_vm()
tx = new_transaction(vm, from_, recipient, amount, key, gas=gas, data=data)
try:
chain.apply_transaction(tx)
vm.apply_transaction(tx)
except ValidationError as exc:
if "Transaction exceeds gas limit" == str(exc):
break
else:
raise exc

assert chain.get_vm().state.gas_used > 0
assert vm.state.gas_used > 0
4 changes: 2 additions & 2 deletions tests/core/vm/test_vm_state.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ def test_apply_transaction( # noqa: F811
chaindb=chaindb1,
execution_context=execution_context,
state_root=block1.header.state_root,
receipts=[],
gas_used=0,
)
parent_hash = copy.deepcopy(prev_hashes[0])

Expand All @@ -124,7 +124,7 @@ def test_apply_transaction( # noqa: F811
chaindb=chaindb1,
execution_context=execution_context,
state_root=block.header.state_root,
receipts=computation.state.receipts,
gas_used=computation.state.gas_used,
)
computation, block, _ = state1.apply_transaction(
tx2,
Expand Down