Conversation
emlautarom1
left a comment
emlautarom1
left a comment
There was a problem hiding this comment.
LGTM, some small comments on some improvements to the tests.
| let sig_types = sig_types.clone(); | ||
| let st = st.clone(); |
There was a problem hiding this comment.
nit: This matches Charon but I don't understand why we need the same data as Vec and HashSet (we do a contains in both cases).
| .await | ||
| .subscribe_internal(sub) | ||
| .await | ||
| .expect("subscribe_internal is infallible"); |
There was a problem hiding this comment.
Why did we make subscribe_internal return a Result if it's infallible? Can we update its definition?
| if !self.sig_types.contains(&sig_type) { | ||
| return Err(ExchangerError::UnrecognisedSigType(sig_type)); | ||
| } |
There was a problem hiding this comment.
This check seems not correct.
As dkg.go they will call SIG_DEPOSIT_DATA + i (https://github.com/ObolNetwork/charon/blob/v1.7.1/dkg/dkg.go#L701)
peerSigs, err := ex.exchange(ctx, sigType(int(sigDepositData)+i), parSig)
But the newExchanger, they only register the sigDepositData
https://github.com/ObolNetwork/charon/blob/v1.7.1/dkg/dkg.go#L242-L246
ex := newExchanger(p2pNode, nodeIdx.PeerIdx, peerIDs, []sigType{
sigLock,
sigDepositData,
sigValidatorRegistration,
}, conf.Timeout)
There was a problem hiding this comment.
Also golang doesn't have that check
| if !self.sig_types.contains(&sig_type) { | ||
| return Err(ExchangerError::UnrecognisedSigType(sig_type)); | ||
| } |
There was a problem hiding this comment.
Also golang doesn't have that check
| let handle = handle_clone.clone(); | ||
| async move { | ||
| if let Err(e) = handle.broadcast(duty, set).await { | ||
| warn!(error = %e, "Failed to broadcast partial signatures during DKG"); |
There was a problem hiding this comment.
One thing I worry about with the rust libp2p design is how we can manage to handle "enough" fail events at the swarm level.
4 participants
No description provided.