implement post-nyota verkle costs #454

gballet · 2024-06-27T16:21:29Z

TODO

gballet · 2024-06-27T16:25:03Z

trie/verkle.go

+			headervals, err := t.root.(*verkle.InternalNode).GetValuesAtStem(key[:31], t.FlatdbNodeResolver)
+			if err != nil {
+				return fmt.Errorf("UpdateContractCode (addr=%x) error while getting account header: %w", addr[:], err)
+			}
+			var cs [8]byte
+			binary.LittleEndian.PutUint64(cs[:], uint64(len(code)))
+			copy(headervals[utils.BasicDataLeafKey][utils.BasicDataCodeSizeOffset:], cs[5:8])
+			values[utils.BasicDataLeafKey] = headervals[utils.BasicDataLeafKey]


This is the part I like the least: the simplicity we gained from using the basic data leaf, we recover in this particular place: now, in order to write the code size, we need to first load the leaf.

Until now, UpdateStem would go over all leaves and only overwrite the leaves that were nil. That used to be enough, because code size was in its own leaf. This is no longer the case, so the leaf has first to be fetched and then the byte range updated.

I think it can be done more efficiently, but I need to write it down. I will not merge this until I got to the bottom of the story.

Yeah, realized this in a previous comment (L173 in this file) before reaching here.

Def complicated.

gballet · 2024-06-27T16:27:36Z

core/state_processor_test.go

+			params.WitnessChunkReadCost+params.WitnessChunkWriteCost+ /* code chunk #12 */
+			params.WitnessChunkReadCost+params.WitnessChunkWriteCost+ /* code chunk #13 */
+			params.WitnessChunkReadCost+params.WitnessChunkWriteCost+ /* code chunk #14 */
+			4844 /* execution costs */)


I had always "aced" the gas costs. I think it's a better idea to break it down one by one.

I spent way too much time on this, and the execution costs - 4844 - is suspicious. But this test passes. The others don't at the moment.

jsign · 2024-06-27T17:32:24Z

core/overlay/conversion.go

-	leafNodeData.Values[utils.CodeSizeLeafKey] = codeSizeBytes[:]
+	var cs [8]byte
+	binary.LittleEndian.PutUint64(cs[:], codeSize)
+	copy(leafNodeData.Values[utils.BasicDataLeafKey][utils.BasicDataCodeSizeOffset:utils.BasicDataNonceOffset], cs[:3])


Suggestion: avoid magic number 3 and define a constant with the byte size.

Also, no need to put the [:utils.BasicDataNonceOffset] since copy will copy the shortest size of both parameters. So we can remove this dependency that the end should match with other field.

jsign · 2024-06-27T17:35:05Z

core/state/access_witness.go

-	if createSendsValue {
-		gas += aw.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.BalanceLeafKey, true)
-	}
+	gas += aw.touchAddressAndChargeGas(addr, zeroTreeIndex, utils.BasicDataLeafKey, true)


We can remove the parameter createSendsValue.

jsign · 2024-06-27T17:37:17Z

core/state/access_witness.go

 	if sendsValue {
-		aw.touchAddressAndChargeGas(targetAddr, zeroTreeIndex, utils.BalanceLeafKey, true)
-	} else {
-		aw.touchAddressAndChargeGas(targetAddr, zeroTreeIndex, utils.BalanceLeafKey, false)
+		aw.touchAddressAndChargeGas(targetAddr, zeroTreeIndex, utils.BasicDataLeafKey, true)
 	}


This code can be removed since we can pass sendsValue as the parameter in L134, right?

jsign · 2024-06-27T17:44:52Z

trie/verkle.go

-	if len(values[utils.NonceLeafKey]) > 0 {
-		acc.Nonce = binary.LittleEndian.Uint64(values[utils.NonceLeafKey])
+	if len(values[utils.BasicDataLeafKey]) > 0 {
+		acc.Nonce = binary.LittleEndian.Uint64(values[utils.BasicDataLeafKey][utils.BasicDataNonceOffset:])


This is debatable, but I don't like this subslicing to not have an ending mark.
I know Uint64 is 8 bytes, and the spec says that Nonce is 8 bytes... so it isn't strictly necessary, but that matching is indirectly assumed.

This doesn't mean I'm suggesting to change it -- maybe I'm being purist.

jsign · 2024-06-27T17:47:31Z

trie/verkle.go

-	copy(balance[:], values[utils.BalanceLeafKey])
-	for i := 0; i < len(balance)/2; i++ {
-		balance[len(balance)-i-1], balance[i] = balance[i], balance[len(balance)-i-1]
+	copy(balance[16:], values[utils.BasicDataLeafKey][utils.BasicDataBalanceOffset:])


Same-ish (annoying) comment on setting the end slice limit. Dismiss if already decided not worth it.

jsign · 2024-06-27T17:54:02Z

trie/verkle.go

 	}

+	values[utils.BasicDataLeafKey] = basicData[:]


Wouldn't this unintentionally erase the code-size that might be already in the basic_data field of the leaf node?

We're building a new basicData [32]byte from scratch here, and if this address is a contract that had a contract_size in the existing basicData then we'd erase it.

Maybe this is a problem since in acc we don't have that info.

LMK if I'm missing something.

I see another comment you did below reg code size. But what if this method is called and not UpdateCode after? Hairy.

yeah it's buggy and I'm going to rewrite it

jsign · 2024-06-27T17:55:13Z

trie/verkle.go

@@ -448,6 +437,7 @@ func (t *VerkleTrie) ClearStrorageRootConversion(addr common.Address) {
 	t.db.ClearStorageRootConversion(addr)
 }

+// Note: the basic data leaf needs to have been previously created for this to work


Do you know if this is a safe assumption?

jsign · 2024-06-27T17:56:38Z

trie/verkle.go

+			headervals, err := t.root.(*verkle.InternalNode).GetValuesAtStem(key[:31], t.FlatdbNodeResolver)
+			if err != nil {
+				return fmt.Errorf("UpdateContractCode (addr=%x) error while getting account header: %w", addr[:], err)
+			}
+			var cs [8]byte
+			binary.LittleEndian.PutUint64(cs[:], uint64(len(code)))
+			copy(headervals[utils.BasicDataLeafKey][utils.BasicDataCodeSizeOffset:], cs[5:8])
+			values[utils.BasicDataLeafKey] = headervals[utils.BasicDataLeafKey]


Yeah, realized this in a previous comment (L173 in this file) before reaching here.

Def complicated.

trie/verkle.go

core/overlay/conversion.go

trie/utils/verkle.go

trie/verkle.go

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

* fixes from replaying nyota costs Signed-off-by: Guillaume Ballet <[email protected]> * add left-out conversion file * fix balance endianness + set code size in UpdateAccount * add leftover endianness change * remove code cleanup leftover * use 5 as the code size offset this matches the spec, even if we have to add a weird -1 here and there * save buffer creation for PutUint32 --------- Signed-off-by: Guillaume Ballet <[email protected]>

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

gballet · 2024-08-26T09:25:26Z

haven't managed to run it in kurtosis because of the genesis testnet generator, but it works in enough other contexts that this is clearly a configuration issue on my part. Merging.

gballet commented Jun 27, 2024

View reviewed changes

jsign reviewed Jun 27, 2024

View reviewed changes

gballet added this to the verkle-gen-devnet-7 milestone Jun 28, 2024

gballet force-pushed the nyota-costs branch from 8eea75f to c065295 Compare July 31, 2024 09:09

gballet commented Aug 14, 2024

View reviewed changes

core/overlay/conversion.go Outdated Show resolved Hide resolved

gballet commented Aug 14, 2024

View reviewed changes

trie/utils/verkle.go Show resolved Hide resolved

gballet commented Aug 14, 2024

View reviewed changes

trie/verkle.go Outdated Show resolved Hide resolved

gballet and others added 5 commits August 23, 2024 15:19

implement post-nyota verkle costs

46ee384

fix tests and a few bugs

baaccb6

make linter happy

92999f7

attempt at fixing replay + shadowfork

6626e04

nyota fixes (#469)

063552e

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

gballet force-pushed the nyota-costs branch from 67f47b7 to edabe0f Compare August 23, 2024 13:24

gballet force-pushed the nyota-costs branch from edabe0f to 1c13629 Compare August 23, 2024 13:25

gballet and others added 3 commits August 23, 2024 18:47

fix most tests

f626498

ci: fix buggy target spec tests (temporary)

c4691c1

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

fix last failing test

9b090a3

gballet marked this pull request as ready for review August 23, 2024 19:05

gballet merged commit df13260 into kaustinen-with-shapella Aug 26, 2024
9 of 10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement post-nyota verkle costs #454

implement post-nyota verkle costs #454

gballet commented Jun 27, 2024 •

edited

Loading

gballet Jun 27, 2024

jsign Jun 27, 2024

gballet Jun 27, 2024

jsign Jun 27, 2024

jsign Jun 27, 2024

jsign Jun 27, 2024

jsign Jun 27, 2024

jsign Jun 27, 2024

jsign Jun 27, 2024

jsign Jun 27, 2024

gballet Aug 14, 2024

jsign Jun 27, 2024

jsign Jun 27, 2024

gballet commented Aug 26, 2024

implement post-nyota verkle costs #454

implement post-nyota verkle costs #454

Conversation

gballet commented Jun 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gballet commented Aug 26, 2024

gballet commented Jun 27, 2024 •

edited

Loading