hsd icon indicating copy to clipboard operation
hsd copied to clipboard

rpc: swap txid and hash in txToJSON function

Open kilpatty opened this issue 5 years ago • 3 comments

Not sure if I'm in love with this just yet. We are using these terms a little different than in Bitcoin (hash being non-witness data, and txid being witness data), but then when we set the txid of the prevouts, we use the function txid() which is actually the hash that does NOT have witness data.

Setting of the prevouts here: https://github.com/handshake-org/hsd/blob/master/lib/node/rpc.js#L2831

txid function: https://github.com/handshake-org/hsd/blob/bf9b1786b1ea0a1efa16e747a3f15124c17fc141/lib/primitives/tx.js#L1588-L1590

Should we actually be using wtxid in place of txid for the above prevouts? Happy to include that in this PR.

kilpatty avatar Jun 11 '20 21:06 kilpatty

So I think txid is correct and should not be changed. The issue is what hash means. In Handshake, all TX are witness tx, so if we follow Bitcoin semantics then hash will always be the wtxid

References from Bitcoin:

https://chainquery.com/bitcoin-cli/decoderawtransaction

  "txid" : "id",        (string) The transaction id
  "hash" : "id",        (string) The transaction hash (differs from txid for witness transactions)

https://chainquery.com/bitcoin-cli/getrawtransaction

  "txid" : "id",        (string) The transaction id (same as provided)
  "hash" : "id",        (string) The transaction hash (differs from txid for witness transactions)

https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#transaction-id

A new data structure, witness, is defined. Each transaction will have 2 IDs.

Definition of txid remains unchanged: the double SHA256 of the traditional serialization format:

[nVersion][txins][txouts][nLockTime]

A new wtxid is defined: the double SHA256 of the new serialization with witness data:

[nVersion][marker][flag][txins][txouts][witness][nLockTime]

pinheadmz avatar Jun 11 '20 21:06 pinheadmz

So I think what may need to change is this in TX.js:

https://github.com/handshake-org/hsd/blob/1da031c1ce3522ef40f2c2ea1ff60cfe70b30ff9/lib/primitives/tx.js#L1693-L1694

it should probably be:

      txid: this.txid(),
      hash: this.wtxid(),

pinheadmz avatar Jun 11 '20 21:06 pinheadmz

Yeah @pinheadmz you are right - following up from our Telegram convo I'm going to change this PR to make the changes in getJSON on the transaction primitive rather than the RPC function as the RPC calls currently match the same format as bitcoind.

kilpatty avatar Jun 11 '20 22:06 kilpatty

If we ever decide to drop compatibility to the bitcoind, I believe the best way to go would be:

  • txid = hash
  • wtxid = witnessHash

That's how code uses it everywhere, so having the same concepts in the API responses makes more sense. Don't like that hash means something else in some API Responses. So would be good to go through APIs and make sure HTTP/RPC responses are following the code definitions.

EDIT: We could maybe even drop txid/wtxid and leave hash and witnessHash.

nodech avatar Oct 18 '23 07:10 nodech