Add test cases. Better error handling when data is null or larger than 4 bytes. Refactor

Author: julia-zack

src/main/java/co/rsk/bitcoinj/script/RedeemScriptValidator.java

@@ -52,8 +52,8 @@ protected static boolean hasStandardRedeemScriptStructure(List<ScriptChunk> chun
             }
 
             int numKeys = secondToLastChunk.decodePositiveN();
-            // and there should be as many data chunks as keys
-            if (numKeys < 1 || chunksSize != numKeys + 3) { // numKeys + M + N + OP_CHECKMULTISIG
+            // and there should be numKeys+3 total chunks (keys + OP_M + OP_N + OP_CHECKMULTISIG)
+            if (chunksSize != numKeys + 3) {
                 return false;
             }
 
@@ -65,7 +65,7 @@ protected static boolean hasStandardRedeemScriptStructure(List<ScriptChunk> chun
 
             return true;
         } catch (IllegalStateException e) {
-            return false;   // Not a number
+            return false; // Not a number
         }
     }
 

src/main/java/co/rsk/bitcoinj/script/ScriptChunk.java

@@ -21,7 +21,6 @@
 import static com.google.common.base.Preconditions.checkState;
 import static java.util.Objects.isNull;
 
-import co.rsk.bitcoinj.core.ScriptException;
 import co.rsk.bitcoinj.core.Utils;
 import com.google.common.base.Objects;
 import java.io.IOException;
@@ -151,7 +150,7 @@ public boolean isPositiveN() {
         try {
             decodePositiveN();
             return true;
-        } catch(IllegalArgumentException | ScriptException e) {
+        } catch (IllegalArgumentException e) {
             return false;
         }
     }
@@ -191,14 +190,11 @@ private int decodePositiveNConsideringEncoding() {
             throw new IllegalArgumentException("Number from chunk is not positive.");
         }
 
-        return castToBigInteger(data).intValue(); // values up to Integer.MAX_VALUE can be cast as ints
-    }
-
-    // copied implementation from Script.castToBigInteger to avoid circular dependency
-    private static BigInteger castToBigInteger(byte[] chunk) throws ScriptException {
-        if (chunk.length > 4)
-            throw new ScriptException("Script attempted to use an integer larger than 4 bytes");
-        return Utils.decodeMPI(Utils.reverseBytes(chunk), false);
+        if (dataLength > 4) {
+            throw new IllegalArgumentException("Number from chunk has more than 4 bytes.");
+        }
+        BigInteger bigInteger = Utils.decodeMPI(Utils.reverseBytes(data), false);
+        return bigInteger.intValue(); // values up to Integer.MAX_VALUE can be cast as ints
     }
 
     @Override

src/test/java/co/rsk/bitcoinj/script/RedeemScriptValidatorTest.java

@@ -23,6 +23,7 @@ public class RedeemScriptValidatorTest {
     private final BtcECKey ecKey1 = BtcECKey.fromPrivate(BigInteger.valueOf(110));
     private final BtcECKey ecKey2 = BtcECKey.fromPrivate(BigInteger.valueOf(220));
     private final BtcECKey ecKey3 = BtcECKey.fromPrivate(BigInteger.valueOf(330));
+    private final BtcECKey ecKey4 = BtcECKey.fromPrivate(BigInteger.valueOf(440));
 
     private Script standardRedeemScript;
     private Script flyoverStandardRedeemScript;
@@ -145,6 +146,46 @@ public void hasStandardRedeemScriptStructure_withLessKeysPushedThanExpected_shou
         Assert.assertFalse(RedeemScriptValidator.hasStandardRedeemScriptStructure(redeemScriptChunks));
     }
 
+    @Test
+    public void hasStandardRedeemScriptStructure_withNegativeThreshold_shouldBeFalse() {
+        for (int n=0; n<20; n++) {
+            int i = -(1 << n); // i = -(2^n)
+            ScriptBuilder builder = new ScriptBuilder();
+            Script redeemScript = builder
+                .number(i)
+                .data(ecKey1.getPubKey())
+                .data(ecKey2.getPubKey())
+                .data(ecKey3.getPubKey())
+                .data(ecKey4.getPubKey())
+                .op(ScriptOpCodes.OP_4)
+                .op(ScriptOpCodes.OP_CHECKMULTISIG)
+                .build();
+
+            List<ScriptChunk> redeemScriptChunks = redeemScript.getChunks();
+            Assert.assertFalse(RedeemScriptValidator.hasStandardRedeemScriptStructure(redeemScriptChunks));
+        }
+    }
+
+    @Test
+    public void hasStandardRedeemScriptStructure_withNegativeTotalKeys_shouldBeFalse() {
+        for (int n=0; n<20; n++) {
+            int i = -(1 << n); // i = -(2^n)
+            ScriptBuilder builder = new ScriptBuilder();
+            Script redeemScript = builder
+                .op(ScriptOpCodes.OP_4)
+                .data(ecKey1.getPubKey())
+                .data(ecKey2.getPubKey())
+                .data(ecKey3.getPubKey())
+                .data(ecKey4.getPubKey())
+                .number(i)
+                .op(ScriptOpCodes.OP_CHECKMULTISIG)
+                .build();
+
+            List<ScriptChunk> redeemScriptChunks = redeemScript.getChunks();
+            Assert.assertFalse(RedeemScriptValidator.hasStandardRedeemScriptStructure(redeemScriptChunks));
+        }
+    }
+
     @Test
     public void hasStandardRedeemScriptStructure_withARedeemLikeScript_withoutOpCheckMultisigAsLastOpcode_shouldBeFalse() {
         ScriptBuilder builder = new ScriptBuilder();

src/test/java/co/rsk/bitcoinj/script/ScriptChunkTest.java

@@ -164,9 +164,9 @@ public void decodePositiveN_withPositiveNumber_returnsExpectedNumber() {
     }
 
     @Test
-    public void decodePositiveN_forZero_throwsNPE() {
-        int zero = 0;
+    public void decodePositiveN_forZero_throwsIAE() {
         ScriptBuilder builder = new ScriptBuilder();
+        int zero = 0;
         builder.number(zero);
         Script script = builder.build();
 
@@ -190,7 +190,19 @@ public void decodePositiveN_withNegativeNumber_throwsIAE() {
     }
 
     @Test
-    public void decodePositiveN_withNullPushData_throwsNPE() {
+    public void decodePositiveN_withNumberLargerThan4Bytes_throwsIAE() {
+        long i = 1L << 32;
+        ScriptBuilder builder = new ScriptBuilder();
+        builder.number(i);
+        Script script = builder.build();
+
+        ScriptChunk chunk = script.chunks.get(0);
+        assertFalse(chunk.isPositiveN());
+        assertThrows(IllegalArgumentException.class, chunk::decodePositiveN);
+    }
+
+    @Test
+    public void decodePositiveN_withNullPushData_throwsIAE() {
         ScriptChunk chunk = new ScriptChunk(OP_PUSHDATA1, null);
         assertFalse(chunk.isPositiveN());
         assertThrows(IllegalArgumentException.class, chunk::decodePositiveN);