Potential HSQLDB deadlock fix

After opening repository connection with RepositoryManager.getRepostory(),
any 'read' from repository (e.g. SELECT) starts the transaction
even though HSQLDB documentation states there are no shared/read locks
in MVCC concurrency model.

The work-around for this is to 'reset' HSQLDB's in-transaction flag
by performing a ROLLBACK (Repository.discardChanges) immediately
after acquiring the blockchain lock (which is used to ringfence
changes that might collide like these).

Also adding an extra check to prevent payments to nonexistent AT
addresses as it touches Transaction.

src/main/java/org/qora/account/Account.java

@@ -165,7 +165,8 @@ public class Account {
 	/**
 	 * Fetch last reference for account, considering unconfirmed transactions only, or return null.
 	 * <p>
-	 * NOTE: a repository savepoint may be used during execution.
+	 * NOTE: calls Transaction.getUnconfirmedTransactions which discards uncommitted
+	 * repository changes.
 	 * 
 	 * @return byte[] reference, or null if no unconfirmed transactions for this account.
 	 * @throws DataException

src/main/java/org/qora/api/resource/AddressesResource.java

@@ -78,8 +78,12 @@ public class AddressesResource {
 			else {
 				// Unconfirmed transactions could update lastReference
 				Account account = new Account(repository, address);
+
+				// Use last reference based on unconfirmed transactions if possible
 				byte[] unconfirmedLastReference = account.getUnconfirmedLastReference();
+
 				if (unconfirmedLastReference != null)
+					// There are unconfirmed transactions so modify returned data
 					accountData.setReference(unconfirmedLastReference);
 			}
 
@@ -112,8 +116,12 @@ public class AddressesResource {
 
 		try (final Repository repository = RepositoryManager.getRepository()) {
 			Account account = new Account(repository, address);
+
+			// Use last reference based on unconfirmed transactions if possible
 			lastReference = account.getUnconfirmedLastReference();
+
 			if (lastReference == null)
+				// No unconfirmed transactions so fallback to using one save in account data
 				lastReference = account.getLastReference();
 		} catch (ApiException e) {
 			throw e;

src/main/java/org/qora/block/BlockGenerator.java

@@ -96,6 +96,9 @@ public class BlockGenerator extends Thread {
 				ReentrantLock blockchainLock = Controller.getInstance().getBlockchainLock();
 				if (blockchainLock.tryLock())
 					generation: try {
+						// Clear repository's "in transaction" state so we don't cause a repository deadlock
+						repository.discardChanges();
+
 						List<Block> goodBlocks = new ArrayList<>();
 
 						for (Block testBlock : newBlocks) {
@@ -118,7 +121,7 @@ public class BlockGenerator extends Thread {
 						int winningIndex = new Random().nextInt(goodBlocks.size());
 						Block newBlock = goodBlocks.get(winningIndex);
 
-						// Delete invalid transactions
+						// Delete invalid transactions. NOTE: discards repository changes on entry, saves changes on exit.
 						deleteInvalidTransactions(repository);
 
 						// Add unconfirmed transactions
@@ -167,6 +170,17 @@ public class BlockGenerator extends Thread {
 		}
 	}
 
+	/**
+	 * Deletes invalid, unconfirmed transactions from repository.
+	 * <p>
+	 * NOTE: calls Transaction.getInvalidTransactions which discards uncommitted
+	 * repository changes.
+	 * <p>
+	 * Also commits the deletion of invalid transactions to the repository.
+	 *  
+	 * @param repository
+	 * @throws DataException
+	 */
 	private static void deleteInvalidTransactions(Repository repository) throws DataException {
 		List<TransactionData> invalidTransactions = Transaction.getInvalidTransactions(repository);
 
@@ -175,9 +189,20 @@ public class BlockGenerator extends Thread {
 			LOGGER.trace(String.format("Deleting invalid, unconfirmed transaction %s", Base58.encode(invalidTransactionData.getSignature())));
 			repository.getTransactionRepository().delete(invalidTransactionData);
 		}
+
 		repository.saveChanges();
 	}
 
+	/**
+	 * Adds unconfirmed transactions to passed block.
+	 * <p>
+	 * NOTE: calls Transaction.getUnconfirmedTransactions which discards uncommitted
+	 * repository changes.
+	 * 
+	 * @param repository
+	 * @param newBlock
+	 * @throws DataException
+	 */
 	private static void addUnconfirmedTransactions(Repository repository, Block newBlock) throws DataException {
 		// Grab all valid unconfirmed transactions (already sorted)
 		List<TransactionData> unconfirmedTransactions = Transaction.getUnconfirmedTransactions(repository);

src/main/java/org/qora/crypto/Crypto.java

@@ -90,6 +90,17 @@ public class Crypto {
 	}
 
 	public static boolean isValidAddress(String address) {
+		return isValidTypedAddress(address, ADDRESS_VERSION, AT_ADDRESS_VERSION);
+	}
+
+	public static boolean isValidAtAddress(String address) {
+		return isValidTypedAddress(address, AT_ADDRESS_VERSION);
+	}
+
+	private static boolean isValidTypedAddress(String address, byte...addressVersions) {
+		if (addressVersions == null || addressVersions.length == 0)
+			return false;
+
 		byte[] addressBytes;
 
 		try {
@@ -104,18 +115,16 @@ public class Crypto {
 			return false;
 
 		// Check by address type
-		switch (addressBytes[0]) {
-			case ADDRESS_VERSION:
-			case AT_ADDRESS_VERSION:
+		for (byte addressVersion : addressVersions)
+			if (addressBytes[0] == addressVersion) {
 				byte[] addressWithoutChecksum = Arrays.copyOf(addressBytes, addressBytes.length - 4);
 				byte[] passedChecksum = Arrays.copyOfRange(addressBytes, addressBytes.length - 4, addressBytes.length);
 
 				byte[] generatedChecksum = Arrays.copyOf(doubleDigest(addressWithoutChecksum), 4);
 				return Arrays.equals(passedChecksum, generatedChecksum);
-
-			default:
-				return false;
 			}
+
+		return false;
 	}
 
 }

src/main/java/org/qora/payment/Payment.java

@@ -60,10 +60,19 @@ public class Payment {
 			if (!Crypto.isValidAddress(paymentData.getRecipient()))
 				return ValidationResult.INVALID_ADDRESS;
 
-			// Do not allow payments to finished/dead ATs
-			ATData atData = this.repository.getATRepository().fromATAddress(paymentData.getRecipient());
+			boolean recipientIsAT = Crypto.isValidAtAddress(paymentData.getRecipient());
+			ATData atData = null;
+
+			// Do not allow payments to finished/dead/nonexistent ATs
+			if (recipientIsAT) {
+				atData = this.repository.getATRepository().fromATAddress(paymentData.getRecipient());
+
+				if (atData == null)
+					return ValidationResult.AT_UNKNOWN;
+
 				if (atData != null && atData.getIsFinished())
 					return ValidationResult.AT_IS_FINISHED;
+			}
 
 			AssetData assetData = assetRepository.fromAssetId(paymentData.getAssetId());
 			// Check asset even exists

src/main/java/org/qora/repository/hsqldb/HSQLDBRepository.java

@@ -242,7 +242,7 @@ public class HSQLDBRepository implements Repository {
 		if (this.sqlStatements == null)
 			return;
 
-		LOGGER.info("HSQLDB SQL statements leading up to this were:");
+		LOGGER.info(String.format("HSQLDB SQL statements (session %d) leading up to this were:", this.sessionId));
 
 		for (String sql : this.sqlStatements)
 			LOGGER.info(sql);
@@ -449,7 +449,7 @@ public class HSQLDBRepository implements Repository {
 
 	/** Logs other HSQLDB sessions then re-throws passed exception */
 	public SQLException examineException(SQLException e) throws SQLException {
-		LOGGER.error("SQL error: " + e.getMessage(), e);
+		LOGGER.error(String.format("HSQLDB error (session %d): %s", this.sessionId, e.getMessage()), e);
 
 		logStatements();
 

src/main/java/org/qora/transaction/Transaction.java

@@ -203,6 +203,7 @@ public abstract class Transaction {
 		INVALID_FORGE_SHARE(77),
 		PUBLIC_KEY_UNKNOWN(78),
 		INVALID_PUBLIC_KEY(79),
+		AT_UNKNOWN(80),
 		NOT_YET_RELEASED(1000);
 
 		public final int value;
@@ -472,12 +473,8 @@ public abstract class Transaction {
 	/**
 	 * Returns whether transaction can be added to unconfirmed transactions.
 	 * <p>
-	 * NOTE: temporarily updates creator's lastReference to that from
-	 * unconfirmed transactions, and hence uses a repository savepoint.
-	 * <p>
-	 * This is not done normally because we don't want unconfirmed transactions affecting validity of transactions already included in a block.
-	 * <p>
-	 * Also temporarily acquires blockchain lock.
+	 * NOTE: temporarily updates accounts' lastReference to check validity.<br>
+	 * To do this, blockchain lock is obtained and pending repository changes are discarded.
 	 * 
 	 * @return true if transaction can be added to unconfirmed transactions, false otherwise
 	 * @throws DataException
@@ -500,12 +497,14 @@ public abstract class Transaction {
 		/*
 		 * We have to grab the blockchain lock because we're updating
 		 * when we fake the creator's last reference,
-		 * even though we throw away the update when we rollback the
-		 * savepoint.
+		 * even though we throw away the update when we discard changes.
 		 */
 		ReentrantLock blockchainLock = Controller.getInstance().getBlockchainLock();
 		blockchainLock.lock();
-		repository.setSavepoint();
+		try {
+			// Clear repository's "in transaction" state so we don't cause a repository deadlock
+			repository.discardChanges();
+
 			try {
 				PublicKeyAccount creator = this.getCreator();
 				if (creator == null)
@@ -527,7 +526,10 @@ public abstract class Transaction {
 
 				return result;
 			} finally {
-			repository.rollbackToSavepoint();
+				repository.discardChanges();
+			}
+		} finally {
+			// In separate finally block just in case rollback throws
 			blockchainLock.unlock();
 		}
 	}
@@ -576,12 +578,10 @@ public abstract class Transaction {
 	/**
 	 * Returns sorted, unconfirmed transactions, excluding invalid.
 	 * <p>
-	 * NOTE: temporarily updates accounts' lastReference to that from
-	 * unconfirmed transactions, hence uses a repository savepoint.
-	 * <p>
-	 * Also temporarily acquires blockchain lock.
+	 * NOTE: temporarily updates accounts' lastReference to check validity.<br>
+	 * To do this, blockchain lock is obtained and pending repository changes are discarded.
 	 * 
-	 * @return sorted unconfirmed transactions
+	 * @return sorted, unconfirmed transactions
 	 * @throws DataException
 	 */
 	public static List<TransactionData> getUnconfirmedTransactions(Repository repository) throws DataException {
@@ -599,7 +599,10 @@ public abstract class Transaction {
 		 */
 		ReentrantLock blockchainLock = Controller.getInstance().getBlockchainLock();
 		blockchainLock.lock();
-		repository.setSavepoint();
+		try {
+			// Clear repository's "in transaction" state so we don't cause a repository deadlock
+			repository.discardChanges();
+
 			try {
 				for (int i = 0; i < unconfirmedTransactions.size(); ++i) {
 					TransactionData transactionData = unconfirmedTransactions.get(i);
@@ -612,7 +615,10 @@ public abstract class Transaction {
 				}
 			} finally {
 				// Throw away temporary updates to account lastReference
-			repository.rollbackToSavepoint();
+				repository.discardChanges();
+			}
+		} finally {
+			// In separate finally block just in case rollback throws
 			blockchainLock.unlock();
 		}
 
@@ -622,12 +628,10 @@ public abstract class Transaction {
 	/**
 	 * Returns invalid, unconfirmed transactions.
 	 * <p>
-	 * NOTE: temporarily updates accounts' lastReference to that from
-	 * unconfirmed transactions, hence uses a repository savepoint.
-	 * <p>
-	 * Also temporarily acquires blockchain lock.
+	 * NOTE: temporarily updates accounts' lastReference to check validity.<br>
+	 * To do this, blockchain lock is obtained and pending repository changes are discarded.
 	 * 
-	 * @return sorted unconfirmed transactions
+	 * @return sorted, invalid, unconfirmed transactions
 	 * @throws DataException
 	 */
 	public static List<TransactionData> getInvalidTransactions(Repository repository) throws DataException {
@@ -646,7 +650,10 @@ public abstract class Transaction {
 		 */
 		ReentrantLock blockchainLock = Controller.getInstance().getBlockchainLock();
 		blockchainLock.lock();
-		repository.setSavepoint();
+		try {
+			// Clear repository's "in transaction" state so we don't cause a repository deadlock
+			repository.discardChanges();
+
 			try {
 				for (int i = 0; i < unconfirmedTransactions.size(); ++i) {
 					TransactionData transactionData = unconfirmedTransactions.get(i);
@@ -661,7 +668,10 @@ public abstract class Transaction {
 				}
 			} finally {
 				// Throw away temporary updates to account lastReference
-			repository.rollbackToSavepoint();
+				repository.discardChanges();
+			}
+		} finally {
+			// In separate finally block just in case rollback throws
 			blockchainLock.unlock();
 		}
 
@@ -709,6 +719,7 @@ public abstract class Transaction {
 		// These updates should be discarded by some caller further up stack
 		PublicKeyAccount creator = new PublicKeyAccount(repository, transactionData.getCreatorPublicKey());
 		creator.setLastReference(transactionData.getSignature());
+
 		return true;
 	}
 

src/test/java/org/qora/test/RepositoryTests.java

@@ -1,6 +1,8 @@
 package org.qora.test;
 
 import org.junit.Test;
+import org.qora.account.Account;
+import org.qora.asset.Asset;
 import org.qora.repository.DataException;
 import org.qora.repository.Repository;
 import org.qora.repository.RepositoryManager;
@@ -8,6 +10,8 @@ import org.qora.test.common.Common;
 
 import static org.junit.Assert.*;
 
+import java.math.BigDecimal;
+
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 
@@ -55,4 +59,31 @@ public class RepositoryTests extends Common {
 		}
 	}
 
+	@Test
+	public void testDeadlock() throws DataException {
+		Common.useDefaultSettings();
+
+		// Open connection 1
+		try (Repository repository1 = RepositoryManager.getRepository()) {
+
+			// Do a database 'read'
+			Account account1 = Common.getTestAccount(repository1, "alice");
+			account1.getLastReference();
+
+			// Open connection 2
+			try (Repository repository2 = RepositoryManager.getRepository()) {
+				// Update account in 2
+				Account account2 = Common.getTestAccount(repository2, "alice");
+				account2.setConfirmedBalance(Asset.QORA, BigDecimal.valueOf(1234L));
+				repository2.saveChanges();
+			}
+
+			repository1.discardChanges();
+
+			// Update account in 1
+			account1.setConfirmedBalance(Asset.QORA, BigDecimal.valueOf(5678L));
+			repository1.saveChanges();
+		}
+	}
+
 }