From fef16e7620714d0700653ebe6306f134445c22c9 Mon Sep 17 00:00:00 2001 From: catbref Date: Wed, 30 Oct 2019 10:23:59 +0000 Subject: [PATCH] Self-reward-share tests and fixes. Added Transaction.isFeeValid() to allow transaction subclasses to override and allow zero fees, etc. Added tests to cover self-reward-shares, including zero fee scenario. Set 'dilbert' test account to level 8 in test genesis block. Removed leftover mention of "previous_level" from HSQLDBAccountLevelTransactions, and AccountLevelTransactionData. (Previous level makes no sense as ACCOUNT_LEVEL transactions are genesis-block only). Fixed some incorrect uses of PrivateKeyAccount.getSharedSecret() to PrivateKeyAccount.getRewardSharePrivateKey() in tests. --- .../AccountLevelTransactionData.java | 20 +----- ...QLDBAccountLevelTransactionRepository.java | 8 +-- .../transaction/RewardShareTransaction.java | 71 ++++++++++++------- .../org/qora/transaction/Transaction.java | 15 +++- .../org/qora/test/common/AccountUtils.java | 8 +-- .../qora/test/common/TransactionUtils.java | 2 +- .../RewardShareTests.java | 50 ++++++++++++- .../{forging => minting}/RewardTests.java | 18 ++--- src/test/resources/test-chain-v2.json | 1 + 9 files changed, 127 insertions(+), 66 deletions(-) rename src/test/java/org/qora/test/{forging => minting}/RewardShareTests.java (71%) rename src/test/java/org/qora/test/{forging => minting}/RewardTests.java (83%) diff --git a/src/main/java/org/qora/data/transaction/AccountLevelTransactionData.java b/src/main/java/org/qora/data/transaction/AccountLevelTransactionData.java index 71f3a0c5..f465d7d0 100644 --- a/src/main/java/org/qora/data/transaction/AccountLevelTransactionData.java +++ b/src/main/java/org/qora/data/transaction/AccountLevelTransactionData.java @@ -21,7 +21,6 @@ public class AccountLevelTransactionData extends TransactionData { private String target; private int level; - private Integer previousLevel; // Constructors @@ -40,20 +39,13 @@ public class AccountLevelTransactionData extends TransactionData { this.creatorPublicKey = GenesisAccount.PUBLIC_KEY; } - /** From repository */ + /** From repository, network/API */ public AccountLevelTransactionData(BaseTransactionData baseTransactionData, - String target, int level, Integer previousLevel) { + String target, int level) { super(TransactionType.ACCOUNT_LEVEL, baseTransactionData); this.target = target; this.level = level; - this.previousLevel = previousLevel; - } - - /** From network/API */ - public AccountLevelTransactionData(BaseTransactionData baseTransactionData, - String target, int level) { - this(baseTransactionData, target, level, null); } // Getters / setters @@ -66,14 +58,6 @@ public class AccountLevelTransactionData extends TransactionData { return this.level; } - public Integer getPreviousLevel() { - return this.previousLevel; - } - - public void setPreviousLevel(Integer previousLevel) { - this.previousLevel = previousLevel; - } - // Re-expose to JAXB @XmlElement(name = "creatorPublicKey") diff --git a/src/main/java/org/qora/repository/hsqldb/transaction/HSQLDBAccountLevelTransactionRepository.java b/src/main/java/org/qora/repository/hsqldb/transaction/HSQLDBAccountLevelTransactionRepository.java index 682c01c7..eb156719 100644 --- a/src/main/java/org/qora/repository/hsqldb/transaction/HSQLDBAccountLevelTransactionRepository.java +++ b/src/main/java/org/qora/repository/hsqldb/transaction/HSQLDBAccountLevelTransactionRepository.java @@ -17,7 +17,7 @@ public class HSQLDBAccountLevelTransactionRepository extends HSQLDBTransactionRe } TransactionData fromBase(BaseTransactionData baseTransactionData) throws DataException { - String sql = "SELECT target, level, previous_level FROM AccountLevelTransactions WHERE signature = ?"; + String sql = "SELECT target, level FROM AccountLevelTransactions WHERE signature = ?"; try (ResultSet resultSet = this.repository.checkedExecute(sql, baseTransactionData.getSignature())) { if (resultSet == null) @@ -26,11 +26,7 @@ public class HSQLDBAccountLevelTransactionRepository extends HSQLDBTransactionRe String target = resultSet.getString(1); int level = resultSet.getInt(2); - Integer previousLevel = resultSet.getInt(3); - if (previousLevel == 0 && resultSet.wasNull()) - previousLevel = null; - - return new AccountLevelTransactionData(baseTransactionData, target, level, previousLevel); + return new AccountLevelTransactionData(baseTransactionData, target, level); } catch (SQLException e) { throw new DataException("Unable to fetch account level transaction from repository", e); } diff --git a/src/main/java/org/qora/transaction/RewardShareTransaction.java b/src/main/java/org/qora/transaction/RewardShareTransaction.java index 4549c367..800acc06 100644 --- a/src/main/java/org/qora/transaction/RewardShareTransaction.java +++ b/src/main/java/org/qora/transaction/RewardShareTransaction.java @@ -61,6 +61,22 @@ public class RewardShareTransaction extends Transaction { return amount; } + private RewardShareData getExistingRewardShare() throws DataException { + // Look up any existing reward-share (using transaction's reward-share public key) + RewardShareData existingRewardShareData = this.repository.getAccountRepository().getRewardShare(this.rewardShareTransactionData.getRewardSharePublicKey()); + if (existingRewardShareData == null) + // No luck, try looking up existing reward-share using minting & recipient account info + existingRewardShareData = this.repository.getAccountRepository().getRewardShare(this.rewardShareTransactionData.getMinterPublicKey(), this.rewardShareTransactionData.getRecipient()); + + return existingRewardShareData; + } + + private boolean doesRewardShareMatch(RewardShareData rewardShareData) { + return rewardShareData.getRecipient().equals(this.rewardShareTransactionData.getRecipient()) + && Arrays.equals(rewardShareData.getMinterPublicKey(), this.rewardShareTransactionData.getMinterPublicKey()) + && Arrays.equals(rewardShareData.getRewardSharePublicKey(), this.rewardShareTransactionData.getRewardSharePublicKey()); + } + // Navigation public PublicKeyAccount getMintingAccount() { @@ -75,6 +91,25 @@ public class RewardShareTransaction extends Transaction { private static final BigDecimal MAX_SHARE = BigDecimal.valueOf(100).setScale(2); + @Override + public ValidationResult isFeeValid() throws DataException { + // Look up any existing reward-share (using transaction's reward-share public key) + RewardShareData existingRewardShareData = this.getExistingRewardShare(); + // If we have an existing reward-share then minter/recipient/reward-share-public-key should all match. + // This is to prevent malicious actors using multiple (fake) reward-share public keys for the same minter/recipient combo, + // or reusing the same reward-share public key for a different minter/recipient pair. + if (existingRewardShareData != null && !this.doesRewardShareMatch(existingRewardShareData)) + return ValidationResult.INVALID_PUBLIC_KEY; + + final boolean isRecipientAlsoMinter = getCreator().getAddress().equals(this.rewardShareTransactionData.getRecipient()); + + // Fee can be zero if setting up new self-share + if (isRecipientAlsoMinter && existingRewardShareData == null && this.transactionData.getFee().compareTo(BigDecimal.ZERO) >= 0) + return ValidationResult.OK; + + return super.isFeeValid(); + } + @Override public ValidationResult isValid() throws DataException { // Check reward share given to recipient @@ -102,34 +137,25 @@ public class RewardShareTransaction extends Transaction { return ValidationResult.ACCOUNT_CANNOT_REWARD_SHARE; // Look up any existing reward-share (using transaction's reward-share public key) - RewardShareData rewardShareData = this.repository.getAccountRepository().getRewardShare(this.rewardShareTransactionData.getRewardSharePublicKey()); - if (rewardShareData != null) { - // If reward-share public key already exists in repository, then it must be for the same minter-recipient combo - if (!rewardShareData.getRecipient().equals(recipient.getAddress()) || !Arrays.equals(rewardShareData.getMinterPublicKey(), creator.getPublicKey())) - return ValidationResult.INVALID_PUBLIC_KEY; - - } else { - // No luck, try looking up existing reward-share using minting & recipient account info - rewardShareData = this.repository.getAccountRepository().getRewardShare(creator.getPublicKey(), recipient.getAddress()); - - if (rewardShareData != null) - // If reward-share between minter & recipient already exists in repository, then it must be have the same public key - if (!Arrays.equals(rewardShareData.getRewardSharePublicKey(), this.rewardShareTransactionData.getRewardSharePublicKey())) - return ValidationResult.INVALID_PUBLIC_KEY; - } + RewardShareData existingRewardShareData = this.getExistingRewardShare(); + // If we have an existing reward-share then minter/recipient/reward-share-public-key should all match. + // This is to prevent malicious actors using multiple (fake) reward-share public keys for the same minter/recipient combo, + // or reusing the same reward-share public key for a different minter/recipient pair. + if (existingRewardShareData != null && !this.doesRewardShareMatch(existingRewardShareData)) + return ValidationResult.INVALID_PUBLIC_KEY; final boolean isSharePercentZero = this.rewardShareTransactionData.getSharePercent().compareTo(BigDecimal.ZERO) == 0; - if (rewardShareData == null) { + if (existingRewardShareData == null) { // This is a new reward-share - // No point starting a new reward-share with 0% share (i.e. delete relationship) + // No point starting a new reward-share with 0% share (i.e. delete reward-share) if (isSharePercentZero) return ValidationResult.INVALID_REWARD_SHARE_PERCENT; // Check the minting account hasn't reach maximum number of reward-shares - int relationshipCount = this.repository.getAccountRepository().countRewardShares(creator.getPublicKey()); - if (relationshipCount >= BlockChain.getInstance().getMaxRewardSharesPerMintingAccount()) + int rewardShareCount = this.repository.getAccountRepository().countRewardShares(creator.getPublicKey()); + if (rewardShareCount >= BlockChain.getInstance().getMaxRewardSharesPerMintingAccount()) return ValidationResult.MAXIMUM_REWARD_SHARES; } else { // This transaction intends to modify/terminate an existing reward-share @@ -140,15 +166,10 @@ public class RewardShareTransaction extends Transaction { } // Fee checking needed if not setting up new self-share - if (!(isRecipientAlsoMinter && rewardShareData == null)) { - // Check fee is positive - if (rewardShareTransactionData.getFee().compareTo(BigDecimal.ZERO) <= 0) - return ValidationResult.NEGATIVE_FEE; - + if (!(isRecipientAlsoMinter && existingRewardShareData == null)) // Check creator has enough funds if (creator.getConfirmedBalance(Asset.QORT).compareTo(rewardShareTransactionData.getFee()) < 0) return ValidationResult.NO_BALANCE; - } return ValidationResult.OK; } diff --git a/src/main/java/org/qora/transaction/Transaction.java b/src/main/java/org/qora/transaction/Transaction.java index 5eb027f5..267f0cbd 100644 --- a/src/main/java/org/qora/transaction/Transaction.java +++ b/src/main/java/org/qora/transaction/Transaction.java @@ -314,6 +314,7 @@ public abstract class Transaction { return Transaction.getDeadline(transactionData); } + /** Returns whether transaction's fee is at least minimum unit fee as specified in blockchain config. */ public boolean hasMinimumFee() { return this.transactionData.getFee().compareTo(BlockChain.getInstance().getUnitFee()) >= 0; } @@ -326,6 +327,7 @@ public abstract class Transaction { } } + /** Returns whether transaction's fee is at least amount needed to cover byte-length of transaction. */ public boolean hasMinimumFeePerByte() { return this.feePerByte().compareTo(BlockChain.getInstance().getMinFeePerByte()) >= 0; } @@ -538,8 +540,9 @@ public abstract class Transaction { return ValidationResult.TIMESTAMP_TOO_NEW; // Check fee is sufficient - if (!hasMinimumFee() || !hasMinimumFeePerByte()) - return ValidationResult.INSUFFICIENT_FEE; + ValidationResult feeValidationResult = isFeeValid(); + if (feeValidationResult != ValidationResult.OK) + return feeValidationResult; /* * We have to grab the blockchain lock because we're updating @@ -591,6 +594,14 @@ public abstract class Transaction { } } + /** Returns whether transaction's fee is valid. Might be overriden in transaction subclasses. */ + protected ValidationResult isFeeValid() throws DataException { + if (!hasMinimumFee() || !hasMinimumFeePerByte()) + return ValidationResult.INSUFFICIENT_FEE; + + return ValidationResult.OK; + } + private boolean isValidTxGroupId() throws DataException { int txGroupId = this.transactionData.getTxGroupId(); diff --git a/src/test/java/org/qora/test/common/AccountUtils.java b/src/test/java/org/qora/test/common/AccountUtils.java index 3e7a53e3..d8ac4e87 100644 --- a/src/test/java/org/qora/test/common/AccountUtils.java +++ b/src/test/java/org/qora/test/common/AccountUtils.java @@ -38,11 +38,11 @@ public class AccountUtils { byte[] reference = mintingAccount.getLastReference(); long timestamp = repository.getTransactionRepository().fromSignature(reference).getTimestamp() + 1; - byte[] rewardSharePrivateKey = mintingAccount.getSharedSecret(recipientAccount.getPublicKey()); - PrivateKeyAccount rewardShareAccount = new PrivateKeyAccount(null, rewardSharePrivateKey); + byte[] rewardSharePrivateKey = mintingAccount.getRewardSharePrivateKey(recipientAccount.getPublicKey()); + byte[] rewardSharePublicKey = PrivateKeyAccount.toPublicKey(rewardSharePrivateKey); BaseTransactionData baseTransactionData = new BaseTransactionData(timestamp, txGroupId, reference, mintingAccount.getPublicKey(), fee, null); - TransactionData transactionData = new RewardShareTransactionData(baseTransactionData, recipientAccount.getAddress(), rewardShareAccount.getPublicKey(), sharePercent); + TransactionData transactionData = new RewardShareTransactionData(baseTransactionData, recipientAccount.getAddress(), rewardSharePublicKey, sharePercent); return transactionData; } @@ -54,7 +54,7 @@ public class AccountUtils { TransactionUtils.signAndMint(repository, transactionData, rewardShareAccount); PrivateKeyAccount recipientAccount = Common.getTestAccount(repository, recipient); - byte[] rewardSharePrivateKey = rewardShareAccount.getSharedSecret(recipientAccount.getPublicKey()); + byte[] rewardSharePrivateKey = rewardShareAccount.getRewardSharePrivateKey(recipientAccount.getPublicKey()); return rewardSharePrivateKey; } diff --git a/src/test/java/org/qora/test/common/TransactionUtils.java b/src/test/java/org/qora/test/common/TransactionUtils.java index 8baf4681..9b3fbc9d 100644 --- a/src/test/java/org/qora/test/common/TransactionUtils.java +++ b/src/test/java/org/qora/test/common/TransactionUtils.java @@ -36,7 +36,7 @@ public class TransactionUtils { assertEquals("Transaction invalid", ValidationResult.OK, result); } - /** Signs transaction using given account and forges a new block, using "alice" account. */ + /** Signs transaction using given account and forges a new block, using "alice" self-reward-share key. */ public static void signAndMint(Repository repository, TransactionData transactionData, PrivateKeyAccount signingAccount) throws DataException { signAsUnconfirmed(repository, transactionData, signingAccount); diff --git a/src/test/java/org/qora/test/forging/RewardShareTests.java b/src/test/java/org/qora/test/minting/RewardShareTests.java similarity index 71% rename from src/test/java/org/qora/test/forging/RewardShareTests.java rename to src/test/java/org/qora/test/minting/RewardShareTests.java index 300b702e..e14c0cb6 100644 --- a/src/test/java/org/qora/test/forging/RewardShareTests.java +++ b/src/test/java/org/qora/test/minting/RewardShareTests.java @@ -1,4 +1,4 @@ -package org.qora.test.forging; +package org.qora.test.minting; import static org.junit.Assert.*; @@ -16,6 +16,7 @@ import org.qora.repository.RepositoryManager; import org.qora.test.common.AccountUtils; import org.qora.test.common.BlockUtils; import org.qora.test.common.Common; +import org.qora.test.common.TransactionUtils; import org.qora.transaction.Transaction; import org.qora.transaction.Transaction.ValidationResult; import org.qora.utils.Base58; @@ -124,4 +125,51 @@ public class RewardShareTests extends Common { } } + @Test + public void testSelfShare() throws DataException { + final String testAccountName = "dilbert"; + + try (final Repository repository = RepositoryManager.getRepository()) { + PrivateKeyAccount signingAccount = Common.getTestAccount(repository, testAccountName); + // byte[] rewardSharePrivateKey = aliceAccount.getRewardSharePrivateKey(aliceAccount.getPublicKey()); + // PrivateKeyAccount rewardShareAccount = new PrivateKeyAccount(repository, rewardSharePrivateKey); + + // Create self-reward-share + TransactionData transactionData = AccountUtils.createRewardShare(repository, testAccountName, testAccountName, BigDecimal.valueOf(100L)); + Transaction transaction = Transaction.fromData(repository, transactionData); + + // Confirm self-share is valid + ValidationResult validationResult = transaction.isValidUnconfirmed(); + assertEquals("Initial self-share should be valid", ValidationResult.OK, validationResult); + + // Check zero fee is valid + transactionData.setFee(BigDecimal.ZERO); + validationResult = transaction.isValidUnconfirmed(); + assertEquals("Zero-fee self-share should be valid", ValidationResult.OK, validationResult); + + TransactionUtils.signAndMint(repository, transactionData, signingAccount); + + // Subsequent non-terminating (0% share) self-reward-share should be invalid + TransactionData newTransactionData = AccountUtils.createRewardShare(repository, testAccountName, testAccountName, BigDecimal.valueOf(99L)); + Transaction newTransaction = Transaction.fromData(repository, newTransactionData); + + // Confirm subsequent self-reward-share is actually invalid + validationResult = newTransaction.isValidUnconfirmed(); + assertNotSame("Subsequent self-share should be invalid", ValidationResult.OK, validationResult); + + // Recheck with zero fee + newTransactionData.setFee(BigDecimal.ZERO); + validationResult = newTransaction.isValidUnconfirmed(); + assertNotSame("Subsequent zero-fee self-share should be invalid", ValidationResult.OK, validationResult); + + // Subsequent terminating (0% share) self-reward-share should be OK + newTransactionData = AccountUtils.createRewardShare(repository, testAccountName, testAccountName, BigDecimal.ZERO); + newTransaction = Transaction.fromData(repository, newTransactionData); + + // Confirm terminating reward-share is valid + validationResult = newTransaction.isValidUnconfirmed(); + assertEquals("Subsequent zero-fee self-share should be invalid", ValidationResult.OK, validationResult); + } + } + } diff --git a/src/test/java/org/qora/test/forging/RewardTests.java b/src/test/java/org/qora/test/minting/RewardTests.java similarity index 83% rename from src/test/java/org/qora/test/forging/RewardTests.java rename to src/test/java/org/qora/test/minting/RewardTests.java index 3df0db9d..7bf4a896 100644 --- a/src/test/java/org/qora/test/forging/RewardTests.java +++ b/src/test/java/org/qora/test/minting/RewardTests.java @@ -1,4 +1,4 @@ -package org.qora.test.forging; +package org.qora.test.minting; import java.math.BigDecimal; import java.math.RoundingMode; @@ -37,11 +37,11 @@ public class RewardTests extends Common { try (final Repository repository = RepositoryManager.getRepository()) { Map> initialBalances = AccountUtils.getBalances(repository, Asset.QORT); - PrivateKeyAccount forgingAccount = Common.getTestAccount(repository, "alice"); + PrivateKeyAccount mintingAccount = Common.getTestAccount(repository, "alice"); BigDecimal blockReward = BlockUtils.getNextBlockReward(repository); - BlockMinter.mintTestingBlock(repository, forgingAccount); + BlockMinter.mintTestingBlock(repository, mintingAccount); BigDecimal expectedBalance = initialBalances.get("alice").get(Asset.QORT).add(blockReward); AccountUtils.assertBalance(repository, "alice", Asset.QORT, expectedBalance); @@ -53,7 +53,7 @@ public class RewardTests extends Common { try (final Repository repository = RepositoryManager.getRepository()) { Map> initialBalances = AccountUtils.getBalances(repository, Asset.QORT); - PrivateKeyAccount forgingAccount = Common.getTestAccount(repository, "alice"); + PrivateKeyAccount mintingAccount = Common.getTestAccount(repository, "alice"); List rewards = BlockChain.getInstance().getBlockRewardsByHeight(); @@ -68,7 +68,7 @@ public class RewardTests extends Common { rewardInfo = rewards.get(rewardIndex); } - BlockMinter.mintTestingBlock(repository, forgingAccount); + BlockMinter.mintTestingBlock(repository, mintingAccount); expectedBalance = expectedBalance.add(rewardInfo.reward); } @@ -77,16 +77,16 @@ public class RewardTests extends Common { } @Test - public void testProxyReward() throws DataException { + public void testRewardSharing() throws DataException { final BigDecimal share = new BigDecimal("12.8"); try (final Repository repository = RepositoryManager.getRepository()) { - byte[] proxyPrivateKey = AccountUtils.rewardShare(repository, "alice", "bob", share); - PrivateKeyAccount proxyAccount = new PrivateKeyAccount(repository, proxyPrivateKey); + byte[] rewardSharePrivateKey = AccountUtils.rewardShare(repository, "alice", "bob", share); + PrivateKeyAccount rewardShareAccount = new PrivateKeyAccount(repository, rewardSharePrivateKey); Map> initialBalances = AccountUtils.getBalances(repository, Asset.QORT); BigDecimal blockReward = BlockUtils.getNextBlockReward(repository); - BlockMinter.mintTestingBlock(repository, proxyAccount); + BlockMinter.mintTestingBlock(repository, rewardShareAccount); // We're expecting reward * 12.8% to Bob, the rest to Alice diff --git a/src/test/resources/test-chain-v2.json b/src/test/resources/test-chain-v2.json index ccc33ed1..fb6de970 100644 --- a/src/test/resources/test-chain-v2.json +++ b/src/test/resources/test-chain-v2.json @@ -52,6 +52,7 @@ { "type": "ISSUE_ASSET", "owner": "QixPbJUwsaHsVEofJdozU9zgVqkK6aYhrK", "assetName": "OTHER", "description": "other test asset", "data": "", "quantity": 1000000, "isDivisible": true, "fee": 0 }, { "type": "ISSUE_ASSET", "owner": "QgV4s3xnzLhVBEJxcYui4u4q11yhUHsd9v", "assetName": "GOLD", "description": "gold test asset", "data": "", "quantity": 1000000, "isDivisible": true, "fee": 0 }, { "type": "ACCOUNT_FLAGS", "target": "QgV4s3xnzLhVBEJxcYui4u4q11yhUHsd9v", "andMask": -1, "orMask": 1, "xorMask": 0 }, + { "type": "ACCOUNT_LEVEL", "target": "Qci5m9k4rcwe4ruKrZZQKka4FzUUMut3er", "level": 8 }, { "type": "REWARD_SHARE", "minterPublicKey": "2tiMr5LTpaWCgbRvkPK8TFd7k63DyHJMMFFsz9uBf1ZP", "recipient": "QgV4s3xnzLhVBEJxcYui4u4q11yhUHsd9v", "rewardSharePublicKey": "7PpfnvLSG7y4HPh8hE7KoqAjLCkv7Ui6xw4mKAkbZtox", "sharePercent": 100 } ] }