EIP-1283 #1410

cburgdorf · 2018-10-18T21:40:38Z

What was wrong?

Need to implement EIP-1283 #1105

How was it fixed?

Overwritten SSTORE opcode in constantinople
Implemented it by following the spec as closely as possible.
changed GasMeter to allow passing a RefundStrategy to allow negative refunds (will balance out within transaction. That said, I wonder if we should introduce a new additional check that raises if it does not balance out on the tx level
added tests

Cute Animal Picture

cburgdorf · 2018-10-19T10:43:16Z

Note to self (from spec):

If an implementation uses “execution-frame level” refund counter (a new refund counter is created at each call frame, and then merged back to parent when the call frame finishes), then the refund counter needs to be changed to signed – at internal calls, a child refund can go below zero.

~~(This, I believe, why some test cases are not working yet)~~ Fixed

pipermerriam · 2018-10-19T15:57:27Z

eth/vm/logic/storage.py

+
+    gas_refund = 0
+
+    if current_value == value:


This whole block looks like an excellent candidate for a stand-alone utility function which can be tested in isolation.

Also, if we convert all of the inputs to booleans:

def get_gas_cost_and_refund(original_is_zero, current_is_zero, value_is_zero, original_equals_current, original_equals_value): ...

We end up with a function that has 2**5 -> 32 possible input combinations for which we should be able to explicitely specify what the expected costs are.

Unfortunately though, following the spec causes negative refunds for some configurations for us. I believe the reason for this is:

If an implementation uses “execution-frame level” refund counter (a new refund counter is created at each call frame, and then merged back to parent when the call frame finishes), then the refund counter needs to be changed to signed – at internal calls, a child refund can go below zero.

This seems to apply to us. So, if you refund counter would be a single instance per transaction, then we would not get negative refunds, but since our refund counter is per Computation/Message, we get negatives.

That doesn't contradict what you wrote, we could just assert getting negative results for some of the combinations. I still have to taker a closer look to see what would be the best way forward.

I think that implies that our GasMeter may need to be changed since it doesn't allow for negative refunds. And likely we just need to create a new ConstantinopleGasMeter since the logic isn't needed for pre-constantinople forks.

I'm trying out something more compositional right now but I'm not sure if I like it because I think it doesn't play so well with the rest of our architecture (e.g. we do not use a DI framework which takes the boilerplate out of composition). So I may fall back to what you suggest but I want to give it a chance at least.

cburgdorf · 2018-10-19T20:56:16Z

eth/vm/forks/constantinople/storage.py

+)
+
+
+def sstore_eip1283(computation):


