From a6897e5eb66eb07501e344dc39e984ffb1952e32 Mon Sep 17 00:00:00 2001 From: cd2357 Date: Tue, 11 May 2021 10:29:32 +0200 Subject: [PATCH 1/7] Improve timer accuracy in BlockParser Measure the entire parseBlock() duration, not just the substep of parsing transactions. --- .../src/main/java/bisq/core/dao/node/parser/BlockParser.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/bisq/core/dao/node/parser/BlockParser.java b/core/src/main/java/bisq/core/dao/node/parser/BlockParser.java index 900634bfb07..8a7e9f3d450 100644 --- a/core/src/main/java/bisq/core/dao/node/parser/BlockParser.java +++ b/core/src/main/java/bisq/core/dao/node/parser/BlockParser.java @@ -76,6 +76,7 @@ public BlockParser(TxParser txParser, * @throws BlockHeightNotConnectingException If new block height is not current chain Height + 1 */ public Block parseBlock(RawBlock rawBlock) throws BlockHashNotConnectingException, BlockHeightNotConnectingException { + long startTs = System.currentTimeMillis(); int blockHeight = rawBlock.getHeight(); log.trace("Parse block at height={} ", blockHeight); @@ -102,7 +103,6 @@ public Block parseBlock(RawBlock rawBlock) throws BlockHashNotConnectingExceptio // There are some blocks with testing such dependency chains like block 130768 where at each iteration only // one get resolved. // Lately there is a patter with 24 iterations observed - long startTs = System.currentTimeMillis(); rawBlock.getRawTxs().forEach(rawTx -> txParser.findTx(rawTx, @@ -111,10 +111,9 @@ public Block parseBlock(RawBlock rawBlock) throws BlockHashNotConnectingExceptio genesisTotalSupply) .ifPresent(tx -> daoStateService.onNewTxForLastBlock(block, tx))); + daoStateService.onParseBlockComplete(block); log.info("Parsing {} transactions at block height {} took {} ms", rawBlock.getRawTxs().size(), blockHeight, System.currentTimeMillis() - startTs); - - daoStateService.onParseBlockComplete(block); return block; } From ede38c13a360c7ebc9ed3b24482c4b066a023ed4 Mon Sep 17 00:00:00 2001 From: cd2357 Date: Tue, 11 May 2021 11:03:57 +0200 Subject: [PATCH 2/7] Cache block hashes in DaoState Improve isBlockHashKnown() by about 100ms per call by caching the hashes of the known blocks. --- .../bisq/core/dao/state/DaoStateService.java | 3 ++- .../bisq/core/dao/state/model/DaoState.java | 17 ++++++++++++++++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/bisq/core/dao/state/DaoStateService.java b/core/src/main/java/bisq/core/dao/state/DaoStateService.java index ebfeddf7819..07f3f23179f 100644 --- a/core/src/main/java/bisq/core/dao/state/DaoStateService.java +++ b/core/src/main/java/bisq/core/dao/state/DaoStateService.java @@ -119,6 +119,7 @@ public void applySnapshot(DaoState snapshot) { daoState.getBlocks().clear(); daoState.getBlocks().addAll(snapshot.getBlocks()); + daoState.setBlockHashCache(snapshot.getBlockHashCache()); daoState.getCycles().clear(); daoState.getCycles().addAll(snapshot.getCycles()); @@ -295,7 +296,7 @@ public LinkedList getBlocks() { * {@code false}. */ public boolean isBlockHashKnown(String blockHash) { - return getBlocks().stream().anyMatch(block -> block.getHash().equals(blockHash)); + return daoState.getBlockHashCache().contains(blockHash); } public Optional getLastBlock() { diff --git a/core/src/main/java/bisq/core/dao/state/model/DaoState.java b/core/src/main/java/bisq/core/dao/state/model/DaoState.java index bb65f7b30dc..9dfb66bc637 100644 --- a/core/src/main/java/bisq/core/dao/state/model/DaoState.java +++ b/core/src/main/java/bisq/core/dao/state/model/DaoState.java @@ -41,6 +41,7 @@ import java.util.LinkedList; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.TreeMap; import java.util.function.Function; import java.util.stream.Collectors; @@ -107,7 +108,8 @@ public static DaoState getClone(DaoState daoState) { // Transient data used only as an index - must be kept in sync with the block list @JsonExclude private transient final Map txCache; // key is txId - + @JsonExclude + private transient final Set blockHashCache; /////////////////////////////////////////////////////////////////////////////////////////// // Constructor @@ -159,6 +161,10 @@ private DaoState(int chainHeight, txCache = blocks.stream() .flatMap(block -> block.getTxs().stream()) .collect(Collectors.toMap(Tx::getId, Function.identity(), (x, y) -> x, HashMap::new)); + + blockHashCache = blocks.stream() + .map(Block::getHash) + .collect(Collectors.toSet()); } @Override @@ -249,10 +255,19 @@ public void setTxCache(Map txCache) { this.txCache.putAll(txCache); } + public void setBlockHashCache(Set blockHashCache) { + this.blockHashCache.clear(); + this.blockHashCache.addAll(blockHashCache); + } + public Map getTxCache() { return Collections.unmodifiableMap(txCache); } + public Set getBlockHashCache() { + return Collections.unmodifiableSet(blockHashCache); + } + @Override public String toString() { return "DaoState{" + From 792d135693a1dd9b903160d42269340b4049cb5a Mon Sep 17 00:00:00 2001 From: cd2357 Date: Tue, 11 May 2021 11:49:30 +0200 Subject: [PATCH 3/7] Remove duplicate lookup method Method is duplicate of isBlockHashKnown(). --- core/src/main/java/bisq/core/dao/state/DaoStateService.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/core/src/main/java/bisq/core/dao/state/DaoStateService.java b/core/src/main/java/bisq/core/dao/state/DaoStateService.java index 07f3f23179f..1cb0dc4f57f 100644 --- a/core/src/main/java/bisq/core/dao/state/DaoStateService.java +++ b/core/src/main/java/bisq/core/dao/state/DaoStateService.java @@ -320,10 +320,6 @@ public boolean containsBlock(Block block) { return getBlocks().contains(block); } - public boolean containsBlockHash(String blockHash) { - return getBlocks().stream().anyMatch(block -> block.getHash().equals(blockHash)); - } - public long getBlockTime(int height) { return getBlockAtHeight(height).map(Block::getTime).orElse(0L); } From fea717eeabf867676b90fb57a89845cf73e961bf Mon Sep 17 00:00:00 2001 From: cd2357 Date: Tue, 11 May 2021 12:03:46 +0200 Subject: [PATCH 4/7] Add blockHeight cache in DaoState Improve block lookup by height by introducing an index, instead of iterating through all blocks and comparing the height. --- .../java/bisq/core/dao/state/DaoStateService.java | 5 ++--- .../java/bisq/core/dao/state/model/DaoState.java | 12 ++++++++++++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/bisq/core/dao/state/DaoStateService.java b/core/src/main/java/bisq/core/dao/state/DaoStateService.java index 1cb0dc4f57f..230929cddce 100644 --- a/core/src/main/java/bisq/core/dao/state/DaoStateService.java +++ b/core/src/main/java/bisq/core/dao/state/DaoStateService.java @@ -120,6 +120,7 @@ public void applySnapshot(DaoState snapshot) { daoState.getBlocks().clear(); daoState.getBlocks().addAll(snapshot.getBlocks()); daoState.setBlockHashCache(snapshot.getBlockHashCache()); + daoState.setBlockHeightCache(snapshot.getBlockHeightCache()); daoState.getCycles().clear(); daoState.getCycles().addAll(snapshot.getCycles()); @@ -311,9 +312,7 @@ public int getBlockHeightOfLastBlock() { } public Optional getBlockAtHeight(int height) { - return getBlocks().stream() - .filter(block -> block.getHeight() == height) - .findAny(); + return Optional.ofNullable(daoState.getBlockHeightCache().get(height)); } public boolean containsBlock(Block block) { diff --git a/core/src/main/java/bisq/core/dao/state/model/DaoState.java b/core/src/main/java/bisq/core/dao/state/model/DaoState.java index 9dfb66bc637..84c636a5738 100644 --- a/core/src/main/java/bisq/core/dao/state/model/DaoState.java +++ b/core/src/main/java/bisq/core/dao/state/model/DaoState.java @@ -109,6 +109,8 @@ public static DaoState getClone(DaoState daoState) { @JsonExclude private transient final Map txCache; // key is txId @JsonExclude + private transient final Map blockHeightCache; // key is block height + @JsonExclude private transient final Set blockHashCache; /////////////////////////////////////////////////////////////////////////////////////////// @@ -165,6 +167,9 @@ private DaoState(int chainHeight, blockHashCache = blocks.stream() .map(Block::getHash) .collect(Collectors.toSet()); + + blockHeightCache = blocks.stream() + .collect(Collectors.toMap(Block::getHeight, Function.identity(), (x, y) -> x, HashMap::new)); } @Override @@ -260,6 +265,11 @@ public void setBlockHashCache(Set blockHashCache) { this.blockHashCache.addAll(blockHashCache); } + public void setBlockHeightCache(Map blockHeightCache) { + this.blockHeightCache.clear(); + this.blockHeightCache.putAll(blockHeightCache); + } + public Map getTxCache() { return Collections.unmodifiableMap(txCache); } @@ -268,6 +278,8 @@ public Set getBlockHashCache() { return Collections.unmodifiableSet(blockHashCache); } + public Map getBlockHeightCache() { return Collections.unmodifiableMap(blockHeightCache); } + @Override public String toString() { return "DaoState{" + From 9977920e4c3cca84468b42aa3df8b89c51995b90 Mon Sep 17 00:00:00 2001 From: cd2357 Date: Tue, 11 May 2021 12:20:51 +0200 Subject: [PATCH 5/7] Address Codacy complaint / newline --- core/src/main/java/bisq/core/dao/state/model/DaoState.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/bisq/core/dao/state/model/DaoState.java b/core/src/main/java/bisq/core/dao/state/model/DaoState.java index 84c636a5738..8746da04d06 100644 --- a/core/src/main/java/bisq/core/dao/state/model/DaoState.java +++ b/core/src/main/java/bisq/core/dao/state/model/DaoState.java @@ -278,7 +278,9 @@ public Set getBlockHashCache() { return Collections.unmodifiableSet(blockHashCache); } - public Map getBlockHeightCache() { return Collections.unmodifiableMap(blockHeightCache); } + public Map getBlockHeightCache() { + return Collections.unmodifiableMap(blockHeightCache); + } @Override public String toString() { From b6dbe85313f0ae2fa4ddec34aa79af5a5da46545 Mon Sep 17 00:00:00 2001 From: cd2357 Date: Sat, 15 May 2021 22:34:27 +0200 Subject: [PATCH 6/7] Strengthen index integrity Update the two new indices on every known write to the list, namely addBlock() and addBlocks(). Restrict ability to modify the blocks list outside of these methods by overwriting the getBlocks() getter such that it returns an unmodifiable list. --- .../core/dao/node/parser/BlockParser.java | 9 ++- .../bisq/core/dao/state/DaoStateService.java | 19 ++--- .../dao/state/DaoStateSnapshotService.java | 7 +- .../bisq/core/dao/state/model/DaoState.java | 69 ++++++++++++++----- 4 files changed, 68 insertions(+), 36 deletions(-) diff --git a/core/src/main/java/bisq/core/dao/node/parser/BlockParser.java b/core/src/main/java/bisq/core/dao/node/parser/BlockParser.java index 8a7e9f3d450..4daa1058736 100644 --- a/core/src/main/java/bisq/core/dao/node/parser/BlockParser.java +++ b/core/src/main/java/bisq/core/dao/node/parser/BlockParser.java @@ -29,7 +29,7 @@ import javax.inject.Inject; -import java.util.LinkedList; +import java.util.List; import lombok.extern.slf4j.Slf4j; @@ -118,16 +118,15 @@ public Block parseBlock(RawBlock rawBlock) throws BlockHashNotConnectingExceptio } private void validateIfBlockIsConnecting(RawBlock rawBlock) throws BlockHashNotConnectingException, BlockHeightNotConnectingException { - LinkedList blocks = daoStateService.getBlocks(); + List blocks = daoStateService.getBlocks(); if (blocks.isEmpty()) return; - Block last = blocks.getLast(); - if (last.getHeight() + 1 != rawBlock.getHeight()) + if (daoStateService.getBlockHeightOfLastBlock() + 1 != rawBlock.getHeight()) throw new BlockHeightNotConnectingException(rawBlock); - if (!last.getHash().equals(rawBlock.getPreviousBlockHash())) + if (!daoStateService.getBlockHashOfLastBlock().equals(rawBlock.getPreviousBlockHash())) throw new BlockHashNotConnectingException(rawBlock); } diff --git a/core/src/main/java/bisq/core/dao/state/DaoStateService.java b/core/src/main/java/bisq/core/dao/state/DaoStateService.java index 230929cddce..7283ebbf9ff 100644 --- a/core/src/main/java/bisq/core/dao/state/DaoStateService.java +++ b/core/src/main/java/bisq/core/dao/state/DaoStateService.java @@ -117,10 +117,7 @@ public void applySnapshot(DaoState snapshot) { daoState.setTxCache(snapshot.getTxCache()); - daoState.getBlocks().clear(); - daoState.getBlocks().addAll(snapshot.getBlocks()); - daoState.setBlockHashCache(snapshot.getBlockHashCache()); - daoState.setBlockHeightCache(snapshot.getBlockHeightCache()); + daoState.clearAndSetBlocks(snapshot.getBlocks()); daoState.getCycles().clear(); daoState.getCycles().addAll(snapshot.getCycles()); @@ -223,7 +220,7 @@ public void onNewBlockWithEmptyTxs(Block block) { "We ignore that block as the first block need to be the genesis block. " + "That might happen in edge cases at reorgs. Received block={}", block); } else { - daoState.getBlocks().add(block); + daoState.addBlock(block); if (parseBlockChainComplete) log.info("New Block added at blockHeight {}", block.getHeight()); @@ -285,7 +282,7 @@ public void onParseBlockChainComplete() { } - public LinkedList getBlocks() { + public List getBlocks() { return daoState.getBlocks(); } @@ -297,12 +294,12 @@ public LinkedList getBlocks() { * {@code false}. */ public boolean isBlockHashKnown(String blockHash) { - return daoState.getBlockHashCache().contains(blockHash); + return daoState.getBlocksByHash().contains(blockHash); } public Optional getLastBlock() { if (!getBlocks().isEmpty()) - return Optional.of(getBlocks().getLast()); + return Optional.of(daoState.getLastBlock()); else return Optional.empty(); } @@ -311,8 +308,12 @@ public int getBlockHeightOfLastBlock() { return getLastBlock().map(Block::getHeight).orElse(0); } + public String getBlockHashOfLastBlock() { + return getLastBlock().map(Block::getHash).orElse(""); + } + public Optional getBlockAtHeight(int height) { - return Optional.ofNullable(daoState.getBlockHeightCache().get(height)); + return Optional.ofNullable(daoState.getBlocksByHeight().get(height)); } public boolean containsBlock(Block block) { diff --git a/core/src/main/java/bisq/core/dao/state/DaoStateSnapshotService.java b/core/src/main/java/bisq/core/dao/state/DaoStateSnapshotService.java index 1752119b269..f9ffb307a3e 100644 --- a/core/src/main/java/bisq/core/dao/state/DaoStateSnapshotService.java +++ b/core/src/main/java/bisq/core/dao/state/DaoStateSnapshotService.java @@ -99,7 +99,7 @@ public void maybeCreateSnapshot(Block block) { daoStateSnapshotCandidate.getChainHeight() != chainHeight; if (isSnapshotHeight(chainHeight) && !daoStateService.getBlocks().isEmpty() && - isValidHeight(daoStateService.getBlocks().getLast().getHeight()) && + isValidHeight(daoStateService.getBlockHeightOfLastBlock()) && noSnapshotCandidateOrDifferentHeight) { // At trigger event we store the latest snapshotCandidate to disc long ts = System.currentTimeMillis(); @@ -126,10 +126,9 @@ public void applySnapshot(boolean fromReorg) { DaoState persistedBsqState = daoStateStorageService.getPersistedBsqState(); LinkedList persistedDaoStateHashChain = daoStateStorageService.getPersistedDaoStateHashChain(); if (persistedBsqState != null) { - LinkedList blocks = persistedBsqState.getBlocks(); int chainHeightOfPersisted = persistedBsqState.getChainHeight(); - if (!blocks.isEmpty()) { - int heightOfLastBlock = blocks.getLast().getHeight(); + if (!persistedBsqState.getBlocks().isEmpty()) { + int heightOfLastBlock = persistedBsqState.getLastBlock().getHeight(); log.debug("applySnapshot from persistedBsqState daoState with height of last block {}", heightOfLastBlock); if (isValidHeight(heightOfLastBlock)) { if (chainHeightOfLastApplySnapshot != chainHeightOfPersisted) { diff --git a/core/src/main/java/bisq/core/dao/state/model/DaoState.java b/core/src/main/java/bisq/core/dao/state/model/DaoState.java index 8746da04d06..7757a3bcf07 100644 --- a/core/src/main/java/bisq/core/dao/state/model/DaoState.java +++ b/core/src/main/java/bisq/core/dao/state/model/DaoState.java @@ -76,7 +76,9 @@ public static DaoState getClone(DaoState daoState) { @Getter private int chainHeight; // Is set initially to genesis height - @Getter + + // We override the getter so callers can't modify the list without also updating + // the block caches and indices below private final LinkedList blocks; @Getter private final LinkedList cycles; @@ -109,9 +111,9 @@ public static DaoState getClone(DaoState daoState) { @JsonExclude private transient final Map txCache; // key is txId @JsonExclude - private transient final Map blockHeightCache; // key is block height + private transient final Map blocksByHeight; // Blocks indexed by height @JsonExclude - private transient final Set blockHashCache; + private transient final Set blocksByHash; // Blocks indexed by hash /////////////////////////////////////////////////////////////////////////////////////////// // Constructor @@ -164,11 +166,11 @@ private DaoState(int chainHeight, .flatMap(block -> block.getTxs().stream()) .collect(Collectors.toMap(Tx::getId, Function.identity(), (x, y) -> x, HashMap::new)); - blockHashCache = blocks.stream() + blocksByHash = blocks.stream() .map(Block::getHash) .collect(Collectors.toSet()); - blockHeightCache = blocks.stream() + blocksByHeight = blocks.stream() .collect(Collectors.toMap(Block::getHeight, Function.identity(), (x, y) -> x, HashMap::new)); } @@ -246,7 +248,7 @@ public byte[] getSerializedStateForHashChain() { // Reorgs are handled by rebuilding the hash chain from last snapshot. // Using the full blocks list becomes quite heavy. 7000 blocks are // about 1.4 MB and creating the hash takes 30 sec. By using just the last block we reduce the time to 7 sec. - return getBsqStateBuilderExcludingBlocks().addBlocks(getBlocks().getLast().toProtoMessage()).build().toByteArray(); + return getBsqStateBuilderExcludingBlocks().addBlocks(getLastBlock().toProtoMessage()).build().toByteArray(); } public void addToTxCache(Tx tx) { @@ -260,26 +262,57 @@ public void setTxCache(Map txCache) { this.txCache.putAll(txCache); } - public void setBlockHashCache(Set blockHashCache) { - this.blockHashCache.clear(); - this.blockHashCache.addAll(blockHashCache); + public Map getTxCache() { + return Collections.unmodifiableMap(txCache); } - public void setBlockHeightCache(Map blockHeightCache) { - this.blockHeightCache.clear(); - this.blockHeightCache.putAll(blockHeightCache); + public Set getBlocksByHash() { + return Collections.unmodifiableSet(blocksByHash); } - public Map getTxCache() { - return Collections.unmodifiableMap(txCache); + public Map getBlocksByHeight() { + return Collections.unmodifiableMap(blocksByHeight); } - public Set getBlockHashCache() { - return Collections.unmodifiableSet(blockHashCache); + /** + * @return Unmodifiable view of the list of blocks. This prevents callers from + * directly modifying the list. We need to do this to make sure the block list is only + * modified together with the corresponding caches and indices. + * + * @see #addBlock(Block) to add a single block + * @see #addBlocks(List) to add a list of blocks + * @see #clearAndSetBlocks(List) to replace existing blocks with a new list + */ + public List getBlocks() { + return Collections.unmodifiableList(blocks); } - public Map getBlockHeightCache() { - return Collections.unmodifiableMap(blockHeightCache); + // Wrapper that directly accesses the LinkedList, such that we don't have to expose + // the LinkedList + public Block getLastBlock() { + return blocks.getLast(); + } + + public void addBlock(Block block) { + blocks.add(block); + blocksByHash.add(block.getHash()); + blocksByHeight.put(block.getHeight(), block); + } + + public void addBlocks(List newBlocks) { + newBlocks.forEach(b -> addBlock(b)); + } + + /** + * Clears the existing block list and caches, and repopulates them with the new list + * @param newBlocks + */ + public void clearAndSetBlocks(List newBlocks) { + blocks.clear(); + blocksByHeight.clear(); + blocksByHash.clear(); + + addBlocks(newBlocks); } @Override From dd803b0ef6be4cfb8ff69a7ae10cd039cf0effd3 Mon Sep 17 00:00:00 2001 From: cd2357 Date: Mon, 17 May 2021 08:38:26 +0200 Subject: [PATCH 7/7] Rename variable for clarity Rename blocksByHash because it is not actually indexing blocks by hashes, instead it's a cache of known hashes. --- .../java/bisq/core/dao/state/DaoStateService.java | 2 +- .../java/bisq/core/dao/state/model/DaoState.java | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/bisq/core/dao/state/DaoStateService.java b/core/src/main/java/bisq/core/dao/state/DaoStateService.java index 7283ebbf9ff..2cfd0fbdb05 100644 --- a/core/src/main/java/bisq/core/dao/state/DaoStateService.java +++ b/core/src/main/java/bisq/core/dao/state/DaoStateService.java @@ -294,7 +294,7 @@ public List getBlocks() { * {@code false}. */ public boolean isBlockHashKnown(String blockHash) { - return daoState.getBlocksByHash().contains(blockHash); + return daoState.getBlockHashes().contains(blockHash); } public Optional getLastBlock() { diff --git a/core/src/main/java/bisq/core/dao/state/model/DaoState.java b/core/src/main/java/bisq/core/dao/state/model/DaoState.java index 7757a3bcf07..4388b77ab6a 100644 --- a/core/src/main/java/bisq/core/dao/state/model/DaoState.java +++ b/core/src/main/java/bisq/core/dao/state/model/DaoState.java @@ -113,7 +113,7 @@ public static DaoState getClone(DaoState daoState) { @JsonExclude private transient final Map blocksByHeight; // Blocks indexed by height @JsonExclude - private transient final Set blocksByHash; // Blocks indexed by hash + private transient final Set blockHashes; // Cache of known block hashes /////////////////////////////////////////////////////////////////////////////////////////// // Constructor @@ -166,7 +166,7 @@ private DaoState(int chainHeight, .flatMap(block -> block.getTxs().stream()) .collect(Collectors.toMap(Tx::getId, Function.identity(), (x, y) -> x, HashMap::new)); - blocksByHash = blocks.stream() + blockHashes = blocks.stream() .map(Block::getHash) .collect(Collectors.toSet()); @@ -266,8 +266,8 @@ public Map getTxCache() { return Collections.unmodifiableMap(txCache); } - public Set getBlocksByHash() { - return Collections.unmodifiableSet(blocksByHash); + public Set getBlockHashes() { + return Collections.unmodifiableSet(blockHashes); } public Map getBlocksByHeight() { @@ -295,7 +295,7 @@ public Block getLastBlock() { public void addBlock(Block block) { blocks.add(block); - blocksByHash.add(block.getHash()); + blockHashes.add(block.getHash()); blocksByHeight.put(block.getHeight(), block); } @@ -310,7 +310,7 @@ public void addBlocks(List newBlocks) { public void clearAndSetBlocks(List newBlocks) { blocks.clear(); blocksByHeight.clear(); - blocksByHash.clear(); + blockHashes.clear(); addBlocks(newBlocks); }