Add test case to remove signatures from a tx with mixed inputs. Impro…

…ve methods naming Fix wrong log
rsksmart · Sep 13, 2024 · 8eb82a0 · 8eb82a0
1 parent 71b7e9c
commit 8eb82a0
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 17 deletions.
diff --git a/rskj-core/src/main/java/co/rsk/peg/bitcoin/BitcoinUtils.java b/rskj-core/src/main/java/co/rsk/peg/bitcoin/BitcoinUtils.java
@@ -63,14 +63,10 @@ public static Optional<Script> extractRedeemScriptFromInput(TransactionInput txI
         }
     }
 
-    public static void removeSignaturesFromTransactionWithInputsWithP2shMultiSigInputScript(BtcTransaction transaction) {
-        if (transaction.getInputs().isEmpty()) {
-            return;
-        }
-
+    public static void removeSignaturesFromTransactionWithP2shMultiSigInputs(BtcTransaction transaction) {
         if (transaction.hasWitness()) {
             String message = "Removing signatures from SegWit transactions is not allowed.";
-            logger.error("[removeSignaturesFromTransactionWithInputsWithP2shMultiSigInputScript] {}", message);
+            logger.error("[removeSignaturesFromTransactionWithP2shMultiSigInputs] {}", message);
             throw new IllegalArgumentException(message);
         }
 
@@ -79,7 +75,7 @@ public static void removeSignaturesFromTransactionWithInputsWithP2shMultiSigInpu
                 .orElseThrow(
                     () -> {
                         String message = "Cannot remove signatures from transaction inputs that do not have p2sh multisig input script.";
-                        logger.error("[removeSignaturesFromTransactionWithInputsWithP2shMultiSigInputScript] {}", message);
+                        logger.error("[removeSignaturesFromTransactionWithP2shMultiSigInputs] {}", message);
                         return new IllegalArgumentException(message);
                     }
                 );

diff --git a/rskj-core/src/test/java/co/rsk/peg/bitcoin/BitcoinTestUtils.java b/rskj-core/src/test/java/co/rsk/peg/bitcoin/BitcoinTestUtils.java
@@ -129,27 +129,28 @@ public static byte[] flatKeysAsByteArray(List<BtcECKey> keys) {
         return flatPubKeys;
     }
 
-    public static void signTransactionInputFromMultiSigWithKeys(BtcTransaction transaction, int inputIndex, List<BtcECKey> keys) {
+    public static void signTransactionInputFromP2shMultiSig(BtcTransaction transaction, int inputIndex, List<BtcECKey> keys) {
         if (transaction.getWitness(inputIndex).getPushCount() == 0) {
-            signLegacyTransactionInputWithP2shMultiSigInputScript(transaction, inputIndex, keys);
+            signLegacyTransactionInputFromP2shMultiSig(transaction, inputIndex, keys);
         }
     }
 
-    private static void signLegacyTransactionInputWithP2shMultiSigInputScript(BtcTransaction transaction, int inputIndex, List<BtcECKey> keys) {
+    private static void signLegacyTransactionInputFromP2shMultiSig(BtcTransaction transaction, int inputIndex, List<BtcECKey> keys) {
         TransactionInput input = transaction.getInput(inputIndex);
 
         Script inputRedeemScript = extractRedeemScriptFromInput(input)
-            .orElseThrow(() -> new IllegalArgumentException("Cannot sign inputs that are not from a multisig"));
+            .orElseThrow(() -> new IllegalArgumentException("Cannot sign inputs that are not from a p2sh multisig"));
 
+        Script outputScript = createP2SHOutputScript(inputRedeemScript);
         Sha256Hash sigHash = transaction.hashForSignature(inputIndex, inputRedeemScript, BtcTransaction.SigHash.ALL, false);
         Script inputScriptSig = input.getScriptSig();
+
         for (BtcECKey key : keys) {
             BtcECKey.ECDSASignature sig = key.sign(sigHash);
             TransactionSignature txSig = new TransactionSignature(sig, BtcTransaction.SigHash.ALL, false);
             byte[] txSigEncoded = txSig.encodeToBitcoin();
 
             int keyIndex = inputScriptSig.getSigInsertionIndex(sigHash, key);
-            Script outputScript = createP2SHOutputScript(inputRedeemScript);
             inputScriptSig = outputScript.getScriptSigWithSignature(inputScriptSig, txSigEncoded, keyIndex);
             input.setScriptSig(inputScriptSig);
         }

diff --git a/rskj-core/src/test/java/co/rsk/peg/bitcoin/BitcoinUtilsTest.java b/rskj-core/src/test/java/co/rsk/peg/bitcoin/BitcoinUtilsTest.java
@@ -308,7 +308,7 @@ void removeSignaturesFromTransaction_whenTransactionDoesNotHaveInputs_shouldRetu
         Sha256Hash transactionHashBeforeRemovingSignatures = transaction.getHash();
 
         // act
-        BitcoinUtils.removeSignaturesFromTransactionWithInputsWithP2shMultiSigInputScript(transaction);
+        BitcoinUtils.removeSignaturesFromTransactionWithP2shMultiSigInputs(transaction);
 
         // assert
         Assertions.assertEquals(transactionHashBeforeRemovingSignatures, transaction.getHash());
@@ -325,7 +325,7 @@ void removeSignaturesFromTransaction_whenTransactionIsSegwit_shouldThrowIllegalA
 
         // act & assert
         IllegalArgumentException exception = Assertions.assertThrows(IllegalArgumentException.class,
-            () -> BitcoinUtils.removeSignaturesFromTransactionWithInputsWithP2shMultiSigInputScript(transaction));
+            () -> BitcoinUtils.removeSignaturesFromTransactionWithP2shMultiSigInputs(transaction));
         Assertions.assertEquals("Removing signatures from SegWit transactions is not allowed.", exception.getMessage());
     }
 
@@ -339,7 +339,25 @@ void removeSignaturesFromTransaction_whenTransactionInputsDoNotHaveP2shMultiSigI
 
         // act & assert
         IllegalArgumentException exception = Assertions.assertThrows(IllegalArgumentException.class,
-            () -> BitcoinUtils.removeSignaturesFromTransactionWithInputsWithP2shMultiSigInputScript(transaction));
+            () -> BitcoinUtils.removeSignaturesFromTransactionWithP2shMultiSigInputs(transaction));
+        Assertions.assertEquals("Cannot remove signatures from transaction inputs that do not have p2sh multisig input script.", exception.getMessage());
+    }
+
+    @Test
+    void removeSignaturesFromTransaction_whenNotAllTransactionInputsHaveP2shMultiSigInputScript_shouldThrowIllegalArgumentException() {
+        // arrange
+        Federation federation = new P2shErpFederationBuilder().build();
+        Script p2shMultiSigScriptSig = federation.getP2SHScript().createEmptyInputScript(null, federation.getRedeemScript());
+        BtcECKey pubKey = new BtcECKey();
+        Script p2pkhScriptSig = ScriptBuilder.createInputScript(null, pubKey);
+
+        BtcTransaction transaction = new BtcTransaction(btcMainnetParams);
+        transaction.addInput(BitcoinTestUtils.createHash(2), 0, p2shMultiSigScriptSig);
+        transaction.addInput(BitcoinTestUtils.createHash(1), 0, p2pkhScriptSig);
+
+        // act & assert
+        IllegalArgumentException exception = Assertions.assertThrows(IllegalArgumentException.class,
+            () -> BitcoinUtils.removeSignaturesFromTransactionWithP2shMultiSigInputs(transaction));
         Assertions.assertEquals("Cannot remove signatures from transaction inputs that do not have p2sh multisig input script.", exception.getMessage());
     }
 
@@ -359,11 +377,11 @@ void removeSignaturesFromTransaction_whenTransactionIsLegacyAndInputsHaveP2shMul
             BitcoinTestUtils.getBtcEcKeysFromSeeds(new String[]{"member01", "member02", "member03", "member04", "member05"}, true); // using private keys from federation declared above
         List<TransactionInput> inputs = transaction.getInputs();
         for (TransactionInput input : inputs) {
-            BitcoinTestUtils.signTransactionInputFromMultiSigWithKeys(transaction, inputs.indexOf(input), keysToSign);
+            BitcoinTestUtils.signTransactionInputFromP2shMultiSig(transaction, inputs.indexOf(input), keysToSign);
         }
 
         // act
-        BitcoinUtils.removeSignaturesFromTransactionWithInputsWithP2shMultiSigInputScript(transaction);
+        BitcoinUtils.removeSignaturesFromTransactionWithP2shMultiSigInputs(transaction);
 
         // assert
         Assertions.assertEquals(transactionWithoutSignaturesHash, transaction.getHash());