dkg: add aggregate lock hash signing skeleton #518
Conversation
Codecov Report
@@ Coverage Diff @@
## main #518 +/- ##
==========================================
- Coverage 55.46% 55.06% -0.40%
==========================================
Files 90 90
Lines 8366 8408 +42
==========================================
- Hits 4640 4630 -10
- Misses 3118 3173 +55
+ Partials 608 605 -3
Continue to review full report at Codecov.
|
corverroos
force-pushed
the
corver/dkgsign
branch
from
e0e49f5
to
723c069
Compare
dkg/dkg.go
Outdated
| db := parsigdb.NewMemDB(lock.Threshold) | ||
| exchange := parsigex.NewParSigEx(tcpNode, peerIdx, peers) | ||
| db.SubscribeInternal(exchange.Broadcast) | ||
| db.SubscribeThreshold(makeSigAgg(sigChan)) | ||
| exchange.Subscribe(db.StoreExternal) |
There was a problem hiding this comment.
using core workflow components for all the heavy lifting.
Note that the generic way these components have been implemented, allows them to be used in a different use-case here in DKG.
There was a problem hiding this comment.
shouldn't we desubscribe when the function finishes, or am I missing something?
There was a problem hiding this comment.
that is the select function below
There was a problem hiding this comment.
I don't understand, which line?
There was a problem hiding this comment.
these methods only wires stuff
the input is:
err = db.StoreInternal(ctx, core.Duty{}, signedSet)
if err != nil {
return nil, err
}
the output is below at:
case sig := <-sigChan:
sigs = append(sigs, sig)
}
if len(sigs) == len(lock.Validators) {
break
}
There was a problem hiding this comment.
yeah that's right, there's no need to unsubscribe. So basically in charon we follow functional programming paradigm where we keep our functions/methods to be stateless and only introduce statefulness whenever necessary. We don't need unsubscription because we aren't persisting anything. So when that function goes out of scope, we are done. Go's GC will take care of that.
On the other hand, we introduce statefulness for functions mainly by returning functions i.e, closures.
Am I right, @corverroos ?
There was a problem hiding this comment.
exactly right! 🚀
There was a problem hiding this comment.
@dB2510 @corverroos
| we keep our functions/methods to be stateless and only introduce statefulness whenever necessary
This is very good, a good practice, but not linked to functional programming, this is done in OOP and any style of programming.
I see now that we are creating our own parsigdb and exchange every time we call this function, and that is the reason the subscribers are garbage collected. I didn't catch that before.
Is this intended for some reason? or just a temporary shortcut that will be refactored in the future?
Just asking...
There was a problem hiding this comment.
Is this intended for some reason? or just a temporary shortcut that will be refactored in the future?
yes, this is temporary. You can refer here for next steps: #522
| // TODO(corver): Implement lock hash signing by all DVs and aggregation. | ||
| // aggSignLockHash returns the aggregated multi signature of the lock hash | ||
| // signed by all the distributed validator group private keys. | ||
| func aggSignLockHash(ctx context.Context, tcpNode host.Host, peerIdx int, |
There was a problem hiding this comment.
@corverroos please, either one line or each parameter on its own line. I think that is how I always saw it. Isn't a linter for that?
There was a problem hiding this comment.
at this point this isn't linted or specified anywhere
| sigChan := make(chan *bls_sig.Signature, len(lock.Validators)) | ||
| sigdb := parsigdb.NewMemDB(lock.Threshold) | ||
| exchange := parsigex.NewParSigEx(tcpNode, peerIdx, peers) | ||
| sigdb.SubscribeInternal(exchange.Broadcast) | ||
| sigdb.SubscribeThreshold(makeSigAgg(sigChan)) | ||
| exchange.Subscribe(sigdb.StoreExternal) |
There was a problem hiding this comment.
nit: add comments for these lines
There was a problem hiding this comment.
sure will do in next PR
corverroos
added
the
merge when ready
Adds the general steps to create an aggregate signature of the lock hash by all validators.
category: feature
ticket: #478