Create processValidationFailure method #2817

julia-zack · 2024-10-21T21:07:29Z

After concluding the validation was a failure, we need to emit the commitFederationFailed event and reset all the values that are used in the process for the federation change to be allowed again.

In this pr, a new processValidationFailure method is created:

emit the new commitFederationFailed Bridge event
clear the proposedFederation storage entry
clear the svp fund tx storage entries
clear the expected svp spend tx storage entry/ies

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

apancorb

Nice job, left some comments with a big focus on clearSvpValues

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

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

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

rskj-core/src/main/java/co/rsk/peg/federation/FederationSupportImpl.java

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

…ate processValidationFailure method

…or new behavior

github-actions · 2024-10-24T22:29:51Z

Dependency Review

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

OpenSSF Scorecard

Package	Version	Score	Details

Scanned Manifest Files

marcos-iov

Looking good, well done. Just a couple of style suggestions

marcos-iov · 2024-10-24T22:37:51Z

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

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

+    protected void processValidationFailure(Federation proposedFederation) {


Suggested change

protected void processValidationFailure(Federation proposedFederation) {

protected void processSVPFailure(Federation proposedFederation) {

What do you think? To give it context since it's part of BridgeSupport class and there's all of type of stuff in there

makes sense, thanks!

marcos-iov · 2024-10-24T22:39:02Z

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

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

+    protected void processValidationFailure(Federation proposedFederation) {
+        eventLogger.logCommitFederationFailure(rskExecutionBlock, proposedFederation);
+        logger.warn("[processValidationFailure] Proposed federation validation failed so svp values will be cleared.");


Should we log something else? The block number where it failed maybe? We could then look for the event in that block

good idea yes

marcos-iov · 2024-10-24T22:40:16Z

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

+
+        provider.getSvpFundTxHashUnsigned().ifPresent(
+            svpFundTxHashUnsigned -> {
+                logger.warn("[clearSvpValues] Fund tx change {} was never registered.", svpFundTxHashUnsigned);


marcos-iov · 2024-10-24T22:47:22Z

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

+        clearSvpValues();
+    }
+
+    private void clearSvpValues() {


Should this maybe be part of BridgeStorageProvider class? That would imply adding tests to it 😬

you right, unfortunately for me

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

marcos-iov · 2024-10-24T22:52:50Z

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

+            // assert
+            assertLogCommitFederationFailed();
+            assertProposedFederationWasCleared();
+            assertFalse(bridgeStorageProvider.getSvpFundTxHashUnsigned().isPresent());


Perhaps have a method that checks that none of the SVP related values are present? fundTx, spendTx, tx wfs, etc

…be called from support.

marcos-iov · 2024-10-29T20:39:49Z

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

+            bridgeStorageProvider.clearSvpValues();
+
+            // assert
+            assertFalse(bridgeStorageProvider.getSvpFundTxHashUnsigned().isPresent());


I would assert that no svp value is present, in all tests

Same as in BridgeSupportSvpTest

… blank line

marcos-iov · 2024-10-29T21:09:15Z

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

+        clearProposedFederation();
+        clearSvpValues();
+    }
+
+    private void clearProposedFederation() {
+        federationSupport.clearProposedFederation();
+    }
+
+    private void clearSvpValues() {
+        provider.clearSvpValues();
+    }


Suggested change

clearProposedFederation();

clearSvpValues();

}

private void clearProposedFederation() {

federationSupport.clearProposedFederation();

}

private void clearSvpValues() {

provider.clearSvpValues();

}

federationSupport.clearProposedFederation();

provider.clearSvpValues();

}

Makes sense?

…r refactor for clearing svp values in support

sonarqubecloud · 2024-10-30T14:20:25Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

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

See analysis details on SonarCloud

julia-zack commented Oct 22, 2024

View reviewed changes

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

julia-zack commented Oct 22, 2024

View reviewed changes

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

julia-zack marked this pull request as ready for review October 22, 2024 12:58

julia-zack requested a review from a team as a code owner October 22, 2024 12:58

apancorb reviewed Oct 22, 2024

View reviewed changes

nathanieliov reviewed Oct 22, 2024

View reviewed changes

apancorb approved these changes Oct 23, 2024

View reviewed changes

nathanieliov approved these changes Oct 23, 2024

View reviewed changes

marcos-iov force-pushed the feature/powpeg_validation_protocol-phase4 branch from e99b87a to 4e9f573 Compare October 24, 2024 22:27

julia-zack added 8 commits October 24, 2024 19:29

Make logs a global variable and remove it from methods arguments. Cre…

bca315c

…ate processValidationFailure method

Correct log

5f5c63f

Correct test class name

57bff23

Modify clearSvpValues to know and log which stage failed. Add tests f…

b5e801a

…or new behavior

Add fund tx signed hash log

227c718

Make events vbles static

53170bf

Refactor clearSvpValues to make code more functional

d9199e2

Add unit test for new clearProposedFederation method

085a14e

marcos-iov force-pushed the process_validation_failure branch from bc59eea to 085a14e Compare October 24, 2024 22:29

marcos-iov reviewed Oct 24, 2024

View reviewed changes

Improve naming and logs. Create clearSvpValues method in provider to …

298fbbb

…be called from support.

marcos-iov reviewed Oct 29, 2024

View reviewed changes

Assert any of svp values are present when clearing value. Add missing…

0016675

… blank line

marcos-iov reviewed Oct 29, 2024

View reviewed changes

Make logging more accurate when clearing svp values in provider. Mino…

e45e32e

…r refactor for clearing svp values in support

marcos-iov approved these changes Oct 30, 2024

View reviewed changes

marcos-iov merged commit 4697a0a into feature/powpeg_validation_protocol-phase4 Oct 30, 2024
8 checks passed

marcos-iov deleted the process_validation_failure branch October 30, 2024 14:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create processValidationFailure method #2817

Create processValidationFailure method #2817

julia-zack commented Oct 21, 2024 •

edited

Loading

apancorb left a comment

github-actions bot commented Oct 24, 2024 •

edited

Loading

marcos-iov left a comment

marcos-iov Oct 24, 2024

julia-zack Oct 29, 2024

julia-zack Oct 29, 2024

marcos-iov Oct 24, 2024

julia-zack Oct 29, 2024

julia-zack Oct 29, 2024

marcos-iov Oct 24, 2024

marcos-iov Oct 24, 2024

julia-zack Oct 29, 2024

julia-zack Oct 29, 2024

julia-zack Oct 29, 2024

marcos-iov Oct 24, 2024

julia-zack Oct 29, 2024

marcos-iov Oct 29, 2024

marcos-iov Oct 29, 2024

julia-zack Oct 29, 2024

julia-zack Oct 29, 2024

marcos-iov Oct 29, 2024

sonarqubecloud bot commented Oct 30, 2024

	protected void processValidationFailure(Federation proposedFederation) {
	protected void processSVPFailure(Federation proposedFederation) {

Create processValidationFailure method #2817

Create processValidationFailure method #2817

Conversation

julia-zack commented Oct 21, 2024 • edited Loading

apancorb left a comment

Choose a reason for hiding this comment

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

Dependency Review

OpenSSF Scorecard

Scanned Manifest Files

marcos-iov 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

sonarqubecloud bot commented Oct 30, 2024

Quality Gate passed

julia-zack commented Oct 21, 2024 •

edited

Loading

github-actions bot commented Oct 24, 2024 •

edited

Loading