-
Notifications
You must be signed in to change notification settings - Fork 14
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this just a draft, added a few comments. Let me know if you need me to give it a deeper look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Now that the upgrade to mir is finally merged, the only thing that needs to be implemented is the mirCrypto.Impl
interface of Mir.
Then we can use it as a module like this
cryptoManager, err := NewCryptoManager(...)
// handle err...
cryptoModule := mirCrypto.New(cryptoManager)
// ...
// use cryptoModule when instantiating a Node with mir.NewNode(...)
DiscussionQuestionWhat is the AnswerIt is meant for verifying signatures on transactions in an SMR system. In Eudico-terms, this would be for verifying signatures on Eudico messages that were produced using a wallet key. On the other hand, the Given that in Eudico, the mempool already only contains Eudico messages that are ready to be ordered and their signatures do not need to be verified by the ordering layer, the QuestionWhat key will we use for signing consensus protocol messages? AnswerIdeally we should use the key that corresponds to the identity of the validator node as defined in the validator set.
ConclusionOptionsWe have two options for implementing the crypto module for Eudico
How to proceedLook for possibilities of signing arbitrary data using the libp2p key, but without spending much time on it (few hours max). On success, use option 1. On failure, use option 2. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I'll adapt the Mir crypto interface so we can simplify even further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gooks good in general, only minor comments, the most important one being the usage of the new crypto module.
We'll need to see about including the request payload, but that's for another PR.
chain/consensus/mir/manager.go
Outdated
|
||
go func() { | ||
select { | ||
case <-ctx.Done(): | ||
log.Debugf("Mir manager: context closed") | ||
log.Infof("Mir manager %s: context closed", m.MirID) | ||
m.Stop() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the manager supposed to stop only when the context given to it from outside is canceled, or should it also stop when the Mir node stops by itself for some reason (e.g. due to an error)?
Currently, if the Mir node encounters an error, the Manager is not stopped.
Moreover, if the Mir node stops by itself without an error, this whole goroutine will keep running until the external context is canceled.
Is that intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Case 1. If the Mir node encounters an error then we send the error to
errChan
and callmanagerCancel()
that will stop the manager as a goroutine but it will not call theStop
function.Stop
doesn't stop the manager it stops the Mir node. The question: if the Mir node has encountered an error does make sense to stop its components gracefully? - Case 2.
Mir node stops by itself without an error
. According to the comments itStops and returns when ctx is canceled.
So it can't just finish all work and stop. Right? It stops via context only and then the special error is returned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I don't quite understand it... In the code it looks like m
is the Manager and the code calls m.Stop()
. Doesn't that stop the Manager?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the Mir node has encountered an error does make sense to stop its components gracefully?
Yes. The Mir Node does not really have a reason (or even a way) of stopping other modules like the WAL or Net on error. So in fact, they still need to be stopped from the outside event if the Node fails. In a way, the modules can be seen as components of the Node, but one can also see them as independent components that only interact using the node (after all they are also started separately from the Node). So it make sense to also stop them (preferably gracefully) separately from the Node (even if it fails).
It also happens to simplify the code to something like this:
func (m *Manager) Start(ctx context.Context) chan error {
log.Infof("Mir manager %s starting", m.MirID)
errChan := make(chan error, 1)
go func() {
// Run Mir node until it stops.
if err := m.MirNode.Run(ctx); err != nil && !errors.Is(err, mir.ErrStopped) {
log.Infof("Mir manager %s: Mir node stopped with error: %v", m.MirID, err)
errChan <- err
} else {
log.Infof("Mir manager %s: Mir node stopped gracefully", m.MirID)
errChan <- nil
}
// Perform cleanup of Node's modules.
m.Stop()
}()
return errChan
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
m.Stop() stops the Mir network and WAL.
вт, 21 июн. 2022 г., 13:22 Matej Pavlovic ***@***.***>:
… ***@***.**** commented on this pull request.
------------------------------
In chain/consensus/mir/manager.go
<#208 (comment)>
:
>
go func() {
select {
case <-ctx.Done():
- log.Debugf("Mir manager: context closed")
+ log.Infof("Mir manager %s: context closed", m.MirID)
m.Stop()
Then I don't quite understand it... In the code it looks like m is the
Manager and the code calls m.Stop(). Doesn't that stop the Manager?
—
Reply to this email directly, view it on GitHub
<#208 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABRYEGMBO4B3E4WQ5EDPPVDVQGJU7ANCNFSM5Y6N4DVQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Codecov Report
@@ Coverage Diff @@
## eudico #208 +/- ##
==========================================
+ Coverage 21.26% 22.56% +1.29%
==========================================
Files 597 609 +12
Lines 65858 66119 +261
==========================================
+ Hits 14006 14919 +913
+ Misses 49246 48516 -730
- Partials 2606 2684 +78
|
This PR adds an implementation of Mir's Crypto interface.