From 0cc9cd728e6205505fcf0a97c41a0b35e271c45f Mon Sep 17 00:00:00 2001 From: catbref Date: Tue, 5 May 2020 11:09:46 +0100 Subject: [PATCH] Fix for chain-stall relating to freshly cancelled reward-shares. In some cases, a freshly cancelled reward-share could still have an associated signed timestamp. Block.mint() failed to spot this and used an incorrect "online account" index when building the to-be-minted block. Block.mint() now checks that AccountRepository.getRewardShareIndex() doesn't return null, i.e. indicating that the associated reward-share for that "online account" no longer exists. In turn, AccountRepository.getRewardShareIndex() didn't fulfill its contract of returning null when the passed public key wasn't present in the repository. So this method has been corrected also. AccountRepository.rewardShareExists(byte[] publicKey) : boolean added. BlockMinter had another bug where it didn't check the return from Block.remint() for null properly. This has been fixed. BlockMinter now has additional logging, with cool-off to prevent log spam, for situations where minting could not happen. Unit test (DisagreementTests) added to cover cancelled reward-share case above. BlockMinter testing support slightly modified to help. --- src/main/java/org/qortal/block/Block.java | 6 +- .../java/org/qortal/block/BlockMinter.java | 49 +++++-- .../qortal/repository/AccountRepository.java | 2 + .../hsqldb/HSQLDBAccountRepository.java | 19 ++- .../test/minting/DisagreementTests.java | 125 ++++++++++++++++++ 5 files changed, 187 insertions(+), 14 deletions(-) create mode 100644 src/test/java/org/qortal/test/minting/DisagreementTests.java diff --git a/src/main/java/org/qortal/block/Block.java b/src/main/java/org/qortal/block/Block.java index 4ad7894f..f0835d2d 100644 --- a/src/main/java/org/qortal/block/Block.java +++ b/src/main/java/org/qortal/block/Block.java @@ -323,7 +323,11 @@ public class Block { if (onlineAccountData.getTimestamp() != onlineAccountsTimestamp) continue; - int accountIndex = repository.getAccountRepository().getRewardShareIndex(onlineAccountData.getPublicKey()); + Integer accountIndex = repository.getAccountRepository().getRewardShareIndex(onlineAccountData.getPublicKey()); + if (accountIndex == null) + // Online account (reward-share) with current timestamp but reward-share cancelled + continue; + indexedOnlineAccounts.put(accountIndex, onlineAccountData); } List accountIndexes = new ArrayList<>(indexedOnlineAccounts.keySet()); diff --git a/src/main/java/org/qortal/block/BlockMinter.java b/src/main/java/org/qortal/block/BlockMinter.java index c5e64728..a9246d27 100644 --- a/src/main/java/org/qortal/block/BlockMinter.java +++ b/src/main/java/org/qortal/block/BlockMinter.java @@ -40,6 +40,8 @@ public class BlockMinter extends Thread { // Other properties private static final Logger LOGGER = LogManager.getLogger(BlockMinter.class); + private static Long lastLogTimestamp; + private static Long logTimeout; // Constructors @@ -151,6 +153,9 @@ public class BlockMinter extends Thread { if (previousBlock == null || !Arrays.equals(previousBlock.getSignature(), lastBlockData.getSignature())) { previousBlock = new Block(repository, lastBlockData); newBlocks.clear(); + + // Reduce log timeout + logTimeout = 10 * 1000L; } // Discard accounts we have already built blocks with @@ -163,19 +168,23 @@ public class BlockMinter extends Thread { // First block does the AT heavy-lifting if (newBlocks.isEmpty()) { Block newBlock = Block.mint(repository, previousBlock.getBlockData(), mintingAccount); - if (newBlock == null) + if (newBlock == null) { // For some reason we can't mint right now + moderatedLog(() -> LOGGER.error("Couldn't build a to-be-minted block")); continue; + } newBlocks.add(newBlock); } else { // The blocks for other minters require less effort... - Block newBlock = newBlocks.get(0); - if (newBlock == null) + Block newBlock = newBlocks.get(0).remint(mintingAccount); + if (newBlock == null) { // For some reason we can't mint right now + moderatedLog(() -> LOGGER.error("Couldn't rebuild a to-be-minted block")); continue; + } - newBlocks.add(newBlock.remint(mintingAccount)); + newBlocks.add(newBlock); } } @@ -193,9 +202,14 @@ public class BlockMinter extends Thread { boolean newBlockMinted = false; try { - // Clear repository's "in transaction" state so we don't cause a repository deadlock + // Clear repository session state so we have latest view of data repository.discardChanges(); + // Now that we have blockchain lock, do final check that chain hasn't changed + BlockData latestBlockData = blockRepository.getLastBlock(); + if (!Arrays.equals(lastBlockData.getSignature(), latestBlockData.getSignature())) + continue; + List goodBlocks = new ArrayList<>(); for (Block testBlock : newBlocks) { // Is new block's timestamp valid yet? @@ -204,8 +218,12 @@ public class BlockMinter extends Thread { continue; // Is new block valid yet? (Before adding unconfirmed transactions) - if (testBlock.isValid() != ValidationResult.OK) + ValidationResult result = testBlock.isValid(); + if (result != ValidationResult.OK) { + moderatedLog(() -> LOGGER.error(String.format("To-be-minted block invalid '%s' before adding transactions?", result.name()))); + continue; + } goodBlocks.add(testBlock); } @@ -356,10 +374,14 @@ public class BlockMinter extends Thread { // Ensure mintingAccount is 'online' so blocks can be minted Controller.getInstance().ensureTestingAccountsOnline(mintingAndOnlineAccounts); - BlockData previousBlockData = repository.getBlockRepository().getLastBlock(); - PrivateKeyAccount mintingAccount = mintingAndOnlineAccounts[0]; + mintTestingBlockRetainingTimestamps(repository, mintingAccount); + } + + public static void mintTestingBlockRetainingTimestamps(Repository repository, PrivateKeyAccount mintingAccount) throws DataException { + BlockData previousBlockData = repository.getBlockRepository().getLastBlock(); + Block newBlock = Block.mint(repository, previousBlockData, mintingAccount); // Make sure we're the only thread modifying the blockchain @@ -387,4 +409,15 @@ public class BlockMinter extends Thread { } } + private static void moderatedLog(Runnable logFunction) { + // We only log if logging at TRACE or previous log timeout has expired + if (!LOGGER.isTraceEnabled() && lastLogTimestamp != null && lastLogTimestamp + logTimeout > System.currentTimeMillis()) + return; + + lastLogTimestamp = System.currentTimeMillis(); + logTimeout = 2 * 60 * 1000L; // initial timeout, can be reduced if new block appears + + logFunction.run(); + } + } diff --git a/src/main/java/org/qortal/repository/AccountRepository.java b/src/main/java/org/qortal/repository/AccountRepository.java index 15aba42a..805b5ece 100644 --- a/src/main/java/org/qortal/repository/AccountRepository.java +++ b/src/main/java/org/qortal/repository/AccountRepository.java @@ -139,6 +139,8 @@ public interface AccountRepository { */ public RewardShareData getRewardShareByIndex(int index) throws DataException; + public boolean rewardShareExists(byte[] rewardSharePublicKey) throws DataException; + public void save(RewardShareData rewardShareData) throws DataException; /** Delete reward-share from repository using passed minting account's public key and recipient's address. */ diff --git a/src/main/java/org/qortal/repository/hsqldb/HSQLDBAccountRepository.java b/src/main/java/org/qortal/repository/hsqldb/HSQLDBAccountRepository.java index c38fa438..6bcaeb3b 100644 --- a/src/main/java/org/qortal/repository/hsqldb/HSQLDBAccountRepository.java +++ b/src/main/java/org/qortal/repository/hsqldb/HSQLDBAccountRepository.java @@ -689,13 +689,13 @@ public class HSQLDBAccountRepository implements AccountRepository { } @Override - public Integer getRewardShareIndex(byte[] publicKey) throws DataException { + public Integer getRewardShareIndex(byte[] rewardSharePublicKey) throws DataException { + if (!this.rewardShareExists(rewardSharePublicKey)) + return null; + String sql = "SELECT COUNT(*) FROM RewardShares WHERE reward_share_public_key < ?"; - try (ResultSet resultSet = this.repository.checkedExecute(sql, publicKey)) { - if (resultSet == null) - return null; - + try (ResultSet resultSet = this.repository.checkedExecute(sql, rewardSharePublicKey)) { return resultSet.getInt(1); } catch (SQLException e) { throw new DataException("Unable to determine reward-share index in repository", e); @@ -724,6 +724,15 @@ public class HSQLDBAccountRepository implements AccountRepository { } } + @Override + public boolean rewardShareExists(byte[] rewardSharePublicKey) throws DataException { + try { + return this.repository.exists("RewardShares", "reward_share_public_key = ?", rewardSharePublicKey); + } catch (SQLException e) { + throw new DataException("Unable to check reward-share exists in repository", e); + } + } + @Override public void save(RewardShareData rewardShareData) throws DataException { HSQLDBSaver saveHelper = new HSQLDBSaver("RewardShares"); diff --git a/src/test/java/org/qortal/test/minting/DisagreementTests.java b/src/test/java/org/qortal/test/minting/DisagreementTests.java new file mode 100644 index 00000000..e71eb9f6 --- /dev/null +++ b/src/test/java/org/qortal/test/minting/DisagreementTests.java @@ -0,0 +1,125 @@ +package org.qortal.test.minting; + +import static org.junit.Assert.*; + +import java.math.BigDecimal; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.qortal.account.PrivateKeyAccount; +import org.qortal.block.BlockMinter; +import org.qortal.controller.Controller; +import org.qortal.data.account.RewardShareData; +import org.qortal.data.block.BlockData; +import org.qortal.data.transaction.TransactionData; +import org.qortal.repository.DataException; +import org.qortal.repository.Repository; +import org.qortal.repository.RepositoryManager; +import org.qortal.test.common.AccountUtils; +import org.qortal.test.common.Common; +import org.qortal.test.common.TestAccount; +import org.qortal.test.common.TransactionUtils; +import org.qortal.transform.block.BlockTransformer; +import org.roaringbitmap.IntIterator; + +import io.druid.extendedset.intset.ConciseSet; + +public class DisagreementTests extends Common { + + private static final BigDecimal CANCEL_SHARE_PERCENT = BigDecimal.ONE.negate(); + + @Before + public void beforeTest() throws DataException { + Common.useDefaultSettings(); + } + + @After + public void afterTest() throws DataException { + Common.orphanCheck(); + } + + /** + * Testing minting a block when there is a signed online account timestamp present + * that no longer has a corresponding reward-share in DB. + *

+ * Something like: + *

    + *
  • Mint block, with tx to create reward-share R
    • + *
  • Sign current timestamp with R
    • + *
  • Mint block including R as online account
    • + *
  • Mint block, with tx to cancel reward-share R
    • + *
  • Mint another block: R's timestamp should be excluded
    • + *
+ * + * @throws DataException + */ + @Test + public void testOnlineAccounts() throws DataException { + final BigDecimal sharePercent = new BigDecimal("12.8"); + + try (final Repository repository = RepositoryManager.getRepository()) { + TestAccount mintingAccount = Common.getTestAccount(repository, "alice-reward-share"); + TestAccount signingAccount = Common.getTestAccount(repository, "alice"); + + // Create reward-share + byte[] testRewardSharePrivateKey = AccountUtils.rewardShare(repository, "alice", "bob", sharePercent); + PrivateKeyAccount testRewardShareAccount = new PrivateKeyAccount(repository, testRewardSharePrivateKey); + + // Confirm reward-share info set correctly + RewardShareData testRewardShareData = repository.getAccountRepository().getRewardShare(testRewardShareAccount.getPublicKey()); + assertNotNull(testRewardShareData); + + // Create signed timestamps + Controller.getInstance().ensureTestingAccountsOnline(mintingAccount, testRewardShareAccount); + + // Mint another block + BlockMinter.mintTestingBlockRetainingTimestamps(repository, mintingAccount); + + // Confirm reward-share's signed timestamp is included + BlockData blockData = repository.getBlockRepository().getLastBlock(); + List rewardSharesData = fetchRewardSharesForBlock(repository, blockData); + boolean doesContainRewardShare = rewardSharesData.stream().anyMatch(rewardShareData -> Arrays.equals(rewardShareData.getRewardSharePublicKey(), testRewardShareData.getRewardSharePublicKey())); + assertTrue(doesContainRewardShare); + + // Cancel reward-share + TransactionData cancelRewardShareTransactionData = AccountUtils.createRewardShare(repository, "alice", "bob", CANCEL_SHARE_PERCENT); + TransactionUtils.signAsUnconfirmed(repository, cancelRewardShareTransactionData, signingAccount); + BlockMinter.mintTestingBlockRetainingTimestamps(repository, mintingAccount); + + // Confirm reward-share no longer exists in repository + RewardShareData cancelledRewardShareData = repository.getAccountRepository().getRewardShare(testRewardShareAccount.getPublicKey()); + assertNull("Reward-share shouldn't exist", cancelledRewardShareData); + + // Attempt to mint with cancelled reward-share + BlockMinter.mintTestingBlockRetainingTimestamps(repository, mintingAccount); + + // Confirm reward-share's signed timestamp is NOT included + blockData = repository.getBlockRepository().getLastBlock(); + rewardSharesData = fetchRewardSharesForBlock(repository, blockData); + doesContainRewardShare = rewardSharesData.stream().anyMatch(rewardShareData -> Arrays.equals(rewardShareData.getRewardSharePublicKey(), testRewardShareData.getRewardSharePublicKey())); + assertFalse(doesContainRewardShare); + } + } + + private List fetchRewardSharesForBlock(Repository repository, BlockData blockData) throws DataException { + byte[] encodedOnlineAccounts = blockData.getEncodedOnlineAccounts(); + ConciseSet accountIndexes = BlockTransformer.decodeOnlineAccounts(encodedOnlineAccounts); + + List rewardSharesData = new ArrayList<>(); + + IntIterator iterator = accountIndexes.iterator(); + while (iterator.hasNext()) { + int accountIndex = iterator.next(); + + RewardShareData rewardShareData = repository.getAccountRepository().getRewardShareByIndex(accountIndex); + rewardSharesData.add(rewardShareData); + } + + return rewardSharesData; + } + +}