Segwit serialization support and nested p2sh signing #50
Conversation
| return writer.toBuffer(); | ||
| }; | ||
|
|
||
| MultiSigScriptHashInput.prototype.getSatoshisBuffer = function() { |
There was a problem hiding this comment.
Maybe move this to input? It helps keep pay-to-script-hash/witness-scripthash handling more general
|
+1. I'll be following this one closely. Thanks for working on this. |
|
@braydonf what's the status on this PR and if it's not finished what is left to do and how can I help? |
|
Rebased on latest master. |
|
@rubensayshi from what I remember, I think these need to be finished (for the initial merge):
Edit: Added this list to a task list in the PR. If there anything else please make a note. |
| * @return {Address} | ||
| */ | ||
| Address.createMultisig = function(publicKeys, threshold, network) { | ||
| Address.createMultisig = function(publicKeys, threshold, network, nestedWitness) { |
There was a problem hiding this comment.
Would Address.createP2SH be better here, allowing arbitrary scripts to be passed in? Technically, multisig outputs don't have an address, but P2SH outputs do
There was a problem hiding this comment.
Address.payingTo is for allowing arbitrary scripts to be passed in. Address.createMultisig is for creating multisig p2sh addresses, as bare multisig doesn't have an address, as you mention.
FWIW: https://github.com/bitcoin/bitcoin/blob/master/src/test/script_tests.cpp#L857 this might also be useful if you don't test against it already. It's mainly for verification purposes, but may turn up bugs outside of this PR. When working on signing, it's best to define your standard types (p2pk/p2pkh/multisig) and be able to deal with them anywhere P2SH, P2WSH comes in, since both of these can be used in tandem. An example of things maybe going awry is this file: https://github.com/bitpay/bitcore-lib/blob/master/lib/transaction/input/multisigscripthash.js & the multisig.js that was added after. P2SH & P2WSH require general modifications to a plain script-in-txout-script style transaction. AFAICT Bitcoin Core does this as well. I would submit a patch, but it's not a trivial refactoring.. just my two cents worth! See serializeSimpleSig / serializeSignatures https://github.com/Bit-Wasp/bitcoin-php/blob/master/src/Transaction/Factory/InputSigner.php#L460-544. The |
|
@afk11 We have tests using https://github.com/bitpay/bitcore-lib/blob/master/test/data/bitcoind/script_invalid.json and https://github.com/bitpay/bitcore-lib/blob/master/test/data/bitcoind/script_valid.json however these should likely also be updated. Added to the list. |
braydonf
changed the title
|
Note witness v0 program can be up to 40 bytes long https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#Witness_program Only use compressed keys as per BIP143 https://github.com/bitcoin/bips/pull/459/files#diff-d6a595a37a5d316054e79e53e9c2382fR128 |
|
isWitnessProgram updated with a max of 40 bytes: |
|
bump |
|
What's the status of this PR? |
|
It's ready for testing, and can go on 0.13 or 1.0.0 branches. |
|
I guess I mean, how are things going supporting [P2PK, P2PKH, multisig] in any representation of [P2SH, P2WSH, P2SH|P2WSH]? Currently there's only a |
|
The scripts for pay-to-witness-public-key-hash and pay-to-witness-script-hash can be built and verified, there are tests for this case. However I don't think there are convenience methods for them. It's related to the deferred BIP142 https://github.com/bitcoin/bips/blob/master/bip-0142.mediawiki So I think adding native segwit convenience methods can be made with another PR, while this one can bring in what is necessary to support the transaction serialization and script verification. |
|
Ready to go? |
Using this code in the test to get test data:
```
string TxHexStr(CTransaction tx)
{
CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION);
ssTx << tx;
std::string ssTxStr = ssTx.str();
return HexStr(ssTxStr);
}
std::cout << "scriptPubkey1: " + HexStr(scriptPubkey1) + "\n";
std::cout << "output1: " + TxHexStr(output1) + "\n";
```
From commit bitcoin core 0.13.0 commit: a402396dce64c42ea73535b7dde4a9164d430438
In the file: src/test/transaction_tests.cpp#L493
From commit bitcoin core 0.13.0 commit: a402396dce64c42ea73535b7dde4a9164d430438 In the file: src/test/transaction_tests.cpp - pay-to-compressed publickey (v0) - p2sh witness pay-to-compressed pubkey (v0) - witness 2-of-2 multisig - p2sh witness 2-of-2 multisig
|
What is the ETA for this to be merged? Does it currently sign all the previously supported script types with P2SH and P2SH|P2WSH? |
|
@afk11 I would not expect to see any additional work on this PR until segwit activation appears imminent. |
|
Any news ? |
|
Per an email to the Segwit 2x mailing list, parts of the bitcore project are being discontinued (called it #119) in favor of bcoin. It's genuinely frustrating I, and certainly @braydonf, put effort into adding segwit support for this library 19 months ago (with others offering to help), for it to be discontinued when people have been testing other implementations for as long. I guess that's what you get for using a business backed bitcoin implementation - and I will hesitate before I do so again (looking at you bcoin) |
|
Is there still progress or do I have to move away from copay/bitcore? |
|
@lichtamberg looks like it. They're suggesting everyone upgrades to btc1 https://blog.bitpay.com/bitcore-segwit-activation/ |
|
Lol, I would never use this shit BTC1.. And I'm shocked that bitpay is distributing malware now: https://twitter.com/larrybitcoin/status/898188496015425536 Bitpay was a really nice company, but within the last months they went fully retard... |
|
FYI: Satoshi Labs, @Runn1ng, has been maintaining a branch that supports segwit for quite some time now: Also Bitcore patches for indexing were applied to 0.13 Bitcoin Core a year ago, which also supports segwit (I made sure that these were applied to support segwit when activated): Running bitcore with insight-api and etc. is ready to go with segwit with this PR and those branches. |
|
Bitcore-node is really not the problem, bitcore-lib is. Bitcore-lib / this repository does not have the BIP143, BIP141 code, unless it's in this pull request (which is also incomplete). So users of this library can't sign segwit transactions, or create segwit addresses, etc. |
|
Why this https://blog.bitpay.com/bitcore-segwit-activation/ was posted, what is the stand of bitpay and bitcore will it only support segwit2x. Has the orginal segwit implemented or not. Please clarify. |
|
@afk11 if you care only about backend for insight/bitcore, this is enough, you don't need the code for creating segwit addresses. Also you don't need that for copay either, it doesn't use segwit currently. So if you don't plan to also update copay to use segwit addresses for user's addresses something like this is enough. |
|
An update on this: There is a small downside to using a nested witness script; the transaction is actually larger than direct P2WPKH or P2PKH, so it is less disk and bandwidth efficient. However it may still mean less fees, especially for the case of a large number of signers. It was unclear what the address would be for segwit at the time this was implemented. Now with BIP173 and bech32, this is a better alternative to using a nested witness script. And there is a push to, at the minimum, support sending to bech32 addresses in more wallets: https://whensegwit.com/ |
Adds initial segwit support, including:
Closes: #49
Closes: #42