SegWit wallet support #11403

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

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

Two new configuration options are added:

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

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

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

To deal with witness outputs requiring a corresponding redeemscript in wallet, three approaches are used:

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

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

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

promag

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

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

src/wallet/wallet.cpp

+CKeyID CWallet::GetKeyForDestination(const CTxDestination& dest) const
+{
+    // Only supports P2PKH, P2WPKH, P2SH-P2WPKH.
+    if (boost::get<CKeyID>(&dest)) {


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

Use boost::apply_visitor instead?

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

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

src/wallet/wallet.cpp

+        CScript script;
+        CTxDestination inner_dest;
+        if (GetCScript(boost::get<CScriptID>(dest), script) && ExtractDestination(script, inner_dest)) {
+            if (boost::get<WitnessV0KeyHash>(&inner_dest)) {


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

Recursive call?

return GetKeyForDestination(inner_dest);

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

Right.

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

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

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

src/wallet/wallet.cpp

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


In commit SegWit wallet support:

Remove?

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

src/wallet/init.cpp

+    std::string style = gArgs.GetArg("-addressstyle", "default");
+    OutputStyle parsed_style = ParseStyle(style);
+    if (parsed_style == STYLE_NONE) {
+        return InitError(strprintf(_("Unknown address style '%s'"), style));


Add init tests (follow up maybe)?

Will add those later.

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

src/wallet/rpcwallet.cpp

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


Missing default value?

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

src/wallet/rpcwallet.cpp

+    if (!request.params[1].isNull()) {
+        style = ParseStyle(request.params[1].get_str());
+        if (style == STYLE_NONE) {
+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address style '%s'", request.params[1].get_str()));


Missing test for this error.

Will add later.

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

src/wallet/rpcwallet.cpp

            "\nReturns a new Bitcoin address for receiving payments.\n"
            "If 'account' is specified (DEPRECATED), it is added to the address book \n"
            "so payments received with the address will be credited to 'account'.\n"
            "\nArguments:\n"
            "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"
+            "2. \"style\"          (string, optional) The address style to use. Valid options are 'p2pkh', 'p2sh_p2wpkh', and 'p2wpkh'\n"


Default value?

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

src/wallet/init.cpp

+    style = gArgs.GetArg("-changestyle", style);
+    parsed_style = ParseStyle(style);
+    if (parsed_style == STYLE_NONE) {
+        return InitError(strprintf(_("Unknown change style '%s'"), style));


Add init tests (follow up maybe)?

Will add those later.

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

src/wallet/rpcwallet.cpp

+    if (!request.params[0].isNull()) {
+        style = ParseStyle(request.params[1].get_str());
+        if (style == STYLE_NONE) {
+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address style '%s'", request.params[0].get_str()));


Missing test.

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

test/functional/segwit.py

-            self.nodes[i].addwitnessaddress(newaddress)
-            self.nodes[i].addwitnessaddress(multiaddress)
+            multiscript = CScript([OP_1, hex_str_to_bytes(self.pubkey[-1]), OP_1, OP_CHECKMULTISIG])
+            p2sh_addr = self.nodes[i].addwitnessaddress(newaddress, True)


Remove , True so that default value is tested.

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

src/wallet/init.cpp

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


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

Added explanation of defaults.

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

src/wallet/wallet.cpp

+    if (style == STYLE_STRING_LEGACY) {
+        return STYLE_LEGACY;
+    } else if (style == STYLE_STRING_P2SH || style == "default") {
+        return STYLE_P2SH;


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

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

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

Yes and no.

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

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

src/wallet/rpcwallet.cpp

            "\nReturns a new Bitcoin address for receiving payments.\n"
            "If 'account' is specified (DEPRECATED), it is added to the address book \n"
            "so payments received with the address will be credited to 'account'.\n"
            "\nArguments:\n"
            "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"
+            "2. \"style\"          (string, optional) The address style to use. Valid options are 'p2pkh', 'p2sh_p2wpkh', and 'p2wpkh'\n"


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

I think that should be done separately.

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

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

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

maybe it should be renamed to -addressstyle=bech32?

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

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

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

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

src/rpc/misc.cpp

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


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

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

(Use after move here)

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

src/wallet/wallet.h

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


Remove plurals from description.

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

Rebased after #11167 merge.

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

src/wallet/wallet.cpp

+
+CKeyID CWallet::GetKeyForDestination(const CTxDestination& dest) const
+{
+    // Only supports P2PKH, P2WPKH, P2SH-P2WPKH.


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

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

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

src/wallet/rpcwallet.cpp

            "\nReturns a new Bitcoin address for receiving payments.\n"
            "If 'account' is specified (DEPRECATED), it is added to the address book \n"
            "so payments received with the address will be credited to 'account'.\n"
            "\nArguments:\n"
            "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"
+            "2. \"style\"          (string, optional) The address style to use. Valid options are 'p2pkh', 'p2sh_p2wpkh', and 'p2wpkh'\n"


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

'p2sh_p2wpkh' vs p2sh etc

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

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

src/wallet/wallet.cpp

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


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

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

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

src/wallet/init.cpp

+        address_style = parsed_style;
+    }
+
+    style = gArgs.GetArg("-changestyle", style);


changestyle needs a test

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

src/wallet/rpcwallet.cpp

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


argument needs a test

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

src/wallet/rpcwallet.cpp

@@ -1234,7 +1254,7 @@ UniValue addwitnessaddress(const JSONRPCRequest& request)

    {
        LOCK(cs_main);
-        if (!IsWitnessEnabled(chainActive.Tip(), Params().GetConsensus()) && !gArgs.GetBoolArg("-walletprematurewitness", false)) {
+        if (!IsWitnessEnabled(pindexBestHeader, Params().GetConsensus()) && !gArgs.GetBoolArg("-walletprematurewitness", false)) {


change seems way out of place here

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

src/wallet/wallet.cpp

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


Same suggestion as address_style

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

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

kallewoof

utACK 5632374f2f71bca384ceed07ca6842fd87102346

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

src/wallet/wallet.cpp

+static const std::string STYLE_STRING_NONE = "";
+static const std::string STYLE_STRING_LEGACY = "legacy";
+static const std::string STYLE_STRING_P2SH = "p2sh";
+static const std::string STYLE_STRING_SEGWIT = "segwit";


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

Fixed, I think it's consistent now.

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

src/wallet/rpcwallet.cpp

        throw std::runtime_error(
-            "getnewaddress ( \"account\" )\n"
+            "getnewaddress ( \"account\" ) ( \"style\" )\n"


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

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

src/wallet/rpcwallet.cpp

        throw std::runtime_error(
-            "getnewaddress ( \"account\" )\n"
+            "getnewaddress ( \"account\" ) ( \"style\" )\n"


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

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

#11403 (comment)

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

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

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

src/script/standard.h

+struct WitnessV0KeyHash : public uint160
+{
+public:
+    WitnessV0KeyHash() : uint160() {}


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

using uint160::uint160;

Cool, done.

theuni

Added a few more comments.

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

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

src/rpc/misc.cpp

+            if (whichType == TX_WITNESS_V0_KEYHASH || whichType == TX_WITNESS_V0_SCRIPTHASH) {
+                UniValue subobj = boost::apply_visitor(*this, addresses[0]);
+                obj.push_back(Pair("embedded", std::move(subobj)));
+                if (subobj.exists("pubkey")) {


Why dupe this? Helps with existing tooling?

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

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

src/wallet/wallet.cpp

+CKeyID CWallet::GetKeyForDestination(const CTxDestination& dest) const
+{
+    // Only supports P2PKH, P2WPKH, P2SH-P2WPKH.
+    if (boost::get<CKeyID>(&dest)) {


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

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

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

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

src/keystore.cpp

+    if (pubkey.IsCompressed()) {
+        CScript script = CScript() << OP_0 << ToByteVector(pubkey.GetID());
+        // This does not use AddCScript, as it may be overridden.
+        mapScripts[CScriptID(script)] = std::move(script);


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

Ugh, thanks for pointing that out.

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

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

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

src/wallet/wallet.cpp

+    // if we were to have the private keys. This is just to make sure that the script is valid and that,
+    // if found in a transaction, we would still accept and relay that transaction. In particular,
+    // it will reject witness outputs that require signing with an uncompressed public key.
+    DummySignatureCreator creator(this);


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

Moved to script/signer.

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

src/wallet/wallet.cpp

+    }
+}
+
+CTxDestination CWallet::GetDestinationForKey(const CPubKey& key, OutputStyle style)


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

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

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

src/wallet/wallet.cpp

+        CScript witprog = GetScriptForDestination(witdest);
+        // Check if the resulting program is solvable (i.e. doesn't use an uncompressed key)
+        if (!IsSolvable(witprog)) return CScriptID(script);
+        AddCScript(witprog);


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

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

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

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

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

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

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

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

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

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

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

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

… GetAccountDestination

@TheBlueMatt

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

@TheBlueMatt

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

@TheBlueMatt

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

@TheBlueMatt

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

@TheBlueMatt

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

@TheBlueMatt

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

…s_type, g_change_type f765bb3 Fix ListCoins test failure due to unset g_address_type, g_change_type (Russell Yanofsky) Pull request description: New global variables were introduced in bitcoin#11403 and not setting them causes: ``` test_bitcoin: wallet/wallet.cpp:4199: CTxDestination GetDestinationForKey(const CPubKey&, OutputType): Assertion `false' failed. unknown location(0): fatal error in "ListCoins": signal: SIGABRT (application abort requested) ``` It's possible to reproduce the failure reliably by running: ``` src/test/test_bitcoin --log_level=test_suite --run_test=wallet_tests/ListCoins ``` Failures happen nondeterministically because boost test framework doesn't run tests in a specified order, and tests that run previously can set the global variables and mask the bug. Example travis failure: https://travis-ci.org/bitcoin/bitcoin/builds/327642495 Tree-SHA512: 3e0875716f66bc0304cf92a26457e6b54ecfe15ed962f4343577b05fc56bb577554422b7f53949ad6085ac5798ad7816b8176c5b01e050ddbfbb925d2732767a

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

… parts of bitcoin#11403)

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

…ype, g_change_type b7f6002 Fix rescan test failure due to unset g_address_type, g_change_type (Russell Yanofsky) Pull request description: New global variables were introduced in bitcoin#11403 and not setting them causes: ``` test_bitcoin: wallet/wallet.cpp:4259: CTxDestination GetDestinationForKey(const CPubKey&, OutputType): Assertion `false' failed. unknown location(0): fatal error in "importwallet_rescan": signal: SIGABRT (application abort requested) ``` It's possible to reproduce the failure reliably by running: ``` src/test/test_bitcoin --log_level=test_suite --run_test=wallet_tests/importwallet_rescan ``` Failures happen nondeterministically because boost test framework doesn't run tests in a specified order, and tests that run previously can set the global variables and mask the bug. This is similar to bug bitcoin#12150. Example travis failure is https://travis-ci.org/bitcoin/bitcoin/jobs/340642010 Tree-SHA512: ab40662b3356892b726f1f552e22d58d86b5e982538741e52b37ee447a0c97c76c24ae543687edf2e25d9dd925722909d37abfae95d93bf09e23fa245a4c3351

@TheBlueMatt

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

@TheBlueMatt

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

… GetAccountDestination

fanquake added the Wallet label Sep 26, 2017

promag reviewed Sep 26, 2017

View reviewed changes

sipa mentioned this pull request Sep 26, 2017

Full BIP173 (Bech32) support #11167

Merged

luke-jr reviewed Sep 26, 2017

View reviewed changes

jl2012 mentioned this pull request Sep 27, 2017

Hardcode CSV and SEGWIT deployment #11398

Closed

instagibbs reviewed Sep 29, 2017

View reviewed changes

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

instagibbs reviewed Sep 29, 2017

View reviewed changes

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

Sjors mentioned this pull request Oct 4, 2017

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

Closed

laanwj mentioned this pull request Oct 5, 2017

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

Merged

maflcko added this to the 0.15.1 milestone Oct 5, 2017

maflcko added the Needs backport label Oct 5, 2017

kallewoof mentioned this pull request Oct 12, 2017

[wallet] sendtoaddress output type argument #11489

Closed

kallewoof approved these changes Oct 12, 2017

View reviewed changes

luke-jr reviewed Oct 12, 2017

View reviewed changes

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

theuni reviewed Oct 17, 2017

View reviewed changes

promag mentioned this pull request Oct 18, 2017

Enable various p2sh-p2wpkh functionality #11089

Closed

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

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

a84e6db

… GetAccountDestination

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

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

96155f1

… GetAccountDestination

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

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

a9a133b

… GetAccountDestination

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

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

9cc5078

… GetAccountDestination

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

More of "Remove direct bitcoin calls from qt/paymentserver.cpp" (Adds…

36cd11f

… parts of bitcoin#11403)

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

Partially backport bitcoin#11403

0bba962

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

Partially backport bitcoin#11403

650bde9

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

Partially backport bitcoin#11403

7ab1051

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

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

74f4d2a

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

xdustinface mentioned this pull request Oct 26, 2020

Backports 0.17 pr20 dashpay/dash#3742

Merged

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

partial merge bitcoin#11403: Abstract out IsSolvable from Witnessifier

9e4ce70

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

partial merge bitcoin#11403: Abstract out IsSolvable from Witnessifier

a6e5a59

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

partial merge bitcoin#11403: Abstract out IsSolvable from Witnessifier

5b96301

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

partial merge bitcoin#11403: Abstract out IsSolvable from Witnessifier

ce5b955

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

partial merge bitcoin#11403: Abstract out IsSolvable from Witnessifier

01fccc4

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

Partially backport bitcoin#11403

e343ae2

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

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

c28588f

… GetAccountDestination

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

SegWit wallet support #11403

SegWit wallet support #11403

Conversation

sipa commented Sep 26, 2017 • edited

promag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petertodd commented Sep 28, 2017

flack commented Sep 28, 2017

meshcollider commented Sep 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sipa commented Sep 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

instagibbs Sep 29, 2017 • edited

Choose a reason for hiding this comment

sipa Nov 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmaxwell commented Oct 11, 2017

kallewoof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theuni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theuni Oct 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sipa commented Sep 26, 2017 •

edited

instagibbs Sep 29, 2017 •

edited

sipa Nov 18, 2017 •

edited

theuni Oct 17, 2017 •

edited