Re-sign and re-share using proofs and EIP1271 signature #100
Conversation
pkgs/initiator/initiator.go
Outdated
| return nil, nil, nil, err | ||
| } | ||
| instanceIDField := zap.String("Ceremony ID", hex.EncodeToString(id[:])) | ||
| c.Logger.Info("🚀 Starting dkg ceremony", zap.String("initiator public key", string(pkBytes)), zap.Uint64s("operator IDs", ids), instanceIDField) |
There was a problem hiding this comment.
Should be "Starting resigning ceremony"
pkgs/wire/testdata/ceremony-2024-04-22--16-36-19.354/proofs.json
Outdated
Show resolved
Hide resolved
pkgs/initiator/initiator.go
Outdated
| dkgResults, err := parseDKGResultsFromBytes(resultsBytes, id) | ||
| if err != nil { | ||
| return nil, nil, nil, err | ||
| } | ||
| depositDataJson, keyshares, err := c.processDKGResultResponse(dkgResults, id, rMsg.Operators, rMsg.Resign.WithdrawalCredentials, rMsg.Resign.Fork, rMsg.Resign.Owner, rMsg.Resign.Nonce) | ||
| if err != nil { | ||
| return nil, nil, nil, err | ||
| } | ||
| c.Logger.Info("✅ verified master signature for ssv contract data") | ||
| if err := crypto.ValidateDepositDataCLI(depositDataJson, common.BytesToAddress(withdraw)); err != nil { | ||
| return nil, nil, nil, err | ||
| } | ||
| if err := crypto.ValidateKeysharesCLI(keyshares, rMsg.Operators, rMsg.Resign.Owner, rMsg.Resign.Nonce, depositDataJson.PubKey); err != nil { | ||
| return nil, nil, nil, err | ||
| } | ||
| // sending back to operators results | ||
| depositData, err := json.Marshal(depositDataJson) | ||
| if err != nil { | ||
| return nil, nil, nil, err | ||
| } | ||
| keysharesData, err := json.Marshal(keyshares) | ||
| if err != nil { | ||
| return nil, nil, nil, err | ||
| } | ||
| var proofsArray []*wire.SignedProof | ||
| for _, res := range dkgResults { | ||
| proofsArray = append(proofsArray, &wire.SignedProof{res.SignedProof}) //nolint:all | ||
| } | ||
| proofsData, err := json.Marshal(proofsArray) | ||
| if err != nil { | ||
| return nil, nil, nil, err | ||
| } | ||
| resultMsg := &wire.ResultData{ | ||
| Operators: ops, | ||
| Identifier: id, | ||
| DepositData: depositData, | ||
| KeysharesData: keysharesData, | ||
| Proofs: proofsData, | ||
| } | ||
| err = c.sendResult(resultMsg, ops, consts.API_RESULTS_URL, id) | ||
| if err != nil { | ||
| return nil, nil, nil, fmt.Errorf("🤖 Error storing results at operators %w", err) | ||
| } | ||
| return depositDataJson, keyshares, proofsArray, nil |
There was a problem hiding this comment.
The codes here after operators resigning are the same with StartDKG above (line 254-293), for better coding practice and future maintainance, maybe wrap it in another function like "processDKGResults"?
pkgs/initiator/initiator.go
Outdated
| rMsg := &wire.ResignMessage{ | ||
| Operators: ops, | ||
| Resign: &spec.Resign{ValidatorPubKey: proofs[0].Proof.ValidatorPubKey, | ||
| Fork: network.GenesisForkVersion(), |
There was a problem hiding this comment.
For now I think it can be genesis
// GenesisForkVersion returns the genesis fork version of the network.
func (n Network) GenesisForkVersion() phase0.Version {
switch n {
case PyrmontNetwork:
return phase0.Version{0, 0, 32, 9}
case PraterNetwork:
return phase0.Version{0x00, 0x00, 0x10, 0x20}
case HoleskyNetwork:
return phase0.Version{0x01, 0x01, 0x70, 0x00}
case MainNetwork:
return phase0.Version{0, 0, 0, 0}
default:
logrus.WithField("network", n).Fatal("undefined network")
return phase0.Version{}
}
}
pkgs/dkg/drand.go
Outdated
| return nil, fmt.Errorf("failed to sign deposit data with partial signature %w", err) | ||
| } | ||
| // Validate partial signature | ||
| if val := depositPartialSignature.VerifyByte(secretKeyBLS.GetPublicKey(), signingRoot[:]); !val { |
There was a problem hiding this comment.
We just signed, and we derive the public key from the secretKey...
We just validating the underlying BLS lib?
Not sure if this is smart or redundant...
We should have tests that verify this functionality
Maybe it is here to check some edge case on messages for BLS?
There was a problem hiding this comment.
Some double checks after debugging are left here. We can remove it of course. We have tests covering BLS work/decoding/encoding. If you have more ideas please feel free to add.
pkgs/dkg/drand.go
Outdated
| return nil, err | ||
| } | ||
| // Encrypt BLS share for SSV contract | ||
| encryptedShare, err := o.encryptFunc([]byte(secretKeyBLS.SerializeToHexStr())) |
There was a problem hiding this comment.
we just decrypted the secretKey... we already have the encrypted share
There was a problem hiding this comment.
same here as above
pkgs/dkg/drand.go
Outdated
| hash := eth_crypto.Keccak256([]byte(data)) | ||
| sigOwnerNonce := secretKeyBLS.SignByte(hash) | ||
| // Verify partial SSV owner + nonce signature | ||
| val := sigOwnerNonce.VerifyByte(secretKeyBLS.GetPublicKey(), hash) |
There was a problem hiding this comment.
again PK is derived from SK so not sure we need this check
There was a problem hiding this comment.
same here as above answered
pkgs/operator/operator.go
Outdated
| } | ||
| // Validate that incoming message has requested type | ||
| if signedMsg.Message.Type != reqMessageType { | ||
| return nil, fmt.Errorf("operator %d, received non-init message to init route, err: %v", operatorID, errors.New("not init message to init route")) |
There was a problem hiding this comment.
The error seems init specific...
but the from logic seems like it can be generic (not just init message)
| if cli_utils.Network != "now_test_network" { | ||
| ethnetwork = e2m_core.NetworkFromString(cli_utils.Network) | ||
| } | ||
| ethNetwork := e2m_core.NetworkFromString(cli_utils.Network) |
There was a problem hiding this comment.
we should make this fail on invalid input (now it fails silently with unknown outcomes)
| if err != nil { | ||
| logger.Fatal("😥 Failed to load participants: ", zap.Error(err)) | ||
| } | ||
| ethNetwork := e2m_core.NetworkFromString(cli_utils.Network) |
There was a problem hiding this comment.
same here as in initiator.go
pkgs/dkg/drand.go
Outdated
| return nil, fmt.Errorf("operator not found among resign operators: %d", o.ID) | ||
| } | ||
| if err := spec.ValidateResignMessage(r.Resign, spec.GetOperator(r.Operators, o.ID), r.Proofs[position]); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
we should wrap returned errors with fmt.Errorf so we can later know where things failed
(note: unnecessary for small funcs which only have a single potential error)
pkgs/dkg/drand.go
Outdated
| // Sign SSV owner + nonce | ||
| data := []byte(fmt.Sprintf("%s:%d", eth_common.Address(o.data.init.Owner).String(), o.data.init.Nonce)) | ||
| hash := eth_crypto.Keccak256([]byte(data)) | ||
| sigOwnerNonce := secretKeyBLS.SignByte(hash) | ||
| // Verify partial SSV owner + nonce signature | ||
| val := sigOwnerNonce.VerifyByte(secretKeyBLS.GetPublicKey(), hash) | ||
| if !val { | ||
| return fmt.Errorf("partial owner + nonce signature isnt valid %x", sigOwnerNonce.Serialize()) | ||
| } |
There was a problem hiding this comment.
why do we no longer need to verify the deposit & owner-nonce signatures here?
There was a problem hiding this comment.
@GalRogozinski suggested that its overkill to check sig after we created one, agree here, maybe its too much.
cli/initiator/resigning.go
Outdated
| id := crypto.NewID() | ||
| nonce := cli_utils.Nonce + uint64(i) | ||
| // Perform the resigning ceremony | ||
| depositData, keyShares, proofs, err := dkgInitiator.StartResigning(id, operatorIDs, arrayOfSignedProofs[i], ethNetwork, cli_utils.WithdrawAddress.Bytes(), cli_utils.OwnerAddress, nonce) |
There was a problem hiding this comment.
remind me if i'm wrong: wasn't the point of proofs to be able to verify the RSA signatures of operators as a confirmation that they approved that file to be used for resiging/resharing? @MatusKysel @pavelkrolevets
the spec has a ValidateSignedProofs func, maybe we can use it here right after loading the proofs?
There was a problem hiding this comment.
we have this check when we write result files to disk: https://github.com/ssvlabs/ssv-dkg/blob/re-sharing/pkgs/validator/validator.go#L135
There was a problem hiding this comment.
yes, but this is a different check on the new proofs right?
i think we should also validate the proofs before resigning and resharing, so that we wont start a process that we know is gonna be invalid because the proofs are, and we can let the user know of this issue early
There was a problem hiding this comment.
Ok, agree, added initial proofs validation at initiator before sending messages
There was a problem hiding this comment.
well done, just a few comments/questions
i noticed the build CI fails, can you please check? @pavelkrolevets
pavelkrolevets
changed the title
|
@moshe-blox @MatusKysel @GalRogozinski @radiken I merged |
pkgs/utils/utils.go
Outdated
| // JoinSets creates a set of two groups of operators. For example: [1,2,3,4] and [1,2,5,6,7] will return [1,2,3,4,5,6,7] | ||
| func JoinSets(oldOperators, newOperators []*spec.Operator) []*spec.Operator { | ||
| tmp := make(map[uint64]*spec.Operator) | ||
| var set []*spec.Operator | ||
| for _, op := range oldOperators { | ||
| if tmp[op.ID] == nil { | ||
| tmp[op.ID] = op | ||
| } | ||
| } | ||
| for _, op := range newOperators { | ||
| if tmp[op.ID] == nil { | ||
| tmp[op.ID] = op | ||
| } | ||
| } | ||
| for _, op := range tmp { | ||
| set = append(set, op) | ||
| } | ||
| return set | ||
| } | ||
|
|
||
| // GetCommonOldOperators returns an old set of operators disjoint from new set | ||
| // For example: old set [1,2,3,4,5]; new set [3,4,5,6,7]; returns [3,4,5] | ||
| func GetCommonOldOperators(oldOperators, newOperators []*spec.Operator) []*spec.Operator { | ||
| tmp := make(map[uint64]*spec.Operator) | ||
| var set []*spec.Operator | ||
| for _, op := range newOperators { | ||
| if tmp[op.ID] == nil { | ||
| tmp[op.ID] = op | ||
| } | ||
| } | ||
| for _, op := range oldOperators { | ||
| if tmp[op.ID] != nil { | ||
| set = append(set, op) | ||
| } | ||
| } | ||
| return set | ||
| } | ||
|
|
||
| // GetDisjointNewOperators returns a new set of operators disjoint from old set | ||
| // For example: old set [1,2,3,4,5]; new set [3,4,5,6,7]; returns [6,7] | ||
| func GetDisjointNewOperators(oldOperators, newOperators []*spec.Operator) []*spec.Operator { | ||
| tmp := make(map[uint64]*spec.Operator) | ||
| var set []*spec.Operator | ||
| for _, op := range newOperators { | ||
| if tmp[op.ID] == nil { | ||
| tmp[op.ID] = op | ||
| } | ||
| } | ||
| for _, op := range oldOperators { | ||
| if tmp[op.ID] != nil { | ||
| delete(tmp, op.ID) | ||
| } | ||
| } | ||
| for _, op := range tmp { | ||
| set = append(set, op) | ||
| } | ||
| return set | ||
| } |
There was a problem hiding this comment.
we're missing unit tests for these 3 funcs
these are important to test because they're quite hard to verify just by looking and to think about all the potential edgecases (imo)
There was a problem hiding this comment.
Okay agree. Ill make unit tests for these.
Re-sign:owner,nonceusing old BLS sharesproofsFilePath: /data/initiator/output/ceremony-2024-04-22--16-36-19.354/proofs.jsonproofs.jsonconsists of many proofs -> bulk resign withnonce+1Re-share: