Prevent that a wrong tx is set as deposit tx #4880
Conversation
We check if the txIds of the inputs matches our maker fee tx and taker fee tx and if the depositTxAddress we use for the confidence lookup is use as an output address. This prevents that past txs which have the our depositTxAddress as input or output (deposit or payout txs) could be interpreted as our deposit tx. This happened because if a bug which caused re-use of the Multisig address entries and if both traders use the same key for multiple trades the depositTxAddress would be the same. We fix that bug as well but we also need to avoid that past already used addresses might be taken again (the Multisig flag got reverted to available in the address entry).
…ries. Remove swap for MULTI_SIG entries at resetAddressEntriesForPendingTrade
…ructor. Rename getCoinLockedInMultiSig to getCoinLockedInMultiSigAsCoin Add comments We use an immutable list when operating on AddressEntry so changes on the object would not be reflected in the list. The only mutable field (beside non critical cache fields) is the keyPair. Might be good to refactor that as well at some point.
- resetCoinLockedInMultiSigAddressEntry - setCoinLockedInMultiSigAddressEntry - renamed methods
There was a problem hiding this comment.
ACK - I have tested this applied to release/v1.5.0 codebase.
It does close #4873.
However I do not see any relation to #4879. In my view they are separate issues.
[EDIT] Scratch that, on further thought, it could definitely relate to #4879. When the problem was happening, I saw the same "protocol did not complete in 30 seconds" issued by the maker side of the trade (but I was mostly focusing on the taker side where the bug manifested). 👍 So yes, all good.
Sorry I made a mistake with the issue #. Have not looked closer to #4879 so not sure if its the same issue. Might have been from missing persistence as well... |
|
After a look to #4879 I dont think thats related.
If it was a pre 1.5.0 version it is likely fixed. |
|
Repeat ACK |
|
Replaced by #4883 |
Fixes #4873
Beside what is mentioned in #4873 I found another (minor) issue. We use an immutable list for address entries but the coinLockedInMultiSig value was set in the object via a setter. This change would be lost when accessed as when one open the wallet tool with cmd+j (after take offe the value was 0 there instead of the locked up funds). As the field is only used for lookup for locked funds from failed trades the bug was not very visible.
coinLockedInMultiSig is now a final field set by the constructor.
EDIT: used wrong issue before...