[WIP] Don't ask for trade volume if non positive price, fix #2925

[sqrrm]

I have been able to test this, not sure how to provoke the 0 price trade. Seems reasonable to return null amount for non positive prices though.

[a123b]

Wouldn't it be much better to fix this by ensuring that the tradePrice property can never be non-positive? Negative or zero values don't make any sense whatsoever and might cause even bigger problems somewhere else in the code...

[devinbileck]

I agree it would be ideal to find and resolve the underlying issue. I am wondering if it is similar to #2915 in that something wasn't completely initialized when the taker took the offer, hence a null peer address and invalid trade price. Perhaps we need an additional check in https://github.com/bisq-network/bisq/blob/master/desktop/src/main/java/bisq/desktop/util/GUIUtil.java#L737

[devinbileck]

I think it may be due to the fact that if the price is not in tolerance, it does not set the trade price.
https://github.com/bisq-network/bisq/blob/master/core/src/main/java/bisq/core/trade/protocol/tasks/maker/MakerProcessPayDepositRequest.java#L85

Perhaps should setTradePrice prior to checkTradePriceTolerance.

[ManfredKarrer]

@devinbileck
No setTradePrice have to be after the check as we don't want to write a possible incorrect price into the trade object. If the price provider never delivered a price the check will fail but I don't remember to have seen that ever and I am not aware of any issues with provider nodes, they run usually very stable.

[ManfredKarrer]

To add the extra check in getTradeVolume would return a null and it can be that this will cause other issuer later, so I am not sure if that fix would make the problem better. I agree we should find out what happened exactly. If the user can provide the full logs that might help. If the price was never delivered that should be visible.

[a123b]

I think I know what's happening:

• TradeManager adds trade in handlePayDepositRequest()
• Price disagreement leads to NullPointerException (see my comment on #2910)
• Thus, cleanupTradableOnFault() never gets called in TradeProtocol.java#L173 and the trade falsely stays in the TradeManager/Open Trades list
• On the next handlePayDepositRequest(), the trade gets added again, so we have it in the TradeManager twice: One correct version and one version without a price

Thoughts?
If this is correct, do you think my fix of the underlying NPE in #2928 is enough or should we ensure that whenever a trade is added to the TradeManager's tradableList, already existing entries with the same ID are removed first to prevent possible similar issues like this in the future?

[ManfredKarrer]

@a123b Great finding! Would be best if you can reproduce that on a local regtest setup to verify the assuption but looks likely that this is a correct analysis. Once reproduceable it's easier to see what path is the best to fix it. The price disagreement should already clean up the trade state, so I think that might be the best place to fix it.

To strategy to fail in case of uncertainty is intentional. Better to end up in a dispute where a human arbitrator can resolve the issue than that the protocol continues and make a wrong payout.

[sqrrm]

I have managed to reproduce it using the following steps in regtest:

1. Alice create offer
2. Bob, running modified source doubling the price when taking offers, tries to take the offer. Alice sees the same issue as Major errors happening when user tries to accept trade. #2910
3. Bob restarts running from unmodified master and takes the same offer. Alice sees the same issue as Locked up funds from a failed trade #2925

[sqrrm]

@a123b After reproducing and following what's going on you seem to be spot on. The fix in #2928 solves all of the issues as you describe in #2910 (comment) and #2926 (comment)

This PR solves the issue of displaying the already erroneously added 0 price trade item which could be nice for the users who already has such trades in their history. We could alternatively look at purging the saved trades for those users.

[sqrrm]

I opened another PR for the purging of failed trades due to price disagreement at #2930

I think that one makes the most sense to merge since it will fix the issues users with broken trades.

@a123b I wasn't able to suggest you as a reviewer, I suppose you're not in the bisq org. Consider joining, your contributions are great.

[sqrrm]

When I think about it, perhaps it makes more sense to just make a separate build for those with broken histories from that branch to avoid affecting everyone else. Once run and the purged trade list is saved they can return to using the main release.