Kaustinen with shapella #259

gballet · 2023-08-29T13:36:10Z

Move kaustinent changes from #176 and update it for the proof to be generated inside FinalizeAndAssemble so that proof generation is available from all places.

In order for the verkle test to work, I had to re-implement a more functional chain reader than the one that is currently available withfakeChainReader (that only returns nil).

~~This effort is currently blocked by the fact that GetProofItems does not resolve HashedNodes.~~ -> merged relevant PR
~~The post-state is also not provided, which is a blocker for the next instantiation of kaustinen.~~ -> fixed with use go-verkle's post-state API to verify proofs #262

gballet · 2023-09-05T18:20:23Z

consensus/beacon/consensus.go

+		k    verkle.StateDiff
+		keys = state.Witness().Keys()
+	)
+	if chain.Config().IsVerkle(header.Number, header.Time) {


This is the extra part: if we are past the fork, generate the proof to go with it

gballet · 2023-09-05T18:31:06Z

consensus/beacon/consensus.go

+		parent := chain.GetHeaderByNumber(header.Number.Uint64() - 1)
+		if parent == nil {
+			return nil, fmt.Errorf("nil parent header for block %d", header.Number)
+		}
+
+		preTrie, err := state.Database().OpenTrie(parent.Root)
+		if err != nil {
+			return nil, fmt.Errorf("error opening pre-state tree root: %w", err)
+		}


This is one very inefficient part: because at this point we only have the post tree, we need to reopen the pre-tree.

There is a better approach, that I tried once but failed to get to work: capture the initial value in the witness so that we don't have to do this. I will have to try again, since I should have been catching this in GetCommittedState instead of all over the codebase. Now that your AccessWitness rewrite simplified a lot of things, it might be easier. Nonetheless, this is for a different PR. Just pointing out the huge optimization that is possible here.

I've a PR candidate trying to optimize this thing about loading the pre-state tree in the access witness #255

We still haven't discussed much that PR... but that's exactly the idea. Build the pre-state tree while we're merging witnesses, so we have all we need to call prove and serialize.

gballet · 2023-09-05T18:31:32Z

consensus/beacon/consensus.go

+		}
+
+		vtrpre, okpre := preTrie.(*trie.VerkleTrie)
+		vtrpost, okpost := state.GetTrie().(*trie.VerkleTrie)


post trie is the current trie

gballet · 2023-09-05T18:33:10Z

core/chain_makers.go

@@ -558,3 +523,59 @@ func (cr *fakeChainReader) GetHeaderByHash(hash common.Hash) *types.Header
 func (cr *fakeChainReader) GetHeader(hash common.Hash, number uint64) *types.Header { return nil }
 func (cr *fakeChainReader) GetBlock(hash common.Hash, number uint64) *types.Block   { return nil }
 func (cr *fakeChainReader) GetTd(hash common.Hash, number uint64) *big.Int          { return nil }
+


I had to make my own fake chain reader, I will work on a geth PR to enhance the default one to reduce the footprint of this PR. Disregard.

gballet · 2023-09-05T18:33:48Z

core/state/database.go

