wallet: Use CTxDestination in CRecipient instead of just scriptPubKey

achow101 commented at 3:16 pm on August 9, 2023: member

For silent payments, we want to provide a SilentPaymentsDestination to be used as the recipient, which requires CRecipient to use something other than just the scriptPubKey as we cannot know the output script for a silent payment prior to transaction creation. CTxDestination seems like the obvious place to add a SilentPaymentsDestination as it is our internal representation of an address.

In order to still allow paying to arbitrary scriptPubKeys (e.g. for data carrier outputs, or the user hand crafted a raw transaction that they have given to fundrawtransaction), CNoDestination is changed to contain raw scripts.

Additionally, P2PK scripts are now interpreted as a new PubKeyDestination rather than PKHash. This results in some things that would have given an address for P2PK scripts to no longer do so. This is arguably more correct.

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.

Builds on #28244

DrahtBot commented at 3:16 pm on August 9, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	josibake
Concept ACK	instagibbs, ismaelsadeeq
Stale ACK	S3RK

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28453 (wallet: Receive silent payment transactions by achow101)
#28202 (Silent Payments: receiving by josibake)
#28201 (Silent Payments: sending by josibake)
#28122 (Silent Payments: Implement BIP352 by josibake)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
#27827 (Silent Payments: send and receive by josibake)
#27260 (Enhanced error messages for invalid network prefix during address parsing. by russeree)
#25273 (wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction by achow101)
#22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Wallet on Aug 9, 2023

bitcoin deleted a comment on Aug 9, 2023

DrahtBot added the label CI failed on Aug 9, 2023

in src/addresstype.h:30 in 34d7ab05b3 outdated

22@@ -23,6 +23,15 @@ class CNoDestination {
23     friend bool operator<(const CNoDestination &a, const CNoDestination &b) { return a.script < b.script; }
24 };
25 
26+struct PubKeyDestination

josibake commented at 7:08 pm on August 13, 2023:

in https://github.com/bitcoin/bitcoin/pull/28246/commits/34d7ab05b3f30e2827d9358a212f25865a71cce8:

Looks like this is causing the fuzzer to fail in src/test/fuzz/util.cpp because this new type is not handled.

achow101 commented at 10:06 pm on August 14, 2023:

Should be fixed.

achow101 force-pushed on Aug 13, 2023

achow101 force-pushed on Aug 14, 2023

DrahtBot removed the label CI failed on Aug 15, 2023

achow101 force-pushed on Aug 18, 2023

achow101 marked this as ready for review on Aug 18, 2023

in src/addresstype.cpp:97 in 4684adcbde outdated

93@@ -94,6 +94,7 @@ bool ExtractDestination(const CScript& scriptPubKey, CTxDestination& addressRet)
94     case TxoutType::MULTISIG:
95     case TxoutType::NULL_DATA:
96     case TxoutType::NONSTANDARD:
97+        addressRet = CNoDestination(scriptPubKey);

S3RK commented at 7:25 am on August 21, 2023:

Comment for ExtractDestination function needs to be updated. Comment also should explain what is the return value and when addressRet is populated.

achow101 commented at 4:04 pm on August 22, 2023:

Updated the comments

in src/rpc/output_script.cpp:283 in 5727940dbd outdated

