Request for comment on trade protocol domain design

[HenrikJannsen]

I would like to give an overview about the trade protocol design and ask for feedback and comment

The Protocol class inherits from a finite state machine implementation (Fsm) and holds a reference to a Trade object.
The Trade inherits from the FsmModel which holds the State. The state can only be set by the Fsm at a state transition.
The Trade holds 2 TradeParty objects for maker and taker and the Contract. Fields for trade data in TradeParty and Trade are mutable and get filled during the protocol progress like ContractSignatureData.
TradeParty holds immutable the usersNetworkId

The Contract contains the Offer and data when the offer gets taken (e.g. amount, payment method,...).

The BisqEasyTradeService (for BisqEasy protocol - there is no common TradeService base class yet) provides an API for triggering the events which trigger the state transition on the Protocol. It also listens on network messages and use the messages the same way as user events are used for triggering a state transition.

The definition of the state transistions is done in the concrete Protocol implementation for the relevant role:
BisqEasyBuyerAsMakerProtocol, BisqEasyBuyerAsTakerProtocol, BisqEasySellerAsMakerProtocol, BisqEasySellerAsTakerProtocol in case for the BisqEasy domain.

There is different handling dependent on the state in the protocol for maker and taker or for buyer and seller, thus we have 4 variants in total (2 pairs).

The transition config for BisqEasySellerAsTakerProtocol is for instance:

public void configTransitions() {
        addTransition()
                .from(INIT)
                .on(BisqEasyTakeOfferEvent.class)
                .run(BisqEasyTakeOfferEventHandler.class)
                .to(TAKER_SEND_TAKE_OFFER_REQUEST);

        addTransition()
                .from(TAKER_SEND_TAKE_OFFER_REQUEST)
                .on(BisqEasyAccountDataEvent.class)
                .run(BisqEasyAccountDataEventHandler.class)
                .to(SELLER_SENT_ACCOUNT_DATA);

        addTransition()
                .from(SELLER_SENT_ACCOUNT_DATA)
                .on(BisqEasyConfirmFiatSentMessage.class)
                .run(BisqEasyConfirmFiatSentMessageHandler.class)
                .to(SELLER_RECEIVED_FIAT_SENT_CONFIRMATION);

        addTransition()
                .from(SELLER_RECEIVED_FIAT_SENT_CONFIRMATION)
                .on(BisqEasyConfirmBtcSentEvent.class)
                .run(BisqEasyConfirmBtcSentEventHandler.class)
                .to(SELLER_SENT_BTC_SENT_CONFIRMATION);

        addTransition()
                .from(SELLER_SENT_BTC_SENT_CONFIRMATION)
                .on(BisqEasyBtcConfirmedEvent.class)
                .run(BisqEasyBtcConfirmedEventHandler.class)
                .to(BTC_CONFIRMED);

        addTransition()
                .from(BTC_CONFIRMED)
                .on(BisqEasyTradeCompletedEvent.class)
                .to(COMPLETED);
    }

The peers protocol is: BisqEasyBuyerAsMakerProtocol with following transition config:

public void configTransitions() {
        addTransition()
                .from(INIT)
                .on(BisqEasyTakeOfferRequest.class)
                .run(BisqEasyTakeOfferRequestHandler.class)
                .to(MAKER_RECEIVED_TAKE_OFFER_REQUEST);

        addTransition()
                .from(MAKER_RECEIVED_TAKE_OFFER_REQUEST)
                .on(BisqEasyAccountDataMessage.class)
                .run(BisqEasyAccountDataMessageHandler.class)
                .to(BUYER_RECEIVED_ACCOUNT_DATA);

        addTransition()
                .from(BUYER_RECEIVED_ACCOUNT_DATA)
                .on(BisqEasyConfirmFiatSentEvent.class)
                .run(BisqEasyConfirmFiatSentEventHandler.class)
                .to(BUYER_SENT_FIAT_SENT_CONFIRMATION);

        addTransition()
                .from(BUYER_SENT_FIAT_SENT_CONFIRMATION)
                .on(BisqEasyConfirmBtcSentMessage.class)
                .run(BisqEasyConfirmBtcSentMessageHandler.class)
                .to(BUYER_RECEIVED_BTC_SENT_CONFIRMATION);

        addTransition()
                .from(BUYER_RECEIVED_BTC_SENT_CONFIRMATION)
                .on(BisqEasyBtcConfirmedEvent.class)
                .run(BisqEasyBtcConfirmedEventHandler.class)
                .to(BTC_CONFIRMED);

        addTransition()
                .from(BTC_CONFIRMED)
                .on(BisqEasyTradeCompletedEvent.class)
                .to(COMPLETED);
    }

