From 1a38b8e6628b8218bf473b15e49d319586cbc1d8 Mon Sep 17 00:00:00 2001 From: Craig Raw Date: Mon, 10 Aug 2020 13:19:24 +0200 Subject: [PATCH] fix transaction signature ordering bug --- .../sparrowwallet/drongo/crypto/ECKey.java | 22 ++++++ .../drongo/protocol/ScriptType.java | 68 ++++++++++--------- .../sparrowwallet/drongo/wallet/Wallet.java | 33 ++++++--- .../drongo/protocol/TransactionTest.java | 24 +++++-- 4 files changed, 101 insertions(+), 46 deletions(-) diff --git a/src/main/java/com/sparrowwallet/drongo/crypto/ECKey.java b/src/main/java/com/sparrowwallet/drongo/crypto/ECKey.java index e5b8141..ac3837d 100644 --- a/src/main/java/com/sparrowwallet/drongo/crypto/ECKey.java +++ b/src/main/java/com/sparrowwallet/drongo/crypto/ECKey.java @@ -909,4 +909,26 @@ public class ECKey implements EncryptableItem { public String toString() { return pub.toString(); } + + public static class LexicographicECKeyComparator implements Comparator { + @Override + public int compare(ECKey leftKey, ECKey rightKey) { + byte[] left = leftKey.getPubKey(); + byte[] right = rightKey.getPubKey(); + + int minLength = Math.min(left.length, right.length); + for (int i = 0; i < minLength; i++) { + int result = compare(left[i], right[i]); + if (result != 0) { + return result; + } + } + + return left.length - right.length; + } + + public static int compare(byte a, byte b) { + return Byte.toUnsignedInt(a) - Byte.toUnsignedInt(b); + } + } } diff --git a/src/main/java/com/sparrowwallet/drongo/protocol/ScriptType.java b/src/main/java/com/sparrowwallet/drongo/protocol/ScriptType.java index c60d83c..7f67cd0 100644 --- a/src/main/java/com/sparrowwallet/drongo/protocol/ScriptType.java +++ b/src/main/java/com/sparrowwallet/drongo/protocol/ScriptType.java @@ -111,12 +111,12 @@ public enum ScriptType { } @Override - public Script getMultisigScriptSig(Script scriptPubKey, int threshold, List pubKeys, List signatures) { + public Script getMultisigScriptSig(Script scriptPubKey, int threshold, Map pubKeySignatures) { throw new ProtocolException(getName() + " is not a multisig script type"); } @Override - public TransactionInput addMultisigSpendingInput(Transaction transaction, TransactionOutput prevOutput, int threshold, List pubKeys, List signatures) { + public TransactionInput addMultisigSpendingInput(Transaction transaction, TransactionOutput prevOutput, int threshold, Map pubKeySignatures) { throw new ProtocolException(getName() + " is not a multisig script type"); } @@ -219,12 +219,12 @@ public enum ScriptType { } @Override - public Script getMultisigScriptSig(Script scriptPubKey, int threshold, List pubKeys, List signatures) { + public Script getMultisigScriptSig(Script scriptPubKey, int threshold, Map pubKeySignatures) { throw new ProtocolException(getName() + " is not a multisig script type"); } @Override - public TransactionInput addMultisigSpendingInput(Transaction transaction, TransactionOutput prevOutput, int threshold, List pubKeys, List signatures) { + public TransactionInput addMultisigSpendingInput(Transaction transaction, TransactionOutput prevOutput, int threshold, Map pubKeySignatures) { throw new ProtocolException(getName() + " is not a multisig script type"); } @@ -274,7 +274,7 @@ public enum ScriptType { } @Override - public Script getOutputScript(int threshold, List pubKeys) { + public Script getOutputScript(int threshold, Collection pubKeys) { if(threshold > pubKeys.size()) { throw new ProtocolException("Threshold of " + threshold + " is greater than number of pubKeys provided (" + pubKeys.size() + ")"); } @@ -382,11 +382,13 @@ public enum ScriptType { } @Override - public Script getMultisigScriptSig(Script scriptPubKey, int threshold, List pubKeys, List signatures) { + public Script getMultisigScriptSig(Script scriptPubKey, int threshold, Map pubKeySignatures) { if(!isScriptType(scriptPubKey)) { throw new ProtocolException("Provided scriptPubKey is not a " + getName() + " script"); } - if(threshold != signatures.size()) { + + List signatures = pubKeySignatures.values().stream().filter(Objects::nonNull).collect(Collectors.toList()); + if(signatures.size() < threshold) { throw new ProtocolException("Only " + signatures.size() + " signatures provided to meet a multisig threshold of " + threshold); } @@ -402,8 +404,8 @@ public enum ScriptType { } @Override - public TransactionInput addMultisigSpendingInput(Transaction transaction, TransactionOutput prevOutput, int threshold, List pubKeys, List signatures) { - Script scriptSig = getMultisigScriptSig(prevOutput.getScript(), threshold, pubKeys, signatures); + public TransactionInput addMultisigSpendingInput(Transaction transaction, TransactionOutput prevOutput, int threshold, Map pubKeySignatures) { + Script scriptSig = getMultisigScriptSig(prevOutput.getScript(), threshold, pubKeySignatures); return transaction.addInput(prevOutput.getHash(), prevOutput.getIndex(), scriptSig); } @@ -503,17 +505,17 @@ public enum ScriptType { } @Override - public Script getMultisigScriptSig(Script scriptPubKey, int threshold, List pubKeys, List signatures) { + public Script getMultisigScriptSig(Script scriptPubKey, int threshold, Map pubKeySignatures) { if(!isScriptType(scriptPubKey)) { throw new ProtocolException("Provided scriptPubKey is not a " + getName() + " script"); } - Script redeemScript = MULTISIG.getOutputScript(threshold, pubKeys); + Script redeemScript = MULTISIG.getOutputScript(threshold, pubKeySignatures.keySet()); if(!scriptPubKey.equals(getOutputScript(redeemScript))) { throw new ProtocolException("P2SH scriptPubKey hash does not match constructed redeem script hash"); } - Script multisigScript = MULTISIG.getMultisigScriptSig(redeemScript, threshold, pubKeys, signatures); + Script multisigScript = MULTISIG.getMultisigScriptSig(redeemScript, threshold, pubKeySignatures); List chunks = new ArrayList<>(multisigScript.getChunks()); ScriptChunk redeemScriptChunk = ScriptChunk.fromData(redeemScript.getProgram()); chunks.add(redeemScriptChunk); @@ -522,8 +524,8 @@ public enum ScriptType { } @Override - public TransactionInput addMultisigSpendingInput(Transaction transaction, TransactionOutput prevOutput, int threshold, List pubKeys, List signatures) { - Script scriptSig = getMultisigScriptSig(prevOutput.getScript(), threshold, pubKeys, signatures); + public TransactionInput addMultisigSpendingInput(Transaction transaction, TransactionOutput prevOutput, int threshold, Map pubKeySignatures) { + Script scriptSig = getMultisigScriptSig(prevOutput.getScript(), threshold, pubKeySignatures); return transaction.addInput(prevOutput.getHash(), prevOutput.getIndex(), scriptSig); } @@ -616,12 +618,12 @@ public enum ScriptType { } @Override - public Script getMultisigScriptSig(Script scriptPubKey, int threshold, List pubKeys, List signatures) { + public Script getMultisigScriptSig(Script scriptPubKey, int threshold, Map pubKeySignatures) { throw new ProtocolException(getName() + " is not a multisig script type"); } @Override - public TransactionInput addMultisigSpendingInput(Transaction transaction, TransactionOutput prevOutput, int threshold, List pubKeys, List signatures) { + public TransactionInput addMultisigSpendingInput(Transaction transaction, TransactionOutput prevOutput, int threshold, Map pubKeySignatures) { throw new ProtocolException(getName() + " is not a multisig script type"); } @@ -698,12 +700,12 @@ public enum ScriptType { } @Override - public Script getMultisigScriptSig(Script scriptPubKey, int threshold, List pubKeys, List signatures) { + public Script getMultisigScriptSig(Script scriptPubKey, int threshold, Map pubKeySignatures) { if(!isScriptType(scriptPubKey)) { throw new ProtocolException("Provided scriptPubKey is not a " + getName() + " script"); } - Script witnessScript = MULTISIG.getOutputScript(threshold, pubKeys); + Script witnessScript = MULTISIG.getOutputScript(threshold, pubKeySignatures.keySet()); Script redeemScript = P2WSH.getOutputScript(witnessScript); if(!scriptPubKey.equals(P2SH.getOutputScript(redeemScript))) { throw new ProtocolException("P2SH scriptPubKey hash does not match constructed redeem script hash"); @@ -714,10 +716,10 @@ public enum ScriptType { } @Override - public TransactionInput addMultisigSpendingInput(Transaction transaction, TransactionOutput prevOutput, int threshold, List pubKeys, List signatures) { - Script scriptSig = getMultisigScriptSig(prevOutput.getScript(), threshold, pubKeys, signatures); - Script witnessScript = MULTISIG.getOutputScript(threshold, pubKeys); - TransactionWitness witness = new TransactionWitness(transaction, signatures, witnessScript); + public TransactionInput addMultisigSpendingInput(Transaction transaction, TransactionOutput prevOutput, int threshold, Map pubKeySignatures) { + Script scriptSig = getMultisigScriptSig(prevOutput.getScript(), threshold, pubKeySignatures); + Script witnessScript = MULTISIG.getOutputScript(threshold, pubKeySignatures.keySet()); + TransactionWitness witness = new TransactionWitness(transaction, pubKeySignatures.values().stream().filter(Objects::nonNull).collect(Collectors.toList()), witnessScript); return transaction.addInput(prevOutput.getHash(), prevOutput.getIndex(), scriptSig, witness); } @@ -812,12 +814,12 @@ public enum ScriptType { } @Override - public Script getMultisigScriptSig(Script scriptPubKey, int threshold, List pubKeys, List signatures) { + public Script getMultisigScriptSig(Script scriptPubKey, int threshold, Map pubKeySignatures) { throw new ProtocolException(getName() + " is not a multisig script type"); } @Override - public TransactionInput addMultisigSpendingInput(Transaction transaction, TransactionOutput prevOutput, int threshold, List pubKeys, List signatures) { + public TransactionInput addMultisigSpendingInput(Transaction transaction, TransactionOutput prevOutput, int threshold, Map pubKeySignatures) { throw new ProtocolException(getName() + " is not a multisig script type"); } @@ -906,12 +908,12 @@ public enum ScriptType { } @Override - public Script getMultisigScriptSig(Script scriptPubKey, int threshold, List pubKeys, List signatures) { + public Script getMultisigScriptSig(Script scriptPubKey, int threshold, Map pubKeySignatures) { if(!isScriptType(scriptPubKey)) { throw new ProtocolException("Provided scriptPubKey is not a " + getName() + " script"); } - Script witnessScript = MULTISIG.getOutputScript(threshold, pubKeys); + Script witnessScript = MULTISIG.getOutputScript(threshold, pubKeySignatures.keySet()); if(!scriptPubKey.equals(P2WSH.getOutputScript(witnessScript))) { throw new ProtocolException("P2WSH scriptPubKey hash does not match constructed witness script hash"); } @@ -920,10 +922,10 @@ public enum ScriptType { } @Override - public TransactionInput addMultisigSpendingInput(Transaction transaction, TransactionOutput prevOutput, int threshold, List pubKeys, List signatures) { - Script scriptSig = getMultisigScriptSig(prevOutput.getScript(), threshold, pubKeys, signatures); - Script witnessScript = MULTISIG.getOutputScript(threshold, pubKeys); - TransactionWitness witness = new TransactionWitness(transaction, signatures, witnessScript); + public TransactionInput addMultisigSpendingInput(Transaction transaction, TransactionOutput prevOutput, int threshold, Map pubKeySignatures) { + Script scriptSig = getMultisigScriptSig(prevOutput.getScript(), threshold, pubKeySignatures); + Script witnessScript = MULTISIG.getOutputScript(threshold, pubKeySignatures.keySet()); + TransactionWitness witness = new TransactionWitness(transaction, pubKeySignatures.values().stream().filter(Objects::nonNull).collect(Collectors.toList()), witnessScript); return transaction.addInput(prevOutput.getHash(), prevOutput.getIndex(), scriptSig, witness); } @@ -993,7 +995,7 @@ public enum ScriptType { public abstract Script getOutputScript(Script script); - public Script getOutputScript(int threshold, List pubKeys) { + public Script getOutputScript(int threshold, Collection pubKeys) { throw new UnsupportedOperationException("Only defined for MULTISIG script type"); } @@ -1025,9 +1027,9 @@ public enum ScriptType { public abstract TransactionInput addSpendingInput(Transaction transaction, TransactionOutput prevOutput, ECKey pubKey, TransactionSignature signature); - public abstract Script getMultisigScriptSig(Script scriptPubKey, int threshold, List pubKeys, List signatures); + public abstract Script getMultisigScriptSig(Script scriptPubKey, int threshold, Map pubKeySignatures); - public abstract TransactionInput addMultisigSpendingInput(Transaction transaction, TransactionOutput prevOutput, int threshold, List pubKeys, List signatures); + public abstract TransactionInput addMultisigSpendingInput(Transaction transaction, TransactionOutput prevOutput, int threshold, Map pubKeySignatures); public static final ScriptType[] SINGLE_HASH_TYPES = {P2PKH, P2SH, P2SH_P2WPKH, P2SH_P2WSH, P2WPKH, P2WSH}; diff --git a/src/main/java/com/sparrowwallet/drongo/wallet/Wallet.java b/src/main/java/com/sparrowwallet/drongo/wallet/Wallet.java index e181280..3d7be49 100644 --- a/src/main/java/com/sparrowwallet/drongo/wallet/Wallet.java +++ b/src/main/java/com/sparrowwallet/drongo/wallet/Wallet.java @@ -2,6 +2,7 @@ package com.sparrowwallet.drongo.wallet; import com.sparrowwallet.drongo.BitcoinUnit; import com.sparrowwallet.drongo.KeyPurpose; +import com.sparrowwallet.drongo.Utils; import com.sparrowwallet.drongo.address.Address; import com.sparrowwallet.drongo.crypto.ECKey; import com.sparrowwallet.drongo.crypto.Key; @@ -14,7 +15,6 @@ import com.sparrowwallet.drongo.psbt.PSBTInput; import java.util.*; import java.util.function.Function; import java.util.stream.Collectors; -import java.util.stream.IntStream; import static com.sparrowwallet.drongo.protocol.Transaction.WITNESS_SCALE_FACTOR; @@ -390,11 +390,11 @@ public class Wallet { } else if(getPolicyType().equals(PolicyType.MULTI)) { List pubKeys = getPubKeys(receiveNode); int threshold = getDefaultPolicy().getNumSignaturesRequired(); - List signatures = new ArrayList<>(threshold); - for(int i = 0; i < threshold; i++) { - signatures.add(TransactionSignature.dummy()); + Map pubKeySignatures = new TreeMap<>(new ECKey.LexicographicECKeyComparator()); + for(int i = 0; i < pubKeys.size(); i++) { + pubKeySignatures.put(pubKeys.get(i), i < threshold ? TransactionSignature.dummy() : null); } - txInput = getScriptType().addMultisigSpendingInput(transaction, prevTxOut, threshold, pubKeys, signatures); + txInput = getScriptType().addMultisigSpendingInput(transaction, prevTxOut, threshold, pubKeySignatures); } assert txInput != null; @@ -491,8 +491,11 @@ public class Wallet { } else if(getPolicyType().equals(PolicyType.MULTI)) { List pubKeys = getPubKeys(walletNode); int threshold = getDefaultPolicy().getNumSignaturesRequired(); - List signatures = IntStream.range(0, threshold).mapToObj(i -> TransactionSignature.dummy()).collect(Collectors.toList()); - return getScriptType().addMultisigSpendingInput(transaction, prevTxOut, threshold, pubKeys, signatures); + Map pubKeySignatures = new TreeMap<>(new ECKey.LexicographicECKeyComparator()); + for(int i = 0; i < pubKeys.size(); i++) { + pubKeySignatures.put(pubKeys.get(i), i < threshold ? TransactionSignature.dummy() : null); + } + return getScriptType().addMultisigSpendingInput(transaction, prevTxOut, threshold, pubKeySignatures); } else { throw new UnsupportedOperationException("Cannot create transaction for policy type " + getPolicyType()); } @@ -647,6 +650,9 @@ public class Wallet { } public void finalise(PSBT psbt) { + System.out.println("Before finalise: "); + System.out.println(Utils.bytesToHex(psbt.serialize())); + int threshold = getDefaultPolicy().getNumSignaturesRequired(); Map signingNodes = getSigningNodes(psbt); @@ -683,12 +689,18 @@ public class Wallet { finalizedTxInput = getScriptType().addSpendingInput(transaction, utxo, pubKey, transactionSignature); } else if(getPolicyType().equals(PolicyType.MULTI)) { List pubKeys = getPubKeys(signingNode); - List signatures = pubKeys.stream().map(psbtInput::getPartialSignature).filter(Objects::nonNull).collect(Collectors.toList()); + + Map pubKeySignatures = new TreeMap<>(new ECKey.LexicographicECKeyComparator()); + for(ECKey pubKey : pubKeys) { + pubKeySignatures.put(pubKey, psbtInput.getPartialSignature(pubKey)); + } + + List signatures = pubKeySignatures.values().stream().filter(Objects::nonNull).collect(Collectors.toList()); if(signatures.size() < threshold) { throw new IllegalArgumentException("Pubkeys of partial signatures do not match wallet pubkeys"); } - finalizedTxInput = getScriptType().addMultisigSpendingInput(transaction, utxo, threshold, pubKeys, signatures); + finalizedTxInput = getScriptType().addMultisigSpendingInput(transaction, utxo, threshold, pubKeySignatures); } else { throw new UnsupportedOperationException("Cannot finalise PSBT for policy type " + getPolicyType()); } @@ -698,6 +710,9 @@ public class Wallet { psbtInput.clearFinalised(); } } + + System.out.println("After finalise: "); + System.out.println(Utils.bytesToHex(psbt.serialize())); } public BitcoinUnit getAutoUnit() { diff --git a/src/test/java/com/sparrowwallet/drongo/protocol/TransactionTest.java b/src/test/java/com/sparrowwallet/drongo/protocol/TransactionTest.java index c1825a8..b7bd516 100644 --- a/src/test/java/com/sparrowwallet/drongo/protocol/TransactionTest.java +++ b/src/test/java/com/sparrowwallet/drongo/protocol/TransactionTest.java @@ -9,7 +9,8 @@ import org.junit.Test; import java.io.ByteArrayOutputStream; import java.io.IOException; -import java.util.List; +import java.util.Map; +import java.util.TreeMap; public class TransactionTest { @Test @@ -302,8 +303,13 @@ public class TransactionTest { ECKey key1 = redeemScript.getChunks().get(2).getPubKey(); ECKey key2 = redeemScript.getChunks().get(3).getPubKey(); + Map pubKeySignatures = new TreeMap<>(new ECKey.LexicographicECKeyComparator()); + pubKeySignatures.put(key0, signature0); + pubKeySignatures.put(key1, signature1); + pubKeySignatures.put(key2, null); + Transaction transaction = new Transaction(); - spent0ScriptType.addMultisigSpendingInput(transaction, spent0Output, 2, List.of(key0, key1, key2), List.of(signature0, signature1)); + spent0ScriptType.addMultisigSpendingInput(transaction, spent0Output, 2, pubKeySignatures); transaction.addOutput(833300, Address.fromString("31mKrRn3xQoGppLY5dU92Dbm4kN4ddkknE")); transaction.addOutput(1222480000, Address.fromString("1CL9kj1seXif6agPfeh6vpKkzc2Hxq1UpM")); @@ -376,9 +382,14 @@ public class TransactionTest { ECKey key1 = witnessScript.getChunks().get(2).getPubKey(); ECKey key2 = witnessScript.getChunks().get(3).getPubKey(); + Map pubKeySignatures = new TreeMap<>(new ECKey.LexicographicECKeyComparator()); + pubKeySignatures.put(key0, signature0); + pubKeySignatures.put(key1, signature1); + pubKeySignatures.put(key2, null); + Transaction transaction = new Transaction(); transaction.setSegwitVersion(1); - TransactionInput input = ScriptType.P2SH_P2WSH.addMultisigSpendingInput(transaction, spent0Output, 2, List.of(key0, key1, key2), List.of(signature0, signature1)); + TransactionInput input = ScriptType.P2SH_P2WSH.addMultisigSpendingInput(transaction, spent0Output, 2, pubKeySignatures); transaction.addOutput(59287429, Address.fromString("3PBjKH4FRuEKy4sD3NfL7tqfZTG5K42owu")); transaction.addOutput(212571, Address.fromString("3KRUgU4XGuErXkjBtFhksPzTGJ4AMwF4jB")); @@ -447,9 +458,14 @@ public class TransactionTest { ECKey key1 = witnessScript.getChunks().get(2).getPubKey(); ECKey key2 = witnessScript.getChunks().get(3).getPubKey(); + Map pubKeySignatures = new TreeMap<>(new ECKey.LexicographicECKeyComparator()); + pubKeySignatures.put(key0, signature0); + pubKeySignatures.put(key1, signature1); + pubKeySignatures.put(key2, null); + Transaction transaction = new Transaction(); transaction.setSegwitVersion(1); - spent0ScriptType.addMultisigSpendingInput(transaction, spent0Output, 2, List.of(key0, key1, key2), List.of(signature0, signature1)); + spent0ScriptType.addMultisigSpendingInput(transaction, spent0Output, 2, pubKeySignatures); transaction.addOutput(10900000, Address.fromString("3Dt17mpd8FDXBjP56rCD7a4Sx7wpL91uhn")); transaction.addOutput(332500000, Address.fromString("1K6igqzm36x8jxRTavPhgWXLVcVZVDTGc9"));