Update witness gas charging to differentiate between access events and write events #37

jwasinger · 2021-11-25T05:37:57Z

This PR updates the witness gas charging scheme to differentiate between witness access events and write events.
Relevant spec doc: https://notes.ethereum.org/-fJSOrnYQl-mqoWKpaTIsQ

TODOs that can be addressed in future PR(s):

Charging for filling a previously-empty leaf is omitted from this PR as it is currently unclear how to expose the db to the EVM in a non-hacky way.
* witness access events/costs are not handled for PUSH. Because executed code is accounted for in the interpreter loop, I think the costs/witnesses would still be correct in most cases. However, I can see issues with witnesses/costs being missed (e.g. PUSH32 could advance the PC over a slot and cause the leaf for that code to not be charged/included). To properly make this change, PUSH will have to be modified to be treated as a dynamically-priced opcode in Geth's EVM. <- done in Refactor witness-accumulation in EVM #42

gballet

Undefined is used to mark that a location has been accessed in the gas accounting code, a point at which the db isn't available. if value is nil, TouchAddress will put the chunk in Undefined instead of Chunks, in order to write down that the data needs to be resolved at a later time. This has some problems, like the inability to tell the difference between a missing value (need for a proof of absence) and an unresolved value (i.e. a bug). Using the suggested mode field in Chunks, it should be possible to tell the difference, and to get rid of Undefined altogether. And also not to pass a confusing nil parameter in the gas accounting code.

gballet · 2021-11-25T08:27:28Z

consensus/ethash/consensus.go

@@ -669,5 +669,5 @@ func accumulateRewards(config *params.ChainConfig, state *state.StateDB, header
 	}
 	state.AddBalance(header.Coinbase, reward)
 	coinbase := utils.GetTreeKeyBalance(header.Coinbase.Bytes())
-	state.Witness().TouchAddress(coinbase, state.GetBalance(header.Coinbase).Bytes())


TouchAddressOnWrite

core/types/access_witness.go

gballet · 2021-11-25T08:46:19Z

core/types/access_witness.go

-		newStem     bool
-		newSelector bool
+		stemRead bool
+		selectorRead bool


linter issue

core/vm/evm.go

… in. Adjust TestProcessStateless to account for increased intrinsic gas cost

jwasinger · 2021-12-02T07:55:47Z

consensus/ethash/consensus.go

@@ -669,5 +669,6 @@ func accumulateRewards(config *params.ChainConfig, state *state.StateDB, header
 	}
 	state.AddBalance(header.Coinbase, reward)
 	coinbase := utils.GetTreeKeyBalance(header.Coinbase.Bytes())
-	state.Witness().TouchAddress(coinbase, state.GetBalance(header.Coinbase).Bytes())
+	state.Witness().TouchAddressOnReadAndChargeGas(coinbase)


note to self TouchAddressOnWrite.

gballet · 2021-12-02T07:58:40Z

core/state_processor_test.go

@@ -378,11 +378,11 @@ func TestProcessStateless(t *testing.T) {
 	blockchain, _ := NewBlockChain(db, nil, gspec.Config, ethash.NewFaker(), vm.Config{}, nil, nil)
 	defer blockchain.Stop()
 	chain, _ := GenerateVerkleChain(gspec.Config, genesis, ethash.NewFaker(), db, 1, func(_ int, gen *BlockGen) {
-		tx, _ := types.SignTx(types.NewTransaction(0, common.Address{1, 2, 3}, big.NewInt(999), params.TxGas, big.NewInt(875000000), nil), signer, testKey)


add a comment or a formula so that people understand where this value comes from

jwasinger · 2021-12-02T08:03:23Z

core/types/access_witness.go

-		Chunks:    make(map[common.Hash][]byte),
-		Undefined: make(map[common.Hash]struct{}),
+		Branches:  make(map[VerkleStem]byte),
+		Chunks:    make(map[common.Hash]ChunkValue),


rename to ChunkState.

gballet · 2021-12-02T08:05:12Z

core/types/access_witness.go

+const (
+	AccessWitnessReadFlag = 1
+	AccessWitnessWriteFlag = 2
+)


Move this above ChunkState, declare a Mode type and set it like:

Suggested change

const (

AccessWitnessReadFlag = 1

AccessWitnessWriteFlag = 2

)

const (

AccessWitnessReadFlag = Mode(1)

AccessWitnessWriteFlag = Mode(2)

)

gballet · 2021-12-02T08:06:58Z

core/types/access_witness.go

+	AccessWitnessWriteFlag = 2
+)
+
+// because of the way Geth's EVM is implemented, the gas cost of an operation


start comment with name of function

gballet · 2021-12-02T08:09:43Z

core/types/access_witness.go

+// because of the way Geth's EVM is implemented, the gas cost of an operation
+// may be needed before the value of the leaf-key can be retrieved. Hence, we
+// break witness access (for the purpose of gas accounting), and filling witness
+// values into two methods


please replace with a comment that explains what the function does

gballet · 2021-12-02T08:11:38Z

core/types/access_witness.go

+	if chunk, exists := aw.Chunks[common.BytesToHash(addr)]; exists {
+		chunk.value = value
+	} else {
+		panic(fmt.Sprintf("address not in access witness: %x", addr))
+	}


Suggested change

if chunk, exists := aw.Chunks[common.BytesToHash(addr)]; exists {

chunk.value = value

} else {

panic(fmt.Sprintf("address not in access witness: %x", addr))

}

chunk, exists := aw.Chunks[common.BytesToHash(addr)]

if !exists {

panic(fmt.Sprintf("address not in access witness: %x", addr))

}

chunk.value = value

gballet · 2021-12-02T08:12:57Z

core/types/access_witness.go

+func (aw *AccessWitness) touchAddressOnWrite(addr []byte) (bool, bool, bool) {
+	var stem        VerkleStem
+	var stemWrite, chunkWrite, chunkFill bool


Suggested change

func (aw *AccessWitness) touchAddressOnWrite(addr []byte) (bool, bool, bool) {

var stem VerkleStem

var stemWrite, chunkWrite, chunkFill bool

func (aw *AccessWitness) touchAddressOnWrite(addr []byte) (stemWrite, chunkWrite, chunkFill bool) {

var stem VerkleStem

gballet · 2021-12-02T08:14:21Z

core/types/access_witness.go

+	// NOTE: stem, selector access flags already exist in their
+	// respective maps because this function is called at the end of 
+	// processing a read access event
+


Suggested change

gballet · 2021-12-02T08:16:30Z

core/types/access_witness.go

 	}
+	*/
+
+	return stemWrite, chunkWrite, chunkFill


Suggested change

return stemWrite, chunkWrite, chunkFill

return

gballet · 2021-12-02T08:19:35Z

core/types/access_witness.go

+	chunkValue := aw.Chunks[common.BytesToHash(addr)]
+	if chunkValue.mode & AccessWitnessWriteFlag != 0 {
+		chunkWrite = true
+		chunkValue.mode |= AccessWitnessWriteFlag
+		aw.Chunks[common.BytesToHash(addr)] = chunkValue
+	}


Suggested change

chunkValue := aw.Chunks[common.BytesToHash(addr)]

if chunkValue.mode & AccessWitnessWriteFlag != 0 {

chunkWrite = true

chunkValue.mode |= AccessWitnessWriteFlag

aw.Chunks[common.BytesToHash(addr)] = chunkValue

}

chunkValue := &aw.Chunks[common.BytesToHash(addr)]

if chunkValue.mode & AccessWitnessWriteFlag != 0 {

chunkWrite = true

chunkValue.mode |= AccessWitnessWriteFlag

}

gballet · 2021-12-02T08:25:18Z

core/types/access_witness.go

 }

 // TouchAddress adds any missing addr to the witness and returns respectively
 // true if the stem or the stub weren't arleady present.
-func (aw *AccessWitness) TouchAddress(addr, value []byte) (bool, bool) {
+func (aw *AccessWitness) touchAddress(addr []byte, isWrite bool) (bool, bool, bool, bool, bool) {


same trick as above, declare return variables here

jwasinger · 2021-12-02T08:28:27Z

core/vm/evm.go

+		return false
+	}
+
+	evm.Accesses.SetLeafValue(key, value)


Move this above gas balance check.

gballet · 2021-12-02T08:29:53Z

core/vm/evm.go

+		return false
+	}
+
+	evm.Accesses.SetLeafValue(key, value)


problem here: SetLeafValue overwrites the original value

gballet · 2021-12-02T08:33:10Z

core/vm/instructions.go

@@ -961,7 +962,8 @@ func makePush(size uint64, pushByteSize int) executionFunc {
 		value[0] = byte(count)
 		copy(value[1:], scope.Contract.Code[chunk*31:endMin])
 		index := trieUtils.GetTreeKeyCodeChunk(scope.Contract.Address().Bytes(), uint256.NewInt(chunk))
-		interpreter.evm.TxContext.Accesses.TouchAddressAndChargeGas(index, nil)
+		interpreter.evm.TxContext.Accesses.TouchAddressOnReadAndChargeGas(index)


move the gas charge to a gas_table.go equivalent of makePush

gballet · 2021-12-02T08:58:06Z

@jwasinger when verifying the proof in the test, this is the PR in which I did this before. #26 It has "bit-rotted" to quite an extent, but it might still be useful as an inspiration. The code that deserializes the proof is here https://github.com/gballet/go-ethereum/pull/26/files#diff-6303aaa1a681f7a75b66a759f7997d98b7795865af5fb26b4310688b14f1f9fbR215 and verification just above.

It's particularly horrible because it wasn't rebased when the reference branch was. I can try to do that if it's really impractical.

jwasinger · 2021-12-14T03:47:57Z

@gballet as we discussed in #39 , the proof is embedded by the miner so it can't be tested by TestProcessStateless right now. I suggest that the test be augmented to include a breakdown which calculates the tx gas values that need to be supplied in the test (witness costs + TxGas). Then I can, in another PR, add more validation to the proofs embedded in the block in this test:

go-ethereum/miner/worker_test.go

Line 320 in 6af78cb

if block.Header().VerkleProof == nil {

…unt-check (ethereum#24765) * cmd/geth, core/state/snapshot: rework journal loading, implement account-check * core/state/snapshot, cmd/geth: polish code (#37) * core/state/snapshot: minor nits * core/state/snapshot: simplify error logic * cmd/geth: go format Co-authored-by: rjl493456442 <garyrong0905@gmail.com>

jwasinger requested a review from holiman as a code owner November 25, 2021 05:37

gballet requested review from gballet and removed request for holiman November 25, 2021 08:27

gballet reviewed Nov 25, 2021

View reviewed changes

gballet force-pushed the verkle-trie-proof-in-block-rebased branch from b8981f6 to 86bdc3f Compare November 26, 2021 15:50

jwasinger added 2 commits November 30, 2021 04:12

wip access witness gas charging for writes

99bf4e2

wip

0ede6e2

jwasinger force-pushed the access-witness-take-2 branch from ca3ccb1 to 0ede6e2 Compare November 30, 2021 07:10

jwasinger added 3 commits November 30, 2021 08:25

wip

2c421d2

builds

500a8c2

make sure intrinsic gas check occurs after witness costs are factored…

acff77d

… in. Adjust TestProcessStateless to account for increased intrinsic gas cost

jwasinger mentioned this pull request Dec 2, 2021

Ensure that witness costs are charged per-tx and not per-block ethereum/go-verkle#137

Closed

jwasinger commented Dec 2, 2021

View reviewed changes

gballet reviewed Dec 2, 2021

View reviewed changes

jwasinger commented Dec 2, 2021

View reviewed changes

gballet reviewed Dec 2, 2021

View reviewed changes

jwasinger commented Dec 2, 2021

View reviewed changes

core/vm/evm.go

return false

}

evm.Accesses.SetLeafValue(key, value)

Copy link

Collaborator Author

jwasinger Dec 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this above gas balance check.

gballet reviewed Dec 2, 2021

View reviewed changes

core/vm/evm.go

return false

}

evm.Accesses.SetLeafValue(key, value)

Copy link

Owner

gballet Dec 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

problem here: SetLeafValue overwrites the original value

gballet reviewed Dec 2, 2021

View reviewed changes

jwasinger mentioned this pull request Dec 6, 2021

Refactor witness-accumulation in EVM #42

Merged

jwasinger changed the title ~~Update witness gas charging~~ Update witness gas charging to differentiate between access events and write events Dec 14, 2021

This was referenced Dec 14, 2021

Message call verkle witness gas charging #43

Closed

Message call witness gas charging #44

Closed

This was referenced Jan 19, 2022

Charge witness write costs ethereum/go-verkle#118

Closed

Implement latest stateless gas charging spec #63

Merged

jwasinger closed this Jan 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update witness gas charging to differentiate between access events and write events #37

Update witness gas charging to differentiate between access events and write events #37

jwasinger commented Nov 25, 2021 •

edited

Loading

gballet left a comment

gballet Nov 25, 2021

gballet Nov 25, 2021

jwasinger Dec 2, 2021

gballet Dec 2, 2021

jwasinger Dec 2, 2021

gballet Dec 2, 2021

gballet Dec 2, 2021

gballet Dec 2, 2021

gballet Dec 2, 2021

gballet Dec 2, 2021

gballet Dec 2, 2021

gballet Dec 2, 2021

gballet Dec 2, 2021

gballet Dec 2, 2021

jwasinger Dec 2, 2021

gballet Dec 2, 2021

gballet Dec 2, 2021

gballet commented Dec 2, 2021 •

edited

Loading

jwasinger commented Dec 14, 2021

Update witness gas charging to differentiate between access events and write events #37

Update witness gas charging to differentiate between access events and write events #37

Conversation

jwasinger commented Nov 25, 2021 • edited Loading

gballet 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gballet commented Dec 2, 2021 • edited Loading

jwasinger commented Dec 14, 2021

jwasinger commented Nov 25, 2021 •

edited

Loading

gballet commented Dec 2, 2021 •

edited

Loading