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

Enable various p2sh-p2wpkh functionality #11089

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@luke-jr
Member

luke-jr commented Aug 18, 2017

A subset of #9017

Show outdated Hide outdated src/base58.cpp Outdated
@MeshCollider

This comment has been minimized.

Show comment
Hide comment
@MeshCollider
Member

MeshCollider commented Aug 19, 2017

utACK 7fb9887

@fanquake fanquake added the Wallet label Aug 20, 2017

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs
Member

instagibbs commented Aug 21, 2017

utACK 8f6765a

@@ -281,6 +281,15 @@ bool CBitcoinAddress::GetKeyID(CKeyID& keyID) const
return true;
}
bool CBitcoinAddress::GetScriptID(CScriptID& scriptID) const
{
if (!IsValid() || vchVersion != Params().Base58Prefix(CChainParams::SCRIPT_ADDRESS)) {

This comment has been minimized.

@jimpo

jimpo Aug 23, 2017

Contributor

nit: Use if (!IsScript())

@jimpo

jimpo Aug 23, 2017

Contributor

nit: Use if (!IsScript())

@laanwj laanwj added this to the 0.15.1 milestone Aug 23, 2017

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Aug 23, 2017

Member

Added 0.15.1 milestone

Member

laanwj commented Aug 23, 2017

Added 0.15.1 milestone

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Aug 23, 2017

Member

IMO it's a 0.16 thing. IMO segwit-interim-release ought to have been called 0.16, but apparently it's 0.15.1. I don't mind in that case.

Member

luke-jr commented Aug 23, 2017

IMO it's a 0.16 thing. IMO segwit-interim-release ought to have been called 0.16, but apparently it's 0.15.1. I don't mind in that case.

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs

instagibbs Sep 7, 2017

Member

needs rebase

Member

instagibbs commented Sep 7, 2017

needs rebase

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Sep 24, 2017

Member

This needs a rebase after CBitcoinAddress was removed in #11117. However, this will become much easier after #11167, as then native witness destinations can be used.

Also, I'm not convinced that dumpprivkey for (p2sh or not) segwit addresses is the right approach, as you can't expect importprivkey to work for those.

Member

sipa commented Sep 24, 2017

This needs a rebase after CBitcoinAddress was removed in #11117. However, this will become much easier after #11167, as then native witness destinations can be used.

Also, I'm not convinced that dumpprivkey for (p2sh or not) segwit addresses is the right approach, as you can't expect importprivkey to work for those.

@sipa sipa referenced this pull request Sep 26, 2017

Merged

SegWit wallet support #11403

@laanwj laanwj removed this from Blockers in High-priority for review Sep 28, 2017

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Oct 4, 2017

Member

Needs rebase

Member

MarcoFalke commented Oct 4, 2017

Needs rebase

@promag

This comment has been minimized.

Show comment
Hide comment
@promag

promag Oct 17, 2017

Member

Either close (because it's in #11403) or rebase?

Member

promag commented Oct 17, 2017

Either close (because it's in #11403) or rebase?

@MarcoFalke MarcoFalke removed this from the 0.15.2 milestone Nov 9, 2017

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Nov 9, 2017

Member

Removing from backport

Member

MarcoFalke commented Nov 9, 2017

Removing from backport

jonasschnelli added a commit that referenced this pull request Jan 11, 2018

Merge #11403: SegWit wallet support
b224a47 Add address_types test (Pieter Wuille)
7ee54fd Support downgrading after recovered keypool witness keys (Pieter Wuille)
940a219 SegWit wallet support (Pieter Wuille)
f37c64e Implicitly know about P2WPKH redeemscripts (Pieter Wuille)
57273f2 [test] Serialize CTransaction with witness by default (Pieter Wuille)
cf2c0b6 Support P2WPKH and P2SH-P2WPKH in dumpprivkey (Pieter Wuille)
37c03d3 Support P2WPKH addresses in create/addmultisig (Pieter Wuille)
3eaa003 Extend validateaddress information for P2SH-embedded witness (Pieter Wuille)
30a27dc Expose method to find key for a single-key destination (Pieter Wuille)
985c795 Improve witness destination types and use them more (Pieter Wuille)
cbe1974 [refactor] GetAccount{PubKey,Address} -> GetAccountDestination (Pieter Wuille)
0c8ea63 Abstract out IsSolvable from Witnessifier (Pieter Wuille)

Pull request description:

  This implements a minimum viable implementation of SegWit wallet support, based on top of #11389, and includes part of the functionality from #11089.

  Two new configuration options are added:
  * `-addresstype`, with options `legacy`, `p2sh`, and `bech32`. It controls what kind of addresses are produced by `getnewaddress`, `getaccountaddress`, and `createmultisigaddress`.
  * `-changetype`, with the same options, and by default equal to `-addresstype`, that controls what kind of change is used.

  All wallet private and public keys can be used for any type of address. Support for address types dependent on different derivation paths will need a major overhaul of how our internal detection of outputs work. I expect that that will happen for a next major version.

  The above also applies to imported keys, as having a distinction there but not for normal operations is a disaster for testing, and probably for comprehension of users. This has some ugly effects, like needing to associate the provided label to `importprivkey` with each style address for the corresponding key.

  To deal with witness outputs requiring a corresponding redeemscript in wallet, three approaches are used:
  * All SegWit addresses created through `getnewaddress` or multisig RPCs explicitly get their redeemscripts added to the wallet file. This means that downgrading after creating a witness address will work, as long as the wallet file is up to date.
  * All SegWit keys in the wallet get an _implicit_ redeemscript added, without it being written to the file. This means recovery of an old backup will work, as long as you use new software.
  * All keypool keys that are seen used in transactions explicitly get their redeemscripts added to the wallet files. This means that downgrading after recovering from a backup that includes a witness address will work.

  These approaches correspond to solutions 3a, 1a, and 5a respectively from https://gist.github.com/sipa/125cfa1615946d0c3f3eec2ad7f250a2. As argued there, there is no full solution for dealing with the case where you both downgrade and restore a backup, so that's also not implemented.

  `dumpwallet`, `importwallet`, `importmulti`, `signmessage` and `verifymessage` don't work with SegWit addresses yet. They're remaining TODOs, for this PR or a follow-up. Because of that, several tests unexpectedly run with `-addresstype=legacy` for now.

Tree-SHA512: d425dbe517c0422061ab8dacdc3a6ae47da071450932ed992c79559d922dff7b2574a31a8c94feccd3761c1dffb6422c50055e6dca8e3cf94a169bc95e39e959
@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Mar 2, 2018

Member

I don't know how to rebase this.

Member

luke-jr commented Mar 2, 2018

I don't know how to rebase this.

@luke-jr luke-jr closed this Mar 2, 2018

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Mar 2, 2018

Member

Isn't all the functionality in this PR merged through other patches? If something isn't, I'd like to know.

Member

sipa commented Mar 2, 2018

Isn't all the functionality in this PR merged through other patches? If something isn't, I'd like to know.

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs

instagibbs Mar 2, 2018

Member
Member

instagibbs commented Mar 2, 2018

@WorkShop-Office

This comment has been minimized.

Show comment
Hide comment
@WorkShop-Office

WorkShop-Office Mar 2, 2018

Is there any bitcoin indonesia here

WorkShop-Office commented Mar 2, 2018

Is there any bitcoin indonesia here

@WorkShop-Office

This comment has been minimized.

Show comment
Hide comment
@WorkShop-Office

WorkShop-Office Mar 2, 2018

dTurn me in. Closed there what ya with me do not so sacred

WorkShop-Office commented Mar 2, 2018

dTurn me in. Closed there what ya with me do not so sacred

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Mar 2, 2018

Member

This had 4 changes:

  • "pubkey" from DescribeAddressVisitor acting on a P2SH-wrapped Segwit address - This seems to be handled recursively or something now (although I don't see logic to prevent P2SH-within-P2SH, but that'd be an unrelated bug)
  • validateaddress RPC gets metadata for 1-prefix addresses from witness addresses using it (if I'm reading the code correctly) - I don't see this in the 0.16 code, but it seems like a bad idea anyway.
  • _createmultisig_redeemScript looked up full pubkey from Segwit addresses - Seems to be correctly handled in a very different way now.
  • dumpprivkey from witness address - Seems to be correctly handled in a different way now.
Member

luke-jr commented Mar 2, 2018

This had 4 changes:

  • "pubkey" from DescribeAddressVisitor acting on a P2SH-wrapped Segwit address - This seems to be handled recursively or something now (although I don't see logic to prevent P2SH-within-P2SH, but that'd be an unrelated bug)
  • validateaddress RPC gets metadata for 1-prefix addresses from witness addresses using it (if I'm reading the code correctly) - I don't see this in the 0.16 code, but it seems like a bad idea anyway.
  • _createmultisig_redeemScript looked up full pubkey from Segwit addresses - Seems to be correctly handled in a very different way now.
  • dumpprivkey from witness address - Seems to be correctly handled in a different way now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment