Repository: bitcoinjs/bitcoinjs-lib
Pull request #875 (2017-08-31 04:00:55)

Fix txb.__overMaximumFees for segwit

dcousens: We need a test to verify this :+1: 
junderw: Actually, this doesn't fix the problem at all... this maximumFee check is broken when TransactionBuilder is imported from Transaction.

ex.

```javascript
var tx = bitcoin.Transaction.fromHex('01000000000101035811767cb7f7050f7ecad148ccff6f20a8deb3b05fbf8126fdcd38444f919e0000000000ffffffff02a7290100000000001600148d7a0a3461e3891723e5fdf8129caa0075060cff01000000000000001600148d7a0a3461e3891723e5fdf8129caa0075060cff0247304402203aa3f264b05286cd3caffafc2697dea3ef29b528ed5e5a9e0fd3741606b8fbf102201a60f641206e88d30a595ec9a3a5b97b0076d7a6ebd81e8612a279cd209eca6801210283409659355b6d1cc3c32decd5d561abaac86c37a353b52895a5e6c196d6f44800000000')
var txb = bitcoin.TransactionBuilder.fromTransaction(tx)
txb.inputs[0].value = 189200
txb.__overMaximumFees(txb.tx.virtualSize())
txb.__overMaximumFees(txb.tx.byteLength())
txb.tx.virtualSize()
// should be 141 but gives us 113
txb.tx.byteLength()
// should be 222 but gives us 113
```

The reason why is because when a TransactionBuilder imports from Transaction, the txb.tx Transaction object loses its witnesses, so txb.tx.hasWitnesses() returns false even though txb has witnesses.

This problem is a little more hairy than I thought... and this bug actually might push people over 1000 satoshi/byte if they make a large segwit tx and aim for a 500 satoshi-byte rate or something (since the broken Transactions byte count will be less than half the size, it would double the rate for this one function only, and cause an error.
dcousens: @junderw could you open an issue for that?
dcousens: @junderw are you sure?
The Transaction has been re-built at this point,  it should have any witnesses it will ever have.
junderw: Try it for yourself.

Run:

```javascript
var tx = bitcoin.Transaction.fromHex('01000000000101035811767cb7f7050f7ecad148ccff6f20a8deb3b05fbf8126fdcd38444f919e0000000000ffffffff02a7290100000000001600148d7a0a3461e3891723e5fdf8129caa0075060cff01000000000000001600148d7a0a3461e3891723e5fdf8129caa0075060cff0247304402203aa3f264b05286cd3caffafc2697dea3ef29b528ed5e5a9e0fd3741606b8fbf102201a60f641206e88d30a595ec9a3a5b97b0076d7a6ebd81e8612a279cd209eca6801210283409659355b6d1cc3c32decd5d561abaac86c37a353b52895a5e6c196d6f44800000000')
var txb = bitcoin.TransactionBuilder.fromTransaction(tx)
```

Then inspect txb.tx it has no witnesses
junderw: Perhaps TransactionBuilder only breaks witnesst on P2WPKH (no P2SH) inputs.

The hex example I gave above is 1 P2WPKH input.
dcousens: > Then inspect txb.tx it has no witnesses

That is expected.
`.tx` is a merely an internal placeholder structure.
dcousens: We should rename `.tx` to `.__tx`.
junderw: Here's the current __build where the problem __overMaximumFees is being called.

I guess from what you're saying, the problem lies with `var tx = this.tx.clone()` meaning we shouldn't be doing this?

if this.tx doesn't have a witness (even though it should) then this.tx.clone() also does not have a witness

The below still gives incorrect byte counts and the tx2.hasWitnesses() is still false.

```javascript
var tx = bitcoin.Transaction.fromHex('01000000000101035811767cb7f7050f7ecad148ccff6f20a8deb3b05fbf8126fdcd38444f919e0000000000ffffffff02a7290100000000001600148d7a0a3461e3891723e5fdf8129caa0075060cff01000000000000001600148d7a0a3461e3891723e5fdf8129caa0075060cff0247304402203aa3f264b05286cd3caffafc2697dea3ef29b528ed5e5a9e0fd3741606b8fbf102201a60f641206e88d30a595ec9a3a5b97b0076d7a6ebd81e8612a279cd209eca6801210283409659355b6d1cc3c32decd5d561abaac86c37a353b52895a5e6c196d6f44800000000')
var txb = bitcoin.TransactionBuilder.fromTransaction(tx)
txb.inputs[0].value = 189200
var tx2 = txb.tx.clone()
txb.__overMaximumFees(tx2.virtualSize())
txb.__overMaximumFees(tx2.byteLength())
tx2.virtualSize()
// should be 141 but gives us 113
tx2.byteLength()
// should be 222 but gives us 113
```
dcousens: @junderw `.clone` does clone witness data,   if it was present.

`txb.tx` does not have witness data **intentionally**,  as discussed on slack.
`TransactionBuilder.prototype.build` uses `(this.__overMaximumFees(tx.byteLength())) `,  where `tx` *has had* `setInputScript` and `setWitness` called,  and therefore **does** have witness data.
dcousens: I think a safe assumption should be:

``` javascript
var txb = bitcoin.TransactionBuilder.fromTransaction(tx)

assert.strictEqual(txb.build().virtualSize(), tx.virtualSize())
```

If that assumption does not hold,  then yes,  something probably needs to be fixed.

dcousens: @junderw could you amend the test fixtures and rebase this PR?
It it is a bug that can be fixed.
junderw: @dcousens Added test case and fixed a fixture that failed due to this PR. (it had high fees, so I increased the output amounts.)
junderw: I also confirmed this test case fails on master.
junderw: @dcousens Added test case and fixed a fixture that failed due to this PR. (it had high fees, so I increased the output amounts.)
junderw: I also confirmed this test case fails on master.

Patch:
diff --git a/src/transaction_builder.js b/src/transaction_builder.js
index 776259ce..e563c2ee 100644
--- a/src/transaction_builder.js
+++ b/src/transaction_builder.js
@@ -648,7 +648,7 @@ TransactionBuilder.prototype.__build = function (allowIncomplete) {
 
   if (!allowIncomplete) {
     // do not rely on this, its merely a last resort
-    if (this.__overMaximumFees(tx.byteLength())) {
+    if (this.__overMaximumFees(tx.virtualSize())) {
       throw new Error('Transaction has absurd fees')
     }
   }


Test patch:
diff --git a/test/fixtures/transaction_builder.json b/test/fixtures/transaction_builder.json
index 2a524776..ad1cb912 100644
--- a/test/fixtures/transaction_builder.json
+++ b/test/fixtures/transaction_builder.json
@@ -885,7 +885,7 @@
       },
       {
         "description": "Sighash V1: 6-of-6",
-        "txHex": "0100000000010136641869ca081e70f394c6948e8af409e18b619df2ed74aa106c1ca29787b96e0100000023220020a16b5755f7f6f96dbd65f5f0d6ab9418b89af4b1f14a1bb8a09062c35f0dcb54ffffffff0200e9a435000000001976a914389ffce9cd9ae88dcc0631e88a821ffdbe9bfe2688acc0832f05000000001976a9147480a33f950689af511e6e84c138dbbd3c3ee41588ac080047304402206ac44d672dac41f9b00e28f4df20c52eeb087207e8d758d76d92c6fab3b73e2b0220367750dbbe19290069cba53d096f44530e4f98acaa594810388cf7409a1870ce01473044022068c7946a43232757cbdf9176f009a928e1cd9a1a8c212f15c1e11ac9f2925d9002205b75f937ff2f9f3c1246e547e54f62e027f64eefa2695578cc6432cdabce271502473044022059ebf56d98010a932cf8ecfec54c48e6139ed6adb0728c09cbe1e4fa0915302e022007cd986c8fa870ff5d2b3a89139c9fe7e499259875357e20fcbb15571c76795403483045022100fbefd94bd0a488d50b79102b5dad4ab6ced30c4069f1eaa69a4b5a763414067e02203156c6a5c9cf88f91265f5a942e96213afae16d83321c8b31bb342142a14d16381483045022100a5263ea0553ba89221984bd7f0b13613db16e7a70c549a86de0cc0444141a407022005c360ef0ae5a5d4f9f2f87a56c1546cc8268cab08c73501d6b3be2e1e1a8a08824730440220525406a1482936d5a21888260dc165497a90a15669636d8edca6b9fe490d309c022032af0c646a34a44d1f4576bf6a4a74b67940f8faa84c7df9abe12a01a11e2b4783cf56210307b8ae49ac90a048e9b53357a2354b3334e9c8bee813ecb98e99a7e07e8c3ba32103b28f0c28bfab54554ae8c658ac5c3e0ce6e79ad336331f78c428dd43eea8449b21034b8113d703413d57761b8b9781957b8c0ac1dfe69f492580ca4195f50376ba4a21033400f6afecb833092a9a21cfdf1ed1376e58c5d1f47de74683123987e967a8f42103a6d48b1131e94ba04d9737d61acdaa1322008af9602b3b14862c07a1789aac162102d8b661b0b3302ee2f162b09e07a55ad5dfbe673a9f01d9f0c19617681024306b56ae00000000",
+        "txHex": "0100000000010136641869ca081e70f394c6948e8af409e18b619df2ed74aa106c1ca29787b96e0100000023220020a16b5755f7f6f96dbd65f5f0d6ab9418b89af4b1f14a1bb8a09062c35f0dcb54ffffffff02e6312761010000001976a914389ffce9cd9ae88dcc0631e88a821ffdbe9bfe2688ac583e0f00000000001976a9147480a33f950689af511e6e84c138dbbd3c3ee41588ac0800483045022100f902f491c4df15199e584790ae8c7202569a977accac0a09fa3f4f3b6ec3517602205961a951c4a12fa966da67b6fd75975b9de156b9895f8ab5f289ecaee12b9b3501473044022068c7946a43232757cbdf9176f009a928e1cd9a1a8c212f15c1e11ac9f2925d9002205b75f937ff2f9f3c1246e547e54f62e027f64eefa2695578cc6432cdabce271502483045022100bd5294e145d729e9593f49079b74e6e4b8aeba63440408595ce0949d5c6450a702207f9c9fb45907fe0180d3f4bee499006007bb90894b5f824a26dfa5d3afec543303483045022100febf9409d7f3c091ddc4d296a483aae7b3d2a91d38f6ea2a153f7ff085fe7766022078d11972c74cd78f816152463a5e1a5d986dfb94b55cf5f7242e4f6d5df000ff81483045022100a5263ea0553ba89221984bd7f0b13613db16e7a70c549a86de0cc0444141a407022005c360ef0ae5a5d4f9f2f87a56c1546cc8268cab08c73501d6b3be2e1e1a8a088247304402201a0e125aed6a700e45d6c86017d5a9d2264c8079319d868f3f163f5d63cb5bfe02200887608f2322ca0d82df67316275371028b0b21750417d594117963fe23b67ec83cf56210307b8ae49ac90a048e9b53357a2354b3334e9c8bee813ecb98e99a7e07e8c3ba32103b28f0c28bfab54554ae8c658ac5c3e0ce6e79ad336331f78c428dd43eea8449b21034b8113d703413d57761b8b9781957b8c0ac1dfe69f492580ca4195f50376ba4a21033400f6afecb833092a9a21cfdf1ed1376e58c5d1f47de74683123987e967a8f42103a6d48b1131e94ba04d9737d61acdaa1322008af9602b3b14862c07a1789aac162102d8b661b0b3302ee2f162b09e07a55ad5dfbe673a9f01d9f0c19617681024306b56ae00000000",
         "inputs": [
           {
             "txId": "6eb98797a21c6c10aa74edf29d618be109f48a8e94c694f3701e08ca69186436",
@@ -940,11 +940,11 @@
         ],
         "outputs": [
           {
-            "value": 900000000,
+            "value": 5924925926,
             "script": "OP_DUP OP_HASH160 389ffce9cd9ae88dcc0631e88a821ffdbe9bfe26 OP_EQUALVERIFY OP_CHECKSIG"
           },
           {
-            "value": 87000000,
+            "value": 999000,
             "script": "OP_DUP OP_HASH160 7480a33f950689af511e6e84c138dbbd3c3ee415 OP_EQUALVERIFY OP_CHECKSIG"
           }
         ],
diff --git a/test/transaction_builder.js b/test/transaction_builder.js
index 029408cb..fbe78425 100644
--- a/test/transaction_builder.js
+++ b/test/transaction_builder.js
@@ -452,6 +452,33 @@ describe('TransactionBuilder', function () {
   describe('various edge case', function () {
     var network = NETWORKS.testnet
 
+    it('should warn of high fee for segwit transaction based on VSize, not Size', function () {
+      var rawtx = '01000000000104fdaac89627208b4733484ca56bc291f4cf4fa8d7c5f29893c52b46788a0a' +
+      '1df90000000000fffffffffdaac89627208b4733484ca56bc291f4cf4fa8d7c5f29893c52b46788a0a1df9' +
+      '0100000000ffffffffa2ef7aaab316a3e5b5b0a78d1d35c774b95a079f9f0c762277a49caf1f26bca40000' +
+      '000000ffffffffa2ef7aaab316a3e5b5b0a78d1d35c774b95a079f9f0c762277a49caf1f26bca401000000' +
+      '00ffffffff0100040000000000001976a914cf307285359ab7ef6a2daa0522c7908ddf5fe7a988ac024730' +
+      '440220113324438816338406841775e079b04c50d04f241da652a4035b1017ea1ecf5502205802191eb49c' +
+      '54bf2a5667aea72e51c3ca92085efc60f12d1ebda3a64aff343201210283409659355b6d1cc3c32decd5d5' +
+      '61abaac86c37a353b52895a5e6c196d6f44802483045022100dc2892874e6d8708e3f5a058c5c9263cdf03' +
+      '969492270f89ee4933caf6daf8bb0220391dfe61a002709b63b9d64422d3db09b727839d1287e10a128a5d' +
+      'b52a82309301210283409659355b6d1cc3c32decd5d561abaac86c37a353b52895a5e6c196d6f448024830' +
+      '450221009e3ed3a6ae93a018f443257b43e47b55cf7f7f3547d8807178072234686b22160220576121cfe6' +
+      '77c7eddf5575ea0a7c926247df6eca723c4f85df306e8bc08ea2df01210283409659355b6d1cc3c32decd5' +
+      'd561abaac86c37a353b52895a5e6c196d6f44802473044022007be81ffd4297441ab10e740fc9bab9545a2' +
+      '194a565cd6aa4cc38b8eaffa343402201c5b4b61d73fa38e49c1ee68cc0e6dfd2f5dae453dd86eb142e87a' +
+      '0bafb1bc8401210283409659355b6d1cc3c32decd5d561abaac86c37a353b52895a5e6c196d6f44800000000'
+      var txb = TransactionBuilder.fromTransaction(Transaction.fromHex(rawtx))
+      txb.inputs[0].value = 81530
+      txb.inputs[1].value = 81530
+      txb.inputs[2].value = 88920
+      txb.inputs[3].value = 88920
+
+      assert.throws(function () {
+        txb.build()
+      }, new RegExp('Transaction has absurd fees'))
+    })
+
     it('should classify witness inputs with witness = true during multisigning', function () {
       var keyPair = ECPair.fromWIF('cRAwuVuVSBZMPu7hdrYvMCZ8eevzmkExjFbaBLhqnDdrezxN3nTS', network)
       var witnessScript = Buffer.from('522102bbbd6eb01efcbe4bd9664b886f26f69de5afcb2e479d72596c8bf21929e352e22102d9c3f7180ef13ec5267723c9c2ffab56a4215241f837502ea8977c8532b9ea1952ae', 'hex')