Mark invalid pegin as processed #2910

nathanieliov · 2025-01-02T06:06:55Z

Mark rejected pegins as processed once RSKIP459 is active.

Motivation and Context

Peg-in transactions rejected by the Bridge should be marked as processed to ensure that the rejected_pegin event is emitted only once per transaction.

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)

github-actions · 2025-01-02T06:07:11Z

Dependency Review

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

OpenSSF Scorecard

Package	Version	Score	Details

Scanned Manifest Files

…jected pegin should be marked as processed - Update BridgeSupportRegisterBtcTransactionTest to assert rejected pegin are marked as processed since RSKIP459 - Add tests asserting rejected pegin are marked as processed once RSKIP459 is active

jeremy-then · 2025-01-02T21:07:43Z

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

+    private static final NetworkParameters btcMainnetParams = bridgeMainnetConstants.getBtcParams();
+    private static final ActivationConfig.ForBlock arrowHeadActivations = ActivationConfigsForTest.arrowhead600().forBlock(0);
+    private static final ActivationConfig.ForBlock lovellActivations = ActivationConfigsForTest.lovell700().forBlock(0);
+    /*private static final ActivationConfig.ForBlock activations = ActivationConfigsForTest.all().forBlock(0);*/


Will we ever need this line? If not, maybe we can remove it.

Good catch! It can be removed. Thanks.

jeremy-then

LGTM.

apancorb · 2025-01-03T10:20:30Z

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

