Fix orders disappearing from orderbook

[shamardy]

fixes #1044
This fixes pair trie to be consistent across old and new nodes preventing inconsistent behavior in keeping orders alive

[sergeyboyko0791]

@shamardy could you please merge the dev branch into your branch?

[sergeyboyko0791]

Thanks for this fast fix!
Please answer my comments :)

[sergeyboyko0791]

How about renaming this function to without_protocol_info or clone_without_protocol_info?
It may confuse a bit, because remove means changing itself.

[sergeyboyko0791]

Correct me please if I'm wrong. I think there can be a not bad but not good case if we don't store the base/rel protocol info in a memory_db.

Let's there are three nodes in a network: our light node A, another light node B, seed node S.

1. B broadcasts a few Maker orders to the network. A and S successfully sync them by process_keep_alive.
2. A goes offline.
3. B broadcasts a new Maker order, S syncs it, therefore stores the order in Ordermatch::memory_db on process_trie_delta. Note that the order is stored without protocol info since we use rmp_serde::to_vec(&order.remove_protocol_info()).
4. A connects to the S and requests the diff of the orderbook on process_orders_keep_alive from S node
5. S extracts the missed order without protocol info and send it to A
6. A generates and broadcasts a Taker order to match the B's Maker order BUT without the protocol info.
7. I guess, Bdoesn't match the Taker order since there are no base/rel protocol infos (not sure about it)

[shamardy]

I think it will match because in point no. 6 A will generate and broadcast a Taker order with its own protocol info and on the maker side B will check if it can match with A or not. So A doesn't use protocol info. of B in TakerRequest, A only sends its own protocol info to let B know if it's an old node when protocol info of A is None thus not matching B Segwit orders only in this case.
I will try to add a test case for the above scenario to check it but it might be tricky disconnecting and connecting A again in testing.

[sergeyboyko0791]

Have you tried to reproduce this? If you are busy now, I can do this)

[sergeyboyko0791]

As we discussed with @shamardy, it may be difficult to reproduce the case above, because the A node should be disconnected without restarting it.
@cipig @tonymorony could you please try to reproduce this case?

[shamardy]

@sergeyboyko0791 Before this last commit aefe460, Segwit orders were not sent to the old nodes in response to GetOrderbook and BestOrders. But I found out that in the case of GetOrderbook this will cause old nodes to have a different trie from the new nodes which will cause problems in the PubkeyKeepAlive messages.

I suggest removing base_protocol_info and rel_protocol_info from MakerOrderCreated and OrderbookItem since the only use for them is displaying the address in the right format in the orderbook. If this is done Segwit orders won't match anyway with the old nodes because of the check for protocol_info in TakerRequest and MakerReserved. To solve the address problem we can show both addresses Legacy/Segwit in the orderbook if the coin supports Segwit. What do you think?

I think this is the only viable solution as I checked if adding versions to differentiate messages from old and new nodes like here https://github.com/KomodoPlatform/atomicDEX-API/blob/aefe4600a32da0c99c890aee4bfe86f8c5279e8a/mm2src/lp_ordermatch.rs#L429 to one of PubkeyKeepAlive and SyncPubkeyOrderbookStateRes or both will solve the problem but it won't as it will either lead to different pair tries again or address not shown correctly.

As for the last commit aefe460, it has no problems except that Legacy address will be shown in the orderbook instead of Segwit if the OrderbookItem was received through PubkeyKeepAlive and SyncPubkeyOrderbookStateRes. Other than that both old and new nodes have the same pair trie and everything should work fine.

[sergeyboyko0791]

I suggest removing base_protocol_info and rel_protocol_info from MakerOrderCreated and OrderbookItem since the only use for them is displaying the address in the right format in the orderbook.

I'd rather vote for this solution - to remove base_protocol_info and rel_protocol_info at all.

As for the last commit aefe460, it has no problems except that Legacy address will be shown in the orderbook instead of Segwit if the OrderbookItem was received through PubkeyKeepAlive and SyncPubkeyOrderbookStateRes. Other than that both old and new nodes have the same pair trie and everything should work fine.

It will look like a bug if orders that actually support Segwit addresses, will contain Legacy or Segwit addresses depending on whether the order was received through OrdermatchRequest::SyncPubkeyOrderbookState or OrdermatchRequest::GetOrderbook.

What do you think @tonymorony?

[shamardy]

@sergeyboyko0791 I have an idea for viewing the correct address for Segwit maker orders that we can open an issue with once this is merged so we can discuss it with @artemii235 in the future. It involves adding the protocol_info fields again to MakerOrderCreated only without adding them to OrderbookItem, then we can insert the protocol_info from MakerOrderCreated inside a new hashmap field in OrderbookPubkeyState which will be separate from the trie_roots. We can then return them to new nodes only in GetOrderbookRes and SyncPubkeyOrderbookStateRes. I think this will work 🙂 and it should be backward compatible with any future releases, I wrote this comment for future reference.

[sergeyboyko0791]

LGTM! let's merge it into dev :)

[artemii235]

I have one question.

[artemii235]

@shamardy Could you please clarify, what was the reason to remove if let Ok(Some((base, rel))) = find_pair(&ctx, &maker_order.base, &maker_order.rel).await? It opens a possibility to have an unmatchable maker order in the orderbook unless the coins are activated.

[shamardy]

I removed it because I added it for maker order protocol info. on this PR #1017 as it wasn't there before. But will add it again to this PR #1054 as we agreed since it appears to have another purpose.