witness: only include elements if there's enough gas for it #495

jsign · 2024-09-12T21:46:54Z

This PR does changes to satisfy a new requirement when adding elements to the execution witness.

The new requirement involves only adding elements to the witness if there's sufficient gas for it.

Examples:

Imagine a JUMP that jumps to a new code chunk, charging 200 gas to be included in the witness. Before this PR, TouchCodeChunksRangeAndChargeGas(...) would add the code chunk to the witness and return the cost to be charged. If we had, say, 100 gas available, this would mean that the user didn't have enough funds to pay for that (i.e: 100<200). But it's too late, and it was already added to the witness.
Imagine an instruction writes a new balance in a non-existen address, thus this address should be created. This involves writing BASIC_DATA and CODEHASH of this address. Before this PR, we always added BASIC_DATA and CODEHASH and returned the cost. If the user only had gas to pay for BASIC_DATA but not CODEHASH, it would be too late since we have already added both.

More generally, if you have X of available gas and include something in the witness cost Y, with Y>X, then we must not include it in the witness. This is made more apparent when we try to include a wide range of code chunks and only have gas for one but are asked to include 15. Before this PR, we were including 15, not only one.

From a stateless client perspective, being this strict is fine. Knowing if you have enough gas to include something in the witness doesn't depend on what you received from the witness (i.e: this PR including less things in the witness doesn't affect it). This is because everything you need to know to be sure that you can add something new to the witness is the access witness state of which branches and code chunks were added.

Signed-off-by: Ignacio Hagopian <[email protected]>

jsign · 2024-09-19T19:20:24Z

cmd/evm/internal/t8ntool/execution.go

-		statedb.Witness().TouchFullAccount(w.Address[:], true)
+		statedb.Witness().TouchFullAccount(w.Address[:], true, nil)


For any Touch*(...) call that doesn't care about charging gas, we send the nil UseGas(...) function.

