Powpeg refactors integration #524

nathanieliov · 2025-11-27T20:34:01Z

This pull request refactors the logic for determining and constructing federations in FederationProviderFromFederatorSupport, streamlining the process and improving maintainability. The main changes include consolidating federation construction logic, improving error handling, updating logging, and simplifying the associated tests.

Refactoring and logic simplification:

Replaced the getExpectedFederation method with a new buildFederation method that tries to construct the correct federation type (standard multisig or P2SH-P2WSH ERP) and throws a clear exception if neither matches the expected address. Supporting helper methods (tryBuildingStandardMultiSigFederation, tryBuildingP2shP2wshErpFederation) were introduced for clarity and modularity.
Updated the logic in getActiveFederation and getRetiringFederation to use the new buildFederation method, and improved debug logging to provide more informative messages.
Simplified getProposedFederation to always build a P2SH-P2WSH ERP federation, removing conditional logic and the now-unused buildProposedFederation method.

Remove old and unused logic

Remove the use of local storage for waitingForSignatures pegouts. The changes involve deleting the BtcReleaseClientStorageAccessor and BtcReleaseClientStorageSynchronizer classes and all related code, simplifying the pegout signing and release process. Makes the flow easier to follow by removing file-based storage and synchronization checks, making the pegout process depend solely on WaitingForSignature pegout map from the rsk.

Test cleanup and modernization:

Refactored tests in FederationProviderFromFederatorSupportTest to use new helper methods (setupActiveFederation, setupActiveFederationKeys) for setting up mocks, removed redundant code, and improved test readability. Also removed unused federation format version constants.

These changes make the federation provider code more modular, easier to maintain, and clearer in its intent, while also improving the reliability and clarity of the test suite.

How Has This Been Tested?

Unit tests

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-11-27T20:34:11Z

Dependency Review

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

Scanned Files

None

… supported federation types(mulsisig and p2wsh). Update related tests accordingly

…per methods.

- Rename federation building methods - Standardize key mocking logic in test helpers

…ability and maintainability

…move unused tests

…d exception messages

…ests

…mVersion in tests

…nit test method names

Copilot

Pull request overview

This pull request refactors the codebase to remove support for legacy HSM versions (V2 and V4), retaining only V1 and the latest V5. The changes consolidate federation construction logic, simplify test setup, and update references throughout the codebase to use a centralized helper for the latest HSM version.

Removed HSMVersion V2 and V4 enum values and related version-specific logic
Introduced TestUtils.getLatestHsmVersion() helper method to centralize HSM version references in tests
Refactored FederationProviderFromFederatorSupport to streamline federation construction with clearer error handling

Reviewed changes

Copilot reviewed 27 out of 27 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`TestUtils.java`	Added `getLatestHsmVersion()` helper and simplified `createBlock()` method signature
`ReleaseRequirementsEnforcerTest.java`	Updated to use centralized HSM version helper and consolidated test methods
`SignerMessageBuilderFactoryTest.java`	Simplified tests using the latest HSM version helper
`ReleaseCreationInformationGetterTest.java`	Updated block creation calls and HSM version references
`PowHSMSignerMessageTest.java`	Removed parameterized legacy tests and simplified remaining tests
`PowHSMSignerMessageBuilderTest.java`	Updated test names and removed HSMVersion parameters from method calls
`PowHSMSigningClientTest.java`	Updated to use latest HSM version and simplified test setup
`PowHSMSigningClientRskMstTest.java`	Updated version references to use centralized helper
`PowHSMSigningClientBtcTest.java`	Consolidated parameterized tests and removed version-specific logic
`HSMSigningClientV1Test.java`	Updated method name for clarity
`HSMSigningClientProviderTest.java`	Enhanced unsupported version testing with specific version cases
`HSMResponseHandlerBaseTest.java`	Updated test names to reflect PowHSM instead of version-specific references
`HsmBookkeepingClientImplTest.java`	Centralized setup and removed version-specific parameterization
`ConfirmedBlocksProviderTest.java`	Updated test names and removed legacy version comparison logic
`HSMVersionTest.java`	Removed tests for deleted enum values and their related methods
`FederationProviderFromFederatorSupportTest.java`	Major refactoring with new helper methods and improved test structure
`FedNodeRunnerTest.java`	Updated to use latest HSM version and removed legacy version configuration tests
`ReleaseRequirementsEnforcer.java`	Updated log message to refer to "Pow HSM" instead of "Version 2+"
`ReleaseCreationInformationGetter.java`	Renamed method to reflect PowHSM instead of version-specific naming
`PowHSMSignerMessage.java`	Removed version parameter from `getMessageToSign()` method
`PowHSMSigningClientBtc.java`	Updated to call `getMessageToSign()` without version parameter
`PowHSMSigningClient.java`	Simplified `getPublicKey()` logic and removed commented code
`HsmBookkeepingClientImpl.java`	Removed version-specific conditional logic for uncles and blockchain parameters
`ConfirmedBlocksProvider.java`	Removed version-specific uncle difficulty consideration logic
`HSMVersion.java`	Removed V2 and V4 enum values and their related methods
`HSMBlockchainBookkeepingRelatedException.java`	Updated documentation reference from "HSM 2" to "Pow HSM"
`FederationProviderFromFederatorSupport.java`	Refactored federation building logic with improved error handling and logging

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-02T19:53:49Z

