Bundle messages at broadcast #4436
Conversation
|
Doing some statistics comparisions now: Received messages will lower as well once the majority of the network is using this version. So we can expect a network traffic reduction from number of msg or factor 5-6 (see msg/sec). Here a screenshot from the test run of both after 10 minutes. Will post another one after a few hours (left side is using this PR). |
chimp1984
changed the title
|
I added WIP prefix to signal that this PR should not be merged directly. We should first try it out at a seed node and analyse the results then. |
|
A weird observation is that we receive more bytes in the new version but less messages. We have to find out why this is the case. I don't have an explaination for that yet. I reduced ping time as well in that PR. Maybe that causes less preferred connectivity to receive less data. So far I see in the logs its less RefreshOfferMessage, BundleOfEnvelopes, AddDataMessage, Ping and Pong. So either one node was connected to less resiliant set of network nodes or some of the changes caused this. I will try later to start again, might be just a random issue and will test with 2 apps with the old version to see if there is also that much difference. New version: versus old version: |
|
Here are statistics after a few hours. Left side is the PR version, right side are 2 apps running old verison. The difference in number of messages is even larger. About factor 10! Data size of sent data is not much different but also a bit lower (about 5%). Data received is more or less same.
|
It is important that we flush our queued requests at shutdown and wait until broadcast is completed as a maker need to remove his offers at shutdown. - Add handling for the case that there are very few connections (as in dev setup). - Make BundleOfEnvelopes extend BroadcastMessage - Add complete handler for broadCaster to shutdown in P2PService and wait with shutdown of other services until broadcaster is completed. - Remove case for repeated shutdown call on P2PService as it cannot happen.
# Conflicts: # p2p/src/main/java/bisq/network/p2p/P2PService.java
…ndle-msg-at-broadcast # Conflicts: # p2p/src/main/java/bisq/network/p2p/P2PService.java
|
@Emzy did run the PR for 6 hours to get some log data and observe if all is ok. Below the results of logs from PR version versus old version (using the PR with better logging). I would suggest that @Emzy starts to use that PR on one seed for an extended test before we merge it. current verison: new version: current verison: new version: |
chimp1984
changed the title
|
Seed node sn3emzy56u3mxzsr4geysc52feoq5qt7ja56km6gygwnszkshunn2sid.onion:8000 (@Emzy ) is running the PR now. I will test with a local node connected only to that seed to observe behaviour. If anyone wants to test or run from the PR would be welcome. P2P network changes always carries some risk and we should test this very well. |
# Conflicts: # p2p/src/main/java/bisq/network/p2p/P2PService.java
…ndle-msg-at-broadcast
I don't know why the tests failed as I just added an overloaded method and it should not have any impact. There is also one exception which makes it even more obscure. I guess its some test framework issue. See comment at the exceptional handling // If we remove the last argument (isNull()) tests fail. No idea why as the broadcast method has an / overloaded method with nullable listener. Seems a testframework issue as it should not matter if the // method with listener is called with null argument or the other method with no listener. We removed the // null value from all other calls but here we can't as it breaks the test.
We use BundleOfEnvelopes at the connection level to collect messages which are sent out with a short time interval. unfortunatelty that approach did not had much effect as it is per connection and per connection we rarely get jammed too much. This PR is applying the idea to the Broadcast class where we aggregate all messages for broadcast and send them as bundle every second to our connections.
If only 1 message arrived in that interval we do not wrap it into a BundleOfEnvelopes but send it directly.
We also improved the handling of a listener used for notifying about the stored in mailbox event. Previously we added a 3 sec. delay after we received the first successful broadcast. Now we trigger the event once we had at least 3 successful broadcasts.
As we have now mixed messages we had to change the handling of own messages. We treat the bundle with priority if at least one message was originated by ourself.
The notification of listeners handled is by message. To achieve that we use the BroadcastRequest value object.
There was a bug with the timeout delay where a value in seconds have been not converted to ms.
We also refactored the convoluted error handling.
For deploying that PR I recommend to start with 1 seed node and observe the statistics if it have led to lower number of sent messages compared to the current version.
The #4435 PR has added better logging for network statistics and is merged into that PR.