Repository: bitcoinjs/bitcoinjs-lib
Pull request #696 (2016-11-09 02:01:56)

TransactionBuilder: add fee safety
**edit:** For future reference,  the merged constant is a maximum, minimum fee-rate, of 1000 satoshis per byte.

dcousens: ping @afk11 for ACK/NACK
afk11: I guess my questions are: 
 - do we always want to check the fee for transactions? Can the check be disabled?
The output value is optional even with segwit (it's not always required, so can safely allow the user not to provide it in those cases), but we can make it required going forward since we're introducing a break?

 - It doesn't seem to check the transaction size, which could more than warrant such a large fee. Could maybe use ABSURD_FEERATE instead?
dcousens: > Can the check be disabled?

In the cases where you wouldn't want it (incomplete transactions),  it is avoided by using `buildIncomplete`.

> Should we require the value going forward?

No? Its optional obviously.

> It doesn't seem to check the transaction size, which could explain such a large fee. Could maybe use ABSURD_FEERATE instead? 

Sure,  we have the signatures at this point too so should be straight forward.
dcousens: - [ ] Change to ABSURD_FEERATE instead of ABSURD_FEE
- [ ] Expose as constant on `TransactionBuilder`
dcousens: The assumptions this makes is:

* Inputs values are `undefined` or `> 0`
* Output values are all `> 0`
* `fee = incoming - outgoing`
* If we have **any** input values,  and their sum is larger than the outgoing,  we can determine the **minimum** fee for the transaction (not the actual fee rate, the *minimum*).

At that point,  we can say,  if the minimum is absurd,  it doesn't matter what the rest of the inputs are, because they only make it worse.

Naturally,  in most cases,  the [detected] minimum fee for the transaction is negative,  because `.value` is `undefined` (and treated as `0`).
But, that is fine,  because this isn't to be relied on,  its simply an aid to prevent [potential] algorithmic mistakes.
dcousens: I think an absurd fee rate of >300 satoshis per byte is a good limit for now.

dcousens: The only remaining part of the TODO for this PR is:

> Add canModifyOutputs check which would stop this from being correct

Personally,  I can't decide if this is a good or a bad thing... so I'm going to leave it to a patch PR later rather than do it.
dcousens: Merging once Travis passes.
dcousens: @afk11 this works a treat :+1: 
dcousens: ping @afk11 on 6a019ee
afk11: ACK 6a019ee

Patch:
diff --git a/src/transaction_builder.js b/src/transaction_builder.js
index 6f5b47aa..459bb5b7 100644
--- a/src/transaction_builder.js
+++ b/src/transaction_builder.js
@@ -228,10 +228,13 @@ function buildInput (input, allowIncomplete) {
   return scriptSig
 }
 
-function TransactionBuilder (network) {
+function TransactionBuilder (network, maximumFeeRate) {
   this.prevTxMap = {}
   this.network = network || networks.bitcoin
 
+  // WARNING: This is __NOT__ to be relied on, its just another potential safety mechanism (safety in-depth)
+  this.maximumFeeRate = maximumFeeRate || 1000
+
   this.inputs = []
   this.tx = new Transaction()
 }
@@ -272,7 +275,10 @@ TransactionBuilder.fromTransaction = function (transaction, network) {
 
   // Copy inputs
   transaction.ins.forEach(function (txIn) {
-    txb.__addInputUnsafe(txIn.hash, txIn.index, txIn.sequence, txIn.script)
+    txb.__addInputUnsafe(txIn.hash, txIn.index, {
+      sequence: txIn.sequence,
+      script: txIn.script
+    })
   })
 
   // fix some things not possible through the public API
@@ -288,6 +294,8 @@ TransactionBuilder.prototype.addInput = function (txHash, vout, sequence, prevOu
     throw new Error('No, this would invalidate signatures')
   }
 
+  var value
+
   // is it a hex string?
   if (typeof txHash === 'string') {
     // transaction hashs's are displayed in reverse order, un-reverse it
@@ -295,14 +303,21 @@ TransactionBuilder.prototype.addInput = function (txHash, vout, sequence, prevOu
 
   // is it a Transaction object?
   } else if (txHash instanceof Transaction) {
-    prevOutScript = txHash.outs[vout].script
+    var txOut = txHash.outs[vout]
+    prevOutScript = txOut.script
+    value = txOut.value
+
     txHash = txHash.getHash()
   }
 
-  return this.__addInputUnsafe(txHash, vout, sequence, null, prevOutScript)
+  return this.__addInputUnsafe(txHash, vout, {
+    sequence: sequence,
+    prevOutScript: prevOutScript,
+    value: value
+  })
 }
 
-TransactionBuilder.prototype.__addInputUnsafe = function (txHash, vout, sequence, scriptSig, prevOutScript) {
+TransactionBuilder.prototype.__addInputUnsafe = function (txHash, vout, options) {
   if (Transaction.isCoinbaseHash(txHash)) {
     throw new Error('coinbase inputs not supported')
   }
@@ -313,16 +328,21 @@ TransactionBuilder.prototype.__addInputUnsafe = function (txHash, vout, sequence
   var input = {}
 
   // derive what we can from the scriptSig
-  if (scriptSig) {
-    input = expandInput(scriptSig)
+  if (options.script !== undefined) {
+    input = expandInput(options.script)
+  }
+
+  // if an input value was given, retain it
+  if (options.value !== undefined) {
+    input.value = options.value
   }
 
   // derive what we can from the previous transactions output script
-  if (!input.prevOutScript && prevOutScript) {
+  if (!input.prevOutScript && options.prevOutScript) {
     var prevOutType
 
     if (!input.pubKeys && !input.signatures) {
-      var expanded = expandOutput(prevOutScript)
+      var expanded = expandOutput(options.prevOutScript)
 
       if (expanded.pubKeys) {
         input.pubKeys = expanded.pubKeys
@@ -332,11 +352,11 @@ TransactionBuilder.prototype.__addInputUnsafe = function (txHash, vout, sequence
       prevOutType = expanded.scriptType
     }
 
-    input.prevOutScript = prevOutScript
-    input.prevOutType = prevOutType || bscript.classifyOutput(prevOutScript)
+    input.prevOutScript = options.prevOutScript
+    input.prevOutType = prevOutType || bscript.classifyOutput(options.prevOutScript)
   }
 
-  var vin = this.tx.addInput(txHash, vout, sequence, scriptSig)
+  var vin = this.tx.addInput(txHash, vout, options.sequence, options.scriptSig)
   this.inputs[vin] = input
   this.prevTxMap[prevTxOut] = true
 
@@ -388,6 +408,13 @@ TransactionBuilder.prototype.__build = function (allowIncomplete) {
     tx.setInputScript(i, scriptSig)
   })
 
+  if (!allowIncomplete) {
+    // do not rely on this, its merely a last resort
+    if (this.__overMaximumFees(tx.byteLength())) {
+      throw new Error('Transaction has absurd fees')
+    }
+  }
+
   return tx
 }
 
@@ -479,4 +506,17 @@ TransactionBuilder.prototype.__canModifyOutputs = function () {
   })
 }
 
+TransactionBuilder.prototype.__overMaximumFees = function (bytes) {
+  // not all inputs will have .value defined
+  var incoming = this.inputs.reduce(function (a, x) { return a + (x.value >>> 0) }, 0)
+
+  // but all outputs do, and if we have any input value
+  // we can immediately determine if the outputs are too small
+  var outgoing = this.tx.outs.reduce(function (a, x) { return a + x.value }, 0)
+  var fee = incoming - outgoing
+  var feeRate = fee / bytes
+
+  return feeRate > this.maximumFeeRate
+}
+
 module.exports = TransactionBuilder
diff --git a/src/types.js b/src/types.js
index 29e53c50..c1e5fb16 100644
--- a/src/types.js
+++ b/src/types.js
@@ -10,7 +10,7 @@ function BIP32Path (value) {
 }
 BIP32Path.toJSON = function () { return 'BIP32 derivation path' }
 
-var SATOSHI_MAX = 2.1 * 1e15
+var SATOSHI_MAX = 21 * 1e14
 function Satoshi (value) {
   return typeforce.UInt53(value) && value <= SATOSHI_MAX
 }


Test patch:
diff --git a/test/fixtures/transaction_builder.json b/test/fixtures/transaction_builder.json
index 25ba79f7..dd0e49b7 100644
--- a/test/fixtures/transaction_builder.json
+++ b/test/fixtures/transaction_builder.json
@@ -317,7 +317,7 @@
       {
         "description": "Transaction w/ 1 pubKeyHash transaction input (Issue #644)",
         "network": "testnet",
-        "txHex": "010000000132595835c74fccf097db4ccae9dc2de621e58e0d3f697a27b469b61c7a223b39000000006b483045022100d771395776280955561190c09a6bca731684d09db8995c53496b816b8222019302202a21c9a90d0b5de188800673ad31861183c3f4cb15ea0988b485686aed9fce1d012103f29374a4c2c218a4077db9ba0b9d674cde3719560460af4eb3190d512dd5de92ffffffff0170170000000000001976a914ff99e06c1a4ac394b4e1cb3d3a4b2b47749e339a88ac00000000",
+        "txHex": "010000000132595835c74fccf097db4ccae9dc2de621e58e0d3f697a27b469b61c7a223b39000000006a47304402202fd41f18f1d7915bc811610236e1d708f9cd3515734abd5db7ac607f030728af02206bee7d163f04c470ce55561523824eb8b3abce80c214aabf7dfb78a073ea4a70012103f29374a4c2c218a4077db9ba0b9d674cde3719560460af4eb3190d512dd5de92ffffffff01808d5b00000000001976a914ff99e06c1a4ac394b4e1cb3d3a4b2b47749e339a88ac00000000",
         "inputs": [
           {
             "txHex": "0100000001f7e6430096cd2790bac115aaab22c0a50fb0a1794305302e1a399e81d8d354f4020000006a47304402205793a862d193264afc32713e2e14541e1ff9ebb647dd7e7e6a0051d0faa87de302205216653741ecbbed573ea2fc053209dd6980616701c27be5b958a159fc97f45a012103e877e7deb32d19250dcfe534ea82c99ad739800295cd5429a7f69e2896c36fcdfeffffff0340420f00000000001976a9145c7b8d623fba952d2387703d051d8e931a6aa0a188ac8bda2702000000001976a9145a0ef60784137d03e7868d063b05424f2f43799f88ac40420f00000000001976a9145c7b8d623fba952d2387703d051d8e931a6aa0a188ac2fcc0e00",
@@ -332,7 +332,7 @@
         "outputs": [
           {
             "script": "OP_DUP OP_HASH160 ff99e06c1a4ac394b4e1cb3d3a4b2b47749e339a OP_EQUALVERIFY OP_CHECKSIG",
-            "value": 6000
+            "value": 6000000
           }
         ]
       }
@@ -657,20 +657,47 @@
         "inputs": [
           {
             "txId": "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff",
-            "vout": 0,
-            "signs": []
+            "vout": 0
           }
         ],
         "outputs": []
       },
+      {
+        "exception": "Transaction has absurd fees",
+        "inputs": [
+          {
+            "txRaw": {
+              "inputs": [],
+              "outputs": [
+                {
+                  "address": "1C5XhB1UkFuyCV1CG9dmXaXGu3xDL4nAjv",
+                  "value": 1000000000
+                }
+              ],
+              "incomplete": true
+            },
+            "vout": 0,
+            "signs": [
+              {
+                "keyPair": "KzBQVXYUGDAvqG7VeU3C7ZMRYiwtsxSVVFcYGzKU9E4aUVDUquZU"
+              }
+            ]
+          }
+        ],
+        "outputs": [
+          {
+            "script": "OP_DUP OP_HASH160 ff99e06c1a4ac394b4e1cb3d3a4b2b47749e339a OP_EQUALVERIFY OP_CHECKSIG",
+            "value": 200000
+          }
+        ]
+      },
       {
         "description": "Incomplete transaction, nothing assumed",
         "exception": "Transaction is not complete",
         "inputs": [
           {
             "txId": "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff",
-            "vout": 0,
-            "signs": []
+            "vout": 0
           }
         ],
         "outputs": [
@@ -697,8 +724,7 @@
           {
             "txId": "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff",
             "vout": 1,
-            "prevTxScript": "OP_DUP OP_HASH160 aa4d7985c57e011a8b3dd8e0e5a73aaef41629c5 OP_EQUALVERIFY OP_CHECKSIG",
-            "signs": []
+            "prevTxScript": "OP_DUP OP_HASH160 aa4d7985c57e011a8b3dd8e0e5a73aaef41629c5 OP_EQUALVERIFY OP_CHECKSIG"
           }
         ],
         "outputs": [
@@ -715,13 +741,11 @@
         "inputs": [
           {
             "txId": "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff",
-            "vout": 0,
-            "signs": []
+            "vout": 0
           },
           {
             "txId": "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff",
-            "vout": 0,
-            "signs": []
+            "vout": 0
           }
         ],
         "outputs": [
diff --git a/test/transaction_builder.js b/test/transaction_builder.js
index 97c52ca3..25641f06 100644
--- a/test/transaction_builder.js
+++ b/test/transaction_builder.js
@@ -21,22 +21,37 @@ function construct (f, sign) {
   if (f.locktime !== undefined) txb.setLockTime(f.locktime)
 
   f.inputs.forEach(function (input) {
-    var prevTxScript
+    var prevTx
+    if (input.txRaw) {
+      var constructed = construct(input.txRaw)
+      if (input.txRaw.incomplete) prevTx = constructed.buildIncomplete()
+      else prevTx = constructed.build()
+    } else if (input.txHex) {
+      prevTx = Transaction.fromHex(input.txHex)
+    } else {
+      prevTx = input.txId
+    }
 
-    if (!input.txHex && input.prevTxScript) {
+    var prevTxScript
+    if (input.prevTxScript) {
       prevTxScript = bscript.fromASM(input.prevTxScript)
     }
 
-    txb.addInput(input.txId || Transaction.fromHex(input.txHex), input.vout, input.sequence, prevTxScript)
+    txb.addInput(prevTx, input.vout, input.sequence, prevTxScript)
   })
 
   f.outputs.forEach(function (output) {
-    txb.addOutput(bscript.fromASM(output.script), output.value)
+    if (output.address) {
+      txb.addOutput(output.address, output.value)
+    } else {
+      txb.addOutput(bscript.fromASM(output.script), output.value)
+    }
   })
 
   if (sign === false) return txb
 
   f.inputs.forEach(function (input, index) {
+    if (!input.signs) return
     input.signs.forEach(function (sign) {
       var keyPair = ECPair.fromWIF(sign.keyPair, network)
       var redeemScript