fix: added check for peer's pairs on order invalidation

[rsercano]

attempts to fix #1530

Not sure if there's any other order event to check, also it would be great if you can tell me if I have to check pair's active status before emitting the event.

[sangaman]

Thanks for addressing this, it's a minor fix but makes sense and will make the logs cleaner.

[raladev]

04/09/2020 13:07:09.941 [P2P] verbose: received node state update packet from 03bdf0e1b1338711cd4374802b6467793fe455f3c7adfd67935e1822c7d12cfc16 (DawnAerobic): {"pairs":["ETH/BTC","LTC/BTC","LTC/USDT","BTC/USDT","USDT/ETH"],"addresses":[{"host":"7w6fk4s6tvpsnesw55voeluvruv3kxtbgdk3gacydcl6wekndns5x7ad.onion","port":28885}],"connextIdentifier":"indra6Wt45v28puTyh5A5dpLJmW8kNfss8vQeS5zGCLNWSUY4tDHDfg","lndPubKeys":{"BTC":"02cbeda716bb4d455f4d45decac5f58b47f46bd2b8dd15951ed63c5413065a697d","LTC":"02cbeda716bb4d455f4d45decac5f58b47f46bd2b8dd15951ed63c5413065a697d"},"lndUris":{"BTC":["02cbeda716bb4d455f4d45decac5f58b47f46bd2b8dd15951ed63c5413065a697d@ue36scvguve6rthvrdwxjsn4fru266ggyvyamotmqrzpgt24p3ip3hqd.onion:29735"],"LTC":["02cbeda716bb4d455f4d45decac5f58b47f46bd2b8dd15951ed63c5413065a697d@wuqkkmuthtmzlvity5g2mwvcqgvccuxyta4asadrldgd6z4exlnya7ad.onion:30735"]},"tokenIdentifiers":{"BTC":"bitcoin-simnet","ETH":"0x0000000000000000000000000000000000000000","LTC":"litecoin-simnet","USDT":"0x61F6b6D86E29fC62224943583F97966F093cC94D"}}


04/09/2020 13:07:09.942 [P2P] trace: Sent Get Orders packet to 03bdf0e1b1338711cd4374802b6467793fe455f3c7adfd67935e1822c7d12cfc16 (DawnAerobic): "{\"body\":{\"pairIds\":[\"USDT/ETH\"]},\"header\":{\"id\":\"8a8d3960-eeaf-11ea-8df1-fd04cc7ae94a\"}}"



04/09/2020 13:07:10.471 [P2P] trace: Received Orders packet from 03bdf0e1b1338711cd4374802b6467793fe455f3c7adfd67935e1822c7d12cfc16 (DawnAerobic): "{\"body\":[],\"header\":{\"id\":\"8abfe220-eeaf-11ea-8d17-c395fd7a8440\",\"reqId\":\"8a8d3960-eeaf-11ea-8df1-fd04cc7ae94a\"}}"
04/09/2020 13:07:10.472 [P2P] verbose: received 0 orders from 03bdf0e1b1338711cd4374802b6467793fe455f3c7adfd67935e1822c7d12cfc16 (DawnAerobic)

• after adding new pair, the peer with new pair sends packet update to his peers. And after that these peers send GetOrders packet to this peer even if they have no this pair. It would be good to not ask orders if we have no this new pair.
  Should we create new issue or we can fix it it this PR? @rsercano @kilrau

• Also it would be good to send GetOrders packet from peer that added pair (if remote peer has this pair). It will help to fix this issue - No active orders from peers after adding new pair #1538

[raladev]

above

[sangaman]

* [ ]  Also it would be good to send GetOrders packet from peer that added pair (if remote peer has this pair). It will help to fix this issue - #1538

I would do this in a separate PR, although this is a good idea. Just don't want this one to get too big or complicated.

[rsercano]

after adding new pair, the peer with new pair sends packet update to his peers. And after that these peers send GetOrders packet to this peer even if they have no this pair. It would be good to not ask orders if we have no this new pair.
Should we create new issue or we can fix it it this PR? @rsercano @kilrau

Alight, I'll check this point within this PR.

[rsercano]

I added a new check as per @raladev's request, now we won't send NodeStateUpdate packet when adding/removing a pair if this pair is not active in the peer.

[raladev]

I added a new check as per @raladev's request, now we won't send NodeStateUpdate packet when adding/removing a pair if this pair is not active in the peer.

Not sure that it is correct fix, we still want to get info about pairs that we dont have, but we dint need info about orders. So, more correct would be remove GetOrders call in this case

[rsercano]

I added a new check as per @raladev's request, now we won't send NodeStateUpdate packet when adding/removing a pair if this pair is not active in the peer.

Not sure that it is correct fix, we still want to get info about pairs that we dont have, but we dint need info about orders. So, more correct would be remove GetOrders call in this case

I see, let me check it this way then

[rsercano]

fixed

[raladev]

fixed

but i still dont see the code for handling of GetOrders autocall on remote peer side.
It seems u just returned UpdatePacket without GetOrders handling.
Will test.

[rsercano]

I thought this would be sufficient https://github.com/ExchangeUnion/xud/pull/1864/files#diff-1934dba4817446d9f1708503332eb124R731

[raladev]

I thought this would be sufficient https://github.com/ExchangeUnion/xud/pull/1864/files#diff-1934dba4817446d9f1708503332eb124R731

• still see GetOrders packet sending :(
  

[raladev]

above

[rsercano]

Alright let me recheck

[sangaman]

This looks good to me. Let's handle the GetOrders packet in another PR, I think the change here can stand on its own and fixes the behavior with the order invalidation packets that are mentioned in the original issue.

[sangaman]

See #1884 which addresses your requested changes @raladev - would you mind approving this PR so we can merge these separately?

[sangaman]

I thought we didn't want to mess with nodestateupdate packets in this PR? I was ready to merge this before the most recent commit. Let's try to keep the scope of PRs small and manageable, we can open separate PRs for separate issues.

[rsercano]

I thought we didn't want to mess with nodestateupdate packets in this PR? I was ready to merge this before the most recent commit. Let's try to keep the scope of PRs small and manageable, we can open separate PRs for separate issues.

No no I didn't mean to mess with it, could you please check "files changed", I just was trying to sign the previous commits to make them mergeable.

[raladev]

I thought we didn't want to mess with nodestateupdate packets in this PR? I was ready to merge this before the most recent commit. Let's try to keep the scope of PRs small and manageable, we can open separate PRs for separate issues.

822418a is just a revert of 7b7e7fc

last commit is revert of not worked fix for GetOrder

[kilrau]

Merging is still blocked, because previous commits were not signed. Maybe you can squash commits into one first, then sign and force push to this branch. That should do the trick. @rsercano

[sangaman]

No no I didn't mean to mess with it, could you please check "files changed", I just was trying to sign the previous commits to make them mergeable.

Got it, sorry that commit just confused me. So the current changes look good to me, if you can squash and sign these commits like kilrau said that would be great. Otherwise I can do it sometime tomorrow.

[rsercano]

I'm opening a new one #1890 sorry for the confusion, squashing looks like a pain since there're several commits between my commits, or Im literally too bad to do it!