Enable various p2sh-p2wpkh functionality

instagibbs commented at 7:44 pm on October 25, 2016: member

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.

in src/base58.cpp: in 38186e2acb outdated

276@@ -277,6 +277,16 @@ bool CBitcoinAddress::GetKeyID(CKeyID& keyID) const
277     return true;
278 }
279 
280+bool CBitcoinAddress::GetScriptID(CScriptID& scriptID) const
281+{
282+    if (!IsValid() || vchVersion != Params().Base58Prefix(CChainParams::SCRIPT_ADDRESS))
283+        return false;
284+    uint160 id;
285+    memcpy(&id, &vchData[0], 20);

JeremyRubin commented at 8:41 pm on October 25, 2016:

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.

ryanofsky commented at 6:17 pm on November 3, 2016:

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

laanwj added the label Wallet on Oct 26, 2016

in src/rpc/misc.cpp: in 8f001d24c7 outdated

134@@ -135,6 +135,12 @@ class DescribeAddressVisitor : public boost::static_visitor<UniValue>
135             int nRequired;
136             ExtractDestinations(subscript, whichType, addresses, nRequired);
137             obj.push_back(Pair("script", GetTxnOutputType(whichType)));
138+            CKeyID keyID;
139+            CPubKey pubkey;
140+            if (pwalletMain->GetWitnessKeyID(scriptID, keyID) &&
141+                pwalletMain->GetPubKey(keyID, pubkey) && pubkey.IsCompressed()) {

ryanofsky commented at 5:29 pm on November 3, 2016:

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

instagibbs commented at 7:56 pm on November 3, 2016:

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.

in src/wallet/crypter.cpp: in f53bc8be5b outdated

276@@ -277,6 +277,20 @@ bool CCryptoKeyStore::GetPubKey(const CKeyID &address, CPubKey& vchPubKeyOut) co
277     return false;
278 }
279 
280+bool CCryptoKeyStore::GetWitnessKeyID(const CScriptID &scriptID, CKeyID &keyID) const

ryanofsky commented at 5:44 pm on November 3, 2016:

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

instagibbs commented at 7:58 pm on November 3, 2016:

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

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

instagibbs commented at 1:33 pm on November 4, 2016:

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.

in src/wallet/crypter.cpp: in f53bc8be5b outdated

282+    CScript subscript;
283+    int witnessVer = 0;
284+    std::vector<unsigned char> witnessProgram;
285+    if (!GetCScript(scriptID, subscript) ||
286+        !subscript.IsWitnessProgram(witnessVer, witnessProgram) ||
287+        witnessProgram.size() != 20) {

ryanofsky commented at 6:07 pm on November 3, 2016:

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

in src/wallet/rpcdump.cpp: in 70ce4ed096 outdated

538@@ -539,11 +539,14 @@ UniValue dumpprivkey(const JSONRPCRequest& request)
539 
540     string strAddress = request.params[0].get_str();
541     CBitcoinAddress address;
542-    if (!address.SetString(strAddress))
543+    if (!address.SetString(strAddress) || !address.IsValid())

ryanofsky commented at 6:30 pm on November 3, 2016:

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

in src/wallet/rpcwallet.cpp: in 2dce040ae0 outdated

1015@@ -1016,51 +1016,6 @@ UniValue addmultisigaddress(const JSONRPCRequest& request)
1016     return CBitcoinAddress(innerID).ToString();
1017 }
1018 
1019-class Witnessifier : public boost::static_visitor<bool>

ryanofsky commented at 6:34 pm on November 3, 2016:

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

in src/wallet/wallet.h: in 2dce040ae0 outdated

985@@ -986,4 +986,49 @@ class CAccount
986     }
987 };
988 
989+class Witnessifier : public boost::static_visitor<bool>

ryanofsky commented at 6:47 pm on November 3, 2016:

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.

instagibbs commented at 2:19 pm on November 4, 2016:

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

in src/wallet/rpcdump.cpp: in 1237cee5ab outdated

132@@ -133,6 +133,14 @@ UniValue importprivkey(const JSONRPCRequest& request)
133         pwalletMain->MarkDirty();
134         pwalletMain->SetAddressBook(vchAddress, strLabel, "receive");
135 
136+        if (IsWitnessEnabled(chainActive.Tip(), Params().GetConsensus()) || GetBoolArg("-walletprematurewitness", false)) {
137+            Witnessifier w;

ryanofsky commented at 7:13 pm on November 3, 2016:

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.

instagibbs commented at 2:10 pm on November 4, 2016:

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.

in src/wallet/rpcdump.cpp: in 1237cee5ab outdated

132@@ -133,6 +133,14 @@ UniValue importprivkey(const JSONRPCRequest& request)
133         pwalletMain->MarkDirty();
134         pwalletMain->SetAddressBook(vchAddress, strLabel, "receive");
135 
136+        if (IsWitnessEnabled(chainActive.Tip(), Params().GetConsensus()) || GetBoolArg("-walletprematurewitness", false)) {

ryanofsky commented at 7:32 pm on November 3, 2016:

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 commented at 0:29 am on November 4, 2016: member

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 force-pushed on Nov 4, 2016

instagibbs commented at 4:28 pm on November 4, 2016: member

@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 commented at 4:31 pm on November 4, 2016: member

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

instagibbs commented at 4:33 pm on November 4, 2016: member

@sipa yes, I misspoke

Add function to retrieve nested p2sh witness program keyhash from keystore dfbf9f5665

Enable pubkey lookup for p2sh-p2wpkh in validateaddress e6a435fce2

Simplify GetKeyID 4591f13995

Add CBitcoinAddress::GetScriptID 57b25a7399

Enable dumpprivkey for p2sh-p2wpkh b62b8f38ff

{sign, verify}message support for p2sh-p2wpkh a3d682803f

Accept p2sh-p2wpkh addr in createmultisig_redeemScript 6a670008ed

instagibbs force-pushed on Nov 7, 2016

instagibbs commented at 5:27 pm on November 7, 2016: member

Moved GetWitnessKeyID outside of Keystore and addressed some nits.

jtimon commented at 9:05 pm on January 25, 2017: contributor

Concept ACK

in src/rpc/misc.cpp: in 6a670008ed

389+        //Failure here could mean address is any other type of p2sh
390+        if (scriptID != nestedScriptID) {
391+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Given P2SH address does not match the signature.");
392+        }
393+    }
394+

luke-jr commented at 5:28 am on February 3, 2017:

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…

in src/rpc/misc.cpp: in 6a670008ed

211@@ -204,7 +212,8 @@ UniValue validateaddress(const JSONRPCRequest& request)
212         if (pwalletMain && pwalletMain->mapAddressBook.count(dest))
213             ret.push_back(Pair("account", pwalletMain->mapAddressBook[dest].name));
214         CKeyID keyID;
215-        if (pwalletMain && address.GetKeyID(keyID) && pwalletMain->mapKeyMetadata.count(keyID) && !pwalletMain->mapKeyMetadata[keyID].hdKeypath.empty())
216+        CScriptID scriptID;
217+        if (pwalletMain && (address.GetKeyID(keyID) || (address.GetScriptID(scriptID) && GetWitnessKeyID(pwalletMain, scriptID, keyID))) && pwalletMain->mapKeyMetadata.count(keyID) && !pwalletMain->mapKeyMetadata[keyID].hdKeypath.empty())

kallewoof commented at 8:18 pm on February 28, 2017:

A bit hard to interpret this line. Maybe

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

I also think a simple inline method like

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

would help, as it would reduce the above to

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

Also see line 255 below.

in src/base58.cpp: in 6a670008ed

276-    keyID = CKeyID(id);
277+    memcpy(keyID.begin(), &vchData[0], 20);
278+    return true;
279+}
280+
281+bool CBitcoinAddress::GetScriptID(CScriptID& scriptID) const

kallewoof commented at 8:49 pm on February 28, 2017:

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.

in src/wallet/wallet.cpp: in 6a670008ed

3739@@ -3740,3 +3740,17 @@ bool CMerkleTx::AcceptToMemoryPool(const CAmount& nAbsurdFee, CValidationState&
3740 {
3741     return ::AcceptToMemoryPool(mempool, state, *this, true, NULL, false, nAbsurdFee);
3742 }
3743+
3744+bool GetWitnessKeyID(const CKeyStore* store, const CScriptID &scriptID, CKeyID &keyID)

kallewoof commented at 8:50 pm on February 28, 2017:

See previous comment regarding Get prefix.

in src/rpc/misc.cpp: in 6a670008ed

250@@ -242,7 +251,8 @@ CScript _createmultisig_redeemScript(const UniValue& params)
251         if (pwalletMain && address.IsValid())
252         {
253             CKeyID keyID;
254-            if (!address.GetKeyID(keyID))
255+            CScriptID scriptID;
256+            if (!address.GetKeyID(keyID) && (!address.GetScriptID(scriptID) || !GetWitnessKeyID(pwalletMain, scriptID, keyID)))

kallewoof commented at 9:03 pm on February 28, 2017:

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

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

->

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

in src/wallet/rpcdump.cpp: in 6a670008ed

541@@ -542,8 +542,11 @@ UniValue dumpprivkey(const JSONRPCRequest& request)
542     if (!address.SetString(strAddress))
543         throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid Bitcoin address");
544     CKeyID keyID;
545-    if (!address.GetKeyID(keyID))
546+    CScriptID scriptID;
547+    if ((!address.GetScriptID(scriptID) || !GetWitnessKeyID(pwalletMain, scriptID, keyID)) &&
548+        !address.GetKeyID(keyID)) {

kallewoof commented at 9:10 pm on February 28, 2017:

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

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

in src/wallet/rpcwallet.cpp: in 6a670008ed

519@@ -520,7 +520,8 @@ UniValue signmessage(const JSONRPCRequest& request)
520         throw JSONRPCError(RPC_TYPE_ERROR, "Invalid address");
521 
522     CKeyID keyID;
523-    if (!addr.GetKeyID(keyID))
524+    CScriptID scriptID;
525+    if (!addr.GetKeyID(keyID) && (!addr.GetScriptID(scriptID) || !GetWitnessKeyID(pwalletMain, scriptID, keyID)))

kallewoof commented at 9:11 pm on February 28, 2017:

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

instagibbs commented at 9:17 pm on February 28, 2017: member

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.

instagibbs closed this on Feb 28, 2017

luke-jr commented at 2:54 pm on August 18, 2017: member

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 commented at 4:55 pm on August 18, 2017: member

Re-opening since segwit is actually activating.

instagibbs reopened this on Aug 18, 2017

instagibbs commented at 5:17 pm on August 18, 2017: member

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

instagibbs closed this on Aug 18, 2017

DrahtBot locked this on Sep 8, 2021

Enable various p2sh-p2wpkh functionality #9017