PaymentAccount object removal from ObservableSet issue

[beingindot]

Issue:
#3461

Addresses issue by adding equals/hashcode override.
Also added one unit test.

fixes #3461

[freimair]

I just tried to reproduce the issue by only using your Test but without your fix. Turns out the test does not fail then either. Have you been able to reproduce the issue with the test?

(because I wanted to try if a @EqualsAndHashCode.Exclude protected String accountName; might do the trick as well.)

EDIT: it seems mockito does not memorize the account name, nor does it memorize anything else. Hence, the hash-method returns always the same (null being null being null). So it seems that this kind of test is pretty useless. @christophsturm do you have an opinion on that?

[julianknutsen]

Test needs some work so it will fail before your patch. I've given an example of one inline.

[beingindot]

Thanks @freimair for the test issue. Have resolved it now.

[freimair]

great! Now testing is good for something. thanks!

Could you try if my @EqualsAndHashCode.Exclude suggestion works? and if yes, update the PR accordingly?

[chimp1984]

This issues also happens at Uphold or MoneyGram. All payment methods using a list of tradeCurrencies.
After more debugging I found out that if you exclude selectedTradeCurrency in PaymentAccount with @EqualsAndHashCode.Exclude the issue is resolved.
But it is very weird as at a breakpoint at the remove method both accounts the one in the set and the one to remove are the same object identity and have exact the same data.
Anyone an idea whats going on here?

[chimp1984]

NACK
We should find the real reason for the issue and not reduce the check to id and date only. See comment above...

[chimp1984]

After further investigation it seems that ObservableSet behaves weird. If you copy the ObservableSet entries into a HashSet you can remove it.

   public void removePaymentAccount(PaymentAccount paymentAccount) {
        HashSet<PaymentAccount> temp = new HashSet<>(paymentAccountsAsObservable);
        boolean changed = temp.remove(paymentAccount);
        paymentAccountsAsObservable.clear();
        paymentAccountsAsObservable.addAll(temp);
        if (changed)
            persist();
    }

[beingindot]

After this change i could remove uphold/revolut without any issue.

What i believe is because of hashcode change, we are not able to remove. hashcode change can be because of some state change. so usually we restrict that to unique identifying fields (id, date in this case) to maintain hashcode contract.

I couldn't create moneygram account even in prod. will open a bug. #3598

[ripcurlx]

NACK

After this change i could remove uphold/revolut without any issue.

What i believe is because of hashcode change, we are not able to remove. hashcode change can be because of some state change. so usually we restrict that to unique identifying fields (id, date in this case) to maintain hashcode contract.

I couldn't create moneygram account even in prod. will open a bug. #3598

But as @chimp1984 mentioned above I'm not sure we should fix it like that with only using id and date. The account name cannot be changed in the interface so this can't be the problem of this bug IMO. Probably this part of the code doesn't need changes, but more the removePaymentAccount method in the User class as @chimp1984 pointed out. We have to find out why ObservableSet is behaving not as expected in that case.

  public void removePaymentAccount(PaymentAccount paymentAccount) {
        HashSet<PaymentAccount> temp = new HashSet<>(paymentAccountsAsObservable);
        boolean changed = temp.remove(paymentAccount);
        paymentAccountsAsObservable.clear();
        paymentAccountsAsObservable.addAll(temp);
        if (changed)
            persist();
    }

[beingindot]

I have done deep debugging of this issue.

• when you insert a payment account, object (with hashcode x) will be inserted in hashset with key as x.
• when remove method called, that time hashcode of the same object will be y (some state change). so when we look for that object in hashset it wont find that key y (because x only will be there as key).
  so remove wont happen.

paymentAccountsAsObservable is just a wrapper of HashSet entries of payment accounts.

what the above code suggests:

1. when we again construct hashset entries using the same collection, it will build the hashset again using current hashcode value( will be y) and remove will work now.
2. we are re-constructing same hashset everytime for remove method.

Real Issue: we need to implement hashcode contract correctly. that's what i believe have done.
@ripcurlx @chimp1984 if that's not the case, i'll add the above code, pls let me know.

[julianknutsen]

I tracked it down and it brings up one of the dangers of modifying elements while they are inside Hash containers. The hash bucket for an item is computed at insert time by calling item.hashCode().

But, if the item is later changed such that hashCode() returns a different value, the remove will look for the item in the bucket specified by the hashCode() return value at removal time. This explains why just moving them all to a map and deleting also worked. Also, why after restart it works.

