Repository: ofek/bit

Issue:
Pay to scripthash
I have no experience with paying to scripthashes except when it's worked automagically for me before.

In #12 it looks like the initial workins of P2SH are being added. I'm not sure how straight forward P2SH is.

But... for the time being shouldn't we throw an exception if we try to `send()` to an address that does not begin with 1? Are there any sideeffects to doing so?

I think I just lost a fair bit of coin not reviewing this properly. Hopefully if we add in an exception we can keep others from doing the same.

Discussion:
teran-mckinney: @bjarnemagnussen, could you also review this since it impacts your pull request?

Unfortunately, I know you'll have to rebase undo most of this with your change.

Thank you!
teran-mckinney: @ofek, could you consider merging this if it looks good to you? Would also appreciate if you can bump the version as this can prevent accidental Bitcoin loss and the tests still pass.

I will merge it into bitcash shortly.
teran-mckinney: @ofek do you have the time to review this? Thank you!
ofek: @teran-mckinney Sorry! I recently started a new job which has decreased my OSS time. Can you please ping me again in a few days? Also, would you like write access/become a maintainer? Do you have time? I don't want this project to die :smile:
teran-mckinney: I completely understand. Congratulations on the new job!

Wow, most certainly. I'm happy to take on bit, especially for smaller patches like this. I have the time for it and I'm already using bit in a couple projects.

I'll ping you in a few days. Thanks for getting back to me.
teran-mckinney: Hey @ofek. Just pinging you.

Patch:
diff --git a/bit/format.py b/bit/format.py
index 5ff8679..088d990 100644
--- a/bit/format.py
+++ b/bit/format.py
@@ -35,6 +35,8 @@ def verify_sig(signature, data, public_key):
 
 
 def address_to_public_key_hash(address):
+    # Raise ValueError if we cannot identify the address.
+    get_version(address)
     return b58decode_check(address)[1:]
 
 


Test patch:
diff --git a/tests/samples.py b/tests/samples.py
index 8a9ad70..82b64ae 100644
--- a/tests/samples.py
+++ b/tests/samples.py
@@ -1,8 +1,10 @@
 BINARY_ADDRESS = b'\x00\x92F\x1b\xdeb\x83\xb4a\xec\xe7\xdd\xf4\xdb\xf1\xe0\xa4\x8b\xd1\x13\xd8&E\xb4\xbf'
 BITCOIN_ADDRESS = '1ELReFsTCUY2mfaDTy32qxYiT49z786eFg'
 BITCOIN_ADDRESS_COMPRESSED = '1ExJJsNLQDNVVM1s1sdyt1o5P3GC5r32UG'
+BITCOIN_ADDRESS_PAY2SH = '39SrGQEfFXcTYJhBvjZeQja66Cpz82EEUn'
 BITCOIN_ADDRESS_TEST = 'mtrNwJxS1VyHYn3qBY1Qfsm3K3kh1mGRMS'
 BITCOIN_ADDRESS_TEST_COMPRESSED = 'muUFbvTKDEokGTVUjScMhw1QF2rtv5hxCz'
