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

Full support for native segwit. #1563

Merged
merged 1 commit into from Feb 11, 2019

Conversation

Projects
None yet
6 participants
@schildbach
Copy link
Member

schildbach commented Apr 21, 2018

This allows to create wallets with a DeterministicKeyChain that derives native segwit addresses and receives on them. By default, new wallets are still created with a legacy P2PKH key-chain.

  • Hierarchical-deterministic derivation of native segwit addresses.
  • Receive payments to native segwit addresses.
  • Spend and sign payments from native segwit addresses.
  • Watch-only wallets with native segwit addresses (zpub/vpub).

There is currently no automatic migration from P2PKH to P2WPKH wallets. Either you add a second DeterministicKeyChain via API or you create a new Wallet(NetworkParams, Script.ScriptType.P2WPKH).

Be aware this adds a new field in the wallet protobuf: output_script_type in Key, which keeps track of the script type of DeterministicKeyChains. Protobufs will be migrated; old DeterministicKeyChains are assumed to be of type P2PKH.

Warning: As of today, I have only tested this excessively on testnet. So for now only use with testnet or really small amounts!

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 21, 2018

Coverage Status

Coverage decreased (-0.01%) to 66.919% when pulling ee6660a on schildbach:segwit-output-script-type into c3d162d on bitcoinj:master.

@schildbach schildbach added this to the 0.15 milestone Aug 9, 2018

@schildbach schildbach force-pushed the schildbach:segwit-output-script-type branch 3 times, most recently from e4d5a23 to a708ad8 Oct 19, 2018

@schildbach schildbach force-pushed the schildbach:segwit-output-script-type branch from a708ad8 to d9989f7 Dec 13, 2018

@schildbach schildbach referenced this pull request Dec 13, 2018

Closed

SegWit wallet features #1672

@schildbach schildbach force-pushed the schildbach:segwit-output-script-type branch from d9989f7 to 79ac65b Dec 13, 2018

@lontivero

This comment has been minimized.

Copy link

lontivero commented Dec 14, 2018

@schildbach i would like to help you adding support for segwit tx and bech32 addresses support but I don't know where to start, there are a couple of PR trying to do the same it seems. How could I help?

@schildbach schildbach force-pushed the schildbach:segwit-output-script-type branch from 79ac65b to 896df4b Jan 4, 2019

@schildbach schildbach force-pushed the schildbach:segwit-output-script-type branch 6 times, most recently from 53442ac to 0a67293 Jan 25, 2019

@schildbach

This comment has been minimized.

Copy link
Member Author

schildbach commented Jan 26, 2019

I rebased this PR on current master, and implemented proper base58 de/serialization of P2WPKH account keys (zpub, zprv rather than xpub, xprv). We're getting closer (-:

@schildbach schildbach force-pushed the schildbach:segwit-output-script-type branch from 0a67293 to 4b6beb7 Jan 26, 2019

@schildbach schildbach force-pushed the schildbach:segwit-output-script-type branch 9 times, most recently from b3bc7f3 to 82dbee4 Jan 29, 2019

@schildbach schildbach force-pushed the schildbach:segwit-output-script-type branch from 4fa8b51 to 9a0bbd3 Feb 2, 2019

@schildbach schildbach changed the title Full support for native segwit addresses. Full support for native segwit. Feb 2, 2019

@schildbach

This comment has been minimized.

Copy link
Member Author

schildbach commented Feb 2, 2019

Great progress on the segwit front: This PR is ready to merge! Please review and test, and most importantly, please report your findings. Also please report if you found no problems. I've updated the first post on this PR.

@nopara73

This comment has been minimized.

Copy link

nopara73 commented Feb 3, 2019

Tests pass, Windows 10, IntelliJ.

image

@schildbach schildbach force-pushed the schildbach:segwit-output-script-type branch 4 times, most recently from 169ec23 to 52b059e Feb 3, 2019

// m / 44' / 0' / 0'
public static final ImmutableList<ChildNumber> BIP44_ACCOUNT_ZERO_PATH = ImmutableList.of(new ChildNumber(44, true),
ChildNumber.ZERO_HARDENED, ChildNumber.ZERO_HARDENED);
public static final ImmutableList<ChildNumber> EXTERNAL_SUBPATH = ImmutableList.of(ChildNumber.ZERO);
public static final ImmutableList<ChildNumber> INTERNAL_SUBPATH = ImmutableList.of(ChildNumber.ONE);

