dkg/sync: enforce version #1901
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1901 +/- ##
==========================================
- Coverage 55.47% 55.45% -0.02%
==========================================
Files 172 173 +1
Lines 22073 22128 +55
==========================================
+ Hits 12244 12271 +27
- Misses 8253 8278 +25
- Partials 1576 1579 +3
... and 6 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
| // Minor returns the minor version of the provided version string. | ||
| func Minor(version string) (string, error) { | ||
| split := strings.Split(version, ".") | ||
| if len(split) < 2 { |
There was a problem hiding this comment.
this should be
| if len(split) < 2 { | |
| if len(split) < 3 { |
There was a problem hiding this comment.
Nope, it supports v0.1 as input and just returns it.
| "github.com/obolnetwork/charon/app/version" | ||
| ) | ||
|
|
||
| func TestMinor(t *testing.T) { |
There was a problem hiding this comment.
also add a case to assert an error from Minor function with error: invalid version string
| @@ -73,6 +76,14 @@ func (s *Server) AwaitAllConnected(ctx context.Context) error { | |||
| } | |||
| } | |||
|
|
|||
| // isError checks if there was any error in between the server flow. | |||
| func (s *Server) setError() { | |||
dkg/sync/server.go
Outdated
| log.Error(ctx, "Received mismatching cluster definition hash from peer", nil) | ||
| } else if ok && !s.isConnected(pID) { | ||
| count := s.setConnected(pID) | ||
| log.Info(ctx, fmt.Sprintf("Connected to peer %d of %d", count, s.allCount)) | ||
| } | ||
|
|
||
| // Verify definition hash |
There was a problem hiding this comment.
this comment should be around line number 200
dkg/sync/server.go
Outdated
| if msg.Version != s.version { | ||
| resp.Error = errInvalidVersion | ||
| s.setError() | ||
| log.Error(ctx, "Received mismatching charon version from peer", nil, | ||
| z.Str("expect", s.version), | ||
| z.Str("got", msg.Version), | ||
| ) | ||
| } else if ok, err := pubkey.Verify(s.defHash, msg.HashSignature); err != nil { // Note: libp2p verify does another hash of defHash. | ||
| return errors.Wrap(err, "verify sig hash") | ||
| } else if !ok { | ||
| resp.Error = errInvalidSig | ||
|
|
||
| s.mu.Lock() | ||
| s.errResponse = true | ||
| s.mu.Unlock() | ||
|
|
||
| s.setError() |
There was a problem hiding this comment.
can we have this message verification as a separate method to server struct?
| @@ -209,6 +214,29 @@ func (s *Server) handleStream(ctx context.Context, stream network.Stream) error | |||
| } | |||
| } | |||
|
|
|||
| // validReq returns an error message and false if the request version or definition hash are invalid. | |||
| // Else it returns true or an error. | |||
| func (s *Server) validReq(ctx context.Context, pubkey crypto.PubKey, msg *pb.MsgSync) (string, bool, error) { | |||
There was a problem hiding this comment.
we are reading s.version and s.defHash in this method without locking mutex
There was a problem hiding this comment.
they are immutable fields, no need to lock
There was a problem hiding this comment.
grouped server fields into immutable and mutable groups
corverroos
added
the
merge when ready
Enforce matching peer minor version. DKG only guarantees compatibility with patch versions.
category: bug
ticket: #1895