New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable various p2sh-p2wpkh functionality #9017

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
8 participants
@instagibbs
Member

instagibbs commented Oct 25, 2016

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.

Show outdated Hide outdated src/base58.cpp
if (!IsValid() || vchVersion != Params().Base58Prefix(CChainParams::SCRIPT_ADDRESS))
return false;
uint160 id;
memcpy(&id, &vchData[0], 20);

This comment has been minimized.

@JeremyRubin

JeremyRubin Oct 25, 2016

Contributor

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.

@JeremyRubin

JeremyRubin Oct 25, 2016

Contributor

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.

This comment has been minimized.

@ryanofsky

ryanofsky Nov 3, 2016

Contributor

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

@ryanofsky

ryanofsky Nov 3, 2016

Contributor

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

@laanwj laanwj added the Wallet label Oct 26, 2016

Show outdated Hide outdated src/rpc/misc.cpp
CKeyID keyID;
CPubKey pubkey;
if (pwalletMain->GetWitnessKeyID(scriptID, keyID) &&
pwalletMain->GetPubKey(keyID, pubkey) && pubkey.IsCompressed()) {

This comment has been minimized.

@ryanofsky

ryanofsky Nov 3, 2016

Contributor

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

@ryanofsky

ryanofsky Nov 3, 2016

Contributor

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

This comment has been minimized.

@instagibbs

instagibbs Nov 3, 2016

Member

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.

@instagibbs

instagibbs Nov 3, 2016

Member

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.

Show outdated Hide outdated src/wallet/crypter.cpp
@@ -277,6 +277,20 @@ bool CCryptoKeyStore::GetPubKey(const CKeyID &address, CPubKey& vchPubKeyOut) co
return false;
}
bool CCryptoKeyStore::GetWitnessKeyID(const CScriptID &scriptID, CKeyID &keyID) const

This comment has been minimized.

@ryanofsky

ryanofsky Nov 3, 2016

Contributor

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

@ryanofsky

ryanofsky Nov 3, 2016

Contributor

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

This comment has been minimized.

@instagibbs

instagibbs Nov 3, 2016

Member

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

instagibbs Nov 3, 2016

Member

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

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

This comment has been minimized.

@instagibbs

instagibbs Nov 4, 2016

Member

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.

@instagibbs

instagibbs Nov 4, 2016

Member

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.

Show outdated Hide outdated src/wallet/crypter.cpp
std::vector<unsigned char> witnessProgram;
if (!GetCScript(scriptID, subscript) ||
!subscript.IsWitnessProgram(witnessVer, witnessProgram) ||
witnessProgram.size() != 20) {

This comment has been minimized.

@ryanofsky

ryanofsky Nov 3, 2016

Contributor

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

ryanofsky Nov 3, 2016

Contributor

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

Show outdated Hide outdated src/base58.cpp
if (!IsValid() || vchVersion != Params().Base58Prefix(CChainParams::SCRIPT_ADDRESS))
return false;
uint160 id;
memcpy(&id, &vchData[0], 20);

This comment has been minimized.

@ryanofsky

ryanofsky Nov 3, 2016

Contributor

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

@ryanofsky

ryanofsky Nov 3, 2016

Contributor

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

Show outdated Hide outdated src/wallet/rpcdump.cpp
@@ -539,11 +539,14 @@ UniValue dumpprivkey(const JSONRPCRequest& request)
string strAddress = request.params[0].get_str();
CBitcoinAddress address;
if (!address.SetString(strAddress))
if (!address.SetString(strAddress) || !address.IsValid())

This comment has been minimized.

@ryanofsky

ryanofsky Nov 3, 2016

Contributor

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

ryanofsky Nov 3, 2016

Contributor

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

Show outdated Hide outdated src/wallet/rpcwallet.cpp
@@ -1016,51 +1016,6 @@ UniValue addmultisigaddress(const JSONRPCRequest& request)
return CBitcoinAddress(innerID).ToString();
}
class Witnessifier : public boost::static_visitor<bool>

This comment has been minimized.

@ryanofsky

ryanofsky Nov 3, 2016

Contributor

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

@ryanofsky

ryanofsky Nov 3, 2016

Contributor

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

Show outdated Hide outdated src/wallet/wallet.h
@@ -986,4 +986,49 @@ class CAccount
}
};
class Witnessifier : public boost::static_visitor<bool>

This comment has been minimized.

@ryanofsky

ryanofsky Nov 3, 2016

Contributor

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.

@ryanofsky

ryanofsky Nov 3, 2016

Contributor

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.

This comment has been minimized.

@instagibbs

instagibbs Nov 4, 2016

Member

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

@instagibbs

instagibbs Nov 4, 2016

Member

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

