[bisq-network/bisq] [WIP] Use keyset delta algorithm to speed up GetData requests (#3896)

[Julian Knutsen]

My 2c since I was the last one in here.

How much testing have you done with this?

My understanding of the startup path from @chimp1984 is that the capabilities isn't set on these initial requests so switching on it isn't going to work. Here is the relevant discussion on my proposed changes to these requests a few cycles back: https://github.com/bisq-network/bisq/issues/3610#issuecomment-554165450

> 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.

This is a risky enough area and a non-trivial amount of work. It would have been good to see a proposal so the work involved to review, merge, and test it could be analyzed before seeing the code. I'm not 100% sure that speeding up this path is the most important thing to do, but I'll leave that to the other contributors that will be responsible for supporting this into release.

Some other minor notes after a quick review:

1. There seems to be quite a few intermingled changes in this one commit. Some performance, some API changes, the actual deltas... For ease of review it would be great to split out the smaller changes so it is easier to see what is actually changing.
2. This code had enough random booleans in it to begin with and it would be a shame to move it back towards an unmaintainable mess. Consider a separate API for the V1 and V2 APIs that can be independently unit and integration tested.
3. Since this is the first feature that needs backwards compatibility, it is probably worth designing tests that specifically run the V2 APIs against a V1 store.
4. Is there a release test plan for this? It is non-trivial work to handle the backwards compatibility especially since full-stack integration tests don't exist.

-- 
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/pull/3896#issuecomment-573857636
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20200113/50c79172/attachment.html>