From 1c5ab8502b92487fdb7db59f98c8ecf0fbbb90c6 Mon Sep 17 00:00:00 2001 From: Marcos Date: Tue, 7 Feb 2023 11:06:44 -0300 Subject: [PATCH 1/3] Skip updateBridge if there is no federation in BtcToRskClient --- .../java/co/rsk/federate/BtcToRskClient.java | 12 ++++++---- .../co/rsk/federate/BtcToRskClientTest.java | 24 ++++++++++++++++--- 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/src/main/java/co/rsk/federate/BtcToRskClient.java b/src/main/java/co/rsk/federate/BtcToRskClient.java index 2d4ee5e8..4892802f 100644 --- a/src/main/java/co/rsk/federate/BtcToRskClient.java +++ b/src/main/java/co/rsk/federate/BtcToRskClient.java @@ -115,7 +115,7 @@ public synchronized void setup( } public void start(Federation federation) { - logger.info("Starting for Federation {}", federation.getAddress().toString()); + logger.info("Starting for Federation {}", federation.getAddress()); this.federation = federation; if (federation.isMember(federatorSupport.getFederationMember())) { logger.info("Watching federation {} since I belong to it", federation.getAddress().toString()); @@ -153,8 +153,12 @@ public synchronized Map> getTransactionsToSendToRsk() { } public void updateBridge() { - if (federation == null) { - logger.warn("[updateBridge] updateBridge skipped because no Federation is associated to this BtcToRskClient"); + if (federation == null || nodeBlockProcessor.hasBetterBlockToSync()) { + String reason = federation == null ? + "no Federation is associated to this BtcToRskClient" : + "the node is syncing blocks"; + logger.warn("[updateBridge] updateBridge skipped because {}", reason); + return; } if (nodeBlockProcessor.hasBetterBlockToSync()) { logger.warn("[updateBridge] updateBridge skipped because the node is syncing blocks"); @@ -194,7 +198,7 @@ public void updateBridge() { logger.debug("[updateBridge] Sending updateCollections"); federatorSupport.sendUpdateCollections(); } catch (Exception e) { - logger.error(e.getMessage(), e); + logger.error("[updateBridge] Error while updating Bridge. {}", e.getMessage()); panicProcessor.panic("btclock", e.getMessage()); } } diff --git a/src/test/java/co/rsk/federate/BtcToRskClientTest.java b/src/test/java/co/rsk/federate/BtcToRskClientTest.java index 401c11fd..db78ac4d 100644 --- a/src/test/java/co/rsk/federate/BtcToRskClientTest.java +++ b/src/test/java/co/rsk/federate/BtcToRskClientTest.java @@ -2233,8 +2233,7 @@ public void updateBridgeBtcCoinbaseTransactions_when_coinbase_map_has_readyToBeI } @Test - public void updateBridgeBtcCoinbaseTransactions_not_removing_from_storage_until_confirmation() - throws Exception { + public void updateBridgeBtcCoinbaseTransactions_not_removing_from_storage_until_confirmation() throws Exception { Sha256Hash blockHash = Sha256Hash.ZERO_HASH; ActivationConfig activations = mock(ActivationConfig.class); @@ -2282,7 +2281,26 @@ public void updateBridge_when_hasBetterBlockToSync_does_not_update_headers() thr NodeBlockProcessor nodeBlockProcessor = mock(NodeBlockProcessor.class); when(nodeBlockProcessor.hasBetterBlockToSync()).thenReturn(true); - BtcToRskClient btcToRskClient = spy(buildWithFactory(mock(FederatorSupport.class), nodeBlockProcessor)); + BtcToRskClient btcToRskClient = spy(buildWithFactory( + mock(FederatorSupport.class), + nodeBlockProcessor + )); + btcToRskClient.start(mock(Federation.class)); // Ensure the federation is not null + + btcToRskClient.updateBridge(); + + verify(btcToRskClient, never()).updateBridgeBtcBlockchain(); + } + + @Test + public void updateBridge_when_federation_is_null_does_not_update_headers() throws IOException, BlockStoreException { + NodeBlockProcessor nodeBlockProcessor = mock(NodeBlockProcessor.class); + when(nodeBlockProcessor.hasBetterBlockToSync()).thenReturn(false); + + BtcToRskClient btcToRskClient = spy(buildWithFactory( + mock(FederatorSupport.class), + nodeBlockProcessor + )); btcToRskClient.updateBridge(); From d2dd1bbf4665392aafebf26b87e796f5a00810b4 Mon Sep 17 00:00:00 2001 From: Marcos Date: Tue, 7 Feb 2023 14:52:31 -0300 Subject: [PATCH 2/3] Put method name in log messages in BtcToRskClient class --- .../java/co/rsk/federate/BtcToRskClient.java | 86 +++++++++++-------- 1 file changed, 52 insertions(+), 34 deletions(-) diff --git a/src/main/java/co/rsk/federate/BtcToRskClient.java b/src/main/java/co/rsk/federate/BtcToRskClient.java index 4892802f..9e30f022 100644 --- a/src/main/java/co/rsk/federate/BtcToRskClient.java +++ b/src/main/java/co/rsk/federate/BtcToRskClient.java @@ -1,12 +1,16 @@ package co.rsk.federate; +import static com.google.common.base.Preconditions.checkNotNull; + import co.rsk.bitcoinj.core.BtcTransaction; import co.rsk.config.BridgeConstants; import co.rsk.federate.adapter.ThinConverter; import co.rsk.federate.bitcoin.BitcoinWrapper; import co.rsk.federate.bitcoin.BlockListener; import co.rsk.federate.bitcoin.TransactionListener; -import co.rsk.federate.io.*; +import co.rsk.federate.io.BtcToRskClientFileData; +import co.rsk.federate.io.BtcToRskClientFileReadResult; +import co.rsk.federate.io.BtcToRskClientFileStorage; import co.rsk.federate.timing.TurnScheduler; import co.rsk.net.NodeBlockProcessor; import co.rsk.panic.PanicProcessor; @@ -14,28 +18,37 @@ import co.rsk.peg.Federation; import co.rsk.peg.PeginInformation; import co.rsk.peg.btcLockSender.BtcLockSender.TxSenderAddressType; +import co.rsk.peg.btcLockSender.BtcLockSenderProvider; import co.rsk.peg.pegininstructions.PeginInstructionsException; import co.rsk.peg.pegininstructions.PeginInstructionsProvider; import com.google.common.annotations.VisibleForTesting; -import co.rsk.peg.btcLockSender.BtcLockSenderProvider; import com.google.common.collect.Lists; -import org.bitcoinj.core.*; -import org.bitcoinj.store.BlockStoreException; -import org.ethereum.config.blockchain.upgrades.ActivationConfig; -import org.ethereum.config.blockchain.upgrades.ConsensusRule; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -import javax.annotation.PreDestroy; import java.io.IOException; import java.time.Clock; -import java.util.*; +import java.util.ArrayList; +import java.util.Collections; +import java.util.LinkedList; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; import java.util.stream.IntStream; - -import static com.google.common.base.Preconditions.checkNotNull; +import javax.annotation.PreDestroy; +import org.bitcoinj.core.Block; +import org.bitcoinj.core.NetworkParameters; +import org.bitcoinj.core.PartialMerkleTree; +import org.bitcoinj.core.Sha256Hash; +import org.bitcoinj.core.StoredBlock; +import org.bitcoinj.core.Transaction; +import org.bitcoinj.core.Utils; +import org.bitcoinj.store.BlockStoreException; +import org.ethereum.config.blockchain.upgrades.ActivationConfig; +import org.ethereum.config.blockchain.upgrades.ConsensusRule; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * Manages the process of informing the RSK bridge news about the bitcoin blockchain @@ -115,7 +128,7 @@ public synchronized void setup( } public void start(Federation federation) { - logger.info("Starting for Federation {}", federation.getAddress()); + logger.info("[start] Starting for Federation {}", federation.getAddress()); this.federation = federation; if (federation.isMember(federatorSupport.getFederationMember())) { logger.info("Watching federation {} since I belong to it", federation.getAddress().toString()); @@ -138,7 +151,7 @@ public void start(Federation federation) { } public void stop() { - logger.info("Stopping"); + logger.info("[stop] Stopping"); federation = null; @@ -206,7 +219,7 @@ public void updateBridge() { @Override public void onBlock(Block block) { synchronized (this) { - logger.debug("onBlock {}", block.getHash()); + logger.debug("[onBlock] onBlock {}", block.getHash()); PartialMerkleTree tree; Transaction coinbase = null; boolean dataToWrite = false; @@ -227,7 +240,7 @@ public void onBlock(Block block) { List proofs = fileData.getTransactionProofs().get(tx.getWTxId()); boolean blockInProofs = proofs.stream().anyMatch(p -> p.getBlockHash().equals(block.getHash())); if (blockInProofs) { - logger.info("Proof for tx {} in block {} already stored", tx, block.getHash()); + logger.info("[onBlock] Proof for tx {} in block {} already stored", tx, block.getHash()); continue; } @@ -249,14 +262,14 @@ public void onBlock(Block block) { // Validate information byte[] witnessReservedValue = coinbaseInformation.getCoinbaseWitnessReservedValue(); if (witnessReservedValue == null) { - logger.error("block {} with lock segwit tx {} has coinbase with no witness reserved value. Aborting block processing.", block.getHash(), tx.getWTxId()); + logger.error("[onBlock] block {} with lock segwit tx {} has coinbase with no witness reserved value. Aborting block processing.", block.getHash(), tx.getWTxId()); // Can't register this transaction, it would be rejected by the Bridge return; } Sha256Hash calculatedWitnessCommitment = Sha256Hash.twiceOf(witnessMerkleRoot.getReversedBytes(), witnessReservedValue); Sha256Hash witnessCommitment = coinbase.findWitnessCommitment(); if (!witnessCommitment.equals(calculatedWitnessCommitment)) { - logger.error("block {} with lock segwit tx {} generated an invalid witness merkle root", tx.getWTxId(), block.getHash()); + logger.error("[onBlock] block {} with lock segwit tx {} generated an invalid witness merkle root", tx.getWTxId(), block.getHash()); // Can't register this transaction, it would be rejected by the Bridge return; } @@ -266,14 +279,14 @@ public void onBlock(Block block) { // Register the coinbase just once per block coinbaseRegistered = true; } catch (Exception e) { - logger.error(e.getMessage(), e); + logger.error("[onBlock] {}", e.getMessage()); // Without a coinbase related to this transaction the Bridge would reject the transaction return; } } proofs.add(new Proof(block.getHash(), tree)); - logger.info("New proof for tx {} in block {}", tx, block.getHash()); + logger.info("[onBlock] New proof for tx {} in block {}", tx, block.getHash()); dataToWrite = true; } @@ -283,9 +296,9 @@ public void onBlock(Block block) { try { this.btcToRskClientFileStorage.write(fileData); - logger.info("Stored proofs for block {}", block.getHash()); + logger.info("[onBlock] Stored proofs for block {}", block.getHash()); } catch (IOException e) { - logger.error(e.getMessage(), e); + logger.error("[onBlock] {}", e.getMessage()); panicProcessor.panic("btclock", e.getMessage()); } } @@ -293,13 +306,13 @@ public void onBlock(Block block) { @Override public void onTransaction(Transaction tx) { - logger.debug("onTransaction {}", tx.getWTxId()); + logger.debug("[onTransaction] onTransaction {}", tx.getWTxId()); synchronized (this) { this.fileData.getTransactionProofs().put(tx.getWTxId(), new ArrayList<>()); try { this.btcToRskClientFileStorage.write(this.fileData); } catch (IOException e) { - logger.error(e.getMessage(), e); + logger.error("[onTransaction] {}", e.getMessage()); panicProcessor.panic("btclock", e.getMessage()); } } @@ -313,8 +326,8 @@ protected int updateBridgeBtcBlockchain() throws BlockStoreException, IOExceptio int federatorBtcBlockchainBestChainHeight = bitcoinWrapper.getBestChainHeight(); if (federatorBtcBlockchainBestChainHeight > bridgeBtcBlockchainBestChainHeight) { logger.debug( - "[updateBridgeBtcBlockchain] BTC blockchain height - Federator : {}, Bridge : {}.", - bitcoinWrapper.getBestChainHeight(), + "[updateBridgeBtcBlockchain] BTC blockchain height - Federator : {}, Bridge : {}", + federatorBtcBlockchainBestChainHeight, bridgeBtcBlockchainBestChainHeight ); // Federator's blockchain has more blocks than bridge's blockchain - go and try to @@ -452,7 +465,11 @@ private StoredBlock findBridgeBtcBlockchainMatchingAncestorUsingBlockLocator() t // Find the last best chain block the bridge has with respect // to the federate node's best chain. Object[] blockLocatorArray = federatorSupport.getBtcBlockchainBlockLocator(); - logger.debug("Block locator size {}, first {}, last {}.", blockLocatorArray.length, blockLocatorArray[0], blockLocatorArray[blockLocatorArray.length - 1]); + logger.debug( + "[findBridgeBtcBlockchainMatchingAncestorUsingBlockLocator] Block locator size {}, first {}, last {}.", + blockLocatorArray.length, blockLocatorArray[0], + blockLocatorArray[blockLocatorArray.length - 1] + ); StoredBlock matchedBlock = null; for (Object o : blockLocatorArray) { @@ -634,7 +651,8 @@ protected void updateBridgeBtcTransactions() { * Gets the first ready to be informed coinbase transaction and informs it */ protected void updateBridgeBtcCoinbaseTransactions() { - Optional coinbaseInformationReadyToInform = fileData.getCoinbaseInformationMap() + Optional coinbaseInformationReadyToInform = fileData + .getCoinbaseInformationMap() .values() .stream() .filter(CoinbaseInformation::isReadyToInform) @@ -705,7 +723,7 @@ private StoredBlock findBestChainStoredBlockFor(Transaction tx) throws IllegalSt @PreDestroy public void tearDown() throws IOException { - logger.info("BtcToRskClient tearDown starting..."); + logger.info("[tearDown] BtcToRskClient tearDown starting..."); if (federation != null) { bitcoinWrapper.removeFederationListener(federation, this); @@ -717,7 +735,7 @@ public void tearDown() throws IOException { this.btcToRskClientFileStorage.write(this.fileData); } - logger.info("BtcToRskClient tearDown finished."); + logger.info("[tearDown] BtcToRskClient tearDown finished."); } private void restoreFileData() throws Exception { @@ -729,12 +747,12 @@ private void restoreFileData() throws Exception { this.fileData = result.getData(); } else { String errorMessage = "Can't operate without a valid storage file"; - logger.error(errorMessage); - panicProcessor.panic("fed-storage",errorMessage); + logger.error("[restoreFileData] {}", errorMessage); + panicProcessor.panic("fed-storage", errorMessage); throw new Exception(errorMessage); } } catch (IOException e) { - logger.error("Failed to read data from BtcToRskClient file: {}", e.getMessage(), e); + logger.error("[restoreFileData] Failed to read data from BtcToRskClient file: {}", e.getMessage(), e); panicProcessor.panic("fed-storage",e.getMessage()); throw new Exception("Failed to read data from BtcToRskClient file", e); } From d93eef7ec957a00ab057fe08b754d3a331634b10 Mon Sep 17 00:00:00 2001 From: Marcos Date: Wed, 8 Feb 2023 09:54:08 -0300 Subject: [PATCH 3/3] Refactor updateBridge validations --- .../java/co/rsk/federate/BtcToRskClient.java | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/main/java/co/rsk/federate/BtcToRskClient.java b/src/main/java/co/rsk/federate/BtcToRskClient.java index 9e30f022..51ec8aa0 100644 --- a/src/main/java/co/rsk/federate/BtcToRskClient.java +++ b/src/main/java/co/rsk/federate/BtcToRskClient.java @@ -254,10 +254,10 @@ public void onBlock(Block block) { try { Sha256Hash witnessMerkleRoot = tree.getTxnHashAndMerkleRoot(new ArrayList<>()); CoinbaseInformation coinbaseInformation = new CoinbaseInformation( - coinbase, - witnessMerkleRoot, - block.getHash(), - coinbasePmt + coinbase, + witnessMerkleRoot, + block.getHash(), + coinbasePmt ); // Validate information byte[] witnessReservedValue = coinbaseInformation.getCoinbaseWitnessReservedValue(); @@ -336,13 +336,9 @@ protected int updateBridgeBtcBlockchain() throws BlockStoreException, IOExceptio // First, find the common ancestor that is in the federator's bestchain // using either the old method -- block locator // or the new one -- block depth incremental search - StoredBlock commonAncestor = null; - if (useBlockDepth) { - commonAncestor = findBridgeBtcBlockchainMatchingAncestor(bridgeBtcBlockchainBestChainHeight); - } else { - commonAncestor = findBridgeBtcBlockchainMatchingAncestorUsingBlockLocator(); - } - + StoredBlock commonAncestor = useBlockDepth ? + findBridgeBtcBlockchainMatchingAncestor(bridgeBtcBlockchainBestChainHeight) : + findBridgeBtcBlockchainMatchingAncestorUsingBlockLocator(); checkNotNull(commonAncestor, "No best chain block found"); logger.debug(