From de654c15b38108a5abd5c0dc47e3b555b6a1755c Mon Sep 17 00:00:00 2001 From: HenrikJannsen Date: Thu, 17 Nov 2022 19:41:32 -0500 Subject: [PATCH 1/5] Improve handling of ConnectionState.expectedInitialDataResponses Rename to expectedInitialDataResponses as we compare with numInitialDataResponses. Add comment to make it more clear how its used. Let the LiteNode increment only if getBlocks get called (e.g. blocks are missing). Signed-off-by: HenrikJannsen --- .../java/bisq/core/dao/node/full/FullNode.java | 2 -- .../java/bisq/core/dao/node/lite/LiteNode.java | 4 ++++ .../network/p2p/network/ConnectionState.java | 17 +++++++++++------ 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/bisq/core/dao/node/full/FullNode.java b/core/src/main/java/bisq/core/dao/node/full/FullNode.java index a35164c6606..7e15d9a44d0 100644 --- a/core/src/main/java/bisq/core/dao/node/full/FullNode.java +++ b/core/src/main/java/bisq/core/dao/node/full/FullNode.java @@ -30,7 +30,6 @@ import bisq.core.dao.state.model.blockchain.Block; import bisq.network.p2p.P2PService; -import bisq.network.p2p.network.ConnectionState; import bisq.common.UserThread; import bisq.common.handlers.ResultHandler; @@ -76,7 +75,6 @@ private FullNode(BlockParser blockParser, this.rpcService = rpcService; this.fullNodeNetworkService = fullNodeNetworkService; - ConnectionState.setExpectedRequests(5); } diff --git a/core/src/main/java/bisq/core/dao/node/lite/LiteNode.java b/core/src/main/java/bisq/core/dao/node/lite/LiteNode.java index ebdee7a3d70..6d4fa40f3c0 100644 --- a/core/src/main/java/bisq/core/dao/node/lite/LiteNode.java +++ b/core/src/main/java/bisq/core/dao/node/lite/LiteNode.java @@ -32,6 +32,7 @@ import bisq.network.p2p.P2PService; import bisq.network.p2p.network.Connection; +import bisq.network.p2p.network.ConnectionState; import bisq.common.Timer; import bisq.common.UserThread; @@ -191,6 +192,9 @@ protected void startParseBlocks() { return; } + // If we request blocks we increment the ConnectionState counter. + ConnectionState.incrementExpectedInitialDataResponses(); + if (chainHeight == daoStateService.getGenesisBlockHeight()) { liteNodeNetworkService.requestBlocks(chainHeight); } else { diff --git a/p2p/src/main/java/bisq/network/p2p/network/ConnectionState.java b/p2p/src/main/java/bisq/network/p2p/network/ConnectionState.java index b92eee32aa8..fa3af3322df 100644 --- a/p2p/src/main/java/bisq/network/p2p/network/ConnectionState.java +++ b/p2p/src/main/java/bisq/network/p2p/network/ConnectionState.java @@ -45,10 +45,15 @@ public class ConnectionState implements MessageListener { private static final long PEER_RESET_TIMER_DELAY_SEC = TimeUnit.MINUTES.toSeconds(4); private static final long COMPLETED_TIMER_DELAY_SEC = 10; - // Number of expected requests in standard case. Can be different according to network conditions. - // Is different for LiteDaoNodes and FullDaoNodes - @Setter - private static int expectedRequests = 6; + // We have 2 GetDataResponses and 3 GetHashResponses. If node is a lite node it also has a GetBlocksResponse if + // blocks are missing. + private static final int MIN_EXPECTED_RESPONSES = 5; + private static int expectedInitialDataResponses = MIN_EXPECTED_RESPONSES; + + // If app runs in LiteNode mode there is one more expected request for the getBlocks request, so we increment standard value. + public static void incrementExpectedInitialDataResponses() { + expectedInitialDataResponses += 1; + } private final Connection connection; @@ -124,7 +129,7 @@ private void onInitialDataExchange() { } private void maybeResetInitialDataExchangeType() { - if (numInitialDataResponses >= expectedRequests) { + if (numInitialDataResponses >= expectedInitialDataResponses) { // We have received the expected messages from initial data requests. We delay a bit the reset // to give time for processing the response and more tolerance to edge cases where we expect more responses. // Reset to PEER does not mean disconnection as well, but just that this connection has lower priority and @@ -168,7 +173,7 @@ public String toString() { ",\n numInitialDataResponses=" + numInitialDataResponses + ",\n lastInitialDataMsgTimeStamp=" + lastInitialDataMsgTimeStamp + ",\n isSeedNode=" + isSeedNode + - ",\n expectedRequests=" + expectedRequests + + ",\n expectedRequests=" + expectedInitialDataResponses + "\n}"; } } From 1e72e265f7b6f8541022249d575d6fb6ddfbdbfc Mon Sep 17 00:00:00 2001 From: HenrikJannsen Date: Thu, 17 Nov 2022 19:44:58 -0500 Subject: [PATCH 2/5] Cleanup comments Improve toString methods Rename var for more clear meaning Signed-off-by: HenrikJannsen --- core/src/main/java/bisq/core/dao/node/BsqNode.java | 10 ++++++---- .../main/java/bisq/core/dao/node/full/FullNode.java | 7 +------ .../main/java/bisq/core/dao/node/full/RawBlock.java | 8 ++++++-- core/src/main/java/bisq/core/dao/node/full/RawTx.java | 10 ++++++++-- .../java/bisq/core/dao/node/full/RawTxOutput.java | 11 ++++++++++- 5 files changed, 31 insertions(+), 15 deletions(-) diff --git a/core/src/main/java/bisq/core/dao/node/BsqNode.java b/core/src/main/java/bisq/core/dao/node/BsqNode.java index 5262b125fd3..6464b26d658 100644 --- a/core/src/main/java/bisq/core/dao/node/BsqNode.java +++ b/core/src/main/java/bisq/core/dao/node/BsqNode.java @@ -232,6 +232,7 @@ protected Optional doParseBlock(RawBlock rawBlock) throws RequiredReorgFr int heightForNextBlock = daoStateService.getChainHeight() + 1; if (rawBlock.getHeight() > heightForNextBlock) { + // rawBlock is not at expected next height but further in the future if (!pendingBlocks.contains(rawBlock)) { pendingBlocks.add(rawBlock); log.info("We received a block with a future block height. We store it as pending and try to apply " + @@ -240,16 +241,17 @@ protected Optional doParseBlock(RawBlock rawBlock) throws RequiredReorgFr log.warn("We received a block with a future block height but we had it already added to our pendingBlocks."); } } else if (rawBlock.getHeight() >= daoStateService.getGenesisBlockHeight()) { + // rawBlock is not expected next height but either same height as chainHead or in the past // We received an older block. We compare if we have it in our chain. - Optional optionalBlock = daoStateService.getBlockAtHeight(rawBlock.getHeight()); - if (optionalBlock.isPresent()) { - if (optionalBlock.get().getHash().equals(rawBlock.getPreviousBlockHash())) { + Optional existingBlockAsSameHeight = daoStateService.getBlockAtHeight(rawBlock.getHeight()); + if (existingBlockAsSameHeight.isPresent()) { + if (existingBlockAsSameHeight.get().getHash().equals(rawBlock.getPreviousBlockHash())) { log.info("We received an old block we have already parsed and added. We ignore it."); } else { log.info("We received an old block with a different hash. We ignore it. Hash={}", rawBlock.getHash()); } } else { - log.info("In case we have reset from genesis height we would not find the block"); + log.info("In case we have reset from genesis height we would not find the existingBlockAsSameHeight"); } } else { log.info("We ignore it as it was before genesis height"); diff --git a/core/src/main/java/bisq/core/dao/node/full/FullNode.java b/core/src/main/java/bisq/core/dao/node/full/FullNode.java index 7e15d9a44d0..0e8470b5ce2 100644 --- a/core/src/main/java/bisq/core/dao/node/full/FullNode.java +++ b/core/src/main/java/bisq/core/dao/node/full/FullNode.java @@ -108,8 +108,6 @@ protected void startParseBlocks() { int startBlockHeight = daoStateService.getChainHeight(); log.info("startParseBlocks: startBlockHeight={}", startBlockHeight); rpcService.requestChainHeadHeight(chainHeight -> { - // If our persisted block is equal to the chain height we have startBlockHeight 1 block higher, - // so we do not call parseBlocksOnHeadHeight log.info("startParseBlocks: chainHeight={}", chainHeight); if (startBlockHeight <= chainHeight) { parseBlocksOnHeadHeight(startBlockHeight, chainHeight); @@ -203,11 +201,8 @@ private void parseBlocksOnHeadHeight(int startBlockHeight, int chainHeight) { chainHeight, this::onNewBlock, () -> { - // We are done but it might be that new blocks have arrived in the meantime, + // We are done, but it might be that new blocks have arrived in the meantime, // so we try again with startBlockHeight set to current chainHeight - // We also set up the listener in the else main branch where we check - // if we are at chainTip, so do not include here another check as it would - // not trigger the listener registration. parseBlocksIfNewBlockAvailable(chainHeight); }, this::handleError); } else { diff --git a/core/src/main/java/bisq/core/dao/node/full/RawBlock.java b/core/src/main/java/bisq/core/dao/node/full/RawBlock.java index 686ecb2dbe7..9c3954e6765 100644 --- a/core/src/main/java/bisq/core/dao/node/full/RawBlock.java +++ b/core/src/main/java/bisq/core/dao/node/full/RawBlock.java @@ -99,7 +99,11 @@ public static RawBlock fromProto(protobuf.BaseBlock proto) { @Override public String toString() { return "RawBlock{" + - "\n rawTxs=" + rawTxs + - "\n} " + super.toString(); + "\n height=" + height + + ",\n time=" + time + + ",\n hash='" + hash + '\'' + + ",\n previousBlockHash='" + previousBlockHash + '\'' + + ",\n rawTxs=" + rawTxs + + "\n}"; } } diff --git a/core/src/main/java/bisq/core/dao/node/full/RawTx.java b/core/src/main/java/bisq/core/dao/node/full/RawTx.java index bd291a0cd49..552c12b27e7 100644 --- a/core/src/main/java/bisq/core/dao/node/full/RawTx.java +++ b/core/src/main/java/bisq/core/dao/node/full/RawTx.java @@ -132,7 +132,13 @@ public static RawTx fromProto(protobuf.BaseTx protoBaseTx) { @Override public String toString() { return "RawTx{" + - "\n rawTxOutputs=" + rawTxOutputs + - "\n} " + super.toString(); + "\n txVersion='" + txVersion + '\'' + + ",\n id='" + id + '\'' + + ",\n blockHeight=" + blockHeight + + ",\n blockHash='" + blockHash + '\'' + + ",\n time=" + time + + ",\n txInputs=" + txInputs + + ",\n rawTxOutputs=" + rawTxOutputs + + "\n }"; } } diff --git a/core/src/main/java/bisq/core/dao/node/full/RawTxOutput.java b/core/src/main/java/bisq/core/dao/node/full/RawTxOutput.java index 2d5599b2fbb..a3c17efb1b8 100644 --- a/core/src/main/java/bisq/core/dao/node/full/RawTxOutput.java +++ b/core/src/main/java/bisq/core/dao/node/full/RawTxOutput.java @@ -22,6 +22,7 @@ import bisq.core.dao.state.model.blockchain.TxOutput; import bisq.common.proto.network.NetworkPayload; +import bisq.common.util.Utilities; import lombok.EqualsAndHashCode; import lombok.Value; @@ -88,6 +89,14 @@ public static RawTxOutput fromProto(protobuf.BaseTxOutput proto) { @Override public String toString() { - return "RawTxOutput{} " + super.toString(); + return "RawTxOutput{" + + "\n index=" + index + + ",\n value=" + value + + ",\n txId='" + txId + '\'' + + ",\n pubKeyScript=" + pubKeyScript + + ",\n address='" + address + '\'' + + ",\n opReturnData=" + Utilities.bytesAsHexString(opReturnData) + + ",\n blockHeight=" + blockHeight + + "\n }"; } } From 811ae52f278c57df66bc7acb5dd0630eb583c738 Mon Sep 17 00:00:00 2001 From: HenrikJannsen Date: Thu, 17 Nov 2022 19:46:38 -0500 Subject: [PATCH 3/5] Fix bug with using getPreviousBlockHash instead of getHash It had no consequences as it was only used for info logging Signed-off-by: HenrikJannsen --- core/src/main/java/bisq/core/dao/node/BsqNode.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/bisq/core/dao/node/BsqNode.java b/core/src/main/java/bisq/core/dao/node/BsqNode.java index 6464b26d658..0b2b606fac6 100644 --- a/core/src/main/java/bisq/core/dao/node/BsqNode.java +++ b/core/src/main/java/bisq/core/dao/node/BsqNode.java @@ -245,7 +245,7 @@ protected Optional doParseBlock(RawBlock rawBlock) throws RequiredReorgFr // We received an older block. We compare if we have it in our chain. Optional existingBlockAsSameHeight = daoStateService.getBlockAtHeight(rawBlock.getHeight()); if (existingBlockAsSameHeight.isPresent()) { - if (existingBlockAsSameHeight.get().getHash().equals(rawBlock.getPreviousBlockHash())) { + if (existingBlockAsSameHeight.get().getHash().equals(rawBlock.getHash())) { log.info("We received an old block we have already parsed and added. We ignore it."); } else { log.info("We received an old block with a different hash. We ignore it. Hash={}", rawBlock.getHash()); From 0f14ea29c9c07803be28ac8925dce8e1cb260e9b Mon Sep 17 00:00:00 2001 From: HenrikJannsen Date: Thu, 17 Nov 2022 19:51:15 -0500 Subject: [PATCH 4/5] Improve handling of case when Bitcoin Core sync is not completed. We repeat with a quadratically increasing delay 5 times, then we give up. In the previous code we repeated forever which could be risky in case that code branch is called unexpectedly. Signed-off-by: HenrikJannsen --- .../bisq/core/dao/node/full/FullNode.java | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/bisq/core/dao/node/full/FullNode.java b/core/src/main/java/bisq/core/dao/node/full/FullNode.java index 0e8470b5ce2..9d769bc3e32 100644 --- a/core/src/main/java/bisq/core/dao/node/full/FullNode.java +++ b/core/src/main/java/bisq/core/dao/node/full/FullNode.java @@ -57,6 +57,7 @@ public class FullNode extends BsqNode { private boolean addBlockHandlerAdded; private int blocksToParseInBatch; private long parseInBatchStartTime; + private int parseBlocksOnHeadHeightCounter; /////////////////////////////////////////////////////////////////////////////////////////// @@ -206,12 +207,19 @@ private void parseBlocksOnHeadHeight(int startBlockHeight, int chainHeight) { parseBlocksIfNewBlockAvailable(chainHeight); }, this::handleError); } else { - log.warn("We are trying to start with a block which is above the chain height of Bitcoin Core. " + - "We need probably wait longer until Bitcoin Core has fully synced. " + - "We try again after a delay of 1 min."); - UserThread.runAfter(() -> rpcService.requestChainHeadHeight(chainHeight1 -> - parseBlocksOnHeadHeight(startBlockHeight, chainHeight1), - this::handleError), 60); + parseBlocksOnHeadHeightCounter++; + if (parseBlocksOnHeadHeightCounter <= 5) { + log.warn("We are trying to start with a block which is above the chain height of Bitcoin Core. " + + "We need to wait longer until Bitcoin Core has fully synced. " + + "We try again after a delay of {} min.", parseBlocksOnHeadHeightCounter * parseBlocksOnHeadHeightCounter); + UserThread.runAfter(() -> rpcService.requestChainHeadHeight(height -> + parseBlocksOnHeadHeight(startBlockHeight, height), + this::handleError), parseBlocksOnHeadHeightCounter * parseBlocksOnHeadHeightCounter * 60L); + } else { + log.warn("We tried {} times to start with startBlockHeight {} which is above the chain height {} of Bitcoin Core. " + + "It might be that Bitcoin Core has not fully synced. We give up now.", + parseBlocksOnHeadHeightCounter, startBlockHeight, chainHeight); + } } } From 671ab1f373e21d98238b17b0dc9fa2d70dcf081d Mon Sep 17 00:00:00 2001 From: HenrikJannsen Date: Thu, 17 Nov 2022 19:55:04 -0500 Subject: [PATCH 5/5] Cleanup Improve toString methods Signed-off-by: HenrikJannsen --- .../bisq/core/dao/state/model/blockchain/TxInput.java | 8 ++++---- .../java/bisq/network/p2p/network/ConnectionState.java | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/bisq/core/dao/state/model/blockchain/TxInput.java b/core/src/main/java/bisq/core/dao/state/model/blockchain/TxInput.java index 0ebcddf937f..044752f5c50 100644 --- a/core/src/main/java/bisq/core/dao/state/model/blockchain/TxInput.java +++ b/core/src/main/java/bisq/core/dao/state/model/blockchain/TxInput.java @@ -83,9 +83,9 @@ public TxOutputKey getConnectedTxOutputKey() { @Override public String toString() { return "TxInput{" + - "\n connectedTxOutputTxId='" + connectedTxOutputTxId + '\'' + - ",\n connectedTxOutputIndex=" + connectedTxOutputIndex + - ",\n pubKey=" + pubKey + - "\n}"; + "\n connectedTxOutputTxId='" + connectedTxOutputTxId + '\'' + + ",\n connectedTxOutputIndex=" + connectedTxOutputIndex + + ",\n pubKey=" + pubKey + + "\n }"; } } diff --git a/p2p/src/main/java/bisq/network/p2p/network/ConnectionState.java b/p2p/src/main/java/bisq/network/p2p/network/ConnectionState.java index fa3af3322df..a30586707a4 100644 --- a/p2p/src/main/java/bisq/network/p2p/network/ConnectionState.java +++ b/p2p/src/main/java/bisq/network/p2p/network/ConnectionState.java @@ -173,7 +173,7 @@ public String toString() { ",\n numInitialDataResponses=" + numInitialDataResponses + ",\n lastInitialDataMsgTimeStamp=" + lastInitialDataMsgTimeStamp + ",\n isSeedNode=" + isSeedNode + - ",\n expectedRequests=" + expectedInitialDataResponses + + ",\n expectedInitialDataResponses=" + expectedInitialDataResponses + "\n}"; } }