[r2r] Zhtlc orders is_mine bug

[laruh]

ZHTLC orders have ismine:false #1461

"is_mine":true for orderbooks with ZHTLC coins when we use methods: setprice, sell, buy.

Deadlock could appear even, when we have one order with ZHTLC coin.
When deadlock happens, we can not sell ZHTLC or buy other coin with ZHTLC. We can use sell or setprice for other coin with rel ZOMBIE and buy ZOMBIE. However, in other cases nothing happens, command just doesn't fail or pass.
For me deadlock happened after 2.5 hours after ZOMBIE activation.
After deadlock we cannot enable ZOMBIE again, it stucks on UpdatingBlocksCache stage (InProgress issue). We have to delete DB cache from folder to fix it.

[artemii235]

It also turns out that OnCompactBlockFn doesn't have to be Sync, so blocks_db doesn't need to be wrapped in Mutex. Please add this diff to the current PR: bea93bc

[artemii235]

Next review iteration 🙂

[artemii235]

Few more notes.

[artemii235]

Thanks for the fixes, great work!

[artemii235]

@ozkanonur Could you take a look too, please?

[onur-ozkan]

Thanks for the fix!

Only one blocker.

[onur-ozkan]

It's only used in the current module, therefore we can avoid defining the function as pub. It could be also great if you can inline this function.

[laruh]

oh, I see, this function should be private.
But in this case, its not so necessary to inline private function according to Kladov's explanation and recommendations
As main usage of #[inline] comes from — it enables cross-crate inlining.
it usually isn’t necessary to apply #[inline] to private functions — within a crate, the compiler generally makes good inline decisions.. There’s a joke that LLVM’s heuristic for when the function should be inlined is “yes”.

Please, correct me, if I'm wrong in my understanding.

[onur-ozkan]

oh, I see, this function should be private. But in this case, its not so necessary to inline private function according to Kladov's explanation and recommendations As main usage of #[inline] comes from — it enables cross-crate inlining. it usually isn’t necessary to apply #[inline] to private functions — within a crate, the compiler generally makes good inline decisions.. There’s a joke that LLVM’s heuristic for when the function should be inlined is “yes”.

Please, correct me, if I'm wrong in my understanding.

If you want to make an order to compiler that function should be inlined, then you use #[inline(always)].

If you want to suggest to compiler that function should be inlined, then you use #[inline] without any flag.

If you want to make an order to compiler that function should never be inlined, then you use #[inline(never)]

If the function is public, without inline/inline(always) attribute it will never be inlined to external crates unless you are using link time optimizations.
But that doesn't mean all in-crate functions are inlined. Inline attributes are useful even for private functions if needed.

Anyway, in this example because of the function context is all about comparison, I can tell you this function will be inlined by the compiler even if you don't give an attribute.
But I personally start giving inline suggestions(or using always flag for strict order) to compiler for even private functions. Because as I said it might not be inlined without giving attribute, and in case you will need to do some compiler or binary level optimizations, you will have less magical/high-level behaviours from compiler without having any knowladge.

[laruh]

Oh, got your point. I added #[inline(always)].

[sergeyboyko0791]

Great work!
Just one question and a few minor changes

[sergeyboyko0791]

We can clone the my_p2p_pubkeys collection to avoid deadlocks

[artemii235]

Cloning can introduce data inconsistency.
As an alternative, we can try moving my_p2p_pubkeys to the orderbook. It will fit the Orderbook structure context.

[onur-ozkan]

Cloning MutexGuard should be safe.

If we can do something like:

