Enable various p2sh-p2wpkh functionality #9017

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.

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.

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.

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

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

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

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.

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.

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

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.

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

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.

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.

See previous comment regarding Get prefix.

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

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

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