Create updateSvpState method #2838

julia-zack · 2024-10-31T20:42:00Z

Background
In order to keep going with the validation process, in every updateCollections we will call a new validation-related method, updateSvpState, that will update the state of the svp process to see if the validation failed, or if conditions are met for creating the fund or the spend transaction.

IN THIS PR
Create a new updateSvpState method that will:

If a proposed federation does not exist, do nothing.
If a proposed federation exists and the validation period is ended, we can conclude that the validation failed.
If a proposed federation exists, the validation period is not ended and the svpFundTxHashUnigned entry is empty, we have to proceed with the fund transaction creation and processing.
If a proposed federation exists, the validation period is not ended and the svpFundTxSigned has a value stored, we will proceed with the spend tx creation.

github-actions · 2024-10-31T20:42:17Z

Dependency Review

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

OpenSSF Scorecard

Package	Version	Score	Details

Scanned Manifest Files

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

apancorb · 2024-11-01T10:40:30Z

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

+        // if the fund tx signed is present, then the fund transaction change was registered,
+        // meaning we can create the spend tx.
+        Optional<BtcTransaction> svpFundTxSigned = provider.getSvpFundTxSigned();
+        if (svpFundTxSigned.isPresent()) {
+            processSvpSpendTransactionUnsigned(rskTxHash, proposedFederation, svpFundTxSigned.get());
+            return;
+        }
+
+        // if the unsigned fund tx hash is not present, meaning we can proceed with the fund tx creation
+        Optional<Sha256Hash> svpFundTxHashUnsigned = provider.getSvpFundTxHashUnsigned();
+        if (svpFundTxHashUnsigned.isEmpty()) {
+            try {
+                processSvpFundTransactionUnsigned(rskTxHash, proposedFederation);
+            } catch (Exception e) {
+                logger.error("[updateSvpState] Error processing svp fund transaction unsigned.");
+            }
+        }


I was thinking on how we can change the order of these two logic blocks get executed. I think it would be easier to understand if the logic related to processing svpFundTxHashUnsigned happens first than the svpFundTxSigned. Maybe we can do something like this:

// Check if the unsigned fund transaction hash is not present, and create the fund transaction if needed Optional<Sha256Hash> svpFundTxHashUnsigned = provider.getSvpFundTxHashUnsigned(); if (svpFundTxHashUnsigned.isEmpty()) { try { processSvpFundTransactionUnsigned(rskTxHash, proposedFederation); } catch (Exception e) { ... } return; } // Check if the signed fund transaction is present, and create the spend transaction if available Optional<BtcTransaction> svpFundTxSigned = provider.getSvpFundTxSigned(); if (svpFundTxSigned.isPresent()) { processSvpSpendTransactionUnsigned(rskTxHash, proposedFederation, svpFundTxSigned.get()); }

remember that when we save the svpFundTxSigned we remove the svpFundTxHashUnsigned. So this order won't have the expected behavior

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

sonarcloud · 2024-11-04T20:46:27Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

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

See analysis details on SonarCloud

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

apancorb · 2024-11-05T11:05:04Z

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

+            logger.error(
+                "[processSvpFundTransactionUnsigned] IOException getting the pegouts waiting for confirmations. Error message: {}",
+                e.getMessage()


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

apancorb

LGTM, nice job

nathanieliov

LGTM

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

marcos-iov · 2024-11-11T19:03:53Z

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

+        // if the fund tx signed is present, then the fund transaction change was registered,
+        // meaning we can create the spend tx.
+        Optional<BtcTransaction> svpFundTxSigned = provider.getSvpFundTxSigned();
+        if (svpFundTxSigned.isPresent()) {


Should we also be checking that there is no spend tx created? Just in case.

I know there should only be one at a time so if both are found perhaps we have a problem

marcos-iov · 2024-11-11T19:14:53Z

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

@@ -1017,13 +1017,51 @@ private void logUpdateCollections(Transaction rskTx) {
        eventLogger.logUpdateCollections(sender);
    }

-    protected void processSVPFailure(Federation proposedFederation) {
+    protected void updateSvpState(Transaction rskTx) {


To be honest, I'd find it easier to follow if the logic was written in chronological order. I know it requires more validations but we can probably make use of auxiliary methods. The way it's written now, for every new condition that appears in the code, you need to keep in mind the previous condition that was already validated a few lines above and the protocol logic about one having one SVP related value in storage at once.

I'd go with a very straight forward approach

if (!proposedFederationExists) return; if (validationPeriodIsOngoing) { if (shouldCreateSvpFundTx()) { createFundTx() return; } if (shouldCreateSvpSpendTx()) { createSpendTx() return; } validationFailed(); }

What do you think? I think that's the only 3 cases we need to cover

makes sense, will update

marcos-iov · 2024-11-11T19:15:53Z

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

-        provider.setSvpFundTxHashUnsigned(svpFundTransactionUnsigned.getHash());
-        PegoutsWaitingForConfirmations pegoutsWaitingForConfirmations = provider.getPegoutsWaitingForConfirmations();
-        settleReleaseRequest(pegoutsWaitingForConfirmations, svpFundTransactionUnsigned, rskTx.getHash(), spendableValueFromProposedFederation);
+    private void processSvpFundTransactionUnsigned(Keccak256 rskTxHash, Federation proposedFederation) {


Maybe call this create rather than process?

i named it process to be consistent with processPegoutOrMigration method. But no big deal, I can change it :)

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

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

Minor refactors to avoid duplication code Rewrite comment Fix sonar complains Handle exception when processing fund transaction. Add logs for when updating svp state Add debug level log for when proposed fed does not exist Not allow validation period end block to be part of validation ongoing period Make updateSvpState private. Call it from updateCollections Add missing import Fix test

sonarcloud · 2024-11-13T15:38:10Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

julia-zack marked this pull request as ready for review October 31, 2024 20:53

julia-zack requested a review from a team as a code owner October 31, 2024 20:53

apancorb reviewed Nov 1, 2024

View reviewed changes

julia-zack force-pushed the update_state_svp branch 5 times, most recently from 45b45d3 to d9cf8e0 Compare November 4, 2024 20:34

apancorb reviewed Nov 5, 2024

View reviewed changes

nathanieliov reviewed Nov 5, 2024

View reviewed changes

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

apancorb approved these changes Nov 7, 2024

View reviewed changes

julia-zack requested a review from nathanieliov November 7, 2024 15:06

nathanieliov approved these changes Nov 7, 2024

View reviewed changes

marcos-iov reviewed Nov 11, 2024

View reviewed changes

marcos-iov force-pushed the feature/powpeg_validation_protocol-phase4 branch from 38c0b3f to 7a04c05 Compare November 12, 2024 19:58

marcos-iov force-pushed the update_state_svp branch from a84f8a7 to 272f561 Compare November 13, 2024 13:49

julia-zack force-pushed the update_state_svp branch from 6994f73 to 0c6aa68 Compare November 13, 2024 13:57

Correct and make updateSvpState easier to follow

4083234

julia-zack force-pushed the update_state_svp branch from 0c6aa68 to 4083234 Compare November 13, 2024 15:23

marcos-iov approved these changes Nov 13, 2024

View reviewed changes

marcos-iov merged commit 3858201 into feature/powpeg_validation_protocol-phase4 Nov 13, 2024
8 checks passed

marcos-iov deleted the update_state_svp branch November 13, 2024 20:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create updateSvpState method #2838

Create updateSvpState method #2838

julia-zack commented Oct 31, 2024 •

edited

Loading

github-actions bot commented Oct 31, 2024 •

edited

Loading

apancorb Nov 1, 2024

julia-zack Nov 4, 2024

julia-zack Nov 13, 2024

sonarcloud bot commented Nov 4, 2024

apancorb Nov 5, 2024

apancorb left a comment

nathanieliov left a comment

marcos-iov Nov 11, 2024

marcos-iov Nov 11, 2024

julia-zack Nov 11, 2024

julia-zack Nov 13, 2024

marcos-iov Nov 11, 2024

julia-zack Nov 11, 2024

sonarcloud bot commented Nov 13, 2024

Create updateSvpState method #2838

Create updateSvpState method #2838

Conversation

julia-zack commented Oct 31, 2024 • edited Loading

github-actions bot commented Oct 31, 2024 • edited Loading

Dependency Review

OpenSSF Scorecard

Scanned Manifest Files

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Nov 4, 2024

Quality Gate passed

Choose a reason for hiding this comment

apancorb left a comment

Choose a reason for hiding this comment

nathanieliov 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

sonarcloud bot commented Nov 13, 2024

Quality Gate passed

julia-zack commented Oct 31, 2024 •

edited

Loading

github-actions bot commented Oct 31, 2024 •

edited

Loading