SegWit wallet support #11403

sipa · 2017-09-26T01:49:42Z

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.

promag

Light review, it would be nice to push base PR's.

promag · 2017-09-26T08:52:45Z

src/wallet/wallet.cpp

In commit Expose method to find key for a single-key destination:

Use boost::apply_visitor instead?

I tried that, but the result is much longer and harder to get right (see below). It's also the wrong approach I think. A visitor is used when you want to handle all possible types - here we specifically only want to support a specific subset of cases (P2PKH, P2WPKH, P2SH-P2WPKH).

promag · 2017-09-26T08:55:24Z

src/wallet/wallet.cpp

In commit Expose method to find key for a single-key destination:

Recursive call?

return GetKeyForDestination(inner_dest);

It's not that easy, as it could match a superset (for example P2SH-P2PKH), or even invalid things (P2SH-P2SH-P2WPKH?).

Right.

Isn't boost::get kind of expensive? Avoid 2nd call?

I think it's very cheap - look at the tag, and return the argument with a cast.

promag · 2017-09-26T09:00:02Z

src/wallet/wallet.cpp

In commit SegWit wallet support:

Remove?

promag · 2017-09-26T11:58:03Z

src/wallet/init.cpp

Add init tests (follow up maybe)?

Will add those later.

promag · 2017-09-26T12:00:16Z

src/wallet/rpcwallet.cpp

Missing default value?

promag · 2017-09-26T12:02:11Z

src/wallet/rpcwallet.cpp

Missing test for this error.

Will add later.

promag · 2017-09-26T12:02:28Z

src/wallet/rpcwallet.cpp

Default value?

promag · 2017-09-26T12:03:46Z

src/wallet/init.cpp

Add init tests (follow up maybe)?

Will add those later.

promag · 2017-09-26T12:04:26Z

src/wallet/rpcwallet.cpp

Missing test.

promag · 2017-09-26T12:07:53Z

test/functional/segwit.py

Remove , True so that default value is tested.

luke-jr · 2017-09-26T20:21:36Z

src/wallet/init.cpp

These should mention the defaults, which likely means refactoring how the "default" string is handled.

Added explanation of defaults.

luke-jr · 2017-09-26T20:22:30Z

src/wallet/wallet.cpp

Default should probably remain legacy until support for newer styles is complete (eg, import* RPC)?

I'd rather not. This makes the existing tests far more powerful.

Tests don't need to (and probably shouldn't) rely on defaults...

Yes and no.

I think that many tests, which are primarily testing things unrelated to addresses, should use the default address type. They don't strictly rely on them, but it's better if they work with either type, and it's certainly easier to not need to change all of them.

luke-jr · 2017-09-26T20:25:56Z

src/wallet/rpcwallet.cpp

Seems like it would be better to change the deprecated "account" into a JSON Object options.

I think that should be done separately.

petertodd · 2017-09-28T19:12:15Z

A potential problem with supporting -addressstyle=segwit w/ Bech32 on day zero is it may confuse people into thinking segwit needs the new address format. OTOH, if we don't, it probably makes the P2SH adoption hell more likely.

flack · 2017-09-28T21:08:38Z

maybe it should be renamed to -addressstyle=bech32?

8213838 [Qt] tolerate BIP173/bech32 addresses during input validation (Jonas Schnelli) 06eaca6 [RPC] Wallet: test importing of native witness scripts (NicolasDorier) fd0041a Use BIP173 addresses in segwit.py test (Pieter Wuille) e278f12 Support BIP173 in addwitnessaddress (Pieter Wuille) c091b99 Implement BIP173 addresses and tests (Pieter Wuille) bd355b8 Add regtest testing to base58_tests (Pieter Wuille) 6565c55 Convert base58_tests from type/payload to scriptPubKey comparison (Pieter Wuille) 8fd2267 Import Bech32 C++ reference code & tests (Pieter Wuille) 1e46ebd Implement {Encode,Decode}Destination without CBitcoinAddress (Pieter Wuille) Pull request description: Builds on top of #11117. This adds support for: * Creating BIP173 addresses for testing (through `addwitnessaddress`, though by default it still produces P2SH versions) * Sending to BIP173 addresses (including non-v0 ones) * Analysing BIP173 addresses (through `validateaddress`) It includes a reformatted version of the [C++ Bech32 reference code](https://github.com/sipa/bech32/tree/master/ref/c%2B%2B) and an independent implementation of the address encoding/decoding logic (integrated with CTxDestination). All BIP173 test vectors are included. Not included (and intended for other PRs): * Full wallet support for SegWit (which would include automatically adding witness scripts to the wallet during automatic keypool topup, SegWit change outputs, ...) [see #11403] * Splitting base58.cpp and tests/base58_tests.cpp up into base58-specific code, and "address encoding"-code [see #11372] * Error locating in UI for BIP173 addresses. Tree-SHA512: 238031185fd07f3ac873c586043970cc2db91bf7735c3c168cb33a3db39a7bda81d4891b649685bb17ef90dc63af0328e7705d8cd3e8dafd6c4d3c08fb230341

