Add PAYGAS opcode #285

NIC619 · 2018-01-18T09:31:30Z

What was wrong?

Implementation of #234

How was it fixed?

Add `PAYGAS` opcode

Carry additional informations such as transaction_gas_limit, paygas_gasprice and paygas_total_fee in ShardingMessage object
- transaction_gas_limit is the same as transaction.gas
- paygas_gasprice is set by PAYGAS opcode and only under two conditions:
  1. in a top level call(i.e., computation.msg.depth == 0)
  2. the value has not been set before during this transaction
- If the two conditions are not met, current action is to do nothing.
- paygas_total_fee is equal to transaction_gas_limit * paygas_gasprice
Once triggered, deduct paygas_total_fee from transaction initiator(transaction.to)'s balance
Refund fee is calculated base on paygas_gasprice instead of transaction.gas_price
Opcode value is currently set as ~~0x5c~~ 0xf5, placed in ~~Stack, Memory, Storage and Flow Operations~~ Systems section. I preferred to put it in Environment Information section but it's full already.

1. Remove coinbase from execute_transaction and ~~transfer the fee directly to PAYGAS_TEMPORARY_ACCOUNT of which the account data is enforced to be permanently stored in database hence it's witness data is not required.~~ store the tx fee amount due in block.tx_fee_sum and transfer all fees to coinbase in finalize_block function
~~2. Add a function that generates a transaction to transfer fees collected from all transactions of this block to coinbase.~~
1. Abstract gas_price away, removing it from transaction format.(Moved to another PR)
1. Add tests.(More to be added in another PR)

Cute Animal Picture

NIC619 · 2018-01-18T14:14:38Z

@vbuterin does the PAYGAS_TEMPORARY_ACCOUNT scheme in WIP item.1 and 2. makes sense?

pipermerriam · 2018-01-18T19:01:23Z

evm/logic/context.py

+
+
+def paygas(computation):
+    gasprice = computation.stack.pop(type_hint=constants.UINT256)


naming nitpick

gas_price for conformity.

pipermerriam · 2018-01-18T19:02:19Z

evm/logic/context.py

+
+    # Only valid if (1) triggered in a top level call and
+    # (2) not been set already during this transaction execution
+    if computation.msg.depth == 0 and computation.msg.paygas_gasprice == 0:


computation.msg.paygas_gasprice == 0 seems problematic because it disallows a gas price of zero which at least historically, has been allowed. Maybe use None as the not-set value?

pipermerriam · 2018-01-18T19:04:11Z

evm/vm/message.py

+            return 0
+
+    @paygas_gasprice.setter
+    def paygas_gasprice(self, gas_price):


probably good to add validation for this value here to be really sure that it's valid.

pipermerriam · 2018-01-18T19:05:14Z

evm/vm/message.py

+
+    @paygas_gasprice.setter
+    def paygas_gasprice(self, gas_price):
+        if self.paygas_gasprice == 0:


It seems like there should be a logic branch here which throws an exception (not a VMException but something that will bubble) if the value is already set, because if I understand correctly, we shouldn't even try to set the value if it's already set.

What about returning True of False in paygas_gasprice.setter to indicate if the value is successfully set or not.

if self.paygas_gasprice is None: self._paygas_gasprice = gas_price return True else: return False

and in PAYGAS opcode check against this returned value and push 1 or 0 to the stack to indicate the outcome of this PAYGAS operation?

def paygas(computation): ... if computation.msg.depth == 0 and computation.msg.paygas_gasprice(gas_price): … computation.stack.push(1) else: computation.stack.push(0)

It's like fail of value transfer in a call. It seems better to me that it does not result in failure of transaction but I'm not that confident about this. Could it be the situation that gas were paid in top level call but later during some calls to other contract, the contract trigger PAYGAS again?

I like making subsequent PAYGAS calls ineffective (ie. as is implemented now) and pushing 1 on success and 0 on failure.

So my suggestion was not focused on VM semantics, but rather our python API semantics. Here's the situation I'm trying to avoid.

# somewhere we set the gas price. msg.paygas_gasprice = 10 # elsewhere in the code, we try to set it again. msg.paygas_gasprice = 100 # this will silently fail

What I would like is that the later case cannot silently fail, hence, suggesting that we raise an exception. Then, if we ever have a location in the code which tries to set the gas price when it's already set which doesn't explicitely handle whatever exception we raise, then we'll get a hard failure in our code, rather than a silent failure that only manifests later at some point when the gas accounting is off.

It seems like there should be a logic branch here which throws an exception (not a VMException but something that will bubble) if the value is already set

@pipermerriam Sorry I don't quite get it. Can you elaborate more on the difference between VMException and something that will bubble?

Should this result in reverting of transaction? One possible scenario is that gas is first paid by the forwarding contract but later during child/grandchild call to other contracts PAYGAS is triggered(accidentally or not) again. IMO it seems like a bad user experience and a problem for debugging if this is to result in reverting the transaction.

@NIC619

Look here: https://github.com/ethereum/py-evm/blob/master/evm/computation.py#L316-L338

Anything inside of vm execution which throws any exception that is a subclass of VMError gets handled there, resulting in computation.is_error being True.

What I'm proposing is something that throws something like a TypeError or another custom exception that is not a VMError subclass. This way, if we write code which sets msg.paygas_gasprice without implementing error handling and the msg.paygas_gasprice has already been set... an error will be thrown in such a way that it won't be caught in the linked code above.

Does that make sense?

NIC619 · 2018-01-23T17:04:20Z

@pipermerriam Made some updates.

Now the exception PAYGASResetGasprice will be raised if trying to reset the already set msg.paygas_gasprice. And in PAYGAS opcode it would use try/except to check if msg.paygas_gasprice has been set already. Push 1 to the stack if not and 0 otherwise.

pipermerriam · 2018-01-24T19:29:02Z

evm/exceptions.py

