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

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();
 

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() {

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<Keccak256, BtcTransaction> 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);

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<Keccak256, BtcTransaction> 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