Repository navigation
Conversation
github-actions
Bot
added
the
branch-invalid
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## oisin/strict-lock-verification #4819 +/- ##
==================================================================
+ Coverage 67.81% 67.89% +0.07%
==================================================================
Files 255 255
Lines 33808 33914 +106
==================================================================
+ Hits 22928 23026 +98
- Misses 10879 10887 +8
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| func (l Lock) VerifyContents() error { | ||
| if err := l.VerifyAddresses(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := l.verifyBuilderRegistrationContents(); err != nil { | ||
| return errors.Wrap(err, "verify builder registrations") | ||
| } | ||
|
|
||
| if err := l.verifyDepositData(); err != nil { | ||
| return errors.Wrap(err, "verify deposit data") | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Some of those are really slow and at worst can slow our startup by a couple of seconds. I'm not convinced we have to check such kinds of stuff on charon run? What do we gain from e.g. verifying deposit data signatures on charon run?
- We have not created those signatures on charon run
- We are not using the deposit data on charon run
- We are not assuring anything with that
I'm fine if we want to verify those on deposit commands, but here it seems irrelevant. When a relevant code path, would that be from Charon or external, needs to use that data it can verify the signature itself, but we shouldn't do all such checks at every single charon run...
This statement kinda holds for the whole PR (and some of the other ones), however, the sig verification of deposit data has the highest hit on performance from what I've reviewed so far, with arguably no benefit.
There was a problem hiding this comment.
Fair point, agreed. I've cut it back to creation only. Contents are now checked in create cluster and at the end of a fresh DKG, where charon has just made those deposit and registration signatures, so it's the one place a bad lock can be caught before it's distributed. The load path (charon run, exits, other commands), the edit protocols and append-validators no longer check contents at all, so startup cost is unchanged. The definition address check in DKG stays (cheap, no BLS). Happy to add it to the deposit commands later if that's useful.
Adds Lock.VerifyContents: deposit data must match the distributed validator pubkey, the definition withdrawal credentials and deposit amounts, and carry a valid BLS signature; builder registrations must match the distributed validator pubkey and the definition fee recipient; addresses must be valid (Definition.VerifyAddresses). Only enforced where charon creates a lock from scratch (create cluster and a fresh DKG), plus address checks on DKG definitions. Loading an existing lock (run, exits, other commands), the edit protocols and appending validators don't check contents, so startup isn't slowed by BLS verifying every deposit signature. category: feature ticket: none
3081c4d to
09a67de
Compare
|
Adds
Lock.VerifyContents. It checks that deposit data matches the DV pubkey, the definition withdrawal credentials and the deposit amounts, and has a valid BLS signature. It checks that builder registrations match the DV pubkey and the definition fee recipient, and that addresses are valid (Definition.VerifyAddresses).create cluster, fresh DKG) and on DKG definitions (bypassable with--no-verify).run, exits, deposit and feerecipient commands), so a running node never fails to restart. Also warn only in the edit protocols and DKG append mode, since their output inherits the old lock's contents.Make it an error in a later release.
category: feature
ticket: none