src/test/java/co/rsk/federate/signing/utils/TestUtils.java

+    public static Block createBlock(List<Transaction> rskTxs) {
+        int blockNumber = 1;
+        int parentBlockNumber = 0;


The createBlock method has been simplified but now hardcodes blockNumber to 1 and parentBlockNumber to 0. This removes flexibility that may be needed in tests requiring different block numbers. Consider keeping the block number as a parameter or document why this hardcoding is acceptable.

Suggested change

public static Block createBlock(List<Transaction> rskTxs) {

int blockNumber = 1;

int parentBlockNumber = 0;

public static Block createBlock(List<Transaction> rskTxs, int blockNumber, int parentBlockNumber) {

For now, it is only being used with those values. What if we let this happen when the demand for it appears?

Copilot · 2025-12-02T19:53:49Z

src/main/java/co/rsk/federate/FederationProviderFromFederatorSupport.java

+            String.format(
+                "Cannot determine federation type for federation with address %s. Tried: standard multiSig, and P2SH-P2WSH ERP federations.",
+                expectedFederationAddress


Corrected capitalization of 'multiSig' to 'multisig' for consistency with the codebase terminology.

This is also being used as multiSig in other places like co.rsk.peg.federation.FederationFactory#buildStandardMultiSigFederation. I think we can discuss it and agree on which one to use, and do the change as part of another task.

Copilot · 2025-12-02T19:53:50Z

src/test/java/co/rsk/federate/FederationProviderFromFederatorSupportTest.java

            Arguments.of(
                activations,
-                createP2shErpFederation()
+                createNonStandardErpFederation()


[nitpick] The method name createNonStandardErpFederation() is ambiguous. Based on the implementation, this appears to create a P2SH ERP federation (non-segwit). Consider renaming to createP2shErpFederationWithNonMatchingAddress() or similar to clarify its purpose in testing invalid federation scenarios.

I believe this is also related to this discussion #524 (comment). Let's tackle that there.

marcos-iov · 2025-12-02T20:09:08Z

src/test/java/co/rsk/federate/signing/hsm/message/PowHSMSignerMessageTest.java


-    @ParameterizedTest
-    @MethodSource("getMessageToSignLegacyArgProvider")
-    void getMessageToSign_whenSighashLegacyMode_ok(HSMVersion version, JsonNode expectedMessageToSend) {


Why remove these test? We still have the logic to sign legacy transactions, don't we?

This is only for PowHSM. It was named legacy because it was only for HSM 5 prior versions. I removed it because we no longer support any of those versions.

marcos-iov · 2025-12-02T20:17:36Z

src/main/java/co/rsk/federate/FederationProviderFromFederatorSupport.java

    public Federation getActiveFederation() {
        List<FederationMember> members = new ArrayList<>();
        int federationSize = federatorSupport.getFederationSize();
        boolean useTypedPublicKeyGetter = federatorSupport.getConfigForBestBlock().isActive(RSKIP123);


Don't think we need this anymore

Or any other activation check in this class I believe

You're right, if we assume all activations are active, we don't need to check activations.

marcos-iov · 2025-12-02T20:19:55Z

src/main/java/co/rsk/federate/FederationProviderFromFederatorSupport.java

+            );
+    }
+
+    private static IllegalStateException buildInvalidFederationException(


Does this function make sense? Why not trow the exception directly? This is only called in one place

I created it to make the functional flow(where the function is being used) more straightforward to follow. But it could be thrown directly.

marcos-iov · 2025-12-02T20:34:35Z

src/test/java/co/rsk/federate/FederationProviderFromFederatorSupportTest.java

    private static final FederationConstants federationConstants = FederationMainNetConstants.getInstance();
    private static final NetworkParameters networkParameters = federationConstants.getBtcParams();
    private static final Instant creationTime = Instant.ofEpochSecond(5);
    private static ActivationConfig.ForBlock activations;


If we remove the need for activations, we should just focus on cases where either the genesis federation is built or a p2wsh. And maybe also federation address not match error

Removed the activations setup from all tests. Now we are checking only Standard(genesis) and P2wsh federation.

The only place where activations is still being used is to create a non-standard ERP federation, using it as an unknown federation (for the federation address not matching error). Wdyt?

Updated as discussed. Removed test using old type of federation and refactor border test case when built federatio do not match bridge federation address result

…MessageTest

…iderFromFederatorSupport

…ederation address

… dependencies

…nsactions

…Hash

… pegout creation block

…er for clarity in pegout transaction processing

…or clarity in `ReleaseCreationInformationGetter`.

Remove use of local storage for waitingForSignatures pegouts

sonarqubecloud · 2025-12-05T19:45:17Z

Quality Gate passed

Issues
31 New issues
0 Accepted issues

Measures
0 Security Hotspots
81.2% Coverage on New Code
0.6% Duplication on New Code

See analysis details on SonarQube Cloud

nathanieliov requested a review from a team as a code owner November 27, 2025 20:34

marcos-iov requested a review from Copilot December 2, 2025 19:52

nathanieliov added 13 commits December 2, 2025 16:52

Refactor FederationProviderFromFederatorSupport to only build current…

1fc65d4

… supported federation types(mulsisig and p2wsh). Update related tests accordingly

Discard logger formatting, fix wrong log message and improve test hel…

c638bac

…per methods.

- Refactored federation-building methods

d7fd4d8

- Rename federation building methods - Standardize key mocking logic in test helpers

Refactor FederationProviderFromFederatorSupportTest for improved read…

1c2a45b

…ability and maintainability

Renamed test helper methods and remove blank lines.

561a175

Update tests to HSM version 5

e427827

Update test method to use version 5 instead of older versions, and re…

94a9a51

…move unused tests

refactor: update references from HSM version 5 to Pow HSM in tests an…

a591585

…d exception messages

refactor: remove block number parameter from createBlock and adjust t…

1f1638c

…ests

refactor: replace HSMVersion.V5 references with TestUtils.getLatestHs…

40e22cf

…mVersion in tests

Remove redundant setup and remove unnecessary version references to u…

32fd3d3

…nit test method names

Fix typo in test provider helper method

a0ce933

Remove unused fields from PowHSMSignerMessageTest

983829c

marcos-iov force-pushed the powpeg-refactors-integration branch from 96d5c5a to 983829c Compare December 2, 2025 19:52

Copilot AI reviewed Dec 2, 2025

View reviewed changes

marcos-iov reviewed Dec 2, 2025

View reviewed changes

nathanieliov added 2 commits December 3, 2025 11:44

Rollback removal of unit test for legacy sighash mode in PowHSMSigner…

37b2511

…MessageTest

Remove legacy activation checks and redundant tests in FederationProv…

28d9905

…iderFromFederatorSupport

nathanieliov force-pushed the powpeg-refactors-integration branch from cc0af43 to 28d9905 Compare December 3, 2025 16:39

nathanieliov and others added 8 commits December 3, 2025 14:26

Refactor border test case when built federation do not match bridge f…

e3f9f6e

…ederation address

Refactor BtcReleaseClient to remove storage accessor and synchronizer…

16819a1

… dependencies

Fix logging and signature handling in BtcReleaseClient for pegout tra…

de78f47

…nsactions

Refactor ReleaseCreationInformation to remove pegoutConfirmationRskTx…

1ccc375

…Hash

Refactor ReleaseCreationInformationGetter to search for events in the…

f720eee

… pegout creation block

Refactor method names and variables in ReleaseCreationInformationGett…

68bcac9

…er for clarity in pegout transaction processing

Remove old test testUsageOfStorageWhenSigning and update comments f…

3e4af17

…or clarity in `ReleaseCreationInformationGetter`.

Refactor BtcReleaseClient to remove activations checks

ccabd24

marcos-iov and others added 12 commits December 4, 2025 13:33

Improve log messages in ReleaseCreationInformationGetter

86c0f1b

Remove comment from FedNodeRunner

5563e3f

Cleanup PowHSMSigningClientBtcTest

cdad8be

Cleanup PowHSMSignerMessageBuilderTest

14417b7

Refactors to BtcReleaseClient. Improve logging and variable reuse

3316a0e

Improve comments in ReleaseCreationInformationGetter

ea5dcf3

Reuse variable in ReleaseCreationInformationGetterTest

6463b87

Update destination address in migration tests

6b03dee

Rename method in EventsTestUtils

1f784af

Rename method in ReleaseCreationInformationGetter

049e379

Remove unnecessary assertion from BtcReleaseClientTest

8131c3f

Merge pull request #525 from rsksmart/poc-remove-wfs-local-map

0982da2

Remove use of local storage for waitingForSignatures pegouts

Powpeg refactors integration #524

Are you sure you want to change the base?

Powpeg refactors integration #524

Uh oh!

Conversation

nathanieliov commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

github-actions bot commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dependency Review

Scanned Files

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathanieliov Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathanieliov Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathanieliov Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Dec 5, 2025

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nathanieliov commented Nov 27, 2025 •

edited

Loading

github-actions bot commented Nov 27, 2025 •

edited

Loading

nathanieliov Dec 3, 2025 •

edited

Loading

nathanieliov Dec 3, 2025 •

edited

Loading

nathanieliov Dec 3, 2025 •

edited

Loading