SegWit wallet support

sipa commented at 1:49 am on September 26, 2017: member

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.

fanquake added the label Wallet on Sep 26, 2017

in src/wallet/wallet.cpp:4144 in 1ae94623ec outdated

4050@@ -4051,3 +4051,24 @@ bool CWallet::IsSolvable(const CScript& script) const
4051     SignatureData sigs;
4052     return (ProduceSignature(creator, script, sigs) && VerifyScript(sigs.scriptSig, script, &sigs.scriptWitness, MANDATORY_SCRIPT_VERIFY_FLAGS | SCRIPT_VERIFY_WITNESS_PUBKEYTYPE, creator.Checker()));
4053 }
4054+
4055+CKeyID CWallet::GetKeyForDestination(const CTxDestination& dest) const
4056+{
4057+    // Only supports P2PKH, P2WPKH, P2SH-P2WPKH.
4058+    if (boost::get<CKeyID>(&dest)) {

promag commented at 8:52 am on September 26, 2017:

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

Use boost::apply_visitor instead?

sipa commented at 6:06 pm on September 26, 2017:

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).

in src/wallet/wallet.cpp:4154 in 1ae94623ec outdated

4063+    }
4064+    if (boost::get<CScriptID>(&dest)) {
4065+        CScript script;
4066+        CTxDestination inner_dest;
4067+        if (GetCScript(boost::get<CScriptID>(dest), script) && ExtractDestination(script, inner_dest)) {
4068+            if (boost::get<WitnessV0KeyHash>(&inner_dest)) {

promag commented at 8:55 am on September 26, 2017:

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

Recursive call?

0            return GetKeyForDestination(inner_dest);

sipa commented at 6:07 pm on September 26, 2017:

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

promag commented at 9:56 am on September 28, 2017:

Right.

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

sipa commented at 0:50 am on November 18, 2017:

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

in src/wallet/wallet.cpp:4162 in a3a79cdb21 outdated

4073@@ -4072,3 +4074,82 @@ CKeyID CWallet::GetKeyForDestination(const CTxDestination& dest) const
4074     }
4075     return CKeyID();
4076 }
4077+
4078+static const std::string STYLE_STRING_NONE = "";

promag commented at 9:00 am on September 26, 2017:

In commit SegWit wallet support:

Remove?

sipa commented at 0:51 am on November 18, 2017:

Done.

in src/wallet/init.cpp:181 in a3a79cdb21 outdated

169@@ -168,6 +170,22 @@ bool WalletParameterInteraction()
170     bSpendZeroConfChange = gArgs.GetBoolArg("-spendzeroconfchange", DEFAULT_SPEND_ZEROCONF_CHANGE);
171     fWalletRbf = gArgs.GetBoolArg("-walletrbf", DEFAULT_WALLET_RBF);
172 
173+    std::string style = gArgs.GetArg("-addressstyle", "default");
174+    OutputStyle parsed_style = ParseStyle(style);
175+    if (parsed_style == STYLE_NONE) {
176+        return InitError(strprintf(_("Unknown address style '%s'"), style));

promag commented at 11:58 am on September 26, 2017:

Add init tests (follow up maybe)?

sipa commented at 0:41 am on November 18, 2017:

Will add those later.

in src/wallet/rpcwallet.cpp:243 in a3a79cdb21 outdated

240-            "getrawchangeaddress\n"
241+            "getrawchangeaddress ( \"style\" )\n"
242             "\nReturns a new Bitcoin address, for receiving change.\n"
243             "This is for use with raw transactions, NOT normal use.\n"
244+            "\nArguments:\n"
245+            "1. \"style\"          (string, optional) The address style to use. Valid options are 'p2pkh', 'p2sh_p2wpkh', and 'p2wpkh'\n"

promag commented at 12:00 pm on September 26, 2017:

Missing default value?

instagibbs commented at 8:56 pm on September 29, 2017:

argument needs a test

sipa commented at 0:42 am on November 18, 2017:

Added.

in src/wallet/rpcwallet.cpp:259 in a3a79cdb21 outdated

253@@ -243,16 +254,24 @@ UniValue getrawchangeaddress(const JSONRPCRequest& request)
254         pwallet->TopUpKeyPool();
255     }
256 
257+    OutputStyle style = change_style;
258+    if (!request.params[0].isNull()) {
259+        style = ParseStyle(request.params[1].get_str());

promag commented at 12:01 pm on September 26, 2017:

Fix index [0].

sipa commented at 0:50 am on November 18, 2017:

Fixing.

in src/wallet/rpcwallet.cpp:165 in a3a79cdb21 outdated

157@@ -157,6 +158,14 @@ UniValue getnewaddress(const JSONRPCRequest& request)
158     if (!request.params[0].isNull())
159         strAccount = AccountFromValue(request.params[0]);
160 
161+    OutputStyle style = address_style;
162+    if (!request.params[1].isNull()) {
163+        style = ParseStyle(request.params[1].get_str());
164+        if (style == STYLE_NONE) {
165+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address style '%s'", request.params[1].get_str()));

promag commented at 12:02 pm on September 26, 2017:

Missing test for this error.

sipa commented at 0:42 am on November 18, 2017:

Will add later.

in src/wallet/rpcwallet.cpp:146 in a3a79cdb21 outdated

143             "\nReturns a new Bitcoin address for receiving payments.\n"
144             "If 'account' is specified (DEPRECATED), it is added to the address book \n"
145             "so payments received with the address will be credited to 'account'.\n"
146             "\nArguments:\n"
147             "1. \"account\"        (string, optional) DEPRECATED. The account name for the address to be linked to. If not provided, the default account \"\" is used. It can also be set to the empty string \"\" to represent the default account. The account does not need to exist, it will be created if there is no account by the given name.\n"
148+            "2. \"style\"          (string, optional) The address style to use. Valid options are 'p2pkh', 'p2sh_p2wpkh', and 'p2wpkh'\n"

promag commented at 12:02 pm on September 26, 2017:

Default value?

luke-jr commented at 8:25 pm on September 26, 2017:

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

instagibbs commented at 8:49 pm on September 29, 2017:

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

‘p2sh_p2wpkh’ vs p2sh etc

sipa commented at 0:41 am on November 18, 2017:

Added.

sipa commented at 0:51 am on November 18, 2017:

I think that should be done separately.

sipa commented at 1:54 am on November 18, 2017:

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.

in src/wallet/init.cpp:189 in a3a79cdb21 outdated

179+    }
180+
181+    style = gArgs.GetArg("-changestyle", style);
182+    parsed_style = ParseStyle(style);
183+    if (parsed_style == STYLE_NONE) {
184+        return InitError(strprintf(_("Unknown change style '%s'"), style));

promag commented at 12:03 pm on September 26, 2017:

Add init tests (follow up maybe)?

sipa commented at 0:41 am on November 18, 2017:

Will add those later.

in src/wallet/rpcwallet.cpp:261 in a3a79cdb21 outdated

253@@ -243,16 +254,24 @@ UniValue getrawchangeaddress(const JSONRPCRequest& request)
254         pwallet->TopUpKeyPool();
255     }
256 
257+    OutputStyle style = change_style;
258+    if (!request.params[0].isNull()) {
259+        style = ParseStyle(request.params[1].get_str());
260+        if (style == STYLE_NONE) {
261+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address style '%s'", request.params[0].get_str()));

promag commented at 12:04 pm on September 26, 2017:

Missing test.

sipa commented at 0:50 am on November 18, 2017:

Will add.

in test/functional/segwit.py:138 in a3a79cdb21 outdated

132@@ -133,8 +133,15 @@ def run_test(self):
133             newaddress = self.nodes[i].getnewaddress()
134             self.pubkey.append(self.nodes[i].validateaddress(newaddress)["pubkey"])
135             multiaddress = self.nodes[i].addmultisigaddress(1, [self.pubkey[-1]])
136-            self.nodes[i].addwitnessaddress(newaddress)
137-            self.nodes[i].addwitnessaddress(multiaddress)
138+            multiscript = CScript([OP_1, hex_str_to_bytes(self.pubkey[-1]), OP_1, OP_CHECKMULTISIG])
139+            p2sh_addr = self.nodes[i].addwitnessaddress(newaddress, True)

promag commented at 12:07 pm on September 26, 2017:

Remove , True so that default value is tested.

sipa commented at 0:51 am on November 18, 2017:

Done.

promag commented at 12:08 pm on September 26, 2017: member

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

in src/wallet/init.cpp:19 in a3a79cdb21 outdated

14@@ -15,6 +15,8 @@
15 std::string GetWalletHelpString(bool showDebug)
16 {
17     std::string strUsage = HelpMessageGroup(_("Wallet options:"));
18+    strUsage += HelpMessageOpt("-addressstyle", _("What style addresses to use (\"p2pkh\", \"p2sh_p2wpkh\", \"p2wpkh\")"));
19+    strUsage += HelpMessageOpt("-changestyle", _("What style change to use (\"p2pkh\", \"p2sh_p2wpkh\", \"p2wpkh\")"));

luke-jr commented at 8:21 pm on September 26, 2017:

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

sipa commented at 0:51 am on November 18, 2017:

Added explanation of defaults.

in src/wallet/wallet.cpp:4139 in a3a79cdb21 outdated

4083+OutputStyle ParseStyle(const std::string& style)
4084+{
4085+    if (style == STYLE_STRING_LEGACY) {
4086+        return STYLE_LEGACY;
4087+    } else if (style == STYLE_STRING_P2SH || style == "default") {
4088+        return STYLE_P2SH;

luke-jr commented at 8:22 pm on September 26, 2017:

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

sipa commented at 10:23 am on September 28, 2017:

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

luke-jr commented at 7:36 pm on September 28, 2017:

Tests don’t need to (and probably shouldn’t) rely on defaults…

sipa commented at 6:47 pm on September 29, 2017:

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.

petertodd commented at 7:12 pm on September 28, 2017: contributor

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 commented at 9:08 pm on September 28, 2017: contributor

maybe it should be renamed to -addressstyle=bech32?

laanwj referenced this in commit aa624b61c9 on Sep 29, 2017

meshcollider commented at 11:57 am on September 29, 2017: contributor

Needs rebase after #11167 merge but that’ll make it clearer what’s new here to review :+1:

in src/rpc/misc.cpp:72 in a3a79cdb21 outdated

66@@ -64,6 +67,13 @@ class DescribeAddressVisitor : public boost::static_visitor<UniValue>
67             obj.push_back(Pair("script", GetTxnOutputType(whichType)));
68             obj.push_back(Pair("hex", HexStr(subscript.begin(), subscript.end())));
69             UniValue a(UniValue::VARR);
70+            if (whichType == TX_WITNESS_V0_KEYHASH || whichType == TX_WITNESS_V0_SCRIPTHASH) {
71+                UniValue subobj = boost::apply_visitor(*this, addresses[0]);
72+                obj.push_back(Pair("embedded", std::move(subobj)));

instagibbs commented at 4:23 pm on September 29, 2017:

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

theuni commented at 8:08 pm on October 17, 2017:

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

(Use after move here)

sipa commented at 1:51 am on November 18, 2017:

Done.

in src/wallet/wallet.h:1146 in 1ae94623ec outdated

1110@@ -1111,6 +1111,9 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
1111      * have all private keys. This is unrelated to whether we consider this
1112      * output to be ours. */
1113     bool IsSolvable(const CScript& script) const;
1114+
1115+    /** Return the hashes of the keys involved in a script. */

instagibbs commented at 4:25 pm on September 29, 2017:

Remove plurals from description.

sipa force-pushed on Sep 29, 2017

sipa commented at 6:36 pm on September 29, 2017: member

Rebased after #11167 merge.

in src/wallet/wallet.cpp:4143 in 7f88d8ad77 outdated

4060@@ -4061,3 +4061,24 @@ bool CWallet::IsSolvable(const CScript& script) const
4061     SignatureData sigs;
4062     return (ProduceSignature(creator, script, sigs) && VerifyScript(sigs.scriptSig, script, &sigs.scriptWitness, MANDATORY_SCRIPT_VERIFY_FLAGS | SCRIPT_VERIFY_WITNESS_PUBKEYTYPE, creator.Checker()));
4063 }
4064+
4065+CKeyID CWallet::GetKeyForDestination(const CTxDestination& dest) const
4066+{
4067+    // Only supports P2PKH, P2WPKH, P2SH-P2WPKH.

instagibbs commented at 8:28 pm on September 29, 2017:

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

sipa commented at 1:58 am on November 18, 2017:

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 ;)

in src/wallet/wallet.cpp:46 in 20da344726 outdated

42@@ -43,6 +43,8 @@ CFeeRate payTxFee(DEFAULT_TRANSACTION_FEE);
43 unsigned int nTxConfirmTarget = DEFAULT_TX_CONFIRM_TARGET;
44 bool bSpendZeroConfChange = DEFAULT_SPEND_ZEROCONF_CHANGE;
45 bool fWalletRbf = DEFAULT_WALLET_RBF;
46+OutputStyle address_style = STYLE_P2SH;

instagibbs commented at 8:51 pm on September 29, 2017:

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?

sipa commented at 1:57 am on November 18, 2017:

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.

in src/wallet/init.cpp:186 in 20da344726 outdated

176+        return InitError(strprintf(_("Unknown address style '%s'"), style));
177+    } else {
178+        address_style = parsed_style;
179+    }
180+
181+    style = gArgs.GetArg("-changestyle", style);

instagibbs commented at 8:55 pm on September 29, 2017:

changestyle needs a test

in src/wallet/rpcwallet.cpp:1297 in 20da344726 outdated

1253@@ -1234,7 +1254,7 @@ UniValue addwitnessaddress(const JSONRPCRequest& request)
1254 
1255     {
1256         LOCK(cs_main);
1257-        if (!IsWitnessEnabled(chainActive.Tip(), Params().GetConsensus()) && !gArgs.GetBoolArg("-walletprematurewitness", false)) {
1258+        if (!IsWitnessEnabled(pindexBestHeader, Params().GetConsensus()) && !gArgs.GetBoolArg("-walletprematurewitness", false)) {

instagibbs commented at 8:59 pm on September 29, 2017:

change seems way out of place here

sipa commented at 1:56 am on November 18, 2017:

Fixed.

in src/wallet/wallet.cpp:47 in 20da344726 outdated

42@@ -43,6 +43,8 @@ CFeeRate payTxFee(DEFAULT_TRANSACTION_FEE);
43 unsigned int nTxConfirmTarget = DEFAULT_TX_CONFIRM_TARGET;
44 bool bSpendZeroConfChange = DEFAULT_SPEND_ZEROCONF_CHANGE;
45 bool fWalletRbf = DEFAULT_WALLET_RBF;
46+OutputStyle address_style = STYLE_P2SH;
47+OutputStyle change_style = STYLE_P2SH;

instagibbs commented at 9:02 pm on September 29, 2017:

Same suggestion as address_style

sipa commented at 1:57 am on November 18, 2017:

Fixed.

sipa force-pushed on Sep 29, 2017

MarcoFalke added this to the milestone 0.15.1 on Oct 5, 2017

MarcoFalke added the label Needs backport on Oct 5, 2017

gmaxwell commented at 8:02 pm on October 11, 2017: contributor

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

in src/wallet/wallet.cpp:4102 in 5632374f2f outdated

4086+}
4087+
4088+static const std::string STYLE_STRING_NONE = "";
4089+static const std::string STYLE_STRING_LEGACY = "legacy";
4090+static const std::string STYLE_STRING_P2SH = "p2sh";
4091+static const std::string STYLE_STRING_SEGWIT = "segwit";

kallewoof commented at 4:38 am on October 12, 2017:

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

sipa commented at 1:59 am on November 18, 2017:

Fixed, I think it’s consistent now.

in src/wallet/rpcwallet.cpp:140 in 5632374f2f outdated

134@@ -135,14 +135,15 @@ UniValue getnewaddress(const JSONRPCRequest& request)
135         return NullUniValue;
136     }
137 
138-    if (request.fHelp || request.params.size() > 1)
139+    if (request.fHelp || request.params.size() > 2)
140         throw std::runtime_error(
141-            "getnewaddress ( \"account\" )\n"
142+            "getnewaddress ( \"account\" ) ( \"style\" )\n"

kallewoof commented at 4:49 am on October 12, 2017:

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

luke-jr commented at 5:56 am on October 12, 2017:

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

gmaxwell commented at 7:13 pm on October 13, 2017:

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

sipa commented at 1:59 am on November 18, 2017:

Fixed.

ryanofsky commented at 6:55 pm on December 6, 2017:

#11403 (review)

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.)

kallewoof approved

kallewoof commented at 4:56 am on October 12, 2017: member

utACK 5632374f2f71bca384ceed07ca6842fd87102346

sipa force-pushed on Oct 14, 2017

in src/script/standard.h:85 in fb0361d74a outdated

83+};
84+
85+struct WitnessV0KeyHash : public uint160
86+{
87+public:
88+    WitnessV0KeyHash() : uint160() {}

theuni commented at 8:02 pm on October 17, 2017:

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

0using uint160::uint160;

sipa commented at 2:08 am on November 18, 2017:

Cool, done.

in src/rpc/misc.cpp:72 in 97f4fc173d outdated

66@@ -67,6 +67,13 @@ class DescribeAddressVisitor : public boost::static_visitor<UniValue>
67             obj.push_back(Pair("script", GetTxnOutputType(whichType)));
68             obj.push_back(Pair("hex", HexStr(subscript.begin(), subscript.end())));
69             UniValue a(UniValue::VARR);
70+            if (whichType == TX_WITNESS_V0_KEYHASH || whichType == TX_WITNESS_V0_SCRIPTHASH) {
71+                UniValue subobj = boost::apply_visitor(*this, addresses[0]);
72+                obj.push_back(Pair("embedded", std::move(subobj)));
73+                if (subobj.exists("pubkey")) {

theuni commented at 8:26 pm on October 17, 2017:

Why dupe this? Helps with existing tooling?

sipa commented at 2:28 am on November 18, 2017:

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

in src/wallet/wallet.cpp:4144 in 8c55a70ceb outdated

4071@@ -4072,3 +4072,24 @@ bool CWallet::IsSolvable(const CScript& script) const
4072     SignatureData sigs;
4073     return (ProduceSignature(creator, script, sigs) && VerifyScript(sigs.scriptSig, script, &sigs.scriptWitness, MANDATORY_SCRIPT_VERIFY_FLAGS | SCRIPT_VERIFY_WITNESS_PUBKEYTYPE, creator.Checker()));
4074 }
4075+
4076+CKeyID CWallet::GetKeyForDestination(const CTxDestination& dest) const
4077+{
4078+    // Only supports P2PKH, P2WPKH, P2SH-P2WPKH.
4079+    if (boost::get<CKeyID>(&dest)) {

theuni commented at 9:02 pm on October 17, 2017:

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

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

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

sipa commented at 3:31 am on November 18, 2017:

Fixed.

in src/keystore.cpp:24 in b7209bdc90 outdated

19+    // This adds the redeemscripts necessary to detect P2WPKH and P2SH-P2WPKH
20+    // outputs.
21+    if (pubkey.IsCompressed()) {
22+        CScript script = CScript() << OP_0 << ToByteVector(pubkey.GetID());
23+        // This does not use AddCScript, as it may be overridden.
24+        mapScripts[CScriptID(script)] = std::move(script);

theuni commented at 9:28 pm on October 17, 2017:

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

sipa commented at 2:27 am on November 18, 2017:

Ugh, thanks for pointing that out.

ryanofsky commented at 7:23 pm on December 6, 2017:

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.

in src/wallet/wallet.cpp:4136 in 4bc304c9fa outdated

4066+{
4067+    // This check is to make sure that the script we created can actually be solved for and signed by us
4068+    // if we were to have the private keys. This is just to make sure that the script is valid and that,
4069+    // if found in a transaction, we would still accept and relay that transaction. In particular,
4070+    // it will reject witness outputs that require signing with an uncompressed public key.
4071+    DummySignatureCreator creator(this);

theuni commented at 10:09 pm on October 17, 2017:

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

sipa commented at 3:32 am on November 18, 2017:

Moved to script/signer.

in src/wallet/wallet.cpp:4170 in f542daeb69 outdated

4135+    } else {
4136+        return std::vector<CTxDestination>{std::move(keyid)};
4137+    }
4138+}
4139+
4140+CTxDestination CWallet::GetDestinationForKey(const CPubKey& key, OutputStyle style)

theuni commented at 10:18 pm on October 17, 2017:

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

sipa commented at 3:33 am on November 18, 2017:

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

in src/wallet/wallet.cpp:4222 in f542daeb69 outdated

4166+    case STYLE_SEGWIT: {
4167+        CTxDestination witdest = WitnessV0ScriptHash(Hash(script.begin(), script.end()));
4168+        CScript witprog = GetScriptForDestination(witdest);
4169+        // Check if the resulting program is solvable (i.e. doesn't use an uncompressed key)
4170+        if (!IsSolvable(witprog)) return CScriptID(script);
4171+        AddCScript(witprog);

theuni commented at 10:30 pm on October 17, 2017:

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.

sipa commented at 3:37 am on November 18, 2017:

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.

theuni commented at 10:38 pm on October 17, 2017: member

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?

Sjors commented at 12:28 pm on October 20, 2017: member

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 commented at 12:30 pm on October 20, 2017: member

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

Sjors commented at 12:40 pm on October 20, 2017: member

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

sipa force-pushed on Oct 24, 2017

fanquake added this to the "In progress" column in a project

MarcoFalke commented at 10:18 pm on November 7, 2017: member

rebase_me_pls

sipa force-pushed on Nov 8, 2017

sipa commented at 0:19 am on November 8, 2017: member

rebased_it

laanwj added this to the "Blockers" column in a project

MarcoFalke removed the label Needs backport on Nov 9, 2017

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

sipa force-pushed on Nov 10, 2017

in src/wallet/init.cpp:19 in 2cd3e3259c outdated

14@@ -15,6 +15,8 @@
15 std::string GetWalletHelpString(bool showDebug)
16 {
17     std::string strUsage = HelpMessageGroup(_("Wallet options:"));
18+    strUsage += HelpMessageOpt("-addressstyle", _("What style addresses to use (\"legacy\", \"p2sh\", \"bech32\", default: \"p2sh\")"));
19+    strUsage += HelpMessageOpt("-changestyle", _("What style change to use (\"legacy\", \"p2sh\", \"bech32\"), default: same as -addressstyle"));

flack commented at 2:09 pm on November 10, 2017:

I’m not native speaker, but “What style change to use” seems like broken English to me. Shouldn’t it be something like “Which style of change addresses to use”?

CydeWeys commented at 9:05 pm on November 10, 2017:

Native English speaker here. Here’s how I’d write these two:

“Receiving address type (…)” “Change address type (…)”

Legacy/p2sh/bech32 feel like types to me, not styles. Though at that point you’d have to change the command line parameters as well.

sipa commented at 6:09 am on November 18, 2017:

Changed to address_type and change_type, and edited the comment language.

pilotmoon commented at 4:19 pm on November 11, 2017: none

What is the reasoning for having addressstyle and changestyle as separate options; that is, what is the envisaged use case for setting these defaults to different things?

martin-lizner commented at 3:55 pm on November 12, 2017: none

Milestone 0.15.2 was removed, does it mean it goes straight to 0.16 only? Thanks.

MarcoFalke commented at 4:42 pm on November 12, 2017: member

Yes

On Nov 12, 2017 10:55, “Martin Lízner” notifications@github.com wrote:

Milestone 0.15.2 was removed, does it mean it goes straight to 0.16 only? Thanks.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/11403#issuecomment-343746694, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmv7mc0a0e0Y92zjU3aPymx17snBlFks5s1xUIgaJpZM4PjkGp .

martin-lizner commented at 6:45 pm on November 12, 2017: none

Does this mean we will have segwit in GUI and bech32 in around May? If that so, would you pls comment on why 0.15.2 milestone was dropped?

MarcoFalke commented at 1:27 am on November 13, 2017: member

The 0.15.2 milestone would only delay segwit wallet. The current plan is to get the segwit wallet out as soon as it is ready with the next major release.

On Sun, Nov 12, 2017 at 6:45 PM, Martin Lízner notifications@github.com wrote:

Does this mean we will have segwit in GUI and bech32 in around May? If that so, would you pls comment on why 0.15.2 milestone was dropped?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/11403#issuecomment-343757936, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmv9SEn6dU3nzJU7gZVx9z2cf7jbCTks5s1zzYgaJpZM4PjkGp .

sipa commented at 7:41 pm on November 16, 2017: member

I’ve made a writeup about wallet design in the future, and how SegWit support now interacts with it: https://gist.github.com/sipa/125cfa1615946d0c3f3eec2ad7f250a2

The PR currently only implements solution (I.1) from the document, but I’m happy to reconsider.

sipa force-pushed on Nov 17, 2017

sipa commented at 3:38 am on November 18, 2017: member

@invariant You may wish to use bech32 as change style as soon as possible (for cost) or once it’s used sufficiently on the network (for privacy), even if your recipients don’t support bech32 yet.

sipa force-pushed on Nov 18, 2017

sipa commented at 3:59 am on November 18, 2017: member

Rebased and addressed my review comments.

sipa force-pushed on Nov 18, 2017

in src/keystore.cpp:20 in 3c44c27398 outdated

12@@ -13,6 +13,19 @@ bool CKeyStore::AddKey(const CKey &key) {
13     return AddKeyPubKey(key, key.GetPubKey());
14 }
15 
16+void CBasicKeyStore::AddRelatedKeyScripts(const CPubKey& pubkey)
17+{
18+    AssertLockHeld(cs_KeyStore);
19+    // This adds the redeemscripts necessary to detect P2WPKH and P2SH-P2WPKH

Sjors commented at 1:10 pm on November 18, 2017:

Can you add: “P2WPKH and P2SH-P2WPKH have the same redeem script.”?

It took me a while to realize that.

sipa commented at 8:13 pm on November 18, 2017:

That’s not technically correct. P2WPKH doesn’t have a redeemscript at all. However, our current IsMine logic requires the corresponding P2SH-P2WPKH redeemscript to be present in the wallet in order to accept payment even to P2WPKH outputs.

Feel free to suggest better language here.

Sjors commented at 9:14 pm on November 18, 2017:

Your comment is probably a good start.

sipa commented at 9:08 pm on November 19, 2017:

Done.

in src/keystore.cpp:160 in 3c44c27398 outdated

153@@ -138,3 +154,24 @@ bool CBasicKeyStore::HaveWatchOnly() const
154     LOCK(cs_KeyStore);
155     return (!setWatchOnly.empty());
156 }
157+
158+CKeyID GetKeyForDestination(const CKeyStore& store, const CTxDestination& dest)
159+{
160+    // Only supports P2PKH, P2WPKH, P2SH-P2WPKH.

Sjors commented at 1:40 pm on November 18, 2017:

Maybe add for redundancy: “Does not support P2SH and, because multi-sig could result in multiple CKeyID’s.”

meshcollider commented at 1:29 am on November 19, 2017:

Or “Only supports destinations which map to single public keys, i.e. P2PKH, P2WPKH, P2SH-P2WPKH.” for more generality

sipa commented at 9:08 pm on November 19, 2017:

Fixed.

in src/wallet/init.cpp:19 in 3c44c27398 outdated

14@@ -15,6 +15,8 @@
15 std::string GetWalletHelpString(bool showDebug)
16 {
17     std::string strUsage = HelpMessageGroup(_("Wallet options:"));
18+    strUsage += HelpMessageOpt("-addresstype", _("What type of addresses to use (\"legacy\", \"p2sh\", \"bech32\", default: \"p2sh\")"));
19+    strUsage += HelpMessageOpt("-changetype", _("What type of change outputs to use (\"legacy\", \"p2sh\", \"bech32\"), default: same as -addresstype"));

Sjors commented at 2:03 pm on November 18, 2017:

Would it make sense to default -changetype to bech32 if -addresstype is not specified? Or should we leave that for QT (since anyone using the RPC probably knows what they’re doing)?

sipa commented at 8:13 pm on November 18, 2017:

I think that’s not acceptable for privacy reasons until bech32 is common on the network.

Sjors commented at 9:13 pm on November 18, 2017:

Privacy is a good point. In that case, wouldn’t it be better to use whichever address format is used for the recipient (unless the parameter is set)?

promag commented at 4:27 pm on November 21, 2017:

default: should be inside ().

Sjors commented at 2:51 pm on November 18, 2017: member

I assume the goal of this PR isn’t to change the GUI, but merely to make sure the QT doesn’t break?

I tested QT with a fresh wallet. The first receive address was a P2SH-P2WPKH address. Sending coins also used a P2SH-P2WPKH address for change. Bump fee also works. That’s certainly an improvement over v0.15.1.

Launching QT with -addresstype=bech32 also worked. I was able to fund* it using a bech32 address, and then send to another bech32 address, which used a bech32 change address.

Any suggestions for potentially problematic scenarios to test?

Using P2SH-P2WPKH as a receive address seems a safe conservative approach, for this PR. But not using bech32 for the change address seems wasteful, although not strictly worse than v0.15.1. See my inline suggestion.

= for anyone trying to get fund a bech32 address without using the RPC: as suggested here, create a non-SegWit Electrum 3 wallet, fund it, then use it to send to to bech32 address.

sipa commented at 8:15 pm on November 18, 2017: member

I assume the goal of this PR isn’t to change the GUI, but merely to make sure the QT doesn’t break?

Exactly. In particular, we’ll want bech32 error detection at some point when entering such addresses, but that’s not a priority now.

in test/functional/segwit.py:361 in 2eda876838 outdated

355@@ -356,8 +356,10 @@ def run_test(self):
356                 [p2wpkh, p2sh_p2wpkh, p2pk, p2pkh, p2sh_p2pk, p2sh_p2pkh, p2wsh_p2pk, p2wsh_p2pkh, p2sh_p2wsh_p2pk, p2sh_p2wsh_p2pkh] = self.p2pkh_address_to_script(v)
357                 # normal P2PKH and P2PK with compressed keys should always be spendable
358                 spendable_anytime.extend([p2pkh, p2pk])
359-                # P2SH_P2PK, P2SH_P2PKH, and witness with compressed keys are spendable after direct importaddress
360-                spendable_after_importaddress.extend([p2wpkh, p2sh_p2wpkh, p2sh_p2pk, p2sh_p2pkh, p2wsh_p2pk, p2wsh_p2pkh, p2sh_p2wsh_p2pk, p2sh_p2wsh_p2pkh])
361+                # P2SH_P2PK, P2SH_P2PKH with compressed keys are spendable after direct importaddress
362+                spendable_after_importaddress.extend([p2sh_p2pk, p2sh_p2pkh, p2wsh_p2pk, p2wsh_p2pkh, p2sh_p2wsh_p2pk, p2sh_p2wsh_p2pkh])
363+                # P2WPKH and P2SH_P2WPKH are always spendable

flack commented at 8:30 pm on November 18, 2017:

Apologies if this is slightly off-topic, but I can sort of see this comment ending up in an angry reddit post / powerpoint

meshcollider commented at 1:47 am on November 19, 2017:

Note compressed keys explicitly because all other comments do? Otherwise it could sound like it includes uncompressed keys. Or merge with the p2pkh and p2pk line above in the same way you did below?

sipa commented at 9:02 pm on November 19, 2017:

Then quote the code around it, which uses the language w.r.t. P2PKH and others. It’s obviously about outputs we have the private keys for.

sipa commented at 9:09 pm on November 19, 2017:

Done.

in src/rpc/misc.cpp:154 in dd94446cff outdated

151@@ -145,6 +152,7 @@ UniValue validateaddress(const JSONRPCRequest& request)
152             "    ]\n"
153             "  \"sigsrequired\" : xxxxx        (numeric, optional) Number of signatures required to spend multisig output\n"
154             "  \"pubkey\" : \"publickeyhex\",    (string) The hex value of the raw public key\n"

meshcollider commented at 1:21 am on November 19, 2017:

Not related to the change in this PR, but the "pubkey" should be optional as well because it isn’t returned for Witness*

sipa commented at 8:18 pm on November 19, 2017:

It is returned for P2WPKH and P2SH-P2WPKH, but not for anything that doesn’t have a unique pubkey.

in test/functional/segwit.py:522 in 2eda876838 outdated

515@@ -514,9 +516,8 @@ def run_test(self):
516                 premature_witaddress.append(script_to_p2sh(p2wsh))
517             else:
518                 [p2wpkh, p2sh_p2wpkh, p2pk, p2pkh, p2sh_p2pk, p2sh_p2pkh, p2wsh_p2pk, p2wsh_p2pkh, p2sh_p2wsh_p2pk, p2sh_p2wsh_p2pkh] = self.p2pkh_address_to_script(v)
519-                # P2WPKH, P2SH_P2WPKH are spendable after addwitnessaddress
520-                spendable_after_addwitnessaddress.extend([p2wpkh, p2sh_p2wpkh])
521-                premature_witaddress.append(script_to_p2sh(p2wpkh))
522+                # P2WPKH, P2SH_P2WPKH are always spendable
523+                spendable_anytime.extend([p2wpkh, p2sh_p2wpkh])

meshcollider commented at 1:55 am on November 19, 2017:

This part of the test doesn’t use spendable_anytime, only spendable_after_addwitnessaddress, so this change (and one below) could just be deleted completely I believe (unless there is a benefit for readability)

sipa commented at 9:10 pm on November 19, 2017:

Better: I’ve added code for testing spendable_anytime and solvable_anytime in these cases.

meshcollider commented at 2:10 am on November 19, 2017: contributor

utACK https://github.com/bitcoin/bitcoin/pull/11403/commits/3c44c27398c8c5e80790f95c747d654cbe716b0c

I’m okay with the approach taken based on the write-up, seems like a good step to me

BTW commit “Expose method to find key for a single-key destination” should be before the two previous commits IMO, which use the method it introduces

sipa force-pushed on Nov 19, 2017

sipa commented at 9:16 pm on November 19, 2017: member

Addressed some comments, and added solution 3a from my writeup.

sipa commented at 9:17 pm on November 19, 2017: member

@MeshCollider The commits are in the right order, it’s just GitHub that shows them by author date rather than dependency ordering.

in src/rpc/misc.cpp:73 in d0c76c351a outdated

66@@ -67,6 +67,13 @@ class DescribeAddressVisitor : public boost::static_visitor<UniValue>
67             obj.push_back(Pair("script", GetTxnOutputType(whichType)));
68             obj.push_back(Pair("hex", HexStr(subscript.begin(), subscript.end())));
69             UniValue a(UniValue::VARR);
70+            if (whichType == TX_WITNESS_V0_KEYHASH || whichType == TX_WITNESS_V0_SCRIPTHASH) {
71+                UniValue subobj = boost::apply_visitor(*this, addresses[0]);
72+                if (subobj.exists("pubkey")) {
73+                    obj.push_back(Pair("pubkey", subobj["pubkey"].get_str()));

promag commented at 4:05 pm on November 20, 2017:

Remove .get_str()?

in src/rpc/misc.cpp:75 in d0c76c351a outdated

66@@ -67,6 +67,13 @@ class DescribeAddressVisitor : public boost::static_visitor<UniValue>
67             obj.push_back(Pair("script", GetTxnOutputType(whichType)));
68             obj.push_back(Pair("hex", HexStr(subscript.begin(), subscript.end())));
69             UniValue a(UniValue::VARR);
70+            if (whichType == TX_WITNESS_V0_KEYHASH || whichType == TX_WITNESS_V0_SCRIPTHASH) {
71+                UniValue subobj = boost::apply_visitor(*this, addresses[0]);
72+                if (subobj.exists("pubkey")) {
73+                    obj.push_back(Pair("pubkey", subobj["pubkey"].get_str()));
74+                }
75+                obj.push_back(Pair("embedded", std::move(subobj)));

promag commented at 4:05 pm on November 20, 2017:

Missing test.

in src/rpc/misc.cpp:225 in d0c76c351a outdated

195@@ -188,8 +196,11 @@ UniValue validateaddress(const JSONRPCRequest& request)
196         }
197         if (pwallet) {
198             const auto& meta = pwallet->mapKeyMetadata;
199-            const CKeyID *keyID = boost::get<CKeyID>(&dest);
200-            auto it = keyID ? meta.find(*keyID) : meta.end();
201+            CKeyID key = GetKeyForDestination(*pwallet, dest);

promag commented at 4:08 pm on November 20, 2017:

Below you use auto key = .... Keep it consistent?

sipa commented at 0:54 am on December 1, 2017:

Choosing to use auto is a balance between clarity (by being explicit) and brevity/flexibility. I don’t think using it here would be an improvement, but I added it on the line below (the iterator type was pretty long…).

in src/wallet/init.cpp:183 in d0c76c351a outdated

174@@ -173,6 +175,22 @@ bool WalletParameterInteraction()
175     bSpendZeroConfChange = gArgs.GetBoolArg("-spendzeroconfchange", DEFAULT_SPEND_ZEROCONF_CHANGE);
176     fWalletRbf = gArgs.GetBoolArg("-walletrbf", DEFAULT_WALLET_RBF);
177 
178+    std::string output_type = gArgs.GetArg("-addresstype", "default");
179+    OutputType parsed_output_type = ParseOutputType(output_type);
180+    if (parsed_output_type == OUTPUT_TYPE_NONE) {
181+        return InitError(strprintf(_("Unknown address type '%s'"), output_type));
182+    } else {
183+        g_address_type = parsed_output_type;

promag commented at 4:11 pm on November 20, 2017:

Remove else?

0
1g_address_type = ParseOutputType(gArgs.GetArg("-addresstype", "default"));
2if (g_address_type == OUTPUT_TYPE_NONE) {
3    return InitError(strprintf(_("Unknown address type '%s'"), gArgs.GetArg("-addresstype")));
4}

in src/wallet/init.cpp:191 in d0c76c351a outdated

186+    output_type = gArgs.GetArg("-changetype", output_type);
187+    parsed_output_type = ParseOutputType(output_type);
188+    if (parsed_output_type == OUTPUT_TYPE_NONE) {
189+        return InitError(strprintf(_("Unknown change type '%s'"), output_type));
190+    } else {
191+        g_change_type = parsed_output_type;

promag commented at 4:12 pm on November 20, 2017:

Same as above.

in src/wallet/rpcwallet.cpp:162 in d0c76c351a outdated

157@@ -157,6 +158,14 @@ UniValue getnewaddress(const JSONRPCRequest& request)
158     if (!request.params[0].isNull())
159         strAccount = AccountFromValue(request.params[0]);
160 
161+    OutputType output_type = g_address_type;

promag commented at 4:15 pm on November 20, 2017:

Mirror argument name:

0OutputType address_type = g_address_type;

in src/wallet/rpcwallet.cpp:259 in d0c76c351a outdated

253@@ -243,16 +254,24 @@ UniValue getrawchangeaddress(const JSONRPCRequest& request)
254         pwallet->TopUpKeyPool();
255     }
256 
257+    OutputType output_type = g_change_type;

promag commented at 4:17 pm on November 20, 2017:

Mirror argument name:

0OutputType address_type = g_change_type;

sipa commented at 0:55 am on December 1, 2017:

I’m using ‘output type’ as the internal name, ‘address type’ is just used for the global/cmdline.

Part of the reason for that is that I feel using the term ‘address’ for change is wrong. Addresses are things intended to be the destination of payments. Change outputs are only created and used internally.

promag commented at 4:34 pm on January 3, 2018:

Part of the reason for that is that I feel using the term ‘address’ for change is wrong

For that reason rename argument above from address_type to change_type?

promag commented at 4:27 pm on November 20, 2017: member

2nd round code review. Will test.

promag commented at 3:30 pm on November 23, 2017: member

Tested ACK d0c76c3.

in src/wallet/rpcwallet.cpp:259 in d0c76c351a outdated

253@@ -243,16 +254,24 @@ UniValue getrawchangeaddress(const JSONRPCRequest& request)
254         pwallet->TopUpKeyPool();
255     }
256 
257+    OutputType output_type = g_change_type;
258+    if (!request.params[0].isNull()) {
259+        output_type = ParseOutputType(request.params[0].get_str());

instagibbs commented at 6:14 pm on November 27, 2017:

I believe this allows the user to put in “default”, which then resolves to “P2SH” no matter what the actual default g_change_type is set to .

in src/wallet/rpcwallet.cpp:163 in d0c76c351a outdated

157@@ -157,6 +158,14 @@ UniValue getnewaddress(const JSONRPCRequest& request)
158     if (!request.params[0].isNull())
159         strAccount = AccountFromValue(request.params[0]);
160 
161+    OutputType output_type = g_address_type;
162+    if (!request.params[1].isNull()) {
163+        output_type = ParseOutputType(request.params[1].get_str());

instagibbs commented at 6:15 pm on November 27, 2017:

I believe this allows the user to put in “default”, which then resolves to “P2SH” no matter what the actual default g_address_type is set to .

instagibbs commented at 6:22 pm on November 27, 2017: member

“default” behavior aside, utACK

in src/wallet/rpcwallet.cpp:147 in d0c76c351a outdated

143             "\nReturns a new Bitcoin address for receiving payments.\n"
144             "If 'account' is specified (DEPRECATED), it is added to the address book \n"
145             "so payments received with the address will be credited to 'account'.\n"
146             "\nArguments:\n"
147             "1. \"account\"        (string, optional) DEPRECATED. The account name for the address to be linked to. If not provided, the default account \"\" is used. It can also be set to the empty string \"\" to represent the default account. The account does not need to exist, it will be created if there is no account by the given name.\n"
148+            "2. \"address_type\"   (string, optional) The address type to use. Options are \"legacy\", \"p2sh\", and \"bech32\". Default is set by -addresstype.\n"

theuni commented at 1:59 am on November 28, 2017:

There’s no indication as to what “-addresstype” currently is. How about something like:

0"... Default (*g_address_type string*) is set by -addresstype.\n"

sipa commented at 0:53 am on December 1, 2017:

Fixed.

instagibbs commented at 7:12 pm on December 27, 2017:

doesn’t seem changed?

sipa commented at 11:58 am on December 28, 2017:

I fixed it and changed it back in response to a comment by @TheBlueMatt.

promag commented at 2:36 am on December 29, 2017:

Should use same format as others?

0            "2. \"address_type\"   (string, optional, default=" + FormatOutputType(g_address_type) + ") The address type to use...

Nit, drop command line argument hint, IMO.

theuni commented at 2:35 am on November 28, 2017: member

utACK after the current comments are addressed. Nice work :)

sipa force-pushed on Dec 1, 2017

sipa commented at 0:41 am on December 1, 2017: member

Addressed comments, and made some significant changes to the command line argument handling.

I also added a commit that explicitly adds the P2SH-P2WPKH script for keypool keys that are seen used on the network. This means that downgrading after restoring a backup that needed recovery is supported.

I would very much like to hear opinions about testing this. It’s nontrivial as the complicated scenarios always involve downgrading/upgrading between versions - something our testing framework can’t really deal with.

sipa force-pushed on Dec 1, 2017

in src/wallet/init.cpp:19 in b088ab6540 outdated

14@@ -15,6 +15,8 @@
15 std::string GetWalletHelpString(bool showDebug)
16 {
17     std::string strUsage = HelpMessageGroup(_("Wallet options:"));
18+    strUsage += HelpMessageOpt("-addresstype", strprintf(_("What type of addresses to use (\"legacy\", \"p2sh\", \"bech32\", default: \"%s\")"), FormatOutputType(OUTPUT_TYPE_DEFAULT)));
19+    strUsage += HelpMessageOpt("-changetype", strprintf(_("What type of change to use (\"legacy\", \"p2sh\", \"bech32\", default is same as -addressstyle: \"%s\")"), FormatOutputType(g_address_type)));

Sjors commented at 11:54 am on December 1, 2017:

-addresstype, not -addressstyle

TheBlueMatt commented at 10:47 pm on December 7, 2017:

The defaults here are a mess - g_address_type is initialized to a constant (OUTPUT_TYPE_P2SH) and not OUTPUT_TYPE_DEFAULT, which would make sense since we want them tied together by default. I think you should default them both to OUTPUT_TYPE_NONE, cause it’d be an error for WalletParameterInteraction to not get run, and then refer to OUTPUT_TYPE_DEFAULT everywhere as the default value, then not have a default argument in ParseOutputType if its gonna return _NONE on error anyway and just default the argument to FormatOutputType(OUTPUT_TYPE_DEFAULT).

Sjors commented at 12:27 pm on December 1, 2017: member

I suggest updating the description of this pull request with a list of all known scenario’s that need to be tested. Under each item, note if there any automated tests that cover it.

Is it a safe assumption that testing on testnet is sufficient? Can anyone think of mainnet specific potential bug related to a change like this?

Can we also assume no support for pre-release wallets after v0.15.1?

To save some permutations, are we worried changetype set to something to other than addresstype?

Here’s a start.

QT

Full UI support is for another PR. Scope is to ensure nothing breaks.

New wallet, default settings

create payment request, address should start with 2 (UI test only checks that button works, not the address type - but see RPC)
fund from external source
- tx should appear under Recent transactions
- popup notification should appear
send to self
- pending balance should be 0 (with or without RBF)
- total balance should be reduced only by the fee of this transaction
send to external P2SH address
send to external bech32 address

New wallet `-addresstype=bech32`

(there’s no bech32 compatible faucet, so just copy-paste a bech32 receive address and relaunch with the normal address type to fund it)

…

Existing HD wallet

…

Existing non-HD wallet

…

Existing wallet that used `getwitnessaddress`

RPC

New wallet, default settings

create new address, should start with 2 ([link to RPC test])

New wallet `-addresstype=bech32`

…

Existing HD wallet

…

Existing non-HD wallet

…

Issues I’m seeing:

(Pending) balance is zero (seems fixed with #11839) Not sure if this is intended / existing behavior. I received coins on a P2SH address, then spent some of it to another P2SH address in the wallet. That transaction is not confirmed yet. I would expect this balance to be pending. (will retest after #11839)

I then sent more funds from an external source: The new funds show up under pending. Suddenly the total balance is correct again.

With bech32 I’m seeing something slightly different:

This time the total balance is correct, but the pending balance should be 0.1 BTC, since that transaction to myself is still unconfirmed (I didn’t use RBF). (seems fixed with #11839)
~“unknown new rules activated (version bit 1)"~ (happens on master as well, so unrelated)
~I didn’t see a popup when transaction arrived (~maybe I missed it~ I can reproduce this)~ (not related, see #11840)

sipa force-pushed on Dec 5, 2017

sipa force-pushed on Dec 6, 2017

sipa commented at 6:04 am on December 6, 2017: member

@Sjors Thanks for the UI testing! I can’t really explain what you’re seeing, but note that sends-to-self always behave somewhat as an edge-case (as they’re both from-self and to-self). Have a look at the address_type.py test I added, which acts entirely as expected, with 16 combinations of sends/receives between address/change types.

sipa force-pushed on Dec 6, 2017

in test/functional/address_types.py:16 in e2e9ead25f outdated

11+        self.num_nodes = 4
12+        self.setup_clean_chain = True
13+        self.extra_args = [["-addresstype=legacy"], ["-addresstype=p2sh"], ["-addresstype=bech32"], ["-addresstype=p2sh", "-changetype=bech32"]]
14+
15+    def run_test(self):
16+        self.nodes[0].generate(120)

Sjors commented at 3:22 pm on December 6, 2017:

Doesn’t the test framework already give you plenty of mature coins out of the box? cc @jnewbery

afk11 commented at 7:47 pm on December 6, 2017:

@Sjors setup_clean_chain was set to True in this test https://github.com/bitcoin/bitcoin/pull/11403/files/e2e9ead25fe0ddb364e35f8eabb5a7f937d3973b#diff-e3da38b280555e81d6b794e1117ed0a8R12

Sjors commented at 8:58 pm on December 6, 2017:

I see. But why? Doesn’t that slow down the tests?

in test/functional/address_types.py:55 in e2e9ead25f outdated

26+                    # In each iteration, one node sends 1/11th of its balance to itself, 2/11ths to the next peer,
27+                    # 3/11ths to the one after that, and 4/11ths to the remaining one.
28+                    sends[address] = to_send * (1 + k)
29+                self.nodes[n].sendmany("", sends)
30+                self.sync_all()
31+                unconf_balances = [self.nodes[i].getunconfirmedbalance() for i in range(4)]

Sjors commented at 3:31 pm on December 6, 2017:

Should getunconfirmedbalance be the same as what QT displays as balance under “Pending”? If so, what could explain that the UI shows different numbers despite these tests passing?

instagibbs commented at 3:45 pm on December 6, 2017: member

@Sjors “That transaction is not confirmed yet. I would expect this balance to be pending.”

Self-sent coins(including change) are considered IsTrusted, which will account for these as “Available” and immediately spendable.

Not sure why it wasn’t showing as Available however. Testing here shows it there.

What type of p2sh address was it? nested-p2wpkh?

instagibbs commented at 3:53 pm on December 6, 2017: member

I’m getting non-determinstic balance behavior. I have done a couple self-sends of the largest coin, seemed fine.

If I stop and restart, sometimes it shows in confirmed balance(-cli) or “Available”(qt), sometimes it doesn’t. Repeating this process results in it either existing or not.

I checked my debug.log, the transactions in the mempool are being accepted in either case.

Generating a block results in it being permanently accounted for. I’ll dig in some more.

Sjors commented at 3:54 pm on December 6, 2017: member

@instagibbs the p2sh generated if you launch without any arguments, so P2SH-P2WPKH. Where are IsTrusted and “Available” determined? At the wallet level, or is QT doing (some of) that?

instagibbs commented at 5:06 pm on December 6, 2017: member

looks like we were chasing our tails, unrelated to this PR https://github.com/bitcoin/bitcoin/pull/11839

in src/script/sign.h:86 in 3e2d5aa9a4 outdated

80@@ -81,4 +81,9 @@ SignatureData CombineSignatures(const CScript& scriptPubKey, const BaseSignature
81 SignatureData DataFromTransaction(const CMutableTransaction& tx, unsigned int nIn);
82 void UpdateTransaction(CMutableTransaction& tx, unsigned int nIn, const SignatureData& data);
83 
84+/* Check whether we know how to sign for an output like this, assuming we
85+ * have all private keys. This is unrelated to whether we consider this
86+ * output to be ours. */

TheBlueMatt commented at 3:04 pm on December 7, 2017:

nit: mention something about what store is actually used for (redeemScript and pubkeyHash->pubkey lookup), as its otherwise confusing why it exists.

sipa commented at 7:09 am on December 11, 2017:

Done.

in src/script/sign.cpp:434 in 3e2d5aa9a4 outdated

429+    // if we were to have the private keys. This is just to make sure that the script is valid and that,
430+    // if found in a transaction, we would still accept and relay that transaction. In particular,
431+    // it will reject witness outputs that require signing with an uncompressed public key.
432+    DummySignatureCreator creator(&store);
433+    SignatureData sigs;
434+    return (ProduceSignature(creator, script, sigs) && VerifyScript(sigs.scriptSig, script, &sigs.scriptWitness, MANDATORY_SCRIPT_VERIFY_FLAGS | SCRIPT_VERIFY_WITNESS_PUBKEYTYPE, creator.Checker()));

TheBlueMatt commented at 3:45 pm on December 7, 2017:

This seems like we’re gonna forget to update these flags in the future. Any reason to not switch to STANDARD_SCRIPT_VERIFY_FLAGS?

sipa commented at 7:09 am on December 11, 2017:

Fixed, and added a static_assert.

in src/rpc/misc.cpp:226 in 90046fe3b9 outdated

195@@ -188,8 +196,11 @@ UniValue validateaddress(const JSONRPCRequest& request)
196         }
197         if (pwallet) {
198             const auto& meta = pwallet->mapKeyMetadata;
199-            const CKeyID *keyID = boost::get<CKeyID>(&dest);
200-            auto it = keyID ? meta.find(*keyID) : meta.end();
201+            CKeyID key = GetKeyForDestination(*pwallet, dest);
202+            auto it = meta.end();

TheBlueMatt commented at 6:16 pm on December 7, 2017:

It kinda really sucks that we use mapKeyMetadata for metadata for both P2SH scripts and pubkeyhash scripts, with a key of CTxDestination which could contain a WitnessV0ScriptHash or WitnessV0KeyHash. It’d be kinda nice to split it and have a mapWatchScriptsMetadata which uses a CScriptID and a mapKeyMetadata which uses a CKeyID, then we’d get nice type-safety to ensure no one breaks this in the future and its pretty easy to just split them at disk-read/write time so it shouldn’t change the on-disk format.

ryanofsky commented at 7:12 pm on December 8, 2017:

https://github.com/bitcoin/bitcoin/pull/11403/files#r155599383

It kinda really sucks that we use mapKeyMetadata for metadata for both P2SH scripts and pubkeyhash scripts

I addressed this in #11854. It does conflict with this PR but the changes are easy to reconcile, so there’s not much dependency, and no need to change anything here I think.

sipa commented at 7:09 am on December 11, 2017:

Let’s improve that outside of this PR.

in src/rpc/misc.cpp:155 in 90046fe3b9 outdated

151@@ -145,6 +152,7 @@ UniValue validateaddress(const JSONRPCRequest& request)
152             "    ]\n"
153             "  \"sigsrequired\" : xxxxx        (numeric, optional) Number of signatures required to spend multisig output\n"
154             "  \"pubkey\" : \"publickeyhex\",    (string) The hex value of the raw public key\n"
155+            "  \"embedded\" : {...},           (object, optional) validateaddress output for the address embedded in P2SH, if any\n"

TheBlueMatt commented at 6:17 pm on December 7, 2017:

This is wrong - it doesn’t contain the full validateaddress output, only a subset thereof. eg ismine, iswatchonly, account, hdkeypath, hdmasterkeyid, etc are all always missing. Of course many of those could optionally be missing anyway, which should be added to existing docs, but this just seems highly misleading.

TheBlueMatt commented at 6:47 pm on December 7, 2017:

Oh, also, this needs a test.

sipa commented at 7:08 am on December 11, 2017:

Fixed. I’ve also expanded the validateaddress output for P2WSH, simplified the implementation, and added comments.

in src/rpc/misc.cpp:283 in 5a36cba99a outdated

245@@ -246,12 +246,12 @@ CScript _createmultisig_redeemScript(CWallet * const pwallet, const UniValue& pa
246         // Case 1: Bitcoin address and we have full public key:
247         CTxDestination dest = DecodeDestination(ks);
248         if (pwallet && IsValidDestination(dest)) {
249-            const CKeyID *keyID = boost::get<CKeyID>(&dest);
250-            if (!keyID) {
251+            auto key = GetKeyForDestination(*pwallet, dest);

TheBlueMatt commented at 6:48 pm on December 7, 2017:

This should get a test (and should be easy to do).

sipa commented at 7:07 am on December 11, 2017:

This is now tested in address_types.py.

TheBlueMatt commented at 6:48 pm on January 8, 2018:

Can we please not auto shit like this? It just needlessly makes reading the code impossible to save on typing 4 charachters.

in test/functional/p2p-fullblocktest.py:38 in 53fdb41a0b outdated

34@@ -35,7 +35,7 @@ def initialize(self, base_block):
35         self.vtx = copy.deepcopy(base_block.vtx)
36         self.hashMerkleRoot = self.calc_merkle_root()
37 
38-    def serialize(self):
39+    def serialize(self, with_witness=True):

TheBlueMatt commented at 6:49 pm on December 7, 2017:

Errr, dont you need to do something with with_witness?

sipa commented at 7:10 am on December 11, 2017:

Good point, fixed.

in test/functional/test_framework/messages.py:571 in 53fdb41a0b outdated

582@@ -583,7 +583,7 @@ def serialize(self, with_witness=False):
583         if with_witness:
584             r += ser_vector(self.vtx, "serialize_with_witness")
585         else:
586-            r += ser_vector(self.vtx)
587+            r += ser_vector(self.vtx, "serialize_without_witness")

TheBlueMatt commented at 6:54 pm on December 7, 2017:

Seems weird to keep CBlock default as non-witness when changing transaction default? Maybe @MarcoFalke or @jnewbery have an opinion?

sipa commented at 7:11 am on December 11, 2017:

I have no strong opinions, but it was a bit of a pain to make this work. So feel free to suggest a more consistent approach if you find one that works, but I’d rather not try to do this again.

in src/script/standard.h:78 in 7cf2eea1eb outdated

72@@ -73,8 +73,21 @@ class CNoDestination {
73     friend bool operator<(const CNoDestination &a, const CNoDestination &b) { return true; }
74 };
75 
76-struct WitnessV0ScriptHash : public uint256 {};
77-struct WitnessV0KeyHash : public uint160 {};
78+struct WitnessV0ScriptHash : public uint256
79+{
80+public:

ryanofsky commented at 8:14 pm on December 7, 2017:

In commit “Improve witness destination types and use them more”

Could drop public: since this is a struct, or s/struct/class/ since parent is a class (also for WitnessV0KeyHash).

sipa commented at 7:12 am on December 11, 2017:

Done.

in src/qt/paymentserver.cpp:648 in 210e90fcbf outdated

644@@ -645,10 +645,10 @@ void PaymentServer::fetchPaymentACK(CWallet* wallet, const SendCoinsRecipient& r
645     else {
646         CPubKey newKey;
647         if (wallet->GetKeyFromPool(newKey)) {
648-            CKeyID keyID = newKey.GetID();
649-            wallet->SetAddressBook(keyID, strAccount, "refund");
650+            CTxDestination dest = wallet->GetDestinationForKey(newKey, g_address_type);

TheBlueMatt commented at 10:50 pm on December 7, 2017:

Shouldnt this really be g_change_type since its not a script that is expected to be expressed anywhere in address form?

sipa commented at 7:07 am on December 11, 2017:

Good point. Fixed, and added a comment.

in src/wallet/wallet.cpp:4198 in e2e9ead25f outdated

4181+        // Check if the resulting program is solvable (i.e. doesn't use an uncompressed key)
4182+        if (!IsSolvable(*this, witprog)) return key.GetID();
4183+        // Even though CBasicKeyStore knows about P2WPKH redeemscripts implicitly, add it explicitly
4184+        // so that downgrades to software without this are supported (as long as the wallet file is
4185+        // up to date).
4186+        AddCScript(witprog);

TheBlueMatt commented at 10:58 pm on December 7, 2017:

Hmm, can we avoid making this a db write if we already have the script in the wallet explicitly somehow?

sipa commented at 7:10 am on December 11, 2017:

That seems hard, as we don’t distinguish scripts that were added explicitly and implicitly.

ryanofsky commented at 8:28 pm on December 12, 2017:

That seems hard, as we don’t distinguish scripts that were added explicitly and implicitly.

Is there a reason not to distinguish scripts that were added explicitly and implicitly? It seems like it would just require storing { CScript script; bool ephemeral; } in mapScripts. Probably better not to do in this PR, but could this be a good idea for a followup?

TheBlueMatt commented at 10:07 pm on December 12, 2017:

I think its worth trying to figure something out. As it is stuff like listaddressesbyaccount (or, in the future, by labels) will suffer a significant performance regression trying to create DB batches per address, add something thats already there, and then close the batch.

in src/wallet/wallet.cpp:4222 in e2e9ead25f outdated

4204+        CTxDestination witdest = WitnessV0ScriptHash(Hash(script.begin(), script.end()));
4205+        CScript witprog = GetScriptForDestination(witdest);
4206+        // Check if the resulting program is solvable (i.e. doesn't use an uncompressed key)
4207+        if (!IsSolvable(*this, witprog)) return CScriptID(script);
4208+        // Add the redeemscript, so that P2WSH and P2SH-P2WSH outputs are recognized as ours.
4209+        AddCScript(witprog);

TheBlueMatt commented at 11:01 pm on December 7, 2017:

This enforces redeemScript being no larger than 520 bytes, which is somewhat strange. I think its fine cause its all checked for at higher (read: RPC) layers, but its something to note that we dont (yet) support larger P2WSH redeemScripts. To do so we probably need to bump wallet minversion to avoid confusing old versions why we have scripts larger than 520 bytes in the wallet.

sipa commented at 7:10 am on December 11, 2017:

Added a comment.

in src/wallet/wallet.cpp:1053 in e2e9ead25f outdated

1049@@ -1048,6 +1050,7 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const CBlockI
1050                     std::map<CKeyID, int64_t>::const_iterator mi = m_pool_key_to_index.find(keyid);
1051                     if (mi != m_pool_key_to_index.end()) {
1052                         LogPrintf("%s: Detected a used keypool key, mark all keypool key up to this key as used\n", __func__);
1053+                        AddRelatedScripts(mi->first);

TheBlueMatt commented at 11:11 pm on December 7, 2017:

Why not check if it was actually affected in segwit form before doing this? We only otherwise AddCScript if we are getting the segwit version of an address.

sipa commented at 7:09 am on December 11, 2017:

Done.

TheBlueMatt commented at 11:12 pm on December 7, 2017: member

Didn’t get through a thourough review, mostly just read new code.

TheBlueMatt commented at 11:14 pm on December 7, 2017: member

Oh, obviously needs some docs that describe how to find lost funds if you downgrade and then upgrade again (rescan from segwit activation height). Though do we want some explicit RPC for this or so that rescans from max(oldest key time, segwit activation height)?

in src/script/standard.h:83 in 7cf2eea1eb outdated

81+    WitnessV0ScriptHash() : uint256() {}
82+    WitnessV0ScriptHash(const uint256& hash) : uint256(hash) {}
83+    using uint256::uint256;
84+};
85+
86+struct WitnessV0KeyHash : public uint160

ryanofsky commented at 4:44 pm on December 8, 2017:

In commit “Improve witness destination types and use them more”

Should this inherit from CKeyID instead of uint160?

Also, would it make sense as a followup to replace CKeyID and CScriptID in the destination variant with NonWitnessKeyHash and NonWitnessScriptHash types for better type checking and more consistency between old and new destinations?

sipa commented at 7:14 am on December 11, 2017:

I tried making it inherit from CKeyID, but it’s not trivial at least, so I’d rather do it separately.

Agreed with replacing CKeyID and CScriptID in the variant. Though perhaps for performance reasons we may want a specialized variant implementation for this (boost::variant needs dynamic allocations) anyway. Let’s consider that for later.

in src/script/standard.h:80 in 7cf2eea1eb outdated

77-struct WitnessV0KeyHash : public uint160 {};
78+struct WitnessV0ScriptHash : public uint256
79+{
80+public:
81+    WitnessV0ScriptHash() : uint256() {}
82+    WitnessV0ScriptHash(const uint256& hash) : uint256(hash) {}

ryanofsky commented at 5:04 pm on December 8, 2017:

In commit “Improve witness destination types and use them more”:

Should probably declare this explicit for better type checking (also for WitnessV0KeyHash).

sipa commented at 7:12 am on December 11, 2017:

Done.

ryanofsky referenced this in commit 9c8eca7704 on Dec 8, 2017

in src/keystore.cpp:25 in da32d5097b outdated

20+    // outputs. Technically P2WPKH outputs don't have a redeemscript to be
21+    // spent. However, our current IsMine logic requires the corresponding
22+    // P2SH-P2WPKH redeemscript to be present in the wallet in order to accept
23+    // payment even to P2WPKH outputs.
24+    if (pubkey.IsCompressed()) {
25+        CScript script = CScript() << OP_0 << ToByteVector(pubkey.GetID());

ryanofsky commented at 8:01 pm on December 8, 2017:

In commit “Implicitly know about P2WPKH redeemscripts”

Seems like you could use GetScriptForDestination here like you do in CWallet::AddRelatedScripts? Or is there a reason why it is better there but not here?

sipa commented at 7:11 am on December 11, 2017:

Done.

in src/wallet/wallet.cpp:1053 in cb6cd672f4 outdated

1049@@ -1050,6 +1050,7 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const CBlockI
1050                     std::map<CKeyID, int64_t>::const_iterator mi = m_pool_key_to_index.find(keyid);
1051                     if (mi != m_pool_key_to_index.end()) {
1052                         LogPrintf("%s: Detected a used keypool key, mark all keypool key up to this key as used\n", __func__);
1053+                        AddRelatedScripts(mi->first);

ryanofsky commented at 8:25 pm on December 8, 2017:

In commit “Support downgrading after recovered keypool witness keys”

Since we know the CPubKey (and are already looking it up in the next line), could this just call the CKeyStore::AddRelatedKeyScripts() method instead of requiring a new CWallet::AddRelatedScripts() method? It would be helpful to have an explanatory comment here whatever the answer is. And maybe the names of the methods could be made more distinct.

sipa commented at 7:15 am on December 11, 2017:

I’ve renamed the CKeyStore::AddRelatedKeyScripts functions to CKeyStore::ImplicitlyLearnRelatedKeyScripts. It’s not usable because that function only adds the script in memory. The point of AddRelatedScript is to make them explicit (in wallet.dat), and it will only be called on keys whose related scripts are already implicitly in the keystore.

in src/wallet/wallet.cpp:4160 in 210e90fcbf outdated

4155+    case OUTPUT_TYPE_BECH32: return OUTPUT_TYPE_STRING_BECH32;
4156+    default: assert(false);
4157+    }
4158+}
4159+
4160+std::vector<CTxDestination> CWallet::GetAllDestinationsForKey(const CPubKey& key)

ryanofsky commented at 8:33 pm on December 8, 2017:

In commit “SegWit wallet support”

Seems like this could be a standalone method rather than a wallet or keystore method. Should probably at least make it static, if not moved out of the wallet entirely.

sipa commented at 7:16 am on December 11, 2017:

Made it a standalone wallet function. I’m leaving it as part of the wallet, as the choice about what types of addresses to support is largely a wallet one.

in src/wallet/wallet.cpp:4193 in 210e90fcbf outdated

4175+    case OUTPUT_TYPE_LEGACY: return key.GetID();
4176+    case OUTPUT_TYPE_P2SH:
4177+    case OUTPUT_TYPE_BECH32: {
4178+        CTxDestination witdest = WitnessV0KeyHash(key.GetID());
4179+        CScript witprog = GetScriptForDestination(witdest);
4180+        // Check if the resulting program is solvable (i.e. doesn't use an uncompressed key)

ryanofsky commented at 8:35 pm on December 8, 2017:

In commit “SegWit wallet support”

Could the comment be expanded to say what else this is checking for besides the uncompressed key? The key is available so obviously that could be checked directly. And the code above in GetAllDestinationsForKey is actually just calling IsCompressed(), so it’d be good to say why doing that isn’t sufficient here.

sipa commented at 7:17 am on December 11, 2017:

Checking for an uncompressed key is all it does. It’s just a very succint way of doing so, which also happens to be very foolproof for future similar changes.

in src/wallet/wallet.cpp:4198 in 210e90fcbf outdated

4180+        // Check if the resulting program is solvable (i.e. doesn't use an uncompressed key)
4181+        if (!IsSolvable(*this, witprog)) return key.GetID();
4182+        // Even though CBasicKeyStore knows about P2WPKH redeemscripts implicitly, add it explicitly
4183+        // so that downgrades to software without this are supported (as long as the wallet file is
4184+        // up to date).
4185+        AddCScript(witprog);

ryanofsky commented at 10:19 pm on December 8, 2017:

In commit “SegWit wallet support”

As brought up previously #11403 (review), this seems really ugly. Having GetDestinationForKey/GetDestinationForScript methods write to the wallet database when all you would expect them to do is get the address of a key or script doesn’t seem safe or untuitive.

It seems like it would be better if the callers were responsible for adding CScripts. Since most callers are just calling GetDestinationForKey after reserving keys, I think if you added the CScripts in CWallet::ReserveKeyFromKeyPool it would take care of most of them.

If that isn’t sufficien,t maybe there is some way to rename or wrap these methods so it is clear the calls are intended to actually update the database, rather than just compute addresses.

sipa commented at 7:20 am on December 11, 2017:

I agree it’s ugly, though I think it is temporary (it’s not needing in my future design (see writeup), and it’s also unnecessary once we drop support for downgrading to pre-implicit-SegWit wallet software - but that needs some thinking about versioning).

Splitting the choice for what to add and what destination to use up seems very inconvenient, though. The necessity of adding depends on the type of address.

I’ve renamed the functions to Add... rather than Get..., to reflect that they may change wallet state.

sipa force-pushed on Dec 11, 2017

sipa commented at 7:26 am on December 11, 2017: member

Pushed a change addressing:

Several of @TheBlueMatt and @ryanofsky’s comments.
Rewrote the validateaddress changes; behaviour of P2SH and P2WSH is now much more consistent and documented (both will return embedded if possible). Also added a pubkeys field (as a replacement for the confusing addresses field for multisig, which remains supported for P2SH).
Renamed several functions (ImplicitlyLearnRelatedScripts, AddDestinationForKey, AddDestinationForScript).
Expanded address_types.py to include validateaddress and addmultisigaddress coverage.

No rebase, but the fixes are all squashed (e2e9ead25fe0ddb364e35f8eabb5a7f937d3973b -> 5b669dd51ae871d55312d2e83ca6c28b2e1e60fb).

sipa force-pushed on Dec 11, 2017

Sjors commented at 11:31 am on December 11, 2017: member

The balance issues I found disappear when I cherry-pick #11839.

in src/qt/paymentserver.cpp:649 in 04e82bca42 outdated

644@@ -645,10 +645,12 @@ void PaymentServer::fetchPaymentACK(CWallet* wallet, const SendCoinsRecipient& r
645     else {
646         CPubKey newKey;
647         if (wallet->GetKeyFromPool(newKey)) {
648-            CKeyID keyID = newKey.GetID();
649-            wallet->SetAddressBook(keyID, strAccount, "refund");
650+            // As we don't care about whether the other side understands a particular address type
651+            // encoding, use the change type here.

Sjors commented at 11:53 am on December 11, 2017:

Maybe replace the comment with, if I understand correctly: “We’ll use the change type here. In the BIP-70 payment protocol the destination for refunds is communicated as an output script, not an address. Therefore we don’t need to care about whether the other side understands a particular address type.”

sipa commented at 11:57 pm on December 11, 2017:

I’ve reformulated it.

in src/qt/paymentserver.cpp:653 in 04e82bca42 outdated

644@@ -645,10 +645,12 @@ void PaymentServer::fetchPaymentACK(CWallet* wallet, const SendCoinsRecipient& r
645     else {
646         CPubKey newKey;
647         if (wallet->GetKeyFromPool(newKey)) {
648-            CKeyID keyID = newKey.GetID();
649-            wallet->SetAddressBook(keyID, strAccount, "refund");
650+            // As we don't care about whether the other side understands a particular address type
651+            // encoding, use the change type here.
652+            CTxDestination dest = wallet->AddDestinationForKey(newKey, g_change_type);

Sjors commented at 11:56 am on December 11, 2017:

Do we really want to use the change type? I think refund addresses should be treated the same as any receive address, including what type you use for those,and the level of trust placed on unconfirmed funds on those addresses. The latter is probably not an issue here.

In a BIP44 address hierarchy I would probably use the receive chain for refunds (not applicable here, but to illustrate my point).

sipa commented at 11:58 pm on December 11, 2017:

I have elaborated on why.

in src/wallet/init.cpp:19 in 04e82bca42 outdated

14@@ -15,6 +15,8 @@
15 std::string GetWalletHelpString(bool showDebug)
16 {
17     std::string strUsage = HelpMessageGroup(_("Wallet options:"));
18+    strUsage += HelpMessageOpt("-addresstype", strprintf(_("What type of addresses to use (\"legacy\", \"p2sh\", \"bech32\", default: \"%s\")"), FormatOutputType(OUTPUT_TYPE_DEFAULT)));
19+    strUsage += HelpMessageOpt("-changetype", strprintf(_("What type of change to use (\"legacy\", \"p2sh\", \"bech32\", default is same as -addressstyle: \"%s\")"), FormatOutputType(OUTPUT_TYPE_DEFAULT)));

Sjors commented at 12:12 pm on December 11, 2017:

-addressstyle should be -addresstype (I can’t see my previous comment about this anymore, so repeating just in case)

sipa commented at 0:01 am on December 12, 2017:

Fixed.

in src/wallet/wallet.cpp:4234 in 04e82bca42 outdated

4204+    case OUTPUT_TYPE_BECH32: {
4205+        WitnessV0ScriptHash hash;
4206+        CSHA256().Write(script.data(), script.size()).Finalize(hash.begin());
4207+        CTxDestination witdest = hash;
4208+        CScript witprog = GetScriptForDestination(witdest);
4209+        // Check if the resulting program is solvable (i.e. doesn't use an uncompressed key)

Sjors commented at 12:19 pm on December 11, 2017:

“i.e.” -> “e.g.” seems more future-proof.

sipa commented at 0:01 am on December 12, 2017:

That’s not what I mean. It’s not just an example - it’s a clarification about why that check is needed.

Sjors commented at 8:04 am on December 12, 2017:

So “i.e.” refers to the reason you need to call IsSolvable, not to what IsSolvable does? Maybe change the comment to “Check if the resulting program doesn’t use an uncompressed key”?

ryanofsky commented at 9:59 pm on December 12, 2017:

#11403 (review)

So “i.e.” refers to the reason you need to call IsSolvable, not to what IsSolvable does? Maybe change the comment to “Check if the resulting program doesn’t use an uncompressed key”?

Funny, even though I originally mistook “i.e.” for “e.g.” when reading the same comment in AddDestinationForKey above (https://github.com/bitcoin/bitcoin/pull/11403#discussion_r155870014), I think sipa’s current comment is clearer than your suggestion, because your suggestion doesn’t directly mention the solvable call.

Also, strictly speaking, unlike in AddDestinationForKey above, IsSolveable here is not only checking for uncompressed keys but also verifying that script was previously added to the wallet.

in test/functional/address_types.py:34 in 04e82bca42 outdated

29+                        addr1 = self.nodes[i].getnewaddress()
30+                        addr2 = self.nodes[i].getnewaddress()
31+                        address = self.nodes[i].addmultisigaddress(2, [addr1, addr2])
32+                    # Do some sanity checking on the created address
33+                    info = self.nodes[i].validateaddress(address)
34+                    print("%i: %r" % (i, info))

Sjors commented at 12:25 pm on December 11, 2017:

Don’t forget to remove this, or use self.log.debug(...).

sipa commented at 0:30 am on December 12, 2017:

Done.

in test/functional/address_types.py:62 in 04e82bca42 outdated

57+                sync_blocks(self.nodes)
58+                new_balances = [self.nodes[i].getbalance() for i in range(4)]
59+                for k in range(3):
60+                    i = (n + k + 1) % 4
61+                    assert_equal(unconf_balances[i], to_send * (2 + k))
62+                    assert_equal(new_balances[i], old_balances[i] + to_send * (2 + k) + (50 if i == 0 else 0))

Sjors commented at 12:43 pm on December 11, 2017:

On my local machine, this line fails with AssertionError: not(1745.04950480 == 1695.04950480) (with and without the #11839 patch). Not sure what could explain an off by 50 BTC error.

It fails when I run the test individually through test/functional/address_types.py. It passes when I run it as part of the full test suite test/functional/test_runner.py , but that in turn causes the (unmodified) wallet-hd.py to fail at line 102 with:

0AssertionError: Block sync to height 402 timed out:
1  {'hash': '62ef92e893723b28577eb0420871b941c8ac6a42c0afc5bf59eb096c8fe72256', 'height': 402}
2  {'hash': '5b9db3fd6a97e193ff780c162941ae6a348a3f29afab04f97620548b68b0ed60', 'height': 393}

When I run this individual test file again after running the full suite, it does pass. 😕

sipa commented at 0:32 am on December 12, 2017:

I can’t reproduce your problem. Are you sure you’re not running against an older bitcoind?

Sjors commented at 7:54 am on December 12, 2017:

I still got the errors after a make clean and make. Not sure if there’s anything else I can run to make sure tests run from a blank slate. I did notice earlier that if you run two tests in parallel in separate tabs, things get really weird.

Sjors commented at 8:01 am on December 12, 2017:

TIL make clean doesn’t erase test/cache. That was probably it. I’ll let you know if I still get this error.

Sjors commented at 1:23 pm on December 12, 2017:

That wasn’t it. It was #11876 (unrelated to this PR, again…).

in test/functional/address_types.py:16 in 04e82bca42 outdated

10+    def set_test_params(self):
11+        self.num_nodes = 4
12+        self.extra_args = [["-addresstype=legacy"], ["-addresstype=p2sh"], ["-addresstype=p2sh", "-changetype=bech32"], ["-addresstype=bech32"]]
13+
14+    def run_test(self):
15+        self.nodes[0].generate(1)

Sjors commented at 1:27 pm on December 11, 2017:

Off topic: why is this needed?

sipa commented at 0:30 am on December 12, 2017:

Otherwise the nodes will think they’re in IBD and not sync transactions.

Sjors commented at 1:50 pm on December 11, 2017: member

Needs a QT label?

I find this latest version much easier to read.

sipa force-pushed on Dec 12, 2017

laanwj added the label GUI on Dec 12, 2017

instagibbs commented at 4:33 pm on December 12, 2017: member

not a request for feature/change, just a note: addmultisigaddress creates a legacy p2sh no matter what argument is set. This means addwitnessaddress is still required for now at least in the case of witness multisig.

Not sure how I got this wrong, it does do addresstype default.

instagibbs commented at 4:54 pm on December 12, 2017: member

addmultisigaddress 1 ‘[“bcrt1qk92l37qy9fvz7pmv88w4m0xz4cqlrag4d5fjxrz79av93mwjh7ssgtkemm”]’ bcrt1qk92l37qy9fvz7pmv88w4m0xz4cqlrag4d5fjxrz79av93mwjh7ssgtkemm does not refer to a key (code -1)

~~While I understand the reasoning behind not allowing it(moving towards always providing a pubkey), the error is pretty vague and confusing if the internals are not understood.~~ I was attempting to use a p2wsh inadvertently.

instagibbs commented at 5:00 pm on December 12, 2017: member

light tACK 0934bf61fbaeb1ea2bf4bd4a99c687a60c9fbf04

in src/script/sign.h:85 in 4dbd4bb8ba outdated

80@@ -81,4 +81,10 @@ SignatureData CombineSignatures(const CScript& scriptPubKey, const BaseSignature
81 SignatureData DataFromTransaction(const CMutableTransaction& tx, unsigned int nIn);
82 void UpdateTransaction(CMutableTransaction& tx, unsigned int nIn, const SignatureData& data);
83 
84+/* Check whether we know how to sign for an output like this, assuming we
85+ * have all private keys. While this function does need keys, the passed

ryanofsky commented at 6:06 pm on December 12, 2017:

In commit “Abstract out IsSolvable from Witnessifier”

s/does need keys/does not need private keys/?

sipa commented at 1:44 am on December 13, 2017:

Done.

in src/rpc/misc.cpp:68 in 05af17c5aa outdated

64+
65+        // Only when the script is multisig (in which case the ExtractDestination above certainly failed):
66+        if (which_type == TX_MULTISIG) {
67+            obj.push_back(Pair("sigsrequired", solutions_data[0][0]));
68+            UniValue pubkeys(UniValue::VARR);
69+            for (size_t i = 1; i < solutions_data.size() - 1; ++i) {

ryanofsky commented at 6:46 pm on December 12, 2017:

In commit “Extend validateaddress information for P2SH-embedded witness”

This seems like it is duplicating code in ExtractDestinations. Is there a reason that can’t be reused here?

sipa commented at 0:07 am on December 13, 2017:

Yes.

The short term answer is that ExtractDestinations doesn’t actually report destination(s) of a script, but reports either (a) the destination a script corresponds to or (b) the destinations corresponding to the P2PKH versions of the public keys involved in a multisig transactions. In this branch, we don’t want public-keys-encoded-as-destinations but the actual public keys.

The longer term answer is that ExtractDestinations is nonsensical and we should get rid of it. A script doesn’t contain multiple destinations. It either corresponds to a single destination (anything we can treat as an address), or not (which is the case for multisig scripts). Historically, we didn’t distinguish between destinations and keys (and P2PKH addresses were just the way to identify a key). I’ll add a TODO that ExtractDestinations should be phased out and replaced.

ryanofsky commented at 8:51 pm on December 13, 2017:

#11403 (review)

Thanks, really appreciate the clarification (and TODOs).

By the same logic, would you also want to deprecate the “addresses” field since it is also using addresses to refer to keys that are not destinations?

sipa commented at 1:24 am on December 14, 2017:

Added a comment about that too.

in src/rpc/misc.cpp:181 in 05af17c5aa outdated

183+            "      \"pubkey\"\n"
184+            "      ,...\n"
185+            "    ]\n"
186+            "  \"sigsrequired\" : xxxxx        (numeric, optional) Number of signatures required to spend multisig output (only if \"script\" is \"multisig\")\n"
187+            "  \"pubkey\" : \"publickeyhex\",    (string, optional) The hex value of the raw public key\n"
188+            "  \"embedded\" : {...},           (object, optional) validateaddress output for the address embedded in P2SH or P2WSH, if any and known. Includes fields: address, pubkey, isscript, script, hex, sigsrequired, pubkeys, addresses, iswitness, witness_version, witness_program\n"

ryanofsky commented at 6:51 pm on December 12, 2017:

In commit “Extend validateaddress information for P2SH-embedded witness”

List of included fields is confusing. Could comment characterize which type of fields are included and which fields aren’t?

sipa commented at 1:44 am on December 13, 2017:

Done.

laanwj referenced this in commit 22149540f9 on Dec 12, 2017

sipa force-pushed on Dec 12, 2017

sipa commented at 7:17 pm on December 12, 2017: member

Rebased after merge of #11854.

in src/keystore.cpp:16 in 8e42f9c1c9 outdated

10@@ -11,6 +11,22 @@ bool CKeyStore::AddKey(const CKey &key) {
11     return AddKeyPubKey(key, key.GetPubKey());
12 }
13 
14+void CBasicKeyStore::ImplicitlyLearnRelatedKeyScripts(const CPubKey& pubkey)
15+{
16+    AssertLockHeld(cs_KeyStore);

ryanofsky commented at 7:26 pm on December 12, 2017:

In commit “Implicitly know about P2WPKH redeemscripts”

Could you add assert(mapKeys.count(pubkey.GetID()) || mapWatchKeys.count(pubkey.GetID())?

The need to add implicit scripts is confusing enough that I think it would help to document assumptions and potentially catch bugs in the implementation adding them.

sipa commented at 11:18 pm on December 12, 2017:

I don’t think those asserts are correct for encrypted wallets.

ryanofsky commented at 10:33 pm on December 13, 2017:

#11403 (review)

I don’t think those asserts are correct for encrypted wallets.

Following seems to work (if you also move the two ImplicitlyLearnRelatedKeyScripts() calls one line down in CBasicKeyStore::AddKeyPubKey and CCryptoKeyStore::AddCryptedKey):

0assert(HaveKey(pubkey.GetID()) || mapWatchKeys.count(pubkey.GetID()));

sipa commented at 1:24 am on December 14, 2017:

Added.

in src/keystore.cpp:24 in 8e42f9c1c9 outdated

16+    AssertLockHeld(cs_KeyStore);
17+    // This adds the redeemscripts necessary to detect P2WPKH and P2SH-P2WPKH
18+    // outputs. Technically P2WPKH outputs don't have a redeemscript to be
19+    // spent. However, our current IsMine logic requires the corresponding
20+    // P2SH-P2WPKH redeemscript to be present in the wallet in order to accept
21+    // payment even to P2WPKH outputs.

ryanofsky commented at 7:29 pm on December 12, 2017:

In commit “Implicitly know about P2WPKH redeemscripts”

Maybe add a sentence like:

“Implicitly” refers to fact that scripts are derived automatically from existing keys, and are only stored in memory, not written to disk.

It might also be worth saying why they aren’t written to disk. It’s not clear if that would be completely useless in any downgrade scenario or if it just doesn’t help with one of the downgrade scenarios we care about.

sipa commented at 1:43 am on December 13, 2017:

Adding with a slight modification.

in src/wallet/wallet.cpp:4237 in 212d794ea5 outdated

4232+    CTxDestination segwit = WitnessV0KeyHash(keyid);
4233+    CTxDestination p2sh = CScriptID(GetScriptForDestination(segwit));
4234+    // For SegWit scriptPubKeys, add the redeemscript explicitly to the wallet, so that
4235+    // older software versions (without implicit scripts) correctly treat these as ours.
4236+    if (scriptPubKey == GetScriptForDestination(segwit) || scriptPubKey == GetScriptForDestination(p2sh)) {
4237+        CTxDestination witdest = WitnessV0KeyHash(keyid);

ryanofsky commented at 7:57 pm on December 12, 2017:

In commit “Support downgrading after recovered keypool witness keys”

Variable duplicates segwit above.

sipa commented at 1:44 am on December 13, 2017:

Fixed.

in src/wallet/wallet.cpp:4238 in 212d794ea5 outdated

4233+    CTxDestination p2sh = CScriptID(GetScriptForDestination(segwit));
4234+    // For SegWit scriptPubKeys, add the redeemscript explicitly to the wallet, so that
4235+    // older software versions (without implicit scripts) correctly treat these as ours.
4236+    if (scriptPubKey == GetScriptForDestination(segwit) || scriptPubKey == GetScriptForDestination(p2sh)) {
4237+        CTxDestination witdest = WitnessV0KeyHash(keyid);
4238+        CScript witprog = GetScriptForDestination(witdest);

ryanofsky commented at 7:58 pm on December 12, 2017:

In commit “Support downgrading after recovered keypool witness keys”

This is also computed in the if statement. Maybe declare this earlier and reuse it there.

sipa commented at 1:44 am on December 13, 2017:

Done.

in src/wallet/wallet.cpp:4239 in 212d794ea5 outdated

4234+    // For SegWit scriptPubKeys, add the redeemscript explicitly to the wallet, so that
4235+    // older software versions (without implicit scripts) correctly treat these as ours.
4236+    if (scriptPubKey == GetScriptForDestination(segwit) || scriptPubKey == GetScriptForDestination(p2sh)) {
4237+        CTxDestination witdest = WitnessV0KeyHash(keyid);
4238+        CScript witprog = GetScriptForDestination(witdest);
4239+        if (IsSolvable(*this, witprog)) {

ryanofsky commented at 7:59 pm on December 12, 2017:

In commit “Support downgrading after recovered keypool witness keys”

Could you add a comment saying what it means if IsSolvable is false? It seems like that should be an error or maybe never happen.

sipa commented at 1:44 am on December 13, 2017:

I removed the check, and added a comment why it’s not needed.

in src/wallet/wallet.cpp:4235 in 212d794ea5 outdated

4225@@ -4225,3 +4226,18 @@ CTxDestination CWallet::AddDestinationForScript(const CScript& script, OutputTyp
4226     default: assert(false);
4227     }
4228 }
4229+
4230+void CWallet::AddRelatedScripts(const CScript& scriptPubKey, const CKeyID& keyid)

ryanofsky commented at 8:12 pm on December 12, 2017:

In commit “Support downgrading after recovered keypool witness keys”

Can this assert(mapKeys.count(keyid))?

sipa commented at 0:46 am on December 13, 2017:

No, that wouldn’t work for encrypted wallets.

ryanofsky commented at 11:02 pm on December 13, 2017:

#11403 (review)

No, that wouldn’t work for encrypted wallets.

I think assert(HaveKey(keyid)) should work.

sipa commented at 1:25 am on December 14, 2017:

Added.

in src/wallet/wallet.cpp:4235 in a7661006b8 outdated

4225+    }
4226+    default: assert(false);
4227+    }
4228+}
4229+
4230+void CWallet::AddRelatedScripts(const CScript& scriptPubKey, const CKeyID& keyid)

ryanofsky commented at 9:47 pm on December 12, 2017:

In commit “Support downgrading after recovered keypool witness keys”

Can this assert(mapKeys.count(keyid))?

sipa commented at 1:47 am on December 13, 2017:

No, that wouldn’t work for encrypted wallets (which use mapCryptedKeys).

in src/wallet/wallet.cpp:4240 in a7661006b8 outdated

4235+    // older software versions (without implicit scripts) correctly treat these as ours.
4236+    if (scriptPubKey == GetScriptForDestination(segwit) || scriptPubKey == GetScriptForDestination(p2sh)) {
4237+        CTxDestination witdest = WitnessV0KeyHash(keyid);
4238+        CScript witprog = GetScriptForDestination(witdest);
4239+        if (IsSolvable(*this, witprog)) {
4240+            AddCScript(witprog);

ryanofsky commented at 9:50 pm on December 12, 2017:

In commit “Support downgrading after recovered keypool witness keys”

No test seems to fail if this is commented out. Since this is only needed for downgrades it’d be good to have some kind of check to know if it stops working.

sipa commented at 1:48 am on December 13, 2017:

Likewise, this is untestable because it only affects downgrading.

in src/wallet/wallet.cpp:4198 in a7661006b8 outdated

4192+        // Check if the resulting program is solvable (i.e. doesn't use an uncompressed key)
4193+        if (!IsSolvable(*this, witprog)) return key.GetID();
4194+        // Even though CBasicKeyStore knows about P2WPKH redeemscripts implicitly, add it explicitly
4195+        // so that downgrades to software without this are supported (as long as the wallet file is
4196+        // up to date).
4197+        AddCScript(witprog);

ryanofsky commented at 9:54 pm on December 12, 2017:

In commit “SegWit wallet support”

No test seems to fail if this is unset. It’d be good to have a check to know if this stops working.

sipa commented at 1:47 am on December 13, 2017:

Yes, that’s inevitable.

We have two conditions under which we add the script explicitly (when generating a new address, and when noticing an IsMine keypool address on the network). Both of these always already have the script added implicitly, so there is no change in behaviour from the explicit add. It’s just there for compatibility with downgrading to earlier software, which we can’t test for in the current test framework AFAIK.

ryanofsky commented at 10:53 pm on December 13, 2017:

#11403 (review)

It’s just there for compatibility with downgrading to earlier software, which we can’t test for in the current test framework AFAIK.

I see the difficulty, and maybe this should be saved for a followup, but I could imagine testing this by subclassing CWallet in a unit test, overriding AddCScript, and ensuring the correct calls are made. Another option could be to trigger this code from python, then shut down the node and do the equivalent of popen("db_dump wallet.dat") and make sure the expected cscript entries are present.

sipa commented at 1:25 am on December 14, 2017:

Yes, right - you can do a unit test for this; it’s only end-to-end testing that’s hard.

sipa force-pushed on Dec 13, 2017

sipa commented at 1:55 am on December 13, 2017: member

Updated to address some of @ryanofsky’s comments: a7661006b82484969506da49ffc695d2a5bea3d2 -> 50038c2cc7d6456a6725e120b0a11537585e2415.

in src/script/sign.cpp:438 in b2c21fce66 outdated

432+    DummySignatureCreator creator(&store);
433+    SignatureData sigs;
434+    // Make sure that STANDARD_SCRIPT_VERIFY_FLAGS includes SCRIPT_VERIFY_WITNESS_PUBKEYTYPE, the most
435+    // important property this function is designed to test for.
436+    static_assert(STANDARD_SCRIPT_VERIFY_FLAGS & SCRIPT_VERIFY_WITNESS_PUBKEYTYPE, "IsSolvable requires standard script flags to include WITNESS_PUBKEYTYPE");
437+    return (ProduceSignature(creator, script, sigs) && VerifyScript(sigs.scriptSig, script, &sigs.scriptWitness, STANDARD_SCRIPT_VERIFY_FLAGS, creator.Checker()));

ryanofsky commented at 8:48 pm on December 13, 2017:

In commit “Abstract out IsSolvable from Witnessifier”

When would ProduceSignature be expected to succeed but VerifyScript be expected to fail? Asking because I don’t know the answer, but also because I think it would be helpful to have a comment here that says whether VerifyScript here changes the logic or is just defensive.

sipa commented at 1:33 am on December 14, 2017:

It’s just defense now, as ProduceSignature already calls VerifyScript with the exact same arguments. I guess we could get rid of it, but it feels much safer this way.

ryanofsky commented at 4:56 pm on December 14, 2017:

#11403 (review)

It’s just defense now

Could you add a comment like “VerifyScript check is just defensive, and should never fail.” I was pretty confused by this code and thought I might be missing something about the way ProduceSignature worked.

promag commented at 1:29 am on December 29, 2017:

should never fail

Sounds like an assert:

0if (!ProduceSignature(creator, script, sigs)) return false;
1assert(VerifyScript(sigs.scriptSig, script, &sigs.scriptWitness, STANDARD_SCRIPT_VERIFY_FLAGS, creator.Checker()));
2return true;

in src/rpc/misc.cpp:49 in f20398b805 outdated

44+    {
45+        // Always present: script type and redeemscript
46+        txnouttype which_type;
47+        std::vector<std::vector<unsigned char>> solutions_data;
48+        Solver(subscript, which_type, solutions_data);
49+        obj.push_back(Pair("script", GetTxnOutputType(which_type)));

ryanofsky commented at 8:54 pm on December 13, 2017:

In commit “Extend validateaddress information for P2SH-embedded witness”

Just FYI, but could pushKV() instead of push_back(Pair()) throughout this function.

sipa commented at 1:26 am on December 14, 2017:

Done.

in src/rpc/misc.cpp:60 in f20398b805 outdated

55+        if (ExtractDestination(subscript, embedded)) {
56+            UniValue subobj = boost::apply_visitor(*this, embedded);
57+            subobj.push_back(Pair("address", EncodeDestination(embedded)));
58+            subobj.push_back(Pair("scriptPubKey", HexStr(subscript.begin(), subscript.end())));
59+            if (subobj.exists("pubkey")) {
60+                obj.push_back(Pair("pubkey", subobj["pubkey"]));

ryanofsky commented at 9:30 pm on December 13, 2017:

In commit “Extend validateaddress information for P2SH-embedded witness”

Why mirror this one field from the embedded object into the parent object and not others? E.g. would it make sense to mirror the “pubkeys” field as well, since it seems to be the equivalent of “pubkey” for multisig scripts?

Also is it worth noting in validateaddress documentation that this will contain pubkey from either a top level or an embedded script?

sipa commented at 1:28 am on December 14, 2017:

Added a comment, and in the RPC documentation.

The reason is simply to not silently break backward compatibility with the expected ability to call validateaddress(getnewaddress())['pubkey']. That concern doesn’t exist for ‘pubkeys’.

ryanofsky commented at 11:08 pm on December 13, 2017: member

utACK 50038c2cc7d6456a6725e120b0a11537585e2415 for C++ code. Haven’t looked closely at tests yet. @sipa thanks so much for answering all my questions and adding so many comments. It’s really helped me understand a lot of wallet code I didn’t get before.

sipa force-pushed on Dec 14, 2017

sipa commented at 1:50 am on December 14, 2017: member

Added in more comments in response to @ryanofsky’s review. 50038c2cc7d6456a6725e120b0a11537585e2415 -> 23bbb033f6551b826ab63cdb57980e483ccecf92.

in src/rpc/misc.cpp:80 in 23bbb033f6 outdated

75+                pubkeys.push_back(HexStr(key.begin(), key.end()));
76+            }
77+            obj.pushKV("pubkeys", std::move(pubkeys));
78+        }
79+
80+        // Only add the 'addresses' field when needed for backward compatibility. New applications can use

ryanofsky commented at 4:32 pm on December 14, 2017:

#11403 (review)

Maybe add a sentence like “The addresses field is problematic because it confuses keys and destination addresses, using addresses to refer to multisig keys that should not be treated like destinations.”

sipa commented at 7:43 pm on December 15, 2017:

Done.

ryanofsky commented at 5:13 pm on December 14, 2017: member

utACK 23bbb033f6551b826ab63cdb57980e483ccecf92. Change looks great and I hope this close to being merged!

I collected remaining TODOs from comments above. IMO, none are particularly important, and it’d be fine to skip or save any of these for future PRs.

add -addresstype and -changetype parsing tests #11403 (review), #11403 (review)
add getnewaddress address_type parsing test #11403 (review)
add getrawchangeaddress address_type parsing test #11403 (review), #11403 (review)
avoid redundant AddCScript calls in AddDestinationForKey, AddDestinationForScript, and AddRelatedScripts #11403 (review)
remove or move AddCScript calls in AddDestinationForKey, AddDestinationForScript by either dropping support for downgrading or by inserting scripts in more appropriate place, like on removal of keys from keypool #11403 (review)
document downgrade/upgrade procedure #11403 (comment)
replace overloaded CKeyID/CScriptID types in CTxDestination variant #11403 (review)
add some kind of test coverage for downgrade code that we may never know is broken #11403 (review)

jnewbery commented at 10:29 pm on December 14, 2017: member

@TheBlueMatt asked me to review the integration tests part of this. I got as far as the new address_types test, but I found it really difficult to read because of the nested for loops with unobvious index names. I ended up rewriting it completely here: https://github.com/jnewbery/bitcoin/tree/pr11403.1 @sipa - can you take a look. Feel free to take the commit entirely or pick out bits that you like. I made a few other changes while I was there:

moved block generation to a 5th node, so accounting in the test didn’t need to worry about coinbases maturing out of nowhere.
overrode the setup_network() method so all nodes are connected in a mesh. Default topology in the test framework is a chain, and I noticed tx propogation was really slow. changing to a full mesh network cuts test time in half.
added commenting and logging.
added checking for the sending node’s {unconfirmed,} balance after each iteration.

Your original test iterated over each node 4 times (twice for single key addresses and twice for multisig addresses). I don’t know if that’s strictly necessary, but I’ve retained it in my version of the test.

sipa force-pushed on Dec 15, 2017

sipa commented at 7:45 pm on December 15, 2017: member

@ryanofsky Thanks a lot for your extensive review and agree with most things on the TODO list, though I prefer not expanding the scope in this PR right now. @jnewbery Thanks, that looks easier to read indeed. I’ve squashed it into my commit, and made some more changes.

All: I noticed that when selecting -addresstype=p2sh, addmultisigaddress would produce P2SH-multisig addresses rather than P2SH-P2WSH-multisig (which isn’t really consistent with getnewaddress which would produce P2SH-P2WPKH in that case). I’ve rectified this, and adapted the tests.

sipa force-pushed on Dec 15, 2017

laanwj commented at 8:37 am on December 16, 2017: member

Fails the windows tests currently on travis:

0Running A272 test cases...
1
2ssertion failed!
3
4Program: Z:\home\travis\build\bitcoin\bitcoin\build\bitcoin-x86_64-w64-mingw32\src\test\test_bitcoin.exe
5File: wallet/wallet.cpp, Line 4202
6
7Expression: false

FelixWeis commented at 3:58 pm on December 17, 2017: contributor

What’s the rationale for not using bech32 by default for -changetype? They are never user-facing so (short term) compatibility isn’t an issue and the sender will enjoy full segwit savings for successive spending operations.

Ulmo commented at 5:50 pm on December 17, 2017: none

I’ve never used GIT for specific patches, and pretty new to GIT overall. How do I get a merge patchball from GIT to apply this change to test it? I just did “git pull origin master”, built, and started “bitcoin-qt -addresstype bech32”, and Console’s “getnewaddress” gave me “16ESqniuMJ3eXViX7MiHWSBb2iGN4PcScS”, which is clearly not a Bech32 address.

I did “make check” and everything passed.

flack commented at 6:06 pm on December 17, 2017: contributor

@Ulmo try https://gist.github.com/piscisaureus/3342247 (or Stackoverflow if it doesn’t work)

hsjoberg commented at 7:00 pm on December 17, 2017: contributor

@Ulmo manually doing

0git remote add sipa https://github.com/sipa/bitcoin
1git fetch sipa 201709_segwitwallet2
2git checkout sipa/201709_segwitwallet2

Should work too.

laanwj commented at 7:05 pm on December 17, 2017: member

Tested ACK e721efb

Should work too.

Alternatively (assuming remote origin is pointed to bitcoin/bitcoin):

0git fetch origin pull/11403/head
1git checkout FETCH_HEAD

jonasschnelli commented at 7:24 pm on December 17, 2017: contributor

Tested ACK e721efb2bd3687176c0e93e1c5a9e9e31f76b083

Compiled and run on different environments
Extensively tested on OSX

Gitian build: https://bitcoin.jonasschnelli.ch/build/422 (failed linux build is unrelated)

hsjoberg commented at 7:46 pm on December 17, 2017: contributor

Tested ACK e721efb.

Compiles and runs fine on Linux x86_64 Debian (Mint). Are there any outstanding issues that need help/testing?

TheBlueMatt commented at 8:35 pm on December 17, 2017: member

bip68-sequence.py is broken on this branch (+ the travis assert failures)

in src/rpc/misc.cpp:169 in e721efb2bd outdated

165@@ -131,23 +166,30 @@ UniValue validateaddress(const JSONRPCRequest& request)
166             "\nResult:\n"
167             "{\n"
168             "  \"isvalid\" : true|false,       (boolean) If the address is valid or not. If not, this is the only property returned.\n"
169-            "  \"address\" : \"address\", (string) The bitcoin address validated\n"
170+            "  \"address\" : \"address\",      (string) The bitcoin address validated\n"

TheBlueMatt commented at 8:45 pm on December 17, 2017:

nit: the indentation here is wrong, should get an additional two spaces.

sipa commented at 4:03 am on December 18, 2017:

Will fix.

in src/rpc/misc.cpp:173 in e721efb2bd outdated

173             "  \"iswatchonly\" : true|false,   (boolean) If the address is watchonly\n"
174-            "  \"isscript\" : true|false,      (boolean) If the key is a script\n"
175-            "  \"script\" : \"type\"             (string, optional) The output script type. Possible types: nonstandard, pubkey, pubkeyhash, scripthash, multisig, nulldata, witness_v0_keyhash, witness_v0_scripthash\n"
176-            "  \"hex\" : \"hex\",                (string, optional) The redeemscript for the p2sh address\n"
177-            "  \"addresses\"                   (string, optional) Array of addresses associated with the known redeemscript\n"
178+            "  \"isscript\" : true|false,      (boolean) If the address is P2SH or P2WSH\n"

TheBlueMatt commented at 9:02 pm on December 17, 2017:

technically optional for WitnessUnknown-type addresses.

sipa commented at 4:04 am on December 18, 2017:

Will fix.

in src/rpc/misc.cpp:175 in e721efb2bd outdated

175-            "  \"script\" : \"type\"             (string, optional) The output script type. Possible types: nonstandard, pubkey, pubkeyhash, scripthash, multisig, nulldata, witness_v0_keyhash, witness_v0_scripthash\n"
176-            "  \"hex\" : \"hex\",                (string, optional) The redeemscript for the p2sh address\n"
177-            "  \"addresses\"                   (string, optional) Array of addresses associated with the known redeemscript\n"
178+            "  \"isscript\" : true|false,      (boolean) If the address is P2SH or P2WSH\n"
179+            "  \"iswitness\" : true|false,     (boolean) If the address is P2WPKH, P2WSH, or an unknown witness version\n"
180+            "  \"script\" : \"type\"             (string, optional) The output script type. Only if \"isscript\" is true. Possible types: nonstandard, pubkey, pubkeyhash, scripthash, multisig, nulldata, witness_v0_keyhash, witness_v0_scripthash, witness_unknown\n"

TheBlueMatt commented at 9:04 pm on December 17, 2017:

s/only if "isscript" is true/only if "isscript" is true and the wallet knows what the redeemScript/. There’s a few more of these, I dunno if you want to bother specifying everything that specifically or just focus on #10583.

sipa commented at 4:12 am on December 18, 2017:

Will fix.

juscamarena commented at 9:04 pm on December 17, 2017: none

@FelixWeis if it’s default I’m not sure every service can receive from bech32, looking at coinbase. I’d prefer it, but it I’m wondering how many other services are as broken as coinbase. https://twitter.com/karel_3d/status/939974972163940353

I guess it’s on them to fix it and conform to standard behavior anyway. There is also privacy related issues with that.

TheBlueMatt commented at 9:06 pm on December 17, 2017: member

@FelixWeis the rationale is to not reveal which output of yours is change by default as it would massively harm privacy by default. @juscamarena it shouldnt matter for change, for default address types, indeed, it would be premature to use bech32 by default, but for change we can do whatever we want.

hsjoberg commented at 9:08 pm on December 17, 2017: contributor

@juscamarena Old nodes wouldn’t be able to trust non-standard transactions, and so they won’t show up before they get confirmed. That’s why the payment processor part of Coinbase and BitPay had problems.

I think this applies to P2WPKH in P2SH too, but perhaps someone else can confirm that.

sturles commented at 9:39 pm on December 17, 2017: none

Testing this now.

My address generation script, which already converts to P2SH-P2WPKH and P2WPKH (bech32) as alternatives to users, was confused by the new default behaviour. The P2SH address can’t be converted to bech32 using addwitnessaddress. Not a bug, but new behaviour which may come as a surprise to some.

The embedded address in P2SH, as reported by validateaddress P2SH-address, does not inherit the label (account) of the P2SH address. IMO it should when it was generated with the account as parameter to getnewaddress, but I can work around it by using setaccount separately on the embedded bech32 address.

sipa commented at 9:49 pm on December 17, 2017: member

@sturles You can also generate a bech32 address directly by passing the addresstype parameter to getnewaddress. Long term, I don’t think we’ll be maintaining the possibility of coverting one address type to another.

in src/rpc/misc.cpp:167 in e721efb2bd outdated

174-            "  \"isscript\" : true|false,      (boolean) If the key is a script\n"
175-            "  \"script\" : \"type\"             (string, optional) The output script type. Possible types: nonstandard, pubkey, pubkeyhash, scripthash, multisig, nulldata, witness_v0_keyhash, witness_v0_scripthash\n"
176-            "  \"hex\" : \"hex\",                (string, optional) The redeemscript for the p2sh address\n"
177-            "  \"addresses\"                   (string, optional) Array of addresses associated with the known redeemscript\n"
178+            "  \"isscript\" : true|false,      (boolean) If the address is P2SH or P2WSH\n"
179+            "  \"iswitness\" : true|false,     (boolean) If the address is P2WPKH, P2WSH, or an unknown witness version\n"

TheBlueMatt commented at 10:19 pm on December 17, 2017:

(not a regression, but might as well fix): Missing witness_version, witness_program.

sipa commented at 4:09 am on December 18, 2017:

Will fix.

in src/rpc/misc.cpp:66 in e721efb2bd outdated

65+        }
66+
67+        // Only when the script is multisig (in which case the ExtractDestination above certainly failed):
68+        if (which_type == TX_MULTISIG) {
69+            // TODO: abstract out the common functionality between this logic and ExtractDestinations.
70+            obj.pushKV("sigsrequired", solutions_data[0][0]);

TheBlueMatt commented at 10:19 pm on December 17, 2017:

Will conflict with #11545. Seems strange to blindly cast here either way, even though we always did this in ExtractDestinations.

sipa commented at 4:01 am on December 18, 2017:

No problem addressing this if #11545 is merged first.

hsjoberg commented at 10:21 pm on December 17, 2017: contributor

Pull request to fix the bip68-sequence.py test open for @sipa’s branch. (I’m not sure if this the correct routine for adding commits to another persons pull request.)

Also, this PR let’s the user set the address type through the configuration file – it would be nice if this could be exposed from the Qt GUI as well. If there’s any interest, I have code ready for that which could be looked into once this PR is merged.

in src/rpc/misc.cpp:80 in e721efb2bd outdated

75+                pubkeys.push_back(HexStr(key.begin(), key.end()));
76+            }
77+            obj.pushKV("pubkeys", std::move(pubkeys));
78+        }
79+
80+        // The "addresses" field is problematic because it confuses keys and destination addresses, using

TheBlueMatt commented at 10:30 pm on December 17, 2017:

I would not put this so strongly. The only thing really wrong with referring to public keys by their base58 address is that we want to split wallet out from other subsystems, ie #10583 implies this is the right direction.

sipa commented at 4:03 am on December 18, 2017:

Will reformulate.

in src/rpc/misc.cpp:177 in e721efb2bd outdated

177-            "  \"addresses\"                   (string, optional) Array of addresses associated with the known redeemscript\n"
178+            "  \"isscript\" : true|false,      (boolean) If the address is P2SH or P2WSH\n"
179+            "  \"iswitness\" : true|false,     (boolean) If the address is P2WPKH, P2WSH, or an unknown witness version\n"
180+            "  \"script\" : \"type\"             (string, optional) The output script type. Only if \"isscript\" is true. Possible types: nonstandard, pubkey, pubkeyhash, scripthash, multisig, nulldata, witness_v0_keyhash, witness_v0_scripthash, witness_unknown\n"
181+            "  \"hex\" : \"hex\",                (string, optional) The redeemscript for the P2SH or P2WSH address\n"
182+            "  \"addresses\"                   (string, optional) Array of addresses associated with the known redeemscript (only if \"script\" is \"multisig\" and \"iswitness\" is false)\n"

TheBlueMatt commented at 10:34 pm on December 17, 2017:

No? This is included if script is anything standard with a key and iswitness is false, not just multisig.

sipa commented at 4:12 am on December 18, 2017:

Will fix.

in src/keystore.cpp:141 in e721efb2bd outdated

132@@ -110,8 +133,10 @@ bool CBasicKeyStore::AddWatchOnly(const CScript &dest)
133     LOCK(cs_KeyStore);
134     setWatchOnly.insert(dest);
135     CPubKey pubKey;
136-    if (ExtractPubKey(dest, pubKey))
137+    if (ExtractPubKey(dest, pubKey)) {
138         mapWatchKeys[pubKey.GetID()] = pubKey;
139+        ImplicitlyLearnRelatedKeyScripts(pubKey);

TheBlueMatt commented at 10:44 pm on December 17, 2017:

Need some kind of corresponding erase in RemoveWatchOnly otherwise removal doesn’t work properly. Not clear how to fix this but I think blind removal is probably better than not removing at all. Given the issue still pending at #11403 (review), I think we’re gonna have to have some kind of marker for “was implicitly added”.

sipa commented at 4:00 am on December 18, 2017:

In what scenario does a stale CScript result in incorrect watch-only behaviour?

Watch-only is supposed to be distinct from the normal ismine/solvability logic.

ryanofsky commented at 9:34 pm on December 31, 2017:

#11403 (review)

Need some kind of corresponding erase in RemoveWatchOnly @TheBlueMatt can you clarify if you still think this is a bug. @sipa it might would be good to have a comment in RemoveWatchOnly saying CScripts are not removed but this should be harmless.

sipa commented at 12:59 pm on January 3, 2018:

Discussed this IRL with @TheBlueMatt. I believe there is no problem here, and I’ve added comments to clarify that.

in src/wallet/wallet.cpp:855 in e721efb2bd outdated

851@@ -850,7 +852,7 @@ bool CWallet::GetAccountPubkey(CPubKey &pubKey, std::string strAccount, bool bFo
852         if (!GetKeyFromPool(account.vchPubKey, false))
853             return false;
854 
855-        SetAddressBook(account.vchPubKey.GetID(), strAccount, "receive");
856+        SetAddressBook(AddDestinationForKey(account.vchPubKey, g_address_type), strAccount, "receive");

TheBlueMatt commented at 11:10 pm on December 17, 2017:

Ugh, this sucks. We return a pubkey, so could end up adding for g_address_type and then getting used as g_change_type. Given this function is only used by GetAccountAddress in rpcwallet (which turns it into an address anyway), can you refactor a bit to make this return an account address instead?

sipa commented at 4:42 am on December 18, 2017:

Will do.

in src/wallet/init.cpp:20 in e721efb2bd outdated

15@@ -16,6 +16,8 @@
16 std::string GetWalletHelpString(bool showDebug)
17 {
18     std::string strUsage = HelpMessageGroup(_("Wallet options:"));
19+    strUsage += HelpMessageOpt("-addresstype", strprintf(_("What type of addresses to use (\"legacy\", \"p2sh\", \"bech32\", default: \"%s\")"), FormatOutputType(OUTPUT_TYPE_DEFAULT)));
20+    strUsage += HelpMessageOpt("-changetype", strprintf(_("What type of change to use (\"legacy\", \"p2sh\", \"bech32\", default is same as -addresstype: \"%s\")"), FormatOutputType(OUTPUT_TYPE_DEFAULT)));

TheBlueMatt commented at 11:19 pm on December 17, 2017:

This is ever so slightly confusing, maybe skip the FormatOutputType part and just say “default is whatever is set for -addresstype”.

sipa commented at 4:24 am on December 18, 2017:

Will fix.

in src/wallet/wallet.cpp:4170 in e721efb2bd outdated

4163+{
4164+    switch (type) {
4165+    case OUTPUT_TYPE_LEGACY: return OUTPUT_TYPE_STRING_LEGACY;
4166+    case OUTPUT_TYPE_P2SH: return OUTPUT_TYPE_STRING_P2SH;
4167+    case OUTPUT_TYPE_BECH32: return OUTPUT_TYPE_STRING_BECH32;
4168+    default: assert(false);

TheBlueMatt commented at 11:29 pm on December 17, 2017:

This seems much too agressive - I dont think we should be using assert(false) for cases of “well, the UI is gonna be very confused cause the enum -> string conversion failed”.

sipa commented at 4:52 am on December 18, 2017:

Well it should be impossible that g_address_type or g_change_type are unset, so if this assert triggers, there is a serious bug.

ryanofsky commented at 9:35 pm on December 31, 2017:

#11403 (review)

it should be impossible that g_address_type or g_change_type are unset, so if this assert triggers, there is a serious bug.

Another option would be to throw std::logic_error.

in src/wallet/wallet.cpp:4235 in e721efb2bd outdated

4216+        WitnessV0ScriptHash hash;
4217+        CSHA256().Write(script.data(), script.size()).Finalize(hash.begin());
4218+        CTxDestination witdest = hash;
4219+        CScript witprog = GetScriptForDestination(witdest);
4220+        // Check if the resulting program is solvable (i.e. doesn't use an uncompressed key)
4221+        if (!IsSolvable(*this, witprog)) return CScriptID(script);

TheBlueMatt commented at 11:33 pm on December 17, 2017:

I believe this wholly prevents you from getting a segwit address for addmultisigaddress if no all the keys are (at least watch only) in your wallet? Needs a test for this issue, probably.

sipa commented at 4:53 am on December 18, 2017:

You just need to know the script.

Agree on adding a test.

sipa commented at 8:13 am on December 19, 2017:

Adding a test to address_types for this.

in src/wallet/rpcdump.cpp:136 in e721efb2bd outdated

130@@ -131,7 +131,11 @@ UniValue importprivkey(const JSONRPCRequest& request)
131     CKeyID vchAddress = pubkey.GetID();
132     {
133         pwallet->MarkDirty();
134-        pwallet->SetAddressBook(vchAddress, strLabel, "receive");
135+
136+        // We don't know which corresponding address will be used; label them all
137+        for (const auto& dest : GetAllDestinationsForKey(pubkey)) {

TheBlueMatt commented at 11:39 pm on December 17, 2017:

If I’m reading this correctly, if you import with importpubkey you can downgrade and you’ll still get the segwit watchonly, but not if you do an importprivkey? This seems strange, but either way the behavior should be documented in the help text of both importprivkey and importpubkey at least until 0.17 (or in release notes).

sipa commented at 4:27 am on December 18, 2017:

I don’t understand.

ryanofsky commented at 9:35 pm on December 31, 2017:

#11403 (review)

If I’m reading this correctly, if you import with importpubkey you can downgrade and you’ll still get the segwit watchonly, but not if you do an importprivkey? @TheBlueMatt can you describe what the documentation should say about this?

sipa commented at 1:00 pm on January 3, 2018:

I discussed this IRL with @TheBlueMatt; what I believe he meant is that importing private keys does not explicitly add the related scripts for those keys to the wallet (possibly hurting the ability to detect outputs to such keys after downgrade). Fixed.

Ulmo commented at 11:45 pm on December 17, 2017: none

Thanks for the git help. Using sipa/201709_segwitwallet2, I get the following startup error (latest macOS):

$ bitcoin-qt -addresstype bech32 PaymentServer::ipcSendCommandLine: Payment request file does not exist: “bech32”

And I get the following response inside the console:

15:39:47 getnewaddress 15:39:58 16ESqniuMJ3eXViX7MiHWSBb2iGN4PcScS

I’ll poke around and see what’s up for a few minutes.

in src/wallet/wallet.cpp:1060 in e721efb2bd outdated

1055@@ -1054,6 +1056,7 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const CBlockI
1056                     std::map<CKeyID, int64_t>::const_iterator mi = m_pool_key_to_index.find(keyid);
1057                     if (mi != m_pool_key_to_index.end()) {
1058                         LogPrintf("%s: Detected a used keypool key, mark all keypool key up to this key as used\n", __func__);
1059+                        AddRelatedScripts(txout.scriptPubKey, mi->first);

TheBlueMatt commented at 11:47 pm on December 17, 2017:

This is not sufficient? What if you have two keys in your pool, in order, A and B, then B gets used as a segwit address on the chain first, and then A…you’d only add B to your watch scripts?

sipa commented at 4:51 am on December 18, 2017:

The keystore implicitly knows about all keys’ corresponding redeemscripts. This function call is not needed except for downgrading to old software after recovery.

sipa commented at 1:02 pm on January 3, 2018:

Discussed this IRL. This was indeed a problem. It’s fixed now by moving the call to MarkReserveKeyAsUsed.

in src/wallet/wallet.cpp:4244 in e721efb2bd outdated

4238+    CTxDestination p2sh = CScriptID(witprog);
4239+    // For SegWit scriptPubKeys, add the redeemscript explicitly to the wallet, so that
4240+    // older software versions (without implicit scripts) correctly treat these as ours.
4241+    if (scriptPubKey == witprog || scriptPubKey == GetScriptForDestination(p2sh)) {
4242+        assert(HaveKey(keyid));
4243+        // No need for a IsSolvable check here, as this function is only called for outputs

TheBlueMatt commented at 11:50 pm on December 17, 2017:

Hmm, if I’m reading this correctly we technically could get here with an uncompressed keyid. Obviously this isn’t the right place to check for that, but we should probably do something to handle it better (some big logprints, etc). Also need a test for this case, I’d think (make sure we dont consider it as valid balance, etc).

sipa commented at 8:15 am on December 19, 2017:

I believe it’s impossible, but it’s certainly not trivial to see - I agree. However, if this code would ever run for an uncompressed key, it’s already too late, as we’re already considering the output to be ours.

Note that just having an uncompressed key’s corresponding CScript in our CKeyStore is not enough to make us treat P2SH-P2WPKH or P2WPKH scriptPubKeys to it as ours.

sipa commented at 11:51 pm on December 17, 2017: member

@Ulmo You need -addresstype=bech32 (note the equals sign).

TheBlueMatt commented at 11:52 pm on December 17, 2017: member

I still think there are some pretty big regressions/bugs that need fixing here. Also including #11403 (review).

Ulmo commented at 0:52 am on December 18, 2017: none

Sipa, thanks. That worked, of course. This time I used pull/11403/head, and started bitcoin-qt as bitcoin-qt -addresstype=bech32, and everything related to bech32 “seems to work”. Detail:

0getnewaddress:  bc1qt0qk2gpdgsh0zyefmy52pwvnd2tk7aafgju6xg

Trezor beta, Ledger, & Gemini still don’t seem to support bech32, giving address errors. Electrum 3.0.1 supports it … let’s try that. First, I had to load a balance from a site that had some Bitcoin; I chose one that required a Segwit or legacy address:

0getnewaddress "" "p2sh": 34hhWKuKYz6dYBateQ366r6s8r53tXmFHa

Gemini sent it, and I received it in the Core wallet in following transaction: https://blockchain.info/tx/da8e2a268f0f0d6ca5123760288c6b03314c4b6fc01156f8a56bdfa650dcc975 debug.log:

02017-12-18 00:22:39 UpdateTip: new best=0000000000000000001994c6c3317b315df6d4465d45a689e8e588ae64429f07 height=499870 version=0x202_work=87.678617 tx=283326467 date='2017-12-18 00:20:32' progress=0.999999 cache=21.7MiB(164816txo)
12017-12-18 00:23:02 AddToWallet da8e2a268f0f0d6ca5123760288c6b03314c4b6fc01156f8a56bdfa650dcc975  new

Balance in core GUI showed 0.00000000 BTC, but this showed up in GUI’s “recent transactions”. Very odd. Then, I sent it to Electrum bech32 address:

 02017-12-18 00:27:06 keypool added 1 keys (0 internal), size=1000 (0 internal)
 12017-12-18 00:27:06 keypool reserve 32
 22017-12-18 00:27:11 keypool return 32
 32017-12-18 00:27:21 keypool reserve 32
 42017-12-18 00:27:21 Fee Calculation: Fee:62217 Bytes:164 Needed:62217 Tgt:2 (requested 2) Reason:"Half Target 60% Threshold" Decay
 5stimation: (366358 - 384675) 65.49% 6939.6/(9527.9 0 mem 1068.7 out) Fail: (348912 - 366358) 50.94% 1305.9/(2313.3 0 mem 250.4 out)
 62017-12-18 00:27:34 CommitTransaction:
 7CTransaction(hash=cbeaee011a, ver=2, vin.size=1, vout.size=2, nLockTime=499775)
 8    CTxIn(COutPoint(da8e2a268f, 0), scriptSig=1600143cd673142bb7c0cb81, nSequence=4294967294)
 9    CScriptWitness(3045022100d927130cf21989bb0e77363b475ce305d4bf9a1203bff955d849e2696ad3de6a0220508d3c53a5766daf470019fb3a97bb9650
10b0ffd3a6b0d573e8263901, 0225bb23f899b89d812ff2054fbc5a5d791d72684ab617d06131e5d70ca8d32bcb)
11    CTxOut(nValue=0.00137783, scriptPubKey=0014cd8b05910abb26f75448307421)
12    CTxOut(nValue=0.00148291, scriptPubKey=0014fe65e91b7d65e4a5df502b23a2)
132017-12-18 00:27:34 keypool keep 32
142017-12-18 00:27:34 AddToWallet cbeaee011a40ca0e8f08730e2f97acd08b8e7a1638152bf2e7d475e0ebe88738  new
152017-12-18 00:27:34 Relaying wtx cbeaee011a40ca0e8f08730e2f97acd08b8e7a1638152bf2e7d475e0ebe88738

After sending this, the “available balance” in the core GUI suddenly magically went UP to .00148291. Then:

02017-12-18 00:29:25 UpdateTip: new best=000000000000000000ad844c4dad30de48af6867567eafc56beba86a867672cb height=499871 version=0x20
12_work=87.678657 tx=283328294 date='2017-12-18 00:27:56' progress=0.999999 cache=28.9MiB(210940txo)
22017-12-18 00:29:26 AddToWallet cbeaee011a40ca0e8f08730e2f97acd08b8e7a1638152bf2e7d475e0ebe88738

https://blockchain.info/tx/cbeaee011a40ca0e8f08730e2f97acd08b8e7a1638152bf2e7d475e0ebe88738 says “Unable to decode output address” «< wtf? Electrum 3.0.1: balance now shows Unconfirmed +0.00137783. Looks right. Now, to send it back to my other core wallet address bc1qt0qk2gpdgsh0zyefmy52pwvnd2tk7aafgju6xg: I got “payment sent” from Electrum for tx 92cf6424a9648ce30b3cba8edb0a122cc2210168547eb06915d7aa8961cb0440: https://blockchain.info/tx/92cf6424a9648ce30b3cba8edb0a122cc2210168547eb06915d7aa8961cb0440 now says “unable to decode address” to “unable to decode address”. Looking good, assuming Blockchain.info is unable to decode bech32 addresses. (Why?) I find it odd mempool doesn’t show in Core any more, or at least not as fast. What’s going on? Are we being spammed, or DDOS? It seems the best way to see an incoming tx is for it to be in an actual block these days. Waiting on core wallet to see the tx … Finally, mempool got it:

02017-12-18 00:41:54 AddToWallet 92cf6424a9648ce30b3cba8edb0a122cc2210168547eb06915d7aa8961cb0440  new

And, it confirmed:

02017-12-18 00:44:04 UpdateTip: new best=0000000000000000000e2532c7d8886363f61ccd43f074aed593763ccc65af01 height=499875 version=0x20
12_work=87.678816 tx=283332610 date='2017-12-18 00:41:53' progress=0.999999 cache=44.3MiB(337087txo)
22017-12-18 00:44:27 AddToWallet 92cf6424a9648ce30b3cba8edb0a122cc2210168547eb06915d7aa8961cb0440  update

Balance in my stupid core GUI wallet still shows 0.00148291, not showing the +.001 that came in. Now, will a hardware wallet think that if I send it out to them from this transaction chain, that it is a legitimate chain of transactions? Especially since it doesn’t support bech32? To my Trezor: Here, in the core GUI, sending a transaction the “Use available balance” DOES pick up the whole balance, and off it goes, core GUI balance now at 0:

 02017-12-18 00:49:13 keypool added 1 keys (0 internal), size=1000 (0 internal)
 12017-12-18 00:49:13 keypool reserve 33
 22017-12-18 00:49:13 keypool return 33
 32017-12-18 00:49:13 Fee Calculation: Fee:58027 Bytes:179 Needed:58027 Tgt:4 (requested 4) Reason:"Conservative Double Target longer horizon" Decay 0.99520: Estimation: (316473 - 332297) 95.16% 6064.9/(6373.7 0 mem 0.0 out) Fail: (301403 - 316473) 94.89% 4055.3/(4273.6 0 mem 0.0 out)
 42017-12-18 00:49:31 keypool reserve 33
 52017-12-18 00:49:31 keypool return 33
 62017-12-18 00:49:31 Fee Calculation: Fee:29286 Bytes:179 Needed:29286 Tgt:12 (requested 12) Reason:"Target 85% Threshold" Decay 0.96200: Estimation: (159841 - 167833) 91.17% 362.9/(392.5 0 mem 5.6 out) Fail: (152229 - 159841) 70.49% 217.2/(287.9 0 mem 20.2 out)
 72017-12-18 00:49:38 CommitTransaction:
 8CTransaction(hash=e8e978558a, ver=2, vin.size=2, vout.size=1, nLockTime=499875)
 9    CTxIn(COutPoint(cbeaee011a, 1), scriptSig=, nSequence=4294967294)
10    CTxIn(COutPoint(92cf6424a9, 0), scriptSig=, nSequence=4294967294)
11    CScriptWitness(30450221009e60a6639ce74817fb47fa871ea97d59d1c2113b1efd3ad79023d4b795c430280220515c8ee031724211a460418839a3603530ba2301e4cf8bd2586e10c25fc0e43c01, 02421c139e26cb8678c8132b6e6d8eb7640d52630bf8c74914d249408cba5debbb)
12    CScriptWitness(304402204eb7e6b53ae136dfae26c0819695c8033c3a6fdf5a3e40f1b0bce119a20e6d87022072ec3cb64dcc45611bd585ddc8b5afe640dd7a224061a40a6e8e7f3444a1a67d01, 023ef1f057355c650821a263dcb78296c61929f98eb03a72b623835f94100305fd)
13    CTxOut(nValue=0.00219005, scriptPubKey=a914405420cce346a85a8481790fd7)
142017-12-18 00:49:38 AddToWallet e8e978558a41b1a883dac0736e5e6fd3c9cbba435cc25a9a6baeb4f0a00a57a1  new
152017-12-18 00:49:38 Relaying wtx e8e978558a41b1a883dac0736e5e6fd3c9cbba435cc25a9a6baeb4f0a00a57a1

That transaction shows up in my Trezor wallet as unconfirmed +0.00219005.

Well, so far so good.

Let’s see Lightning Network reduce those fees, though. Also, time for Gemini, Kraken, Coinbase, Trezor and Ledger to get in shape, too. Thanks to Electrum for being a good way to test this.

Sjors commented at 10:09 am on December 18, 2017: member

@FelixWeis @hsjoberg I added a todo to my ever growing list to add a check box in the UI where the user can specifically ask for a native SegWit bech32 address. It would be a separate PR. Happy to review if someone else is already doing this.

In addition, there could be a UI setting that toggles to bech32 as a default, including change address, and then flipping the meaning of the above check box. Again a separate PR.

Whenever the wallet creates a transaction it should, again not this PR, probably be smart and use the same address type for change as the destination. @sipa is there anything in this PR that could be merged to master separately (and first)?

Sjors commented at 10:39 am on December 18, 2017: member

@Ulmo I don’t see “Unable to decode output address” for your transaction 92cf…. I’ve seen it work correctly when I tried a few weeks ago. Maybe it was a glitch?

Note however that you can’t click on a bech32 address to see all transactions. As far as I know no block explorer supports that yet; I’m sure that’ll change, but it’s another reason why it’s better to make bech32 addresses opt-in, but encouraged, initially.

hsjoberg commented at 12:43 pm on December 18, 2017: contributor

I added a todo to my ever growing list to add a check box in the UI where the user can specifically ask for a native SegWit bech32 address. It would be a separate PR. Happy to review if someone else is already doing this. @Sjors Right, I have code ready for a GUI setting to decide the address type to use. It’s directly tied to the addresstype parameter and thus it’s radio buttons and not single checkbox. Perhaps I could re-work to be a checkbox if people like it better. It seems out of scope of this PR so I’ll propose it after this one has been merged.

As far as I know no block explorer supports that yet @Sjors @Ulmo I think https://btc.com supports BIP173/bech32-addresses better.

in src/wallet/init.cpp:19 in e721efb2bd outdated

15@@ -16,6 +16,8 @@
16 std::string GetWalletHelpString(bool showDebug)
17 {
18     std::string strUsage = HelpMessageGroup(_("Wallet options:"));
19+    strUsage += HelpMessageOpt("-addresstype", strprintf(_("What type of addresses to use (\"legacy\", \"p2sh\", \"bech32\", default: \"%s\")"), FormatOutputType(OUTPUT_TYPE_DEFAULT)));

laanwj commented at 1:15 pm on December 18, 2017:

nit: -addresstype=<type> to make it clear that this option takes a value

Sjors commented at 1:18 pm on December 18, 2017: member

@hsjoberg ah, yes it does.

Can you make a WIP PR that uses this branch as its base? That way it’s kept separate yet is easy to review. Once this PR is merged, you switch the base to master.

A radio button for the global GUI setting makes sense to me. I would only use a check box for overriding the setting in the receive screen.

Users need to be able to use both address types depending on who they’re requesting money from. Having to restart every time, which a global setting requires, would be inconvenient. Having a way to do this every time you generate a new address seems more useful, but that can exist side-by-side.

hsjoberg commented at 2:24 pm on December 18, 2017: contributor

@Sjors I’ll put it up as soon as I can.

Having to restart every time

Interestingly enough, with a GUI-setting, it works without restarting if we directly change g_address_type global, I’m not sure if this is good practice though.

sipa force-pushed on Dec 19, 2017

sipa commented at 8:26 am on December 19, 2017: member

@TheBlueMatt I’ve addressed several of your comments. However, for a few of them I think you’re missing that AddCScript does not automatically make a script/output watched. It simply teaches the signing logic how to sign for such a P2SH/P2WSH script if it would appear somewhere. Having it there may result in considering something ours (but generally only if we also have the actual keys available).

ecdsa commented at 9:27 am on December 21, 2017: none

@sipa sorry to intervene late, but “All wallet private and public keys can be used for any type of address.” sounds like a terrible idea. Even if you plan to add address-dependent derivation path later, once the harm is done, it will be too late to go back.

importprivkey will need to lookup several output scripts per key. This will break compatibility with Electrum, and with various other wallets. Wallets that do not support segwit will return false negatives when attempting to import a private keys that has coins sent to segwit scripts. It will also create technological debt for wallets that do support segwit, because they will have to lookup old p2pkh scripts in order to be compatible with Bitcoin Core’s private key import format.

Please consider using one address per private key, and a private key export format that includes information about the type of output script used with the key. This will avoid false negatives, and it will free new wallets from being forced to implement deprecated address types.

NLNicoo commented at 9:55 am on December 21, 2017: none

Tiny meta suggestion, I think this PR can use 0.16.0 milestone tag?

laanwj added this to the milestone 0.16.0 on Dec 21, 2017

laanwj commented at 10:34 am on December 21, 2017: member

@sipa is there anything in this PR that could be merged to master separately (and first)?

Same question - is there a way we can help move things forward, reduce this patch stack, by merging some parts? It looks like some smaller UI things are still being discussed while some of the base work was reviewed and found to be OK a long time ago. Blocking the entire thing for so long is not productive.

Tiny meta suggestion, I think this PR can use 0.16.0 milestone tag?

Done, although it was implicit, there will be no 0.16.0 release without this.

sipa commented at 10:45 am on December 21, 2017: member

sorry to intervene late, but “All wallet private and public keys can be used for any type of address.” sounds like a terrible idea. Even if you plan to add address-dependent derivation path later, once the harm is done, it will be too late to go back.

I agree, it’s a terrible situation to be in, but unfortunately the harm is already done. Right now, the IsMine logic effectively works by seeing what we could sign for. If you have a key, and the corresponding witness script in your wallet, we will accept payment to P2PK, P2PKH, P2SH-P2PKH, P2WPKH, P2SH-P2WPKH, and even more insane combinations. Due to the backward compatibility requirement with the existing addwitnessaddress RPC, we can’t drop that support.

Please read the gist I’ve written up for how to improve things in the future, but it’s not something that can be done without very significant modifications and compatibility breaks first.

ajtowns commented at 6:38 pm on December 21, 2017: member

Cursory ACK ff295c33d7e26293229a05f4039a13fc747efcc0

I don’t see any obvious problems with the code, but that doesn’t necessarily mean a lot. Setting p2wpkh-nested-in-p2sh as the default address type makes sense, and setting the change type to be the same likewise makes sense. The strategy outlined in the gist makes sense (despite being complicated and continuing the maintenance burden) and appears to work as intended. The test case looks good.

Upgrading and downgrading between this and 0.15 seems to work pretty intuitively. I hit a few unrelated problems: segwit vbparams changes in regtest between 0.15 and master results in confusing errors, installing master then downgrading to 0.15 doesn’t work the same way as upgrading from 0.15 then downgrading back to 0.15 because of the walletdir changes (#11466), and –with-incompatible-bdb results in the expected incompatibilities…

Not even nits: “legacy” and “p2sh” maybe aren’t the clearest possible names, but they’re good enough. I wonder if ParseOutputType wouldn’t have been better as bool ParseOutputType(OutputType &type, str, default), in which case OUTPUT_TYPE_NONE wouldn’t have been needed.

SomberNight commented at 8:50 pm on December 21, 2017: contributor

Please consider using one address per private key, and a private key export format that includes information about the type of output script used with the key. This will avoid false negatives, and it will free new wallets from being forced to implement deprecated address types.

The critical part of this, that I would like to emphasize, would be to have the private key export format include the output script type. Even if you allow all private keys internally to be associated with any script type, please include the script type in the export serialization format. The serialization of a private key (e.g. WIF) is more than an ecdsa private key, it should also contain metadata about how the ecdsa key is to be used.

As an example, for serializing private keys, currently Electrum uses an extended WIF format. It includes a prefix byte (similar to the compressed suffix byte) that encodes the script type to be used.

It would be great if the dumpprivkey and dumpwallet (and possibly other?) commands did something similar. For instance if the same ecdsa key is used with three different addresses, for each address there should be a corresponding “WIF” serialization.

stevenroose commented at 10:00 am on December 23, 2017: contributor

What is the argumentation here for using legacy instead of p2pkh? Legacy is an ambiguous term. At some point in the past, p2pkcould have been considered legacy while using addresses was the new way. In the future, SegWit/bech32 could be considered legacy over a next generation addressing convention.

sipa commented at 10:08 am on December 23, 2017: member

@stevenroose Good point, it’s not perfectly unambiguous. However, “legacy” does not mean just P2PKH. For multisig it means P2SH-multisig.

Arguably “p2sh” is also not perfect, as for multisig it refers to P2SH-P2WSH-multisig.

Unambiguous would be using the names “no-witness” (instead of legacy), “p2sh-witness-v0” (instead of p2sh), and “native-witness-v0” (instead of bech32), but those aren’t really well known things, and aren’t really related to the names people use for the address encodings.

Suggestions welcome.

flack commented at 11:43 am on December 23, 2017: contributor

maybe call it something like pre-segwit? Not especially elegant, but it would probably age better than legacy

stevenroose commented at 12:42 pm on December 23, 2017: contributor

The two important factors are where the keys are taken from in the BIP32 hierarchy and how the address is formatted visibly.

p2sh-p2wsh-multisig is indeed a bit more complex than what you want to address in a single parameter..

Maybe legacy is a good name when referring to the “gen1” addressing and we can see what gen2 brings.

Tl;dr can’t directly come up with a better naming idea.

On 23 Dec 2017 12:44 pm, “flack” notifications@github.com wrote:

maybe call it something like pre-segwit? Not especially elegant, but it would probably age better than legacy

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/11403#issuecomment-353721884, or mute the thread https://github.com/notifications/unsubscribe-auth/AA0F3K71ugnEGbWLcA1vjaBB763mkfAMks5tDOeSgaJpZM4PjkGp .

ajtowns commented at 1:07 pm on December 23, 2017: member

If v1 scripts will introduce new address types, maybe using “bech32-v0” and “p2sh-v0” now is a good idea? At least “p2sh-v0” is suggestive of witness support, unlike plain “p2sh” which sounds like a purely legacy mode.

Naming the format after bips could also work: maybe “bip16”, “bip141-v0”, “bip173-v0” – except bip16 isn’t really accurate, and none of those are very intuitive. “pre-witness” or “pre-segwit” seems better than legacy if this option will still be useful when v1 scripts get implemented. Alternatively, “base58” or “base58-legacy” might match “bech32-v0” well.

sipa force-pushed on Dec 23, 2017

sipa commented at 5:31 pm on December 23, 2017: member

@ryanofsky @TheBlueMatt Updated address_types.py to also test getrawchangeaddress, and the address_type parameter to both RPCs.

juscamarena commented at 0:38 am on December 25, 2017: none

Tested ACK 10bd7d6c20028309b45d44d8f50188649a00825e

Tested on OSX on bitcoin testnet with an older HD Wallet, the various change addresses and address style changes worked fine.

Currently testing a linux build irresponsibly live on mainnet on one of my nodes with low volume outgoing transactions and it works great so far. Extensively tested on testnet before doing so.

instagibbs approved

instagibbs commented at 8:14 pm on December 27, 2017: member

re-ACK

fanquake deleted a comment on Dec 27, 2017

in src/wallet/wallet.cpp:4149 in 5d9243963a outdated

4144+static const std::string OUTPUT_TYPE_STRING_P2SH = "p2sh";
4145+static const std::string OUTPUT_TYPE_STRING_BECH32 = "bech32";
4146+
4147+OutputType ParseOutputType(const std::string& type, OutputType default_type)
4148+{
4149+    if (type == "") {

Talkless commented at 7:37 am on December 28, 2017:

I would suggest to use type.empty() instead, no need to call string comparison fn. Generated code is much better.

sipa commented at 12:06 pm on December 28, 2017:

Fixed.

sipa force-pushed on Dec 28, 2017

in src/wallet/rpcwallet.cpp:178 in 63d4c88441 outdated

176@@ -177,12 +177,12 @@ UniValue getnewaddress(const JSONRPCRequest& request)
177 
178 CTxDestination GetAccountAddress(CWallet* const pwallet, std::string strAccount, bool bForceNew=false)

promag commented at 1:33 am on December 29, 2017:

In commit “[refactor] GetAccountPubKey -> GetAccountDestination”

Nit, could also rename GetAccountAddress to GetAccountDestination? (only 3 calls)

sipa commented at 1:43 pm on January 3, 2018:

Done.

in src/script/standard.h:159 in 5886f93487 outdated

154@@ -155,6 +155,10 @@ bool ExtractDestination(const CScript& scriptPubKey, CTxDestination& addressRet)
155  * addressRet is populated with a single value and nRequiredRet is set to 1.
156  * Returns true if successful. Currently does not extract address from
157  * pay-to-witness scripts.
158+ *
159+ * This function confuses destinations (a subset of CScripts that are encodable

promag commented at 1:54 am on December 29, 2017:

In commit “Extend validateaddress information for P2SH-embedded witness”

Nit, tag this comment as NOTE?

sipa commented at 1:37 pm on January 3, 2018:

OKAY.

in src/rpc/misc.cpp:168 in 5886f93487 outdated

174-            "  \"script\" : \"type\"             (string, optional) The output script type. Possible types: nonstandard, pubkey, pubkeyhash, scripthash, multisig, nulldata, witness_v0_keyhash, witness_v0_scripthash\n"
175-            "  \"hex\" : \"hex\",                (string, optional) The redeemscript for the p2sh address\n"
176-            "  \"addresses\"                   (string, optional) Array of addresses associated with the known redeemscript\n"
177+            "  \"isscript\" : true|false,      (boolean, optional) If the address is P2SH or P2WSH. Not included for unknown witness types.\n"
178+            "  \"iswitness\" : true|false,     (boolean) If the address is P2WPKH, P2WSH, or an unknown witness version\n"
179+            "  \"witness_version\" : version   (number, optional) For all witness output types, gives the version number.\n"

promag commented at 2:17 am on December 29, 2017:

Missing test.

sipa commented at 1:24 pm on January 3, 2018:

Fixed.

in src/rpc/misc.cpp:169 in 5886f93487 outdated

175-            "  \"hex\" : \"hex\",                (string, optional) The redeemscript for the p2sh address\n"
176-            "  \"addresses\"                   (string, optional) Array of addresses associated with the known redeemscript\n"
177+            "  \"isscript\" : true|false,      (boolean, optional) If the address is P2SH or P2WSH. Not included for unknown witness types.\n"
178+            "  \"iswitness\" : true|false,     (boolean) If the address is P2WPKH, P2WSH, or an unknown witness version\n"
179+            "  \"witness_version\" : version   (number, optional) For all witness output types, gives the version number.\n"
180+            "  \"witness_program\" : \"hex\"     (string, optional) For all witness output types, gives the script or key hash present in the address.\n"

promag commented at 2:17 am on December 29, 2017:

Missing test.

sipa commented at 1:25 pm on January 3, 2018:

Fixed (just testing the length/presence).

in src/rpc/misc.cpp:74 in 5886f93487 outdated

69+            // TODO: abstract out the common functionality between this logic and ExtractDestinations.
70+            obj.pushKV("sigsrequired", solutions_data[0][0]);
71+            UniValue pubkeys(UniValue::VARR);
72+            for (size_t i = 1; i < solutions_data.size() - 1; ++i) {
73+                CPubKey key(solutions_data[i].begin(), solutions_data[i].end());
74+                a.push_back(EncodeDestination(key.GetID()));

promag commented at 2:24 am on December 29, 2017:

Same as https://github.com/bitcoin/bitcoin/pull/11403/files#r159020401.

sipa commented at 4:35 pm on December 29, 2017:

Why?

promag commented at 4:58 pm on December 29, 2017:

Because a is only used if include_addresses? Or am I missing something?

sipa commented at 1:04 pm on January 3, 2018:

Thanks, I needed more context to see what you meant. Fixed.

in src/wallet/wallet.cpp:4161 in 5a280466a3 outdated

4156+    } else {
4157+        return OUTPUT_TYPE_NONE;
4158+    }
4159+}
4160+
4161+

promag commented at 2:39 am on December 29, 2017:

In commit “SegWit wallet support”

Nit, remove 2nd newline.

sipa commented at 1:44 pm on January 3, 2018:

Done.

promag commented at 2:44 am on December 29, 2017: member

utACK 17a7a72.

in src/rpc/misc.cpp:64 in 17a7a72559 outdated

59+            if (subobj.exists("pubkey")) {
60+                // Always report the pubkey at the top level, so that `getnewaddress()['pubkey']` always works.
61+                obj.pushKV("pubkey", subobj["pubkey"]);
62+            }
63+            obj.pushKV("embedded", std::move(subobj));
64+            a.push_back(EncodeDestination(embedded));

promag commented at 2:46 am on December 29, 2017:

Could add if (include_addresses) a.push_back(...);

sipa commented at 4:34 pm on December 29, 2017:

Why? embedded should always be reported, it’s not obsolete like addresses.

sipa commented at 1:44 pm on January 3, 2018:

Fixed, see elsewhere.

ryanofsky commented at 9:38 pm on December 31, 2017: member

utACK 17a7a7255958237b235c7c4104af7d9dd4482c8e. Changes since previous review were various documentation / comment / test updates, GetAccountPubkey -> GetAccountDestination refactor, changing AddDestinationForScript so addmultisigaddress returns P2SH-P2WSH instead of P2SH addresses (https://github.com/bitcoin/bitcoin/pull/11403#issuecomment-352095070).

Updated TODO list (again, all things that could be followed up on in future PRs):

add -addresstype and -changetype parsing tests #11403 (review) (promag 9/26), #11403 (review) (instagibbs 9/29)
~~add getnewaddress address_type parsing test #11403 (review) (promag 9/26)~~
~~add getrawchangeaddress address_type parsing test #11403 (review) (promag 9/26), #11403 (review) (instagibbs 9/29)~~
avoid slow/redundant AddCScript calls in AddDestinationForKey, AddDestinationForScript, and AddRelatedScripts #11403 (review) (TheBlueMatt 12/7) and #11403#pullrequestreview-84010629 (TheBlueMatt 12/17)
remove or move AddCScript calls in AddDestinationForKey, AddDestinationForScript by either dropping support for downgrading or by inserting scripts in more appropriate place, like on removal of keys from keypool #11403 (review) (theuni 10/17, ryanofsky 12/8)
document downgrade/upgrade procedure #11403 (comment) (TheBlueMatt 12/7)
replace overloaded CKeyID/CScriptID types in CTxDestination variant #11403 (review) (ryanofsky 12/8)
add some kind of test coverage for downgrade code that we may never know is broken #11403 (review) (ryanofsky 12/12)
copy address labels in addwitnessaddress #11403 (comment) (sturles 12/17)
add a test verifying we don’t count segwit outputs paying to a uncompressed public key as part of our balance #11403 (review) (TheBlueMatt 12/17)
indicate associated addresses types when exporting private keys #11403 (comment) (SomberNight 12/21)
various promag 12/28 review suggestions #11403#pullrequestreview-85903766

achow101 commented at 11:19 pm on January 2, 2018: member

Regarding what to call the address types, this comment came up elsewhere about the p2sh type:

That’s nice but it’s ambiguous tho’ ; “P2SH” doesn’t tell you what script is inside (segwit, multisig, etc).

I think that it should be slightly more specific to refer to segwit, something like p2sh-segwit. That way users will understand that those addresses are segwit addresses too.

Abstract out IsSolvable from Witnessifier 0c8ea6380c

[refactor] GetAccount{PubKey,Address} -> GetAccountDestination cbe197470e

Improve witness destination types and use them more 985c79552c

Expose method to find key for a single-key destination 30a27dc5b1

Extend validateaddress information for P2SH-embedded witness

This adds new fields 'pubkeys' and 'embedded' to the RPC's output, and improves the
documentation for previously added 'witness_version' and 'witness_program' fields.

3eaa003c88

sipa force-pushed on Jan 3, 2018

sipa commented at 1:57 pm on January 3, 2018: member

Pushed a significant change: 17a7a7255958237b235c7c4104af7d9dd4482c8e -> 06cdb1dadc93b1fb273db3e74095656a1955e63c (squashed, not rebased):

Several comments addressing how it’s harmless to have additional CScripts in the wallet, following comments by @TheBlueMatt.
Split up AddDestinationForKey into GetDestinationForKey (function) and LearnRelatedScripts (wallet method) as requested by @theuni and @ryanofsky. While being a bit more verbose, it’s probably easier to read and more efficient for a few use cases.
Renamed p2sh as address type to p2sh-segwit as suggested by @achow101.
Restructured and improved the efficiency of DescribeAddressVisitor::ProcessSubScript a bit, as suggested by @promag.
Hopefully fixed two bugs noticed by @TheBlueMatt w.r.t. detection of wallet outputs after downgrade (specifically for imported keys, and for out-of-order recovery-detected keys); note that there are no tests for this - it only impacts downgrade to older software.
Added more comments and commit messages.
Added tests for witness_version and witness_program fields.

Sorry for the review that this invalidates, but I hope this can be the last update to this PR.

instagibbs commented at 3:17 pm on January 3, 2018: member

address_types.py appears to be failing on this assert for a single build: assert_equal(unconf_balances[to_node], to_send * 10 * (2 + n))

instagibbs commented at 3:54 pm on January 3, 2018: member

@sipa I remembered I had power to kick travis, and did. All good.

in src/wallet/wallet.h:1148 in 6689c3c256 outdated

1141@@ -1129,6 +1142,20 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
1142      * deadlock
1143      */
1144     void BlockUntilSyncedToCurrentChain();
1145+
1146+    /**
1147+     * Explicitly make the wallet learn the related scripts for outputs to the
1148+     * given key. This is purely for backward compatibility with earlier wallet

ryanofsky commented at 3:57 pm on January 3, 2018:

In commit “SegWit wallet support”

Saying this is for “backwards compatibility” is a bit vague since that could imply this is needed for new software to load old wallets, but this is really for old software to be able to load new wallets. Maybe change “earlier wallet versions” to “earlier wallet software” in the next line to clarify.

The wording in the earlier version of this PR also seemed good: “For SegWit scriptPubKeys, add the redeemscript explicitly to the wallet, so that older software versions (without implicit scripts) correctly treat these as ours.”

sipa commented at 4:00 pm on January 5, 2018:

Rewritten a bit.

in src/wallet/wallet.h:1152 in 6689c3c256 outdated

1147+     * Explicitly make the wallet learn the related scripts for outputs to the
1148+     * given key. This is purely for backward compatibility with earlier wallet
1149+     * versions, as CBasicKeyStore automatically does this implicitly for all
1150+     * keys now.
1151+     */
1152+    void LearnRelatedScripts(const CPubKey& key, OutputType = OUTPUT_TYPE_P2SH_SEGWIT);

ryanofsky commented at 5:11 pm on January 3, 2018:

In commit “SegWit wallet support”

Maybe explain OUTPUT_TYPE_P2SH_SEGWIT default argument in the comment since it seems a little arbitrary. I guess if no OutputType parameter is passed you want to store the p2wpkh script unconditionally.

sipa commented at 4:01 pm on January 5, 2018:

Addressed by splitting the function up in two.

ryanofsky commented at 5:43 pm on January 3, 2018: member

utACK 06cdb1dadc93b1fb273db3e74095656a1955e63c. Changes since previous review were the ones listed #11403 (comment).

in src/wallet/wallet.cpp:838 in 6689c3c256 outdated

834@@ -833,7 +835,7 @@ bool CWallet::GetAccountDestination(CTxDestination &dest, std::string strAccount
835             bForceNew = true;
836         else {
837             // Check if the current key has been used
838-            CScript scriptPubKey = GetScriptForDestination(account.vchPubKey.GetID());
839+            CScript scriptPubKey = GetScriptForDestination(GetDestinationForKey(account.vchPubKey, g_address_type));

jimpo commented at 3:54 am on January 4, 2018:

If a key receives funds to an alternate address type, should the wallet force a new key?

sipa commented at 12:02 pm on January 5, 2018:

Perhaps, but is it worth implementing that for accounts (which are deprecated)?

ryanofsky commented at 12:27 pm on January 5, 2018:

#11403 (review)

Perhaps, but is it worth implementing that for accounts (which are deprecated)?

This is also used by getlabeladdress in #7729. (If #11536 were merged, I think it would clear up some ongoing confusion about accounts and labels). But I think it still probably not worth changing the behavior in this PR, which is already doing enough things. Could have a comment here saying this code could check other address types besides g_address_type or store label/account addresses instead of pubkeys to be more reliable in the future.

sipa commented at 4:01 pm on January 5, 2018:

Added a TODO.

sipa force-pushed on Jan 5, 2018

sneurlax commented at 4:54 pm on January 6, 2018: none

Built, tested, working here.

ghost commented at 5:21 pm on January 7, 2018: none

Built in a Ubuntu VM for Windows. Tested: *creation of p2sh, *creation of bech32 using ‘-addresstype=bech32’.

If you create a new wallet using 0.15.0, it adds 1 address to the Receiving tab on creation: https://i.imgur.com/u0ibz3a.png That is not the case with this branch (both p2sh and addresstype=bech32): https://i.imgur.com/H78fGtk.png I’m not sure whether this change is intended or whether it is relevant, but I’d thought I’d mention it. @sipa ?

Tested on mainnet. ACK.

instagibbs commented at 5:49 pm on January 7, 2018: member

I believe this is due to a different change. The removal of the default key.

On Jan 7, 2018 12:22 PM, “Lauda” notifications@github.com wrote:

Built in a Ubuntu VM for Windows. Tested: *creation of p2sh, *creation of bech32 using ‘-addresstype=bech32’.

If you create a new wallet using 0.15.0, it adds 1 address to the Receiving tab on creation: https://i.imgur.com/u0ibz3a.png http://url That is not the case with this branch (both p2sh and addresstype=bech32): https://i.imgur.com/H78fGtk.png http://url I’m not sure whether this change is intended or whether it is relevant, but I’d thought I’d mention it. @sipa https://github.com/sipa ?

Tested on mainnet. ACK.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/11403#issuecomment-355837804, or mute the thread https://github.com/notifications/unsubscribe-auth/AFgC08wP3MrUjvE-8hlOBJOM0IZ7PoZNks5tIP1pgaJpZM4PjkGp .

ghost commented at 6:34 pm on January 7, 2018: none

@instagibbs Tested using the ’legacy’ option and you are right.

gmaxwell commented at 2:43 am on January 8, 2018: contributor

Tested ACK.

in src/wallet/rpcwallet.cpp:295 in cbe197470e outdated

291@@ -292,8 +292,8 @@ UniValue setaccount(const JSONRPCRequest& request)
292         // Detect when changing the account of an address that is the 'unused current key' of another account:
293         if (pwallet->mapAddressBook.count(dest)) {
294             std::string strOldAccount = pwallet->mapAddressBook[dest].name;
295-            if (dest == GetAccountAddress(pwallet, strOldAccount)) {
296-                GetAccountAddress(pwallet, strOldAccount, true);
297+            if (dest == GetAccountDestination(pwallet, strOldAccount)) {

TheBlueMatt commented at 6:03 pm on January 8, 2018:

As discussed on IRC this stuff sucks. If you add a bech32 address to your mapAddressBook it comes back out as 3QJmnh in 0.15 and prior. I’m ok with leaving it after discussion, but needs good documentation.

sipa commented at 11:38 pm on January 9, 2018:

Release notes are for later I think (after importmulti/signmessage/dumpwallet/importwallet have been figured out).

stefment commented at 7:40 am on January 11, 2018:

I love you guys.

ryanofsky commented at 7:51 pm on January 8, 2018: member

utACK e5b4d3fb6ec60d393d763f77b5095919baa7c82f. Changes since last review: “SegWit wallet support” commit now has description, LearnAllRelatedScripts convenience function added, GetAccountDestination todo added.

in src/wallet/rpcwallet.cpp:1310 in 899c0ef898 outdated

1306@@ -1307,7 +1307,7 @@ UniValue addwitnessaddress(const JSONRPCRequest& request)
1307             throw JSONRPCError(RPC_WALLET_ERROR, "Cannot convert between witness address types");
1308         }
1309     } else {
1310-        pwallet->AddCScript(witprogram);
1311+        pwallet->AddCScript(witprogram); // Implicit for single-key, but necessary for multisig

TheBlueMatt commented at 8:21 pm on January 8, 2018:

Comment is wrong - we use the AddCScript call to write the redeemscript to disk to support downgrade.

sipa commented at 11:37 pm on January 9, 2018:

Fixed.

in src/wallet/rpcdump.cpp:442 in fce5d09ae6 outdated

437@@ -433,7 +438,9 @@ UniValue importpubkey(const JSONRPCRequest& request)
438 
439     LOCK2(cs_main, pwallet->cs_wallet);
440 
441-    ImportAddress(pwallet, pubKey.GetID(), strLabel);
442+    for (const auto& dest : GetAllDestinationsForKey(pubKey)) {
443+        ImportAddress(pwallet, dest, strLabel);

TheBlueMatt commented at 11:28 pm on January 8, 2018:

Is it safe that we do not do a AddCScript based on a importpubkey? Seems to me that you’d never match the HaveCScript condition if you downgrade so maybe something might break? The final script will still be watched, but it just seems weird to not have the symmetry with importprivkey here.

sipa commented at 11:37 pm on January 9, 2018:

I believe you’re right. Fixed.

TheBlueMatt commented at 11:54 pm on January 8, 2018: member

Seems fine to me, but I’m sick and cant review properly. Also not worth holding this up anymore for me.

sipa deleted a comment on Jan 9, 2018

Support P2WPKH addresses in create/addmultisig 37c03d3e05

Support P2WPKH and P2SH-P2WPKH in dumpprivkey cf2c0b6f5c

[test] Serialize CTransaction with witness by default 57273f2b30

Implicitly know about P2WPKH redeemscripts

Make CKeyStore automatically known about the redeemscripts necessary for P2SH-P2WPKH
(and due to the extra checks in IsMine, also P2WPKH) spending.

f37c64e477

SegWit wallet support

This introduces two command line flags (-addresstype and -changetype) which control
the type of addresses/outputs created by the GUI and RPCs. Certain RPCs allow
overriding these (`getnewaddress` and `getrawchangeaddress`). Supported types
are "legacy" (P2PKH and P2SH-multisig), "p2sh-segwit" (P2SH-P2WPKH and P2SH-P2WSH-multisig),
and "bech32" (P2WPKH and P2WSH-multisig).

A few utility functions are added to the wallet to construct different address type
and to add the necessary entries to the wallet file to be compatible with earlier
versions (see `CWallet::LearnRelatedScripts`, `GetDestinationForKey`,
`GetAllDestinationsForKey`, `CWallet::AddAndGetDestinationForScript`).

940a21932b

Support downgrading after recovered keypool witness keys 7ee54fd7c7

Add address_types test

Improvements and cleanups by John Newbery

b224a47a1a

sipa force-pushed on Jan 9, 2018

ryanofsky commented at 8:48 pm on January 10, 2018: member

utACK b224a47a1a5feac380506abff63fae91d7a93b39. Only real change since last review is adding LearnAllRelatedScripts call in importpubkey (aside from updating a comment and removing auto).

promag commented at 10:46 pm on January 10, 2018: member

jonasschnelli merged this on Jan 11, 2018

jonasschnelli closed this on Jan 11, 2018

jonasschnelli commented at 6:56 am on January 11, 2018: contributor

Re-Tested ACK b224a47a1

jonasschnelli referenced this in commit d889c036cd on Jan 11, 2018

tonyalester commented at 6:58 am on January 11, 2018: none

ty

jonasschnelli deleted a comment on Jan 11, 2018

jonasschnelli locked this on Jan 11, 2018

laanwj removed this from the "Blockers" column in a project

laanwj unlocked this on Jan 11, 2018

luke-jr commented at 8:08 pm on January 11, 2018: member

Post-merge utACK

laanwj deleted a comment on Jan 11, 2018

practicalswift referenced this in commit f765bb3788 on Jan 11, 2018

practicalswift referenced this in commit 92a810d04b on Jan 11, 2018

classesjack referenced this in commit ff062feb5e on Jan 12, 2018

fanquake moved this from the "In progress" to the "Done" column in a project

jonasschnelli referenced this in commit 7abb0f0929 on Jan 24, 2018

Christewart referenced this in commit 4ea2dad433 on Feb 8, 2018

hkjn referenced this in commit 42b71f40ac on Feb 12, 2018

ryanofsky referenced this in commit b7f6002ed5 on Feb 13, 2018

sipa referenced this in commit 88ca1257f6 on Feb 14, 2018

sipa referenced this in commit 252ae7111c on Feb 14, 2018

laanwj referenced this in commit ff44101e8d on Feb 14, 2018

esotericnonsense referenced this in commit f5ffec1a55 on Feb 19, 2018

Willtech referenced this in commit 62ffac072d on Feb 23, 2018

HashUnlimited referenced this in commit b72a99d854 on Mar 15, 2018

HashUnlimited referenced this in commit 1b4948eddd on Mar 16, 2018

HashUnlimited referenced this in commit 2f20e65301 on Mar 16, 2018

virtload referenced this in commit 170460d4ad on Apr 4, 2018

HashUnlimited referenced this in commit caaf69f9c9 on Jul 31, 2018

ccebrecos referenced this in commit 20f290880c on Sep 14, 2018

lionello referenced this in commit d3339182ec on Nov 7, 2018

lionello referenced this in commit 124c744c89 on Nov 7, 2018

PastaPastaPasta referenced this in commit 7d12744e2f on Jan 17, 2020

MarcoFalke referenced this in commit caa2f3af29 on Feb 12, 2020

PastaPastaPasta referenced this in commit 98ae41c821 on Feb 12, 2020

PastaPastaPasta referenced this in commit a84e6db1e2 on Feb 13, 2020

PastaPastaPasta referenced this in commit 96155f1d93 on Feb 27, 2020

PastaPastaPasta referenced this in commit a9a133b04d on Feb 27, 2020

PastaPastaPasta referenced this in commit 9cc5078218 on Feb 27, 2020

PastaPastaPasta referenced this in commit 3810f0123c on Mar 23, 2020

PastaPastaPasta referenced this in commit 318232503e on Mar 28, 2020

PastaPastaPasta referenced this in commit cc041952b7 on Mar 29, 2020

PastaPastaPasta referenced this in commit 2f72fcc35a on Mar 31, 2020

UdjinM6 referenced this in commit e09077af41 on Mar 31, 2020

PastaPastaPasta referenced this in commit 9d3c7c3ca6 on Apr 1, 2020

PastaPastaPasta referenced this in commit 5ca5adca7b on Apr 6, 2020

deadalnix referenced this in commit 87f67834ec on May 19, 2020

ftrader referenced this in commit f3a0c25c99 on Aug 17, 2020

xdustinface referenced this in commit 36cd11f23a on Oct 2, 2020

xdustinface referenced this in commit 0bba962370 on Oct 5, 2020

xdustinface referenced this in commit 650bde9e4e on Oct 5, 2020

xdustinface referenced this in commit 7ab1051bf0 on Oct 14, 2020

UdjinM6 referenced this in commit 74f4d2a898 on Oct 14, 2020

ckti referenced this in commit 889455cfab on Mar 28, 2021

ckti referenced this in commit 2f08e2d954 on Mar 28, 2021

UdjinM6 referenced this in commit 9e4ce70f34 on Jun 19, 2021

UdjinM6 referenced this in commit a6e5a59fe4 on Jun 24, 2021

UdjinM6 referenced this in commit 5b96301c5c on Jun 26, 2021

UdjinM6 referenced this in commit ce5b9559a7 on Jun 26, 2021

PastaPastaPasta referenced this in commit a4c7a60f18 on Jun 27, 2021

UdjinM6 referenced this in commit 01fccc4694 on Jun 28, 2021

gades referenced this in commit e343ae2d9c on Jul 1, 2021

malbit referenced this in commit c141d9fef4 on Nov 5, 2021

gades referenced this in commit b516baa6af on Feb 14, 2022

gades referenced this in commit c28588fdf3 on Feb 15, 2022

DrahtBot locked this on Feb 15, 2022

SegWit wallet support #11403

QT

New wallet, default settings

New wallet -addresstype=bech32

Existing HD wallet

Existing non-HD wallet

Existing wallet that used getwitnessaddress

RPC

New wallet, default settings

New wallet -addresstype=bech32

Existing HD wallet

Existing non-HD wallet

Issues I’m seeing:

New wallet `-addresstype=bech32`

Existing wallet that used `getwitnessaddress`

New wallet `-addresstype=bech32`