Add checkstyle

[marcos-iov]

This pull request introduces Checkstyle static analysis to the project and updates the build workflow to support ratcheted Checkstyle validation on pull requests. It also adds the necessary configuration and verification metadata for new dependencies required by Checkstyle.

Checkstyle integration and configuration:

• Added a new checkstyle.xml configuration file under config/checkstyle/ with a comprehensive set of style and correctness checks, and a placeholder suppressions.xml for custom rule suppressions. [1] [2]
• Updated the build workflow (.github/workflows/build_and_test.yml) to support ratcheted Checkstyle runs on pull requests by setting the base branch for comparison and passing the appropriate argument to Gradle. This ensures that only changes introduced in the PR are checked for new violations.

Dependency and verification updates:

• Added Checkstyle (com.puppycrawl.tools:checkstyle:12.2.0) and its transitive dependencies to gradle/verification-metadata.xml, along with other required libraries such as guava, commons-beanutils, commons-codec, picocli, and Saxon-HE. This ensures dependency integrity and reproducibility for the new static analysis tooling. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12]

These changes collectively enable and enforce Java code style and correctness checks in CI, helping to maintain code quality as the codebase evolves.

[github-actions]

Dependency Review

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

Scanned Files

None

[Copilot]

Pull request overview

This PR introduces Checkstyle static analysis to the PowPeg node Gradle build, adds a ratcheting mechanism to run Checkstyle only on changed Java files for PR builds, and updates dependency verification metadata to keep builds reproducible.

Changes:

• Add Checkstyle plugin/configuration (including a ratcheted file set computed via git diff) and new helper tasks (checkstyleAll, checkstyleFile) in Gradle.
• Update CI workflow to pass a -PratchetFrom=... base ref argument on PR builds.
• Add Checkstyle config files and update verification metadata for newly required dependencies; apply small code cleanups to satisfy style rules (empty catch comments, unused imports, hashCode()).

Reviewed changes

Copilot reviewed 13 out of 19 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/test/java/co/rsk/federate/signing/utils/TestUtils.java	Tighten constructor visibility to satisfy modifier/style checks.
src/test/java/co/rsk/federate/signing/ECDSACompositeSignerTest.java	Avoid multiple var declarations; make empty catch blocks non-empty.
src/test/java/co/rsk/federate/BtcToRskClientForkTest.java	Add comment to best-effort cleanup catch blocks.
src/main/java/co/rsk/federate/util/FederatorWalletReader.java	Trailing whitespace/newline cleanup.
src/main/java/co/rsk/federate/util/FederationKeysGenerator.java	Trailing whitespace/newline cleanup.
src/main/java/co/rsk/federate/signing/keyfile/KeyFileChecker.java	Rename local var for clarity; align with style rules.
src/main/java/co/rsk/federate/signing/hsm/message/UpdateAncestorBlockMessage.java	Trailing whitespace/newline cleanup.
src/main/java/co/rsk/federate/signing/ECDSAHSMSigner.java	Trailing whitespace/newline cleanup.
src/main/java/co/rsk/federate/signing/ECDSACompositeSigner.java	Fix modifier/spacing in inner constructor.
src/main/java/co/rsk/federate/rpc/JsonRpcClient.java	Remove unused import.
src/main/java/co/rsk/federate/log/FederateLogger.java	Remove unused imports.
src/main/java/co/rsk/federate/gas/BestBlockMinGasPriceProvider.java	Remove unused imports.
src/main/java/co/rsk/federate/bitcoin/BitcoinWrapperImpl.java	Add hashCode() consistent with custom equals().
src/main/java/co/rsk/federate/adapter/package-info.java	Formatting adjustment to satisfy style checks.
gradle/verification-metadata.xml	Add verification entries for Checkstyle and transitive deps.
config/checkstyle/suppressions.xml	Add baseline suppressions file placeholder.
config/checkstyle/checkstyle.xml	Add Checkstyle rule set for style/correctness checks.
build.gradle	Enable Checkstyle and implement ratcheted file selection via git diff.
.github/workflows/build_and_test.yml	Pass ratchet base ref property to Gradle on PR builds.

[Copilot]

Pull request overview

Copilot reviewed 13 out of 19 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/test/java/co/rsk/federate/signing/ECDSACompositeSignerTest.java:127

