From b55313054a5952e054a695a18d9f66b76c38619a Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Mon, 14 Dec 2020 21:29:07 -0500 Subject: [PATCH 1/7] Cleanup Remove debug and trace logs Add curly brackets --- .../java/bisq/core/offer/OpenOfferManager.java | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/core/src/main/java/bisq/core/offer/OpenOfferManager.java b/core/src/main/java/bisq/core/offer/OpenOfferManager.java index 496a0ac2f2..a7ba8b7e20 100644 --- a/core/src/main/java/bisq/core/offer/OpenOfferManager.java +++ b/core/src/main/java/bisq/core/offer/OpenOfferManager.java @@ -900,12 +900,10 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe offerBookService.addOffer(openOffer.getOffer(), () -> { if (!stopped) { - log.debug("Successfully added offer to P2P network."); // Refresh means we send only the data needed to refresh the TTL (hash, signature and sequence no.) - if (periodicRefreshOffersTimer == null) + if (periodicRefreshOffersTimer == null) { startPeriodicRefreshOffersTimer(); - } else { - log.debug("We have stopped already. We ignore that offerBookService.republishOffers.onSuccess call."); + } } }, errorMessage -> { @@ -914,26 +912,21 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe stopRetryRepublishOffersTimer(); retryRepublishOffersTimer = UserThread.runAfter(OpenOfferManager.this::republishOffers, RETRY_REPUBLISH_DELAY_SEC); - } else { - log.debug("We have stopped already. We ignore that offerBookService.republishOffers.onFault call."); } }); } private void startPeriodicRepublishOffersTimer() { stopped = false; - if (periodicRepublishOffersTimer == null) + if (periodicRepublishOffersTimer == null) { periodicRepublishOffersTimer = UserThread.runPeriodically(() -> { if (!stopped) { republishOffers(); - } else { - log.debug("We have stopped already. We ignore that periodicRepublishOffersTimer.run call."); } }, REPUBLISH_INTERVAL_MS, TimeUnit.MILLISECONDS); - else - log.trace("periodicRepublishOffersTimer already stated"); + } } private void startPeriodicRefreshOffersTimer() { From 7bfbd0fd797e03ef5261fe82e7c5572018518608 Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Mon, 14 Dec 2020 21:32:35 -0500 Subject: [PATCH 2/7] Change handling of delay at republishing The delay was too long. Some users have 100 - 200 offers and with the 700 ms delay it takes 70-140 sec. This causes more stress for the network and UI due permanent adding of offers. We decreased delay to 30 ms per offer. So with 200 offers it would be about 6 sec. Maybe we could reduce it even further but as it is hard to test in the live network with so many offers it is better to not be too radical with the change. --- .../bisq/core/offer/OpenOfferManager.java | 55 +++++++++++-------- 1 file changed, 33 insertions(+), 22 deletions(-) diff --git a/core/src/main/java/bisq/core/offer/OpenOfferManager.java b/core/src/main/java/bisq/core/offer/OpenOfferManager.java index a7ba8b7e20..ccc658e52e 100644 --- a/core/src/main/java/bisq/core/offer/OpenOfferManager.java +++ b/core/src/main/java/bisq/core/offer/OpenOfferManager.java @@ -871,32 +871,36 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe /////////////////////////////////////////////////////////////////////////////////////////// private void republishOffers() { - int size = openOffers.size(); - final ArrayList openOffersList = new ArrayList<>(openOffers.getList()); - if (!stopped) { - stopPeriodicRefreshOffersTimer(); - for (int i = 0; i < size; i++) { - // we delay to avoid reaching throttle limits - - long delay = 700; - final long minDelay = (i + 1) * delay; - final long maxDelay = (i + 2) * delay; - final OpenOffer openOffer = openOffersList.get(i); - UserThread.runAfterRandomDelay(() -> { - if (openOffers.contains(openOffer)) { - String id = openOffer.getId(); - if (id != null && !openOffer.isDeactivated()) - republishOffer(openOffer); - } - - }, minDelay, maxDelay, TimeUnit.MILLISECONDS); - } - } else { - log.debug("We have stopped already. We ignore that republishOffers call."); + if (stopped) { + return; } + + List openOffersList = new ArrayList<>(openOffers.getList()); + republishOffers(openOffersList); + + stopPeriodicRefreshOffersTimer(); + } + + private void republishOffers(List list) { + if (list.isEmpty()) { + return; + } + + OpenOffer openOffer = list.remove(0); + if (!openOffers.contains(openOffer) || openOffer.isDeactivated()) { + republishOffers(list); + } + + republishOffer(openOffer, + () -> UserThread.runAfter(() -> republishOffers(list), + 30, TimeUnit.MILLISECONDS)); } private void republishOffer(OpenOffer openOffer) { + republishOffer(openOffer, null); + } + + private void republishOffer(OpenOffer openOffer, @Nullable Runnable completeHandler) { offerBookService.addOffer(openOffer.getOffer(), () -> { if (!stopped) { @@ -904,6 +908,9 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe if (periodicRefreshOffersTimer == null) { startPeriodicRefreshOffersTimer(); } + if (completeHandler != null) { + completeHandler.run(); + } } }, errorMessage -> { @@ -912,6 +919,10 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe stopRetryRepublishOffersTimer(); retryRepublishOffersTimer = UserThread.runAfter(OpenOfferManager.this::republishOffers, RETRY_REPUBLISH_DELAY_SEC); + + if (completeHandler != null) { + completeHandler.run(); + } } }); } From 8e71575afd0d823357753a11e7250ce490e1e2f5 Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Wed, 16 Dec 2020 08:24:48 -0500 Subject: [PATCH 3/7] Add else case for check for offer is in source list and deActivate state Invert the if branch so the normal case is first Add comment --- .../java/bisq/core/offer/OpenOfferManager.java | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/bisq/core/offer/OpenOfferManager.java b/core/src/main/java/bisq/core/offer/OpenOfferManager.java index ccc658e52e..551b5b99de 100644 --- a/core/src/main/java/bisq/core/offer/OpenOfferManager.java +++ b/core/src/main/java/bisq/core/offer/OpenOfferManager.java @@ -876,24 +876,26 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe } List openOffersList = new ArrayList<>(openOffers.getList()); - republishOffers(openOffersList); + processListForRepublishOffers(openOffersList); stopPeriodicRefreshOffersTimer(); } - private void republishOffers(List list) { + private void processListForRepublishOffers(List list) { if (list.isEmpty()) { return; } OpenOffer openOffer = list.remove(0); - if (!openOffers.contains(openOffer) || openOffer.isDeactivated()) { - republishOffers(list); + if (openOffers.contains(openOffer) && !openOffer.isDeactivated()) { + republishOffer(openOffer, + () -> UserThread.runAfter(() -> processListForRepublishOffers(list), + 30, TimeUnit.MILLISECONDS)); + } else { + // If the offer was removed in the meantime or if its deactivated we skip and call + // processListForRepublishOffers again with the list where we removed the offer already. + processListForRepublishOffers(list); } - - republishOffer(openOffer, - () -> UserThread.runAfter(() -> republishOffers(list), - 30, TimeUnit.MILLISECONDS)); } private void republishOffer(OpenOffer openOffer) { From 24f53e0c0126efa994bf0e5c72e6534821e2892e Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Wed, 16 Dec 2020 09:01:12 -0500 Subject: [PATCH 4/7] Add flush method. Call flush at openOfferManager shutdown. Remove unused method. Force broadcaster to send out immediately, otherwise we could have a 2 sec delay until the bundled messages sent out. --- .../main/java/bisq/core/offer/OpenOfferManager.java | 13 +++++++++---- .../java/bisq/network/p2p/peers/Broadcaster.java | 4 ++++ 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/bisq/core/offer/OpenOfferManager.java b/core/src/main/java/bisq/core/offer/OpenOfferManager.java index 551b5b99de..d57198d30e 100644 --- a/core/src/main/java/bisq/core/offer/OpenOfferManager.java +++ b/core/src/main/java/bisq/core/offer/OpenOfferManager.java @@ -49,6 +49,7 @@ import bisq.network.p2p.DecryptedMessageWithPubKey; import bisq.network.p2p.NodeAddress; import bisq.network.p2p.P2PService; import bisq.network.p2p.SendDirectMessageListener; +import bisq.network.p2p.peers.Broadcaster; import bisq.network.p2p.peers.PeerManager; import bisq.common.Timer; @@ -117,6 +118,7 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe private final RefundAgentManager refundAgentManager; private final DaoFacade daoFacade; private final FilterManager filterManager; + private final Broadcaster broadcaster; private final PersistenceManager> persistenceManager; private final Map offersToBeEdited = new HashMap<>(); private final TradableList openOffers = new TradableList<>(); @@ -148,6 +150,7 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe RefundAgentManager refundAgentManager, DaoFacade daoFacade, FilterManager filterManager, + Broadcaster broadcaster, PersistenceManager> persistenceManager) { this.createOfferService = createOfferService; this.keyRing = keyRing; @@ -166,6 +169,7 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe this.refundAgentManager = refundAgentManager; this.daoFacade = daoFacade; this.filterManager = filterManager; + this.broadcaster = broadcaster; this.persistenceManager = persistenceManager; this.persistenceManager.initialize(openOffers, "OpenOffers", PersistenceManager.Source.PRIVATE); @@ -214,10 +218,6 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe }); } - private void shutDown() { - shutDown(null); - } - public void shutDown(@Nullable Runnable completeHandler) { stopped = true; p2PService.getPeerManager().removeListener(this); @@ -235,6 +235,11 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe UserThread.execute(() -> openOffers.forEach( openOffer -> offerBookService.removeOfferAtShutDown(openOffer.getOffer().getOfferPayload()) )); + + // Force broadcaster to send out immediately, otherwise we could have a 2 sec delay until the + // bundled messages sent out. + broadcaster.flush(); + if (completeHandler != null) { // For typical number of offers we are tolerant with delay to give enough time to broadcast. // If number of offers is very high we limit to 3 sec. to not delay other shutdown routines. diff --git a/p2p/src/main/java/bisq/network/p2p/peers/Broadcaster.java b/p2p/src/main/java/bisq/network/p2p/peers/Broadcaster.java index cdfdde5c46..e48b867571 100644 --- a/p2p/src/main/java/bisq/network/p2p/peers/Broadcaster.java +++ b/p2p/src/main/java/bisq/network/p2p/peers/Broadcaster.java @@ -73,6 +73,10 @@ public class Broadcaster implements BroadcastHandler.ResultHandler { } } + public void flush() { + maybeBroadcastBundle(); + } + private void doShutDown() { broadcastHandlers.forEach(BroadcastHandler::cancel); if (timer != null) { From 1049b7da7714bd1768115fdb9c7057d6b2982d43 Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Wed, 16 Dec 2020 09:04:36 -0500 Subject: [PATCH 5/7] Remove delay for publishing This version is intended to be deployed to a power user to actually try it out live as testing it with real conditions is very difficult. --- core/src/main/java/bisq/core/offer/OpenOfferManager.java | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/bisq/core/offer/OpenOfferManager.java b/core/src/main/java/bisq/core/offer/OpenOfferManager.java index d57198d30e..6b7b3cca3c 100644 --- a/core/src/main/java/bisq/core/offer/OpenOfferManager.java +++ b/core/src/main/java/bisq/core/offer/OpenOfferManager.java @@ -893,9 +893,14 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe OpenOffer openOffer = list.remove(0); if (openOffers.contains(openOffer) && !openOffer.isDeactivated()) { - republishOffer(openOffer, + // TODO It is not clear yet if it is better for the node and the network to send out all add offer + // messages in one go or to spread it over a delay. With power users who have 100-200 offers that can have + // some significant impact to user experience and the network + republishOffer(openOffer, () -> processListForRepublishOffers(list)); + + /* republishOffer(openOffer, () -> UserThread.runAfter(() -> processListForRepublishOffers(list), - 30, TimeUnit.MILLISECONDS)); + 30, TimeUnit.MILLISECONDS));*/ } else { // If the offer was removed in the meantime or if its deactivated we skip and call // processListForRepublishOffers again with the list where we removed the offer already. From 83e620d462b30700079f9d23d4985983070eade6 Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Wed, 16 Dec 2020 10:08:55 -0500 Subject: [PATCH 6/7] Fix missing param in test --- .../src/test/java/bisq/core/offer/OpenOfferManagerTest.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/src/test/java/bisq/core/offer/OpenOfferManagerTest.java b/core/src/test/java/bisq/core/offer/OpenOfferManagerTest.java index 0fcf60daa4..5cc984687a 100644 --- a/core/src/test/java/bisq/core/offer/OpenOfferManagerTest.java +++ b/core/src/test/java/bisq/core/offer/OpenOfferManagerTest.java @@ -49,7 +49,7 @@ public class OpenOfferManagerTest { final OpenOfferManager manager = new OpenOfferManager(null, null, null, p2PService, null, null, null, offerBookService, null, null, null, - null, null, null, null, null, null, + null, null, null, null, null, null, null, persistenceManager); AtomicBoolean startEditOfferSuccessful = new AtomicBoolean(false); @@ -81,7 +81,7 @@ public class OpenOfferManagerTest { final OpenOfferManager manager = new OpenOfferManager(null, null, null, p2PService, null, null, null, offerBookService, null, null, null, - null, null, null, null, null, null, + null, null, null, null, null, null, null, persistenceManager); AtomicBoolean startEditOfferSuccessful = new AtomicBoolean(false); @@ -106,7 +106,7 @@ public class OpenOfferManagerTest { final OpenOfferManager manager = new OpenOfferManager(null, null, null, p2PService, null, null, null, offerBookService, null, null, null, - null, null, null, null, null, null, + null, null, null, null, null, null, null, persistenceManager); AtomicBoolean startEditOfferSuccessful = new AtomicBoolean(false); From c5d0fa789bad527e7eb645cc73df7f9662ce5551 Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Tue, 22 Dec 2020 21:08:42 -0500 Subject: [PATCH 7/7] Move stopPeriodicRefreshOffersTimer before processListForRepublishOffers The periodicRefreshOffersTimer gets started at offer publishing. Before the stopPeriodicRefreshOffersTimer got overwritten by the start in offer publishing so it did not had any effect beside that we restarted it. Now we process offer publishing without delay and a stop after the call would stop the refresh timer. --- core/src/main/java/bisq/core/offer/OpenOfferManager.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/bisq/core/offer/OpenOfferManager.java b/core/src/main/java/bisq/core/offer/OpenOfferManager.java index 6b7b3cca3c..d8d38fb107 100644 --- a/core/src/main/java/bisq/core/offer/OpenOfferManager.java +++ b/core/src/main/java/bisq/core/offer/OpenOfferManager.java @@ -880,10 +880,10 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe return; } + stopPeriodicRefreshOffersTimer(); + List openOffersList = new ArrayList<>(openOffers.getList()); processListForRepublishOffers(openOffersList); - - stopPeriodicRefreshOffersTimer(); } private void processListForRepublishOffers(List list) {