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
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
187 | @@ -188,7 +188,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact 188 | ++nAddresses; 189 | 190 | CScript scriptPubKey = GetScriptForDestination(DecodeDestination(rcp.address.toStdString())); 191 | - CRecipient recipient = {scriptPubKey, rcp.amount, rcp.fSubtractFeeFromAmount}; 192 | + CRecipient recipient{CNoDestination{scriptPubKey}, rcp.amount, rcp.fSubtractFeeFromAmount};
We know the destination here. Otherwise we wouldn't be able to decode it from the string (the validateAddress() check that is performed above would fail). This should be:
CTxDestination dest = DecodeDestination(rcp.address.toStdString());
CRecipient recipient = {dest, rcp.amount, rcp.fSubtractFeeFromAmount};
This fixes #28246 (review).
Jup, already had this, but forgot to push my local commit :sweat_smile:
Nice catch
77 | @@ -78,7 +78,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_duplicated_preset_inputs_test, TestChain100Setup) 78 | 79 | // Try to create a tx that spends more than what preset inputs + wallet selected inputs are covering for. 80 | // The wallet can cover up to 200 BTC, and the tx target is 299 BTC. 81 | - std::vector<CRecipient> recipients = {{GetScriptForDestination(*Assert(wallet->GetNewDestination(OutputType::BECH32, "dummy"))), 82 | + std::vector<CRecipient> recipients{{CNoDestination{GetScriptForDestination(*Assert(wallet->GetNewDestination(OutputType::BECH32, "dummy")))},
~Same as above, can just pass GetScriptForDestination return value without passing through GetScriptForDestination. The destination was created by the wallet, thus is already known.~
You already fixed it.
186 | @@ -187,8 +187,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact 187 | setAddress.insert(rcp.address); 188 | ++nAddresses; 189 | 190 | - CScript scriptPubKey = GetScriptForDestination(DecodeDestination(rcp.address.toStdString())); 191 | - CRecipient recipient = {scriptPubKey, rcp.amount, rcp.fSubtractFeeFromAmount}; 192 | + 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.
tACK https://github.com/bitcoin/bitcoin/commit/fa7184cff5e03e7f5a54f616630f181737093ebf
Good catch! Reviewed the code and tested and it looks good. There is a typo in the https://github.com/bitcoin/bitcoin/commit/fa7184cff5e03e7f5a54f616630f181737093ebf 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.
Thanks, fixed the typo in the pull request description.
100 | // Generate chain; each coinbase will have two outputs to fill-up the wallet
101 | const auto& params = Params();
102 | unsigned int chain_size = 5000; // 5k blocks means 10k UTXO for the wallet (minus 200 due COINBASE_MATURITY)
103 | for (unsigned int i = 0; i < chain_size; ++i) {
104 | - generateFakeBlock(params, test_setup->m_node, wallet, dest);
105 | + generateFakeBlock(params, test_setup->m_node, wallet, GetScriptForDestination(dest));
This will call GetScriptForDestination() 5000 times when dest is constant. Why not leaving GetScriptForDestination where it was before?
It wouldn't compile if this file was not touched. For reference, you can also see the refactor-transformation of the same commit https://github.com/bitcoin/bitcoin/commit/fa5f1f054d7daf2d295b44786b61868aad89898b#diff-e9fe86a6cb5a18f8bf472c45e9d02cca8aea5dac8e3d2a0b81ce25a734458139
Yeah, needs another variable:
diff --git a/src/bench/wallet_create_tx.cpp b/src/bench/wallet_create_tx.cpp
--- a/src/bench/wallet_create_tx.cpp (revision d53400e75e2a4573229dba7f1a0da88eb936811c)
+++ b/src/bench/wallet_create_tx.cpp (date 1698266114002)
@@ -94,13 +94,14 @@
}
// Generate destinations
- CScript dest = GetScriptForDestination(getNewDestination(wallet, output_type));
+ CTxDestination dest{getNewDestination(wallet, output_type)};
+ CScript out_script{GetScriptForDestination(dest)};
// Generate chain; each coinbase will have two outputs to fill-up the wallet
const auto& params = Params();
unsigned int chain_size = 5000; // 5k blocks means 10k UTXO for the wallet (minus 200 due COINBASE_MATURITY)
for (unsigned int i = 0; i < chain_size; ++i) {
- generateFakeBlock(params, test_setup->m_node, wallet, dest);
+ generateFakeBlock(params, test_setup->m_node, wallet, out_script);
}
// Check available balance
I don't think it matters, because generateFakeBlock is likely more expensive than shuffling a few bytes.
Either way, done in the last push.
604 | @@ -605,7 +605,7 @@ BOOST_FIXTURE_TEST_CASE(ListCoinsTest, ListCoinsTestingSetup) 605 | // returns the coin associated with the change address underneath the 606 | // coinbaseKey pubkey, even though the change address has a different 607 | // pubkey. 608 | - AddTx(CRecipient{GetScriptForRawPubKey({}), 1 * COIN, /*subtract_fee=*/false}); 609 | + 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.
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.
This should fix the bug reported in
https://github.com/bitcoin/bitcoin/pull/28246#discussion_r1371640502,
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.
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.
ACK https://github.com/bitcoin/bitcoin/commit/1111475b41698260cda0f25a96c051fd18d66129
Nice to see changing PubKeyDestination to explicit didn't surface any surprises.
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.
Code review ACK 1111475
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?
ACK 1111475b41698260cda0f25a96c051fd18d66129
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.
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-
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.
post merge ack
Milestone
26.0