feat(disputes/coordinator): implements the coordinator module for the disputes-coordinator #3647
Conversation
kanishkatn
requested review from
edwardmack,
timwu20,
EclesioMeloJunior,
jimjbrettj,
kishansagathiya and
dimartiro
as code owners
kanishkatn
force-pushed
the
feat/k/disputes/coordinator
branch
from
03fac30 to
fd20fb7
Compare
kanishkatn
force-pushed
the
feat/k/disputes/coordinator
branch
3 times, most recently
from
6185550 to
ade4382
Compare
kanishkatn
force-pushed
the
feat/k/disputes/coordinator
branch
from
ade4382 to
e888108
Compare
There was a problem hiding this comment.
Still reviewing this.
Since this is a big PR, I am not reviewing this entirely at a time but part by part.
I will also point out the obvious problem in base branch of this PR. into feat/k/disputes-coordinator from feat/k/disputes/coordinator doesn't make sense, please change the base branch to feat/parachain
| b.recentDisputes.RLock() | ||
| recentDisputes := b.recentDisputes.Copy() | ||
| recentDisputes, err := b.GetRecentDisputes() | ||
| if err != nil { | ||
| return scale.BTree{}, fmt.Errorf("get recent disputes: %w", err) | ||
| } | ||
| b.recentDisputes.RUnlock() |
There was a problem hiding this comment.
This is deadlock.
At line 165 b.recentDisputes is readlocked and then it gets readlocked again GetRecentDisputes.
Lock-Unlock are not needed here. Let's do them only inside GetRecentDisputes
| if err != nil { | ||
| return false |
There was a problem hiding this comment.
getting an error does not mean that dispute is still active or inactive.
| } | ||
|
|
||
| if concludedAt != nil && *concludedAt+uint64(ActiveDuration.Seconds()) > uint64(now) { | ||
| if !isDisputeInactive(dispute.DisputeStatus) { |
There was a problem hiding this comment.
Also, instead of using a variable named isDisputeInactive and then checking !isDisputeInactive, I would prefer IsDisputeActive, That is more readable and easy to understand.
| if err != nil { | ||
| logger.Errorf("failed to get concluded at: %s", err) | ||
| return true | ||
| isDisputeInactive := func(status types.DisputeStatusVDT) bool { |
There was a problem hiding this comment.
There is no need to create this extra function. Plus, instead of ConcludedAt, use dispute.DisputeStatus.IsActive directly.
| @@ -97,10 +101,10 @@ func (b *overlayBackend) GetEarliestSession() (*parachainTypes.SessionIndex, err | |||
| return b.inner.GetEarliestSession() | |||
| } | |||
|
|
|||
| func (b *overlayBackend) GetRecentDisputes() (*btree.BTree, error) { | |||
| func (b *overlayBackend) GetRecentDisputes() (scale.BTree, error) { | |||
There was a problem hiding this comment.
Github doesn't let me comment on line number 86, so doing it here.
The comment on overlayBackend struct is // overlayBackend implements OverlayBackend.. If I go to OverlayBackend interface line 32, that says // OverlayBackend is the overlay backend for the disputes coordinator module.
We should put better comments on these things to give sufficient explanation to whoever reads this code.
| // update db | ||
| if recentDisputes.Len() > 0 { | ||
| if err = b.SetRecentDisputes(newRecentDisputes); err != nil { | ||
| return fmt.Errorf("set recent disputes: %w", err) |
There was a problem hiding this comment.
lets stick to the common pattern we have used across our code base of using -ing while wrapping errors.
| return fmt.Errorf("set recent disputes: %w", err) | |
| return fmt.Errorf("setting recent disputes: %w", err) |
| // clear recent disputes metadata | ||
| recentDisputes, err := b.GetRecentDisputes() | ||
| if err != nil { | ||
| return fmt.Errorf("get recent disputes: %w", err) |
There was a problem hiding this comment.
| return fmt.Errorf("get recent disputes: %w", err) | |
| return fmt.Errorf("getting recent disputes: %w", err) |
| @@ -0,0 +1,77 @@ | |||
| package dispute | |||
| "github.com/ChainSafe/gossamer/pkg/scale" | ||
| ) | ||
|
|
||
| type runtimeInfo struct { |
There was a problem hiding this comment.
add a comment about what this struct is for.
| @@ -0,0 +1,153 @@ | |||
| package comm | |||
There was a problem hiding this comment.
is comm short for something? Change this to full name? We could also add a comment about what this package is for.
|
|
||
| const timeout = 10 * time.Second | ||
|
|
||
| var logger = log.NewFromGlobal(log.AddContext("parachain", "disputes")) |
There was a problem hiding this comment.
let's keep parachain-disputes for package dot/parachain/disputes. Let's use parachain-disputes-comm here
| respCh := make(chan any, 1) | ||
| relayParent, err := receipt.Hash() | ||
| if err != nil { | ||
| return 0, fmt.Errorf("get hash: %w", err) |
There was a problem hiding this comment.
| return 0, fmt.Errorf("get hash: %w", err) | |
| return 0, fmt.Errorf("getting hash: %w", err) |
| t.Run("successful send", func(t *testing.T) { | ||
| ch := make(chan any, 1) | ||
| defer close(ch) | ||
|
|
||
| err := SendMessage(ch, "test") | ||
| require.NoError(t, err) | ||
| }) | ||
| t.Run("timeout", func(t *testing.T) { | ||
| ch := make(chan any) | ||
| defer close(ch) | ||
|
|
||
| err := SendMessage(ch, "test") | ||
| require.Error(t, err) | ||
| }) |
There was a problem hiding this comment.
run this as a loop with two test cases. A test case struct in this case, would contain two things description and channel size
| return n - getByzantineThreshold(n) | ||
| } | ||
|
|
||
| // Coordinator is the disputes coordinator |
There was a problem hiding this comment.
Let's add some more information.
| type Signal[data any] struct { | ||
| Data data | ||
| ResponseChannel chan<- any | ||
| } |
There was a problem hiding this comment.
a signal does not require a response. It is a signal from overseer to subsystems and then subsystems would react/process based on that. Some subsystems, don't react at all.
q9f
added
A-stale
Changes
Implements the coordinator module for the disputes coordinator.
Please skip reviewing BTree since it's waiting on an upstream update. Refer: #3536
Issues
#3593
Primary Reviewer
@EclesioMeloJunior