implement FILL_COSTS on top of devnet7 #524

gballet · 2024-11-04T21:06:44Z

TODO:

rewrite code so that the access event takes the isFill from the statedb directly, as much as possible.

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

core/vm/operations_verkle.go

gballet · 2024-11-04T21:09:31Z

core/state/access_witness.go

+	// The isFill here is a bit strange: one would have to pay for the fill costs
+	// and potentially run out of gas before the account is created. This is due
+	// to the charging for a potential account creation being split in two locations:
+	// the basic data is written in the gas function, and the code hash in opCall.
+	_, wanted2 := aw.touchAddressAndChargeGas(targetAddr, zeroTreeIndex, utils.BasicDataLeafKey, true, isFill, availableGas-wanted1)


@jsign this is the comment describing the situation we just discussed. I think it's terrible. Let me know if you have a better way to describe this, otherwise I'll take another shot at it tomorrow.

Yeah, I see. I don't think is that bad. If I look at this method in a vaccum, what is doing seems logical compared to how the API is proposed. i.e: the caller should already exist so looks reasonable L121 is ignoring isFill and setting to false, and the targetAddr we trust the callr to tell us if fill cost should be charged.

core/state_processor_test.go

jsign

I left some comments for your consideration. I think most of what I say won't be surprising since I think I shared those thoughts a while ago when chatting about FILL_COSTs.

Also, not proposing doing a refactor on this PR. If the geth team is OK with this solution, I think it's fine (of course) -- just that form I prefer less assumptions in calls.

jsign · 2024-11-04T21:34:43Z

cmd/evm/internal/t8ntool/execution.go

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


I think this code line already shows something evident in the proposed isFill bool API.

Every caller should decide if the witness addition should charge a fill cost, which seems odd unless every caller knows if the underlying tree has a "hole" in the tree at that place.

For example, for this line, it doesn't seem obvious why a withdrawal would need to charge fill cost unconditionally since that seems to depend on runtime conditions. In this case isn't that relevant since we aren't charging that to anyone -- but just feels odd.

The much nicer alternative is not including this isFill bool in the APIs, and let AccessWitness figure out if the write should charge fill cost, probably assisted by statedb.

I think I mentioned this a while ago when we discussed about FILL_COST, so shouldn't be a big surprise this thought came again now -- resharing just in case.

The much nicer alternative is not including this isFill bool in the APIs, and let AccessWitness figure out if the write should charge fill cost, probably assisted by statedb.

yeah that is right, we would save some typing for sure. This is meant for measurements so I won't do it before devcon, but you are correct that we should do it this way.

jsign · 2024-11-04T21:37:22Z

core/state/access_witness.go