+BITCOIN_ADDRESS_TEST_PAY2SH = '2NFKbBHzzh32q5DcZJNgZE9sF7gYmtPbawk'
 PRIVATE_KEY_BYTES = b'\xc2\x8a\x9f\x80s\x8fw\rRx\x03\xa5f\xcfo\xc3\xed\xf6\xce\xa5\x86\xc4\xfcJR#\xa5\xady~\x1a\xc3'
 PRIVATE_KEY_DER = (b"0\x81\x84\x02\x01\x000\x10\x06\x07*\x86H\xce=\x02\x01\x06"
                    b"\x05+\x81\x04\x00\n\x04m0k\x02\x01\x01\x04 \xc2\x8a\x9f"
diff --git a/tests/test_format.py b/tests/test_format.py
index b338b03..86dcace 100644
--- a/tests/test_format.py
+++ b/tests/test_format.py
@@ -6,9 +6,11 @@ from bit.format import (
     public_key_to_address, verify_sig, wif_checksum_check, wif_to_bytes
 )
 from .samples import (
-    BITCOIN_ADDRESS, BITCOIN_ADDRESS_COMPRESSED, BITCOIN_ADDRESS_TEST_COMPRESSED,
-    BITCOIN_ADDRESS_TEST, PRIVATE_KEY_BYTES, PUBKEY_HASH, PUBKEY_HASH_COMPRESSED,
-    PUBLIC_KEY_COMPRESSED, PUBLIC_KEY_UNCOMPRESSED, PUBLIC_KEY_X, PUBLIC_KEY_Y,
+    BITCOIN_ADDRESS, BITCOIN_ADDRESS_COMPRESSED, BITCOIN_ADDRESS_PAY2SH,
+    BITCOIN_ADDRESS_TEST_COMPRESSED, BITCOIN_ADDRESS_TEST,
+    BITCOIN_ADDRESS_TEST_PAY2SH, PRIVATE_KEY_BYTES, PUBKEY_HASH,
+    PUBKEY_HASH_COMPRESSED, PUBLIC_KEY_COMPRESSED, PUBLIC_KEY_UNCOMPRESSED,
+    PUBLIC_KEY_X, PUBLIC_KEY_Y,
     WALLET_FORMAT_COMPRESSED_MAIN, WALLET_FORMAT_COMPRESSED_TEST,
     WALLET_FORMAT_MAIN, WALLET_FORMAT_TEST
 )
@@ -41,6 +43,14 @@ class TestGetVersion:
         with pytest.raises(ValueError):
             get_version('dg2dNAjuezub6iJVPNML5pW5ZQvtA9ocL')
 
+    def test_mainnet_pay2sh(self):
+        with pytest.raises(ValueError):
+            get_version(BITCOIN_ADDRESS_PAY2SH)
+
+    def test_testnet_pay2sh(self):
+        with pytest.raises(ValueError):
+            get_version(BITCOIN_ADDRESS_TEST_PAY2SH)
+
 
 class TestVerifySig:
     def test_valid(self):
@@ -146,3 +156,7 @@ def test_point_to_public_key():
 def test_address_to_public_key_hash():
     assert address_to_public_key_hash(BITCOIN_ADDRESS) == PUBKEY_HASH
     assert address_to_public_key_hash(BITCOIN_ADDRESS_COMPRESSED) == PUBKEY_HASH_COMPRESSED
+    with pytest.raises(ValueError):
+        address_to_public_key_hash(BITCOIN_ADDRESS_PAY2SH)
+    with pytest.raises(ValueError):
+        address_to_public_key_hash(BITCOIN_ADDRESS_TEST_PAY2SH)
diff --git a/tests/test_wallet.py b/tests/test_wallet.py
index 44d882d..6fbbfdd 100644
--- a/tests/test_wallet.py
+++ b/tests/test_wallet.py
@@ -238,6 +238,21 @@ class TestPrivateKeyTestnet:
 
         assert current > initial
 
+    def test_send_pay2sh(self):
+        """
+        We don't yet support pay2sh, so we must throw an exception if we get one.
+        Otherwise, we could send coins into an unrecoverable blackhole, needlessly.
+        pay2sh addresses begin with 2 in testnet and 3 on mainnet.
+        """
+        if TRAVIS and sys.version_info[:2] != (3, 6):
+            return
+
+        private_key = PrivateKeyTestnet(WALLET_FORMAT_COMPRESSED_TEST)
+        private_key.get_unspents()
+
+        with pytest.raises(ValueError):
+            private_key.send([('2NFKbBHzzh32q5DcZJNgZE9sF7gYmtPbawk', 1, 'jpy')])
+
     def test_cold_storage(self):
         if TRAVIS and sys.version_info[:2] != (3, 6):
             return