@@ -365,8 +365,9 @@ func (db *cachingDB) openStorageMPTrie(stateRoot common.Hash, address common.Add

 // OpenStorageTrie opens the storage trie of an account
 func (db *cachingDB) OpenStorageTrie(stateRoot common.Hash, address common.Address, root common.Hash, self Trie) (Trie, error) {
+	// TODO this should only return a verkle tree


This will disappear with the rebase.

gballet · 2023-09-05T18:41:07Z

consensus/beacon/consensus.go

+			}
+
+			if len(keys) > 0 {
+				p, k, err = trie.ProveAndSerialize(vtrpre, vtrpost, keys, vtrpre.FlatdbNodeResolver)


I had to export (*VerkleTrie).FlatdbNodeResolver so that it can be used here, because in the case of a stem proving the absence of another stem, the proving code will need to resolve that former stem when it hasn't been read yet.

I would have expected all the necessary stems for the proof be already loaded in the L410 loop.
If one of the keys is absent, wouldn't that indirectly load the stem that prove it's absence?

In any case, you can ignore my comment. Prob we can continue this chat when we evaluate #255, and maybe we realize that can't work for this reason.

gballet · 2023-09-05T18:43:53Z

consensus/beacon/consensus.go

+			for _, key := range keys {
+				_, err := vtrpre.GetWithHashedKey(key)
+				if err != nil {
+					panic(err)
+				}
+			}


I have to resolve all keys from the witness here, because otherwise they would be hashed nodes. That precedes the Prover taking a resolving function as a parameter, so it might no longer be necessary.

Of course, it's even less necessary if we can get these values from the access witness, but I see now that we won't be able to completely remove the need to open the pre-trie, because of that need to get the proof-of-absence stem...

gballet · 2023-09-05T18:44:37Z

trie/verkle.go

 	}

-	tree, err := verkle.TreeFromProof(proof, rootC)
+	pretree, err := verkle.PreStateTreeFromProof(proof, rootC)


First step of the proof reconstruction: rebuild the pre tree from the proof.

gballet · 2023-09-05T18:46:43Z

trie/verkle.go

+		return fmt.Errorf("error rebuilding the pre-tree from proof: %w", err)
 	}
 	for _, stemdiff := range statediff {
 		for _, suffixdiff := range stemdiff.SuffixDiffs {
 			var key [32]byte
 			copy(key[:31], stemdiff.Stem[:])
 			key[31] = suffixdiff.Suffix

-			val, err := tree.Get(key[:], nil)
+			val, err := pretree.Get(key[:], nil)
 			if err != nil {
-				return nil, nil, nil, nil, fmt.Errorf("could not find key %x in tree rebuilt from proof: %w", key, err)
+				return fmt.Errorf("could not find key %x in tree rebuilt from proof: %w", key, err)
 			}
 			if len(val) > 0 {
 				if !bytes.Equal(val, suffixdiff.CurrentValue[:]) {
-					return nil, nil, nil, nil, fmt.Errorf("could not find correct value at %x in tree rebuilt from proof: %x != %x", key, val, *suffixdiff.CurrentValue)
+					return fmt.Errorf("could not find correct value at %x in tree rebuilt from proof: %x != %x", key, val, *suffixdiff.CurrentValue)
 				}
 			} else {
 				if suffixdiff.CurrentValue != nil && len(suffixdiff.CurrentValue) != 0 {
-					return nil, nil, nil, nil, fmt.Errorf("could not find correct value at %x in tree rebuilt from proof: %x != %x", key, val, *suffixdiff.CurrentValue)
+					return fmt.Errorf("could not find correct value at %x in tree rebuilt from proof: %x != %x", key, val, *suffixdiff.CurrentValue)


This is the same thing as line 410 & following in consensus/beacon/consensus.go, except it might not be necessary. I still need to check that.

In theory, the tree was reconstructed using statediff.. So technically, this is only for defensive programming regarding implementation bugs and not malicious input, right?

Saying it another way: if there're no bugs in DeserializeProof(vp ,statediff) then these checks can never fail.

Mentioning this to double check the intention is catching implementation bugs, and not "malicious" provers.

(Is that what you refer to "might not be necessary"?)

yeah it's code that has changed a lot in the past, I would keep it for now in order to make sure that nothing breaks. I will add a reminder though.

gballet · 2023-09-05T18:47:33Z

trie/verkle.go

-	// no need to resolve as the tree has been reconstructed from the proof
-	// and must not contain any unresolved nodes.
-	pe, _, _, err := tree.GetProofItems(proof.Keys)
+	posttree, err := verkle.PostStateTreeFromProof(pretree, statediff)


2nd part of making the proof: reconstruct the post tree from the pre-tree by copying the pre-tree and inserting all the NewValues from the proof into that post trie.

gballet · 2023-09-05T18:48:04Z

trie/verkle.go

 	}

-	return proof, pe.Cis, pe.Zis, pe.Yis, err
+	return verkle.VerifyVerkleProofWithPreAndPostTrie(proof, pretree, posttree, nil /* no resolver needed TODO: remove it from the interface */)


Good point Guillaume, remove it from the interface!

jsign

Left some comments for your consideration.

jsign · 2023-09-05T20:21:20Z

trie/verkle.go

+	if posttrie != nil {
+		postroot = posttrie.root
+	}


Can we safely assume that posttrie was posttrie.Commit()ed?

yes, because it happens after the block finalization code has been called.

jsign · 2023-09-05T20:22:08Z

trie/verkle.go

-
-	return nil
-}
+	rootC.SetBytesUnsafe(root)


Is this correct? (compared to using the safe version that does security check)

If we receive the root from a block that is untrusted, I don't think so.

jsign · 2023-09-05T20:31:19Z

trie/verkle.go

@@ -338,69 +342,55 @@ func addKey(s set, key []byte) {

 func DeserializeAndVerifyVerkleProof(vp *verkle.VerkleProof, root []byte, statediff verkle.StateDiff) error {


Side-question: is there a reason StateDIff isn't part of verkle.VerkleProof? Looks like only having verkle.VerkleProof is somewhat incomplete to do anything relevant?

I guess StateDiff is considered part of the witness, so not part of the proof per se.

jsign · 2023-09-05T20:32:36Z

trie/verkle.go

+	if len(proof.Keys) != len(proof.PreValues) {
+		return fmt.Errorf("keys and values are of different length %d != %d", len(proof.Keys), len(proof.PreValues))
 	}


Might be useful to move this check to PreStateTreeFromProof.

jsign · 2023-09-05T20:35:54Z

consensus/beacon/consensus.go

+		parent := chain.GetHeaderByNumber(header.Number.Uint64() - 1)
+		if parent == nil {
+			return nil, fmt.Errorf("nil parent header for block %d", header.Number)
+		}
+
+		preTrie, err := state.Database().OpenTrie(parent.Root)
+		if err != nil {
+			return nil, fmt.Errorf("error opening pre-state tree root: %w", err)
+		}


I've a PR candidate trying to optimize this thing about loading the pre-state tree in the access witness #255

We still haven't discussed much that PR... but that's exactly the idea. Build the pre-state tree while we're merging witnesses, so we have all we need to call prove and serialize.

jsign · 2023-09-05T20:53:30Z

core/types/block.go

+
+		copy(ret.VerkleProof.DepthExtensionPresent, ew.VerkleProof.DepthExtensionPresent)
+		for i := range ew.VerkleProof.CommitmentsByPath {
+			copy(ret.VerkleProof.CommitmentsByPath[i][:], ew.VerkleProof.CommitmentsByPath[i][:])


Nit: Could be simplified as: ret.VerkleProof.CommitmentsByPath[i] = ew.VerkleProof.CommitmentsByPath[i] since arrays are values in Go. So avoid copy and [:] stuff.

The code is still correct, to up to you.

jsign · 2023-09-05T20:54:59Z

core/types/block.go

+		for i := range ew.VerkleProof.CommitmentsByPath {
+			copy(ret.VerkleProof.CommitmentsByPath[i][:], ew.VerkleProof.CommitmentsByPath[i][:])
+		}
+		copy(ret.VerkleProof.D[:], ew.VerkleProof.D[:])


jsign · 2023-09-05T20:57:37Z

core/types/block.go

+		if ew.VerkleProof.IPAProof != nil {
+			for i := range ew.VerkleProof.IPAProof.CL {
+				copy(ret.VerkleProof.IPAProof.CL[i][:], ew.VerkleProof.IPAProof.CL[i][:])
+				copy(ret.VerkleProof.IPAProof.CR[i][:], ew.VerkleProof.IPAProof.CR[i][:])
+			}
+			copy(ret.VerkleProof.IPAProof.FinalEvaluation[:], ew.VerkleProof.IPAProof.FinalEvaluation[:])
+		}


Ditto. In this case we can get rid of the for loop.
ret.VerkleProof.IPAProof.CL = ew.VerkleProof.IPAProof.CL (and same with the other).

Proof in playground.

So actually you can do ew.VerkleProof.IPAProof = ret.VerkleProof.IPAProof and all L100-L104 are gone. And it's a copy, not a reference.

jsign · 2023-09-05T21:03:43Z

trie/verkle.go

+		return fmt.Errorf("error rebuilding the pre-tree from proof: %w", err)
 	}
 	for _, stemdiff := range statediff {
 		for _, suffixdiff := range stemdiff.SuffixDiffs {
 			var key [32]byte
 			copy(key[:31], stemdiff.Stem[:])
 			key[31] = suffixdiff.Suffix

-			val, err := tree.Get(key[:], nil)
+			val, err := pretree.Get(key[:], nil)
 			if err != nil {
-				return nil, nil, nil, nil, fmt.Errorf("could not find key %x in tree rebuilt from proof: %w", key, err)
+				return fmt.Errorf("could not find key %x in tree rebuilt from proof: %w", key, err)
 			}
 			if len(val) > 0 {
 				if !bytes.Equal(val, suffixdiff.CurrentValue[:]) {
-					return nil, nil, nil, nil, fmt.Errorf("could not find correct value at %x in tree rebuilt from proof: %x != %x", key, val, *suffixdiff.CurrentValue)
+					return fmt.Errorf("could not find correct value at %x in tree rebuilt from proof: %x != %x", key, val, *suffixdiff.CurrentValue)
 				}
 			} else {
 				if suffixdiff.CurrentValue != nil && len(suffixdiff.CurrentValue) != 0 {
-					return nil, nil, nil, nil, fmt.Errorf("could not find correct value at %x in tree rebuilt from proof: %x != %x", key, val, *suffixdiff.CurrentValue)
+					return fmt.Errorf("could not find correct value at %x in tree rebuilt from proof: %x != %x", key, val, *suffixdiff.CurrentValue)


In theory, the tree was reconstructed using statediff.. So technically, this is only for defensive programming regarding implementation bugs and not malicious input, right?

Saying it another way: if there're no bugs in DeserializeProof(vp ,statediff) then these checks can never fail.

Mentioning this to double check the intention is catching implementation bugs, and not "malicious" provers.

(Is that what you refer to "might not be necessary"?)

jsign · 2023-09-05T21:11:02Z

trie/verkle.go

 			}
 			if len(val) > 0 {
 				if !bytes.Equal(val, suffixdiff.CurrentValue[:]) {
-					return nil, nil, nil, nil, fmt.Errorf("could not find correct value at %x in tree rebuilt from proof: %x != %x", key, val, *suffixdiff.CurrentValue)
+					return fmt.Errorf("could not find correct value at %x in tree rebuilt from proof: %x != %x", key, val, *suffixdiff.CurrentValue)
 				}
 			} else {
 				if suffixdiff.CurrentValue != nil && len(suffixdiff.CurrentValue) != 0 {


This code (else case) won't return an error in the following cases:

val = nil and suffixdiff.CurrentValue = []byte{}, and

val = []byte{} and suffixdiff.CurrentValue = nil

Is that fine?

yes, nil and []byte{} both represent an absence

gballet · 2023-09-09T20:47:52Z

This branch, once all bugs have been solved, should be merged. There's little point having 3 branches (Beverly hills, kautinen and replays) when we can only have 2: kaustinen and replays. Beverly hills is nice for other clients to check they support the basic tree structures without the proofs, but this could be controlled by a simple command line switch.

gballet · 2023-09-10T06:11:51Z

left to do before merge:

rebase
fix test dependency cycle
ensure that TestProcessVerkle still runs and see if it can use the "new" merge technique
add a similar test that does the transition at the 2nd block
act on review
add cli switch to disable proofs in blocks, so that this branch can work on beverly hills as well
move all replay-specific code to the replay branch, in order to have only 2 branches to maintain

activate proof generation on fork + remove code dups use go-verkle's post-state API to verify proofs (#262) use prague as the verkle activation fork (#263) upgrade to latest go-ipa activate verkle transition in "miner" (#265) fix: do not force cancunTime upon verkle activation workaround: do not use root translation in replay workaround: deactivate overlay transition for now fixes from trying to get the devnet to work (#267) this line was left out from the previous commit upgrade to go-verkle with fixed newvalue serialization fix: ensure point cache isn't nil in copy (#268) fix: dependency cycle in tests (#269) upgrade to latest go-verkle fix: write trie preimage data to db (#274) fix: zero-root in produced block + sync (#275) upgrade go-ipa fix build fix typo include review feedback add switch to add proofs to blocks (#278) add fee recipient to witness (#279) touch all fields in withdrawal account header (#277)

* implement eip 2935 * add touched historical contract slot to the witness

* fix: support a verkle pre-tree at the conversion block * make linter happy

* fix: decide if genesis is verkle using genesis timestamp * fix linter message

* set verkle mode from genesis with override * make linter happy * fix more linter snafu