Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Segregated Witness Support - WIP #520

Closed
wants to merge 1 commit into from
Closed

Conversation

@rubensayshi
Copy link
Contributor

rubensayshi commented Jan 15, 2016

WORK IN PROGRESS

getting shit to work before making it pretty; almost done with getting shit to work!

depends on: #540

first time spending a p2wpkh (6a539927494f3d6f07078dd1c70a28e0c4910662593072305529eaa66257f59d) and p2wpkh as p2sh (4704de796b8ea819f262eb3399fad85b838327497639da2ceafbd04a08a06be3) with bitcoinjs-lib :D

ToDo

  • Output script creation for P2WPKH
  • Output script creation for P2WSH
  • Transaction encoding / decode
  • SignatureHash v2
  • Transaction signing of P2WPKH
  • Transaction signing of P2WPKH as P2SH
  • Transaction signing of P2WSH
  • toBuffer should be able to return both segwit and non-segwit TX
    • fix getTxId using the segwit TX atm
  • BIP142 addresses for P2WPKH
  • Add value arg to addInput and use it in TransactionBuilder.sign instead of value arg there?
  • Refactoring of changes made so far
  • More tests
    • Add tests for mix of non witness and witness (already manually tested and works)
    • Integration tests (and an API to already run them against segnet)
    • Check bitcoin core projects for any vectors that we can copy
  • Review and discuss changes for BIP142 addresses
  • Review and discuss changes to TransactionBuilder.sign
  • Review and discuss changes to TransactionBuilder.extractInput / script.classifyInput
  • Cache hashPrevouts, hashSequence and hashOutputs for all signatures in a transaction (since that was one of the main reasons for changing the signatureHash xD)

Usage

Signing Native P2WPKH

transactionBuilder.sign(0, keyPair, null, null, /* segWit = */ true, /* txIn.value = */ parseInt(0.9998 * 1e8)))

Signing P2WPKH nested in P2SH

var pubKeyHash = bcrypto.hash160(keyPair.getPublicKeyBuffer())
var redeemScript = bscript.segWitPubKeyHashOutput(pubKeyHash)

transactionBuilder.sign(0, keyPair, redeemScript, null, /* segWit = */ true, /* txIn.value = */ parseInt(0.9998 * 1e8)))

References

@dcousens dcousens added the feature label Jan 15, 2016
@dcousens dcousens added this to the 2.2.0 milestone Jan 15, 2016
@rubensayshi rubensayshi force-pushed the blocktrail:segwit branch 6 times, most recently from d548668 to 636999b Jan 15, 2016
greenaddress added a commit to greenaddress/GreenAddressWebFiles that referenced this pull request Jan 21, 2016
greenaddress added a commit to greenaddress/WalletCordova that referenced this pull request Jan 21, 2016
@rubensayshi rubensayshi force-pushed the blocktrail:segwit branch 9 times, most recently from ca8ae32 to 2ac407e Jan 22, 2016
}

