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.

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

tree: add new method GetAndLoadForProof #383

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jsign
Copy link
Collaborator

@jsign jsign commented Aug 9, 2023

This PR adds a new helper method that will assist clients in preloading the tree for proof generation.

See gballet/go-ethereum#255

@jsign jsign force-pushed the jsign-load-key-for-proof branch from 2a6d843 to 131aace Compare August 9, 2023 17:54
@jsign jsign changed the title tree: add new method LoadKeyForProof tree: add new method GetAndLoadForProof Aug 9, 2023
Copy link
Member

@gballet gballet left a comment

Choose a reason for hiding this comment

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

Left some comments. I am not sure I understand why you don't simply use Get?

tree.go Outdated
}
pe.Vals = append(pe.Vals, nil) // TODO(PR-temp): workaround proving bug.
Copy link
Member

Choose a reason for hiding this comment

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

Could you please move this to another PR and come up with a test that demonstrates the bug?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tree.go Outdated Show resolved Hide resolved
@jsign
Copy link
Collaborator Author

jsign commented Oct 4, 2023

Left some comments. I am not sure I understand why you don't simply use Get?

When you call Get, it will resolve all internal nodes directly in the path to the corresponding leaf. As in, the minimal node loading needed to reach the desired leaf (as expected).

This method does something extra needed for proof generation. When we want to generate a proof for a leaf value, we also need all the internal node children in that path. That's because we need all the evaluations of the vector in that internal node to prove the opening in internal nodes. This part of the PR code is exactly doing this (also note some code comment a few lines above).

Doing all this at the Geth layer is very complicated and confusing. This API is providing a way of asking: "Load the tree for this key, and also all the auxiliary stuff that I'll need to generate a proof without any further resolving" (as you'd have with using Get).

That means that Geth can call this method for every key in the witness. After finishing with these simple calls it will know the loaded tree is fully ready to generate a proof (without a resolver or anything extra for proof generation).

That's exactly the idea of the referenced Geth PR in the PR description. While the EVM is collecting accessed keys in the witness, we use this API to load in the background the needed tree with all the resolved stuff. When the EVM finishes, the Witness already has loaded all that it needs so Geth can use this tree to generate the proof.

@jsign jsign mentioned this pull request Oct 4, 2023
@gballet
Copy link
Member

gballet commented Oct 4, 2023

ah right of course. LGTM.

Copy link
Member

@gballet gballet left a comment

Choose a reason for hiding this comment

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

LGTM

@jsign jsign force-pushed the jsign-load-key-for-proof branch from 56b32d7 to de81293 Compare October 4, 2023 13:09
proof_ipa.go Outdated
Comment on lines 63 to 65
for i := range vp.OtherStems {
ret.OtherStems[i] = vp.OtherStems[i]
}

copy(ret.OtherStems, vp.OtherStems)
copy(ret.DepthExtensionPresent, vp.DepthExtensionPresent)
for i := range vp.CommitmentsByPath {
ret.CommitmentsByPath[i] = vp.CommitmentsByPath[i]
}
copy(ret.CommitmentsByPath, vp.CommitmentsByPath)
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 new linter was complaining about this (unrelated to the PR), but it's still a good point and fixed.

@jsign jsign force-pushed the jsign-load-key-for-proof branch from f3de2cb to 186176e Compare October 13, 2023 13:03
jsign added 3 commits October 18, 2023 15:56
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
@jsign jsign force-pushed the jsign-load-key-for-proof branch from 186176e to e0c4061 Compare October 18, 2023 18:56
@jsign jsign added the on-hold label Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants