Skip to content
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.

Sign up for GitHub

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

Jump to bottom

wallet: Use CTxDestination in CRecipient instead of just scriptPubKey #28246

Merged
merged 4 commits into from
Sep 19, 2023
Merged

wallet: Use CTxDestination in CRecipient instead of just scriptPubKey #28246

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8dd0670
Make WitnessUnknown members private
achow101 Sep 5, 2023
1a98a51
Allow CNoDestination to represent a raw script
achow101 Aug 9, 2023
07d3bdf
Add PubKeyDestination for P2PK scripts
achow101 Aug 9, 2023
ad0c469
wallet: Use CTxDestination in CRecipient rather than scriptPubKey
achow101 Aug 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 25 additions & 6 deletions src/addresstype.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,12 @@ bool ExtractDestination(const CScript& scriptPubKey, CTxDestination& addressRet)
switch (whichType) {
case TxoutType::PUBKEY: {
CPubKey pubKey(vSolutions[0]);
if (!pubKey.IsValid())
return false;

addressRet = PKHash(pubKey);
return true;
if (!pubKey.IsValid()) {
addressRet = CNoDestination(scriptPubKey);
} else {
addressRet = PubKeyDestination(pubKey);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks incorrect. The PUBKEY code path now always returns "false". Is this intentional?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. See the first comment in this PR:

ExtractDestination's behavior is slightly changed for the above. It now returns true for those destinations that have addresses, so P2PK scripts now result in false. Even though it returns false for CNoDestination, the script will now be included in that CNoDestination.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why is P2PK treated differently now?
Previously when the function returned "false", it meant the result is CNoDestination, but now that's ambiguous and could also be PubKeyDestination, and thus requires further checks.
This kind of makes the return value not very useful, IMHO.

Copy link

@denavila denavila Oct 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, looks like OutputTypeFromDestination needs to be updated to support PubKeyDestination?
I'm hitting an issue in my PR, with some wallet tests that use addresses from mined blocks (GetScriptForRawPubKey(coinbaseKey.GetPubKey())), and OutputTypeFromDestination is returning nullopt now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously when the function returned "false", it meant the result is CNoDestination, but now that's ambiguous and could also be PubKeyDestination, and thus requires further checks.
This kind of makes the return value not very useful, IMHO.

As noted in the change to the documentation for this function, ExtractDestination only returns true for things that have addresses. P2PK does not have an address. This is useful in other parts of the codebase and in future work such as silent payments. If you want to check if it is a CNoDestination, then you can do that directly. However, it should generally be okay to not need to check for that. For the most part, checking for CNoDestination is not all that useful and there's probably a better way to achieve what you want to do.

Hmm, looks like OutputTypeFromDestination needs to be updated to support PubKeyDestination?
I'm hitting an issue in my PR, with some wallet tests that use addresses from mined blocks (GetScriptForRawPubKey(coinbaseKey.GetPubKey())), and OutputTypeFromDestination is returning nullopt now.

No, it does not. OutputType is also a synonym for address type in the context of giving an address to the user. P2PK does not have an address type and users cannot get an address for them. If you are using things that relied on the incorrect behavior of getting a PKHash for a P2PK script, then I suggest that you don't do that. P2PK is not PKHash and you shouldn't expect it to be giving you those scripts.

Copy link

@denavila denavila Oct 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I don't think I understand why it's considered to have no address ...
In the wallet test, I'm using GetScriptForRawPubKey(coinbaseKey.GetPubKey()) to mine a block (via the MineBlock function), and then create a wallet with that coinbase key and issue transactions with coins returned from AvailableCoins.
With the new code path, it seems the script coming from AvailableCoins gets classified as PubKeyDestination, which fails to get an OutputTypeFromDestination, however before this change, this code worked and generated successful transactions.
Is there another way to mine blocks and get non-P2PK script from the coinbase key?

For context, the code that used to work but is now failing is in the function CreateDeniabilizationTransaction, in spend.cpp in my PR (https://github.com/bitcoin/bitcoin/pull/27792/files#)

   // validate that all UTXOs share the same address
    std::optional<CTxDestination> op_shared_destination;
    for (const auto& coin : preset_inputs.coins) {
        CTxDestination destination = CNoDestination();
        ExtractDestination(coin->txout.scriptPubKey, destination);
        if (!std::get_if<CNoDestination>(&destination) && !op_shared_destination) {
            op_shared_destination = destination;
        }
        if (!op_shared_destination || !(*op_shared_destination == destination)) {
            return util::Error{_("Input addresses must all match.")};
        }
    }

And further down in the same function (with the workaround I just added to pass CI):

    std::optional<OutputType> op_output_type;
    if (std::get_if<PubKeyDestination>(&shared_destination)) {
        op_output_type = OutputType::LEGACY;
    } else {
        op_output_type = OutputTypeFromDestination(shared_destination);
    }
    if (!op_output_type) {
        op_output_type = wallet.m_default_change_type;
    }
    if (!op_output_type) {
        return util::Error{_("Unable to determine output type.")};
    }

Copy link
Member Author

@achow101 achow101 Oct 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I don't think I understand why it's considered to have no address

They literally do not have addresses. There is no address type which maps to a P2PK script. That this and other software sometimes display an address for P2PK scripts is simply wrong. They are displaying P2PK addresses. If you send to those addresses, a P2PK script is not created.

In the wallet test, I'm using GetScriptForRawPubKey(coinbaseKey.GetPubKey()) to mine a block (via the MineBlock function), and then create a wallet with that coinbase key and issue transactions with coins returned from AvailableCoins.
With the new code path, it seems the address coming from AvailableCoins gets classified as PubKeyDestination, which fails to get an OutputTypeFromDestination, however before this change, this code worked and generated successful transactions.

AvailableCoins does not operate on destinations, it operates on scripts. This is way more precise since there are more standard script types than there are standard addresses (e.g. P2PK, bare multisig). If you have modified it to operate on destinations, don't do that.

Please take this discussion to your PR as it would be more helpful to see what you've done rather than to continue discussing code proposed in another PR in this thread. Just leave a review comment on the relevant line in your PR and mention me.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! It's clear I didn't have a good understanding of the relationship between scripts and addresses. I'll move the discussion to my PR.

return false;
}
case TxoutType::PUBKEYHASH: {
addressRet = PKHash(uint160(vSolutions[0]));
Expand Down Expand Up @@ -108,6 +109,11 @@ class CScriptVisitor
return dest.GetScript();
}

CScript operator()(const PubKeyDestination& dest) const
{
return CScript() << ToByteVector(dest.GetPubKey()) << OP_CHECKSIG;
achow101 marked this conversation as resolved.
Show resolved Hide resolved
}

CScript operator()(const PKHash& keyID) const
{
return CScript() << OP_DUP << OP_HASH160 << ToByteVector(keyID) << OP_EQUALVERIFY << OP_CHECKSIG;
Expand Down Expand Up @@ -138,6 +144,19 @@ class CScriptVisitor
return CScript() << CScript::EncodeOP_N(id.GetWitnessVersion()) << id.GetWitnessProgram();
}
};

class ValidDestinationVisitor
{
public:
bool operator()(const CNoDestination& dest) const { return false; }
bool operator()(const PubKeyDestination& dest) const { return false; }
bool operator()(const PKHash& dest) const { return true; }
bool operator()(const ScriptHash& dest) const { return true; }
bool operator()(const WitnessV0KeyHash& dest) const { return true; }
bool operator()(const WitnessV0ScriptHash& dest) const { return true; }
bool operator()(const WitnessV1Taproot& dest) const { return true; }
bool operator()(const WitnessUnknown& dest) const { return true; }
};
} // namespace

CScript GetScriptForDestination(const CTxDestination& dest)
Expand All @@ -146,5 +165,5 @@ CScript GetScriptForDestination(const CTxDestination& dest)
}

