The process manager should forward events, not make decisions

[rishflab]

Currently the process manager makes the decision to broadcast a transaction based on the occurrence of an event. The problem with this strategy is that the process manager often does not have enough information to make the correct decision. For example, the process manager broadcasts the refund transaction when the refund timelock expires. This is not the desired behavior. We only want to broadcast the refund transaction if refund_timelock_expired && !attestation_received. Because the process manager does not build an aggregate it cannot check that these conditions are met and makes the incorrect decision to broadcast the refund transaction when a decrypted cet is present.

We can solve this problem my blindly forwarding events to interested actors and in this case, leave the decision to broadcast the refund transaction to the monitor::Actor. The monitor actor can build an aggregate and make the correct decision to broadcast the refund transaction when refund_timelock_expired && !attestation_received.

tldr;
Make the process manager dumb. It just forwards events to interested actors instead of trying to make decisions like broadcasting a specific tx. A single event itself does not provide enough information to make the right decision, we need an aggregate.

Issue which triggered this discussion: #1230

[da-kami]

Is the concrete problem you are describing actually the problem of the process manager?

I feel the underlying problem is, that model::Cfd logic does not depict refund_timelock_expired && !attestation_received correctly, and makes the refund transaction available when we have an attestation. I think we can change that in the model (and add model tests that ensure the desired behavior).

My understanding: Refund should only be available if both oracle and maker are unavailable. If we get an attestation we should not have refund available. So, the model is wrong to make it available!
When it comes to producing events all decisions should be done by model::Cfd. So, if a decision is not depicted correctly we should change the model and not try to reason in other places. (Unless we really can't do that - but so far there was not case like that)

Or am I misunderstanding something?

[rishflab]

Is the concrete problem you are describing actually the problem of the process manager?

Nope. This could be solved performing a check in the "TryBroadcastTransaction" handler. Moving the decision to broadcast the refund tx out of the process manager is a solution and I think in the medium to long term it will be a good solution.

I feel the underlying problem is, that model::Cfd logic does not depict refund_timelock_expired && !attestation_received correctly, and makes the refund transaction available when we have an attestation. I think we can change that in the model (and add model tests that ensure the desired behavior).

My understanding: Refund should only be available if both oracle and maker are unavailable. If we get an attestation we should not have refund available. So, the model is wrong to make it available! When it comes to producing events all decisions should be done by model::Cfd. So, if a decision is not depicted correctly we should change the model and not try to reason in other places. (Unless we really can't do that - but so far there was not case like that)

Or am I misunderstanding something?

The model::Cfd can depict refund_timelock_expired && !attestation_received correctly. This is not the problem. The problem is that process manager instead of the monitor::Actor is making decision to broadcast the refund transaction. The process manager does not have access to model::Cfd and therefore incorrectly broadcasts even the attestation has been received. What I am saying is the process manager should just forward events to relevant actors leaving them to make the business decision as the actors can build aggregates and are therefore in a better to make a business decision.

In the process manager we try to broadcast refund when the timelock expires without checking whether an attestation is received. We can't check this in the process manager because it does not build model::Cfd or any aggregate.

   RefundTimelockExpired { refund_tx: tx } => {
                self.try_broadcast_transaction
                    .send_async_safe(monitor::TryBroadcastTransaction {
                        tx,
                        kind: TransactionKind::Refund,
                    })
                    .await?;
            }

[da-kami]

The problem is that process manager instead of the monitor::Actor is making decision to broadcast the refund transaction.

Based on what is this decision made? I thought it always bases the decision on events?

I still think that the problem comes from the model though - we should not have the refund transaction available for broadcast if we have an attestation!

We might have to ensure that we process oracle attestations before we check for blockchain timelock expiry, but that's a different problem.

[thomaseizinger]

Based on what is this decision made? I thought it always bases the decision on events?

I still think that the problem comes from the model though - we should not have the refund transaction available for broadcast if we have an attestation!

Yes the decision is always based on the events that we are producing.

Nope. This could be solved performing a check in the "TryBroadcastTransaction" handler.

That would completely change the scope of the monitor actor and scatter business logic all of the place. There is two ways of how we can model these actors:

1. As "servants": They react to messages that tell them to do stuff and they will just do that. Like TryBroadcastTransaction. (This is what we currently do.) The upside of this is that these actors are really simple and there are no nasty surprises. If you are using such an API, you expect the transaction to be broadcasted. It keeps the API simple and unopinionated which fosters reuse in different usecases.
2. As part of the business logic: Instead of telling them what to do, you let them know about which event happened and they reason about the event themselves. This couples the components more to the business logic.

I prefer option (1) because it creates more independent components. Most certainly we should not mix the two, that will just end in absolute spaghetti code on what conditions are checked where.

[rishflab]

Yep, agreed that (1) is a better design.

[da-kami]

What I am saying is the process manager should just forward events to relevant actors leaving them to make the business decision as the actors can build aggregates and are therefore in a better to make a business decision.

That I agree with, but I would try to keep an actor's business logic to a minimum and cover most cases (including tests) of what is "possible" and what not in model::Cfd. We always base our decision on events - and the events are produced by model::Cfd. We can reason on the history of events, but we should first ensure that the events produced are actually sane.

If reasoning logic is necessary for prioritizing transactions then the monitor, as you say, is the right place for this. But I would ask: Why was the event that gave you refund available in the first place?

[thomaseizinger]

We always base our decision on events - and the events are produced by model::Cfd. We can reason on the history of events, but we should first ensure that the events produced are actually sane.

If reasoning logic is necessary for prioritizing transactions then the monitor, as you say, is the right place for this. But I would ask: Why was the event that gave you refund available in the first place?

Yes, this is the correct way to think about this problem IMO.

Emitting an event is a state change to the system. If the state change is not correct, we should not emit the event. If we cannot make the correct the decision on what to do after an event, then we maybe need to split up the event to convey more meaning? That is what we did for the attestation and the CET timelock.

I think it is correct to add another check to the refund_timelock_expired command handler to ensure we don't have a CET already. However, I don't think this will solve #1230 because #1230 is about a race condition on startup that never happens under continuous operation because the time of attestation and the refund timelock are purposely apart from each other.

[rishflab]

If we cannot make the correct the decision on what to do after an event, then we maybe need to split up the event to convey more meaning?

What is the advantage of this strategy?

One disadvantage I can see is that you need to ensure your events contain enough information for future business logic decisions. This is inflexible and anytime we get this wrong we will have to change our data model.

The alternative would be to have lean events which can be used to build a state (model::Cfd). You make your business decisions against this state. All you need to consider when designing an event is whether or not it is an "irreversible" fact that something happened. You don't need think of how it will be used or if it will be used.

...then we maybe need to split up the event to convey more meaning? That is what we did for the attestation and the CET timelock.

I am seeing this as combining not splitting. OracleAttestedPriorCetTimelock and OracleAttestedPostCetTimelock are the two permutations of the OracleAttested and CetTimelock event.

[thomaseizinger]

One disadvantage I can see is that you need to ensure your events contain enough information for future business logic decisions.

Yes you are picking up on a good point. There is an idea of "Event-Driven State Transfer" mentioned in https://www.youtube.com/watch?v=STKCRSUsyP0 that we partly following here.

...then we maybe need to split up the event to convey more meaning? That is what we did for the attestation and the CET timelock.

I am seeing this as combining not splitting. OracleAttestedPriorCetTimelock and OracleAttestedPostCetTimelock are the two permutations of the OracleAttested and CetTimelock event.

The consequence of more fine-granular events is that each event consumer needs to also keep a record of all events to be able to reason about their order and thus about what behaviour needs to be triggered.

It is a trade-off and for now, we have opted for containing that reasoning within the Cfd model and emit events that precisely describe what happened. Maybe we should not do that though. An alternative I can see is emitting only events that describe the state change and have each interested party rebuild the aggregate and issue queries against it like: "Are you holding a CET that needs to be broadcasted in your current state?".

This would result in an execution flow like:

• oracle::Actor: Fetches an attestation and calls decrypt_cet
• Event gets produced and we save it to the DB
• A list of actors gets notified that the CFD with a particular ID changed
• Each actors rehydrates the CFD and performs actions based on the latest state

Likely, we would want those actors to sit inbetween the notification and the actual "servant" actor like monitor. Say for example, we have a CetBroadcastProcessActor. It would receive the state change notification, rehydrate the (model) CFD, check for CETs that need to be broadcasted and if so, send a message to the corresponding actor for broadcasting transactions.

I think you'd want this kind of decoupling to not overload actors like monitor with functionality.