From e86143426bcd512704f3a8a19603cf3af89fb613 Mon Sep 17 00:00:00 2001 From: catbref Date: Fri, 1 May 2020 08:57:15 +0100 Subject: [PATCH] Fix potentially overflowing multiply in Block reward processing. Change BlockChain config to use AmountTypeAdapter instead of creating duplicated long versions of BigDecimal values. Some tidying to Amounts class. --- src/main/java/org/qortal/block/Block.java | 14 ++-- .../java/org/qortal/block/BlockChain.java | 67 +++++-------------- .../org/qortal/transaction/Transaction.java | 6 +- src/main/java/org/qortal/utils/Amounts.java | 20 +++--- .../common/transaction/TestTransaction.java | 2 +- .../org/qortal/test/minting/RewardTests.java | 8 +-- 6 files changed, 43 insertions(+), 74 deletions(-) diff --git a/src/main/java/org/qortal/block/Block.java b/src/main/java/org/qortal/block/Block.java index 0ad91e18..a4c7131c 100644 --- a/src/main/java/org/qortal/block/Block.java +++ b/src/main/java/org/qortal/block/Block.java @@ -1313,10 +1313,10 @@ public class Block { } protected void processBlockRewards() throws DataException { - Long reward = BlockChain.getInstance().getRewardAtHeight(this.blockData.getHeight()); + long reward = BlockChain.getInstance().getRewardAtHeight(this.blockData.getHeight()); // No reward for our height? - if (reward == null) + if (reward == 0) return; distributeBlockReward(reward); @@ -1548,10 +1548,10 @@ public class Block { } protected void orphanBlockRewards() throws DataException { - Long reward = BlockChain.getInstance().getRewardAtHeight(this.blockData.getHeight()); + long reward = BlockChain.getInstance().getRewardAtHeight(this.blockData.getHeight()); // No reward for our height? - if (reward == null) + if (reward == 0) return; distributeBlockReward(0 - reward); @@ -1656,7 +1656,7 @@ public class Block { for (int s = 0; s < sharesByLevel.size(); ++s) { final int binIndex = s; - long binAmount = (totalAmount * sharesByLevel.get(binIndex).unscaledShare) / 100000000L; + long binAmount = Amounts.roundDownScaledMultiply(totalAmount, sharesByLevel.get(binIndex).share); LOGGER.trace(() -> String.format("Bin %d share of %s: %s", binIndex, Amounts.prettyAmount(totalAmount), Amounts.prettyAmount(binAmount))); // Spread across all accounts in bin. getShareBin() returns -1 for minter accounts that are also founders, so they are effectively filtered out. @@ -1677,12 +1677,12 @@ public class Block { } private long distributeBlockRewardToQoraHolders(long totalAmount) throws DataException { - long qoraHoldersAmount = (BlockChain.getInstance().getQoraHoldersUnscaledShare() * totalAmount) / 100000000L; + long qoraHoldersAmount = Amounts.roundDownScaledMultiply(totalAmount, BlockChain.getInstance().getQoraHoldersShare()); LOGGER.trace(() -> String.format("Legacy QORA holders share of %s: %s", Amounts.prettyAmount(totalAmount), Amounts.prettyAmount(qoraHoldersAmount))); final boolean isProcessingNotOrphaning = totalAmount >= 0; - long qoraPerQortReward = BlockChain.getInstance().getUnscaledQoraPerQortReward(); + long qoraPerQortReward = BlockChain.getInstance().getQoraPerQortReward(); List qoraHolders = this.repository.getAccountRepository().getEligibleLegacyQoraHolders(isProcessingNotOrphaning ? null : this.blockData.getHeight()); long totalQoraHeld = 0; diff --git a/src/main/java/org/qortal/block/BlockChain.java b/src/main/java/org/qortal/block/BlockChain.java index 4ac0e2d6..01c09655 100644 --- a/src/main/java/org/qortal/block/BlockChain.java +++ b/src/main/java/org/qortal/block/BlockChain.java @@ -3,7 +3,6 @@ package org.qortal.block; import java.io.File; import java.io.FileNotFoundException; import java.io.InputStream; -import java.math.BigDecimal; import java.sql.SQLException; import java.util.ArrayList; import java.util.Collections; @@ -55,13 +54,14 @@ public class BlockChain { /** Transaction expiry period, starting from transaction's timestamp, in milliseconds. */ private long transactionExpiryPeriod; - private BigDecimal unitFee; - private long unscaledUnitFee; // calculated after unmarshal + @XmlJavaTypeAdapter(value = org.qortal.api.AmountTypeAdapter.class) + private long unitFee; private int maxBytesPerUnitFee; /** Maximum acceptable timestamp disagreement offset in milliseconds. */ private long blockTimestampMargin; + /** Maximum block size, in bytes. */ private int maxBlockSize; @@ -86,28 +86,26 @@ public class BlockChain { /** Block rewards by block height */ public static class RewardByHeight { public int height; - public BigDecimal reward; - public long unscaledReward; // reward * 1e8, calculated after unmarshal + @XmlJavaTypeAdapter(value = org.qortal.api.AmountTypeAdapter.class) + public long reward; } List rewardsByHeight; /** Share of block reward/fees by account level */ public static class ShareByLevel { public List levels; - public BigDecimal share; - public long unscaledShare; // share * 1e8, calculated after unmarshal + @XmlJavaTypeAdapter(value = org.qortal.api.AmountTypeAdapter.class) + public long share; } List sharesByLevel; /** Share of block reward/fees to legacy QORA coin holders */ - BigDecimal qoraHoldersShare; - /** Unscaled (* 1e8) share of block reward/fees to legacy QORA coin holders */ - private long qoraHoldersUnscaledShare; // calculated after unmarshal + @XmlJavaTypeAdapter(value = org.qortal.api.AmountTypeAdapter.class) + private Long qoraHoldersShare; /** How many legacy QORA per 1 QORT of block reward. */ - BigDecimal qoraPerQortReward; - /** How many legacy QORA per 1 QORT of block reward. Unscaled (* 1e8). */ - private long unscaledQoraPerQortReward; // calculated after unmarshal + @XmlJavaTypeAdapter(value = org.qortal.api.AmountTypeAdapter.class) + private Long qoraPerQortReward; /** * Number of minted blocks required to reach next level from previous. @@ -268,14 +266,10 @@ public class BlockChain { return this.isTestChain; } - public BigDecimal getUnitFee() { + public long getUnitFee() { return this.unitFee; } - public long getUnscaledUnitFee() { - return this.unscaledUnitFee; - } - public int getMaxBytesPerUnitFee() { return this.maxBytesPerUnitFee; } @@ -321,22 +315,14 @@ public class BlockChain { return this.cumulativeBlocksByLevel; } - public BigDecimal getQoraHoldersShare() { + public long getQoraHoldersShare() { return this.qoraHoldersShare; } - public long getQoraHoldersUnscaledShare() { - return this.qoraHoldersUnscaledShare; - } - - public BigDecimal getQoraPerQortReward() { + public long getQoraPerQortReward() { return this.qoraPerQortReward; } - public long getUnscaledQoraPerQortReward() { - return this.unscaledQoraPerQortReward; - } - public int getMinAccountLevelToMint() { return this.minAccountLevelToMint; } @@ -365,13 +351,13 @@ public class BlockChain { // More complex getters for aspects that change by height or timestamp - public Long getRewardAtHeight(int ourHeight) { + public long getRewardAtHeight(int ourHeight) { // Scan through for reward at our height for (int i = rewardsByHeight.size() - 1; i >= 0; --i) if (rewardsByHeight.get(i).height <= ourHeight) - return rewardsByHeight.get(i).unscaledReward; + return rewardsByHeight.get(i).reward; - return null; + return 0; } public BlockTimingByHeight getBlockTimingByHeight(int ourHeight) { @@ -431,10 +417,7 @@ public class BlockChain { /** Minor normalization, cached value generation, etc. */ private void fixUp() { - this.unitFee = this.unitFee.setScale(8); - this.unscaledUnitFee = this.unitFee.unscaledValue().longValue(); - - // Pre-calculate cumulative blocks required for each level + // Calculate cumulative blocks required for each level int cumulativeBlocks = 0; this.cumulativeBlocksByLevel = new ArrayList<>(this.blocksNeededByLevel.size() + 1); for (int level = 0; level <= this.blocksNeededByLevel.size(); ++level) { @@ -444,20 +427,6 @@ public class BlockChain { cumulativeBlocks += this.blocksNeededByLevel.get(level); } - // Calculate unscaled long versions of block rewards by height - for (RewardByHeight rewardByHeight : this.rewardsByHeight) - rewardByHeight.unscaledReward = rewardByHeight.reward.setScale(8).unscaledValue().longValue(); - - // Calculate unscaled long versions of block reward shares-by-level - for (ShareByLevel shareByLevel : this.sharesByLevel) - shareByLevel.unscaledShare = shareByLevel.share.setScale(8).unscaledValue().longValue(); - - // Calculate unscaled long version of Qora-holders block reward share - this.qoraHoldersUnscaledShare = this.qoraHoldersShare.setScale(8).unscaledValue().longValue(); - - // Calculate unscaled long version of Qora-per-Qort block reward - this.unscaledQoraPerQortReward = this.qoraPerQortReward.setScale(8).unscaledValue().longValue(); - // Convert collections to unmodifiable form this.rewardsByHeight = Collections.unmodifiableList(this.rewardsByHeight); this.sharesByLevel = Collections.unmodifiableList(this.sharesByLevel); diff --git a/src/main/java/org/qortal/transaction/Transaction.java b/src/main/java/org/qortal/transaction/Transaction.java index d93ba732..35c0076e 100644 --- a/src/main/java/org/qortal/transaction/Transaction.java +++ b/src/main/java/org/qortal/transaction/Transaction.java @@ -320,7 +320,7 @@ public abstract class Transaction { /** Returns whether transaction's fee is at least minimum unit fee as specified in blockchain config. */ public boolean hasMinimumFee() { - return this.transactionData.getFee() >= BlockChain.getInstance().getUnscaledUnitFee(); + return this.transactionData.getFee() >= BlockChain.getInstance().getUnitFee(); } public long feePerByte() { @@ -333,7 +333,7 @@ public abstract class Transaction { /** Returns whether transaction's fee is at least amount needed to cover byte-length of transaction. */ public boolean hasMinimumFeePerByte() { - long unitFee = BlockChain.getInstance().getUnscaledUnitFee(); + long unitFee = BlockChain.getInstance().getUnitFee(); int maxBytePerUnitFee = BlockChain.getInstance().getMaxBytesPerUnitFee(); return this.feePerByte() >= maxBytePerUnitFee / unitFee; @@ -351,7 +351,7 @@ public abstract class Transaction { int unitFeeCount = ((dataLength - 1) / maxBytePerUnitFee) + 1; - return BlockChain.getInstance().getUnscaledUnitFee() * unitFeeCount; + return BlockChain.getInstance().getUnitFee() * unitFeeCount; } /** diff --git a/src/main/java/org/qortal/utils/Amounts.java b/src/main/java/org/qortal/utils/Amounts.java index 1be28c47..750b6919 100644 --- a/src/main/java/org/qortal/utils/Amounts.java +++ b/src/main/java/org/qortal/utils/Amounts.java @@ -14,13 +14,13 @@ public abstract class Amounts { public static String prettyAmount(long amount) { StringBuilder stringBuilder = new StringBuilder(20); - stringBuilder.append(amount / 100000000L); + stringBuilder.append(amount / MULTIPLIER); stringBuilder.append('.'); int dpLength = stringBuilder.length(); - stringBuilder.append(Math.abs(amount % 100000000L)); + stringBuilder.append(Math.abs(amount % MULTIPLIER)); int paddingRequired = 8 - (stringBuilder.length() - dpLength); if (paddingRequired > 0) @@ -48,20 +48,20 @@ public abstract class Amounts { return Math.abs(a); } - public static long roundUpScaledMultiply(BigInteger amount, BigInteger price) { - return amount.multiply(price).add(ROUNDING).divide(MULTIPLIER_BI).longValue(); + public static long roundUpScaledMultiply(BigInteger multiplicand, BigInteger multiplier) { + return multiplicand.multiply(multiplier).add(ROUNDING).divide(MULTIPLIER_BI).longValue(); } - public static long roundUpScaledMultiply(long amount, long price) { - return roundUpScaledMultiply(BigInteger.valueOf(amount), BigInteger.valueOf(price)); + public static long roundUpScaledMultiply(long multiplicand, long multiplier) { + return roundUpScaledMultiply(BigInteger.valueOf(multiplicand), BigInteger.valueOf(multiplier)); } - public static long roundDownScaledMultiply(BigInteger amount, BigInteger price) { - return amount.multiply(price).divide(MULTIPLIER_BI).longValue(); + public static long roundDownScaledMultiply(BigInteger multiplicand, BigInteger multiplier) { + return multiplicand.multiply(multiplier).divide(MULTIPLIER_BI).longValue(); } - public static long roundDownScaledMultiply(long amount, long price) { - return roundDownScaledMultiply(BigInteger.valueOf(amount), BigInteger.valueOf(price)); + public static long roundDownScaledMultiply(long multiplicand, long multiplier) { + return roundDownScaledMultiply(BigInteger.valueOf(multiplicand), BigInteger.valueOf(multiplier)); } public static long scaledDivide(long dividend, long divisor) { diff --git a/src/test/java/org/qortal/test/common/transaction/TestTransaction.java b/src/test/java/org/qortal/test/common/transaction/TestTransaction.java index f1553a2c..00aa92de 100644 --- a/src/test/java/org/qortal/test/common/transaction/TestTransaction.java +++ b/src/test/java/org/qortal/test/common/transaction/TestTransaction.java @@ -13,7 +13,7 @@ public abstract class TestTransaction { protected static final Random random = new Random(); public static BaseTransactionData generateBase(PrivateKeyAccount account) throws DataException { - return new BaseTransactionData(System.currentTimeMillis(), Group.NO_GROUP, account.getLastReference(), account.getPublicKey(), BlockChain.getInstance().getUnscaledUnitFee(), null); + return new BaseTransactionData(System.currentTimeMillis(), Group.NO_GROUP, account.getLastReference(), account.getPublicKey(), BlockChain.getInstance().getUnitFee(), null); } } diff --git a/src/test/java/org/qortal/test/minting/RewardTests.java b/src/test/java/org/qortal/test/minting/RewardTests.java index 20bcb982..c96e4482 100644 --- a/src/test/java/org/qortal/test/minting/RewardTests.java +++ b/src/test/java/org/qortal/test/minting/RewardTests.java @@ -71,7 +71,7 @@ public class RewardTests extends Common { BlockUtils.mintBlock(repository); - expectedBalance += rewardInfo.unscaledReward; + expectedBalance += rewardInfo.reward; } AccountUtils.assertBalance(repository, "alice", Asset.QORT, expectedBalance); @@ -106,8 +106,8 @@ public class RewardTests extends Common { public void testLegacyQoraReward() throws DataException { Common.useSettings("test-settings-v2-qora-holder.json"); - long qoraHoldersShare = BlockChain.getInstance().getQoraHoldersUnscaledShare(); - long qoraPerQort = BlockChain.getInstance().getUnscaledQoraPerQortReward(); + long qoraHoldersShare = BlockChain.getInstance().getQoraHoldersShare(); + long qoraPerQort = BlockChain.getInstance().getQoraPerQortReward(); try (final Repository repository = RepositoryManager.getRepository()) { Map> initialBalances = AccountUtils.getBalances(repository, Asset.QORT, Asset.LEGACY_QORA, Asset.QORT_FROM_QORA); @@ -161,7 +161,7 @@ public class RewardTests extends Common { public void testMaxLegacyQoraReward() throws DataException { Common.useSettings("test-settings-v2-qora-holder.json"); - long qoraPerQort = BlockChain.getInstance().getUnscaledQoraPerQortReward(); + long qoraPerQort = BlockChain.getInstance().getQoraPerQortReward(); try (final Repository repository = RepositoryManager.getRepository()) { Map> initialBalances = AccountUtils.getBalances(repository, Asset.QORT, Asset.LEGACY_QORA, Asset.QORT_FROM_QORA);