Refactor addSignature as previous step to create addSvpSpendTxSignature method #2788

julia-zack · 2024-10-03T18:08:33Z

No description provided.

julia-zack · 2024-10-03T19:20:14Z

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

+    private void logMissingSignatures(BtcTransaction btcTx, Keccak256 rskTxHash, int neededSignatures, int federationSize) {
+        int missingSignatures = BridgeUtils.countMissingSignatures(btcContext, btcTx);
+        int signaturesCount = neededSignatures - missingSignatures;
+
+        logger.debug("Tx {} not yet fully signed. Requires {}/{} signatures but has {}",
+            rskTxHash, neededSignatures, federationSize, signaturesCount);
+    }


this might seem unnecessary, but it would be needed when adding new method

julia-zack · 2024-10-03T19:20:23Z

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

+    private void logReleaseBtc(BtcTransaction btcTx, byte[] rskTxHashSerialized) {
+        logger.info("Tx fully signed {}. Hex: {}", btcTx, Bytes.of(btcTx.bitcoinSerialize()));
+        eventLogger.logReleaseBtc(btcTx, rskTxHashSerialized);
+    }


this might seem unnecessary, but it would be needed when adding new method

apancorb

Very nice refactor, left comments :)

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

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

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

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

marcos-iov · 2024-10-09T17:33:48Z

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

@@ -90,9 +90,23 @@ public static Script createBaseP2SHInputScriptThatSpendsFromRedeemScript(Script
        return outputScript.createEmptyInputScript(null, redeemScript);
    }

