Correct and make updateSvpState easier to follow

rskj-core/src/main/java/co/rsk/peg/BridgeSupport.java

@@ -1022,7 +1022,7 @@ private void logUpdateCollections(Transaction rskTx) {
     private void updateSvpState(Transaction rskTx) {
         Optional<Federation> proposedFederationOpt = federationSupport.getProposedFederation();
         if (proposedFederationOpt.isEmpty()) {
-            logger.debug("[updateSvpState] Proposed federation does not exist, so there's no svp going on.");
+            logger.trace("[updateSvpState] Proposed federation does not exist, so there's no svp going on.");
             return;
         }
 
@@ -1034,11 +1034,19 @@ private void updateSvpState(Transaction rskTx) {
                 "[updateSvpState] Proposed federation validation failed at block {}. SVP failure will be processed",
                 rskExecutionBlock.getNumber()
             );
-            processSVPFailure(proposedFederation);
+            processSvpFailure(proposedFederation);
             return;
         }
 
         Keccak256 rskTxHash = rskTx.getHash();
+
+        if (shouldCreateAndProcessSvpFundTransaction()) {
+            logger.info(
+                "[updateSvpState] No svp values were found, so fund tx creation will be processed."
+            );
+            processSvpFundTransactionUnsigned(rskTxHash, proposedFederation);
+        }
+
         // if the fund tx signed is present, then the fund transaction change was registered,
         // meaning we can create the spend tx.
         Optional<BtcTransaction> svpFundTxSigned = provider.getSvpFundTxSigned();
@@ -1047,20 +1055,22 @@ private void updateSvpState(Transaction rskTx) {
                 "[updateSvpState] Fund tx signed was found, so spend tx creation will be processed."
             );
             processSvpSpendTransactionUnsigned(rskTxHash, proposedFederation, svpFundTxSigned.get());
-            return;
         }
+    }
 
-        // if the unsigned fund tx hash is not present, meaning we can proceed with the fund tx creation
+    private boolean shouldCreateAndProcessSvpFundTransaction() {
+        // the fund tx will be created when the svp starts,
+        // so we must ensure all svp values are clear to proceed with its creation
         Optional<Sha256Hash> svpFundTxHashUnsigned = provider.getSvpFundTxHashUnsigned();
-        if (svpFundTxHashUnsigned.isEmpty()) {
-            logger.info(
-                "[updateSvpState] Fund tx hash unsigned was not found, so fund tx creation will be processed."
-            );
-            processSvpFundTransactionUnsigned(rskTxHash, proposedFederation);
-        }
+        Optional<BtcTransaction> svpFundTxSigned = provider.getSvpFundTxSigned();
+        Optional<Sha256Hash> svpSpendTxHashUnsigned = provider.getSvpSpendTxHashUnsigned(); // spendTxHash will be removed the last, after spendTxWFS, so is enough checking just this value
+
+        return svpFundTxHashUnsigned.isEmpty()
+            && svpFundTxSigned.isEmpty()
+            && svpSpendTxHashUnsigned.isEmpty();
     }
 
