Cache and use justified and finalized payload block hash - #10657
Merged
prylabs-bulldozer[bot] merged 10 commits intoMay 10, 2022
Merged
Conversation
rauljordan
reviewed
May 9, 2022
| } | ||
| } | ||
| s.saveInitSyncBlock(blockRoots[i], b) | ||
| if err = s.handleBlockAfterBatchVerify(ctx, b, blockRoots[i], fCheckpoints[i], jCheckpoints[i]); err != nil { |
Collaborator
Author
There was a problem hiding this comment.
To cache justified and finalized block hashes before calling forkchoice updated
rauljordan
reviewed
May 9, 2022
| s.Lock() | ||
| defer s.Unlock() | ||
| s.finalizedCheckpt = cp | ||
| s.finalizedPayloadBlockHash = h |
Contributor
There was a problem hiding this comment.
A lot of the tests use [32]byte{}. I could remove this line and every test would still pass. Perhaps we can try a more realistic payload hash when using it in unit tests
terencechain
commented
May 9, 2022
Comment on lines
-64
to
+63
| finalizedHash, err := s.getPayloadHash(ctx, arg.finalizedRoot) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "could not get finalized block hash") | ||
| } | ||
| justifiedHash, err := s.getPayloadHash(ctx, arg.justifiedRoot) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "could not get justified block hash") | ||
| } | ||
| finalizedHash := s.store.FinalizedPayloadBlockHash() | ||
| justifiedHash := s.store.JustifiedPayloadBlockHash() |
Collaborator
Author
There was a problem hiding this comment.
Use cached payload hashes instead of retrieving it from DB from given input roots
terencechain
commented
May 9, 2022
Comment on lines
-429
to
+446
| f := fCheckpoints[len(fCheckpoints)-1] | ||
| j := jCheckpoints[len(jCheckpoints)-1] | ||
| arg := ¬ifyForkchoiceUpdateArg{ | ||
| headState: preState, | ||
| headRoot: lastBR, | ||
| headBlock: lastB.Block(), | ||
| finalizedRoot: bytesutil.ToBytes32(f.Root), | ||
| justifiedRoot: bytesutil.ToBytes32(j.Root), | ||
| headState: preState, | ||
| headRoot: lastBR, | ||
| headBlock: lastB.Block(), |
Collaborator
Author
There was a problem hiding this comment.
Nice clean-up. We no longer require input justified and finalized roots. We could retrieve it from the cache
terencechain
commented
May 9, 2022
Comment on lines
-97
to
-100
| if err = s.handleBlockAfterBatchVerify(ctx, blockCopy, blkRoots[i], fCheckpoints[i], jCheckpoints[i]); err != nil { | ||
| tracing.AnnotateError(span, err) | ||
| return err | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Move this to before calling fork choice updated to use cache payload hashes. There's no functionality changes by moving this
rauljordan
previously approved these changes
May 10, 2022
rauljordan
approved these changes
May 10, 2022
delete-merged-branch
Bot
deleted the
cache-justified-finalized-payload-hash
branch
May 10, 2022 21:20
rkapka
pushed a commit
that referenced
this pull request
May 11, 2022
* Cache and use justified and finalized payload block hash * Fix tests * Use real byte * Fix conflicts Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>
prylabs-bulldozer Bot
added a commit
that referenced
this pull request
May 16, 2022
* SubmitBlockSSZ grpc * SubmitBlockSSZ middleware * test fixes * use VersionedUnmarshaller * use VersionedUnmarshaller (cherry picked from commit 7388eeb) * tests * fuzz: Add fuzz tests for sparse merkle trie (#10662) * Add fuzz tests for sparse merkle trie and change HTR signature to return an error * fix capitalization of error message * Add engine timeout values (#10645) * Add timeout values * Update engine_client.go * Update engine_client.go * Update beacon-chain/powchain/engine_client.go Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com> * Update beacon-chain/powchain/engine_client.go Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com> * Update beacon-chain/powchain/engine_client.go Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com> * Update engine_client.go Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com> * Cleanup of `stategen` package (#10607) * powchain and stategen * revert powchain changes * rename field to blockRootsOfSavedStates * rename params to blockRoot * review feedback * fix loop Co-authored-by: Raul Jordan <raul@prysmaticlabs.com> Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com> * Process atts and update head before proposing (#10653) * Process atts and updeate head * Fix ctx * New test and old tests * Update validator_test.go * Update validator_test.go * Update service.go * Rename to UpdateHead * Update receive_attestation.go * Update receive_attestation.go Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com> * Add link to e2e docs in `README` (#10672) * Improve `ReceiveBlock`'s comment (#10671) Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com> * Call fcu on invalid payload (#10565) * Starting * remove finalized root * Just call fcu * Review feedbacks * fix one test * Fix conflicts * Update execution_engine_test.go * Add a test for invalid recursive call * Add comprehensive recursive test * dissallow override empty hash Co-authored-by: Potuz <potuz@prysmaticlabs.com> Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com> * Cache and use justified and finalized payload block hash (#10657) * Cache and use justified and finalized payload block hash * Fix tests * Use real byte * Fix conflicts Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com> * do not export slotFromBlock * simplify tests * grpc * middleware * extract package-level consts * Simplify SSZ handling * fix tests * test fixes * test hack Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com> Co-authored-by: terencechain <terence@prysmaticlabs.com> Co-authored-by: Raul Jordan <raul@prysmaticlabs.com> Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com> Co-authored-by: Potuz <potuz@prysmaticlabs.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #10628
This PR caches and uses justified and finalized payload block hash. The win is saving 2 database lookups for justified and finalized blocks every time we update the head. ~100ms on my machine.
This PR also refactors the initial syncing code path to call
handleBlockAfterBatchVerifyearlier and withinonBlockBatch. There's no functional impact to that, it's a nice cleanup and but we do need to callhandleBlockAfterBatchVerifybefore we callnotifyForkchoiceUpdate. Another nice clean up from this isnotifyForkchoiceUpdaterequire two less arguments, finalized and justified roots are no longer required