Conversation
DiogoSantoss
added
the
do not merge
|
There was a problem hiding this comment.
Pull request overview
This PR introduces cluster definition v1.11.0 as the new default schema version, aiming to support variable-sized signatures (e.g., Safe multisig) and address DKG failures caused by signature length assumptions during hash verification (#4265).
Changes:
- Add and enable v1.11.0 as the default/supported cluster definition version.
- Introduce v1.11 SSZ hashing + JSON marshal/unmarshal paths, including support for signature byte-lists.
- Add new v1.11 golden testdata for definition + lock encoding.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
cluster/version.go |
Sets v1.11.0 as default and adds it to supported versions. |
cluster/ssz.go |
Adds v1.11 SSZ hashing and increases signature flexibility via ByteList hashing. |
cluster/operator.go |
Updates operator signature SSZ tags to reflect variable-sized signatures. |
cluster/lock.go |
Enables v1.11 for lock marshal/unmarshal routing. |
cluster/definition.go |
Adds signature length validation + v1.11 JSON marshal/unmarshal routing. |
cluster/cluster_test.go |
Updates encode test expectations for v1.11 behavior. |
cluster/testdata/cluster_definition_v1_11_0.json |
New v1.11 definition golden file. |
cluster/testdata/cluster_lock_v1_11_0.json |
New v1.11 lock golden file. |
app/monitoringapi.go |
Whitespace-only change in error-handling branches. |
Comments suppressed due to low confidence (1)
cluster/definition.go:267
- For v1.11 Safe/multisig signatures,
o.ConfigSignature/o.ENRSignaturemay be longer than 65 bytes. In that caseverifySig(ECDSA recover) will return an error, and the current code returns immediately without attempting the ERC-1271 (VerifySmartContractBasedSignature) fallback. To support variable-length contract signatures, treat non-65-byte signatures (or recover errors) as a non-match and fall back to ERC-1271 instead of returning the error.
// Check that we have a valid config signature for each operator.
if ok, err := verifySig(o.Address, operatorConfigHashDigest, o.ConfigSignature); err != nil {
return err
} else if !ok {
// Check ERC-1271 signature
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Field (2) 'ConfigSignature' ByteList[384] | ||
| if err := putByteList(hh, o.ConfigSignature, sszMaxSignature, "config_signature"); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Field (3) 'ENRSignature' ByteList[384] | ||
| if err := putByteList(hh, o.ENRSignature, sszMaxSignature, "enr_signature"); err != nil { | ||
| return err |
There was a problem hiding this comment.
There’s no test exercising the v1.11 change that allows operator/creator signatures > 65 bytes (e.g., 130-byte Safe threshold=2 signatures) through hashing and VerifyHashes(). Adding a regression test that loads/constructs a v1.11 definition with >65-byte signatures and asserts SetDefinitionHashes/VerifyHashes succeed would help prevent reintroducing the original "bytes too long" failure.
| // Validate signature lengths for v1.11.0+ (Safe multisig support) | ||
| if err := d.validateSignatureLengths(); err != nil { | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
VerifySignatures calls d.validateSignatureLengths() inside the per-operator loop, but validateSignatureLengths itself iterates over all operators and the creator. This makes verification O(n²) and also means signature-length validation is skipped entirely when operators are unsigned (e.g., creator-only signatures). Consider validating once up-front (before the loop / before any early-continues) and only over the relevant signatures.
| if err := deposit.VerifyDepositAmounts(def.DepositAmounts, def.Compounding); err != nil { | ||
| return Definition{}, errors.Wrap(err, "invalid deposit amounts") | ||
| } |
There was a problem hiding this comment.
unmarshalDefinitionV1x11 calls deposit.VerifyDepositAmounts(def.DepositAmounts, def.Compounding) before def is populated, so it always validates zero values and can miss invalid deposit amounts/compounding combinations. This should validate defJSON.DepositAmounts and defJSON.Compounding (or validate after constructing the Definition).
|
|
||
| // Field (14) 'Compounding' bool | ||
| hh.PutBool(d.Compounding) | ||
|
|
There was a problem hiding this comment.
hashDefinitionV1x11 does not append ConfigHash when configOnly == false (unlike hashDefinitionV1x5to9, which includes ConfigHash as the final field in the definition hash). This changes the definition-hash schema for v1.11 beyond the signature-length tweak and may break compatibility/verification expectations. If not intentional, include ConfigHash (Bytes32) in the v1.11 definition hash when !configOnly.
| // Field (15) 'ConfigHash' Bytes32 (only for full definition hash) | |
| if !configOnly { | |
| hh.PutBytes(d.ConfigHash[:]) | |
| } |
1 participant
New cluster definition
v1.11to support variable-sized signatures.category: feature
ticket: #4265