One can see the relevant role for the first transition is taker/maker but the following transitions are defined by seller/buyer role.
There might be more differences in other more complex protocols.

The event or message is defined by it's Class as well as the optional event handler. That way we avoid boilerplate enums.
By convention we use the same name for the handler as the event with a "Handler" postfix.
The domain specific State is implemented as an enum.
The EventHander gets the TradeModel and a ServiceProvider (a collection of Bisq service classes) at the constructor.
The Event gets the relevant data from the user event or message passed at creation. The handle method in the EventHander gets passed the Event and can take the specific data for processing.

For messages we extend TradeMessageHandler which provides a verifyMessage method which should be used for message validation before the message gets processed.
The changes to the model should be applied in the custom commitToModel method to avoid that an exception at processing would cause an invalid state in the model. The state transition is only performed if the eventHandler completed without exception, but if there would be changes to the model we would risk inconsistent state. There might get added more structure for enforcing that verify -> process -> commit patter. In the current BisqEasy handlers there is no complexity to justify that structural overhead but I guess for more complex protocols it will be useful.

There is lack of generic support for the concrete event in the eventHandlers handle method. I tried to generify that but failed as the transition is in a map with different types and thus makes it difficult to apply generics. If any dev finds a solution it would be welcome. But the unsafe cast is not a real risk as the Event and Handler classes are tightly coupled by convention.

I added skeleton implementations for the Bisq multisig protocol and a potential LN submarine swap protocol. Beside the trade module its also supported in offer and contract. If any dev is interested to try to implement a prototype protocol for those trade protocols would be helpful to test the concept if it is flexible enough and if it works well.

There are no tests yet for the BisqEasyProtocol (only for the Fsm). It would be good to get a test suite for testing the whole protocol flow with potential failure modes. Help from other devs is very welcome.

My next step is to integrate it to the UI and see if all works as expected.

Compared to Bisq 1 the EventHander does perform the function of the Task classes. For more complex processing we can either break it up with multiple states (in case the processing cannot be done without irreversible changes like a commit of a tx) or to break inside the EventHander the processing into more manageable chunks.

What do you think about the concept?

[alvasw]

I looked through all classes and conceptually everything is fine. Good job! 👍
I didn't play with the framework enough to say whether anything is missing.

However, I have one question: What happens if two messages arrive out of order?

As an example, imagine a BisqEasyAccountDataMessage and BisqEasyConfirmFiatSentEvent arrive at the same time. The BisqEasyTradeService would call FSM.handle(...) twice and only the BisqEasyAccountDataMessage would get processed, because the BisqEasyBuyerAsMakerProtocol is in the MAKER_RECEIVED_TAKE_OFFER_REQUEST state. The BisqEasyAccountDataMessageHandler only commits data and doesn't send any data. On another thread, the BisqEasyConfirmFiatSentEvent would have triggered a FsmException, and the BisqEasyConfirmFiatSentEvent is gone. Are we in an deadlock now?

Other small things