+func NewAccessWitness(pointCache *utils.PointCache, fills map[chunkAccessKey]struct{}) *AccessWitness {
 	return &AccessWitness{
 		branches:   make(map[branchAccessKey]mode),
 		chunks:     make(map[chunkAccessKey]mode),
+		fills:      fills,


First reaction looking at this, is why you'd like to initialize a witness with a pre-filled map.

My intuition this is related with AccessWitness not having access to statedb, so now you have to deal with previous tx in the same block having done writes that this tx shouldn't charge. So.. kind of a second order "dirty" map.

This might add up to the previous comment I did why I think AccessWitness having access to statedb might be a good idea. You can avoid having to do this trick, and just rely on statedb in a state already committed by previous tx, so no "special consideration" is needed.

(Note: not saying we should do this in this PR or similar -- mostly talking about long term strategies)

the difference is that writes are per-tx while the fills are per-block. Not sure the stateedb can always provide us with this info, I'll have to think about it when rewriting.

jsign · 2024-11-04T21:41:11Z

core/state/access_witness.go

 	return naw
 }

-func (aw *AccessWitness) TouchFullAccount(addr []byte, isWrite bool, availableGas uint64) uint64 {
+func (aw *AccessWitness) TouchFullAccount(addr []byte, isWrite, isFill bool, availableGas uint64) uint64 {


As mentioned before: assuming the caller always is write about knowing the tree doesn't have the value, then I guess this is fine.

Note this also shows as fundamental flaw in the API design: someone calling isWrite=false nad isFill=true.

I think I had said this before too and suggested maybe a flag-ish style API? Like isRead | isWrite | isWriteWithFill. Despite this solves the flaw, it can be somewhat annoying from the caller side -- but looks safer.

Maybe we could add:

if !isWrite && isFill { panic("can't can't charge fill cost in a state read") }

or similar.

you can't charge fill costs in a state read, if write is false.

jsign · 2024-11-04T21:43:09Z

core/state/access_witness.go

+	// The isFill here is a bit strange: one would have to pay for the fill costs
+	// and potentially run out of gas before the account is created. This is due
+	// to the charging for a potential account creation being split in two locations:
+	// the basic data is written in the gas function, and the code hash in opCall.
+	_, wanted2 := aw.touchAddressAndChargeGas(targetAddr, zeroTreeIndex, utils.BasicDataLeafKey, true, isFill, availableGas-wanted1)


Yeah, I see. I don't think is that bad. If I look at this method in a vaccum, what is doing seems logical compared to how the API is proposed. i.e: the caller should already exist so looks reasonable L121 is ignoring isFill and setting to false, and the targetAddr we trust the callr to tell us if fill cost should be charged.

jsign · 2024-11-04T21:48:49Z

core/state/access_witness.go

@@ -294,8 +313,8 @@ func (aw *AccessWitness) TouchCodeChunksRangeAndChargeGas(contractAddr []byte, s
 	return statelessGasCharged, statelessGasCharged
 }

-func (aw *AccessWitness) TouchBasicData(addr []byte, isWrite bool, availableGas uint64, warmCostCharging bool) uint64 {
-	_, wanted := aw.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.BasicDataLeafKey, isWrite, availableGas)
+func (aw *AccessWitness) TouchBasicData(addr []byte, isWrite, isFill bool, availableGas uint64, warmCostCharging bool) uint64 {


Feel some tension here between isFill and warmCostCharging. Looks like when isFill=true, whatever warmCostCharging say is not relevant.

Not saying this is a huge problem... just feels a bit weird.

agreed, not a problem but we're starting to accumulate booleans and that creates for non-sensical situations that should never happen ™️ but might be triggered at some point

jsign · 2024-11-04T21:52:16Z

core/state_processor.go

@@ -192,5 +192,5 @@ func ProcessParentBlockHash(statedb *state.StateDB, prevNumber uint64, prevHash
 	var key common.Hash
 	binary.BigEndian.PutUint64(key[24:], ringIndex)
 	statedb.SetState(params.HistoryStorageAddress, key, prevHash)
-	statedb.Witness().TouchSlotAndChargeGas(params.HistoryStorageAddress[:], key, true, math.MaxUint64, false)
+	statedb.Witness().TouchSlotAndChargeGas(params.HistoryStorageAddress[:], key, true, true, math.MaxUint64, false)


Ditto. It's correct for some time while the ring buffer is empty, but after some point is actually always false. (Again can argue doesn't matter since we ignore the output, but still).

jsign · 2024-11-04T21:52:30Z

core/state_transition.go

-			st.evm.Accesses.TouchFullAccount(st.evm.Context.Coinbase[:], true, math.MaxUint64)
+			st.evm.Accesses.TouchFullAccount(st.evm.Context.Coinbase[:], true, true, math.MaxUint64)


core/state_processor_test.go

jsign · 2024-11-04T21:56:29Z

core/vm/instructions.go

Yeah, so the three diffs in this file are "redundant cases" of isFill since isWrite=false. Just a consequence of the API.

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

jsign · 2024-12-06T18:13:25Z

core/state/state_object.go

+func (s *stateObject) GetOriginState(key common.Hash) common.Hash {
+	if value, cached := s.originStorage[key]; cached {
+		return value
+	}
+	return common.Hash{}
+}


Could be somehow better signal that originStorage is a cache and not a fully reliable source of truth? Maybe renaming to originStorageCache and also rename the method to GetOriginStateCache or something?

I don't like the rename to GetOriginStateCache but L309 of core/state/statedb.go was quite surprising to me, and then I realized it's because it's only asking for a cache.

And even a more important reason to be super-clear about this GetOriginState being a cache, is that this method is public. If I see that name in an API and returns common.Hash{} I'd assume it doesn't exist, not that "potentially I don't know".

implement FILL_COSTS on top of devnet7

8101511

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

gballet commented Nov 4, 2024

View reviewed changes

jsign reviewed Nov 4, 2024

View reviewed changes

gballet added 2 commits November 5, 2024 11:55

fix gas costs

544b460

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

implement storageexist

f4dcec5

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

jsign reviewed Dec 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement FILL_COSTS on top of devnet7 #524

implement FILL_COSTS on top of devnet7 #524

gballet commented Nov 4, 2024 •

edited

Loading

gballet Nov 4, 2024

jsign Nov 4, 2024

jsign left a comment

jsign Nov 4, 2024

gballet Nov 5, 2024

jsign Nov 4, 2024

gballet Nov 5, 2024

jsign Nov 4, 2024

jsign Nov 4, 2024

jsign Nov 4, 2024

gballet Nov 5, 2024

jsign Nov 4, 2024

jsign Nov 4, 2024

gballet Nov 5, 2024

jsign Nov 4, 2024

jsign Nov 4, 2024

jsign Nov 4, 2024

jsign Dec 6, 2024

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

		st.evm.Accesses.TouchFullAccount(st.evm.Context.Coinbase[:], true, math.MaxUint64)
		st.evm.Accesses.TouchFullAccount(st.evm.Context.Coinbase[:], true, true, math.MaxUint64)

implement FILL_COSTS on top of devnet7 #524

Are you sure you want to change the base?

implement FILL_COSTS on top of devnet7 #524

Conversation

gballet commented Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsign 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gballet commented Nov 4, 2024 •

edited

Loading