Genericise script number encoder

This replaces the limited script number encoder previously used for block
height with a version that matches the exact format generated by Bitcoin
Core (which can include an additional byte for sign in some cases).

With thanks to Kalpesh Parmar for pointing out that some blocks on the
testnet have coinbase transactions with height encoded in 2 bytes.

core/src/main/java/org/bitcoinj/core/Block.java

@@ -863,8 +863,7 @@ void addCoinbaseTransaction(byte[] pubKeyTo, Coin value, final int height) {
         final ScriptBuilder inputBuilder = new ScriptBuilder();
 
         if (height >= Block.BLOCK_HEIGHT_GENESIS) {
-            final byte[] blockHeightBytes = ScriptBuilder.createHeightScriptData(height);
-            inputBuilder.data(blockHeightBytes);
+            inputBuilder.number(height);
         }
         inputBuilder.data(new byte[]{(byte) txCounter, (byte) (txCounter++ >> 8)});
 

core/src/main/java/org/bitcoinj/core/Transaction.java

@@ -1176,7 +1176,7 @@ public void checkCoinBaseHeight(final int height)
         // Check block height is in coinbase input script
         final TransactionInput in = this.getInputs().get(0);
         final ScriptBuilder builder = new ScriptBuilder();
-        builder.data(ScriptBuilder.createHeightScriptData(height));
+        builder.number(height);
         final byte[] expected = builder.build().getProgram();
         final byte[] actual = in.getScriptBytes();
         if (actual.length < expected.length) {

core/src/main/java/org/bitcoinj/script/ScriptBuilder.java

@@ -27,11 +27,10 @@
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
+import java.util.Stack;
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkState;
-import java.nio.ByteBuffer;
-import java.nio.ByteOrder;
 import static org.bitcoinj.script.ScriptOpCodes.*;
 
 /**
@@ -107,18 +106,108 @@ public ScriptBuilder data(int index, byte[] data) {
         return addChunk(index, new ScriptChunk(opcode, copy));
     }
 
-    /** Adds the given number as a OP_N opcode to the end of the program. */
+    /**
+     * Adds the given number to the end of the program. Automatically uses
+     * shortest encoding possible.
+     */
+    public ScriptBuilder number(long num) {
+        if (num >= 0 && num < 16) {
+            return smallNum((int) num);
+        } else {
+            return bigNum(num);
+        }
+    }
+
+    /**
+     * Adds the given number to the given index in the program. Automatically
+     * uses shortest encoding possible.
+     */
+    public ScriptBuilder number(int index, long num) {
+        if (num >= 0 && num < 16) {
+            return addChunk(index, new ScriptChunk(Script.encodeToOpN((int) num), null));
+        } else {
+            return bigNum(index, num);
+        }
+    }
+
+    /**
+     * Adds the given number as a OP_N opcode to the end of the program.
+     * Only handles values 0-16 inclusive.
+     * 
+     * @see #number(int)
+     */
     public ScriptBuilder smallNum(int num) {
         return smallNum(chunks.size(), num);
     }
 
-    /** Adds the given number as a OP_N opcode to the given index in the program. */
+    /** Adds the given number as a push data chunk.
+     * This is intended to use for negative numbers or values > 16, and although
+     * it will accept numbers in the range 0-16 inclusive, the encoding would be
+     * considered non-standard.
+     * 
+     * @see #number(int)
+     */
+    protected ScriptBuilder bigNum(long num) {
+        return bigNum(chunks.size(), num);
+    }
+
+    /**
+     * Adds the given number as a OP_N opcode to the given index in the program.
+     * Only handles values 0-16 inclusive.
+     * 
+     * @see #number(int)
+     */
     public ScriptBuilder smallNum(int index, int num) {
         checkArgument(num >= 0, "Cannot encode negative numbers with smallNum");
         checkArgument(num <= 16, "Cannot encode numbers larger than 16 with smallNum");
         return addChunk(index, new ScriptChunk(Script.encodeToOpN(num), null));
     }
 
+    /**
+     * Adds the given number as a push data chunk to the given index in the program.
+     * This is intended to use for negative numbers or values > 16, and although
+     * it will accept numbers in the range 0-16 inclusive, the encoding would be
+     * considered non-standard.
+     * 
+     * @see #number(int)
+     */
+    protected ScriptBuilder bigNum(int index, long num) {
+        final byte[] data;
+
+        if (num == 0) {
+            data = new byte[0];
+        } else {
+            Stack<Byte> result = new Stack<Byte>();
+            final boolean neg = num < 0;
+            long absvalue = Math.abs(num);
+
+            while (absvalue != 0) {
+                result.push((byte) (absvalue & 0xff));
+                absvalue >>= 8;
+            }
+
+            if ((result.peek() & 0x80) != 0) {
+                // The most significant byte is >= 0x80, so push an extra byte that
+                // contains just the sign of the value.
+                result.push((byte) (neg ? 0x80 : 0));
+            } else if (neg) {
+                // The most significant byte is < 0x80 and the value is negative,
+                // set the sign bit so it is subtracted and interpreted as a
+                // negative when converting back to an integral.
+                result.push((byte) (result.pop() | 0x80));
+            }
+
+            data = new byte[result.size()];
+            for (int byteIdx = 0; byteIdx < data.length; byteIdx++) {
+                data[byteIdx] = result.get(byteIdx);
+            }
+        }
+
+        // At most the encoded value could take up to 8 bytes, so we don't need
+        // to use OP_PUSHDATA opcodes
+        return addChunk(index, new ScriptChunk(data.length, data));
+    }
+
     /** Creates a new immutable Script based on the state of the builder. */
     public Script build() {
         return new Script(chunks);
@@ -346,17 +435,4 @@ public static Script createOpReturnScript(byte[] data) {
         checkArgument(data.length <= 40);
         return new ScriptBuilder().op(OP_RETURN).data(data).build();
     }
-
-    /**
-     * Create script data bytes to represent the given block height.
-     */
-    public static byte[] createHeightScriptData(final int height) {
-        // TODO: Replace with something generic to any integer value
-        final byte[] int32Buffer = ByteBuffer.allocate(4).order(ByteOrder.LITTLE_ENDIAN).putInt(height).array();
-        if (int32Buffer[3] == 0) {
-            return Arrays.copyOf(int32Buffer, 3);
-        } else {
-            return int32Buffer;
-        }
-    }
 }

core/src/test/java/org/bitcoinj/core/BlockTest.java

@@ -17,7 +17,9 @@
 
 package org.bitcoinj.core;
 
+import com.google.common.io.ByteStreams;
 import org.bitcoinj.params.TestNet2Params;
+import org.bitcoinj.params.TestNet3Params;
 import org.bitcoinj.params.UnitTestParams;
 import org.bitcoinj.script.ScriptOpCodes;
 import org.junit.Before;
@@ -166,4 +168,30 @@ public void testUpdateLength() {
         assertEquals(block.length, origBlockLen + tx.length);
         assertEquals(tx.length, origTxLength + 41); // - 1 + 40 + 1 + 1
     }
+
+    @Test
+    public void testCoinbaseHeightTestnet() throws Exception {
+        // Testnet block 21066 (hash 0000000004053156021d8e42459d284220a7f6e087bf78f30179c3703ca4eefa)
+        // contains a coinbase transaction whose height is two bytes, which is
+        // shorter than we see in most other cases.
+
+        Block block = TestNet3Params.get().getDefaultSerializer().makeBlock(
+            ByteStreams.toByteArray(getClass().getResourceAsStream("block_testnet21066.dat")));
+
+        // Check block.
+        assertEquals("0000000004053156021d8e42459d284220a7f6e087bf78f30179c3703ca4eefa", block.getHashAsString());
+        block.verify(21066, EnumSet.of(Block.VerifyFlag.HEIGHT_IN_COINBASE));
+
+        // Testnet block 32768 (hash 000000007590ba495b58338a5806c2b6f10af921a70dbd814e0da3c6957c0c03)
+        // contains a coinbase transaction whose height is three bytes, but could
+        // fit in two bytes. This test primarily ensures script encoding checks
+        // are applied correctly.
+
+        block = TestNet3Params.get().getDefaultSerializer().makeBlock(
+            ByteStreams.toByteArray(getClass().getResourceAsStream("block_testnet32768.dat")));
+
+        // Check block.
+        assertEquals("000000007590ba495b58338a5806c2b6f10af921a70dbd814e0da3c6957c0c03", block.getHashAsString());
+        block.verify(32768, EnumSet.of(Block.VerifyFlag.HEIGHT_IN_COINBASE));
+    }
 }

core/src/test/java/org/bitcoinj/script/ScriptTest.java

@@ -412,4 +412,65 @@ public void getToAddress() throws Exception {
     public void getToAddressNoPubKey() throws Exception {
         ScriptBuilder.createOutputScript(new ECKey()).getToAddress(params, false);
     }
+
+    /** Test encoding of zero, which should result in an opcode */
+    @Test
+    public void numberBuilderZero() {
+        final ScriptBuilder builder = new ScriptBuilder();
+
+        // 0 should encode directly to 0
+        builder.number(0);
+        assertArrayEquals(new byte[] {
+            0x00         // Pushed data
+        }, builder.build().getProgram());
+    }
+
+    @Test
+    public void numberBuilderPositiveOpCode() {
+        final ScriptBuilder builder = new ScriptBuilder();
+
+        builder.number(5);
+        assertArrayEquals(new byte[] {
+            0x55         // Pushed data
+        }, builder.build().getProgram());
+    }
+
+    @Test
+    public void numberBuilderBigNum() {
+        ScriptBuilder builder = new ScriptBuilder();
+        // 21066 should take up three bytes including the length byte
+        // at the start
+
+        builder.number(0x524a);
+        assertArrayEquals(new byte[] {
+            0x02,             // Length of the pushed data
+            0x4a, 0x52        // Pushed data
+        }, builder.build().getProgram());
+
+        // Test the trimming code ignores zeroes in the middle
+        builder = new ScriptBuilder();
+        builder.number(0x110011);
+        assertEquals(4, builder.build().getProgram().length);
+
+        // Check encoding of a value where signed/unsigned encoding differs
+        // because the most significant byte is 0x80, and therefore a
+        // sign byte has to be added to the end for the signed encoding.
+        builder = new ScriptBuilder();
+        builder.number(0x8000);
+        assertArrayEquals(new byte[] {
+            0x03,             // Length of the pushed data
+            0x00, (byte) 0x80, 0x00  // Pushed data
+        }, builder.build().getProgram());
+    }
+
+    @Test
+    public void numberBuilderNegative() {
+        // Check encoding of a negative value
+        final ScriptBuilder builder = new ScriptBuilder();
+        builder.number(-5);
+        assertArrayEquals(new byte[] {
+            0x01,        // Length of the pushed data
+            ((byte) 133) // Pushed data
+        }, builder.build().getProgram());
+    }
 }