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

overlay benchmark: enable proof generation #255

Draft
wants to merge 4 commits into
base: jsign-access-witness-new
Choose a base branch
from

Conversation

jsign
Copy link
Collaborator

@jsign jsign commented Aug 10, 2023

This PR enables proof generation in the block processing pipeline.

The strategy is to allow the access witness to generate an adequately loaded tree in the background while txnContext.AccessWitness is merged in the main block AccessWitness:

  • When Merge() is called, it push a set of new keys that will be included in the proof.
  • The background loader will leverage a new API candidate in go-verkle that loads the paths for those keys, plus all children in touched InternalNodes, which is also required for proving.
  • That API candidate also returns the respective value of the key, which helps building also the keyValues for calling ProveAndSerialize(...).
  • This change indirectly offloads to the background calculating the Pedersen Hashing, which before this change was a blocking procedure when calling accessWitness.Keys(). Now this accessWitness.Keys() API could be removed (to be discussed).

This PR is sitting on top of the new access witness (not merged) PR since it’s the most reasonable base target. I couldn’t test this branch since the base branch sits atop another (temporarily) broken branch. At some point, I’ll be able to run it and confirm if it works as expected (and fix any remaining nit).

This is still a draft PR:

  • When the base-base branch works, run and verify this PR works as expected.
  • When the target branch of this PR is merged, rebase to the new base branch and open for review.
  • Double-check/confirm new included API names.
  • Decide on .Keys() API removal.
  • Add extra logs/stats that can help in understanding overheads.
  • Rebase to Kaustinen
  • Update go-verkle

Comment on lines -28 to +29
github.com/gballet/go-verkle v0.0.0-20230725193842-b2d852dc666b
github.com/gballet/go-verkle v0.0.0-20230809181720-8d8980709295
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
@jsign jsign force-pushed the jsign-enable-proofs-overlay branch from 6598141 to 5f011ab Compare August 28, 2023 12:57
@jsign jsign mentioned this pull request Sep 5, 2023
2 tasks
Copy link
Owner

@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. I believe the target branch has already been merged, so would it be possible to recreate this PR against kaustinen?

@@ -212,7 +212,8 @@ func (s *StateDB) NewAccessWitness() *AccessWitness {

func (s *StateDB) Witness() *AccessWitness {
if s.witness == nil {
s.witness = s.NewAccessWitness()
verkleTree := s.GetTrie().(*trie.TransitionTrie).Overlay()
Copy link
Owner

Choose a reason for hiding this comment

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

there should be a switch to check if the trie is a *VerkleTrie here. This is the case after the transition (and on testnets that "verge" at genesis)

trie/verkle.go Show resolved Hide resolved
@gballet
Copy link
Owner

gballet commented Oct 5, 2023

Regarding the .Keys() removal, I would wait a tad longer in case it's still useful. I would just add a TODO for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants