Added p2wpkh-p2sh addresses support (segwit)

[madacol]

For BIP49 wallets I also needed to specify the path in the command-line like this --bip32-path "m/49'/0'/0'/0"

Travis checks fails getting the dependencies, nothing related to this PR

Related issues #295 #174 #221 #282 #276 #278

[jonathancross]

@madacol Thanks for creating this PR!
I am seeing a possible regression when testing the --addr-limit option -- it seems to be ignored now.
Is it possible your change could have broken this?

Test data

Example seed:

final round trust era topic march brain envelope spoon minimum bunker start

Passphrase: 12345

Here is the list of expected addresses:

p2wpkh-p2sh:

BIP32 path	address
m/49'/0'/0'/0/0	3G2rhsBYmP6RKrjW1kVE1kJB9qjwggaCZw
m/49'/0'/0'/0/1	3Bk7x41FSw5nKzbStAdCWnnq51CGXxLxUy
m/49'/0'/0'/0/2	36JgXRarbiw8fASvFaDNNg6LJdTPCUKdH9
m/49'/0'/0'/0/3	32NcnLxRqbJ4Z5HXospHYTf4PeTpjBtyBC
m/49'/0'/0'/0/4	3KurtNhsTjMjNCrp8PDEBZ7bpHnbh8W1sN

non-segwit P2PKH:

BIP32 path	address
m/44'/0'/0'/0/0	16s8RrnPZCdLcamu7ApSqKMagB9RXsGtmX
m/44'/0'/0'/0/1	1QBqRfDDUctpPa31ESTmtNjy2NJKmFhDTx
m/44'/0'/0'/0/2	1LNiMtUomHRiwct2XtVwqU8W96nCrLX4QQ
m/44'/0'/0'/0/3	1Kp4u6vfVRRwd9t8Zh2gDV2YxEJR5pkyo1

Demo of the issue

The first p2wpkh-p2sh address (/0) works as expected:

echo "12345" | ./btcrecover.py --bip39 --addr-limit 10 --passwordlist --addrs 3G2rhsBYmP6RKrjW1kVE1kJB9qjwggaCZw --bip32-path "m/49'/0'/0'/0"

Same for non-segwit P2PKH:

echo "12345" | ./btcrecover.py --bip39 --addr-limit 10 --passwordlist --addrs 16s8RrnPZCdLcamu7ApSqKMagB9RXsGtmX --bip32-path "m/44'/0'/0'/0"

However, any subsequent addresses (eg /1) are not found for either address type:

echo "12345" | ./btcrecover.py --bip39 --addr-limit 10 --passwordlist --addrs 3Bk7x41FSw5nKzbStAdCWnnq51CGXxLxUy --bip32-path "m/49'/0'/0'/0"

echo "12345" | ./btcrecover.py --bip39 --addr-limit 10 --passwordlist --addrs 1QBqRfDDUctpPa31ESTmtNjy2NJKmFhDTx --bip32-path "m/44'/0'/0'/0"

[madacol]

Thank's for reviewing.

Sorry I didn't notice that --addr-limit was being ignored, I can confirm, I'll check into that.

[madacol]

Done!
I Screwed up with the 2nd commit, reverted it and problem solved.

[jonathancross]

Thanks for the fix. Now seems to pass the (very basic) tests above as well as this:

echo -e "123\n1234\n12345" | ./btcrecover.py --bip39 --addr-limit 5 --passwordlist --addrs 3KurtNhsTjMjNCrp8PDEBZ7bpHnbh8W1sN --bip32-path "m/49'/0'/0'/0"

I don't consider myself qualified to do a proper review of the implementation, but I don't see any red flags. :-)

[jonathancross]

Nit: style here suggests adding a single space before and after !=

[madacol]

I did it on purpose to make it clearer where each condition begins and end, but i see now that you're right, I'm not following original style

[jonathancross]

LGTM
Hopefully @gurnec will review soon. :-)

[HardCorePawn]

Does this commit also factor in P2WPKH-P2SH addresses when generating the address database?

