Implement local watcher #182

manoranjith · 2021-09-09T17:53:48Z

Overview

This PR implements the server side component local watcher as per the design described in proposal#4.
This component should be initialized by the user of the framework and pass it as an argument to the client.New API.

Implementation details

The interface definitions are located in a package: go-perun/watcher.
The implementation of local watcher is located in a go-perun/watcher/local.

Data structures

Watcher:
- This represents the watcher instance, initializes using the NewWatcher function.
- It contains
  - channel.RegistererSubscriber instance for interacting with the blockchain.
  - registry that holds a map of all the channels currently registered with the Watcher.
- Both the above data structures are safe for concurrent use.
ch:
- Represents the data corresponding to a channel.
- It contains
  - id of the channel.
  - params of the channel. Required while registering dispute for the channel. This will be passed when registering the channel with the Watcher via StartWatching function.
  - parent of this channel. For ledger channels, this is set to channel.Zero.
  - subChsAccess, (relevant only for ledger channel) a mutex used for concurrency safe access of a family of channels. When a dispute registration is required for a channel, we traverse back to the parent and lock this mutex. It is unlocked only after the dispute registration is completed. This also ensures, we lock only the relevant channels and not the entire registry when registering disputes.
  - registeredVersion: when a dispute is registered for a ledger, sub or virtual channel, this field is updated. This is used to ensure, we do not register the same version more than once.
  - requestLatestTx and latestTx are a set of channels used for retrieving the latest state of this channel without use of mutex locks.
registry: Used to store and retrieve channels registered with the watcher. It is protected by a mutex.
StatesPubSub and AdjudicatorPubSub implements the functionalities exactly as described in the proposal.

Methods

