Svp success #2854

julia-zack · 2024-11-20T19:52:35Z

No description provided.

github-actions · 2024-11-20T19:52:55Z

Dependency Review

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

OpenSSF Scorecard

Package	Version	Score	Details

Scanned Manifest Files

julia-zack · 2024-11-21T12:43:49Z

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

-        if (activations.isActive(RSKIP186)) {
-            // since we are creating the to-be-active-fed in this block,
-            // its creation block height is this block number
-            saveFederationChangeInfo(rskExecutionBlock.getNumber());
-        }


this is being done in handover method now

julia-zack · 2024-11-21T12:49:53Z

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

removing the re-declaration of activations since its being done in the setUp

jeremy-then · 2024-11-21T15:24:57Z

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

+
+    public static Script getFederationMembersP2SHScript(ActivationConfig.ForBlock activations, Federation federation) {
+        // when the federation is a standard multisig,
+        // the members p2sh script is the p2sh script


nit: I think there's space for this to be in a single line with the above. It's a bit confusing I think.

jeremy-then · 2024-11-21T15:25:06Z

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

+        }
+
+        // when the federation also has erp keys,
+        // the members p2sh script is the default p2sh script


The same here.

apancorb

Looking good, just once thing we might what to make the base branch this one #2857

apancorb · 2024-11-22T09:54:58Z

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

@@ -52,8 +53,7 @@
 import java.util.List;
 import java.util.stream.Collectors;

-import static org.ethereum.config.blockchain.upgrades.ConsensusRule.RSKIP284;
-import static org.ethereum.config.blockchain.upgrades.ConsensusRule.RSKIP293;
+import static org.ethereum.config.blockchain.upgrades.ConsensusRule.*;


nit: move the static imports to top file

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

apancorb · 2024-11-22T10:07:51Z

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

+        if (!(federation instanceof ErpFederation)) {
+            return federation.getP2SHScript();
+        }
+
+        // when the federation also has erp keys,
+        // the members p2sh script is the default p2sh script
+        return ((ErpFederation) federation).getDefaultP2SHScript();


Is there a way we can avoid doing explicit casting?

not right now

apancorb · 2024-11-22T10:08:15Z

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

        Federation currentOldFederation = provider.getOldFederation(constants, activations);
        Federation currentNewFederation = provider.getNewFederation(constants, activations);
        logCommitmentWithVotedFederation(eventLogger, currentOldFederation, currentNewFederation);

        return FederationChangeResponseCode.SUCCESSFUL;
    }

+    public void commitProposedFederation() {
+        Federation proposedFederation = provider.getProposedFederation(constants, activations)
+                .orElseThrow(IllegalStateException::new);


nit: big identation, only four spaces

apancorb · 2024-11-22T10:10:08Z

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

-        List<UTXO> utxosToMove = new ArrayList<>(provider.getNewFederationBtcUTXOs(constants.getBtcParams(), activations));
+        // since the current active fed reference will change from being 'new' to 'old',
+        // we have to change the UTXOs reference to match it
+        List<UTXO> activeFederationUTXOs = new ArrayList<>(provider.getNewFederationBtcUTXOs(constants.getBtcParams(), activations));


why pass the list to the constructor of another list?

I think is to create a shallow copy of the list before clear the original one. Btw, you can do this instead:

Suggested change

List<UTXO> activeFederationUTXOs = new ArrayList<>(provider.getNewFederationBtcUTXOs(constants.getBtcParams(), activations));

List<UTXO> activeFederationUTXOs = provider.getNewFederationBtcUTXOs(constants.getBtcParams(), activations).stream().toList();

Or simply use List.copyOf

apancorb · 2024-11-22T10:12:30Z

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

 import co.rsk.peg.utils.BridgeEventLogger;
 import co.rsk.peg.utils.BridgeEventLoggerImpl;
 import co.rsk.test.builders.BridgeSupportBuilder;
 import java.io.IOException;
 import java.util.*;
 import java.util.stream.IntStream;
+


nit: remove blank line

apancorb · 2024-11-22T10:15:53Z

rskj-core/src/test/java/co/rsk/peg/federation/VoteFederationChangeTest.java

+import static co.rsk.peg.BridgeUtils.getFederationMembersP2SHScript;
 import static co.rsk.peg.bitcoin.BitcoinTestUtils.flatKeysAsByteArray;
-import static org.ethereum.config.blockchain.upgrades.ConsensusRule.RSKIP377;
+import static co.rsk.peg.federation.FederationStorageIndexKey.NEW_FEDERATION_BTC_UTXOS_KEY;
 import static org.junit.jupiter.api.Assertions.*;
 import static org.mockito.Mockito.mock;


nit: I would move the static imports to the top for consistency

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

rskj-core/src/test/java/co/rsk/peg/federation/VoteFederationChangeTest.java

nathanieliov · 2024-11-22T15:54:29Z

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

        }

        @Test
