From 7631118e042d48941317b156be32639515c35423 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Fri, 15 Sep 2023 14:32:08 +0700 Subject: [PATCH 01/11] fix: correct parsing pubkey in merge bitcoin#18204: improve descriptor cache and cache xpubs --- src/script/descriptor.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp index eff6e3442ef5..a0b973ddeefa 100644 --- a/src/script/descriptor.cpp +++ b/src/script/descriptor.cpp @@ -819,12 +819,12 @@ std::unique_ptr ParseScript(uint32_t key_exp_index, Span(std::move(pubkey)); } if (Func("pkh", expr)) { - auto pubkey = ParsePubkey(key_exp_index, expr, ctx != ParseScriptContext::P2SH, out, error); + auto pubkey = ParsePubkey(key_exp_index, expr, true, out, error); if (!pubkey) return nullptr; return std::make_unique(std::move(pubkey)); } @@ -851,7 +851,7 @@ std::unique_ptr ParseScript(uint32_t key_exp_index, SpanGetSize() + 1; providers.emplace_back(std::move(pk)); From fa23a64471276d0ea7b9c698221e892307d1710c Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Tue, 29 Aug 2023 14:39:50 +0700 Subject: [PATCH 02/11] partial (missing function GetKeyForDestination) Merge #11403: SegWit wallet support --- src/script/signingprovider.cpp | 10 ++++++++++ src/script/signingprovider.h | 3 +++ 2 files changed, 13 insertions(+) diff --git a/src/script/signingprovider.cpp b/src/script/signingprovider.cpp index fd5231519bde..7ce92a546336 100644 --- a/src/script/signingprovider.cpp +++ b/src/script/signingprovider.cpp @@ -150,3 +150,13 @@ bool FillableSigningProvider::GetCScript(const CScriptID &hash, CScript& redeemS } return false; } + +CKeyID GetKeyForDestination(const SigningProvider& store, const CTxDestination& dest) +{ + // Only supports destinations which map to single public keys, i.e. P2PKH + const PKHash *pkhash = std::get_if(&dest); + + if (pkhash != nullptr) return ToKeyID(*pkhash); + + return CKeyID(); +} diff --git a/src/script/signingprovider.h b/src/script/signingprovider.h index 468dbb4a0674..8ab2793a5b5d 100644 --- a/src/script/signingprovider.h +++ b/src/script/signingprovider.h @@ -131,4 +131,7 @@ class FillableSigningProvider : public SigningProvider virtual bool GetCScript(const CScriptID &hash, CScript& redeemScriptOut) const override; }; +/** Return the CKeyID of the key involved in a script (if there is a unique one). */ +CKeyID GetKeyForDestination(const SigningProvider& store, const CTxDestination& dest); + #endif // BITCOIN_SCRIPT_SIGNINGPROVIDER_H From 6183bd1085dd895f9012ba98554ed4d0352a7d78 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Tue, 29 Aug 2023 13:26:47 +0700 Subject: [PATCH 03/11] follow-up Merge #17260: Split some CWallet functions into new LegacyScriptPubKeyMan Some changes are missing or incorrectly backported during CWallet refactoring in #17260, #17261 such as: - Missing changes for CWallet::GetOldestKeyPoolTime - useless check of spk_man existance in getnewaddress - GetHDChain is used assuming it exists only legacy keymanager - using internal spk_man API instead wallet's in getwalletinfo --- src/wallet/rpcwallet.cpp | 8 ++------ src/wallet/wallet.cpp | 25 +++++++++++++++++++------ 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 752098cbf57c..218929c9ec53 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -258,10 +258,6 @@ UniValue getnewaddress(const JSONRPCRequest& request) if (!wallet) return NullUniValue; CWallet* const pwallet = wallet.get(); - LegacyScriptPubKeyMan* spk_man = pwallet->GetLegacyScriptPubKeyMan(); - if (!spk_man) { - throw JSONRPCError(RPC_WALLET_ERROR, "This type of wallet does not support this command"); - } LOCK(pwallet->cs_wallet); if (!pwallet->CanGetAddresses()) { @@ -2560,9 +2556,9 @@ static UniValue getwalletinfo(const JSONRPCRequest& request) if (spk_man) { obj.pushKV("timefirstkey", spk_man->GetTimeFirstKey()); obj.pushKV("keypoololdest", spk_man->GetOldestKeyPoolTime()); - obj.pushKV("keypoolsize", (int64_t)spk_man->KeypoolCountExternalKeys()); - obj.pushKV("keypoolsize_hd_internal", (int64_t)(spk_man->KeypoolCountInternalKeys())); } + obj.pushKV("keypoolsize", (int64_t)pwallet->KeypoolCountExternalKeys()); + obj.pushKV("keypoolsize_hd_internal", (int64_t)(pwallet->KeypoolCountInternalKeys())); obj.pushKV("keys_left", pwallet->nKeysLeftSinceAutoBackup); if (pwallet->IsCrypted()) obj.pushKV("unlocked_until", pwallet->nRelockTime); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 69a62b07a169..7f609f7ef32e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -649,9 +649,11 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase) // must get current HD chain before EncryptKeys CHDChain hdChainCurrent; - // GetHDChain exist only at legacy.... how to validate it? just do static cast? we don't have any other type yet so may be ok temporary! - if (auto spk_man = GetLegacyScriptPubKeyMan()) { - spk_man->GetHDChain(hdChainCurrent); + for (const auto& spk_man_pair : m_spk_managers) { + auto spk_man = spk_man_pair.second.get(); + LegacyScriptPubKeyMan *spk_man_legacy = dynamic_cast(spk_man); + if (spk_man_legacy != nullptr) spk_man_legacy->GetHDChain(hdChainCurrent); + if (!spk_man->Encrypt(_vMasterKey, encrypted_batch)) { encrypted_batch->TxnAbort(); delete encrypted_batch; @@ -661,10 +663,10 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase) assert(false); } if (!hdChainCurrent.IsNull()) { - assert(spk_man->EncryptHDChain(_vMasterKey)); + assert(spk_man_legacy->EncryptHDChain(_vMasterKey)); CHDChain hdChainCrypted; - assert(spk_man->GetHDChain(hdChainCrypted)); + assert(spk_man_legacy->GetHDChain(hdChainCrypted)); DBG( tfm::format(std::cout, "EncryptWallet -- current seed: '%s'\n", HexStr(hdChainCurrent.GetSeed())); @@ -675,7 +677,7 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase) assert(hdChainCurrent.GetID() == hdChainCrypted.GetID()); assert(hdChainCurrent.GetSeedHash() != hdChainCrypted.GetSeedHash()); - assert(spk_man->SetCryptedHDChain(*encrypted_batch, hdChainCrypted, false)); + assert(spk_man_legacy->SetCryptedHDChain(*encrypted_batch, hdChainCrypted, false)); } } @@ -4034,6 +4036,17 @@ bool CWallet::GetNewChangeDestination(CTxDestination& dest, std::string& error) return true; } +int64_t CWallet::GetOldestKeyPoolTime() const +{ + LOCK(cs_wallet); + int64_t oldestKey = std::numeric_limits::max(); + for (const auto& spk_man_pair : m_spk_managers) { + oldestKey = std::min(oldestKey, spk_man_pair.second->GetOldestKeyPoolTime()); + } + + return oldestKey; +} + void CWallet::MarkDestinationsDirty(const std::set& destinations) { for (auto& entry : mapWallet) { CWalletTx& wtx = entry.second; From 0065d7fefd23b7767174f96a9be11f311afbbef6 Mon Sep 17 00:00:00 2001 From: Samuel Dobson Date: Thu, 23 Apr 2020 13:57:16 +1200 Subject: [PATCH 04/11] Merge #18671: wallet: Add BlockUntilSyncedToCurrentChain to dumpwallet fa60afc4fb957875bab1c8982d9d9e4999a3814c wallet: Add BlockUntilSyncedToCurrentChain to dumpwallet (MarcoFalke) Pull request description: dumpwallet includes the block hash in the output, so this method depends on the chainstate. According to the developer notes https://github.com/bitcoin/bitcoin/blame/e84a5f000493fe39adb2a5f22b43c3848dcd0a4f/doc/developer-notes.md#L1095 it must include a `BlockUntilSyncedToCurrentChain`. This is a minor fix and does not need backport, I think. It fixes test failures such as https://travis-ci.org/github/bitcoin/bitcoin/jobs/675487097#L2657 , which can only happen in master because the test was not backported. ACKs for top commit: promag: Code review ACK fa60afc4fb957875bab1c8982d9d9e4999a3814c. ryanofsky: Code review ACK fa60afc4fb957875bab1c8982d9d9e4999a3814c meshcollider: utACK fa60afc4fb957875bab1c8982d9d9e4999a3814c Tree-SHA512: 8df70b06b226b2cdf880dec9264adb72d66fd81b09b404fd1665a79e5f5236d26122eebf15df00fe71ee292b5c91b2dc23a0a42b2aa50a8d690604b23832723f --- src/wallet/rpcdump.cpp | 28 ++++++++++++++++------------ src/wallet/test/wallet_tests.cpp | 16 ++++++++++------ 2 files changed, 26 insertions(+), 18 deletions(-) diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 0ff6074e6317..0f6c551795c4 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -908,15 +908,19 @@ UniValue dumpwallet(const JSONRPCRequest& request) }, }.Check(request); - std::shared_ptr const wallet = GetWalletForJSONRPCRequest(request); - if (!wallet) return NullUniValue; - const CWallet* const pwallet = wallet.get(); + std::shared_ptr const pwallet = GetWalletForJSONRPCRequest(request); + if (!pwallet) return NullUniValue; - LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*wallet); + CWallet& wallet = *pwallet; + LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(wallet); - LOCK2(pwallet->cs_wallet, spk_man.cs_KeyStore); + // Make sure the results are valid at least up to the most recent block + // the user could have gotten from another RPC command prior to now + wallet.BlockUntilSyncedToCurrentChain(); - EnsureWalletIsUnlocked(pwallet); + LOCK2(wallet.cs_wallet, spk_man.cs_KeyStore); + + EnsureWalletIsUnlocked(&wallet); fs::path filepath = request.params[0].get_str(); filepath = fs::absolute(filepath); @@ -937,7 +941,7 @@ UniValue dumpwallet(const JSONRPCRequest& request) std::map mapKeyBirth; const std::map& mapKeyPool = spk_man.GetAllReserveKeys(); - pwallet->GetKeyBirthTimes(mapKeyBirth); + wallet.GetKeyBirthTimes(mapKeyBirth); std::set scripts = spk_man.GetCScripts(); @@ -952,16 +956,16 @@ UniValue dumpwallet(const JSONRPCRequest& request) // produce output file << strprintf("# Wallet dump created by Dash Core %s\n", CLIENT_BUILD); file << strprintf("# * Created on %s\n", FormatISO8601DateTime(GetTime())); - file << strprintf("# * Best block at time of backup was %i (%s),\n", pwallet->GetLastBlockHeight(), pwallet->GetLastBlockHash().ToString()); + file << strprintf("# * Best block at time of backup was %i (%s),\n", wallet.GetLastBlockHeight(), wallet.GetLastBlockHash().ToString()); int64_t block_time = 0; - CHECK_NONFATAL(pwallet->chain().findBlock(pwallet->GetLastBlockHash(), FoundBlock().time(block_time))); + CHECK_NONFATAL(wallet.chain().findBlock(wallet.GetLastBlockHash(), FoundBlock().time(block_time))); file << strprintf("# mined on %s\n", FormatISO8601DateTime(block_time)); file << "\n"; UniValue obj(UniValue::VOBJ); obj.pushKV("dashcoreversion", CLIENT_BUILD); - obj.pushKV("lastblockheight", pwallet->GetLastBlockHeight()); - obj.pushKV("lastblockhash", pwallet->GetLastBlockHash().ToString()); + obj.pushKV("lastblockheight", wallet.GetLastBlockHeight()); + obj.pushKV("lastblockhash", wallet.GetLastBlockHash().ToString()); obj.pushKV("lastblocktime", block_time); // add the base58check encoded extended master if the wallet uses HD @@ -1012,7 +1016,7 @@ UniValue dumpwallet(const JSONRPCRequest& request) CKey key; if (spk_man.GetKey(keyid, key)) { file << strprintf("%s %s ", EncodeSecret(key), strTime); - const auto* address_book_entry = pwallet->FindAddressBookEntry(pkhash); + const auto* address_book_entry = wallet.FindAddressBookEntry(pkhash); if (address_book_entry) { file << strprintf("label=%s", EncodeDumpString(address_book_entry->GetLabel())); } else if (mapKeyPool.count(keyid)) { diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index dc72bee749f1..c37d45a7dee9 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -264,17 +264,21 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup) // Import key into wallet and call dumpwallet to create backup file. { std::shared_ptr wallet = std::make_shared(m_node.chain.get(), "", CreateDummyWalletDatabase()); - auto spk_man = wallet->GetOrCreateLegacyScriptPubKeyMan(); - LOCK2(wallet->cs_wallet, spk_man->cs_KeyStore); - spk_man->mapKeyMetadata[coinbaseKey.GetPubKey().GetID()].nCreateTime = KEY_TIME; - spk_man->AddKeyPubKey(coinbaseKey, coinbaseKey.GetPubKey()); + { + auto spk_man = wallet->GetOrCreateLegacyScriptPubKeyMan(); + LOCK2(wallet->cs_wallet, spk_man->cs_KeyStore); + spk_man->mapKeyMetadata[coinbaseKey.GetPubKey().GetID()].nCreateTime = KEY_TIME; + spk_man->AddKeyPubKey(coinbaseKey, coinbaseKey.GetPubKey()); + AddWallet(wallet); + wallet->SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash()); + } CoreContext context{m_node}; JSONRPCRequest request(context); + request.params.setArray(); request.params.push_back(backup_file); - AddWallet(wallet); - wallet->SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash()); + ::dumpwallet(request); RemoveWallet(wallet, std::nullopt); } From f6f9b9851f3ab05e7662ccc9545246dcc7da9183 Mon Sep 17 00:00:00 2001 From: Samuel Dobson Date: Sat, 18 Apr 2020 21:37:20 +1200 Subject: [PATCH 05/11] Merge #17219: wallet: allow transaction without change if keypool is empty 92bcd70808b9cac56b184903aa6d37baf9641b37 [wallet] allow transaction without change if keypool is empty (Sjors Provoost) 709f8685ac37510aa145ac259753583c82280038 [wallet] CreateTransaction: simplify change address check (Sjors Provoost) 5efc25f9638866941028454cfa9bae27f1519cb4 [wallet] translate "Keypool ran out" message (Sjors Provoost) Pull request description: Extracted from #16944 First this PR simplifies the check when generating a change address, by dropping `CanGetAddresses` and just letting `reservedest.GetReservedDestination` do this check. Second, when the keypool is empty, instead of immediately giving up, we create a dummy change address and pass that to coin selection. If we didn't need the change address (e.g. when spending the entire balance), then it's all good. If we did need a change address, we throw the original error. ACKs for top commit: fjahr: Code review ACK 92bcd70808b9cac56b184903aa6d37baf9641b37 jonasschnelli: utACK 92bcd70808b9cac56b184903aa6d37baf9641b37 achow101: ACK 92bcd70808b9cac56b184903aa6d37baf9641b37 meshcollider: Code review ACK 92bcd70808b9cac56b184903aa6d37baf9641b37 Tree-SHA512: 07b8c8251f57061c58a85ebf0359be63583c23bac7a2c4cefdc14820c0cdebcc90a2bb218e5ede0db11d1e204cda149e056dfd18614642070b3d56efe2735006 --- src/wallet/scriptpubkeyman.cpp | 2 +- src/wallet/wallet.cpp | 23 ++++++----- test/functional/rpc_fundrawtransaction.py | 14 +++++-- test/functional/wallet_keypool_hd.py | 48 ++++++++++++++++++++++- 4 files changed, 70 insertions(+), 17 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 7260aae0b733..0fe85bff7a63 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -23,7 +23,7 @@ bool LegacyScriptPubKeyMan::GetNewDestination(CTxDestination& dest, std::string& // Generate a new key that is added to wallet CPubKey new_key; if (!GetKeyFromPool(new_key, false)) { - error = "Error: Keypool ran out, please call keypoolrefill first"; + error = _("Error: Keypool ran out, please call keypoolrefill first").translated; return false; } //LearnRelatedScripts(new_key); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 7f609f7ef32e..1eea4a6f00fe 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3461,20 +3461,14 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CTransac // rediscover unknown transactions that were written with keys of ours to recover // post-backup change. - // Reserve a new key pair from key pool - if (!CanGetAddresses(true)) { - error = _("Can't generate a change-address key. No keys in the internal keypool and can't generate any keys."); - return false; - } + // Reserve a new key pair from key pool. If it fails, provide a dummy + // destination in case we don't need change. CTxDestination dest; - bool ret = reservedest.GetReservedDestination(dest, true); - if (!ret) - { - error = _("Keypool ran out, please call keypoolrefill first"); - return false; + if (!reservedest.GetReservedDestination(dest, true)) { + error = _("Transaction needs a change address, but we can't generate it. Please call keypoolrefill first."); } - scriptChange = GetScriptForDestination(dest); + assert(!dest.empty() || scriptChange.empty()); } nFeeRet = 0; @@ -3724,6 +3718,11 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CTransac error = _("Exceeded max tries."); return false; } + + // Give up if change keypool ran out and we failed to find a solution without change: + if (scriptChange.empty() && nChangePosInOut != -1) { + return false; + } } // Make sure change position was updated one way or another @@ -4028,7 +4027,7 @@ bool CWallet::GetNewChangeDestination(CTxDestination& dest, std::string& error) ReserveDestination reservedest(this); if (!reservedest.GetReservedDestination(dest, true)) { - error = "Error: Keypool ran out, please call keypoolrefill first"; + error = _("Error: Keypool ran out, please call keypoolrefill first").translated; return false; } diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index 4b95562ad8e4..d61a561c5c06 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -492,12 +492,20 @@ def test_locked_wallet(self): # Drain the keypool. self.nodes[1].getnewaddress() - inputs = [] - outputs = {self.nodes[0].getnewaddress():1.1} + inputs = self.nodes[1].listunspent() + # Deduce fee to produce a changeless transaction + # bnb doesn't work same way as in bitcoin, so, `value` is also calculated by different way + value = sum(input_it["amount"] for input_it in inputs) - Decimal("0.00002200") + outputs = {self.nodes[0].getnewaddress():value} rawtx = self.nodes[1].createrawtransaction(inputs, outputs) + # fund a transaction that does not require a new key for the change output + self.nodes[1].fundrawtransaction(rawtx) + # fund a transaction that requires a new key for the change output # creating the key must be impossible because the wallet is locked - assert_raises_rpc_error(-4, "Keypool ran out, please call keypoolrefill first", self.nodes[1].fundrawtransaction, rawtx) + outputs = {self.nodes[0].getnewaddress():1.1} + rawtx = self.nodes[1].createrawtransaction(inputs, outputs) + assert_raises_rpc_error(-4, "Transaction needs a change address, but we can't generate it. Please call keypoolrefill first.", self.nodes[1].fundrawtransaction, rawtx) # Refill the keypool. self.nodes[1].walletpassphrase("test", 100) diff --git a/test/functional/wallet_keypool_hd.py b/test/functional/wallet_keypool_hd.py index d22effdeafb6..0e45cdb52a09 100755 --- a/test/functional/wallet_keypool_hd.py +++ b/test/functional/wallet_keypool_hd.py @@ -8,6 +8,7 @@ # Add python-bitcoinrpc to module search path: import time +from decimal import Decimal from test_framework.authproxy import JSONRPCException from test_framework.test_framework import BitcoinTestFramework @@ -16,7 +17,6 @@ class KeyPoolTest(BitcoinTestFramework): def set_test_params(self): - self.setup_clean_chain = True self.num_nodes = 1 self.extra_args = [['-usehd=1']] @@ -101,5 +101,51 @@ def run_test(self): assert_equal(wi['keypoolsize_hd_internal'], 100) assert_equal(wi['keypoolsize'], 100) + # create a blank wallet + nodes[0].createwallet(wallet_name='w2', blank=True) + w2 = nodes[0].get_wallet_rpc('w2') + + # refer to initial wallet as w1 + w1 = nodes[0].get_wallet_rpc(self.default_wallet_name) + + # import private key and fund it + address = addr.pop() + privkey = w1.dumpprivkey(address) + res = w2.importmulti([{'scriptPubKey': {'address': address}, 'keys': [privkey], 'timestamp': 'now'}]) + assert_equal(res[0]['success'], True) + w1.walletpassphrase('test', 100) + + res = w1.sendtoaddress(address=address, amount=0.00010000) + nodes[0].generate(1) + destination = addr.pop() + + # Using a fee rate (10 sat / byte) well above the minimum relay rate + # creating a 5,000 sat transaction with change should not be possible + assert_raises_rpc_error(-4, "Transaction needs a change address, but we can't generate it. Please call keypoolrefill first.", w2.walletcreatefundedpsbt, inputs=[], outputs=[{addr.pop(): 0.00005000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.000010}) + + # creating a 10,000 sat transaction without change, with a manual input, should still be possible + res = w2.walletcreatefundedpsbt(inputs=w2.listunspent(), outputs=[{destination: 0.00010000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.000010}) + assert_equal("psbt" in res, True) + + # creating a 10,000 sat transaction without change should still be possible + res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00010000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.000010}) + assert_equal("psbt" in res, True) + # should work without subtractFeeFromOutputs if the exact fee is subtracted from the amount + res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00008900}], options={"feeRate": 0.000010}) + assert_equal("psbt" in res, True) + + # dust change should be removed + res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00008800}], options={"feeRate": 0.000010}) + assert_equal("psbt" in res, True) + + # create a transaction without change at the maximum fee rate, such that the output is still spendable: + res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00010000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.00008824}) + assert_equal("psbt" in res, True) + assert_equal(res["fee"], Decimal("0.00001685")) + + # creating a 10,000 sat transaction with a manual change address should be possible + res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00010000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.000010, "changeAddress": addr.pop()}) + assert_equal("psbt" in res, True) + if __name__ == '__main__': KeyPoolTest().main() From 849b5ed2a4d183053f6e38496fbbaa3b5970f64c Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Wed, 6 May 2020 14:19:26 +0200 Subject: [PATCH 06/11] Merge #18853: wallet: Fix typo in assert that is compile-time true fa47cf9d95dc2c2822fc96df16f179176935bf96 wallet: Fix typo in assert that is compile-time true (MarcoFalke) Pull request description: Commit 92bcd70808b9cac56b184903aa6d37baf9641b37 presumably added a check that a `dest` of type `CNoDestination` implies an empty `scriptChange`. However, it accidentally checked for `boost::variant::empty`, which always returns false: https://www.boost.org/doc/libs/1_72_0/doc/html/boost/variant.html#id-1_3_46_5_4_1_1_16_2-bb ACKs for top commit: Sjors: utACK fa47cf9d95dc2c2822fc96df16f179176935bf96 Tree-SHA512: 9626b1e2947039853703932a362c2ee204e002d3344856eb93eef0e0f833401336f2dfa80fd43b83c8ec6eac624e6302aee771fb67aec436ba6483be02b8d615 --- src/test/script_standard_tests.cpp | 6 ++++++ src/wallet/wallet.cpp | 7 +++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/test/script_standard_tests.cpp b/src/test/script_standard_tests.cpp index cf0899cdc7d9..2ab7f72c5cef 100644 --- a/src/test/script_standard_tests.cpp +++ b/src/test/script_standard_tests.cpp @@ -12,6 +12,12 @@ BOOST_FIXTURE_TEST_SUITE(script_standard_tests, BasicTestingSetup) +BOOST_AUTO_TEST_CASE(dest_default_is_no_dest) +{ + CTxDestination dest; + BOOST_CHECK(!IsValidDestination(dest)); +} + BOOST_AUTO_TEST_CASE(script_standard_Solver_success) { CKey keys[3]; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 1eea4a6f00fe..c291e5278126 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3468,7 +3468,10 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CTransac error = _("Transaction needs a change address, but we can't generate it. Please call keypoolrefill first."); } scriptChange = GetScriptForDestination(dest); - assert(!dest.empty() || scriptChange.empty()); + // A valid destination implies a change script (and + // vice-versa). An empty change script will abort later, if the + // change keypool ran out, but change is required. + CHECK_NONFATAL(IsValidDestination(dest) != scriptChange.empty()); } nFeeRet = 0; @@ -3719,7 +3722,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CTransac return false; } - // Give up if change keypool ran out and we failed to find a solution without change: + // Give up if change keypool ran out and change is required if (scriptChange.empty() && nChangePosInOut != -1) { return false; } From d11a933c5ba4d77611ccf5e3bac84fd2fa77a503 Mon Sep 17 00:00:00 2001 From: Samuel Dobson Date: Wed, 6 May 2020 11:14:32 +1200 Subject: [PATCH 07/11] Merge #9381: Remove CWalletTx merging logic from AddToWallet 28b112e9bd3fd1181c0720306051ba7efca8b436 Get rid of BindWallet (Russell Yanofsky) d002f9d15d938e78360ad906f2d74a249c7e923e Disable CWalletTx copy constructor (Russell Yanofsky) 65b9d8f8ddb5a838454efc8bdd6576f0deb65f6d Avoid copying CWalletTx in LoadToWallet (Russell Yanofsky) bd2fbc7cdbec46400341209f4cb7e69e5b2cee19 Get rid of unneeded CWalletTx::Init parameter (Russell Yanofsky) 2b9cba206594bfbcefcef0c88a0bf793819643bd Remove CWalletTx merging logic from AddToWallet (Russell Yanofsky) Pull request description: This is a pure refactoring, no behavior is changing. Instead of AddToWallet taking a temporary CWalletTx object and then potentially merging it with a pre-existing CWalletTx, have it take a callback so callers can update the pre-existing CWalletTx directly. This makes AddToWallet simpler because now it is only has to be concerned with saving CWalletTx objects and not merging them. This makes AddToWallet calls clearer because they can now make direct updates to CWalletTx entries without having to make temporary objects and then worry about how they will be merged. Motivation for this change came from the bumpfee PR #8456 where we wanted to be able to call AddToWallet to make a simple update to an existing transaction, but were reluctant to, because the existing CWalletTx merging logic did not apply and seemed dangerous try to update as part of that PR. After this refactoring, the bumpfee PR could call AddToWallet safely instead of implementing a duplicate AddToWallet function. This also allows getting rid of the CWalletTx copy constructor to prevent unintentional copying. ACKs for top commit: MarcoFalke: Anyway, re-ACK 28b112e9bd3fd1181c0720306051ba7efca8b436 Tree-SHA512: 528dd088714472a237500b200f4433db850bdb7fc29c5e5d81cae48072061dfb967f7c37edd90b33f24901239f9be982988547c1f8c80abc25fb243fbf7330ef --- src/wallet/rpcdump.cpp | 7 +- src/wallet/rpcwallet.cpp | 2 +- src/wallet/test/coinselector_tests.cpp | 9 +-- src/wallet/test/psbt_wallet_tests.cpp | 6 +- src/wallet/test/wallet_tests.cpp | 17 ++--- src/wallet/wallet.cpp | 98 ++++++++++++-------------- src/wallet/wallet.h | 37 ++++++---- src/wallet/walletdb.cpp | 68 +++++++++--------- src/wallet/walletdb.h | 2 +- 9 files changed, 119 insertions(+), 127 deletions(-) diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 0f6c551795c4..bdc8a81e9be5 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -302,7 +302,6 @@ UniValue importprunedfunds(const JSONRPCRequest& request) throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed. Make sure the tx has at least one input."); } uint256 hashTx = tx.GetHash(); - CWalletTx wtx(pwallet, MakeTransactionRef(std::move(tx))); CDataStream ssMB(ParseHexV(request.params[1], "proof"), SER_NETWORK, PROTOCOL_VERSION); CMerkleBlock merkleBlock; @@ -329,10 +328,10 @@ UniValue importprunedfunds(const JSONRPCRequest& request) unsigned int txnIndex = vIndex[it - vMatch.begin()]; CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, height, merkleBlock.header.GetHash(), txnIndex); - wtx.m_confirm = confirm; - if (pwallet->IsMine(*wtx.tx)) { - pwallet->AddToWallet(wtx, false); + CTransactionRef tx_ref = MakeTransactionRef(tx); + if (pwallet->IsMine(*tx_ref)) { + pwallet->AddToWallet(std::move(tx_ref), confirm); return NullUniValue; } diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 218929c9ec53..6be1ae9be650 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -1628,7 +1628,7 @@ static UniValue listsinceblock(const JSONRPCRequest& request) UniValue transactions(UniValue::VARR); for (const std::pair& pairWtx : pwallet->mapWallet) { - CWalletTx tx = pairWtx.second; + const CWalletTx& tx = pairWtx.second; if (depth == -1 || abs(tx.GetDepthInMainChain()) < depth) { ListTransactions(pwallet, tx, 0, true, transactions, filter, nullptr /* filter_label */); diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 8cbf8dbdfd17..80db47319b36 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -25,8 +25,6 @@ BOOST_FIXTURE_TEST_SUITE(coinselector_tests, WalletTestingSetup) // we repeat those tests this many times and only complain if all iterations of the test fail #define RANDOM_REPEATS 5 -std::vector> wtxn; - typedef std::set CoinSet; static std::vector vCoins; @@ -76,16 +74,14 @@ static void add_coin(CWallet& wallet, const CAmount& nValue, int nAge = 6*24, bo // so stop vin being empty, and cache a non-zero Debit to fake out IsFromMe() tx.vin.resize(1); } - std::unique_ptr wtx = std::make_unique(&wallet, MakeTransactionRef(std::move(tx))); + CWalletTx* wtx = wallet.AddToWallet(MakeTransactionRef(std::move(tx)), /* confirm= */ {}); if (fIsFromMe) { wtx->m_amounts[CWalletTx::DEBIT].Set(ISMINE_SPENDABLE, 1); wtx->m_is_cache_empty = false; } - COutput output(wtx.get(), nInput, nAge, true /* spendable */, true /* solvable */, true /* safe */); + COutput output(wtx, nInput, nAge, true /* spendable */, true /* solvable */, true /* safe */); vCoins.push_back(output); - wallet.AddToWallet(*wtx.get()); - wtxn.emplace_back(std::move(wtx)); } static void add_coin(const CAmount& nValue, int nAge = 6*24, bool fIsFromMe = false, int nInput=0, bool spendable = false) { @@ -95,7 +91,6 @@ static void add_coin(const CAmount& nValue, int nAge = 6*24, bool fIsFromMe = fa static void empty_wallet(void) { vCoins.clear(); - wtxn.clear(); balance = 0; } diff --git a/src/wallet/test/psbt_wallet_tests.cpp b/src/wallet/test/psbt_wallet_tests.cpp index e425be6dbcd5..9680da50708c 100644 --- a/src/wallet/test/psbt_wallet_tests.cpp +++ b/src/wallet/test/psbt_wallet_tests.cpp @@ -23,14 +23,12 @@ BOOST_AUTO_TEST_CASE(psbt_updater_test) CDataStream s_prev_tx1(ParseHex("0200000000010158e87a21b56daf0c23be8e7070456c336f7cbaa5c8757924f545887bb2abdd7501000000171600145f275f436b09a8cc9a2eb2a2f528485c68a56323feffffff02d8231f1b0100000017a914aed962d6654f9a2b36608eb9d64d2b260db4f1118700c2eb0b0000000017a914b7f5faf40e3d40a5a459b1db3535f2b72fa921e88702483045022100a22edcc6e5bc511af4cc4ae0de0fcd75c7e04d8c1c3a8aa9d820ed4b967384ec02200642963597b9b1bc22c75e9f3e117284a962188bf5e8a74c895089046a20ad770121035509a48eb623e10aace8bfd0212fdb8a8e5af3c94b0b133b95e114cab89e4f7965000000"), SER_NETWORK, PROTOCOL_VERSION); CTransactionRef prev_tx1; s_prev_tx1 >> prev_tx1; - CWalletTx prev_wtx1(&m_wallet, prev_tx1); - m_wallet.mapWallet.emplace(prev_wtx1.GetHash(), std::move(prev_wtx1)); + m_wallet.mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(prev_tx1->GetHash()), std::forward_as_tuple(&m_wallet, prev_tx1)); CDataStream s_prev_tx2(ParseHex("0200000001aad73931018bd25f84ae400b68848be09db706eac2ac18298babee71ab656f8b0000000048473044022058f6fc7c6a33e1b31548d481c826c015bd30135aad42cd67790dab66d2ad243b02204a1ced2604c6735b6393e5b41691dd78b00f0c5942fb9f751856faa938157dba01feffffff0280f0fa020000000017a9140fb9463421696b82c833af241c78c17ddbde493487d0f20a270100000017a91429ca74f8a08f81999428185c97b5d852e4063f618765000000"), SER_NETWORK, PROTOCOL_VERSION); CTransactionRef prev_tx2; s_prev_tx2 >> prev_tx2; - CWalletTx prev_wtx2(&m_wallet, prev_tx2); - m_wallet.mapWallet.emplace(prev_wtx2.GetHash(), std::move(prev_wtx2)); + m_wallet.mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(prev_tx2->GetHash()), std::forward_as_tuple(&m_wallet, prev_tx2)); // Add scripts CScript rs1; diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index c37d45a7dee9..dbe1350c0153 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -419,6 +419,7 @@ BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup) static int64_t AddTx(ChainstateManager& chainman, CWallet& wallet, uint32_t lockTime, int64_t mockTime, int64_t blockTime) { CMutableTransaction tx; + CWalletTx::Confirmation confirm; tx.nLockTime = lockTime; SetMockTime(mockTime); CBlockIndex* block = nullptr; @@ -430,23 +431,15 @@ static int64_t AddTx(ChainstateManager& chainman, CWallet& wallet, uint32_t lock block = inserted.first->second; block->nTime = blockTime; block->phashBlock = &hash; + confirm = {CWalletTx::Status::CONFIRMED, block->nHeight, hash, 0}; } - CWalletTx wtx(&wallet, MakeTransactionRef(tx)); - LOCK(wallet.cs_wallet); // If transaction is already in map, to avoid inconsistencies, unconfirmation // is needed before confirm again with different block. - std::map::iterator it = wallet.mapWallet.find(wtx.GetHash()); - if (it != wallet.mapWallet.end()) { + return wallet.AddToWallet(MakeTransactionRef(tx), confirm, [&](CWalletTx& wtx, bool /* new_tx */) { wtx.setUnconfirmed(); - wallet.AddToWallet(wtx); - } - if (block) { - CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, block->nHeight, block->GetBlockHash(), 0); - wtx.m_confirm = confirm; - } - wallet.AddToWallet(wtx); - return wallet.mapWallet.at(wtx.GetHash()).nTimeSmart; + return true; + })->nTimeSmart; } // Simple test to verify assignment of CWalletTx::nSmartTime value. Could be diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c291e5278126..8e31c449d7c1 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -848,19 +848,19 @@ bool CWallet::IsSpentKey(const uint256& hash, unsigned int n) const return false; } -bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose) +CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const CWalletTx::Confirmation& confirm, const UpdateWalletTxFn& update_wtx, bool fFlushOnClose) { LOCK(cs_wallet); WalletBatch batch(GetDatabase(), fFlushOnClose); - uint256 hash = wtxIn.GetHash(); + uint256 hash = tx->GetHash(); if (IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE)) { // Mark used destinations std::set tx_destinations; - for (const CTxIn& txin : wtxIn.tx->vin) { + for (const CTxIn& txin : tx->vin) { const COutPoint& op = txin.prevout; SetSpentKeyState(batch, op.hash, op.n, true, tx_destinations); } @@ -869,11 +869,12 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose) } // Inserts only if not already there, returns tx inserted or tx found - std::pair::iterator, bool> ret = mapWallet.insert(std::make_pair(hash, wtxIn)); + auto ret = mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(hash), std::forward_as_tuple(this, tx)); CWalletTx& wtx = (*ret.first).second; - wtx.BindWallet(this); bool fInsertedNew = ret.second; + bool fUpdated = update_wtx && update_wtx(wtx, fInsertedNew); if (fInsertedNew) { + wtx.m_confirm = confirm; wtx.nTimeReceived = GetTime(); wtx.nOrderPos = IncOrderPosNext(&batch); wtx.m_it_wtxOrdered = wtxOrdered.insert(std::make_pair(wtx.nOrderPos, &wtx)); @@ -891,24 +892,18 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose) } } - bool fUpdated = false; if (!fInsertedNew) { - if (wtxIn.m_confirm.status != wtx.m_confirm.status) { - wtx.m_confirm.status = wtxIn.m_confirm.status; - wtx.m_confirm.nIndex = wtxIn.m_confirm.nIndex; - wtx.m_confirm.hashBlock = wtxIn.m_confirm.hashBlock; - wtx.m_confirm.block_height = wtxIn.m_confirm.block_height; + if (confirm.status != wtx.m_confirm.status) { + wtx.m_confirm.status = confirm.status; + wtx.m_confirm.nIndex = confirm.nIndex; + wtx.m_confirm.hashBlock = confirm.hashBlock; + wtx.m_confirm.block_height = confirm.block_height; fUpdated = true; } else { - assert(wtx.m_confirm.nIndex == wtxIn.m_confirm.nIndex); - assert(wtx.m_confirm.hashBlock == wtxIn.m_confirm.hashBlock); - assert(wtx.m_confirm.block_height == wtxIn.m_confirm.block_height); - } - if (wtxIn.fFromMe && wtxIn.fFromMe != wtx.fFromMe) - { - wtx.fFromMe = wtxIn.fFromMe; - fUpdated = true; + assert(wtx.m_confirm.nIndex == confirm.nIndex); + assert(wtx.m_confirm.hashBlock == confirm.hashBlock); + assert(wtx.m_confirm.block_height == confirm.block_height); } auto mnList = deterministicMNManager->GetListAtChainTip(); @@ -924,12 +919,12 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose) } //// debug print - WalletLogPrintf("AddToWallet %s %s%s\n", wtxIn.GetHash().ToString(), (fInsertedNew ? "new" : ""), (fUpdated ? "update" : "")); + WalletLogPrintf("AddToWallet %s %s%s\n", hash.ToString(), (fInsertedNew ? "new" : ""), (fUpdated ? "update" : "")); // Write to disk if (fInsertedNew || fUpdated) if (!batch.WriteTx(wtx)) - return false; + return nullptr; // Break debit/credit balance caches: wtx.MarkDirty(); @@ -943,7 +938,7 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose) if (!strCmd.empty()) { - ReplaceAll(strCmd, "%s", wtxIn.GetHash().GetHex()); + ReplaceAll(strCmd, "%s", hash.GetHex()); #ifndef WIN32 // Substituting the wallet name isn't currently supported on windows // because windows shell escaping has not been implemented yet: @@ -960,36 +955,37 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose) fAnonymizableTallyCached = false; fAnonymizableTallyCachedNonDenom = false; - return true; + return &wtx; } -void CWallet::LoadToWallet(CWalletTx& wtxIn) +bool CWallet::LoadToWallet(const uint256& hash, const UpdateWalletTxFn& fill_wtx) { + const auto& ins = mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(hash), std::forward_as_tuple(this, nullptr)); + CWalletTx& wtx = ins.first->second; + if (!fill_wtx(wtx, ins.second)) { + return false; + } // If wallet doesn't have a chain (e.g dash-wallet), don't bother to update txn. if (HaveChain()) { bool active; int height; - if (chain().findBlock(wtxIn.m_confirm.hashBlock, FoundBlock().inActiveChain(active).height(height)) && active) { + if (chain().findBlock(wtx.m_confirm.hashBlock, FoundBlock().inActiveChain(active).height(height)) && active) { // Update cached block height variable since it not stored in the // serialized transaction. - wtxIn.m_confirm.block_height = height; - } else if (wtxIn.isConflicted() || wtxIn.isConfirmed()) { + wtx.m_confirm.block_height = height; + } else if (wtx.isConflicted() || wtx.isConfirmed()) { // If tx block (or conflicting block) was reorged out of chain // while the wallet was shutdown, change tx status to UNCONFIRMED // and reset block height, hash, and index. ABANDONED tx don't have // associated blocks and don't need to be updated. The case where a // transaction was reorged out while online and then reconfirmed // while offline is covered by the rescan logic. - wtxIn.setUnconfirmed(); - wtxIn.m_confirm.hashBlock = uint256(); - wtxIn.m_confirm.block_height = 0; - wtxIn.m_confirm.nIndex = 0; + wtx.setUnconfirmed(); + wtx.m_confirm.hashBlock = uint256(); + wtx.m_confirm.block_height = 0; + wtx.m_confirm.nIndex = 0; } } - uint256 hash = wtxIn.GetHash(); - const auto& ins = mapWallet.emplace(hash, wtxIn); - CWalletTx& wtx = ins.first->second; - wtx.BindWallet(this); if (/* insertion took place */ ins.second) { wtx.m_it_wtxOrdered = wtxOrdered.insert(std::make_pair(wtx.nOrderPos, &wtx)); } @@ -1003,6 +999,7 @@ void CWallet::LoadToWallet(CWalletTx& wtxIn) } } } + return true; } bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, CWalletTx::Confirmation confirm, WalletBatch& batch, bool fUpdate) @@ -1048,13 +1045,9 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, CWalletTx::Co } } - CWalletTx wtx(this, ptx); - // Block disconnection override an abandoned tx as unconfirmed // which means user may have to call abandontransaction again - wtx.m_confirm = confirm; - - return AddToWallet(wtx, false); + return AddToWallet(MakeTransactionRef(tx), confirm, /* update_wtx= */ nullptr, /* fFlushOnClose= */ false); } } return false; @@ -3771,32 +3764,34 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CTransac void CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector> orderForm) { LOCK(cs_wallet); + WalletLogPrintf("CommitTransaction:\n%s", tx->ToString()); /* Continued */ - CWalletTx wtxNew(this, std::move(tx)); - wtxNew.mapValue = std::move(mapValue); - wtxNew.vOrderForm = std::move(orderForm); - wtxNew.fTimeReceivedIsTxTime = true; - wtxNew.fFromMe = true; - - WalletLogPrintf("CommitTransaction:\n%s", wtxNew.tx->ToString()); /* Continued */ // Add tx to wallet, because if it has change it's also ours, // otherwise just for transaction history. - AddToWallet(wtxNew); + AddToWallet(tx, {}, [&](CWalletTx& wtx, bool new_tx) { + CHECK_NONFATAL(wtx.mapValue.empty()); + CHECK_NONFATAL(wtx.vOrderForm.empty()); + wtx.mapValue = std::move(mapValue); + wtx.vOrderForm = std::move(orderForm); + wtx.fTimeReceivedIsTxTime = true; + wtx.fFromMe = true; + return true; + }); // Notify that old coins are spent std::set updated_hahes; - for (const CTxIn& txin : wtxNew.tx->vin){ + for (const CTxIn& txin : tx->vin){ // notify only once if(updated_hahes.find(txin.prevout.hash) != updated_hahes.end()) continue; CWalletTx &coin = mapWallet.at(txin.prevout.hash); - coin.BindWallet(this); + coin.MarkDirty(); NotifyTransactionChanged(this, txin.prevout.hash, CT_UPDATED); updated_hahes.insert(txin.prevout.hash); } // Get the inserted-CWalletTx from mapWallet so that the // fInMempool flag is cached properly - CWalletTx& wtx = mapWallet.at(wtxNew.GetHash()); + CWalletTx& wtx = mapWallet.at(tx->GetHash()); if (!fBroadcastTransactions) { // Don't submit tx to the mempool @@ -3904,7 +3899,6 @@ DBErrors CWallet::ZapSelectTx(std::vector& vHashIn, std::vector void Unserialize(Stream& s) { - Init(nullptr); + Init(); std::vector dummy_vector1; //!< Used to be vMerkleBranch std::vector dummy_vector2; //!< Used to be vtxPrev @@ -483,12 +483,6 @@ class CWalletTx m_is_cache_empty = true; } - void BindWallet(CWallet *pwalletIn) - { - pwallet = pwalletIn; - MarkDirty(); - } - const CWallet* GetWallet() const { return pwallet; @@ -585,6 +579,12 @@ class CWalletTx const uint256& GetHash() const { return tx->GetHash(); } bool IsCoinBase() const { return tx->IsCoinBase(); } bool IsImmatureCoinBase() const; + + // Disable copying of CWalletTx objects to prevent bugs where instances get + // copied in and out of the mapWallet map, and fields are updated in the + // wrong copy. + CWalletTx(CWalletTx const &) = delete; + void operator=(CWalletTx const &x) = delete; }; struct WalletTxHasher @@ -984,8 +984,17 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati DBErrors ReorderTransactions(); void MarkDirty(); - bool AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose=true); - void LoadToWallet(CWalletTx& wtxIn) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + + //! Callback for updating transaction metadata in mapWallet. + //! + //! @param wtx - reference to mapWallet transaction to update + //! @param new_tx - true if wtx is newly inserted, false if it previously existed + //! + //! @return true if wtx is changed and needs to be saved to disk, otherwise false + using UpdateWalletTxFn = std::function; + + CWalletTx* AddToWallet(CTransactionRef tx, const CWalletTx::Confirmation& confirm, const UpdateWalletTxFn& update_wtx=nullptr, bool fFlushOnClose=true); + bool LoadToWallet(const uint256& hash, const UpdateWalletTxFn& fill_wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void transactionAddedToMempool(const CTransactionRef& tx, int64_t nAcceptTime) override; void blockConnected(const CBlock& block, int height) override; void blockDisconnected(const CBlock& block, int height) override; diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 9b922201bb3f..023a4b94a556 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -264,36 +264,43 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, } else if (strType == DBKeys::TX) { uint256 hash; ssKey >> hash; - CWalletTx wtx(nullptr /* pwallet */, MakeTransactionRef()); - ssValue >> wtx; - if (wtx.GetHash() != hash) - return false; + // LoadToWallet call below creates a new CWalletTx that fill_wtx + // callback fills with transaction metadata. + auto fill_wtx = [&](CWalletTx& wtx, bool new_tx) { + assert(new_tx); + ssValue >> wtx; + if (wtx.GetHash() != hash) + return false; - // Undo serialize changes in 31600 - if (31404 <= wtx.fTimeReceivedIsTxTime && wtx.fTimeReceivedIsTxTime <= 31703) - { - if (!ssValue.empty()) - { - char fTmp; - char fUnused; - std::string unused_string; - ssValue >> fTmp >> fUnused >> unused_string; - strErr = strprintf("LoadWallet() upgrading tx ver=%d %d %s", - wtx.fTimeReceivedIsTxTime, fTmp, hash.ToString()); - wtx.fTimeReceivedIsTxTime = fTmp; - } - else + // Undo serialize changes in 31600 + if (31404 <= wtx.fTimeReceivedIsTxTime && wtx.fTimeReceivedIsTxTime <= 31703) { - strErr = strprintf("LoadWallet() repairing tx ver=%d %s", wtx.fTimeReceivedIsTxTime, hash.ToString()); - wtx.fTimeReceivedIsTxTime = 0; + if (!ssValue.empty()) + { + char fTmp; + char fUnused; + std::string unused_string; + ssValue >> fTmp >> fUnused >> unused_string; + strErr = strprintf("LoadWallet() upgrading tx ver=%d %d %s", + wtx.fTimeReceivedIsTxTime, fTmp, hash.ToString()); + wtx.fTimeReceivedIsTxTime = fTmp; + } + else + { + strErr = strprintf("LoadWallet() repairing tx ver=%d %s", wtx.fTimeReceivedIsTxTime, hash.ToString()); + wtx.fTimeReceivedIsTxTime = 0; + } + wss.vWalletUpgrade.push_back(hash); } - wss.vWalletUpgrade.push_back(hash); - } - if (wtx.nOrderPos == -1) - wss.fAnyUnordered = true; + if (wtx.nOrderPos == -1) + wss.fAnyUnordered = true; - pwallet->LoadToWallet(wtx); + return true; + }; + if (!pwallet->LoadToWallet(hash, fill_wtx)) { + return false; + } } else if (strType == DBKeys::WATCHS) { wss.nWatchKeys++; CScript script; @@ -665,7 +672,7 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) return result; } -DBErrors WalletBatch::FindWalletTx(std::vector& vTxHash, std::vector& vWtx) +DBErrors WalletBatch::FindWalletTx(std::vector& vTxHash, std::list& vWtx) { DBErrors result = DBErrors::LOAD_OK; @@ -703,12 +710,9 @@ DBErrors WalletBatch::FindWalletTx(std::vector& vTxHash, std::vector> hash; - - CWalletTx wtx(nullptr /* pwallet */, MakeTransactionRef()); - ssValue >> wtx; - vTxHash.push_back(hash); - vWtx.push_back(wtx); + vWtx.emplace_back(nullptr /* wallet */, nullptr /* tx */); + ssValue >> vWtx.back(); } } } catch (...) { @@ -723,7 +727,7 @@ DBErrors WalletBatch::ZapSelectTx(std::vector& vTxHashIn, std::vector vTxHash; - std::vector vWtx; + std::list vWtx; DBErrors err = FindWalletTx(vTxHash, vWtx); if (err != DBErrors::LOAD_OK) { return err; diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index 1ebfedfb8d29..b7863fc66060 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -208,7 +208,7 @@ class WalletBatch bool EraseDestData(const std::string &address, const std::string &key); DBErrors LoadWallet(CWallet* pwallet); - DBErrors FindWalletTx(std::vector& vTxHash, std::vector& vWtx); + DBErrors FindWalletTx(std::vector& vTxHash, std::list& vWtx); DBErrors ZapSelectTx(std::vector& vHashIn, std::vector& vHashOut); /* Function to determine if a certain KV/key-type is a key (cryptographical key) type */ static bool IsKeyType(const std::string& strType); From f7f29d72fa5ee94908280bc7afd52ba3da5a932c Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Wed, 24 Oct 2018 13:04:41 -0400 Subject: [PATCH 08/11] follow-up Merge #14559: appveyor: Enable multiwallet tests - adds missing changes for wallet_multiwallet.py test 4dca7d0a98 appveyor: Enable multiwallet test (Chun Kuan Lee) Pull request description: Based on #14320 This PR enable multiwallet test on appveyor. Also re-enable symlink tests on Windows which is available after Windows Vista. I disable these tests in #13964 because I suppose that Windows does not support symlink, but I was wrong. Tree-SHA512: 852cd4dedf36ec9c34aff8926cb34e6a560aea0bb9170c7a2264fc292dbb605622d561568d8df39aeb90d3d2bb700901d218ea7e7c5e21d84827c40d6370b369 --- test/functional/wallet_multiwallet.py | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py index 5ec99a4233e2..cb0dde6625eb 100755 --- a/test/functional/wallet_multiwallet.py +++ b/test/functional/wallet_multiwallet.py @@ -75,9 +75,8 @@ def wallet_file(name): # create symlink to verify wallet directory path can be referenced # through symlink - if os.name != 'nt': - os.mkdir(wallet_dir('w7')) - os.symlink('w7', wallet_dir('w7_symlink')) + os.mkdir(wallet_dir('w7')) + os.symlink('w7', wallet_dir('w7_symlink')) # rename wallet.dat to make sure plain wallet file paths (as opposed to # directory paths) can be loaded @@ -109,8 +108,6 @@ def wallet_file(name): to_load.append('w8') wallet_names = to_create + to_load # Wallet names loaded in the wallet in_wallet_dir += to_load # The loaded wallets are also in the wallet dir - if os.name == 'nt': - wallet_names.remove('w7_symlink') self.start_node(0) for wallet_name in to_create: @@ -146,9 +143,8 @@ def wallet_file(name): self.nodes[0].assert_start_raises_init_error(['-wallet=w8', '-wallet=w8_copy'], exp_stderr, match=ErrorMatch.PARTIAL_REGEX) # should not initialize if wallet file is a symlink - if os.name != 'nt': - os.symlink('w8', wallet_dir('w8_symlink')) - self.nodes[0].assert_start_raises_init_error(['-wallet=w8_symlink'], r'Error: Invalid -wallet path \'w8_symlink\'\. .*', match=ErrorMatch.FULL_REGEX) + os.symlink('w8', wallet_dir('w8_symlink')) + self.nodes[0].assert_start_raises_init_error(['-wallet=w8_symlink'], r'Error: Invalid -wallet path \'w8_symlink\'\. .*', match=ErrorMatch.FULL_REGEX) # should not initialize if the specified walletdir does not exist self.nodes[0].assert_start_raises_init_error(['-walletdir=bad'], 'Error: Specified -walletdir "bad" does not exist') @@ -300,8 +296,7 @@ def wallet_file(name): assert_raises_rpc_error(-4, "BerkeleyDatabase: Can't open database w8_copy (duplicates fileid", self.nodes[0].loadwallet, 'w8_copy') # Fail to load if wallet file is a symlink - if os.name != 'nt': - assert_raises_rpc_error(-4, "Wallet file verification failed. Invalid -wallet path 'w8_symlink'", self.nodes[0].loadwallet, 'w8_symlink') + assert_raises_rpc_error(-4, "Wallet file verification failed. Invalid -wallet path 'w8_symlink'", self.nodes[0].loadwallet, 'w8_symlink') self.log.info("Test dynamic wallet creation.") From 94643ca47db32c02de88736450150ff67e06a372 Mon Sep 17 00:00:00 2001 From: Samuel Dobson Date: Mon, 2 Nov 2020 11:54:49 +1300 Subject: [PATCH 09/11] Merge #20230: wallet: Fix bug when just created encrypted wallet cannot get address bf6855a9096b25aa75bba61b57ee1b2433d49707 wallet: Fix bug when just created encrypted wallet cannot get address (Hennadii Stepanov) Pull request description: Fix https://github.com/bitcoin-core/gui/issues/105 ACKs for top commit: achow101: Tested ACK bf6855a9096b25aa75bba61b57ee1b2433d49707 kristapsk: ACK bf6855a9096b25aa75bba61b57ee1b2433d49707 meshcollider: Tested ACK bf6855a9096b25aa75bba61b57ee1b2433d49707 Tree-SHA512: eca0ab306d7206f2e5db568e83217bd854caac104379f4d8fb261db832d4d6310cbb1eab44ce9b05a5ac2eb5879a623b729752a88810f8370c24518a8d81292d --- src/wallet/wallet.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 8e31c449d7c1..7cfc2e2763ac 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -122,6 +122,7 @@ bool AddWallet(const std::shared_ptr& wallet) assert(::masternodeSync != nullptr && ::coinJoinClientManagers != nullptr); ::coinJoinClientManagers->Add(*wallet); g_wallet_init_interface.InitCoinJoinSettings(*::coinJoinClientManagers); + wallet->NotifyCanGetAddressesChanged(); return true; } From e88bf528afbb3b2068810009d7c8ce4ba550a38d Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Mon, 2 Oct 2023 02:14:36 +0700 Subject: [PATCH 10/11] follow-up bitcoin#17926 - add missing changes --- src/test/fuzz/key_io.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/fuzz/key_io.cpp b/src/test/fuzz/key_io.cpp index 181789a2505b..30f4160c4fde 100644 --- a/src/test/fuzz/key_io.cpp +++ b/src/test/fuzz/key_io.cpp @@ -40,7 +40,7 @@ FUZZ_TARGET_INIT(key_io, initialize_key_io) const CTxDestination tx_destination = DecodeDestination(random_string); (void)DescribeAddress(tx_destination); - // (void)GetKeyForDestination(/* store */ {}, tx_destination); + (void)GetKeyForDestination(/* store */ {}, tx_destination); (void)GetScriptForDestination(tx_destination); (void)IsValidDestination(tx_destination); From da212ad425f1fb1bd671acfe2f0bab6a1895cb02 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Mon, 2 Oct 2023 02:18:26 +0700 Subject: [PATCH 11/11] follow-up Merge #17381: LegacyScriptPubKeyMan code cleanups - now it's possible to remove workaround --- src/wallet/rpcwallet.cpp | 2 +- src/wallet/scriptpubkeyman.cpp | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 6be1ae9be650..41b756df1c88 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3829,12 +3829,12 @@ UniValue getaddressinfo(const JSONRPCRequest& request) ScriptPubKeyMan* spk_man = pwallet->GetScriptPubKeyMan(scriptPubKey); if (spk_man) { - const PKHash *pkhash = std::get_if(&dest); if (const CKeyMetadata* meta = spk_man->GetMetadata(dest)) { ret.pushKV("timestamp", meta->nCreateTime); CHDChain hdChainCurrent; LegacyScriptPubKeyMan* legacy_spk_man = pwallet->GetLegacyScriptPubKeyMan(); if (legacy_spk_man != nullptr) { + const PKHash *pkhash = std::get_if(&dest); if (pkhash && legacy_spk_man->HaveHDKey(ToKeyID(*pkhash), hdChainCurrent)) { ret.pushKV("hdchainid", hdChainCurrent.GetID().GetHex()); } diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 0fe85bff7a63..78a2dbc2bc09 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -749,9 +749,9 @@ const CKeyMetadata* LegacyScriptPubKeyMan::GetMetadata(const CTxDestination& des { LOCK(cs_KeyStore); - const PKHash *pkhash = std::get_if(&dest); - if (pkhash != nullptr && !ToKeyID(*pkhash).IsNull()) { - auto it = mapKeyMetadata.find(ToKeyID(*pkhash)); + CKeyID key_id = GetKeyForDestination(*this, dest); + if (!key_id.IsNull()) { + auto it = mapKeyMetadata.find(key_id); if (it != mapKeyMetadata.end()) { return &it->second; }