From 6532c258f6f79edfd734b1800c6b7a3922dc94cd Mon Sep 17 00:00:00 2001 From: CalDescent Date: Mon, 10 May 2021 09:10:14 +0100 Subject: [PATCH 01/12] Reduced log spam. --- src/main/java/org/qortal/network/Handshake.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/qortal/network/Handshake.java b/src/main/java/org/qortal/network/Handshake.java index 8bee63a2..78b181ce 100644 --- a/src/main/java/org/qortal/network/Handshake.java +++ b/src/main/java/org/qortal/network/Handshake.java @@ -75,7 +75,7 @@ public enum Handshake { // Ensure the peer is running at least the minimum version allowed for connections final String minPeerVersion = Settings.getInstance().getMinPeerVersion(); if (peer.isAtLeastVersion(minPeerVersion) == false) { - LOGGER.info(String.format("Ignoring peer %s because it is on an old version (%s)", peer, versionString)); + LOGGER.debug(String.format("Ignoring peer %s because it is on an old version (%s)", peer, versionString)); return null; } } From d2649b237c9d036c35e41beada0580d8ccac3b45 Mon Sep 17 00:00:00 2001 From: CalDescent Date: Tue, 11 May 2021 19:01:23 +0100 Subject: [PATCH 02/12] Moved chain weight calculation log from DEBUG to TRACE. --- src/main/java/org/qortal/block/Block.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/qortal/block/Block.java b/src/main/java/org/qortal/block/Block.java index 7f1f9da9..de52b0c4 100644 --- a/src/main/java/org/qortal/block/Block.java +++ b/src/main/java/org/qortal/block/Block.java @@ -831,7 +831,7 @@ public class Block { if (NTP.getTime() >= BlockChain.getInstance().getCalcChainWeightTimestamp() && parentHeight >= maxHeight) break; } - LOGGER.debug(String.format("Chain weight calculation was based on %d blocks", blockCount)); + LOGGER.trace(String.format("Chain weight calculation was based on %d blocks", blockCount)); return cumulativeWeight; } From deb8adafc99023f2fe71df1e4b27fc20419bc740 Mon Sep 17 00:00:00 2001 From: CalDescent Date: Sat, 15 May 2021 09:15:29 +0100 Subject: [PATCH 03/12] Added org.json dependency. The com.googlecode.json-simple dependency we use in other parts of the project isn't ideal for some of the more complex parsing. --- pom.xml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pom.xml b/pom.xml index 43e9eae1..9e5d8aff 100644 --- a/pom.xml +++ b/pom.xml @@ -439,6 +439,11 @@ json-simple 1.1.1 + + org.json + json + 20210307 + org.apache.commons commons-text From 5824f75669b43eb6f0d86a9056416f9238bcad58 Mon Sep 17 00:00:00 2001 From: CalDescent Date: Sat, 15 May 2021 12:19:15 +0100 Subject: [PATCH 04/12] Rework of the repository export and import functions. The existing HSQL export/import (PERFORM EXPORT SCRIPT and PERFORM IMPORT SCRIPT) have been replaced with a custom JSON import and export. Whilst this is less generic, it has some significant advantages: - When exporting data, it is now able to combine the exported data with any data that already exists in the backup file. This prevents a backup after a bootstrap from overwriting data from before the bootstrap, and removes the need for all of the "archive" files that we currently create. - Adds support for partial imports, and updates. Previously an import would fail if any of the data being imported already existed in the db. It will now add new rows and update existing ones. - The format and contents of the exported trade bot data now matches the output of the /crosschain/tradebot API. - Data is retrieved without the need for a database lock, and therefore the export process is much faster and less invasive. This should prevent the lockups and other problems seen when using the trade portal. For now, there are a couple of trade-offs to using this new approach: - The minting key import/export has been temporarily removed until there is more time to transition it to this new format. - Existing .script backups can no longer be imported using versions higher than 1.5.1. Both of these can be solved by temporarily running version 1.5.1, performing the necessary imports/exports, then returning to the latest version. Longer term the minting keys export/import will be reimplemented using the JSON format. --- .../qortal/api/resource/AdminResource.java | 19 +--- .../qortal/controller/tradebot/TradeBot.java | 12 +-- .../qortal/data/crosschain/TradeBotData.java | 55 ++++++++++ .../org/qortal/repository/Repository.java | 2 +- .../repository/hsqldb/HSQLDBRepository.java | 101 +++++++++--------- 5 files changed, 115 insertions(+), 74 deletions(-) diff --git a/src/main/java/org/qortal/api/resource/AdminResource.java b/src/main/java/org/qortal/api/resource/AdminResource.java index c295b90b..719a3b9d 100644 --- a/src/main/java/org/qortal/api/resource/AdminResource.java +++ b/src/main/java/org/qortal/api/resource/AdminResource.java @@ -542,19 +542,8 @@ public class AdminResource { Security.checkApiCallAllowed(request); try (final Repository repository = RepositoryManager.getRepository()) { - ReentrantLock blockchainLock = Controller.getInstance().getBlockchainLock(); - - blockchainLock.lockInterruptibly(); - - try { - repository.exportNodeLocalData(true); - return "true"; - } finally { - blockchainLock.unlock(); - } - } catch (InterruptedException e) { - // We couldn't lock blockchain to perform export - return "false"; + repository.exportNodeLocalData(); + return "true"; } catch (DataException e) { throw ApiExceptionFactory.INSTANCE.createException(request, ApiError.REPOSITORY_ISSUE, e); } @@ -564,7 +553,7 @@ public class AdminResource { @Path("/repository/data") @Operation( summary = "Import data into repository.", - description = "Imports data from file on local machine. Filename is forced to 'import.script' if apiKey is not set.", + description = "Imports data from file on local machine. Filename is forced to 'import.json' if apiKey is not set.", requestBody = @RequestBody( required = true, content = @Content( @@ -588,7 +577,7 @@ public class AdminResource { // Hard-coded because it's too dangerous to allow user-supplied filenames in weaker security contexts if (Settings.getInstance().getApiKey() == null) - filename = "import.script"; + filename = "import.json"; try (final Repository repository = RepositoryManager.getRepository()) { ReentrantLock blockchainLock = Controller.getInstance().getBlockchainLock(); diff --git a/src/main/java/org/qortal/controller/tradebot/TradeBot.java b/src/main/java/org/qortal/controller/tradebot/TradeBot.java index 94c7cefb..fa3b599e 100644 --- a/src/main/java/org/qortal/controller/tradebot/TradeBot.java +++ b/src/main/java/org/qortal/controller/tradebot/TradeBot.java @@ -272,15 +272,9 @@ public class TradeBot implements Listener { // Attempt to backup the trade bot data. This an optional step and doesn't impact trading, so don't throw an exception on failure try { LOGGER.info("About to backup trade bot data..."); - ReentrantLock blockchainLock = Controller.getInstance().getBlockchainLock(); - blockchainLock.lockInterruptibly(); - try { - repository.exportNodeLocalData(true); - } finally { - blockchainLock.unlock(); - } - } catch (InterruptedException | DataException e) { - LOGGER.info(String.format("Failed to obtain blockchain lock when exporting trade bot data: %s", e.getMessage())); + repository.exportNodeLocalData(); + } catch (DataException e) { + LOGGER.info(String.format("Repository issue when exporting trade bot data: %s", e.getMessage())); } } diff --git a/src/main/java/org/qortal/data/crosschain/TradeBotData.java b/src/main/java/org/qortal/data/crosschain/TradeBotData.java index b360c53e..19481466 100644 --- a/src/main/java/org/qortal/data/crosschain/TradeBotData.java +++ b/src/main/java/org/qortal/data/crosschain/TradeBotData.java @@ -6,6 +6,9 @@ import javax.xml.bind.annotation.XmlTransient; import javax.xml.bind.annotation.adapters.XmlJavaTypeAdapter; import io.swagger.v3.oas.annotations.media.Schema; +import org.json.JSONObject; + +import org.qortal.utils.Base58; // All properties to be converted to JSON via JAXB @XmlAccessorType(XmlAccessType.FIELD) @@ -205,6 +208,58 @@ public class TradeBotData { return this.receivingAccountInfo; } + public JSONObject toJson() { + JSONObject jsonObject = new JSONObject(); + jsonObject.put("tradePrivateKey", Base58.encode(this.getTradePrivateKey())); + jsonObject.put("acctName", this.getAcctName()); + jsonObject.put("tradeState", this.getState()); + jsonObject.put("tradeStateValue", this.getStateValue()); + jsonObject.put("creatorAddress", this.getCreatorAddress()); + jsonObject.put("atAddress", this.getAtAddress()); + jsonObject.put("timestamp", this.getTimestamp()); + jsonObject.put("qortAmount", this.getQortAmount()); + if (this.getTradeNativePublicKey() != null) jsonObject.put("tradeNativePublicKey", Base58.encode(this.getTradeNativePublicKey())); + if (this.getTradeNativePublicKeyHash() != null) jsonObject.put("tradeNativePublicKeyHash", Base58.encode(this.getTradeNativePublicKeyHash())); + jsonObject.put("tradeNativeAddress", this.getTradeNativeAddress()); + if (this.getSecret() != null) jsonObject.put("secret", Base58.encode(this.getSecret())); + if (this.getHashOfSecret() != null) jsonObject.put("hashOfSecret", Base58.encode(this.getHashOfSecret())); + jsonObject.put("foreignBlockchain", this.getForeignBlockchain()); + if (this.getTradeForeignPublicKey() != null) jsonObject.put("tradeForeignPublicKey", Base58.encode(this.getTradeForeignPublicKey())); + if (this.getTradeForeignPublicKeyHash() != null) jsonObject.put("tradeForeignPublicKeyHash", Base58.encode(this.getTradeForeignPublicKeyHash())); + jsonObject.put("foreignKey", this.getForeignKey()); + jsonObject.put("foreignAmount", this.getForeignAmount()); + if (this.getLastTransactionSignature() != null) jsonObject.put("lastTransactionSignature", Base58.encode(this.getLastTransactionSignature())); + jsonObject.put("lockTimeA", this.getLockTimeA()); + if (this.getReceivingAccountInfo() != null) jsonObject.put("receivingAccountInfo", Base58.encode(this.getReceivingAccountInfo())); + return jsonObject; + } + + public static TradeBotData fromJson(JSONObject json) { + return new TradeBotData( + json.isNull("tradePrivateKey") ? null : Base58.decode(json.getString("tradePrivateKey")), + json.isNull("acctName") ? null : json.getString("acctName"), + json.isNull("tradeState") ? null : json.getString("tradeState"), + json.isNull("tradeStateValue") ? null : json.getInt("tradeStateValue"), + json.isNull("creatorAddress") ? null : json.getString("creatorAddress"), + json.isNull("atAddress") ? null : json.getString("atAddress"), + json.isNull("timestamp") ? null : json.getLong("timestamp"), + json.isNull("qortAmount") ? null : json.getLong("qortAmount"), + json.isNull("tradeNativePublicKey") ? null : Base58.decode(json.getString("tradeNativePublicKey")), + json.isNull("tradeNativePublicKeyHash") ? null : Base58.decode(json.getString("tradeNativePublicKeyHash")), + json.isNull("tradeNativeAddress") ? null : json.getString("tradeNativeAddress"), + json.isNull("secret") ? null : Base58.decode(json.getString("secret")), + json.isNull("hashOfSecret") ? null : Base58.decode(json.getString("hashOfSecret")), + json.isNull("foreignBlockchain") ? null : json.getString("foreignBlockchain"), + json.isNull("tradeForeignPublicKey") ? null : Base58.decode(json.getString("tradeForeignPublicKey")), + json.isNull("tradeForeignPublicKeyHash") ? null : Base58.decode(json.getString("tradeForeignPublicKeyHash")), + json.isNull("foreignAmount") ? null : json.getLong("foreignAmount"), + json.isNull("foreignKey") ? null : json.getString("foreignKey"), + json.isNull("lastTransactionSignature") ? null : Base58.decode(json.getString("lastTransactionSignature")), + json.isNull("lockTimeA") ? null : json.getInt("lockTimeA"), + json.isNull("receivingAccountInfo") ? null : Base58.decode(json.getString("receivingAccountInfo")) + ); + } + // Mostly for debugging public String toString() { return String.format("%s: %s (%d)", this.atAddress, this.tradeState, this.tradeStateValue); diff --git a/src/main/java/org/qortal/repository/Repository.java b/src/main/java/org/qortal/repository/Repository.java index 5438f1d9..656e6e1e 100644 --- a/src/main/java/org/qortal/repository/Repository.java +++ b/src/main/java/org/qortal/repository/Repository.java @@ -49,7 +49,7 @@ public interface Repository extends AutoCloseable { public void performPeriodicMaintenance() throws DataException; - public void exportNodeLocalData(boolean keepArchivedCopy) throws DataException; + public void exportNodeLocalData() throws DataException; public void importDataFromFile(String filename) throws DataException; diff --git a/src/main/java/org/qortal/repository/hsqldb/HSQLDBRepository.java b/src/main/java/org/qortal/repository/hsqldb/HSQLDBRepository.java index 5557c13e..09c6a6d4 100644 --- a/src/main/java/org/qortal/repository/hsqldb/HSQLDBRepository.java +++ b/src/main/java/org/qortal/repository/hsqldb/HSQLDBRepository.java @@ -2,6 +2,7 @@ package org.qortal.repository.hsqldb; import java.awt.TrayIcon.MessageType; import java.io.File; +import java.io.FileWriter; import java.io.IOException; import java.math.BigDecimal; import java.nio.file.Files; @@ -15,23 +16,19 @@ import java.sql.ResultSet; import java.sql.SQLException; import java.sql.Savepoint; import java.sql.Statement; -import java.util.ArrayDeque; -import java.util.ArrayList; -import java.util.Collection; -import java.util.Collections; -import java.util.Comparator; -import java.util.Deque; -import java.util.HashMap; -import java.util.List; -import java.util.Map; +import java.util.*; import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.stream.Stream; +import org.json.JSONArray; +import org.json.JSONObject; + import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.qortal.account.PrivateKeyAccount; import org.qortal.crypto.Crypto; +import org.qortal.data.crosschain.TradeBotData; import org.qortal.globalization.Translator; import org.qortal.gui.SysTray; import org.qortal.repository.ATRepository; @@ -52,7 +49,7 @@ import org.qortal.repository.TransactionRepository; import org.qortal.repository.VotingRepository; import org.qortal.repository.hsqldb.transaction.HSQLDBTransactionRepository; import org.qortal.settings.Settings; -import org.qortal.utils.NTP; +import org.qortal.utils.Base58; public class HSQLDBRepository implements Repository { @@ -460,8 +457,7 @@ public class HSQLDBRepository implements Repository { } @Override - public void exportNodeLocalData(boolean keepArchivedCopy) throws DataException { - + public void exportNodeLocalData() throws DataException { // Create the qortal-backup folder if it doesn't exist Path backupPath = Paths.get("qortal-backup"); try { @@ -471,52 +467,59 @@ public class HSQLDBRepository implements Repository { throw new DataException("Unable to create backup folder"); } - // We need to rename or delete an existing TradeBotStates backup before creating a new one - File tradeBotStatesBackupFile = new File("qortal-backup/TradeBotStates.script"); - if (tradeBotStatesBackupFile.exists()) { - if (keepArchivedCopy) { - // Rename existing TradeBotStates backup, to make sure that we're not overwriting any keys - File archivedBackupFile = new File(String.format("qortal-backup/TradeBotStates-archive-%d.script", NTP.getTime())); - if (tradeBotStatesBackupFile.renameTo(archivedBackupFile)) - LOGGER.info(String.format("Moved existing TradeBotStates backup file to %s", archivedBackupFile.getPath())); - else - throw new DataException("Unable to rename existing TradeBotStates backup"); - } else { - // Delete existing copy - LOGGER.info("Deleting existing TradeBotStates backup because it is being replaced with a new one"); - tradeBotStatesBackupFile.delete(); + try { + // Load trade bot data + List allTradeBotData = this.getCrossChainRepository().getAllTradeBotData(); + JSONArray allTradeBotDataJson = new JSONArray(); + for (TradeBotData tradeBotData : allTradeBotData) { + JSONObject tradeBotDataJson = tradeBotData.toJson(); + allTradeBotDataJson.put(tradeBotDataJson); } - } - // There's currently no need to take an archived copy of the MintingAccounts data - just delete the old one if it exists - File mintingAccountsBackupFile = new File("qortal-backup/MintingAccounts.script"); - if (mintingAccountsBackupFile.exists()) { - LOGGER.info("Deleting existing MintingAccounts backup because it is being replaced with a new one"); - mintingAccountsBackupFile.delete(); - } + // We need to combine existing TradeBotStates data before overwriting + String fileName = "qortal-backup/TradeBotStates.json"; + File tradeBotStatesBackupFile = new File(fileName); + if (tradeBotStatesBackupFile.exists()) { + String jsonString = new String(Files.readAllBytes(Paths.get(fileName))); + JSONArray allExistingTradeBotData = new JSONArray(jsonString); + Iterator iterator = allExistingTradeBotData.iterator(); + while(iterator.hasNext()) { + JSONObject existingTradeBotData = (JSONObject)iterator.next(); + String existingTradePrivateKey = (String) existingTradeBotData.get("tradePrivateKey"); + // Check if we already have an entry for this trade + boolean found = allTradeBotData.stream().anyMatch(tradeBotData -> Base58.encode(tradeBotData.getTradePrivateKey()).equals(existingTradePrivateKey)); + if (found == false) + // We need to add this to our list + allTradeBotDataJson.put(existingTradeBotData); + } + } - try (Statement stmt = this.connection.createStatement()) { - stmt.execute("PERFORM EXPORT SCRIPT FOR TABLE MintingAccounts DATA TO 'qortal-backup/MintingAccounts.script'"); - stmt.execute("PERFORM EXPORT SCRIPT FOR TABLE TradeBotStates DATA TO 'qortal-backup/TradeBotStates.script'"); - LOGGER.info("Exported sensitive/node-local data: minting keys and trade bot states"); - } catch (SQLException e) { - throw new DataException("Unable to export sensitive/node-local data from repository"); + FileWriter writer = new FileWriter(fileName); + writer.write(allTradeBotDataJson.toString()); + writer.close(); + LOGGER.info("Exported sensitive/node-local data: trade bot states"); + + } catch (DataException | IOException e) { + throw new DataException("Unable to export trade bot states from repository"); } } @Override public void importDataFromFile(String filename) throws DataException { - try (Statement stmt = this.connection.createStatement()) { - LOGGER.info(() -> String.format("Importing data into repository from %s", filename)); - - String escapedFilename = stmt.enquoteLiteral(filename); - stmt.execute("PERFORM IMPORT SCRIPT DATA FROM " + escapedFilename + " CONTINUE ON ERROR"); - - LOGGER.info(() -> String.format("Imported data into repository from %s", filename)); - } catch (SQLException e) { - LOGGER.info(() -> String.format("Failed to import data into repository from %s: %s", filename, e.getMessage())); - throw new DataException("Unable to import sensitive/node-local data to repository: " + e.getMessage()); + LOGGER.info(() -> String.format("Importing data into repository from %s", filename)); + try { + String jsonString = new String(Files.readAllBytes(Paths.get(filename))); + JSONArray tradeBotDataToImport = new JSONArray(jsonString); + Iterator iterator = tradeBotDataToImport.iterator(); + while(iterator.hasNext()) { + JSONObject tradeBotDataJson = (JSONObject)iterator.next(); + TradeBotData tradeBotData = TradeBotData.fromJson(tradeBotDataJson); + this.getCrossChainRepository().save(tradeBotData); + } + } catch (IOException e) { + throw new DataException("Unable to import sensitive/node-local trade bot states to repository: " + e.getMessage()); } + LOGGER.info(() -> String.format("Imported trade bot states into repository from %s", filename)); } @Override From 92d8c37d7d80d1db3075e43cd548049df77e6e96 Mon Sep 17 00:00:00 2001 From: CalDescent Date: Sat, 15 May 2021 12:54:46 +0100 Subject: [PATCH 05/12] Added AT count to block debug logs. --- src/main/java/org/qortal/block/Block.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/org/qortal/block/Block.java b/src/main/java/org/qortal/block/Block.java index de52b0c4..3cb134ff 100644 --- a/src/main/java/org/qortal/block/Block.java +++ b/src/main/java/org/qortal/block/Block.java @@ -2011,6 +2011,7 @@ public class Block { LOGGER.debug(String.format("Timestamp: %d", this.getBlockData().getTimestamp())); LOGGER.debug(String.format("Minter level: %d", minterLevel)); LOGGER.debug(String.format("Online accounts: %d", this.getBlockData().getOnlineAccountsCount())); + LOGGER.debug(String.format("AT count: %d", this.getBlockData().getATCount())); BlockSummaryData blockSummaryData = new BlockSummaryData(this.getBlockData()); if (this.getParent() == null || this.getParent().getSignature() == null || blockSummaryData == null || minterLevel == 0) From 66711c2e9db5afa02a28b41d72f29a5030f83010 Mon Sep 17 00:00:00 2001 From: CalDescent Date: Sun, 16 May 2021 08:45:23 +0100 Subject: [PATCH 06/12] Require a complete sync in syncToPeerChain() We have gone backwards and forwards on this one a lot recently, but now that stability has returned, it is best to tighten this up. Previously it was loosened to help reduce network load, but that is no longer a problem. With this stricter approach, it should prevent a node ending up in an incomplete state after syncing, which is the main cause of the shorter re-orgs we are seeing. --- .../org/qortal/controller/Synchronizer.java | 21 ++++--------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/src/main/java/org/qortal/controller/Synchronizer.java b/src/main/java/org/qortal/controller/Synchronizer.java index 42b46a31..948e9785 100644 --- a/src/main/java/org/qortal/controller/Synchronizer.java +++ b/src/main/java/org/qortal/controller/Synchronizer.java @@ -785,19 +785,13 @@ public class Synchronizer { if (cachedCommonBlockData != null) cachedCommonBlockData.setBlockSummariesAfterCommonBlock(null); - // If we have already received recent or newer blocks from this peer, go ahead and apply them + // If we have already received newer blocks from this peer that what we have already, go ahead and apply them if (peerBlocks.size() > 0) { final BlockData ourLatestBlockData = repository.getBlockRepository().getLastBlock(); final Block peerLatestBlock = peerBlocks.get(peerBlocks.size() - 1); final Long minLatestBlockTimestamp = Controller.getMinimumLatestBlockTimestamp(); if (ourLatestBlockData != null && peerLatestBlock != null && minLatestBlockTimestamp != null) { - // If we have received at least one recent block, we can apply them - if (peerLatestBlock.getBlockData().getTimestamp() > minLatestBlockTimestamp) { - LOGGER.debug("Newly received blocks are recent, so we will apply them"); - break; - } - // If our latest block is very old.... if (ourLatestBlockData.getTimestamp() < minLatestBlockTimestamp) { // ... and we have received a block that is more recent than our latest block ... @@ -813,7 +807,7 @@ public class Synchronizer { } } } - // Otherwise, give up and move on to the next peer, to avoid putting our chain into an outdated state + // Otherwise, give up and move on to the next peer, to avoid putting our chain into an outdated or incomplete state return SynchronizationResult.NO_REPLY; } @@ -837,20 +831,13 @@ public class Synchronizer { nextHeight, Base58.encode(nextPeerSignature))); if (retryCount >= maxRetries) { - - // If we have already received recent or newer blocks from this peer, go ahead and apply them + // If we have already received newer blocks from this peer that what we have already, go ahead and apply them if (peerBlocks.size() > 0) { final BlockData ourLatestBlockData = repository.getBlockRepository().getLastBlock(); final Block peerLatestBlock = peerBlocks.get(peerBlocks.size() - 1); final Long minLatestBlockTimestamp = Controller.getMinimumLatestBlockTimestamp(); if (ourLatestBlockData != null && peerLatestBlock != null && minLatestBlockTimestamp != null) { - // If we have received at least one recent block, we can apply them - if (peerLatestBlock.getBlockData().getTimestamp() > minLatestBlockTimestamp) { - LOGGER.debug("Newly received blocks are recent, so we will apply them"); - break; - } - // If our latest block is very old.... if (ourLatestBlockData.getTimestamp() < minLatestBlockTimestamp) { // ... and we have received a block that is more recent than our latest block ... @@ -866,7 +853,7 @@ public class Synchronizer { } } } - // Otherwise, give up and move on to the next peer, to avoid putting our chain into an outdated state + // Otherwise, give up and move on to the next peer, to avoid putting our chain into an outdated or incomplete state return SynchronizationResult.NO_REPLY; } else { From 28d50bccf9bf5da17ca6e0d0be653cb14b7a8af0 Mon Sep 17 00:00:00 2001 From: CalDescent Date: Sun, 16 May 2021 09:15:37 +0100 Subject: [PATCH 07/12] Exclude peers if we don't have a complete set of their block summaries. This tightens up the decision making by adding two requirements: 1. The peer must return the same number of summaries to the ones requested. 2. The peer must return a summary that matches its latest reported signature. This ensures we are always making sync decisions based on accurate data, and removes peers that are currently mid re-org. This is probably more validation than is actually necessary, but it's best to be really thorough here so it is as optimized as possible. --- .../org/qortal/controller/Synchronizer.java | 26 +++++++++++++++---- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/qortal/controller/Synchronizer.java b/src/main/java/org/qortal/controller/Synchronizer.java index 948e9785..8b566fdc 100644 --- a/src/main/java/org/qortal/controller/Synchronizer.java +++ b/src/main/java/org/qortal/controller/Synchronizer.java @@ -282,7 +282,9 @@ public class Synchronizer { return peers; // Count the number of blocks this peer has beyond our common block - final int peerHeight = peer.getChainTipData().getLastHeight(); + final PeerChainTipData peerChainTipData = peer.getChainTipData(); + final int peerHeight = peerChainTipData.getLastHeight(); + final byte[] peerLastBlockSignature = peerChainTipData.getLastBlockSignature(); final int peerAdditionalBlocksAfterCommonBlock = peerHeight - commonBlockSummary.getHeight(); // Limit the number of blocks we are comparing. FUTURE: we could request more in batches, but there may not be a case when this is needed int summariesRequired = Math.min(peerAdditionalBlocksAfterCommonBlock, MAXIMUM_REQUEST_SIZE); @@ -302,15 +304,23 @@ public class Synchronizer { if (summariesRequired > 0) { LOGGER.trace(String.format("Requesting %d block summar%s from peer %s after common block %.8s. Peer height: %d", summariesRequired, (summariesRequired != 1 ? "ies" : "y"), peer, Base58.encode(commonBlockSummary.getSignature()), peerHeight)); - List blockSummaries = this.getBlockSummaries(peer, commonBlockSummary.getSignature(), summariesRequired); - peer.getCommonBlockData().setBlockSummariesAfterCommonBlock(blockSummaries); + // Forget any cached summaries + peer.getCommonBlockData().setBlockSummariesAfterCommonBlock(null); + // Request new block summaries + List blockSummaries = this.getBlockSummaries(peer, commonBlockSummary.getSignature(), summariesRequired); if (blockSummaries != null) { LOGGER.trace(String.format("Peer %s returned %d block summar%s", peer, blockSummaries.size(), (blockSummaries.size() != 1 ? "ies" : "y"))); if (blockSummaries.size() < summariesRequired) - // This could mean that the peer has re-orged. But we still have the same common block, so it's safe to proceed with this set of signatures instead. - LOGGER.debug(String.format("Peer %s returned %d block summar%s instead of expected %d", peer, blockSummaries.size(), (blockSummaries.size() != 1 ? "ies" : "y"), summariesRequired)); + // This could mean that the peer has re-orged. Exclude this peer until they return the summaries we expect. + LOGGER.debug(String.format("Peer %s returned %d block summar%s instead of expected %d - excluding them from this round", peer, blockSummaries.size(), (blockSummaries.size() != 1 ? "ies" : "y"), summariesRequired)); + else if (blockSummaryWithSignature(peerLastBlockSignature, blockSummaries) == null) + // We don't have a block summary for the peer's reported chain tip, so should exclude it + LOGGER.debug(String.format("Peer %s didn't return a block summary with signature %.8s - excluding them from this round", peer, Base58.encode(peerLastBlockSignature))); + else + // All looks good, so store the retrieved block summaries in the peer's cache + peer.getCommonBlockData().setBlockSummariesAfterCommonBlock(blockSummaries); } } else { // There are no block summaries after this common block @@ -451,6 +461,12 @@ public class Synchronizer { return minChainLength; } + private BlockSummaryData blockSummaryWithSignature(byte[] signature, List blockSummaries) { + if (blockSummaries != null) + return blockSummaries.stream().filter(blockSummary -> Arrays.equals(blockSummary.getSignature(), signature)).findAny().orElse(null); + return null; + } + /** * Attempt to synchronize blockchain with peer. From 84bf5702430b2a724a5519b9fec3be9d33f0cea0 Mon Sep 17 00:00:00 2001 From: CalDescent Date: Sun, 16 May 2021 09:51:11 +0100 Subject: [PATCH 08/12] Added optional "maxtrades" parameter to /crosschain/price/{blockchain} API This specifies the maximum number of trades to be used when calculating the price. Default: 10 --- .../java/org/qortal/api/resource/CrossChainResource.java | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/qortal/api/resource/CrossChainResource.java b/src/main/java/org/qortal/api/resource/CrossChainResource.java index d1692b71..fdd74b7d 100644 --- a/src/main/java/org/qortal/api/resource/CrossChainResource.java +++ b/src/main/java/org/qortal/api/resource/CrossChainResource.java @@ -255,14 +255,19 @@ public class CrossChainResource { description = "foreign blockchain", example = "LITECOIN", schema = @Schema(implementation = SupportedBlockchain.class) - ) @PathParam("blockchain") SupportedBlockchain foreignBlockchain) { + ) @PathParam("blockchain") SupportedBlockchain foreignBlockchain, + @Parameter( + description = "Maximum number of trades to include in price calculation", + example = "10", + schema = @Schema(type = "integer", defaultValue = "10") + ) @QueryParam("maxtrades") Integer maxtrades) { // foreignBlockchain is required if (foreignBlockchain == null) throw ApiExceptionFactory.INSTANCE.createException(request, ApiError.INVALID_CRITERIA); // We want both a minimum of 5 trades and enough trades to span at least 4 hours int minimumCount = 5; - int maximumCount = 10; + int maximumCount = maxtrades != null ? maxtrades : 10; long minimumPeriod = 4 * 60 * 60 * 1000L; // ms Boolean isFinished = Boolean.TRUE; From 1ba64d97459857558e255dd1b29514d60eec8e14 Mon Sep 17 00:00:00 2001 From: CalDescent Date: Sun, 16 May 2021 10:00:28 +0100 Subject: [PATCH 09/12] Bumped bitcoinj version from 0.15.6 to 0.15.10 --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 9e5d8aff..526ed35d 100644 --- a/pom.xml +++ b/pom.xml @@ -8,7 +8,7 @@ true bf9fb80 - 0.15.6 + 0.15.10 1.64 ${maven.build.timestamp} 1.3.8 From 3bedba71d5eac36255041b03df40fae5852ef426 Mon Sep 17 00:00:00 2001 From: CalDescent Date: Sun, 16 May 2021 10:36:41 +0100 Subject: [PATCH 10/12] Reduced frequency and level of some synchronizer logs. --- .../java/org/qortal/controller/Controller.java | 2 +- .../java/org/qortal/controller/Synchronizer.java | 16 +++++++++++----- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/qortal/controller/Controller.java b/src/main/java/org/qortal/controller/Controller.java index 9123a130..9d80dba7 100644 --- a/src/main/java/org/qortal/controller/Controller.java +++ b/src/main/java/org/qortal/controller/Controller.java @@ -677,7 +677,7 @@ public class Controller extends Thread { peers.removeIf(hasInferiorChainTip); final int peersRemoved = peersBeforeComparison - peers.size(); - if (peersRemoved > 0) + if (peersRemoved > 0 && peers.size() > 0) LOGGER.info(String.format("Ignoring %d peers on inferior chains. Peers remaining: %d", peersRemoved, peers.size())); if (peers.isEmpty()) diff --git a/src/main/java/org/qortal/controller/Synchronizer.java b/src/main/java/org/qortal/controller/Synchronizer.java index 8b566fdc..8876549f 100644 --- a/src/main/java/org/qortal/controller/Synchronizer.java +++ b/src/main/java/org/qortal/controller/Synchronizer.java @@ -111,6 +111,7 @@ public class Synchronizer { LOGGER.debug(String.format("Searching for common blocks with %d peers...", peers.size())); final long startTime = System.currentTimeMillis(); int commonBlocksFound = 0; + boolean wereNewRequestsMade = false; for (Peer peer : peers) { // Are we shutting down? @@ -131,10 +132,15 @@ public class Synchronizer { Synchronizer.getInstance().findCommonBlockWithPeer(peer, repository); if (peer.getCommonBlockData() != null) commonBlocksFound++; + + // This round wasn't served entirely from the cache, so we may want to log the results + wereNewRequestsMade = true; } - final long totalTimeTaken = System.currentTimeMillis() - startTime; - LOGGER.info(String.format("Finished searching for common blocks with %d peer%s. Found: %d. Total time taken: %d ms", peers.size(), (peers.size() != 1 ? "s" : ""), commonBlocksFound, totalTimeTaken)); + if (wereNewRequestsMade) { + final long totalTimeTaken = System.currentTimeMillis() - startTime; + LOGGER.info(String.format("Finished searching for common blocks with %d peer%s. Found: %d. Total time taken: %d ms", peers.size(), (peers.size() != 1 ? "s" : ""), commonBlocksFound, totalTimeTaken)); + } return SynchronizationResult.OK; } finally { @@ -294,7 +300,7 @@ public class Synchronizer { if (peer.canUseCachedCommonBlockData()) { if (peer.getCommonBlockData().getBlockSummariesAfterCommonBlock() != null) { if (peer.getCommonBlockData().getBlockSummariesAfterCommonBlock().size() == summariesRequired) { - LOGGER.debug(String.format("Using cached block summaries for peer %s", peer)); + LOGGER.trace(String.format("Using cached block summaries for peer %s", peer)); useCachedSummaries = true; } } @@ -434,14 +440,14 @@ public class Synchronizer { for (Peer peer : peers) { if (peer.getCommonBlockData() != null && peer.getCommonBlockData().getCommonBlockSummary() != null) { - LOGGER.debug(String.format("Peer %s has common block %.8s", peer, Base58.encode(peer.getCommonBlockData().getCommonBlockSummary().getSignature()))); + LOGGER.trace(String.format("Peer %s has common block %.8s", peer, Base58.encode(peer.getCommonBlockData().getCommonBlockSummary().getSignature()))); BlockSummaryData commonBlockSummary = peer.getCommonBlockData().getCommonBlockSummary(); if (!commonBlocks.contains(commonBlockSummary)) commonBlocks.add(commonBlockSummary); } else { - LOGGER.debug(String.format("Peer %s has no common block data. Skipping...", peer)); + LOGGER.trace(String.format("Peer %s has no common block data. Skipping...", peer)); } } From 65c26f17df084848391adb578fc93ca068f347be Mon Sep 17 00:00:00 2001 From: CalDescent Date: Sun, 16 May 2021 10:45:40 +0100 Subject: [PATCH 11/12] Reduced "Error while trying to find common block with peer" log from INFO to DEBUG when determining which peer to sync with. When performing the actual synchronization, use INFO logging as this is a more serious error. --- .../java/org/qortal/api/resource/PeersResource.java | 2 +- .../java/org/qortal/controller/Synchronizer.java | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/qortal/api/resource/PeersResource.java b/src/main/java/org/qortal/api/resource/PeersResource.java index 70f0e3e9..244a1569 100644 --- a/src/main/java/org/qortal/api/resource/PeersResource.java +++ b/src/main/java/org/qortal/api/resource/PeersResource.java @@ -321,7 +321,7 @@ public class PeersResource { boolean force = true; List peerBlockSummaries = new ArrayList<>(); - SynchronizationResult findCommonBlockResult = Synchronizer.getInstance().fetchSummariesFromCommonBlock(repository, targetPeer, ourInitialHeight, force, peerBlockSummaries); + SynchronizationResult findCommonBlockResult = Synchronizer.getInstance().fetchSummariesFromCommonBlock(repository, targetPeer, ourInitialHeight, force, peerBlockSummaries, true); if (findCommonBlockResult != SynchronizationResult.OK) return null; diff --git a/src/main/java/org/qortal/controller/Synchronizer.java b/src/main/java/org/qortal/controller/Synchronizer.java index 8876549f..f9a2c9c8 100644 --- a/src/main/java/org/qortal/controller/Synchronizer.java +++ b/src/main/java/org/qortal/controller/Synchronizer.java @@ -178,7 +178,7 @@ public class Synchronizer { ourInitialHeight, Base58.encode(ourLastBlockSignature), ourLatestBlockData.getTimestamp())); List peerBlockSummaries = new ArrayList<>(); - SynchronizationResult findCommonBlockResult = fetchSummariesFromCommonBlock(repository, peer, ourInitialHeight, false, peerBlockSummaries); + SynchronizationResult findCommonBlockResult = fetchSummariesFromCommonBlock(repository, peer, ourInitialHeight, false, peerBlockSummaries, false); if (findCommonBlockResult != SynchronizationResult.OK) { // Logging performed by fetchSummariesFromCommonBlock() above peer.setCommonBlockData(null); @@ -508,7 +508,7 @@ public class Synchronizer { ourInitialHeight, Base58.encode(ourLastBlockSignature), ourLatestBlockData.getTimestamp())); List peerBlockSummaries = new ArrayList<>(); - SynchronizationResult findCommonBlockResult = fetchSummariesFromCommonBlock(repository, peer, ourInitialHeight, force, peerBlockSummaries); + SynchronizationResult findCommonBlockResult = fetchSummariesFromCommonBlock(repository, peer, ourInitialHeight, force, peerBlockSummaries, true); if (findCommonBlockResult != SynchronizationResult.OK) { // Logging performed by fetchSummariesFromCommonBlock() above // Clear our common block cache for this peer @@ -590,7 +590,7 @@ public class Synchronizer { * @throws DataException * @throws InterruptedException */ - public SynchronizationResult fetchSummariesFromCommonBlock(Repository repository, Peer peer, int ourHeight, boolean force, List blockSummariesFromCommon) throws DataException, InterruptedException { + public SynchronizationResult fetchSummariesFromCommonBlock(Repository repository, Peer peer, int ourHeight, boolean force, List blockSummariesFromCommon, boolean infoLogWhenNotFound) throws DataException, InterruptedException { // Start by asking for a few recent block hashes as this will cover a majority of reorgs // Failing that, back off exponentially int step = INITIAL_BLOCK_STEP; @@ -619,8 +619,12 @@ public class Synchronizer { blockSummariesBatch = this.getBlockSummaries(peer, testSignature, step); if (blockSummariesBatch == null) { + if (infoLogWhenNotFound) + LOGGER.info(String.format("Error while trying to find common block with peer %s", peer)); + else + LOGGER.debug(String.format("Error while trying to find common block with peer %s", peer)); + // No response - give up this time - LOGGER.info(String.format("Error while trying to find common block with peer %s", peer)); return SynchronizationResult.NO_REPLY; } From 13dcf7f72abb81e15cfcbca884e298de4970ddce Mon Sep 17 00:00:00 2001 From: CalDescent Date: Sun, 16 May 2021 11:03:11 +0100 Subject: [PATCH 12/12] Added/updated some comments relating to a possible future optimization. --- src/main/java/org/qortal/controller/Synchronizer.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/qortal/controller/Synchronizer.java b/src/main/java/org/qortal/controller/Synchronizer.java index f9a2c9c8..62064d96 100644 --- a/src/main/java/org/qortal/controller/Synchronizer.java +++ b/src/main/java/org/qortal/controller/Synchronizer.java @@ -399,8 +399,8 @@ public class Synchronizer { peers.remove(peer); } else { - // Our chain is inferior - LOGGER.debug(String.format("Peer %s is on a better chain to us. We will compare the other peers sharing this common block against each other, and drop all peers sharing higher common blocks.", peer)); + // Our chain is inferior or equal + LOGGER.debug(String.format("Peer %s is on an equal or better chain to us. We will compare the other peers sharing this common block against each other, and drop all peers sharing higher common blocks.", peer)); dropPeersAfterCommonBlockHeight = commonBlockSummary.getHeight(); superiorPeersForComparison.add(peer); } @@ -422,6 +422,9 @@ public class Synchronizer { peers.remove(peer); } } + // FUTURE: we may want to prefer peers with additional blocks, and compare the additional blocks against each other. + // This would fast track us to the best candidate for the latest block. + // Right now, peers with the exact same chain as us are treated equally to those with an additional block. } }