meshcollider · 2017-09-29T11:57:57Z

Needs rebase after #11167 merge but that'll make it clearer what's new here to review 👍

instagibbs · 2017-09-29T16:23:36Z

src/rpc/misc.cpp

This field needs to be added anywhere it's used in help text for RPC calls.

You might want to copy the field to the help text rather than move it, though :p

(Use after move here)

instagibbs · 2017-09-29T16:25:13Z

src/wallet/wallet.h

Remove plurals from description.

sipa · 2017-09-29T18:36:44Z

Rebased after #11167 merge.

instagibbs · 2017-09-29T20:28:13Z

src/wallet/wallet.cpp

pedantic nit, but this only supports v0 of each witness version

I've always considered P2WPKH as specifically referring to v0 20-byte witness programs, and P2WSH as v0 32-byte ones. We'll need to come up with even clumsier names later ;)

instagibbs · 2017-09-29T20:49:11Z

src/wallet/rpcwallet.cpp

startup options strings being different than getnewaddress options is a bit odd to me?

'p2sh_p2wpkh' vs p2sh etc

Fixed. Standardized on 'legacy', 'p2sh', and 'bech32'. It's not entirely accurate, as P2SH is a scriptPubKey type and not its address encoding, but meh - they're probably the most recognizable names.

instagibbs · 2017-09-29T20:51:49Z

src/wallet/wallet.cpp

No solution off the top of my head, but it makes sense that this would be set per-wallet. If not, could we name it g_address_style?

Fixed as a global now, as we don't really have a way to configure it per wallet. Can be revised if there ever is.

instagibbs · 2017-09-29T20:55:21Z

src/wallet/init.cpp

changestyle needs a test

instagibbs · 2017-09-29T20:56:35Z

src/wallet/rpcwallet.cpp

argument needs a test

instagibbs · 2017-09-29T20:59:35Z

src/wallet/rpcwallet.cpp

change seems way out of place here

instagibbs · 2017-09-29T21:02:18Z

src/wallet/wallet.cpp

Same suggestion as address_style

gmaxwell · 2017-10-11T20:02:48Z

Regarding petertodd's concern, I think something like flack's suggestion would address it.

kallewoof

utACK 5632374f2f71bca384ceed07ca6842fd87102346

kallewoof · 2017-10-12T04:38:39Z

src/wallet/wallet.cpp

The help for e.g. getnewaddress says "p2wpkh" etc. One or the other needs to be changed.

Fixed, I think it's consistent now.

kallewoof · 2017-10-12T04:49:26Z

src/wallet/rpcwallet.cpp

I think you want "getnewaddress ( \"account\" \"style\" )\n"

luke-jr · 2017-10-12T05:56:19Z

src/wallet/rpcwallet.cpp

This shouldn't just be tacked on the end. Accounts are deprecated. Just replace "account" with an options Object.

That would be API breaking for a lot of parties which is at odds with rapid deployment.

#11403 (comment)

This shouldn't just be tacked on the end. Accounts are deprecated. Just replace "account" with an options Object.

In this particular context, account will just be renamed to label, rather than removed entirely. (At least this is what #11536 does.)

theuni · 2017-10-17T20:02:36Z

src/script/standard.h

You could instead inherit the ctors from uint160/uint256 to shortcut the construction from WitnessV0KeyHash(uint160(foo) to WitnessV0KeyHash(foo):

using uint160::uint160;

Cool, done.

theuni

Added a few more comments.

I suspect I'm missing something about the generic-ish functions in CWallet. Do you maybe have plans to make those per-wallet as @instagibbs suggested?

theuni · 2017-10-17T20:26:40Z

src/rpc/misc.cpp

Why dupe this? Helps with existing tooling?

Yes, tests that expect to see a pubkey field when calling validateaddress on the output of getnewaddress.

theuni · 2017-10-17T21:02:22Z

src/wallet/wallet.cpp

imo, it's clearer (and less redundant) to test the result of the assignment:

if (const CKeyID* ret = boost::get<CKeyID>(&dest)) { return *ret; }

Edit: That's also the common paradigm for using std::weak_ptr::lock()

theuni · 2017-10-17T21:28:51Z

src/keystore.cpp

I'm not sure about the sequencing rules here. I'd feel safer with a temporary CScriptID.

Ugh, thanks for pointing that out.

I'm not sure about the sequencing rules here. I'd feel safer with a temporary CScriptID.

For the record, there should be no issue here. std::move just does an rvalue reference cast to influence template deduction and function overloading. It doesn't actually actually move anything or do anything else at runtime.

theuni · 2017-10-17T22:09:59Z

src/wallet/wallet.cpp

Can't this pass in nullptr and move out of CWallet? Unless I'm missing something, it's not actually related.

Moved to script/signer.

theuni · 2017-10-17T22:18:10Z

src/wallet/wallet.cpp

If IsSolvable moves out of CWallet, I believe these can too.

Done. IsSolvable is moved to script/sign, GetDestinationForKey is moved to keystore.

theuni · 2017-10-17T22:30:54Z

src/wallet/wallet.cpp

Seems the caller should be responsible for adding this if it's a WitnessV0ScriptHash? Like the others, this doesn't seem like wallet functionality to me.

Yes, it's ugly. I don't immediately see a nicer solution though, except for a callback to pass in, or duplicating part of this function just to find the correct program.

Note that if it weren't for the implicitly-know-about-witness-versions-of-keys logic, an equivalent line would be needed in GetDestinationForKey or its callers.

Sjors · 2017-10-20T12:28:22Z

The wallet probably needs to be able to generate both p2sh and bech32 addresses, depending on who the user needs to communicate the address to. In fact, the UI might need to display both at the same time.

For example, I might send an email requesting payment to a bech32 address, and telling the recipient to use the p2sh address only if their wallet doesn't support the new format. A backwards compatible QR code could contain (admittedly ad hoc): bitcoin:3ba1...?amount=0.01&bech32=bc1qw5....

So rather than -addressstyle, isn't it better to make address style an argument for calls to getnewaddress and createmultisigaddress?

I think this was brought up elsewhere, but If bech32 is the default, wouldn't that confuse RPC consumers, especially if they do any input validation? p2sh seems like a safer default.
Alternatively, perhaps the default can be bech32, but with ~~a configure flag~~ command line option to use a different default (and a warning in the release notes). It would be nice if RPC calls were versioned.

sipa · 2017-10-20T12:30:32Z

@Sjors There is an RPC argument added to address-creating RPCs to override the default.

Sjors · 2017-10-20T12:40:49Z

@sipa I see, perhaps -addressstyle should be -addressstyledefault instead to make that more clear?

… GetAccountDestination

@TheBlueMatt

… safety 9c8eca7 Split up key and script metadata for better type safety (Russell Yanofsky) Pull request description: Suggested by @TheBlueMatt bitcoin#11403 (comment) Combining the maps was probably never a good arrangement but is more problematic now in presence of WitnessV0ScriptHash and WitnessV0KeyHash types. Tree-SHA512: 9263e9c01090fb49221e91d88a88241a9691dda3e92d86041c8e284306a64d3af5e2438249f9dcc3e6e4a5c11c1a89f975a86d55690adf95bf2636f15f99f92a

Summary: Improvements and cleanups by John Newbery This is from [[bitcoin/bitcoin#11403 | PR11403]] : bitcoin/bitcoin@b224a47 We initially decided not to backport it because we don't have this feature, but it turns out it exercice various parts of the code and is useful. In addition, it makes backport easier. Also include a trick similar to [[bitcoin/bitcoin#17124 | PR17124]] to significantly speedup the test. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D6131

backport: bitcoin#10244, bitcoin#18123, bitcoin#12906 + Parts of bitcoin#11403

@TheBlueMatt

9c8eca7 Split up key and script metadata for better type safety (Russell Yanofsky) Pull request description: Suggested by @TheBlueMatt bitcoin/bitcoin#11403 (comment) Combining the maps was probably never a good arrangement but is more problematic now in presence of WitnessV0ScriptHash and WitnessV0KeyHash types. Tree-SHA512: 9263e9c01090fb49221e91d88a88241a9691dda3e92d86041c8e284306a64d3af5e2438249f9dcc3e6e4a5c11c1a89f975a86d55690adf95bf2636f15f99f92a

@TheBlueMatt

… safety 9c8eca7 Split up key and script metadata for better type safety (Russell Yanofsky) Pull request description: Suggested by @TheBlueMatt bitcoin#11403 (comment) Combining the maps was probably never a good arrangement but is more problematic now in presence of WitnessV0ScriptHash and WitnessV0KeyHash types. Tree-SHA512: 9263e9c01090fb49221e91d88a88241a9691dda3e92d86041c8e284306a64d3af5e2438249f9dcc3e6e4a5c11c1a89f975a86d55690adf95bf2636f15f99f92a

… GetAccountDestination

fanquake added the Wallet label Sep 26, 2017

promag reviewed Sep 26, 2017

View reviewed changes

sipa mentioned this pull request Sep 26, 2017

Full BIP173 (Bech32) support #11167

Merged

luke-jr reviewed Sep 26, 2017

View reviewed changes

jl2012 mentioned this pull request Sep 27, 2017

Hardcode CSV and SEGWIT deployment #11398

Closed

instagibbs reviewed Sep 29, 2017

View reviewed changes

src/wallet/wallet.h Outdated

Copy link

Member

instagibbs Sep 29, 2017

Choose a reason for hiding this comment

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

Remove plurals from description.

sipa force-pushed the 201709_segwitwallet2 branch from a3a79cd to 20da344 Compare September 29, 2017 18:36

instagibbs reviewed Sep 29, 2017

View reviewed changes

sipa force-pushed the 201709_segwitwallet2 branch from 20da344 to 5632374 Compare September 29, 2017 23:50

Sjors mentioned this pull request Oct 4, 2017

"Warning: Unknown Change Address" when using P2SH-P2WPKH wallet address #11451

Closed

laanwj mentioned this pull request Oct 5, 2017

Support having SegWit always active in regtest (sipa, ajtowns, jnewbery) #11389

Merged

maflcko added this to the 0.15.1 milestone Oct 5, 2017

maflcko added the Needs backport label Oct 5, 2017

kallewoof mentioned this pull request Oct 12, 2017

[wallet] sendtoaddress output type argument #11489

Closed

kallewoof approved these changes Oct 12, 2017

View reviewed changes

luke-jr reviewed Oct 12, 2017

View reviewed changes

sipa force-pushed the 201709_segwitwallet2 branch from 5632374 to f542dae Compare October 14, 2017 02:00

theuni reviewed Oct 17, 2017

View reviewed changes

promag mentioned this pull request Oct 18, 2017

Enable various p2sh-p2wpkh functionality #11089

Closed

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 27, 2020

Partial Merge bitcoin#11403: [refactor] GetAccount{PubKey,Address} ->…

9cc5078

… GetAccountDestination

xdustinface added a commit to xdustinface/dash that referenced this pull request Oct 14, 2020

Partially backport bitcoin#11403

7ab1051

UdjinM6 added a commit to dashpay/dash that referenced this pull request Oct 14, 2020

Merge pull request #3682 from xdustinface/backport-10244-18123

74f4d2a

backport: bitcoin#10244, bitcoin#18123, bitcoin#12906 + Parts of bitcoin#11403

xdustinface mentioned this pull request Oct 26, 2020

Backports 0.17 pr20 dashpay/dash#3742

Merged

UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request Jun 28, 2021

partial merge bitcoin#11403: Abstract out IsSolvable from Witnessifier

01fccc4

gades pushed a commit to cosanta/cosanta-core that referenced this pull request Jul 1, 2021

Partially backport bitcoin#11403

e343ae2

gades pushed a commit to cosanta/cosanta-core that referenced this pull request Feb 15, 2022

Partial Merge bitcoin#11403: [refactor] GetAccount{PubKey,Address} ->…

c28588f

… GetAccountDestination

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

SegWit wallet support #11403

SegWit wallet support #11403

Uh oh!

Conversation

sipa commented Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petertodd commented Sep 28, 2017

Uh oh!

flack commented Sep 28, 2017

Uh oh!

meshcollider commented Sep 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Sep 26, 2017 •

edited

Loading

instagibbs Sep 29, 2017 •

edited

Loading

sipa Nov 18, 2017 •

edited

Loading