Merge pull request #16 from twostack/txbuilder-fees

stephanfeb · web-flow · commit be70d9bf434b · 2022-01-21T15:18:58.000+08:00
TransactionBuilder Signature generation refactor
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,3 +1,18 @@
+#Release 1.6.0
+### TransactionBuilder Signature generation refactor
+
+Transaction building suffered from a rather pernicious problem  
+wherein it becomes hard/complicated to calculate fees.  
+This stems from the fact that when you try to large number of  
+utxos, the consequent large number of inputs in the spending tx  
+leads to guesswork about the appropriate fee calculation.  
+
+This update refactors the process of Transaction Signing so that  
+the builder can directly generate the signed inputs and therefore  
+perform the work of fee calculation internally.  
+
+Please see the transaction/TransactionBuilderTest.java for an example use.  
+
 # Release 1.5.5
 Made constructors public so to allow outside-package subclassing
 
diff --git a/build.gradle b/build.gradle
@@ -6,7 +6,7 @@ plugins {
 }
 
 group 'org.twostack'
-version '1.5.5'
+version '1.6.0'
 
 repositories {
     mavenCentral()
diff --git a/src/main/java/org/twostack/bitcoin4j/transaction/TransactionBuilder.java b/src/main/java/org/twostack/bitcoin4j/transaction/TransactionBuilder.java
@@ -18,12 +18,16 @@
 
 import org.twostack.bitcoin4j.Address;
 import org.twostack.bitcoin4j.Utils;
+import org.twostack.bitcoin4j.exception.SigHashException;
+import org.twostack.bitcoin4j.exception.SignatureDecodeException;
 import org.twostack.bitcoin4j.exception.TransactionException;
 import org.twostack.bitcoin4j.script.Script;
 
 import javax.annotation.Nullable;
+import java.io.IOException;
 import java.math.BigInteger;
 import java.util.*;
+import java.util.stream.Stream;
 
 import static org.twostack.bitcoin4j.Utils.HEX;
 
@@ -63,6 +67,29 @@ public class TransactionBuilder {
 
     private long nLockTime = 0;
 
+    private HashMap<String, SignerDto> signerMap = new HashMap();
+
+
+    private class SignerDto{
+        private TransactionSigner signer;
+        private TransactionOutpoint outpoint;
+
+        private SignerDto(){}
+
+        SignerDto(TransactionSigner signer, TransactionOutpoint outpoint){
+            this.signer = signer;
+            this.outpoint = outpoint;
+        }
+
+        public TransactionSigner getSigner() {
+            return signer;
+        }
+
+        public TransactionOutpoint getOutpoint() {
+            return outpoint;
+        }
+    }
+
     /*
         utxoMap is expected to have :
 
@@ -99,6 +126,36 @@ public TransactionBuilder spendFromUtxoMap(Map<String, Object> utxoMap, @Nullabl
 
     }
 
+    public TransactionBuilder spendFromTransaction(TransactionSigner signer, Transaction txn, int outputIndex, long sequenceNumber, UnlockingScriptBuilder unlocker){
+
+        //save the transactionId. This is expensive operation which serialises the Tx.
+        String transactionId = txn.getTransactionId();
+
+        //construct the data to save to signerMap
+        TransactionOutput output = txn.getOutputs().get(outputIndex);
+
+        TransactionOutpoint outpoint = new TransactionOutpoint();
+        outpoint.setOutputIndex(outputIndex);
+        outpoint.setLockingScript(output.getScript());
+        outpoint.setSatoshis(output.getAmount());
+        outpoint.setTransactionId(transactionId);
+
+        this.signerMap.put(transactionId, new SignerDto(signer, outpoint));
+
+        //update the spending transactionInput
+        TransactionInput input = new TransactionInput(
+                Utils.reverseBytes(txn.getTransactionIdBytes()),
+                outputIndex,
+                sequenceNumber,
+                unlocker
+        );
+
+        spendingMap.put(transactionId, txn.getOutputs().get(outputIndex).getAmount());
+
+        inputs.add(input);
+        return this;
+    }
+
     public TransactionBuilder spendFromTransaction(Transaction txn, int outputIndex, long sequenceNumber, UnlockingScriptBuilder unlocker){
 
         TransactionInput input = new TransactionInput(
@@ -264,7 +321,8 @@ TransactionBuilder lockUntilBlockHeight(int blockHeight) {
     }
      */
 
-    public Transaction build(boolean performChecks) throws TransactionException {
+
+    public Transaction build(boolean performChecks) throws TransactionException, IOException, SigHashException, SignatureDecodeException {
         if (performChecks){
             runTransactionChecks();
         }
@@ -277,6 +335,30 @@ public Transaction build(boolean performChecks) throws TransactionException {
         //add transaction outputs
         tx.addOutputs(outputs);
 
+        //update inputs with signatures
+        String txId = tx.getTransactionId();
+        for (int index = 0; index < inputs.size() ; index++) {
+            TransactionInput currentInput = inputs.get(index);
+
+            Optional<Map.Entry<String, SignerDto>> result = signerMap.entrySet().stream().filter( (Map.Entry<String, SignerDto> entry) -> {
+                //drop everything from stream except what we're looking for
+                return !(entry.getValue().outpoint.getTransactionId() == HEX.encode(currentInput.getPrevTxnId()) &&
+                        entry.getValue().outpoint.getOutputIndex() == currentInput.getPrevTxnOutputIndex());
+            }).findFirst();
+
+            if (result.isPresent()) {
+
+                SignerDto dto = result.get().getValue();
+                TransactionOutput utxoToSpend = new TransactionOutput(dto.outpoint.getSatoshis(), dto.outpoint.getLockingScript());
+
+                //TODO: this side-effect programming where the signer mutates my local variable
+                //      still bothers me.
+                dto.signer.sign(tx, utxoToSpend, index);
+            }
+        }
+
+
+
         if (changeScriptBuilder != null) {
             tx.addOutput(getChangeOutput());
         }
diff --git a/src/main/java/org/twostack/bitcoin4j/transaction/TransactionSigner.java b/src/main/java/org/twostack/bitcoin4j/transaction/TransactionSigner.java
@@ -34,6 +34,8 @@ public class TransactionSigner {
     private Transaction signedTransaction;
     private int sigHashType;
     private TransactionSignature signature;
+    private PrivateKey signingKey;
+
 
     public byte[] getHash() {
         return hash;
@@ -55,6 +57,52 @@ public byte[] getPreImage() {
         return preImage;
     }
 
+    /** Constructs a new instance of the TransactionSigner.
+     * NOTE: The SigHashType for signing will default to a value of
+     *       (SigHashType.ALL.value | SigHashType.FORKID.value)
+     */
+//     TransactionSigner(){
+//        this.sigHashType = SigHashType.ALL.value | SigHashType.FORKID.value;
+//    }
+
+    /** Constructs a new instance of the TransactionSigner.
+     * NOTE: The SigHashType for signing will default to a value of
+     *       (SigHashType.ALL.value | SigHashType.FORKID.value)
+     *
+     * @param sigHashType - Flags that govern which SigHash algorithm to use during signature generation
+     */
+    TransactionSigner(int sigHashType, PrivateKey signingKey){
+        this.sigHashType = sigHashType;
+        this.signingKey = signingKey;
+    }
+
+    /** Signs the provided transaction, and populates the corresponding input's
+     *  LockingScriptBuilder with the signature. Responsibility for what to
+     *  do with the Signature (populate appropriate template) is left to the
+     *  LockingScriptBuilder instance.
+     *
+     *  NOTE: This invocation will use the SigHashType set as part of the
+     *  constructor invocation. If the default constructor is invoked then
+     *  the SigHashType defaults to (SigHashType.ALL.value | SigHashType.FORKID.value) :
+     *
+     *
+     * @param unsignedTxn  - Unsigned Transaction
+     * @param utxo - Funding transaction's Output to sign over
+     * @param inputIndex - Input of the current Transaction we are signing for
+     * @return Signed Transaction
+     * @throws TransactionException
+     * @throws IOException
+     * @throws SigHashException
+     * @throws SignatureDecodeException
+     */
+    public Transaction sign(
+            Transaction unsignedTxn,
+            TransactionOutput utxo,
+            int inputIndex) throws TransactionException, IOException, SigHashException, SignatureDecodeException {
+
+        return this.sign(unsignedTxn, utxo, inputIndex, signingKey, sigHashType);
+    }
+
     /** Signs the provided transaction, and populates the corresponding input's
      *  LockingScriptBuilder with the signature. Responsibility for what to
      *  do with the Signature (populate appropriate template) is left to the
@@ -120,6 +168,14 @@ public TransactionSignature signPreimage(PrivateKey signingKey, byte[] preImage,
         return new TransactionSignature(ecSig.r, ecSig.s, sigHashType);
     }
 
+    public PrivateKey getSigningKey() {
+        return signingKey;
+    }
+
+    public void setSigningKey (PrivateKey privateKey) {
+        this.signingKey = privateKey;
+    }
+
 
 /** sf:> This seems like more Core buggery to me. What sort of TX remains valid without proper SighashType ???
      *
diff --git a/src/test/java/org/twostack/bitcoin4j/transaction/LockUnlockBuilderTests.java b/src/test/java/org/twostack/bitcoin4j/transaction/LockUnlockBuilderTests.java
@@ -159,7 +159,7 @@ public void canCreateP2PKHScript(){
     }
 
     @Test
-    public void canUseP2PKInTransactionBuilder() throws InvalidKeyException, TransactionException {
+    public void canUseP2PKInTransactionBuilder() throws InvalidKeyException, TransactionException, SigHashException, SignatureDecodeException, IOException {
 
         //Create a Transaction instance from the RAW transaction data create by bitcoin-cli.
         //this transaction contains the UTXO we are interested in
@@ -181,7 +181,8 @@ public void canUseP2PKInTransactionBuilder() throws InvalidKeyException, Transac
 
         //simply check that we have clean e2e execution
         Assertions.assertThatCode(() -> {
-            new TransactionSigner().sign(unsignedTxn, utxoToSign,0, privateKey, SigHashType.ALL.value | SigHashType.FORKID.value);
+            TransactionSigner signer = new TransactionSigner( SigHashType.ALL.value | SigHashType.FORKID.value, privateKey);
+            signer.sign(unsignedTxn, utxoToSign,0);
         }).doesNotThrowAnyException();
     }
 
@@ -323,10 +324,11 @@ public void canSpendFromMultiSig() throws InvalidKeyException, TransactionExcept
             .withFeePerKb(512)
             .build(false);
 
-        TransactionSigner signer = new TransactionSigner();
+        TransactionSigner signer1 = new TransactionSigner(SigHashType.ALL.value | SigHashType.FORKID.value, private1 );
+        TransactionSigner signer2 = new TransactionSigner(SigHashType.ALL.value | SigHashType.FORKID.value, private2 );
         TransactionOutput utxo = new TransactionOutput(BigInteger.valueOf(100000), lockBuilder);
-        signer.sign(tx, utxo, 0, private1, SigHashType.ALL.value | SigHashType.FORKID.value);
-        signer.sign(tx, utxo, 0, private2, SigHashType.ALL.value | SigHashType.FORKID.value);
+        signer1.sign(tx, utxo, 0);
+        signer2.sign(tx, utxo, 0);
 
         assertEquals(2, unlockBuilder.getSignatures().size() );
 
diff --git a/src/test/java/org/twostack/bitcoin4j/transaction/TransactionBuilderTest.java b/src/test/java/org/twostack/bitcoin4j/transaction/TransactionBuilderTest.java
@@ -0,0 +1,70 @@
+package org.twostack.bitcoin4j.transaction;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.assertj.core.api.Assertions;
+import org.junit.Test;
+import org.twostack.bitcoin4j.Address;
+import org.twostack.bitcoin4j.PrivateKey;
+import org.twostack.bitcoin4j.Utils;
+import org.twostack.bitcoin4j.exception.InvalidKeyException;
+import org.twostack.bitcoin4j.exception.SigHashException;
+import org.twostack.bitcoin4j.exception.SignatureDecodeException;
+import org.twostack.bitcoin4j.exception.TransactionException;
+import org.twostack.bitcoin4j.params.NetworkAddressType;
+import org.twostack.bitcoin4j.script.Script;
+import org.twostack.bitcoin4j.script.ScriptError;
+
+import java.io.IOException;
+import java.io.InputStreamReader;
+import java.math.BigInteger;
+import java.nio.charset.StandardCharsets;
+import java.util.Set;
+
+import static org.twostack.bitcoin4j.utils.TestUtil.parseVerifyFlags;
+
+public class TransactionBuilderTest {
+
+    @Test
+    public void processAndSignMultiInput() throws IOException, InvalidKeyException, SignatureDecodeException, TransactionException, SigHashException {
+
+        //This WIF is for a private key that actually has testnet coins on TESTNET
+        //The transactions in multi_input.json are UTXOs that exist(ed) on TESTNET
+        // at time of writing this test, and can be viewed on TESTNET using a block explorer
+        String wif = "cRTUuWgPdp7tJPrn1Xeq196eZa4ZCpg8n3cgDJsJmgDHBZ8x9fpv";
+        PrivateKey privateKey = PrivateKey.fromWIF(wif);
+
+        JsonNode json = new ObjectMapper().readTree(
+                new InputStreamReader(getClass().getResourceAsStream("multi_input.json"),
+                        StandardCharsets.UTF_8)
+        );
+
+        //build one large transaction that spends all the inputs
+        TransactionBuilder builder = new TransactionBuilder();
+        for (JsonNode utxoInfo : json) {
+
+            Integer fundingOutputIndex = utxoInfo.get("tx_pos").asInt();
+            String rawTxHex = utxoInfo.get("raw_tx").asText();
+
+            Transaction fundingTx = Transaction.fromHex(rawTxHex);
+
+            UnlockingScriptBuilder unlocker = new P2PKHUnlockBuilder(privateKey.getPublicKey());
+
+            TransactionSigner signer = new TransactionSigner(SigHashType.ALL.value | SigHashType.FORKID.value, privateKey);
+            builder.spendFromTransaction(signer, fundingTx, fundingOutputIndex, TransactionInput.MAX_SEQ_NUMBER, unlocker);
+
+        }
+
+        Address recipientAddress = Address.fromKey(NetworkAddressType.TEST_PKH, privateKey.getPublicKey());
+
+
+        Assertions.assertThatCode(() -> {
+            Transaction broadcastTx = builder.withFeePerKb(512)
+                    .spendTo(new P2PKHLockBuilder(recipientAddress), BigInteger.valueOf(100000))
+                    .sendChangeTo(recipientAddress)
+                    .build(true);
+        }).doesNotThrowAnyException();
+
+    }
+
+}
diff --git a/src/test/java/org/twostack/bitcoin4j/transaction/TransactionTest.java b/src/test/java/org/twostack/bitcoin4j/transaction/TransactionTest.java
@@ -78,7 +78,7 @@ public void TestTransactionIdMatchesTransactionHash() {
     }
 
     @Test
-    public void canCreateAndSignTransactionWithoutChange() throws InvalidKeyException, TransactionException, IOException, SigHashException {
+    public void canCreateAndSignTransactionWithoutChange() throws InvalidKeyException, TransactionException, IOException, SigHashException, SignatureDecodeException {
 
         PrivateKey privateKey = PrivateKey.fromWIF("cVVvUsNHhbrgd7aW3gnuGo2qJM45LhHhTCVXrDSJDDcNGE6qmyCs");
         Address changeAddress = Address.fromString(NetworkType.TEST, "mu4DpTaD75nheE4z5CQazqm1ivej1vzL4L"); // my address
@@ -103,14 +103,15 @@ public void canCreateAndSignTransactionWithoutChange() throws InvalidKeyExceptio
 
         //simply check that we have clean e2e execution
         Assertions.assertThatCode(() -> {
-            new TransactionSigner().sign(unsignedTxn, utxoToSign,0, privateKey, SigHashType.ALL.value | SigHashType.FORKID.value);
+            TransactionSigner signer = new TransactionSigner(SigHashType.ALL.value | SigHashType.FORKID.value, privateKey);
+            signer.sign(unsignedTxn, utxoToSign,0);
         }).doesNotThrowAnyException();
 
         //System.out.println(HEX.encode(signedTx.serialize()));
     }
 
     @Test
-    public void can_create_and_sign_transaction() throws InvalidKeyException, TransactionException, IOException, SigHashException {
+    public void can_create_and_sign_transaction() throws InvalidKeyException, TransactionException, IOException, SigHashException, SignatureDecodeException {
 
         PrivateKey privateKey = PrivateKey.fromWIF("cVVvUsNHhbrgd7aW3gnuGo2qJM45LhHhTCVXrDSJDDcNGE6qmyCs");
         Address changeAddress = Address.fromString(NetworkType.TEST, "mu4DpTaD75nheE4z5CQazqm1ivej1vzL4L"); // my address
@@ -136,7 +137,8 @@ public void can_create_and_sign_transaction() throws InvalidKeyException, Transa
 
          //simply check that we have clean e2e execution
          Assertions.assertThatCode(() -> {
-             new TransactionSigner().sign(unsignedTxn, utxoToSign,0, privateKey, SigHashType.ALL.value | SigHashType.FORKID.value);
+             TransactionSigner signer = new TransactionSigner(SigHashType.ALL.value | SigHashType.FORKID.value, privateKey );
+             signer.sign(unsignedTxn, utxoToSign,0);
          }).doesNotThrowAnyException();
 
         //System.out.println(HEX.encode(signedTx.serialize()));
@@ -204,13 +206,11 @@ public void test_transaction_serialization_vectors() throws IOException, Transac
 
             Transaction tx = builder.build(false);
 
-            TransactionSigner signer = new TransactionSigner();
+            TransactionSigner signer = new TransactionSigner( sighashType, privateKey);
             signer.sign(
                     tx,
                     new TransactionOutput(BigInteger.valueOf(satoshis), scriptPubKey),
-                    0,
-                    privateKey,
-                    sighashType);
+                    0 );
 
             assertEquals(serializedTx, HEX.encode(tx.serialize()));
 
diff --git a/src/test/resources/org/twostack/bitcoin4j/transaction/multi_input.json b/src/test/resources/org/twostack/bitcoin4j/transaction/multi_input.json

Original file line number	Diff line number	Diff line change
`@@ -6,7 +6,7 @@ plugins {`
`6`	`6`	`}`
`7`	`7`
`8`	`8`	`group 'org.twostack'`
`9`		`-version '1.5.5'`
	`9`	`+version '1.6.0'`
`10`	`10`
`11`	`11`	`repositories {`
`12`	`12`	`mavenCentral()`