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

Add P2SH-Multisig support #12

Merged
6 commits merged into from
Aug 15, 2018
Merged

Add P2SH-Multisig support #12

6 commits merged into from
Aug 15, 2018

Conversation

bjarnemagnussen
Copy link
Collaborator

Adds support for Pay-To-Script-Hash (P2SH) multi-signature contracts.

The multi-signature scheme is compatible with Bitcoin Core, and therefore partially-signed transactions can be co-signed between both implementations.

New classes:

MultiSig and MultiSigTestnet.

Example usage:

from bit import *

# Creating three private keys:
key1 = PrivateKeyTestnet("cNZYKrewErp2wG9LXiAU4H6dzgtyYsSm78tSU14QST9Hu57CMRaJ")
key2 = PrivateKeyTestnet("cMgHThJYoUYc19UjMWn1xabRJLh3PCKyKfiTM1ssV2ZPe7V6eaBL")
key3 = PrivateKeyTestnet("cR1C9ZB7qoVktmQDQsUP8KcTM37G1PYN3oR2B6ua27EycGDVN6fE")

# Making a list containing the public keys in hex:
key1p = key1.pub_to_hex()
key2p = key2.pub_to_hex()
key3p = key3.pub_to_hex()
plist = [key1p, key2p, key3p]

# Creating a P2SH multi-signature contract instance with key1 requiring two signatures from the total of three keys:
multi = MultiSigTestnet(key1, plist, 2)

# Showing the multi-signature address to send (testnet) bitcoins to:
multi.address
# 2MvkZtmW1w7hBPMSjQmEhHzrtcgiQGEBrtD

# Create and partially-sign a new transaction spending from the multi-signature contract:
tx = multi.create_transaction([], fee=21, leftover='mpDJqjKMyQaFjEMwYHKJBP3mM4rM2ejJdk')

# Create another instance of the multi-signature contract with key2:
multi2 = MultiSigTestnet(key2, plist, 2)

# Fully sign the spending transaction using our second instance of the multi-signature contract:
tx_signed = multi2.sign_transaction(tx)

This is only thought of as a first start to implement P2SH and therefore bugs are still expected and more rigorous testing and optimization may be necessary.

Bjarne Magnussen added 2 commits October 12, 2017 20:58
Adds support for Pay-To-Script-Hash (P2SH) multi-signature contracts.

The multi-signature scheme is compatible with Bitcoin Core, and
therefore partially-signed transactions can be co-signed between both
implementations.

New classes:

MultiSig and MultiSigTestnet.

Example usage:

from bit import *
key1 =
PrivateKeyTestnet("cNZYKrewErp2wG9LXiAU4H6dzgtyYsSm78tSU14QST9Hu57CMRaJ")
key2 =
PrivateKeyTestnet("cMgHThJYoUYc19UjMWn1xabRJLh3PCKyKfiTM1ssV2ZPe7V6eaBL")
key3 =
PrivateKeyTestnet("cR1C9ZB7qoVktmQDQsUP8KcTM37G1PYN3oR2B6ua27EycGDVN6fE")

key1p = key1.pub_to_hex()
key2p = key2.pub_to_hex()
key3p = key3.pub_to_hex()
plist = [key1p, key2p, key3p]

from the total of three keys:
multi = MultiSigTestnet(key1, plist, 2)

multi.address

tx = multi.create_transaction([], fee=21,
leftover='mpDJqjKMyQaFjEMwYHKJBP3mM4rM2ejJdk', unspents=unspent)

multi2 = MultiSigTestnet(key2, plist, 2)

tx_signed = multi2.sign_transaction(tx)

spending from the multi-signature contract.

This is only thought of as a first start to implement P2SH and therefore
bugs are still expected and more rigorous testing and optimization may
be necessary.
@ofek
Copy link
Owner

ofek commented Oct 14, 2017

You have done a lot of work here, thanks! I'll look soon.

Copy link
Collaborator Author

@bjarnemagnussen bjarnemagnussen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a bug where string is compared with bytes when constructing outputs and checking the cases of the output-types (P2PKH/P2SH)

