Adjudicator subscription logic

[manoranjith]

Context

Currently, the adjudicator subscription is designed in such a way that, calling the Next method returns the latest event. For instance, when initializing the subscription,

1. If 4 registered events have occurred, Next will return the 4th registered event.
2. If 4 registered events and 1 progressed event has occurred, Next will return the progressed event.
3. If 4 registered events and 2 progressed event has occurred, Next will return the 2nd progressed event.
4. If 4 registered events, 2 progressed event and concluded event has occurred, Next will return the concluded event.

In all the 3 cases, further calls to Next will block until a future event occurs and returns it. If the subscription is closed, Next always returns nil.

Problem

This design has caused a few inconveniences when using the subscription.

1. As described in 207 fix ensure registered #208: additional logic is required in ensureRegistered to detect if a channel is registered: a channel is registered if there is a registered event, progressed event or concluded event.
2. As described in Refactor AdjudicatorSubscription #118: In order to return only the latest event, some additional logic is required in the implementation of the adjudicator, which might also introduce race conditions.

Possible solutions

I thought, by redesigning the subscription, we could

1. Avoid race condition (only client will read, no event replacement logic required).
2. Make it easier to retrieve events in any manner (read all events, read latest registered event, read latest progressed event). Thereby removing additional logics and assumptions in implementation and usage of the subscription.

For this, we will have to

1. Redesign the Next method to return a channel, into which all the adjudicator events (past and future) will be dumped into.
   We could also rename the Next method, if required.
2. On client side, it could listen for adjudicator events using a for-select pattern.

for {
       select {
       case e, ok := <-event.Sub:
           if !ok {
                // handle subscription closed
           }
           // handle event 
       }
}

3. On the client side, it could retrieve the latest event could be read using a for-select-default pattern.

func retreiveLatestEvent(sub channel.AdjudicatorSubscription) channel.AdjudicatorEvent {
for {
    var latestEvent channel.AdjudicatorEvent
    select {
       case e, ok := <-event.Sub:
           if !ok {
                return latestEvent // as subscription was closed.    
           }
           latestEvent = e
       default:
          return latestEvent
    }
}
}

With little modification, it is also possible to retrieve the latest Registered Event, latest Progressed Event or latest Concluded Event.

[manoranjith]

@matthiasgeihs @ggwpez

[matthiasgeihs]

Im generally in favor simplifying the subscription logic as I see several downsides of the current implementation but not really clear benefits.

However, I wonder: What was the intention behind choosing the more complex design in the first place? Are we losing some benefits if we change to a simpler subscription logic?

[manoranjith]

From the commit history, I see that this behaviour was introduced in b85185b. But I could not find reference to an issue from the commit message. Probably, @sebastianst (author) could give use more context ?

[matthiasgeihs]

As I currently understand, you are suggesting that a subscription should always return all past and future events. One downside could be that a ledger backend does not provide the functionality to retrieve historic events.

Maybe a more widely supported approach would be that we simply guarantee that all events from the point of starting the subscription will be returned. In that case, we would need to make sure that all users of the subscription start it before they expect events.

In some cases, however, it may be necessary to retrieve the most recent event, for example, when resuming a client. For this, we would need an additional function that guarantees the retrieval of the latest event.

In any case, we could probably remove the requirement that only the latest event should be returned and instead allow the subscription to return all events. I can't imagine that this requirement has any clear benefits that outweigh the complexity introduced by it because a subscription user must be able to handle all events anyways.

[manoranjith]

I had missed to see the case where a ledger backend does not provide the functionality to historic events.

If we have a closer look into how this subscription is used:

1. We use it in the watcher.
   • For perun protocol to work, we require the watcher to be running throughout the lifetime of the channel.
   • Hence, the watcher does not need historic events.
2. We use it in the specific backend to retrieve events when registering, concluding etc.,
   • Here, we might need historic events.

Since, we need the historic events only we are aware of the backend, we could provide an optional flag retrieveHisoricEvents to the Subscribe method.
While subscribing in the watcher (backend agnostic), this option is not used.
While subscribing in the backend, we could use this option.