Fix issue with offers getting disabled for no apparent reason

[Android-X13]

See comments by Droidus (me) in this forum thread.

Steps to reproduce:

1. Create an offer (suppose that current market price is 20.000$)

2. Set a price trigger (say, 20.200$) and confirm

3. Change your mind and re-edit the offer, set a different price trigger (say, 21.000$) and confirm

4. You now unknowingly have two trigger prices waiting in a queue instead of one, and the first to be triggered is not the last one that you've set. If you wait a while and assuming the market price will go up, your offer will be "disabled" way before the market price reaches 21.000$ and will leave you wondering why that has happened.

I put "disabled" inside quotes because the offer is not exactly getting disabled but is definitely thrown off the offerbook, and the log states the following:

Market price exceeded the trigger price of the open offer.
We deactivate the open offer with ID XXXXX.
Currency: XXX;
Offer direction: XXX;
Market price: XXXXX.XXX;
Trigger price: <PREVIOUS_TRIGGER_PRICE>

However if you go to PORTFOLIO > OPEN OFFERS you will see that the offer is still "enabled", but the only way to really bring it back is by manually disabling and re-enabling it (assuming the market price has not yet reached the next in line price trigger).

If you keep editing the price trigger then your offer will keep getting randomly deactivated for no apparent reason at all. This is a real life scenario and a very frustrating one by the way because it was happening to my own offers lately (as the market price was fluctuating I kept changing the price trigger but without realizing what was happening under the hood, so I ended up with problematic offers that couldn't stay up for long).

set.remove does not work in this case, onRemovedOpenOffers method does nothing and openOffersByCurrency keeps getting larger after every modification of an open offer. So when onPriceFeedChanged is triggered it iterates through all openOffersByCurrency and since it finds all the canceled ones as well, a previous trigger price is used instead of the current one.

Tested live (I haven't setup regtest yet)

[ripcurlx]

This code change would work, but what I'm not getting right now is, why the remove is not working. It is the same object (in memory pointer) that is in the set and also the same in-memory object passed in the remove handler. It is not found in the set when looking with contains, but when retrieving it from the set via streams the objects are equal. What is even weirder is that the object I retrieve from the set is also not found via contains. 🤔

[ripcurlx]

Something must be different, as I can add it again only once. So something must have changed the hash which I haven't found yet.

[ripcurlx]

Even the hashCode is the same. No idea why this is not working as expected.

[Android-X13]

@ripcurlx I had the same question when trying to debug this code but after a while I stopped investigating when I figured I could just bypass this problem with the given solution. After examining the code and running some tests again, I think that the point of failure is the offer's state:

1. Create a brand new offer (or start Bisq with an already existing open offer) --> the offer's state initially is UNKNOWN.
2. Edit the offer --> its state becomes REMOVED
3. onRemovedOpenOffers is triggered and it tries to remove it from the set, but it finds an OpenOffer object whose contained Offer has a state property that is different than the initial, hence the hashcode being different (from my own tests I see that the hashcode is indeed different ?), equals not working, contains not working, remove not working.

OpenOffer excludes from equals and hashcode its own state field, but what about its contained Offer's state which has changed?

[Android-X13]

When I say to "bypass this problem" I don't mean this in a lazy way... removeIf is a legitimate way to remove an object when problems like this arise. I figured it's better to do it this way than messing with hashcode() and changing things that could potentially affect other parts of a huge project like this (with which I am not yet familiar).

[ripcurlx]

ah - I missed that the hashcode is generated and used internally in the HashMap during adding and the referenced object is not immutable. I hate this mutable stuff. I'll merge it the way you implemented it.

[ripcurlx]

ACK - Tested it locally on Regtest. Works as expected. 👍

[ripcurlx]

@Android-X13 Your last commit is not signed.

[ripcurlx]

ACK - again because of force push