-
Notifications
You must be signed in to change notification settings - Fork 216
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
fix EIP-7044 implementation when using batch verification #5953
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
In #5120, EIP-7044 support got added to the state transition function to force `CAPELLA_FORK_VERSION` to be used when validiting `VoluntaryExit` messages, irrespective of their `epoch`. In #5637, similar logic was added when batch verifying BLS signatures, which is used during gossip validation (libp2p gossipsub, and req/resp). However, that logic did not match the one introduced in #5120, and only uses `CAPELLA_FORK_VERSION` when a `VoluntaryExit`'s `epoch` was set to a value `>= CAPELLA_FORK_EPOCH`. Otherwise, `BELLATRIX_FORK_VERSION` would still be used when validating `VoluntaryExit`, e.g., with `epoch` set to `0`, as is the case in this Holesky block: - https://holesky.beaconcha.in/slot/1076985#voluntary-exits Extracting the correct logic from #5120 into a function, and reusing it when verifying BLS signatures fixes this issue, and also leverages the exhaustive EF test suite that covers the (correct) #5120 logic. This fix only affects networks that have EIP-7044 applied (post-Deneb). Without the fix, Deneb blocks with a `VoluntaryExit` with `epoch` set to `< CAPELLA_FORK_EPOCH` incorrectly fail to validate despite being valid. Incorrect blocks that contain a malicious `VoluntaryExit` with `epoch` set to `< CAPELLA_FORK_EPOCH` and signed using `BELLATRIX_FORK_VERSION` _would_ pass the BLS verification stage, but subsequently fail the state transition logic. Such blocks would still correctly be labeled invalid.
Tested on Holesky, the block that previously failed to apply now applies fine, and can sync past it. |
arnetheduck
reviewed
Feb 25, 2024
arnetheduck
approved these changes
Feb 25, 2024
Tested once more that the revised logic also still syncs fine on Holesky. |
arnetheduck
reviewed
Feb 25, 2024
arnetheduck
pushed a commit
that referenced
this pull request
Feb 25, 2024
In #5120, EIP-7044 support got added to the state transition function to force `CAPELLA_FORK_VERSION` to be used when validiting `VoluntaryExit` messages, irrespective of their `epoch`. In #5637, similar logic was added when batch verifying BLS signatures, which is used during gossip validation (libp2p gossipsub, and req/resp). However, that logic did not match the one introduced in #5120, and only uses `CAPELLA_FORK_VERSION` when a `VoluntaryExit`'s `epoch` was set to a value `>= CAPELLA_FORK_EPOCH`. Otherwise, `BELLATRIX_FORK_VERSION` would still be used when validating `VoluntaryExit`, e.g., with `epoch` set to `0`, as is the case in this Holesky block: - https://holesky.beaconcha.in/slot/1076985#voluntary-exits Extracting the correct logic from #5120 into a function, and reusing it when verifying BLS signatures fixes this issue, and also leverages the exhaustive EF test suite that covers the (correct) #5120 logic. This fix only affects networks that have EIP-7044 applied (post-Deneb). Without the fix, Deneb blocks with a `VoluntaryExit` with `epoch` set to `< CAPELLA_FORK_EPOCH` incorrectly fail to validate despite being valid. Incorrect blocks that contain a malicious `VoluntaryExit` with `epoch` set to `< CAPELLA_FORK_EPOCH` and signed using `BELLATRIX_FORK_VERSION` _would_ pass the BLS verification stage, but subsequently fail the state transition logic. Such blocks would still correctly be labeled invalid.
etan-status
added a commit
that referenced
this pull request
Feb 25, 2024
When trying to sign `VoluntaryExit` via keymanager API, the logic is not yet aware of EIP-7044 (part of Deneb). This patch adds missing EIP-7044 support to the keymanager API as well. As part of this, the VC needs to become aware about: - `CAPELLA_FORK_VERSION`: To correctly form the EIP-7044 signing domain. The fork schedule does not indicate which of the results, if any, corresponds to Capella. - `CAPELLA_FORK_EPOCH`: To detect whether Capella was scheduled. If a BN does not have it in its config while other BNs have it, this leads to a log if Capella has not activated yet, or marks the BN as incompatible if Capella already activated. - `DENEB_FORK_EPOCH`: To check whether EIP-7044 logic should be used. Related PRs: - #5120 added support for processing EIP-7044 `VoluntaryExit` messages as part of the state transition functions (tested by EF spec tests). - #5953 synced the support from #5120 to gossip validation. - #5954 added support to the `nimbus_beacon_node deposits exit` command. - #5956 contains an alternative generic version of `VCForkConfig`.
arnetheduck
pushed a commit
that referenced
this pull request
Feb 26, 2024
* add EIP-7044 support to keymanager API When trying to sign `VoluntaryExit` via keymanager API, the logic is not yet aware of EIP-7044 (part of Deneb). This patch adds missing EIP-7044 support to the keymanager API as well. As part of this, the VC needs to become aware about: - `CAPELLA_FORK_VERSION`: To correctly form the EIP-7044 signing domain. The fork schedule does not indicate which of the results, if any, corresponds to Capella. - `CAPELLA_FORK_EPOCH`: To detect whether Capella was scheduled. If a BN does not have it in its config while other BNs have it, this leads to a log if Capella has not activated yet, or marks the BN as incompatible if Capella already activated. - `DENEB_FORK_EPOCH`: To check whether EIP-7044 logic should be used. Related PRs: - #5120 added support for processing EIP-7044 `VoluntaryExit` messages as part of the state transition functions (tested by EF spec tests). - #5953 synced the support from #5120 to gossip validation. - #5954 added support to the `nimbus_beacon_node deposits exit` command. - #5956 contains an alternative generic version of `VCForkConfig`. * address reviewer feedback: letter case, module location, double lookup --------- Co-authored-by: cheatfate <[email protected]> * Update beacon_chain/rpc/rest_constants.nim * move `VCRuntimeConfig` back to `rest_types` --------- Co-authored-by: cheatfate <[email protected]> * fix `getForkVersion` helper --------- Co-authored-by: cheatfate <[email protected]>
zah
pushed a commit
that referenced
this pull request
Feb 27, 2024
In #5120, EIP-7044 support got added to the state transition function to force `CAPELLA_FORK_VERSION` to be used when validiting `VoluntaryExit` messages, irrespective of their `epoch`. In #5637, similar logic was added when batch verifying BLS signatures, which is used during gossip validation (libp2p gossipsub, and req/resp). However, that logic did not match the one introduced in #5120, and only uses `CAPELLA_FORK_VERSION` when a `VoluntaryExit`'s `epoch` was set to a value `>= CAPELLA_FORK_EPOCH`. Otherwise, `BELLATRIX_FORK_VERSION` would still be used when validating `VoluntaryExit`, e.g., with `epoch` set to `0`, as is the case in this Holesky block: - https://holesky.beaconcha.in/slot/1076985#voluntary-exits Extracting the correct logic from #5120 into a function, and reusing it when verifying BLS signatures fixes this issue, and also leverages the exhaustive EF test suite that covers the (correct) #5120 logic. This fix only affects networks that have EIP-7044 applied (post-Deneb). Without the fix, Deneb blocks with a `VoluntaryExit` with `epoch` set to `< CAPELLA_FORK_EPOCH` incorrectly fail to validate despite being valid. Incorrect blocks that contain a malicious `VoluntaryExit` with `epoch` set to `< CAPELLA_FORK_EPOCH` and signed using `BELLATRIX_FORK_VERSION` _would_ pass the BLS verification stage, but subsequently fail the state transition logic. Such blocks would still correctly be labeled invalid.
zah
pushed a commit
that referenced
this pull request
Feb 27, 2024
* add EIP-7044 support to keymanager API When trying to sign `VoluntaryExit` via keymanager API, the logic is not yet aware of EIP-7044 (part of Deneb). This patch adds missing EIP-7044 support to the keymanager API as well. As part of this, the VC needs to become aware about: - `CAPELLA_FORK_VERSION`: To correctly form the EIP-7044 signing domain. The fork schedule does not indicate which of the results, if any, corresponds to Capella. - `CAPELLA_FORK_EPOCH`: To detect whether Capella was scheduled. If a BN does not have it in its config while other BNs have it, this leads to a log if Capella has not activated yet, or marks the BN as incompatible if Capella already activated. - `DENEB_FORK_EPOCH`: To check whether EIP-7044 logic should be used. Related PRs: - #5120 added support for processing EIP-7044 `VoluntaryExit` messages as part of the state transition functions (tested by EF spec tests). - #5953 synced the support from #5120 to gossip validation. - #5954 added support to the `nimbus_beacon_node deposits exit` command. - #5956 contains an alternative generic version of `VCForkConfig`. * address reviewer feedback: letter case, module location, double lookup --------- Co-authored-by: cheatfate <[email protected]> * Update beacon_chain/rpc/rest_constants.nim * move `VCRuntimeConfig` back to `rest_types` --------- Co-authored-by: cheatfate <[email protected]> * fix `getForkVersion` helper --------- Co-authored-by: cheatfate <[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.
In #5120, EIP-7044 support got added to the state transition function to force
CAPELLA_FORK_VERSION
to be used when validitingVoluntaryExit
messages, irrespective of theirepoch
.In #5637, similar logic was added when batch verifying BLS signatures, which is used during gossip validation (libp2p gossipsub, and req/resp). However, that logic did not match the one introduced in #5120, and only uses
CAPELLA_FORK_VERSION
when aVoluntaryExit
'sepoch
was set to a value>= CAPELLA_FORK_EPOCH
. Otherwise,BELLATRIX_FORK_VERSION
would still be used when validatingVoluntaryExit
, e.g., withepoch
set to0
, as is the case in this Holesky block:Extracting the correct logic from #5120 into a function, and reusing it when verifying BLS signatures fixes this issue, and also leverages the exhaustive EF test suite that covers the (correct) #5120 logic.
This fix only affects networks that have EIP-7044 applied (post-Deneb).
Without the fix, Deneb blocks with a
VoluntaryExit
withepoch
set to< CAPELLA_FORK_EPOCH
incorrectly fail to validate despite being valid.Incorrect blocks that contain a malicious
VoluntaryExit
withepoch
set to< CAPELLA_FORK_EPOCH
and signed usingBELLATRIX_FORK_VERSION
would pass the BLS verification stage, but subsequently fail the state transition logic. Such blocks would still correctly be labeled invalid.