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

maflcko · 2023-10-25T11:25:09Z

It seems confusing to allow any script, even one with a corresponding address, to silently convert to CNoDestination.

Make the converstion explicit in the code, and fix any bugs that were previously introduced.

In a follow-up, the class can be renamed, or the documentation can be updated to better reflect what the code does.

This should cut some include bloat and seems fine to do, because prevector exists primarily to represent scripts. Also, add missing includes to script.h and addresstype.h

DrahtBot · 2023-10-25T11:25:12Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	josibake, furszy, achow101

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:

#28690 (build: Introduce internal kernel library by TheCharlatan)
#28550 (Covenant tools softfork by jamesob)
#28400 (Make provably unsignable standard P2PK and P2MS outpoints unspendable. by russeree)

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.

furszy

Nice catch

src/qt/walletmodel.cpp

src/wallet/test/spend_tests.cpp

josibake

tACK fa7184c

Good catch! Reviewed the code and tested and it looks good. There is a typo in the fa7184c commit message (destiantion -> destination) and a typo in the description (Make the confusion -> Make the conversion). Not a big deal, but happy to re-ack if you end up re-pushing to fix the commit message typo.

josibake · 2023-10-25T14:12:32Z

src/qt/walletmodel.cpp

-            CScript scriptPubKey = GetScriptForDestination(DecodeDestination(rcp.address.toStdString()));
-            CRecipient recipient = {scriptPubKey, rcp.amount, rcp.fSubtractFeeFromAmount};
+            CRecipient recipient{DecodeDestination(rcp.address.toStdString()), rcp.amount, rcp.fSubtractFeeFromAmount};


To check my understanding:

The bug was introduced by not updating this line in #28246 to use the CTxDestination returned by DecodeDestination directly in the CRecipient object. Instead, it was converting to a CScript and silently converting to a CNoDestination, since CRecipient now expects a CTxDestination. So if we had marked the CNoDestination constructor as explicit, the compiler would have warned us here.

I changed it back to try and use the CScript directly in the CRecipient and confirmed I got a compiler warning:

qt/walletmodel.cpp:192:34: error: could not convert ‘spk’ from ‘const CScript’ to ‘CTxDestination’

Yes, sounds correct.

maflcko · 2023-10-25T14:29:19Z

Thanks, fixed the typo in the pull request description.

furszy

Aside from the two comments, do you think that mixing the bug fix with some code styling improvements and the iwyu commit is really desirable?
I know that those are small, good and easy reviewable changes but.. it might be better for backporting and historical context to leave this PR only with the bug fix.

src/bench/wallet_create_tx.cpp

furszy · 2023-10-25T20:10:07Z

src/wallet/test/wallet_tests.cpp

@@ -605,7 +605,7 @@ BOOST_FIXTURE_TEST_CASE(ListCoinsTest, ListCoinsTestingSetup)
    // returns the coin associated with the change address underneath the
    // coinbaseKey pubkey, even though the change address has a different
    // pubkey.
-    AddTx(CRecipient{GetScriptForRawPubKey({}), 1 * COIN, /*subtract_fee=*/false});
+    AddTx(CRecipient{CPubKey{}, 1 * COIN, /*subtract_fee=*/false});


Not for this PR but based on this recent not so good experience, I think we should make all destinations constructors explicit.

Here, we are providing a CPubKey to implicitly construct a PubKeyDestination, which is not very intuitive. There is a PKHash constructor (the CTxDestination type of a p2pkh) that also accepts a CPubKey as argument.

Thanks, done in the last push. I missed that CPubKey was also ambiguous.

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.

maflcko · 2023-10-25T20:53:15Z

iwyu commit is really desirable?

Missing includes have been a source for compile failures in the past. Though, I couldn't care less whether it is backported or not. I think it is up to the person doing the backport to decide what commits to backport, if any at all.

josibake · 2023-10-26T10:20:16Z

ACK 1111475

Nice to see changing PubKeyDestination to explicit didn't surface any surprises.

furszy

Missing includes have been a source for compile failures in the past.

Hard to disagree with it.

Though, I couldn't care less whether it is backported or not. I think it is up to the person doing the backport to decide what commits to backport, if any at all.

