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

[Julian Knutsen]

In this issue, I will explain the source of the bug that resulted in removed Compensation Request proposals persisting through the Proposal phase and being broadcast during the Blind Vote phase. At the end, I will throw out a design idea, but we should have a design discussion on what the options are and the best path forward to fix this in a safe way.

## Repro

1. Start a regtest cluster with Alice, Bob, and a SeedNode.
2. Alice publishes a compensation request through the UI
3. Add a block to bitcoind so the tx is broadcast and proposal is now seen on Bob's UI
4. Kill Bob instance of Bisq
5. Alice removes compensation request (it is now removed on Alice UI)
6. Restart Bob instance of Bisq (request is still available on Bob UI)
7. Add blocks until phase switches to Blind Vote
8. Compensation request now shows up on Alice's UI (now on both)

## Root Cause
TempProposalPayload objects implement the ProtectedStoragePayload & PersistablePayload interfaces. This means that they will be stored to disk. But the current system design does not handle removes of persistable Entrys that occur when nodes are offline.

When Bob's Bisq instance is taken down, the TempProposal object is written to disk. When it is restarted it will read the on-disk instances and recreate the TempProposal that is consumed by the ProposalService.

Removes are never rebroadcast after the initial RemoveData message and the RequestData/GetData path does not handle removes, only adds. So, Bob will never learn that a RemoveData message was sent for the TempProposal that he recreated. At this point, it is only a local problem.

However, once blocks are added to move to the Blind Vote phase, maybePublishToAppendOnlyDataStore() is called on Bob which recognizes that the AppendOnlyDataStore does not have the proposal and it is broadcast to the entire network as a PersistableNetworkPayload.

So, it only takes 1 node not seeing the RemoveData that causes all nodes on the network to receive it as append-only data.

## Risks & Mitigation

For now, this bug is handled through publishing the appropriate proposal tx id with the compensation request on the Github issue in the proposals repo. It is up to all voters to vote NO on any duplicates. This seems like an OK solution for the current cycle.

## Designs moving forward

1. Change TempProposals to be non-persistable
This would require the network to stay healthy throughout the proposal phase until the Blind Vote phase where all non-persistent proposals will be added as append-only data.

Work would be required to bootstrap this and remove any saved TempProposals on nodes that currently exist.

2. Add remove tracking to GetData/RequestData
This is a larger design and probably worthy of its own design doc and discussion, but here is an idea off the top of my head:

Add fields to the GetData message handshake which includes hashes that have been removed:

* A node sends the set of known hashes and the last seen sequence number for each hash to seednode. (knownHashes = set<pair<hash, latest_seqnr>>)
* If the seed node recognizes that a known hash is actually removed (exists in seq nr map, but not hash map), it sends back the seq nr for the remove operation. (RemovedEntrys = set(pair<hash, latest_seqnr>)
* Node post-processes the information in this set as a remove()

This requires a new GetData message type field and the interop between old and new nodes would require some thorough testing.



-- 
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20191114/199365ae/attachment.html>