Delete gas used from state

[carver]

What was wrong?

Related to #599 & #507 - there was state mutation inside VM.apply_transaction(), to update gas_used, and get_block_by_header lived in VM (we're generally looking to move block manipulation out of VM, into Chain).

How was it fixed?

There wasn't really any need for gas_used to be in the state, we can verify the transaction against the header in the VM, before even calling state.apply_transaction.

• add a new transaction verification, explicitly against the header
• make_receipt new uses the header to generate the cumulative gas used
• apply_transaction now builds on top of a passed-in header (rather than using a local mutation of self.block.header)
• get_block_by_header really needed to belong in Chain instead of VM.
• use only account_db instead of the full state to validate transaction (after separately running the transaction->header validation)

Yay for more use of (immutable) rlp object, and less state mutation.

Cute Animal Picture

[carver]

Just some comments of what I expect to follow up on in the morning. I'm checking out for the night.

[carver]

I'm wrapping for the day, but on another review, this should probably be status_code instead of success_code

[carver]

It was nice to pull this chunk of logic out, but it was awkward to both:

• chain the result of one application into the next, and
• accumulate the results

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.

[pipermerriam]

Hammered out this accumulation based zero mutation way to do this.

import functools
from cytoolz import accumulate

transactions = ('a', 'b', 'c', 'd')


def apply_transaction(header, txn):
    return header + 1, txn * 3

def test_it():
    txn_fns = [
        functools.partial(apply_transaction, txn=transaction)
        for transaction in transactions
    ]
    return accumulate(lambda prev, txn_fn: txn_fn(prev[0]), txn_fns, (0, None))


if __name__ == "__main__":
    print(tuple(test_it()))

# ((0, None), (1, 'aaa'), (2, 'bbb'), (3, 'ccc'), (4, 'ddd'))

Requires using the toolz.accumulate because it allows you to supply an initial value. Result. Probably want to do away with the lambda for readability.

[carver]

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.

[pipermerriam]

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 VM class. We could still use the logic above to encapulate the act of applying all of the transactions and getting back the resulting header and list of receipts but still keep it encapsulated in a utility function to keep the import_block method easier to read.

Dunno, your call. Just wanted to see it all written out mostly.

[carver]

Oops, I'll swap the order of these params in the docs. I went back and forth a few times locally. In the end a semi-chronological order made sense to me: the header is in the state before the transaction is applied, so comes first.

[pipermerriam]

That's roughly the order I settled on as well. 👍

Rather than chronological I think hierarchical this is the natural order with header being higher in the API than transaction.

[carver]

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.

[carver]

This was the inspiration for most of the PR.

[pipermerriam]

That's roughly the order I settled on as well. 👍

Rather than chronological I think hierarchical this is the natural order with header being higher in the API than transaction.

[pipermerriam]

Hammered out this accumulation based zero mutation way to do this.

import functools
from cytoolz import accumulate

transactions = ('a', 'b', 'c', 'd')


def apply_transaction(header, txn):
    return header + 1, txn * 3

def test_it():
    txn_fns = [
        functools.partial(apply_transaction, txn=transaction)
        for transaction in transactions
    ]
    return accumulate(lambda prev, txn_fn: txn_fn(prev[0]), txn_fns, (0, None))


if __name__ == "__main__":
    print(tuple(test_it()))

# ((0, None), (1, 'aaa'), (2, 'bbb'), (3, 'ccc'), (4, 'ddd'))

Requires using the toolz.accumulate because it allows you to supply an initial value. Result. Probably want to do away with the lambda for readability.

[cburgdorf]

Great to see you're taking the chance to directly improve the documentation ❤️

[cburgdorf]

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.

[carver]

⚖️ 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.