Fix issues with missing persistence for trade state #4816
Conversation
Seems the persistence at shutdown is too unsafe and we got bug reports where data was missing. bisq-network#4806 Use millisec instead of sec for delay Rename delayInSec to delay
|
This would not resolve #4806 since [EDIT] Ok, I see you're trying to persist in SIGTERM. Maybe it would be ok. If the delay is only 200ms maybe that is enough time before the process is shut down. It wouldn't help in the case of power cut though. |
|
Oh, yes you are right. I though at trade task complete we call it but I fear that got lost in some merge. It was a bit complicate with several larger PRs (refactoring of trade protocol + refactoring of persistence). |
We relied on the shutdwon routine to be called reliably but it seems that is not the case as some bug reports show. So we call requestPersistence at every write access of the trade object
|
I added the |
chimp1984
changed the title
|
Failing test is resolved by @stejbac (#4789 (comment)). Will merge in once hie PR is merged. |
|
ACK, tested, fixes #4806. |
|
I double checked all trade (and sub models) related fields as well as all trade protocol tasks and protocols. Hope I did not miss a call but as there is quite a lot of redundancy I think its safe now. |
|
I feel it might be cleaner to do the persist in the Apart from that I think this looks ok and could be merged. |
core/src/main/java/bisq/core/trade/protocol/BuyerAsTakerProtocol.java
Outdated
Show resolved
Hide resolved
core/src/main/java/bisq/core/trade/protocol/SellerAsTakerProtocol.java
Outdated
Show resolved
Hide resolved
I was wondering myself if we should add at the tradeTask complete handler a perist call. It would add extra redundancy in case I missed one as well in case we add some tasks or have some changes and the call is forgotten. |
We get called some setter methods from protobuf methods before tradeManager is set.
The deposit confirmed state is set after we applied the mailbox messages, which led to a task failure due wrong phase and the message was not applied. Further it can be that the wallet is still syncing and the deposit confirmed state is set in any time in the future. To fix the first problem we add a bit of delay so that the trade has been updated when we apply the mailbox messages. A better fix would be to change the order of the methods but that is a bit tricky to get right and I dont want to risk that for that release. The second problem would require a large change to trigger the mailbox processing based on wallet state. We prefer to be more tolerant with the expected phase instead so allow the mailbox message to be processed also in the DEPOSIT_PUBLISHED state. This has no risks as the payout tx would be invalid anyway if the buyer has cheated and sent the msg in not confirmed deposit tx state (only possible with code manipulation). A better fix would to add a listener for the wallet and process the mailbox msg once wallet is ready and trade state set, but I leave that for another PR.
This is not really needed as we call it at each state change of the trade but gives more redundancy in case we missed one or once changes are applied and a dev forgets to call it. Multiple repeated calls do have close to zero costs.
We need to set addDecryptedDirectMessageListener without delay as otherwise we could miss direct messages (detected with localhost testing, with tor its likely slower and would not have been triggered).
|
With the persistence call in complete() it's not necessary to persist in each trade task. |
A reason why I did not do want to rely only on complete is as there are async tasks and i want to be sure that if the task never completes (shutdown) that the intermediate state change is persisted. But also not happy that its that verbose now ;-(. But its more similar as it was before, just that some calls have been done inside the trade setter methods. |
|
Agreed that some persist calls should still be done in the tasks, but all of those done synchronously just before complete() are no longer needed. |
ghost
mentioned this pull request
There are some bug reports which suggest that the persistence at shutdown is not working reliable (hard kill, crash,...).
We reduce the persistence interval radically to ensure important data is written immediately (200 ms delay).
Further the
requestPersistencehas been missing in most trade tasks. I guess that happened when merging the large refactoring projects (trade protocol, persistence). If shutdown routine gets executed it did not had any effect as we wrote then the trade state, but if that was missing we might miss important trade state/data changes.Fixes #4806
Fixes #4762