-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dkg: strengthen logic for steps update #2797
Changes from 3 commits
41cfa32
7fe4a3d
4702006
3bf141e
8566fa5
14ade9d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,7 +38,7 @@ func NewClient(tcpNode host.Host, peer peer.ID, hashSig []byte, version version. | |
done: make(chan struct{}), | ||
reconnect: true, | ||
version: version, | ||
period: 250 * time.Millisecond, | ||
period: 100 * time.Millisecond, // Must be at least two times lower than the sync timeout (dkg.go, startSyncProtocol) | ||
} | ||
|
||
for _, opt := range opts { | ||
|
@@ -53,7 +53,7 @@ func NewClient(tcpNode host.Host, peer peer.ID, hashSig []byte, version version. | |
// supports reestablishing on relay circuit recycling, and supports soft shutdown. | ||
type Client struct { | ||
// Mutable state | ||
mu sync.Mutex | ||
mu sync.RWMutex | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need a read-write mutex here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because in many places we need just read. Using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @pinebit Absolutely agree that By what magnitude do you believe using Else, we can discuss this async and create a ticket to refactor mutexes in the codebase to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is more about training muscle memory and develop good habits to use the appropriate API. Not necessary to optimize the performance in this particular case. Also, this improves readability - you show an intention. For example if I see RLock(), I can quickly realize this function should not modify the state and then I will think twice when introducing a change. I do not suggest to go and rewrite the entire codebase to use RWLock right away, we can do this whenever we make a change in an affect file, for example, like adding/changing a getter or setter. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes precisely, this removes |
||
connected bool | ||
reconnect bool | ||
step int | ||
|
@@ -113,16 +113,16 @@ func (c *Client) SetStep(step int) { | |
|
||
// getStep returns the current step. | ||
func (c *Client) getStep() int { | ||
c.mu.Lock() | ||
defer c.mu.Unlock() | ||
c.mu.RLock() | ||
defer c.mu.RUnlock() | ||
|
||
return c.step | ||
} | ||
|
||
// IsConnected returns if client is connected to the server or not. | ||
func (c *Client) IsConnected() bool { | ||
c.mu.Lock() | ||
defer c.mu.Unlock() | ||
c.mu.RLock() | ||
defer c.mu.RUnlock() | ||
|
||
return c.connected | ||
} | ||
|
@@ -255,8 +255,8 @@ func (c *Client) DisableReconnect() { | |
|
||
// shouldReconnect returns true if clients should re-attempt connecting to peers. | ||
func (c *Client) shouldReconnect() bool { | ||
c.mu.Lock() | ||
defer c.mu.Unlock() | ||
c.mu.RLock() | ||
defer c.mu.RUnlock() | ||
|
||
return c.reconnect | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
// Copyright © 2022-2023 Obol Labs Inc. Licensed under the terms of a Business Source License 1.1 | ||
|
||
package sync | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/libp2p/go-libp2p/core/peer" | ||
"github.com/stretchr/testify/require" | ||
|
||
"github.com/obolnetwork/charon/app/version" | ||
"github.com/obolnetwork/charon/testutil" | ||
) | ||
|
||
func TestUpdateStep(t *testing.T) { | ||
sv, err := version.Parse("v0.1") | ||
require.NoError(t, err) | ||
|
||
server := &Server{ | ||
defHash: testutil.RandomBytes32(), | ||
tcpNode: nil, | ||
allCount: 1, | ||
shutdown: make(map[peer.ID]struct{}), | ||
connected: make(map[peer.ID]struct{}), | ||
steps: make(map[peer.ID]int), | ||
version: sv, | ||
} | ||
|
||
t.Run("wrong initial step", func(t *testing.T) { | ||
err = server.updateStep("alpha", 100) | ||
require.ErrorContains(t, err, "peer reported abnormal initial step, expected 0 or 1") | ||
}) | ||
|
||
t.Run("valid peer step update", func(t *testing.T) { | ||
err = server.updateStep("bravo", 1) | ||
require.NoError(t, err) | ||
|
||
err = server.updateStep("bravo", 1) | ||
require.NoError(t, err) // same step is allowed | ||
|
||
err = server.updateStep("bravo", 2) | ||
require.NoError(t, err) // next step is allowed | ||
}) | ||
|
||
t.Run("peer step is behind", func(t *testing.T) { | ||
err = server.updateStep("behind", 1) | ||
require.NoError(t, err) | ||
|
||
err = server.updateStep("behind", 0) | ||
require.ErrorContains(t, err, "peer reported step is behind the last known step") | ||
}) | ||
|
||
t.Run("peer step is ahead", func(t *testing.T) { | ||
err = server.updateStep("ahead", 1) | ||
require.NoError(t, err) | ||
|
||
err = server.updateStep("ahead", 3) | ||
require.ErrorContains(t, err, "peer reported step is ahead the last known step") | ||
}) | ||
} |
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.
can you please point me to the code where this is specified?
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.
charon/dkg/sync/client.go
Line 41 in d2748d5