proofs: remove post-values in Multiproof #297

jsign · 2023-10-24T21:49:46Z

This PR does some client-side changes to adjust proofs to not include post-state values.

It depends on ethereum/go-verkle#407 being merged.

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

jsign · 2023-10-24T21:51:08Z

consensus/beacon/consensus.go

-			// TODO: see if this can be captured at the witness
-			// level, like it used to.
-			for _, key := range keys {
-				// WORKAROUND: the post trie would normally not
-				// need to be searched for keys, as all of them
-				// were resolved during block execution.
-				// But since the prefetcher isn't currently used
-				// with verkle, the values that are read but not
-				// written to, are not resolved as they are read
-				// straight from the snapshot. They must be read
-				// in order to build the proof.
-				_, err = vtrpost.GetWithHashedKey(key)
-				if err != nil {
-					panic(err)
-				}
-			}


This shouldn't be needed from the previous PR that we properly use the resolver in the GetElementsForProof. Now, this is even more true since we don't do that for proof generation so this should be safe.

I can't test it directly since this code path only works in real proof generation, so let's be aware the next time we update nodes in Kaustinen.

well if it fails, and looking at it and the fact that it doesn't in sync mode, I'm comfortable enough merging that. If it fails, we'll figure it out fast enough after relaunch.

jsign · 2023-10-24T21:52:13Z

core/state_processor_test.go

@@ -518,7 +518,7 @@ func TestProcessVerkle(t *testing.T) {
 	//f.Write(buf.Bytes())
 	//fmt.Printf("root= %x\n", chain[0].Root())
 	// check the proof for the last block
-	err := trie.DeserializeAndVerifyVerkleProof(proofs[1], chain[0].Root().Bytes(), keyvals[1])
+	err := trie.DeserializeAndVerifyVerkleProof(proofs[1], chain[0].Root().Bytes(), chain[1].Root().Bytes(), keyvals[1])


The DeserializeAndVerifyVerkleProof now also receives the expected post-tree root.
This is because now we do an extra check to see if the reconstructed post-state tree root, matches the block state root.

This must be true. If it's false, the block is invalid (or there's a bug in post-tree reconstruction).

jsign · 2023-10-24T21:52:44Z

go.mod

-	github.com/gballet/go-verkle v0.1.1-0.20231020124853-d124d1998b1a
+	github.com/gballet/go-verkle v0.1.1-0.20231024193227-1127148c02ea


Pointing to latest commit in ethereum/go-verkle#407

When that PR is merged, I'll update this PR to point to master.

jsign · 2023-10-24T21:54:50Z

trie/verkle.go

