Encapsulate mutable Block tx list and DaoState tx cache

Avoid mutating the Block tx list or the DaoState tx cache/index via a Lombok getter. Instead wrap each in an unmodifiable[List|Map] & provide specific mutator methods for use by DaoStateService to add newly parsed transactions or load a DAO snapshot. Also rename txMap to txCache, replace remaining use of getTxStream() in the JSON file exporter with getUnorderedTxStream() (as this is safe) and swap the arguments of the txCache initialisation merge function, for exact consistency with the pre-caching behaviour. Finally, add a missing assertDaoStateChange() and remove a potentially harmful assertion from DaoStateService.onNewTxForLastBlock. This is based on a suggested patch by @chimp1984 in the PR #3773 review.
bisq-network · Dec 12, 2019 · b22e4ad · b22e4ad
1 parent 4277cf8
commit b22e4ad
Show file tree

Hide file tree

Showing 7 changed files with 56 additions and 35 deletions.
diff --git a/core/src/main/java/bisq/core/dao/DaoFacade.java b/core/src/main/java/bisq/core/dao/DaoFacade.java
@@ -94,7 +94,6 @@
 import java.io.IOException;
 
 import java.util.ArrayList;
-import java.util.Collection;
 import java.util.List;
 import java.util.Optional;
 import java.util.Set;
@@ -625,9 +624,8 @@ public Set<TxOutput> getUnspentTxOutputs() {
         return daoStateService.getUnspentTxOutputs();
     }
 
-    // Returns a view rather than a copy of all the txs.
-    public Collection<Tx> getTxs() {
-        return daoStateService.getTxs();
+    public int getNumTxs() {
+        return daoStateService.getNumTxs();
     }
 
     public Optional<TxOutput> getLockupTxOutput(String txId) {

diff --git a/core/src/main/java/bisq/core/dao/node/explorer/ExportJsonFilesService.java b/core/src/main/java/bisq/core/dao/node/explorer/ExportJsonFilesService.java
@@ -140,7 +140,7 @@ public void maybeExportToJson() {
             // Access to daoStateService is single threaded, we must not access daoStateService from the thread.
             List<JsonTxOutput> allJsonTxOutputs = new ArrayList<>();
 
-            List<JsonTx> jsonTxs = daoStateService.getTxStream()
+            List<JsonTx> jsonTxs = daoStateService.getUnorderedTxStream()
                     .map(tx -> {
                         JsonTx jsonTx = getJsonTx(tx);
                         allJsonTxOutputs.addAll(jsonTx.getOutputs());

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
@@ -53,7 +53,6 @@ public class BlockParser {
     // Constructor
     ///////////////////////////////////////////////////////////////////////////////////////////
 
-    @SuppressWarnings("WeakerAccess")
     @Inject
     public BlockParser(TxParser txParser,
                        DaoStateService daoStateService) {

diff --git a/core/src/main/java/bisq/core/dao/state/DaoStateService.java b/core/src/main/java/bisq/core/dao/state/DaoStateService.java
@@ -42,11 +42,7 @@
 
 import javax.inject.Inject;
 
-import com.google.common.base.Preconditions;
-
 import java.util.ArrayList;
-import java.util.Collection;
-import java.util.Collections;
 import java.util.Comparator;
 import java.util.HashSet;
 import java.util.LinkedList;
@@ -119,8 +115,7 @@ public void applySnapshot(DaoState snapshot) {
 
         daoState.setChainHeight(snapshot.getChainHeight());
 
-        daoState.getTxMap().clear();
-        daoState.getTxMap().putAll(snapshot.getTxMap());
+        daoState.setTxCache(snapshot.getTxCache());
 
         daoState.getBlocks().clear();
         daoState.getBlocks().addAll(snapshot.getBlocks());
@@ -235,11 +230,20 @@ public void onNewBlockWithEmptyTxs(Block block) {
 
     // Third we add each successfully parsed BSQ tx to the last block
     public void onNewTxForLastBlock(Block block, Tx tx) {
-        // At least one block must be present else no rawTx would have been recognised as a BSQ tx.
-        Preconditions.checkArgument(block == getLastBlock().orElseThrow());
+        assertDaoStateChange();
 
-        block.getTxs().add(tx);
-        daoState.getTxMap().put(tx.getId(), tx);
+        getLastBlock().ifPresent(lastBlock -> {
+            if (block == lastBlock) {
+                // We need to ensure that the txs in all blocks are in sync with the txs in our txMap (cache).
+                block.addTx(tx);
+                daoState.addToTxCache(tx);
+            } else {
+                // Not clear if this case can happen but at onNewBlockWithEmptyTxs we handle such a potential edge
+                // case as well, so we need to reflect that here as well.
+                log.warn("Block for parsing does not match last block. That might happen in edge cases at reorgs. " +
+                        "Received block={}", block);
+            }
+        });
     }
 
     // Fourth we get the onParseBlockComplete called after all rawTxs of blocks have been parsed
@@ -359,17 +363,12 @@ public Optional<Tx> getGenesisTx() {
     // Tx
     ///////////////////////////////////////////////////////////////////////////////////////////
 
-    public Stream<Tx> getTxStream() {
-        return getBlocks().stream()
-                .flatMap(block -> block.getTxs().stream());
-    }
-
-    private Stream<Tx> getUnorderedTxStream() {
-        return getTxs().stream();
+    public Stream<Tx> getUnorderedTxStream() {
+        return daoState.getTxCache().values().stream();
     }
 
-    public Collection<Tx> getTxs() {
-        return Collections.unmodifiableCollection(daoState.getTxMap().values());
+    public int getNumTxs() {
+        return daoState.getTxCache().size();
     }
 
     public List<Tx> getInvalidTxs() {
@@ -381,7 +380,7 @@ public List<Tx> getIrregularTxs() {
     }
 
     public Optional<Tx> getTx(String txId) {
-        return Optional.ofNullable(daoState.getTxMap().get(txId));
+        return Optional.ofNullable(daoState.getTxCache().get(txId));
     }
 
     public boolean containsTx(String txId) {

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
@@ -36,6 +36,7 @@
 import javax.inject.Inject;
 
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.LinkedList;
 import java.util.List;
@@ -47,7 +48,6 @@
 import lombok.Getter;
 import lombok.extern.slf4j.Slf4j;
 
-
 /**
  * Root class for mutable state of the DAO.
  * Holds both blockchain data as well as data derived from the governance process (voting).
@@ -103,9 +103,8 @@ public static DaoState getClone(DaoState daoState) {
     private final List<DecryptedBallotsWithMerits> decryptedBallotsWithMeritsList;
 
     // Transient data used only as an index - must be kept in sync with the block list
-    @Getter
     @JsonExclude
-    private transient final Map<String, Tx> txMap; // key is txId
+    private transient final Map<String, Tx> txCache; // key is txId
 
 
     ///////////////////////////////////////////////////////////////////////////////////////////
@@ -155,9 +154,9 @@ private DaoState(int chainHeight,
         this.evaluatedProposalList = evaluatedProposalList;
         this.decryptedBallotsWithMeritsList = decryptedBallotsWithMeritsList;
 
-        txMap = blocks.stream()
+        txCache = blocks.stream()
                 .flatMap(block -> block.getTxs().stream())
-                .collect(Collectors.toMap(Tx::getId, Function.identity(), (x, y) -> y, HashMap::new));
+                .collect(Collectors.toMap(Tx::getId, Function.identity(), (x, y) -> x, HashMap::new));
     }
 
     @Override
@@ -237,6 +236,21 @@ public byte[] getSerializedStateForHashChain() {
         return getBsqStateBuilderExcludingBlocks().addBlocks(getBlocks().getLast().toProtoMessage()).build().toByteArray();
     }
 
+    public void addToTxCache(Tx tx) {
+        // We shouldn't get duplicate txIds, but use putIfAbsent instead of put for consistency with the map merge
+        // function used in the constructor to initialise txCache (and to exactly match the pre-caching behaviour).
+        txCache.putIfAbsent(tx.getId(), tx);
+    }
+
+    public void setTxCache(Map<String, Tx> txCache) {
+        this.txCache.clear();
+        this.txCache.putAll(txCache);
+    }
+
+    public Map<String, Tx> getTxCache() {
+        return Collections.unmodifiableMap(txCache);
+    }
+
     @Override
     public String toString() {
         return "DaoState{" +
@@ -250,7 +264,7 @@ public String toString() {
                 ",\n     paramChangeList=" + paramChangeList +
                 ",\n     evaluatedProposalList=" + evaluatedProposalList +
                 ",\n     decryptedBallotsWithMeritsList=" + decryptedBallotsWithMeritsList +
-                ",\n     txMap=" + txMap +
+                ",\n     txCache=" + txCache +
                 "\n}";
     }
 }
diff --git a/core/src/main/java/bisq/core/dao/state/model/blockchain/Block.java b/core/src/main/java/bisq/core/dao/state/model/blockchain/Block.java
@@ -24,11 +24,11 @@
 import com.google.common.collect.ImmutableList;
 
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
 import java.util.stream.Collectors;
 
 import lombok.EqualsAndHashCode;
-import lombok.Value;
 
 /**
  * The Block which gets persisted in the DaoState. During parsing transactions can be
@@ -44,8 +44,8 @@
  *
  */
 @EqualsAndHashCode(callSuper = true)
-@Value
 public final class Block extends BaseBlock implements PersistablePayload, ImmutableDaoStateModel {
+    // We do not expose txs with a Lombok getter. We cannot make it immutable as we add transactions during parsing.
     private final List<Tx> txs;
 
     public Block(int height, long time, String hash, String previousBlockHash) {
@@ -93,6 +93,17 @@ public static Block fromProto(protobuf.BaseBlock proto) {
                 txs);
     }
 
+    public void addTx(Tx tx) {
+        txs.add(tx);
+    }
+
+    // We want to guarantee that no client can modify the list. We use unmodifiableList and not ImmutableList as
+    // we want that clients reflect any change to the source list. Also ImmutableList is more expensive as it
+    // creates a copy.
+    public List<Tx> getTxs() {
+        return Collections.unmodifiableList(txs);
+    }
+
     @Override
     public String toString() {
         return "Block{" +

diff --git a/desktop/src/main/java/bisq/desktop/main/dao/economy/transactions/BSQTransactionsView.java b/desktop/src/main/java/bisq/desktop/main/dao/economy/transactions/BSQTransactionsView.java
@@ -143,7 +143,7 @@ public void onParseBlockCompleteAfterBatchProcessing(Block block) {
     ///////////////////////////////////////////////////////////////////////////////////////////
 
     private void updateWithBsqBlockChainData() {
-        allTxTextField.setText(String.valueOf(daoFacade.getTxs().size()));
+        allTxTextField.setText(String.valueOf(daoFacade.getNumTxs()));
         utxoTextField.setText(String.valueOf(daoFacade.getUnspentTxOutputs().size()));
         compensationIssuanceTxTextField.setText(String.valueOf(daoFacade.getNumIssuanceTransactions(IssuanceType.COMPENSATION)));
         reimbursementIssuanceTxTextField.setText(String.valueOf(daoFacade.getNumIssuanceTransactions(IssuanceType.REIMBURSEMENT)));