backport: merge bitcoin#15596, #21207, partial bitcoin#22155 (wallet refactoring) #6543
Conversation
|
This pull request has conflicts, please rebase. |
WalkthroughThe changes introduce new header and source files in the wallet module, including files for receiving, spending, and transaction handling. New methods have been added in these files to assess transaction ownership and credit, calculate change and balance, manage coin selection, and handle transaction signing and CoinJoin operations. The modifications refactor existing wallet functionality by removing outdated functions and structures from central wallet headers and implementations, thereby modularizing the code. Test files have been updated to incorporate a unified wallet synchronization setup using a new helper function. Finally, expected circular dependencies are updated in the linting configuration to account for the temporary dependencies between wallet components introduced by these restructuring efforts. Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (20)
src/wallet/transaction.h (3)
26-28: Consider forward-declaringCWalletor including<wallet/wallet.h>.
Currently,CalculateMaximumSignedInputSizereferencesCWalletwithout an explicit forward declaration or header include statement. This can sometimes lead to compilation issues in certain build configurations or compilers.+class CWallet; // Forward declaration near the top of this file if not already declared int CalculateMaximumSignedInputSize(const CTxOut& txout, const CWallet* pwallet, bool use_max_sig = false);
29-44: ValidatemapValue["n"]parsing.
IfmapValue["n"]contains a non-integer value,LocaleIndependentAtoicould throw or produce an undefined result. Consider adding explicit error handling or logs for safer deserialization.if (!mapValue.count("n")) { nOrderPos = -1; return; } -int64_t temp = LocaleIndependentAtoi<int64_t>(mapValue["n"]); -nOrderPos = temp; +try { + int64_t temp = LocaleIndependentAtoi<int64_t>(mapValue["n"]); + nOrderPos = temp; +} catch (const std::exception& e) { + // Log or handle invalid integer + nOrderPos = -1; +}
299-304: Concurrency annotation reminder.
GetSpendSizeusespwalletwithout a lock. The comment mentions a planned replacement ofNO_THREAD_SAFETY_ANALYSIS. Ensure proper synchronization once the wallet concurrency model is finalized.src/wallet/receive.cpp (5)
11-22: Check for invalidprevout.nindexing.
Although there is a boundary check, consider logging or explicitly handling unusual cases (e.g., out-of-range index caused by corruption or malicious input).
45-51: Error handling for negative values.
InGetCredit(const CTxOut&, ...), there is aMoneyRangecheck that throws. This is fine, but consider whether it might be better to handle or log gracefully if the wallet must remain robust under corrupted data scenarios.
65-85:IsChangelogic is simplistic.
The comment already mentions a TODO for better detection of change outputs. This approach could become brittle in multi-sig or complex script scenarios.Do you want help drafting a more robust solution for identifying change outputs?
113-136: Split up logic for clarity inGetCredit(const CTransaction&, ...).
Collecting credits within the loop might be clearer if refactored into smaller, well-named utility methods, keeping the function more maintainable.
274-306: Trusted transaction logic.
IsTrustedrecurses over inputs to ensure they are also trusted. This can be expensive for large transaction chains. If performance issues arise, consider memoization or caching.src/wallet/spend.cpp (4)
66-193: RefineAvailableCoinscomplexity.
There are numerous parameters and nested checks. For readability, consider segmenting logic for coin-type filtering, coin-control constraints, and quantity/amount checks into separate smaller functions.
274-360: Consider grouping partial outputs more dynamically.
GroupOutputslogic is thorough, but partial groups may be skipped unless specifically allowed. For large UTXO sets, the complex grouping can lead to suboptimal coin selection. Consider adding usage metrics or user toggles to tailor grouping behavior.
495-536: Zero confirmation coin spending logic.
SelectCoinsMinConfwith minimal confirmations can be risky. The user might inadvertently spend inputs from mempool or from other external wallets. Double-check that the user experience is acceptable and consider warnings.
607-995: Long transaction creation logic.
Overall flow is well-structured, but the loop approach with repeatedly recalculating fees can be somewhat difficult to follow. Keep an eye on potential performance hits with large coin sets or complex transactions. Additional comments or micro-optimizations might help.src/wallet/wallet.cpp (3)
2232-2239: Consider adding a locking assertion or clarifying concurrency.Currently,
GetCoinJoinSalt()returns a const reference tonCoinJoinSaltwithout asserting the wallet lock. To prevent potential data races when multiple threads read or write the salt, consider addingAssertLockHeld(cs_wallet), or documenting that the caller must hold the wallet lock.🧰 Tools
🪛 GitHub Actions: Check Potential Conflicts
[error] Conflicts detected with multiple pull requests.
2240-2249: Log a warning for unsuccessful DB reads.If both attempts to read the CoinJoin salt fail, the code silently leaves
nCoinJoinSaltnull. Logging a warning or error would help diagnose issues, for example:if (!batch.ReadCoinJoinSalt(nCoinJoinSalt) && !batch.ReadCoinJoinSalt(nCoinJoinSalt, true)) { + WalletLogPrintf("Warning: Unable to load CoinJoin salt from DB, leaving nCoinJoinSalt as null.\n"); }🧰 Tools
🪛 GitHub Actions: Check Potential Conflicts
[error] Conflicts detected with multiple pull requests.
2251-2260: Add a log entry when WriteCoinJoinSalt fails.This logic returns false if the DB write fails but omits any logging. Consider logging the failure to aid debugging:
if (!batch.WriteCoinJoinSalt(cj_salt)) { + WalletLogPrintf("Error: Failed to store CoinJoin salt in the wallet database.\n"); return false; }🧰 Tools
🪛 GitHub Actions: Check Potential Conflicts
[error] Conflicts detected with multiple pull requests.
src/wallet/receive.h (1)
13-18: Add documentation for theCOutputEntrystructure.Please add documentation explaining the purpose of the structure and its members:
destination: The destination address for the transactionamount: The amount of cryptocurrencyvout: The output index in the transactionsrc/wallet/test/util.h (1)
17-17: Fix typo in namespace comment.-} //namespsace CoinJoin +} // namespace CoinJoinsrc/wallet/transaction.cpp (2)
7-14: Optimize transaction comparison.The current implementation creates unnecessary copies of both transactions. Consider comparing the transactions directly and only clearing the script signatures in memory.
bool CWalletTx::IsEquivalentTo(const CWalletTx& _tx) const { - CMutableTransaction tx1 {*this->tx}; - CMutableTransaction tx2 {*_tx.tx}; - for (auto& txin : tx1.vin) txin.scriptSig = CScript(); - for (auto& txin : tx2.vin) txin.scriptSig = CScript(); - return CTransaction(tx1) == CTransaction(tx2); + if (this->tx->GetHash() != _tx.tx->GetHash()) return false; + if (this->tx->vin.size() != _tx.tx->vin.size()) return false; + for (size_t i = 0; i < this->tx->vin.size(); i++) { + if (this->tx->vin[i].prevout != _tx.tx->vin[i].prevout) return false; + } + return true; }
21-25: Improve readability of time selection.The ternary operator makes the code less readable. Consider using an if statement for clarity.
int64_t CWalletTx::GetTxTime() const { - int64_t n = nTimeSmart; - return n ? n : nTimeReceived; + if (nTimeSmart != 0) { + return nTimeSmart; + } + return nTimeReceived; }src/wallet/spend.h (1)
1-11: Add missing header documentation.Consider adding a brief description of the file's purpose and responsibilities at the top of the file.
// Copyright (c) 2021 The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. + +// This file defines classes and utilities for managing spendable outputs in the wallet.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/Makefile.am(2 hunks)src/Makefile.test.include(1 hunks)src/wallet/receive.cpp(1 hunks)src/wallet/receive.h(1 hunks)src/wallet/spend.cpp(1 hunks)src/wallet/spend.h(1 hunks)src/wallet/test/util.cpp(1 hunks)src/wallet/test/util.h(1 hunks)src/wallet/test/wallet_tests.cpp(2 hunks)src/wallet/transaction.cpp(1 hunks)src/wallet/transaction.h(1 hunks)src/wallet/wallet.cpp(1 hunks)src/wallet/wallet.h(1 hunks)test/lint/lint-circular-dependencies.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Check Potential Conflicts
src/Makefile.test.include
[error] Conflicts detected with multiple pull requests.
src/Makefile.am
[error] Conflicts detected with multiple pull requests.
src/wallet/wallet.h
[error] Conflicts detected with multiple pull requests.
src/wallet/wallet.cpp
[error] Conflicts detected with multiple pull requests.
🔇 Additional comments (21)
src/wallet/transaction.h (4)
5-6: Header guard and license look good.
No issues spotted here.
46-65: Legacy deserialization approach.
CMerkleTxis retained just for backward compatibility. Implementation seems sufficient for its stated purpose.
72-73: Ensurepwalletremains valid for the lifetime ofCWalletTx.
Storing a raw pointer toCWalletmight result in dangling references if the wallet object goes out of scope before this transaction object. Confirm that the wallet’s lifecycle is properly managed.
376-381: Disabling copy constructors is a solid approach.
Preventing accidental copying ofCWalletTxobjects avoids subtle bugs with shared data.src/wallet/receive.cpp (1)
219-272: Potential double-counting inGetAmounts.
Be mindful that if a transaction is both sending and receiving to the same wallet, you might inadvertently double-count for certain filter flags. The current logic looks correct, but consider more explicit tests to ensure the sent amounts and received amounts are not conflated.src/wallet/spend.cpp (3)
31-39: Check out-of-range returns.
CalculateMaximumSignedInputSizereturns-1to indicate failure. Ensure caller logic gracefully handles this, especially in fee calculations that might skip negative results inadvertently.
210-226: Validate chain-of-change detection inFindNonChangeParentOutput.
The loop traces back through the inputs whileIsChangeis true. If the wallet reference is incomplete or the mapWallet is missing entries, this could lead to unexpected break conditions.
423-469: Remove preset inputs fromvCoinscarefully.
Erasing while iterating must be done with caution. The current approach is correct (it = vCoins.erase(it)), but remain vigilant to avoid invalidating the iterator in different contexts.src/wallet/wallet.h (1)
29-32: Clean modular separation appears correct.The new includes for
<wallet/receive.h>,<wallet/spend.h>, and<wallet/transaction.h>align with the PR’s goal of moving transaction-related content out ofwallet.hand into specialized modules. This change helps improve maintainability by reducing the size and complexity of the wallet header.🧰 Tools
🪛 GitHub Actions: Check Potential Conflicts
[error] Conflicts detected with multiple pull requests.
src/wallet/wallet.cpp (2)
2262-2270: Comparator logic looks correct.The
CompareByPriorityfunctor cleanly orders outputs by effective value. No issues found.🧰 Tools
🪛 GitHub Actions: Check Potential Conflicts
[error] Conflicts detected with multiple pull requests.
2271-2273: Ensure wallet remains locked for signing.The explicit
AssertLockHeld(cs_wallet)is commendable, ensuring synchronized access. Overall, this function’s structure seems straightforward. No further change is needed.🧰 Tools
🪛 GitHub Actions: Check Potential Conflicts
[error] Conflicts detected with multiple pull requests.
src/wallet/test/util.cpp (2)
31-32: Add error handling for rescan reservation.The rescan reservation should be checked for success.
- reserver.reserve(); + if (!reserver.reserve()) { + throw std::runtime_error("Failed to reserve for wallet rescan"); + }✅ Verification successful
Error: No shell scripts or web queries found
Action: Add error handling for rescan reservation in src/wallet/test/util.cpp
The current code calls
reserver.reserve()without checking if the reservation succeeded. To avoid proceeding with an unsuccessful reservation (which could lead to further issues in the rescan process), the review comment’s suggestion to check the reservation result and throw an error if it fails is appropriate.
- Location:
src/wallet/test/util.cpp(lines 31–32)- Recommended Change:
if (!reserver.reserve()) { throw std::runtime_error("Failed to reserve for wallet rescan"); }This change ensures that the function does not continue with an invalid reservation state.
24-24: Add error handling for wallet loading.The
LoadWalletcall should check for errors and handle them appropriately.- wallet->LoadWallet(); + DatabaseStatus status; + if (!wallet->LoadWallet(status)) { + throw std::runtime_error(strprintf("Failed to load wallet. Status: %s", status.ToString())); + }✅ Verification successful
Error: No shell scripts or web queries found
Wallet Loading Error Handling Verified
The updated code adds error checking for the wallet loading process by verifying the return value of
wallet->LoadWallet(status)and throwing a runtime error if the loading fails.
- The new diff introduces a
DatabaseStatusvariable and checks the operation's result.- Upon failure, it constructs an error message using
status.ToString()and throws an exception.src/wallet/spend.h (2)
12-62: LGTM! Well-structured class with clear documentation.The
COutputclass is well-designed with:
- Clear member variable documentation
- Efficient constructor implementation
- Proper initialization of computed values
64-72: Consider adding test coverage for BIP69 compliance.The
CompareInputCoinBIP69struct ensures BIP69 compliant input ordering, but there are no tests verifying this behavior.Would you like me to generate test cases to verify BIP69 compliance?
test/lint/lint-circular-dependencies.py (1)
26-29: LGTM! Well-documented temporary dependencies.The added circular dependencies are:
- Clearly marked as temporary
- Have a documented removal plan (in refactor: Make CWalletTx sync state type-safe bitcoin/bitcoin#21206)
- Are necessary for the MOVEONLY commit
src/Makefile.test.include (1)
216-217: LGTM! Test utilities properly added.The test utility files are correctly:
- Added within the wallet-enabled conditional block
- Placed alongside other wallet test files
🧰 Tools
🪛 GitHub Actions: Check Potential Conflicts
[error] Conflicts detected with multiple pull requests.
src/Makefile.am (2)
404-410: LGTM! New wallet headers properly integrated.The new wallet headers are correctly added to
BITCOIN_CORE_Hin alphabetical order.🧰 Tools
🪛 GitHub Actions: Check Potential Conflicts
[error] Conflicts detected with multiple pull requests.
602-607: LGTM! New wallet sources properly integrated.The new wallet source files are correctly added to
libbitcoin_wallet_a_SOURCES.🧰 Tools
🪛 GitHub Actions: Check Potential Conflicts
[error] Conflicts detected with multiple pull requests.
src/wallet/test/wallet_tests.cpp (2)
29-29: LGTM! Include added for test utilities.The new include provides access to the
CreateSyncedWallethelper function, which is part of the wallet refactoring effort.
589-589: LGTM! Simplified wallet initialization using helper function.The change replaces multiple explicit initialization calls with a single
CreateSyncedWalletcall, improving code maintainability and reducing duplication.
|
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
src/wallet/test/util.cpp (1)
33-36: Avoid usingBOOST_CHECKassertions in utility functionsThe use of
BOOST_CHECK_EQUALandBOOST_CHECKmacros within theCreateSyncedWalletutility function is not recommended. These macros are intended for test cases, and their use here could suppress failures or produce unintended side effects. Consider handling errors by checking theresultobject and returning an error status or throwing exceptions as appropriate.Apply this diff to handle errors without using
BOOST_CHECK:CWallet::ScanResult result = wallet->ScanForWalletTransactions(cchain.Genesis()->GetBlockHash(), 0 /* start_height */, {} /* max_height */, reserver, false /* update */); - BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::SUCCESS); - BOOST_CHECK_EQUAL(result.last_scanned_block, cchain.Tip()->GetBlockHash()); - BOOST_CHECK_EQUAL(*result.last_scanned_height, cchain.Height()); - BOOST_CHECK(result.last_failed_block.IsNull()); + if (result.status != CWallet::ScanResult::SUCCESS || + result.last_scanned_block != cchain.Tip()->GetBlockHash() || + *result.last_scanned_height != cchain.Height() || + !result.last_failed_block.IsNull()) { + throw std::runtime_error("Failed to synchronize wallet with the chain."); + }src/wallet/transaction.h (1)
296-344: Address the use ofNO_THREAD_SAFETY_ANALYSISannotationsThe methods
GetAvailableCredit,GetConflicts, andGetDepthInMainChainuse theNO_THREAD_SAFETY_ANALYSISannotation to suppress thread-safety warnings due to access of incomplete types. Consider resolving the underlying issue by forward-declaring necessary types or restructuring code to ensure that thread-safety annotations can be correctly applied.src/wallet/receive.cpp (1)
388-478: RefactorCWallet::GetAddressGroupingsfor improved clarity and performanceThe
GetAddressGroupingsmethod contains complex logic with manual memory management using raw pointers and multiple nested loops. Refactor this code to use modern C++ features such as smart pointers (std::shared_ptr,std::unique_ptr) and algorithms from the Standard Library to enhance readability, reduce the likelihood of memory leaks, and improve performance.src/wallet/spend.cpp (1)
900-911: Simplify change output index retrievalInstead of manually iterating through
txNew.voutto findnewTxOut, consider usingstd::findto simplify the code.Apply this diff to simplify the code:
if (nChangePosInOut != -1) { - int i = 0; - for (const CTxOut& txOut : txNew.vout) - { - if (txOut == newTxOut) - { - nChangePosInOut = i; - break; - } - i++; - } + auto it = std::find(txNew.vout.begin(), txNew.vout.end(), newTxOut); + if (it != txNew.vout.end()) { + nChangePosInOut = std::distance(txNew.vout.begin(), it); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/Makefile.am(2 hunks)src/Makefile.test.include(1 hunks)src/wallet/receive.cpp(1 hunks)src/wallet/receive.h(1 hunks)src/wallet/spend.cpp(1 hunks)src/wallet/spend.h(1 hunks)src/wallet/test/util.cpp(1 hunks)src/wallet/test/util.h(1 hunks)src/wallet/test/wallet_tests.cpp(2 hunks)src/wallet/transaction.cpp(1 hunks)src/wallet/transaction.h(1 hunks)src/wallet/wallet.cpp(1 hunks)src/wallet/wallet.h(1 hunks)test/lint/lint-circular-dependencies.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/Makefile.test.include
- src/wallet/receive.h
- test/lint/lint-circular-dependencies.py
- src/wallet/transaction.cpp
- src/wallet/test/util.h
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Image
🔇 Additional comments (10)
src/wallet/test/util.cpp (1)
27-27: Verify the lock acquisition order to prevent potential deadlocksEnsure that the locks
wallet->cs_walletandspk_man->cs_KeyStoreare acquired in the correct order as per the project's locking policy. Inconsistent lock ordering can lead to deadlocks. According to common practice, it's advisable to lockcs_KeyStorebeforecs_wallet.src/wallet/transaction.h (1)
78-78: Confirm the use ofuint256::ONEforABANDON_HASHThe definition of
ABANDON_HASHusesuint256::ONE. Ensure that this value does not conflict with any valid block hashes and that it uniquely signifies an abandoned transaction. Using a value of all zeros or an unlikely hash may be safer to prevent accidental collisions.src/wallet/spend.h (1)
12-62: Well-structured implementation of theCOutputclassThe
COutputclass is well-defined with proper initialization and encapsulation of transaction output details. The methods provided are appropriate and necessary.src/Makefile.am (1)
404-411: New wallet files correctly integrated into the buildThe new header and source files for
wallet/receive,wallet/spend, andwallet/transactionare properly added toBITCOIN_CORE_Handlibbitcoin_wallet_a_SOURCES. This ensures they will be correctly compiled and linked.Also applies to: 602-608
src/wallet/wallet.h (1)
29-32: LGTM! Good modularization of wallet functionality.The addition of new header files improves code organization by separating wallet functionality into distinct modules for receiving, spending, and transaction handling.
src/wallet/test/wallet_tests.cpp (3)
29-29: LGTM! Good addition of test utilities header.Including the test utilities header improves code organization and reusability.
589-589: LGTM! Good refactoring of wallet setup.Replacing direct wallet initialization with
CreateSyncedWalletimproves code reusability by centralizing the wallet setup logic.
1386-1386: Address the TODO comment about null coinjoin_loader.The comment indicates a potential issue with
coinjoin_loaderbeing null. This should be investigated and fixed.Please verify:
- Is it intentional for
coinjoin_loaderto be null in this test case?- What are the implications of a null
coinjoin_loader?- Should there be a mock or stub implementation instead?
src/wallet/wallet.cpp (2)
2262-2269: LGTM!The comparison struct correctly implements strict weak ordering required for sorting containers and uses const member function as recommended.
2271-2302: Verify transaction signing with test vectorsThe implementation looks correct and handles errors properly. The functions try signing with all available ScriptPubKeyMans and return early on success.
Consider adding test vectors to verify signing behavior:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Search for existing transaction signing test vectors # Test: Look for test files containing transaction signing tests rg -A 5 "SignTransaction.*test"Length of output: 33
🏁 Script executed:
#!/bin/bash # Description: Search for occurrences of 'SignTransaction' in test files to check for existing test vectors # We search within the 'src/test' directory which is the likely location for test cases. rg -A 5 "SignTransaction" src/testLength of output: 8234
Transaction Signing Verified – Approved
The implementation correctly assembles the coins map and iterates through each available ScriptPubKeyMan to sign transactions, returning early on success and handling errors appropriately. Our search in the test directory shows that the signing functionality is exercised in various tests (e.g., in block reward reallocation, deterministic masternode tests, fuzz tests, and setup common utility tests), which indirectly verifies the behavior.
- The wallet signing functions are correctly implemented and covered by existing tests.
- Adding dedicated test vectors for explicit edge cases could further strengthen test coverage.
| isminetype fIsMine = pwallet->IsMine(txout); | ||
| // Only need to handle txouts if AT LEAST one of these is true: | ||
| // 1) they debit from us (sent) | ||
| // 2) the output is to us (received) | ||
| if (nDebit > 0) | ||
| { | ||
| // Don't report 'change' txouts | ||
| if (pwallet->IsChange(txout)) | ||
| continue; | ||
| } | ||
| else if (!(fIsMine & filter)) | ||
| continue; |
There was a problem hiding this comment.
Correct the ownership check in GetAmounts
The condition at line 250 may incorrectly skip outputs that belong to the wallet. The bitwise AND operation with the filter may not yield the expected result if multiple isminefilter flags are set. Ensure that the ownership check correctly identifies outputs owned by the wallet.
Apply this diff to fix the condition:
- else if (!(fIsMine & filter))
+ else if ((fIsMine & filter) == ISMINE_NO)
continue;📝 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.
| isminetype fIsMine = pwallet->IsMine(txout); | |
| // Only need to handle txouts if AT LEAST one of these is true: | |
| // 1) they debit from us (sent) | |
| // 2) the output is to us (received) | |
| if (nDebit > 0) | |
| { | |
| // Don't report 'change' txouts | |
| if (pwallet->IsChange(txout)) | |
| continue; | |
| } | |
| else if (!(fIsMine & filter)) | |
| continue; | |
| isminetype fIsMine = pwallet->IsMine(txout); | |
| // Only need to handle txouts if AT LEAST one of these is true: | |
| // 1) they debit from us (sent) | |
| // 2) the output is to us (received) | |
| if (nDebit > 0) | |
| { | |
| // Don't report 'change' txouts | |
| if (pwallet->IsChange(txout)) | |
| continue; | |
| } | |
| else if ((fIsMine & filter) == ISMINE_NO) | |
| continue; |
There was a problem hiding this comment.
This pull request primarily focuses on trivial and move-only changes. All suggestions that involve behavior change are out of scope.
There was a problem hiding this comment.
@kwvg Understood, I'll retract the suggestion as it involves a behavior change. You're right that this PR should focus on move-only changes.
✏️ Learnings added
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.282Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| bool CWallet::IsTrusted(const CWalletTx& wtx, std::set<uint256>& trusted_parents) const | ||
| { | ||
| AssertLockHeld(cs_wallet); | ||
| // Quick answer in most cases | ||
| if (!chain().checkFinalTx(*wtx.tx)) return false; | ||
| int nDepth = wtx.GetDepthInMainChain(); | ||
| if (nDepth >= 1) return true; | ||
| if (nDepth < 0) return false; | ||
| if (wtx.IsLockedByInstantSend()) return true; | ||
| // using wtx's cached debit | ||
| if (!m_spend_zero_conf_change || !wtx.IsFromMe(ISMINE_ALL)) return false; | ||
|
|
||
| // Don't trust unconfirmed transactions from us unless they are in the mempool. | ||
| if (!wtx.InMempool()) return false; | ||
|
|
||
| // Trusted if all inputs are from us and are in the mempool: | ||
| for (const CTxIn& txin : wtx.tx->vin) | ||
| { | ||
| // Transactions not sent by us: not trusted | ||
| const CWalletTx* parent = GetWalletTx(txin.prevout.hash); | ||
| if (parent == nullptr) return false; | ||
| const CTxOut& parentOut = parent->tx->vout[txin.prevout.n]; | ||
| // Check that this specific input being spent is trusted | ||
| if (IsMine(parentOut) != ISMINE_SPENDABLE) return false; | ||
| // If we've already trusted this parent, continue | ||
| if (trusted_parents.count(parent->GetHash())) continue; | ||
| // Recurse to check that the parent is also trusted | ||
| if (!IsTrusted(*parent, trusted_parents)) return false; | ||
| trusted_parents.insert(parent->GetHash()); | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a recursion depth limit in CWallet::IsTrusted to prevent stack overflows
The IsTrusted function uses recursion to assess the trustworthiness of parent transactions. In cases where there is a very deep or circular transaction chain, this could lead to a stack overflow. Implement a recursion depth limit or convert the recursive logic to an iterative approach to enhance robustness against malicious or unintended complex transaction graphs.
Apply this diff to add a recursion depth limit:
+static constexpr int MAX_TRUSTED_DEPTH = 100;
bool CWallet::IsTrusted(const CWalletTx& wtx, std::set<uint256>& trusted_parents, int depth) const
{
+ if (depth >= MAX_TRUSTED_DEPTH) return false;
AssertLockHeld(cs_wallet);
// Existing code...
for (const CTxIn& txin : wtx.tx->vin)
{
// Existing code...
- if (!IsTrusted(*parent, trusted_parents)) return false;
+ if (!IsTrusted(*parent, trusted_parents, depth + 1)) return false;
trusted_parents.insert(parent->GetHash());
}
return true;
}Update the function declaration to include the depth parameter:
-bool CWallet::IsTrusted(const CWalletTx& wtx, std::set<uint256>& trusted_parents) const
+bool CWallet::IsTrusted(const CWalletTx& wtx, std::set<uint256>& trusted_parents, int depth = 0) const📝 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 CWallet::IsTrusted(const CWalletTx& wtx, std::set<uint256>& trusted_parents) const | |
| { | |
| AssertLockHeld(cs_wallet); | |
| // Quick answer in most cases | |
| if (!chain().checkFinalTx(*wtx.tx)) return false; | |
| int nDepth = wtx.GetDepthInMainChain(); | |
| if (nDepth >= 1) return true; | |
| if (nDepth < 0) return false; | |
| if (wtx.IsLockedByInstantSend()) return true; | |
| // using wtx's cached debit | |
| if (!m_spend_zero_conf_change || !wtx.IsFromMe(ISMINE_ALL)) return false; | |
| // Don't trust unconfirmed transactions from us unless they are in the mempool. | |
| if (!wtx.InMempool()) return false; | |
| // Trusted if all inputs are from us and are in the mempool: | |
| for (const CTxIn& txin : wtx.tx->vin) | |
| { | |
| // Transactions not sent by us: not trusted | |
| const CWalletTx* parent = GetWalletTx(txin.prevout.hash); | |
| if (parent == nullptr) return false; | |
| const CTxOut& parentOut = parent->tx->vout[txin.prevout.n]; | |
| // Check that this specific input being spent is trusted | |
| if (IsMine(parentOut) != ISMINE_SPENDABLE) return false; | |
| // If we've already trusted this parent, continue | |
| if (trusted_parents.count(parent->GetHash())) continue; | |
| // Recurse to check that the parent is also trusted | |
| if (!IsTrusted(*parent, trusted_parents)) return false; | |
| trusted_parents.insert(parent->GetHash()); | |
| } | |
| return true; | |
| } | |
| static constexpr int MAX_TRUSTED_DEPTH = 100; | |
| bool CWallet::IsTrusted(const CWalletTx& wtx, std::set<uint256>& trusted_parents, int depth = 0) const | |
| { | |
| if (depth >= MAX_TRUSTED_DEPTH) return false; | |
| AssertLockHeld(cs_wallet); | |
| // Quick answer in most cases | |
| if (!chain().checkFinalTx(*wtx.tx)) return false; | |
| int nDepth = wtx.GetDepthInMainChain(); | |
| if (nDepth >= 1) return true; | |
| if (nDepth < 0) return false; | |
| if (wtx.IsLockedByInstantSend()) return true; | |
| // using wtx's cached debit | |
| if (!m_spend_zero_conf_change || !wtx.IsFromMe(ISMINE_ALL)) return false; | |
| // Don't trust unconfirmed transactions from us unless they are in the mempool. | |
| if (!wtx.InMempool()) return false; | |
| // Trusted if all inputs are from us and are in the mempool: | |
| for (const CTxIn& txin : wtx.tx->vin) | |
| { | |
| // Transactions not sent by us: not trusted | |
| const CWalletTx* parent = GetWalletTx(txin.prevout.hash); | |
| if (parent == nullptr) return false; | |
| const CTxOut& parentOut = parent->tx->vout[txin.prevout.n]; | |
| // Check that this specific input being spent is trusted | |
| if (IsMine(parentOut) != ISMINE_SPENDABLE) return false; | |
| // If we've already trusted this parent, continue | |
| if (trusted_parents.count(parent->GetHash())) continue; | |
| // Recurse to check that the parent is also trusted | |
| if (!IsTrusted(*parent, trusted_parents, depth + 1)) return false; | |
| trusted_parents.insert(parent->GetHash()); | |
| } | |
| return true; | |
| } |
| if (!reservedest.GetReservedDestination(dest, true)) { | ||
| error = _("Transaction needs a change address, but we can't generate it. Please call keypoolrefill first."); | ||
| } |
There was a problem hiding this comment.
Handle failure in GetReservedDestination appropriately
The call to reservedest.GetReservedDestination(dest, true) may fail, leaving dest invalid. However, the code proceeds to use dest in GetScriptForDestination(dest) without checking if the function succeeded. Please handle the failure case properly by returning early.
Apply this diff to handle the error:
if (!reservedest.GetReservedDestination(dest, true)) {
error = _("Transaction needs a change address, but we can't generate it. Please call keypoolrefill first.");
+ return false;
}
scriptChange = GetScriptForDestination(dest);📝 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.
| if (!reservedest.GetReservedDestination(dest, true)) { | |
| error = _("Transaction needs a change address, but we can't generate it. Please call keypoolrefill first."); | |
| } | |
| if (!reservedest.GetReservedDestination(dest, true)) { | |
| error = _("Transaction needs a change address, but we can't generate it. Please call keypoolrefill first."); | |
| return false; | |
| } | |
| scriptChange = GetScriptForDestination(dest); |
| if (nBytes < 0) { | ||
| error = _("Signing transaction failed"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Handle failure in CalculateMaximumSignedTxSize
The CalculateMaximumSignedTxSize function may return -1 to indicate failure, but this case is not handled properly. Ensure that you check for this failure and handle it appropriately by returning an error.
Apply this diff to handle the error:
nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly);
if (nBytes < 0) {
error = _("Failed to calculate transaction size");
+ return false;
}📝 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.
| if (nBytes < 0) { | |
| error = _("Signing transaction failed"); | |
| return false; | |
| } | |
| nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), this, coin_control.fAllowWatchOnly); | |
| if (nBytes < 0) { | |
| error = _("Failed to calculate transaction size"); | |
| return false; | |
| } |
| const uint256& CWallet::GetCoinJoinSalt() | ||
| { | ||
| LOCK(cs_wallet); | ||
|
|
||
| CAmount balance = 0; | ||
| std::vector<COutput> vCoins; | ||
| AvailableCoins(vCoins, coinControl); | ||
| for (const COutput& out : vCoins) { | ||
| if (out.fSpendable) { | ||
| balance += out.tx->tx->vout[out.i].nValue; | ||
| } | ||
| if (nCoinJoinSalt.IsNull()) { | ||
| InitCJSaltFromDb(); | ||
| } | ||
| return balance; | ||
| return nCoinJoinSalt; | ||
| } |
There was a problem hiding this comment.
Add thread safety to GetCoinJoinSalt()
The function uses lazy initialization pattern but lacks proper synchronization. This could lead to race conditions if multiple threads call this function simultaneously.
Add lock to prevent concurrent initialization:
-const uint256& CWallet::GetCoinJoinSalt()
+const uint256& CWallet::GetCoinJoinSalt()
{
+ LOCK(cs_wallet);
if (nCoinJoinSalt.IsNull()) {
InitCJSaltFromDb();
}
return nCoinJoinSalt;
}📝 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.
| const uint256& CWallet::GetCoinJoinSalt() | |
| { | |
| LOCK(cs_wallet); | |
| CAmount balance = 0; | |
| std::vector<COutput> vCoins; | |
| AvailableCoins(vCoins, coinControl); | |
| for (const COutput& out : vCoins) { | |
| if (out.fSpendable) { | |
| balance += out.tx->tx->vout[out.i].nValue; | |
| } | |
| if (nCoinJoinSalt.IsNull()) { | |
| InitCJSaltFromDb(); | |
| } | |
| return balance; | |
| return nCoinJoinSalt; | |
| } | |
| const uint256& CWallet::GetCoinJoinSalt() | |
| { | |
| LOCK(cs_wallet); | |
| if (nCoinJoinSalt.IsNull()) { | |
| InitCJSaltFromDb(); | |
| } | |
| return nCoinJoinSalt; | |
| } |
(wallet backports: part 1) 153bdc2 merge bitcoin#22155: Add test for subtract fee from recipient behavior (Kittywhiskers Van Gogh) 0185847 fix: correct fee calculations in `CreateTransactionInternal` (Kittywhiskers Van Gogh) 445f489 merge bitcoin#17331: Use effective values throughout coin selection (Kittywhiskers Van Gogh) 7e54bd9 wallet: copy and sort `vecSend` if BIP69 sorting enabled, enable sorting (Kittywhiskers Van Gogh) 9e9e66f partial bitcoin#17331: Use effective values throughout coin selection (Kittywhiskers Van Gogh) 66fe2d4 merge bitcoin#25497: more accurate target for large transactions (Kittywhiskers Van Gogh) 6e4d789 wallet: add back missing `CoinSelectionParams` assignments (Kittywhiskers Van Gogh) bd35042 wallet: move `CoinSelectionParams` to positions expected upstream (Kittywhiskers Van Gogh) 0711e67 wallet: shuffle transaction inputs if we aren't using BIP69 (Kittywhiskers Van Gogh) 1cf1c58 refactor: move selected coin and txin sorting to the end of the scope (Kittywhiskers Van Gogh) ab756ba wallet: Fail if maximum weight is too large (Kittywhiskers Van Gogh) 05c319e refactor: move oversized transaction check to tail end of scope (Kittywhiskers Van Gogh) 6ca51df wallet: Use existing feerate instead of getting a new one (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6543 * Dependency for #6529 * [bitcoin#17331](bitcoin#17331) logically partially reverts [dash#3368](#3668) as Dash Core implemented a calculate-before approach (compared to _then_ Bitcoin Core's calculate-and-adjust approach) and it is being replaced with the current upstream calculate-after approach done in a single-pass instead of iteratively (like the former two). * As the changes are non-trivial, they have been split into a "partial" and a "merge" commit, the first half dedicated just to the logical partial revert and the latter half dedicated to using effective values in coin selection. * BIP69 sorting is disabled in the former half to allow the fix to be in a separate commit while allowing validation of the remaining set of changes. The fix re-enables BIP69 sorting. * Due to the changes introduced in [dash#3368](#3668), a lot of then-redundant code was removed and changes to it upstream were not mirrored in Dash Core. To allow [bitcoin#17331](bitcoin#17331) to work properly, a lot of that unmirrored code was reintroduced and existing code readjusted to match upstream. * `coin_selection_params.tx_noinputs_size` is said to have a size (sans output count) of `9` instead of `10` as we don't have a SegWit field (referred to as `1 witness overhead (dummy, flag, stack size)` in a code comment) on account of not having SegWit. * To allow for backporting [bitcoin#17331](bitcoin#17331), portions of [bitcoin#21083](bitcoin#21083) (1a6a0b0) and [bitcoin#20536](bitcoin@51e2cd3) (3e69939) were backported. * [bitcoin#17331](bitcoin#17331) seems to have silently broken `CreateTransactionInternal` as functional tests fail (see below) despite the backport not intending to change behavior. This was caught due to unit tests introduced in [dash#3667](#3667). The aberration seems be remedied by portions of [bitcoin#25647](bitcoin#25647) and [bitcoin#26643](bitcoin#26643) and they have been incorporated into this pull request in a separate commit. **Special thanks to UdjinM6 for figuring this out!** 🎉 <details> <summary>Error log:</summary> ``` dash@479e0aa4ebbf:/src/dash$ ./src/test/test_dash -t coinselector_tests,wallet_tests Running 21 test cases... wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true] CreateTransactionTest failed at: 2 - 5 wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true] CreateTransactionTest failed at: 4 - 4 wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true] CreateTransactionTest failed at: 4 - 5 wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true] CreateTransactionTest failed at: 6 - 0 wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true] CreateTransactionTest failed at: 6 - 2 wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true] CreateTransactionTest failed at: 6 - 4 wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true] CreateTransactionTest failed at: 6 - 5 *** 7 failures are detected in the test module "Dash Core Test Suite" ``` </details> ## How Has This Been Tested? 153bdc2 was tested on Debian 12 (`bookworm`) mixing ~2 tDASH on default settings.  ## Breaking Changes * If a transaction isn't shuffled using BIP69 (i.e. if an explicit position for the change txout is specified), it will be randomly shuffled (mirroring upstream behavior, [source](https://github.com/bitcoin/bitcoin/blob/51a3ac242c92e69b59df26f8f9e287b31e5c3b0f/src/wallet/wallet.cpp#L3048)). This deviates from earlier behavior where no shuffling would be done at all if BIP69 isn't applied. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [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: UdjinM6: utACK 153bdc2 PastaPastaPasta: utACK 153bdc2 Tree-SHA512: 709b77dce3cea2bbf09eab42c7e70171c3bc6527ff4c387a4db75994da5c0d59025b641d90f734b87a62cdfa8e422d09513697a6e875635de2765a1c9141233e
Additional Information
As dash#6517 and dash#6529 both introduce behavior changes and workarounds that, changes from both PRs that do not affect behavior have been extracted and spun-off into this PR and they will be subsequently rebased on this PR upon merger into
develop. This should let us at least get some of the larger (mostly move-only) diffs merged in and ease review for the aforementioned PRs.bitcoin#21207 can be reviewed using
git log -p -n1 --color-moved=dimmed_zebraBreaking Changes
None expected, no changes in behavior.
Checklist