<p></p>
<p><b>@sqrrm</b> commented on this pull request.</p>
<p>I've read through the core parts of the PR. For now just adding comments to gain a better understanding. This PR makes me concerned that we might lose data during upgrades or otherwise, but I don't have a full understanding yet.</p><hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4233#discussion_r427424028">p2p/src/main/java/bisq/network/p2p/storage/persistence/SplitStoreService.java</a>:</p>
<pre style='color:#555'>> + if (getMap().containsKey(hash))
+ return;
+
+ store.getMap().put(hash, payload);
+ persist();
+ }
+
+ @Override
+ protected PersistableNetworkPayload putIfAbsent(P2PDataStorage.ByteArray hash,
+ PersistableNetworkPayload payload) {
+ // make sure we do not add data that we already have (in a bin of historical data)
+ if (getMap().containsKey(hash))
+ return null;
+
+ PersistableNetworkPayload previous = store.getMap().putIfAbsent(hash, payload);
+ persist();
</pre>
<p>Seems excessive to persist if nothing was written, but I notice that's what MapStoreService does.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4233#discussion_r427430085">p2p/src/main/java/bisq/network/p2p/storage/persistence/SplitStoreService.java</a>:</p>
<pre style='color:#555'>> + PersistableNetworkPayload payload) {
+ // make sure we do not add data that we already have (in a bin of historical data)
+ if (getMap().containsKey(hash))
+ return null;
+
+ PersistableNetworkPayload previous = store.getMap().putIfAbsent(hash, payload);
+ persist();
+ return previous;
+ }
+
+
+ @Override
+ public Map<P2PDataStorage.ByteArray, PersistableNetworkPayload> getMap() {
+ HashMap<P2PDataStorage.ByteArray, PersistableNetworkPayload> result = new HashMap<>();
+ result.putAll(store.getMap());
+ history.forEach((s, store) -> result.putAll(store.getMap()));
</pre>
<p>getMap() seems to be called a fair bit and it's recreating the map every time. Not sure how expensive that is, but perhaps some caching should be considered.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4233#discussion_r427433153">p2p/src/main/java/bisq/network/p2p/storage/persistence/SplitStoreService.java</a>:</p>
<pre style='color:#555'>> + history.forEach((s, store) -> result.putAll(store.getMap()));
+
+ return result;
+ }
+
+ @Override
+ public Map<P2PDataStorage.ByteArray, PersistableNetworkPayload> getMap(String filter) {
+ HashMap<P2PDataStorage.ByteArray, PersistableNetworkPayload> result = new HashMap<>();
+ result.putAll(store.getMap());
+
+ // TODO do a proper language, possibly classes
+ if (filter.startsWith("since ")) {
+ filter = filter.replace("since ", "");
+ if (!filter.equals(Version.VERSION)) {
+ String finalFilter = filter;
+ history.entrySet().stream().filter(entry -> Integer.valueOf(entry.getKey().replace(".", "")) > Integer.valueOf(finalFilter.replace(".", ""))).forEach(entry -> result.putAll(entry.getValue().getMap()));
</pre>
<p>Try to keep lines < 120 chars</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4233#discussion_r427436981">p2p/src/main/java/bisq/network/p2p/storage/persistence/SplitStoreService.java</a>:</p>
<pre style='color:#555'>> + if (filter.startsWith("since ")) {
+ filter = filter.replace("since ", "");
+ if (!filter.equals(Version.VERSION)) {
+ String finalFilter = filter;
+ history.entrySet().stream().filter(entry -> Integer.valueOf(entry.getKey().replace(".", "")) > Integer.valueOf(finalFilter.replace(".", ""))).forEach(entry -> result.putAll(entry.getValue().getMap()));
+ }
+ }
+
+ return result;
+ }
+
+ @Override
+ protected void readFromResources(String postFix) {
+ // check Version.VERSION and see if we have latest r/o data store file in working directory
+ if (!new File(absolutePathOfStorageDir + File.separator + getFileName() + "_" + Version.VERSION).exists())
+ makeFileFromResourceFile(postFix); // if we have the latest file, we are good, else do stuff // TODO are we?
</pre>
<p>This checks if the latest file exists...</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4233#discussion_r427439825">p2p/src/main/java/bisq/network/p2p/storage/persistence/SplitStoreService.java</a>:</p>
<pre style='color:#555'>> + history.entrySet().stream().filter(entry -> Integer.valueOf(entry.getKey().replace(".", "")) > Integer.valueOf(finalFilter.replace(".", ""))).forEach(entry -> result.putAll(entry.getValue().getMap()));
+ }
+ }
+
+ return result;
+ }
+
+ @Override
+ protected void readFromResources(String postFix) {
+ // check Version.VERSION and see if we have latest r/o data store file in working directory
+ if (!new File(absolutePathOfStorageDir + File.separator + getFileName() + "_" + Version.VERSION).exists())
+ makeFileFromResourceFile(postFix); // if we have the latest file, we are good, else do stuff // TODO are we?
+ else {
+ // load stores/storage
+ File dbDir = new File(absolutePathOfStorageDir);
+ List<File> resourceFiles = Arrays.asList(dbDir.list((dir, name) -> name.startsWith(getFileName() + "_"))).stream().map(s -> new File(s)).collect(Collectors.toList());
</pre>
<p>... if it does exist we make a list of all the files we have.</p>
<p>Also, line breaking streams make them a lot easier to follow.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4233#discussion_r427444614">p2p/src/main/java/bisq/network/p2p/storage/persistence/SplitStoreService.java</a>:</p>
<pre style='color:#555'>> +
+ @Override
+ protected void readFromResources(String postFix) {
+ // check Version.VERSION and see if we have latest r/o data store file in working directory
+ if (!new File(absolutePathOfStorageDir + File.separator + getFileName() + "_" + Version.VERSION).exists())
+ makeFileFromResourceFile(postFix); // if we have the latest file, we are good, else do stuff // TODO are we?
+ else {
+ // load stores/storage
+ File dbDir = new File(absolutePathOfStorageDir);
+ List<File> resourceFiles = Arrays.asList(dbDir.list((dir, name) -> name.startsWith(getFileName() + "_"))).stream().map(s -> new File(s)).collect(Collectors.toList());
+
+ history = new HashMap<>();
+ store = readStore(getFileName());
+ resourceFiles.forEach(file -> {
+ SplitStore tmp = readStore(file.getName().replace(postFix, ""));
+ history.put(file.getName().replace(postFix, "").replace(getFileName(), "").replace("_", ""), tmp);
</pre>
<p>Then we add them to our history. What is the key, not easy to follow. Do we depend on having all the previous stores since we're only checking for the last one?</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4233#discussion_r427453901">p2p/src/main/java/bisq/network/p2p/storage/persistence/SplitStoreService.java</a>:</p>
<pre style='color:#555'>> + }
+
+ @Override
+ protected void makeFileFromResourceFile(String postFix) {
+ File dbDir = new File(absolutePathOfStorageDir);
+ if (!dbDir.exists() && !dbDir.mkdir())
+ log.warn("make dir failed.\ndbDir=" + dbDir.getAbsolutePath());
+
+ // check resources for files
+ List<String> versions = new ArrayList<>();
+ versions.add(Version.VERSION);
+ versions.addAll(Version.history);
+ List<String> resourceFiles = versions.stream().map(s -> getFileName() + "_" + s + postFix).collect(Collectors.toList());
+
+ // if not, copy and split
+ resourceFiles.forEach(file -> {
</pre>
<p><code>file</code> is a filename string...</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4233#discussion_r427454240">p2p/src/main/java/bisq/network/p2p/storage/persistence/SplitStoreService.java</a>:</p>
<pre style='color:#555'>> + @Override
+ protected void makeFileFromResourceFile(String postFix) {
+ File dbDir = new File(absolutePathOfStorageDir);
+ if (!dbDir.exists() && !dbDir.mkdir())
+ log.warn("make dir failed.\ndbDir=" + dbDir.getAbsolutePath());
+
+ // check resources for files
+ List<String> versions = new ArrayList<>();
+ versions.add(Version.VERSION);
+ versions.addAll(Version.history);
+ List<String> resourceFiles = versions.stream().map(s -> getFileName() + "_" + s + postFix).collect(Collectors.toList());
+
+ // if not, copy and split
+ resourceFiles.forEach(file -> {
+ final File destinationFile = new File(Paths.get(absolutePathOfStorageDir, file.replace(postFix, "")).toString());
+ final String resourceFileName = file;
</pre>
<p>...making this var a bit excessive.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4233#discussion_r427455440">p2p/src/main/java/bisq/network/p2p/storage/persistence/SplitStoreService.java</a>:</p>
<pre style='color:#555'>> + log.error("Could not copy resourceFile " + resourceFileName + " to " +
+ destinationFile.getAbsolutePath() + ".\n" + e.getMessage());
+ e.printStackTrace();
+ }
+ } else {
+ log.debug(file + " file exists already.");
+ }
+ });
+
+ // split
+ // - get all
+ history = new HashMap<>();
+ store = readStore(getFileName());
+ resourceFiles.forEach(file -> {
+ SplitStore tmp = readStore(file.replace(postFix, ""));
+ history.put(file.replace(postFix, "").replace(getFileName(), "").replace("_", ""), tmp);
</pre>
<p>A helper method to do these string manipulations would help readability.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4233#discussion_r427461213">p2p/src/main/java/bisq/network/p2p/storage/persistence/SplitStoreService.java</a>:</p>
<pre style='color:#555'>> + e.printStackTrace();
+ }
+ } else {
+ log.debug(file + " file exists already.");
+ }
+ });
+
+ // split
+ // - get all
+ history = new HashMap<>();
+ store = readStore(getFileName());
+ resourceFiles.forEach(file -> {
+ SplitStore tmp = readStore(file.replace(postFix, ""));
+ history.put(file.replace(postFix, "").replace(getFileName(), "").replace("_", ""), tmp);
+ // - subtract all that is in resource files
+ store.getMap().keySet().removeAll(tmp.getMap().keySet());
</pre>
<p>With this change I think we're making the resource files the final arbiter of what is the distributed data. Is that what we want? Even for full nodes that are always on.</p>
<p>From what I understood so far, nodes will keep getting data from the network. With new versions all new data since the last version will be stored in a new <code>resource_x.x.x_MAINNET</code> file and all nodes will restore this data from resources. It looks like it will just throw away its own local data on new versions though, that seems incorrect.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4233#discussion_r427464365">p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java</a>:</p>
<pre style='color:#555'>> @@ -205,24 +213,47 @@ public GetUpdatedDataRequest buildGetUpdatedDataRequest(NodeAddress senderNodeAd
return new GetUpdatedDataRequest(senderNodeAddress, nonce, this.getKnownPayloadHashes());
}
+ private byte[] getSpecialKey() {
+ byte[] result = new byte[20];
+ Arrays.fill(result, (byte) 0);
+ System.arraycopy(Version.VERSION.getBytes(), 0, result, 0, Version.VERSION.length());
+ return result;
+ }
+
+ private String containsSpecialKey(Set<P2PDataStorage.ByteArray> collection) {
+ Optional<String> result = collection.stream().map(byteArray -> new String(byteArray.bytes).trim()).filter(s -> s.matches("^[0-9]\\.[0-9]\\.[0-9]$")).findFirst();
+ if (result.isPresent())
+ return result.get();
+ else
+ return "";
</pre>
<p>Easier to read would be <code> return result.orElse("");</code><br>
or even</p>
<pre><code> return collection.stream()
.map(byteArray -> new String(byteArray.bytes).trim())
.filter(s -> s.matches("^[0-9]\\.[0-9]\\.[0-9]$"))
.findFirst()
.orElse("");
</code></pre>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4233#discussion_r427471959">p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java</a>:</p>
<pre style='color:#555'>> @@ -205,24 +213,47 @@ public GetUpdatedDataRequest buildGetUpdatedDataRequest(NodeAddress senderNodeAd
return new GetUpdatedDataRequest(senderNodeAddress, nonce, this.getKnownPayloadHashes());
}
+ private byte[] getSpecialKey() {
+ byte[] result = new byte[20];
+ Arrays.fill(result, (byte) 0);
+ System.arraycopy(Version.VERSION.getBytes(), 0, result, 0, Version.VERSION.length());
+ return result;
+ }
+
+ private String containsSpecialKey(Set<P2PDataStorage.ByteArray> collection) {
+ Optional<String> result = collection.stream().map(byteArray -> new String(byteArray.bytes).trim()).filter(s -> s.matches("^[0-9]\\.[0-9]\\.[0-9]$")).findFirst();
</pre>
<p>Is String(byte[]) safe enough? I suspect so but I'm not sure.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4233#discussion_r427474935">p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java</a>:</p>
<pre style='color:#555'>> // We collect the keys of the PersistableNetworkPayload items so we exclude them in our request.
// PersistedStoragePayload items don't get removed, so we don't have an issue with the case that
// an object gets removed in between PreliminaryGetDataRequest and the GetUpdatedDataRequest and we would
// miss that event if we do not load the full set or use some delta handling.
- Set<byte[]> excludedKeys = this.appendOnlyDataStoreService.getMap().keySet().stream()
- .map(e -> e.bytes)
- .collect(Collectors.toSet());
+ Set<byte[]> excludedKeys;
+ if (seedNodeRepository != null && seedNodeRepository.isSeedNode(networkNode.getNodeAddress()))
</pre>
<p>Braces around multi-line blocks really helps readability in my opinion.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4233#discussion_r427478979">p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java</a>:</p>
<pre style='color:#555'>> + Map<ByteArray, PersistableNetworkPayload> tmp;
+ if ("".equals(specialKey))
+ tmp = this.appendOnlyDataStoreService.getMap();
+ else {
+ tmp = this.appendOnlyDataStoreService.getMap("since " + specialKey);
+ }
</pre>
<p>Is this the separation of old and new requests for data? More comments would be helpful for understanding.</p>
<p>Variable name <code>tmp</code> isn't really descriptive, better naming would help.</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/4233#pullrequestreview-414605107">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AJFFTNQWBYIFOAUYN6CEHILRSK72VANCNFSM4MY33J4A">unsubscribe</a>.<img src="https://github.com/notifications/beacon/AJFFTNTO7MIOZFSZAP6BUADRSK72VA5CNFSM4MY33J4KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGODC3F6MY.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/4233#pullrequestreview-414605107",
"url": "https://github.com/bisq-network/bisq/pull/4233#pullrequestreview-414605107",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>