fix nhashtype not getting set by sighash.sign

[mruddy]

I noticed that the SIGHASH value was not getting set while I was trying some code that did essentially this:
var sig = bitcore.Transaction.sighash.sign(tx, key, bitcore.crypto.Signature.SIGHASH_ALL, 0, script);
console.log(sig.toTxFormat().toString('hex'));

sighash.sign is correctly trying to set the nhashtype, but it's not being picked up in the setter method. looks like a simple oversight. i added in the setter and added some more unit tests. i also de-linted an easy double quote nit of an existing test case.

[braydonf]

LGTM. Good catch

[mruddy]

Cool, here's the actual usage example that led me to find this: https://github.com/mruddy/bip65-demos/blob/master/freeze.js

Also, should we add an upper case alias for bitcore.Transaction.sighash and plan to remove the lower case later? The lower case seems inconsistent with the rest of the library:

bitcore/index.js

Line 70 in 1c8ebc0

bitcore.Transaction.sighash = require('./lib/transaction/sighash');

[eordano]

LGTM

[braydonf]

This line I think can be removed, since this PR fixes it: https://github.com/bitpay/bitcore/blob/master/lib/transaction/input/multisigscripthash.js#L146

[pnagurny]

LGTM

[mruddy]

@braydonf I have the changes locally to remove the line in the file you mentioned as well as in https://github.com/bitpay/bitcore/blob/master/lib/transaction/input/input.js#L168

Should I add those two line removals to a re-based commit for this PR? Or should I do that in a separate PR? Just wondering since some others have already reviewed the existing commit. Either way is fine with me. Thanks!

[braydonf]

I think adding to this one is okay. @eordano @pnagurny ?

[pnagurny]

@mruddy yeah just go ahead and add it to this PR.

[maraoz]

LGTM, great catch @mruddy

[mruddy]

@braydonf @pnagurny @eordano @maraoz
Thanks for reviewing these changes. I just removed the now unnecessary nhashtype references from lib/transaction/input/input.js and lib/transaction/input/multisigscripthash.js. I also removed another lint and fixed some white space formatting in the latter file while I was in there.

[braydonf]

I think the fixme comments are not relevant anymore with the lines removed: https://github.com/bitpay/bitcore/pull/1278/files#diff-7541e8aa080e031cbd44309592b5b366L145

[mruddy]

@braydonf I was wondering about whether to leave those or not. Latest commit removes the fixme comments too.

[mruddy]

@braydonf I was just going through and looking at lib/transaction/signature.js and lib/crypto/signature.js. There may be cases where people create TransactionSignature's with a sigtype, but the signature member gets populated from a buffer that does not set the nhashtype. So, it's probably safer to go back to my original commit that leaves in the couple of lines that I removed today. What do you think?

[braydonf]

You're probably right. The tests pass, however there is a case that nhashtype wouldn't be set, and sighash.verify expects it.

[mruddy]

@braydonf Exactly. I've reset back to the original commit. This should be good to go.

[braydonf]

Thanks! Landed in release v0.12.11