-    private void processSVPFailure(Federation proposedFederation) {
+    private void processSvpFailure(Federation proposedFederation) {
         eventLogger.logCommitFederationFailure(rskExecutionBlock, proposedFederation);
 
         logger.info("[processSVPFailure] Federation election will be allowed again.");

rskj-core/src/test/java/co/rsk/peg/BridgeSupportSvpTest.java

@@ -141,8 +141,8 @@ void setUp() {
 
     @Nested
     @TestInstance(TestInstance.Lifecycle.PER_CLASS)
-    @Tag("SVP failure tests")
-    class SVPFailureTests {
+    @Tag("Failure tests")
+    class FailureTests {
 
         @BeforeEach
         void setUp() {
@@ -259,6 +259,7 @@ private void assertNoSVPValues() {
     @TestInstance(TestInstance.Lifecycle.PER_CLASS)
     @Tag("Fund transaction creation and processing tests")
     class FundTxCreationAndProcessingTests {
+
         @Test
         void updateCollections_whenProposedFederationDoesNotExist_shouldNotCreateFundTransaction() throws IOException {
             // arrange
@@ -288,10 +289,10 @@ void updateCollections_whenThereAreNoEnoughUTXOs_shouldNotCreateFundTransaction(
         void updateCollections_whenFundTxCanBeCreated_createsExpectedFundTxAndSavesTheHashInStorageEntryAndPerformsPegoutActions() throws Exception {
             // act
             bridgeSupport.updateCollections(rskTx);
-            bridgeStorageProvider.save(); // to save the tx sig hash
+            bridgeStorageProvider.save();
 
             // assert
-            assertSvpFundTxHashUnsignedWasSavedInStorage();
+            assertSvpFundTxHashUnsignedIsSavedInStorage();
             assertSvpFundTxReleaseWasSettled();
             assertSvpFundTransactionHasExpectedInputsAndOutputs();
         }
@@ -335,6 +336,59 @@ private void assertOneOutputIsToProposedFederationWithFlyoverPrefixWithExpectedA
 
             assertOutputWasSentToExpectedScriptWithExpectedAmount(svpFundTransactionUnsignedOutputs, proposedFederationWithFlyoverPrefixScriptPubKey, spendableValueFromProposedFederation);
         }
+
+        @Test
+        void updateCollections_whenWaitingForFundTxToBeRegistered_shouldNotCreateFundTransactionAgain() throws Exception {
+            // arrange
+            arrangeSvpFundTransactionUnsigned();
+            assertSvpFundTxHashUnsignedIsSavedInStorage();
+
+            // act
+            bridgeSupport.updateCollections(rskTx);
+            bridgeStorageProvider.save();
+
+            // assert
+            assertJustUpdateCollectionsWasLogged();
+        }
+
+        @Test
+        void updateCollections_whenFundTxSignedIsSaved_shouldNotCreateFundTransaction() throws Exception {
+            // arrange
+            arrangeSvpFundTransactionSigned();
+
+            // act
+            bridgeSupport.updateCollections(rskTx);
+            bridgeStorageProvider.save();
+
+            // assert
+            assertNoSvpFundTxHashUnsigned();
+        }
+
+        @Test
+        void updateCollections_whenWaitingForSpendTxToBeRegistered_shouldNotCreateFundTransaction() throws Exception {
+            // arrange
+            arrangeSvpSpendTransaction();
+
+            // act
+            bridgeSupport.updateCollections(rskTx);
+            bridgeStorageProvider.save();
+
+            // assert
+            assertNoSvpFundTxHashUnsigned();
+            assertJustUpdateCollectionsWasLogged();
+        }
+
+        private void assertJustUpdateCollectionsWasLogged() {
+            assertEquals(1, logs.size());
+
+            CallTransaction.Function updateCollectionsEvent = BridgeEvents.UPDATE_COLLECTIONS.getEvent();
+            List<DataWord> encodedTopics = getEncodedTopics(updateCollectionsEvent);
+            assertEventWasEmittedWithExpectedTopics(encodedTopics);
+
+            String senderData = rskTx.getSender(mock(SignatureCache.class)).toHexString();
+            byte[] encodedData = getEncodedData(updateCollectionsEvent, senderData);
+            assertEventWasEmittedWithExpectedData(encodedData);
+        }
     }
 
     private void arrangeSvpFundTransactionUnsigned() {
@@ -358,7 +412,7 @@ private void saveSvpFundTransactionHashUnsigned(Sha256Hash svpFundTransactionHas
         bridgeSupport.save();
     }
 
-    private void assertSvpFundTxHashUnsignedWasSavedInStorage() {
+    private void assertSvpFundTxHashUnsignedIsSavedInStorage() {
         Optional<Sha256Hash> svpFundTransactionHashUnsignedOpt = bridgeStorageProvider.getSvpFundTxHashUnsigned();
         assertTrue(svpFundTransactionHashUnsignedOpt.isPresent());
 
@@ -937,13 +991,17 @@ private void arrangeExecutionBlockIsAfterValidationPeriodEnded() {
     }
 
     private void arrangeSvpFundTransactionSigned() {
-        recreateSvpFundTransactionUnsigned();
-        signInputs(svpFundTransaction);
+        recreateSvpFundTransactionSigned();
 
         bridgeStorageProvider.setSvpFundTxSigned(svpFundTransaction);
         bridgeStorageProvider.save();
     }
 
+    private void recreateSvpFundTransactionSigned() {
+        recreateSvpFundTransactionUnsigned();
+        signInputs(svpFundTransaction);
+    }
+
     private void recreateSvpFundTransactionUnsigned() {
         svpFundTransaction = new BtcTransaction(btcMainnetParams);
 
@@ -991,14 +1049,14 @@ private void signInputs(BtcTransaction transaction) {
 
     private void arrangeSvpSpendTransaction() {
         recreateSvpSpendTransactionUnsigned();
-        saveSvpSpendTransactionWFSValues();
+        saveSvpSpendTransactionValues();
     }
 
     private void recreateSvpSpendTransactionUnsigned() {
         svpSpendTransaction = new BtcTransaction(btcMainnetParams);
         svpSpendTransaction.setVersion(BTC_TX_VERSION_2);
 
-        arrangeSvpFundTransactionSigned();
+        recreateSvpFundTransactionSigned();
         // add inputs
         addInputFromMatchingOutputScript(svpSpendTransaction, svpFundTransaction, proposedFederation.getP2SHScript());
         Script proposedFederationRedeemScript = proposedFederation.getRedeemScript();
@@ -1014,9 +1072,12 @@ private void recreateSvpSpendTransactionUnsigned() {
         svpSpendTransaction.addOutput(Coin.valueOf(1762), federationSupport.getActiveFederationAddress());
     }
 
-    private void saveSvpSpendTransactionWFSValues() {
+    private void saveSvpSpendTransactionValues() {
+        bridgeStorageProvider.setSvpSpendTxHashUnsigned(svpSpendTransaction.getHash());
+
         Map.Entry<Keccak256, BtcTransaction> svpSpendTxWaitingForSignatures = new AbstractMap.SimpleEntry<>(svpSpendTxCreationHash, svpSpendTransaction);
         bridgeStorageProvider.setSvpSpendTxWaitingForSignatures(svpSpendTxWaitingForSignatures);
+
         bridgeStorageProvider.save();
     }