This PR is simple enough. I just would have preferred to not add the code styling fixes within the bugfix commit, even when they are tempting small things to tackle.

About the conceptual discussion
In general, I believe we all care about not releasing a new version with broken code. And, we can support this goal by making any post-branch-off pre-release bugfix as simple as possible.

furszy

Code review ACK 1111475

maflcko · 2023-10-26T13:51:35Z

I just would have preferred to not add the code styling fixes within the bugfix commit, even when they are tempting small things to tackle.

If you are referring to the missing newline at the end of the file, this is not something I do on purpose. This is done automatically by most vanilla editors when saving the file. A missing trailing newline will cause problems with those editors and git, so I think it makes sense to add a linter for this to prevent people from adding such a file in the first place?

achow101 · 2023-10-26T14:56:21Z

ACK 1111475

furszy · 2023-10-26T14:58:49Z

I just would have preferred to not add the code styling fixes within the bugfix commit, even when they are tempting small things to tackle.

If you are referring to the missing newline at the end of the file, this is not something I do on purpose. This is done automatically by most vanilla editors when saving the file. A missing trailing newline will cause problems with those editors and git, so I think it makes sense to add a linter for this to prevent people from adding such a file in the first place?

yeah, that would be nice.

maflcko · 2023-10-26T15:33:10Z

Unrelated from this bugfix, I also wondered if there should be a "true" "no destiantion" type. Currently, a non-standard empty script is mapped to the same value like a default constructed CNoDestination{}.

I guess this is fine, but if in the future there is a need to have a "real null", CNoDestination can be renamed to mean "non-standard". That is:

9e5f699969f85a31e229cf89558f2bc59b64e8df
    scripted-diff: Rename CNoDestination to NonStandardDestination
    
    Generally it is a good idea to change the name of a class when the
    meaning of the class changes. This ensures that a compilation failure
    will be emitted when a developer uses the old name (and meaning) of the
    class.
    
    Since CNoDestination changed its meaning from "holding no destination"
    to "holding a nonstandard destination", rename it to that.
    
    -BEGIN VERIFY SCRIPT-
     sed -i 's/CNoDestination/NonStandardDestination/g' $( git grep -l CNoDestination )
    -END VERIFY SCRIPT-

achow101 · 2023-10-26T15:35:37Z

Unrelated from this bugfix, I also wondered if there should be a "true" "no destiantion" type. Currently, a non-standard empty script is mapped to the same value like a default constructed CNoDestination{}.

I don't think it is all that useful to have an actual "no destination". It could easily be served by using a std::optional<CTxDestination>. I've got an unfinished branch that does that.

jarolrod

post merge ack

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

fanquake · 2023-10-30T13:25:43Z

Backported in #28754.

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

e4e8479 doc: update manual pages for v26.0rc2 (fanquake) 0b189a9 build: bump version to v26.0rc2 (fanquake) e097d4c gui: fix crash on selecting "Mask values" in transaction view (Sebastian Falbesoner) 05e8874 guix: update signapple (fanquake) deccc50 guix: Zip needs to include all files with time as SOURCE_DATE_EPOCH (Andrew Chow) fe57abd test: add coverage for snapshot chainstate not matching AssumeUTXO parameters (pablomartin4btc) b761a58 assumeutxo, blockstorage: prevent core dump on invalid hash (pablomartin4btc) d3ebf6e [test] Test i2p private key constraints (Vasil Dimov) 1f11784 [net] Check i2p private key constraints (dergoegge) 6544ffa bugfix: Mark CNoDestination and PubKeyDestination constructor explicit (MarcoFalke) Pull request description: Backports for v26.0rc2: * #28695 * #28698 * #28728 * #28757 * #28759 * bitcoin-core/gui#774 ACKs for top commit: josibake: ACK e4e8479 hebasto: re-ACK e4e8479, only a backport of bitcoin-core/gui#774 added since my [recent](#28754 (review)) review. TheCharlatan: Re-ACK e4e8479 Tree-SHA512: 4b95afd26b8bf91250cb883423de8b274cefa48dc474734f5900aeb756eee3a6c656116efcfa2caff3c250678c16b70cc6b7a5d840018dc7e2c1e8161622cd61

