From 7a04c058756de713f0a365bbfd2935afdb6c2396 Mon Sep 17 00:00:00 2001 From: julia-zack Date: Tue, 29 Oct 2024 15:50:56 -0300 Subject: [PATCH] Improve naming and logs. Create clearSvpValues method in provider to be called from support. Assert any of svp values are present when clearing value. Add missing blank line Make logging more accurate when clearing svp values in provider. Minor refactor for clearing svp values in support Correct test --- .../co/rsk/peg/BridgeStorageProvider.java | 31 ++++++++ .../main/java/co/rsk/peg/BridgeSupport.java | 46 +++--------- .../co/rsk/peg/BridgeStorageProviderTest.java | 74 +++++++++++++++++++ .../java/co/rsk/peg/BridgeSupportSvpTest.java | 42 ++++++----- 4 files changed, 138 insertions(+), 55 deletions(-) diff --git a/rskj-core/src/main/java/co/rsk/peg/BridgeStorageProvider.java b/rskj-core/src/main/java/co/rsk/peg/BridgeStorageProvider.java index 1f573fa4c4b..dc3e231c47d 100644 --- a/rskj-core/src/main/java/co/rsk/peg/BridgeStorageProvider.java +++ b/rskj-core/src/main/java/co/rsk/peg/BridgeStorageProvider.java @@ -31,6 +31,8 @@ import org.ethereum.config.blockchain.upgrades.ActivationConfig; import org.ethereum.core.Repository; import org.ethereum.vm.DataWord; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.spongycastle.util.encoders.Hex; /** @@ -40,6 +42,7 @@ * @author Oscar Guindzberg */ public class BridgeStorageProvider { + private static final Logger logger = LoggerFactory.getLogger(BridgeStorageProvider.class); // Dummy value to use when saving key only indexes private static final byte TRUE_VALUE = (byte) 1; @@ -679,6 +682,34 @@ private void saveSvpSpendTxWaitingForSignatures() { ); } + public void clearSvpValues() { + if (svpFundTxHashUnsigned != null) { + logger.warn("[clearSvpValues] Clearing fund tx hash unsigned {} value.", svpFundTxHashUnsigned); + setSvpFundTxHashUnsigned(null); + } + + if (svpFundTxSigned != null) { + logger.warn("[clearSvpValues] Clearing fund tx signed {} value.", svpFundTxSigned.getHash()); + setSvpFundTxSigned(null); + } + + if (svpSpendTxWaitingForSignatures != null) { + Keccak256 rskCreationHash = svpSpendTxWaitingForSignatures.getKey(); + BtcTransaction svpSpendTx = svpSpendTxWaitingForSignatures.getValue(); + logger.warn( + "[clearSvpValues] Clearing spend tx waiting for signatures with spend tx {} and rsk creation hash {} value.", + svpSpendTx.getHash(), rskCreationHash + ); + setSvpSpendTxWaitingForSignatures(null); + setSvpSpendTxHashUnsigned(null); + } + + if (svpSpendTxHashUnsigned != null) { + logger.warn("[clearSvpValues] Clearing spend tx hash unsigned {} value.", svpSpendTxHashUnsigned); + setSvpSpendTxHashUnsigned(null); + } + } + public void save() { saveBtcTxHashesAlreadyProcessed(); diff --git a/rskj-core/src/main/java/co/rsk/peg/BridgeSupport.java b/rskj-core/src/main/java/co/rsk/peg/BridgeSupport.java index 78d6649c84c..6c9a001dbb3 100644 --- a/rskj-core/src/main/java/co/rsk/peg/BridgeSupport.java +++ b/rskj-core/src/main/java/co/rsk/peg/BridgeSupport.java @@ -1017,47 +1017,19 @@ private void logUpdateCollections(Transaction rskTx) { eventLogger.logUpdateCollections(sender); } - protected void processValidationFailure(Federation proposedFederation) { + protected void processSVPFailure(Federation proposedFederation) { eventLogger.logCommitFederationFailure(rskExecutionBlock, proposedFederation); - logger.warn("[processValidationFailure] Proposed federation validation failed so svp values will be cleared."); - clearSvpValues(); - } - - private void clearSvpValues() { - federationSupport.clearProposedFederation(); - - provider.getSvpFundTxHashUnsigned().ifPresent( - svpFundTxHashUnsigned -> { - logger.warn("[clearSvpValues] Fund tx change {} was never registered.", svpFundTxHashUnsigned); - provider.setSvpFundTxHashUnsigned(null); - } + logger.warn( + "[processSVPFailure] Proposed federation validation failed at block {}, so federation election will be allowed again.", + rskExecutionBlock.getNumber() ); - provider.getSvpFundTxSigned().ifPresent( - svpFundTxSigned -> { - logger.warn("[clearSvpValues] Spend tx was never created. Fund tx hash: {}", svpFundTxSigned.getHash()); - provider.setSvpFundTxSigned(null); - } - ); - - provider.getSvpSpendTxWaitingForSignatures().ifPresent( - svpSpendTxWFS -> { - Keccak256 rskCreationHash = svpSpendTxWFS.getKey(); - BtcTransaction svpSpendTx = svpSpendTxWFS.getValue(); - - logger.warn("[clearSvpValues] Spend tx {} was not fully signed. Rsk creation hash: {}.", - svpSpendTx.getHash(), rskCreationHash); - provider.setSvpSpendTxWaitingForSignatures(null); - provider.setSvpSpendTxHashUnsigned(null); - } - ); + allowFederationElectionAgain(); + } - provider.getSvpSpendTxHashUnsigned().ifPresent( - svpSpendTxHashUnsigned -> { - logger.warn("[clearSvpValues] Spend tx {} was not registered.", svpSpendTxHashUnsigned); - provider.setSvpSpendTxHashUnsigned(null); - } - ); + private void allowFederationElectionAgain() { + federationSupport.clearProposedFederation(); + provider.clearSvpValues(); } private boolean svpIsOngoing() { diff --git a/rskj-core/src/test/java/co/rsk/peg/BridgeStorageProviderTest.java b/rskj-core/src/test/java/co/rsk/peg/BridgeStorageProviderTest.java index 6fdf7e7d00c..6679779c13c 100644 --- a/rskj-core/src/test/java/co/rsk/peg/BridgeStorageProviderTest.java +++ b/rskj-core/src/test/java/co/rsk/peg/BridgeStorageProviderTest.java @@ -1162,6 +1162,80 @@ void getSvpSpendTxWaitingForSignatures_whenEntryIsCached_shouldReturnTheCachedEn } } + @Nested + @TestInstance(TestInstance.Lifecycle.PER_CLASS) + @Tag("clear svp values tests") + class ClearSvpValuesTests { + private BridgeStorageProvider bridgeStorageProvider; + + @BeforeEach + void setup() { + Repository repository = createRepository(); + bridgeStorageProvider = createBridgeStorageProvider(repository, mainnetBtcParams, activationsAllForks); + } + + @Test + void clearSvpValues_whenFundTxHashUnsigned_shouldClearValue() { + // arrange + Sha256Hash svpFundTxHashUnsigned = BitcoinTestUtils.createHash(1); + bridgeStorageProvider.setSvpFundTxHashUnsigned(svpFundTxHashUnsigned); + + // act + bridgeStorageProvider.clearSvpValues(); + + // assert + assertNoSVPValues(); + } + + @Test + void clearSvpValues_whenFundTxSigned_shouldClearValue() { + // arrange + BtcTransaction svpFundTxSigned = new BtcTransaction(mainnetBtcParams); + bridgeStorageProvider.setSvpFundTxSigned(svpFundTxSigned); + + // act + bridgeStorageProvider.clearSvpValues(); + + // assert + assertNoSVPValues(); + } + + @Test + void clearSvpValues_whenSpendTxWFS_shouldClearSpendTxValues() { + // arrange + Keccak256 svpSpendTxCreationHash = RskTestUtils.createHash(1); + BtcTransaction svpSpendTx = new BtcTransaction(mainnetBtcParams); + Map.Entry svpSpendTxWFS = new AbstractMap.SimpleEntry<>(svpSpendTxCreationHash, svpSpendTx); + bridgeStorageProvider.setSvpSpendTxWaitingForSignatures(svpSpendTxWFS); + + // act + bridgeStorageProvider.clearSvpValues(); + + // assert + assertNoSVPValues(); + } + + @Test + void clearSvpValues_whenSpendTxHashUnsigned_shouldClearValue() { + // arrange + Sha256Hash svpSpendTxCreationHash = BitcoinTestUtils.createHash(1); + bridgeStorageProvider.setSvpSpendTxHashUnsigned(svpSpendTxCreationHash); + + // act + bridgeStorageProvider.clearSvpValues(); + + // assert + assertNoSVPValues(); + } + + private void assertNoSVPValues() { + assertFalse(bridgeStorageProvider.getSvpFundTxHashUnsigned().isPresent()); + assertFalse(bridgeStorageProvider.getSvpFundTxSigned().isPresent()); + assertFalse(bridgeStorageProvider.getSvpSpendTxWaitingForSignatures().isPresent()); + assertFalse(bridgeStorageProvider.getSvpSpendTxHashUnsigned().isPresent()); + } + } + @Test void getReleaseRequestQueue_before_rskip_146_activation() throws IOException { Repository repositoryMock = mock(Repository.class); diff --git a/rskj-core/src/test/java/co/rsk/peg/BridgeSupportSvpTest.java b/rskj-core/src/test/java/co/rsk/peg/BridgeSupportSvpTest.java index 0aaac81f615..36abb4ccad2 100644 --- a/rskj-core/src/test/java/co/rsk/peg/BridgeSupportSvpTest.java +++ b/rskj-core/src/test/java/co/rsk/peg/BridgeSupportSvpTest.java @@ -112,8 +112,8 @@ void setUp() { @Nested @TestInstance(TestInstance.Lifecycle.PER_CLASS) - @Tag("Validation failure tests") - class ValidationFailureTests { + @Tag("SVP failure tests") + class SVPFailureTests { @BeforeEach void setUp() { logs = new ArrayList<>(); @@ -152,45 +152,44 @@ void processValidationFailure_whenSvpFundTxHashUnsigned_shouldLogValidationFailu bridgeStorageProvider.setSvpFundTxHashUnsigned(svpFundTxHashUnsigned); // act - bridgeSupport.processValidationFailure(proposedFederation); + bridgeSupport.processSVPFailure(proposedFederation); // assert assertLogCommitFederationFailed(); - assertProposedFederationWasCleared(); - assertFalse(bridgeStorageProvider.getSvpFundTxHashUnsigned().isPresent()); + assertNoProposedFederation(); + assertNoSVPValues(); } @Test void processValidationFailure_whenSvpFundTxSigned_shouldLogValidationFailureAndClearValue() { // arrange - BtcTransaction svpFundTx = mock(BtcTransaction.class); + BtcTransaction svpFundTx = new BtcTransaction(btcMainnetParams); bridgeStorageProvider.setSvpFundTxSigned(svpFundTx); // act - bridgeSupport.processValidationFailure(proposedFederation); + bridgeSupport.processSVPFailure(proposedFederation); // assert assertLogCommitFederationFailed(); - assertProposedFederationWasCleared(); - assertFalse(bridgeStorageProvider.getSvpFundTxSigned().isPresent()); + assertNoProposedFederation(); + assertNoSVPValues(); } @Test void processValidationFailure_whenSvpSpendTxWFS_shouldLogValidationFailureAndClearSpendTxValues() { // arrange Keccak256 svpSpendTxCreationHash = RskTestUtils.createHash(1); - BtcTransaction svpSpendTx = mock(BtcTransaction.class); + BtcTransaction svpSpendTx = new BtcTransaction(btcMainnetParams); Map.Entry svpSpendTxWFS = new AbstractMap.SimpleEntry<>(svpSpendTxCreationHash, svpSpendTx); bridgeStorageProvider.setSvpSpendTxWaitingForSignatures(svpSpendTxWFS); // act - bridgeSupport.processValidationFailure(proposedFederation); + bridgeSupport.processSVPFailure(proposedFederation); // assert assertLogCommitFederationFailed(); - assertProposedFederationWasCleared(); - assertFalse(bridgeStorageProvider.getSvpSpendTxWaitingForSignatures().isPresent()); - assertFalse(bridgeStorageProvider.getSvpSpendTxHashUnsigned().isPresent()); + assertNoProposedFederation(); + assertNoSVPValues(); } @Test @@ -200,12 +199,12 @@ void processValidationFailure_whenSvpSpendTxHashUnsigned_shouldLogValidationFail bridgeStorageProvider.setSvpSpendTxHashUnsigned(svpSpendTxHash); // act - bridgeSupport.processValidationFailure(proposedFederation); + bridgeSupport.processSVPFailure(proposedFederation); // assert assertLogCommitFederationFailed(); - assertProposedFederationWasCleared(); - assertFalse(bridgeStorageProvider.getSvpSpendTxHashUnsigned().isPresent()); + assertNoProposedFederation(); + assertNoSVPValues(); } private void assertLogCommitFederationFailed() { @@ -218,9 +217,16 @@ private void assertLogCommitFederationFailed() { assertEventWasEmittedWithExpectedData(encodedData); } - private void assertProposedFederationWasCleared() { + private void assertNoProposedFederation() { assertFalse(federationSupport.getProposedFederation().isPresent()); } + + private void assertNoSVPValues() { + assertFalse(bridgeStorageProvider.getSvpFundTxHashUnsigned().isPresent()); + assertFalse(bridgeStorageProvider.getSvpFundTxSigned().isPresent()); + assertFalse(bridgeStorageProvider.getSvpSpendTxWaitingForSignatures().isPresent()); + assertFalse(bridgeStorageProvider.getSvpSpendTxHashUnsigned().isPresent()); + } } @Nested