[bisq-network/bisq] [WIP] Remove address prefix for mailbox messages (#4609)

[sqrrm]

@sqrrm commented on this pull request.



> @@ -40,5 +40,6 @@
     SIGNED_ACCOUNT_AGE_WITNESS,         // Supports the signed account age witness feature
     MEDIATION,                          // Supports mediation feature
     REFUND_AGENT,                       // Supports refund agents
-    TRADE_STATISTICS_HASH_UPDATE        // We changed the hash method in 1.2.0 and that requires update to 1.2.2 for handling it correctly, otherwise the seed nodes have to process too much data.
+    TRADE_STATISTICS_HASH_UPDATE,       // We changed the hash method in 1.2.0 and that requires update to 1.2.2 for handling it correctly, otherwise the seed nodes have to process too much data.
+    NO_ADDRESS_PRE_FIX                   // At 1.4.0 we removed the prefix filter for mailbox messages. If a peer has that capability we do not sent the prefix.

```suggestion
    NO_ADDRESS_PRE_FIX                  // At 1.4.0 we removed the prefix filter for mailbox messages. If a peer has that capability we do not sent the prefix.
```

> +        Collection<ProtectedMailboxStorageEntry> entries = protectedStorageEntries.stream()
+                .filter(e -> e instanceof ProtectedMailboxStorageEntry)
+                .map(e -> (ProtectedMailboxStorageEntry) e)
+                .filter(e -> networkNode.getNodeAddress() != null)
+                .filter(e -> !seedNodeRepository.isSeedNode(networkNode.getNodeAddress())) // Seed nodes don't expect mailbox messages
+                .collect(Collectors.toSet());
+        if (entries.size() > 1) {
+            threadedBatchProcessMailboxEntries(entries);
+        } else if (entries.size() == 1) {
+            processSingleMailboxEntry(entries);
+        }
+    }
+
+    private void processSingleMailboxEntry(Collection<ProtectedMailboxStorageEntry> protectedMailboxStorageEntries) {
+        var decryptedEntries = new ArrayList<>(getDecryptedEntries(protectedMailboxStorageEntries));
+        checkArgument(decryptedEntries.size() == 1);

There doesn't seem to be a guarantee that this should be `1` here. If the message in`protectedMailboxStorageEntries` is not for us, this would fail.

Not sure what the intention is here. Perhaps do
```
checkArgument(protectedMailboxStorageEntries.size() == 1);
decryptedEntries.foreach(this::storeMailboxDataAndNotifyListeners);
```

>      }
 
+    @Nullable
+    private Tuple2<ProtectedMailboxStorageEntry, DecryptedMessageWithPubKey> decryptProtectedMailboxStorageEntry(
+            ProtectedMailboxStorageEntry protectedMailboxStorageEntry) {
+        try {
+            DecryptedMessageWithPubKey decryptedMessageWithPubKey = encryptionService.decryptAndVerify(protectedMailboxStorageEntry
+                    .getMailboxStoragePayload()
+                    .getPrefixedSealedAndSignedMessage()

Is your plan to keep the prefixed messages but just use a random prefix so that messages are not attributable? That would be a little bit of overhead but make for an easier transition.

Can we then get rid of the prefix later?

> @@ -513,61 +583,41 @@ public void onFailure(@NotNull Throwable throwable) {
         }
     }
 
+    private PrefixedSealedAndSignedMessage getPrefixedSealedAndSignedMessage(NodeAddress peersNodeAddress,
+                                                                             PubKeyRing pubKeyRing,
+                                                                             NetworkEnvelope message) throws CryptoException {
+        byte[] addressPrefixHash;
+        if (peerManager.peerHasCapability(peersNodeAddress, Capability.NO_ADDRESS_PRE_FIX)) {

Oh, I see, keep prefix structure but don't set it, makes sense.

> +        return getPeersNodeAddressOptional().isPresent() ?
+                getPeersNodeAddressOptional().get() :
+                networkEnvelope instanceof SendersNodeAddressMessage ?
+                        ((SendersNodeAddressMessage) networkEnvelope).getSenderNodeAddress() :
+                        null;

There is `orElse` for Optional
```suggestion
        return getPeersNodeAddressOptional().orElse(
                networkEnvelope instanceof SendersNodeAddressMessage ?
                        ((SendersNodeAddressMessage) networkEnvelope).getSenderNodeAddress() :
                        null);
```

> @@ -470,6 +523,17 @@ public void addToReportedPeers(Set<Peer> reportedPeersToAdd, Connection connecti
         }
     }
 
+    private void applyCapabilities(Connection connection, Capabilities capabilities) {
+        if (capabilities != null && !capabilities.isEmpty()) {

Better return early

-- 
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/4609#pullrequestreview-504263744
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20201007/34db7841/attachment-0001.html>