Skip to content

Make it possible to disable BLS verification for everything except Deposits (attempt 2)#7065

Merged
rolfyone merged 8 commits intoConsensys:masterfrom
Nashatyrev:feature/alter-deposit-bls-verifier-1
Apr 24, 2023
Merged

Make it possible to disable BLS verification for everything except Deposits (attempt 2)#7065
rolfyone merged 8 commits intoConsensys:masterfrom
Nashatyrev:feature/alter-deposit-bls-verifier-1

Conversation

@Nashatyrev
Copy link
Contributor

@Nashatyrev Nashatyrev commented Apr 21, 2023

PR Description

Replace AbstractBlockProcessor.blsVerifyDeposit static flag with BLSSignatureVerifier reference to be able to set the verifier to the working instance even if the BLS verification is globally disabled with BLSConstants.disableBLSVerification()

This is the second attempt to enable this feature.
The first one #6116 had an issue #6147 and had to be reverted: #6148

The problem was in inconsistent BLS.verify* methods behavior with respect to invalid BLSPublicKey: some methods returned false, some thrown BLSException. This PR also aligns these methods behavior: they are now all return false. Additionally catch(BLSException) clauses were added around verify* calls in AbstractBlockProcessor before (in revert PR #6148)

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

@Nashatyrev Nashatyrev marked this pull request as ready for review April 21, 2023 14:25
@tbenr
Copy link
Contributor

tbenr commented Apr 21, 2023

Today @rolfyone was actually working on the BLS verification area. You may need some coordination :)

@rolfyone
Copy link
Contributor

Today @rolfyone was actually working on the BLS verification area. You may need some coordination :)

I'll keep digging and see if theres reasons I can't do what I'm thinking, but I'd really like to be avoiding using BlsSignatureVerifier.SIMPLE, or somehow making sure we minimise individual verify calls where possible...

It's just that in production we seem to be spending a significant amount of time doing individual verifies rather than batching... I'll drill into uses of the single verify function when i get a sec...

Copy link
Contributor

@rolfyone rolfyone left a comment

Choose a reason for hiding this comment

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

LGTM

*/
public static boolean blsVerifyDeposit = true;
@VisibleForTesting
public static BLSSignatureVerifier depositSignatureVerifier = DEFAULT_DEPOSIT_SIGNATURE_VERIFIER;
Copy link
Contributor

Choose a reason for hiding this comment

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

are we planning on differentiating? why not just reference the default_deposit_signature_verifier?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DEFAULT_DEPOSIT_SIGNATURE_VERIFIER was intended as a default value for resetting the depositSignatureVerifier field after testing with disabled verifier. But you are right, it's probably an over-complication for the production code.

@rolfyone rolfyone merged commit fa09295 into Consensys:master Apr 24, 2023
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.

3 participants