NewWatcher initializes the watcher.
Watcher.StartWatchingForLedgerChannel and Watcher.StartWatchingForNonLedgerChannel` APIs are used to register a channel with the watcher. Though the implementations of both the APIs are same, two APIs are provided because
- LedgerChannel does not require parent while,
- Sub-Channel requires a parent.
- Regarding the names, I also think we need a better name, used this for time being.
AdjudicatorSub.EventStream can be used by the client for receiving adjudicator events from the watcher.
StatesPub.Publish can be used by the client to send newer transactions to the watcher.
Watcher.StopWatching and Watcher.Shutdown are yet to implemented.

Tests

It was difficult for me to figure out how to add tests using the simulated backend. Particularly, I couldn't figure out how to make the client send events to the watcher, before integrating the watcher API into client.

However, I implemented some tests based on generated mocks , which I used these to guide my development. I have added them in the PR (in separate commits) for time being. After figuring out how to test simulated backed, we could also replace these tests.

Open points

What do we do when registration fails ? Should we load the error information into the adjudicator events pub-sub to close it ?
What do we do in the below scenario ?
1. Ledger channel (LC1) is registered with the watcher.
2. Client opens a sub-channel (SC1).
3. Funding transaction is completed on LC1 and updated to the watcher.
4. Before SC1 registers itself with the watcher, an event is RegisteredEvent is received for LC1.
5. Now, when we want to collect the latest state for all sub-channels of LC1, we will not be able to fund SC1.

Resolves #174

- Previously, the adjudicator interface consisted of four methods. - But, only some of the methods were required in any particular place. - Hence, define an interface for each of the methods and compose the adjudicator interfaces from these smaller interfaces. Signed-off-by: Manoranjith <[email protected]>

- Add an interface "RegisterSubscriber" that combines the Register and Subscribe methods. This will be used by the watcher. Signed-off-by: Manoranjith <[email protected]>

Signed-off-by: Manoranjith <[email protected]>

- Implementation is same for ledger and non-ledger channels (sub/virtual). - However, since non-ledger channel require an additional parameter (parent), seaprate API interfaces are provided. - Mocks (placed in watcher/internal/mocks) were generated using testify framework. Signed-off-by: Manoranjith <[email protected]>

Signed-off-by: Manoranjith <[email protected]>

- Use a channel based mechanism to retrieve the latest transactions while handling dispute in order to avoid mutex locks. Signed-off-by: Manoranjith <[email protected]>

Signed-off-by: Manoranjith <[email protected]>

- Usage of the API is described in doc comments and all the scenarios are covered in tests. Signed-off-by: Manoranjith <[email protected]>

matthiasgeihs

Just a few high level comments. Could you give me and @ggwpez a code intro in our next meeting?

matthiasgeihs · 2021-09-17T10:39:13Z

watcher/watcher.go

 	//
 	// This channel will be closed when client requests the watcher to stop
 	// watching or when there is an error. After the channel is closed, error
 	// message can be read using the Err method.
 	AdjudicatorSub interface {
-		Next() <-chan channel.AdjudicatorEvent
+		EventStream() <-chan channel.AdjudicatorEvent


Wha happens if EventStream() is called twice? Should the implementation support multiplexing?

Updated documentation that Repeated calls to the EventStream method returns the same channel instance. in PR #212.

I think multiplexing is not necessary in our use case, because the only consumer of the events is the watch function.

If at a later point in time, we have the need for consuming the events in multiple places, then we could implement multiplexing.

matthiasgeihs · 2021-09-17T10:50:22Z

watcher/local/watcher.go

+}
+
+func (w *Watcher) startWatching(ctx context.Context, parent channel.ID, signedState channel.SignedState) (
+	watcher.StatesPub, watcher.AdjudicatorSub, error) {


The format looks odd. It looks like this is the first line of the function while in fact it is the beginning of the return parameters. If the line gets too long, you can place each parameter on a separate line, like

func (w *Watcher) StartWatchingSubChannel( ctx context.Context, parent channel.ID, signedState channel.SignedState, ) (watcher.StatesPub, watcher.AdjudicatorSub, error) {

I think this is more intuitive to read.

The same comment applies to all functions using that format.

Have updated in PR #213.

matthiasgeihs · 2021-09-17T10:57:13Z

watcher/local/watcher.go

+// StartWatchingLedgerChannel starts watching for a ledger channel.
+func (w *Watcher) StartWatchingLedgerChannel(ctx context.Context, signedState channel.SignedState) (
+	watcher.StatesPub, watcher.AdjudicatorSub, error) {
+	return w.startWatching(ctx, channel.Zero, signedState)


I don't like the usage channel.Zero. Even though it seems very unlikely that a channel will have ID zero, it would still be better to not make any assumptions about certain ID values.

It would be better to explicitly mark the parent parameter as optional. You could use a pointer for that and check for nil, which is also not super clean but at least it does not require any assumptions about the ID value.

Have updated the implementation to use nil instead of Channel.Zero See PRs #213 and #214.

matthiasgeihs · 2021-09-17T11:26:54Z

watcher/local/watcher.go

+	thisCh *ch) {
+	for e := eventsFromChainSub.Next(); e != nil; e = eventsFromChainSub.Next() {
+		switch e.(type) {
+		case *channel.RegisteredEvent:


As discussed in #207 , you may need to check also for the Progressed and Concluded events to be sure that you notice channel registration.

Updated in #215.

ggwpez

Comments from the meeting.

watcher/watcher.go

watcher/local/statespubsub.go

watcher/local/registry.go

watcher/local/watcher_test.go

watcher/local/watcher.go

manoranjith · 2021-09-20T10:02:54Z

channel/adjudicator.go

+		Registerer
+		Withdrawer
+		Progresser
+		EventSubscriber


This can be removed. As adjudicator does not make subscriptions.

EventSubscriber removed from Adjudicator, as it not used by the client anymore. (It is used only by the Watcher and Watcher uses the RegisterSubscriber method. See PR #212.

matthiasgeihs · 2021-10-29T13:53:27Z

Closed in favor #248

manoranjith force-pushed the 174-implement-local-watcher branch 3 times, most recently from 8d2d2a9 to b36713f Compare September 9, 2021 18:40

matthiasgeihs self-requested a review September 10, 2021 07:37

manoranjith mentioned this pull request Sep 13, 2021

Define smaller interfaces for methods in adjudcator #185

Closed

manoranjith force-pushed the 174-implement-local-watcher branch 2 times, most recently from 659c280 to 096646e Compare September 13, 2021 14:10

manoranjith added 5 commits September 16, 2021 05:37

✨ [channel/adjudicator,watcher] Add interface defs for watcher

1733588

- Add an interface "RegisterSubscriber" that combines the Register and Subscribe methods. This will be used by the watcher. Signed-off-by: Manoranjith <[email protected]>

✨ [watcher/local] Implement local states pub-sub

cab8bfa

Signed-off-by: Manoranjith <[email protected]>

✨ [watcher/local] Implement local adjudicator pub-sub

e68a7fc

Signed-off-by: Manoranjith <[email protected]>

✨ [watcher/local] Implement a channel registry

2fcc5e3

Signed-off-by: Manoranjith <[email protected]>

manoranjith force-pushed the 174-implement-local-watcher branch from 096646e to fcb78d0 Compare September 16, 2021 00:17

manoranjith marked this pull request as ready for review September 16, 2021 00:19

manoranjith requested review from matthiasgeihs and removed request for matthiasgeihs September 16, 2021 00:20

manoranjith added 5 commits September 16, 2021 12:30

✨ [watcher/local] Handle adj events without dispute

3e5b474

Signed-off-by: Manoranjith <[email protected]>

✨ [watcher/local] Handle adj events with dispute

6373839

- Use a channel based mechanism to retrieve the latest transactions while handling dispute in order to avoid mutex locks. Signed-off-by: Manoranjith <[email protected]>

✨ [watcher/local] Relay progressed events to client

4544b58

Signed-off-by: Manoranjith <[email protected]>

✨ [watcher/local] Implement stop watching API

ba8bbb9

- Usage of the API is described in doc comments and all the scenarios are covered in tests. Signed-off-by: Manoranjith <[email protected]>

manoranjith force-pushed the 174-implement-local-watcher branch from fcb78d0 to ba8bbb9 Compare September 16, 2021 07:09

manoranjith changed the title ~~174 Implement local watcher~~ Implement local watcher Sep 16, 2021

manoranjith mentioned this pull request Sep 16, 2021

Integrate StatesPub into client #198

Closed

matthiasgeihs reviewed Sep 17, 2021

View reviewed changes

ggwpez reviewed Sep 20, 2021

View reviewed changes

manoranjith marked this pull request as draft September 21, 2021 05:36

manoranjith marked this pull request as ready for review September 21, 2021 05:40

manoranjith commented Sep 21, 2021

View reviewed changes

ggwpez mentioned this pull request Sep 21, 2021

Implement local watcher (2/5): StartWatching functionality #213

Merged

matthiasgeihs added the watcher label Oct 13, 2021

matthiasgeihs closed this Oct 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement local watcher #182

Implement local watcher #182

manoranjith commented Sep 9, 2021 •

edited

Loading

matthiasgeihs left a comment

matthiasgeihs Sep 17, 2021

manoranjith Sep 21, 2021 •

edited

Loading

manoranjith Sep 21, 2021

matthiasgeihs Sep 17, 2021

manoranjith Sep 21, 2021

matthiasgeihs Sep 17, 2021

manoranjith Sep 21, 2021

matthiasgeihs Sep 17, 2021

manoranjith Sep 21, 2021

ggwpez left a comment

manoranjith Sep 20, 2021

manoranjith Sep 21, 2021 •

edited

Loading

matthiasgeihs commented Oct 29, 2021

Implement local watcher #182

Implement local watcher #182

Conversation

manoranjith commented Sep 9, 2021 • edited Loading

Overview

Implementation details

Data structures

Methods

Tests

Open points

Resolves #174

matthiasgeihs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manoranjith Sep 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggwpez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manoranjith Sep 21, 2021 • edited Loading

Choose a reason for hiding this comment

matthiasgeihs commented Oct 29, 2021

manoranjith commented Sep 9, 2021 •

edited

Loading

manoranjith Sep 21, 2021 •

edited

Loading

manoranjith Sep 21, 2021 •

edited

Loading