backport: merge bitcoin#25218, #25721, #25656, #25616, #26005, #24855, #18554, #17204, #20562, #21166, #25044, #25364, #25525, #25512, #24678, #26747, partial bitcoin#22154, #24584 (wallet backports: part 8) #6823
Conversation
…port for fetching bech32m addresses includes: - 754f134
…in selection excludes: - da03cb4 (no alternate address format to test against)
…in signing code
…atureExtractorClass subclass it
With bitcoin#19937 (backported as ae7e4cb), GBT doesn't require peers if using test chains, we can drop the extra node.
…wtransaction.py
…egacy wallet restore
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThis change set refactors wallet and selection APIs and updates callers across the codebase. It introduces util::Result (replacing BResult) and migrates many wallet interfaces (new/change destination, create/load/restore wallet, create transaction) to Result-based returns. Coin enumeration/selection is reworked to use CoinsResult with type buckets and updated selectors. ReserveDestination/GetNewDestination now return values instead of out-params. RPC, Qt, tests, and fuzz paths are adjusted accordingly. A default wallet accessor is added, -walletcrosschain is introduced with attach-time checks, and related functional tests are added. Script changes add DeferringSignatureChecker and a PushAll tweak for OP_1NEGATE. Numerous tests are updated/added/removed to match new APIs. Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/rpc/evo.cpp (1)
273-279: Select only spendable coins from the fee sourceAvailableCoinsListUnspent(...).all() can include non-spendable/watch-only outputs. For these ProTx funding flows we need spendable inputs to sign. Filter on out.fSpendable to avoid selecting unusable UTXOs.
- for (const auto& out : AvailableCoinsListUnspent(wallet).all()) { + for (const auto& out : AvailableCoinsListUnspent(wallet).all()) { + if (!out.fSpendable) continue; CTxDestination txDest; if (ExtractDestination(out.txout.scriptPubKey, txDest) && txDest == fundDest) { coinControl.Select(out.outpoint); } }src/wallet/scriptpubkeyman.cpp (1)
1778-1813: Ensure consistent error propagation instead of throwing an exceptionThe
throw std::runtime_errorat src/wallet/scriptpubkeyman.cpp:1807 will escape uncaught—no callers aroundDescriptorScriptPubKeyMan::GetNewDestination()catch this exception, leading to a potential crash rather than returning autil::Error. To maintain uniform error handling:• Location: src/wallet/scriptpubkeyman.cpp, line ~1807
• Change:- } else { - throw std::runtime_error(std::string(__func__) + ": Types are inconsistent. Stored type does not match type of newly generated address"); - } + } else { + return util::Error{std::string(__func__) + ": Types are inconsistent. Stored type does not match type of newly generated address"}; + }This will propagate the mismatch through the
util::ResultAPI rather than via an uncaught exception.
🧹 Nitpick comments (43)
test/functional/rpc_rawtransaction.py (3)
141-147: Synchronize after generating blocks used by both nodes to avoid timing-related flakesAfter generating blocks on node2, the test immediately queries both node 0 and node 2 for the tx/chain status with and without -txindex. A brief sync avoids intermittent failures due to propagation delay.
tx = self.wallet.send_self_transfer(from_node=self.nodes[2])['txid'] block1, block2 = self.generate(self.nodes[2], 2) + self.sync_all() for n in [0, 2]: self.log.info(f"Test getrawtransaction {'with' if n == 0 else 'without'} -txindex, with blockhash")
271-284: Avoid float literals for fee rates in RPC calls (use Decimal or string)Using Python float (0.00001000) risks precision issues in JSON encoding. Prefer Decimal or string for fee parameters to ensure exact values are passed.
- testres = self.nodes[2].testmempoolaccept([tx['hex']], 0.00001000)[0] + testres = self.nodes[2].testmempoolaccept([tx['hex']], '0.00001000')[0] ... - assert_raises_rpc_error(-25, fee_exceeds_max, self.nodes[2].sendrawtransaction, tx['hex'], 0.00001000) + assert_raises_rpc_error(-25, fee_exceeds_max, self.nodes[2].sendrawtransaction, tx['hex'], '0.00001000')If you prefer Decimal:
- testres = self.nodes[2].testmempoolaccept([tx['hex']], 0.00001000)[0] + testres = self.nodes[2].testmempoolaccept([tx['hex']], Decimal('0.00001000'))[0] ... - assert_raises_rpc_error(-25, fee_exceeds_max, self.nodes[2].sendrawtransaction, tx['hex'], 0.00001000) + assert_raises_rpc_error(-25, fee_exceeds_max, self.nodes[2].sendrawtransaction, tx['hex'], Decimal('0.00001000'))
92-132: Use consistent variable naming: txid instead of txIdMinor style nit for consistency with common usage and other tests. Rename txId -> txid across this block.
- txId = tx['txid'] + txid = tx['txid'] ... - assert_equal(self.nodes[n].getrawtransaction(txId), tx['hex']) - assert_equal(self.nodes[n].getrawtransactionmulti({"0":[txId]})[txId], tx['hex']) + assert_equal(self.nodes[n].getrawtransaction(txid), tx['hex']) + assert_equal(self.nodes[n].getrawtransactionmulti({"0":[txid]})[txid], tx['hex']) ... - assert_equal(self.nodes[n].getrawtransaction(txId, 0), tx['hex']) - assert_equal(self.nodes[n].getrawtransactionmulti({"0":[txId]}, 0)[txId], tx['hex']) + assert_equal(self.nodes[n].getrawtransaction(txid, 0), tx['hex']) + assert_equal(self.nodes[n].getrawtransactionmulti({"0":[txid]}, 0)[txid], tx['hex']) ... - assert_equal(self.nodes[n].getrawtransaction(txId, False), tx['hex']) - assert_equal(self.nodes[n].getrawtransactionmulti({"0":[txId]}, False)[txId], tx['hex']) + assert_equal(self.nodes[n].getrawtransaction(txid, False), tx['hex']) + assert_equal(self.nodes[n].getrawtransactionmulti({"0":[txid]}, False)[txid], tx['hex']) ... - assert_equal(self.nodes[n].getrawtransaction(txId, 1)["hex"], tx['hex']) - assert_equal(self.nodes[n].getrawtransactionmulti({"0":[txId]}, 1)[txId]['hex'], tx['hex']) + assert_equal(self.nodes[n].getrawtransaction(txid, 1)["hex"], tx['hex']) + assert_equal(self.nodes[n].getrawtransactionmulti({"0":[txid]}, 1)[txid]['hex'], tx['hex']) ... - assert_equal(self.nodes[n].getrawtransaction(txId, True)["hex"], tx['hex']) - assert_equal(self.nodes[n].getrawtransactionmulti(verbose=True, transactions={"0":[txId]})[txId]['hex'], tx['hex']) + assert_equal(self.nodes[n].getrawtransaction(txid, True)["hex"], tx['hex']) + assert_equal(self.nodes[n].getrawtransactionmulti(verbose=True, transactions={"0":[txid]})[txid]['hex'], tx['hex']) ... - assert_raises_rpc_error(-5, err_msg, self.nodes[n].getrawtransaction, txId, verbose) + assert_raises_rpc_error(-5, err_msg, self.nodes[n].getrawtransaction, txid, verbose) ... - assert_equal(self.nodes[n].getrawtransactionmulti({"0":[txId]}, verbose)[txId], "None") + assert_equal(self.nodes[n].getrawtransactionmulti({"0":[txid]}, verbose)[txid], "None") ... - assert_raises_rpc_error(-1, "not a boolean", self.nodes[n].getrawtransaction, txid=txId, verbose=value) - assert_raises_rpc_error(-1, "not a boolean", self.nodes[n].getrawtransactionmulti, transactions={"0":[txId]}, verbose=value) + assert_raises_rpc_error(-1, "not a boolean", self.nodes[n].getrawtransaction, txid=txid, verbose=value) + assert_raises_rpc_error(-1, "not a boolean", self.nodes[n].getrawtransactionmulti, transactions={"0":[txid]}, verbose=value) ... - assert_raises_rpc_error(-1, "not a boolean", self.nodes[n].getrawtransaction, txId, []) + assert_raises_rpc_error(-1, "not a boolean", self.nodes[n].getrawtransaction, txid, []) ... - assert_raises_rpc_error(-1, "not a boolean", self.nodes[n].getrawtransaction, txId, {}) + assert_raises_rpc_error(-1, "not a boolean", self.nodes[n].getrawtransaction, txid, {})test/functional/wallet_keypool_hd.py (1)
200-200: Nit: assert on a stable substring to reduce brittlenessMinor: asserting just a stable substring can reduce future churn if punctuation changes.
- assert_raises_rpc_error(-4, "Transaction needs a change address, but we can't generate it.", w2.walletcreatefundedpsbt, inputs=[], outputs=[{addr.pop(): 0.00005000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.000010}) + assert_raises_rpc_error(-4, "Transaction needs a change address", w2.walletcreatefundedpsbt, inputs=[], outputs=[{addr.pop(): 0.00005000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.000010})test/functional/rpc_fundrawtransaction.py (1)
586-586: Nit: prefer substring match for error messageTo avoid future punctuation/text drift causing false negatives.
- assert_raises_rpc_error(-4, "Transaction needs a change address, but we can't generate it.", self.nodes[1].fundrawtransaction, rawtx) + assert_raises_rpc_error(-4, "Transaction needs a change address", self.nodes[1].fundrawtransaction, rawtx)test/functional/wallet_avoidreuse.py (1)
118-128: Reacquire wallet RPC wrapper after unload to avoid potential stalenessWhile the current wrapper likely works (path-based), reacquiring it after re-loading the wallet avoids any future surprises if the wrapper starts caching connection state.
Apply this diff within the selected block:
- w.unloadwallet() - self.nodes[1].loadwallet("avoid_reuse_persist") - assert_equal(w.getwalletinfo()["avoid_reuse"], False) + w.unloadwallet() + self.nodes[1].loadwallet("avoid_reuse_persist") + w = self.nodes[1].get_wallet_rpc("avoid_reuse_persist") + assert_equal(w.getwalletinfo()["avoid_reuse"], False)src/script/interpreter.h (1)
198-212: Optional: mark ctor explicitThe constructor could be marked explicit to avoid accidental implicit conversions from BaseSignatureChecker&. Harmless either way given current usage.
- DeferringSignatureChecker(BaseSignatureChecker& checker) : m_checker(checker) {} + explicit DeferringSignatureChecker(BaseSignatureChecker& checker) : m_checker(checker) {}src/script/sign.cpp (1)
229-237: Optional: call base CheckSig for clarityCalling the base’s delegating CheckSig makes the intent (defer + extract) explicit rather than reaching into m_checker directly.
- if (m_checker.CheckSig(scriptSig, vchPubKey, scriptCode, sigversion)) { + if (DeferringSignatureChecker::CheckSig(scriptSig, vchPubKey, scriptCode, sigversion)) {test/functional/rpc_signrawtransactionwithkey.py (3)
14-21: Remove or fix unused helper referencing undefined attributessend_to_address() references self.block_hash and self.blk_idx which are never defined in this test or the base class. It’s also unused. Prefer removing it to avoid dead code and future AttributeError if referenced.
Apply this diff to remove the method:
- def send_to_address(self, addr, amount): - input = {"txid": self.nodes[0].getblock(self.block_hash[self.blk_idx])["tx"][0], "vout": 0} - output = {addr: amount} - self.blk_idx += 1 - rawtx = self.nodes[0].createrawtransaction([input], output) - txid = self.nodes[0].sendrawtransaction(self.nodes[0].signrawtransactionwithkey(rawtx, [self.nodes[0].get_deterministic_priv_key().key])["hex"], 0) - return txid
23-29: Docstring says “one input” but test uses two inputsEither adjust the docstring or reduce inputs to one. Keeping two inputs is fine—update the wording.
- """Create and sign a valid raw transaction with one input. + """Create and sign a valid raw transaction with two inputs.
30-47: Minor naming cleanup for readabilityUse snake_case for local vars: priv_keys, raw_tx, raw_tx_signed. This aligns with project conventions in other tests.
- privKeys = ['cUeKHd5orzT3mz8P9pxyREHfsWtVfgsfDjiZZBcjUBAaGk1BTj7N', 'cVKpPfVKSJxKqVpE9awvXNWuLHCa5j5tiE7K6zbUSptFpTEtiFrA'] + priv_keys = ['cUeKHd5orzT3mz8P9pxyREHfsWtVfgsfDjiZZBcjUBAaGk1BTj7N', 'cVKpPfVKSJxKqVpE9awvXNWuLHCa5j5tiE7K6zbUSptFpTEtiFrA'] @@ - rawTx = self.nodes[0].createrawtransaction(inputs, outputs) - rawTxSigned = self.nodes[0].signrawtransactionwithkey(rawTx, privKeys, inputs) + raw_tx = self.nodes[0].createrawtransaction(inputs, outputs) + raw_tx_signed = self.nodes[0].signrawtransactionwithkey(raw_tx, priv_keys, inputs) @@ - assert rawTxSigned['complete'] + assert raw_tx_signed['complete'] @@ - assert 'errors' not in rawTxSigned + assert 'errors' not in raw_tx_signedtest/functional/mempool_updatefromblock.py (1)
49-55: Seed priv_keys and fallback address are reasonable; minor robustness tweakInitializing self.priv_keys with the deterministic key is appropriate for spending cached coinbases. Consider guarding n_tx_to_mine usage to avoid TypeError when None.
- if not end_address: + if not end_address: end_address = self.get_new_address()And later at the first usage (Line 100):
- if tx_count in n_tx_to_mine: + if n_tx_to_mine and tx_count in n_tx_to_mine:test/functional/wallet_crosschain.py (1)
59-65: Preserve -maxconnections=0 on restart with -walletcrosschainOn restart, node1 loses the -maxconnections=0 flag, potentially attempting to contact public testnet peers. Keep it to avoid flakiness and speed up runs.
- self.start_nodes([['-walletcrosschain']] * self.num_nodes) + extra = [['-walletcrosschain'], ['-walletcrosschain', '-maxconnections=0']] + self.start_nodes(extra)test/functional/feature_nulldummy.py (2)
60-67: Avoid reusing “input” name and ensure prevtx details variable clarityOverwriting the input dict with the merged prevtx details is a bit confusing. Use a separate prevtxs variable for readability.
- input = {"txid": txid, "vout": 0} - output = {addr: amount} - rawtx = self.nodes[0].createrawtransaction([input], output) - # Details only needed for scripthash or witness spends - input = None if not input_details else [{**input, **input_details}] - signedtx = self.nodes[0].signrawtransactionwithkey(rawtx, [privkey], input) + inpt = {"txid": txid, "vout": 0} + output = {addr: amount} + rawtx = self.nodes[0].createrawtransaction([inpt], output) + # Details only needed for scripthash or witness spends + prevtxs = None if not input_details else [{**inpt, **input_details}] + signedtx = self.nodes[0].signrawtransactionwithkey(rawtx, [privkey], prevtxs)
112-113: Use explicit deep copy for test6txs to avoid accidental mutationCTransaction(test4tx) may rely on implicit copy semantics. Safer to clone via hex to guarantee independence before mutating test4tx.
- test6txs = [CTransaction(test4tx)] + test6txs = [tx_from_hex(test4tx.serialize().hex())]src/util/result.h (1)
8-12: Header hygiene: include and to avoid transitive dependenciesThis header uses assert, std::move, std::forward, and in_place_index_t. Relying on transitive includes is brittle.
Apply this diff to add the missing headers:
#include <attributes.h> #include <util/translation.h> +#include <cassert> +#include <utility> #include <variant>src/test/result_tests.cpp (2)
5-7: Add missing standard header for std::unique_ptrunique_ptr is used in NoCopy; include explicitly to avoid relying on transitive includes.
#include <util/result.h> #include <boost/test/unit_test.hpp> +#include <memory>
70-74: Remove unused template parameter pack in ExpectFailThe parameter pack Args is unused and may confuse readers.
-template <typename T, typename... Args> -void ExpectFail(const util::Result<T>& result, const bilingual_str& str) +template <typename T> +void ExpectFail(const util::Result<T>& result, const bilingual_str& str) { ExpectResult(result, false, str); }test/functional/wallet_signrawtransactionwithwallet.py (1)
190-208: Avoid shadowing Python built-in 'type'Using 'type' as a variable shadows the built-in. Rename for clarity.
- for type in ["legacy"]: - self.log.info(f"Test signing with missing prevtx info ({type})") + for wallet_type in ["legacy"]: + self.log.info(f"Test signing with missing prevtx info ({wallet_type})") addr = self.nodes[0].getnewaddress("") addrinfo = self.nodes[0].getaddressinfo(addr) pubkey = addrinfo["scriptPubKey"] inputs = [{'txid': txid, 'vout': 3, 'sequence': 1000}] outputs = {self.nodes[0].getnewaddress(): 1} rawtx = self.nodes[0].createrawtransaction(inputs, outputs) prevtx = dict(txid=txid, scriptPubKey=pubkey, vout=3, amount=1) succ = self.nodes[0].signrawtransactionwithwallet(rawtx, [prevtx]) assert succ["complete"] - if type == "legacy": + if wallet_type == "legacy": del prevtx["amount"] succ = self.nodes[0].signrawtransactionwithwallet(rawtx, [prevtx]) assert succ["complete"] else: assert_raises_rpc_error(-3, "Missing amount", self.nodes[0].signrawtransactionwithwallet, rawtx, [ { "txid": txid, "scriptPubKey": pubkey, "vout": 3, } ])src/rpc/masternode.cpp (1)
143-145: Use of WITH_LOCK in range-for is correct; consider a small readability tweakThe switch to AvailableCoinsListUnspent(*wallet, &coin_control).all() guarded by cs_wallet looks good and aligns with the new API. For readability and to avoid a macro-heavy range expression, consider storing the result in a local const before iterating.
- for (const auto& out : WITH_LOCK(wallet->cs_wallet, return AvailableCoinsListUnspent(*wallet, &coin_control).all())) { + const auto outs = WITH_LOCK(wallet->cs_wallet, return AvailableCoinsListUnspent(*wallet, &coin_control).all()); + for (const auto& out : outs) { outputsArr.push_back(out.outpoint.ToStringShort()); }src/qt/walletmodel.cpp (1)
256-271: Result-based createTransaction integration looks correct; ensure ErrorString header is included
- Using the util::Result return to set newTx and emitting the translated error via util::ErrorString(res) is correct and matches the new API.
- Minor: make sure the header that declares util::ErrorString is included explicitly to avoid relying on transitive includes (typically util/error.h in Core).
If not already present via transitive includes, add:
#include <util/error.h>This ensures util::ErrorString(res) is declared on all build configurations.
src/bench/coin_selection.cpp (1)
57-61: LGTM: bench updated for CoinsResult and mixed-output AttemptSelection
- Populating available_coins.legacy with COutput entries matches the new data structure.
- AttemptSelection invocation with allow_mixed_output_types=true is consistent with the updated signature and behavior.
Optional: since this is a benchmark, you could reserve capacity for available_coins.legacy to avoid reallocations when inserting 1001 entries.
- wallet::CoinsResult available_coins; + wallet::CoinsResult available_coins; + available_coins.legacy.reserve(wtxs.size());Also applies to: 76-77
src/wallet/coinjoin.cpp (1)
1578-1582: Assert on GetReservedDestination optional is acceptable here; comment is slightly misleadingAsserting on
GetReservedDestination(true)is in line with the upstream backport intention that this must not fail during normal operation. However, the comment “as we just unlocked” is misleading in this scope; the wallet is required to be unlocked earlier, but not “just unlocked” here. Consider rewording the comment.- auto dest_opt = reserveDest.GetReservedDestination(true); - assert(dest_opt); // should never fail, as we just unlocked + auto dest_opt = reserveDest.GetReservedDestination(true); + assert(dest_opt); // should not fail while wallet is unlocked and keypool is availablesrc/wallet/test/fuzz/notifications.cpp (1)
71-78: Result-based destination retrieval: correct; minor readability tweakMigrating to util::Result and using
*Assert(op_dest)is consistent with the new API. You can slightly simplify the initialization for readability.- util::Result<CTxDestination> op_dest{util::Error{}}; - if (fuzzed_data_provider.ConsumeBool()) { - op_dest = wallet->GetNewDestination(""); - } else { - op_dest = wallet->GetNewChangeDestination(); - } + auto op_dest = fuzzed_data_provider.ConsumeBool() + ? wallet->GetNewDestination("") + : wallet->GetNewChangeDestination(); return GetScriptForDestination(*Assert(op_dest));src/bench/wallet_loading.cpp (1)
51-51: Use a local variable for the destination to improve readability and diagnosticsThe inline
*Assert(wallet.GetNewDestination(""))is fine for benches but slightly opaque. Capturing the destination first improves readability and makes it easier to add logging or inspect failures if needed.Apply this refactor:
- mtx.vout.push_back({COIN, GetScriptForDestination(*Assert(wallet.GetNewDestination("")))}); + const auto dest = *Assert(wallet.GetNewDestination("")); + mtx.vout.emplace_back(COIN, GetScriptForDestination(dest));src/test/evo_deterministicmns_tests.cpp (1)
259-259: Consistent migration to CScript-based coinbase scriptPubKey (LGTM)Replacing CKey-based overloads with
GetScriptForRawPubKey(...) and passingCScripttoCreate{,AndProcess}Blockaligns with the upstream API and removes the ambiguous overloads. Usage is consistent across functions.If you touch the naming again, consider
coinbase_scriptinstead ofcoinbase_pkto reflect the type and avoid confusion with pubkeys.Also applies to: 263-274, 270-271, 298-303, 319-320, 339-340, 352-353, 364-365, 385-386, 431-432, 466-467, 477-478, 487-487, 509-510, 522-523, 532-533, 545-546, 574-575, 580-581, 598-599, 617-617, 639-640, 763-764, 785-786, 818-819, 830-831, 944-949, 959-963
src/test/util/setup_common.cpp (1)
445-449: Switch to GetScriptForRawPubKey in mineBlocks (LGTM); consider explicit header includeThe change matches the new test helpers that accept
CScript. To avoid relying on transitive includes, consider explicitly including the declaration header forGetScriptForRawPubKey.Add near the other includes at the top of the file:
#include <script/standard.h>src/wallet/test/spend_tests.cpp (1)
28-40: Clean up: remove unused variable and prefer BOOST_REQUIRE for result presence
bilingual_str error;is now unused and can be dropped.- Use
BOOST_REQUIRE(res);to abort the test early on failure before dereferencing.- bilingual_str error; CCoinControl coin_control; coin_control.m_feerate.emplace(10000); coin_control.fOverrideFeeRate = true; - auto res = CreateTransaction(*wallet, {recipient}, RANDOM_CHANGE_POSITION, coin_control); - BOOST_CHECK(res); + auto res = CreateTransaction(*wallet, {recipient}, RANDOM_CHANGE_POSITION, coin_control); + BOOST_REQUIRE(res); const auto& txr = *res;src/wallet/test/coinjoin_tests.cpp (2)
190-193: Strengthen the reservation assertion to fail fastUse BOOST_REQUIRE to abort the test immediately if reservation fails (more actionable than a soft check).
- auto dest_opt = reserveDest.GetReservedDestination(false); - BOOST_CHECK(dest_opt); + auto dest_opt = reserveDest.GetReservedDestination(false); + BOOST_REQUIRE(dest_opt); tallyItem.txdest = *dest_opt;
197-201: Surface CreateTransaction errors in test outputLeverage the Result’s error string for easier debugging when creation fails.
- auto res = CreateTransaction(*wallet, {{GetScriptForDestination(tallyItem.txdest), nAmount, false}}, nChangePosRet, coinControl); - BOOST_CHECK(res); + auto res = CreateTransaction(*wallet, {{GetScriptForDestination(tallyItem.txdest), nAmount, false}}, nChangePosRet, coinControl); + BOOST_REQUIRE_MESSAGE(res, util::ErrorString(res).original); tx = res->tx; nChangePosRet = res->change_pos;src/wallet/rpc/spend.cpp (1)
68-78: Result-based CreateTransaction integration looks correct; consider refining RPC error code mappingThe switch to util::Result, util::ErrorString, and res->fee_calc.reason is good. One minor consideration: CreateTransaction can fail for reasons other than insufficient funds (e.g., invalid params). Mapping all failures to RPC_WALLET_INSUFFICIENT_FUNDS may be slightly misleading. If desired, consider mapping specific errors to RPC_WALLET_ERROR and reserving RPC_WALLET_INSUFFICIENT_FUNDS for actual funding failures.
src/wallet/test/availablecoins_tests.cpp (1)
56-81: Use BOOST_REQUIRE (not BOOST_ASSERT) and tighten lock scope for clarity
- BOOST_ASSERT may be compiled out; use BOOST_REQUIRE (or BOOST_CHECK) in tests to ensure failures are reported consistently.
- Optional: Keep the wallet lock only where required (AvailableCoins and GetNewDestination), to reduce nested/recursive locking during AddTx.
- CoinsResult available_coins; - util::Result<CTxDestination> dest{util::Error{}}; - LOCK(wallet->cs_wallet); + CoinsResult available_coins; + util::Result<CTxDestination> dest; + LOCK(wallet->cs_wallet); @@ - dest = wallet->GetNewDestination(""); - BOOST_ASSERT(dest); + dest = wallet->GetNewDestination(""); + BOOST_REQUIRE(dest);If you prefer, you can also narrow the LOCK scope to just the calls that require it.
src/qt/walletcontroller.cpp (3)
261-267: Reset error/warning state before each createWallet attemptAvoid surfacing stale errors/warnings from a previous run by clearing the state before invoking the loader.
- auto wallet{node().walletLoader().createWallet(name, m_passphrase, flags, m_warning_message)}; + m_error_message = {}; + m_warning_message.clear(); + auto wallet{node().walletLoader().createWallet(name, m_passphrase, flags, m_warning_message)};
337-343: Reset error/warning state before each loadWallet attemptSame rationale as for createWallet.
- auto wallet{node().walletLoader().loadWallet(path, m_warning_message)}; + m_error_message = {}; + m_warning_message.clear(); + auto wallet{node().walletLoader().loadWallet(path, m_warning_message)};
386-390: Reset error/warning state before each restoreWallet attemptSame rationale as above.
- auto wallet{node().walletLoader().restoreWallet(backup_file, wallet_name, m_warning_message)}; + m_error_message = {}; + m_warning_message.clear(); + auto wallet{node().walletLoader().restoreWallet(backup_file, wallet_name, m_warning_message)};src/wallet/wallet.cpp (1)
2354-2369: Minor: take label by const reference to avoid an unnecessary copyThe implementation takes label by value. Prefer const std::string& to match the interface layer and avoid copies.
-util::Result<CTxDestination> CWallet::GetNewDestination(const std::string label) +util::Result<CTxDestination> CWallet::GetNewDestination(const std::string& label)Note: Ensure the declaration in src/wallet/wallet.h uses the same signature.
src/wallet/interfaces.cpp (1)
607-611: Nit: avoid returning from a void functionregisterOtherRpcs returns void. The explicit return is redundant.
- { - return RegisterRPCs(commands); - } + { + RegisterRPCs(commands); + }src/wallet/spend.h (2)
31-56: Prefer size_t for CoinsResult::size() to match standard containersReturning uint64_t diverges from STL conventions (size() usually returns size_t). It is harmless but can promote needless casts/mismatches on some platforms. Suggest returning size_t.
Apply this diff:
- uint64_t size() const; + size_t size() const;And in the implementation (spend.cpp):
-uint64_t CoinsResult::size() const +size_t CoinsResult::size() const { return legacy.size() + other.size(); }
114-124: Typo in param docs: “eligilibity_filter”Minor spelling fix in the documentation to avoid confusion.
- * param@[in] eligilibity_filter A filter containing rules for which coins are allowed to be included in this selection + * param@[in] eligibility_filter A filter containing rules for which coins are allowed to be included in this selectionsrc/wallet/spend.cpp (4)
218-251: Classification of P2SH via redeemScript is fine; consider safe fallback instead of skipping coinIf provider->GetCScript unexpectedly fails while solvable is true, the code continues without adding the coin, dropping a usable UTXO. While this should be rare (solvable typically implies redeemScript is available), a defensive fallback avoids silent loss.
Apply this minimal fallback:
- if (output.scriptPubKey.IsPayToScriptHash() && solvable) { + if (output.scriptPubKey.IsPayToScriptHash() && solvable) { CScript redeemScript; CTxDestination destination; if (!ExtractDestination(output.scriptPubKey, destination)) continue; const CScriptID& hash = CScriptID(std::get<ScriptHash>(destination)); - if (!provider->GetCScript(hash, redeemScript)) - continue; - type = Solver(redeemScript, return_values_unused); + if (provider && provider->GetCScript(hash, redeemScript)) { + type = Solver(redeemScript, return_values_unused); + } else { + // Fall back to classifying by scriptPubKey (will be SCRIPTHASH) + type = Solver(output.scriptPubKey, return_values_unused); + } } else { type = Solver(output.scriptPubKey, return_values_unused); }
316-321: ListCoins iterates over all() — minor allocation overheadUsing all() constructs a temporary vector. Given typical UTXO counts this is acceptable, but you can avoid the extra allocation by iterating over both buckets.
Example:
- for (const COutput& coin : AvailableCoinsListUnspent(wallet).all()) { + const CoinsResult coins = AvailableCoinsListUnspent(wallet); + for (const COutput& coin : coins.legacy) { + ... + } + for (const COutput& coin : coins.other) { + ... + }
471-475: BnB and SRD are effectively disabled via clearing positive_groups; make it explicitYou clear positive_groups then still call SelectCoinsBnB and SelectCoinsSRD on empty input. This is harmless but confusing. Either don’t build/clear positive_groups when skipping, or guard the calls.
E.g.:
- std::vector<OutputGroup> positive_groups = GroupOutputs(wallet, available_coins, coin_selection_params, eligibility_filter, true /* positive_only */); - positive_groups.clear(); // Cleared to skip BnB and SRD as they're unaware of mixed coins - if (auto bnb_result{SelectCoinsBnB(positive_groups, nTargetValue, coin_selection_params.m_cost_of_change)}) { + // Skip BnB as it's unaware of mixed coins + std::vector<OutputGroup> positive_groups; // leave empty intentionally + // if (auto bnb_result{SelectCoinsBnB(positive_groups, nTargetValue, coin_selection_params.m_cost_of_change)}) { results.push_back(*bnb_result); - } + // } ... - if (auto srd_result{SelectCoinsSRD(positive_groups, srd_target, coin_selection_params.rng_fast)}) { + // Skip SRD for the same reason + // if (auto srd_result{SelectCoinsSRD(positive_groups, srd_target, coin_selection_params.rng_fast)}) { srd_result->ComputeAndSetWaste(coin_selection_params.m_cost_of_change); results.push_back(*srd_result); - } + // }Also applies to: 498-501
573-586: Avoid building a temporary with available_coins.all() in tight loopsThis loop is on a hot path. Iterate buckets directly to avoid a combined copy.
- for (const COutput& out : available_coins.all()) { + for (const auto* bucket : {&available_coins.legacy, &available_coins.other}) { + for (const COutput& out : *bucket) { if (!out.spendable) continue; if (preset_coins.count(out.outpoint)) { preset_candidates.Insert(out, /*ancestors=*/0, /*descendants=*/0, /*positive_only=*/false); } if (preset_candidates.GetSelectionAmount() >= nTargetValue) { result.AddInput(preset_candidates); success = true; break; } } + if (success) break; - } + }
| auto dest_opt = reserveDestination.GetReservedDestination(false); | ||
| assert(dest_opt); | ||
| dest = *dest_opt; | ||
| } |
There was a problem hiding this comment.
Assert should be replaced with proper error handling
The constructor now uses GetReservedDestination(false) which returns a util::Result. While asserting the result exists works, this could fail in production if the wallet cannot provide addresses (e.g., keypool exhausted). Consider proper error handling instead of assertions.
Consider propagating the error instead of asserting:
- auto dest_opt = reserveDestination.GetReservedDestination(false);
- assert(dest_opt);
- dest = *dest_opt;
+ auto dest_result = reserveDestination.GetReservedDestination(false);
+ if (!dest_result) {
+ throw std::runtime_error(strprintf("CKeyHolder: %s", util::ErrorString(dest_result).original));
+ }
+ dest = *dest_result;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto dest_opt = reserveDestination.GetReservedDestination(false); | |
| assert(dest_opt); | |
| dest = *dest_opt; | |
| } | |
| auto dest_result = reserveDestination.GetReservedDestination(false); | |
| if (!dest_result) { | |
| throw std::runtime_error(strprintf("CKeyHolder: %s", util::ErrorString(dest_result).original)); | |
| } | |
| dest = *dest_result; | |
| } |
🤖 Prompt for AI Agents
In src/coinjoin/util.cpp around lines 32–35, replace the assert on the result of
GetReservedDestination(false) with proper error handling: check whether dest_opt
contains a value, and if it does, assign dest = *dest_opt; if it does not,
propagate the failure instead of asserting — for example, throw an appropriate
exception (e.g., std::runtime_error) or convert and return a util::Result error
containing dest_opt's error text so callers can handle keypool exhaustion or
other failures gracefully.
| auto dest_opt = dest.GetReservedDestination(false); | ||
| assert(dest_opt); | ||
| script = ::GetScriptForDestination(*dest_opt); | ||
| } |
There was a problem hiding this comment.
Another assertion that should handle errors gracefully
Similar to the CKeyHolder constructor, this assertion could fail if the wallet cannot provide addresses. The code should handle this error case properly.
Consider proper error handling:
- auto dest_opt = dest.GetReservedDestination(false);
- assert(dest_opt);
- script = ::GetScriptForDestination(*dest_opt);
+ auto dest_result = dest.GetReservedDestination(false);
+ if (!dest_result) {
+ throw std::runtime_error(strprintf("CTransactionBuilderOutput: %s", util::ErrorString(dest_result).original));
+ }
+ script = ::GetScriptForDestination(*dest_result);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto dest_opt = dest.GetReservedDestination(false); | |
| assert(dest_opt); | |
| script = ::GetScriptForDestination(*dest_opt); | |
| } | |
| auto dest_result = dest.GetReservedDestination(false); | |
| if (!dest_result) { | |
| throw std::runtime_error(strprintf( | |
| "CTransactionBuilderOutput: %s", util::ErrorString(dest_result).original)); | |
| } | |
| script = ::GetScriptForDestination(*dest_result); | |
| } |
🤖 Prompt for AI Agents
In src/coinjoin/util.cpp around lines 105 to 108, replace the bare assert on
dest_opt with proper runtime error handling like the CKeyHolder constructor:
check if dest_opt is set, and if not, log an error with context (including any
relevant wallet/address info) and return an appropriate failure (e.g., return
false/empty optional or throw a descriptive exception depending on surrounding
function semantics) instead of asserting; only call
::GetScriptForDestination(*dest_opt) when dest_opt is valid so the failure is
handled gracefully.
| //! Register non-core wallet RPCs | ||
| virtual void registerOtherRpcs(const Span<const CRPCCommand>& commands) = 0; | ||
|
|
||
| //! Create new wallet. | ||
| virtual std::unique_ptr<Wallet> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, bilingual_str& error, std::vector<bilingual_str>& warnings) = 0; | ||
| //! Create new wallet. | ||
| virtual util::Result<std::unique_ptr<Wallet>> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, std::vector<bilingual_str>& warnings) = 0; | ||
|
|
||
| //! Load existing wallet. | ||
| virtual std::unique_ptr<Wallet> loadWallet(const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings) = 0; | ||
| //! Load existing wallet. | ||
| virtual util::Result<std::unique_ptr<Wallet>> loadWallet(const std::string& name, std::vector<bilingual_str>& warnings) = 0; | ||
|
|
||
| //! Return default wallet directory. | ||
| virtual std::string getWalletDir() = 0; | ||
| //! Return default wallet directory. | ||
| virtual std::string getWalletDir() = 0; | ||
|
|
||
| //! Restore backup wallet | ||
| virtual BResult<std::unique_ptr<Wallet>> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, std::vector<bilingual_str>& warnings) = 0; | ||
| //! Restore backup wallet | ||
| virtual util::Result<std::unique_ptr<Wallet>> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, std::vector<bilingual_str>& warnings) = 0; | ||
|
|
||
| //! Return available wallets in wallet directory. | ||
| virtual std::vector<std::string> listWalletDir() = 0; | ||
| //! Return available wallets in wallet directory. | ||
| virtual std::vector<std::string> listWalletDir() = 0; | ||
|
|
||
| //! Return interfaces for accessing wallets (if any). | ||
| virtual std::vector<std::unique_ptr<Wallet>> getWallets() = 0; | ||
| //! Return interfaces for accessing wallets (if any). | ||
| virtual std::vector<std::unique_ptr<Wallet>> getWallets() = 0; | ||
|
|
||
| //! Register handler for load wallet messages. This callback is triggered by | ||
| //! createWallet and loadWallet above, and also triggered when wallets are | ||
| //! loaded at startup or by RPC. | ||
| using LoadWalletFn = std::function<void(std::unique_ptr<Wallet> wallet)>; | ||
| virtual std::unique_ptr<Handler> handleLoadWallet(LoadWalletFn fn) = 0; | ||
| //! Register handler for load wallet messages. This callback is triggered by | ||
| //! createWallet and loadWallet above, and also triggered when wallets are | ||
| //! loaded at startup or by RPC. | ||
| using LoadWalletFn = std::function<void(std::unique_ptr<Wallet> wallet)>; | ||
| virtual std::unique_ptr<Handler> handleLoadWallet(LoadWalletFn fn) = 0; | ||
|
|
||
| //! Return pointer to internal context, useful for testing. | ||
| virtual wallet::WalletContext* context() { return nullptr; } | ||
| //! Return pointer to internal context, useful for testing. | ||
| virtual wallet::WalletContext* context() { return nullptr; } | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
WalletLoader methods migrated to util::Result — good, but verify call sites are updated
The create/load/restore methods now return util::Result and drop the error out-param. This ripples through GUI/RPC. Please verify there are no remaining callers expecting the old signatures.
Run this script to catch old-style usages (extra args/out-params):
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for old getNewDestination usages (with extra out-params)..."
rg -nP --type=cpp -C2 '\b(getNewDestination|GetNewDestination)\s*\(\s*[^,()]+\s*,\s*[^)]' || true
echo
echo "Searching for old createTransaction usages (fail_reason out-param or boolean return)..."
# Look for calls passing a bilingual_str or expecting bool
rg -nP --type=cpp -C3 '\bcreateTransaction\s*\(' || true
rg -nP --type=cpp -C2 'bilingual_str\s+\w+\s*;.*\bcreateTransaction\s*\(' || true
echo
echo "Searching for WalletLoader old create/load/restore signatures..."
rg -nP --type=cpp -C2 '\.createWallet\s*\([^)]{60,}\)' || true
rg -nP --type=cpp -C2 '\.loadWallet\s*\([^)]{60,}\)' || true
rg -nP --type=cpp -C2 '\.restoreWallet\s*\([^)]{60,}\)' || trueLength of output: 2428
Fix outdated createTransaction usage in Qt WalletModel
We confirmed there are no remaining old-style calls to getNewDestination or to the migrated createWallet/loadWallet/restoreWallet APIs. However, the Qt UI still invokes the legacy createTransaction signature with both change_pos and fee out-params. Please update it to the new util::Result<CTransactionRef> API (which no longer takes a fee out-param) and handle the returned Result.
• src/qt/walletmodel.cpp:257
replace
const auto& res = m_wallet->createTransaction(
vecSend, coinControl, !wallet().privateKeysDisabled(), nChangePosRet, nFeeRequired);with a call matching the interface’s new signature (e.g. passing only vecSend, coinControl, sign, nChangePosRet), then unpack res to obtain both the transaction and fee.
🤖 Prompt for AI Agents
In src/qt/walletmodel.cpp around line 257, the code still calls the legacy
createTransaction that returned both change_pos and fee via out-params; update
the call to the new signature that returns util::Result<CTransactionRef> (pass
vecSend, coinControl, sign flag, and nChangePosRet only), capture the
util::Result, check for and handle errors before proceeding, and then unpack the
successful Result to obtain the CTransactionRef and the fee value and replace
uses of the old nFeeRequired out-param with the fee extracted from the Result.
| @@ -121,6 +121,7 @@ void WalletInit::AddWalletOptions(ArgsManager& argsman) const | |||
| #endif | |||
|
|
|||
| argsman.AddArg("-walletrejectlongchains", strprintf("Wallet will not create transactions that violate mempool chain limits (default: %u)", DEFAULT_WALLET_REJECT_LONG_CHAINS), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::WALLET_DEBUG_TEST); | |||
| argsman.AddArg("-walletcrosschain", strprintf("Allow reusing wallet files across chains (default: %u)", DEFAULT_WALLETCROSSCHAIN), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::WALLET_DEBUG_TEST); | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
New -walletcrosschain option wiring looks correct; verify default and gating sites
The arg is added with ALLOW_ANY | DEBUG_ONLY under WALLET_DEBUG_TEST. Please verify:
- DEFAULT_WALLETCROSSCHAIN is defined (likely in wallet.h).
- The flag is actually consulted in wallet attach/safety logic (e.g., AttachChain or equivalent).
- If a new non-upstream test file (wallet_crosschain.py) is added in this PR, ensure it’s listed in test/util/data/non-backported.txt per project convention.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Search for DEFAULT_WALLETCROSSCHAIN definition:"
rg -nP '\bDEFAULT_WALLETCROSSCHAIN\b' -C2 || true
echo
echo "Search for code paths that read -walletcrosschain:"
rg -nP 'GetBoolArg\(\s*"-walletcrosschain"' -C3 || true
echo
echo "Locate wallet attach/safety gating references (AttachChain or similar):"
rg -nP '(?s)\bAttachChain\s*\(|wallet.*cross.*chain' -n -C3 src || true
echo
echo "Check for the cross-chain functional test file presence:"
fd -a 'wallet_crosschain.py' || true
echo
if [ -f test/util/data/non-backported.txt ]; then
echo "Verify non-backported list includes test/functional/wallet_crosschain.py (if present):"
if fd -a 'wallet_crosschain.py' | grep -q 'test/functional/wallet_crosschain.py'; then
if rg -n 'test/functional/wallet_crosschain.py' test/util/data/non-backported.txt > /dev/null; then
echo "OK: Listed in non-backported.txt"
else
echo "MISSING: test/functional/wallet_crosschain.py not found in test/util/data/non-backported.txt"
exit 2
fi
else
echo "Note: wallet_crosschain.py not found in repo; skip non-backported check."
fi
fiLength of output: 7001
Add new functional test to non-backported list
The wiring for -walletcrosschain is correct:
- DEFAULT_WALLETCROSSCHAIN is defined in src/wallet/wallet.h:112.
- The flag is consulted in CWallet::AttachChain (src/wallet/wallet.cpp:3095–3102).
However, the new functional test test/functional/wallet_crosschain.py is not listed in test/util/data/non-backported.txt. Please add:
test/functional/wallet_crosschain.py
to test/util/data/non-backported.txt per project convention.
🤖 Prompt for AI Agents
In test/util/data/non-backported.txt around end of file (add new line), add a
single entry for the new functional test:
"test/functional/wallet_crosschain.py". Update the file by appending that exact
path on its own line (no extra characters or comments) so the new test is
included in the non-backported list per project convention.
| std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const CoinsResult& available_coins, | ||
| const CoinSelectionParams& coin_selection_params, bool allow_mixed_output_types, CoinType nCoinType) | ||
| { | ||
| // Run coin selection on each OutputType and compute the Waste Metric | ||
| std::vector<SelectionResult> results; | ||
| if (auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.legacy, coin_selection_params, nCoinType)}) { | ||
| results.push_back(*result); | ||
| } | ||
|
|
||
| // If we can't fund the transaction from any individual OutputType, run coin selection | ||
| // over all available coins, else pick the best solution from the results | ||
| if (results.size() == 0) { | ||
| if (allow_mixed_output_types) { | ||
| if (auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.all(), coin_selection_params, nCoinType)}) { | ||
| return result; | ||
| } | ||
| } | ||
| return std::optional<SelectionResult>(); | ||
| }; | ||
| std::optional<SelectionResult> result{*std::min_element(results.begin(), results.end())}; | ||
| return result; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
AttemptSelection only tries legacy; it never attempts “other” bucket — selection may fail unnecessarily
The intent (and docs) say “run selection on each OutputType,” but the code only runs on available_coins.legacy. If a transaction could be funded entirely from “other” UTXOs, we currently skip that attempt and may incorrectly fall back to mixing (or returning failure when mixing is disallowed).
Add an AttemptSelection pass for available_coins.other before falling back to mixed:
std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const CoinsResult& available_coins,
const CoinSelectionParams& coin_selection_params, bool allow_mixed_output_types, CoinType nCoinType)
{
// Run coin selection on each OutputType and compute the Waste Metric
std::vector<SelectionResult> results;
if (auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.legacy, coin_selection_params, nCoinType)}) {
results.push_back(*result);
}
+ if (auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.other, coin_selection_params, nCoinType)}) {
+ results.push_back(*result);
+ }
// If we can't fund the transaction from any individual OutputType, run coin selection
// over all available coins, else pick the best solution from the results
if (results.size() == 0) {
if (allow_mixed_output_types) {
if (auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.all(), coin_selection_params, nCoinType)}) {
return result;
}
}
return std::optional<SelectionResult>();
};
std::optional<SelectionResult> result{*std::min_element(results.begin(), results.end())};
return result;
};Please add/adjust unit tests to cover:
- AttemptSelection succeeds with only “other” coins available.
- AttemptSelection respects allow_mixed_output_types=false and doesn’t mix when a single-bucket solution exists.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const CoinsResult& available_coins, | |
| const CoinSelectionParams& coin_selection_params, bool allow_mixed_output_types, CoinType nCoinType) | |
| { | |
| // Run coin selection on each OutputType and compute the Waste Metric | |
| std::vector<SelectionResult> results; | |
| if (auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.legacy, coin_selection_params, nCoinType)}) { | |
| results.push_back(*result); | |
| } | |
| // If we can't fund the transaction from any individual OutputType, run coin selection | |
| // over all available coins, else pick the best solution from the results | |
| if (results.size() == 0) { | |
| if (allow_mixed_output_types) { | |
| if (auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.all(), coin_selection_params, nCoinType)}) { | |
| return result; | |
| } | |
| } | |
| return std::optional<SelectionResult>(); | |
| }; | |
| std::optional<SelectionResult> result{*std::min_element(results.begin(), results.end())}; | |
| return result; | |
| }; | |
| std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const CoinsResult& available_coins, | |
| const CoinSelectionParams& coin_selection_params, bool allow_mixed_output_types, CoinType nCoinType) | |
| { | |
| // Run coin selection on each OutputType and compute the Waste Metric | |
| std::vector<SelectionResult> results; | |
| if (auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.legacy, coin_selection_params, nCoinType)}) { | |
| results.push_back(*result); | |
| } | |
| if (auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.other, coin_selection_params, nCoinType)}) { | |
| results.push_back(*result); | |
| } | |
| // If we can't fund the transaction from any individual OutputType, run coin selection | |
| // over all available coins, else pick the best solution from the results | |
| if (results.size() == 0) { | |
| if (allow_mixed_output_types) { | |
| if (auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.all(), coin_selection_params, nCoinType)}) { | |
| return result; | |
| } | |
| } | |
| return std::optional<SelectionResult>(); | |
| }; | |
| std::optional<SelectionResult> result{*std::min_element(results.begin(), results.end())}; | |
| return result; | |
| }; |
🤖 Prompt for AI Agents
In src/wallet/spend.cpp around lines 441-462, AttemptSelection currently only
runs ChooseSelectionResult against available_coins.legacy and then either tries
a mixed selection or fails; add a selection pass for available_coins.other (call
ChooseSelectionResult with available_coins.other and handle its optional result
the same as legacy) before deciding to mix or return failure so single-bucket
“other” solutions are not skipped; ensure the function still prefers the best
single-bucket result (compare legacy and other results) and only falls back to
available_coins.all() when allow_mixed_output_types is true and no single-bucket
solutions exist; add unit tests that (1) verify AttemptSelection succeeds when
only “other” coins are available and (2) verify AttemptSelection with
allow_mixed_output_types=false does not mix when a single-bucket solution
(legacy or other) exists.
| def setup_network(self): | ||
| super().setup_network() | ||
| self.connect_nodes(0, 2) | ||
|
|
There was a problem hiding this comment.
Fully connect nodes; node1 is currently isolated and will cause sync_all hangs and inconsistent assertions
Only nodes 0 and 2 are connected. The test later calls sync_all() (e.g., Lines 356, 377, 418, 441) and iterates over all nodes (Lines 301-306), which requires all three nodes to share the same tip. With node1 isolated, sync_all() can hang and "txn-already-known" assertions can flake/fail on node1.
Connect all nodes in setup_network.
Apply this diff:
def setup_network(self):
super().setup_network()
- self.connect_nodes(0, 2)
+ # Fully mesh the network so sync_all() works and blocks/txs propagate to all nodes
+ self.connect_nodes(0, 1)
+ self.connect_nodes(1, 2)
+ self.connect_nodes(0, 2)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def setup_network(self): | |
| super().setup_network() | |
| self.connect_nodes(0, 2) | |
| def setup_network(self): | |
| super().setup_network() | |
| # Fully mesh the network so sync_all() works and blocks/txs propagate to all nodes | |
| self.connect_nodes(0, 1) | |
| self.connect_nodes(1, 2) | |
| self.connect_nodes(0, 2) |
🤖 Prompt for AI Agents
In test/functional/rpc_rawtransaction.py around lines 71 to 74, setup_network
only connects nodes 0 and 2 leaving node 1 isolated which can cause sync_all()
to hang and produce flaky "txn-already-known" assertions; update setup_network
to fully connect the three nodes (for example call connect_nodes(0,1) and
connect_nodes(1,2) in addition to the existing connect_nodes(0,2)) so all nodes
share the same tip during sync_all and subsequent assertions.
| self.log.info("Test sendrawtransaction/testmempoolaccept with tx already in the chain") | ||
| self.generate(self.nodes[2], 1) | ||
| for node in self.nodes: | ||
| testres = node.testmempoolaccept([rawTxSigned['hex']])[0] | ||
| testres = node.testmempoolaccept([tx['hex']])[0] | ||
| assert_equal(testres['allowed'], False) | ||
| assert_equal(testres['reject-reason'], 'txn-already-known') | ||
| assert_raises_rpc_error(-27, 'Transaction already in block chain', node.sendrawtransaction, rawTxSigned['hex']) | ||
| assert_raises_rpc_error(-27, 'Transaction already in block chain', node.sendrawtransaction, tx['hex']) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Sync blocks before asserting tx is “already in chain” on all nodes
Right after mining a block on node2, the loop checks all nodes for "txn-already-known". Without a sync, node0/node1 may not yet know the block (especially node1 if left unconnected), leading to flakiness.
Add sync_all() after mining.
self.log.info("Test sendrawtransaction/testmempoolaccept with tx already in the chain")
self.generate(self.nodes[2], 1)
+ # Ensure all nodes have the block that confirms the tx
+ self.sync_all()
for node in self.nodes:
testres = node.testmempoolaccept([tx['hex']])[0]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.log.info("Test sendrawtransaction/testmempoolaccept with tx already in the chain") | |
| self.generate(self.nodes[2], 1) | |
| for node in self.nodes: | |
| testres = node.testmempoolaccept([rawTxSigned['hex']])[0] | |
| testres = node.testmempoolaccept([tx['hex']])[0] | |
| assert_equal(testres['allowed'], False) | |
| assert_equal(testres['reject-reason'], 'txn-already-known') | |
| assert_raises_rpc_error(-27, 'Transaction already in block chain', node.sendrawtransaction, rawTxSigned['hex']) | |
| assert_raises_rpc_error(-27, 'Transaction already in block chain', node.sendrawtransaction, tx['hex']) | |
| self.log.info("Test sendrawtransaction/testmempoolaccept with tx already in the chain") | |
| self.generate(self.nodes[2], 1) | |
| # Ensure all nodes have the block that confirms the tx | |
| self.sync_all() | |
| for node in self.nodes: | |
| testres = node.testmempoolaccept([tx['hex']])[0] | |
| assert_equal(testres['allowed'], False) | |
| assert_equal(testres['reject-reason'], 'txn-already-known') | |
| assert_raises_rpc_error(-27, 'Transaction already in block chain', node.sendrawtransaction, tx['hex']) |
🤖 Prompt for AI Agents
In test/functional/rpc_rawtransaction.py around lines 299 to 306, after
generating a block on self.nodes[2] the test immediately checks all nodes for
txn-already-known which can be flaky because other nodes may not have the new
block yet; call self.sync_all() (or otherwise sync the nodes) immediately after
self.generate(self.nodes[2], 1) so all nodes see the new block before asserting
testmempoolaccept/sendrawtransaction outcomes.
src/wallet/scriptpubkeyman.cpp
Outdated
| @@ -291,14 +291,16 @@ bool LegacyScriptPubKeyMan::Encrypt(const CKeyingMaterial& master_key, WalletBat | |||
| return true; | |||
| } | |||
|
|
|||
| bool LegacyScriptPubKeyMan::GetReservedDestination(bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool) | |||
| bool LegacyScriptPubKeyMan::GetReservedDestination(bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool, bilingual_str& error) | |||
There was a problem hiding this comment.
Why using bilingual_str instead of std::string and .translated?
Additional Information
The
CKeyoverload forCreate{,AndProcess}Block()was introduced as a convenience function in dash#2264. To allow for the easier backport of bitcoin#24584 (by resolving an ambiguous overload error) and becauseGetScriptForRawPubKeymakes the same construction, it was opted to realign with upstream and drop the overload.Even though we don't actively support any address types that utilize Bech32m, bitcoin#25656 assumes that
GetReservedDestinationmodifies abilingual_strfor its error case in order to encapsulate it in autil::Result. In order to complete that backport, bitcoin#22154 has been partially backported.CKeyHolderhas always assumed thatGetReservedDestinationwill succeed without validation of this assumption (source). As bitcoin#25656 forces us to unwrap the returned value, this assumption has been documented with anassert(source)When backporting bitcoin#21166, we don't have the luxury of a
scriptWitnessto include the redemption script, sorpc_signrawtransaction.pymust import the script in order to spend it. This creates an additional complication as descriptor wallets do not permit wallets to mix descriptors with and without private keys (source) and as P2SH descriptors don't involve a private key, those specific subtests need to be skipped for descriptor wallets.OP_TRUEhas been appended to the script, this mirrors theOP_TRUEin thescriptWitness(source)The subtest
test_signing_with_missing_prevtx_info()introduced in bitcoin#25044 requires an extrawalletpassphraseas the preceding tests that unlock the wallet are not called for descriptor wallets, making the call necessary. It is redundant but harmless for legacy wallets.With the backport of bitcoin#19937 in dash#6726 (as ae7e4cb), we can remove a workaround in
feature_nulldummy.pywhere we used two nodes to satisfygetblocktemplate's requirement of a connected node as that requirement was lifted on test chains.Breaking Changes
None expected.
How Has This Been Tested?
A full CoinJoin session run on c0f9225
Checklist