279@@ -280,7 +280,10 @@ static RPCHelpMan deriveaddresses()
280                 for (const CScript& script : scripts) {
281                     CTxDestination dest;
282                     if (!ExtractDestination(script, dest)) {
283-                        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Descriptor does not have a corresponding address");
284+                        if (!std::get_if<PubKeyDestination>(&dest)) {

S3RK commented at 7:37 am on August 21, 2023:

Why do we need to special case P2PK here?

S3RK commented at 7:47 am on August 21, 2023:

oh, I see. It’s because combo() descriptors generate P2PK.. ugh..

achow101 commented at 4:04 pm on August 22, 2023:

Added a comment. Also noticed a minor bug with deriving a pk() descriptor, so added a fix and test for that.

in src/wallet/spend.cpp:494 in 5727940dbd outdated

390@@ -391,8 +391,14 @@ std::map<CTxDestination, std::vector<COutput>> ListCoins(const CWallet& wallet)
391     coins_params.skip_locked = false;
392     for (const COutput& coin : AvailableCoins(wallet, &coin_control, /*feerate=*/std::nullopt, coins_params).All()) {
393         CTxDestination address;
394-        if ((coin.spendable || (wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && coin.solvable)) &&
395-            ExtractDestination(FindNonChangeParentOutput(wallet, coin.outpoint).scriptPubKey, address)) {
396+        if ((coin.spendable || (wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && coin.solvable))) {
397+            if (!ExtractDestination(FindNonChangeParentOutput(wallet, coin.outpoint).scriptPubKey, address)) {

S3RK commented at 7:45 am on August 21, 2023:

Maybe add a comment that this is done for backward compatibility?

achow101 commented at 4:04 pm on August 22, 2023:

Done

S3RK commented at 8:01 am on August 21, 2023: contributor

CodeReview ACK b2a0586cae3650975f8f1a2048cdad4e0274a9d1 with some nits

Sjors commented at 12:22 pm on August 21, 2023: member

Can we add CRawDestination and keep CNoDestination for where we couldn’t parse the destination? This seems safer, e.g. AddressTableModel::setData (GUI) checks for this.

Builds on #28244

That was merged.

The first commit triggers a maybe-uninitialized warning for me (Ubuntu 23.10 with gcc 12.3.0):

0/prevector.h:170:37: error: ‘*(const prevector<28, unsigned char, unsigned int, int>*)((char*)&<unnamed> + offsetof(std::CTxDestination, std::variant<CNoDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, WitnessUnknown>::<unnamed>.std::__detail::__variant::_Variant_base<CNoDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, WitnessUnknown>::<unnamed>.std::__detail::__variant::_Move_assign_base<false, CNoDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, WitnessUnknown>::<unnamed>.std::__detail::__variant::_Copy_assign_base<false, CNoDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, WitnessUnknown>::<unnamed>.std::__detail::__variant::_Move_ctor_base<false, CNoDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, WitnessUnknown>::<unnamed>.std::__detail::__variant::_Copy_ctor_base<false, CNoDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, WitnessUnknown>::<unnamed>.std::__detail::__variant::_Variant_storage<false, CNoDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, WitnessUnknown>::_M_u)).prevector<28, unsigned char>::_size’ may be used uninitialized [-Werror=maybe-uninitialized]
1  170 |     bool is_direct() const { return _size <= N; }
2      |                                     ^~~~~
3util/message.cpp: In function ‘MessageVerificationResult MessageVerify(const std::string&, const std::string&, const std::string&)’:
4util/message.cpp:50:40: note: ‘<anonymous>’ declared here
5   50 |     if (!(CTxDestination(PKHash(pubkey)) == destination)) {

achow101 commented at 3:43 pm on August 22, 2023: member

Can we add CRawDestination and keep CNoDestination for where we couldn’t parse the destination?

That isn’t a useful distinction. We can’t parse the destination when it’s just a random script, and a random script would be classified as a raw destination. Decoding into a raw script is also fundamentally impossible

achow101 force-pushed on Aug 22, 2023

Sjors commented at 5:42 pm on August 22, 2023: member

We can’t parse the destination when it’s just a random script

But a random script is distinct from an invalid encoding. For the UI that different matters. (though maybe doesn’t require a different CDestination thing)

a random script would be classified as a raw destination

That sounds correct to me?

Decoding into a raw script is also fundamentally impossible

How do you mean?

achow101 commented at 6:56 pm on August 22, 2023: member

We can’t parse the destination when it’s just a random script

But a random script is distinct from an invalid encoding. For the UI that different matters. (though maybe doesn’t require a different CDestination thing)

The UI is still getting a CNoDestination anyways. It’s only decoding addresses (or things purporting to be addresses) which cannot be arbitrary scripts.

Decoding into a raw script is also fundamentally impossible

How do you mean?

There’s no address type that maps to arbitrary scripts. It’s literally impossible for DecodeDestination to return a CNoDestination that contains an arbitrary script since decoding fails first.

This PR makes it so that a CNoDestination continues to mean that when returned from DecodeDestination, there was failure to decode, and when returned from ExtractDestination, the provided script does not match any standard template. I think what you’re proposing is to essentially break that up, but in that case, CNoDestination is basically just a boolean that would be better served as a util::Result as it has no meaning outside of that context.

Sjors commented at 7:15 pm on August 22, 2023: member

I’m thinking we might have a future where someone - who knows what they’re doing - can paste in an arbitrary scriptPubKey in hex format. At least in the RPC, and maybe also in the GUI. E.g. because they want to send to a bare multisig. Or to some far future soft fork script that’s not following the bc1[version, rest of script] segwit pattern (and for some reason they don’t want to upgrade the wallet). This could then be stored as a CRawDestination.

It’s literally impossible for DecodeDestination to return a CNoDestination that contains an arbitrary script since decoding fails first.

I think what you’re proposing is to essentially break that up

I think I’m proposing a change to DecodeDestination so that it returns CRawDestination if it’s a scriptPubKey and CNoDestination if it’s really unparseable.

Sjors commented at 7:21 pm on August 22, 2023: member

That said, since we don’t need CRawDestination for Silent Payments, maybe it’s a bit overkill and we can introduce something like CRawDestination later.

achow101 commented at 7:25 pm on August 22, 2023: member

I think I’m proposing a change to DecodeDestination so that it returns CRawDestination if it’s a scriptPubKey and CNoDestination if it’s really unparseable.

That’s way out of scope for this PR.

in src/addresstype.h:139 in e5143d0a04 outdated

124@@ -115,8 +125,8 @@ bool IsValidDestination(const CTxDestination& dest);
125  * is assigned to addressRet.
126  * For all other scripts. addressRet is assigned as a CNoDestination containing the scriptPubKey.
127  *
128- * Returns true for standard destinations - P2PK, P2PKH, P2SH, P2WPKH, P2WSH, and P2TR scripts.
129- * Returns false for non-standard destinations - P2PK with invalid pubkeys, P2W???, bare multisig, null data, and nonstandard scripts.
130+ * Returns true for standard destinations - P2PKH, P2SH, P2WPKH, P2WSH, and P2TR scripts.

murchandamus commented at 9:05 pm on August 24, 2023:

From what I understand now, this refers to output scripts with address standards, but when I first saw it, I thought it was referring to outputs permitted in standard transactions.

0 * Returns true for destinations with address standards - P2PKH, P2SH, P2WPKH, P2WSH, and P2TR scripts.

josibake commented at 8:20 am on August 29, 2023: member

Concept ACK

In #28122 , I also need to update fuzz/util.cpp to handle the new V0SilentPaymentsDestination type. I realised we are now duplicating code in three places to create pubkeys for the fuzzer, so I pulled your code for creating compressed/uncompressed pubkeys into a function here: #28361.

Can simplify this PR a bit by rebasing on top of #28361 (assuming you agree with the approach)

josibake commented at 8:18 am on August 30, 2023: member

Can we add CRawDestination and keep CNoDestination for where we couldn’t parse the destination? This seems safer, e.g. AddressTableModel::setData (GUI) checks for this.

Builds on #28244

That was merged.

The first commit triggers a maybe-uninitialized warning for me (Ubuntu 23.10 with gcc 12.3.0):

0/prevector.h:170:37: error: ‘*(const prevector<28, unsigned char, unsigned int, int>*)((char*)&<unnamed> + offsetof(std::CTxDestination, std::variant<CNoDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, WitnessUnknown>::<unnamed>.std::__detail::__variant::_Variant_base<CNoDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, WitnessUnknown>::<unnamed>.std::__detail::__variant::_Move_assign_base<false, CNoDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, WitnessUnknown>::<unnamed>.std::__detail::__variant::_Copy_assign_base<false, CNoDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, WitnessUnknown>::<unnamed>.std::__detail::__variant::_Move_ctor_base<false, CNoDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, WitnessUnknown>::<unnamed>.std::__detail::__variant::_Copy_ctor_base<false, CNoDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, WitnessUnknown>::<unnamed>.std::__detail::__variant::_Variant_storage<false, CNoDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, WitnessUnknown>::_M_u)).prevector<28, unsigned char>::_size’ may be used uninitialized [-Werror=maybe-uninitialized]
1  170 |     bool is_direct() const { return _size <= N; }
2      |                                     ^~~~~
3util/message.cpp: In function ‘MessageVerificationResult MessageVerify(const std::string&, const std::string&, const std::string&)’:
4util/message.cpp:50:40: note: ‘<anonymous>’ declared here
5   50 |     if (!(CTxDestination(PKHash(pubkey)) == destination)) {

Seeing this as well on the latest push

achow101 commented at 3:48 pm on August 30, 2023: member

The first commit triggers a maybe-uninitialized warning for me (Ubuntu 23.10 with gcc 12.3.0):

Unable to get this warning at all even with gcc 12.3.0

josibake commented at 4:25 pm on August 30, 2023: member

Unable to get this warning at all even with gcc 12.3.0

Could be a false positive? But not sure how to confirm that

maflcko commented at 4:28 pm on August 30, 2023: member

Happy to take a look, if there are exact steps to reproduce. Ideally starting from a fresh install of the operating system.

instagibbs commented at 2:22 pm on August 31, 2023: member

concept ACK, makes sense to ferry additional info around and use CTxDestination in wallet contexts like this

DrahtBot added the label CI failed on Sep 4, 2023

DrahtBot removed the label CI failed on Sep 4, 2023

in src/addresstype.h:23 in e30d40b356 outdated

20+    CScript script;
21+
22+    CNoDestination() = default;
23+    CNoDestination(const CScript& script) : script(script) {}
24+    friend bool operator==(const CNoDestination &a, const CNoDestination &b) { return a.script == b.script; }
25+    friend bool operator<(const CNoDestination &a, const CNoDestination &b) { return a.script < b.script; }

maflcko commented at 11:11 am on September 5, 2023:

in e30d40b356a4bec0fc7e64b746d1f31a927e01ba: Seems fragile to make script public and mutable. How is this going to interact when it is added to a container that uses operator< to sort elements, and the script is changed after calling operator<?

It would be good to make it const.

Also, could run clang-format on this new code?

achow101 commented at 4:42 pm on September 5, 2023:

Made private. Also done for WitnessUnknown and PubKeyDestination. The members can’t be made const since std::variant needs them to be assignable.

in src/addresstype.h:32 in e5143d0a04 outdated

27+{
28+    CPubKey pubkey;
29+
30+    PubKeyDestination(const CPubKey& pubkey) : pubkey(pubkey) {}
31+    friend bool operator==(const PubKeyDestination &a, const PubKeyDestination &b) { return a.pubkey == b.pubkey; }
32+    friend bool operator<(const PubKeyDestination &a, const PubKeyDestination &b) { return a.pubkey < b.pubkey; }

maflcko commented at 11:15 am on September 5, 2023:

Same? Also, unrelated, for WitnessUnknown?

achow101 force-pushed on Sep 5, 2023

in src/addresstype.h:80 in e3d6bd4162 outdated

78+
79+public:
80+    WitnessUnknown(unsigned int version, std::vector<unsigned char> program) : m_version(version), m_program(program) {}
81+
82+    unsigned int GetWitnessVersion() const { return m_version; }
83+    const std::vector<unsigned char>& GetWitnessProgram() const { return m_program; }

maflcko commented at 6:51 am on September 6, 2023:

nit in https://github.com/bitcoin/bitcoin/commit/e3d6bd41629a0735d7fb1475b77ffd3be7280c9a: LIFETIMEBOUND?

achow101 commented at 7:40 pm on September 7, 2023:

Done

in src/rpc/util.cpp:306 in e3d6bd4162 outdated

302@@ -303,8 +303,8 @@ class DescribeAddressVisitor
303     {
304         UniValue obj(UniValue::VOBJ);
305         obj.pushKV("iswitness", true);
306-        obj.pushKV("witness_version", (int)id.version);
307-        obj.pushKV("witness_program", HexStr({id.program, id.length}));
308+        obj.pushKV("witness_version", (int)id.GetWitnessVersion());

maflcko commented at 6:52 am on September 6, 2023:

nit in https://github.com/bitcoin/bitcoin/commit/e3d6bd41629a0735d7fb1475b77ffd3be7280c9a: I think the (int) is no longer required to compile with univalue on current master and can be dropped while touching this line?

achow101 commented at 7:40 pm on September 7, 2023:

Done

in src/test/fuzz/util.cpp:200 in e3d6bd4162 outdated

178@@ -179,15 +179,11 @@ CTxDestination ConsumeTxDestination(FuzzedDataProvider& fuzzed_data_provider) no
179             tx_destination = WitnessV1Taproot{XOnlyPubKey{ConsumeUInt256(fuzzed_data_provider)}};
180         },
181         [&] {
182-            WitnessUnknown witness_unknown{};
183-            witness_unknown.version = fuzzed_data_provider.ConsumeIntegralInRange(2, 16);
184-            std::vector<uint8_t> witness_unknown_program_1{fuzzed_data_provider.ConsumeBytes<uint8_t>(40)};
185-            if (witness_unknown_program_1.size() < 2) {
186-                witness_unknown_program_1 = {0, 0};
187+            std::vector<unsigned char> program{ConsumeRandomLengthByteVector(fuzzed_data_provider, /*max_length=*/40)};

maflcko commented at 6:53 am on September 6, 2023:

note in https://github.com/bitcoin/bitcoin/commit/e3d6bd41629a0735d7fb1475b77ffd3be7280c9a: This changes the fuzz input format, but I agree with the change, as it should allow the fuzz engine to produce programs of size less than 40 even when the input buffer is not yet exhausted.

in src/test/script_standard_tests.cpp:252 in e3d6bd4162 outdated

248@@ -249,10 +249,7 @@ BOOST_AUTO_TEST_CASE(script_standard_ExtractDestination)
249     s.clear();
250     s << OP_1 << ToByteVector(pubkey);
251     BOOST_CHECK(ExtractDestination(s, address));
252-    WitnessUnknown unk;
253-    unk.length = 33;
254-    unk.version = 1;
255-    std::copy(pubkey.begin(), pubkey.end(), unk.program);
256+    WitnessUnknown unk(1, ToByteVector(pubkey));

maflcko commented at 6:54 am on September 6, 2023:

nit in https://github.com/bitcoin/bitcoin/commit/e3d6bd41629a0735d7fb1475b77ffd3be7280c9a: Could use non-narrowing WitnessUnknonwn{a,b} constructor for new code? Here, and everywhere else in the commit?

achow101 commented at 7:40 pm on September 7, 2023:

Done

in src/addresstype.h:77 in e3d6bd4162 outdated

75+private:
76+    unsigned int m_version;
77+    std::vector<unsigned char> m_program;
78+
79+public:
80+    WitnessUnknown(unsigned int version, std::vector<unsigned char> program) : m_version(version), m_program(program) {}

maflcko commented at 7:01 am on September 6, 2023:

nit in https://github.com/bitcoin/bitcoin/commit/e3d6bd41629a0735d7fb1475b77ffd3be7280c9a: Could std::move program to m_program?

achow101 commented at 7:40 pm on September 7, 2023:

Done

in src/addresstype.h:40 in 248f881c5b outdated

35+public:
36+    PubKeyDestination(const CPubKey& pubkey) : m_pubkey(pubkey) {}
37+
38+    const CPubKey& GetPubKey() const { return m_pubkey; }
39+
40+    friend bool operator==(const PubKeyDestination &a, const PubKeyDestination &b) { return a.GetPubKey() == b.GetPubKey(); }

maflcko commented at 7:05 am on September 6, 2023:

style nits in 248f881c5b86511676f16d787f46df00c75916c4:

LIFETIMEBOUND for the getter?
clang-format for the new code?

achow101 commented at 7:40 pm on September 7, 2023:

Done

maflcko commented at 7:06 am on September 6, 2023: member

left some style nits, nothing blocking, feel free to ignore

fanquake referenced this in commit 238d29aff9 on Sep 7, 2023

in src/addresstype.h:74 in e3d6bd4162 outdated

68@@ -69,22 +69,25 @@ struct WitnessV1Taproot : public XOnlyPubKey
69 //! CTxDestination subtype to encode any future Witness version
70 struct WitnessUnknown
71 {
72-    unsigned int version;
73-    unsigned int length;
74-    unsigned char program[40];

josibake commented at 4:41 pm on September 7, 2023:

AFAICT, changing this to std::vector was causing the -Wuninitialized warning. I played around with overriding the default constructor but kept getting the same error. What was causing the error was this line:

https://github.com/bitcoin/bitcoin/blob/e3d6bd41629a0735d7fb1475b77ffd3be7280c9a/src/util/message.cpp#L50

where we temporarily create a CTxDestination(PKHash(pubkey)) to compare it to a CTxDestination. From what I can tell, creating the temporary CTxDestination with only a PKHash in it and then immediately calling the destructor was triggering the error for WitnessUnknown since it was never really constructed. Either way, I think it makes more sense to just create a PKHash and then compare it to the CTxDestination with a std::get_if like so:

0-    if (!(CTxDestination(PKHash(pubkey)) == destination)) {
1+    if (!(PKHash(pubkey) == *std::get_if<PKHash>(&destination))) {

I made this change in https://github.com/bitcoin/bitcoin/commit/50fd2a5455288a87249ef5ed73f7db34f814bf8a and verified that everything compiles without the warning, feel free to cherry-pick it

achow101 commented at 7:40 pm on September 7, 2023:

Done as suggested.

josibake commented at 5:49 am on September 12, 2023:

in https://github.com/bitcoin/bitcoin/pull/28246/commits/c14ad74f8eeaa744d831a77654164c856a86fdee:

I’m getting the -Wunitialized error again, due to this line:

https://github.com/bitcoin/bitcoin/blob/c14ad74f8eeaa744d831a77654164c856a86fdee/src/test/fuzz/key.cpp#L189

IIRC, this was fixed in a previous version of this PR, not sure when it changed, but this should be:

0-        assert(CTxDestination{PKHash{pubkey}} == tx_destination);
1+        assert(PKHash{pubkey} == *std::get_if<PKHash>(&tx_destination));

achow101 commented at 4:14 pm on September 12, 2023:

Fixed in fuzz/key.cpp

in src/test/fuzz/util.cpp:183 in 248f881c5b outdated

167+            uint8_t pk_type = fuzzed_data_provider.PickValueInArray({0x02, 0x03, 0x04, 0x06, 0x07});
168+            std::vector<uint8_t> pk_data = ConsumeFixedLengthByteVector(fuzzed_data_provider, pk_type >= 0x04 ? 65 : 33);
169+            pk_data[0] = pk_type;
170+            CPubKey pk{pk_data.begin(), pk_data.end()};
171+            tx_destination = PubKeyDestination{pk};
172+        },

josibake commented at 4:48 pm on September 7, 2023:

#28361 merged, so this can become:

0        [&] {
1            bool compressed = fuzzed_data_provider.ConsumeBool();
2            CPubKey pk{ConstructPubKeyBytes(
3                    fuzzed_data_provider,
4                    ConsumeFixedLengthByteVector(fuzzed_data_provider, (compressed ? CPubKey::COMPRESSED_SIZE : CPubKey::SIZE)),
5                    compressed
6            )};
7            tx_destination = PubKeyDestination{pk};
8        },

achow101 commented at 7:40 pm on September 7, 2023:

Done

achow101 force-pushed on Sep 7, 2023

in src/addresstype.h:22 in 102fc00a18 outdated

19+
20 public:
21-    friend bool operator==(const CNoDestination &a, const CNoDestination &b) { return true; }
22-    friend bool operator<(const CNoDestination &a, const CNoDestination &b) { return true; }
23+    CNoDestination() = default;
24+    CNoDestination(const CScript& script) : m_script(script) {}

josibake commented at 11:27 am on September 8, 2023:

in https://github.com/bitcoin/bitcoin/pull/28246/commits/102fc00a1809f8786c03a0359ad2eb689e7e40dc:

Shouldn’t this also be “pass by value and move”? In the previous commit, WitnessUnknown was changed to use a pass by value and move constructor. IIUC, we always want to make a copy when creating a WitnessUnknown or CNoDestination object from an existing variable (lvalue), but it would also be nice to avoid the copy if creating either of these objects from a temporary value (rvalue).

0    CNoDestination(CScript script) : m_script(std::move(script)) {}

achow101 commented at 4:03 pm on September 11, 2023:

No, I think WitnessUnknown should actually be using a reference rather than value. AFAIK, both are equally efficient.

in src/addresstype.h:118 in 102fc00a18 outdated

107- *  * WitnessV0ScriptHash: TxoutType::WITNESS_V0_SCRIPTHASH destination (P2WSH)
108- *  * WitnessV0KeyHash: TxoutType::WITNESS_V0_KEYHASH destination (P2WPKH)
109- *  * WitnessV1Taproot: TxoutType::WITNESS_V1_TAPROOT destination (P2TR)
110- *  * WitnessUnknown: TxoutType::WITNESS_UNKNOWN destination (P2W???)
111+ * A txout script categorized into standard templates.
112+ *  * CNoDestination: A script, no corresponding address.

josibake commented at 11:30 am on September 8, 2023:

in https://github.com/bitcoin/bitcoin/pull/28246/commits/102fc00a1809f8786c03a0359ad2eb689e7e40dc:

The default constructor can make a CNoDestination with an empty script.

achow101 commented at 4:07 pm on September 11, 2023:

Clarified that it is optionally a script.

in src/addresstype.h:35 in 6c87957a8e outdated

26@@ -27,6 +27,19 @@ class CNoDestination {
27     friend bool operator<(const CNoDestination& a, const CNoDestination& b) { return a.GetScript() < b.GetScript(); }
28 };
29 
30+struct PubKeyDestination {
31+private:
32+    CPubKey m_pubkey;
33+
34+public:
35+    PubKeyDestination(const CPubKey& pubkey) : m_pubkey(pubkey) {}

josibake commented at 11:38 am on September 8, 2023:

in https://github.com/bitcoin/bitcoin/pull/28246/commits/6c87957a8e51cef93647858d61a5d38f3336eeaa:

Same question as before: is there a reason to not use pass by value and move?

0    PubKeyDestination(CPubKey pubkey) : m_pubkey(std::move(pubkey) {}

in src/addresstype.h:140 in 6c87957a8e outdated

135@@ -122,8 +136,8 @@ bool IsValidDestination(const CTxDestination& dest);
136  * is assigned to addressRet.
137  * For all other scripts. addressRet is assigned as a CNoDestination containing the scriptPubKey.
138  *
139- * Returns true for standard destinations - P2PK, P2PKH, P2SH, P2WPKH, P2WSH, and P2TR scripts.
140- * Returns false for non-standard destinations - P2PK with invalid pubkeys, P2W???, bare multisig, null data, and nonstandard scripts.
141+ * Returns true for standard destinations - P2PKH, P2SH, P2WPKH, P2WSH, and P2TR scripts.
142+ * Returns false for non-standard destinations - P2PK, P2W???, bare multisig, null data, and nonstandard scripts.

josibake commented at 11:49 am on September 8, 2023:

in https://github.com/bitcoin/bitcoin/pull/28246/commits/6c87957a8e51cef93647858d61a5d38f3336eeaa:

P2PK is still considered standard per https://github.com/bitcoin/bitcoin/blob/6c87957a8e51cef93647858d61a5d38f3336eeaa/src/script/solver.cpp#L188 and https://github.com/bitcoin/bitcoin/blob/6c87957a8e51cef93647858d61a5d38f3336eeaa/src/policy/policy.cpp#L70

achow101 commented at 4:07 pm on September 11, 2023:

Clarified that it’s looking for standard destinations with addresses. P2PK does not have an address.

in src/rpc/output_script.cpp:284 in 6c87957a8e outdated

279@@ -280,6 +280,11 @@ static RPCHelpMan deriveaddresses()
280                 for (const CScript& script : scripts) {
281                     CTxDestination dest;
282                     if (!ExtractDestination(script, dest)) {
283+                        // P2PK is no longer considered a valid destination
284+                        // However combo will output P2PK and should just ignore that script

josibake commented at 11:51 am on September 8, 2023:

in https://github.com/bitcoin/bitcoin/pull/28246/commits/6c87957a8e51cef93647858d61a5d38f3336eeaa:

Where are we saying this is no longer a valid destination? Per IsStandard and the Solver it seems to be fine, but perhaps I’m missing something.

achow101 commented at 4:08 pm on September 11, 2023:

Clarified that P2PK does not correspond to an address.

in src/key_io.cpp:193 in d33fa1fae6 outdated

189@@ -190,7 +190,7 @@ CTxDestination DecodeDestination(const std::string& str, const CChainParams& par
190                 return CNoDestination();
191             }
192 
193-            return WitnessUnknown{version, data};
194+            return WitnessUnknown{static_cast<unsigned int>(version), data};

josibake commented at 12:04 pm on September 8, 2023:

in https://github.com/bitcoin/bitcoin/pull/28246/commits/d33fa1fae66635992e6a0a980e7d16a9ebe4c852:

Is there a reason to do this here, as opposed to making an overloaded constructor for WitnessUnknown, like:

0WitnessUnknown(int version, std::vector<unsigned char> program)
1        : m_version(static_cast<unsigned int>(version)), m_program(std::move(program)) {}

that way, anything that creates a WitnessUnknown can just be WitnessUnknown{version, program}

achow101 commented at 4:08 pm on September 11, 2023:

Done

josibake commented at 12:07 pm on September 8, 2023: member

Looks good! I had some questions about the constructors for CNoDestination and PubKeyDestination, seems like there’s an opportunity to make them consistent. Regarding one of the new comments, it mentions that P2PK is non-standard, but I don’t think that’s true.

As far as testing, I’ve been rebasing my silent payments PRs on this one and haven’t ran into any issues

Frank-GER referenced this in commit 086142fb64 on Sep 8, 2023

in src/addresstype.h:130 in 102fc00a18 outdated

119  *  A CTxDestination is the internal data type encoded in a bitcoin address
120  */
121 using CTxDestination = std::variant<CNoDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessV1Taproot, WitnessUnknown>;
122 
123-/** Check whether a CTxDestination is a CNoDestination. */
124+/** Check whether a CTxDestination corresponds to one with an address. */

S3RK commented at 7:19 am on September 11, 2023:

IIUC IsValidDestination will return true for PubKeyDestination, but it doesn’t have an address.

ismaelsadeeq commented at 12:38 pm on September 11, 2023:

May be should be

0/** Check whether a CTxDestination is not isCNoDestination . */

Since thats what it does

achow101 commented at 4:17 pm on September 11, 2023:

The comment was what I intended for IsValidDestination, but the implementation got lost somewhere. Fixed.

in src/addresstype.h:138 in 102fc00a18 outdated

130- * P2PKH, P2SH, P2WPKH, and P2WSH scripts.
131+ * Parse a scriptPubKey for the destination.
132+ *
133+ * For standard scripts that have addresses (and P2PK as an exception), a corresponding CTxDestination
134+ * is assigned to addressRet.
135+ * For all other scripts. addressRet is assigned as a CNoDestination containing the scriptPubKey.

S3RK commented at 7:24 am on September 11, 2023:

The comment needs to be updated. Below you say that WitnessUnknown is not standard, but it has its own type, not CNoDestination.

achow101 commented at 4:11 pm on September 11, 2023:

Moved WitnessUnknown to the standard group.

in src/util/message.cpp:50 in 6f7bdea83f outdated

46@@ -47,7 +47,7 @@ MessageVerificationResult MessageVerify(
47         return MessageVerificationResult::ERR_PUBKEY_NOT_RECOVERED;
48     }
49 
50-    if (!(CTxDestination(PKHash(pubkey)) == destination)) {
51+    if (!(PKHash(pubkey) == *std::get_if<PKHash>(&destination))) {

ismaelsadeeq commented at 12:35 pm on September 11, 2023:

If IIUC this is a refactor. So should be in it’s own commit since the change does not concern ‘Make WitnessUnknown members private’ ?

josibake commented at 3:16 pm on September 11, 2023:

see #28246 (review)

It’s not actually a refactor, but a fix for some very strange / edge case behavior with std::variant and std::vector

ismaelsadeeq commented at 12:38 pm on September 11, 2023: member

Concept ACK

achow101 force-pushed on Sep 11, 2023

in src/addresstype.cpp:114 in 22d54175ab outdated

108@@ -108,6 +109,11 @@ class CScriptVisitor
109         return dest.GetScript();
110     }
111 
112+    CScript operator()(const PubKeyDestination& dest) const
113+    {
114+        return CScript() << ToByteVector(dest.GetPubKey()) << OP_CHECKSIG;

josibake commented at 5:46 pm on September 11, 2023:

in https://github.com/bitcoin/bitcoin/pull/28246/commits/22d54175ab14e6f8f0426dc015b897a4c4a35284:

This line is causing the fuzzer to fail in src/test/fuzz/script.cpp:155, because it first calls GetScriptForDestination, (which now returns a script for PubKeDestination), and then calls IsValidDestination and checks if the dest.empty() and IsValidDestination match. Can probably just special case PubKeyDestination? Or maybe there’s a more clever way to handle it

josibake commented at 6:00 pm on September 11, 2023:

This should do the trick (and seems reasonable): https://github.com/bitcoin/bitcoin/commit/7431452ab61a46715c31c8e0453234d328973088#diff-23790a106f16f165ad579a3966ae9faa871f148ca4640bc870f6071a121d5314L156-R161

achow101 commented at 11:15 pm on September 11, 2023:

Fixed

achow101 force-pushed on Sep 11, 2023

Make WitnessUnknown members private

Make sure that nothing else can change WitnessUnknown's data members by
making them private. Also change the program to use a vector rather than
C-style array.

8dd067088d

Allow CNoDestination to represent a raw script

Even if a script is not a standard destination type, it can still be
useful to have a CTxDestination that stores the script.

1a98a51c66

Add PubKeyDestination for P2PK scripts

P2PK scripts are not PKHash destinations, they should have their own
type.

This also results in no longer showing a p2pkh address for p2pk outputs.
However for backwards compatibility, ListCoinst will still do this
conversion.

07d3bdf4eb

wallet: Use CTxDestination in CRecipient rather than scriptPubKey ad0c469d98

achow101 force-pushed on Sep 12, 2023

josibake commented at 5:34 pm on September 12, 2023: member

ACK https://github.com/bitcoin/bitcoin/pull/28246/commits/ad0c469d98c51931b98b7fd937c6ac3eeaed024e

Verified that the -Wuninitialized error is now fixed. Code looks good and I’ve been using this in #28122 and https://github.com/bitcoin/bitcoin/pull/28201

DrahtBot requested review from S3RK on Sep 12, 2023

fanquake merged this on Sep 19, 2023

fanquake closed this on Sep 19, 2023

in src/addresstype.h:22 in ad0c469d98

13@@ -14,9 +14,30 @@
14 #include <algorithm>
15 
16 class CNoDestination {
17+private:
18+    CScript m_script;
19+
20+public:
21+    CNoDestination() = default;
22+    CNoDestination(const CScript& script) : m_script(script) {}

maflcko commented at 10:07 am on September 20, 2023:

Shouldn’t this be explicit? Otherwise passing a single CScript in the “wrong” context could decay to CNoDestiantion by accident?

maflcko commented at 11:25 am on October 25, 2023:

See https://github.com/bitcoin/bitcoin/pull/28728

in src/addresstype.h:35 in ad0c469d98

32+    CPubKey m_pubkey;
33+
34 public:
35-    friend bool operator==(const CNoDestination &a, const CNoDestination &b) { return true; }
36-    friend bool operator<(const CNoDestination &a, const CNoDestination &b) { return true; }
37+    PubKeyDestination(const CPubKey& pubkey) : m_pubkey(pubkey) {}

maflcko commented at 10:10 am on September 20, 2023:

same?

maflcko commented at 11:26 am on October 25, 2023:

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

in src/addresstype.h:24 in ad0c469d98

19+
20+public:
21+    CNoDestination() = default;
22+    CNoDestination(const CScript& script) : m_script(script) {}
23+
24+    const CScript& GetScript() const { return m_script; }

maflcko commented at 10:10 am on September 20, 2023:

nit: Could add LIFETIMEBOUND, if you re-touch?

maflcko commented at 11:26 am on October 25, 2023:

See https://github.com/bitcoin/bitcoin/pull/28728

domob1812 referenced this in commit 6f77f653e6 on Sep 25, 2023

Frank-GER referenced this in commit 10cb4a508a on Sep 25, 2023

sidhujag referenced this in commit 72cd9fd914 on Sep 26, 2023

domob1812 referenced this in commit 248e07e0e9 on Sep 27, 2023

Sjors commented at 1:49 pm on September 28, 2023: member

Verified that the -Wuninitialized error is now fixed.

Found another one post-merge (while testing things on master), although only when you ./configure with --enable-c++20. It was introduced by 8dd067088d41f021b357d7db5fa5f0a9f61edddc. Compiled on Ubuntu 23.10 with gcc 12.3.0.

Log output: https://gist.github.com/Sjors/22873167e460eb1265652e5dca973db2

0./configure --enable-c++20 --enable-suppress-external-warnings --disable-fuzz-binary --without-gui --enable-werror

maflcko commented at 6:03 pm on September 28, 2023: member

Found another one post-merge

That sounds like a bug in gcc, no?

Sjors commented at 7:45 pm on September 28, 2023: member

@MarcoFalke all I know is that --enable-c++20 has worked fine on master, and most of the PRs I reviewed, for several months, but I have no idea whose fault it is :-) Compiler says No.

maflcko commented at 10:23 am on September 29, 2023: member

Compiled on Ubuntu 23.10 with gcc 12.3.0.

Have you tried using 13.2 from the same distro? https://packages.ubuntu.com/mantic/g++

in src/addresstype.cpp:61 in 07d3bdf4eb outdated

61-        return true;
62+        if (!pubKey.IsValid()) {
63+            addressRet = CNoDestination(scriptPubKey);
64+        } else {
65+            addressRet = PubKeyDestination(pubKey);
66+        }

denavila commented at 4:59 pm on October 6, 2023:

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

murchandamus commented at 6:34 pm on October 6, 2023:

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.

denavila commented at 2:25 pm on October 7, 2023:

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.

denavila commented at 2:49 pm on October 7, 2023:

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.

achow101 commented at 3:29 pm on October 7, 2023:

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.

denavila commented at 5:08 pm on October 7, 2023:

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

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

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

 0    std::optional<OutputType> op_output_type;
 1    if (std::get_if<PubKeyDestination>(&shared_destination)) {
 2        op_output_type = OutputType::LEGACY;
 3    } else {
 4        op_output_type = OutputTypeFromDestination(shared_destination);
 5    }
 6    if (!op_output_type) {
 7        op_output_type = wallet.m_default_change_type;
 8    }
 9    if (!op_output_type) {
10        return util::Error{_("Unable to determine output type.")};
11    }

achow101 commented at 5:19 pm on October 7, 2023:

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.

denavila commented at 5:42 pm on October 7, 2023:

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.

Sjors commented at 8:03 am on October 11, 2023: member

@maflcko wrote:

Compiled on Ubuntu 23.10 with gcc 12.3.0.

Have you tried using 13.2 from the same distro? https://packages.ubuntu.com/mantic/g++

That did the trick. Although for some reason it picks 13.1.0 when I install gcc-13 and g++-13.

in src/wallet/wallet.cpp:2225 in ad0c469d98

2226             any_sh = true;
2227-        } else if (type == TxoutType::PUBKEYHASH) {
2228+        } else if (std::get_if<PKHash>(&recipient.dest)) {
2229             any_pkh = true;
2230         }
2231     }

maflcko commented at 12:08 pm on October 25, 2023:

Looking at the code, and the missing explicit keyword, I presume this broke this feature in the GUI?

maflcko commented at 12:12 pm on October 25, 2023:

Confirmed locally that this breaks the feature for the gui. Can be fixed on current master by calling:

0git revert ad0c469d98c51931b98b7fd937c6ac3eeaed024e

furszy commented at 12:50 pm on October 25, 2023:

Looking at the code, and the missing explicit keyword, I presume this broke this feature in the GUI?

Great catch. Can fix it with #28728 (review).

maflcko added the label Bug on Oct 25, 2023

maflcko referenced this in commit fa7184cff5 on Oct 25, 2023

maflcko referenced this in commit 1111475b41 on Oct 25, 2023

fanquake referenced this in commit 6772a62721 on Oct 30, 2023

fanquake referenced this in commit 6544ffa01f on Oct 31, 2023

janus referenced this in commit f2dca844af on Apr 1, 2024

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

Reviews

Conflicts