• Same as above: this test should assert the expected SignerException rather than catching any Exception, so it fails if an unexpected exception type is thrown.

        try {
            when(signer1.canSignWith(new KeyId("another-key"))).thenReturn(false);
            when(signer2.canSignWith(new KeyId("another-key"))).thenReturn(false);
            signer.getPublicKey(new KeyId("another-id"));
            fail();

src/main/java/co/rsk/federate/signing/keyfile/KeyFileChecker.java:71

• Setting privateKey = null does not clear the private key bytes from memory; it only drops the reference. Since this is key material, it should be actively wiped in a finally block (e.g., Arrays.fill) so it happens even on early returns/exceptions.

        try {
            byte[] privateKey;
            KeyFileHandler keyHandler = new KeyFileHandler(this.filePath);
            privateKey = keyHandler.privateKey();
            boolean sizeOk = this.validateKeyLength(privateKey);
            privateKey = null;

src/test/java/co/rsk/federate/signing/ECDSACompositeSignerTest.java:101

• Catching a broad Exception here makes the test accept any failure mode. Since ECDSACompositeSigner throws SignerException when no signer matches, asserting the specific exception type with assertThrows makes the test stronger and clearer (and avoids needing an empty-catch comment).

This issue also appears on line 123 of the same file.

        try {
            when(signer1.canSignWith(new KeyId("another-key"))).thenReturn(false);
            when(signer2.canSignWith(new KeyId("another-key"))).thenReturn(false);
            signer.sign(new KeyId("another-id"), new SignerMessageV1(Hex.decode("aabbcc")));
            fail();

[Copilot]

Pull request overview

Copilot reviewed 13 out of 19 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/test/java/co/rsk/federate/signing/ECDSACompositeSignerTest.java:130

• This test passes on any thrown Exception, which can hide unexpected failures and makes the assertion less precise. Prefer asserting the expected exception type via assertThrows (SignerException) instead of try/catch + fail().

            signer.getPublicKey(new KeyId("another-id"));
            fail();
        } catch (Exception e) {
            // expected
        }

src/main/java/co/rsk/federate/signing/keyfile/KeyFileChecker.java:71

• Setting the privateKey reference to null does not wipe the key material from memory; it only drops the reference. If the goal is to reduce exposure of private key bytes, overwrite the array before nulling it (at least best-effort).

            byte[] privateKey;
            KeyFileHandler keyHandler = new KeyFileHandler(this.filePath);
            privateKey = keyHandler.privateKey();
            boolean sizeOk = this.validateKeyLength(privateKey);
            privateKey = null;

src/test/java/co/rsk/federate/signing/ECDSACompositeSignerTest.java:104

• This test passes on any thrown Exception, which can hide unexpected failures (e.g., NullPointerException) and makes the assertion less precise. Prefer asserting the expected exception type via assertThrows (SignerException) instead of try/catch + fail().

This issue also appears on line 126 of the same file.

            signer.sign(new KeyId("another-id"), new SignerMessageV1(Hex.decode("aabbcc")));
            fail();
        } catch (Exception e) {
            // expected
        }

[Copilot]

Pull request overview

Copilot reviewed 13 out of 19 changed files in this pull request and generated no new comments.

Suppressed comments (3)

config/checkstyle/checkstyle.xml:139

• IllegalIdentifierName's format is currently a negative-lookahead that matches every identifier except record|yield|var|_. Since this module flags identifiers that match the regex as illegal, this would effectively make almost all names illegal and break Checkstyle runs. The regex should match only the identifiers you want to ban.

        <module name="IllegalIdentifierName">
            <property name="format" value="(?i)^(?!(record|yield|var|_)$).+$"/>
        </module>

src/test/java/co/rsk/federate/signing/ECDSACompositeSignerTest.java:130

• Same as above: the broad catch (Exception) will treat any exception as success and doesn't verify the intended behavior. getPublicKey(...) declares throws SignerException, so assert SignerException via Assertions.assertThrows(...) to keep the test precise.

            signer.getPublicKey(new KeyId("another-id"));
            fail();
        } catch (Exception e) {
            // expected
        }

src/test/java/co/rsk/federate/signing/ECDSACompositeSignerTest.java:104

• Catching a broad Exception here will make the test pass for unexpected failures (e.g., NPEs) and doesn't assert the expected contract. Since ECDSACompositeSigner.sign(...) throws SignerException when no suitable signer is found, assert that specific exception (and optionally its message) with Assertions.assertThrows(...) instead of try/catch.

This issue also appears on line 126 of the same file.

            signer.sign(new KeyId("another-id"), new SignerMessageV1(Hex.decode("aabbcc")));
            fail();
        } catch (Exception e) {
            // expected
        }

[Copilot]

Pull request overview

Copilot reviewed 13 out of 19 changed files in this pull request and generated no new comments.

Suppressed comments (3)

config/checkstyle/checkstyle.xml:138

• The regex used for IllegalIdentifierName is the inverse of what the surrounding comment describes: it matches everything except the contextual keywords (record|yield|var|_), which can unintentionally invert the rule (and potentially flag almost every identifier if Checkstyle treats format as the illegal-name pattern). Update the pattern to match only the identifiers that should be banned.

            <property name="format" value="(?i)^(?!(record|yield|var|_)$).+$"/>

src/test/java/co/rsk/federate/signing/ECDSACompositeSignerTest.java:130

• Same issue here: catching Exception makes the test succeed even if a different, unexpected error is thrown. Use assertThrows for the specific expected exception so the test only passes for the intended failure mode.

            signer.getPublicKey(new KeyId("another-id"));
            fail();
        } catch (Exception e) {
            // expected
        }

src/test/java/co/rsk/federate/signing/ECDSACompositeSignerTest.java:104

• This test currently passes for any exception (including unexpected ones), which can mask real regressions. Since the intent is to verify that no signer matches the KeyId, assert the expected exception type with assertThrows instead of catching Exception.

This issue also appears on line 126 of the same file.

            signer.sign(new KeyId("another-id"), new SignerMessageV1(Hex.decode("aabbcc")));
            fail();
        } catch (Exception e) {
            // expected
        }

[sonarqubecloud]

Quality Gate passed

Issues
100 New issues
0 Accepted issues

Measures
0 Security Hotspots
84.8% Coverage on New Code
0.3% Duplication on New Code

See analysis details on SonarQube Cloud