Refactor with hotstuff.Distributor#8156
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
| if err != nil { | ||
| return nil, fmt.Errorf("could not create follower engine: %w", err) | ||
| } | ||
| builder.FollowerDistributor.AddOnBlockFinalizedConsumer(builder.FollowerEng.OnFinalizedBlock) |
There was a problem hiding this comment.
This was error-prone because builder.FollowerEng depended on the caller (access_node_builder) to manually register it with the follower distributor in order to receive block-finalization events. If that line was removed—or simply forgotten—the compiler would not complain, but the follower engine would silently stop receiving finalization events.
The goal of the refactor is to ensure that any component needing block-finalization notifications receives them automatically. Each such component now accepts a hotstuff.Distributor as a constructor argument and registers itself with the distributor during initialization, guaranteeing that it is properly subscribed.
Many of the changes stem from this principle, including extracting the hotstuff.Distributor interface (see the new file at consensus/hotstuff/distributor.go). The builder.FollowerDistributor now implements this interface and is passed into constructors, so callers no longer need to remember to invoke AddOnBlockFinalizedConsumer manually.
| d.blockIncorporatedConsumers = append(d.blockIncorporatedConsumers, consumer) | ||
| } | ||
|
|
||
| func (d *FinalizationDistributor) AddFinalizationConsumer(consumer hotstuff.FinalizationConsumer) { |
There was a problem hiding this comment.
This is redundant. If the caller needs to add a finalization consumer, it can just use AddOnBlockFinalizedConsumer and AddOnBlockIncorporatedConsumer to subscribe for both events.
There was a problem hiding this comment.
This function exists to follow the pattern set by other consumer interfaces in this package. Similar events are grouped together, so that most consumer components would want to subscribe to the entire group of events. Then, the component can implement the interface and call one function to subscribe, instead of calling one function per event type. (We also added options to specifically subscribe to each event individually, because these events are frequently used.)
For example, we have consumer interfaces for TimeoutAggregationViolationConsumer, TimeoutCollectorConsumer, ParticipantConsumer etc.
Some of these interfaces are small, but some are quite large (eg. ParticipantConsumer has over 10 methods). The difference between calling AddFinalizationConsumer() and AddOnBlockFinalizedConsumer(); AddOnBlockFinalizedConsumer() isn't a big deal, butI don't think we want to call 10 subscribe functions to add a ParticipantConsumer.
In general I think we should keep AddFinalizationConsumer around, to maintain a consistent pattern across the package.
There was a problem hiding this comment.
Makes sense. 74b364c
I added back the AddFinalizationConsumer method, and internally it's implemented using AddOnBlockFinalizedConsumer and AddOnBlockIncorporatedConsumer. What I wanted to get rid of is the consumers field, which is actually redundant.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
zhangchiqing
requested review from
AlexHentschel,
jordanschalm and
tim-barry
jordanschalm
left a comment
jordanschalm
left a comment
There was a problem hiding this comment.
Nice work. I agree that it is better for correctness-critical notification subscription to happen in the relevant component's constructor. Added some documentation suggestions, and a request to keep AddFinalizationConsumer to maintain consistency across the pubsub interfaces.
| distributor.AddOnBlockFinalizedConsumer(e.onFinalizedBlock) | ||
|
|
There was a problem hiding this comment.
| distributor.AddOnBlockFinalizedConsumer(e.onFinalizedBlock) | |
| distributor.AddOnBlockFinalizedConsumer(e.onFinalizedBlock) |
| // register callback with distributor | ||
| followerDistributor.AddOnBlockFinalizedConsumer(blockConsumer.onFinalizedBlock) |
There was a problem hiding this comment.
| // register callback with distributor | |
| followerDistributor.AddOnBlockFinalizedConsumer(blockConsumer.onFinalizedBlock) | |
| followerDistributor.AddOnBlockFinalizedConsumer(blockConsumer.onFinalizedBlock) |
mostly redundant comment
| // register callback with distributor | ||
| followerDistributor.AddOnBlockFinalizedConsumer(e.onBlockFinalized) |
There was a problem hiding this comment.
| // register callback with distributor | |
| followerDistributor.AddOnBlockFinalizedConsumer(e.onBlockFinalized) | |
| followerDistributor.AddOnBlockFinalizedConsumer(e.onBlockFinalized) |
| d.blockIncorporatedConsumers = append(d.blockIncorporatedConsumers, consumer) | ||
| } | ||
|
|
||
| func (d *FinalizationDistributor) AddFinalizationConsumer(consumer hotstuff.FinalizationConsumer) { |
There was a problem hiding this comment.
This function exists to follow the pattern set by other consumer interfaces in this package. Similar events are grouped together, so that most consumer components would want to subscribe to the entire group of events. Then, the component can implement the interface and call one function to subscribe, instead of calling one function per event type. (We also added options to specifically subscribe to each event individually, because these events are frequently used.)
For example, we have consumer interfaces for TimeoutAggregationViolationConsumer, TimeoutCollectorConsumer, ParticipantConsumer etc.
Some of these interfaces are small, but some are quite large (eg. ParticipantConsumer has over 10 methods). The difference between calling AddFinalizationConsumer() and AddOnBlockFinalizedConsumer(); AddOnBlockFinalizedConsumer() isn't a big deal, butI don't think we want to call 10 subscribe functions to add a ParticipantConsumer.
In general I think we should keep AddFinalizationConsumer around, to maintain a consistent pattern across the package.
Co-authored-by: Jordan Schalm <jordan.schalm@flowfoundation.org>
4 participants
This PR refactors the finalization-events consumer to accept a hotstuff.Distributor and automatically register itself for event consumption. This prevents mistakes where callers forget to manually subscribe the consumer, which previously led to silent failures in receiving block-finalization events.
See details in comments.