-
-	var others set = set{} // Mark when an "other" stem has been seen
+func DeserializeAndVerifyVerkleProof(vp *verkle.VerkleProof, preStateRoot []byte, postStateRoot []byte, statediff verkle.StateDiff) error {
+	// TODO: check that `OtherStems` have expected length and values.


This is something that we should do. This prob needs to be done in PreStateTreeFromProof(...). The pre-state tree reconstruction should consume exactly all elements in OtherStems. If some extra value isn't used, then the proof should be considered incorrect (IMHO).

This is unrelated with all this post-state change; just something I realized that I believe was the original idea of this deleted code in L339-349 & L356-359 (all that code wasn't doing anything).

To do this extra check, we don't need that code, so I'm suggesting here to be removed (since it's currently not doing anything).

sure yes we should add this, in another PR if you prefer.

jsign · 2023-10-24T21:55:26Z

trie/verkle.go

+	// TODO: this is necessary to verify that the post-values are the correct ones.
+	// But all this can be avoided with a even faster way. The EVM block execution can
+	// keep track of the written keys, and compare that list with this post-values list.
+	// This can avoid regenerating the post-tree which is somewhat expensive.
 	posttree, err := verkle.PostStateTreeFromStateDiff(pretree, statediff)
 	if err != nil {
 		return fmt.Errorf("error rebuilding the post-tree from proof: %w", err)
 	}
+	regeneratedPostTreeRoot := posttree.Commitment().Bytes()
+	if !bytes.Equal(regeneratedPostTreeRoot[:], postStateRoot) {
+		return fmt.Errorf("post tree root mismatch: %x != %x", regeneratedPostTreeRoot, postStateRoot)
+	}


As I mentioned in Matrix; just re-explaining in this TODO.

Something to explore in the future after doing more measurements.

but wait, root computation will be performed in IntermediateRoot and there is a check that it matches the header in the block. If you already check that all written values are the same as were found during execution, there is no need to compute/check the root here.

That's exactly what I describe in my TODO above saying:

// TODO: this is necessary to verify that the post-values are the correct ones. // But all this can be avoided with a even faster way. The EVM block execution can // keep track of the written keys, and compare that list with this post-values list. // This can avoid regenerating the post-tree which is somewhat expensive.

If we do that, we can avoid it.

Or maybe are you referring to something different?

no that's what I meant: let's do that, so that we don't have to recompute the root

gballet

LGTM. Let's wait for the go-verkle PR to be merged.

Left one comment about the root commitment verification that we can discuss in matrix.

gballet · 2023-10-25T08:03:06Z

trie/verkle.go

-
-	var others set = set{} // Mark when an "other" stem has been seen
+func DeserializeAndVerifyVerkleProof(vp *verkle.VerkleProof, preStateRoot []byte, postStateRoot []byte, statediff verkle.StateDiff) error {
+	// TODO: check that `OtherStems` have expected length and values.


sure yes we should add this, in another PR if you prefer.

gballet · 2023-10-25T08:04:51Z

trie/verkle.go

+	// TODO: this is necessary to verify that the post-values are the correct ones.
+	// But all this can be avoided with a even faster way. The EVM block execution can
+	// keep track of the written keys, and compare that list with this post-values list.
+	// This can avoid regenerating the post-tree which is somewhat expensive.


let's do it, unless it makes the PR so much more complex, that it makes sense to split it?

gballet · 2023-10-25T08:12:12Z

trie/verkle.go

+	// TODO: this is necessary to verify that the post-values are the correct ones.
+	// But all this can be avoided with a even faster way. The EVM block execution can
+	// keep track of the written keys, and compare that list with this post-values list.
+	// This can avoid regenerating the post-tree which is somewhat expensive.
 	posttree, err := verkle.PostStateTreeFromStateDiff(pretree, statediff)
 	if err != nil {
 		return fmt.Errorf("error rebuilding the post-tree from proof: %w", err)
 	}
+	regeneratedPostTreeRoot := posttree.Commitment().Bytes()
+	if !bytes.Equal(regeneratedPostTreeRoot[:], postStateRoot) {
+		return fmt.Errorf("post tree root mismatch: %x != %x", regeneratedPostTreeRoot, postStateRoot)
+	}


but wait, root computation will be performed in IntermediateRoot and there is a check that it matches the header in the block. If you already check that all written values are the same as were found during execution, there is no need to compute/check the root here.

gballet · 2023-10-25T08:15:23Z

consensus/beacon/consensus.go

-			// TODO: see if this can be captured at the witness
-			// level, like it used to.
-			for _, key := range keys {
-				// WORKAROUND: the post trie would normally not
-				// need to be searched for keys, as all of them
-				// were resolved during block execution.
-				// But since the prefetcher isn't currently used
-				// with verkle, the values that are read but not
-				// written to, are not resolved as they are read
-				// straight from the snapshot. They must be read
-				// in order to build the proof.
-				_, err = vtrpost.GetWithHashedKey(key)
-				if err != nil {
-					panic(err)
-				}
-			}


well if it fails, and looking at it and the fact that it doesn't in sync mode, I'm comfortable enough merging that. If it fails, we'll figure it out fast enough after relaunch.

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

gballet

LGTM

* update proof generation Signed-off-by: Ignacio Hagopian <[email protected]> * simplification and comment Signed-off-by: Ignacio Hagopian <[email protected]> * update go-verkle Signed-off-by: Ignacio Hagopian <[email protected]> --------- Signed-off-by: Ignacio Hagopian <[email protected]>

jsign added 2 commits October 24, 2023 16:36

update proof generation

f66bf79

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

simplification and comment

ac60a21

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

jsign changed the title ~~Jsign postvalues proof rm~~ proofs: remove post-values in Multiproof Oct 24, 2023

jsign commented Oct 24, 2023

View reviewed changes

gballet reviewed Oct 25, 2023

View reviewed changes

update go-verkle

ec187bd

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

jsign requested a review from gballet October 25, 2023 15:59

jsign marked this pull request as ready for review October 25, 2023 15:59

gballet approved these changes Oct 25, 2023

View reviewed changes

gballet merged commit 3f4159b into kaustinen-with-shapella Oct 25, 2023
2 of 3 checks passed

jsign deleted the jsign-postvalues-proof-rm branch October 25, 2023 18:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proofs: remove post-values in Multiproof #297

proofs: remove post-values in Multiproof #297

jsign commented Oct 24, 2023

jsign Oct 24, 2023

gballet Oct 25, 2023

jsign Oct 24, 2023

jsign Oct 24, 2023

jsign Oct 24, 2023

gballet Oct 25, 2023

jsign Oct 24, 2023

gballet Oct 25, 2023

jsign Oct 25, 2023

gballet Oct 25, 2023

gballet left a comment

gballet Oct 25, 2023

gballet Oct 25, 2023

gballet Oct 25, 2023

gballet Oct 25, 2023

gballet left a comment

		github.com/gballet/go-verkle v0.1.1-0.20231020124853-d124d1998b1a
		github.com/gballet/go-verkle v0.1.1-0.20231024193227-1127148c02ea

proofs: remove post-values in Multiproof #297

proofs: remove post-values in Multiproof #297

Conversation

jsign commented Oct 24, 2023

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

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