From 6544ffa01fc1f219817e8c22b5d1d44ea2efa465 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 25 Oct 2023 12:13:44 +0200 Subject: [PATCH] bugfix: Mark CNoDestination and PubKeyDestination constructor explicit 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. GitHub-Pull: #28728 Rebased-From: 1111475b41698260cda0f25a96c051fd18d66129 --- src/addresstype.h | 9 +++++---- src/bench/wallet_create_tx.cpp | 7 ++++--- src/qt/walletmodel.cpp | 3 +-- src/wallet/test/spend_tests.cpp | 2 +- src/wallet/test/wallet_tests.cpp | 2 +- 5 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/addresstype.h b/src/addresstype.h index d3422c68130..522f58fef13 100644 --- a/src/addresstype.h +++ b/src/addresstype.h @@ -13,15 +13,16 @@ #include #include -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 +33,7 @@ private: 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 index 160534b63ca..632918c0ca9 100644 --- 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); \ No newline at end of file +BENCHMARK(WalletAvailableCoins, benchmark::PriorityLevel::LOW); diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index ee3327530c4..a45579fa0d0 100644 --- 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/wallet/test/spend_tests.cpp b/src/wallet/test/spend_tests.cpp index 68c98ae6b9e..5926d881290 100644 --- 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 recipients = {{GetScriptForDestination(*Assert(wallet->GetNewDestination(OutputType::BECH32, "dummy"))), + std::vector 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 index ad4bb3a9d2b..dea7be03a69 100644 --- 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);