Show outdated Hide outdated src/wallet/rpcdump.cpp
@@ -133,6 +133,14 @@ UniValue importprivkey(const JSONRPCRequest& request)
pwalletMain->MarkDirty();
pwalletMain->SetAddressBook(vchAddress, strLabel, "receive");
if (IsWitnessEnabled(chainActive.Tip(), Params().GetConsensus()) || GetBoolArg("-walletprematurewitness", false)) {
Witnessifier w;

This comment has been minimized.

@ryanofsky

ryanofsky Nov 3, 2016

Contributor

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.

@ryanofsky

ryanofsky Nov 3, 2016

Contributor

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.

This comment has been minimized.

@instagibbs

instagibbs Nov 4, 2016

Member

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.

@instagibbs

instagibbs Nov 4, 2016

Member

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.

Show outdated Hide outdated src/wallet/rpcdump.cpp
@@ -133,6 +133,14 @@ UniValue importprivkey(const JSONRPCRequest& request)
pwalletMain->MarkDirty();
pwalletMain->SetAddressBook(vchAddress, strLabel, "receive");
if (IsWitnessEnabled(chainActive.Tip(), Params().GetConsensus()) || GetBoolArg("-walletprematurewitness", false)) {

This comment has been minimized.

@ryanofsky

ryanofsky Nov 3, 2016

Contributor

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.

@ryanofsky

ryanofsky Nov 3, 2016

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@sipa

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

Member

sipa commented Nov 4, 2016

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

This comment has been minimized.

Show comment
Hide comment
@instagibbs

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

Member

instagibbs commented Nov 4, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 4, 2016

Member

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

Member

sipa commented Nov 4, 2016

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

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs

instagibbs Nov 4, 2016

Member

@sipa yes, I misspoke

Member

instagibbs commented Nov 4, 2016

@sipa yes, I misspoke

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs

instagibbs Nov 7, 2016

Member

Moved GetWitnessKeyID outside of Keystore and addressed some nits.

Member

instagibbs commented Nov 7, 2016

Moved GetWitnessKeyID outside of Keystore and addressed some nits.

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Jan 25, 2017

Member

Concept ACK

Member

jtimon commented Jan 25, 2017

Concept ACK

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

This comment has been minimized.

@luke-jr

luke-jr Feb 3, 2017

Member

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

@luke-jr

luke-jr Feb 3, 2017

Member

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

@@ -204,7 +212,8 @@ UniValue validateaddress(const JSONRPCRequest& request)
if (pwalletMain && pwalletMain->mapAddressBook.count(dest))
ret.push_back(Pair("account", pwalletMain->mapAddressBook[dest].name));
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())

This comment has been minimized.

@kallewoof

kallewoof Feb 28, 2017

Member

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

kallewoof Feb 28, 2017

Member

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.

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

This comment has been minimized.

@kallewoof

kallewoof Feb 28, 2017

Member

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

kallewoof Feb 28, 2017

Member

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.

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

This comment has been minimized.

@kallewoof

kallewoof Feb 28, 2017

Member

See previous comment regarding Get prefix.

@kallewoof

kallewoof Feb 28, 2017

Member

See previous comment regarding Get prefix.

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

This comment has been minimized.

@kallewoof

kallewoof Feb 28, 2017

Member

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

kallewoof Feb 28, 2017

Member

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 (!address.GetKeyID(keyID))
CScriptID scriptID;
if ((!address.GetScriptID(scriptID) || !GetWitnessKeyID(pwalletMain, scriptID, keyID)) &&
!address.GetKeyID(keyID)) {

This comment has been minimized.

@kallewoof

kallewoof Feb 28, 2017

Member

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

kallewoof Feb 28, 2017

Member

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)) {
@@ -520,7 +520,8 @@ UniValue signmessage(const JSONRPCRequest& request)
throw JSONRPCError(RPC_TYPE_ERROR, "Invalid address");
CKeyID keyID;
if (!addr.GetKeyID(keyID))
CScriptID scriptID;
if (!addr.GetKeyID(keyID) && (!addr.GetScriptID(scriptID) || !GetWitnessKeyID(pwalletMain, scriptID, keyID)))

This comment has been minimized.

@kallewoof

kallewoof Feb 28, 2017

Member
    if (!FetchWalletKeyID(pwalletMain, addr, keyID))
@kallewoof

kallewoof Feb 28, 2017

Member
    if (!FetchWalletKeyID(pwalletMain, addr, keyID))
@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs

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

Member

instagibbs commented Feb 28, 2017

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 instagibbs closed this Feb 28, 2017

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Aug 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.

Member

luke-jr commented Aug 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@instagibbs

instagibbs Aug 18, 2017

Member

Re-opening since segwit is actually activating.

Member

instagibbs commented Aug 18, 2017

Re-opening since segwit is actually activating.

@instagibbs instagibbs reopened this Aug 18, 2017

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs

instagibbs Aug 18, 2017

Member

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

Member

instagibbs commented Aug 18, 2017

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

@instagibbs instagibbs closed this Aug 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment