Kauri-impl #158
Kauri-impl #158
Conversation
Omit Handel Aggregation protocol
This also recompiles all the .pb.go files to the latest version of protoc-gen-go.
But we retain the uint32 and input to the SetTreeConfig() method to allow passing the uint32 from the proto file directly into the SetTreeConfig() method.
meling
left a comment
meling
left a comment
There was a problem hiding this comment.
I'm adding my comments, specifically to the Kauri code... I'll follow up with a commit and push to fix the issues myself to save time, but I'm leaving the comments for the record.
Some of them are still valid questions from my side. I especially feel we should add a few tests for some of the logic. It should be easy to test the canMergeContributions function (since it no longer needs the Kauri struct.)
kauri/kauri.go
Outdated
| } | ||
|
|
||
| func (k *Kauri) WaitToAggregate(t time.Duration, view hotstuff.View) { | ||
| ticker := time.NewTicker(t) |
There was a problem hiding this comment.
Here, you should use time.Sleep(t). This code allocates a new ticker every time it is called. That is not the intended use of a ticker. From the go documentation:
NewTicker returns a new Ticker containing a channel that will send the current time on the channel after each tick. The period of the ticks is specified by the duration argument. The ticker will adjust the time interval or drop ticks to make up for slow receivers. The duration d must be greater than zero; if not, NewTicker will panic.
That said, I wonder if you could use the event loop to wait for the event "types" you want to aggregate based on instead of a time-based waiting. For instance, the event handler for aggregation events could count the events received and only when sufficiently many events have been received does the actual aggregation happen. Or if each event can be aggregated individually into an aggregate state variable, perhaps that's even better?
kauri/kauri.go
Outdated
| k.aggSent = false | ||
| } | ||
|
|
||
| func (k *Kauri) WaitToAggregate(t time.Duration, view hotstuff.View) { |
kauri/kauri.go
Outdated
|
|
||
| func (k *Kauri) postInit() { | ||
| k.logger.Info("Kauri: Initializing") | ||
| kauripb.RegisterKauriServer(k.server.GetGorumsServer(), serviceImpl{k}) |
There was a problem hiding this comment.
Looks like we won't need anything else from the Kauri struct in the serviceImpl, so we can do this:
kauripb.RegisterKauriServer(k.server.GetGorumsServer(), serviceImpl{k.eventLoop})And then use the eventLoop directly below in the SendContribution method. (see other comments below)
kauri/kauri.go
Outdated
| } | ||
|
|
||
| type serviceImpl struct { | ||
| k *Kauri |
There was a problem hiding this comment.
Could be:
eventLoop *eventloop.EventLoop
kauri/kauri.go
Outdated
| Contribution *kauripb.Contribution | ||
| } | ||
|
|
||
| func (k *Kauri) canMergeContributions(a, b hotstuff.QuorumSignature) bool { |
There was a problem hiding this comment.
I believe the following logic is a bit clearer:
func (k *Kauri) canMergeContributions(a, b hotstuff.QuorumSignature) bool {
if a == nil || b == nil {
k.logger.Info("kauri: cannot merge nil contributions")
return false
}
a.Participants().RangeWhile(func(i hotstuff.ID) bool {
// cannot merge a and b if both contain a contribution from the same ID.
return !b.Participants().Contains(i)
})
return true
}But I would be more confident in my statement if we had a test for this one. Could you please add a test?
This replaces the deprecated RegisterObserver with RegisterHandler
and the eventloop.Prioritize() option.
I replaced the Kauri reference in serviceImpl with a reference to
the eventloop instead; since we only used the eventLoop from the
Kauri struct anyway.
I replaced the time.NewTicker() (3 lines) with a simple time.Sleep,
since we should not allocate tickers for every time we call
WaitToAggregate().
I also removed the unused bool from mergeContribution; it was not
used and makes it harder to understand the code.
I replaced (k *Kauri) canMergeContributions with a function:
func canMergeContributions(a, b hotstuff.QuorumSignature) error
that returns an error instead of a bool. This allows better error
propagation and avoids logging error messages when anyway want to
pass the error up the call stack.
I removed verifyContribution() since it was easier to process call
the block fetching and directly erroring out from the mergeContribution
method.
Finally, I revised the method:
func (k *Kauri) mergeContribution(currentSignature hotstuff.QuorumSignature) error {
So that it returns only an error and follows the guard pattern and
thus avoids the nesting inside if-statements becomes untenable.
Please please learn the guard pattern!! It is ubiquitous across
many langauges...
if err != nil {
return err
}
// continue with program logic (without indentation)
...
if err != nil {
return err
}
// more program logic without indentation
...
|
I am working on merging this branch https://github.com/relab/hotstuff/tree/153-cue-integration ...into this Kauri branch and I was wondering how Kauri should be run? I am not seeing the |
…oval Remove deprecated observer pattern in event loop.
Porting Kauri implementation from previous implementation