Keep isPositiveN method to check whether a chunk is a positive n. Create castToBigInteger in ScriptChunk class to avoid circular dependency with Script class

Author: julia-zack

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

@@ -46,15 +46,12 @@ protected static boolean hasStandardRedeemScriptStructure(List<ScriptChunk> chun
             // Second to last chunk must be a number for the keys
             int secondToLastChunkIndex = chunksSize - 2;
             ScriptChunk secondToLastChunk = chunks.get(secondToLastChunkIndex);
-            int numKeys;
 
-            try {
-                firstChunk.decodePositiveN();
-                numKeys = secondToLastChunk.decodePositiveN();
-            } catch (IllegalArgumentException e) {
+            if (!(firstChunk.isPositiveN() && secondToLastChunk.isPositiveN())) {
                 return false;
             }
 
+            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
                 return false;

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

@@ -774,7 +774,7 @@ private static boolean castToBool(byte[] data) {
      * sizes.
      * @throws ScriptException if the chunk is longer than 4 bytes.
      */
-    public static BigInteger castToBigInteger(byte[] chunk) throws ScriptException {
+    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);

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

@@ -18,13 +18,15 @@
 package co.rsk.bitcoinj.script;
 
 import static co.rsk.bitcoinj.script.ScriptOpCodes.*;
-import static com.google.common.base.Preconditions.checkNotNull;
 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;
 import java.io.OutputStream;
+import java.math.BigInteger;
 import java.util.Arrays;
 import javax.annotation.Nullable;
 
@@ -145,6 +147,15 @@ public void write(OutputStream stream) throws IOException {
         }
     }
 
+    public boolean isPositiveN() {
+        try {
+            decodePositiveN();
+            return true;
+        } catch(IllegalArgumentException | ScriptException e) {
+            return false;
+        }
+    }
+
     public int decodePositiveN() {
         if (isOpcodeSmallNumber()) {
             return decodeOpN();
@@ -169,7 +180,9 @@ public boolean isOpCheckMultiSig() {
     }
 
     private int decodePositiveNConsideringEncoding() {
-        checkNotNull(data);
+        if (isNull(data)) {
+            throw new IllegalArgumentException("Chunk has null data.");
+        }
         int dataLength = data.length;
 
         int signByte = data[dataLength - 1] & 0x80;
@@ -178,10 +191,14 @@ private int decodePositiveNConsideringEncoding() {
             throw new IllegalArgumentException("Number from chunk is not positive.");
         }
 
-        if (dataLength == 1) {
-            return data[0];
-        }
-        return Script.castToBigInteger(data).intValue(); // values down to Integer.MIN_VALUE and up to Integer.MAX_VALUE can be cast as ints
+        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);
     }
 
     @Override

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

@@ -144,6 +144,7 @@ public void decodePositiveN_withPositiveSmallNumber_returnsExpectedNumber() {
             Script script = builder.build();
 
             ScriptChunk chunk = script.chunks.get(0);
+            assertTrue(chunk.isPositiveN());
             assertEquals(i, chunk.decodePositiveN());
         }
     }
@@ -157,6 +158,7 @@ public void decodePositiveN_withPositiveNumber_returnsExpectedNumber() {
             Script script = builder.build();
 
             ScriptChunk chunk = script.chunks.get(0);
+            assertTrue(chunk.isPositiveN());
             assertEquals(i, chunk.decodePositiveN());
         }
     }
@@ -169,7 +171,8 @@ public void decodePositiveN_forZero_throwsNPE() {
         Script script = builder.build();
 
         ScriptChunk chunk = script.chunks.get(0);
-        assertThrows(NullPointerException.class, chunk::decodePositiveN);
+        assertFalse(chunk.isPositiveN());
+        assertThrows(IllegalArgumentException.class, chunk::decodePositiveN);
     }
 
     @Test
@@ -181,34 +184,42 @@ public void decodePositiveN_withNegativeNumber_throwsIAE() {
             Script script = builder.build();
 
             ScriptChunk chunk = script.chunks.get(0);
+            assertFalse(chunk.isPositiveN());
             assertThrows(IllegalArgumentException.class, chunk::decodePositiveN);
         }
     }
 
     @Test
     public void decodePositiveN_withNullPushData_throwsNPE() {
         ScriptChunk chunk = new ScriptChunk(OP_PUSHDATA1, null);
-        assertThrows(NullPointerException.class, chunk::decodePositiveN);
+        assertFalse(chunk.isPositiveN());
+        assertThrows(IllegalArgumentException.class, chunk::decodePositiveN);
 
         chunk = new ScriptChunk(OP_PUSHDATA2, null);
-        assertThrows(NullPointerException.class, chunk::decodePositiveN);
+        assertFalse(chunk.isPositiveN());
+        assertThrows(IllegalArgumentException.class, chunk::decodePositiveN);
 
         chunk = new ScriptChunk(OP_PUSHDATA4, null);
-        assertThrows(NullPointerException.class, chunk::decodePositiveN);
+        assertFalse(chunk.isPositiveN());
+        assertThrows(IllegalArgumentException.class, chunk::decodePositiveN);
     }
 
     @Test
     public void decodePositiveN_withWrongOpcodes_throwsIAE() {
         ScriptChunk chunk = new ScriptChunk(ScriptOpCodes.OP_CHECKMULTISIG, null);
+        assertFalse(chunk.isPositiveN());
         assertThrows(IllegalArgumentException.class, chunk::decodePositiveN);
 
         chunk = new ScriptChunk(ScriptOpCodes.OP_CHECKSIG, null);
+        assertFalse(chunk.isPositiveN());
         assertThrows(IllegalArgumentException.class, chunk::decodePositiveN);
 
         chunk = new ScriptChunk(ScriptOpCodes.OP_RETURN, null);
+        assertFalse(chunk.isPositiveN());
         assertThrows(IllegalArgumentException.class, chunk::decodePositiveN);
 
         chunk = new ScriptChunk(ScriptOpCodes.OP_DROP, null);
+        assertFalse(chunk.isPositiveN());
         assertThrows(IllegalArgumentException.class, chunk::decodePositiveN);
     }
 }