-        void registerBtcTransaction_whenIsTheSpendTransaction_shouldProcessSpendTx() throws BlockStoreException, BridgeIllegalArgumentException, IOException {
+        void registerBtcTransaction_whenIsNotTheSpendTransaction_shouldNotProcessSpendTx() throws BlockStoreException, BridgeIllegalArgumentException, IOException {


Should we add the test cases when tx is a PEGIN or UNKNOWN also?

the pegin case could be, the unknown i dont think is necessary.
but we definitely should be testing that the spend tx is being processed even when it does not have the minimum pegin value! So thanks

This is interesting, if the spend tx is below the min peg-in value I don't think the powpeg will inform it to the Bridge

nathanieliov · 2024-11-22T16:01:57Z

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

        }

        @Test
-        void registerBtcTransaction_whenSpendTransactionHashIsNotSaved_shouldNotProcessSpendTx() throws BlockStoreException, BridgeIllegalArgumentException, IOException {
+        void registerBtcTransaction_whenIsTheSpendTransaction_shouldProcessSpendTxAndSvpSuccess() throws BlockStoreException, BridgeIllegalArgumentException, IOException {


I think we are missing some border cases:

Trying to register the spend transaction for a second time. What is the expected result?

Trying to register the spend transaction spent by the ERP Flow. I guess it should be registered if the transaction arrives on time.

Trying to register the spend transaction for a second time. What is the expected result?

the tx is being marked as processed when registered, same as any other tx. So it won't be registered again. I will add a test for that case

Trying to register the spend transaction spent by the ERP Flow. I guess it should be registered if the transaction arrives on time.

Well, no. We specifically wanna test that the proposed federators can spend.

Not sure I followed the second part, you need a year to sign ith the ERP flow

True, the value set is 1 year. But shouldn't we also check that we can spend using the ERP Flow? Just an idea that came to my mind.

#2854 (comment)

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

marcos-iov · 2024-11-22T16:53:54Z

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

@@ -619,4 +619,17 @@ public static int calculatePegoutTxSize(ActivationConfig.ForBlock activations, F

        return baseSize + signingSize;
    }
+
+    public static Script getFederationMembersP2SHScript(ActivationConfig.ForBlock activations, Federation federation) {


This is quite confusing, really hard to understand what's going on.

What's the idea? Get the default p2sh script or the full p2sh script? Seems like any of those can be achieved through Federation class methods

marcos-iov · 2024-11-22T17:13:38Z

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

-        List<UTXO> utxosToMove = new ArrayList<>(provider.getNewFederationBtcUTXOs(constants.getBtcParams(), activations));
+        // since the current active fed reference will change from being 'new' to 'old',
+        // we have to change the UTXOs reference to match it
+        List<UTXO> activeFederationUTXOs = new ArrayList<>(provider.getNewFederationBtcUTXOs(constants.getBtcParams(), activations));


Or simply use List.copyOf

marcos-iov · 2024-11-22T17:14:51Z

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

        }

        @Test
-        void registerBtcTransaction_whenSpendTransactionHashIsNotSaved_shouldNotProcessSpendTx() throws BlockStoreException, BridgeIllegalArgumentException, IOException {
+        void registerBtcTransaction_whenIsTheSpendTransaction_shouldProcessSpendTxAndSvpSuccess() throws BlockStoreException, BridgeIllegalArgumentException, IOException {


Not sure I followed the second part, you need a year to sign ith the ERP flow

…e called from bridge

…ridgeUtils

sonarcloud · 2024-11-25T13:13:16Z

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

julia-zack force-pushed the svp_success branch 2 times, most recently from 8c79e6e to 41fccdc Compare November 20, 2024 20:10

julia-zack commented Nov 21, 2024

View reviewed changes

julia-zack marked this pull request as ready for review November 21, 2024 15:00

julia-zack requested a review from a team as a code owner November 21, 2024 15:00

jeremy-then reviewed Nov 21, 2024

View reviewed changes

Base automatically changed from register_spend_tx to feature/powpeg_validation_protocol-phase4 November 21, 2024 20:53

apancorb reviewed Nov 22, 2024

View reviewed changes

julia-zack force-pushed the svp_success branch from 20afc09 to 1d0aabe Compare November 22, 2024 12:56

nathanieliov reviewed Nov 22, 2024

View reviewed changes

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

apancorb approved these changes Nov 22, 2024

View reviewed changes

nathanieliov reviewed Nov 22, 2024

View reviewed changes

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

nathanieliov reviewed Nov 22, 2024

View reviewed changes

marcos-iov reviewed Nov 22, 2024

View reviewed changes

marcos-iov approved these changes Nov 22, 2024

View reviewed changes

nathanieliov approved these changes Nov 22, 2024

View reviewed changes

julia-zack added 8 commits November 25, 2024 09:58

Create new public method in BridgeUtils to be reused

26f5580

Create new commitProposedFederation public method in fed support to b…

c15030c

…e called from bridge

Process svp success

f7143a6

Refactor tests and improve comments

f74c53f

Reorder imports. Fix indentation

827b33a

Improve comment

ecb3227

Add test cases when registering spend tx. Rollback moving method to B…

f6d89a9

…ridgeUtils

Fix Sonar issues

f1dad07

julia-zack force-pushed the svp_success branch from 9628200 to f1dad07 Compare November 25, 2024 12:59

julia-zack merged commit 2ae9834 into feature/powpeg_validation_protocol-phase4 Nov 25, 2024
8 checks passed

	List<UTXO> activeFederationUTXOs = new ArrayList<>(provider.getNewFederationBtcUTXOs(constants.getBtcParams(), activations));
	List<UTXO> activeFederationUTXOs = provider.getNewFederationBtcUTXOs(constants.getBtcParams(), activations).stream().toList();

Svp success #2854

Svp success #2854

Conversation

julia-zack commented Nov 20, 2024

github-actions bot commented Nov 20, 2024 • edited Loading

Dependency Review

OpenSSF Scorecard

Scanned Manifest Files

julia-zack Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

julia-zack Nov 21, 2024 • edited Loading

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

julia-zack Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathanieliov Nov 22, 2024 • edited Loading

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 25, 2024

Quality Gate passed

github-actions bot commented Nov 20, 2024 •

edited

Loading

julia-zack Nov 21, 2024 •

edited

Loading

julia-zack Nov 21, 2024 •

edited

Loading

julia-zack Nov 22, 2024 •

edited

Loading

nathanieliov Nov 22, 2024 •

edited

Loading