Register spend tx #2851

julia-zack · 2024-11-19T17:10:51Z

No description provided.

github-actions · 2024-11-19T17:11:09Z

Dependency Review

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

OpenSSF Scorecard

Package	Version	Score	Details

Scanned Manifest Files

… without them.

julia-zack · 2024-11-19T18:04:56Z

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

+                markTxAsProcessed(btcTx);
+                saveNewUTXOs(btcTx);
+                provider.setSvpSpendTxHashUnsigned(null);
+                // proceed with svp success


this will be addressed in next task

julia-zack · 2024-11-19T18:07:16Z

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

-        private void setUpForTransactionRegistration(BtcTransaction transaction) throws BlockStoreException {
-            // recreate a valid chain that has the tx, so it passes the previous checks in registerBtcTransaction
-            BtcBlockStoreWithCache.Factory btcBlockStoreFactory = new RepositoryBtcBlockStoreWithCache.Factory(btcMainnetParams, 100, 100);
-            BtcBlockStoreWithCache btcBlockStoreWithCache = btcBlockStoreFactory.newInstance(repository, bridgeMainNetConstants, bridgeStorageProvider, allActivations);
-
-            pmtWithTransactions = createValidPmtForTransactions(Collections.singletonList(transaction.getHash()), btcMainnetParams);
-            btcBlockWithPmtHeight = bridgeMainNetConstants.getBtcHeightWhenPegoutTxIndexActivates() + bridgeMainNetConstants.getPegoutTxIndexGracePeriodInBtcBlocks(); // we want pegout tx index to be activated
-
-            int chainHeight = btcBlockWithPmtHeight + bridgeMainNetConstants.getBtc2RskMinimumAcceptableConfirmations();
-            recreateChainFromPmt(btcBlockStoreWithCache, chainHeight, pmtWithTransactions, btcBlockWithPmtHeight, btcMainnetParams);
-
-            bridgeStorageProvider.save();
-
-            bridgeSupport = bridgeSupportBuilder
-                .withBridgeConstants(bridgeMainNetConstants)
-                .withProvider(bridgeStorageProvider)
-                .withActivations(allActivations)
-                .withFederationSupport(federationSupport)
-                .withFeePerKbSupport(feePerKbSupport)
-                .withExecutionBlock(rskExecutionBlock)
-                .withBtcBlockStoreFactory(btcBlockStoreFactory)
-                .withRepository(repository)
-                .build();
-        }
-
-        private void assertActiveFederationUtxosSize(int expectedActiveFederationUtxosSize) {
-            assertEquals(expectedActiveFederationUtxosSize, federationSupport.getActiveFederationBtcUTXOs().size());
-        }
-
-        private void assertTransactionWasProcessed(Sha256Hash transactionHash) throws IOException {
-            Optional<Long> rskBlockHeightAtWhichBtcTxWasProcessed = bridgeStorageProvider.getHeightIfBtcTxhashIsAlreadyProcessed(transactionHash);
-            assertTrue(rskBlockHeightAtWhichBtcTxWasProcessed.isPresent());
-
-            long rskExecutionBlockNumber = rskExecutionBlock.getNumber();
-            assertEquals(rskExecutionBlockNumber, rskBlockHeightAtWhichBtcTxWasProcessed.get());
-        }
-


this was just moved outside to be reused

jeremy-then

LGTM.

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

rskj-core/src/test/java/co/rsk/peg/bitcoin/BitcoinUtilsTest.java

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

rskj-core/src/main/java/co/rsk/peg/bitcoin/BitcoinUtils.java

nathanieliov

Well done. Just some minor comments to improve the naming of the unit tests to be more specific on what's the result expected

…cleaner

apancorb

LGTM

marcos-iov · 2024-11-21T15:12:30Z

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

+        return provider.getSvpSpendTxHashUnsigned()
+            .map(svpSpendTransactionHashUnsigned ->
+                getMultiSigTransactionHashWithoutSignatures(networkParameters, transaction).equals(svpSpendTransactionHashUnsigned)
+            )
+            .orElse(false);


Suggested change

return provider.getSvpSpendTxHashUnsigned()

.map(svpSpendTransactionHashUnsigned ->

getMultiSigTransactionHashWithoutSignatures(networkParameters, transaction).equals(svpSpendTransactionHashUnsigned)

)

.orElse(false);

return provider.getSvpFundTxHashUnsigned()

.filter(svpFundTransactionHashUnsigned ->

getMultiSigTransactionHashWithoutSignatures(networkParameters, transaction).equals(svpFundTransactionHashUnsigned)

)

.isPresent();

Just an alternative for you to consider. No difference in practice, not sure if somehow using a filter to find something is more appropiate than using a map

marcos-iov · 2024-11-21T16:49:53Z

rskj-core/src/main/java/co/rsk/peg/bitcoin/BitcoinUtils.java

@@ -57,7 +57,17 @@ public static Optional<Script> extractRedeemScriptFromInput(TransactionInput txI
        }
    }

-    public static void removeSignaturesFromTransactionWithP2shMultiSigInputs(BtcTransaction transaction) {
+    public static Sha256Hash getMultiSigTransactionHashWithoutSignatures(NetworkParameters networkParameters, BtcTransaction transaction) {
+        if (!transaction.hasWitness()) {


Suggested change

if (!transaction.hasWitness()) {

if (transaction.hasWitness()) {

return transaction.getHash();

}

...

Nit, just to return early

i put it this way considering the txs are normally segwit (correct me if im wrong! :) ), so we will have the common behavior outside the check

marcos-iov · 2024-11-21T16:53:01Z

rskj-core/src/test/java/co/rsk/peg/bitcoin/BitcoinUtilsTest.java

    }

    @Test
-    void removeSignaturesFromTransaction_whenTransactionInputsDoNotHaveP2shMultiSigInputScript_shouldThrowIllegalArgumentException() {
+    void getTransactionHashWithoutSignatures_whenTxIsNotSegwitAndTransactionInputsDoNotHaveP2shMultiSigInputScript_shouldThrowIllegalArgumentException() {


An idea to add a test for this new method and removeSignatures.. one.

Take a pegout from mainnet. Get the raw transaction from a release_btc event, that is the signed transaction. Then get the unsigned tx hash from the corresponding release_requested event. We can use these 2 values to do a test with real transactions

super nice, thanks!

sonarcloud · 2024-11-21T17:40:42Z

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 register_spend_tx branch from 5b4e5cc to 9dd9f2a Compare November 19, 2024 17:23

Modify public method to remove signatures for just returning the hash…

3513c98

… without them.

julia-zack force-pushed the register_spend_tx branch from 9dd9f2a to 65cb239 Compare November 19, 2024 18:04

julia-zack commented Nov 19, 2024

View reviewed changes

Register spend transaction

30dafce

julia-zack force-pushed the register_spend_tx branch from 65cb239 to 30dafce Compare November 19, 2024 18:11

julia-zack marked this pull request as ready for review November 19, 2024 18:12

julia-zack requested a review from a team as a code owner November 19, 2024 18:12

jeremy-then reviewed Nov 19, 2024

View reviewed changes

nathanieliov reviewed Nov 20, 2024

View reviewed changes

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

nathanieliov reviewed Nov 20, 2024

View reviewed changes

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

nathanieliov reviewed Nov 20, 2024

View reviewed changes

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

nathanieliov reviewed Nov 20, 2024

View reviewed changes

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

apancorb reviewed Nov 20, 2024

View reviewed changes

julia-zack force-pushed the register_spend_tx branch from fbb0e14 to 32e510c Compare November 20, 2024 14:39

nathanieliov approved these changes Nov 20, 2024

View reviewed changes

Create new registerSvpSpendTransaction method. Refactor to make code …

007ffad

…cleaner

julia-zack force-pushed the register_spend_tx branch from 32e510c to 007ffad Compare November 20, 2024 14:54

apancorb approved these changes Nov 21, 2024

View reviewed changes

marcos-iov reviewed Nov 21, 2024

View reviewed changes

Add test for getTxHashWithoutSigs using a real pegout. Minor refactor

29bc369

marcos-iov approved these changes Nov 21, 2024

View reviewed changes

marcos-iov merged commit 0d4c61c into feature/powpeg_validation_protocol-phase4 Nov 21, 2024
8 checks passed

marcos-iov deleted the register_spend_tx branch November 21, 2024 20:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Register spend tx #2851

Register spend tx #2851

julia-zack commented Nov 19, 2024

github-actions bot commented Nov 19, 2024 •

edited

Loading

julia-zack Nov 19, 2024

julia-zack Nov 19, 2024

jeremy-then left a comment

nathanieliov left a comment

apancorb left a comment

marcos-iov Nov 21, 2024

julia-zack Nov 21, 2024

marcos-iov Nov 21, 2024

julia-zack Nov 21, 2024

marcos-iov Nov 21, 2024

julia-zack Nov 21, 2024

julia-zack Nov 21, 2024

sonarcloud bot commented Nov 21, 2024

-        if (!transaction.hasWitness()) {
+        if (transaction.hasWitness()) {
+           return transaction.getHash();
+        }
+        ...

Register spend tx #2851

Register spend tx #2851

Conversation

julia-zack commented Nov 19, 2024

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

Dependency Review

OpenSSF Scorecard

Scanned Manifest Files

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremy-then left a comment

Choose a reason for hiding this comment

nathanieliov left a 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

sonarcloud bot commented Nov 21, 2024

Quality Gate passed

github-actions bot commented Nov 19, 2024 •

edited

Loading