+        logger.debug("[{}] Rejected peg-in, reason {}", METHOD_NAME, rejectedPeginReason);
+        eventLogger.logRejectedPegin(btcTx, rejectedPeginReason);
+
+        switch (peginProcessAction) {


Maybe we can use here the new switch expression with arrow labels

apancorb · 2025-01-03T10:24:44Z

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

+        // If the peg-in cannot be registered means it should be rejected
+        Optional<RejectedPeginReason> rejectedPeginReasonOptional = peginEvaluationResult.getRejectedPeginReason();
+        if (rejectedPeginReasonOptional.isEmpty()) {
+            // This flow should never be reached. There should always be a rejected pegin reason.
+            String message = "Invalid state. No rejected reason was returned for an invalid pegin.";
+            logger.error("[{}] {}", METHOD_NAME, message);
+            throw new IllegalStateException(message);
+        }
+        RejectedPeginReason rejectedPeginReason = rejectedPeginReasonOptional.get();
+        logger.debug("[{}] Rejected peg-in, reason {}", METHOD_NAME, rejectedPeginReason);
+        eventLogger.logRejectedPegin(btcTx, rejectedPeginReason);


Maybe this can be made more functional since we are using the Optional API as:

reason = peginEvaluationResult.getRejectedPeginReason() .map(rejectedPeginReason -> { logger.debug("[{}] Rejected peg-in, reason {}", METHOD_NAME, rejectedPeginReason); eventLogger.logRejectedPegin(btcTx, rejectedPeginReason); return rejectedPeginReason; // Placeholder if further processing is needed }) .orElseThrow(() -> { String message = "Invalid state. No rejected reason was returned for an invalid pegin."; logger.error("[{}] {}", METHOD_NAME, message); return new IllegalStateException(message); });

Also would it make sense to abstract this logic to a private method?

Just this part regarding getting the rejected pegin reason? If so, to me looks clear how it is now.

apancorb · 2025-01-03T10:27:27Z

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

@@ -1543,12 +1555,11 @@ void pegin_legacy_from_segwit_to_active_fed_cannot_be_processed(
        );

        // assert
-
-        // SINCE RSKIP379 ONLY TRANSACTIONS THAT REALLY ARE PROCESSED, REFUNDS OR REGISTER WILL BE MARK AS PROCESSED.
        if (activations == fingerrootActivations){


Suggested change

if (activations == fingerrootActivations){

if (activations == fingerrootActivations) {

apancorb · 2025-01-03T10:35:56Z

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

+            btcHeightWhenPegoutTxIndexActivates + pegoutTxIndexGracePeriodInBtcBlocks;
+    }
+
+    private PartialMerkleTree createPmtAndMockBlockStore(BtcTransaction btcTransaction)


Wasn't there a method in utils already for this? Maybe I am wrong

Did you intend to remove this method?

No, this method is needed to recreate a valid chain for the given tx to satisfy the checks in registerBtcTransaction method

apancorb · 2025-01-03T10:37:55Z

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

+         This is a hypothetical case, which is not realistic with the implementation of the svp.
+         A btc tx hash that is not saved as a spend tx, is identified as a pegin, and will be
+         rejected due to invalid amount. This is because the pegin amount is below the minimum.
+         Therefore, this tx should be rejected as pegin and mark as processed


Suggested change

This is a hypothetical case, which is not realistic with the implementation of the svp.

A btc tx hash that is not saved as a spend tx, is identified as a pegin, and will be

rejected due to invalid amount. This is because the pegin amount is below the minimum.

Therefore, this tx should be rejected as pegin and mark as processed

This is a hypothetical case, which is not realistic with the implementation of the svp.

A btc tx hash that is not saved as a spend tx is identified as a pegin and will be

rejected due to invalid amount, since the pegin amount is below the minimum.

Therefore, this tx should be rejected as pegin and mark as processed.

- Make logic handling rejected pegin functional using the Optional API - Fix formatting

apancorb

LGTM

marcos-iov · 2025-01-08T17:32:47Z

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

-    private void handleUnprocessableBtcTx(
+    private RejectedPeginReason handleRejectedPegin(BtcTransaction btcTx,
+        PeginEvaluationResult peginEvaluationResult) {
+        return peginEvaluationResult.getRejectedPeginReason().map(reason -> {


I don't think it's a good idea to return the rejected peg-in reason here. The method is called handle so I would expect it to do something but not to return anything.

The rejected reason is obtained from PeginEvaluationResult that is received by param. So it's basically returning something that was passed by parameter. Why can't the caller simply get that info from the object?

Maybe this method should renamed to logRejectedPegin? That's what it's doing right?

Renamed and rearrange

marcos-iov · 2025-01-08T17:33:17Z

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

+        });
+    }
+
+    private void handleNonRefundablePegin(


Perhaps the same renaming idea could apply here

marcos-iov · 2025-01-08T17:40:51Z

rskj-core/src/main/java/co/rsk/peg/pegin/PeginProcessAction.java

@@ -3,5 +3,5 @@
 public enum PeginProcessAction {
    CAN_BE_REGISTERED,
    CAN_BE_REFUNDED,
-    CANNOT_BE_PROCESSED
+    CANNOT_BE_REFUNDED


Just an idea, to rename this for clarity

REGISTER, REFUND, NO_REFUND

What do you think? A bit more straightforward. Last one could also be IGNORE perhaps, not sure

Looks simpler. I'll update it.

marcos-iov · 2025-01-08T17:42:17Z

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

+    private static final NetworkParameters btcMainnetParams = bridgeMainnetConstants.getBtcParams();
+    private static final ActivationConfig.ForBlock arrowHeadActivations = ActivationConfigsForTest.arrowhead600()
+        .forBlock(0);
+    private static final ActivationConfig.ForBlock lovellActivations = ActivationConfigsForTest.lovell700()


Shouldn't we use all activations? To make sure this keeps working in future network upgrades

marcos-iov · 2025-01-08T17:43:19Z

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

+            btcHeightWhenPegoutTxIndexActivates + pegoutTxIndexGracePeriodInBtcBlocks;
+    }
+
+    private PartialMerkleTree createPmtAndMockBlockStore(BtcTransaction btcTransaction)


Did you intend to remove this method?

marcos-iov · 2025-01-09T19:56:47Z

rskj-core/src/main/java/co/rsk/peg/utils/NoRefundPeginReason.java

@@ -1,13 +1,13 @@
 package co.rsk.peg.utils;

-public enum UnrefundablePeginReason {
+public enum NoRefundPeginReason {


Suggested change

public enum NoRefundPeginReason {

public enum NonRefundablePeginReason {

I think that sounds better

marcos-iov · 2025-01-09T19:57:14Z

rskj-core/src/main/java/co/rsk/peg/utils/BridgeEventLoggerImpl.java

@@ -212,7 +212,7 @@ public void logRejectedPegin(BtcTransaction btcTx, RejectedPeginReason reason) {
    }

    @Override
-    public void logUnrefundablePegin(BtcTransaction btcTx, UnrefundablePeginReason reason) {
+    public void logNoRefundPegin(BtcTransaction btcTx, NoRefundPeginReason reason) {


Suggested change

public void logNoRefundPegin(BtcTransaction btcTx, NoRefundPeginReason reason) {

public void logNonRefundablePegin(BtcTransaction btcTx, NoRefundPeginReason reason) {

marcos-iov · 2025-01-09T19:59:29Z

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

-                logger.debug("[{}] Unprocessable transaction {}.", METHOD_NAME, btcTx.getHash());
-                handleUnprocessableBtcTx(btcTx, peginInformation.getProtocolVersion(), rejectedPeginReason);
+            }
+            case NO_REFUND -> {


Suggested change

case NO_REFUND -> {

case NO_REFUND ->

See Sonar report

marcos-iov · 2025-01-09T20:01:47Z

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

+                peginEvaluationResult.getRejectedPeginReason()
+                    .map(reason -> {
+                        logger.debug("[{}] Rejected peg-in, reason {}", METHOD_NAME, reason);
+                        eventLogger.logRejectedPegin(btcTx, reason);
+                        return reason;
+                    }).orElseThrow(() -> {
+                        // This flow should never be reached. There should always be a rejected pegin reason.
+                        String message = "Invalid state. No rejected reason was returned for an invalid pegin.";
+                        logger.error("[{}] {}", METHOD_NAME, message);
+                        return new IllegalStateException(message);
+                    });
+
+                logger.debug("[{}] Refunding to address {} ",
+                    METHOD_NAME, peginInformation.getBtcRefundAddress());
+                generateRejectionRelease(btcTx, peginInformation.getBtcRefundAddress(), rskTxHash,
+                    totalAmount);


Looked better when all this logic was in its own method, why move it all here? It creates a code smell

Because of the number of parameters that needs to be passed.

handleRefundablePegin(BtcTransaction btcTx, Keccak256 rskTxHash, PeginEvaluationResult peginEvaluationResult, Address btcRefundAddress, Coin totalAmount)

marcos-iov · 2025-01-09T20:03:40Z

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

+                    .map(reason -> {
+                        logger.debug("[{}] Rejected peg-in, reason {}", METHOD_NAME, reason);
+                        eventLogger.logRejectedPegin(btcTx, reason);
+                        return reason;


Do you need this return here? Nobody is storing that value

You right. It can be removed.

- Enhance switch for processing each PeginProcessAction

sonarqubecloud · 2025-01-09T21:16:20Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

Mark rejected pegins as processed once RSKIP459 is active

aca9920

nathanieliov requested a review from a team January 2, 2025 06:06

nathanieliov force-pushed the mark-invalid-pegin-as-processed branch from d8f72bb to 208b14f Compare January 2, 2025 19:01

nathanieliov marked this pull request as ready for review January 2, 2025 19:02

jeremy-then reviewed Jan 2, 2025

View reviewed changes

jeremy-then approved these changes Jan 2, 2025

View reviewed changes

apancorb reviewed Jan 3, 2025

View reviewed changes

- Use new switch with arrow labels

b2a1d48

- Make logic handling rejected pegin functional using the Optional API - Fix formatting

apancorb approved these changes Jan 7, 2025

View reviewed changes

Improve switch use to handle pegins

61bd76f

marcos-iov reviewed Jan 8, 2025

View reviewed changes

marcos-iov reviewed Jan 9, 2025

View reviewed changes

nathanieliov force-pushed the mark-invalid-pegin-as-processed branch from 39a23fd to 38f7bf2 Compare January 9, 2025 20:49

- Rename UnrefundablePeginReason to NonRefundablePeginReason

bf15222

- Enhance switch for processing each PeginProcessAction

nathanieliov force-pushed the mark-invalid-pegin-as-processed branch from 38f7bf2 to bf15222 Compare January 9, 2025 21:01

marcos-iov approved these changes Jan 10, 2025

View reviewed changes

marcos-iov merged commit ac0df42 into invalid-pegin-as-processed-integration Jan 10, 2025
9 checks passed

	if (activations == fingerrootActivations){
	if (activations == fingerrootActivations) {

	public enum NoRefundPeginReason {
	public enum NonRefundablePeginReason {

	public void logNoRefundPegin(BtcTransaction btcTx, NoRefundPeginReason reason) {
	public void logNonRefundablePegin(BtcTransaction btcTx, NoRefundPeginReason reason) {

Mark invalid pegin as processed #2910

Mark invalid pegin as processed #2910

Conversation

nathanieliov commented Jan 2, 2025 • edited Loading

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

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

Dependency Review

OpenSSF Scorecard

Scanned Manifest Files

Choose a reason for hiding this comment

nathanieliov Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

jeremy-then left a comment

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apancorb left a comment

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

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

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

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

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 9, 2025

Quality Gate passed

nathanieliov commented Jan 2, 2025 •

edited

Loading

github-actions bot commented Jan 2, 2025 •

edited

Loading

nathanieliov Jan 2, 2025 •

edited

Loading