core/consensus: integrate qbft #603
Conversation
| return nil, nil, err | ||
| } | ||
|
|
||
| if featureset.Enabled(featureset.QBFTConsensus) { |
There was a problem hiding this comment.
this is how feature flags are used
| @@ -43,9 +43,7 @@ const ( | |||
| ) | |||
|
|
|||
| // NewComponent returns a new consensus QBFT component. | |||
| func NewComponent(tcpNode host.Host, peers []p2p.Peer, | |||
| peerIdx int64, p2pKey *ecdsa.PrivateKey, | |||
There was a problem hiding this comment.
removing peerIdx, rather inferring it from tcphost to avoid mistakes.
| if msg == nil { | ||
| return [32]byte{}, nil | ||
| } |
There was a problem hiding this comment.
Checking if an interface is nil is bad, since nil pointer implementations of the interface isn't nil...
Codecov Report
@@ Coverage Diff @@
## main #603 +/- ##
==========================================
+ Coverage 54.58% 56.44% +1.85%
==========================================
Files 96 96
Lines 8975 9062 +87
==========================================
+ Hits 4899 5115 +216
+ Misses 3373 3208 -165
- Partials 703 739 +36
Continue to review full report at Codecov.
|
core/consensus/component.go
Outdated
| func NewComponent(tcpNode host.Host, peers []p2p.Peer, | ||
| peerIdx int64, p2pKey *ecdsa.PrivateKey, | ||
| ) (*Component, error) { | ||
| func NewComponent(tcpNode host.Host, peers []p2p.Peer, p2pKey *ecdsa.PrivateKey) (*Component, error) { |
There was a problem hiding this comment.
As this is the only component created in this package, the constructor should be named New . Also, the component should not be called "Component" but actually what it is Perhaps "QBFT".
There was a problem hiding this comment.
That way it will read consensu.QBFT in other packages.
| ) (host.Host, *enode.LocalNode, error) { | ||
| p2pKey := conf.TestConfig.P2PKey |
There was a problem hiding this comment.
why this was moved from here to Run function?
There was a problem hiding this comment.
previously this was only used in p2p wiring, now it is used in p2p and qbft (need to sign messages). So moved it one level up.
| return nil, nil, err | ||
| } | ||
|
|
||
| if featureset.Enabled(featureset.QBFTConsensus) { |
| @@ -42,8 +42,8 @@ const ( | |||
| protocolID = "/charon/consensus/qbft/1.0.0" | |||
| ) | |||
|
|
|||
| // NewComponent returns a new consensus QBFT component. | |||
| func NewComponent(tcpNode host.Host, peers []p2p.Peer, p2pKey *ecdsa.PrivateKey) (*Component, error) { | |||
There was a problem hiding this comment.
NewComponent looks more relevant 🙃
There was a problem hiding this comment.
if we name components "component", everything would be named component, that is why we should avoid those types of generics in naming
corverroos
added
the
merge when ready
Integrates consensus QBFT component into charon run.
Also adds unit test for consensus component.
category: feature
ticket: #602
feature_flag: qbft_consensus