Added a test to check what is the maximum number of inputs allowed #3405

julianlen · 2025-12-05T20:30:55Z

Description

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-12-05T20:31:10Z

Dependency Review

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

Scanned Files

None

Copilot

Pull request overview

This PR adds tests to verify the maximum number of inputs allowed in Bitcoin pegout transactions while staying within the Bitcoin standard transaction size limit. The changes include updates to the PegoutTransactionBuilder to accept custom script signatures, a new utility method for calculating SegWit transaction virtual size, and two test cases that verify transaction sizes with different input counts.

Added calculateSignedSegwitBtcTxVirtualSize utility method for calculating transaction virtual sizes
Updated PegoutTransactionBuilder.withInput to accept a script parameter instead of hardcoding an empty byte array
Added tests verifying that 150 inputs stay below the limit while 210 inputs exceed it

Reviewed changes

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

Show a summary per file

File	Description
rskj-core/src/main/java/co/rsk/peg/BridgeUtils.java	Extracted virtual byte calculation into reusable methods and added public method for calculating SegWit transaction virtual size
rskj-core/src/test/java/co/rsk/peg/BridgeUtilsTest.java	Added test case to verify virtual size calculation using a real SegWit transaction
rskj-core/src/test/java/co/rsk/peg/ReleaseTransactionBuilderTest.java	Added tests verifying transaction size limits with varying input counts and refactored UTXO generation helper
rskj-core/src/test/java/co/rsk/test/builders/PegoutTransactionBuilder.java	Modified `withInput` method to accept script parameter for more flexible test setup
rskj-core/src/test/java/co/rsk/peg/BridgeSupportSvpTest.java	Updated test calls to match new `withInput` signature

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

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

Copilot

Pull request overview

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

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

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

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

julianlen · 2025-12-09T15:06:30Z

As a comment, the method calculateSignedSegwitBtcTxVirtualSize https://github.com/rsksmart/rskj/pull/3405/files#diff-a33646b11694d377aa69060bbfd25b2800f9e3f72e5ecfbbb3b1b4ab16c4da2fR674 is in BridgeUtils.java as production code. Although the tests only use this method.

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

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

julia-zack · 2025-12-10T15:56:13Z

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

+        int actualVirtualBytes = calculateSignedSegwitBtcTxVirtualSize(btcTx);
+
+        //assert
+        Assertions.assertEquals(expectedVirtualBytes, actualVirtualBytes);


very very nice

rskj-core/src/test/java/co/rsk/test/builders/PegoutTransactionBuilder.java

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

julia-zack · 2025-12-12T00:16:45Z

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

    }