diff --git a/mm2src/mm2_main/src/lp_ordermatch/orderbook_rpc.rs b/mm2src/mm2_main/src/lp_ordermatch/orderbook_rpc.rs
index dbebf4078..484cc4f9f 100644
--- a/mm2src/mm2_main/src/lp_ordermatch/orderbook_rpc.rs
+++ b/mm2src/mm2_main/src/lp_ordermatch/orderbook_rpc.rs
@@ -311,7 +311,6 @@ pub async fn orderbook_rpc_v2(
         .map_to_mm(OrderbookRpcError::P2PSubscribeError)?;

     let orderbook = ordermatch_ctx.orderbook.lock();
-    let my_orders_pubkeys = ordermatch_ctx.my_p2p_pubkeys.lock();
     let my_pubsecp = ctx.secp256k1_key_pair_as_option().map(|_| {
         CryptoCtx::from_ctx(&ctx)
             .expect("ctx is available")
@@ -337,7 +336,7 @@ pub async fn orderbook_rpc_v2(
                         continue;
                     },
                 };
-                let is_mine = is_my_order(&my_orders_pubkeys, &my_pubsecp, &ask.pubkey);
+                let is_mine = is_my_order(&ordermatch_ctx.my_p2p_pubkeys.lock(), &my_pubsecp, &ask.pubkey);
                 orderbook_entries.push(ask.as_rpc_v2_entry_ask(address, is_mine));
             }
             orderbook_entries
@@ -367,7 +366,7 @@ pub async fn orderbook_rpc_v2(
                         continue;
                     },
                 };
-                let is_mine = is_my_order(&my_orders_pubkeys, &my_pubsecp, &bid.pubkey);
+                let is_mine = is_my_order(&ordermatch_ctx.my_p2p_pubkeys.lock(), &my_pubsecp, &bid.pubkey);
                 orderbook_entries.push(bid.as_rpc_v2_entry_bid(address, is_mine));
             }
             orderbook_entries

We can avoid leaving my_p2p_pubkeys as locked for longer than we need.

[sergeyboyko0791]

Since we lock the OrdermatchContext::orderbook mutex, we can be sure that it won't change during this function executes. Hence the my_p2p_pubkeys collection won't change.
Am I missing something?

Cloning can introduce data inconsistency.

[artemii235]

@sergeyboyko0791 These mutexes are not locked simultaneously in all cases. For example:
https://github.com/KomodoPlatform/atomicDEX-API/blob/33fdfe8ff64093744454587da5babd9f07019ed1/mm2src/mm2_main/src/lp_ordermatch.rs#L4479-L4490

So here, orderbook is locked in maker_order_created_p2p_notify and a new order is added there. It seems that we already have data inconsistency because there is a time window when order is already in the Orderbook, but its pubkey is not in my_p2p_pubkeys, so we can get a wrong is_mine:false from RPC.

I think it's one more reason to move my_p2p_pubkeys to Orderbook, create fn like

fn insert_or_update_order_my(ctx: &MmArc, item: OrderbookItem, my_order: &MakerOrder) {
    let ordermatch_ctx = OrdermatchContext::from_ctx(ctx).expect("from_ctx failed");
    let mut orderbook = ordermatch_ctx.orderbook.lock();
    orderbook.insert_or_update_order_update_trie(item);
    if let Some(key) = my_order.p2p_privkey {
        orderbook.my_p2p_pubkeys.insert(hex::encode(key.public_slice()));
    }
}

And use it in maker_order_created_p2p_notify.

[sergeyboyko0791]

Now I see what you meant by data inconsistency!

I like this idea

I think it's one more reason to move my_p2p_pubkeys to Orderbook, create fn like

[artemii235]

@laruh could you please implement the forementioned change suggestion? 🙂

[laruh]

Moved my_p2p_pubkeys to Orderbook.
In accordance with the keys addition in maker_order_created_p2p_notify, we remove pubkeys in maker_order_cancelled_p2p_notify. In addition, I decided to keep adding pubkeys in orders_kick_start function.

Only one moment. In fn lp_ordermatch_loop we use maker_order_created_p2p_notify to notify other nodes, and later we use only delete_my_maker_order function. I think to some extent we could have data inconsistency. But as Sergey said forgetting to delete the data is not as bad as skipping it.

[sergeyboyko0791]

Here the same

[artemii235]

This likely last review iteration 🙂

[artemii235]

Good work, thanks! @sergeyboyko0791 @ozkanonur waiting for your next review iterations 🙂

[sergeyboyko0791]

Awesome!

[onur-ozkan]

LGTM