# Real recipient
if amount:
# P2SH
if amount and (b58decode_check(dest)[0] == MAIN_SCRIPT_HASH or
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I produced a bug here. b58decode_check(dest)[0] casts to a string which is then compared to MAIN_SCRIPT_HASH which is bytes. This is serious, because outputs to P2SH will not be catched and instead be interpreted as P2PKH outputs...

if amount:
# P2SH
if amount and (b58decode_check(dest)[0] == MAIN_SCRIPT_HASH or
b58decode_check(dest)[0] == TEST_SCRIPT_HASH):
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same problem as above.

@ghost ghost mentioned this pull request Dec 17, 2017
@ghost
Copy link

ghost commented Dec 17, 2017

In #23 I talked about throwing an exception for mainnet addresses that do not begin with a 1. Am I correct in understanding that there are no tests in place for that currently?

@ofek
Copy link
Owner

ofek commented Dec 17, 2017

@teran-mckinney Yes, not yet.

ofek pushed a commit that referenced this pull request Jan 15, 2018
We don't yet support pay2sh. Until #12 is ready, we should throw
exceptions if given an address in an unexpected format.
@dspechnikov
Copy link

Any chance this will be finished anytime soon? I need to be able to send transactions to P2SH addresses.

@bjarnemagnussen
Copy link
Collaborator Author

bjarnemagnussen commented Apr 8, 2018

@dspechnikov: I hope to be able to continue working on it. But you can maybe try to test this branch a little, because it actually does allow you to send to P2SH addresses, see here:
https://github.com/bjarnemagnussen/bit/blob/c17d730c8d84ffe4602bb53ca964db123794fc0e/bit/transaction.py#L247

I just highly recommend not to use it in production, as it has not yet been tested thoroughly.

@impowski
Copy link

@bjarnemagnussen Any progress on this one?

@bjarnemagnussen
Copy link
Collaborator Author

Just to make sure: the master branch of my fork (https://github.com/bjarnemagnussen/bit) already has the ability to make P2SH-Multisig and also supports sending to Segwit addresses (including bech32) and receiving to P2SH-nested Segwit addresses. I have been fixing a few bugs I found regarding the interoperability with Bitcoin Core, so that e.g. a partially signed transaction from either Bit or Bitcoin Core can now be exchanged and fully signed.

But since only I have been testing it, I am not confident in suggesting to use it in production. However, all the functionality is implemented and I did not encount any problems. So maybe just check it out with testnet coins or small amounts and let me know if there are any problems.

I have not yet written any documentation for the Segwit features. But you can see the segwit address by calling

key = PrivateKey()
key.sw_address

Bit will then by itself take care of creating/signing transactions using segwit inputs if there are any (make sure to call key.get_unspents() before creating/signing transactions, just as before). It is similar for Multig instances.

Some parts of the code are still a little cluttered and I want to clean it up a bit. Of course, that should not influence the operation of it.

@impowski
Copy link

@bjarnemagnussen could you please give me little guide how to use it with multisig?

@bjarnemagnussen
Copy link
Collaborator Author

@impowski: Have a look at my first message in this pull request. Let me know if you need some clarification on the example.

@impowski
Copy link

@bjarnemagnussen when I did sign_transaction my Python freezes, maybe it's me, need to check it.

@impowski
Copy link

@bjarnemagnussen Yeah, it doesn't work I guess, can you check it on your machine?

@bjarnemagnussen
Copy link
Collaborator Author

@impowski Can you show me how you initiate your keys and call the function so that I can check it?

@impowski
Copy link

@bjarnemagnussen

from bit import PrivateKeyTestnet, MultiSigTestnet

private_keys_hex = ['b4741e2e87c19256c81a0662480b75f15831c7017d28b4586882c52b89f91bcf', '3c212d5724a2a6bd5004c502a4be7ba55a1cf4a0923ffd1a9bb3cb6fd7201bed']

private_keys = [
    PrivateKeyTestnet.from_hex(private_keys_hex[0]),
    PrivateKeyTestnet.from_hex(private_keys_hex[1])
]

private_keys[0].instance

public_keys = [
    private_keys[0].pub_to_hex(),
    private_keys[1].pub_to_hex()
]

multisig = MultiSigTestnet(private_keys[0], public_keys, 2)
unspents = multisig.get_unspents()

tx = multisig.create_transaction([('2Mv7MJVHeqjyLVFUg7hc1oJoWedYySeGBsh', 100, 'usd')], unspents=unspents)

multisig2 = MultiSigTestnet(private_keys[1], public_keys, 2)

tx_signed = multisig2.sign_transaction(tx)

print(tx_signed)

@impowski
Copy link

@bjarnemagnussen And when I take for example partially signed transaction and signing it with second key myself I can't broadcast it to the network. Because for the first input witness script is different and I have a non-segwit address in the output.

@bjarnemagnussen
Copy link
Collaborator Author

bjarnemagnussen commented Jun 14, 2018

@impowski Thanks for your feedback! I have found the bug and fixed it with a new commit to my master branch. The problem was in the way signatures from partially signed transactions were extracted, since coincidently the redeemscript was exactly of the same length as typically a signature is. It is on my todo-list to improve it further.

Furthermore, since you are using the segwit functionality you must make sure that also the instance multisig2 has the correct unspents initiated (e.g. by calling multisig2.get_unspents() before multisig2.sign_transaction()). This is because signatures in segwit are done for a string containing the amount it spends with the inputs. The amounts are provided from the unspents in multisig2.

@impowski
Copy link

@bjarnemagnussen It signs transaction right now, but can't broadcast it:
You can decode it on https://live.blockcypher.com/btc-testnet/decodetx/

010000000001021b119ace084fd2bdb0bb687389f6f51346481ba5646a1eeec3fe70750a09452c0100000023220020d99be0d7e677f86ac50fe8a1e833cb1ef3e99539f9337f08469deed67b649c6affffffffb0cbb5ac31474f3de5128ca60e59c483510bee13d720fa01a928588aef04e74b0000000023220020d99be0d7e677f86ac50fe8a1e833cb1ef3e99539f9337f08469deed67b649c6affffffff0224ce17000000000017a9141f6b27ac1ad977dc69642248e5e765fc526d24d8873ba32e350000000017a914a0ae2c9a1885a63b24538e8d9b842da853085748870400473044022031ca119ec537bb5d08a9b2066f567ea2696e4e00c809630fdddda4017d220189022065c8f720eacbcab280e01cfc4263770919344f99cfb7d0164c97f9bcefcef9b60147304402200d6668dd8a03f43613155a1dc8db2dad912e3f246b79a7ea4608c382f3774946022019173cf43f2449d0adfb59785e519ab6a93813fc413137b34582c202957082ea01475221020d2036fea368d0400acabe96e975b6f4e24c6f40f2e45db4264c2f117702c1e72102d0b84f4a3d4890b2fdde4dc74db4328fb3504e0247187d8286311c75b3b6bdeb52ae0400483045022100e7d810ac62ed873527edecc89297efd8126ee95a186c6c23bfb3ef32dea5cc4e02200bf4e093113230218c89a09dda85d7fdf663bde82276d7c39d033c203713f4f501473044022035218317ce3b611c8d9303e9c2df0846ca587d2d003bcfee98ae891ba1476a7b02200ace5331af9b8f97ce1d6749cfe444b4f7b2b6172fae7e838da04a55a46ecf4901475221020d2036fea368d0400acabe96e975b6f4e24c6f40f2e45db4264c2f117702c1e72102d0b84f4a3d4890b2fdde4dc74db4328fb3504e0247187d8286311c75b3b6bdeb52ae00000000

The output address is different, input address is basically SegWit address and in the output it's just basic address and the witness script is kinda messy I guess.

@bjarnemagnussen
Copy link
Collaborator Author

bjarnemagnussen commented Jun 14, 2018

@impowski I think this is unfortunately a problem with blockcypher using testnet.

Can you try to push the transaction on testnet using blockchain.info (https://testnet.blockchain.info/pushtx) or your own node and let me know if it worked?

Edit: Regarding the output address. For now it simply uses the original implemention to determine a return address, which therefore is the legacy address of the multisig. I will later at a later point make a check so that if it spends from at least one segwit input it returns to the segwit address.

@impowski
Copy link

@bjarnemagnussen How do you recommend sending it? Via NetworkAPI or smth different? Aaaaaaaaaaaand it works, we will use your fork in production until PR drops.

@impowski
Copy link

@bjarnemagnussen And the address is still different. If I write private_key.address I get this one 2N7tpkpBJPmv2BD37eTXDUJJzhpXEoyyNU9, if I write private_key.sw_address I get this one which I need 2N2qWuN9rpfw4DvPqjJ3SD1adNnmZdeM5NU. The problem is that an output gives me the first one. Here is the transaction https://chain.so/tx/BTCTEST/2c13d962de6c63253daf477214ea3207cfdab14b8844003e096f331052198b1a

@impowski
Copy link

@bjarnemagnussen I don't know what to do now with my faucet funds they've gone to the beyond world I guess 😨

@bjarnemagnussen
Copy link
Collaborator Author

@impowski It should work to use a direct call to NetworkAPI.broadcast_tx_testnet(tx_hex) which uses a call to BitPay or alternatively SmartBit to push the testnet transaction. But I haven't tested the NetworkAPI with testnet a lot.

In the future I will make the send-function allow to finally sign and then directly broadcast a transaction. But until now this is not possible.

I am not sure I understand your last question correctly. You can still spend the new testnet coins you received to the legacy multisig (2N7tpkpBJPmv2BD37eTXDUJJzhpXEoyyNU9). The multisig instance will always check both legacy and segwit unspents. You just have to make sure your multisig instance has access to the unspents (by running multisig.get_unspents() and then create and sign your transaction just as before).

You are very welcome to open an issue on my fork if you find any problems!

@impowski
Copy link

impowski commented Jun 14, 2018

@bjarnemagnussen But why it sent on legacy address when I needed it on segwit address? That a kinda problem I guess. Oh I see...
I guess that's why:

unspents, outputs = sanitize_tx_data(
            unspents or self.unspents,
            outputs,
            fee or get_fee_cached(),
            leftover or self.address,
            combine=combine,
            message=message,
            compressed=self._pk.is_compressed(),
            version='test'
        )

@bjarnemagnussen
Copy link
Collaborator Author

bjarnemagnussen commented Jun 14, 2018

@impowski You can always manually set the return address to the segwit-address like this multisig.create_transaction([('2Mv7MJVHeqjyLVFUg7hc1oJoWedYySeGBsh', 100, 'usd')], leftover=multisig.sw_address, unspents=unspents)

Edit: But this is getting a little off-topic for the multisig-implementation, so maybe open a new issue on my fork if there should be something else.

@impowski
Copy link

@bjarnemagnussen Oh thanks, totally forgot about that. We will use it in production some time and I'll let you know if anything new will get in the way. Big thanks for help and for that fix 💯

Make some better checks to validate signatures extracted from partially signed transactions.
@impowski
Copy link

impowski commented Jul 4, 2018

Used this PR in production, everything seems fine so far. Can't wait till it be finally merged in master and published. Amazing work @bjarnemagnussen 🎉

@7hacker
Copy link

7hacker commented Aug 10, 2018

🎉 Works on mainnet! Moved some coins to a multisig wallet on Coinbase from a wallet generated by this library

@ghost
Copy link

ghost commented Aug 14, 2018

@ofek, what do you think about merging this?

@ofek
Copy link
Owner

ofek commented Aug 14, 2018

@teran-mckinney Go ahead 🙂

@ghost
Copy link

ghost commented Aug 15, 2018

Here goes nothing... might need some cleanup but seems like it's time.

@ghost ghost merged commit 3a79a28 into ofek:master Aug 15, 2018
@7hacker
Copy link

7hacker commented Aug 15, 2018

woot! 👏 Thanks for your great work here!

ghost pushed a commit that referenced this pull request Sep 13, 2018
Updates branch to current master including MultiSig
@bjarnemagnussen bjarnemagnussen deleted the multisig branch January 19, 2019 05:24
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants