Bitcoin Cash TX redemption #831

afk11 · 2017-07-31T09:03:59Z

Alternative to #826, with thanks to @junderw.

Avoids making Bitcoin Cash the default hashType, and exposes the functionality once the transaction_builder has been set to allow it. Adds a test case for P2PKH ALL|FORKID.

TransactionBuilder.enableBitcoinCash(setting)
Transaction.hashForCashSignature

Opening this so it's visible and people can use if needed, see #826 for discussions

dcousens · 2017-07-31T09:56:10Z

src/transaction.js

+/**
+ * Hash transaction for signing a specific input for Bitcoin Cash.
+ */
+Transaction.prototype.hashForCashSignature = function (inIndex, prevOutScript, inAmount, hashType) {


hashForCash 😆

finding humor in the little things, but will change it if need be :)

dcousens · 2017-07-31T09:56:44Z

src/transaction_builder.js

  this.tx = new Transaction()
 }

+TransactionBuilder.prototype.enableBitcoinCash = function (setting) {
+  if (typeof setting === 'undefined') {


s/setting/enable

dcousens · 2017-07-31T09:57:34Z

Same PR disclaimers as #826, please no political discussion 👍 .

dcousens

LGTM (but not to merge)

rubensayshi · 2017-07-31T20:58:11Z

I think when enableBitcoinCash has been set it the default SIGHASH should be ALL | FORKID, it makes no sense to want just ALL since its not even a valid tx (until block 600k or smt at least), and the .sign is a bit verbose with 2 SIGHASH flags.

I agree that merging can be held off until it becomes clear if it's worth having more code to maintain sitting around in the repo.
most bitcoin devs should be capable enough of running a small fork of the lib for their own use if they want it right now (as we do).

I think it's good to have a PR that is reviewed and in a good enough state to be merged, it means others can use the branch without much worry and it's ready to go if there's a desire to merge it.

junderw · 2017-08-01T03:21:29Z

test/bitcoincash.test.js

+    txb.addInput(txid, vout, Transaction.DEFAULT_SEQUENCE, spk)
+    txb.addOutput('mzDktdwPcWwqg8aZkPotx6aYi4mKvDD7ay', value)
+    txb.enableBitcoinCash(true)
+    txb.setVersion(2)


@afk11 @dcousens Does Bitcoin Cash require transactions to be version 2?

I don't think so, I just wanted to reproduce it's RPC output.

panlilu · 2017-08-02T22:26:47Z

@afk11 I use your branch afk11:opt-in-bitcoincash-sighash with the mainnet bcc transaction. But when I broadcast it, returns "16: mandatory-script-verify-flag-failed (Script failed an OP_EQUALVERIFY operation)"

here is my raw transaction:
01000000015acf8c1eecafa55796aed361d72bf61c8174363214d58ba90c0b86839656714c0100000049483045022100bec812c796366d39f4365e632abc9412123b3579b036575b87890ff6c6086e2a0220172ab41a91cbbdbbfc169f7273ab743f5f7c348db1250e2175c3b0000c6e941c41ffffffff0188130000000000001976a91439588eb259de2cc58c332ffb894934ee6432d52888ac00000000

The code is just like your bcc testnet test code except the network.

junderw · 2017-08-03T01:16:26Z

@panlilu

(Script failed an OP_EQUALVERIFY operation) This is unrelated to the current code changes. Your script is broken in an unrelated area.

dcousens · 2017-08-03T01:17:37Z

@panlilu post the code you used?

junderw · 2017-08-03T01:18:45Z

Your scriptSig only contains one signature with no pubkey.

junderw · 2017-08-03T01:24:29Z

@panlilu Yeah, you are missing your pubkey.

01000000 //version
01       //input count
5acf8c1eecafa55796aed361d72bf61c8174363214d58ba90c0b86839656714c // tx hash
01000000 //utxo index
49       // push scriptSig 73 bytes
483045022100bec812c796366d39f4365e632abc9412123b3579b036575b87890ff6c6086e2a0220172ab41a91cbbdbbfc169f7273ab743f5f7c348db1250e2175c3b0000c6e941c41 // scriptSig (73 bytes)
ffffffff // sequence
01 //output count
8813000000000000 // satoshi amount
19 // push scriptPubkey 25 bytes
76a91439588eb259de2cc58c332ffb894934ee6432d52888ac // scriptPubkey
00000000 // Locktime

panlilu · 2017-08-03T02:04:12Z

@dcousens @junderw
Did I miss someting? bccr is the namespace of bitcoinjs with browserify
Why pubkey is missing in scriptSig? Which step adds pub key to scriptSig?

Here is my code:

var network = bccr.bitcoin.networks['bitcoin']
var value = 5000
var input_value = 10000
var txid = '4c71569683860b0ca98bd514323674811cf62bd761d3ae9657a5afec1e8ccf5a'
var vout = 1

var wif = '5Hpiz6vMcsUakDg52g4FAabDSe3ejj2CjXP8sTuuft93iyMUfAf'
var keyPair = bccr.bitcoin.ECPair.fromWIF(wif, network)

var pk = keyPair.getPublicKeyBuffer()
var spk = bccr.bitcoin.script.pubKey.output.encode(pk)

var txb = new bccr.bitcoin.TransactionBuilder(network)
txb.addInput(txid, vout, bccr.bitcoin.Transaction.DEFAULT_SEQUENCE, spk)
txb.addOutput('16EDc1dvditMcc9MEHRicnjgN7nZGe4A7G', value)
txb.enableBitcoinCash(true)


var hashType = bccr.bitcoin.Transaction.SIGHASH_ALL | bccr.bitcoin.Transaction.SIGHASH_BITCOINCASHBIP143

txb.sign(0, keyPair, null, hashType, input_value)

var tx = txb.build()
var hex = tx.toHex()
console.log(hex)

Result:

01000000015acf8c1eecafa55796aed361d72bf61c8174363214d58ba90c0b86839656714c0100000049483045022100bec812c796366d39f4365e632abc9412123b3579b036575b87890ff6c6086e2a0220172ab41a91cbbdbbfc169f7273ab743f5f7c348db1250e2175c3b0000c6e941c41ffffffff0188130000000000001976a91439588eb259de2cc58c332ffb894934ee6432d52888ac00000000

panlilu · 2017-08-03T06:41:12Z

My project: https://github.com/panlilu/bcc-web-redemption
Waiting to solve this issue.

dcousens · 2017-08-03T07:24:34Z

4c71569683860b0ca98bd514323674811cf62bd761d3ae9657a5afec1e8ccf5a:1, according to http://blockchain.info/tx/4c71569683860b0ca98bd514323674811cf62bd761d3ae9657a5afec1e8ccf5a?show_adv=true, is a P2PKH output, not a P2PK... probably your issue.

panlilu · 2017-08-03T07:42:36Z

@dcousens
Then how to fix it. P2PKH output as input is not supported yet?

dcousens · 2017-08-03T07:51:05Z

@panlilu pay-2-pubkeyhash is the default output type, you use it above.

P2PKH:

txb.addOutput('16EDc1dvditMcc9MEHRicnjgN7nZGe4A7G', value)

You could probably fix your code by doing:

txb.addInput(txid, vout)

Instead of specifying P2PK.

P2PK is paying DIRECTLY to the public key, no address involved. This is rare, and technically not as safe cryptographically.

dcousens · 2017-08-03T07:54:08Z

@panlilu the following document might help in any misunderstandings.

panlilu · 2017-08-03T07:58:19Z

@dcousens WOOOOW, It works! THANK YOU!

afk11 · 2017-08-23T20:36:26Z

Just noticed, this PR doesn't play well with multisig's right now, fixMultisigInput doesn't yet know about Bitcoin Cash..this might actually be quite hard since UTXO values aren't known in places like fromTransaction..

dabura667 · 2017-08-23T23:51:25Z

@afk11 We didn't have any problem, but then of course we had the satoshi values readily available from the txb.sign calling function.

I think if you pass the satoshi value to txb.sign it works fine.

afk11 · 2017-08-24T13:13:13Z

sign can only be called once you have a TransactionBuilder instance. new TransactionBuilder is called inside fromTransaction, but it hasn't got the line for enableBitcoinCash yet.

So if you had a partially signed tx, and wanted to use TransactionBuilder.fromTransaction(tx) it wouldn't correctly fix the signature order, and would then refuse to sign.

afk11 · 2017-08-24T13:18:19Z

@dcousens should I push this branch to bitcoinjs's origin? whatever about ourselves, other developers may feel better about using this remote instead of mine.

Adds an extra parameter to fromTransaction, which tells the library to expect a value property to be added on each txin which uses bitcoin cash's sighashtype

dcousens · 2017-08-24T13:24:38Z

@afk11 yes

afk11 · 2017-08-24T13:36:20Z

Pushed a commit which patches fromTransaction so it will work (or make an obvious error) if it isn't provided the value for a Bitcoin Cash input.

If you use fromTransaction, and you parse incomplete multisig inputs, two things are required for this to work as before.

First, a new parameter is required here:

TransactionBuilder.fromTransaction(transaction, network, enableBitcoinCash)
and second, for each transaction.ins, set the input.value field to the correct value (in satoshis)

If this is not done, transaction_builder will not parse your input signatures, and you will be confused why it isn't signing.

skynxt · 2017-09-28T07:08:29Z

test/bitcoincash.test.js

@@ -0,0 +1,37 @@
+/* global describe, it */


Creating bitcoin cash raw transaction and broadcasting on network throws following error:
16: mandatory-script-verify-flag-failed (Script failed an OP_EQUALVERIFY operation). Code:-26
I am broadcasting through API call using URL https://cashexplorer.bitcoin.com/api/tx/send

Show us the code you used to make the transaction. Thanks.

Wrong pubkeyhash I would say.. Did you sign with the wrong private key?

@junderw
code excerpt which works for BTC:
https://gist.github.com/skynxt/d395a115ae9bfbde6e9744b0e962beaa

code excerpt creating tx for BCH that shows error:
https://gist.github.com/skynxt/2e17f5ff088fc4f47d339d55aee14045

Another excerpt of BCH create tx which throws error too :
https://gist.github.com/skynxt/d395a115ae9bfbde6e9744b0e962beaa

@afk11 I am using correct private key for signing. Probably the above code might give you insight on how transactions are being created at my end.
Thank you.

You’re signing the output value not the prevOut value.

You need to pass sign the input value (previous Output value) not the output value

nicolaslopezj · 2017-10-12T13:39:05Z

Why this is not merged?

dabura667 · 2017-10-12T13:58:59Z

Because bitcoinjs-lib is for Bitcoin.

nicolaslopezj · 2017-10-12T14:47:13Z

test/bitcoincash.test.js

+    var spk = bscript.pubKey.output.encode(pk)
+
+    var txb = new TransactionBuilder(network)
+    txb.addInput(txid, vout, Transaction.DEFAULT_SEQUENCE, spk)


Why is Transaction.DEFAULT_SEQUENCE, spk necessary? I made a transaction without those parameters

afk11 · 2017-10-12T16:13:08Z

Pretty much echo what @dabura667 said. Our company is getting by fine just rebasing this on top of our own fork, with modifications :)

To explain why - there are several coins that make irreconcilable changes with bitcoin, and would only cause a maintenance burden on the developers here. Some coins change transaction serialization format, others use different hash functions, and bitcoin cash poaches BIP143 as it's replay protection.

There should be zero expectation that bitcoinjs merges changes for every alt & hard-fork away from bitcoin. Each warrants its own dedicated repository with far better attention and test coverage than we can manage here.

eragon321 · 2017-11-14T17:04:54Z

Does this PR still work? When I tried broadcasting a BCH tx I created with this branch on https://bitcoincash.blockexplorer.com I get the following error.

An error occured:
16: mandatory-script-verify-flag-failed (Signature must be zero for failed CHECK(MULTI)SIG operation). Code:-26

dabura667 · 2017-11-14T23:43:19Z

Did the same code work before Monday’s Hardfork?

eragon321 · 2017-11-15T00:30:04Z

Today is the first time I tried this branch so I don't know wether it worked before Monday’s Hardfork.

afk11 · 2017-11-15T09:31:36Z

Sounds like you made an invalid signature somewhere.. could you have signed with the wrong txout/script?

eragon321 · 2017-11-15T18:59:42Z

Your right. I was doing txb.addInput(txid, vout), when I needed to do txb.addInput(txid, vout, bccr.bitcoin.Transaction.DEFAULT_SEQUENCE, spk). Thank you for the help.

ArnyS · 2017-11-18T08:52:16Z

Please!
Could anybody give full simple example of this build and sign correct transaction for bitcoin cash?
It's very difficult for beginner to understand

fanatid · 2017-11-20T10:20:50Z

Can we change name in package.json here for using this branch and bitcoinjs-lib in one project?

afk11 force-pushed the opt-in-bitcoincash-sighash branch from 129dfe4 to 9cf4237 Compare July 31, 2017 09:09

dcousens reviewed Jul 31, 2017

View reviewed changes

opt-in bitcoin-cash support in transaction_builder

4a4b7fd

afk11 force-pushed the opt-in-bitcoincash-sighash branch from 9cf4237 to 4a4b7fd Compare July 31, 2017 10:44

dcousens approved these changes Jul 31, 2017

View reviewed changes

junderw reviewed Aug 1, 2017

View reviewed changes

dcousens added the feature label Aug 3, 2017

dcousens mentioned this pull request Aug 3, 2017

Add BitcoinCash sighash support #826

Closed

afk11 mentioned this pull request Aug 4, 2017

SIGHASH_FORKID support. #838

Closed

dcousens added external breaking change labels Aug 9, 2017

TransactionBuilder.fromTransaction & Bitcoin Cash

9ac221c

Adds an extra parameter to fromTransaction, which tells the library to expect a value property to be added on each txin which uses bitcoin cash's sighashtype

dcousens changed the title ~~Opt in Bitcoin Cash tx redemption~~ [DO NOT MERGE] Bitcoin Cash TX redemption Sep 5, 2017

dcousens changed the title ~~[DO NOT MERGE] Bitcoin Cash TX redemption~~ [NOT FOR MERGE] Bitcoin Cash TX redemption Sep 5, 2017

skynxt reviewed Sep 28, 2017

View reviewed changes

nicolaslopezj reviewed Oct 12, 2017

View reviewed changes

dcousens changed the title ~~[NOT FOR MERGE] Bitcoin Cash TX redemption~~ [NO MERGE] Bitcoin Cash TX redemption Oct 19, 2017

dcousens mentioned this pull request Nov 1, 2017

Add Bitcoin Gold support #918

Closed

dcousens mentioned this pull request Nov 23, 2017

[NO MERGE] Bitcoin Gold and Bitcoin Cash support on v3.3.0 #932

Closed

dcousens added the ready to merge label Nov 28, 2017

dcousens mentioned this pull request Nov 28, 2017

Bitcoin Cash (and Gold) TX redemption #937

Closed

dcousens closed this Nov 28, 2017

dcousens changed the title ~~[NO MERGE] Bitcoin Cash TX redemption~~ Bitcoin Cash TX redemption Nov 28, 2017

Bitcoin Cash TX redemption #831

Bitcoin Cash TX redemption #831

Conversation

afk11 commented Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

afk11 Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcousens commented Jul 31, 2017

dcousens left a comment

Choose a reason for hiding this comment

rubensayshi commented Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panlilu commented Aug 2, 2017

junderw commented Aug 3, 2017

dcousens commented Aug 3, 2017

junderw commented Aug 3, 2017

junderw commented Aug 3, 2017

panlilu commented Aug 3, 2017 • edited Loading

panlilu commented Aug 3, 2017

dcousens commented Aug 3, 2017

panlilu commented Aug 3, 2017 • edited Loading

dcousens commented Aug 3, 2017 • edited Loading

dcousens commented Aug 3, 2017 • edited Loading

panlilu commented Aug 3, 2017

afk11 commented Aug 23, 2017 • edited Loading

dabura667 commented Aug 23, 2017

afk11 commented Aug 24, 2017

afk11 commented Aug 24, 2017

dcousens commented Aug 24, 2017 • edited Loading

afk11 commented Aug 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afk11 Sep 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolaslopezj commented Oct 12, 2017

dabura667 commented Oct 12, 2017

Choose a reason for hiding this comment

afk11 commented Oct 12, 2017

eragon321 commented Nov 14, 2017

dabura667 commented Nov 14, 2017

eragon321 commented Nov 15, 2017

afk11 commented Nov 15, 2017

eragon321 commented Nov 15, 2017

ArnyS commented Nov 18, 2017

fanatid commented Nov 20, 2017

afk11 commented Jul 31, 2017 •

edited

Loading

afk11 Jul 31, 2017 •

edited

Loading

rubensayshi commented Jul 31, 2017 •

edited

Loading

panlilu commented Aug 3, 2017 •

edited

Loading

panlilu commented Aug 3, 2017 •

edited

Loading

dcousens commented Aug 3, 2017 •

edited

Loading

dcousens commented Aug 3, 2017 •

edited

Loading

afk11 commented Aug 23, 2017 •

edited

Loading

dcousens commented Aug 24, 2017 •

edited

Loading

afk11 commented Aug 24, 2017 •

edited

Loading

afk11 Sep 28, 2017 •

edited

Loading