Create Federation Change Integration Test With Migration Phase #2935

apancorb · 2025-01-16T12:00:22Z

Description

This new integration test will validate that when a federation changes, the UTXOs will be moved and migrated as expected after Lovell is activated.

Motivation and Context

How Has This Been Tested?

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
Tests for the changes have been added (for bug fixes / features)
Requires Activation Code (Hard Fork)

Other information:

github-actions · 2025-01-16T12:00:38Z

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

Package	Version	Score	Details

Scanned Manifest Files

…hase of a federation change

rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java

nathanieliov · 2025-01-16T15:47:08Z

rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java

+        for (PegoutsWaitingForConfirmations.Entry pegoutEntry : bridgeStorageProvider.getPegoutsWaitingForConfirmations().getEntries()) {
+            var pegoutBtcTransaction = pegoutEntry.getBtcTransaction();
+            for (TransactionInput input : pegoutBtcTransaction.getInputs()) {
+                // Each input should contain the right scriptsig


Suggested change

// Each input should contain the right scriptsig

// Each input should contain the right scriptSig

nathanieliov · 2025-01-16T15:53:16Z

rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java

+        var retiringFederation = federationStorageProvider.getOldFederation(
+            BRIDGE_CONSTANTS.getFederationConstants(), activations);
+
+        for (PegoutsWaitingForConfirmations.Entry pegoutEntry : bridgeStorageProvider.getPegoutsWaitingForConfirmations().getEntries()) {


Not sure about this. I think we should assert against constant values. I mean, for example, based on the initial conditions of the test, we should know the number of migration pegouts that should be created and assert that that number of pegouts is precisely what gets created. Then, check each one was created as expected. Wdyt?

The UTXOs for the original federation are randomly generated. So the size may vary per test. It was already done in the legacy tests here: https://github.com/rsksmart/rskj/blob/feature/powpeg_validation_protocol-phase4/rskj-core/src/test/java/co/rsk/peg/federation/PowpegMigrationTest.java

We've been removing/getting away from random behavior from tests. I think, we should discuss if we should keep this being random or make it deterministic.

rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java

… new federation

sonarqubecloud · 2025-01-17T15:00:03Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

julia-zack · 2025-01-17T15:02:01Z

rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java

+        assertNull(bridgeSupport.getRetiringFederationAddress());
+    }
+
+    private void assertLastRetiredFederationP2SHMatchesWithOriginalFederation(FederationType federationType, Federation originalFederation, ActivationConfig.ForBlock activations) {


Suggested change

private void assertLastRetiredFederationP2SHMatchesWithOriginalFederation(FederationType federationType, Federation originalFederation, ActivationConfig.ForBlock activations) {

private void assertLastRetiredFederationP2SHScriptMatchesWithOriginalFederationOne(FederationType federationType, Federation originalFederation, ActivationConfig.ForBlock activations) {

wdyt?

nathanieliov · 2025-01-17T15:04:59Z

rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java

@@ -414,8 +556,130 @@ private void assertNewAndOldFederationsHaveExpectedAddress(
        assertEquals(expectedOldFederationAddress, bridgeSupport.getRetiringFederationAddress());
    }

-    private void assertFundsWereNotMigrated() throws Exception {
+    private void assertMigrationHasNotStarted() throws Exception {


I think this method is missing some assertion to match what the method's name means about what it is doing. I think, to assert the migration has not started, it should check:

Current block is behind fedActivationAge + fundsMigrationAgeBegin

No existing retiring fed utxos has been transferred to the new fed. For this, it should check retiring fed utxos still the same, and there is no active fed utxo.

Wdyt?

julia-zack · 2025-01-17T15:26:27Z

rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java

+        var activations = ActivationConfigsForTest.all().forBlock(0);
+        setUpFederationChange(activations);
+        // Create a default original federation using the list of UTXOs
+        var originalFederation = createOriginalFederation(
+            FederationType.P2SH_ERP, ORIGINAL_FEDERATION_KEYS, activations);
+        var originalUTXOs = federationStorageProvider.getNewFederationBtcUTXOs(
+            BRIDGE_CONSTANTS.getBtcParams(), activations);
+
+        // Act & Assert
+
+        // Create pending federation using the new federation keys
+        var newFederation = createPendingFederation(NEW_FEDERATION_KEYS, activations); 
+        commitPendingFederation();
+        // Since Lovell is activated we will commit the proposed federation
+        commitProposedFederation(activations);
+        // Move blockchain until the activation phase
+        activateNewFederation(activations);
+
+        assertUTXOsReferenceMovedFromNewToOldFederation(
+            originalUTXOs, activations);
+        assertNewAndOldFederationsHaveExpectedAddress(
+            newFederation.getAddress(), originalFederation.getAddress());
+        assertMigrationHasNotStarted();


all this part is basically the first test right? maybe we should keep just this one, that tests the whole process. Wdyt?

julia-zack · 2025-01-17T17:26:58Z

rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java

+        // Migrate while there are still utxos to migrate
+        var remainingUTXOs = federationStorageProvider.getOldFederationBtcUTXOs();
+        while (!remainingUTXOs.isEmpty()) {
+            var updateCollectionsTx = buildUpdateCollectionsTx();


Suggested change

// Migrate while there are still utxos to migrate

var remainingUTXOs = federationStorageProvider.getOldFederationBtcUTXOs();

while (!remainingUTXOs.isEmpty()) {

var updateCollectionsTx = buildUpdateCollectionsTx();

// Migrate while there are still utxos to migrate

var remainingUTXOs = federationStorageProvider.getOldFederationBtcUTXOs();

var updateCollectionsTx = buildUpdateCollectionsTx();

while (!remainingUTXOs.isEmpty()) {

it always creates the same tx right?

julia-zack · 2025-01-17T18:10:29Z

rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java

+        if (activations.isActive(ConsensusRule.RSKIP379)) {
+            assertTrue(bridgeStorageProvider.hasPegoutTxSigHash(lastPegoutSigHash.get()));
+        } else {
+            assertFalse(bridgeStorageProvider.hasPegoutTxSigHash(lastPegoutSigHash.get()));
+        }


i'd try to avoid booleans in tests. I think its better to have both assertPegoutTxSigHashesAreSaved and assertPegoutTxSigHashesAreNotSaved, wdyt?

julia-zack · 2025-01-17T18:13:52Z

rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java

+        verifySigHashes(activations);
+        verifyPegoutTransactionCreatedEventWasEmitted(activations);
+        verifyPegouts(activations);


wdyt about having the calls for these three methods inside the assertMigrationHasStarted?

julia-zack · 2025-01-17T18:19:09Z

rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java

+                Optional<Federation> spendingFederationOptional = Optional.empty();
+                if (inputStandardRedeemScript.equals(getFederationDefaultRedeemScript(activeFederation))) {
+                    spendingFederationOptional = Optional.of(activeFederation);
+                } else if (retiringFederation != null &&
+                    inputStandardRedeemScript.equals(getFederationDefaultRedeemScript(retiringFederation))) {
+                    spendingFederationOptional = Optional.of(retiringFederation);
+                } else {
+                    fail("Pegout scriptsig does not match any Federation");
+                }


the migration shouldn't always be from the retiring to the active fed? I mean, at this point the only pegouts expected are from the migration right?

also, i find this really hard to follow. Lets try to avoid booleans in tests logic and have different methods for different expected behavior

julia-zack · 2025-01-17T18:20:30Z

rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java

+                }
+
+                int index = spendingFederation.getNumberOfSignaturesRequired() + 1;
+                if (spendingFederation instanceof ErpFederation) {


we could filter for federation type here also, right?

apancorb self-assigned this Jan 16, 2025

apancorb marked this pull request as ready for review January 16, 2025 12:02

apancorb requested a review from a team as a code owner January 16, 2025 12:02

feat(federation): add integration test that completes the migration p…

0813889

…hase of a federation change

apancorb force-pushed the feat/migration_test branch from 9e7d495 to 0813889 Compare January 16, 2025 13:13

nathanieliov reviewed Jan 16, 2025

View reviewed changes

rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java Show resolved Hide resolved

nathanieliov reviewed Jan 16, 2025

View reviewed changes

feat(federation): add assert to check if utxos have moved from old to…

690db38

… new federation

julia-zack reviewed Jan 17, 2025

View reviewed changes

nathanieliov reviewed Jan 17, 2025

View reviewed changes

julia-zack reviewed Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create Federation Change Integration Test With Migration Phase #2935

Create Federation Change Integration Test With Migration Phase #2935

apancorb commented Jan 16, 2025

github-actions bot commented Jan 16, 2025 •

edited

Loading

nathanieliov Jan 16, 2025

nathanieliov Jan 16, 2025

apancorb Jan 17, 2025

nathanieliov Jan 17, 2025

sonarqubecloud bot commented Jan 17, 2025

julia-zack Jan 17, 2025

nathanieliov Jan 17, 2025

julia-zack Jan 17, 2025

julia-zack Jan 17, 2025 •

edited

Loading

julia-zack Jan 17, 2025

julia-zack Jan 17, 2025

julia-zack Jan 17, 2025 •

edited

Loading

julia-zack Jan 17, 2025

julia-zack Jan 17, 2025

	// Each input should contain the right scriptsig
	// Each input should contain the right scriptSig

	private void assertLastRetiredFederationP2SHMatchesWithOriginalFederation(FederationType federationType, Federation originalFederation, ActivationConfig.ForBlock activations) {
	private void assertLastRetiredFederationP2SHScriptMatchesWithOriginalFederationOne(FederationType federationType, Federation originalFederation, ActivationConfig.ForBlock activations) {

Create Federation Change Integration Test With Migration Phase #2935

Are you sure you want to change the base?

Create Federation Change Integration Test With Migration Phase #2935

Conversation

apancorb commented Jan 16, 2025

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

github-actions bot commented Jan 16, 2025 • edited Loading

Dependency Review

OpenSSF Scorecard

Scanned Manifest Files

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 17, 2025

Quality Gate passed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julia-zack Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julia-zack Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 16, 2025 •

edited

Loading

julia-zack Jan 17, 2025 •

edited

Loading

julia-zack Jan 17, 2025 •

edited

Loading