Skip to content

refactor: remove the RSKIP643 pegout tx index grace period - #3795

Merged
marcos-iov merged 23 commits into
registerPegoutTransaction-integrationfrom
add-gracePeriod-registerBtcTransaction
Oct 23, 2026
Merged

marcos-iov merged 23 commits into
registerPegoutTransaction-integrationfrom
add-gracePeriod-registerBtcTransaction

Conversation

@julianlen

@julianlen julianlen commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Description

We decided to drop the grace period for the pegout tx index after RSKIP643. A release created before RSKIP643 is only recorded in the pegout tx index, by sighash. A release created after it has its outputs to the live federations stored in federationsPendingBtcUTXOs. registerBtcTransaction now recognises both kinds at any BTC height, so there is no longer a height at which the Bridge stops looking up the pegout tx index.

This PR removes what was left of the grace period:

  • PegUtils.isFederationsPendingBtcUTXOsEnabled, which nothing called anymore.
  • btcHeightWhenFederationsPendingBtcUTXOsActivates and its getter from BridgeConstants, plus the placeholder values in the mainnet, testnet, testnet2 and regtest constants.

Production behaviour is the same as on the base branch. With RSKIP643 active, registerBtcTransaction handles releases like this:

Release Registered after RSKIP643
Created for Vetiver, only in the pegout tx index PEGOUT_OR_MIGRATION, outputs to the live federations are registered
Created for Cardamom, in federationsPendingBtcUTXOs processed through registerNewUTXOsByTxIdAndMarkTxHashAsProcessed

Motivation and Context

How Has This Been Tested?

  • BridgeSupportRegisterBtcTransactionTest
    • Pegouts with and without change, and migrations, created for Vetiver and created for Cardamom, all registered with every activation enabled. Vetiver releases register at HEIGHT_AT_WHICH_TO_START_USING_PEGOUT_INDEX. Cardamom releases register at HEIGHT_TO_USE_FEDERATIONS_PENDING_BTC_UTXOS, 100 blocks later.
    • New forVetiver cases for segwit pegouts and migrations in PegoutTransaction and MigrationTransaction.
    • A tx with change to the federation that is not in the pegout tx index is still rejected as a legacy pegin and refunded.
  • BridgeSupportSvpTest: the SVP fund tx created for Vetiver and created for Cardamom, registered while the SVP is ongoing and after its values were cleared.
  • BridgeConstantsTest: removed the getBtcHeightWhenFederationsPendingBtcUTXOsActivates test together with the constant.
  • PegUtilsTest: readability only, with static imports for PegTxType and one argument per line.

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:
    Gated by RSKIP643. The mainnet/testnet btcHeightWhenFederationsPendingBtcUTXOsActivates values on the base branch are placeholders (10_000_000) until the activation is scheduled.

@julianlen
julianlen requested a review from a team as a code owner October 21, 2026 19:20
Copilot AI balanced review requested due to automatic review settings October 21, 2026 19:20
@github-actions

Copy link
Copy Markdown

Dependency Review

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

Scanned Files

None

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Existing legacy-release registration tests still expect processing at heights beyond the new cutoff.

1 open issue
What changed in this PR

Updates bridge transaction classification to stop consulting the legacy pegout index after the RSKIP643 grace period.

Changes:

  • Passes bridge constants and BTC transaction height into classification.
  • Skips sighash computation and index lookup after the cutoff.
  • Adds grace-period classification tests for pegouts and migrations.
File Description
rskj-core/​src/​test/​java/​co/​rsk/​peg/​PegUtilsTest.java Updates classifier calls and tests grace-period boundaries.
rskj-core/​src/​main/​java/​co/​rsk/​peg/​PegUtils.java Gates legacy index lookup on the RSKIP643 cutoff.

Review effort: Balanced

Comment thread rskj-core/src/main/java/co/rsk/peg/PegUtils.java Outdated
Copilot AI balanced review requested due to automatic review settings October 21, 2026 20:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Consensus-sensitive bridge classification and documented placeholder activation heights require final human validation.

1 open issue

Review effort: Balanced

@julianlen
julianlen force-pushed the gracePeriod-generalization branch 2 times, most recently from 629ba82 to 83d7450 Compare October 21, 2026 23:15
@julianlen
julianlen force-pushed the add-gracePeriod-registerBtcTransaction branch from a75b273 to b6bd65c Compare October 21, 2026 23:21
Copilot AI balanced review requested due to automatic review settings October 21, 2026 23:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Consensus-sensitive release classification requires domain-owner validation of the cutoff heights and consequences for older release funds.

1 open issue

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Consensus-critical bridge classification and activation boundaries require final human validation with passing test results.

1 open issue

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 21, 2026 23:58
@julianlen
julianlen force-pushed the add-gracePeriod-registerBtcTransaction branch from 1a970c2 to b1960ac Compare October 21, 2026 23:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The consensus-sensitive change to legacy-release recognition needs human validation of activation thresholds and fund-registration behavior.

1 open issue

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 22, 2026 00:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The consensus-sensitive cutoff changes recognition of historical releases and registration of their funds, requiring final human review and confirmation of test results.

1 open issue

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 22, 2026 12:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Consensus-critical changes to release classification and refund behavior at the activation boundary require final review by bridge maintainers.

1 open issue

Review effort: Balanced

@marcos-iov
marcos-iov force-pushed the add-gracePeriod-registerBtcTransaction branch from 70e5184 to 1409e06 Compare October 22, 2026 22:46
Copilot AI balanced review requested due to automatic review settings October 22, 2026 22:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The cutoff changes consensus-critical handling of federation funds and refunds, requiring final bridge-maintainer review.

No open issues 🎉

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 23, 2026 14:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The registration tests do not exercise the intended RSKIP643 cutoff boundary, leaving the consensus-sensitive transition unverified.

1 open issue

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 23, 2026 15:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The supplied diff and stated cutoff behavior need human reconciliation before approving this consensus-sensitive bridge change.

1 open issue

Review effort: Balanced

@sonarqubecloud

Copy link
Copy Markdown

@julianlen
julianlen requested review from julia-zack and a balanced review from Copilot October 23, 2026 15:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

Production changes remove unused code and constants, and static review found no blocking issues in the accompanying test updates.

1 open issue

Review effort: Balanced

@julianlen julianlen changed the title feat: Stop looking up the pegout tx index in registerBtcTransaction after the RSKIP643 grace period refactor: remove the RSKIP643 pegout tx index grace period Oct 23, 2026
@marcos-iov
marcos-iov merged commit 461300b into registerPegoutTransaction-integration Oct 23, 2026
10 checks passed
@marcos-iov
marcos-iov deleted the add-gracePeriod-registerBtcTransaction branch October 23, 2026 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants