Add merge transition/finalization banners #3963
Merged
Conversation
This file contains 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
g11tech
commented
May 2, 2022
| @@ -0,0 +1,36 @@ | |||
| /* eslint-disable no-useless-escape */ | |||
| export const POS_PANDA_FINALIZED_BANNER = ` | |||
There was a problem hiding this comment.
@philknows , do we want a similar banner on the finalization of the merge, i.e. first finalized checkpoint post merge? we can remove this or make it super minimal or have something else about it.
There was a problem hiding this comment.
I think just the merge transition block is enough for the banner. It's what the other clients are doing. Finalization can just remain a normal log statement.
Codecov Report
@@ Coverage Diff @@
## master #3963 +/- ##
==========================================
- Coverage 36.83% 36.81% -0.03%
==========================================
Files 324 325 +1
Lines 9187 9198 +11
Branches 1494 1495 +1
==========================================
+ Hits 3384 3386 +2
- Misses 5612 5621 +9
Partials 191 191 |
Performance Report✔️ no performance regression detected Full benchmark results
|
g11tech
commented
May 2, 2022
| } | ||
| if (!prevIsMergeTransitionFinalized && chain.forkChoice.getMergeTransitionFinalized()) { | ||
| const finalizedCheckpoint = chain.forkChoice.getFinalizedCheckpoint(); | ||
| chain.logger.info(POS_PANDA_FINALIZED_BANNER); |
There was a problem hiding this comment.
@philknows yea this banner can go, or it can be super minimal, or we can add it later now we have a spot
dapplion
reviewed
May 4, 2022
| blockHash: mergeBlockHash, | ||
| executionHash: mergeExecutionHash, | ||
| powHash: mergePowHash, | ||
| }); |
There was a problem hiding this comment.
To keep this function cleaner we can move this logic into a function such as
if (onBlockPrecachedData.isMergeTransitionBlock) {
logOnPowBlock(chain, block)
} else if (!chain.forkChoice.getMergeTransitionFinalized() && chain.forkChoice.getMergeTransitionFinalized()) {
logOnPowBlockFinalized(chain, block)
}
dapplion
reviewed
May 4, 2022
| // in forkchoice! Its now PoS all the way!!!! | ||
| if (onBlockPrecachedData.isMergeTransitionBlock) { | ||
| const mergeBlock = block.message as bellatrix.BeaconBlock; | ||
| const mergeBlockHash = toHexString(ssz.bellatrix.BeaconBlock.hashTreeRoot(mergeBlock)); |
There was a problem hiding this comment.
This is not safe, you must use config to get the block type from the slot. There will be more forks after bellatrix
dapplion
reviewed
May 4, 2022
| @@ -0,0 +1,36 @@ | |||
| /* eslint-disable no-useless-escape */ | |||
| export const POS_PANDA_TRANSITIONING_BANNER = ` | |||
There was a problem hiding this comment.
Just put this in utils, not sure we will have that many banners hahaha
dapplion
reviewed
May 4, 2022
| ) { | ||
| const finalizedBlock = this.getFinalizedBlock(); | ||
| if (finalizedBlock.executionStatus !== ExecutionStatus.PreMerge) { | ||
| this.isMergeTransitionFinalized = true; |
There was a problem hiding this comment.
Is this strategy sufficient to not log again after restarting?
There was a problem hiding this comment.
Moving this logic here just for the log doesn't feel great. Is lighthouse doing the same?
There was a problem hiding this comment.
lighthouse doesn't seem to be logging finalized, i wanted to add this on my own.
on restart, if if it starts post merge, isMergeTransitionFinalized should be set true on init and shouldn't log.
There was a problem hiding this comment.
There was a problem hiding this comment.
While I know it's not a lot, I'm not super keen on adding some complexity for a non critical item like this. While we (coredevs) celebrate finalization users may only care about the merge block, and it's very easy to detect once. Let me know what you think.
There was a problem hiding this comment.
I also have no solicited any banner work for finalized at this time.
There was a problem hiding this comment.
ok no issues, removed 🙂
dapplion
reviewed
May 5, 2022
| @@ -290,5 +288,10 @@ export async function verifyBlockStateTransition( | |||
| }); | |||
| } | |||
|
|
|||
| return {postState, executionStatus}; | |||
| const isMergeTransitionBlock = | |||
There was a problem hiding this comment.
Why has this variable been moved to verifyBlock?
There was a problem hiding this comment.
need preState to evaluate it, on postState it was always coming as false as on merge block poststate will have merge complete
dapplion
reviewed
May 5, 2022
g11tech
force-pushed
the
g11tech/merge-transition-banner
branch
from
4f1b7c1 to
16d4fae
Compare
dapplion
reviewed
May 12, 2022
| return {postState, executionStatus}; | ||
| } | ||
|
|
||
| function logOnPowBlock(chain: VerifyBlockModules, block: allForks.SignedBeaconBlock): void { |
There was a problem hiding this comment.
I think it would be safer to have
function logOnPowBlock(chain: VerifyBlockModules, block: bellatrix.BeaconBlock): voidand do the casting right below isMergeTransitionBlock where it's clear that the casting is safe
dapplion
reviewed
May 12, 2022
| @@ -43,14 +44,13 @@ export async function verifyBlock( | |||
| ): Promise<FullyVerifiedBlock> { | |||
| const parentBlock = verifyBlockSanityChecks(chain, partiallyVerifiedBlock); | |||
|
|
|||
| const {postState, executionStatus} = await verifyBlockStateTransition(chain, partiallyVerifiedBlock, opts); | |||
| const processedBlockData = await verifyBlockStateTransition(chain, partiallyVerifiedBlock, opts); | |||
There was a problem hiding this comment.
Why this change? Being explicit and not doing the spread feels more readable in my opinion
dapplion
approved these changes
May 12, 2022
dapplion
added a commit
that referenced
this pull request
May 30, 2022
* New metric filtering missed blocks (#3927) * Log block delay second * Add elappsedTimeTillBecomeHead metric * Add 'till become head' metric to dashboard * chore: correct the metric name to elapsedTimeTillBecomeHead * Add and use secFromSlot to clock * Track block source * Revert "Track block source" This reverts commit 5fe6220. * Update bucket values * Limit how old blocks are tracked in elapsedTimeTillBecomeHead * Simplify secFromSlot Co-authored-by: dapplion <[email protected]> * Fix the terminal validations of the merge block (#3984) * Fix the terminal validations of the merge block * activate merge transition block spec tests * some comments to explain the merge block validations movement * Extend error messages when voluntary exit errors because of present of lockfile (#3935) * Extend error and Clean up * Only showing the message to use --force to override in case of voluntary exit * Simplify gitData and version guessing (#3992) Don't print double slash in version string Dont add git-data.json to NPM releases Write git-data.json only in from source docker build Remove numCommits Test git-data.json generation from within the test Move comment Revert "Dont add git-data.json to NPM releases" This reverts commit 5fe2d38. Simplify gitData and version guessing Run cmd * Activate ex-ante fork-choice spec tests (#4003) * Prepare custom version on next release (#3990) * Prepare custom version on next release * Test in branch * Don't set version in advance * Remove --canary flag * Change and commit version * Setup git config * Revert temp changes * Lightclient e2e: increase validator client (#4006) * Bump to v0.37.0 nightly builds (#4013) * Guarantee full spec tests coverage (#4012) * Ensure all spec tests are run * Fix general bls tests * Improve docs of specTestIterator * Fix fork_choice tests * Remove Check spec tests step * Add merge transition/finalization banners (#3963) * Add merge transition/finalization banners * fix signatures * Benchmark initial sync (#3995) * Basic range sync perf test * Benchmark initial sync * Add INFURA_ETH2_CREDENTIALS to benchmark GA * Download test cache file from alternative source * Re-org beforeValue and testCase helpers * Break light-client - state-transition test dependency * Revert adding downloadTestCacheFile * Download files from a Github release * Clarify #3977 with unbounded uint issue (#4018) * Update mainnet-shadow-5 configs (#4021) * Bump moment from 2.29.1 to 2.29.2 (#3901) Bumps [moment](https://github.com/moment/moment) from 2.29.1 to 2.29.2. - [Release notes](https://github.com/moment/moment/releases) - [Changelog](https://github.com/moment/moment/blob/develop/CHANGELOG.md) - [Commits](moment/moment@2.29.1...2.29.2) --- updated-dependencies: - dependency-name: moment dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Implement support for validator next-epoch proposer duties (#3782) * Implementation to be able to get block proposer an epoch ahead - still need optimization * revert changes made to waitForSlot * caching the results of computing future proposers. Also extended test * using effectiveBalanceIncrements from state instead of recomputing it * fix lint errors * revert check not needed in getBeaconProposer * Update tests to include assertion messages * Move caching of next proposer duties to BeaconChain class * Delete the block proposer previously cached when next proposer was requested at current epoch * moved next epoch proposers from the chain to the state * Compute next proposer on demand and cache * Fix lint errors * update implementation to work with changes from master * caching epoch seed in context so that getNextEpochBeaconProposer can be independent of state * Revert "caching epoch seed in context so that getNextEpochBeaconProposer can be independent of state" This reverts commit 02a722a. * caching epoch seed in context so that getNextEpochBeaconProposer can be independent of state * removing the need to delete from nextEpochProposers in call to getBeaconProposer * no need to recompute currrentProposerSeed again * Revert "no need to recompute currrentProposerSeed again" This reverts commit b6b1b8c. * removed empty file left after fixing merge conflicts * remove some unnecessary variable from the epoch context. * add some comments * Fix lint * import from the right location * Review PR * Merge imports * Delete get proposers api impl test * Remove duplicated comment Co-authored-by: dapplion <[email protected]> * Extend timeout for gitData unit test (#4026) * Fix readAndGetGitData (#4025) * Ensure light client update is in a single period (#4029) * Handle merge block fetch error (#4016) * Handle merge block fetch error * Log errors on fetch errors for terminal pow * docs: Update nodeJS minimum requirement (#4037) * Remove child_process call in gitData before step (#4033) * Oppool aggregates use BitArray only for set logic (#4034) * Use BitArrays for aggregate merging * Test intersectUint8Arrays * Review PR * Update tests * Remove un-used code * Modify gossipsub params following consensus spec v1.1.10 (#4011) * Modify gossipsub params following consensus spec v1.1.10 * Specify GOSSIPSUB_HEARTBEAT_INTERVAL as a constant * Throw a more informative error on invalid keystore (#4022) * Throw a more informative error on invalid keystore * Make error more descriptive * Use template string * Update keys.ts * Update keys.ts Co-authored-by: Lion - dapplion <[email protected]> * Ignore gossip AggregateAndProof if aggregate is seen (#4019) * Ignore gossip AggregateAndProof if aggregate is seen * Check for non-strict superset of seen attestation data * Fix validateGossipAggregateAndProof benchmark test * Fix import * Ultilize intersectUint8Arrays() * Implement SeenContributionAndProof.participantsKnown * Add metrics to seen cache * Add perf tests * Change method name to isSuperSetOrEqual() * Refactor metric names * Specify lerna exact version for release-nightly workflow (#4049) * Add ropsten network (#4051) * Force all packages to be versioned for exact (#4052) * Update discv5 to v0.7.1 (#4044) * Add ability to update the fee recipient for execution via beacon and/or validator defaults (#3958) * Add and use a default fee recipient for a validator process * transfer the proposer cache to beacon chain * mock chain fixes * test and perf fixes * fee recipient validation change * track and use free recipient as string instead of ExecutionAddress * fix unit test * fix merge test * use dummy address * refac and add proposer cache pruning * tests for beacon proposer cache * merge interop fee recipient check * fix the optional * feeRecipient confirmation and small refac * add the missing map * add flag to enable strict fee recipient check * Small refactor to setup merge for ropsten using baked in configs (#4053) * Issue advance fcU for builing the EL block (#3965) rebaseing to the refactored prepare beacon proposer refac payload id cache as separate class and add pruning issue payload fcus if synced rename issueNext.. to maybeIssueNext... * Simplify release process (#4030) * Simplify release process * Remove old postrelease script * Add lerna version check * Tweak RELEASE.md * Add force-publish to lerna version command * Update the proposer boost percentage to 40% (#4055) * ESM Support (#3978) * ESM changes * Fix root lodestar script * Fix some linter errors * trying directly re-exporting under an alias from networks module * Fix types exports * Fix more linter errors * Fix spec test download * Update bls to 7.1.0 * Fix spec tests * temp reverting eslint parser option to 10 and disabling the check of .js file extenstion. Should fix lint errors * temp commented out file-extension-in-import * Disable readme checks * Fix check-build * Fix params e2e tests * Bump @chainsafe/threads * Bump bls to v7.1.1 * Add timeouts after node initialization but before sim test run * Tweak timeouts * Tweak timeout * Tweak sim merge timeout * Tweak sim merge timeout * Tweak sim merge timeout * Tweak sim merge timeout * Add more timeouts * Add another timeout * Fix linter errors * Fix some tests * Fix some linter errors and spec tests * Fix benchmarks * Fix linter errors * Update each bls dependency * Tweak timeouts * Add another timeout * More timeouts * Fix bls pool size * Set root package.json to ESM * Remove old linter comment * Revert "Set root package.json to ESM" This reverts commit 347b0fd. * Remove stray file (probably old) * Undo unnecessary diff * Add comment on __dirname replacement * Import type @chainsafe/bls/types * Use lodestar path imports * Revert multifork to lodestar package * Format .mocharc.yaml * Use same @chainsafe/as-sha256 version * Fix lodash path imports * Use src instead of lib * Load db metrics * Remove experimental-specifier-resolution * Remove lodestat/chain export * Add stray missing file extension * Revert ValidatorDir changes * Fix stray missing file extensions * Fix check-types Co-authored-by: Dadepo Aderemi <[email protected]> Co-authored-by: dapplion <[email protected]> * chore(release): v0.37.0-beta.0 * Bump to v0.37.0 Co-authored-by: tuyennhv <[email protected]> Co-authored-by: g11tech <[email protected]> Co-authored-by: dadepo <[email protected]> Co-authored-by: Cayman <[email protected]> Co-authored-by: Phil Ngo <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: g11tech <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation
This PR adds a merge transition/merge finalization banner capabilities plus additional helpful log around merge block/finalization.
Also fixes the flag of
isMergeTransitionBlockon the flags passed to the forkchoice. Earlier it was being evaluated on apostStatewhich was problematic, because even on the merge transition block, postState would already have show the mergeCompleted. Fixed it to evaluate onpreState.PS: Banners could be updated later as per contributions to #3961 (which is a separate issue of having the banner art contributions, hence this PR doesn't close that issue, but only enables it)