Transaction.prototype.hashForSignatureV2 = function (inIndex, prevOutScript, amount, hashType) {
var hashPrevouts = new Buffer("00" * 32, 'hex')

This comment has been minimized.

Copy link
@fanatid

fanatid Jan 23, 2016

Member

@rubensayshi it isn't python 😆

This comment has been minimized.

Copy link
@rubensayshi

rubensayshi Jan 23, 2016

Author Contributor

hehe yea noticed that yday, been switching between projects too much :P

@rubensayshi rubensayshi force-pushed the blocktrail:segwit branch from 2ac407e to c2a4f70 Jan 23, 2016
@dcousens

This comment has been minimized.

Copy link
Contributor

dcousens commented Jan 23, 2016

@rubensayshi when is the next time you can be available on slack? It'd be good to talk over how this [and future changes] will integrate with TransactionBuilder and Transaction.

@rubensayshi rubensayshi force-pushed the blocktrail:segwit branch from c2a4f70 to 6c742f3 Jan 24, 2016
@rubensayshi

This comment has been minimized.

Copy link
Contributor Author

rubensayshi commented Jan 24, 2016

@dcousens I've got slack on my phone so you can always poke me but I usually prefer keeping my weekends code-free, though this weekend is an exception and working on this PR atm :D
poke me whenever you have time this week, I'm available all week (GMT+1)

@@ -13,6 +13,7 @@ function Buffer256bit (value) { return nBuffer(value, 32) }
var UINT53_MAX = Math.pow(2, 53) - 1
function UInt2 (value) { return (value & 3) === value }
function UInt8 (value) { return (value & 0xff) === value }
function UInt16 (value) { return (value >> 0) === value } // @TODO: is this correct? copy paste of UInt32

This comment has been minimized.

Copy link
@rubensayshi

rubensayshi Jan 24, 2016

Author Contributor

is this okay to do ?

@rubensayshi rubensayshi force-pushed the blocktrail:segwit branch 2 times, most recently from 5ef44ce to 9099951 Jan 24, 2016
if (decode.version === network.pubKeyHash) return bscript.pubKeyHashOutput(decode.hash)
if (decode.version === network.scriptHash) return bscript.scriptHashOutput(decode.hash)
if (decode.version === network.segWitPubKeyHash) {
if (decode.segWitVersion !== 0 && decode.segWitPadding !== 0) {
throw new TypeError(address + ' is too long for normal address and not a valid segWit address')

This comment has been minimized.

Copy link
@rubensayshi

rubensayshi Jan 24, 2016

Author Contributor

not sure if this should go here or in fromBase58Check, considering the length checks are in fromBase58Check, this code has gotten a bit akward now ...

consider one of the test vectors for fromBase58Check we had of an address with hash length of 22, it no longer throws a 'is too long' exception and because we're unaware of network in fromBase58Check we can't really check if it's sane either ...

what was the original reason for seperating fromBase58Check and putting the exceptions there?

also is fromBase58Check considered public API? is it painful to change things?

@rubensayshi

This comment has been minimized.

Copy link
Contributor Author

rubensayshi commented Jun 28, 2016

@dcousens I was about to ask you about this, because I noticed the refactoring of a few things ...

let me know if there's anything I can do!

@btcdrak

This comment has been minimized.

Copy link
Contributor

btcdrak commented Jun 30, 2016

It would be great to get this finished and merged so it can be used on testnet. Segwit is merged to bitcoin/master and active on Bitcoin testnet. It means people can start working on integration in earnest.

@dcousens

This comment has been minimized.

Copy link
Contributor

dcousens commented Jun 30, 2016

@btcdrak I'll see if I can't work towards a 10th July PR date.

@dcousens dcousens changed the title [WIP] Segregated Witness Support Segregated Witness Support - WIP Jul 11, 2016
}
}

Transaction.prototype.hashForSignatureV2 = function (inIndex, prevOutScript, amount, hashType) {

This comment has been minimized.

Copy link
@dcousens

dcousens Jul 14, 2016

Contributor

Why is this necessary?

This comment has been minimized.

Copy link
@afk11

afk11 Oct 7, 2016

Contributor

Not sure if this was cleared up - segwit has a different signature hashing scheme which commits the input amount in the signature, amongst other changes. It has a reusable midstate for all inputs in a transaction which may be cached, so avoids the O(n^2) hashing issue.


function Transaction () {
this.version = 1
this.marker = null
this.flag = null

This comment has been minimized.

Copy link
@dcousens

dcousens Jul 14, 2016

Contributor

Are these ever going to change?
My understanding was the 0 marker was to allow for easy detection in serialization?

This comment has been minimized.

Copy link
@afk11

afk11 Oct 7, 2016

Contributor

The zero marker makes it obvious to old clients that something is wrong (zero-inputs), and I don't think it will change. Flags may change - it's interpreted as a bit-field where the first indicates segwit support, but setting anything else will intentionally result in an error. https://github.com/bitcoin/bitcoin/blob/2f71490d21796594ca6f55e375558944de9db5a0/src/primitives/transaction.h#L320

@dcousens

This comment has been minimized.

Copy link
Contributor

dcousens commented Jul 14, 2016

Lots of debugging console.log lefts in here

@@ -0,0 +1,55 @@
var bufferutils = require('./bufferutils')

This comment has been minimized.

Copy link
@dcousens

dcousens Jul 14, 2016

Contributor

I'd rather opt for something like varstruct than go this route if possible


throw new Error(bscript.toASM(scriptPubKey) + ' has no matching Address')
}

function toBase58Check (hash, version) {
typeforce(types.tuple(types.Hash160bit, types.UInt8), arguments)
function toBase58Check (hash, version, segWitVersion) {

This comment has been minimized.

Copy link
@dcousens

dcousens Jul 14, 2016

Contributor

It'd be great if we can split out all the address based changes to another PR

This comment has been minimized.

Copy link
@afk11

afk11 Oct 13, 2016

Contributor

They shelved that proposal until later - pending feedback/proposals from wallet developers IIRC

@dcousens

This comment has been minimized.

Copy link
Contributor

dcousens commented Jul 14, 2016

Ideally it'd be great to do this in 4 PRs.

scripts (#602)
Transaction
TransactionBuilder
address

@dcousens

This comment has been minimized.

Copy link
Contributor

dcousens commented Jul 14, 2016

Cache hashPrevouts, hashSequence and hashOutputs for all signatures in a transaction (since that was one of the main reasons for changing the signatureHash xD)

Why are we doing such a change in this PR?

@btcdrak

This comment has been minimized.

Copy link
Contributor

btcdrak commented Oct 4, 2016

bump

@btcdrak

This comment has been minimized.

Copy link
Contributor

btcdrak commented Oct 4, 2016

Note for OP, BIP142 was deferred

@btcdrak

This comment has been minimized.

Copy link
Contributor

btcdrak commented Oct 4, 2016

The witness program size was extended from 32 to max 40 bytes https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#Witness_program

@dcousens

This comment has been minimized.

Copy link
Contributor

dcousens commented Oct 4, 2016

@btcdrak support for SegWit scripts was already added a small while ago. #602
This PR is just serving as a reminder to implement the transaction serialisation logic.

See https://github.com/bitcoinjs/bitcoinjs-lib/blob/master/src/script.js#L209-L223 and https://github.com/bitcoinjs/bitcoinjs-lib/blob/master/src/script.js#L343-L354 and https://github.com/bitcoinjs/bitcoinjs-lib/blob/master/src/script.js#L382

@btcdrak

This comment has been minimized.

Copy link
Contributor

btcdrak commented Oct 5, 2016

Also need to disable uncompressed keys which are disallowed under segwit.

@dcousens

This comment has been minimized.

Copy link
Contributor

dcousens commented Oct 5, 2016

Also need to disable uncompressed keys which are disallowed under segwit.

Is that in the BIP?

@btcdrak

This comment has been minimized.

@afk11

This comment has been minimized.

Copy link
Contributor

afk11 commented Oct 7, 2016

I might find some time for this coming up, the release including segwit activation is coming up pretty close. There are some breaks in the API, probably passing the sigVersion in places, and the input amount now needs to be known during signing.

Mostly, sigVersion can be determined in situ, and by including the amount, the full TxOut is now a requirement for signing (previously it was just the scriptPubKey). To address the upcoming BC break, I'd suggest accepting a full txout in the signing API (eg, hashForSignatureV2) mostly to avoid cluttering things with extra parameters.

@dcousens

This comment has been minimized.

Copy link
Contributor

dcousens commented Oct 7, 2016

and the input amount now needs to be known during signing.

That's the first I've seen of that, but again, my attention has been diverted? Any references?

@afk11 do you have a medium with which we can collaborate on that effort more closely?

@afk11

This comment has been minimized.

Copy link
Contributor

afk11 commented Oct 7, 2016

That's the first I've seen of that, but again, my attention has been diverted? Any references?

I think the 'motivation' is scenarios like a Trezor being fed the wrong UTXO amount, since it blindly signs. If we commit the amount in the sig-hash, full nodes would reject a signature where the wrong amount was used.

https://github.com/bitcoin/bips/blob/master/bip-0143.mediawiki#specification

I'm starting to think for a signing API, the requirements (bar keypairs) are the TxOut, redeemScript, witnessScript, and hashType. Looking at it further, I'd love to remove the prevOutScript from addInput, and consolidate it into a txout parameter that replaces amount in TransactionBuilder.sign as seen in this PR.

Inferences

I notice for P2SH the library infers prevOutScript based on the redeemScript. It could probably do this based with segwit scripts too, although I prefer requiring the full TxOut from users always, and only optionally taking a redeemScript / witnessScript.

Core doesn't infer things, it works from the scriptPubKey and decides what it needs next. (Solver in script/standard.cpp) While checking p2sh?/witness?/bare-scriptPubKey? it determines "is it a standard type, and what sighash sigVersion do I use?". P2SH / P2WSH are treated specially, and involve checking elsewhere to see that the hashes match and another call to Solver.

Probably not a major issue, but I like there being a chain of hashes indicating which witnessScript to use (the reverse of infering the scriptPubKey) because then your wallet works based strictly on what scriptPubKey's do you have the required data for.

@afk11 do you have a medium with which we can collaborate on that effort?

Core's slack could work, otherwise happy to use email / XMPP at me$(at)$(IRL name)$(dot)io

@dcousens

This comment has been minimized.

Copy link
Contributor

dcousens commented Oct 7, 2016

I'd love to remove the prevOutScript from addInput, and consolidate it into a txout parameter that replaces amount in TransactionBuilder.sign as seen in this PR.

Interestingly, I'd prefer to move more to addInput and keep sign as minimal as possible, be good to hear your opinions on why the other way.

@afk11

This comment has been minimized.

Copy link
Contributor

afk11 commented Oct 7, 2016

Ah ok :) I suppose the main reason would be to keep signing atomic - given a transaction, signing only takes so much (nIn, PrivateKeyInterface key, TransactionOutputInterface txOut, ScriptInterface redeemScript = null, ScriptInterface witnessScript = null, sigHashType = SigHashInterface::ALL)

Whether we pass all the arguments at signing, or all the arguments at addInput probably doesn't matter, but at least keep it pure, since I don't see any other method to set the prevOutScript (except for directly modifying internal values). I guess my disagreement stems from (i) it being split across two functions and, (ii) we're saving state anticipating it will only be used in that later function, so why not provide it only when necessary?

Also believe the scriptPubKey should be the thing you check a redeemScript against to test suitability when signing, rather than inferring a scriptPubKey from an arbitrary redeemScript and signing with it. I really like how Core work out what to sign with given only a scriptPubKey and it's key/script store: https://github.com/bitcoin/bitcoin/blob/master/src/script/sign.cpp#L139

< /opinion>

@dcousens

This comment has been minimized.

Copy link
Contributor

dcousens commented Oct 7, 2016

As discussed on slack, we should set up an example somewhere that idiomatically shows
P2PKH, P2SH, P2WPKH and P2WSH examples in succession, with all the necessary pieces of information.
Obviously the TransactionBuilder API isn't ready for the task, but from that point we could play with the API prospectively, seeing what fits nicely and what doesn't.

@dcousens

This comment has been minimized.

Copy link
Contributor

dcousens commented Oct 13, 2016

Updated milestone to 3.1.0

@dcousens dcousens modified the milestones: 3.1.0, 3.0.0 Oct 13, 2016
@dcousens

This comment has been minimized.

Copy link
Contributor

dcousens commented Nov 4, 2016

Closing in favour of the newer PRs.
Captured some of the checklist with #686 and #685

@dcousens dcousens closed this Nov 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.