Assert equal params with only whitelisted keys - #4062
Merged
Conversation
Contributor
Performance Report✔️ no performance regression detected Full benchmark results
|
dadepo
reviewed
May 25, 2022
dadepo
reviewed
May 25, 2022
dadepo
reviewed
May 25, 2022
| * So this check only compares a specific list of parameters that are consensus critical, ignoring the rest. Typed | ||
| * config and preset ensure new parameters are leveled critical or ignore, faciliating manteinance of the list. | ||
| */ | ||
| export function assertEqualParams(localConfig: IChainConfig, externalSpecJson: Record<string, string>): void { |
Contributor
There was a problem hiding this comment.
it would also be nice to update the tests for this logic with this new requirement
wemeetagain
reviewed
May 25, 2022
Contributor
|
got this (lighthouse BN): May-25 18:11:16.064[] info: Genesis available
✖ Error: Not equal BeaconParams
INACTIVITY_PENALTY_QUOTIENT_BELLATRIX not defined in external config
INACTIVITY_PENALTY_QUOTIENT_BELLATRIX different value: 16777216 != undefined
MIN_SLASHING_PENALTY_QUOTIENT_BELLATRIX not defined in external config
MIN_SLASHING_PENALTY_QUOTIENT_BELLATRIX different value: 32 != undefined
PROPORTIONAL_SLASHING_MULTIPLIER_BELLATRIX not defined in external config
PROPORTIONAL_SLASHING_MULTIPLIER_BELLATRIX different value: 3 != undefined
MAX_BYTES_PER_TRANSACTION not defined in external config
MAX_BYTES_PER_TRANSACTION different value: 1073741824 != undefined
MAX_TRANSACTIONS_PER_PAYLOAD not defined in external config
MAX_TRANSACTIONS_PER_PAYLOAD different value: 1048576 != undefined
BYTES_PER_LOGS_BLOOM not defined in external config
BYTES_PER_LOGS_BLOOM different value: 256 != undefined
MAX_EXTRA_DATA_BYTES not defined in external config
MAX_EXTRA_DATA_BYTES different value: 32 != undefined
at assertEqualParams (file:///usr/app/packages/validator/src/util/params.ts:65:11)
at Function.initializeFromBeaconNode (file:///usr/app/packages/validator/src/validator.ts:161:5)
at processTicksAndRejections (node:internal/process/task_queues:96:5) |
Contributor
|
prsym: May-25 18:16:39.480[] info: Lodestar network=kiln, version=v0.37.0/c165a5c, commit=c165a5cf7f56b34d37492942db9b094d6ee3c0b5
May-25 18:16:39.571[] info: Decrypted 6 local keystores
May-25 18:16:39.571[] info: 0xb8720175cf41e0a67313224e85f86ad6f2ee0f775717646b21fb373eafeb015cdf88721387a989bfe71676405f01552c
May-25 18:16:39.571[] info: 0x93a8b44b4a6cee4611cf2b3efc689cf7ca9a6be3638fefe8ecf10da98b9f03e2bef867bf3a7d3ae77cd86a31c8d83cf5
May-25 18:16:39.572[] info: 0xb76d52873e0e1ebd2746af664a1ca015e5ff6c02287bf010b550123b752c31659c5207c70670ee34b07a7bb25b564529
May-25 18:16:39.572[] info: 0xa1da06aff8e369e5c871c4b63e930e8b5359f05591e6f336684f1e9ee8e952ed7541f082d9482f01d934501f3de58c1e
May-25 18:16:39.572[] info: 0xa3c9b8d51584e7f37c3bef871c9339eeb0ac805705a7aaf151becde5322edded237198ea798791d68b84444eb6dc0e24
May-25 18:16:39.572[] info: 0xb2fd9f2f484781147238cc0bc1cc80c0f4f05d28efd8813dcae7a5f8cde0ee2074dbca69fac9b25f9c4d87705732fd37
May-25 18:16:40.022[] info: Genesis available
✖ Error: Not equal BeaconParams
MAX_BYTES_PER_TRANSACTION not defined in external config
MAX_BYTES_PER_TRANSACTION different value: 1073741824 != undefined
MAX_TRANSACTIONS_PER_PAYLOAD not defined in external config
MAX_TRANSACTIONS_PER_PAYLOAD different value: 1048576 != undefined
BYTES_PER_LOGS_BLOOM not defined in external config
BYTES_PER_LOGS_BLOOM different value: 256 != undefined
MAX_EXTRA_DATA_BYTES not defined in external config
MAX_EXTRA_DATA_BYTES different value: 32 != undefined
at assertEqualParams (file:///usr/app/packages/validator/src/util/params.ts:65:11)
at Function.initializeFromBeaconNode (file:///usr/app/packages/validator/src/validator.ts:161:5)
at processTicksAndRejections (node:internal/process/task_queues:96:5)
at Object.validatorHandler [as handler] (file:///usr/app/packages/cli/src/cmds/validator/handler.ts:122:21) |
Contributor
Author
|
@g11tech thanks for checking! Makes sense, all of those params are for future forks so I will ignore them conditionally. |
Co-authored-by: Cayman <caymannava@gmail.com>
Contributor
Author
|
Not that easy lol, checked Lighthouse source and they manually declare which parameters to expose. The list is a bit random and can be updated at any point without notice. What should we do? |
Contributor
Author
|
Changed code to only consider keys that are:
lodestar/packages/validator/src/util/params.ts Lines 45 to 52 in d3a460f @g11tech tests work |
g11tech
approved these changes
May 26, 2022
wemeetagain
approved these changes
May 27, 2022
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
4 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.
Motivation
Lodestar validator client asserts that the beacon node is connecting to has the same configuration. This check is very important to exit early in case of misconfiguration. However, in the past it has broke interoperability with other clients due to the checks being too strict on params that are not guaranteed to be the same.
Latest consensus release changed
SHARDING_FORK_VERSION. We don't know when each client will update this value since it doesn't matter now.https://github.com/ethereum/consensus-specs/pull/2896/files#diff-fa42a5809dc8042fed427fd9d2d54c2595789c5cad751b03a8d118871285c26fL48
We need a more resilient check that only looks are "relevant" keys, and ignores the ones that are likely to change or not critical for interoperability.
Description
Assert equal params with only whitelisted keys.
So this check only compares a specific list of parameters that are consensus critical, ignoring the rest. Typed config and preset ensure new parameters are leveled critical or ignore, facilitating maintenance of the list
Please @g11tech and @dadepo can you ensure that this PR doesn't break interop with other CL clients?