*: removed manifest pb #4130
*: removed manifest pb #4130
Conversation
pinebit
requested review from
DiogoSantoss,
KaloyanTanev,
OisinKyne and
Copilot
There was a problem hiding this comment.
Pull request overview
This PR removes the cluster/manifest package and cluster/manifestpb/v1 protobuf definitions, eliminating over 3,200 lines of code related to cluster manifest management. The codebase now exclusively uses the cluster.Lock structure instead of the protobuf-based manifestpb.Cluster.
Key changes:
- Replaced manifest-based cluster loading with direct
cluster.Lockusage throughout the codebase - Added
Charon-UUIDHTTP header to P2P relay connections for future load balancing - Deprecated
--manifest-fileCLI flag with backward compatibility note
Reviewed changes
Copilot reviewed 56 out of 56 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| p2p/bootnode.go | Added uuid parameter to relay functions and new Charon-Uuid HTTP header |
| cluster/load.go | New file implementing cluster lock loading and verification |
| app/disk.go | Simplified to use new cluster.LoadClusterLock function |
| app/app.go | Migrated from manifestpb.Cluster to cluster.Lock throughout |
| eth2util/keystore/keystore.go | Updated to use cluster.Lock instead of manifestpb.Cluster |
| cmd/*.go | Updated all commands to use cluster.LoadClusterLockAndVerify |
| cmd/combine/combine.go | Removed manifest loading support, now lock-only |
| docs/configuration.md | Marked --manifest-file flag as deprecated |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // LoadClusterLockAndVerify loads and verifies the cluster lock. Suitable for cmd tools. | ||
| func LoadClusterLockAndVerify(ctx context.Context, lockFilePath string) (*Lock, error) { | ||
| eth1Cl := eth1wrap.NewDefaultEthClientRunner("") |
There was a problem hiding this comment.
Is this meant to be empty ?
There was a problem hiding this comment.
It shall not be, but most of the commands that load cluster-lock do not have Execution layer address params in their flags, and therefore we cannot instantiate this client...
This may be another follow up change to go over ALL the commands and add EL address flags, then this won't be empty.
There was a problem hiding this comment.
What's worse, the previous code passed nil instead of eth1 client to VerifySignatures.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4130 +/- ##
==========================================
+ Coverage 56.43% 57.02% +0.58%
==========================================
Files 245 235 -10
Lines 31175 30346 -829
==========================================
- Hits 17595 17306 -289
+ Misses 11272 10834 -438
+ Partials 2308 2206 -102 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
KaloyanTanev
added
the
do not merge
This PR removes the entire
cluster/manifestpackage and its associated protobuf definitions (cluster/manifestpb/v1), eliminating over 3,200 lines of code.Key changes:
cluster/manifest/package including cluster management, mutations (add validators, node approval, legacy lock), and all helper functionsCluster-Uuidthat will be used in future for relays load balancing instead of Lock Hash. This is because edit commands modify lock hash.--manifest-file stringCLI flag.category: refactor
ticket: none