[bisq-network/bisq] Bundle of envelopes (#2939)

[sqrrm]

sqrrm commented on this pull request.

Seems like it's a bit risky, but not a bad idea. Need a bit more time to think on it.

> @@ -66,13 +66,19 @@ message NetworkEnvelope {
         NewBlindVoteStateHashMessage new_blind_vote_state_hash_message = 40;
         GetBlindVoteStateHashesRequest get_blind_vote_state_hashes_request = 41;
         GetBlindVoteStateHashesResponse get_blind_vote_state_hashes_response = 42;
+

```suggestion
```

> +                    long elapsed = now - lastSendTimeStamp;
+                    if (elapsed < sendMsgThrottleTrigger) {
+                        log.debug("We got 2 sendMessage requests in less than {} ms. We set the thread to sleep " +
+                                        "for {} ms to avoid flooding our peer. lastSendTimeStamp={}, now={}, elapsed={}, networkEnvelope={}",
+                                sendMsgThrottleTrigger, sendMsgThrottleSleep, lastSendTimeStamp, now, elapsed,
+                                networkEnvelope.getClass().getSimpleName());
+
+                        // check if BundleOfEnvelopes is supported
+                        if(getCapabilities().containsAll(new Capabilities(Capability.ENVELOPE_OF_ENVELOPES))) {
+                            synchronized (lock) {
+                                // check if current envelope fits size
+                                // - no? create new envelope
+                                if(queueOfBundles.isEmpty() || queueOfBundles.element().toProtoNetworkEnvelope().getSerializedSize() + networkEnvelope.toProtoNetworkEnvelope().getSerializedSize() > MAX_PERMITTED_MESSAGE_SIZE * 0.9) {
+                                    // - no? create a bucket
+                                    queueOfBundles.add(new BundleOfEnvelopes());
+                                    System.err.println("added fresh container");

Why print?  Debug cruft?

> +                                        "for {} ms to avoid flooding our peer. lastSendTimeStamp={}, now={}, elapsed={}, networkEnvelope={}",
+                                sendMsgThrottleTrigger, sendMsgThrottleSleep, lastSendTimeStamp, now, elapsed,
+                                networkEnvelope.getClass().getSimpleName());
+
+                        // check if BundleOfEnvelopes is supported
+                        if(getCapabilities().containsAll(new Capabilities(Capability.ENVELOPE_OF_ENVELOPES))) {
+                            synchronized (lock) {
+                                // check if current envelope fits size
+                                // - no? create new envelope
+                                if(queueOfBundles.isEmpty() || queueOfBundles.element().toProtoNetworkEnvelope().getSerializedSize() + networkEnvelope.toProtoNetworkEnvelope().getSerializedSize() > MAX_PERMITTED_MESSAGE_SIZE * 0.9) {
+                                    // - no? create a bucket
+                                    queueOfBundles.add(new BundleOfEnvelopes());
+                                    System.err.println("added fresh container");
+
+                                    // - and schedule it for sending
+                                    lastSendTimeStamp += sendMsgThrottleSleep;

This will distort the lastSendTimeStamp by setting it to the future. Won't this add cumulatively more time to lastSendTimeStamp if there are multiple messages that need to be sent in a short times pan?

> @@ -393,7 +432,13 @@ private boolean violatesThrottleLimit(NetworkEnvelope networkEnvelope) {
     @Override
     public void onMessage(NetworkEnvelope networkEnvelope, Connection connection) {
         checkArgument(connection.equals(this));
-        UserThread.execute(() -> messageListeners.forEach(e -> e.onMessage(networkEnvelope, connection)));
+
+        if(networkEnvelope instanceof BundleOfEnvelopes)

Style is off (missing space). Best to use some tool that handles that automatically.

-- 
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/2939#pullrequestreview-258867420
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20190708/7e060473/attachment.html>