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

achow101 · 2023-08-09T15:16:28Z

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 · 2023-08-09T15:16:33Z

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.

src/addresstype.h

josibake · 2023-09-12T17:34:54Z

ACK ad0c469

Verified that the -Wuninitialized error is now fixed. Code looks good and I've been using this in #28122 and #28201

maflcko · 2023-09-20T10:07:54Z

src/addresstype.h

+
+public:
+    CNoDestination() = default;
+    CNoDestination(const CScript& script) : m_script(script) {}


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

maflcko · 2023-09-20T10:10:02Z

src/addresstype.h

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


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

maflcko · 2023-09-20T10:10:40Z

src/addresstype.h

+    CNoDestination() = default;
+    CNoDestination(const CScript& script) : m_script(script) {}
+
+    const CScript& GetScript() const { return m_script; }


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

Upstream bitcoin/bitcoin#28246 changed CRecipient to use a CTxDestination instead of a raw script; it supports raw scripts with CNoDestination as a fallbackup. This is how it currently continues to handle name scripts.

… of just scriptPubKey

… of jus…

Upstream changed CRecipient to hold a CTxDestination for the "recipient address" instead of a raw script, see bitcoin/bitcoin#28246. While this continues to "just work" for name operations since it still supports arbitrary scripts, this is of course not fully in line with the upstream refactor. In this commit, we change CRecipient to hold a name operation, if any, in a separate field that just contains the name script. The recipient address is kept in the CTxDestination. With this, name operations will more transparently blend in with the upstream code, and will more directly benefit from future things like silent payments.

Sjors · 2023-09-28T13:49:53Z

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 8dd0670. Compiled on Ubuntu 23.10 with gcc 12.3.0.

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

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

maflcko · 2023-09-28T18:03:32Z

Found another one post-merge

That sounds like a bug in gcc, no?

Sjors · 2023-09-28T19:45:26Z

@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 · 2023-09-29T10:23:24Z

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

denavila · 2023-10-06T16:59:53Z

src/addresstype.cpp

+            addressRet = CNoDestination(scriptPubKey);
+        } else {
+            addressRet = PubKeyDestination(pubKey);
+        }


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

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.

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.

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.

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.

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.")}; }

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.

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.

murchandamus · 2023-08-24T21:05:15Z

src/addresstype.h

@@ -115,8 +125,8 @@ bool IsValidDestination(const CTxDestination& dest);
 * is assigned to addressRet.
 * For all other scripts. addressRet is assigned as a CNoDestination containing the scriptPubKey.
 *
- * 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 - P2PKH, P2SH, P2WPKH, P2WSH, and P2TR scripts.


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.

Suggested change

* Returns true for standard destinations - P2PKH, P2SH, P2WPKH, P2WSH, and P2TR scripts.

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

murchandamus · 2023-10-06T18:34:01Z

src/addresstype.cpp

+            addressRet = CNoDestination(scriptPubKey);
+        } else {
+            addressRet = PubKeyDestination(pubKey);
+        }


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.

Sjors · 2023-10-11T08:03:20Z

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

maflcko · 2023-10-25T12:08:19Z

src/wallet/wallet.cpp

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


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

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

git revert ad0c469d98c51931b98b7fd937c6ac3eeaed024e

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

This should fix the bug reported in bitcoin#28246 (comment), which caused the GUI to not detect the destination type of recipients, thus picking the wrong change destiantion type. Also, add missing lifetimebound attribute to a getter method.

This should fix the bug reported in bitcoin#28246 (comment), which caused the GUI to not detect the destination type of recipients, thus picking the wrong change destination type. Also, add missing lifetimebound attribute to a getter method.

This should fix the bug reported in bitcoin#28246 (comment), which caused the GUI to not detect the destination type of recipients, thus picking the wrong change destination type. Also, add missing lifetimebound attribute to a getter method. GitHub-Pull: bitcoin#28728 Rebased-From: 1111475

This should fix the bug reported in bitcoin/bitcoin#28246 (comment), which caused the GUI to not detect the destination type of recipients, thus picking the wrong change destination type. Also, add missing lifetimebound attribute to a getter method.