bool IsValidDestination(const CTxDestination& dest) {
return dest.index() != 0;
return std::visit(ValidDestinationVisitor(), dest);
}
22 changes: 18 additions & 4 deletions src/addresstype.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,19 @@ class CNoDestination {
friend bool operator<(const CNoDestination& a, const CNoDestination& b) { return a.GetScript() < b.GetScript(); }
};

struct PubKeyDestination {
private:
CPubKey m_pubkey;

public:
PubKeyDestination(const CPubKey& pubkey) : m_pubkey(pubkey) {}
achow101 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it is fine to let this be implicit. There should be no risk.


const CPubKey& GetPubKey() const LIFETIMEBOUND { return m_pubkey; }

friend bool operator==(const PubKeyDestination& a, const PubKeyDestination& b) { return a.GetPubKey() == b.GetPubKey(); }
friend bool operator<(const PubKeyDestination& a, const PubKeyDestination& b) { return a.GetPubKey() < b.GetPubKey(); }
};

struct PKHash : public BaseHash<uint160>
{
PKHash() : BaseHash() {}
Expand Down Expand Up @@ -103,6 +116,7 @@ struct WitnessUnknown
/**
* A txout script categorized into standard templates.
* * CNoDestination: Optionally a script, no corresponding address.
* * PubKeyDestination: TxoutType::PUBKEY (P2PK), no corresponding address
* * PKHash: TxoutType::PUBKEYHASH destination (P2PKH address)
* * ScriptHash: TxoutType::SCRIPTHASH destination (P2SH address)
* * WitnessV0ScriptHash: TxoutType::WITNESS_V0_SCRIPTHASH destination (P2WSH address)
Expand All @@ -111,9 +125,9 @@ struct WitnessUnknown
* * WitnessUnknown: TxoutType::WITNESS_UNKNOWN destination (P2W??? address)
* A CTxDestination is the internal data type encoded in a bitcoin address
*/
using CTxDestination = std::variant<CNoDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, WitnessUnknown>;
using CTxDestination = std::variant<CNoDestination, PubKeyDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, WitnessUnknown>;

/** Check whether a CTxDestination is a CNoDestination. */
/** Check whether a CTxDestination corresponds to one with an address. */
achow101 marked this conversation as resolved.
Show resolved Hide resolved
bool IsValidDestination(const CTxDestination& dest);

/**
Expand All @@ -123,8 +137,8 @@ bool IsValidDestination(const CTxDestination& dest);
* is assigned to addressRet.
* For all other scripts. addressRet is assigned as a CNoDestination containing the scriptPubKey.
achow101 marked this conversation as resolved.
Show resolved Hide resolved
*
* Returns true for standard destinations - P2PK, P2PKH, P2SH, P2WPKH, P2WSH, and P2TR scripts.
* Returns false for non-standard destinations - P2PK with invalid pubkeys, P2W???, bare multisig, null data, and nonstandard scripts.
* Returns true for standard destinations with addresses - P2PKH, P2SH, P2WPKH, P2WSH, P2TR and P2W??? scripts.
* Returns false for non-standard destinations and those without addresses - P2PK, bare multisig, null data, and nonstandard scripts.
*/
bool ExtractDestination(const CScript& scriptPubKey, CTxDestination& addressRet);

Expand Down
1 change: 1 addition & 0 deletions src/key_io.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ class DestinationEncoder
}

