Transaction: Witness serialization support default only for some func… #701
Conversation
| buffer.writeInt32LE(hashType, buffer.length - 4) | ||
| txTmp.toBuffer(buffer, 0) | ||
| txTmp.__toBuffer(buffer, 0, false) |
There was a problem hiding this comment.
@afk11 this is my understanding, hashForSignature (the original) should always sign the legacy serialisation of a transaction.
There was a problem hiding this comment.
For now that makes sense! There might be room for a convenience function that takes (scriptCode, sigVersion, sigHashType, [amount]) and returns the appropriate hash? It's not very long!
| @@ -395,7 +399,11 @@ Transaction.prototype.getId = function () { | |||
| } | |||
|
|
|||
| Transaction.prototype.toBuffer = function (buffer, initialOffset) { | |||
| if (!buffer) buffer = new Buffer(this.byteLength()) | |||
| return this.__toBuffer(buffer, initialOffset, true) | |||
| } | |||
There was a problem hiding this comment.
Note the public API does not change
| @@ -386,7 +390,7 @@ Transaction.prototype.hashForWitnessV0 = function (inIndex, prevOutScript, value | |||
| } | |||
|
|
|||
| Transaction.prototype.getHash = function () { | |||
| return bcrypto.hash256(this.toBuffer()) | |||
| return bcrypto.hash256(this.__toBuffer(undefined, undefined, false)) | |||
| } | |||
There was a problem hiding this comment.
This has to use an internal function such that getHash is always returning the legacy TXID of the transaction, not WTXID
|
@afk11 the only hard parts of any of this: naming things, and finding test fixtures |
|
I'll dig up the wtxid tomorrow - the witness fixtures are on testnet! |
…ed hashs (failing)
| "raw": { | ||
| "version": 1, | ||
| "ins": [ | ||
| { | ||
| "hash": "97990032af6e5c1741021c017cdd86b081019de4189ee995486941e10af4ae93", |
There was a problem hiding this comment.
Bitcoin core has ambiguity between hash and an id.
For testing purposes, we always call the hash the raw hex string which would have been returned by sha256, and the id is reversed.
This amends the test fixtures to reflect that.
There was a problem hiding this comment.
Doh, I should have double checked that.
| @@ -311,13 +269,12 @@ | |||
| "id": "d4ebb5e09c06dd375f265b51eb7f1a14d12deba71329500e2a00a905fdfdc27d", | |||
| "hash": "7dc2fdfd05a9002a0e502913a7eb2dd1141a7feb515b265f37dd069ce0b5ebd4", | |||
| "hex": "0100000001a30e865fa60f6c25a8b218bb5a6b9acc7cf3f1db2f2e3a7114b51af5d6ae811f000000006c473044022026d2b56b6cb0269bf4e80dd655b9e917019e2ccef57f4b858d03bb45a2da59d9022010519a7f327f03e7c9613e0694f929544af29d3682e7ec8f19147e7a86651ecd012321038de63cf582d058a399a176825c045672d5ff8ea25b64d28d4375dcdb14c02b2bacffffffff0160ea0000000000001976a914851a33a5ef0d4279bd5854949174e2c65b1d450088ac00000000", | |||
| "hasWitness": false, | |||
| "witnessHex": "", | |||
There was a problem hiding this comment.
no need having both
| @@ -95,7 +94,7 @@ describe('Transaction', function () { | |||
| it('exports ' + f.description + ' (' + f.id + ')', function () { | |||
| var actual = fromRaw(f.raw) | |||
|
|
|||
| assert.strictEqual(actual.toHex(), f.hex, actual.toHex()) | |||
| assert.strictEqual(actual.toHex(), f.whex || f.hex, actual.toHex()) | |||
There was a problem hiding this comment.
We can import both, but since this exports from the raw data, we have to test against whex first.
|
|
||
| assert.strictEqual(actual.toHex(), f.hex, actual.toHex()) | ||
| var actual = fromRaw(f.raw, true) | ||
| assert.strictEqual(actual.toHex(), f.hex) | ||
| }) |
There was a problem hiding this comment.
might as well test both, because we can
|
@afk11 we can put |
|
This PR purely focuses on fixing the |
|
Good to merge! Waiting on tests |
|
@afk11 nothing controversial here AFAIK, so I've merged to ease the rebase hell. But please still review. |
…tions