DrahtBot added the Wallet label Aug 9, 2023

DrahtBot mentioned this pull request Aug 9, 2023

Add Import to Wallet GUI bitcoin-core/gui#650

Closed

2 tasks

bitcoin deleted a comment from reclaim4u Aug 9, 2023

DrahtBot added the CI failed label Aug 9, 2023

josibake reviewed Aug 13, 2023

View reviewed changes

src/addresstype.h Outdated Show resolved Hide resolved

achow101 force-pushed the ctxdest-in-crecipient branch 3 times, most recently from 12419d6 to 46ffe75 Compare August 14, 2023 22:06

DrahtBot removed the CI failed label Aug 15, 2023

DrahtBot requested a review from S3RK September 12, 2023 17:34

fanquake merged commit 53313c4 into bitcoin:master Sep 19, 2023
15 checks passed

maflcko reviewed Sep 20, 2023

View reviewed changes

Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Sep 25, 2023

Merge bitcoin#28246: wallet: Use CTxDestination in CRecipient instead…

10cb4a5

… of just scriptPubKey

josibake mentioned this pull request Sep 26, 2023

BIP352 tracking issue #28536

Open

15 tasks

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 26, 2023

Merge bitcoin#28246: wallet: Use CTxDestination in CRecipient instead…

72cd9fd

… of jus…

denavila mentioned this pull request Oct 6, 2023

wallet: Deniability API (Unilateral Transaction Meta-Privacy) #27792

Open

denavila reviewed Oct 6, 2023

View reviewed changes

murchandamus reviewed Oct 6, 2023

View reviewed changes

maflcko reviewed Oct 25, 2023

View reviewed changes

maflcko added the Bug label Oct 25, 2023

furszy mentioned this pull request Oct 25, 2023

wallet: [bugfix] Mark CNoDestination and PubKeyDestination constructor explicit #28728

Merged

Ayush170-Future mentioned this pull request Nov 4, 2023

fuzz: wallet, add target for Spend #28236

Draft

This was referenced Dec 5, 2023

wallet: Problems with scriptPubKey in CRecipient litecoin-project/litecoin#931

Open

wallet: Problems with scriptPubKey in CRecipient BTCGPU/BTCGPU#458

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

achow101 commented Aug 9, 2023

DrahtBot commented Aug 9, 2023 •

edited

Loading

josibake commented Sep 12, 2023

maflcko Sep 20, 2023

maflcko Oct 25, 2023

maflcko Sep 20, 2023

maflcko Oct 25, 2023

maflcko Sep 20, 2023

maflcko Oct 25, 2023

Sjors commented Sep 28, 2023

maflcko commented Sep 28, 2023

Sjors commented Sep 28, 2023

maflcko commented Sep 29, 2023

denavila Oct 6, 2023

murchandamus Oct 6, 2023

denavila Oct 7, 2023

denavila Oct 7, 2023 •

edited

Loading

achow101 Oct 7, 2023

denavila Oct 7, 2023 •

edited

Loading

achow101 Oct 7, 2023 •

edited

Loading

denavila Oct 7, 2023

murchandamus Aug 24, 2023

murchandamus Oct 6, 2023

Sjors commented Oct 11, 2023

maflcko Oct 25, 2023

maflcko Oct 25, 2023

furszy Oct 25, 2023

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

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

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

Conversation

achow101 commented Aug 9, 2023

DrahtBot commented Aug 9, 2023 • edited Loading

Reviews

Conflicts

josibake commented Sep 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sjors commented Sep 28, 2023

maflcko commented Sep 28, 2023

Sjors commented Sep 28, 2023

maflcko commented Sep 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denavila Oct 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denavila Oct 7, 2023 • edited Loading

Choose a reason for hiding this comment

achow101 Oct 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sjors commented Oct 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrahtBot commented Aug 9, 2023 •

edited

Loading

denavila Oct 7, 2023 •

edited

Loading

denavila Oct 7, 2023 •

edited

Loading

achow101 Oct 7, 2023 •

edited

Loading