+    @Test
+    void createPegoutTransaction_with210Inputs_with50Outputs_shouldHaveASizeAboveMaximumBtcStandardSize() {


should we do these tests for migrations also?

As it is right now, a migration tx has only 3 outputs (if there is change). Would you think it makes sense at this stage? Perhaps in the future with multiple utxos?

i mean creating it manually, no big deal at all anyways

(btw, there's no change in the migration tx)

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

nathanieliov · 2025-12-12T01:37:31Z

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

    }
+
+    @Test
+    void calculateSignedSegwitBtcTxVirtualSize_withASegwitTx_shouldReturnCorrectVirtualSize() {


Suggested change

void calculateSignedSegwitBtcTxVirtualSize_withASegwitTx_shouldReturnCorrectVirtualSize() {

void calculateSignedSegwitBtcTxVirtualSize_whenSignedSegwitTx_shouldReturnCorrectVirtualSize() {

Wdyt?

hmm the tx is not being signed

I missed the article. I refer to the noun ASignedSegwitTx, which denotes a signed segwit tx is being passed. The point is to make that test case explicit and to test it with other types of txs as well.

it was changed to calculateBtcTxVirtualSize_withARealBtcTx_shouldReturnCorrectVirtualSize

nathanieliov · 2025-12-12T01:40:34Z

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

        return baseSize + signingSize;
    }

+    public static int calculateSignedSegwitBtcTxVirtualSize(BtcTransaction btcTx) {


What happens if you pass an unsigned segwit transaction? Does it work? Return the correct value? Or should throw an exception?

I think we are missing more tests for this method. To mention some cases:

Passing unsigned segwit tx(as said)?

Bech32 tx?

Legacy tx?

For unsigned segwit tx there is a different method that estimates the size calculateSegwitTxSize

If you pass an unsigned segwit tx or another type of tx, it will attempt to calculate the btc tx size. Will it succeed on it? If so, will the result be correct? I guess no. Since this used is not expected, should we throw an exception if the method is misused so we can catch this error early?

The thing is that this method isn't used anywhere but to calculate the btc tx size limit for a SegWit tx.

As discussed, let's create a task to further analyze the missing test cases.

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

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

nathanieliov · 2025-12-12T01:55:46Z

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

    }

+    @Test
+    void createPegoutTransaction_with210Inputs_with50Outputs_shouldHaveASizeAboveMaximumBtcStandardSize() {


The test naming is confusing to me. The name says you are testing the createPegoutTransaction method, but it is a test helper method, so I guess what is being tested is calculateSignedSegwitBtcTxVirtualSize. Then, why have these tests in ReleaseTransactionBuilderTest? According to what I see, these tests are using nothing from it. I would rather move them to BridgeUtilsTest, or better yet, to their own test class. Wdyt?

these tests are one step before creating migration transactions with multiple utxos. That is why it is confusing. The test will be createMigrationTransactionWithMultipleUtxos..

In BridgeUtilsTest I am testing calculateBtcTxVirtualSize_withARealBtcTx_shouldReturnCorrectVirtualSize

The same, let's create a task for the future refactors.

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

julia-zack · 2025-12-12T15:24:32Z

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

-    private final Federation p2shP2wshErpProposedFederation = P2shP2wshErpFederationBuilder.builder().build();
+    private final List<BtcECKey> signingKeys = BitcoinTestUtils.getBtcEcKeys(20);
+    private final Federation p2shP2wshErpProposedFederation = P2shP2wshErpFederationBuilder.builder().withMembersBtcPublicKeys(signingKeys).build();
+    private final Federation p2shP2wshErpFederation = P2shP2wshErpFederationBuilder.builder().withMembersBtcPublicKeys(signingKeys).build();


Suggested change

private final Federation p2shP2wshErpFederation = P2shP2wshErpFederationBuilder.builder().withMembersBtcPublicKeys(signingKeys).build();

private final Federation activeP2shP2wshErpFederation = P2shP2wshErpFederationBuilder.builder().withMembersBtcPublicKeys(signingKeys).build();

wdyt? and move the declaration below the activeP2shErpFederation

julia-zack

great work! left one suggestion

sonarqubecloud · 2025-12-12T16:04:01Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

julianlen requested a review from a team as a code owner December 5, 2025 20:30

julianlen requested review from Copilot and removed request for a team December 5, 2025 20:31

Copilot AI reviewed Dec 5, 2025

View reviewed changes

rskj-core/src/test/java/co/rsk/peg/ReleaseTransactionBuilderTest.java Outdated Show resolved Hide resolved

rskj-core/src/test/java/co/rsk/peg/ReleaseTransactionBuilderTest.java Outdated Show resolved Hide resolved

added a test to check what is the maximum number of inputs allowed

9fbd0d3

julianlen force-pushed the input-limits branch from 1044f07 to 9fbd0d3 Compare December 5, 2025 20:32

julianlen requested a review from Copilot December 5, 2025 20:34

Copilot AI reviewed Dec 5, 2025

View reviewed changes

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

rskj-core/src/main/java/co/rsk/peg/BridgeUtils.java Show resolved Hide resolved

julia-zack reviewed Dec 10, 2025

View reviewed changes

rskj-core/src/test/java/co/rsk/peg/BridgeUtilsTest.java Outdated Show resolved Hide resolved

julia-zack reviewed Dec 10, 2025

View reviewed changes

rskj-core/src/main/java/co/rsk/peg/BridgeUtils.java Outdated Show resolved Hide resolved

julia-zack reviewed Dec 10, 2025

View reviewed changes

julia-zack reviewed Dec 12, 2025

View reviewed changes

rskj-core/src/test/java/co/rsk/test/builders/PegoutTransactionBuilder.java Outdated Show resolved Hide resolved

julia-zack reviewed Dec 12, 2025

View reviewed changes

rskj-core/src/test/java/co/rsk/peg/ReleaseTransactionBuilderTest.java Outdated Show resolved Hide resolved

julia-zack reviewed Dec 12, 2025

View reviewed changes

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

julia-zack reviewed Dec 12, 2025

View reviewed changes

rskj-core/src/test/java/co/rsk/peg/ReleaseTransactionBuilderTest.java Outdated Show resolved Hide resolved

julia-zack reviewed Dec 12, 2025

View reviewed changes

rskj-core/src/test/java/co/rsk/peg/ReleaseTransactionBuilderTest.java Outdated Show resolved Hide resolved

julia-zack reviewed Dec 12, 2025

View reviewed changes

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

nathanieliov reviewed Dec 12, 2025

View reviewed changes

rskj-core/src/test/java/co/rsk/peg/BridgeSupportSvpTest.java Outdated Show resolved Hide resolved

nathanieliov reviewed Dec 12, 2025

View reviewed changes

renamings, tests improvements, and fixing typos

bccc44d

julianlen requested review from julia-zack and nathanieliov December 12, 2025 15:19

julia-zack reviewed Dec 12, 2025

View reviewed changes

julia-zack approved these changes Dec 12, 2025

View reviewed changes

little renaming

649ea08

nathanieliov approved these changes Dec 16, 2025

View reviewed changes

	void calculateSignedSegwitBtcTxVirtualSize_withASegwitTx_shouldReturnCorrectVirtualSize() {
	void calculateSignedSegwitBtcTxVirtualSize_whenSignedSegwitTx_shouldReturnCorrectVirtualSize() {

	private final Federation p2shP2wshErpFederation = P2shP2wshErpFederationBuilder.builder().withMembersBtcPublicKeys(signingKeys).build();
	private final Federation activeP2shP2wshErpFederation = P2shP2wshErpFederationBuilder.builder().withMembersBtcPublicKeys(signingKeys).build();

Added a test to check what is the maximum number of inputs allowed #3405

Are you sure you want to change the base?

Added a test to check what is the maximum number of inputs allowed #3405

Uh oh!

Conversation

julianlen commented Dec 5, 2025

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

github-actions bot commented Dec 5, 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!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

julianlen commented Dec 9, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

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!

nathanieliov Dec 12, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

github-actions bot commented Dec 5, 2025 •

edited

Loading

nathanieliov Dec 12, 2025 •

edited

Loading