core/consensus: implement qbft component #514
Conversation
Codecov Report
@@ Coverage Diff @@
## main #514 +/- ##
==========================================
- Coverage 57.42% 55.12% -2.30%
==========================================
Files 87 90 +3
Lines 8032 8318 +286
==========================================
- Hits 4612 4585 -27
- Misses 2819 3128 +309
- Partials 601 605 +4
Continue to review full report at Codecov.
|
| @@ -59,7 +60,8 @@ func RandomCorePubKey(t *testing.T) core.PubKey { | |||
| // RandomEth2PubKey returns a random eth2 phase0 bls pubkey. | |||
| func RandomEth2PubKey(t *testing.T) eth2p0.BLSPubKey { | |||
| t.Helper() | |||
| pubkey, _, err := tbls.Keygen() | |||
| random := rand.New(rand.NewSource(rand.Int63())) | |||
There was a problem hiding this comment.
yup, since it uses math.Rand for the seed
| tcpNode: tcpNode, | ||
| peers: peers, | ||
| p2pKey: p2pKey, | ||
| recvBuffers: make(map[core.Duty]chan msg), |
There was a problem hiding this comment.
why do we need receive buffers for each duty with size 100?
and why 100?
There was a problem hiding this comment.
I'll add this comment: Allow receiving some initial messages before this node has started
corverroos
added
the
merge when ready
| const ( | ||
| recvBuffer = 100 | ||
| period = time.Second | ||
| protocolID = "/charon/consensus/1.0.0" |
There was a problem hiding this comment.
Will we have only one type of consensus message protocol do you think?
What if:
/charon/consensus/qbft/1.0.0
/charon/consensus/hotstuff/0.1.0
/charon/consensus/tendermint/0.5.0
etc.
core/consensus/component.go
Outdated
| ) | ||
|
|
||
| const ( | ||
| recvBuffer = 100 |
There was a problem hiding this comment.
Same q as dhruv, do we need this to be a parameter? Do we expect this to be a dos vector? Should we slap a prom gauge/metric around it somewhere to keep an eye on the space in the buffer?
|
|
||
| // 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.
Noob go question, here we're passing only a pointer to the one PrivateKey in memory, not duplicating the value everywhere, right? Is exposing the raw memory address of the PKey to many places encapsulated enough? Do we need any sort of mutex for signing with it or is it all good cause it's all reads no writes? Is this QBFT Component a singleton that does all QBFT or will there be parallel instantiations of it?
| t.setValues(msg) | ||
|
|
||
| select { | ||
| case <-ctx.Done(): |
There was a problem hiding this comment.
This is in many places so I take it its intentional, but what's with a select { } block not counting for an extra indentation, the cases aren't tabbed, convention I guess but seems weird to me.
| if err != nil { | ||
| return errors.Wrap(err, "new stream") | ||
| } | ||
| defer s.Close() |
There was a problem hiding this comment.
There's no scenario where s can be non-nil when err is non-nil, right? Otherwise we could have a memory leak from returning before deferring the close.
Implements a qbft consensus component that integrates qbft into p2p and the core workflow.
Note this isn't wired yet. Tests will also be added in subsequent PR.
category: feature
ticket: #445
feature_flag: qbft_consensus