From 6983bb2be9ebcf7644c5bfe5ed6a808117f0eefb Mon Sep 17 00:00:00 2001
From: julia-zack <julia.zack@iovlabs.org>
Date: Thu, 7 Nov 2024 12:05:24 -0300
Subject: [PATCH 1/2] Do not set last retired fed p2sh script in new
 commitFederation impl

---
 .../peg/federation/FederationSupportImpl.java | 20 ++++++++++++-------
 .../federation/VoteFederationChangeTest.java  | 11 +++++-----
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/rskj-core/src/main/java/co/rsk/peg/federation/FederationSupportImpl.java b/rskj-core/src/main/java/co/rsk/peg/federation/FederationSupportImpl.java
index d17fd155ccc..762272cc069 100644
--- a/rskj-core/src/main/java/co/rsk/peg/federation/FederationSupportImpl.java
+++ b/rskj-core/src/main/java/co/rsk/peg/federation/FederationSupportImpl.java
@@ -702,7 +702,8 @@ private FederationChangeResponseCode legacyCommitPendingFederation(PendingFedera
         clearPendingFederationVoting();
 
         if (activations.isActive(RSKIP186)) {
-            preserveFederationChangeInfo(activeFederation);
+            setNewActiveFederationCreationBlockHeight();
+            preserveLastRetiredFederationScript();
         }
 
         Federation currentOldFederation = provider.getOldFederation(constants, activations);
@@ -733,12 +734,11 @@ private FederationChangeResponseCode commitPendingFederation(PendingFederation c
         // set proposed federation
         Federation proposedFederation = buildFederationFromPendingFederation(currentPendingFederation);
         provider.setProposedFederation(proposedFederation);
+        setNewActiveFederationCreationBlockHeight();
 
         clearPendingFederationVoting();
 
-        Federation activeFederation = getActiveFederation();
-        preserveFederationChangeInfo(activeFederation);
-        logCommitmentWithVotedFederation(eventLogger, activeFederation, proposedFederation);
+        logCommitmentWithVotedFederation(eventLogger, getActiveFederation(), proposedFederation);
 
         return FederationChangeResponseCode.SUCCESSFUL;
     }
@@ -756,13 +756,19 @@ private void clearPendingFederationVoting() {
         provider.getFederationElection(constants.getFederationChangeAuthorizer()).clear();
     }
 
-    private void preserveFederationChangeInfo(Federation activeFederation) {
-        provider.setNextFederationCreationBlockHeight(rskExecutionBlock.getNumber());
-
+    private void preserveLastRetiredFederationScript() {
+        Federation activeFederation = getActiveFederation();
         Script activeFederationMembersP2SHScript = getFederationMembersP2SHScript(activeFederation);
         provider.setLastRetiredFederationP2SHScript(activeFederationMembersP2SHScript);
     }
 
+    private void setNewActiveFederationCreationBlockHeight() {
+        // since we are creating the to-be-active-fed in this block,
+        // its creation block height is this block number
+        long newActiveFederationCreationBlockHeight = rskExecutionBlock.getNumber();
+        provider.setNextFederationCreationBlockHeight(newActiveFederationCreationBlockHeight);
+    }
+
     private Script getFederationMembersP2SHScript(Federation federation) {
         // when the federation is a standard multisig,
         // the members p2sh script is the p2sh script
diff --git a/rskj-core/src/test/java/co/rsk/peg/federation/VoteFederationChangeTest.java b/rskj-core/src/test/java/co/rsk/peg/federation/VoteFederationChangeTest.java
index 610720b6bb1..a8675376694 100644
--- a/rskj-core/src/test/java/co/rsk/peg/federation/VoteFederationChangeTest.java
+++ b/rskj-core/src/test/java/co/rsk/peg/federation/VoteFederationChangeTest.java
@@ -415,7 +415,8 @@ void voteCommitFederation_postRSKIP186_preRSKIP419_whenPendingFederationIsSet_sh
 
         assertPendingFederationVotingWasCleaned();
 
-        assertFederationChangeInfoWasSet();
+        assertNewActiveFederationCreationBlockHeightWasSet();
+        assertLastRetiredFederationScriptWasSet();
 
         Federation oldFederation = storageProvider.getOldFederation(federationMainnetConstants, activations);
         Federation newFederation = storageProvider.getNewFederation(federationMainnetConstants, activations);
@@ -443,7 +444,7 @@ void voteCommitFederation_postRSKIP419_whenPendingFederationIsSet_shouldPerformC
 
         assertPendingFederationVotingWasCleaned();
 
-        assertFederationChangeInfoWasSet();
+        assertNewActiveFederationCreationBlockHeightWasSet();
 
         assertLogCommitFederation(activeFederation, proposedFederation.get());
 
@@ -509,13 +510,13 @@ private void assertPendingFederationVotingWasCleaned() {
         assertTrue(federationElectionVotes.isEmpty());
     }
 
-    private void assertFederationChangeInfoWasSet() {
-        // assert federation creation block height was set correctly
+    private void assertNewActiveFederationCreationBlockHeightWasSet() {
         Optional<Long> nextFederationCreationBlockHeight = storageProvider.getNextFederationCreationBlockHeight(activations);
         assertTrue(nextFederationCreationBlockHeight.isPresent());
         assertEquals(RSK_EXECUTION_BLOCK_NUMBER, nextFederationCreationBlockHeight.get());
+    }
 
-        // assert last retired federation p2sh script was set correctly
+    private void assertLastRetiredFederationScriptWasSet() {
         Script activeFederationMembersP2SHScript = getFederationMembersP2SHScript(activations, federationSupport.getActiveFederation());
         Optional<Script> lastRetiredFederationP2SHScript = storageProvider.getLastRetiredFederationP2SHScript(activations);
         assertTrue(lastRetiredFederationP2SHScript.isPresent());

From 80b0d63376c48d8684bbed874954dd52bb407675 Mon Sep 17 00:00:00 2001
From: julia-zack <julia.zack@iovlabs.org>
Date: Tue, 19 Nov 2024 10:13:55 -0300
Subject: [PATCH 2/2] Do not set active federation creation block height before
 validation

---
 .../peg/federation/FederationSupportImpl.java | 20 +++++++++----------
 .../federation/VoteFederationChangeTest.java  | 16 ++++++++++++---
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/rskj-core/src/main/java/co/rsk/peg/federation/FederationSupportImpl.java b/rskj-core/src/main/java/co/rsk/peg/federation/FederationSupportImpl.java
index 762272cc069..c4b7cf6b6a9 100644
--- a/rskj-core/src/main/java/co/rsk/peg/federation/FederationSupportImpl.java
+++ b/rskj-core/src/main/java/co/rsk/peg/federation/FederationSupportImpl.java
@@ -702,8 +702,9 @@ private FederationChangeResponseCode legacyCommitPendingFederation(PendingFedera
         clearPendingFederationVoting();
 
         if (activations.isActive(RSKIP186)) {
-            setNewActiveFederationCreationBlockHeight();
-            preserveLastRetiredFederationScript();
+            // since we are creating the to-be-active-fed in this block,
+            // its creation block height is this block number
+            saveFederationChangeInfo(rskExecutionBlock.getNumber());
         }
 
         Federation currentOldFederation = provider.getOldFederation(constants, activations);
@@ -734,7 +735,6 @@ private FederationChangeResponseCode commitPendingFederation(PendingFederation c
         // set proposed federation
         Federation proposedFederation = buildFederationFromPendingFederation(currentPendingFederation);
         provider.setProposedFederation(proposedFederation);
-        setNewActiveFederationCreationBlockHeight();
 
         clearPendingFederationVoting();
 
@@ -756,19 +756,17 @@ private void clearPendingFederationVoting() {
         provider.getFederationElection(constants.getFederationChangeAuthorizer()).clear();
     }
 
-    private void preserveLastRetiredFederationScript() {
+    private void saveFederationChangeInfo(long newActiveFederationCreationBlockHeight) {
+        saveLastRetiredFederationScript();
+        provider.setNextFederationCreationBlockHeight(newActiveFederationCreationBlockHeight);
+    }
+
+    private void saveLastRetiredFederationScript() {
         Federation activeFederation = getActiveFederation();
         Script activeFederationMembersP2SHScript = getFederationMembersP2SHScript(activeFederation);
         provider.setLastRetiredFederationP2SHScript(activeFederationMembersP2SHScript);
     }
 
-    private void setNewActiveFederationCreationBlockHeight() {
-        // since we are creating the to-be-active-fed in this block,
-        // its creation block height is this block number
-        long newActiveFederationCreationBlockHeight = rskExecutionBlock.getNumber();
-        provider.setNextFederationCreationBlockHeight(newActiveFederationCreationBlockHeight);
-    }
-
     private Script getFederationMembersP2SHScript(Federation federation) {
         // when the federation is a standard multisig,
         // the members p2sh script is the p2sh script
diff --git a/rskj-core/src/test/java/co/rsk/peg/federation/VoteFederationChangeTest.java b/rskj-core/src/test/java/co/rsk/peg/federation/VoteFederationChangeTest.java
index a8675376694..9abfd170c71 100644
--- a/rskj-core/src/test/java/co/rsk/peg/federation/VoteFederationChangeTest.java
+++ b/rskj-core/src/test/java/co/rsk/peg/federation/VoteFederationChangeTest.java
@@ -444,11 +444,11 @@ void voteCommitFederation_postRSKIP419_whenPendingFederationIsSet_shouldPerformC
 
         assertPendingFederationVotingWasCleaned();
 
-        assertNewActiveFederationCreationBlockHeightWasSet();
-
         assertLogCommitFederation(activeFederation, proposedFederation.get());
 
-        // assert new and old federation were not set and utxos were not moved
+        // assert some values were not set
+        assertNewActiveFederationCreationBlockHeightWasNotSet();
+        assertLastRetiredFederationScriptWasNotSet();
         assertNewAndOldFederationsWereNotSet();
         assertUTXOsWereNotMovedFromNewToOldFederation();
     }
@@ -516,6 +516,11 @@ private void assertNewActiveFederationCreationBlockHeightWasSet() {
         assertEquals(RSK_EXECUTION_BLOCK_NUMBER, nextFederationCreationBlockHeight.get());
     }
 
+    private void assertNewActiveFederationCreationBlockHeightWasNotSet() {
+        Optional<Long> nextFederationCreationBlockHeight = storageProvider.getNextFederationCreationBlockHeight(activations);
+        assertFalse(nextFederationCreationBlockHeight.isPresent());
+    }
+
     private void assertLastRetiredFederationScriptWasSet() {
         Script activeFederationMembersP2SHScript = getFederationMembersP2SHScript(activations, federationSupport.getActiveFederation());
         Optional<Script> lastRetiredFederationP2SHScript = storageProvider.getLastRetiredFederationP2SHScript(activations);
@@ -523,6 +528,11 @@ private void assertLastRetiredFederationScriptWasSet() {
         assertEquals(activeFederationMembersP2SHScript, lastRetiredFederationP2SHScript.get());
     }
 
+    private void assertLastRetiredFederationScriptWasNotSet() {
+        Optional<Script> lastRetiredFederationP2SHScript = storageProvider.getLastRetiredFederationP2SHScript(activations);
+        assertFalse(lastRetiredFederationP2SHScript.isPresent());
+    }
+
     private Script getFederationMembersP2SHScript(ActivationConfig.ForBlock activations, Federation federation) {
         // when the federation is a standard multisig,
         // the members p2sh script is the p2sh script