[bisq-network/bisq] Compensation Request Proposals not deleted as expected (#3610)

[chimp1984]

Thanks for looking into that!
I was looking a while back into it and as far I remember I came to the same conclusion of the problem. As it is not trivial to solve that I postponed to try to fix that...

Regarding your suggestions:
Re 1:
I agree that the TempProposals should be not persisted (have not thought in depth about it and don't remember if there was a important reason why we persist it). 
I think it would not lead to DAO consensus issues as the TempProposals is not relevant for DAO consensus and will be converted to the Proposal objects which will then be used for consensus. So old nodes should not have an issue if they do not recognize the new TempProposal version.
We have the policy that active DAO partizipants need to update if there are important changes, so we can ignore the problem that old nodes would not see any new TempProposal.

Maybe you can try that out and see if it causes any problems. Important will be to test also the mixed cases with an old and new client (I usually have then 2 IDEA instances open, one with old version, one with new version). There might be the problem that old node creating a `TempProposal` and later convert that to a `Proposals` could cause issues. Not sure if it is an critical issue beside that the node running the new version will not see the old `TempProposal`. 

If we change the TempProposal we likely do not to support the old version (maintaining 2 classes and lists) as it is not part of consensus and resyncing the DAO should not have any effect on that.

We have a version flag for all DAO txs (in `Version` class) as well as the `disableDaoBelowVersion` field in Filter (A filter message can be sent out from a Christoph and all nodes use that as long they have not set the `ignoreDevFlags` option to `true`). I am a bit scared to use the version flags for the DAO txs as that was never much tested (or not tested at all). But using the Filter flag should be low risk. With that no one can use the DAO if not updated, so it is partial enforced update for those who want to use the DAO need. We can also use activation blocks to give more time so that the majority have already updated when we deploy it.

Maybe we should make a summary of all our features how we can deal with backward compatibility. Its quite complex and there are already several features available.

Re 2:
The requesting node would in that case trust the seed node that the remove was verified as it does not have the signature from the object owner. I am not sure if that is a problem as we do trust seeds anyway with adding objects and other things to a certain degree. Thought to extend the required trust for seeds does not feel so good. But maybe we can alter your idea that all nodes are keeping the `RemoveDataMessage`s and if a requesting node has an older `seqNr` it send the `RemoveDataMessage` in the reply to that node so the node itself is doing the verification? The node only need to keep the latest `RemoveDataMessage` and we could use the TTL for purging old data. `RemoveDataMessage` are anyway not very frequent so it should not cause any resource/performance issues.

But also here the main challenge will be backward compatibility.
For the `TempProposal` issue I think it would not cause much issues as old nodes could still get the old removed `TempProposal` if another old node republished it but new nodes would get the `RemoveDataMessage` and would ignore such a republished `TempProposal` as the `seqNr` will be lower. 
The `GetDataRequest` would have additional fields which does not break backward compatibility. We just need to deal with the valid case that such fields can be null if the peer is not updated. We cannot use the capability feature here as the `GetDataRequest` is the first message and the capability not known at that moment. Thought if it turns out that its absolutely needed we could work on a capability exchange protocol which is anyway needed at some point (exchanging capabilities would be then the very first step). Currently the capability feature is a bit unreliable as it depends on the context if the peers have exchanges it.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/issues/3610#issuecomment-554165450
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20191114/1765e122/attachment-0001.html>