iwyu: Export prevector.h from script.h

fa5ccc4

This should cut some include bloat and seems fine to do, because prevector exists primarily to represent scripts. Also, add missing includes to script.h and addresstype.h

DrahtBot changed the title ~~refactor: Mark CNoDestination constructor explicit~~ refactor: Mark CNoDestination constructor explicit Oct 25, 2023

DrahtBot added the Refactoring label Oct 25, 2023

maflcko mentioned this pull request Oct 25, 2023

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

Merged

maflcko force-pushed the 2310-explicit-CNoDestination- branch from fa6eec0 to fa5f1f0 Compare October 25, 2023 11:45

DrahtBot added the CI failed label Oct 25, 2023

maflcko changed the title ~~refactor: Mark CNoDestination constructor explicit~~ bugfix: Mark CNoDestination constructor explicit Oct 25, 2023

furszy reviewed Oct 25, 2023

View reviewed changes

src/qt/walletmodel.cpp Outdated Show resolved Hide resolved

maflcko force-pushed the 2310-explicit-CNoDestination- branch from fa5f1f0 to fa7184c Compare October 25, 2023 13:00

furszy reviewed Oct 25, 2023

View reviewed changes

src/wallet/test/spend_tests.cpp Outdated Show resolved Hide resolved

maflcko removed the Refactoring label Oct 25, 2023

maflcko changed the title ~~bugfix: Mark CNoDestination constructor explicit~~ wallet: [bugfix] Mark CNoDestination constructor explicit Oct 25, 2023

This was referenced Oct 25, 2023

build: Introduce internal kernel library #28690

Open

Covenant tools softfork #28550

Closed

josibake approved these changes Oct 25, 2023

View reviewed changes

DrahtBot mentioned this pull request Oct 25, 2023

Make provably unsignable standard P2PK and P2MS outpoints unspendable. #28400

Closed

achow101 added the Needs backport (26.x) label Oct 25, 2023

achow101 added this to the 26.0 milestone Oct 25, 2023

furszy reviewed Oct 25, 2023

View reviewed changes

maflcko changed the title ~~wallet: [bugfix] Mark CNoDestination constructor explicit~~ wallet: [bugfix] Mark CNoDestination and PubKeyDestination constructor explicit Oct 25, 2023

maflcko force-pushed the 2310-explicit-CNoDestination- branch from fa7184c to 1111475 Compare October 25, 2023 20:47

DrahtBot removed the CI failed label Oct 25, 2023

furszy reviewed Oct 26, 2023

View reviewed changes

achow101 merged commit cb8844e into bitcoin:master Oct 26, 2023

maflcko deleted the 2310-explicit-CNoDestination- branch October 26, 2023 15:27

jarolrod reviewed Oct 27, 2023

View reviewed changes

fanquake mentioned this pull request Oct 30, 2023

[26.x] Backports for rc2 #28754

Merged

fanquake removed the Needs backport (26.x) label Oct 30, 2023

DrahtBot added the Wallet label Oct 30, 2023

bitcoin locked and limited conversation to collaborators Oct 29, 2024

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

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

Uh oh!

Conversation

maflcko commented Oct 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Oct 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

josibake left a comment

Choose a reason for hiding this comment

Uh oh!

josibake Oct 25, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko Oct 25, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko commented Oct 25, 2023

Uh oh!

furszy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy Oct 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Oct 25, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko commented Oct 25, 2023

Uh oh!

josibake commented Oct 26, 2023

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Oct 26, 2023

Uh oh!

achow101 commented Oct 26, 2023

Uh oh!

furszy commented Oct 26, 2023

Uh oh!

maflcko commented Oct 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Oct 26, 2023

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Oct 30, 2023

Uh oh!

Uh oh!

maflcko commented Oct 25, 2023 •

edited

Loading

DrahtBot commented Oct 25, 2023 •

edited

Loading

furszy left a comment •

edited

Loading

furszy Oct 25, 2023 •

edited

Loading

maflcko commented Oct 26, 2023 •

edited

Loading