The "fix" is to ensure the hashCode is stable before inserting it into the HashSet. The attached patch works, but I would recommend testing it more or auditing the other users that do something similar. For example, the code is almost duplicated in AltCoinAccountsDataModel.

diff --git a/desktop/src/main/java/bisq/desktop/main/account/content/fiataccounts/FiatAccountsDataModel.java b/desktop/src/main/java/bisq/desktop/main/account/content/fiataccounts/FiatAccountsDataModel.java
index 568b3d8c2..ed281def8 100644
--- a/desktop/src/main/java/bisq/desktop/main/account/content/fiataccounts/FiatAccountsDataModel.java
+++ b/desktop/src/main/java/bisq/desktop/main/account/content/fiataccounts/FiatAccountsDataModel.java
@@ -106,7 +106,6 @@ class FiatAccountsDataModel extends ActivatableDataModel {
     ///////////////////////////////////////////////////////////////////////////////////////////
 
     public void onSaveNewAccount(PaymentAccount paymentAccount) {
-        user.addPaymentAccount(paymentAccount);
         TradeCurrency singleTradeCurrency = paymentAccount.getSingleTradeCurrency();
         List<TradeCurrency> tradeCurrencies = paymentAccount.getTradeCurrencies();
         if (singleTradeCurrency != null) {
@@ -128,6 +127,8 @@ class FiatAccountsDataModel extends ActivatableDataModel {
             });
         }
 
+        user.addPaymentAccount(paymentAccount);
+
         accountAgeWitnessService.publishMyAccountAgeWitness(paymentAccount.getPaymentAccountPayload());
     }

[chimp1984]

@julianknutsen

modifying elements while they are inside Hash containers

Could you see that the objects got modified? I have not seen that but if so that would explain it.
I suspect the selectedTradeCurrency as when it was excluded it worked.

[julianknutsen]

Yes, the screenshots show the tradeCurrencies object was different inside the debugger at insert time and remove time. This is confirmed with the attached patch that "fixes" it by moving the insert until after the tradeCurrencies are set. Previously, it was inserted, then modified.

A quick run through the UI shows that none of the other fields are modifiable after creation time, but I don't have enough background to know other operations that modify that state and if it will have the same issue. For example, can the list of supported currencies change after creation time which would cause the hash to change again?

It seems like using the fields that are guaranteed to be unique and stable is the way to go, but I'll leave that to the devs with more knowledge of that area. Knowing why it behaved strangely should be good enough to move forward now.

[chimp1984]

can the list of supported currencies change after creation time which would cause the hash to change again?

No. That the tradeCurrencies are not set initially is a bug. Once the account is set it is immutable.

[chimp1984]

Fixed with #3612
Thanks @julianknutsen for your hint!

[beingindot]

It seems like using the fields that are guaranteed to be unique and stable is the way to go,

This is what i tried in this PR.

[freimair]

no worries, although the bug does not appear anymore due to @chimp1984 s PR, we are currently discussing if your approach breaks anything (we like your idea). But please use lombok for the Equals and Hash stuff.

[julianknutsen]

I think the issue was more with understanding why it failed initially and not directly tied to the fix. The initial PR suggested that the state changed, but until there was a good understanding of which specific state changed, was it changed intentionally, and what is the scope of a PaymentAccount mutating, it is hard to come to a consensus on the right set of fields to guarantee uniqueness.

@beingindot I really appreciate the work to identify and work on this bug. You've started a good conversation on how to approach this type of issue in the future.

[beingindot]

Thanks @freimair @julianknutsen. Have pushed the review changes accordingly.

[chimp1984]

NACK
I don't see the need for excluding the other fields. It add some unneeded risk and testing effort. The objects should be immutable so any fields has to be fix. If we go further on that area I would recommend to think how we can refactor to make all those accounts immutable. But I think there are many other higher prio stuff to do....

[beingindot]

Is it ok to add @EqualsAndHashCode.Exclude to only selectedTradeCurrency?
If not required, we can close this PR.

[chimp1984]

Is it ok to add @EqualsAndHashCode.Exclude to only selectedTradeCurrency?
If not required, we can close this PR.

I think we should not change the @EqualsAndHashCode as that was not the source of the problem and the reason for the problem is solved. Manipulating the @EqualsAndHashCode should be only done in cases where we really need it IMO. So I suggest to close that PR.