From 5f5ee3a166ea9ad3727f6b4df036cff995daa0f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kyle=20=F0=9F=90=86?= Date: Sat, 5 Sep 2026 20:28:44 -0400 Subject: [PATCH] Keep the signature that opts in when a quorum has more than it needs --- .../sparrowwallet/drongo/wallet/Wallet.java | 45 ++ .../drongo/wallet/QuorumFinaliseTest.java | 384 ++++++++++++++++++ 2 files changed, 429 insertions(+) create mode 100644 src/test/java/com/sparrowwallet/drongo/wallet/QuorumFinaliseTest.java diff --git a/src/main/java/com/sparrowwallet/drongo/wallet/Wallet.java b/src/main/java/com/sparrowwallet/drongo/wallet/Wallet.java index 9cd137cc..85484a1d 100644 --- a/src/main/java/com/sparrowwallet/drongo/wallet/Wallet.java +++ b/src/main/java/com/sparrowwallet/drongo/wallet/Wallet.java @@ -2021,6 +2021,49 @@ public List computeSilentPaymentOutputs(PSBT psbt, Map pubKeySignatures, int threshold, PSBTInput psbtInput) { + List present = pubKeySignatures.entrySet().stream().filter(entry -> entry.getValue() != null) + .map(Map.Entry::getKey).collect(Collectors.toList()); + if(present.size() <= threshold) { + return; + } + + //By the pair, not by the signature. A signature does not carry the key that made it and compares by hash type + //and by r and s, so asking whether it is among the verified ones answers yes for a second key that files a + //copy of it: both then took a slot, and the one whose key does not verify it does not spend. Where nothing can + //be checked this finds none preferred and falls back to key order, which is what it did before. + Map verified = psbtInput.getVerifiedPartialSignatures(pubKeySignatures.keySet()); + + List keep = new ArrayList<>(); + for(ECKey pubKey : present) { + TransactionSignature signature = pubKeySignatures.get(pubKey); + if(keep.size() < threshold && (signature.sighashFlags & SigHash.UNIFIED_FLAG) != 0 + && signature.equals(verified.get(pubKey))) { + keep.add(pubKey); + } + } + for(ECKey pubKey : present) { + if(keep.size() < threshold && !keep.contains(pubKey)) { + keep.add(pubKey); + } + } + + for(ECKey pubKey : present) { + if(!keep.contains(pubKey)) { + pubKeySignatures.put(pubKey, null); + } + } + } + public void finalise(PSBT psbt) { int threshold = getDefaultPolicy().getNumSignaturesRequired(); Map signingNodes = getSigningNodes(psbt); @@ -2076,6 +2119,7 @@ public int getIndex() { throw new IllegalArgumentException("Pubkeys of partial signatures do not match wallet pubkeys"); } + retainThreshold(pubKeySignatures, threshold, psbtInput); finalizedTxInput = signingWallet.getScriptType().addMultisigSpendingInput(signingWallet.getPolicyType(), transaction, utxo, threshold, pubKeySignatures); } else { throw new UnsupportedOperationException("Cannot finalise PSBT for policy type " + signingWallet.getPolicyType()); @@ -2125,6 +2169,7 @@ private void finaliseExternalInput(PSBTInput psbtInput, TransactionOutput utxo, return; } + retainThreshold(pubKeySignatures, inputThreshold, psbtInput); finalizedTxInput = inputScriptType.addMultisigSpendingInput(PolicyType.MULTI_HD, transaction, utxo, inputThreshold, pubKeySignatures); } else if(inputThreshold == 1) { ECKey pubKey; diff --git a/src/test/java/com/sparrowwallet/drongo/wallet/QuorumFinaliseTest.java b/src/test/java/com/sparrowwallet/drongo/wallet/QuorumFinaliseTest.java new file mode 100644 index 00000000..b14394a5 --- /dev/null +++ b/src/test/java/com/sparrowwallet/drongo/wallet/QuorumFinaliseTest.java @@ -0,0 +1,384 @@ +package com.sparrowwallet.drongo.wallet; + +import com.sparrowwallet.drongo.KeyPurpose; +import com.sparrowwallet.drongo.Utils; +import com.sparrowwallet.drongo.crypto.ECKey; +import com.sparrowwallet.drongo.policy.Policy; +import com.sparrowwallet.drongo.policy.PolicyType; +import com.sparrowwallet.drongo.protocol.Script; +import com.sparrowwallet.drongo.protocol.ScriptType; +import com.sparrowwallet.drongo.protocol.Sha256Hash; +import com.sparrowwallet.drongo.protocol.SigHash; +import com.sparrowwallet.drongo.protocol.Transaction; +import com.sparrowwallet.drongo.protocol.TransactionOutput; +import com.sparrowwallet.drongo.protocol.TransactionSignature; +import com.sparrowwallet.drongo.psbt.PSBT; +import com.sparrowwallet.drongo.psbt.PSBTInput; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import java.util.ArrayList; +import java.util.List; + +/** + * Which signatures a quorum keeps when more signers signed than it needs. + * + * A 2 of 3 whose marked device signs third has three signatures and room for two. They are chosen in key order, so + * the opted-in one can be the one dropped, and the transaction broadcasts carrying no replay protection at all, + * having had it. The label is right about the result and the wallet threw the protection away. + */ +public class QuorumFinaliseTest { + private static final String[] WORDS = { + "absent essay fox snake vast pumpkin height crouch silent bulb excuse razor", + "sell arrive brand fluid cousin twin trap bar hen fine bicycle rack", + "quantum lens tag pencil kingdom obey noise pigeon oyster shoulder ordinary tilt"}; + + private Wallet wallet(ScriptType scriptType) throws Exception { + Wallet wallet = new Wallet(); + wallet.setPolicyType(PolicyType.MULTI_HD); + wallet.setScriptType(scriptType); + for(String words : WORDS) { + wallet.getKeystores().add(Keystore.fromSeed( + new DeterministicSeed(words, "", 0, DeterministicSeed.Type.BIP39), + PolicyType.MULTI_HD, scriptType.getDefaultDerivation())); + } + wallet.setDefaultPolicy(Policy.getPolicy(PolicyType.MULTI_HD, scriptType, wallet.getKeystores(), 2)); + wallet.getNode(KeyPurpose.RECEIVE); + return wallet; + } + + /** + * The marked signer signs last in key order, which is the case that loses. Every signer signs, so there is one + * more signature than the threshold needs and one has to go. + */ + @Test + public void a_quorum_with_a_spare_signature_keeps_the_one_that_opts_in() throws Exception { + //Every script type a quorum can be spent by. Nulling an entry the map used to have a signature for is new + //state for all of their scriptSig and witness builders, not only the one this was found on. + for(ScriptType scriptType : new ScriptType[] {ScriptType.P2WSH, ScriptType.P2SH, ScriptType.P2SH_P2WSH}) { + //Bare multisig has no wallet script type of its own; its assembly path is reached through the others + assertKeepsTheOptedInSignature(scriptType); + } + } + + private void assertKeepsTheOptedInSignature(ScriptType scriptType) throws Exception { + byte unifiedAll = (byte)(SigHash.UNIFIED_FLAG | SigHash.ALL.byteValue()); + Wallet wallet = wallet(scriptType); + WalletNode node = wallet.getNode(KeyPurpose.RECEIVE).getChildren().iterator().next(); + Script spk = wallet.getOutputScript(node); + + //Whichever keystore's key sorts last is the one a key ordered choice drops + List ordered = new ArrayList<>(node.getPubKeys()); + ordered.sort(new ECKey.LexicographicECKeyComparator()); + ECKey last = ordered.get(ordered.size() - 1); + int lastKeystore = -1; + for(int i = 0; i < wallet.getKeystores().size(); i++) { + if(wallet.getKeystores().get(i).getPubKey(node).equals(last)) { + lastKeystore = i; + } + } + Assertions.assertTrue(lastKeystore >= 0, "the fixture must find the keystore that sorts last"); + + Transaction transaction = new Transaction(); + transaction.setVersion(2); + transaction.addInput(Sha256Hash.ZERO_HASH, 0, new Script(new byte[0])); + transaction.addOutput(90_000L, spk); + + PSBT psbt = new PSBT(transaction); + PSBTInput psbtInput = psbt.getPsbtInputs().get(0); + psbtInput.setWitnessUtxo(new TransactionOutput(null, 100_000L, spk.getProgram())); + Script multisigScript = ScriptType.MULTISIG.getOutputScript(2, node.getPubKeys()); + if(scriptType == ScriptType.P2SH) { + psbtInput.setRedeemScript(multisigScript); + } else if(scriptType == ScriptType.P2SH_P2WSH) { + psbtInput.setRedeemScript(ScriptType.P2WSH.getOutputScript(multisigScript)); + psbtInput.setWitnessScript(multisigScript); + } else { + psbtInput.setWitnessScript(multisigScript); + } + + //Everyone signs. Only the one that sorts last opts in, which is the marked device in the case this is about. + for(int i = 0; i < wallet.getKeystores().size(); i++) { + psbtInput.setSigHash(i == lastKeystore ? SigHash.fromByte(unifiedAll) : SigHash.ALL); + Assertions.assertTrue(psbtInput.sign(wallet.getKeystores().get(i).getKey(node)), + "every keystore must sign"); + } + Assertions.assertEquals(3, psbtInput.getPartialSignatures().size(), "all three must have signed"); + + wallet.finalise(psbt); + + PSBTInput finalised = psbt.getPsbtInputs().get(0); + List kept = new ArrayList<>(finalised.getFinalScriptWitness() != null + ? finalised.getFinalScriptWitness().getSignatures() + : finalised.getFinalScriptSig().getSignatures()); + Assertions.assertEquals(2, kept.size(), scriptType + " keeps two for a 2 of 3"); + Assertions.assertTrue(kept.stream().anyMatch(signature -> (signature.sighashFlags & SigHash.UNIFIED_FLAG) != 0), + scriptType + " threw the opted-in signature away and broadcasts unprotected"); + } + + /** + * The choice at its edges. Whichever way it goes it has to keep exactly the threshold, and it must not depend on + * any of them opting in: a quorum where none does is the ordinary case and still has to spend. + */ + @Test + public void it_keeps_exactly_the_threshold_however_many_opt_in() throws Exception { + Assertions.assertEquals(0, keptOptedIn(0), "none opted in, so none can be kept"); + Assertions.assertEquals(1, keptOptedIn(1), "the one that opts in is the one worth keeping"); + Assertions.assertEquals(2, keptOptedIn(2), "both of them fit"); + Assertions.assertEquals(2, keptOptedIn(3), "more opt in than there is room for, so it keeps the threshold"); + } + + /** Signs a 2 of 3 with every keystore, the first optingIn of them opting in, and returns how many kept opt in. */ + private int keptOptedIn(int optingIn) throws Exception { + byte unifiedAll = (byte)(SigHash.UNIFIED_FLAG | SigHash.ALL.byteValue()); + Wallet wallet = wallet(ScriptType.P2WSH); + WalletNode node = wallet.getNode(KeyPurpose.RECEIVE).getChildren().iterator().next(); + Script spk = wallet.getOutputScript(node); + + Transaction transaction = new Transaction(); + transaction.setVersion(2); + transaction.addInput(Sha256Hash.ZERO_HASH, 0, new Script(new byte[0])); + transaction.addOutput(90_000L, spk); + + PSBT psbt = new PSBT(transaction); + PSBTInput psbtInput = psbt.getPsbtInputs().get(0); + psbtInput.setWitnessUtxo(new TransactionOutput(null, 100_000L, spk.getProgram())); + psbtInput.setWitnessScript(ScriptType.MULTISIG.getOutputScript(2, node.getPubKeys())); + + for(int i = 0; i < wallet.getKeystores().size(); i++) { + psbtInput.setSigHash(i < optingIn ? SigHash.fromByte(unifiedAll) : SigHash.ALL); + Assertions.assertTrue(psbtInput.sign(wallet.getKeystores().get(i).getKey(node)), "every keystore must sign"); + } + + wallet.finalise(psbt); + + List kept = new ArrayList<>( + psbt.getPsbtInputs().get(0).getFinalScriptWitness().getSignatures()); + Assertions.assertEquals(2, kept.size(), "a 2 of 3 keeps two whatever they carry"); + return (int)kept.stream().filter(signature -> (signature.sighashFlags & SigHash.UNIFIED_FLAG) != 0).count(); + } + + /** Exactly the threshold signed, so there is no choice to make and nothing may be dropped. */ + @Test + public void a_quorum_with_no_spare_signature_is_left_alone() throws Exception { + Wallet wallet = wallet(ScriptType.P2WSH); + WalletNode node = wallet.getNode(KeyPurpose.RECEIVE).getChildren().iterator().next(); + Script spk = wallet.getOutputScript(node); + + Transaction transaction = new Transaction(); + transaction.setVersion(2); + transaction.addInput(Sha256Hash.ZERO_HASH, 0, new Script(new byte[0])); + transaction.addOutput(90_000L, spk); + + PSBT psbt = new PSBT(transaction); + PSBTInput psbtInput = psbt.getPsbtInputs().get(0); + psbtInput.setWitnessUtxo(new TransactionOutput(null, 100_000L, spk.getProgram())); + psbtInput.setWitnessScript(ScriptType.MULTISIG.getOutputScript(2, node.getPubKeys())); + psbtInput.setSigHash(SigHash.ALL); + + //The two whose keys sort last, so a choice that preferred the earlier ones would be visible + List ordered = new ArrayList<>(node.getPubKeys()); + ordered.sort(new ECKey.LexicographicECKeyComparator()); + for(Keystore keystore : wallet.getKeystores()) { + if(!keystore.getPubKey(node).equals(ordered.get(0))) { + Assertions.assertTrue(psbtInput.sign(keystore.getKey(node)), "the last two must sign"); + } + } + Assertions.assertEquals(2, psbtInput.getPartialSignatures().size(), "exactly the threshold must have signed"); + + wallet.finalise(psbt); + Assertions.assertEquals(2, + psbt.getPsbtInputs().get(0).getFinalScriptWitness().getSignatures().size(), + "nothing was spare, so nothing may have been dropped"); + } + + /** + * The kept signatures stay in the order of the keys, which is what CHECKMULTISIG requires and the whole reason + * this may only choose which to keep and never reorder. Proven to spend for every script type against a node in + * spare_quorum_finalise.py; asserted here so a regression is caught without one. + */ + @Test + public void what_it_keeps_stays_in_the_order_of_the_keys() throws Exception { + byte unifiedAll = (byte)(SigHash.UNIFIED_FLAG | SigHash.ALL.byteValue()); + Wallet wallet = wallet(ScriptType.P2WSH); + WalletNode node = wallet.getNode(KeyPurpose.RECEIVE).getChildren().iterator().next(); + Script spk = wallet.getOutputScript(node); + + Transaction transaction = new Transaction(); + transaction.setVersion(2); + transaction.addInput(Sha256Hash.ZERO_HASH, 0, new Script(new byte[0])); + transaction.addOutput(90_000L, spk); + + PSBT psbt = new PSBT(transaction); + PSBTInput psbtInput = psbt.getPsbtInputs().get(0); + psbtInput.setWitnessUtxo(new TransactionOutput(null, 100_000L, spk.getProgram())); + psbtInput.setWitnessScript(ScriptType.MULTISIG.getOutputScript(2, node.getPubKeys())); + + //Only the middle key opts in, so the choice has to reach past the first and stop before the last + List ordered = new ArrayList<>(node.getPubKeys()); + ordered.sort(new ECKey.LexicographicECKeyComparator()); + ECKey middle = ordered.get(1); + for(Keystore keystore : wallet.getKeystores()) { + psbtInput.setSigHash(keystore.getPubKey(node).equals(middle) ? SigHash.fromByte(unifiedAll) : SigHash.ALL); + Assertions.assertTrue(psbtInput.sign(keystore.getKey(node)), "every keystore must sign"); + } + + //What the signatures would be in key order, which the kept ones have to be a run of + List inKeyOrder = new ArrayList<>(); + for(ECKey pubKey : ordered) { + inKeyOrder.add(psbtInput.getPartialSignature(pubKey)); + } + + wallet.finalise(psbt); + List kept = new ArrayList<>( + psbt.getPsbtInputs().get(0).getFinalScriptWitness().getSignatures()); + + int at = 0; + for(TransactionSignature signature : kept) { + while(at < inKeyOrder.size() && !inKeyOrder.get(at).equals(signature)) { + at++; + } + Assertions.assertTrue(at < inKeyOrder.size(), + "the kept signatures are not in the order of the keys, so this will not spend"); + at++; + } + Assertions.assertTrue(kept.stream().anyMatch(signature -> (signature.sighashFlags & SigHash.UNIFIED_FLAG) != 0), + "and the one that opts in is still among them"); + } + + /** + * A signature that claims the opt-in and verifies nothing must not take a slot from one that verifies. + * + * The bit and the key naming it are both written by whoever wrote the PSBT. Preferring on the bit alone made a + * signature that verifies nothing win a place deterministically, where key order gave it one only by luck, and + * the transaction that comes out of that does not spend at all. + */ + @Test + public void a_signature_claiming_the_opt_in_that_verifies_nothing_is_not_preferred() throws Exception { + byte unifiedAll = (byte)(SigHash.UNIFIED_FLAG | SigHash.ALL.byteValue()); + Wallet wallet = wallet(ScriptType.P2WSH); + WalletNode node = wallet.getNode(KeyPurpose.RECEIVE).getChildren().iterator().next(); + Script spk = wallet.getOutputScript(node); + + Transaction transaction = new Transaction(); + transaction.setVersion(2); + transaction.addInput(Sha256Hash.ZERO_HASH, 0, new Script(new byte[0])); + transaction.addOutput(90_000L, spk); + + PSBT psbt = new PSBT(transaction); + PSBTInput psbtInput = psbt.getPsbtInputs().get(0); + psbtInput.setWitnessUtxo(new TransactionOutput(null, 100_000L, spk.getProgram())); + psbtInput.setWitnessScript(ScriptType.MULTISIG.getOutputScript(2, node.getPubKeys())); + psbtInput.setSigHash(SigHash.ALL); + + for(Keystore keystore : wallet.getKeystores()) { + Assertions.assertTrue(psbtInput.sign(keystore.getKey(node)), "every keystore must sign"); + } + + //The one that sorts last, replaced with a mangled copy that claims the opt-in and verifies nothing + List ordered = new ArrayList<>(node.getPubKeys()); + ordered.sort(new ECKey.LexicographicECKeyComparator()); + ECKey last = ordered.get(ordered.size() - 1); + TransactionSignature real = psbtInput.getPartialSignature(last); + byte[] mangled = real.encodeToBitcoin(); + mangled[10] ^= 0x01; + mangled[mangled.length - 1] = unifiedAll; + psbtInput.getPartialSignatures().put(last, + TransactionSignature.decodeFromBitcoin(TransactionSignature.Type.ECDSA, mangled, false)); + + wallet.finalise(psbt); + List kept = new ArrayList<>( + psbt.getPsbtInputs().get(0).getFinalScriptWitness().getSignatures()); + + Assertions.assertEquals(2, kept.size(), "a 2 of 3 keeps two"); + Assertions.assertTrue(kept.stream().noneMatch(signature -> (signature.sighashFlags & SigHash.UNIFIED_FLAG) != 0), + "the only signature claiming the opt-in verifies nothing, so it may not have taken a slot"); + } + + /** + * With nothing to prefer this has to leave the old answer exactly as it was, since every multisig wallet that has + * never heard of the opt-in goes through it. The two kept are the two whose keys sort first, which is what taking + * them in key order and stopping at the threshold did. + */ + @Test + public void a_quorum_where_nothing_opts_in_keeps_what_key_order_kept() throws Exception { + Wallet wallet = wallet(ScriptType.P2WSH); + WalletNode node = wallet.getNode(KeyPurpose.RECEIVE).getChildren().iterator().next(); + Script spk = wallet.getOutputScript(node); + + Transaction transaction = new Transaction(); + transaction.setVersion(2); + transaction.addInput(Sha256Hash.ZERO_HASH, 0, new Script(new byte[0])); + transaction.addOutput(90_000L, spk); + + PSBT psbt = new PSBT(transaction); + PSBTInput psbtInput = psbt.getPsbtInputs().get(0); + psbtInput.setWitnessUtxo(new TransactionOutput(null, 100_000L, spk.getProgram())); + psbtInput.setWitnessScript(ScriptType.MULTISIG.getOutputScript(2, node.getPubKeys())); + psbtInput.setSigHash(SigHash.ALL); + + for(Keystore keystore : wallet.getKeystores()) { + Assertions.assertTrue(psbtInput.sign(keystore.getKey(node)), "every keystore must sign"); + } + + List ordered = new ArrayList<>(node.getPubKeys()); + ordered.sort(new ECKey.LexicographicECKeyComparator()); + List expected = List.of( + psbtInput.getPartialSignature(ordered.get(0)), psbtInput.getPartialSignature(ordered.get(1))); + + wallet.finalise(psbt); + + Assertions.assertEquals(expected, new ArrayList<>( + psbt.getPsbtInputs().get(0).getFinalScriptWitness().getSignatures()), + "nothing opts in, so this has to keep exactly what key order kept"); + } + + /** + * The same signature filed under two keys must not take two slots. + * + * Preference is decided by asking whether a signature is among the verified ones, and TransactionSignature + * compares by hash type and r and s. So a PSBT that files a byte identical copy of a verified opted-in signature + * under a second quorum key answers yes for both, and the witness then carries it in a slot whose key does not + * verify it, which does not spend. Key order kept the two lowest keys and produced a valid witness, so preferring + * on a value match turned a failure that needed luck into one that happens every time. + */ + @Test + public void one_signature_filed_under_two_keys_does_not_take_two_slots() throws Exception { + byte unifiedAll = (byte)(SigHash.UNIFIED_FLAG | SigHash.ALL.byteValue()); + Wallet wallet = wallet(ScriptType.P2WSH); + WalletNode node = wallet.getNode(KeyPurpose.RECEIVE).getChildren().iterator().next(); + Script spk = wallet.getOutputScript(node); + + Transaction transaction = new Transaction(); + transaction.setVersion(2); + transaction.addInput(Sha256Hash.ZERO_HASH, 0, new Script(new byte[0])); + transaction.addOutput(90_000L, spk); + + PSBT psbt = new PSBT(transaction); + PSBTInput psbtInput = psbt.getPsbtInputs().get(0); + psbtInput.setWitnessUtxo(new TransactionOutput(null, 100_000L, spk.getProgram())); + psbtInput.setWitnessScript(ScriptType.MULTISIG.getOutputScript(2, node.getPubKeys())); + psbtInput.setSigHash(SigHash.fromByte(unifiedAll)); + + for(Keystore keystore : wallet.getKeystores()) { + Assertions.assertTrue(psbtInput.sign(keystore.getKey(node)), "every keystore must sign"); + } + + //The middle key's signature, copied over the first key's, so the copy is met first in key order and can take + //a slot from a signature that verifies. It verifies under the middle key and under nothing else. + List ordered = new ArrayList<>(node.getPubKeys()); + ordered.sort(new ECKey.LexicographicECKeyComparator()); + ECKey first = ordered.get(0); + ECKey middle = ordered.get(1); + psbtInput.getPartialSignatures().put(first, psbtInput.getPartialSignature(middle)); + + wallet.finalise(psbt); + + //Whatever it kept, each kept signature has to verify under the key holding its slot, or this will not spend + PSBTInput finalised = psbt.getPsbtInputs().get(0); + List kept = new ArrayList<>(finalised.getFinalScriptWitness().getSignatures()); + Assertions.assertEquals(2, kept.size(), "a 2 of 3 keeps two"); + Assertions.assertEquals(2, kept.stream().distinct().count(), + "the same signature was kept twice, in a slot whose key does not verify it"); + } +}