[bisq-network/bisq] periodical UpdateDataReq for Seednodes (#3202)

[chimp1984]

chimp1984 requested changes on this pull request.

NACK, see comments

> @@ -208,7 +208,7 @@ else if (networkEnvelope instanceof RemoveDataMessage)
                     final Set<ProtectedStorageEntry> dataSet = getDataResponse.getDataSet();
                     Set<PersistableNetworkPayload> persistableNetworkPayloadSet = getDataResponse.getPersistableNetworkPayloadSet();
 
-                    if (log.isDebugEnabled()) logContents(networkEnvelope, dataSet, persistableNetworkPayloadSet);
+                    if (log.isInfoEnabled()) logContents(networkEnvelope, dataSet, persistableNetworkPayloadSet);

logContents prints very important information which we also want to log in debug mode.

> @@ -190,6 +193,11 @@ public void onFailure(@NotNull Throwable throwable) {
 
     @Override
     public void onMessage(NetworkEnvelope networkEnvelope, Connection connection) {
+        if (networkEnvelope instanceof RemoveMailboxDataMessage)
+            recentlyRemovedMessages.add(new P2PDataStorage.ByteArray(P2PDataStorage.get32ByteHash(((RemoveMailboxDataMessage) networkEnvelope).getProtectedMailboxStorageEntry().getProtectedStoragePayload())));
+        else if (networkEnvelope instanceof RemoveDataMessage)
+            recentlyRemovedMessages.add(new P2PDataStorage.ByteArray(P2PDataStorage.get32ByteHash(((RemoveDataMessage) networkEnvelope).getProtectedStorageEntry().getProtectedStoragePayload())));
+

What is the intended usage of recentlyRemovedMessages? It is only filled but not queried beside the cleanup loop. Handling for remove msg was added recently to P2PDataStorage. It seems it still covers only MailboxStoragePayload but that would probably require further implementaions of the AddOncePayload interface... Would need moe time to have a clear opinion about that....

> @@ -190,6 +193,11 @@ public void onFailure(@NotNull Throwable throwable) {
 
     @Override
     public void onMessage(NetworkEnvelope networkEnvelope, Connection connection) {
+        if (networkEnvelope instanceof RemoveMailboxDataMessage)
+            recentlyRemovedMessages.add(new P2PDataStorage.ByteArray(P2PDataStorage.get32ByteHash(((RemoveMailboxDataMessage) networkEnvelope).getProtectedMailboxStorageEntry().getProtectedStoragePayload())));
+        else if (networkEnvelope instanceof RemoveDataMessage)
+            recentlyRemovedMessages.add(new P2PDataStorage.ByteArray(P2PDataStorage.get32ByteHash(((RemoveDataMessage) networkEnvelope).getProtectedStorageEntry().getProtectedStoragePayload())));
+

Also I would prefer a more verbose style as otherwise its very hard to follow:

``` 
if (networkEnvelope instanceof RemoveMailboxDataMessage) {
            RemoveMailboxDataMessage removeMailboxDataMessage = (RemoveMailboxDataMessage) networkEnvelope;
            ProtectedStoragePayload protectedStoragePayload = removeMailboxDataMessage.getProtectedMailboxStorageEntry().getProtectedStoragePayload();
            P2PDataStorage.ByteArray byteArray = new P2PDataStorage.ByteArray(P2PDataStorage.get32ByteHash(protectedStoragePayload));
            recentlyRemovedMessages.add(byteArray);
        } else if (networkEnvelope instanceof RemoveDataMessage) {
            RemoveDataMessage removeDataMessage = (RemoveDataMessage) networkEnvelope;
            ProtectedStoragePayload protectedStoragePayload = removeDataMessage.getProtectedStorageEntry().getProtectedStoragePayload();
            P2PDataStorage.ByteArray byteArray = new P2PDataStorage.ByteArray(P2PDataStorage.get32ByteHash(protectedStoragePayload));
            recentlyRemovedMessages.add(byteArray);
        }
```

> @@ -336,12 +337,11 @@ public void onPreliminaryDataReceived() {
 
     @Override
     public void onUpdatedDataReceived() {
-        if (!isBootstrapped) {
-            isBootstrapped = true;
-            maybeProcessAllMailboxEntries();
-            p2pServiceListeners.stream().forEach(P2PServiceListener::onUpdatedDataReceived);
+        isBootstrapped = true;
+        maybeProcessAllMailboxEntries();
+        p2pServiceListeners.stream().forEach(P2PServiceListener::onUpdatedDataReceived);
+        if(!isBootstrapped)
             p2PDataStorage.onBootstrapComplete();

The new version would behave different, even it might be not drastic and visible changes, I think that this is dangerous.

To make it more clear I post the old and the new method:
New version:
```
    public void onUpdatedDataReceived() {
        if (!isBootstrapped() || Capabilities.app.containsAll(Capability.SEED_NODE)) {
            maybeProcessAllMailboxEntries();
            p2pServiceListeners.stream().forEach(P2PServiceListener::onUpdatedDataReceived);
        }

        if (!isBootstrapped()) {
            isBootstrapped = true;
            p2PDataStorage.onBootstrapComplete();
        }
    }
```
Old version:
```
    public void onUpdatedDataReceived1() {
        if (!isBootstrapped) {
            isBootstrapped = true;
            maybeProcessAllMailboxEntries();
            p2pServiceListeners.stream().forEach(P2PServiceListener::onUpdatedDataReceived);
            p2PDataStorage.onBootstrapComplete();
        }
    }
```

  isBootstrapped = true; is set after maybeProcessAllMailboxEntries(); is called, but there we check for isBootstrapped so that will behave differently. the flag need to be set initially before any other calls.


`p2pServiceListeners.stream().forEach(P2PServiceListener::onUpdatedDataReceived);`

I am not sure if a repated onUpdatedDataReceived notifiaction might cause issues. It is expected to be only called once. I know its for seed node only and likely will not have effects, but I think it would be better to add a new handler (can be default in the interface so it will not require clients to implement new handler method).

And a minor issue:
`isBootstrapped()` should be replaced by `isBootstrapped`.


-- 
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/3202#pullrequestreview-285231623
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20190908/cf0fb4e9/attachment-0001.html>