Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proofs: remove post-values in Multiproof #297

Merged
merged 3 commits into from
Oct 25, 2023
Merged

proofs: remove post-values in Multiproof #297

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f66bf79
update proof generation
jsign Oct 24, 2023
ac60a21
simplification and comment
jsign Oct 24, 2023
ec187bd
update go-verkle
jsign Oct 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 0 additions & 17 deletions consensus/beacon/consensus.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,23 +426,6 @@ func (beacon *Beacon) FinalizeAndAssemble(chain consensus.ChainHeaderReader, hea
panic("invalid tree type")
}
if okpre && okpost {
// 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)
}
}
Comment on lines -429 to -444
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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.


if len(keys) > 0 {
p, k, err = trie.ProveAndSerialize(vtrpre, vtrpost, keys, vtrpre.FlatdbNodeResolver)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion core/state_processor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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])
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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).

if err != nil {
t.Fatal(err)
}
Expand Down
4 changes: 2 additions & 2 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ require (
github.com/cloudflare/cloudflare-go v0.14.0
github.com/cockroachdb/pebble v0.0.0-20230209160836-829675f94811
github.com/consensys/gnark-crypto v0.12.1
github.com/crate-crypto/go-ipa v0.0.0-20231015184653-ceac2650f699
github.com/crate-crypto/go-ipa v0.0.0-20231025140028-3c0104f4b233
github.com/crate-crypto/go-kzg-4844 v0.3.0
github.com/davecgh/go-spew v1.1.1
github.com/deckarep/golang-set/v2 v2.1.0
Expand All @@ -26,7 +26,7 @@ require (
github.com/fjl/memsize v0.0.0-20190710130421-bcb5799ab5e5
github.com/fsnotify/fsnotify v1.6.0
github.com/gballet/go-libpcsclite v0.0.0-20190607065134-2772fd86a8ff
github.com/gballet/go-verkle v0.1.1-0.20231020124853-d124d1998b1a
github.com/gballet/go-verkle v0.1.1-0.20231025151349-87337dd2894a
github.com/go-stack/stack v1.8.1
github.com/gofrs/flock v0.8.1
github.com/golang-jwt/jwt/v4 v4.3.0
Expand Down
8 changes: 4 additions & 4 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,8 @@ github.com/cpuguy83/go-md2man v1.0.10/go.mod h1:SmD6nW6nTyfqj6ABTjUi3V3JVMnlJmwc
github.com/cpuguy83/go-md2man/v2 v2.0.0-20190314233015-f79a8a8ca69d/go.mod h1:maD7wRr/U5Z6m/iR4s+kqSMx2CaBsrgA7czyZG/E6dU=
github.com/cpuguy83/go-md2man/v2 v2.0.2 h1:p1EgwI/C7NhT0JmVkwCD2ZBK8j4aeHQX2pMHHBfMQ6w=
github.com/cpuguy83/go-md2man/v2 v2.0.2/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46tRHOmNcaadrF8o=
github.com/crate-crypto/go-ipa v0.0.0-20231015184653-ceac2650f699 h1:ng/jln5iPr92iLbq6dHHa5dbObAgUmAoQO7Zjx1vYHM=
github.com/crate-crypto/go-ipa v0.0.0-20231015184653-ceac2650f699/go.mod h1:J+gsi6D4peY0kyhaklyXFRVHOQWI2I5uU0c2+/90HYc=
github.com/crate-crypto/go-ipa v0.0.0-20231025140028-3c0104f4b233 h1:d28BXYi+wUpz1KBmiF9bWrjEMacUEREV6MBi2ODnrfQ=
github.com/crate-crypto/go-ipa v0.0.0-20231025140028-3c0104f4b233/go.mod h1:geZJZH3SzKCqnz5VT0q/DyIG/tvu/dZk+VIfXicupJs=
github.com/crate-crypto/go-kzg-4844 v0.3.0 h1:UBlWE0CgyFqqzTI+IFyCzA7A3Zw4iip6uzRv5NIXG0A=
github.com/crate-crypto/go-kzg-4844 v0.3.0/go.mod h1:SBP7ikXEgDnUPONgm33HtuDZEDtWa3L4QtN1ocJSEQ4=
github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
Expand Down Expand Up @@ -144,8 +144,8 @@ github.com/garslo/gogen v0.0.0-20170306192744-1d203ffc1f61/go.mod h1:Q0X6pkwTILD
github.com/gavv/httpexpect v2.0.0+incompatible/go.mod h1:x+9tiU1YnrOvnB725RkpoLv1M62hOWzwo5OXotisrKc=
github.com/gballet/go-libpcsclite v0.0.0-20190607065134-2772fd86a8ff h1:tY80oXqGNY4FhTFhk+o9oFHGINQ/+vhlm8HFzi6znCI=
github.com/gballet/go-libpcsclite v0.0.0-20190607065134-2772fd86a8ff/go.mod h1:x7DCsMOv1taUwEWCzT4cmDeAkigA5/QCwUodaVOe8Ww=
github.com/gballet/go-verkle v0.1.1-0.20231020124853-d124d1998b1a h1:kqhR2nTIep0lw7zJBp2ju+fWbqP3PojUw3L+jv1qBK4=
github.com/gballet/go-verkle v0.1.1-0.20231020124853-d124d1998b1a/go.mod h1:7JamHhSTnnHDhcI3G8r4sWaD9XlleriqVlC3FeAQJKM=
github.com/gballet/go-verkle v0.1.1-0.20231025151349-87337dd2894a h1:UV5Et3Ab62e6hQ6vDZC0h0i9hmKm8KKtV78zDOzud08=
github.com/gballet/go-verkle v0.1.1-0.20231025151349-87337dd2894a/go.mod h1:QNpY22eby74jVhqH4WhDLDwxc/vqsern6pW+u2kbkpc=
github.com/getkin/kin-openapi v0.53.0/go.mod h1:7Yn5whZr5kJi6t+kShccXS8ae1APpYTW6yheSwk8Yi4=
github.com/getkin/kin-openapi v0.61.0/go.mod h1:7Yn5whZr5kJi6t+kShccXS8ae1APpYTW6yheSwk8Yi4=
github.com/getsentry/sentry-go v0.12.0/go.mod h1:NSap0JBYWzHND8oMbyi0+XZhUalc1TBdRL1M71JZW2c=
Expand Down
29 changes: 13 additions & 16 deletions trie/verkle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,27 +336,16 @@ func ProveAndSerialize(pretrie, posttrie *VerkleTrie, keys [][]byte, resolver ve
return p, kvps, nil
}

type set = map[string]struct{}

func addKey(s set, key []byte) {
s[string(key)] = struct{}{}
}

func DeserializeAndVerifyVerkleProof(vp *verkle.VerkleProof, root []byte, statediff verkle.StateDiff) error {
rootC := new(verkle.Point)
rootC.SetBytes(root)

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.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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).

Copy link
Owner

Choose a reason for hiding this comment

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

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


proof, err := verkle.DeserializeProof(vp, statediff)
if err != nil {
return fmt.Errorf("verkle proof deserialization error: %w", err)
}

for _, stem := range proof.PoaStems {
addKey(others, stem)
}

rootC := new(verkle.Point)
rootC.SetBytes(preStateRoot)
pretree, err := verkle.PreStateTreeFromProof(proof, rootC)
if err != nil {
return fmt.Errorf("error rebuilding the pre-tree from proof: %w", err)
Expand Down Expand Up @@ -385,12 +374,20 @@ func DeserializeAndVerifyVerkleProof(vp *verkle.VerkleProof, root []byte, stated
}
}

// 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.
Comment on lines +377 to +380
Copy link
Owner

Choose a reason for hiding this comment

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

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

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)
}
Comment on lines +377 to +388
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Something to explore in the future after doing more measurements.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Owner

Choose a reason for hiding this comment

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

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


return verkle.VerifyVerkleProofWithPreAndPostTrie(proof, pretree, posttree)
return verkle.VerifyVerkleProofWithPreState(proof, pretree)
}

// ChunkedCode represents a sequence of 32-bytes chunks of code (31 bytes of which
Expand Down
Loading