I moved this out into constantinople/storage.py because I strongly feel cluttering our main code base with fork specific logic is wrong. We currently have a bit of a mix (with the majority of fork specific logic being nicely isolated in vm/forks/*) Kinda related to our discussion we had in #1146

I'm completely onboard with this. I've got a code reorganization building up in my head to move all of the EVM stuff under eth.evm the same way the beacon chain stuff is under eth.beacon, which would also meaning moving a lot of stuff thats in eth.validation, eth.constants, eth.rlp etc around a bit to match.

cburgdorf · 2018-10-19T21:00:07Z

eth/vm/gas_meter.py

-    def __init__(self, start_gas: int) -> None:
+    def __init__(self,
+                 start_gas: int,
+                 refund_strategy: RefundStrategy = default_refund_strategy) -> None:


@pipermerriam @carver I wanted to avoid ConstantinopleGasMeter and reached for a more compositional approach that passes in a RefundStrategy. I don't have strong feelings about it. If you rather like this to follow the path that the rest of the code base uses (introducing ConstantinopleGasMeter and overwriting refund_gas) that is cool with me as well. In general, I'm more a friend of the compositional approach, it's just that this change is so tiny that I think there isn't much harm from an inheritance approach either.

Yeah, seems promising, and a reasonable place to experiment with the approach because it's so simple. 👍

carver

Some thoughts on the composition approach.

carver · 2018-10-19T21:24:57Z

eth/vm/gas_meter.py

+        meter.gas_refunded - amount,
+        amount,
+        meter.gas_refunded,
+    )


Looks like these logs can stay in the GasMeter

I was wondering about that and then said to myself: If it's really just this single line, why not duplicate it to allow entirely swapping out the implementation including the generated log.

carver · 2018-10-19T21:26:52Z

eth/vm/gas_meter.py

-    def __init__(self, start_gas: int) -> None:
+    def __init__(self,
+                 start_gas: int,
+                 refund_strategy: RefundStrategy = default_refund_strategy) -> None:


Yeah, seems promising, and a reasonable place to experiment with the approach because it's so simple. 👍

carver · 2018-10-19T21:28:54Z

eth/vm/gas_meter.py

+
+
+def allow_negative_refund_strategy(meter: "GasMeter", amount: int) -> None:
+    meter.gas_refunded += amount


So it raises a flag for me when there is a mutual dependency (GasMeter on the refund strategy and refund strategy on GasMeter).

One alternative:

def allow_negative_refund_strategy(previous_refund_total: int, amount: int) -> None: return previous_refund_total + amount

Which makes the strategy both stateless and side-effect-free.

I also thought about that but it is limiting the usefulness heavily. E.g. what if you want to implement a strategy that throws if e.g. gas_refunded or gas_remaining is above some treshold (just making things up). Sure, we could pass these as well but at this point we may just pass the entire meter and let the function inspect whatever it wants?

Ok, I'm not married to that particular alternative, but the mutual dependency seems fairly undesirable. If there are no other options come to mind, then we can just drop the compositional approach.

Well ok, maybe designing this in the most flexible way shouldn't be our main priority for now. We can design it in the stateless way you suggest as it is flexible enough to fulfill our current needs and if we ever come across the need for an alternative refund strategy that this system can't handle we can reconsider.

carver · 2018-10-19T21:31:39Z

eth/vm/gas_meter.py

-            amount,
-            self.gas_refunded,
-        )
+        return self.refund_strategy(self, amount)


Then this would be something like:

Suggested change

return self.refund_strategy(self, amount)

self.gas_refunded = refund_strategy(self.gas_refunded, amount)

self.logger.trace(

'GAS REFUND: %s + %s -> %s',

self.gas_refunded - amount,

amount,

self.gas_refunded,

)

return self.gas_refunded

pipermerriam · 2018-10-22T16:25:13Z

eth/vm/forks/constantinople/constants.py

@@ -2,5 +2,11 @@


 GAS_EXTCODEHASH_EIP1052 = 400
+GAS_SSTORE_NOOP_EIP1283 = 200


It'd be ideal if the EIP1283 namespace was located before the parts that are different.

GAS_SSTORE_EIP1283_INIT = ... GAS_SSTORE_EIP1283_CLEAN = ...

Makes the namespace more effective imho.

carver · 2018-10-22T18:23:44Z

eth/db/account.py

-    def _get_account(self, address):
-        rlp_account = self._journaltrie.get(address, b'')
+    def _get_account(self, address, from_journal=True):
+        rlp_account = (self._journaltrie if from_journal else self._trie).get(address, b'')


I think self._trie_cache is preferable to self._trie here, for performance.

Also, this approach might stop working once Byzantium+ VMs are optimized to only modify the trie after every block (instead of every transaction). But this is clean for now, so we can save the problem for later.

Good catch!

carver · 2018-10-22T18:36:01Z

eth/vm/forks/constantinople/storage.py

+            if original_value == 0:
+                gas_cost = constants.GAS_SSTORE_INIT_EIP1283
+            else:
+                gas_cost = constants.GAS_SSTORE_CLEAN_EIP1283


The name clean wasn't immediately meaningful to me. (It's also tough, because I wasn't sure if it was a verb or an adjective, and if it was an adjective, what noun is it modifying). I had to cross check the EIP a few times. I guess this means you are storing into a not-dirty slot.

Some options to replace ~~CLEAN~~:

FIRST_CHANGE

MAKE_DIRTY

TO_CLEAN_SLOT

I looked into geth and ethereum-js and I think one more and they all used these names, so I just copied them from there. My preference would be to stick to that name just for the sake of sharing the vocabulary with them.

Fair enough, convention vs clarity is a tightrope walk. I feel ambivalent about this one. If it were my PR, I'd probably pick the name that I thought was clearest and add a comment in the constant file to reference the conventional name. But I don't feel strongly enough to try to convince you to change it. :D

cburgdorf force-pushed the christoph/feat/eip-1283 branch 2 times, most recently from 69b84f1 to 20e3d44 Compare October 19, 2018 10:41

pipermerriam reviewed Oct 19, 2018

View reviewed changes

cburgdorf force-pushed the christoph/feat/eip-1283 branch from 20e3d44 to 37ed35e Compare October 19, 2018 20:51

cburgdorf commented Oct 19, 2018

View reviewed changes

carver reviewed Oct 19, 2018

View reviewed changes

cburgdorf force-pushed the christoph/feat/eip-1283 branch from 37ed35e to e555f2a Compare October 20, 2018 10:23

cburgdorf requested a review from carver October 20, 2018 10:32

pipermerriam approved these changes Oct 22, 2018

View reviewed changes

carver approved these changes Oct 22, 2018

View reviewed changes

Implement EIP-1283

dec4137

cburgdorf force-pushed the christoph/feat/eip-1283 branch from e555f2a to dec4137 Compare October 22, 2018 23:01

cburgdorf merged commit 90423f1 into ethereum:master Oct 22, 2018

cburgdorf mentioned this pull request Oct 22, 2018

Constantinople Progress Tracker ethereum/pm#53

Closed

5chdn mentioned this pull request Nov 14, 2018

EIP1283 - Net gas metering #1105

Closed

lithp mentioned this pull request Dec 10, 2018

The Progress Tracker should say trinity has not implemented Net Gas Metering ethereum/pm#68

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EIP-1283 #1410

EIP-1283 #1410

cburgdorf commented Oct 18, 2018 •

edited

Loading

cburgdorf commented Oct 19, 2018 •

edited

Loading

pipermerriam Oct 19, 2018

cburgdorf Oct 19, 2018

pipermerriam Oct 19, 2018

cburgdorf Oct 19, 2018

cburgdorf Oct 19, 2018

pipermerriam Oct 19, 2018

cburgdorf Oct 19, 2018

carver Oct 19, 2018

carver left a comment

carver Oct 19, 2018

cburgdorf Oct 19, 2018

carver Oct 19, 2018

carver Oct 19, 2018

cburgdorf Oct 19, 2018

carver Oct 19, 2018

cburgdorf Oct 20, 2018

carver Oct 19, 2018

pipermerriam Oct 22, 2018

carver Oct 22, 2018

cburgdorf Oct 22, 2018

carver Oct 22, 2018

cburgdorf Oct 22, 2018

carver Oct 23, 2018 •

edited

Loading



		def allow_negative_refund_strategy(meter: "GasMeter", amount: int) -> None:
		meter.gas_refunded += amount

-        return self.refund_strategy(self, amount)
+        self.gas_refunded = refund_strategy(self.gas_refunded, amount)
+        self.logger.trace(
+            'GAS REFUND: %s + %s -> %s',
+            self.gas_refunded - amount,
+            amount,
+            self.gas_refunded,
+        )
+        return self.gas_refunded

		@@ -2,5 +2,11 @@


		GAS_EXTCODEHASH_EIP1052 = 400
		GAS_SSTORE_NOOP_EIP1283 = 200

EIP-1283 #1410

EIP-1283 #1410

Conversation

cburgdorf commented Oct 18, 2018 • edited Loading

What was wrong?

How was it fixed?

Cute Animal Picture

cburgdorf commented Oct 19, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carver left a 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

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

carver Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

cburgdorf commented Oct 18, 2018 •

edited

Loading

cburgdorf commented Oct 19, 2018 •

edited

Loading

carver Oct 23, 2018 •

edited

Loading