<p></p>
<p><b>@sqrrm</b> commented on this pull request.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4609#discussion_r501307330">common/src/main/java/bisq/common/app/Capability.java</a>:</p>
<pre style='color:#555'>> @@ -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.
</pre>
⬇️ Suggested change
<pre style="color: #555">- 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.
+ 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.
</pre>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4609#discussion_r501314014">p2p/src/main/java/bisq/network/p2p/P2PService.java</a>:</p>
<pre style='color:#555'>> + 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);
</pre>
<p>There doesn't seem to be a guarantee that this should be <code>1</code> here. If the message in<code>protectedMailboxStorageEntries</code> is not for us, this would fail.</p>
<p>Not sure what the intention is here. Perhaps do</p>
<pre><code>checkArgument(protectedMailboxStorageEntries.size() == 1);
decryptedEntries.foreach(this::storeMailboxDataAndNotifyListeners);
</code></pre>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4609#discussion_r501319546">p2p/src/main/java/bisq/network/p2p/P2PService.java</a>:</p>
<pre style='color:#555'>> }
+ @Nullable
+ private Tuple2<ProtectedMailboxStorageEntry, DecryptedMessageWithPubKey> decryptProtectedMailboxStorageEntry(
+ ProtectedMailboxStorageEntry protectedMailboxStorageEntry) {
+ try {
+ DecryptedMessageWithPubKey decryptedMessageWithPubKey = encryptionService.decryptAndVerify(protectedMailboxStorageEntry
+ .getMailboxStoragePayload()
+ .getPrefixedSealedAndSignedMessage()
</pre>
<p>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.</p>
<p>Can we then get rid of the prefix later?</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4609#discussion_r501322301">p2p/src/main/java/bisq/network/p2p/P2PService.java</a>:</p>
<pre style='color:#555'>> @@ -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)) {
</pre>
<p>Oh, I see, keep prefix structure but don't set it, makes sense.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4609#discussion_r501326600">p2p/src/main/java/bisq/network/p2p/network/Connection.java</a>:</p>
<pre style='color:#555'>> + return getPeersNodeAddressOptional().isPresent() ?
+ getPeersNodeAddressOptional().get() :
+ networkEnvelope instanceof SendersNodeAddressMessage ?
+ ((SendersNodeAddressMessage) networkEnvelope).getSenderNodeAddress() :
+ null;
</pre>
<p>There is <code>orElse</code> for Optional</p>
⬇️ Suggested change
<pre style="color: #555">- return getPeersNodeAddressOptional().isPresent() ?
- getPeersNodeAddressOptional().get() :
- networkEnvelope instanceof SendersNodeAddressMessage ?
- ((SendersNodeAddressMessage) networkEnvelope).getSenderNodeAddress() :
- null;
+ return getPeersNodeAddressOptional().orElse(
+ networkEnvelope instanceof SendersNodeAddressMessage ?
+ ((SendersNodeAddressMessage) networkEnvelope).getSenderNodeAddress() :
+ null);
</pre>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4609#discussion_r501330827">p2p/src/main/java/bisq/network/p2p/peers/PeerManager.java</a>:</p>
<pre style='color:#555'>> @@ -470,6 +523,17 @@ public void addToReportedPeers(Set<Peer> reportedPeersToAdd, Connection connecti
}
}
+ private void applyCapabilities(Connection connection, Capabilities capabilities) {
+ if (capabilities != null && !capabilities.isEmpty()) {
</pre>
<p>Better return early</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/bisq-network/bisq/pull/4609#pullrequestreview-504263744">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AJFFTNV752DTSQPUYJ44LJ3SJTQL5ANCNFSM4SHU4CTQ">unsubscribe</a>.<img src="https://github.com/notifications/beacon/AJFFTNWBTOC5ORLCI3MTTT3SJTQL5A5CNFSM4SHU4CT2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGODYHHIQA.gif" height="1" width="1" alt="" /></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/bisq-network/bisq/pull/4609#pullrequestreview-504263744",
"url": "https://github.com/bisq-network/bisq/pull/4609#pullrequestreview-504263744",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>