[bisq-network/bisq] (4/4) Update behavior of add/remove/refresh on duplicate sequence numbers (#3558)

[chimp1984]

chimp1984 commented on this pull request.

Regarding the concurrency tests:
Bisq uses single threading model as far as possible (exceptions, P2P connection class, BitcoinJ internal classes, disc persistance classes, some util stuff,...). The P2PDataStorage has no concurrent access as all network data are mapped to the UserThread.

>              return false;
-        }
 
         boolean hasSequenceNrIncreased = hasSequenceNrIncreased(protectedStorageEntry.getSequenceNumber(), hashOfPayload);
 
         // If we have seen a more recent operation for this payload, we ignore the current one
         // TODO: I think we can return false here. All callers use the get() public API which increments the sequence number
         // leaving only the onMessage() handler which doesn't look at the return value. It makes more intuitive sense that adds() that don't

This comment ("leaving only the onMessage() handler which doesn't look at the return value") is not true. In P2PService is a method which further gets used by client code ('p2PService.addProtectedStorageEntry') and some are operating on the return value. There are a few of those cases (I have not looked up now if it was in that particular case but I guess the statement that only the onMessage handler is calling the method is not correct. 

-- 
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/3558#pullrequestreview-312182914
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20191105/68bc6bd3/attachment-0001.html>