From 85caf88913b91e6c8f0938bdeedfdd0e18b24270 Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Wed, 30 Dec 2020 20:20:57 -0500 Subject: [PATCH 1/3] We need to use the getMapOfAllData method for HistoricalDataStoreServices --- .../storage/persistence/AppendOnlyDataStoreService.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/p2p/src/main/java/bisq/network/p2p/storage/persistence/AppendOnlyDataStoreService.java b/p2p/src/main/java/bisq/network/p2p/storage/persistence/AppendOnlyDataStoreService.java index 927d81db578..25ce17f0e95 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/persistence/AppendOnlyDataStoreService.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/persistence/AppendOnlyDataStoreService.java @@ -78,7 +78,12 @@ public void readFromResourcesSync(String postFix) { public Map getMap() { return services.stream() - .flatMap(service -> service.getMap().entrySet().stream()) + .flatMap(service -> { + Map map = service instanceof HistoricalDataStoreService ? + ((HistoricalDataStoreService) service).getMapOfAllData() : + service.getMap(); + return map.entrySet().stream(); + }) .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); } From 4489e57849996648d9d2ec5c18f98c914becac62 Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Wed, 30 Dec 2020 20:36:45 -0500 Subject: [PATCH 2/3] Use concrete dataStorageServices instead p2PService.getP2PDataStorage().getAppendOnlyDataStoreMap(). p2PService.getP2PDataStorage().getAppendOnlyDataStoreMap() iterates over all services including the historical data store service. It used the getMap method which should not be used at historical data store service as it is not clear if the live data or all data should be accessed. --- .../bisq/core/account/sign/SignedWitnessService.java | 4 +++- core/src/main/java/bisq/core/app/BisqSetup.java | 6 +++++- .../core/dao/governance/proposal/ProposalService.java | 4 +++- .../java/bisq/desktop/main/market/MarketView.java | 11 +++++------ .../java/bisq/network/p2p/storage/P2PDataStorage.java | 7 +++++-- 5 files changed, 21 insertions(+), 11 deletions(-) diff --git a/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java b/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java index 69a7d49feb0..126d38a0ce3 100644 --- a/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java +++ b/core/src/main/java/bisq/core/account/sign/SignedWitnessService.java @@ -73,6 +73,7 @@ public class SignedWitnessService { private final KeyRing keyRing; private final P2PService p2PService; private final ArbitratorManager arbitratorManager; + private final SignedWitnessStorageService signedWitnessStorageService; private final User user; private final FilterManager filterManager; @@ -98,6 +99,7 @@ public SignedWitnessService(KeyRing keyRing, this.keyRing = keyRing; this.p2PService = p2PService; this.arbitratorManager = arbitratorManager; + this.signedWitnessStorageService = signedWitnessStorageService; this.user = user; this.filterManager = filterManager; @@ -117,7 +119,7 @@ public void onAllServicesInitialized() { }); // At startup the P2PDataStorage initializes earlier, otherwise we get the listener called. - p2PService.getP2PDataStorage().getAppendOnlyDataStoreMap().values().forEach(e -> { + signedWitnessStorageService.getMap().values().forEach(e -> { if (e instanceof SignedWitness) addToMap((SignedWitness) e); }); diff --git a/core/src/main/java/bisq/core/app/BisqSetup.java b/core/src/main/java/bisq/core/app/BisqSetup.java index 769cdcf4486..677b4e76784 100644 --- a/core/src/main/java/bisq/core/app/BisqSetup.java +++ b/core/src/main/java/bisq/core/app/BisqSetup.java @@ -18,6 +18,7 @@ package bisq.core.app; import bisq.core.account.sign.SignedWitness; +import bisq.core.account.sign.SignedWitnessStorageService; import bisq.core.account.witness.AccountAgeWitnessService; import bisq.core.alert.Alert; import bisq.core.alert.AlertManager; @@ -124,6 +125,7 @@ default void onRequestWalletPassword() { private final WalletsSetup walletsSetup; private final BtcWalletService btcWalletService; private final P2PService p2PService; + private final SignedWitnessStorageService signedWitnessStorageService; private final TradeManager tradeManager; private final OpenOfferManager openOfferManager; private final Preferences preferences; @@ -205,6 +207,7 @@ public BisqSetup(DomainInitialisation domainInitialisation, WalletsSetup walletsSetup, BtcWalletService btcWalletService, P2PService p2PService, + SignedWitnessStorageService signedWitnessStorageService, TradeManager tradeManager, OpenOfferManager openOfferManager, Preferences preferences, @@ -225,6 +228,7 @@ public BisqSetup(DomainInitialisation domainInitialisation, this.walletsSetup = walletsSetup; this.btcWalletService = btcWalletService; this.p2PService = p2PService; + this.signedWitnessStorageService = signedWitnessStorageService; this.tradeManager = tradeManager; this.openOfferManager = openOfferManager; this.preferences = preferences; @@ -652,7 +656,7 @@ private void maybeShowAccountSigningStateInfo() { private void checkSigningState(AccountAgeWitnessService.SignState state, String key, Consumer displayHandler) { - boolean signingStateFound = p2PService.getP2PDataStorage().getAppendOnlyDataStoreMap().values().stream() + boolean signingStateFound = signedWitnessStorageService.getMap().values().stream() .anyMatch(payload -> isSignedWitnessOfMineWithState(payload, state)); maybeTriggerDisplayHandler(key, displayHandler, signingStateFound); diff --git a/core/src/main/java/bisq/core/dao/governance/proposal/ProposalService.java b/core/src/main/java/bisq/core/dao/governance/proposal/ProposalService.java index e91616d9e1d..a50b7e2f354 100644 --- a/core/src/main/java/bisq/core/dao/governance/proposal/ProposalService.java +++ b/core/src/main/java/bisq/core/dao/governance/proposal/ProposalService.java @@ -69,6 +69,7 @@ public class ProposalService implements HashMapChangedListener, AppendOnlyDataSt DaoStateListener, DaoSetupService { private final P2PService p2PService; private final PeriodService periodService; + private final ProposalStorageService proposalStorageService; private final DaoStateService daoStateService; private final ProposalValidatorProvider validatorProvider; @@ -100,6 +101,7 @@ public ProposalService(P2PService p2PService, @Named(Config.DAO_ACTIVATED) boolean daoActivated) { this.p2PService = p2PService; this.periodService = periodService; + this.proposalStorageService = proposalStorageService; this.daoStateService = daoStateService; this.validatorProvider = validatorProvider; @@ -217,7 +219,7 @@ private void fillListFromProtectedStore() { } private void fillListFromAppendOnlyDataStore() { - p2PService.getP2PDataStorage().getAppendOnlyDataStoreMap().values().forEach(e -> onAppendOnlyDataAdded(e, false)); + proposalStorageService.getMap().values().forEach(e -> onAppendOnlyDataAdded(e, false)); } private void maybePublishToAppendOnlyDataStore() { diff --git a/desktop/src/main/java/bisq/desktop/main/market/MarketView.java b/desktop/src/main/java/bisq/desktop/main/market/MarketView.java index 7220952576b..65a789b38b7 100644 --- a/desktop/src/main/java/bisq/desktop/main/market/MarketView.java +++ b/desktop/src/main/java/bisq/desktop/main/market/MarketView.java @@ -36,11 +36,10 @@ import bisq.core.locale.Res; import bisq.core.offer.OfferPayload; import bisq.core.trade.statistics.TradeStatistics3; +import bisq.core.trade.statistics.TradeStatistics3StorageService; import bisq.core.util.FormattingUtils; import bisq.core.util.coin.CoinFormatter; -import bisq.network.p2p.P2PService; - import bisq.common.util.Utilities; import javax.inject.Inject; @@ -71,7 +70,7 @@ public class MarketView extends ActivatableView { @FXML Tab offerBookTab, tradesTab, spreadTab; private final ViewLoader viewLoader; - private final P2PService p2PService; + private final TradeStatistics3StorageService tradeStatistics3StorageService; private final OfferBook offerBook; private final CoinFormatter formatter; private final Navigation navigation; @@ -83,12 +82,12 @@ public class MarketView extends ActivatableView { @Inject public MarketView(CachingViewLoader viewLoader, - P2PService p2PService, + TradeStatistics3StorageService tradeStatistics3StorageService, OfferBook offerBook, @Named(FormattingUtils.BTC_FORMATTER_KEY) CoinFormatter formatter, Navigation navigation) { this.viewLoader = viewLoader; - this.p2PService = p2PService; + this.tradeStatistics3StorageService = tradeStatistics3StorageService; this.offerBook = offerBook; this.formatter = formatter; this.navigation = navigation; @@ -181,7 +180,7 @@ private String getAllTradesWithReferralId() { // all items of both traders in case the referral ID was only set by one trader. // If both traders had set it the tradeStatistics is only delivered once. // If both traders used a different referral ID then we would get 2 objects. - List list = p2PService.getP2PDataStorage().getAppendOnlyDataStoreMap().values().stream() + List list = tradeStatistics3StorageService.getMapOfAllData().values().stream() .filter(e -> e instanceof TradeStatistics3) .map(e -> (TradeStatistics3) e) .filter(tradeStatistics3 -> tradeStatistics3.getExtraDataMap() != null) diff --git a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java index 243418deff0..1ed4cc30b47 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -534,7 +534,10 @@ public void onBootstrapComplete() { removeExpiredEntriesTimer = UserThread.runPeriodically(this::removeExpiredEntries, CHECK_TTL_INTERVAL_SEC); } - public Map getAppendOnlyDataStoreMap() { + // Domain access should use the concrete appendOnlyDataStoreService if available. The Historical data store require + // care which data should be accessed (live data or all data). + @VisibleForTesting + Map getAppendOnlyDataStoreMap() { return appendOnlyDataStoreService.getMap(); } @@ -642,7 +645,7 @@ private boolean addPersistableNetworkPayload(PersistableNetworkPayload payload, } ByteArray hashAsByteArray = new ByteArray(payload.getHash()); - boolean payloadHashAlreadyInStore = getAppendOnlyDataStoreMap().containsKey(hashAsByteArray); + boolean payloadHashAlreadyInStore = appendOnlyDataStoreService.getMap().containsKey(hashAsByteArray); // Store already knows about this payload. Ignore it unless the caller specifically requests a republish. if (payloadHashAlreadyInStore && !reBroadcast) { From 6a35410c69ffea2aedee727b50b46e345465ba3e Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Wed, 30 Dec 2020 20:47:15 -0500 Subject: [PATCH 3/3] Log error (and throw exception if in devMode) if HistoricalDataStoreService.getMap is called. HistoricalDataStoreService.getMap should not be used by domain clients but rather the custom methods getMapOfAllData, getMapOfLiveData or getMapSinceVersion. As we have not removed the calls from ProposalService and other domains we return getMapOfAllData() instead of the live map. This was prevented earlier for performance reasons. It is more safe thought to return in case of an illegal access all data instead of live data only. --- .../persistence/HistoricalDataStoreService.java | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/p2p/src/main/java/bisq/network/p2p/storage/persistence/HistoricalDataStoreService.java b/p2p/src/main/java/bisq/network/p2p/storage/persistence/HistoricalDataStoreService.java index 45c744d0b64..1510092654b 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/persistence/HistoricalDataStoreService.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/persistence/HistoricalDataStoreService.java @@ -20,6 +20,7 @@ import bisq.network.p2p.storage.P2PDataStorage; import bisq.network.p2p.storage.payload.PersistableNetworkPayload; +import bisq.common.app.DevEnv; import bisq.common.app.Version; import bisq.common.persistence.PersistenceManager; @@ -109,15 +110,11 @@ public Map getMapOfAllData( // MapStoreService /////////////////////////////////////////////////////////////////////////////////////////// - // TODO optimize so that callers to AppendOnlyDataStoreService are not invoking that often getMap - // ProposalService is one of the main callers and could avoid it by using the ProposalStoreService directly - // instead of AppendOnlyDataStoreService - - // By default we return the live data only. This method should not be used by domain clients but rather the - // custom methods getMapOfAllData, getMapOfLiveData or getMapSinceVersion @Override public Map getMap() { - return store.getMap(); + DevEnv.logErrorAndThrowIfDevMode("HistoricalDataStoreService.getMap should not be used by domain " + + "clients but rather the custom methods getMapOfAllData, getMapOfLiveData or getMapSinceVersion"); + return getMapOfAllData(); } @Override