Add new helper methods to Transaction[Signature]

Add a method for signing a given input and use it.
Convert some code that was working with raw bytes to use TransactionSignature.
Rename Transaction.hashTransactionForSignature to just hashForSignature.
General API tweaks and cleanups.

core/src/main/java/com/google/bitcoin/core/ECKey.java

@@ -475,57 +475,24 @@ public class ECKey implements Serializable {
     }
 
     /**
-     * Returns true if the given signature is in canonical form (ie will be accepted as standard by the reference client)
+     * Returns true if this pubkey is canonical, i.e. the correct length taking into account compression.
      */
-    public static boolean isSignatureCanonical(byte[] signature) {
+    public boolean isPubKeyCanonical() {
-        // See reference client's IsCanonicalSignature, https://bitcointalk.org/index.php?topic=8392.msg127623#msg127623
+        return isPubKeyCanonical(pub);
-        // A canonical signature exists of: <30> <total len> <02> <len R> <R> <02> <len S> <S> <hashtype>
-        // Where R and S are not negative (their first byte has its highest bit not set), and not
-        // excessively padded (do not start with a 0 byte, unless an otherwise negative number follows,
-        // in which case a single 0 byte is necessary and even required).
-        if (signature.length < 9 || signature.length > 73)
-            return false;
-
-        int hashType = signature[signature.length-1] & ((int)(~Transaction.SIGHASH_ANYONECANPAY_VALUE));
-        if (hashType < (Transaction.SigHash.ALL.ordinal() + 1) || hashType > (Transaction.SigHash.SINGLE.ordinal() + 1))
-            return false;
-
-        //                   "wrong type"                  "wrong length marker"
-        if ((signature[0] & 0xff) != 0x30 || (signature[1] & 0xff) != signature.length-3)
-            return false;
-
-        int lenR = signature[3] & 0xff;
-        if (5 + lenR >= signature.length || lenR == 0)
-            return false;
-        int lenS = signature[5+lenR] & 0xff;
-        if (lenR + lenS + 7 != signature.length || lenS == 0)
-            return false;
-
-        //    R value type mismatch          R value negative
-        if (signature[4-2] != 0x02 || (signature[4] & 0x80) == 0x80)
-            return false;
-        if (lenR > 1 && signature[4] == 0x00 && (signature[4+1] & 0x80) != 0x80)
-            return false; // R value excessively padded
-
-        //       S value type mismatch                    S value negative
-        if (signature[6 + lenR - 2] != 0x02 || (signature[6 + lenR] & 0x80) == 0x80)
-            return false;
-        if (lenS > 1 && signature[6 + lenR] == 0x00 && (signature[6 + lenR + 1] & 0x80) != 0x80)
-            return false; // S value excessively padded
-
-        return true;
     }
 
     /**
-     * Returns true if the given pubkey is canonical (ie the correct length for its declared type)
+     * Returns true if the given pubkey is canonical, i.e. the correct length taking into account compression.
      */
     public static boolean isPubKeyCanonical(byte[] pubkey) {
         if (pubkey.length < 33)
             return false;
-        if (pubkey[0] == 0x04) { // Uncompressed pubkey
+        if (pubkey[0] == 0x04) {
+            // Uncompressed pubkey
             if (pubkey.length != 65)
                 return false;
-        } else if (pubkey[0] == 0x02 || pubkey[0] == 0x03) { // Compressed pubkey
+        } else if (pubkey[0] == 0x02 || pubkey[0] == 0x03) {
+            // Compressed pubkey
             if (pubkey.length != 33)
                 return false;
         } else

core/src/main/java/com/google/bitcoin/core/Transaction.java

@@ -797,11 +797,7 @@ public class Transaction extends ChildMessage implements Serializable {
             // The anyoneCanPay feature isn't used at the moment.
             boolean anyoneCanPay = false;
             byte[] connectedPubKeyScript = input.getOutpoint().getConnectedPubKeyScript();
-            Sha256Hash hash = hashTransactionForSignature(i, connectedPubKeyScript, hashType, anyoneCanPay);
+            signatures[i] = calculateSignature(i, key, aesKey, connectedPubKeyScript, hashType, anyoneCanPay);
-
-            // Now calculate the signatures we need to prove we own this transaction and are authorized to claim the
-            // associated money.
-            signatures[i] = new TransactionSignature(key.sign(hash, aesKey), hashType, anyoneCanPay);
         }
 
         // Now we have calculated each signature, go through and create the scripts. Reminder: the script consists:
@@ -828,6 +824,45 @@ public class Transaction extends ChildMessage implements Serializable {
         // Every input is now complete.
     }
 
+    /**
+     * Calculates a signature that is valid for being inserted into the input at the given position. This is simply
+     * a wrapper around calling {@link Transaction#hashForSignature(int, byte[], com.google.bitcoin.core.Transaction.SigHash, boolean)}
+     * followed by {@link ECKey#sign(Sha256Hash, org.spongycastle.crypto.params.KeyParameter)} and then returning
+     * a new {@link TransactionSignature}.
+     *
+     * @param inputIndex Which input to calculate the signature for, as an index.
+     * @param key The private key used to calculate the signature.
+     * @param aesKey If not null, this will be used to decrypt the key.
+     * @param connectedPubKeyScript Byte-exact contents of the scriptPubKey that is being satisified.
+     * @param hashType Signing mode, see the enum for documentation.
+     * @param anyoneCanPay Signing mode, see the SigHash enum for documentation.
+     * @return A newly calculated signature object that wraps the r, s and sighash components.
+     */
+    public synchronized  TransactionSignature calculateSignature(int inputIndex, ECKey key, KeyParameter aesKey,
+                                                                 byte[] connectedPubKeyScript,
+                                                                 SigHash hashType, boolean anyoneCanPay) {
+        Sha256Hash hash = hashForSignature(inputIndex, connectedPubKeyScript, hashType, anyoneCanPay);
+        return new TransactionSignature(key.sign(hash, aesKey), hashType, anyoneCanPay);
+    }
+
+    /**
+     * Calculates a signature that is valid for being inserted into the input at the given position. This is simply
+     * a wrapper around calling {@link Transaction#hashForSignature(int, byte[], com.google.bitcoin.core.Transaction.SigHash, boolean)}
+     * followed by {@link ECKey#sign(Sha256Hash)} and then returning a new {@link TransactionSignature}.
+     *
+     * @param inputIndex Which input to calculate the signature for, as an index.
+     * @param key The private key used to calculate the signature.
+     * @param connectedPubKeyScript The scriptPubKey that is being satisified.
+     * @param hashType Signing mode, see the enum for documentation.
+     * @param anyoneCanPay Signing mode, see the SigHash enum for documentation.
+     * @return A newly calculated signature object that wraps the r, s and sighash components.
+     */
+    public synchronized  TransactionSignature calculateSignature(int inputIndex, ECKey key, Script connectedPubKeyScript,
+                                                                 SigHash hashType, boolean anyoneCanPay) {
+        Sha256Hash hash = hashForSignature(inputIndex, connectedPubKeyScript.getProgram(), hashType, anyoneCanPay);
+        return new TransactionSignature(key.sign(hash), hashType, anyoneCanPay);
+    }
+
     /**
      * <p>Calculates a signature hash, that is, a hash of a simplified form of the transaction. How exactly the transaction
      * is simplified is specified by the type and anyoneCanPay parameters.</p>
@@ -840,10 +875,10 @@ public class Transaction extends ChildMessage implements Serializable {
      * @param type Should be SigHash.ALL
      * @param anyoneCanPay should be false.
      */
-    public synchronized Sha256Hash hashTransactionForSignature(int inputIndex, byte[] connectedScript,
+    public synchronized Sha256Hash hashForSignature(int inputIndex, byte[] connectedScript,
                                                     SigHash type, boolean anyoneCanPay) {
         byte sigHashType = (byte) TransactionSignature.calcSigHashValue(type, anyoneCanPay);
-        return hashTransactionForSignature(inputIndex, connectedScript, sigHashType);
+        return hashForSignature(inputIndex, connectedScript, sigHashType);
     }
 
     /**
@@ -858,17 +893,17 @@ public class Transaction extends ChildMessage implements Serializable {
      * @param type Should be SigHash.ALL
      * @param anyoneCanPay should be false.
      */
-    public synchronized Sha256Hash hashTransactionForSignature(int inputIndex, Script connectedScript,
+    public synchronized Sha256Hash hashForSignature(int inputIndex, Script connectedScript,
                                                     SigHash type, boolean anyoneCanPay) {
         int sigHash = TransactionSignature.calcSigHashValue(type, anyoneCanPay);
-        return hashTransactionForSignature(inputIndex, connectedScript.getProgram(), (byte) sigHash);
+        return hashForSignature(inputIndex, connectedScript.getProgram(), (byte) sigHash);
     }
 
     /**
      * This is required for signatures which use a sigHashType which cannot be represented using SigHash and anyoneCanPay
      * See transaction c99c49da4c38af669dea436d3e73780dfdb6c1ecf9958baa52960e8baee30e73, which has sigHashType 0
      */
-    public synchronized Sha256Hash hashTransactionForSignature(int inputIndex, byte[] connectedScript, byte sigHashType) {
+    public synchronized Sha256Hash hashForSignature(int inputIndex, byte[] connectedScript, byte sigHashType) {
         // The SIGHASH flags are used in the design of contracts, please see this page for a further understanding of
         // the purposes of the code in this method:
         //

core/src/main/java/com/google/bitcoin/crypto/TransactionSignature.java

@@ -18,6 +18,7 @@ package com.google.bitcoin.crypto;
 
 import com.google.bitcoin.core.ECKey;
 import com.google.bitcoin.core.Transaction;
+import com.google.bitcoin.core.VerificationException;
 
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
@@ -55,6 +56,53 @@ public class TransactionSignature extends ECKey.ECDSASignature {
         return sighashFlags;
     }
 
+    /**
+     * Returns true if the given signature is has canonical encoding, and will thus be accepted as standard by
+     * the reference client. DER and the SIGHASH encoding allow for quite some flexibility in how the same structures
+     * are encoded, and this can open up novel attacks in which a man in the middle takes a transaction and then
+     * changes its signature such that the transaction hash is different but it's still valid. This can confuse wallets
+     * and generally violates people's mental model of how Bitcoin should work, thus, non-canonical signatures are now
+     * not relayed by default.
+     */
+    public static boolean isEncodingCanonical(byte[] signature) {
+        // See reference client's IsCanonicalSignature, https://bitcointalk.org/index.php?topic=8392.msg127623#msg127623
+        // A canonical signature exists of: <30> <total len> <02> <len R> <R> <02> <len S> <S> <hashtype>
+        // Where R and S are not negative (their first byte has its highest bit not set), and not
+        // excessively padded (do not start with a 0 byte, unless an otherwise negative number follows,
+        // in which case a single 0 byte is necessary and even required).
+        if (signature.length < 9 || signature.length > 73)
+            return false;
+
+        int hashType = signature[signature.length-1] & ((int)(~Transaction.SIGHASH_ANYONECANPAY_VALUE));
+        if (hashType < (Transaction.SigHash.ALL.ordinal() + 1) || hashType > (Transaction.SigHash.SINGLE.ordinal() + 1))
+            return false;
+
+        //                   "wrong type"                  "wrong length marker"
+        if ((signature[0] & 0xff) != 0x30 || (signature[1] & 0xff) != signature.length-3)
+            return false;
+
+        int lenR = signature[3] & 0xff;
+        if (5 + lenR >= signature.length || lenR == 0)
+            return false;
+        int lenS = signature[5+lenR] & 0xff;
+        if (lenR + lenS + 7 != signature.length || lenS == 0)
+            return false;
+
+        //    R value type mismatch          R value negative
+        if (signature[4-2] != 0x02 || (signature[4] & 0x80) == 0x80)
+            return false;
+        if (lenR > 1 && signature[4] == 0x00 && (signature[4+1] & 0x80) != 0x80)
+            return false; // R value excessively padded
+
+        //       S value type mismatch                    S value negative
+        if (signature[6 + lenR - 2] != 0x02 || (signature[6 + lenR] & 0x80) == 0x80)
+            return false;
+        if (lenS > 1 && signature[6 + lenR] == 0x00 && (signature[6 + lenR + 1] & 0x80) != 0x80)
+            return false; // S value excessively padded
+
+        return true;
+    }
+
     /** Configures the sighashFlags field as appropriate. */
     public void setSigHash(Transaction.SigHash mode, boolean anyoneCanPay) {
         sighashFlags = calcSigHashValue(mode, anyoneCanPay);
@@ -93,13 +141,16 @@ public class TransactionSignature extends ECKey.ECDSASignature {
      * Returns a decoded signature.
      * @throws RuntimeException if the signature is invalid or unparseable in some way.
      */
-    public static TransactionSignature decodeFromBitcoin(byte[] bytes) {
+    public static TransactionSignature decodeFromBitcoin(byte[] bytes, boolean requireCanonical) throws VerificationException {
         // Bitcoin encoding is DER signature + sighash byte.
+        if (requireCanonical && !isEncodingCanonical(bytes))
+            throw new VerificationException("Signature encoding is not canonical.");
         ECKey.ECDSASignature sig = ECKey.ECDSASignature.decodeFromDER(bytes);
         if (sig == null)
-            throw new RuntimeException("Could not DER decode signature.");
+            throw new VerificationException("Could not decode DER component.");
         TransactionSignature tsig = new TransactionSignature(sig.r, sig.s);
-        // In Bitcoin, any value of the final byte is valid unfortunately. However it may not be "canonical".
+        // In Bitcoin, any value of the final byte is valid, but not necessarily canonical. See javadocs for
+        // isEncodingCanonical to learn more about this.
         tsig.sighashFlags = bytes[bytes.length - 1];
         return tsig;
     }

core/src/main/java/com/google/bitcoin/script/Script.java

@@ -1061,8 +1061,8 @@ public class Script {
         // TODO: Use int for indexes everywhere, we can't have that many inputs/outputs
         boolean sigValid = false;
         try {
-            TransactionSignature sig  = TransactionSignature.decodeFromBitcoin(sigBytes);
+            TransactionSignature sig  = TransactionSignature.decodeFromBitcoin(sigBytes, false);
-            Sha256Hash hash = txContainingThis.hashTransactionForSignature(index, connectedScript, (byte)sig.sighashFlags);
+            Sha256Hash hash = txContainingThis.hashForSignature(index, connectedScript, (byte) sig.sighashFlags);
             sigValid = ECKey.verify(hash.getBytes(), sig, pubKey);
         } catch (Exception e1) {
             // There is (at least) one exception that could be hit here (EOFException, if the sig is too short)
@@ -1131,8 +1131,8 @@ public class Script {
             // We could reasonably move this out of the loop, but because signature verification is significantly
             // more expensive than hashing, its not a big deal.
             try {
-                TransactionSignature sig = TransactionSignature.decodeFromBitcoin(sigs.getFirst());
+                TransactionSignature sig = TransactionSignature.decodeFromBitcoin(sigs.getFirst(), false);
-                Sha256Hash hash = txContainingThis.hashTransactionForSignature(index, connectedScript, (byte)sig.sighashFlags);
+                Sha256Hash hash = txContainingThis.hashForSignature(index, connectedScript, (byte) sig.sighashFlags);
                 if (ECKey.verify(hash.getBytes(), sig, pubKey))
                     sigs.pollFirst();
             } catch (Exception e) {

core/src/test/java/com/google/bitcoin/core/ECKeyTest.java

@@ -19,6 +19,7 @@ package com.google.bitcoin.core;
 import com.google.bitcoin.crypto.EncryptedPrivateKey;
 import com.google.bitcoin.crypto.KeyCrypter;
 import com.google.bitcoin.crypto.KeyCrypterScrypt;
+import com.google.bitcoin.crypto.TransactionSignature;
 import com.google.bitcoin.params.MainNetParams;
 import com.google.bitcoin.params.TestNet3Params;
 import com.google.bitcoin.params.UnitTestParams;
@@ -404,7 +405,7 @@ public class ECKeyTest {
             while (in.available() > 0 && (c = in.read()) != '"')
                 sig.append((char)c);
 
-            assertTrue(ECKey.isSignatureCanonical(Hex.decode(sig.toString())));
+            assertTrue(TransactionSignature.isEncodingCanonical(Hex.decode(sig.toString())));
         }
         in.close();
     }
@@ -426,7 +427,7 @@ public class ECKeyTest {
                 sig.append((char)c);
 
             try {
-                assertFalse(ECKey.isSignatureCanonical(Hex.decode(sig.toString())));
+                assertFalse(TransactionSignature.isEncodingCanonical(Hex.decode(sig.toString())));
             } catch (StringIndexOutOfBoundsException e) { } // Expected for non-hex strings in the JSON that we should ignore
         }
         in.close();
@@ -447,7 +448,7 @@ public class ECKeyTest {
         byte[] sigBytes = key.sign(new Sha256Hash(hash)).encodeToDER();
         byte[] encodedSig = Arrays.copyOf(sigBytes, sigBytes.length + 1);
         encodedSig[sigBytes.length] = (byte) (Transaction.SigHash.ALL.ordinal() + 1);
-        if (!ECKey.isSignatureCanonical(encodedSig)) {
+        if (!TransactionSignature.isEncodingCanonical(encodedSig)) {
             log.error(Utils.bytesToHexString(sigBytes));
             fail();
         }

core/src/test/java/com/google/bitcoin/core/FullBlockTestGenerator.java

@@ -716,7 +716,7 @@ public class FullBlockTestGenerator {
 
                 if (scriptSig == null) {
                     // Exploit the SigHash.SINGLE bug to avoid having to make more than one signature
-                    Sha256Hash hash = tx.hashTransactionForSignature(1, b39p2shScriptPubKey, SigHash.SINGLE, false);
+                    Sha256Hash hash = tx.hashForSignature(1, b39p2shScriptPubKey, SigHash.SINGLE, false);
 
                     // Sign input
                     try {
@@ -785,7 +785,7 @@ public class FullBlockTestGenerator {
 
                     if (scriptSig == null) {
                         // Exploit the SigHash.SINGLE bug to avoid having to make more than one signature
-                        Sha256Hash hash = tx.hashTransactionForSignature(1,
+                        Sha256Hash hash = tx.hashForSignature(1,
                                 b39p2shScriptPubKey, SigHash.SINGLE, false);
 
                         // Sign input
@@ -1565,7 +1565,7 @@ public class FullBlockTestGenerator {
         t.addInput(input);
 
         byte[] connectedPubKeyScript = prevOut.scriptPubKey.getProgram();
-        Sha256Hash hash = t.hashTransactionForSignature(0, connectedPubKeyScript, SigHash.ALL, false);
+        Sha256Hash hash = t.hashForSignature(0, connectedPubKeyScript, SigHash.ALL, false);
 
         // Sign input
         try {

core/src/test/java/com/google/bitcoin/core/FullPrunedBlockChainTest.java

@@ -160,7 +160,7 @@ public class FullPrunedBlockChainTest {
         TransactionInput input = new TransactionInput(params, t, new byte[]{}, prevOut);
         t.addInput(input);
 
-        Sha256Hash hash = t.hashTransactionForSignature(0, prevOutScriptPubKey, SigHash.ALL, false);
+        Sha256Hash hash = t.hashForSignature(0, prevOutScriptPubKey, SigHash.ALL, false);
 
         // Sign input
         try {