Merge bitcoin/bitcoin#28728: wallet: [bugfix] Mark CNoDestination and…

… PubKeyDestination constructor explicit 1111475 bugfix: Mark CNoDestination and PubKeyDestination constructor explicit (MarcoFalke) fa5ccc4 iwyu: Export prevector.h from script.h (MarcoFalke) Pull request description: 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. ACKs for top commit: josibake: ACK bitcoin/bitcoin@1111475 achow101: ACK 1111475 furszy: Code review ACK 1111475 Tree-SHA512: d8b5f54d0cd8649a31e227ef164bb13e5b81ee9820f1976fd70c7a0de6841fba72d549c2f63e351c8cdda37dceb4763eca203e1c8ef385f46d9da6f1855c39ec
bitcoin-core · Oct 26, 2023 · cb8844e · cb8844e
2 parents 2a349f9 + 1111475
commit cb8844e
Show file tree

Hide file tree

Showing 7 changed files with 26 additions and 19 deletions.
diff --git a/src/addresstype.h b/src/addresstype.h
@@ -5,23 +5,26 @@
 #ifndef BITCOIN_ADDRESSTYPE_H
 #define BITCOIN_ADDRESSTYPE_H
 
+#include <attributes.h>
 #include <pubkey.h>
 #include <script/script.h>
 #include <uint256.h>
 #include <util/hash_type.h>
 
-#include <variant>
 #include <algorithm>
+#include <variant>
+#include <vector>
 
-class CNoDestination {
+class CNoDestination
+{
 private:
     CScript m_script;
 
 public:
     CNoDestination() = default;
-    CNoDestination(const CScript& script) : m_script(script) {}
+    explicit CNoDestination(const CScript& script) : m_script(script) {}
 
-    const CScript& GetScript() const { return m_script; }
+    const CScript& GetScript() const LIFETIMEBOUND { return m_script; }
 
     friend bool operator==(const CNoDestination& a, const CNoDestination& b) { return a.GetScript() == b.GetScript(); }
     friend bool operator<(const CNoDestination& a, const CNoDestination& b) { return a.GetScript() < b.GetScript(); }
@@ -32,7 +35,7 @@ struct PubKeyDestination {
     CPubKey m_pubkey;
 
 public:
-    PubKeyDestination(const CPubKey& pubkey) : m_pubkey(pubkey) {}
+    explicit PubKeyDestination(const CPubKey& pubkey) : m_pubkey(pubkey) {}
 
     const CPubKey& GetPubKey() const LIFETIMEBOUND { return m_pubkey; }
 

diff --git a/src/bench/wallet_create_tx.cpp b/src/bench/wallet_create_tx.cpp
@@ -94,13 +94,14 @@ static void WalletCreateTx(benchmark::Bench& bench, const OutputType output_type
     }
 
     // Generate destinations
-    CScript dest = GetScriptForDestination(getNewDestination(wallet, output_type));
+    const auto dest{getNewDestination(wallet, output_type)};
 
     // Generate chain; each coinbase will have two outputs to fill-up the wallet
     const auto& params = Params();
+    const CScript coinbase_out{GetScriptForDestination(dest)};
     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, coinbase_out);
     }
 
     // Check available balance
@@ -185,4 +186,4 @@ static void WalletAvailableCoins(benchmark::Bench& bench) { AvailableCoins(bench
 
 BENCHMARK(WalletCreateTxUseOnlyPresetInputs, benchmark::PriorityLevel::LOW)
 BENCHMARK(WalletCreateTxUsePresetInputsAndCoinSelection, benchmark::PriorityLevel::LOW)
-BENCHMARK(WalletAvailableCoins, benchmark::PriorityLevel::LOW);
+BENCHMARK(WalletAvailableCoins, benchmark::PriorityLevel::LOW);
diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp
@@ -187,8 +187,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact
             setAddress.insert(rcp.address);
             ++nAddresses;
 
-            CScript scriptPubKey = GetScriptForDestination(DecodeDestination(rcp.address.toStdString()));
-            CRecipient recipient = {scriptPubKey, rcp.amount, rcp.fSubtractFeeFromAmount};
+            CRecipient recipient{DecodeDestination(rcp.address.toStdString()), rcp.amount, rcp.fSubtractFeeFromAmount};
             vecSend.push_back(recipient);
 
             total += rcp.amount;

diff --git a/src/script/script.cpp b/src/script/script.cpp
@@ -1,11 +1,14 @@
 // Copyright (c) 2009-2010 Satoshi Nakamoto
-// Copyright (c) 2009-2021 The Bitcoin Core developers
+// Copyright (c) 2009-present The Bitcoin Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <script/script.h>
 
+#include <crypto/common.h>
 #include <hash.h>
+#include <uint256.h>
+#include <util/hash_type.h>
 #include <util/strencodings.h>
 
 #include <string>

diff --git a/src/script/script.h b/src/script/script.h
@@ -1,5 +1,5 @@
 // Copyright (c) 2009-2010 Satoshi Nakamoto
-// Copyright (c) 2009-2022 The Bitcoin Core developers
+// Copyright (c) 2009-present The Bitcoin Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
@@ -8,18 +8,19 @@
 
 #include <attributes.h>
 #include <crypto/common.h>
-#include <prevector.h>
+#include <prevector.h> // IWYU pragma: export
 #include <serialize.h>
 #include <uint256.h>
 #include <util/hash_type.h>
 
-#include <assert.h>
-#include <climits>
+#include <cassert>
+#include <cstdint>
+#include <cstring>
 #include <limits>
 #include <stdexcept>
-#include <stdint.h>
-#include <string.h>
 #include <string>
+#include <type_traits>
+#include <utility>
 #include <vector>
 
 // Maximum number of bytes pushable to the stack

diff --git a/src/wallet/test/spend_tests.cpp b/src/wallet/test/spend_tests.cpp
@@ -78,7 +78,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_duplicated_preset_inputs_test, TestChain100Setup)
 
     // Try to create a tx that spends more than what preset inputs + wallet selected inputs are covering for.
     // The wallet can cover up to 200 BTC, and the tx target is 299 BTC.
-    std::vector<CRecipient> recipients = {{GetScriptForDestination(*Assert(wallet->GetNewDestination(OutputType::BECH32, "dummy"))),
+    std::vector<CRecipient> recipients{{*Assert(wallet->GetNewDestination(OutputType::BECH32, "dummy")),
                                            /*nAmount=*/299 * COIN, /*fSubtractFeeFromAmount=*/true}};
     CCoinControl coin_control;
     coin_control.m_allow_other_inputs = true;

diff --git a/src/wallet/test/wallet_tests.cpp b/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{PubKeyDestination{{}}, 1 * COIN, /*subtract_fee=*/false});
     {
         LOCK(wallet->cs_wallet);
         list = ListCoins(*wallet);