backport: merge bitcoin#22100, #25148, #22941, #20591, #23596, #22868, #24225 (wallet backports: part 3) #6633
Conversation
WalkthroughThis set of changes introduces a significant refactoring and modularization of wallet-related logic, especially around transaction processing, coin selection, and CoinJoin functionality. Many methods that were previously member functions of ✨ Finishing Touches
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:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (6)
src/wallet/rpcwallet.cpp (1)
730-745:⚠️ Potential issue
addlockedparameter is silently ignored ingetbalanceThe RPC handler still parses the
addlockedflag (lines 735‑739) but the value is never forwarded toGetBalance(...), so users invoking
getbalance "*"<minconf> <addlocked=true>will always get the same result as withaddlocked=false.This is a behavioural regression compared to the previous implementation where
addlockedinfluenced the balance calculation. Either:
- Re‑introduce support by extending
GetBalance(...)to accept the flag, or- Remove the parameter from the help string and argument parsing to avoid a misleading API.
- const UniValue& addlocked = request.params[2]; - bool fAddLocked = false; - if (!addlocked.isNull()) { - fAddLocked = addlocked.get_bool(); - } ... - const auto bal = GetBalance(*pwallet, min_depth, avoid_reuse); + // TODO: addlocked currently unsupported – decide whether to wire it through + const auto bal = GetBalance(*pwallet, min_depth, /*avoid_reuse=*/avoid_reuse /*, fAddLocked */);Failing to address this causes incorrect wallet balances for users relying on InstantSend‑locked transactions.
src/wallet/spend.cpp (3)
380-385: 🛠️ Refactor suggestion
⚠️ Potential issueDead code left behind – constant
false && ...if (false && SelectCoinsBnB(...)) { ... }This block will never execute and will be optimised away, but the side‑effects inside it (waste calculation) are still compiled, increasing maintenance cost and confusing readers.
Remove the check or gate execution with a real feature flag (e.g.
g_select_coins_bnb_enabled).
586-637: 🛠️ Refactor suggestion
⚠️ Potential issueOver‑aggressive
assert()s may abort well‑formed wallets
DiscourageFeeSniping()asserts that every input sequence is either
MAX_SEQUENCE_NONFINALorSEQUENCE_FINAL - 1. Any future feature that
needs a different relative‑lock‑time (e.g. BIP‑68) would trigger a fatal
assert and bring the node down.Consider downgrading these to
CHECK_NONFATALor returning an error so the
caller can surface a meaningful message instead of crashing the process.- assert(false); + return; // or CHECK_NONFATAL(false) with proper logging
36-44: 💡 Verification agent🧩 Analysis chain
Same locking issue for
CalculateMaximumSignedInputSize()This helper performs an in‑place dummy signature which mutates
txn. While that object is local, the call towallet->DummySignInput()again touches wallet state without holdingcs_wallet.Either:
- Add the same lock annotation (preferred), or
- Document that the caller must already be holding the lock (but then every call‑site has to comply).
Currently callers such as
GetTxSpendSize()(see previous comment) do not hold the lock – producing an unsound contract.
🏁 Script executed:
#!/bin/bash set -e echo "Searching for DummySignInput definition and annotations:" rg -n -C3 "DummySignInput" .Length of output: 2188
Enforce Wallet Locking for CalculateMaximumSignedInputSize
CalculateMaximumSignedInputSize()invokeswallet->DummySignInput(), which touches shared wallet state without holdingcs_wallet. To avoid data races and make the locking contract explicit, apply one of the following fixes:• Preferred (annotation):
- In src/wallet/spend.cpp, add
EXCLUSIVE_LOCKS_REQUIRED(cs_wallet)to theCalculateMaximumSignedInputSizesignature so callers must hold the lock.- In src/wallet/wallet.h, annotate
CWallet::DummySignInputwithEXCLUSIVE_LOCKS_REQUIRED(cs_wallet)to document its requirement.Example diffs:
--- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -int CalculateMaximumSignedInputSize(const CTxOut& txout, const CWallet* wallet, bool use_max_sig) +int CalculateMaximumSignedInputSize(const CTxOut& txout, const CWallet* wallet, bool use_max_sig) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { CMutableTransaction txn; txn.vin.push_back(CTxIn(COutPoint()));--- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -709,7 +709,7 @@ class CWallet : public WalletStorage - bool DummySignInput(CTxIn &tx_in, const CTxOut &txout, bool use_max_sig = false) const; + bool DummySignInput(CTxIn &tx_in, const CTxOut &txout, bool use_max_sig = false) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);• Alternative (in‐function lock):
Acquire the lock insideCalculateMaximumSignedInputSize()before callingDummySignInput:int CalculateMaximumSignedInputSize(...) { LOCK(cs_wallet); // … }Either approach closes the unsound contract where callers like
GetTxSpendSize()currently call these helpers without any wallet lock.src/wallet/wallet.cpp (2)
1820-1837:⚠️ Potential issueAttempt to mutate a
constparameter breaks compilation
SubmitTxMemoryPoolAndRelay()now takes the wallet‑tx as aconst CWalletTx&, yet the body alterswtx.fInMempooldirectly (wtx.fInMempool |= ret;).
On a non‑mutablemember this is illegal and will not compile (earlier code usedconst_castfor this very reason).- bool ret = chain().broadcastTransaction(wtx.tx, m_default_max_tx_fee, relay, err_string); - wtx.fInMempool |= ret; + bool ret = chain().broadcastTransaction(wtx.tx, m_default_max_tx_fee, relay, err_string); + // fInMempool is NOT mutable, so cast‑away constness explicitly + const_cast<CWalletTx&>(wtx).fInMempool |= ret;Alternatively declare
fInMempoolasmutableinsideCWalletTx, but that is a wider‑ranging change.
2556-2613:⚠️ Potential issue
ComputeTimeSmartre‑declared – ODR violationA full copy of
CWallet::ComputeTimeSmart()already exists ~1 000 lines above.
Keeping two identical bodies in the same TU violates the One‑Definition‑Rule and the file will fail to build.Remove this second copy or move the implementation to a common header/inline file.
-unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx, bool rescanning_old_block) const -{ - ... -}
♻️ Duplicate comments (1)
src/wallet/spend.h (1)
108-110: Update prototype to match const‑reference changeAfter switching
AttemptSelectionto takecoinsbyconst std::vector<COutput>&
(see comment in.cpp), remember to change the header accordingly to avoid
ODR mismatches.
🧹 Nitpick comments (11)
src/wallet/rpcwallet.cpp (2)
533-535: Possible double‑work & stale cache look‑ups in balance helpersBoth
listaddressgroupingsandlistaddressbalancescallGetAddressBalances(*pwallet)before iterating through the (potentially large) data structures that they themselves build.
GetAddressBalancesalready walks the entire wallet; the subsequent loops repeat similar work, so the initial call can be an expensive no‑op if its results are not reused elsewhere.Consider:
- std::map<CTxDestination, CAmount> balances = GetAddressBalances(*pwallet); + // Fetch balances lazily when needed instead of up‑front. + const auto balances = GetAddressBalances(*pwallet);and remove any redundant invocations in tight loops. This will shave noticeable latency on wallets with many transactions.
Also applies to: 476-478
3932-3936:ScriptIsChangenow a free function – minor naming mismatch
ScriptIsChange(*pwallet, scriptPubKey)is called correctly, but the variable nameischangein the resulting
JSON (line 3934) may mislead callers into believing it is a wallet‑agnostic property.
To avoid confusion, consider exposing it asis_changeor document that the flag is wallet‑specific.src/wallet/transaction.h (1)
117-121: Constructor should be markedexplicitand take the argument byconst&
CTransactionRefis an alias forstd::shared_ptr<const CTransaction>.
Passing it by value causes an extra atomicadd_ref/releasepair, and the
lack ofexplicitallows implicit conversions such asCWalletTx wtx = MakeTransactionRef(...); // implicitwhich is rarely what we want.
- CWalletTx(CTransactionRef arg) + explicit CWalletTx(const CTransactionRef& arg)This reduces one atomic op on every construction and prevents unintended
conversions.src/wallet/coinjoin.cpp (1)
68-69:Shuffleon reverse‑iterators is confusing – use forward iterators insteadUsing
Shuffle(vCoins.rbegin(), vCoins.rend(), …)compiles, but readers
typically expectbegin()/end(). Becausestd::shufflepermutes the supplied
range in‑place, shuffling the reverse range first and then traversingvCoins
forward produces a different permutation than shuffling the forward range
directly.Unless a reversed shuffle is intentional (and documented), prefer the simpler
form:- Shuffle(vCoins.rbegin(), vCoins.rend(), FastRandomContext()); + Shuffle(vCoins.begin(), vCoins.end(), FastRandomContext());src/wallet/spend.cpp (1)
366-369: Avoid copying the wholecoinsvector
AttemptSelection()takes itscoinsargument by value, immediately making a full copy that can be several megabytes for large wallets. The function never mutates the vector; a const‑reference is sufficient and avoids the copy.-bool AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, - const CoinEligibilityFilter& eligibility_filter, std::vector<COutput> coins, +bool AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, + const CoinEligibilityFilter& eligibility_filter, const std::vector<COutput>& coins, std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params, CoinType nCoinType)Do not forget to adjust the declaration in
spend.hand all call‑sites.
[performance]🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 366-366: Function parameter 'coins' should be passed by const reference.
(passedByValue)
src/wallet/spend.h (1)
68-76:CalculateMaximumSignedTxSizelock annotation dereferences a potentially null pointerThe annotation uses
wallet->cs_walletbut the parameter is a raw pointer.
If callers ever passnullptrthe attribute triggers UB during static‑analysis
macro expansion. Either enforce non‑null (withassert(wallet)) or switch to
reference semantics.-int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, - bool use_max_sig = false) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet); +int64_t CalculateMaximumSignedTxSize(const CTransaction& tx, const CWallet& wallet, + bool use_max_sig = false) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);This also harmonises the API with the rest of the free‑function style which
passesCWallet&.src/wallet/wallet.cpp (1)
903-916: Tiny optimisation/nit – avoid double lookup
mapWallet.emplace(..., std::forward_as_tuple(tx))already yields an
iterator; you later dereference it immediately to obtainwtx.
Prefer structured binding to make the intent clearer and avoid the extra(*ret.first)dereference.- auto ret = mapWallet.emplace(std::piecewise_construct, - std::forward_as_tuple(hash), - std::forward_as_tuple(tx)); - CWalletTx& wtx = (*ret.first).second; + auto [it, inserted] = mapWallet.emplace(std::piecewise_construct, + std::forward_as_tuple(hash), + std::forward_as_tuple(tx)); + CWalletTx& wtx = it->second;src/wallet/wallet.h (2)
498-506: NO_THREAD_SAFETY_ANALYSIS masks real racesThe trio of helpers (
GetTxConflicts,GetTxDepthInMainChain, …) deliberately skip clang‑TSA (thread safety analysis).
This was necessary whenCWalletwas an incomplete type, but the surrounding refactor removed that restriction. Keeping the suppression hides potential data‐race regressions introduced by this back‑port.Remove
NO_THREAD_SAFETY_ANALYSISand add the proper
EXCLUSIVE_LOCKS_REQUIRED(cs_wallet)annotation instead. The compiler will
now guide us if future edits forget to hold the lock.
698-702:CanTxBeResent/SubmitTxMemoryPoolAndRelaycould beconstqualifiedBoth helpers only read state; marking them
constmakes intent explicit and
prevents accidental mutations during rebroadcast logic.src/wallet/receive.cpp (2)
12-24: Thread‑safety annotation missing
InputIsMine()assertscs_walletis held but does not advertise this
requirement to the compiler. Mark it (and the other helpers) with
EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)to catch misuse early.
217-234: Lock held during potentially expensive extraction
CachedTxGetAmounts()grabswallet.cs_walletfor the whole
sent/received‑loop (lines 233‑234).ExtractDestination()does heavy script
parsing and could stall other wallet threads. Consider copying the required
voutslice first (still under lock), then unlocking while doing the costly
parsing and bookkeeping.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
src/Makefile.am(2 hunks)src/bench/coin_selection.cpp(4 hunks)src/bench/wallet_balance.cpp(2 hunks)src/coinjoin/client.cpp(3 hunks)src/coinjoin/util.cpp(2 hunks)src/rpc/coinjoin.cpp(3 hunks)src/rpc/evo.cpp(3 hunks)src/rpc/governance.cpp(2 hunks)src/rpc/masternode.cpp(2 hunks)src/wallet/coincontrol.h(1 hunks)src/wallet/coinjoin.cpp(1 hunks)src/wallet/coinjoin.h(1 hunks)src/wallet/context.h(1 hunks)src/wallet/interfaces.cpp(10 hunks)src/wallet/load.cpp(1 hunks)src/wallet/receive.cpp(10 hunks)src/wallet/receive.h(1 hunks)src/wallet/rpcwallet.cpp(22 hunks)src/wallet/spend.cpp(39 hunks)src/wallet/spend.h(3 hunks)src/wallet/test/coinjoin_tests.cpp(2 hunks)src/wallet/test/coinselector_tests.cpp(5 hunks)src/wallet/test/psbt_wallet_tests.cpp(1 hunks)src/wallet/test/spend_tests.cpp(2 hunks)src/wallet/test/wallet_tests.cpp(20 hunks)src/wallet/transaction.h(3 hunks)src/wallet/wallet.cpp(22 hunks)src/wallet/wallet.h(7 hunks)src/wallet/walletdb.cpp(1 hunks)test/functional/test_runner.py(1 hunks)test/functional/wallet_basic.py(1 hunks)test/functional/wallet_transactiontime_rescan.py(1 hunks)test/lint/lint-circular-dependencies.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (8)
src/bench/wallet_balance.cpp (2)
src/wallet/receive.h (1)
GetBalance(62-62)src/wallet/receive.cpp (2)
GetBalance(319-356)GetBalance(319-319)
src/wallet/test/spend_tests.cpp (1)
src/wallet/spend.cpp (2)
CreateTransaction(965-1019)CreateTransaction(965-975)
src/bench/coin_selection.cpp (1)
src/wallet/spend.cpp (2)
AttemptSelection(366-414)AttemptSelection(366-367)
src/coinjoin/client.cpp (4)
src/wallet/receive.h (1)
GetBalance(62-62)src/wallet/receive.cpp (2)
GetBalance(319-356)GetBalance(319-319)src/wallet/spend.cpp (3)
AvailableCoins(71-197)AvailableCoins(71-71)vCoins(420-420)src/wallet/spend.h (1)
AvailableCoins(81-81)
src/rpc/evo.cpp (2)
src/wallet/spend.cpp (4)
AvailableCoins(71-197)AvailableCoins(71-71)CreateTransaction(965-1019)CreateTransaction(965-975)src/wallet/spend.h (2)
AvailableCoins(81-81)CreateTransaction(127-127)
src/wallet/test/coinselector_tests.cpp (2)
src/wallet/spend.cpp (4)
GroupOutputs(278-364)GroupOutputs(278-278)SelectCoins(416-566)SelectCoins(416-416)src/wallet/spend.h (2)
GroupOutputs(95-95)SelectCoins(119-120)
src/wallet/spend.cpp (6)
src/wallet/wallet.h (45)
wallet(399-399)wtx(292-292)wtx(505-505)wtx(519-519)wtx(520-520)wtx(520-520)wtx(522-522)wtx(523-523)wtx(530-530)wtx(531-531)wtx(602-602)wtx(699-699)wtx(701-701)LOCK(819-819)tx(300-300)tx(319-319)tx(325-325)tx(329-329)tx(621-621)tx(623-623)tx(645-645)tx(654-654)tx(656-656)tx(696-696)tx(793-793)tx(795-795)tx(796-796)tx(918-918)output(564-564)output(565-565)filter(762-762)outpoint(291-291)outpoint(546-546)outpoint(548-548)outpoint(552-552)outpoint(553-553)txout(792-792)txin(791-791)hash(497-497)hash(555-555)hash(558-558)hash(561-561)hash(563-563)hash(622-622)hash(662-662)src/wallet/spend.h (11)
CalculateMaximumSignedInputSize(69-69)COutput(50-58)AvailableCoins(81-81)FindNonChangeParentOutput(88-88)GroupOutputs(95-95)GetTxSpendSize(14-14)AttemptSelection(108-109)SelectCoins(119-120)CalculateMaximumSignedTxSize(75-75)CalculateMaximumSignedTxSize(76-76)CreateTransaction(127-127)src/wallet/coincontrol.h (7)
nCoinType(111-114)output(79-82)output(79-79)output(84-87)output(84-84)output(89-92)output(89-89)src/wallet/receive.cpp (8)
CachedTxIsTrusted(279-310)CachedTxIsTrusted(279-279)CachedTxIsTrusted(312-317)CachedTxIsTrusted(312-312)OutputIsChange(88-91)OutputIsChange(88-88)CachedTxIsFromMe(274-277)CachedTxIsFromMe(274-274)src/wallet/transaction.h (2)
tx(249-249)tx(269-269)src/wallet/coinselection.cpp (2)
KnapsackSolver(246-354)KnapsackSolver(246-246)
src/wallet/wallet.cpp (1)
src/wallet/wallet.h (49)
tx(300-300)tx(319-319)tx(325-325)tx(329-329)tx(621-621)tx(623-623)tx(645-645)tx(654-654)tx(656-656)tx(696-696)tx(793-793)tx(795-795)tx(796-796)tx(918-918)hash(497-497)hash(555-555)hash(558-558)hash(561-561)hash(563-563)hash(622-622)hash(662-662)wtx(292-292)wtx(505-505)wtx(519-519)wtx(520-520)wtx(520-520)wtx(522-522)wtx(523-523)wtx(530-530)wtx(531-531)wtx(602-602)wtx(699-699)wtx(701-701)batch(334-334)batch(337-337)batch(340-340)batch(559-559)batch(608-608)batch(808-808)batch(811-811)block(624-624)block(625-625)block_height(1019-1019)WalletLogPrintf(963-965)WalletLogPrintf(963-963)assert(408-412)assert(490-490)assert(493-493)assert(1072-1083)
🪛 GitHub Actions: Clang Diff Format Check
src/coinjoin/util.cpp
[error] 7-17: Clang format differences found. The include directive for <util/translation.h> is misplaced and needs reordering.
src/rpc/evo.cpp
[error] 276-278: Clang format differences found. The if statement parameters should be split across two lines for proper formatting.
🪛 Cppcheck (2.10-2)
src/wallet/spend.cpp
[performance] 366-366: Function parameter 'coins' should be passed by const reference.
(passedByValue)
🔇 Additional comments (70)
src/wallet/load.cpp (1)
19-19: Include directive added for wallet spend functionality.The added include of
<wallet/spend.h>aligns with the broader refactoring that moves wallet member functions to free functions, ensuring that code inload.cppcan access the wallet spending functions that have been moved to their own module.src/wallet/context.h (1)
41-42: Important locking order comment added to prevent deadlocks.This warning comment is crucial for thread safety as it explicitly documents the locking order constraints. When converting wallet member functions to free functions that take explicit wallet references, proper mutex locking order becomes even more important to maintain.
test/functional/test_runner.py (1)
208-209: New functional tests added for transaction time during wallet rescans.The addition of these tests ensures that transaction time handling during wallet rescans works correctly for both descriptor and legacy wallets. This is particularly important given the wallet refactoring in this PR.
src/wallet/coincontrol.h (1)
16-16: Explicit uint8_t type specification for CoinType enum.Explicitly specifying the enum's underlying type as
uint8_tis a good optimization that:
- Reduces the memory footprint from the default size (typically 4 bytes) to 1 byte
- Makes the code more explicit about the intended type size
- Ensures consistent enum size across different platforms and compilers
This change improves code clarity and efficiency without affecting functionality.
test/functional/wallet_basic.py (1)
655-655: Comment updated to reference the new coin selection function name.The comment has been updated to refer to "AttemptSelection" instead of "SelectCoinsMinConf", matching the wallet refactoring where coin selection functions were moved from member methods to free functions.
src/wallet/test/psbt_wallet_tests.cpp (2)
35-35: Construction of CWalletTx no longer takes wallet pointer.This change removes the wallet pointer from CWalletTx construction, aligning with the refactoring that removes the
CWalletTx::pwalletmember. This helps eliminate circular dependencies between wallet components.
40-40: Construction of CWalletTx no longer takes wallet pointer.This change removes the wallet pointer from CWalletTx construction, consistent with the previous change and the broader refactoring effort to simplify the CWalletTx class.
src/coinjoin/util.cpp (2)
12-12: Added include for wallet/spend.h to provide CreateTransaction function declaration.Adding this include is necessary to provide the declaration of the free function CreateTransaction that's now used in this file.
279-279: Updated to use free function instead of member function.The code now calls the free function
CreateTransactionthat takes the wallet as an explicit first argument, instead of using it as a member functionm_wallet.CreateTransaction(). This is part of the broader refactoring to move wallet transaction methods to free functions.src/wallet/test/coinjoin_tests.cpp (2)
18-18: Added include for wallet/spend.h to provide CreateTransaction function declaration.This addition is necessary to provide the declaration for the free function CreateTransaction that's now used in this file.
197-197: Updated to use free function instead of member function.The code now calls the free function
CreateTransactionthat takes the wallet as an explicit first argument (*wallet), instead of using it as a member function (wallet->CreateTransaction()). This is consistent with the wallet refactoring that moves transaction creation functions from member methods to free functions.src/bench/wallet_balance.cpp (2)
12-12: Added header for wallet balance functionality.This include is necessary to access the free function
GetBalancethat's now being used instead of the member function.
39-39: Replaced wallet member function call with a free function.The code now uses
GetBalance(wallet)instead of the previous member function callwallet.GetBalance(). This change is part of the broader refactoring that moves wallet-dependent logic out of the member functions and into free functions with explicit wallet references.The change improves modularity and makes the wallet code more testable by explicitly passing wallet references.
Also applies to: 43-43
src/wallet/walletdb.cpp (1)
971-971: Updated CWalletTx construction to match new signature.The
CWalletTxconstructor now takes a singlenullptrparameter (for the transaction) instead of two parameters (wallet and transaction). This aligns with the removal of theCWalletTx::pwalletmember as mentioned in the PR objectives.This change is part of a larger refactoring effort to eliminate circular dependencies between wallet files.
src/Makefile.am (2)
395-395: Added CoinJoin header to core headers list.Added
wallet/coinjoin.htoBITCOIN_CORE_Hto make CoinJoin wallet functionality available to the rest of the codebase.This change properly integrates the new CoinJoin module into the build system.
595-595: Added CoinJoin implementation to wallet library sources.Added
wallet/coinjoin.cpptolibbitcoin_wallet_a_SOURCESto include the implementation of CoinJoin wallet functionality in the wallet library.This ensures that the new CoinJoin wallet features are compiled and linked as part of the wallet library, completing the module integration.
src/rpc/coinjoin.cpp (2)
15-15: Added header for wallet balance functionality.This include is necessary to access the free function
GetBalancethat's now being used instead of the member function.
282-284: Refactored wallet balance retrieval to use free function.The code now uses
GetBalance(*wallet)instead of the previous member function callwallet->GetBalance(). This change is consistent with the wallet refactoring pattern across the codebase.This change is part of the architectural shift described in the PR objectives, where wallet-dependent logic is moved out of member functions and into free functions that take explicit wallet references.
Also applies to: 384-386
test/lint/lint-circular-dependencies.py (1)
87-87: Added circular dependency for the new wallet/coinjoin moduleThis change adds a new expected circular dependency between
wallet/coinjoinandwallet/receive, which aligns with the PR objective of moving CoinJoin-specific code to its own source file. The circular dependency indicates that the new module structure has interdependencies that can't be easily eliminated.src/rpc/masternode.cpp (2)
30-30: Include wallet/spend.h for AvailableCoins declarationAdded include for
wallet/spend.hto provide the declaration for the free function version ofAvailableCoins.
139-139: Refactored wallet member function to free functionChanged the wallet member function call
wallet->AvailableCoins()to the free function callAvailableCoins(*wallet, ...), which takes the wallet as an explicit parameter. This modification follows the wallet refactoring pattern to reduce circular dependencies in the codebase.src/wallet/test/spend_tests.cpp (2)
8-8: Include wallet/spend.h for CreateTransaction declarationAdded include for
wallet/spend.hto provide the declaration for the free functionCreateTransaction.
36-36: Refactored wallet member function to free functionUpdated test code to use the new free function
CreateTransaction(*wallet, ...)instead of the member functionwallet->CreateTransaction(...). This change follows the wallet refactoring pattern to eliminate circular dependencies and improve code modularity.src/rpc/governance.cpp (2)
31-31: Include wallet/spend.h for GenBudgetSystemCollateralTx declarationAdded include for
wallet/spend.hto provide the declaration for the free functionGenBudgetSystemCollateralTx.
222-222: Renamed and refactored wallet member function to free functionChanged the wallet member function call
wallet->GetBudgetSystemCollateralTX()to the free function callGenBudgetSystemCollateralTx(*wallet, ...). This change not only follows the wallet refactoring pattern but also renames the function to better reflect its purpose as a generator function.src/rpc/evo.cpp (2)
39-39: Appropriate include added for free function declarations.The addition of
wallet/spend.his necessary to access the free function declarations ofAvailableCoinsandCreateTransactionwhich are now used instead of member functions.
260-260: Correctly updated to use the free function API.This change properly updates the coin selection call to use the new free function
AvailableCoinswhich takes the wallet as its first parameter, aligning with the wallet refactoring to move from member functions to free functions.src/coinjoin/client.cpp (3)
27-30: Appropriate includes added for free function declarations.The addition of these three wallet-related headers is necessary to access the free function declarations that replace the wallet member functions. This change aligns with the broader wallet API refactoring that moves from member functions to free functions with explicit wallet parameters.
850-850: Correctly updated to use the GetBalance free function.This change properly updates the balance retrieval call to use the new free function
GetBalancewhich takes the wallet as its first parameter, replacing the previous member function approach. This is consistent with the wallet refactoring pattern applied throughout the codebase.
1572-1572: Correctly updated to use the AvailableCoins free function.This change properly updates the coin selection call to use the new free function
AvailableCoinswhich takes the wallet as its first parameter, replacing the previous member function approach. This is consistent with the wallet refactoring pattern applied throughout the codebase.src/bench/coin_selection.cpp (4)
9-9: Appropriate include added for free function declarations.The addition of
wallet/spend.his necessary to access the free function declarations, particularlyAttemptSelection.
21-21: Correctly updated CWalletTx constructor to remove wallet pointer.This change removes the wallet pointer parameter from the CWalletTx constructor, reflecting the removal of the unused
CWalletTx::pwalletmember mentioned in the PR objectives. This is part of the architectural update to simplify wallet transaction handling.
48-48: Correctly updated COutput constructor to include wallet reference.This change adds the wallet reference as the first parameter to the COutput constructor, which is necessary since COutput now needs the wallet reference explicitly instead of obtaining it through CWalletTx.
58-58: Correctly updated to use the AttemptSelection free function.This change properly updates the coin selection call to use the new free function
AttemptSelectionwhich takes the wallet as its first parameter, replacing the previous wallet member function approach. This is consistent with the wallet refactoring pattern applied throughout the codebase.src/wallet/test/wallet_tests.cpp (7)
30-31: Architecture improvement: Added header files to support new free function interfaces.The addition of
receive.handspend.hreflects the architectural shift to modular free functions that take explicit wallet references instead of member functions. This change improves separation of concerns and reduces circular dependencies between wallet components.
131-132: Consistent refactoring from member functions to free functions.The wallet balance query has been converted from a member function call
wallet.GetBalance()to a free functionGetBalance(wallet)that takes an explicit wallet reference. This pattern is applied consistently throughout the file, improving modularity and maintainability.Also applies to: 151-152, 180-181, 207-208
354-355: CWalletTx constructor no longer takes wallet pointer.The
CWalletTxconstructor signature has been updated to remove the wallet pointer parameter, consistent with the broader refactoring to eliminate theCWalletTx::pwalletmember. This simplifies ownership relationships and reduces coupling between wallet transactions and the wallet.
87-88: Updated COutput constructor to include wallet reference.The
COutputconstructor now takes an explicit wallet reference as its first parameter instead of relying on wallet pointer stored inCWalletTx. This change supports the pattern of explicit dependencies rather than implicit ones.
575-576: Wallet coin listing changed to use free function.The
ListCoinsoperation has been converted from a member function to a free function, consistent with the pattern used for other wallet operations. This maintains consistency in the refactored API.Also applies to: 591-592
601-602: Available coins retrieval converted to free function.Similar to other wallet operations, the
AvailableCoinsfunctionality has been moved from a member function to a free function that takes the wallet as an explicit parameter.Also applies to: 613-614
545-546: Transaction creation moved to free function.The
CreateTransactionmethod has been converted from a member function to a free function, consistent with the refactoring of other wallet operations. This change helps reduce circular dependencies and improves modularity.src/wallet/test/coinselector_tests.cpp (4)
15-15: Required header for accessing free functions.Added
wallet/spend.hto access the new free function declarations for coin selection and grouping that were previously defined as CWallet member functions.
143-144: Converted GroupOutputs to use free function interface.Changed from calling the member function
wallet.GroupOutputs(...)to using the free functionGroupOutputs(wallet, ...)that takes the wallet as an explicit parameter. This is consistent with the broader wallet refactoring pattern.
324-325: SelectCoins function call updated to free function interface.Updated from member function call
wallet->SelectCoins(...)to free functionSelectCoins(*wallet, ...)that takes a wallet reference. This matches the new wallet API design that favors explicit dependencies over implicit member function calls.
87-88: Updated COutput constructor with explicit wallet reference.The
COutputconstructor now takes the wallet as its first parameter, replacing the previous pattern where the wallet reference was obtained through the transaction object. This aligns with the design goals of making dependencies explicit.test/functional/wallet_transactiontime_rescan.py (5)
1-27: Well-structured test for transaction time handling during rescanning.This test verifies that transaction time metadata is correctly preserved during wallet rescans. The test design with three separate nodes (miner, user, and restore) provides a clean separation of concerns for generating, receiving, and rescanning transactions.
106-116: Thorough validation of transaction timestamps.The test properly validates both
blocktimeandtimefields to ensure transaction timestamps are correctly recorded and match the expected values. This verification is crucial to ensure wallet rescanning doesn't alter transaction timing metadata.
127-130: Careful handling of descriptor wallet behavior.The test includes specific handling for descriptor wallets to prevent automatic rescanning when importing addresses. This shows attention to detail for different wallet implementations and prevents false test failures.
139-143: Tests both partial and full rescans.Testing both a partial rescan (from a specific height) and a full rescan provides thorough coverage of the rescanning functionality and helps validate that transaction times are preserved correctly in both scenarios.
149-159: Verification after rescan ensures data consistency.The final verification ensures that transaction timestamps remain consistent after rescanning, validating that the refactored wallet code correctly preserves transaction metadata during the rescan process.
src/wallet/coinjoin.h (4)
1-8: Modular header for CoinJoin functionality.This new header file properly encapsulates CoinJoin-related functionality into a dedicated module, improving separation of concerns and reducing coupling with the main wallet code.
18-23: Effective logging macro design.The
WalletCJLogPrintmacro is well-designed, allowing conditional evaluation of arguments only when logging is enabled for the CoinJoin category. This improves performance by avoiding unnecessary string formatting when logging is disabled.
27-32: Thread safety annotation needs to be addressed.The TODO comment correctly identifies an issue with thread safety annotations. The
NO_THREAD_SAFETY_ANALYSISannotation should be replaced with properEXCLUSIVE_LOCKS_REQUIREDannotations once the circular dependency issues are resolved.Please make sure to address this TODO in a future commit to ensure proper thread safety enforcement.
34-42: Clean struct definition for CoinJoin credits.The
CoinJoinCreditsstruct provides a clear organization of anonymized and denominated amounts along with confirmation status. The functionCachedTxGetAvailableCoinJoinCreditsfollows the established pattern of taking explicit wallet references, consistent with the rest of the wallet refactoring.src/wallet/interfaces.cpp (14)
24-31: Added necessary includes for refactored functionalityThe new includes for coinjoin.h, receive.h, and spend.h are appropriate as part of the wallet refactoring, where functionality was moved from CWalletTx methods to free functions organized by category.
64-64: Approve function replacement for ownership checksReplaced direct wallet method with the free function
InputIsMine()that takes the wallet as an explicit parameter, aligning with the overall refactoring goal to reduce CWalletTx dependencies.
82-84: Approve function replacements for transaction attributesReplaced transaction methods with free functions that operate on the transaction with explicit wallet parameter:
wtx.GetCredit()→CachedTxGetCredit(wallet, wtx, ...)wtx.GetDebit()→CachedTxGetDebit(wallet, wtx, ...)wtx.GetChange()→CachedTxGetChange(wallet, wtx)These changes support the removal of CWalletTx::pwallet dependency as outlined in the PR objectives.
102-103: Methods moved from transaction to wallet classRelocated transaction state methods to the wallet class, following the refactoring pattern to have the wallet explicitly maintain transaction state:
wtx.GetBlocksToMaturity()→wallet.GetTxBlocksToMaturity(wtx)wtx.GetDepthInMainChain()→wallet.GetTxDepthInMainChain(wtx)
107-107: Replaced trust verification methodChanged from transaction method to a free function that explicitly takes the wallet:
wtx.IsTrusted()→CachedTxIsTrusted(wallet, wtx)
110-112: Transaction status methods moved to wallet classMoved chain status verification methods from transaction to wallet class with renamed methods that follow consistent naming patterns:
wtx.IsInMainChain()→wallet.IsTxInMainChain(wtx)wtx.IsChainLocked()→wallet.IsTxChainLocked(wtx)wtx.LockedByInstantSend()→wallet.IsTxLockedByInstantSend(wtx)This aligns with the PR objectives to move these methods to CWallet and rename them for consistency.
281-282: Transaction creation refactored to free functionChanged from wallet member method to a free function:
m_wallet->CreateTransaction(...)→CreateTransaction(*m_wallet, ...)This maintains the same functionality while supporting better modularity and separation of concerns.
385-385: Balance retrieval moved to free functionChanged from wallet member method to free function:
m_wallet->GetBalance()→GetBalance(*m_wallet)
411-411: Balance accessor modified to use new APIUpdated to use the same free function as elsewhere in the code:
m_wallet->GetBalance().m_mine_trusted→GetBalance(*m_wallet).m_mine_trustedThis ensures consistency in how balances are retrieved throughout the codebase.
427-430: Specialized balance functions moved to free functionsChanged wallet member methods to free functions for different balance types:
m_wallet->GetBalanceAnonymized(coin_control)→GetBalanceAnonymized(*m_wallet, coin_control)m_wallet->GetAvailableBalance(&coin_control)→GetAvailableBalance(*m_wallet, &coin_control)This change is part of the modularization effort to separate CoinJoin-specific functionality into its own file.
435-435: Input ownership check moved to free functionChanged from wallet member method to free function:
m_wallet->IsMine(txin)→InputIsMine(*m_wallet, txin)
450-450: Output credit calculation moved to free functionChanged from wallet member method to free function:
m_wallet->GetCredit(txout, filter)→OutputGetCredit(*m_wallet, txout, filter)
456-456: Coin listing moved to free functionChanged from wallet member method to free function:
m_wallet->ListCoins()→ListCoins(*m_wallet)
474-474: Transaction depth query moved to wallet methodChanged from transaction method to wallet method:
it->second.GetDepthInMainChain()→m_wallet->GetTxDepthInMainChain(it->second)This is consistent with the PR objective to move transaction state verification to the wallet.
src/wallet/coinjoin.cpp (1)
272-277:⚠️ Potential issueUnsafe pointer/reference into
mapOutpointRoundsCachemay be invalidated
std::map::emplacedoes not invalidate existing references, but subsequent
recursive calls toGetRealOutpointCoinJoinRoundsalso performemplace, which
can trigger tree re‑balancing and invalidatenRoundsRef.A safe pattern is to avoid storing a raw pointer and operate on the iterator
returned byemplaceinstead:- auto pair = mapOutpointRoundsCache.emplace(outpoint, -10); - auto nRoundsRef = &pair.first->second; + auto [it, inserted] = mapOutpointRoundsCache.emplace(outpoint, -10);and then dereference
it->secondwhere needed.
This guarantees lifetime even when the map mutates in deeper recursive calls.Likely an incorrect or invalid review comment.
src/wallet/wallet.h (1)
315-320:✅ Verification successful
Parameter added – double‑check all translation units
AddToWalletIfInvolvingMe()now takes the extrarescanning_old_blockflag (lines 315‑320).
Please ensure every definition and every call‑site inwallet/*.cpp, RPC helpers, tests, and, crucially, in the new back‑portedreceive.cpp/spend.cppcode are updated. A signature mismatch will only surface at link‑time and is easy to overlook in a large back‑port.
🏁 Script executed:
#!/bin/bash # Find calls with the old 4‑parameter form (tx, confirm, batch, update_tx) ast-grep --pattern $'AddToWalletIfInvolvingMe($_, $_, $_, $_)' | headLength of output: 236
To ensure we catch all call‑sites (and avoid parsing non‑UTF8 test files), let’s switch to
rgand inspect every invocation ofAddToWalletIfInvolvingMein the wallet code:
🏁 Script executed:
#!/bin/bash # List all invocations of AddToWalletIfInvolvingMe() in src/wallet to confirm they use the new 5-parameter signature rg "AddToWalletIfInvolvingMe\\(" -n src/walletLength of output: 567
To verify there are no lingering 4‑parameter calls (including RPC helpers, tests, and back‑port files), let’s scan the entire
src/tree:
🏁 Script executed:
#!/bin/bash # Find every invocation of AddToWalletIfInvolvingMe in src/, ensuring they all use the new 5-parameter signature rg "AddToWalletIfInvolvingMe\\(" -n srcLength of output: 560
All call‑sites updated to the 5‑parameter signature
Verified via a full
rgsearch acrosssrc/that the only occurrences ofAddToWalletIfInvolvingMe(are the declaration inwallet.hand its definition and invocation inwallet.cpp, all using the newrescanning_old_blockparameter. No remaining 4‑parameter calls exist.
| /** Get the marginal bytes if spending the specified output from this transaction */ | ||
| int GetTxSpendSize(const CWallet& wallet, const CWalletTx& wtx, unsigned int out, bool use_max_sig = false); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing lock annotation mirrors runtime race flagged in .cpp
GetTxSpendSize() touches wallet internals (see earlier comment) but the
declaration lacks EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet), making the
contract unclear and the static analyser unable to help.
-int GetTxSpendSize(const CWallet& wallet, const CWalletTx& wtx, unsigned int out, bool use_max_sig = false);
+int GetTxSpendSize(const CWallet& wallet, const CWalletTx& wtx, unsigned int out,
+ bool use_max_sig = false) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);Aligning the header and implementation prevents accidental unsynchronised
calls elsewhere.
| /* A helper function which loops through wallet UTXOs */ | ||
| std::unordered_set<const CWalletTx*, WalletTxHasher> GetSpendableTXs() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Returned pointers become dangling as soon as the wallet unlocks
GetSpendableTXs() returns an unordered_set<const CWalletTx*>.
Because the pointers refer to objects owned by mapWallet, they are only valid while cs_wallet remains locked and the entries stay intact.
Future maintainers could easily call the method, release the lock, and keep using the set – an instant U.B. footgun.
-std::unordered_set<const CWalletTx*, WalletTxHasher> GetSpendableTXs() const;
+std::vector<std::reference_wrapper<const CWalletTx>> GetSpendableTXs() const;A reference_wrapper (or even std::vector<const uint256&> with later mapWallet lookup) prevents dangling addresses without losing performance.
Committable suggestion skipped: line range outside the PR's diff.
| ret.m_watchonly_untrusted_pending += tx_credit_watchonly; | ||
| } | ||
| ret.m_mine_immature += wtx.GetImmatureCredit(); | ||
| ret.m_watchonly_immature += wtx.GetImmatureWatchOnlyCredit(); | ||
| ret.m_mine_immature += CachedTxGetImmatureCredit(wallet, wtx); | ||
| ret.m_watchonly_immature += CachedTxGetImmatureWatchOnlyCredit(wallet, wtx); | ||
| if (cj_enabled) { | ||
| const auto balance_anonymized = wtx.GetAvailableCoinJoinCredits(); | ||
| const auto balance_anonymized = CachedTxGetAvailableCoinJoinCredits(wallet, wtx); |
There was a problem hiding this comment.
Compile‑time error: missing fUseCache argument
CachedTxGetImmatureCredit() and CachedTxGetImmatureWatchOnlyCredit() now
require a third bool fUseCache parameter, but the calls inside
GetBalance() pass only two arguments (lines 342‑344).
The file will fail to compile.
- ret.m_mine_immature += CachedTxGetImmatureCredit(wallet, wtx);
- ret.m_watchonly_immature += CachedTxGetImmatureWatchOnlyCredit(wallet, wtx);
+ ret.m_mine_immature += CachedTxGetImmatureCredit(wallet, wtx, /*fUseCache=*/true);
+ ret.m_watchonly_immature += CachedTxGetImmatureWatchOnlyCredit(wallet, wtx, /*fUseCache=*/true);📝 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.
| ret.m_watchonly_untrusted_pending += tx_credit_watchonly; | |
| } | |
| ret.m_mine_immature += wtx.GetImmatureCredit(); | |
| ret.m_watchonly_immature += wtx.GetImmatureWatchOnlyCredit(); | |
| ret.m_mine_immature += CachedTxGetImmatureCredit(wallet, wtx); | |
| ret.m_watchonly_immature += CachedTxGetImmatureWatchOnlyCredit(wallet, wtx); | |
| if (cj_enabled) { | |
| const auto balance_anonymized = wtx.GetAvailableCoinJoinCredits(); | |
| const auto balance_anonymized = CachedTxGetAvailableCoinJoinCredits(wallet, wtx); | |
| ret.m_watchonly_untrusted_pending += tx_credit_watchonly; | |
| } | |
| - ret.m_mine_immature += CachedTxGetImmatureCredit(wallet, wtx); | |
| - ret.m_watchonly_immature += CachedTxGetImmatureWatchOnlyCredit(wallet, wtx); | |
| + ret.m_mine_immature += CachedTxGetImmatureCredit(wallet, wtx, /*fUseCache=*/true); | |
| + ret.m_watchonly_immature += CachedTxGetImmatureWatchOnlyCredit(wallet, wtx, /*fUseCache=*/true); | |
| if (cj_enabled) { | |
| const auto balance_anonymized = CachedTxGetAvailableCoinJoinCredits(wallet, wtx); |
…bitcoin#29486, bitcoin#25972 (ensure `WARN_CXXFLAGS` are populated to ensure expected `--enable-werror` behavior) af14f23 ci: don't build using `-Werror` when using Clang (Kittywhiskers Van Gogh) 3b837c8 ci: don't build using `-Werror` when using GCC (Kittywhiskers Van Gogh) 04e036e revert: make fuzzing builds stricter by enabling -Werror by default (Kittywhiskers Van Gogh) 29090a0 merge bitcoin#25972: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set (Kittywhiskers Van Gogh) d0548f8 merge bitcoin#29486: remove -Wdocumentation conditional (Kittywhiskers Van Gogh) 0f4812f merge bitcoin#27872: suppress external warnings by default (Kittywhiskers Van Gogh) 7100780 merge bitcoin#28999: Enable -Wunreachable-code (Kittywhiskers Van Gogh) 5471c58 merge bitcoin#28092: document that -Wreturn-type has been fixed upstream (mingw-w64) (Kittywhiskers Van Gogh) 9098c9c build: always enable -Wsuggest-override (Kittywhiskers Van Gogh) Pull request description: ## Motivation While working on [dash#6633](#6633), I had built `develop` (5e4a892) but the build _failed_ locally due to a `-Wthread-safety` warning (see below) that was introduced by [bitcoin#25337](bitcoin#25337) ([commit](a7d4127)). It was caught because I use additional `CXXFLAGS` on my local system but it _should_ have been caught by CI, especially since [bitcoin#20182](bitcoin#20182) ([commit](14a67ee)) made `--enable-werror` the default for CI and the sole exception (the Windows build) was remedied with [bitcoin#20586](bitcoin#20586) ([commit](750447e)), so every build variant should've run with `-Werror`. <details> <summary>Compile error:</summary> ``` CXX wallet/libbitcoin_wallet_a-sqlite.o wallet/rpcwallet.cpp:1038:36: error: calling function 'IsMine' requires holding mutex 'wallet.cs_wallet' exclusively [-Werror,-Wthread-safety-analysis] 1038 | isminefilter mine = wallet.IsMine(address); | ^ ``` </details> But that didn't happen. Till [bitcoin#23149](70ed6b4), there were a separate set of warnings (overridable by `CXXFLAGS`) and errors (overridable by `CXXFLAG_WERROR`). _Before_ the backport, coverage was as expected ([build](https://gitlab.com/dashpay/dash/-/jobs/9221165750#L786), search for `-Werror=thread-safety`) but _after_ the backport, `thread-safety` (and other expected warnings) were no longer being evaluated ([build](https://gitlab.com/dashpay/dash/-/jobs/9238308844#L740), search for `-Werror` and `-Wthread-safety`, only the former is present). Expected `CXXFLAGS`: ``` CXXFLAGS = -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_top_srcdir)=. -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wreorder -Wvla -Wshadow-field -Wthread-safety -Wloop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Woverloaded-virtual -Wsuggest-override -Wunreachable-code-loop-increment -Wimplicit-fallthrough -Wno-unused-parameter -Wno-self-assign -Werror -pipe -std=c++20 -O2 -O0 -g0 ``` Actual `CXXFLAGS`: ``` CXXFLAGS = -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_top_srcdir)=. -Werror -pipe -std=c++20 -O2 -O0 -g0 ``` This happened because `CXXFLAGS` are overridden by default which results in none of the warnings making it to the final `CXXFLAGS`, which reduced the effectiveness of `-Werror` substantially (while `CXXFLAG_WERROR` was left undisturbed, which allowed pre-backport builds to include coverage). This is remedied by backporting [bitcoin#25972](bitcoin#25972) (done by this PR), which will ensure that `WARN_CXXFLAGS` are included _even if `CXXFLAGS` are overridden_ and is possible because [bitcoin#24391](bitcoin#24391) ([commit](11323c3)) is already in `develop`. ## Additional Information * Dependency for #6638 * Dependency for #6639 * Because the warnings (converted to errors with `-Werror`) cast a wider net than the older set of error flags, `develop` in its current state does not compile. To allow CI to bless this PR, `-Werror` for both Clang and GCC-based builds have been **tentatively** disabled and will be re-enabled by the dependent PRs listed above. It is recommended to read the pull request description for both dependents while reviewing this PR. * `-Wsuggest-override` was made unconditional in [bitcoin#28348](bitcoin#28348) ([commit](c71e3df)) **but** there were two such conditional checks, they were deduplicated in [bitcoin#23149](bitcoin#23149) but the former was merged before the latter (i.e. out-of-order) and one conditional check lingered around. This lingering check has been removed as we don't support GCC 9.2. * `CXXFLAGS` set for the fuzz build ([commit](184bd60)) that enabled `-Werror` are made redundant with [bitcoin#20182](bitcoin#20182) and therefore, have been removed. ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK af14f23 Tree-SHA512: ccdaf71cf79eb3aec2468c4c1eaa696cd120c03e9665a3c4b56da6ef17cca9585ef8c66ac1625f2ba243c7f80f15e92a336c0bd90b5f11969fabb3adde3c8125
…n#27872, bitcoin#29486, bitcoin#25972 (ensure `WARN_CXXFLAGS` are populated to ensure expected `--enable-werror` behavior) af14f23 ci: don't build using `-Werror` when using Clang (Kittywhiskers Van Gogh) 3b837c8 ci: don't build using `-Werror` when using GCC (Kittywhiskers Van Gogh) 04e036e revert: make fuzzing builds stricter by enabling -Werror by default (Kittywhiskers Van Gogh) 29090a0 merge bitcoin#25972: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set (Kittywhiskers Van Gogh) d0548f8 merge bitcoin#29486: remove -Wdocumentation conditional (Kittywhiskers Van Gogh) 0f4812f merge bitcoin#27872: suppress external warnings by default (Kittywhiskers Van Gogh) 7100780 merge bitcoin#28999: Enable -Wunreachable-code (Kittywhiskers Van Gogh) 5471c58 merge bitcoin#28092: document that -Wreturn-type has been fixed upstream (mingw-w64) (Kittywhiskers Van Gogh) 9098c9c build: always enable -Wsuggest-override (Kittywhiskers Van Gogh) Pull request description: ## Motivation While working on [dash#6633](dashpay#6633), I had built `develop` (5e4a892) but the build _failed_ locally due to a `-Wthread-safety` warning (see below) that was introduced by [bitcoin#25337](bitcoin#25337) ([commit](dashpay@a7d4127)). It was caught because I use additional `CXXFLAGS` on my local system but it _should_ have been caught by CI, especially since [bitcoin#20182](bitcoin#20182) ([commit](dashpay@14a67ee)) made `--enable-werror` the default for CI and the sole exception (the Windows build) was remedied with [bitcoin#20586](bitcoin#20586) ([commit](dashpay@750447e)), so every build variant should've run with `-Werror`. <details> <summary>Compile error:</summary> ``` CXX wallet/libbitcoin_wallet_a-sqlite.o wallet/rpcwallet.cpp:1038:36: error: calling function 'IsMine' requires holding mutex 'wallet.cs_wallet' exclusively [-Werror,-Wthread-safety-analysis] 1038 | isminefilter mine = wallet.IsMine(address); | ^ ``` </details> But that didn't happen. Till [bitcoin#23149](dashpay@70ed6b4), there were a separate set of warnings (overridable by `CXXFLAGS`) and errors (overridable by `CXXFLAG_WERROR`). _Before_ the backport, coverage was as expected ([build](https://gitlab.com/dashpay/dash/-/jobs/9221165750#L786), search for `-Werror=thread-safety`) but _after_ the backport, `thread-safety` (and other expected warnings) were no longer being evaluated ([build](https://gitlab.com/dashpay/dash/-/jobs/9238308844#L740), search for `-Werror` and `-Wthread-safety`, only the former is present). Expected `CXXFLAGS`: ``` CXXFLAGS = -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_top_srcdir)=. -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wreorder -Wvla -Wshadow-field -Wthread-safety -Wloop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Woverloaded-virtual -Wsuggest-override -Wunreachable-code-loop-increment -Wimplicit-fallthrough -Wno-unused-parameter -Wno-self-assign -Werror -pipe -std=c++20 -O2 -O0 -g0 ``` Actual `CXXFLAGS`: ``` CXXFLAGS = -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_top_srcdir)=. -Werror -pipe -std=c++20 -O2 -O0 -g0 ``` This happened because `CXXFLAGS` are overridden by default which results in none of the warnings making it to the final `CXXFLAGS`, which reduced the effectiveness of `-Werror` substantially (while `CXXFLAG_WERROR` was left undisturbed, which allowed pre-backport builds to include coverage). This is remedied by backporting [bitcoin#25972](bitcoin#25972) (done by this PR), which will ensure that `WARN_CXXFLAGS` are included _even if `CXXFLAGS` are overridden_ and is possible because [bitcoin#24391](bitcoin#24391) ([commit](dashpay@11323c3)) is already in `develop`. ## Additional Information * Dependency for dashpay#6638 * Dependency for dashpay#6639 * Because the warnings (converted to errors with `-Werror`) cast a wider net than the older set of error flags, `develop` in its current state does not compile. To allow CI to bless this PR, `-Werror` for both Clang and GCC-based builds have been **tentatively** disabled and will be re-enabled by the dependent PRs listed above. It is recommended to read the pull request description for both dependents while reviewing this PR. * `-Wsuggest-override` was made unconditional in [bitcoin#28348](bitcoin#28348) ([commit](dashpay@c71e3df)) **but** there were two such conditional checks, they were deduplicated in [bitcoin#23149](bitcoin#23149) but the former was merged before the latter (i.e. out-of-order) and one conditional check lingered around. This lingering check has been removed as we don't support GCC 9.2. * `CXXFLAGS` set for the fuzz build ([commit](dashpay@184bd60)) that enabled `-Werror` are made redundant with [bitcoin#20182](bitcoin#20182) and therefore, have been removed. ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK af14f23 Tree-SHA512: ccdaf71cf79eb3aec2468c4c1eaa696cd120c03e9665a3c4b56da6ef17cca9585ef8c66ac1625f2ba243c7f80f15e92a336c0bd90b5f11969fabb3adde3c8125
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
src/wallet/spend.cpp (1)
417-492: 🛠️ Refactor suggestionFunction relies on caller holding
cs_walletbut does not assert
SelectCoins()accesseswallet.mapWallet, reads cached debit/credit, and calls other helpers that expect the wallet lock, yet the function itself is neither annotated nor asserts the lock. Future callers may omit the lock by mistake.Add
EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)and an internalAssertLockHeld(wallet.cs_wallet);to document the contract and fail fast.[concurrency]
♻️ Duplicate comments (3)
src/wallet/spend.h (1)
13-15:⚠️ Potential issueMissing lock annotation mirrors runtime race flagged in
.cpp
GetTxSpendSize()touches wallet internals but the declaration lacksEXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet), making the contract unclear and the static analyzer unable to help.-int GetTxSpendSize(const CWallet& wallet, const CWalletTx& wtx, unsigned int out, bool use_max_sig = false); +int GetTxSpendSize(const CWallet& wallet, const CWalletTx& wtx, unsigned int out, bool use_max_sig = false) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);src/wallet/spend.cpp (1)
26-29:⚠️ Potential issueLock‑safety missing around
DummySignInputcall
GetTxSpendSize()invokesCalculateMaximumSignedInputSize(), which in turn callswallet->DummySignInput().
Unlike the similar helpers that were annotated withEXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet), this new wrapper has no lock annotation or internalAssertLockHeld. A concurrent RPC thread performing size estimation can therefore walk wallet data without holdingcs_wallet, re‑introducing the data race highlighted in earlier reviews.-int GetTxSpendSize(const CWallet& wallet, const CWalletTx& wtx, unsigned int out, bool use_max_sig) -{ - return CalculateMaximumSignedInputSize(wtx.tx->vout[out], &wallet, use_max_sig); -} +int GetTxSpendSize(const CWallet& wallet, const CWalletTx& wtx, unsigned int out, bool use_max_sig) + EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) +{ + AssertLockHeld(wallet.cs_wallet); + return CalculateMaximumSignedInputSize(wtx.tx->vout[out], &wallet, use_max_sig); +}src/wallet/wallet.h (1)
462-464: Dangling‑pointer risk inGetSpendableTXs()remainsPrevious feedback noted that returning
std::unordered_set<const CWalletTx*>can leave callers with dangling pointers oncecs_walletis released. The method now carriesEXCLUSIVE_LOCKS_REQUIRED(cs_wallet), which helps, but the fundamental lifetime hazard persists.Consider returning lightweight identifiers (txids) or
std::reference_wrapper<const CWalletTx>to make misuse impossible.
🧹 Nitpick comments (1)
src/wallet/spend.cpp (1)
366-368: Avoid copying potentially largestd::vector<COutput>
AttemptSelection()receivescoinsby value, immediately creating a full copy of the candidate‑coin list even though the function never mutates it.-bool AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector<COutput> coins, +bool AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const std::vector<COutput>& coins,The same applies to the call sites.
[performance]
🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 366-366: Function parameter 'coins' should be passed by const reference.
(passedByValue)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
src/Makefile.am(2 hunks)src/bench/coin_selection.cpp(4 hunks)src/bench/wallet_balance.cpp(2 hunks)src/coinjoin/client.cpp(3 hunks)src/coinjoin/util.cpp(2 hunks)src/rpc/coinjoin.cpp(3 hunks)src/rpc/evo.cpp(3 hunks)src/rpc/governance.cpp(2 hunks)src/rpc/masternode.cpp(2 hunks)src/wallet/coincontrol.h(1 hunks)src/wallet/coinjoin.cpp(1 hunks)src/wallet/coinjoin.h(1 hunks)src/wallet/context.h(1 hunks)src/wallet/interfaces.cpp(10 hunks)src/wallet/load.cpp(1 hunks)src/wallet/receive.cpp(10 hunks)src/wallet/receive.h(1 hunks)src/wallet/rpcwallet.cpp(22 hunks)src/wallet/spend.cpp(39 hunks)src/wallet/spend.h(3 hunks)src/wallet/test/coinjoin_tests.cpp(2 hunks)src/wallet/test/coinselector_tests.cpp(5 hunks)src/wallet/test/psbt_wallet_tests.cpp(1 hunks)src/wallet/test/spend_tests.cpp(2 hunks)src/wallet/test/wallet_tests.cpp(20 hunks)src/wallet/transaction.h(3 hunks)src/wallet/wallet.cpp(22 hunks)src/wallet/wallet.h(7 hunks)src/wallet/walletdb.cpp(1 hunks)test/functional/test_runner.py(1 hunks)test/functional/wallet_basic.py(1 hunks)test/functional/wallet_transactiontime_rescan.py(1 hunks)test/lint/lint-circular-dependencies.py(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/wallet/load.cpp
- src/wallet/context.h
- test/functional/wallet_basic.py
🚧 Files skipped from review as they are similar to previous changes (25)
- src/bench/wallet_balance.cpp
- src/wallet/test/psbt_wallet_tests.cpp
- src/rpc/coinjoin.cpp
- src/coinjoin/util.cpp
- src/wallet/walletdb.cpp
- test/functional/test_runner.py
- src/wallet/test/spend_tests.cpp
- src/wallet/coincontrol.h
- src/Makefile.am
- src/rpc/masternode.cpp
- src/wallet/test/coinjoin_tests.cpp
- src/rpc/evo.cpp
- src/bench/coin_selection.cpp
- src/rpc/governance.cpp
- src/coinjoin/client.cpp
- src/wallet/test/wallet_tests.cpp
- src/wallet/receive.h
- src/wallet/test/coinselector_tests.cpp
- src/wallet/coinjoin.h
- test/functional/wallet_transactiontime_rescan.py
- test/lint/lint-circular-dependencies.py
- src/wallet/interfaces.cpp
- src/wallet/rpcwallet.cpp
- src/wallet/coinjoin.cpp
- src/wallet/receive.cpp
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/wallet/spend.cpp (6)
src/wallet/wallet.h (62)
wallet(399-399)wtx(292-292)wtx(499-499)wtx(507-507)wtx(508-508)wtx(514-514)wtx(515-515)wtx(522-522)wtx(523-523)wtx(594-594)wtx(691-691)wtx(693-693)LOCK(812-812)tx(300-300)tx(319-319)tx(325-325)tx(329-329)tx(613-613)tx(615-615)tx(637-637)tx(646-646)tx(648-648)tx(688-688)tx(786-786)tx(788-788)tx(789-789)tx(911-911)output(556-556)output(557-557)address(796-796)address(798-798)filter(755-755)outpoint(291-291)outpoint(538-538)outpoint(540-540)outpoint(544-544)outpoint(545-545)EXCLUSIVE_LOCKS_REQUIRED(291-291)EXCLUSIVE_LOCKS_REQUIRED(292-292)EXCLUSIVE_LOCKS_REQUIRED(300-300)EXCLUSIVE_LOCKS_REQUIRED(319-319)EXCLUSIVE_LOCKS_REQUIRED(325-325)EXCLUSIVE_LOCKS_REQUIRED(327-327)EXCLUSIVE_LOCKS_REQUIRED(329-329)EXCLUSIVE_LOCKS_REQUIRED(463-463)EXCLUSIVE_LOCKS_REQUIRED(476-476)EXCLUSIVE_LOCKS_REQUIRED(497-497)EXCLUSIVE_LOCKS_REQUIRED(499-499)EXCLUSIVE_LOCKS_REQUIRED(507-507)EXCLUSIVE_LOCKS_REQUIRED(508-512)EXCLUSIVE_LOCKS_REQUIRED(514-514)EXCLUSIVE_LOCKS_REQUIRED(515-515)EXCLUSIVE_LOCKS_REQUIRED(522-522)txout(785-785)txin(784-784)hash(497-497)hash(547-547)hash(550-550)hash(553-553)hash(555-555)hash(614-614)hash(654-654)src/wallet/spend.h (10)
CalculateMaximumSignedInputSize(69-69)COutput(50-58)AvailableCoins(81-81)FindNonChangeParentOutput(88-88)GroupOutputs(95-95)GetTxSpendSize(14-14)AttemptSelection(108-109)SelectCoins(119-120)CalculateMaximumSignedTxSize(75-75)CalculateMaximumSignedTxSize(76-76)src/wallet/coincontrol.h (7)
nCoinType(111-114)output(79-82)output(79-79)output(84-87)output(84-84)output(89-92)output(89-89)src/wallet/receive.cpp (8)
CachedTxIsTrusted(287-318)CachedTxIsTrusted(287-287)CachedTxIsTrusted(320-325)CachedTxIsTrusted(320-320)OutputIsChange(88-91)OutputIsChange(88-88)CachedTxIsFromMe(282-285)CachedTxIsFromMe(282-282)src/wallet/coinselection.cpp (2)
KnapsackSolver(246-354)KnapsackSolver(246-246)src/wallet/wallet.cpp (1)
outpoint(604-604)
🪛 Cppcheck (2.10-2)
src/wallet/spend.cpp
[performance] 366-366: Function parameter 'coins' should be passed by const reference.
(passedByValue)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
🔇 Additional comments (15)
src/wallet/transaction.h (3)
117-118: Constructor simplification through removal of wallet dependencyThe constructor has been simplified to only take a transaction reference, removing any wallet dependency. This is a good architectural change that reduces coupling between wallet and transaction objects.
177-179: Improved conditional serialization logicThis change provides clearer code for serializing the order position only when it has a valid value.
219-221: Cleaner approach to extracting optional map valuesUsing a find-and-check approach for deserialization is cleaner than the previous implementation and consistently handles the case when the key is not present.
src/wallet/spend.h (2)
50-57: Constructor updated to use free function correctlyThe
COutputconstructor has been properly updated to take a wallet reference and use the new free function for transaction spend size calculation.
135-135:❌ Incorrect review comment
Missing lock annotation on
GenBudgetSystemCollateralTxThis function likely accesses wallet data but lacks a lock annotation. Add one if it requires wallet-level locks.
🏁 Script executed:
#!/bin/bash # Check if GenBudgetSystemCollateralTx accesses wallet locks or calls functions requiring wallet locks rg -A 3 -B 3 "GenBudgetSystemCollateralTx" --type cppLength of output: 1675
🏁 Script executed:
#!/bin/bash # Display the full implementation of GenBudgetSystemCollateralTx to check for wallet lock usage rg -n "bool GenBudgetSystemCollateralTx" -A 200 src/wallet/spend.cppLength of output: 922
🏁 Script executed:
#!/bin/bash # Locate the definition of CreateTransaction to check for internal wallet locking or annotations rg -n "bool CreateTransaction" -A 10 -B 10 src/wallet/spend.cppLength of output: 2395
🏁 Script executed:
#!/bin/bash # Show the wrapper implementation of CreateTransaction to verify locking behavior rg -n "bool CreateTransaction" -A 20 -B 20 src/wallet/spend.cppLength of output: 4235
🏁 Script executed:
#!/bin/bash # Show the wrapper implementation of CreateTransaction to verify if it acquires wallet lock sed -n '960,1040p' src/wallet/spend.cppLength of output: 3826
GenBudgetSystemCollateralTx acquires the wallet lock via CreateTransaction
GenBudgetSystemCollateralTx calls CreateTransaction, which internally does
LOCK(wallet.cs_wallet)before invokingCreateTransactionInternal(annotated withEXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)). No additional lock annotation is needed on GenBudgetSystemCollateralTx.Likely an incorrect or invalid review comment.
src/wallet/wallet.cpp (10)
882-882: Added rescanning parameter to handle transaction timestamps correctlyAdding the
rescanning_old_blockparameter allows for different timestamp handling during initial wallet load/rescan versus normal operation.
903-903: More efficient map insertion using in-place constructionUsing
emplacewithstd::piecewise_constructandstd::forward_as_tupleavoids unnecessary copying or moving of objects when inserting into the map.
987-987: Consistent use of efficient map insertion patternThe same efficient map insertion pattern is used here as in
AddToWallet, maintaining consistency.
1045-1045: Propagated rescanning parameter for consistent timestamp handlingThe
rescanning_old_blockparameter is correctly propagated to maintain consistent timestamp handling during wallet rescans.
1100-1100: Wallet methods now used instead of transaction methodsThese calls have been updated to use the new wallet methods that take transaction references, consistent with the architectural change to reduce coupling between wallet and transaction classes.
Also applies to: 1107-1107, 1133-1133, 1186-1186, 1791-1791, 1800-1800
1244-1246: Propagated rescanning parameter through transaction synchronizationThe
rescanning_old_blockparameter is correctly propagated through the transaction synchronization path, ensuring consistent timestamp handling.
1728-1728: Correctly set rescanning flag during wallet rescanThe transaction synchronization during rescan properly sets the
rescanning_old_blockflag to ensure appropriate timestamp handling.
1804-1852: Implemented transaction-related wallet methods to replace CWalletTx membersThese wallet methods now handle functionality that was previously implemented as transaction methods, consistent with the architectural change to reduce coupling.
2578-2620: Improved timestamp handling during rescanThe updated
ComputeTimeSmartfunction uses the block's maximum time when rescanning old blocks, avoiding the need to compute time based on transaction history that might not be fully loaded yet during a rescan.
3436-3479: Moved transaction status methods to the wallet classThese methods replace what were previously member functions of
CWalletTx, taking aCWalletTxreference as a parameter instead. This is consistent with the overall direction of reducing coupling.
| bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl); | ||
|
|
There was a problem hiding this comment.
Missing lock annotation on FundTransaction
The FundTransaction function likely calls CreateTransaction or other wallet-locking functions but lacks an explicit lock annotation.
-bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl);
+bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);📝 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.
| bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl); | |
| bool FundTransaction(CWallet& wallet, | |
| CMutableTransaction& tx, | |
| CAmount& nFeeRet, | |
| int& nChangePosInOut, | |
| bilingual_str& error, | |
| bool lockUnspents, | |
| const std::set<int>& setSubtractFeeFromOutputs, | |
| CCoinControl) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); |
| bool CreateTransaction(CWallet& wallet, const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, FeeCalculation& fee_calc_out, bool sign = true, int nExtraPayloadSize = 0); | ||
|
|
There was a problem hiding this comment.
Missing lock annotation on CreateTransaction
The CreateTransaction function almost certainly accesses wallet data and likely calls functions requiring wallet locks, but lacks an explicit lock annotation.
-bool CreateTransaction(CWallet& wallet, const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, FeeCalculation& fee_calc_out, bool sign = true, int nExtraPayloadSize = 0);
+bool CreateTransaction(CWallet& wallet, const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, FeeCalculation& fee_calc_out, bool sign = true, int nExtraPayloadSize = 0) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);📝 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.
| bool CreateTransaction(CWallet& wallet, const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, FeeCalculation& fee_calc_out, bool sign = true, int nExtraPayloadSize = 0); | |
| bool CreateTransaction(CWallet& wallet, | |
| const std::vector<CRecipient>& vecSend, | |
| CTransactionRef& tx, | |
| CAmount& nFeeRet, | |
| int& nChangePosInOut, | |
| bilingual_str& error, | |
| const CCoinControl& coin_control, | |
| FeeCalculation& fee_calc_out, | |
| bool sign = true, | |
| int nExtraPayloadSize = 0) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); |
| void AvailableCoins(const CWallet& wallet, std::vector<COutput>& vCoins, const CCoinControl* coinControl = nullptr, const CAmount& nMinimumAmount = 1, const CAmount& nMaximumAmount = MAX_MONEY, const CAmount& nMinimumSumAmount = MAX_MONEY, const uint64_t nMaximumCount = 0) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); | ||
|
|
||
| CAmount GetAvailableBalance(const CWallet& wallet, const CCoinControl* coinControl = nullptr); | ||
|
|
There was a problem hiding this comment.
Missing lock annotation on GetAvailableBalance
While AvailableCoins correctly has a lock annotation, GetAvailableBalance is missing one. Since it likely calls AvailableCoins internally, it should also have the same lock requirement.
-CAmount GetAvailableBalance(const CWallet& wallet, const CCoinControl* coinControl = nullptr);
+CAmount GetAvailableBalance(const CWallet& wallet, const CCoinControl* coinControl = nullptr) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);📝 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.
| void AvailableCoins(const CWallet& wallet, std::vector<COutput>& vCoins, const CCoinControl* coinControl = nullptr, const CAmount& nMinimumAmount = 1, const CAmount& nMaximumAmount = MAX_MONEY, const CAmount& nMinimumSumAmount = MAX_MONEY, const uint64_t nMaximumCount = 0) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); | |
| CAmount GetAvailableBalance(const CWallet& wallet, const CCoinControl* coinControl = nullptr); | |
| void AvailableCoins(const CWallet& wallet, std::vector<COutput>& vCoins, const CCoinControl* coinControl = nullptr, const CAmount& nMinimumAmount = 1, const CAmount& nMaximumAmount = MAX_MONEY, const CAmount& nMinimumSumAmount = MAX_MONEY, const uint64_t nMaximumCount = 0) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); | |
| - CAmount GetAvailableBalance(const CWallet& wallet, const CCoinControl* coinControl = nullptr); | |
| + CAmount GetAvailableBalance(const CWallet& wallet, const CCoinControl* coinControl = nullptr) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); |
| bool AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector<COutput> coins, | ||
| std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params, CoinType nCoinType = CoinType::ALL_COINS); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Missing lock annotation on AttemptSelection
This function is likely to access wallet data when performing coin selection, but lacks a lock annotation. Add one if it requires wallet-level locks.
🏁 Script executed:
#!/bin/bash
# Check if AttemptSelection accesses wallet locks or calls functions requiring wallet locks
rg -A 3 -B 3 "AttemptSelection" --type cppLength of output: 6943
Add Thread-Safety Annotation to AttemptSelection
The free function AttemptSelection reads from wallet (e.g. wallet.m_spend_zero_conf_change) and invokes other wallet‐member APIs assuming the caller holds the wallet’s mutex. To make this contract explicit, annotate its declaration with the shared‐lock requirement.
Please update src/wallet/spend.h as follows:
--- a/src/wallet/spend.h
+++ b/src/wallet/spend.h
@@
-bool AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector<COutput> coins,
+/**\n
+ * Attempt to select coins from `wallet` for a target value.\n
+ * @pre The caller must hold `wallet.cs_wallet` (shared or exclusive).\n
+ */
+bool AttemptSelection(const CWallet& wallet
+ SHARED_LOCKS_REQUIRED(wallet.cs_wallet),
+ const CAmount& nTargetValue,
+ const CoinEligibilityFilter& eligibility_filter,
+ std::vector<COutput> coins,
std::set<CInputCoin>& setCoinsRet,
CAmount& nValueRet,
const CoinSelectionParams& coin_selection_params,
CoinType nCoinType = CoinType::ALL_COINS);– This makes it clear that callers must hold the wallet’s cs_wallet mutex before invoking coin‐selection.
📝 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.
| bool AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector<COutput> coins, | |
| std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params, CoinType nCoinType = CoinType::ALL_COINS); | |
| /** | |
| * Attempt to select coins from `wallet` for a target value. | |
| * @pre The caller must hold `wallet.cs_wallet` (shared or exclusive). | |
| */ | |
| bool AttemptSelection(const CWallet& wallet | |
| SHARED_LOCKS_REQUIRED(wallet.cs_wallet), | |
| const CAmount& nTargetValue, | |
| const CoinEligibilityFilter& eligibility_filter, | |
| std::vector<COutput> coins, | |
| std::set<CInputCoin>& setCoinsRet, | |
| CAmount& nValueRet, | |
| const CoinSelectionParams& coin_selection_params, | |
| CoinType nCoinType = CoinType::ALL_COINS); |
| bool spendable = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (((mine & ISMINE_WATCH_ONLY) != ISMINE_NO) && (coinControl && coinControl->fAllowWatchOnly && solvable)); | ||
|
|
||
| vCoins.push_back(COutput(&wtx, i, nDepth, spendable, solvable, safeTx, (coinControl && coinControl->fAllowWatchOnly))); | ||
| vCoins.push_back(COutput(wallet, wtx, i, nDepth, spendable, solvable, safeTx, (coinControl && coinControl->fAllowWatchOnly))); | ||
|
|
There was a problem hiding this comment.
use_max_sig flag is being confused with “allow watch‑only”
The last parameter of COutput’s constructor and of CalculateMaximumSignedTxSize() is a size‑estimation flag (use_max_sig).
Passing coinControl->fAllowWatchOnly (a watch‑only spendability flag) here changes fee‑estimation semantics and can lead to large fee mis‑calculations.
Fix by always forwarding an explicit boolean for size‑estimation (typically false here):
- vCoins.push_back(COutput(wallet, wtx, i, nDepth, spendable, solvable, safeTx, (coinControl && coinControl->fAllowWatchOnly)));
+ vCoins.push_back(COutput(wallet, wtx, i, nDepth, spendable, solvable, safeTx, /*use_max_sig=*/false));- int nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, coin_control.fAllowWatchOnly);
+ int nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, /*use_max_sig=*/false);- nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, coin_control.fAllowWatchOnly);
+ nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, /*use_max_sig=*/false);Also applies to: 837-838, 865-866
| bool GenBudgetSystemCollateralTx(CWallet& wallet, CTransactionRef& tx, uint256 hash, CAmount amount, const COutPoint& outpoint) | ||
| { | ||
| const CScript scriptChange{CScript() << OP_RETURN << ToByteVector(hash)}; | ||
| const std::vector<CRecipient> vecSend{{scriptChange, amount, false}}; | ||
|
|
||
| CCoinControl coinControl; | ||
| if (!outpoint.IsNull()) { | ||
| coinControl.Select(outpoint); | ||
| } | ||
|
|
||
| bilingual_str error; | ||
| CAmount nFeeRet{0}; | ||
| int nChangePosRet{-1}; | ||
| FeeCalculation fee_calc_out; | ||
|
|
||
| bool success{CreateTransaction(wallet, vecSend, tx, nFeeRet, nChangePosRet, error, coinControl, fee_calc_out)}; | ||
| if (!success) { | ||
| wallet.WalletLogPrintf("%s -- Error: %s\n", __func__, error.original); | ||
| } | ||
|
|
||
| return success; |
There was a problem hiding this comment.
Compile‑time error: wrong CreateTransaction arity
GenBudgetSystemCollateralTx() invokes CreateTransaction() with only eight arguments, omitting the mandatory sign and nExtraPayloadSize parameters introduced in the new signature. The file will not build.
- bool success{CreateTransaction(wallet, vecSend, tx, nFeeRet, nChangePosRet, error, coinControl, fee_calc_out)};
+ const bool sign_tx = true;
+ const int extra_payload = 0;
+ bool success{CreateTransaction(wallet, vecSend, tx, nFeeRet, nChangePosRet, error,
+ coinControl, fee_calc_out, sign_tx, extra_payload)};📝 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.
| bool GenBudgetSystemCollateralTx(CWallet& wallet, CTransactionRef& tx, uint256 hash, CAmount amount, const COutPoint& outpoint) | |
| { | |
| const CScript scriptChange{CScript() << OP_RETURN << ToByteVector(hash)}; | |
| const std::vector<CRecipient> vecSend{{scriptChange, amount, false}}; | |
| CCoinControl coinControl; | |
| if (!outpoint.IsNull()) { | |
| coinControl.Select(outpoint); | |
| } | |
| bilingual_str error; | |
| CAmount nFeeRet{0}; | |
| int nChangePosRet{-1}; | |
| FeeCalculation fee_calc_out; | |
| bool success{CreateTransaction(wallet, vecSend, tx, nFeeRet, nChangePosRet, error, coinControl, fee_calc_out)}; | |
| if (!success) { | |
| wallet.WalletLogPrintf("%s -- Error: %s\n", __func__, error.original); | |
| } | |
| return success; | |
| bool GenBudgetSystemCollateralTx(CWallet& wallet, CTransactionRef& tx, uint256 hash, CAmount amount, const COutPoint& outpoint) | |
| { | |
| const CScript scriptChange{CScript() << OP_RETURN << ToByteVector(hash)}; | |
| const std::vector<CRecipient> vecSend{{scriptChange, amount, false}}; | |
| CCoinControl coinControl; | |
| if (!outpoint.IsNull()) { | |
| coinControl.Select(outpoint); | |
| } | |
| bilingual_str error; | |
| CAmount nFeeRet{0}; | |
| int nChangePosRet{-1}; | |
| FeeCalculation fee_calc_out; | |
| const bool sign_tx = true; | |
| const int extra_payload = 0; | |
| bool success{CreateTransaction(wallet, vecSend, tx, nFeeRet, nChangePosRet, error, | |
| coinControl, fee_calc_out, sign_tx, extra_payload)}; | |
| if (!success) { | |
| wallet.WalletLogPrintf("%s -- Error: %s\n", __func__, error.original); | |
| } | |
| return success; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/wallet/spend.cpp (1)
417-496: 🛠️ Refactor suggestion
SelectCoins()accesses wallet internals without holdingcs_walletInside this block the function:
- reads
wallet.mapWallet- calls
wallet.IsFullyMixed,wallet.IsSpent,wallet.IsMine,wallet.chain().getPackageLimits()…Yet the function is not annotated
EXCLUSIVE_LOCKS_REQUIRED, nor does itLOCK(wallet.cs_wallet).
This violates the wallet’s lock discipline and can race with concurrent RPC threads.Refactor to lock (or annotate) the whole function body:
-bool SelectCoins(const CWallet& wallet, ... ) +bool SelectCoins(const CWallet& wallet, ... ) + EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) { - // ... + LOCK(wallet.cs_wallet); // existing logic }
♻️ Duplicate comments (10)
src/wallet/receive.h (1)
15-16:⚠️ Potential issueMissing thread-safety annotation on
AllInputsMineThe function
AllInputsMinelikely callsInputIsMine(which has a lock annotation) but doesn't have the same lock annotation, creating an inconsistency in thread-safety requirements.Add the missing thread-safety annotation:
-bool AllInputsMine(const CWallet& wallet, const CTransaction& tx, const isminefilter& filter); +bool AllInputsMine(const CWallet& wallet, const CTransaction& tx, const isminefilter& filter) + EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);src/wallet/spend.h (5)
13-14:⚠️ Potential issueMissing thread-safety annotation on
GetTxSpendSizeThe
GetTxSpendSizefunction likely accesses wallet data but is missing a thread-safety annotation.Add the missing thread-safety annotation:
-int GetTxSpendSize(const CWallet& wallet, const CWalletTx& wtx, unsigned int out, bool use_max_sig = false); +int GetTxSpendSize(const CWallet& wallet, const CWalletTx& wtx, unsigned int out, bool use_max_sig = false) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
83-83:⚠️ Potential issueMissing thread-safety annotation on
GetAvailableBalanceThe
GetAvailableBalancefunction likely callsAvailableCoins(which has a lock annotation) but doesn't have the same lock annotation.Add the missing thread-safety annotation:
-CAmount GetAvailableBalance(const CWallet& wallet, const CCoinControl* coinControl = nullptr); +CAmount GetAvailableBalance(const CWallet& wallet, const CCoinControl* coinControl = nullptr) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
108-109:⚠️ Potential issueMissing thread-safety annotation on
AttemptSelectionThe
AttemptSelectionfunction accesses wallet data but is missing a thread-safety annotation.Add the missing thread-safety annotation:
-bool AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector<COutput> coins, - std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params, CoinType nCoinType = CoinType::ALL_COINS); +bool AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector<COutput> coins, + std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params, CoinType nCoinType = CoinType::ALL_COINS) + SHARED_LOCKS_REQUIRED(wallet.cs_wallet);
127-127:⚠️ Potential issueMissing thread-safety annotation on
CreateTransactionThe
CreateTransactionfunction accesses wallet data but is missing a thread-safety annotation.Add the missing thread-safety annotation:
-bool CreateTransaction(CWallet& wallet, const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, FeeCalculation& fee_calc_out, bool sign = true, int nExtraPayloadSize = 0); +bool CreateTransaction(CWallet& wallet, const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, FeeCalculation& fee_calc_out, bool sign = true, int nExtraPayloadSize = 0) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
133-133:⚠️ Potential issueMissing thread-safety annotation on
FundTransactionThe
FundTransactionfunction likely callsCreateTransactionor other wallet-locking functions but lacks an explicit lock annotation.Add the missing thread-safety annotation:
-bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl); +bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);src/wallet/spend.cpp (2)
837-838:⚠️ Potential issue
use_max_sigflag still mis-used — will distort fee estimation
CalculateMaximumSignedTxSize(..., /*use_max_sig=*/coin_control.fAllowWatchOnly)
passes the watch-only flag where a “estimate worst-case signature size” boolean is expected.
This was raised previously.Use an explicit
false(or dedicated argument) instead:-int nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, coin_control.fAllowWatchOnly); +int nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, /*use_max_sig=*/false);Same fix required after dropping the change output.
Also applies to: 865-866
1080-1096:⚠️ Potential issueCompile-time error: wrong
CreateTransactionarity inGenBudgetSystemCollateralTx
CreateTransactionexpects the parameters(…, coinControl, fee_calc_out, bool sign, int nExtraPayloadSize).
The current call omitssignandnExtraPayloadSize; the file will not build.-bool success{CreateTransaction(wallet, vecSend, tx, nFeeRet, nChangePosRet, error, - coinControl, fee_calc_out)}; +const bool sign_tx = true; +const int extra_payload = 0; +bool success{CreateTransaction(wallet, vecSend, tx, nFeeRet, nChangePosRet, error, + coinControl, fee_calc_out, sign_tx, extra_payload)};src/wallet/wallet.h (1)
462-464: 🛠️ Refactor suggestionReturning raw pointers outside lock is dangerous
GetSpendableTXs()returnsunordered_set<const CWalletTx*>.
The pointed‐to objects live inmapWallet; oncecs_walletis released these
pointers may dangle if the map mutates.Consider returning a safer view, e.g.
std::vector<std::reference_wrapper<const CWalletTx>>,
or ensure callers keep the lock while iterating.
This was raised earlier and remains unresolved.src/wallet/receive.cpp (1)
350-352:⚠️ Potential issueMissing
fUseCacheargument — code will not compile
CachedTxGetImmatureCreditandCachedTxGetImmatureWatchOnlyCreditwere
updated to take a thirdbool fUseCacheparameter, but the calls here supply
only two arguments.- ret.m_mine_immature += CachedTxGetImmatureCredit(wallet, wtx); - ret.m_watchonly_immature += CachedTxGetImmatureWatchOnlyCredit(wallet, wtx); + ret.m_mine_immature += CachedTxGetImmatureCredit(wallet, wtx, /*fUseCache=*/true); + ret.m_watchonly_immature += CachedTxGetImmatureWatchOnlyCredit(wallet, wtx, /*fUseCache=*/true);
🧹 Nitpick comments (5)
src/wallet/coinjoin.cpp (1)
279-280: TODO comment about pointer usage should be addressedThere's a TODO comment suggesting that
wtxshould refer to aCWalletTxobject, not a pointer, to be consistent with surrounding code.Consider replacing the pointer with a reference or removing the TODO comment after verifying the current implementation is correct.
src/wallet/wallet.cpp (2)
985-991: Prefer an explicit emptyCTransactionRefovernullptr.Passing
nullptrworks because the constructor takes aCTransactionRef(which is astd::shared_ptr<const CTransaction>), but the intent is clearer – and avoids an extra implicit conversion – if we construct the empty shared-ptr directly.- const auto& ins = mapWallet.emplace(std::piecewise_construct, - std::forward_as_tuple(hash), - std::forward_as_tuple(nullptr)); + const auto& ins = mapWallet.emplace(std::piecewise_construct, + std::forward_as_tuple(hash), + std::forward_as_tuple(CTransactionRef{}));
2578-2614: Timestamp computation: possible micro-optimisation.Inside the backward iteration the code now builds
nSmartTimein two steps:int64_t nSmartTime; nSmartTime = pwtx->nTimeSmart; if (!nSmartTime) nSmartTime = pwtx->nTimeReceived;A direct ternary keeps it tighter and avoids an un-initialised-value warning from some static analysers, e.g.
const int64_t nSmartTime = pwtx->nTimeSmart ? pwtx->nTimeSmart : pwtx->nTimeReceived;No behavioural change, purely cosmetic.
src/wallet/spend.cpp (2)
366-373: Avoid the unnecessary copy ofcoinsinAttemptSelection
coinsis passed by value which forces an additional copy of the whole
std::vector<COutput>(potentially thousands of entries).
Pass it by (const) reference and make a local copy only if mutation is really needed.-bool AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, - const CoinEligibilityFilter& eligibility_filter, std::vector<COutput> coins, +bool AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, + const CoinEligibilityFilter& eligibility_filter, const std::vector<COutput>& coins,[performance]
🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 366-366: Function parameter 'coins' should be passed by const reference.
(passedByValue)
26-29: Runclang-tidywallet‐locking checksMultiple sections touched here would have been flagged automatically by the
-Wthread-safety-analysispass. Integrating such a check in CI will highlight
missingLOCK()/AssertLockHeld()annotations early.Also applies to: 417-496, 366-373, 837-838, 865-866, 1080-1096
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
src/Makefile.am(2 hunks)src/bench/coin_selection.cpp(4 hunks)src/bench/wallet_balance.cpp(2 hunks)src/coinjoin/client.cpp(3 hunks)src/coinjoin/util.cpp(2 hunks)src/rpc/coinjoin.cpp(3 hunks)src/rpc/evo.cpp(3 hunks)src/rpc/governance.cpp(2 hunks)src/rpc/masternode.cpp(2 hunks)src/wallet/coincontrol.h(1 hunks)src/wallet/coinjoin.cpp(1 hunks)src/wallet/coinjoin.h(1 hunks)src/wallet/context.h(1 hunks)src/wallet/interfaces.cpp(10 hunks)src/wallet/load.cpp(1 hunks)src/wallet/receive.cpp(10 hunks)src/wallet/receive.h(1 hunks)src/wallet/rpcwallet.cpp(22 hunks)src/wallet/spend.cpp(39 hunks)src/wallet/spend.h(3 hunks)src/wallet/test/coinjoin_tests.cpp(2 hunks)src/wallet/test/coinselector_tests.cpp(5 hunks)src/wallet/test/psbt_wallet_tests.cpp(1 hunks)src/wallet/test/spend_tests.cpp(2 hunks)src/wallet/test/wallet_tests.cpp(20 hunks)src/wallet/transaction.h(3 hunks)src/wallet/wallet.cpp(22 hunks)src/wallet/wallet.h(7 hunks)src/wallet/walletdb.cpp(1 hunks)test/functional/test_runner.py(1 hunks)test/functional/wallet_basic.py(1 hunks)test/functional/wallet_transactiontime_rescan.py(1 hunks)test/lint/lint-circular-dependencies.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/wallet/context.h
- src/wallet/load.cpp
🚧 Files skipped from review as they are similar to previous changes (23)
- test/functional/test_runner.py
- test/functional/wallet_basic.py
- src/bench/wallet_balance.cpp
- src/wallet/coincontrol.h
- src/coinjoin/util.cpp
- src/wallet/test/psbt_wallet_tests.cpp
- test/lint/lint-circular-dependencies.py
- src/Makefile.am
- src/rpc/governance.cpp
- src/rpc/masternode.cpp
- src/bench/coin_selection.cpp
- src/wallet/test/coinjoin_tests.cpp
- src/wallet/walletdb.cpp
- src/wallet/test/spend_tests.cpp
- src/rpc/evo.cpp
- test/functional/wallet_transactiontime_rescan.py
- src/coinjoin/client.cpp
- src/wallet/test/wallet_tests.cpp
- src/wallet/coinjoin.h
- src/wallet/interfaces.cpp
- src/wallet/test/coinselector_tests.cpp
- src/rpc/coinjoin.cpp
- src/wallet/rpcwallet.cpp
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/wallet/receive.h (3)
src/wallet/wallet.h (50)
wallet(399-399)txin(784-784)EXCLUSIVE_LOCKS_REQUIRED(291-291)EXCLUSIVE_LOCKS_REQUIRED(292-292)EXCLUSIVE_LOCKS_REQUIRED(300-300)EXCLUSIVE_LOCKS_REQUIRED(319-319)EXCLUSIVE_LOCKS_REQUIRED(325-325)EXCLUSIVE_LOCKS_REQUIRED(327-327)EXCLUSIVE_LOCKS_REQUIRED(329-329)EXCLUSIVE_LOCKS_REQUIRED(463-463)EXCLUSIVE_LOCKS_REQUIRED(476-476)EXCLUSIVE_LOCKS_REQUIRED(497-497)EXCLUSIVE_LOCKS_REQUIRED(499-499)EXCLUSIVE_LOCKS_REQUIRED(507-507)EXCLUSIVE_LOCKS_REQUIRED(508-512)EXCLUSIVE_LOCKS_REQUIRED(514-514)EXCLUSIVE_LOCKS_REQUIRED(515-515)EXCLUSIVE_LOCKS_REQUIRED(522-522)tx(300-300)tx(319-319)tx(325-325)tx(329-329)tx(613-613)tx(615-615)tx(637-637)tx(646-646)tx(648-648)tx(688-688)tx(786-786)tx(788-788)tx(789-789)tx(911-911)filter(755-755)txout(785-785)script(779-779)script(976-976)script(981-981)script(984-984)script(985-985)wtx(292-292)wtx(499-499)wtx(507-507)wtx(508-508)wtx(514-514)wtx(515-515)wtx(522-522)wtx(523-523)wtx(594-594)wtx(691-691)wtx(693-693)src/wallet/interfaces.cpp (12)
txin(435-439)txin(435-435)txin(445-449)txin(445-445)txout(440-444)txout(440-440)txout(450-454)txout(450-450)script(170-177)script(170-170)script(182-186)script(182-182)src/wallet/transaction.h (2)
tx(249-249)tx(269-269)
src/wallet/coinjoin.cpp (5)
src/wallet/wallet.cpp (19)
outpoint(604-604)GetTxBlocksToMaturity(3461-3471)GetTxBlocksToMaturity(3461-3461)GetTxDepthInMainChain(3436-3442)GetTxDepthInMainChain(3436-3436)IsMine(1382-1386)IsMine(1382-1382)IsMine(1388-1392)IsMine(1388-1388)IsMine(1394-1402)IsMine(1394-1394)IsMine(1404-1411)IsMine(1404-1404)IsSpent(602-619)IsSpent(602-602)IsLockedCoin(2453-2459)IsLockedCoin(2453-2453)GetWalletTx(407-414)GetWalletTx(407-407)src/wallet/spend.cpp (3)
vCoins(421-421)AvailableCoins(71-197)AvailableCoins(71-71)src/wallet/receive.h (6)
CachedTxIsTrusted(48-48)CachedTxIsTrusted(49-49)CachedTxGetDebit(29-29)CachedTxGetCredit(26-26)InputIsMine(13-13)OutputGetCredit(18-18)src/crypto/sha256.h (1)
CSHA256(23-23)src/wallet/coinjoin.h (1)
CachedTxGetAnonymizedCredit(28-28)
src/wallet/wallet.cpp (1)
src/wallet/wallet.h (49)
tx(300-300)tx(319-319)tx(325-325)tx(329-329)tx(613-613)tx(615-615)tx(637-637)tx(646-646)tx(648-648)tx(688-688)tx(786-786)tx(788-788)tx(789-789)tx(911-911)hash(497-497)hash(547-547)hash(550-550)hash(553-553)hash(555-555)hash(614-614)hash(654-654)wtx(292-292)wtx(499-499)wtx(507-507)wtx(508-508)wtx(514-514)wtx(515-515)wtx(522-522)wtx(523-523)wtx(594-594)wtx(691-691)wtx(693-693)batch(334-334)batch(337-337)batch(340-340)batch(551-551)batch(600-600)batch(801-801)batch(804-804)LOCK(812-812)block(616-616)block(617-617)block_height(1012-1012)WalletLogPrintf(956-958)WalletLogPrintf(956-956)assert(408-412)assert(490-490)assert(493-493)assert(1065-1076)
🪛 Cppcheck (2.10-2)
src/wallet/spend.cpp
[performance] 366-366: Function parameter 'coins' should be passed by const reference.
(passedByValue)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
🔇 Additional comments (19)
src/wallet/transaction.h (3)
117-118: Constructor simplified - wallet pointer removedThe
CWalletTxconstructor has been simplified to only take aCTransactionRefparameter, removing the wallet pointer. This aligns with the PR objectives to remove the unusedCWalletTx::pwalletmember, decoupling wallet transaction data from wallet context.
177-179: Inline serialization of order positionThe code for serializing the
nOrderPosvalue has been simplified and inlined directly in theSerializemethod, removing a helper function. This change reduces complexity while maintaining the same functionality.
219-220: Modified deserialization of order positionThe approach for reading
nOrderPosfrommapValuehas been modified to use a direct iterator lookup. This is consistent with the serialization changes and maintains proper functionality.src/wallet/coinjoin.cpp (5)
17-46: Salt management for CoinJoin looks goodThe implementation of CoinJoin salt management functions (
InitCJSaltFromDb,GetCoinJoinSalt,SetCoinJoinSalt) includes proper database handling, migration of legacy data, and thread safety with appropriate assertions.
47-91: Coin selection by denomination is well-implementedThe
SelectTxDSInsByDenominationfunction properly handles selection of inputs for CoinJoin, with appropriate validation, logging, and randomization of input order. It correctly filters for coins that are ready to mix and have the exact denomination amount.
415-425: Free function for anonymized balance calculationThe
GetBalanceAnonymizedfunction is implemented as a free function that takes a wallet reference rather than a member function, consistent with the modular design approach in this PR. It properly locks the wallet when iterating through transactions.
526-575: Well-implemented cached transaction credits retrievalThe
CachedTxGetAvailableCoinJoinCreditsfunction efficiently handles cached transaction credits while maintaining safety checks for immature coinbase transactions and proper error handling for value range validation.
260-346:❌ Incorrect review comment
Recursive function might lead to stack overflow for deep CoinJoin chains
The
GetRealOutpointCoinJoinRoundsfunction is recursive and calls itself when checking inputs. While there is a maximum depth check at the start of the function, the recursive calls could still potentially lead to stack overflow for particularly complex transaction chains.Consider implementing an iterative approach or ensuring the recursion depth is strictly limited. You could verify the maximum recursion depth with:
🏁 Script executed:
#!/bin/bash # Find the maximum possible recursion depth for GetRealOutpointCoinJoinRounds rg -A 1 "MAX_COINJOIN_ROUNDS" --type cppLength of output: 1674
🏁 Script executed:
#!/bin/bash # Locate the implementation of CCoinJoinClientOptions::GetRandomRounds to determine its return range rg -n -C5 "GetRandomRounds" src/coinjoinLength of output: 1966
🏁 Script executed:
#!/bin/bash # Find where nCoinJoinRandomRounds is initialized and its default rg -n -C3 "nCoinJoinRandomRounds" src/coinjoin/options.cpp # Find DEFAULT_COINJOIN_RANDOM_ROUNDS definition rg -n -C3 "DEFAULT_COINJOIN_RANDOM_ROUNDS" -C3 srcLength of output: 109
🏁 Script executed:
#!/bin/bash # Display CCoinJoinClientOptions member declarations and defaults echo "=== src/coinjoin/options.h (first 200 lines) ===" sed -n '1,200p' src/coinjoin/options.h echo echo "=== src/coinjoin/options.cpp (first 200 lines) ===" sed -n '1,200p' src/coinjoin/options.cppLength of output: 8488
No stack overflow risk: recursion depth is strictly bounded
The recursion in GetRealOutpointCoinJoinRounds is capped by MAX_COINJOIN_ROUNDS (16) plus COINJOIN_RANDOM_ROUNDS (3), so the maximum call depth is 19. This fixed, small limit prevents any realistic stack overflow. No iterative rewrite is needed.Likely an incorrect or invalid review comment.
src/wallet/receive.h (4)
13-50: Good use of thread-safety annotations for most wallet functionsMost of the free functions in this file have appropriate
EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)annotations, ensuring thread safety when accessing wallet data. This is particularly important for the migration to free functions from member functions.
51-61: Well-structured Balance structThe
Balancestruct provides a clear and comprehensive representation of different balance types, including trusted, untrusted pending, immature, watchonly balances, and CoinJoin-specific balances.
62-62: GetBalance function replaces member functionThe
GetBalancefree function replaces the previous wallet member function, taking explicit wallet reference and parameters for filtering by depth and address reuse.
64-65: Address balance and grouping functions complete the wallet API refactoringThe functions
GetAddressBalancesandGetAddressGroupingscomplete the set of wallet balance-related operations as free functions, maintaining thread safety with appropriate annotations.src/wallet/spend.h (2)
50-58: Updated COutput constructor to use wallet referencesThe
COutputconstructor now takesconst CWallet&andconst CWalletTx&references instead of pointers, making the ownership and usage semantics clearer. It also uses the newGetTxSpendSizefunction for input size calculation.
68-76: Transaction size calculation functions with proper thread-safety annotationsThe functions
CalculateMaximumSignedInputSizeandCalculateMaximumSignedTxSizehave appropriate thread-safety annotations, ensuring proper usage in threaded contexts.src/wallet/wallet.cpp (5)
613-616: No functional issues spotted – looks fine.
depthis computed once and reused, lock ordering is respected, and the new call toGetTxDepthInMainChain()is consistent with the rest of the refactor.
882-917:ComputeTimeSmartnow honours the rescanning context – good change.Passing
rescanning_old_blockall the way down ensures historical rescans get deterministic timestamps (Bitcoin PR bitcoin#22100 behaviour).
The order “insert intowtxOrdered→ callComputeTimeSmart” avoids self-reference inside the sliding-window loop, so the logic is sound.No further issues spotted.
1045-1091: Propagation of the newrescanning_old_blockflag is complete.
AddToWalletIfInvolvingMe()now relays the flag to the innerAddToWallet()call, keeping smart-timestamp behaviour consistent across rescans.Implementation LGTM.
3436-3479: Helper methods consolidated – duplication appears resolved.
GetTxDepthInMainChain,IsTxLockedByInstantSend,IsTxChainLocked,GetTxBlocksToMaturity, andIsTxImmatureCoinBasenow exist only once in the file, removing the linker-time re-definition issue raised in earlier reviews.
1804-1820:✅ Verification successful
Confirm
fInMempoolis declaredmutableto preserve const-correctness.Both
CanTxBeResentandSubmitTxMemoryPoolAndRelayareconst, yet the latter mutateswtx.fInMempool. This is only standards-compliant if the member is declaredmutable.
🏁 Script executed:
#!/bin/bash # Verify that fInMempool in CWalletTx is mutable ast-grep --pattern $'class CWalletTx { $$$ mutable bool fInMempool $$ }'Length of output: 77
I wasn’t able to locate a
mutablequalifier onfInMempoolyet. Let’s search for its declaration across the repo:
🏁 Script executed:
#!/bin/bash grep -R "fInMempool" -n .Length of output: 606
No const-correctness issue –
fInMempoolis already mutableI confirmed in src/wallet/transaction.h (line 114) that
fInMempoolis declared as:mutable bool fInMempool;This preserves const-correctness when it’s modified in
SubmitTxMemoryPoolAndRelay. No changes needed.
| */ | ||
| bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl); | ||
|
|
||
| bool GenBudgetSystemCollateralTx(CWallet& wallet, CTransactionRef& tx, uint256 hash, CAmount amount, const COutPoint& outpoint = COutPoint()); |
There was a problem hiding this comment.
Missing thread-safety annotation on GenBudgetSystemCollateralTx
The GenBudgetSystemCollateralTx function likely accesses wallet data but is missing a thread-safety annotation.
Add a thread-safety annotation:
-bool GenBudgetSystemCollateralTx(CWallet& wallet, CTransactionRef& tx, uint256 hash, CAmount amount, const COutPoint& outpoint = COutPoint());
+bool GenBudgetSystemCollateralTx(CWallet& wallet, CTransactionRef& tx, uint256 hash, CAmount amount, const COutPoint& outpoint = COutPoint()) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);📝 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.
| bool GenBudgetSystemCollateralTx(CWallet& wallet, CTransactionRef& tx, uint256 hash, CAmount amount, const COutPoint& outpoint = COutPoint()); | |
| bool GenBudgetSystemCollateralTx(CWallet& wallet, CTransactionRef& tx, uint256 hash, CAmount amount, const COutPoint& outpoint = COutPoint()) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); |
| int GetTxSpendSize(const CWallet& wallet, const CWalletTx& wtx, unsigned int out, bool use_max_sig) | ||
| { | ||
| return CalculateMaximumSignedInputSize(wtx.tx->vout[out], &wallet, use_max_sig); | ||
| } |
There was a problem hiding this comment.
Add wallet lock assertion and thread-safety annotation to GetTxSpendSize
CalculateMaximumSignedInputSize() eventually calls wallet.DummySignInput(), which touches mutable wallet state that is protected by cs_wallet.
GetTxSpendSize() neither acquires nor asserts the lock, so callers may invoke it without holding cs_wallet, causing data-races in multi-threaded builds.
-int GetTxSpendSize(const CWallet& wallet, const CWalletTx& wtx, unsigned int out, bool use_max_sig)
+int GetTxSpendSize(const CWallet& wallet, const CWalletTx& wtx, unsigned int out, bool use_max_sig)
+ EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
{
- return CalculateMaximumSignedInputSize(wtx.tx->vout[out], &wallet, use_max_sig);
+ AssertLockHeld(wallet.cs_wallet);
+ return CalculateMaximumSignedInputSize(wtx.tx->vout[out], &wallet, use_max_sig);
}📝 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.
| int GetTxSpendSize(const CWallet& wallet, const CWalletTx& wtx, unsigned int out, bool use_max_sig) | |
| { | |
| return CalculateMaximumSignedInputSize(wtx.tx->vout[out], &wallet, use_max_sig); | |
| } | |
| int GetTxSpendSize(const CWallet& wallet, const CWalletTx& wtx, unsigned int out, bool use_max_sig) | |
| EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) | |
| { | |
| AssertLockHeld(wallet.cs_wallet); | |
| return CalculateMaximumSignedInputSize(wtx.tx->vout[out], &wallet, use_max_sig); | |
| } |
|
This pull request has conflicts, please rebase. |
Introduced in dash#3155, it is now unused.
Functions are in order of declaration in `wallet/wallet.h`. Review with `git log -p -n1 --color-moved=dimmed_zebra`
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
src/wallet/wallet.cpp (2)
1045-1091: 🛠️ Refactor suggestionPossible lock-order inversion between
cs_walletandChain
AddToWalletIfInvolvingMe()executeschain().findBlock()(lines 1075-1079) whilecs_walletis held (asserted above).
Downstream,findBlock()may acquire chain-state locks and ValidationInterface callbacks also grabcs_wallet, giving a classic inversion scenario (wallet → chain here vs. chain → wallet in callbacks).Consider the safer pattern:
- AssertLockHeld(cs_wallet); - ... - bool found_block = chain().findBlock(...); + LOCK(cs_wallet); + ... + // Release wallet lock before querying the chain +} +bool found_block = chain().findBlock(...); +{ + LOCK(cs_wallet); + ...At minimum, add a comment explaining why the current ordering is proven safe for Dash’s lock hierarchy.
882-917: 💡 Verification agent🧩 Analysis chain
Ensure every call-site passes the new
rescanning_old_blockflag
AddToWallet()now expects the additionalrescanning_old_blockboolean which ultimately influences timestamp calculation viaComputeTimeSmart().
Please double-check that all invocations have been updated (tools, tests, RPC paths, coinjoin, etc.). A stale call will silently build with defaultedfalseand may re-introduce the “future-timestamp” bug during rescans.
🏁 Script executed:
#!/bin/bash # List all call-sites that still use the old 4-parameter signature. # Expect no output. rg --line-number --fixed-strings "AddToWallet(" | gawk '!/AddToWallet\([^,]*,[^,]*,[^,]*,[^,]*,[^)]/ {print}'Length of output: 423
Update remaining AddToWallet call-sites to pass the new rescanning_old_block flag
We still have three calls using the old signature (defaulting rescanning_old_block to
falsesilently):
src/wallet/test/wallet_tests.cpp:396src/wallet/rpcdump.cpp:351src/wallet/wallet.cpp:2052Please update each invocation to supply the
rescanning_old_blockboolean (andfFlushOnCloseif needed). For non-rescan paths you can passfalse; inside rescanning logic (e.g. inScanForWalletTransactions) you should forward the currentrescanning_old_blockvalue.
♻️ Duplicate comments (13)
src/wallet/receive.h (1)
13-17:⚠️ Potential issueMissing thread-safety annotation on
AllInputsMine
InputIsMinecorrectly requiresEXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet), butAllInputsMine(which presumably invokes it) has no lock annotation.Either add the same annotation or acquire the lock internally:
-bool AllInputsMine(const CWallet& wallet, const CTransaction& tx, const isminefilter& filter); +bool AllInputsMine(const CWallet& wallet, const CTransaction& tx, + const isminefilter& filter) + EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);src/wallet/spend.h (6)
13-15:⚠️ Potential issueMissing thread-safety annotation on
GetTxSpendSizeThe function accesses wallet data but lacks the appropriate lock annotation.
Add the thread-safety annotation:
-int GetTxSpendSize(const CWallet& wallet, const CWalletTx& wtx, unsigned int out, bool use_max_sig = false); +int GetTxSpendSize(const CWallet& wallet, const CWalletTx& wtx, unsigned int out, + bool use_max_sig = false) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
81-84:⚠️ Potential issueMissing thread-safety annotation on
GetAvailableBalanceWhile
AvailableCoinscorrectly has a lock annotation,GetAvailableBalanceis missing one despite likely callingAvailableCoinsinternally.Add the thread-safety annotation:
-CAmount GetAvailableBalance(const CWallet& wallet, const CCoinControl* coinControl = nullptr); +CAmount GetAvailableBalance(const CWallet& wallet, const CCoinControl* coinControl = nullptr) + EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
108-110:⚠️ Potential issueMissing thread-safety annotation on
AttemptSelectionThe function reads from wallet and invokes wallet-member APIs assuming the caller holds the wallet's mutex, but lacks a lock annotation.
Add the thread-safety annotation:
-bool AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector<COutput> coins, - std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params, CoinType nCoinType = CoinType::ALL_COINS); +bool AttemptSelection(const CWallet& wallet + SHARED_LOCKS_REQUIRED(wallet.cs_wallet), + const CAmount& nTargetValue, + const CoinEligibilityFilter& eligibility_filter, + std::vector<COutput> coins, + std::set<CInputCoin>& setCoinsRet, + CAmount& nValueRet, + const CoinSelectionParams& coin_selection_params, + CoinType nCoinType = CoinType::ALL_COINS);
127-128:⚠️ Potential issueMissing thread-safety annotation on
CreateTransactionThe function accesses wallet data and likely calls functions requiring wallet locks, but lacks an explicit lock annotation.
Add the thread-safety annotation:
-bool CreateTransaction(CWallet& wallet, const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, FeeCalculation& fee_calc_out, bool sign = true, int nExtraPayloadSize = 0); +bool CreateTransaction(CWallet& wallet, const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, const CCoinControl& coin_control, FeeCalculation& fee_calc_out, bool sign = true, int nExtraPayloadSize = 0) + EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
133-134:⚠️ Potential issueMissing thread-safety annotation on
FundTransactionThe function likely calls
CreateTransactionor other wallet-locking functions but lacks an explicit lock annotation.Add the thread-safety annotation:
-bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl); +bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl) + EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
135-136:⚠️ Potential issueMissing thread-safety annotation on
GenBudgetSystemCollateralTxThe function likely accesses wallet data but is missing a thread-safety annotation.
Add the thread-safety annotation:
-bool GenBudgetSystemCollateralTx(CWallet& wallet, CTransactionRef& tx, uint256 hash, CAmount amount, const COutPoint& outpoint = COutPoint()); +bool GenBudgetSystemCollateralTx(CWallet& wallet, CTransactionRef& tx, uint256 hash, CAmount amount, const COutPoint& outpoint = COutPoint()) + EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);src/wallet/spend.cpp (5)
180-180:⚠️ Potential issue
use_max_sigflag is being confused with "allow watch‑only"The last parameter of
COutput's constructor is a size‑estimation flag (use_max_sig). PassingcoinControl->fAllowWatchOnly(a watch‑only spendability flag) here changes fee‑estimation semantics and can lead to large fee mis‑calculations.- vCoins.push_back(COutput(wallet, wtx, i, nDepth, spendable, solvable, safeTx, (coinControl && coinControl->fAllowWatchOnly))); + vCoins.push_back(COutput(wallet, wtx, i, nDepth, spendable, solvable, safeTx, /*use_max_sig=*/false));
837-838:⚠️ Potential issue
use_max_sigflag is being confused with "allow watch‑only"Similar to the issue in COutput construction, the last parameter of
CalculateMaximumSignedTxSize()is a size‑estimation flag (use_max_sig). Passingcoin_control.fAllowWatchOnlyhere changes fee‑estimation semantics.- int nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, coin_control.fAllowWatchOnly); + int nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, /*use_max_sig=*/false);
865-866:⚠️ Potential issue
use_max_sigflag is being confused with "allow watch‑only"Same issue as identified above, needs to be fixed here as well.
- nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, coin_control.fAllowWatchOnly); + nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, /*use_max_sig=*/false);
1080-1101:⚠️ Potential issueWrong
CreateTransactionarity: missing mandatory parameters
GenBudgetSystemCollateralTx()invokesCreateTransaction()with only eight arguments, omitting the mandatorysignandnExtraPayloadSizeparameters introduced in the new signature. This will cause a compilation error.- bool success{CreateTransaction(wallet, vecSend, tx, nFeeRet, nChangePosRet, error, coinControl, fee_calc_out)}; + const bool sign_tx = true; + const int extra_payload = 0; + bool success{CreateTransaction(wallet, vecSend, tx, nFeeRet, nChangePosRet, error, + coinControl, fee_calc_out, sign_tx, extra_payload)};
26-29:⚠️ Potential issueAdd wallet lock assertion and thread-safety annotation
This function accesses wallet data but doesn't have proper thread-safety protections. It should assert that the wallet lock is held and include the appropriate annotation.
-int GetTxSpendSize(const CWallet& wallet, const CWalletTx& wtx, unsigned int out, bool use_max_sig) +int GetTxSpendSize(const CWallet& wallet, const CWalletTx& wtx, unsigned int out, bool use_max_sig) + EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) { + AssertLockHeld(wallet.cs_wallet); return CalculateMaximumSignedInputSize(wtx.tx->vout[out], &wallet, use_max_sig); }src/wallet/wallet.h (1)
462-464:⚠️ Potential issueReturned pointers become dangling as soon as the wallet unlocks
GetSpendableTXs()returns anunordered_set<const CWalletTx*>. Because the pointers refer to objects owned bymapWallet, they are only valid whilecs_walletremains locked and the entries stay intact. If a caller releases the lock after calling this method, they could end up with dangling pointers.A safer approach would be to return reference wrappers or key references:
-std::unordered_set<const CWalletTx*, WalletTxHasher> GetSpendableTXs() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); +std::vector<std::reference_wrapper<const CWalletTx>> GetSpendableTXs() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
🧹 Nitpick comments (3)
src/wallet/wallet.cpp (2)
1823-1841: Mutable member access throughconstreference – acceptable but document
SubmitTxMemoryPoolAndRelay()mutateswtx.fInMempooleven thoughwtxis passed asconst&. This relies onfInMempoolbeing declaredmutable.
To avoid confusion for future maintainers, add a short comment at the field declaration (or right here) stating that the “mutable” qualifier is intentional and thread-safe becausecs_walletis held.
3444-3459: Clarify InstantSend vs ChainLock precedenceReturning
falsefromIsTxLockedByInstantSend()when a ChainLock exists is correct (ChainLocks dominate).
A quick one-liner comment above theif (IsTxChainLocked(wtx))guard would make that subtle policy obvious.src/wallet/spend.cpp (1)
366-368: Function parameter 'coins' should be passed by const referenceFor performance reasons, large parameter types like vectors should be passed by const reference rather than by value to avoid unnecessary copies.
-bool AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector<COutput> coins, +bool AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const std::vector<COutput>& coins, std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params, CoinType nCoinType)🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 366-366: Function parameter 'coins' should be passed by const reference.
(passedByValue)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
src/Makefile.am(2 hunks)src/bench/coin_selection.cpp(4 hunks)src/bench/wallet_balance.cpp(2 hunks)src/coinjoin/client.cpp(3 hunks)src/coinjoin/util.cpp(2 hunks)src/rpc/coinjoin.cpp(3 hunks)src/rpc/evo.cpp(3 hunks)src/rpc/governance.cpp(2 hunks)src/rpc/masternode.cpp(2 hunks)src/wallet/coincontrol.h(1 hunks)src/wallet/coinjoin.cpp(1 hunks)src/wallet/coinjoin.h(1 hunks)src/wallet/context.h(1 hunks)src/wallet/interfaces.cpp(10 hunks)src/wallet/load.cpp(1 hunks)src/wallet/receive.cpp(10 hunks)src/wallet/receive.h(1 hunks)src/wallet/rpcwallet.cpp(22 hunks)src/wallet/spend.cpp(39 hunks)src/wallet/spend.h(3 hunks)src/wallet/test/coinjoin_tests.cpp(2 hunks)src/wallet/test/coinselector_tests.cpp(5 hunks)src/wallet/test/psbt_wallet_tests.cpp(1 hunks)src/wallet/test/spend_tests.cpp(2 hunks)src/wallet/test/wallet_tests.cpp(20 hunks)src/wallet/transaction.h(3 hunks)src/wallet/wallet.cpp(22 hunks)src/wallet/wallet.h(7 hunks)src/wallet/walletdb.cpp(1 hunks)test/functional/test_runner.py(1 hunks)test/functional/wallet_basic.py(1 hunks)test/functional/wallet_transactiontime_rescan.py(1 hunks)test/lint/lint-circular-dependencies.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/wallet/load.cpp
- src/wallet/context.h
🚧 Files skipped from review as they are similar to previous changes (25)
- src/wallet/coincontrol.h
- src/bench/wallet_balance.cpp
- src/coinjoin/util.cpp
- test/functional/test_runner.py
- src/wallet/test/psbt_wallet_tests.cpp
- src/rpc/evo.cpp
- src/rpc/masternode.cpp
- src/rpc/governance.cpp
- src/wallet/test/spend_tests.cpp
- src/bench/coin_selection.cpp
- src/rpc/coinjoin.cpp
- src/wallet/walletdb.cpp
- test/functional/wallet_basic.py
- src/wallet/test/wallet_tests.cpp
- test/functional/wallet_transactiontime_rescan.py
- src/Makefile.am
- test/lint/lint-circular-dependencies.py
- src/wallet/test/coinjoin_tests.cpp
- src/wallet/coinjoin.h
- src/coinjoin/client.cpp
- src/wallet/interfaces.cpp
- src/wallet/coinjoin.cpp
- src/wallet/rpcwallet.cpp
- src/wallet/test/coinselector_tests.cpp
- src/wallet/receive.cpp
🧰 Additional context used
🪛 Cppcheck (2.10-2)
src/wallet/spend.cpp
[performance] 366-366: Function parameter 'coins' should be passed by const reference.
(passedByValue)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
🔇 Additional comments (14)
src/wallet/transaction.h (3)
117-121: Constructor simplification looks good!The constructor has been simplified to only accept a
CTransactionRefargument, removing the wallet dependency and aligning with the broader refactoring effort to remove the unusedCWalletTx::pwalletmember.
177-179: Inlined order position serializationThe serialization of the order position has been inlined instead of using a helper function, which simplifies the code.
219-221: Simplified order position deserializationThe deserialization of the order position is now handled directly in the
Unserializemethod rather than through a helper function, maintaining consistency with the serialization changes.src/wallet/receive.h (1)
51-62: Well-structuredBalancestructThe new
Balancestruct effectively aggregates various types of wallet balances, providing a clean interface for balance retrieval. This is a good approach that encapsulates related data and simplifies balance handling throughout the codebase.src/wallet/spend.h (1)
50-58: UpdatedCOutputconstructor to use free functionsThe constructor now properly takes references to
CWalletandCWalletTxinstead of pointers, and uses the newGetTxSpendSizefunction. This aligns with the overall refactoring approach.src/wallet/wallet.cpp (5)
613-615: Depth check refactor looks correctSwitching to
GetTxDepthInMainChain()guarantees that chain-lock / conflict status is taken into account when deciding whether an outpoint is spent.
No further action required.
987-994: LGTM – constructor call matches the newCWalletTx(tx)signatureThe
piecewise_construct+nullptrplaceholder keeps backward compatibility when loading historical txns without touching disk I/O. 👍
1804-1820: Re-broadcast eligibility helper is correct but watch for future criteria drift
CanTxBeResent()now relies on the new wallet-level helpers (GetTxDepthInMainChain,IsTxLockedByInstantSend, etc.).
Keep in mind that additional “non-rebroadcastable” states (e.g. anti-fee-sniping, v3 package-relay) may appear upstream; encapsulating this logic here is good, but please remember to update it during the next backport.
2578-2614: New rescanning path inComputeTimeSmart()looks saneUsing
block_max_timefor historical blocks fixes the “all tx get NOW()” anomaly during rescans and matches upstream behaviour.
One edge-case to re-verify in functional tests: blocks whose maximum tx time is after the node’s current clock (e.g. due to system clock drift). Consider boundingnTimeSmartbyGetTime()to stay monotonic.
3436-3442: Depth calculation mirrors upstream – good
GetTxDepthInMainChain()now bases depth solely on cached block height plus conflict flag. This avoids expensive ValidationInterface queries on every call. 👍src/wallet/spend.cpp (1)
587-638:✅ Verification successful
Verify all call sites for DiscourageFeeSniping
This function has been significantly refactored from returning a value to directly modifying the transaction. It now also expects that all inputs have been added and performs assertions on sequence numbers.
The implementation looks good and includes comprehensive checks. Let's verify all callers properly prepare the transaction before calling this function:
🏁 Script executed:
#!/bin/bash # Find all callers of DiscourageFeeSniping to verify they properly prepare transactions rg -A 5 'DiscourageFeeSniping' --type cppLength of output: 1249
All call sites properly prepare the transaction for DiscourageFeeSniping
I’ve verified that the only invocation in src/wallet/spend.cpp does the following before calling DiscourageFeeSniping(txNew,…):
- Sets each input’s sequence to
CTxIn::SEQUENCE_FINAL – 1(non-final)- Pushes all inputs into
txNew.vin(ensuring it’s not empty)This satisfies the function’s preconditions (
!tx.vin.empty()and valid non-final sequences). No further changes are needed.src/wallet/wallet.h (3)
594-594:✅ Verification successful
Verify rescanning functionality with new time calculation
The added
rescanning_old_blockparameter allows different timestamp handling during rescans versus real-time transaction discovery.This change aligns with the PR objectives that mention handling transaction timestamps differently during rescans. Let's verify the implementation:
🏁 Script executed:
#!/bin/bash # Find implementations of ComputeTimeSmart to verify usage of the new parameter rg -A 10 "ComputeTimeSmart\(" --type cppLength of output: 2807
Confirmed ComputeTimeSmart handles rescanning_old_block correctly
The added
rescanning_old_blockflag is passed to every call ofComputeTimeSmartand is used within its implementation to selectblock_max_timeduring rescan vs. the usual timestamp logic. No further changes are needed.
690-695:✅ Verification successful
Ensure consistent transaction submission across the codebase
These new methods centralize transaction resubmission logic within the wallet, which is a good architectural improvement.
Let's check that all transaction submission code has been updated to use these new methods:
🏁 Script executed:
#!/bin/bash # Find places where transactions are submitted to ensure they use the new methods rg "AcceptToMemoryPool" --type cppLength of output: 1926
No missing wallet submission updates detected
A search for
AcceptToMemoryPoolshows usages only in core validation, mempool, networking, and test code—not in wallet transaction paths. The new CWallet methods (CanTxBeResentandSubmitTxMemoryPoolAndRelay) cover all wallet-level submission logic, and other subsystems should continue to callAcceptToMemoryPooldirectly. No further changes needed here.
507-516:✅ Verification successful
Verify that all call sites are updated for the new transaction status methods
These methods replace the previous
CWalletTxmember functions and centralize transaction state queries withinCWallet. This is a good refactoring that eliminates circular dependencies.Let's verify that all call sites have been updated to use these new methods:
🏁 Script executed:
#!/bin/bash # Check for any remaining references to the old methods rg "wtx.*->IsChainLocked\(\)" --type cpp rg "wtx.*->LockedByInstantSend\(\)" --type cppLength of output: 91
I’ll broaden the search to catch any remaining direct calls to the old methods:
🏁 Script executed:
#!/bin/bash # Search for any usage of the deprecated CWalletTx methods across the repo rg -n "LockedByInstantSend\(" --type cpp rg -n "IsChainLocked\(" --type cppLength of output: 1489
All deprecated CWalletTx calls have been removed
Verified that there are no remainingwtx.IsChainLocked()orwtx.LockedByInstantSend()calls. Every reference now correctly invokes
wallet.IsTxChainLocked(wtx)wallet.IsTxLockedByInstantSend(wtx)No further updates are required.
Why changing their behaviour though? d29b5ee |
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Need to move last `CreateTransaction` usage away from `wallet.cpp` so that circular dependencies can be resolved in an upcoming commit. `GetBudgetSystemCollateralTX()` is not a getter, it *generates* a transaction, renaming to `GenBudgetSystemCollateralTx()`.
…itcoin#21207 Includes changes that primarily affect `wallet/receive.cpp`
…itcoin#21207 Includes changes that primarily affect `wallet/spend.cpp`, resolves circular dependency.
…/benchmarking code
…ors` and add to test runner
continuation of 87d775d from dash#6192
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/wallet/spend.cpp (1)
587-637:⚠️ Potential issueRemove the assertive false statement in sequence number validation
The code explicitly asserts false if input sequence number is not MAX_SEQUENCE_NONFINAL or SEQUENCE_FINAL, which would cause a program crash if the wallet ever needs to support other sequence numbers in the future.
for (const auto& in : tx.vin) { // Can not be FINAL for locktime to work assert(in.nSequence != CTxIn::SEQUENCE_FINAL); // May be MAX NONFINAL to disable BIP68 if (in.nSequence == CTxIn::MAX_SEQUENCE_NONFINAL) continue; // The wallet does not support any other sequence-use right now. - assert(false); + // Handle other sequence numbers more gracefully + wallet.WalletLogPrintf("WARNING: Unexpected sequence number in transaction: %u\n", in.nSequence); }This change replaces the assertion with a warning log message, which is more appropriate for handling unexpected but not necessarily fatal conditions.
♻️ Duplicate comments (11)
src/wallet/spend.h (4)
81-84:GetAvailableBalancestill lacks theEXCLUSIVE_LOCKS_REQUIREDannotation
This exact issue was raised previously; the annotation is still missing.
108-110:AttemptSelectionis still missing a lock annotation
Earlier feedback pointed this out; please addSHARED_LOCKS_REQUIRED(wallet.cs_wallet)(or stronger) to make the contract explicit.
127-135:CreateTransaction,FundTransaction, andGenBudgetSystemCollateralTxremain un-annotated
All three functions walk the wallet UTXO set and must be called with the wallet mutex held; addEXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet).-bool CreateTransaction(CWallet& wallet, const std::vector<CRecipient>& vecSend, CTransactionRef& tx, ... +bool CreateTransaction(CWallet& wallet, const std::vector<CRecipient>& vecSend, CTransactionRef& tx, ... + EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); -bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, ... +bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, ... + EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); -bool GenBudgetSystemCollateralTx(CWallet& wallet, CTransactionRef& tx, ... +bool GenBudgetSystemCollateralTx(CWallet& wallet, CTransactionRef& tx, ... + EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
13-15: 🛠️ Refactor suggestion
GetTxSpendSizeshould advertise its lock requirement
The implementation touches signing data and queries the wallet; callers are expected to holdwallet.cs_wallet.
Document this with a thread-safety annotation to let clang-thread-safety help:-int GetTxSpendSize(const CWallet& wallet, const CWalletTx& wtx, unsigned int out, bool use_max_sig = false); +int GetTxSpendSize(const CWallet& wallet, const CWalletTx& wtx, unsigned int out, + bool use_max_sig = false) + SHARED_LOCKS_REQUIRED(wallet.cs_wallet);src/wallet/wallet.cpp (1)
1823-1841: Same const-violation pattern inSubmitTxMemoryPoolAndRelay()
wtx.fInMempool |= ret;(l. 1840) writes to a member of aconst CWalletTx&.
Please apply the same fix/verification as forIsTxLockedByInstantSend().- wtx.fInMempool |= ret; + // cast away const because fInMempool is cached-state (should be mutable) + const_cast<CWalletTx&>(wtx).fInMempool |= ret;(or mark
fInMempoolasmutable).src/wallet/spend.cpp (5)
178-180:⚠️ Potential issue
use_max_sigflag is being confused with "allow watch‑only"The last parameter of
COutput's constructor and ofCalculateMaximumSignedTxSize()is a size‑estimation flag (use_max_sig).
PassingcoinControl->fAllowWatchOnly(a watch‑only spendability flag) here changes fee‑estimation semantics and can lead to large fee mis‑calculations.- vCoins.push_back(COutput(wallet, wtx, i, nDepth, spendable, solvable, safeTx, (coinControl && coinControl->fAllowWatchOnly))); + vCoins.push_back(COutput(wallet, wtx, i, nDepth, spendable, solvable, safeTx, /*use_max_sig=*/false));
837-838:⚠️ Potential issueUse explicit size estimation flag in
CalculateMaximumSignedTxSizeAs with the COutput constructor, passing
coin_control.fAllowWatchOnlyto a size estimation parameter can lead to incorrect fee calculations.- int nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, coin_control.fAllowWatchOnly); + int nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, /*use_max_sig=*/false);
865-866:⚠️ Potential issueUse explicit size estimation flag in second
CalculateMaximumSignedTxSizecallThis is the same issue as the previous one, where a watch-only flag is used for size estimation.
- nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, coin_control.fAllowWatchOnly); + nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, /*use_max_sig=*/false);
1080-1101:⚠️ Potential issueCompile-time error: wrong
CreateTransactionarity
GenBudgetSystemCollateralTx()invokesCreateTransaction()with only eight arguments, omitting the mandatorysignandnExtraPayloadSizeparameters introduced in the new signature. The file will not build.- bool success{CreateTransaction(wallet, vecSend, tx, nFeeRet, nChangePosRet, error, coinControl, fee_calc_out)}; + const bool sign_tx = true; + const int extra_payload = 0; + bool success{CreateTransaction(wallet, vecSend, tx, nFeeRet, nChangePosRet, error, + coinControl, fee_calc_out, sign_tx, extra_payload)};
26-29:⚠️ Potential issueAdd wallet lock assertion and thread-safety annotation
The
GetTxSpendSizefunction accesses wallet state throughCalculateMaximumSignedInputSizebut doesn't have proper thread-safety annotations, which could lead to data races.-int GetTxSpendSize(const CWallet& wallet, const CWalletTx& wtx, unsigned int out, bool use_max_sig) +int GetTxSpendSize(const CWallet& wallet, const CWalletTx& wtx, unsigned int out, bool use_max_sig) + EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) { + AssertLockHeld(wallet.cs_wallet); return CalculateMaximumSignedInputSize(wtx.tx->vout[out], &wallet, use_max_sig); }src/wallet/wallet.h (1)
462-464: 🛠️ Refactor suggestionReturn a safer collection type from GetSpendableTXs
The current implementation returns raw pointers to wallet transactions, which can easily lead to dangling pointers if the wallet is unlocked or modified after this function is called.
- std::unordered_set<const CWalletTx*, WalletTxHasher> GetSpendableTXs() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + std::vector<std::reference_wrapper<const CWalletTx>> GetSpendableTXs() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);Using
std::reference_wrapperor transaction hashes would prevent dangling pointers while maintaining performance. The function implementation would need to be updated to match this signature, and all callers would need to be updated accordingly.
🧹 Nitpick comments (2)
src/wallet/wallet.cpp (1)
3436-3442:GetTxDepthInMainChainmishandles unknown chain heightIf the wallet has not yet attached to a chain (
GetLastBlockHeight()returns
-1), the expression
(-1 - wtx.m_confirm.block_height + 1)yields a negative depth for a fresh
unconfirmed transaction, which callers interpret as conflicted.Guard against an uninitialised best-height:
- return (GetLastBlockHeight() - wtx.m_confirm.block_height + 1) * (wtx.isConflicted() ? -1 : 1); + const int tip_height = GetLastBlockHeight(); + if (tip_height < 0) return 0; // chain not ready yet + return (tip_height - wtx.m_confirm.block_height + 1) * + (wtx.isConflicted() ? -1 : 1);src/wallet/spend.cpp (1)
366-367: Passcoinsparameter by const reference for better performanceThe
coinsparameter should be passed by const reference instead of by value to avoid unnecessary copying of potentially large objects.-bool AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector<COutput> coins, +bool AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const std::vector<COutput>& coins, std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params, CoinType nCoinType)🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 366-366: Function parameter 'coins' should be passed by const reference.
(passedByValue)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
src/bench/coin_selection.cpp(4 hunks)src/bench/wallet_balance.cpp(2 hunks)src/coinjoin/client.cpp(3 hunks)src/coinjoin/util.cpp(2 hunks)src/rpc/coinjoin.cpp(3 hunks)src/rpc/evo.cpp(3 hunks)src/rpc/governance.cpp(2 hunks)src/rpc/masternode.cpp(2 hunks)src/wallet/coincontrol.h(1 hunks)src/wallet/coinjoin.cpp(1 hunks)src/wallet/context.h(1 hunks)src/wallet/interfaces.cpp(10 hunks)src/wallet/load.cpp(1 hunks)src/wallet/receive.cpp(10 hunks)src/wallet/receive.h(1 hunks)src/wallet/rpcwallet.cpp(22 hunks)src/wallet/spend.cpp(39 hunks)src/wallet/spend.h(3 hunks)src/wallet/test/coinjoin_tests.cpp(2 hunks)src/wallet/test/coinselector_tests.cpp(5 hunks)src/wallet/test/psbt_wallet_tests.cpp(1 hunks)src/wallet/test/spend_tests.cpp(2 hunks)src/wallet/test/wallet_tests.cpp(20 hunks)src/wallet/transaction.h(4 hunks)src/wallet/wallet.cpp(22 hunks)src/wallet/wallet.h(7 hunks)src/wallet/walletdb.cpp(1 hunks)test/functional/test_runner.py(1 hunks)test/functional/wallet_basic.py(1 hunks)test/functional/wallet_transactiontime_rescan.py(1 hunks)test/lint/lint-circular-dependencies.py(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- src/wallet/load.cpp
- src/bench/wallet_balance.cpp
- src/wallet/walletdb.cpp
- src/rpc/governance.cpp
- src/wallet/context.h
🚧 Files skipped from review as they are similar to previous changes (20)
- src/wallet/coincontrol.h
- test/functional/wallet_basic.py
- test/lint/lint-circular-dependencies.py
- src/wallet/test/spend_tests.cpp
- src/coinjoin/util.cpp
- src/bench/coin_selection.cpp
- src/rpc/masternode.cpp
- src/rpc/evo.cpp
- src/rpc/coinjoin.cpp
- test/functional/test_runner.py
- src/wallet/test/psbt_wallet_tests.cpp
- src/wallet/test/coinselector_tests.cpp
- src/coinjoin/client.cpp
- src/wallet/test/coinjoin_tests.cpp
- src/wallet/test/wallet_tests.cpp
- src/wallet/interfaces.cpp
- src/wallet/receive.h
- src/wallet/rpcwallet.cpp
- src/wallet/coinjoin.cpp
- src/wallet/receive.cpp
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/wallet/spend.h (4)
src/wallet/wallet.h (51)
wallet(399-399)wtx(292-292)wtx(499-499)wtx(507-507)wtx(508-508)wtx(514-514)wtx(515-515)wtx(522-522)wtx(523-523)wtx(594-594)wtx(691-691)wtx(693-693)tx(300-300)tx(319-319)tx(325-325)tx(329-329)tx(613-613)tx(615-615)tx(637-637)tx(646-646)tx(648-648)tx(688-688)tx(786-786)tx(788-788)txout(785-785)EXCLUSIVE_LOCKS_REQUIRED(291-291)EXCLUSIVE_LOCKS_REQUIRED(292-292)EXCLUSIVE_LOCKS_REQUIRED(300-300)EXCLUSIVE_LOCKS_REQUIRED(319-319)EXCLUSIVE_LOCKS_REQUIRED(325-325)EXCLUSIVE_LOCKS_REQUIRED(327-327)EXCLUSIVE_LOCKS_REQUIRED(329-329)EXCLUSIVE_LOCKS_REQUIRED(463-463)EXCLUSIVE_LOCKS_REQUIRED(476-476)EXCLUSIVE_LOCKS_REQUIRED(497-497)EXCLUSIVE_LOCKS_REQUIRED(499-499)EXCLUSIVE_LOCKS_REQUIRED(507-507)EXCLUSIVE_LOCKS_REQUIRED(508-512)EXCLUSIVE_LOCKS_REQUIRED(514-514)EXCLUSIVE_LOCKS_REQUIRED(515-515)EXCLUSIVE_LOCKS_REQUIRED(522-522)output(556-556)output(557-557)filter(755-755)hash(497-497)hash(547-547)hash(550-550)hash(553-553)hash(555-555)hash(614-614)hash(654-654)src/wallet/spend.cpp (2)
GetTxSpendSize(26-29)GetTxSpendSize(26-26)src/wallet/interfaces.cpp (12)
txout(440-444)txout(440-440)txout(450-454)txout(450-450)output(246-251)output(246-246)output(252-257)output(252-252)output(258-262)output(258-258)outputs(468-484)outputs(468-468)src/wallet/coincontrol.h (6)
output(79-82)output(79-79)output(84-87)output(84-84)output(89-92)output(89-89)
src/wallet/spend.cpp (4)
src/wallet/wallet.h (55)
wallet(399-399)wtx(292-292)wtx(499-499)wtx(507-507)wtx(508-508)wtx(514-514)wtx(515-515)wtx(522-522)wtx(523-523)wtx(594-594)wtx(691-691)wtx(693-693)LOCK(812-812)tx(300-300)tx(319-319)tx(325-325)tx(329-329)tx(613-613)tx(615-615)tx(637-637)tx(646-646)tx(648-648)tx(688-688)tx(786-786)tx(788-788)tx(789-789)tx(911-911)output(556-556)output(557-557)filter(755-755)EXCLUSIVE_LOCKS_REQUIRED(291-291)EXCLUSIVE_LOCKS_REQUIRED(292-292)EXCLUSIVE_LOCKS_REQUIRED(300-300)EXCLUSIVE_LOCKS_REQUIRED(319-319)EXCLUSIVE_LOCKS_REQUIRED(325-325)EXCLUSIVE_LOCKS_REQUIRED(327-327)EXCLUSIVE_LOCKS_REQUIRED(329-329)EXCLUSIVE_LOCKS_REQUIRED(463-463)EXCLUSIVE_LOCKS_REQUIRED(476-476)EXCLUSIVE_LOCKS_REQUIRED(497-497)EXCLUSIVE_LOCKS_REQUIRED(499-499)EXCLUSIVE_LOCKS_REQUIRED(507-507)EXCLUSIVE_LOCKS_REQUIRED(508-512)EXCLUSIVE_LOCKS_REQUIRED(514-514)EXCLUSIVE_LOCKS_REQUIRED(515-515)EXCLUSIVE_LOCKS_REQUIRED(522-522)txout(785-785)txin(784-784)hash(497-497)hash(547-547)hash(550-550)hash(553-553)hash(555-555)hash(614-614)hash(654-654)src/wallet/spend.h (10)
COutput(50-58)AvailableCoins(81-81)FindNonChangeParentOutput(88-88)GroupOutputs(95-95)GetTxSpendSize(14-14)AttemptSelection(108-109)SelectCoins(119-120)CalculateMaximumSignedTxSize(75-75)CalculateMaximumSignedTxSize(76-76)CreateTransaction(127-127)src/wallet/receive.cpp (8)
CachedTxIsTrusted(287-318)CachedTxIsTrusted(287-287)CachedTxIsTrusted(320-325)CachedTxIsTrusted(320-320)OutputIsChange(88-91)OutputIsChange(88-88)CachedTxIsFromMe(282-285)CachedTxIsFromMe(282-282)src/wallet/coinselection.cpp (2)
KnapsackSolver(246-354)KnapsackSolver(246-246)
src/wallet/wallet.h (4)
src/wallet/transaction.h (4)
tx(253-253)tx(273-273)tx(274-274)tx(275-275)src/wallet/interfaces.cpp (2)
tx(290-297)tx(290-292)src/wallet/scriptpubkeyman.h (4)
tx(212-212)tx(212-212)tx(358-358)tx(583-583)src/wallet/wallet.cpp (18)
batch(498-498)batch(759-759)batch(886-886)batch(1124-1124)batch(1202-1202)batch(1261-1261)batch(1306-1306)batch(1318-1318)batch(1339-1339)batch(1463-1463)batch(1607-1607)batch(1692-1692)batch(2145-2145)batch(2206-2206)batch(2213-2213)batch(2445-2445)GetTxDepthInMainChain(3436-3442)GetTxDepthInMainChain(3436-3436)
🪛 Cppcheck (2.10-2)
src/wallet/spend.cpp
[performance] 366-366: Function parameter 'coins' should be passed by const reference.
(passedByValue)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: mac-build / Build source
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (5)
src/wallet/transaction.h (1)
105-117: Chain-/InstantSend-lock cache flags are never invalidated – possible stale state
MarkDirty()refreshes all cached amount fields but leavesfIsChainlockedandfIsInstantSendLockeduntouched.
After a reorg or ChainLock/ISL re-evaluation these booleans may contain outdated results, unlike the amounts that get reset.Consider clearing them alongside the other caches:
void MarkDirty() { @@ m_is_cache_empty = true; + fIsChainlocked = false; + fIsInstantSendLocked = false; }(If higher-level logic already forces a full recomputation on every call you may ignore, but please verify.)
src/wallet/wallet.cpp (1)
3445-3453:✅ Verification successful
IsTxLockedByInstantSend()mutatesconst CWalletTx&– breaks const-correctnessInside this
constmethod we assign towtx.fIsChainlockedand
wtx.fIsInstantSendLocked.
That only compiles if both data members are declaredmutable; otherwise it is
undefined behaviour via implicitconst_cast.
- Confirm these members are indeed marked
mutableinwallet.h.- If they are not, add
mutableor re-design to return an updated copy
instead of mutating throughconst.
To confirm whether
fIsChainlockedandfIsInstantSendLockedare declaredmutable, let’s inspect their definitions inwallet.h.
🏁 Script executed:
#!/bin/bash # Locate the wallet header fd wallet.h # Examine CWalletTx and the relevant members grep -R "class CWalletTx" -n src/wallet/wallet.h grep -R "fIsChainlocked" -n src/wallet/wallet.h grep -R "fIsInstantSendLocked" -n src/wallet/wallet.h # Check if they’re declared mutable grep -R "mutable.*fIsChainlocked" -n src/wallet/wallet.h grep -R "mutable.*fIsInstantSendLocked" -n src/wallet/wallet.hLength of output: 430
Let’s locate the actual member names and their declarations:
🏁 Script executed:
#!/bin/bash # Show wallet.h around the CWalletTx declaration echo "=== src/wallet/wallet.h (lines 90–160) ===" sed -n '90,160p' src/wallet/wallet.h || true echo # Show wallet.cpp around the usage snippet echo "=== src/wallet/wallet.cpp (lines 3430–3470) ===" sed -n '3430,3470p' src/wallet/wallet.cpp || true echo # Find all occurrences of the suspect members echo "=== grep fIsChainlocked ===" grep -R "fIsChainlocked" -n . || true echo "=== grep fIsInstantSendLocked ===" grep -R "fIsInstantSendLocked" -n . || true echo # Look for any mutable declarations in wallet.h echo "=== grep mutable in src/wallet/wallet.h ===" grep -R "mutable" -n src/wallet/wallet.h || trueLength of output: 5769
Const correctness: mutable members are already declared
Both fIsChainlocked and fIsInstantSendLocked are declared mutable in src/wallet/transaction.h, so their mutation inside the const methods is intentional and valid. No changes required.
- src/wallet/transaction.h:115–116 declare
mutable bool fIsChainlocked;
mutable bool fIsInstantSendLocked;src/wallet/wallet.h (3)
315-319: Good addition ofrescanning_old_blockparameterThe new parameter helps differentiate transactions discovered during rescans from those discovered in real time. This improves timestamp handling and transaction sorting.
507-515: Good addition of transaction state query methodsThese methods properly centralize transaction state querying in the
CWalletclass and have appropriate thread safety annotations. This is a good refactoring step in removing the dependency ofCWalletTxon the wallet pointer.
690-695: Well-designed transaction management methodsThe new transaction management methods follow the same pattern of centralizing wallet functionality in the
CWalletclass with proper thread safety annotations.
| def set_test_params(self): | ||
| self.disable_mocktime = True | ||
| self.setup_clean_chain = False | ||
| self.num_nodes = 3 | ||
|
|
||
| def skip_test_if_missing_module(self): |
There was a problem hiding this comment.
disable_mocktime=True prevents every set_node_times() call – test will raise RPC errors
With self.disable_mocktime = True the nodes are started with the -disablemocktime flag, which blocks the setmocktime RPC that set_node_times() relies on.
Every call at lines 39-41, 70-72, 81-83, 92-94, 128-130 will therefore fail with JSONRPCError(-1, 'setmocktime not allowed while -disablemocktime is set'), so the test never reaches the assertions.
- self.disable_mocktime = True
+ # Allow setmocktime so we can deterministically move the chain forward
+ self.disable_mocktime = FalsePlease flip (or drop) this flag so the functional test executes successfully.
📝 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 set_test_params(self): | |
| self.disable_mocktime = True | |
| self.setup_clean_chain = False | |
| self.num_nodes = 3 | |
| def skip_test_if_missing_module(self): | |
| def set_test_params(self): | |
| # Allow setmocktime so we can deterministically move the chain forward | |
| self.disable_mocktime = False | |
| self.setup_clean_chain = False | |
| self.num_nodes = 3 | |
| def skip_test_if_missing_module(self): |
| unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx, bool rescanning_old_block) const | ||
| { | ||
| unsigned int nTimeSmart = wtx.nTimeReceived; | ||
| if (!wtx.isUnconfirmed() && !wtx.isAbandoned()) { | ||
| int64_t blocktime; | ||
| if (chain().findBlock(wtx.m_confirm.hashBlock, FoundBlock().time(blocktime))) { | ||
| int64_t latestNow = wtx.nTimeReceived; | ||
| int64_t latestEntry = 0; | ||
|
|
||
| // Tolerate times up to the last timestamp in the wallet not more than 5 minutes into the future | ||
| int64_t latestTolerated = latestNow + 300; | ||
| const TxItems& txOrdered = wtxOrdered; | ||
| for (auto it = txOrdered.rbegin(); it != txOrdered.rend(); ++it) { | ||
| CWalletTx* const pwtx = it->second; | ||
| if (pwtx == &wtx) { | ||
| continue; | ||
| } | ||
| int64_t nSmartTime; | ||
| nSmartTime = pwtx->nTimeSmart; | ||
| if (!nSmartTime) { | ||
| nSmartTime = pwtx->nTimeReceived; | ||
| } | ||
| if (nSmartTime <= latestTolerated) { | ||
| latestEntry = nSmartTime; | ||
| if (nSmartTime > latestNow) { | ||
| latestNow = nSmartTime; | ||
| int64_t block_max_time; | ||
| if (chain().findBlock(wtx.m_confirm.hashBlock, FoundBlock().time(blocktime).maxTime(block_max_time))) { | ||
| if (rescanning_old_block) { | ||
| nTimeSmart = block_max_time; | ||
| } else { | ||
| int64_t latestNow = wtx.nTimeReceived; | ||
| int64_t latestEntry = 0; | ||
|
|
||
| // Tolerate times up to the last timestamp in the wallet not more than 5 minutes into the future | ||
| int64_t latestTolerated = latestNow + 300; | ||
| const TxItems& txOrdered = wtxOrdered; | ||
| for (auto it = txOrdered.rbegin(); it != txOrdered.rend(); ++it) { | ||
| CWalletTx* const pwtx = it->second; | ||
| if (pwtx == &wtx) { | ||
| continue; | ||
| } | ||
| int64_t nSmartTime; | ||
| nSmartTime = pwtx->nTimeSmart; | ||
| if (!nSmartTime) { | ||
| nSmartTime = pwtx->nTimeReceived; | ||
| } | ||
| if (nSmartTime <= latestTolerated) { | ||
| latestEntry = nSmartTime; | ||
| if (nSmartTime > latestNow) { | ||
| latestNow = nSmartTime; | ||
| } | ||
| break; | ||
| } | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| nTimeSmart = std::max(latestEntry, std::min(blocktime, latestNow)); | ||
| nTimeSmart = std::max(latestEntry, std::min(blocktime, latestNow)); | ||
| } | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
ComputeTimeSmart() assigns future-dated timestamps during rescans
When rescanning_old_block is true, the new branch sets
nTimeSmart = block_max_time; (l. 2584-2587).
block_max_time is the maximum timestamp seen among all blocks up to the
tip, which can be much later than the actual block time of the
transaction being scanned.
During a deep rescan this mis-orders historical transactions and may even put
them in the future, breaking UI ordering and potentially confusing downstream
logic that relies on monotonically increasing nTimeSmart.
- if (rescanning_old_block) {
- nTimeSmart = block_max_time;
+ // For historical blocks we should stick to the block's own time
+ // to keep chronological ordering intact.
+ if (rescanning_old_block) {
+ nTimeSmart = blocktime;📝 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.
| unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx, bool rescanning_old_block) const | |
| { | |
| unsigned int nTimeSmart = wtx.nTimeReceived; | |
| if (!wtx.isUnconfirmed() && !wtx.isAbandoned()) { | |
| int64_t blocktime; | |
| if (chain().findBlock(wtx.m_confirm.hashBlock, FoundBlock().time(blocktime))) { | |
| int64_t latestNow = wtx.nTimeReceived; | |
| int64_t latestEntry = 0; | |
| // Tolerate times up to the last timestamp in the wallet not more than 5 minutes into the future | |
| int64_t latestTolerated = latestNow + 300; | |
| const TxItems& txOrdered = wtxOrdered; | |
| for (auto it = txOrdered.rbegin(); it != txOrdered.rend(); ++it) { | |
| CWalletTx* const pwtx = it->second; | |
| if (pwtx == &wtx) { | |
| continue; | |
| } | |
| int64_t nSmartTime; | |
| nSmartTime = pwtx->nTimeSmart; | |
| if (!nSmartTime) { | |
| nSmartTime = pwtx->nTimeReceived; | |
| } | |
| if (nSmartTime <= latestTolerated) { | |
| latestEntry = nSmartTime; | |
| if (nSmartTime > latestNow) { | |
| latestNow = nSmartTime; | |
| int64_t block_max_time; | |
| if (chain().findBlock(wtx.m_confirm.hashBlock, FoundBlock().time(blocktime).maxTime(block_max_time))) { | |
| if (rescanning_old_block) { | |
| nTimeSmart = block_max_time; | |
| } else { | |
| int64_t latestNow = wtx.nTimeReceived; | |
| int64_t latestEntry = 0; | |
| // Tolerate times up to the last timestamp in the wallet not more than 5 minutes into the future | |
| int64_t latestTolerated = latestNow + 300; | |
| const TxItems& txOrdered = wtxOrdered; | |
| for (auto it = txOrdered.rbegin(); it != txOrdered.rend(); ++it) { | |
| CWalletTx* const pwtx = it->second; | |
| if (pwtx == &wtx) { | |
| continue; | |
| } | |
| int64_t nSmartTime; | |
| nSmartTime = pwtx->nTimeSmart; | |
| if (!nSmartTime) { | |
| nSmartTime = pwtx->nTimeReceived; | |
| } | |
| if (nSmartTime <= latestTolerated) { | |
| latestEntry = nSmartTime; | |
| if (nSmartTime > latestNow) { | |
| latestNow = nSmartTime; | |
| } | |
| break; | |
| } | |
| break; | |
| } | |
| } | |
| nTimeSmart = std::max(latestEntry, std::min(blocktime, latestNow)); | |
| nTimeSmart = std::max(latestEntry, std::min(blocktime, latestNow)); | |
| } | |
| } else { | |
| unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx, bool rescanning_old_block) const | |
| { | |
| unsigned int nTimeSmart = wtx.nTimeReceived; | |
| if (!wtx.isUnconfirmed() && !wtx.isAbandoned()) { | |
| int64_t blocktime; | |
| int64_t block_max_time; | |
| if (chain().findBlock(wtx.m_confirm.hashBlock, FoundBlock().time(blocktime).maxTime(block_max_time))) { | |
| // For historical blocks we should stick to the block’s own time | |
| // to keep chronological ordering intact. | |
| if (rescanning_old_block) { | |
| nTimeSmart = blocktime; | |
| } else { | |
| int64_t latestNow = wtx.nTimeReceived; | |
| int64_t latestEntry = 0; | |
| // Tolerate times up to the last timestamp in the wallet not more than 5 minutes into the future | |
| int64_t latestTolerated = latestNow + 300; | |
| const TxItems& txOrdered = wtxOrdered; | |
| for (auto it = txOrdered.rbegin(); it != txOrdered.rend(); ++it) { | |
| CWalletTx* const pwtx = it->second; | |
| if (pwtx == &wtx) { | |
| continue; | |
| } | |
| int64_t nSmartTime = pwtx->nTimeSmart; | |
| if (!nSmartTime) { | |
| nSmartTime = pwtx->nTimeReceived; | |
| } | |
| if (nSmartTime <= latestTolerated) { | |
| latestEntry = nSmartTime; | |
| if (nSmartTime > latestNow) { | |
| latestNow = nSmartTime; | |
| } | |
| break; | |
| } | |
| } | |
| nTimeSmart = std::max(latestEntry, std::min(blocktime, latestNow)); | |
| } | |
| } else { | |
| // … rest of the code … |
2b3d4a5 fix: use `GetTxDepthInMainChain` for CoinJoin depth checks (Kittywhiskers Van Gogh) Pull request description: ## Motivation When testing CoinJoin on [dash#6654](#6654), it was found that CoinJoin on `develop` (fc96190) had stopped working as expected. The regression was traced back to 9a5dc62 in [dash#6633](#6633). Specifically, erroneous substitution of `GetDepthInMainChain()` calls with `GetTxBlocksToMaturity()`. This has been resolved by changing them to the intended substitution, `GetTxDepthInMainChain()` and now CoinJoin should work as intended. ## How Has This Been Tested? A full CoinJoin session run on 2b3d4a5  ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 2b3d4a5 UdjinM6: utACK 2b3d4a5 Tree-SHA512: f4cd134cc5104397e137d80406f3b8afe4c8268a33926186ec441a4e1030c8b57eae5796c5d9dac09c9fc02a7fdd3254d2910d18c75f44723a1d986918ff59c2
…ic, apply `clang-format`, minor housekeeping f4e1846 trivial: minor housekeeping of `wallet/coinjoin.cpp` (Kittywhiskers Van Gogh) a93f5cb chore: classify `wallet/coinjoin.*` as Dash-specific, run `clang-format` (Kittywhiskers Van Gogh) Pull request description: ## Motivation CoinJoin-specific wallet code was split into separate source files in [dash#6633](#6633) but were not listed as Dash-specific (and therefore covered by our set of linters) as cleanup was not within the scope of [dash#6633](#6633). This pull request closes that gap. ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK f4e1846 UdjinM6: utACK f4e1846 Tree-SHA512: 362538af782261a7f62b0ffc6ecad528ccb4c765c86473f97ab3890a024c2d50ae6b75911fe7e617a4debbb4a00c36e041be2eb6ebf596e91bf5475f687a8c2e
Additional Information
CWalletTx::GetWallet()was introduced in dash#3155 (commit) and its use was curtailed in dash#3682 (commit) but it has lingered around. As bitcoin#22100 will get rid ofCWalletTx::pwalletand it is unused, it is removed.As bitcoin#22100 has a substantial diff, it has been split into two commits for ease of review, the former centers around
wallet/receive.cppand the latter aroundwallet/spend.cpp. The former has additional changes that are supplanted/removed in the latter to ensure they can be independently validated.To compare the diff against upstream, squashing the two halves may be desirable.
As the goal of the backport to remove the circular dependencies between
wallet.cppand bothspend.cppandreceive.cpp, Dash-specific code that contributes to this dependency had to be moved.To this effect, CoinJoin-specific code has been moved to its own source file as it relies on code in both
spend.cppandreceive.cpp.GetBudgetSystemCollateralTX()was moved tospend.cppas it usedCreateTransaction()and given a better representative name.CWalletTx::Is{ChainLocked, LockedByInstantSend}()was moved toCWalletand renamed to match the convention ofCWallet::GetTxDepthInMainChain()in preparation for bitcoin#22100. A drawback of the new implementation is that both calls (but especiallyIsTxLockedByInstantSend()) is more expensive as per-transaction result caching is no longer available.Portions of bitcoin#25148 were included in previous commits to ensure that
-Wthread-safetyis satisfied (e.g. annotations forMakeWalletTxStatus()andWalletTxToJSON()were needed whenIsTxLockedByInstantSend()andIsTxChainLocked()were introduced)Breaking Changes
None expected.
Checklist