+    public static Script getRedeemScriptFromP2SHInputScript(Script inputScript) {


Looking a bit more into this, we already have a method here extractRedeemScriptFromInput. Why not use that one passing the input directly? Instead of extracting the input script and then calling this

could be, i find it weird that that method returns an optional. I think it should return the script or an exception, wdyt?

I think the Optional gives a bit more flexibility, it get the redeem script if there is one. Then you can do all kinds of functional stuff with the optional return.

apancorb

LGTM

apancorb · 2024-10-10T08:41:28Z

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

-        if (rskTxHash.length!=32) {
-            throw new BridgeIllegalArgumentException("Invalid rsk tx hash " + Bytes.of(rskTxHash));
+        byte[] rskTxHashSerialized = (byte[]) args[2];
+        if (rskTxHashSerialized.length!=32) {


Suggested change

if (rskTxHashSerialized.length!=32) {

if (rskTxHashSerialized.length != 32) {

Or,

Suggested change

if (rskTxHashSerialized.length!=32) {

if (rskTxHashSerialized.length != Keccak256.HASH_LEN) {

Or,

Suggested change

if (rskTxHashSerialized.length!=32) {

Keccack256 rskTxHash = BridgeUtils.deserializeKeccack256(rskTxHashSerialized);

That would imply writing deserializeKeccack256 function, which should check for length and throw an exception if not valid. And add tests for that function 😁

updateddddd

marcos-iov · 2024-10-10T14:43:42Z

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

-        if (rskTxHash.length!=32) {
-            throw new BridgeIllegalArgumentException("Invalid rsk tx hash " + Bytes.of(rskTxHash));
+        byte[] rskTxHashSerialized = (byte[]) args[2];
+        if (rskTxHashSerialized.length!=32) {


Or,

Suggested change

if (rskTxHashSerialized.length!=32) {

if (rskTxHashSerialized.length != Keccak256.HASH_LEN) {

Or,

Suggested change

if (rskTxHashSerialized.length!=32) {

Keccack256 rskTxHash = BridgeUtils.deserializeKeccack256(rskTxHashSerialized);

That would imply writing deserializeKeccack256 function, which should check for length and throw an exception if not valid. And add tests for that function 😁

marcos-iov · 2024-10-10T14:46:02Z

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

+        try {
+            txSigs = getTransactionSignatures(federatorBtcPublicKey, sigHashes, signatures);
+        } catch (SignatureException e) {
+            logger.error("[processSigning] Unable to proceed with signing as the transaction signatures are incorrect.");


Suggested change

logger.error("[processSigning] Unable to proceed with signing as the transaction signatures are incorrect.");

logger.error("[processSigning] Unable to proceed with signing as the transaction signatures are incorrect. {}", e.getMessage());

marcos-iov · 2024-10-10T15:38:22Z

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

@@ -90,9 +90,23 @@ public static Script createBaseP2SHInputScriptThatSpendsFromRedeemScript(Script
        return outputScript.createEmptyInputScript(null, redeemScript);
    }

+    public static Script getRedeemScriptFromP2SHInputScript(Script inputScript) {


Looking a bit more into this, we already have a method here extractRedeemScriptFromInput. Why not use that one passing the input directly? Instead of extracting the input script and then calling this

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

…re method

…at type

…doc. Replace times(0) for never().

…ility to be more accurate. Change btcTx name for releaseTx name. Return false when catching exception when signing

…eccak rskTxHash. Remove logger.isDebugEnabled condition.

…cated exception in addSignature to better handling. Make deserializeRskTxHash method public to reuse it. Add tests.

…ary new one

marcos-iov · 2024-10-11T18:06:11Z

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

-        } catch (Exception e) {
+        } catch (IllegalArgumentException e) {
+            throw new BridgeIllegalArgumentException("Invalid rsk tx hash " + Bytes.of(rskTxHashSerialized));
+        } catch (IOException e) {


Is this change a good idea? I think all Bridge methods are capturing generic exceptions

There might be unchecked exception thrown

marcos-iov · 2024-10-11T18:09:22Z

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

@@ -2025,4 +2026,27 @@ void testGetUTXOsSentToAddresses_no_utxo_sent_to_given_address_after_RSKIP293()

        Assertions.assertTrue(foundUTXOs.isEmpty());
    }
+
+    @Test


Should these tests go in BridgeSerializationUtilsTest?

yes, my bad. Thanks

marcos-iov · 2024-10-11T18:09:47Z

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

+    }
+
+    @Test
+    void deserializeRskTxHash_withInvalidLength_throwsIllegalArgumentException() {


A test passing a null value?

good catch, thanks

…right file. Catch generic exceptions in Bridge method

sonarqubecloud · 2024-10-14T15:21:32Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

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

See analysis details on SonarCloud

julia-zack marked this pull request as ready for review October 3, 2024 18:14

julia-zack requested a review from a team as a code owner October 3, 2024 18:14

julia-zack commented Oct 3, 2024

View reviewed changes

apancorb reviewed Oct 4, 2024

View reviewed changes

marcos-iov force-pushed the feature/powpeg_validation_protocol-phase3 branch from 5713282 to 68da5aa Compare October 8, 2024 16:22

marcos-iov force-pushed the refactor_add_signature branch from 5c281f6 to 40882bd Compare October 8, 2024 16:36

marcos-iov force-pushed the feature/powpeg_validation_protocol-phase3 branch from 68da5aa to 1a6e1ea Compare October 8, 2024 18:34

marcos-iov force-pushed the refactor_add_signature branch from 40882bd to ae68f5e Compare October 8, 2024 18:37

marcos-iov reviewed Oct 8, 2024

View reviewed changes

marcos-iov reviewed Oct 9, 2024

View reviewed changes

apancorb approved these changes Oct 10, 2024

View reviewed changes

marcos-iov reviewed Oct 10, 2024

View reviewed changes

julia-zack added 12 commits October 10, 2024 17:07

Refactor addSignature as previous step to create addSvpSpendTxSignatu…

c6c6a5f

…re method

Remove addSvpSpendTxSignature method that would be part of another pr

d0eb08b

Remove optional logic to verify signatures

e1b8704

Catch specific SignatureException

249cbd0

Make getDecodedSignatures throw signature exception to handle just th…

ccfdcde

…at type

Avoid using isPresent / get and make code more functional. Align java…

f14bece

…doc. Replace times(0) for never().

Change boolean method name to be affirmative. Change method responsib…

31a9c69

…ility to be more accurate. Change btcTx name for releaseTx name. Return false when catching exception when signing

Use getScriptSigWithSignature instead of updateScriptWithSignature

074d749

Rename boolean method. Replace rskTxHashSerialized argument for the k…

5a186d9

…eccak rskTxHash. Remove logger.isDebugEnabled condition.

Add tests for new utility methods. Rename method to be more specific

afccbd5

Add test for when method should throw

5aa566c

Add log when catching exception to avoid sonar warning

e7a67b1

julia-zack force-pushed the refactor_add_signature branch from c130496 to e7a67b1 Compare October 10, 2024 20:11

julia-zack added 3 commits October 10, 2024 18:00

Add logging error message when logging caught exception. Throw a dedi…

361b013

…cated exception in addSignature to better handling. Make deserializeRskTxHash method public to reuse it. Add tests.

Remove unused import

2fabadb

Use already existing method to extract redeem script. Remove unnecess…

c6af3f4

…ary new one

marcos-iov reviewed Oct 11, 2024

View reviewed changes

Throw exception when trying to serialize a null value. Move tests to …

5c43d68

…right file. Catch generic exceptions in Bridge method

marcos-iov approved these changes Oct 14, 2024

View reviewed changes

marcos-iov merged commit 1060083 into feature/powpeg_validation_protocol-phase3 Oct 14, 2024
7 checks passed

marcos-iov deleted the refactor_add_signature branch October 14, 2024 19:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor addSignature as previous step to create addSvpSpendTxSignature method #2788

Refactor addSignature as previous step to create addSvpSpendTxSignature method #2788

julia-zack commented Oct 3, 2024

julia-zack Oct 3, 2024

julia-zack Oct 3, 2024

apancorb left a comment

marcos-iov Oct 9, 2024

julia-zack Oct 9, 2024

marcos-iov Oct 10, 2024

julia-zack Oct 10, 2024 •

edited

Loading

marcos-iov Oct 10, 2024

julia-zack Oct 11, 2024

apancorb left a comment

apancorb Oct 10, 2024

marcos-iov Oct 10, 2024

julia-zack Oct 10, 2024

marcos-iov Oct 10, 2024

marcos-iov Oct 10, 2024

julia-zack Oct 10, 2024

marcos-iov Oct 10, 2024

marcos-iov Oct 11, 2024

marcos-iov Oct 11, 2024

julia-zack Oct 14, 2024

marcos-iov Oct 11, 2024

julia-zack Oct 11, 2024

julia-zack Oct 14, 2024

marcos-iov Oct 11, 2024

julia-zack Oct 11, 2024

julia-zack Oct 14, 2024

sonarqubecloud bot commented Oct 14, 2024

	if (rskTxHashSerialized.length!=32) {
	if (rskTxHashSerialized.length != 32) {

	if (rskTxHashSerialized.length!=32) {
	if (rskTxHashSerialized.length != Keccak256.HASH_LEN) {

	if (rskTxHashSerialized.length!=32) {
	Keccack256 rskTxHash = BridgeUtils.deserializeKeccack256(rskTxHashSerialized);

	logger.error("[processSigning] Unable to proceed with signing as the transaction signatures are incorrect.");
	logger.error("[processSigning] Unable to proceed with signing as the transaction signatures are incorrect. {}", e.getMessage());

Refactor addSignature as previous step to create addSvpSpendTxSignature method #2788

Refactor addSignature as previous step to create addSvpSpendTxSignature method #2788

Conversation

julia-zack commented Oct 3, 2024

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

julia-zack Oct 10, 2024 • edited Loading

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

sonarqubecloud bot commented Oct 14, 2024

Quality Gate passed

julia-zack Oct 10, 2024 •

edited

Loading