my understanding of the code here: https://github.com/gurnec/btcrecover/blob/master/btcrecover/addressset.py#L386-L389

    # If this is a P2PKH script (OP_DUP OP_HASH160 PUSH(20) <20 address bytes> OP_EQUALVERIFYOP_CHECKSIG)
    if pkscript_len == 25 and block[offset:offset+3] == b"\x76\xa9\x14" and block[offset+23:offset+25] == b"\x88\xac":
        # Add the discovered address to the address set
        address_set.add(block[offset+3:offset+23])

is that it will only add P2PKH addresses...

[madacol]

I'm not sure what you are talking about, but probably this PR does not do what you want. I've tried to make this PR as good as I can, but it's still a bit hacky.

How is that piece of code you mentioned used, or what is it used for?

[HardCorePawn]

One of the options that btcrecover provides is creating an "Address Database" from your Bitcoin Core blocks data for when you are unsure of the address you are looking for. Refer: https://github.com/gurnec/btcrecover/blob/master/docs/Seedrecover_Quick_Start_Guide.md#recovery-with-an-address-database

btcrecover then searches this database for the address(es) generated by a test seed.

However, the code above (used as part of the Address Database generation) only seems to find (and record) P2PKH addresses that are on the blockchain, It does not look like it will record P2WPKH-P2SH addresses.

So, if a user is attempting to recover a seed used to generate P2WPKH-P2SH addresses by using the Address Database option, it will never succeed, even if the correct seed is used.

[madacol]

Ok, I understand, but I didn't touch that part of the code.

If you are interested in making those changes yourself, this is the relevant code that allows P2SH(P2WPKH)

P2PKH_VERSION_BYTE = "\x00"
P2SH_VERSION_BYTE = "\x05"

if version_byte == P2SH_VERSION_BYTE: # assuming P2SH(P2WPKH) BIP141
    WITNESS_VERSION = "\x00\x14"
    witness_program = WITNESS_VERSION + pubkey_hash160
    witness_program_hash160 = hashlib.new("ripemd160", hashlib.sha256(witness_program).digest()).digest()
    if witness_program_hash160 == hash160:
        return True
else:   # defaults to P2PKH
    if pubkey_hash160 == hash160:
      return True

You can see from there that P2SH(P2WPKH) is the same P2PKH but with this extra steps:

    WITNESS_VERSION = "\x00\x14"
    witness_program = WITNESS_VERSION + pubkey_hash160
    witness_program_hash160 = hashlib.new("ripemd160", hashlib.sha256(witness_program).digest()).digest()

It took me many days of reading to realize that, hope it helps!

[jonathancross]

From what I can see, @gurnec was very active in the Bitcoin space until Dec 2017 (GitHub, SE), but has disappeared since then. Doesn't seem this is going to be merged. :-(

[jlopp]

It's a shame that this project was abandoned, especially with outstanding work completed. I think that it has been abandoned long enough to merit someone stepping up as maintainer of a new fork. https://twitter.com/lopp/status/1136641698145157122

[madacol]

I doubt to be qualified enough to maintain it, but I can help!

[Connor-Knabe]

@madacol thank you for adding this!! 🙌 I'll be sending some BTC your way when I get into my wallet. Send me over your address if you'd like. 😁

[3rdIteration]

Have merged this Segwit improvement (along with a temp AddressDB fix) and LTC address support over at https://github.com/3rdIteration/btcrecover/

Planning to push the whole thing to Python3 and then do maintenance from there...

[jlopp]

Thanks @3rdIteration - I've changed the link on my site over to your repo. https://www.lopp.net/bitcoin-information/developer-tools.html

[jonathancross]

Thanks @3rdIteration.
PS: I noticed that "Issues" are disabled on your fork... is this intentional?

[3rdIteration]

Unintentionally left off, fixed

…

On Sat, 9 Nov. 2019, 21:17 Jonathan Cross, ***@***.***> wrote: Thanks @3rdIteration <https://github.com/3rdIteration>. I noticed that "Issues" are disabled on your fork... is this intentional? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#302?email_source=notifications&email_token=AARAQLQL5AGCVHIQEWQG6PLQS2L5PA5CNFSM4F7YFRD2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDUDW2Q#issuecomment-552090474>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARAQLX3HZ6KB533M7LVNODQS2L5PANCNFSM4F7YFRDQ> .