@@ -150,3 +150,7 @@ class UnannouncedStateAccess(VMError):
    been specified in the transaction's read or write list, respectively.
    """
    pass
+
+
+class PAYGASResetGasprice(Exception):


other name suggestions:

GasPriceAlreadySet

GasPriceOverwriteProtection

I'm thinking the inclusing of PAYGAS in the name isn't needed and is a little visually confusing.

pipermerriam · 2018-01-24T19:30:10Z

evm/logic/context.py

+
+    # Only valid if (1) triggered in a top level call and
+    # (2) not been set already during this transaction execution
+    if computation.msg.depth == 0:


The way this is written without an else clause this does nothing if triggered from depth > 0. Is this intended? Should it push 0 onto the stack if depth > 0?

You are right. It should push 0 onto the stack if depth > 0.

pipermerriam · 2018-01-24T19:33:24Z

evm/logic/context.py

+    # (2) not been set already during this transaction execution
+    if computation.msg.depth == 0:
+        try:
+            computation.msg.paygas_gasprice(gas_price)


This looks like an error. You are calling a property which I believe should raise an exception stating that either NoneType or the integers are not callable.

I do however like using a setter function rather than assigning to a property that throws an exception. Maybe rename to something like computation.msg.set_PAYGAS_gasprice(...)?

My mistake here. I will update the setter to set_PAYGAS_gasprice function.

Do you think it's worth implementing a few minimal test cases for this opcode to hit these code paths? I know the JSON test suite will cover these eventually, but

we'd probably benefit from having basic test cases for opcodes that were not buried in the hard to decipher JSON fixture tests.

until we have JSON fixture tests for this it'd be nice to know it works (from a testing/CI perspective)

those test cases can be use to fill out whatever EIP is written to specify this opcode.

pipermerriam · 2018-01-24T19:34:25Z

evm/vm/message.py

+            return self._paygas_gasprice
+
+    @paygas_gasprice.setter
+    def paygas_gasprice(self, gas_price):


Continuation of previous suggestion to remove the setter and just make this a method set_PAYGAS_gasprice. Seems a bit more intuitive/correct/something.

pipermerriam · 2018-01-24T19:35:05Z

evm/vm/message.py

+    @property
+    def paygas_gasprice(self):
+        if self._paygas_gasprice is None:
+            return 0


I'm still curious about returning 0 instead of None here. It seems valuable to be able to distinguish between not-set and set-to-0 which cannot be done in the current implementation.

My rationale was that from inside(Message object) the two cases are distinguished by msg._paygas_gasprice and from outside one indeed can not tell the difference. But since when msg.paygas_gasprice is used to compute gas charge/refund (from outside) the two cases are basically the same.

Also if msg.paygas_gasprice could return None, we need to add a condition check every time it's used to compute gas charge/refund.

gas_refund_amount = (gas_refund + gas_remaining) * computation.msg.paygas_gasprice if computation.msg.paygas_gasprice is not None else 0

So i'm on the fence about this. Your call. Here's some extra thoughts on why it might still be good to use None.

The Computation object has an existing convention of having get_XXX methods for things to account for these little bits of internal logic. I think the following API is consisten with the rest of the class and clean.

class Computation: ... def get_PAYGAS_gas_price(self): if self._paygas_gasprice is None: return 0 else: return self._paygas_gasprice # and then used elsewhere gas_refund_amount = (gas_refund + gas_remaining) * computation.get_paygas_gasprice()

It also has the nice benefit of not reaching down into the message object.

Again, your call. I'm +0 on this change if you don't think there is a valid use case for differentiating between set to 0 and unset.

…dySet exception if gas price is already set

NIC619 · 2018-01-28T09:38:57Z

Since this had to be rebased and some part of this PR is already been taken cared of in other new PR, I just re-do it along with PR feedbacks.

I changed it back so that paygas_gasprice will return None if not set yet because the other case doesn't seem to have much advantage over this one.

I moved paygas_garprice and it's getter/setter to Computation. But at this moment of writing I feel like I might have misunderstood what you mean. @pipermerriam
Do you mean (1)keep the paygas_gasprice in Message and use getter/setter in Computation to get/set this value, or (2)move paygas_gasprice and it's getter/setter together to Computation?

NIC619 · 2018-01-29T16:05:51Z

Another thing is transferring the transaction fee to coinbase. I was thinking about hooking this information on to computation object(computation.tx_fee) since it's what's returned after executing transaction. Then add the fee information to Block.tx_fee_sum and finally transfer all the fees collected to coinbase in finalize_block.

def execute_transaction(self, transaction):
    …
    transaction_fee = (transaction.gas - gas_remaining - gas_refund) * PAYGAS_gasprice
    computation.tx_fee = transaction_fee
    …

def add_transaction(self, transaction, computation, block):
    …
    if block.tx_fee_sum:
        block.tx_fee_sum += computation.tx_fee
    else:
        block.tx_fee_sum = computation.tx_fee
    …

def finalize_block(self, block):
    …
    with self.state_db() as state_db:
            state_db.delta_balance(block.header.coinbase, block.tx_fee_sum)
            self.logger.debug(
                "TOTAL TRANSACTON FEE: %s -> %s",
                block.tx_fee_sum,
                block.header.coinbase,
            )
    …

pipermerriam · 2018-01-29T17:29:27Z

evm/vm/forks/sharding/computation.py

+    def get_PAYGAS_gas_price(self):
+        return self._paygas_gasprice
+
+    def set_PAYGAS_gasprice(self, gas_price):


I think that this is a good move (moving the value and the setters co Computation). This function could use one extra validation check to enforce depth == 0 within the setter itself.

@pipermerriam
Added the depth == 0 validation check along with PAYGAS testing contract fixture and test suite.

…n import

… them altogether in `finalize_block`

hwwhww · 2018-01-31T07:27:03Z

According to our last discussion, the GAS_PAYGAS is 3, please update it.

pipermerriam · 2018-01-31T16:55:56Z

evm/logic/system.py

@@ -228,6 +230,7 @@ def __call__(self, computation):
            value=value,
            data=b'',
            code=call_data,
+            transaction_gas_limit=computation.msg.transaction_gas_limit,


FYI, this will need to be reworked to use the new TransactionContext from #327 once it's complete and merged.

Will do. 👍

pipermerriam · 2018-01-31T16:57:38Z

evm/logic/system.py

+
+    # Only valid if (1) triggered in a top level call and
+    # (2) not been set already during this transaction execution
+    if computation.msg.depth == 0:


Now that computation.set_PAYGAS_gasprice has an internal check for the depth, what do you think about removing this outer if statement and just adding another except clause to the try/except/else block?

pipermerriam · 2018-01-31T16:58:58Z

evm/vm/forks/sharding/computation.py

+    def set_PAYGAS_gasprice(self, gas_price):
+        validate_uint256(gas_price, title="PAYGAY.gas_price")
+        if self.msg.depth != 0:
+            raise NotTopLevelCall("This should only happen in a top level call context.")


nitpick

I think the exception message would be clearer as:

"The set_PAYGAS_gasprice API is only valid when called from the top level computation"

Makes it a little clearer what "this" is.

pipermerriam · 2018-01-31T17:02:55Z

evm/vm/forks/sharding/vm_state.py

        )
-        with state_db_cm() as state_db:
-            state_db.delta_balance(self.coinbase, transaction_fee)
+        computation.tx_fee = transaction_fee


Rather than storing this value on the computation object what do you think about just re-calculating it within add_transaction? It looks like we have all of the necessary information already available in the context of that function to do the computations.

You are right. Totally makes sense.

pipermerriam · 2018-01-31T17:05:27Z

tests/core/helpers.py

@@ -32,7 +32,8 @@ def new_sharding_transaction(

    [destination, value, msg_data, vrs].
    """
-    tx_data = pad32(data_destination) + pad32(bytes([data_value])) + pad32(data_msgdata) + pad32(data_vrs)  # noqa: E501
+    assert len(data_vrs) in (0, 65)
+    tx_data = pad32(data_destination) + pad32(bytes([data_value])) + pad32(data_msgdata) + pad32(data_vrs[:32]) + pad32(data_vrs[32:64]) + bytes(data_vrs[64:])  # noqa: E501


suggest reformatting to

tx_data = ( pad32(data_destination) + pad32(bytes([data_value])) + pad32(data_msgdata) + pad32(data_vrs[:32]) + pad32(data_vrs[32:64]) + bytes(data_vrs[64:] )

…rove its exception message

… computation object

pipermerriam · 2018-02-01T17:11:53Z

evm/logic/call.py

+                    child_computation.output[:actual_output_size],
+                )
+
+            if not child_computation.should_burn_gas:


Note to self.

this is a confusing if statement because of the need for the not. We might make a second property which returns the inverse of this as computation.should_return_gas.

Noted. Will make a PR to update this.

pipermerriam · 2018-02-01T17:22:27Z

evm/vm/forks/sharding/vm_state.py

+        gas_remaining = computation.get_gas_remaining()
+        gas_refunded = computation.get_gas_refund()
+        gas_used = transaction.gas - gas_remaining
+        gas_refund = min(gas_refunded, gas_used // 2)


Hrm, rather than repeating this block of code both here and above, I think we should dig it out into a utility of some sort.

def compute_transaction_gas_amounts(computation, transaction_gas, gas_price): gas_remaining = computation.get_gas_remaining() gas_refunded = computation.get_gas_refund() gas_used = transaction_gas - gas_remaining gas_refund = min(gas_refunded, gas_used // 2) tx_fee = (transaction_gas - gas_remaining - gas_refund) * gas_price return tx_fee, gas_used, gas_refund

Something like this. Could also be implemented as a method on the Computation class which would allow removing the gas_price argument from the call signature.

pipermerriam · 2018-02-01T17:26:01Z

evm/vm/forks/sharding/vm_state.py

+        tx_fee = (transaction.gas - gas_remaining - gas_refund) * tx_PAYGAS_gasprice
+        # Bookkeep this transaction fee
+        if hasattr(block, "tx_fee_sum"):
+            block.tx_fee_sum += tx_fee


I'd like to figure out a way to do this which doesn't involve storing data on the block object. Looking around at the code however, it's not very clear where that would be.

For now, lets just formalize this property on the block object so that we don't have to do hasattr checks.

…as charge fix in ethereum#319

…efund`

NIC619 added PR state: WIP eth2.0 labels Jan 18, 2018

pipermerriam reviewed Jan 18, 2018

View reviewed changes

hwwhww mentioned this pull request Jan 20, 2018

Standard Account Contract #225

Closed

2 tasks

NIC619 force-pushed the add_payags branch 2 times, most recently from f8c04b9 to c0f5276 Compare January 23, 2018 14:30

pipermerriam requested changes Jan 24, 2018

View reviewed changes

hwwhww force-pushed the sharding branch from c0c1008 to 7a1db70 Compare January 25, 2018 08:23

NIC619 force-pushed the add_payags branch 2 times, most recently from c2be9cb to b47cfce Compare January 26, 2018 01:59

NIC619 added 3 commits January 28, 2018 16:39

Add transaction gas limit to ShardingMessage

d6fda0e

Add PAYGAS gas price to ShardingComputation and trigger GasPriceAlrea…

deddbea

…dySet exception if gas price is already set

Implement PAYGAS opcode

d257d23

NIC619 force-pushed the add_payags branch from b47cfce to d257d23 Compare January 28, 2018 09:06

pipermerriam reviewed Jan 29, 2018

View reviewed changes

pipermerriam approved these changes Jan 29, 2018

View reviewed changes

NIC619 added 8 commits January 30, 2018 19:33

Add top level call exception check in set_PAYGAS_gas_price

44b37ba

Patch: missinig transaction_gas_limit and missing exception/validatio…

7461278

…n import

fix: wrong function name

b74d4bc

Bookkeep transaction fee in computation and block object and transfer…

38edda6

… them altogether in `finalize_block`

Add PAYGAS testing contract fixture

7cdd673

Fix: passed in signature should be empty or 65 bytes long

44a2e8d

Add basic PAYGAS test which checks if PAYGAS is successfully triggered

441a23f

Remove dynamic coinbase xfail case

2c1667c

NIC619 added Needs Review and removed PR state: WIP labels Jan 31, 2018

NIC619 added 3 commits January 31, 2018 15:51

Fix: override __call__ in CallSharding

107ae45

Update PAYGAS opcode gas cost

db00e9a

Patch: remove dynamic coinbase xfail case in auxiliary test suite

2ab3f8a

ethereum deleted a comment from NIC619 Jan 31, 2018

pipermerriam approved these changes Jan 31, 2018

View reviewed changes

NIC619 added 2 commits February 1, 2018 14:52

Change NotTopLevelCall to normal exception instead of VMError and imp…

e49a822

…rove its exception message

Remove top level call check in PAYGAS

0e9f51a

NIC619 mentioned this pull request Feb 1, 2018

Contract creation transactions don't specify a salt #326

Closed

NIC619 added 2 commits February 1, 2018 16:01

Calculate transaction fee in add_transaction instead of storing it in…

e8a3a88

… computation object

Re-formatting tx_data in new_sharding_transaction helper function

787c04f

NIC619 force-pushed the add_payags branch from 5bd235f to 787c04f Compare February 1, 2018 08:04

flake8

f2c9b11

pipermerriam reviewed Feb 1, 2018

View reviewed changes

pipermerriam approved these changes Feb 1, 2018

View reviewed changes

NIC619 added 3 commits February 2, 2018 08:23

Fix: update transaction gas provided in auxiliary test suite due to g…

02eaa8e

…as charge fix in ethereum#319

Replace repeating gas calculation with `compute_transaction_fee_and_r…

60b2466

…efund`

Make transaction_fee_sum a property of Block

134b47c

NIC619 merged commit 808ddc4 into ethereum:sharding Feb 2, 2018

NIC619 mentioned this pull request Feb 24, 2018

Add PAYGAS opcode #234

Closed



		def paygas(computation):
		gasprice = computation.stack.pop(type_hint=constants.UINT256)

Add PAYGAS opcode #285

Add PAYGAS opcode #285

Conversation

NIC619 commented Jan 18, 2018 • edited Loading

What was wrong?

How was it fixed?

Add PAYGAS opcode

Cute Animal Picture

NIC619 commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbuterin Jan 21, 2018 • edited Loading

Choose a reason for hiding this comment

pipermerriam Jan 21, 2018 • edited Loading

Choose a reason for hiding this comment

NIC619 Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NIC619 commented Jan 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pipermerriam Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NIC619 commented Jan 28, 2018

NIC619 commented Jan 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwwhww commented Jan 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NIC619 commented Jan 18, 2018 •

edited

Loading

Add `PAYGAS` opcode

vbuterin Jan 21, 2018 •

edited

Loading

pipermerriam Jan 21, 2018 •

edited

Loading

NIC619 Jan 22, 2018 •

edited

Loading

pipermerriam Jan 26, 2018 •

edited

Loading