This is the equivalent action of the previous API where we ignored the returned value (since we didn't charge that gas to anyone).

jsign · 2024-09-19T19:26:15Z

core/state/access_witness.go

@@ -30,6 +29,9 @@ import (
 // * the second bit is set if the branch has been read
 type mode byte

+// UseGasFn is a function that can be used to charge gas for a given amount.
+type UseGasFn func(uint64) bool


Now Touch*(....) functions receive a function that we'll use as a callback to charge the required gas.

Many Touch*(...) do more than one charging. This means that this callback could be called multiple times.

Before I tried another idea which was not passing this callback, but asking for availableGas. The problem with that is that it adds a lot of strain in callers to solve partial charging. The Touch*(...) calls can charge for more than one thing. In the case the availableGas was sufficient for some of them but not all, that means the API should return:

gasThatMustBeCharged

and ok bool to signal if the availableGas was sufficient for everything or not.

The caller should:

Always charge gasThatMustBeCharged, respectively of what ok is. Since the passed availableGas was at least sufficient to put some stuff in the witness.

if ok is false, fail due to out of gas.

The above logic should be done in all the callers.

This proposed API where we pass a callback function, avoids all this logic in all callers. The API call will already charge for whatever was allowed, and simply return false signaling that the available gas was insufficient and let the caller fail (i.e: the caller shouldn't deal with charging stuff since that was already done in the API call).

That's the "TLDR" of this approach. I know was mentioned this still sounds like can get some pushback from the geth team, so we can try again with the other approach... but I found it a bit annoying.

jsign · 2024-09-19T19:28:34Z

core/state/access_witness.go

 }

-// touchAddress adds any missing access event to the witness.
-func (aw *AccessWitness) touchAddress(addr []byte, treeIndex uint256.Int, subIndex byte, isWrite bool) (bool, bool, bool, bool, bool) {
+func (aw *AccessWitness) touchAddressAndChargeGas(addr []byte, treeIndex uint256.Int, subIndex byte, isWrite bool, useGasFn UseGasFn) (uint64, bool) {


This is the new "base" fundamental method that charges for stuff.
Basically:

we fill the boolanes if this charging required branchRead, chunkRead, etc. But we don't yet affect aw.[branches|chunks] since we want to do that ONLY if we have available gas.

(Continue to read below comments for further bullets)

jsign · 2024-09-19T19:29:03Z

core/state/access_witness.go

+	var gas uint64
+	if branchRead {
+		gas += params.WitnessBranchReadCost
+	}
+	if chunkRead {
+		gas += params.WitnessChunkReadCost
+	}
+	if branchWrite {
+		gas += params.WitnessBranchWriteCost
+	}
+	if chunkWrite {
+		gas += params.WitnessChunkWriteCost
+	}
+	if chunkFill {
+		gas += params.WitnessChunkFillCost
+	}


With the booleans, we calcualte the gas as usual.

jsign · 2024-09-19T19:29:40Z

core/state/access_witness.go

+	if useGasFn != nil {
+		if ok := useGasFn(gas); !ok {
+			return 0, false
+		}
+	}


Ask for the callback to charge this gas, and signal us if it was possible or not.

If wasn't possible, return early. Notice that we haven't technically added anything to the witness. This is important.

jsign · 2024-09-19T19:41:40Z

core/vm/instructions.go

+		chargedGas, ok := interpreter.evm.Accesses.TouchBasicData(address[:], false, scope.Contract.UseGas)
+		if !ok || (chargedGas == 0 && !scope.Contract.UseGas(params.WarmStorageReadCostEIP2929)) {


You could be wondering why some of the Touch*(...) APIs returned the charged gas if we were sending the callback?

This is the reason. We have this rule that if on many charges the actual charged gas was 0, then we charge warm. We ned a way to know if that UseGas was asked to charge 0 or not.

If was 0, we do a usual scope.Contract.UseGas(Warm).

hmmm yeah but we could simplify this by adding a chargeWarm flag to the function, so that we don't need to clutter the code too much here. If you still have to call UseGas after calling the function you pass it to, this makes the code that much harder to read.

jsign · 2024-09-19T19:42:30Z

core/vm/instructions.go

+func chargeCallVariantEIP4762(evm *EVM, scope *ScopeContext) bool {
+	target := common.Address(scope.Stack.Back(1).Bytes20())
+	if _, isPrecompile := evm.precompile(target); isPrecompile {
+		return scope.Contract.UseGas(params.WarmStorageReadCostEIP2929)
+	}
+	// The charging for the value transfer is done BEFORE subtracting
+	// the 1/64th gas, as this is considered part of the CALL instruction.
+	// (so before we get to this point)
+	// But the message call is part of the subcall, for which only 63/64th
+	// of the gas should be available.
+	chargedGas, ok := evm.Accesses.TouchAndChargeMessageCall(target.Bytes(), scope.Contract.UseGas)
+	if !ok || (chargedGas == 0 && !scope.Contract.UseGas(params.WarmStorageReadCostEIP2929)) {
+		return false
+	}
+	return true
+
+}


This is the makeCallVariantGas4762 layer that we removed and moved to op*Call(...) codes.

jsign · 2024-09-19T19:42:42Z

core/vm/instructions.go

+	if interpreter.evm.chainRules.IsEIP4762 {
+		address := common.Address(scope.Stack.Back(1).Bytes20())
+		transfersValue := !scope.Stack.Back(2).IsZero()
+
+		// If value is transferred, it is charged before 1/64th
+		// is subtracted from the available gas pool.
+		if transfersValue {
+			if !interpreter.evm.Accesses.TouchAndChargeValueTransfer(scope.Contract.Address().Bytes()[:], address.Bytes()[:], scope.Contract.UseGas) {
+				return nil, ErrExecutionReverted
+			}
+		}
+	}
+
+	var err error
+	interpreter.evm.callGasTemp, err = callGas(interpreter.evm.chainRules.IsEIP150, scope.Contract.Gas, 0, scope.Stack.Back(0))
+	if err != nil {
+		return nil, err
+	}
+	if !scope.Contract.UseGas(interpreter.evm.callGasTemp) {
+		return nil, ErrOutOfGas
+	}
+	if !chargeCallVariantEIP4762(interpreter.evm, scope) {
+		return nil, ErrExecutionReverted
+	}


For CALL we moved the 1/64 stuff here.

jsign · 2024-09-19T19:43:04Z

core/vm/instructions.go

@@ -762,6 +852,9 @@ func opCall(pc *uint64, interpreter *EVMInterpreter, scope *ScopeContext) ([]byt
 }

 func opCallCode(pc *uint64, interpreter *EVMInterpreter, scope *ScopeContext) ([]byte, error) {
+	if !chargeCallVariantEIP4762(interpreter.evm, scope) {


For the rest of *CALL, we simply call the helper so keeps being relatively simple.

jsign · 2024-09-19T19:43:49Z

core/vm/operations_verkle.go

@@ -1,160 +0,0 @@
-// Copyright 2024 The go-ethereum Authors


Nothing about this file survived, which makes sense.
The gas*(...) style of apis can't work with what we need to do, so it makes sense that everything here went away (since was mostly moved to op*(..) above).

Signed-off-by: Ignacio Hagopian <[email protected]>

gballet

just leaving a comment on how to improve the handling of warm costs, going to experiment quickly but if I can't get anything to work today, I'll merge and try to fix it later.

gballet · 2024-09-23T13:08:22Z

core/vm/instructions.go

+		chargedGas, ok := interpreter.evm.Accesses.TouchBasicData(address[:], false, scope.Contract.UseGas)
+		if !ok || (chargedGas == 0 && !scope.Contract.UseGas(params.WarmStorageReadCostEIP2929)) {


hmmm yeah but we could simplify this by adding a chargeWarm flag to the function, so that we don't need to clutter the code too much here. If you still have to call UseGas after calling the function you pass it to, this makes the code that much harder to read.

Signed-off-by: Ignacio Hagopian <[email protected]>

gballet

This is just a brain-dumping wall of text, so that I can come back to it when there is pushback by the geth team.

Doesn't LGTM, as I expect there will be substantial pushback when mainlining this, but I tried using the upper bound for myself and can confirm the difficulty of the approach. I still think there is a way to make this PR less intrusive that it already is.

It took me a bit to get to the depth of the reason for this function parameter, but I agree it will be hard to work around it. The approach that consumes all the gas and returns the consumed gas works most of the time, but it doesn't make the difference between something that took all the remaining gas (for which we should still execute the action), and something that went out of gas (for which we should not).

It is possible to share this information and let the calling code consume the gas, which is much cleaner. I am currently trying this approach, and will throw in the towel if I can't make it work in the next couple of hours.

Signed-off-by: Ignacio Hagopian <[email protected]>

* fix Signed-off-by: Ignacio Hagopian <[email protected]> * eip4762: push witness fix Signed-off-by: Ignacio Hagopian <[email protected]> --------- Signed-off-by: Ignacio Hagopian <[email protected]>

* fix Signed-off-by: Ignacio Hagopian <[email protected]> * nit Signed-off-by: Ignacio Hagopian <[email protected]> --------- Signed-off-by: Ignacio Hagopian <[email protected]>

* ci: new workflows for testing Signed-off-by: Ignacio Hagopian <[email protected]> * nit Signed-off-by: Ignacio Hagopian <[email protected]> * ci: fix stable consumption Signed-off-by: Ignacio Hagopian <[email protected]> * fix Signed-off-by: Ignacio Hagopian <[email protected]> * more fixes Signed-off-by: Ignacio Hagopian <[email protected]> * more fixes Signed-off-by: Ignacio Hagopian <[email protected]> * update tag Signed-off-by: Ignacio Hagopian <[email protected]> --------- Signed-off-by: Ignacio Hagopian <[email protected]>

Signed-off-by: Ignacio Hagopian <[email protected]>

core/vm/eips.go

Signed-off-by: Guillaume Ballet <[email protected]>

…CODEHASH (#508) * avoid extra 100 gas in value-bearing CALL Signed-off-by: Ignacio Hagopian <[email protected]> * missing codeHash write event Signed-off-by: Ignacio Hagopian <[email protected]> * feedback Signed-off-by: Ignacio Hagopian <[email protected]> * Update core/vm/evm.go Co-authored-by: Guillaume Ballet <[email protected]> --------- Signed-off-by: Ignacio Hagopian <[email protected]> Co-authored-by: Guillaume Ballet <[email protected]>

Signed-off-by: Ignacio Hagopian <[email protected]>

* fix EXTCODESIZE rule for system contract Signed-off-by: Ignacio Hagopian <[email protected]> * 21000 witness target write touch CODEHASH Signed-off-by: Ignacio Hagopian <[email protected]> * fix EXTCODECOPY witness for system contract Signed-off-by: Ignacio Hagopian <[email protected]> * rename and comment Signed-off-by: Ignacio Hagopian <[email protected]> --------- Signed-off-by: Ignacio Hagopian <[email protected]>

Signed-off-by: Ignacio Hagopian <[email protected]>

jsign · 2024-10-23T19:52:27Z

This PR served us well to generate the latest v0.0.6 fixtures, and planting a flag for stable fixtures.

Now closing this since we'll end up using #502 which that all bugs were fixed to pass the fixtures.

jsign changed the title ~~Jsign witness fix~~ witness: only include elements if there's enough gas for it Sep 12, 2024

jsign force-pushed the jsign-witness-fix branch from 7f2d3d5 to 7aedcc9 Compare September 12, 2024 22:04

gballet added this to the verkle-gen-devnet-7 milestone Sep 13, 2024

jsign force-pushed the jsign-witness-fix branch from ce68c07 to 5e0aed2 Compare September 19, 2024 15:03

jsign added 10 commits September 19, 2024 14:58

state: access witness with partial cost charging

9f44b79

Signed-off-by: Ignacio Hagopian <[email protected]>

fix most problems

8b45fb7

Signed-off-by: Ignacio Hagopian <[email protected]>

rebase fixes

d46fa01

Signed-off-by: Ignacio Hagopian <[email protected]>

fixes

d9bab66

Signed-off-by: Ignacio Hagopian <[email protected]>

fixes

3d79a1d

Signed-off-by: Ignacio Hagopian <[email protected]>

.

59720c4

Signed-off-by: Ignacio Hagopian <[email protected]>

remove 4762 gas call wrappers

49d84a2

Signed-off-by: Ignacio Hagopian <[email protected]>

more progress

ed4e5e5

Signed-off-by: Ignacio Hagopian <[email protected]>

call gas

4ecd713

Signed-off-by: Ignacio Hagopian <[email protected]>

fix

95f771d

Signed-off-by: Ignacio Hagopian <[email protected]>

jsign force-pushed the jsign-witness-fix branch from 314ebde to 95f771d Compare September 19, 2024 18:00

jsign added 3 commits September 19, 2024 15:11

fix

81940b9

Signed-off-by: Ignacio Hagopian <[email protected]>

fix

09be3aa

Signed-off-by: Ignacio Hagopian <[email protected]>

fix

f147cdc

Signed-off-by: Ignacio Hagopian <[email protected]>

jsign force-pushed the jsign-witness-fix branch from 3bd3e33 to f147cdc Compare September 19, 2024 18:44

jsign added 4 commits September 19, 2024 15:52

fix

b2e2fb8

Signed-off-by: Ignacio Hagopian <[email protected]>

fix

e98688e

Signed-off-by: Ignacio Hagopian <[email protected]>

fix

f29b148

Signed-off-by: Ignacio Hagopian <[email protected]>

fix

05e6092

Signed-off-by: Ignacio Hagopian <[email protected]>

jsign commented Sep 19, 2024

View reviewed changes

jsign requested a review from gballet September 19, 2024 19:44

jsign marked this pull request as ready for review September 19, 2024 19:44

jsign force-pushed the jsign-witness-fix branch from e028e81 to 05e6092 Compare September 20, 2024 14:05

fix

fdb9c50

Signed-off-by: Ignacio Hagopian <[email protected]>

gballet reviewed Sep 23, 2024

View reviewed changes

simplify warm costs

9ea37be

Signed-off-by: Ignacio Hagopian <[email protected]>

gballet reviewed Sep 24, 2024

View reviewed changes

gballet mentioned this pull request Sep 24, 2024

Witness charging order with limits #502

Merged

jsign added 2 commits September 25, 2024 10:15

cleanup

83aca33

Signed-off-by: Ignacio Hagopian <[email protected]>

commit statedb

ffc1f2d

Signed-off-by: Ignacio Hagopian <[email protected]>

jsign force-pushed the jsign-witness-fix branch from 16afde4 to ffc1f2d Compare September 27, 2024 13:10

jsign added 6 commits October 1, 2024 15:58

add proper verkle flag

a00d50a

Signed-off-by: Ignacio Hagopian <[email protected]>

spec bugfixes (#505)

85196cc

* fix Signed-off-by: Ignacio Hagopian <[email protected]> * eip4762: push witness fix Signed-off-by: Ignacio Hagopian <[email protected]> --------- Signed-off-by: Ignacio Hagopian <[email protected]>

selfdestruct: avoid BASIC_DATA inclusion for system contracts (#506)

8b106f2

* fix Signed-off-by: Ignacio Hagopian <[email protected]> * nit Signed-off-by: Ignacio Hagopian <[email protected]> --------- Signed-off-by: Ignacio Hagopian <[email protected]>

eip4762: change returned error for witness oog (#507)

51dca93

Signed-off-by: Ignacio Hagopian <[email protected]>

eip4762: move *CALL BASIC_DATA charging before reservation

b32bca9

Signed-off-by: Ignacio Hagopian <[email protected]>

jsign force-pushed the jsign-witness-fix branch from 1fe1124 to b32bca9 Compare October 10, 2024 16:46

jsign added 2 commits October 11, 2024 10:58

CALL witness charge ordering change

26452f4

Signed-off-by: Ignacio Hagopian <[email protected]>

criminal hacks

4d4eabc

Signed-off-by: Ignacio Hagopian <[email protected]>

gballet reviewed Oct 15, 2024

View reviewed changes

core/vm/eips.go Show resolved Hide resolved

gballet and others added 2 commits October 15, 2024 17:25

fix: create init order redux (#510)

f4733ce

Signed-off-by: Guillaume Ballet <[email protected]>

jsign force-pushed the jsign-witness-fix branch from 23b6788 to 514b4d5 Compare October 15, 2024 18:47

filling fixes

9635cc1

Signed-off-by: Ignacio Hagopian <[email protected]>

jsign force-pushed the jsign-witness-fix branch from b4b1ff0 to 9635cc1 Compare October 15, 2024 19:06

jsign added 6 commits October 16, 2024 16:08

nit

b2b96a2

Signed-off-by: Ignacio Hagopian <[email protected]>

extra fix (#512)

8057173

Signed-off-by: Ignacio Hagopian <[email protected]>

avoid BASIC_DATA branch edit for free (#513)

59426d9

Signed-off-by: Ignacio Hagopian <[email protected]>

ci: stable fixtures target v0.0.5 (#514)

4694243

Signed-off-by: Ignacio Hagopian <[email protected]>

fix

c329550

Signed-off-by: Ignacio Hagopian <[email protected]>

jsign closed this Oct 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

witness: only include elements if there's enough gas for it #495

witness: only include elements if there's enough gas for it #495

jsign commented Sep 12, 2024 •

edited

Loading

jsign Sep 19, 2024

jsign Sep 19, 2024

jsign Sep 19, 2024

jsign Sep 19, 2024

jsign Sep 19, 2024

jsign Sep 19, 2024

gballet Sep 23, 2024

jsign Sep 19, 2024

jsign Sep 19, 2024

jsign Sep 19, 2024

jsign Sep 19, 2024

gballet left a comment

gballet Sep 23, 2024

gballet left a comment

jsign commented Oct 23, 2024

		statedb.Witness().TouchFullAccount(w.Address[:], true)
		statedb.Witness().TouchFullAccount(w.Address[:], true, nil)

		chargedGas, ok := interpreter.evm.Accesses.TouchBasicData(address[:], false, scope.Contract.UseGas)
		if !ok \|\| (chargedGas == 0 && !scope.Contract.UseGas(params.WarmStorageReadCostEIP2929)) {

witness: only include elements if there's enough gas for it #495

witness: only include elements if there's enough gas for it #495

Conversation

jsign commented Sep 12, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gballet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gballet left a comment

Choose a reason for hiding this comment

jsign commented Oct 23, 2024

jsign commented Sep 12, 2024 •

edited

Loading