private static ImmutableList<ChildNumber> defaultAccountPathFor(Script.ScriptType outputScriptType) {

This comment has been minimized.

@lontivero

lontivero Feb 4, 2019

Why is this? Is it for bip49?

This comment has been minimized.

@schildbach

schildbach Feb 4, 2019

Author Member

No. BIP49 is out of scope, as it deals with P2SH-P2WPKH only.

This comment has been minimized.

@schildbach

schildbach Feb 4, 2019

Author Member

If you look at the implementation of the method, there is one path for P2PKH (the default old one) and another for the new P2WPKH output script type.

This comment has been minimized.

@lontivero

lontivero Feb 4, 2019

Yes, and that's the part that I don't understand (I am not very familiar with the derivation schemes). Why is a different account for P2PKH and P2WPKH?

This comment has been minimized.

@schildbach

schildbach Feb 4, 2019

Author Member

It's meant to keep the different script types separated.

This comment has been minimized.

@schildbach

schildbach Feb 11, 2019

Author Member

@lontivero Also see #1563 (comment); I moved the method into a pluggable interface.

@lontivero

This comment has been minimized.

Copy link

lontivero commented Feb 5, 2019

@schildbach Ive finished my review and it looks great. I also compared the test related to segwit (no in this PR) against the test vectors in the bips and nbitcoin and they are okay (in fact I had to update the test vectors for bip173 in nbitcoin as result of this work).

Bitcoinj is huge and contains many concepts that I was not familiar with so, there are part of the code that I just couldn't review properly because of my lack of context. There are also some test data files (the deterministic-(sic)-serialization.txt files) that I don't know how to validate, I guess that I could load it with nbitcoin and check they pass too...

Please let me know if there is something else I can do.

@schildbach schildbach force-pushed the bitcoinj:master branch 2 times, most recently from 2604b6e to 53a63c4 Feb 6, 2019

@schildbach schildbach force-pushed the schildbach:segwit-output-script-type branch from 52b059e to a3974a2 Feb 6, 2019

@oscarguindzberg

This comment has been minimized.

Copy link
Contributor

oscarguindzberg commented Feb 6, 2019

Would you confirm this is the case: MarriedKeyChain ability to work with P2WSH is still to be implemented. MarriedKeyChain still works fine with legacy P2SH.

@oscarguindzberg

This comment has been minimized.

Copy link
Contributor

oscarguindzberg commented Feb 6, 2019

There is currently no automatic migration from P2PKH to P2WPKH wallets. Either you add a second DeterministicKeyChain via API or you create a new Wallet(NetworkParams, Script.ScriptType.P2WPKH).

Since bech32 addresses are not supported everywhere yet I assume many apps using bitcoinj would want to be able to expose both types of addresses (1xxx and bc1xxx) to users.

I see that could be problematic using any of the 2 approaches you suggest. Adding a second DeterministicKeyChain is problematic since KeyChainGroup.getActiveKeyChain() is used a lot in Wallet and KeyChainGroup. Having 2 wallets forces apps to store 2 files and it would be hard to combine utxos from the 2 wallets when spending.

Please, let me know if I am missing something here.

@schildbach

This comment has been minimized.

Copy link
Member Author

schildbach commented Feb 7, 2019

@lontivero Thanks for your review, it's much appreciated and useful.

@oscarguindzberg Yes, for P2WSH we only have some building blocks but no usecase. The tests for MarriedKeyChain still work, but I didn't try it manually.

Yes, the migration is still todo. I planned to provide a KeyChainGroup.getActiveKeyChain(Script.ScriptType) variant so that you can have two active keychains (one for each script type). And in Wallet maybe only provide similar getCurrentAddress() and getFreshAddress() variants?

For determinism purposes, I'd like to stick to this upgrade path:

Basic Keychain --> P2PKH keychain --> P2WPKH keychain

Then for old wallets that get migrated to P2WPKH a fallback P2PKH keychain is always available, but unfortunately for a newly created P2WPKH there would be no downgrade path to P2PKH. I currently don't know how to deal with this case or maybe just don't care.

@schildbach schildbach force-pushed the schildbach:segwit-output-script-type branch 2 times, most recently from 63f7ff7 to 5746c35 Feb 8, 2019

@schildbach

This comment has been minimized.

Copy link
Member Author

schildbach commented Feb 11, 2019

I have pushed another iteration of this, hopefully the last before I can merged this very soon.

  • KeyChainGroups are now created using a builder. This is actually already merged to master, but this PR now builds on top of it.
  • There is a new interface KeyChainGroupStructure which currently defines one method ImmutableList<ChildNumber> accountPathFor(Script.ScriptType outputScriptType), and a default implementation for BIP32-structure-style wallets. I hope this will make it easier to support other structures.

@schildbach schildbach force-pushed the schildbach:segwit-output-script-type branch from 5746c35 to f68a646 Feb 11, 2019

Receive to and send from native segwit addresses
- Hierarchical-deterministic derivation of native segwit addresses.
- Receive payments to native segwit addresses.
- Spend and sign payments from native segwit addresses.
- Watch-only wallets with native segwit addresses (zpub/vpub).
- WalletAppKit, Wallet-tool and Wallet-template are taught to deal with segwit-enabled wallets.

Be aware this adds a new field in the wallet protobuf: output_script_type in Key, which keeps track
of the script type of DeterministicKeyChains. Protobufs will be migrated; old DeterministicKeyChains
are assumed to be of type P2PKH.

Includes some code by Fabrice Drouin.

@schildbach schildbach force-pushed the schildbach:segwit-output-script-type branch from f68a646 to bfe2a19 Feb 11, 2019

@schildbach schildbach merged commit bfe2a19 into bitcoinj:master Feb 11, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@schildbach

This comment has been minimized.

Copy link
Member Author

schildbach commented Feb 11, 2019

Merged – yipiieh!

@githorray

This comment has been minimized.

Copy link

githorray commented Feb 12, 2019

Congratulations!!! Lots of upstreams folks can't put off segwit support anymore! ;-)

@jonathancross jonathancross referenced this pull request Feb 12, 2019

Closed

Segwit #1618

@schildbach schildbach deleted the schildbach:segwit-output-script-type branch Feb 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.