std::string operator()(const CNoDestination& no) const { return {}; }
std::string operator()(const PubKeyDestination& pk) const { return {}; }
};

CTxDestination DecodeDestination(const std::string& str, const CChainParams& params, std::string& error_str, std::vector<int>* error_locations)
Expand Down
5 changes: 5 additions & 0 deletions src/rpc/output_script.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,11 @@ static RPCHelpMan deriveaddresses()
for (const CScript& script : scripts) {
CTxDestination dest;
if (!ExtractDestination(script, dest)) {
// ExtractDestination no longer returns true for P2PK since it doesn't have a corresponding address
// However combo will output P2PK and should just ignore that script
if (scripts.size() > 1 && std::get_if<PubKeyDestination>(&dest)) {
continue;
}
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Descriptor does not have a corresponding address");
}

Expand Down
5 changes: 5 additions & 0 deletions src/rpc/util.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,11 @@ class DescribeAddressVisitor
return UniValue(UniValue::VOBJ);
}

UniValue operator()(const PubKeyDestination& dest) const
{
return UniValue(UniValue::VOBJ);
}

UniValue operator()(const PKHash& keyID) const
{
UniValue obj(UniValue::VOBJ);
Expand Down
9 changes: 6 additions & 3 deletions src/test/fuzz/script.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,13 +149,16 @@ FUZZ_TARGET(script, .init = initialize_script)
const CTxDestination tx_destination_2{ConsumeTxDestination(fuzzed_data_provider)};
const std::string encoded_dest{EncodeDestination(tx_destination_1)};
const UniValue json_dest{DescribeAddress(tx_destination_1)};
Assert(tx_destination_1 == DecodeDestination(encoded_dest));
(void)GetKeyForDestination(/*store=*/{}, tx_destination_1);
const CScript dest{GetScriptForDestination(tx_destination_1)};
const bool valid{IsValidDestination(tx_destination_1)};
Assert(dest.empty() != valid);

Assert(valid == IsValidDestinationString(encoded_dest));
if (!std::get_if<PubKeyDestination>(&tx_destination_1)) {
// Only try to round trip non-pubkey destinations since PubKeyDestination has no encoding
Assert(dest.empty() != valid);
Assert(tx_destination_1 == DecodeDestination(encoded_dest));
Assert(valid == IsValidDestinationString(encoded_dest));
}

(void)(tx_destination_1 < tx_destination_2);
if (tx_destination_1 == tx_destination_2) {
Expand Down
9 changes: 9 additions & 0 deletions src/test/fuzz/util.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,15 @@ CTxDestination ConsumeTxDestination(FuzzedDataProvider& fuzzed_data_provider) no
[&] {
tx_destination = CNoDestination{};
},
[&] {
bool compressed = fuzzed_data_provider.ConsumeBool();
CPubKey pk{ConstructPubKeyBytes(
fuzzed_data_provider,
ConsumeFixedLengthByteVector(fuzzed_data_provider, (compressed ? CPubKey::COMPRESSED_SIZE : CPubKey::SIZE)),
compressed
)};
tx_destination = PubKeyDestination{pk};
},
[&] {
tx_destination = PKHash{ConsumeUInt160(fuzzed_data_provider)};
},
Expand Down
4 changes: 2 additions & 2 deletions src/test/script_standard_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,8 +203,8 @@ BOOST_AUTO_TEST_CASE(script_standard_ExtractDestination)
// TxoutType::PUBKEY
s.clear();
s << ToByteVector(pubkey) << OP_CHECKSIG;
BOOST_CHECK(ExtractDestination(s, address));
BOOST_CHECK(std::get<PKHash>(address) == PKHash(pubkey));
BOOST_CHECK(!ExtractDestination(s, address));
BOOST_CHECK(std::get<PubKeyDestination>(address) == PubKeyDestination(pubkey));

// TxoutType::PUBKEYHASH
s.clear();
Expand Down
1 change: 1 addition & 0 deletions src/wallet/rpc/addresses.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,7 @@ class DescribeWalletAddressVisitor
explicit DescribeWalletAddressVisitor(const SigningProvider* _provider) : provider(_provider) {}

UniValue operator()(const CNoDestination& dest) const { return UniValue(UniValue::VOBJ); }
UniValue operator()(const PubKeyDestination& dest) const { return UniValue(UniValue::VOBJ); }

UniValue operator()(const PKHash& pkhash) const
{
Expand Down
11 changes: 9 additions & 2 deletions src/wallet/spend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,8 +490,15 @@ std::map<CTxDestination, std::vector<COutput>> ListCoins(const CWallet& wallet)
coins_params.skip_locked = false;
for (const COutput& coin : AvailableCoins(wallet, &coin_control, /*feerate=*/std::nullopt, coins_params).All()) {
CTxDestination address;
if ((coin.spendable || (wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && coin.solvable)) &&
ExtractDestination(FindNonChangeParentOutput(wallet, coin.outpoint).scriptPubKey, address)) {
if ((coin.spendable || (wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && coin.solvable))) {
if (!ExtractDestination(FindNonChangeParentOutput(wallet, coin.outpoint).scriptPubKey, address)) {
achow101 marked this conversation as resolved.
Show resolved Hide resolved
// For backwards compatibility, we convert P2PK output scripts into PKHash destinations
if (auto pk_dest = std::get_if<PubKeyDestination>(&address)) {
address = PKHash(pk_dest->GetPubKey());
} else {
continue;
}
}
result[address].emplace_back(coin);
}
}
Expand Down
5 changes: 4 additions & 1 deletion test/functional/rpc_deriveaddresses.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,10 @@ def run_test(self):
assert_raises_rpc_error(-8, "Range should be greater or equal than 0", self.nodes[0].deriveaddresses, descsum_create("wpkh(tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/1/1/*)"), [-1, 0])

combo_descriptor = descsum_create("combo(tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/1/1/0)")
assert_equal(self.nodes[0].deriveaddresses(combo_descriptor), ["mtfUoUax9L4tzXARpw1oTGxWyoogp52KhJ", "mtfUoUax9L4tzXARpw1oTGxWyoogp52KhJ", address, "2NDvEwGfpEqJWfybzpKPHF2XH3jwoQV3D7x"])
assert_equal(self.nodes[0].deriveaddresses(combo_descriptor), ["mtfUoUax9L4tzXARpw1oTGxWyoogp52KhJ", address, "2NDvEwGfpEqJWfybzpKPHF2XH3jwoQV3D7x"])

# P2PK does not have a valid address
assert_raises_rpc_error(-5, "Descriptor does not have a corresponding address", self.nodes[0].deriveaddresses, descsum_create("pk(tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK)"))

# Before #26275, bitcoind would crash when deriveaddresses was
# called with derivation index 2147483647, which is the maximum
Expand Down