Enable various p2sh-p2wpkh functionality by instagibbs · Pull Request #9017 · bitcoin/bitcoin

instagibbs · 2016-10-25T19:44:43Z

A followup to #8992 which includes a number of additional changes.

The only addition of data to wallets is done in importprivkey with the normal IsWitnessEnabled and -prematurewalletwitness gating.

I can write up tests for these if I get concept ACKs.

I wrote and manually tested these by turning on p2sh-p2wpkh by default via getnewaddress, and seeing what failed. Rest of failures seem to involve no-witness-yet and related sigop counting errors.

JeremyRubin · 2016-10-25T20:41:31Z

src/base58.cpp

This is safe, but technically you are copying the vchData, which is specified to be zero_after_free memory, so you lose any guarantee that isn't somewhere in memory still.

You also may as well just directly use &scriptID (or scriptID.begin()) here rather than creating a temp.

Should at least replace &id with id.begin()` on this line to avoid making an assumption about the layout of uint160 objects.

ryanofsky · 2016-11-03T17:29:22Z

src/rpc/misc.cpp

+            CKeyID keyID;
+            CPubKey pubkey;
+            if (pwalletMain->GetWitnessKeyID(scriptID, keyID) &&
+                pwalletMain->GetPubKey(keyID, pubkey) && pubkey.IsCompressed()) {


Curious when the pubkey.IsCompressed condition would not be true? Might be nice to note when the condition is expected in a comment. Alternately it might make more sense as an assertion if it's never expected, or as a separate JSON attribute like in the CKeyID handler above (line 123): obj.push_back(Pair("iscompressed", vchPubKey.IsCompressed()));

True an assert may make the most sense here. It really shouldn't happen, and if it is we want to find out as soon as possible to avoid loss of funds.

ryanofsky · 2016-11-03T17:44:15Z

src/wallet/crypter.cpp

    return false;
 }

+bool CCryptoKeyStore::GetWitnessKeyID(const CScriptID &scriptID, CKeyID &keyID) const


Unless I'm missing something, nothing in this function seems specific to the CCryptoKeyStore type as opposed to a more general type like CKeyStore. If this is the case, it might be better to make this a CKeyStore member or a standalone function taking a const CKeyStore&.

CKeyStore doesn't have access to keys if they're crypted.

A standalone function may make more sense, I'm really ambivalent about this.

Hmm, actually you're right, nothing in particular needs a CCryptoKeyStore. I thoroughly convinced myself otherwise previously somehow. Regardless, I'll divorce it from the keystore altogether.

ryanofsky · 2016-11-03T18:07:12Z

src/wallet/crypter.cpp

+    std::vector<unsigned char> witnessProgram;
+    if (!GetCScript(scriptID, subscript) ||
+        !subscript.IsWitnessProgram(witnessVer, witnessProgram) ||
+        witnessProgram.size() != 20) {


The size 20 check and explicit uint160 construction below seem like low level encoding details that are out of place here. Maybe this logic could go into to a new CScript method like: bool CScript::GetWitnessKeyID(CKeyID& keyID) or bool CScript::GetV0WitnessKeyID(uint160& keyID).

ryanofsky · 2016-11-03T18:17:28Z

src/base58.cpp

Should at least replace &id with id.begin()` on this line to avoid making an assumption about the layout of uint160 objects.

ryanofsky · 2016-11-03T18:30:44Z

src/wallet/rpcdump.cpp

    string strAddress = request.params[0].get_str();
    CBitcoinAddress address;
-    if (!address.SetString(strAddress))
+    if (!address.SetString(strAddress) || !address.IsValid())


This change makes the error message below somewhat inaccurate. Maybe replace "Invalid Bitcoin address" with string("Invalid Bitcoin address for network ") + Params().NetworkIDString() below.

ryanofsky · 2016-11-03T18:34:16Z

src/wallet/rpcwallet.cpp

    return CBitcoinAddress(innerID).ToString();
 }

-class Witnessifier : public boost::static_visitor<bool>


Maybe note in commit description that this change is moveonly (no code changes).

ryanofsky · 2016-11-03T18:47:08Z

src/wallet/wallet.h

    }
 };

+class Witnessifier : public boost::static_visitor<bool>


It might be easier for callers if instead of exposing the Witnessifier class in wallet.h, you expose a simpler function that can be called by both importprivkey and addwitnessaddress and hides all the boost visitor implementation details:

bool AddWitnessCScript(CTxDestination dest, CScriptID &id);

This could even be a CWallet member function.

Sounds like a good refactor for another PR, especially if I get rid of importprivkey commit. I'll drop this commit as well otherwise.

ryanofsky · 2016-11-03T19:13:10Z

src/wallet/rpcdump.cpp

This block of code seems very similar to code in addwitnessaddress:

https://github.com/instagibbs/bitcoin/blob/1237cee5ab3506188fec78d33577f49ac78cd570/src/wallet/rpcwallet.cpp#L1053

except this ignores the apply_visitor return value and passes a nonempty strlabel to SetAddressBook. Can both places be changed to call a common function?

Also is it ok to ignore the apply_visitor return value here? Consider adding a comment explaining why if it is ok.

Not sure I should over-optimize this for 2 instances. Although I'm also getting concerned about the logic in this commit.

If the user already has the p2pkh key in address book but not the nested version, it won't rescan(just a bug I can fix)

I'm concerned of the user flow here. As soon as segwit activates it starts adding segwit addresses to the address book. This kind of behavior would be really unsafe for getnewaddress and it seems unsafe for the same reason here. It might make more sense to simply tell the user to importprivkey, then addwitnessaddress. That is literally equivalent.

In future wallet updates when getnewaddress is flipped to nested p2sh using some command line init variable, we can have this flip as well, or simply add both as a precaution.

Unless there is a strong counter-argument here I think I should just delete this commit.

ryanofsky · 2016-11-03T19:32:01Z

src/wallet/rpcdump.cpp

Would it be useful to callers to have an error message in the case where an update to the address book is skipped because this condition is false? Either way an explanatory comment might be good here.

sipa · 2016-11-04T00:29:52Z

Why do you need GetWitnessKeyID in keystore? That doesn't seem to be something that belongs in the interface, and could just be implemented on top in the signing logic.

instagibbs · 2016-11-04T16:28:36Z

@sipa it certainly does not need to be there and is likely inappropriate. I'm not sure exactly where to put it:
Will require access to a wallet, and is used in rpcmisc, rpcdump, and rpcwallet.cpp.

sipa · 2016-11-04T16:31:49Z

It only needs access to a keystore, no? Not to a wallet.

instagibbs · 2016-11-04T16:33:34Z

@sipa yes, I misspoke

…store

instagibbs · 2016-11-07T17:27:43Z

Moved GetWitnessKeyID outside of Keystore and addressed some nits.

jtimon · 2017-01-25T21:05:17Z

Concept ACK

luke-jr · 2017-02-03T05:28:27Z

src/rpc/misc.cpp

+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Given P2SH address does not match the signature.");
+        }
+    }
+


I'm not sure we want to extend the current message signing to new address formats as-is. Maybe better to leave this out for now...

kallewoof · 2017-02-28T20:18:00Z

src/rpc/misc.cpp

        CKeyID keyID;
-        if (pwalletMain && address.GetKeyID(keyID) && pwalletMain->mapKeyMetadata.count(keyID) && !pwalletMain->mapKeyMetadata[keyID].hdKeypath.empty())
+        CScriptID scriptID;
+        if (pwalletMain && (address.GetKeyID(keyID) || (address.GetScriptID(scriptID) && GetWitnessKeyID(pwalletMain, scriptID, keyID))) && pwalletMain->mapKeyMetadata.count(keyID) && !pwalletMain->mapKeyMetadata[keyID].hdKeypath.empty())


A bit hard to interpret this line. Maybe

if (pwalletMain && (address.GetKeyID(keyID) || (address.GetScriptID(scriptID) && GetWitnessKeyID(pwalletMain, scriptID, keyID))) && pwalletMain->mapKeyMetadata.count(keyID) && !pwalletMain->mapKeyMetadata[keyID].hdKeypath.empty())

I also think a simple inline method like

inline bool FetchWalletKeyID(CWallet* pwallet, const CBitcoinAddress& address, CKeyID& keyID) { CScriptID scriptID; return pwallet && (address.GetKeyID(keyID) || (address.GetScriptID(scriptID) && GetWitnessKeyID(pwallet, scriptID, keyID))); }

would help, as it would reduce the above to

if (FetchWalletKeyID(pwalletMain, address, keyID) && pwalletMain->mapKeyMetadata.count(keyID) && !pwalletMain->mapKeyMetadata[keyID].hdKeypath.empty())

Also see line 255 below.

kallewoof · 2017-02-28T20:49:35Z

src/base58.cpp

+    return true;
+}
+
+bool CBitcoinAddress::GetScriptID(CScriptID& scriptID) const


I know this is done a lot elsewhere, but I think Get prefixed methods should return the result, not store it in a by-ref parameter. It's super easy to get confused especially with lines like if (GetX(y)) ..., where you presume it's some operation based on y returning an x, when in reality y is set and something else is returned.

Maybe use a different verb like FetchScriptID or MakeScriptID to hint at the difference.

kallewoof · 2017-02-28T20:50:48Z

src/wallet/wallet.cpp

    return ::AcceptToMemoryPool(mempool, state, *this, true, NULL, false, nAbsurdFee);
 }
+
+bool GetWitnessKeyID(const CKeyStore* store, const CScriptID &scriptID, CKeyID &keyID)


See previous comment regarding Get prefix.

kallewoof · 2017-02-28T21:03:39Z

src/rpc/misc.cpp

            CKeyID keyID;
-            if (!address.GetKeyID(keyID))
+            CScriptID scriptID;
+            if (!address.GetKeyID(keyID) && (!address.GetScriptID(scriptID) || !GetWitnessKeyID(pwalletMain, scriptID, keyID)))


Assuming an inline method as described on line 216 is added,

if (!address.GetKeyID(keyID) && (!address.GetScriptID(scriptID) || !GetWitnessKeyID(pwalletMain, scriptID, keyID)))

->

if (!FetchWalletKeyID(pwalletMain, address, keyID))

kallewoof · 2017-02-28T21:10:51Z

src/wallet/rpcdump.cpp

-    if (!address.GetKeyID(keyID))
+    CScriptID scriptID;
+    if ((!address.GetScriptID(scriptID) || !GetWitnessKeyID(pwalletMain, scriptID, keyID)) &&
+        !address.GetKeyID(keyID)) {


If I read this right, the logic is the same as previous two cases, except you are preferring witness style fetch over address.GetKeyID fetch. If this is not done for a specific reason, you can

if (!FetchWalletKeyID(pwalletMain, address, keyID)) {

kallewoof · 2017-02-28T21:11:50Z

src/wallet/rpcwallet.cpp

    CKeyID keyID;
-    if (!addr.GetKeyID(keyID))
+    CScriptID scriptID;
+    if (!addr.GetKeyID(keyID) && (!addr.GetScriptID(scriptID) || !GetWitnessKeyID(pwalletMain, scriptID, keyID)))


if (!FetchWalletKeyID(pwalletMain, addr, keyID))

instagibbs · 2017-02-28T21:17:37Z

I'm going to close this.

Segwit activation is a bit away so there's not much rush for this code, and we're expecting a new address format regardless so we'll likely only be supporting that.

luke-jr · 2017-08-18T14:54:48Z

we're expecting a new address format regardless so we'll likely only be supporting that.

Unlikely. Current wallets, even 0.15, can't send to bech32, so we'll need to support P2SH-wrapped addresses at least initially.

instagibbs · 2017-08-18T16:55:46Z

Re-opening since segwit is actually activating.

instagibbs · 2017-08-18T17:17:26Z

oops, @luke-jr opened his own pared-down copy, closing

JeremyRubin reviewed Oct 25, 2016

View reviewed changes

laanwj added the Wallet label Oct 26, 2016

ryanofsky reviewed Nov 3, 2016

View reviewed changes

instagibbs force-pushed the p2shp2wpkhstuff branch 9 times, most recently from 2cba903 to 1c6fcd5 Compare November 4, 2016 15:53

instagibbs added 7 commits November 7, 2016 12:25

Add function to retrieve nested p2sh witness program keyhash from key…

dfbf9f5

…store

Enable pubkey lookup for p2sh-p2wpkh in validateaddress

e6a435f

Simplify GetKeyID

4591f13

Add CBitcoinAddress::GetScriptID

57b25a7

Enable dumpprivkey for p2sh-p2wpkh

b62b8f3

{sign, verify}message support for p2sh-p2wpkh

a3d6828

Accept p2sh-p2wpkh addr in createmultisig_redeemScript

6a67000

instagibbs force-pushed the p2shp2wpkhstuff branch from 1c6fcd5 to 6a67000 Compare November 7, 2016 17:26

instagibbs mentioned this pull request Jan 4, 2017

Enable pubkey lookup for p2sh-p2wpkh in validateaddress #8992

Closed

luke-jr reviewed Feb 3, 2017

View reviewed changes

kallewoof reviewed Feb 28, 2017

View reviewed changes

instagibbs closed this Feb 28, 2017

luke-jr mentioned this pull request Aug 18, 2017

Enable various p2sh-p2wpkh functionality #11089

Closed

instagibbs reopened this Aug 18, 2017

instagibbs closed this Aug 18, 2017

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

instagibbs commented Oct 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

instagibbs Nov 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Nov 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

instagibbs commented Nov 4, 2016

Uh oh!

sipa commented Nov 4, 2016

Uh oh!

instagibbs commented Nov 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

instagibbs commented Nov 7, 2016

Uh oh!

jtimon commented Jan 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Feb 28, 2017

Uh oh!

luke-jr commented Aug 18, 2017

Uh oh!

instagibbs commented Aug 18, 2017

Uh oh!

instagibbs commented Aug 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

instagibbs Nov 4, 2016 •

edited

Loading

sipa commented Nov 4, 2016 •

edited

Loading

instagibbs commented Nov 4, 2016 •

edited

Loading