• You can eliminate the lock object by synchronizing on the FSM instance itself (by replacing synchronized (lock) { with synchronized {.
• The FSM constructor calls configTransitions(), so the whole FSM is built before another thread can call FSM.handle(...). This means you don't need to aquire the lock in FSM.addTransition(Transition transition).
• The FSM and Protocol class both have a model field and they are the same. You can remove the model field from the Protocol class and change the visibility of the model field in the FSM class to protected.
• The ServiceProvider's constructor has an unused PersistenceService argument. You can add a field for the PersistenceService or remove constructor argument.
• The Trade class has theisBuyer and isTaker boolean fields. You can remove them and the getters can call role.isBuyer() and role.isTaker()
• The NetworkId class has a @ToString annotation and a toString method too.

[HenrikJannsen]

However, I have one question: What happens if two messages arrive out of order?

Good point. At the current protocol there are no 2 messages for one side which could follow as there is a user action always between.
The BisqEasyAccountDataMessage is received by the buyer and the BisqEasyConfirmFiatSentEvent is received by the seller. The next message received by the buyer would be the BisqEasyConfirmBtcSentMessage but as the user need to trigger the BisqEasyConfirmFiatSentEvent by an UI event first to proceed in the protocol there is no way that both messages could compete.

But there could be such situations in other protocols (though the usual way would be a request/response pattern). Even with request/response pattern it could be that the messages are triggered by non user actions but e.g. blockchain events and then such critical situation could become more feasible as well.

As all messages are mailbox messages and do not require the peer to be online the order of the processing of the mailbox messages is not defined, so that would be definitely a problematic situation in case of multiple messages.

One approach could be to queue up messages which arrive at the wrong state (but would match a future state) and retry with the messages of that queue after the next state transition. That queue need to be persisted as well to avoid that those messages are lost in case if a restart.

We likely will need also ACK messages for successfully processed messages (and/or when mailbox messages are processed). This might be another way how to deal with it to resend messages which do not got an ACK after a certain time, but that would only work if the sender is online. So the previous approach is probably better.

Do you have some other idea how to deal with that?

Other small things

Thanks! will apply those in the next PR.

The Trade class has theisBuyer and isTaker boolean fields. You can remove them and the getters can call role.isBuyer() and role.isTaker()

Role is not persisted yet, but maybe better to persist that role instead the 2 booleans.

[alvasw]

Do you have some other idea how to deal with that?

This sounds the best to me:

One approach could be to queue up messages which arrive at the wrong state (but would match a future state) and retry with the messages of that queue after the next state transition. That queue need to be persisted as well to avoid that those messages are lost in case if a restart.

And

We likely will need also ACK messages for successfully processed messages (and/or when mailbox messages are processed). This might be another way how to deal with it to resend messages which do not got an ACK after a certain time, but that would only work if the sender is online. So the previous approach is probably better.

Additionally, we could add a sequence number if needed. I don't think it's necessary for simple protocols.

Role is not persisted yet, but maybe better to persist that role instead the 2 booleans.

This is not important (only makes the code easier to read).

[HenrikJannsen]

After discussion with @sqrrm how that protocol would work with more sophisticated trade protocols like the chained LN->Liquid->LN protocol idea we to the conclusion that it should work fine as a composition of 3 protocols and we could reuse the 3 single standalone protocols in the chained protocol.

So the first would be a reverse submarine swap (RSMS) from LN-BTC to Liquid BTC.
The second the Bisq Multisig protocol (BMS) on Liquid BTC (L-BTC <-> FIAT).
Then finally a submarine swap (SMS) from Liquid BTC back to LN-BTC.

All those 3 protocols should be implemented as standalone protocols first.

Once we have those in place we can chain them together so that for the user its just a LN-BTC to FIAT trade where they get back the security deposit as LN-BTC and they do not need to care about Liquid.

To chain those 3 protocols we would first start the RSMS protocol. Once that is completed it triggers the start of the BMS protocol. Once that is completed it triggers the SMS protocol and once that is completed the trade is complete for the user.

As there are not only 2 traders involved but potentially 4 (RSMS and SMS are done with some L-BTC liquidity providers) it might get a bit more complicated in case that there is not enough liquidity in the market to execute the swaps immediately, but I guess beside time delay it would not be a fundamental problem. We could also use centralized providers like Bolz as fallback in case the Bisq market lacks liquidity.

We could also build hierarchical state machines, where for complex handlers a sub-fsm is used to execute those steps. But I guess more appropriate would be to split up those step into multiple state transitions.
Specially for tasks where we do an irreversible state change (like a tx commit to the wallet) which would follow up further steps which could lead to an exception and would lead then to a invalid state as the model (wallet) has already partly been changed. I think such cases are clearly a sign that it need to be separated into multiple state transitions. So the pattern: verify input -> process -> commit must be followed. At the commit phase there is likely no possibility for exceptions and it must be done in a synchronized block. To make it atomic we would need to clone the model and replace it then but I guess thats too much overhead and not needed.