Transaction: get normalized hash / id

[mhanne]

Function to get the normalized tx hash / id.

This is needed convenient to deal with transaction malleability and is useful to keep track of partially signed multisig transactions where the regular hash changes after each signature is added.

[coveralls]

Coverage increased (+0.01%) to 98.26% when pulling 5257d56 on mhanne:normalized_id into abf870f on bitcoinjs:master.

[dcousens]

Nit: please adhere to project style by adding whitespace after the if keyword, and always use strict equality where possible.

if (hashScript !== null && inIndex >= 0) {
  txTmp.ins[inIndex].script = hashScript
}

[dcousens]

Everything looks great after you fix the above comments :).

The only remaining question is, what problems does this solve?
What this is actually doing is exactly as the implementation looks like:

return this.hashForSignature(-1, null, Transaction.SIGHASH_ALL)

It returns a hash which accounts for all inputs disregarding their script signatures.
This allows you to identify transactions despite potentially having different script signatures (malleability, double spend), but it also stops you from semantically recognizing transactions that may have variable inputs.

I feel like there is a hidden gotcha in what might allow unknowing users to use this without knowing its consequence... however, granted, they might use the standard Id/Hash also...

@gmaxwell makes a succinct case-in point why this could be a bad idea.

The Bitcoin core team eventually voted against merging it, but it should be noted large companies like Coinbase mentioned that they did use a normalized hash internally.

bitcoinj have no such thing.

The Bitcore team merged it.

@ryanxcharles, thoughts on whether you felt normalized hash has had a positive net gain for Bitcore? (or even https://github.com/ryanxcharles/fullnode)

[dcousens]

Also pinging @jprichardson, @locksley, @weilu

[dcousens]

@mhanne I'm almost tempted to not add the methods, but allow for the parameter support of:

return this.hashForSignature(-1, null, Transaction.SIGHASH_ALL)

Which you have only just added, that way the implications are obvious, but the potential for mistakes by users who don't know what the implications are, is hopefully less.

[coveralls]

Coverage increased (+0.01%) to 98.27% when pulling 3e4ae73 on mhanne:normalized_id into 75ca355 on bitcoinjs:master.

[mhanne]

Alright, I fixed the issues you mentioned, thanks for pointing it out :)

I think there is a valid use for an identifier of a specific set of changes to the current state of the blockchain.

If you look at it like that, it's not supposed to be the same hash for different stages of ANYONECANPAY, nor should it care about how a transaction is signed (ie. who signed it):

• If the actual inputs (not just the signatures) change, it would result in a different state change in the blockchain
• Likewise, it doesn't matter who signed it or if it even has a valid signature at all, it represents the same (potential) change to the utxo set.

The "sighash with all inputs zeroed out" kind of suggests itself, though I agree that "normalized hash/id" probably isn't the best name. Maybe "empty sighash" or "null sighash" or something like that? "normalized sighash"? That would make it clearer and hint at what it actually is.

Unless I'm mistaken, @gmaxwell is only arguing why it wouldn't be a workable solution to exclusively use the normalized hash for tracking, not how it could hurt to have it additionally.

It's not meant to replace the regular tx hash, nor is it a simple drop-in replacement to magically fix all code that is vulnerable to malleability. It's also not required to deal with it, but it can be handy. I don't see how it not being applicable to every situation would be a reason not to use it when it is.

[mhanne]

To clarify, the specific usecases I'm thinking of are:

1. when sending (or receiving) an unconfirmed transaction, use it to check if the tx - or its equivalent - is being included in a block

2. when passing a multisig tx around between signers, use it to identify the transaction while the regular hash changes

Is there anything fundamentally wrong with that?

I'm not actually using bitcoinjs to send/receive transactions, so to me the first point is more theoretical, but I'm working on something involving case two right now.

Personally, I'd be fine with calling hashForSignature() directly, but having a shortcut/label for it would be nice.

[dcousens]

ACK on all changes, good to merge. Just a matter of whether I agree with the fundamentals now :). Still thinking it over and waiting for others feedback if possible. Thanks @mhanne

[weilu]

I honestly think this function can be achieved much cleaner without using and modifying hashForSignature:

function getNormalizedHash = function(tx) {
  var txTmp = tx.clone()

  txTmp.ins.forEach(function(txIn) {
    txIn.script = Script.EMPTY
  })

  var hashTypeBuffer = new Buffer(4)
  hashTypeBuffer.writeInt32LE(Transaction.SIGHASH_ALL, 0)

  return crypto.hash256(Buffer.concat([txTmp.toBuffer(), hashTypeBuffer]))
}

@dcousens put up a very nice summary of the whole debate. IMHO, aligning bitcoinjs' features with bitcoin core as much as possible provides consistency and least surprise for devs. I'm not comfortable merging a feature explicitly rejected by the core team after much debate, especially when this feature is very easy to implement when one indeed needs it for their own purposes.

[jprichardson]

@weilu said it best:

I'm not comfortable merging a feature explicitly rejected by the core team after much debate, especially when this feature is very easy to implement when one indeed needs it for their own purposes.

👍

[mhanne]

Alright then, thanks for taking the time to consider it.