backport: merge bitcoin#15596, #25497, #17331, #22155 (wallet backports: part 1) #6517
Conversation
WalkthroughThe pull request introduces comprehensive modifications to the Bitcoin Core wallet and testing infrastructure. The changes primarily focus on enhancing coin selection logic, refactoring wallet transaction handling, and expanding the testing framework. A new The testing suite has been expanded with new test files, including The modifications aim to improve code clarity, maintainability, and testing coverage for the wallet functionality, with a focus on simplifying complex logic and providing more structured parameter management for coin selection processes. 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: 3
🧹 Nitpick comments (10)
src/wallet/coinselection.cpp (5)
54-57: Improve parameter documentation
The doc comment is good but could explicitly mention “upper bound =selection_target + cost_of_change” to remove ambiguity about the target range.
66-66: Check for reference consistency
All references to the removed parameter should be updated. Ensure that external call sites or comments referencingnot_input_feesare removed or updated accordingly.
81-81: Add diagnostic logging
When returningfalsehere, consider loggingcurr_available_valueandselection_targetto ease troubleshooting coin selection shortfalls.
139-139: Clarify negative path
Document whycurr_available_valueis decremented byGetSelectionAmount(). A brief comment can clarify that this UTXO is now “used” and no longer available.
336-336: Group condition clarity
Breaking((nBest != nTargetValue && nBest < nTargetValue + nMinChange) || lowest_larger->GetSelectionAmount() <= nBest)into multiple steps can improve maintainability.src/wallet/wallet.h (2)
856-856: Keep coin selection usage consistent
Ensure that new fields inSelectCoins(notablycoin_selection_params) are consistently provided across wallet code, so that fee estimations remain correct.
970-970: Cross-check min confirmations
Double-check thatSelectCoinsMinConfaligns with the user’s intent for partial spending and updated fee logic. Thorough coverage in tests is recommended.src/wallet/wallet.cpp (1)
3007-3042: Streamline fallback selection
Multiple fallback steps for coin selection can complicate debugging. In logs or code comments, clarify each step so future maintainers easily see why a fallback was triggered.src/wallet/test/util.h (1)
20-21: Expand usage notes
CreateSyncedWalletis a handy utility for setting up a test wallet. Consider adding doc comments on recommended parameters (chain state, key validity) to help test authors.src/wallet/coinselection.h (1)
164-164: Helper method naming.
GetSelectionAmount()is a straightforward name for retrieving final amounts. Keep method naming consistent with the rest of the codebase for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/Makefile.test.include(2 hunks)src/bench/coin_selection.cpp(2 hunks)src/wallet/coinselection.cpp(9 hunks)src/wallet/coinselection.h(2 hunks)src/wallet/test/coinselector_tests.cpp(24 hunks)src/wallet/test/spend_tests.cpp(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/wallet.cpp(24 hunks)src/wallet/wallet.h(4 hunks)
🧰 Additional context used
🪛 cppcheck (2.10-2)
src/wallet/test/spend_tests.cpp
[error] 13-13: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (76)
src/wallet/coinselection.cpp (8)
19-19: Ensure consistent usage in sorting comparator
This comparator is straightforward, ensuring descending order by comparingGetSelectionAmount(). Confirm that strictly using>here is the intended ordering logic.
100-101: Double-check waste increments
Confirm that incrementingcurr_wasteby(curr_value - selection_target)won’t cause confusion ifcurr_valueis only slightly above the target. Possibly add a safety check or commentary.
114-114: Avoid negative waste
Subtracting(curr_value - selection_target)can yield negativecurr_wasteif the logic path leads to overshoot. Consider a boundary check or comment clarifying the expected range.
133-133: Subtracting coin value
curr_value -= utxo.GetSelectionAmount();is consistent with previous additions, but reconsider if partial spends might break any assumptions about leftover funds.
144-144: Consider approximate checks
Using exact equality onGetSelectionAmount()might be vulnerable if small rounding differences arise in effective values. Verify that strict equality is truly required.
150-150: Inspect partial exploration
Adding this UTXO’s selection amount tocurr_valuecan significantly impact backtracking logic. Confirm that subsequent backtracking steps remain consistent with new sums.
283-283: Exact match condition
if (group.GetSelectionAmount() == nTargetValue)might skip near matches due to rounding or fees. Consider a small margin if fees could alter the effective value.
287-290: Revisit boundary comparisons
The ranges checked fornTargetValue + nMinChangeandlowest_largerdepend on correct ordering. Minor misalignment might cause skipping feasible UTXOs.src/wallet/wallet.h (2)
104-105: Confirm placeholder sizes
DUMMY_NESTED_P2PKH_INPUT_SIZEis crucial for dummy signature sizing. Verify that 113 bytes remains accurate for nested P2PKH in all relevant scenarios.
997-997: Potential performance overhead
GroupOutputscan be expensive with large output sets. Consider short-circuiting or an alternative grouping approach to optimize for bigger wallets.
[performance]src/wallet/wallet.cpp (3)
2545-2565: Validate multi-balance logic
This updated iteration calculates multiple balance categories (mine_trusted, watchonly_trusted, etc.). Confirm that merging InstantSend lock checks with depth-based checks yields correct final tallies.
2671-2689: Check final vs. spendable condition
The new code ensures final transactions and safeTx usage for zero-depth TXs. Confirm that reorg or conflicting re-check logic handles partial-lifecycle transactions gracefully.
Line range hint
3529-3836: Review fee, change, and subtract-from-amount handling
InCreateTransactionInternal, the logic combining subtract-fee outputs and partial-spend avoidance is intricate. Ensure testing covers edge cases with multiple recipients, dust, and large mempool states.src/wallet/test/util.cpp (1)
17-38: Validate test wallet approach
This function ensures a fresh wallet is loaded, a key is added, and a chain rescan is done. Confirm error states (e.g., missing block data or locked wallet) are handled gracefully to avoid false test pass/fail.src/wallet/test/spend_tests.cpp (4)
13-13: Acknowledge cppcheck macro warning.Cppcheck indicates an unknown macro for
BOOST_FIXTURE_TEST_SUITE. Typically, this can be safely ignored or resolved by configuring cppcheck to recognize Boost test macros. No changes needed here from a functional perspective.🧰 Tools
🪛 cppcheck (2.10-2)
[error] 13-13: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
15-19: Initialization approach looks good.Creating and processing a block, then creating a synced wallet is a clean approach for test setup. The straightforward setup fosters clear, reproducible testing conditions.
25-40: Lambda structure fosters maintainability.Defining
check_txas a lambda is elegantly done. This design localizes the test logic, improving clarity and reuse for the multiple checks that follow.
42-59: Comprehensive coverage of leftover fee scenarios.The test correctly ensures leftover input amounts are credited to the recipient rather than the miner in multiple edge cases (full amount, partial leftover, etc.). This suite thoroughly validates subtract-fee behavior in various conditions.
src/bench/coin_selection.cpp (3)
51-53: Establishing clear coin selection parameters.Introducing
CoinSelectionParamswith explicit defaults (e.g.,avoid_partial=false) simplifies the code and clarifies how parameters are configured. This helps maintain a more predictable coin selection environment.
58-58: Assertion logic ensures correct selection.Verifying the selection of exactly 1003 * COIN, requiring two inputs, detects unintended changes and ensures a stable coin selection process.
100-100: Clear demonstration of BnB exhaustion.The benchmark code is descriptive, ensuring that the BnB algorithm fails gracefully (returns no exact match). This fosters realistic performance measurements for near-impossible coin selection scenarios.
src/wallet/coinselection.h (4)
60-97: NewCoinSelectionParamsstruct provides clarity and flexibility.Encapsulating parameters (e.g., fee rates, output sizes, partial spend settings) into a dedicated struct clarifies coin selection logic and paves the way for future extensibility.
151-153: Fee subtraction flag clarifies logic.The new
m_subtract_fee_outputsfield makes the coin selection flow more transparent, clarifying whether to calculate effective value or actual UTXO value.
156-159: Constructor alignment withCoinSelectionParams.By passing
params.m_subtract_fee_outputs(and fee rates) directly to theOutputGroupconstructor, you maintain consistency and reduce possible misconfiguration.
167-167: RefinedSelectCoinsBnBsignature.Dropping extra parameters (e.g.,
not_input_fees) simplifies usage. The new interface is more focused and less error-prone.src/Makefile.test.include (2)
196-196: Test file addition recognized.Including
wallet/test/spend_tests.cppunder theENABLE_WALLETguard properly ensures that the new test suite builds only when the wallet is enabled.
216-217: Expanded build integration for wallet test utilities.Adding
wallet/test/util.cppandwallet/test/util.hintoBITCOIN_TEST_SUITEcentralizes wallet-related test utilities, promoting greater code reuse and consistency.src/wallet/test/coinselector_tests.cpp (47)
37-37: Initialize coin_selection_params with default arguments
No issues identified. This setup appears consistent with the rest of the code.
141-141: Check for BnB selection with empty UTXO pool
Test logic is clear: verifying no selection is possible when pool is empty.
152-152: Assert BnB selection for 1 Cent
Straightforward test to ensure minimal pick. No issues found.
160-160: Assert BnB selection for 2 Cent
Functional check of coin selection with small increments. Looks good.
169-169: Assert BnB selection for 5 Cent
Test logic properly validates combination of coins.
176-176: Attempt selecting 11 Cent
Ensures correct behavior when target exceeds available UTXOs.
182-182: Verify selection for 0.9 Cent with cost of change
Edge-case check for small values. Implementation is correct.
189-189: Ensure BnB fails with zero cost of change
Another edge-case scenario. No issues found.
198-198: Select 10 Cent from multiple coins
Test ensures combined coverage for bigger sums.
209-209: Check negative effective value scenario
Implementation clarifies partial coin usage with small fees.
215-215: Reject selecting 0.25 Cent
Properly confirms negative scenario for insufficient UTXO coverage.
221-221: Iteration exhaustion test for 17
Ensures that the algorithm exhausts possible combos.
223-223: Iteration exhaustion test for 14
Ensures success outcome below certain threshold.
240-240: BnB selection for 30 Cent from a large pool
Verifies performance with multiple coins. Looks good.
254-254: Repeated test to ensure no solution found for 1 Cent from large coins
Good check for unresolvable scenario.
258-258: CoinSelectionParams coin_selection_params_bnb with specific fees
Ensures distinct fee rates for coin selection. Implementation is consistent.
274-274: Check negative effective value coin
Test ensures that coins with negative effective value are not selected.
281-281: Test with subtract_fee_outputs = true
Verifies correct selection when fee is subtracted from outputs.
302-302: Select coins summing to 10 Cent
Confirms correct combination of multiple coins. No findings.
323-323: Check insufficient funds for 1 Cent
Valid test of newly loaded wallet state.
328-328: Mature vs. new coin check
Verifies that newly added coins are not counted as mature.
331-331: Select 1 Cent from newly added coin
Test ensures newly added coin can be spent with correct filters.
337-337: Inability to make 3 cents of mature coins
Demonstrates that partial sums from non-mature coins are excluded.
340-340: Select 3 Cent from new coins
Checks correct usage of newly minted coins.
350-350: Try to make 38 Cent only from mature coins
Ensures selection fails if new coins are excluded.
352-352: Try to make 38 Cent with standard extra filter
Same scenario with tighter restrictions. Looks correct.
354-354: Make 37 Cent allowing self-sent new coins
Checks correct recognition of self-sent coin classification.
357-357: Make 38 Cent if all coins are accepted
Ensures the test covers the final combination. No issues.
361-361: Try making 34 Cent
Tests best combination approach.
366-366: Try making 7 Cent
Verifies selection from multiple smaller coins.
371-371: Try making 8 Cent
Ensures correct picks from small denominations.
376-376: Try making 9 Cent
Confirms fallback to next bigger coin.
390-390: Check total 71 Cent selection
Tests upper boundary scenario with available coins.
391-391: Check fail for 72 Cent
Verifies correct shortfall detection.
394-394: Try making 16 Cent
Ensures coin selection picks the next bigger coin if partial sum is suboptimal.
401-401: Try making 16 Cent with an extra 5 Cent
Confirms improved combination for closer total.
408-408: Coin selection tie scenario
Tests that biggest coin is chosen in the event of a tie.
413-413: Try making 11 Cent
Checks smaller picks 5+6 as the best combination.
422-422: Try making 95 Cent
Tests picking a single coin of 1 * COIN. Implementation correct.
426-426: Try making 195 Cent
Ensures the next single coin of 2 * COIN is chosen.
518-518: Large input set test
Verifies if selection can handle up to 676 inputs.
544-545: KnapsackSolver random selection check
Ensures random subsets differ between repeated calls.
551-556: KnapsackSolver repeats test
Covers scenario where repeated random picks should differ.
576-577: KnapsackSolver 90 Cent
Verifies random selection from multiple small coins.
602-602: SelectCoinsMinConf for 1003 * COIN
Tests large coin selection scenario. No issues found.
641-644: CoinSelectionParams with default zero fees
Tests stable random target scenario in subsequent code.
647-648: wallet->SelectCoins usage
Checks final selection with coin control. Implementation consistent.src/wallet/test/wallet_tests.cpp (2)
29-29: Include the new wallet test utilities header
This addition enhances reusability of utility methods among tests.
589-589: Use CreateSyncedWallet within ListCoinsTestingSetup
Streamlines wallet creation and ensures consistent chain tip sync.
| assert(utxo.GetSelectionAmount() > 0); | ||
| curr_available_value += utxo.GetSelectionAmount(); |
There was a problem hiding this comment.
Convert assertion to runtime check
assert(utxo.GetSelectionAmount() > 0) might be better replaced by a runtime check and graceful error if zero-amount UTXOs appear unexpectedly.
| if (curr_value + curr_available_value < selection_target || // Cannot possibly reach target with the amount remaining in the curr_available_value. | ||
| curr_value > selection_target + cost_of_change || // Selected value is out of range, go back and try other branch |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refine complex condition
The compound boolean expression could be split or clarified for readability. A helper function or inline narratives could aid in describing each threshold check.
| @@ -123,7 +120,7 @@ | |||
| // Walk backwards to find the last included UTXO that still needs to have its omission branch traversed. | |||
| while (!curr_selection.empty() && !curr_selection.back()) { | |||
| curr_selection.pop_back(); | |||
| curr_available_value += utxo_pool.at(curr_selection.size()).effective_value; | |||
| curr_available_value += utxo_pool.at(curr_selection.size()).GetSelectionAmount(); | |||
There was a problem hiding this comment.
Protect array indexing with checks
utxo_pool.at(curr_selection.size()) relies on curr_selection.size() for indexing. Ensure it never exceeds utxo_pool.size() to avoid out-of-bounds on corner cases.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
src/wallet/test/util.cpp (1)
33-36: Consider using BOOST_REQUIRE instead of BOOST_CHECK for critical assertions.These checks validate critical wallet functionality. If they fail, the test should abort immediately rather than continue with an invalid wallet state.
- 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()); + BOOST_REQUIRE_EQUAL(result.status, CWallet::ScanResult::SUCCESS); + BOOST_REQUIRE_EQUAL(result.last_scanned_block, cchain.Tip()->GetBlockHash()); + BOOST_REQUIRE_EQUAL(*result.last_scanned_height, cchain.Height()); + BOOST_REQUIRE(result.last_failed_block.IsNull());src/wallet/test/spend_tests.cpp (1)
32-33: Extract fee rate constant and document the choice of value.The fee rate of 10000 is hardcoded without explanation. Consider extracting it as a named constant and documenting why this specific value was chosen.
+ // Use 10000 sat/kB fee rate for testing + static constexpr int TEST_FEE_RATE = 10000; - coin_control.m_feerate.emplace(10000); + coin_control.m_feerate.emplace(TEST_FEE_RATE);src/bench/coin_selection.cpp (3)
51-54: Document the rationale for chosen parameter values.The
CoinSelectionParamsconstructor uses specific values for sizes and fee rates without explanation. Consider adding comments to explain these choices and their implications for the benchmark.+ // Use standardized sizes for P2PKH: + // - change_output_size: 34 bytes (P2PKH output) + // - change_spend_size: 148 bytes (P2PKH input) const CoinSelectionParams coin_selection_params(/* change_output_size= */ 34, /* change_spend_size= */ 148, /* effective_feerate= */ CFeeRate(0), /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0), /* tx_no_inputs_size= */ 0, /* avoid_partial= */ false);
100-100: Consider adding performance expectations.The BnB exhaustion test is designed to be computationally intensive, but there are no comments about expected performance characteristics or what constitutes acceptable performance.
Add documentation about expected performance characteristics and thresholds for this benchmark.
Line range hint
31-35: Consider varying coin amounts in benchmark setup.The benchmark uses a fixed pattern of coin amounts (1000 COIN + 3 COIN). Consider parameterizing the test to cover more realistic UTXO distributions.
Would you like me to suggest an implementation that generates more realistic UTXO distributions based on historical blockchain data?
src/wallet/coinselection.h (1)
151-153: Consider adding validation in OutputGroup constructor.While the
m_subtract_fee_outputsfield is well-documented, consider adding validation in the constructor to ensure this flag is used consistently with other fee-related parameters.OutputGroup(const CoinSelectionParams& params) : m_effective_feerate(params.m_effective_feerate), m_long_term_feerate(params.m_long_term_feerate), - m_subtract_fee_outputs(params.m_subtract_fee_outputs) + m_subtract_fee_outputs(params.m_subtract_fee_outputs) +{ + // Validate fee parameters consistency + if (m_subtract_fee_outputs) { + assert(m_effective_feerate != CFeeRate(0) || m_long_term_feerate != CFeeRate(0)); + } +}src/wallet/coinselection.cpp (1)
96-100: Consider breaking down complex condition.The compound condition for backtracking could be split into helper methods for better readability.
+bool isTargetUnreachable(const CAmount& curr_value, const CAmount& curr_available_value, + const CAmount& selection_target) { + return curr_value + curr_available_value < selection_target; +} + +bool isValueOutOfRange(const CAmount& curr_value, const CAmount& selection_target, + const CAmount& cost_of_change) { + return curr_value > selection_target + cost_of_change; +} + +bool isWasteIncreasing(const CAmount& curr_waste, const CAmount& best_waste, + const OutputGroup& first_utxo) { + return curr_waste > best_waste && (first_utxo.fee - first_utxo.long_term_fee) > 0; +} + if (isTargetUnreachable(curr_value, curr_available_value, selection_target) || isValueOutOfRange(curr_value, selection_target, cost_of_change) || isWasteIncreasing(curr_waste, best_waste, utxo_pool.at(0))) { backtrack = true; }src/wallet/test/coinselector_tests.cpp (2)
Line range hint
37-41: Add test cases for CoinSelectionParams variations.Consider adding specific test cases for different combinations of
CoinSelectionParamsto ensure the new parameter struct works correctly in all scenarios.// Add these test cases BOOST_AUTO_TEST_CASE(coin_selection_params_test) { CoinSelectionParams params_no_change(0, 0, CFeeRate(0), CFeeRate(0), CFeeRate(0), 0, false); CoinSelectionParams params_with_change(34, 148, CFeeRate(1000), CFeeRate(100), CFeeRate(0), 0, false); // Test with different params configurations }
641-644: Consider testing edge cases for fee parameters.The test could benefit from additional cases testing boundary conditions for fee calculations.
// Add edge cases for fee parameters CoinSelectionParams cs_params_high_fee(34, 148, CFeeRate(MAX_FEE), CFeeRate(0), CFeeRate(0), 0, false); CoinSelectionParams cs_params_min_fee(34, 148, CFeeRate(MIN_FEE), CFeeRate(0), CFeeRate(0), 0, false);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/Makefile.test.include(2 hunks)src/bench/coin_selection.cpp(2 hunks)src/wallet/coinselection.cpp(9 hunks)src/wallet/coinselection.h(2 hunks)src/wallet/test/coinselector_tests.cpp(24 hunks)src/wallet/test/spend_tests.cpp(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/wallet.cpp(24 hunks)src/wallet/wallet.h(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Makefile.test.include
- src/wallet/test/util.h
- src/wallet/wallet.cpp
🧰 Additional context used
🪛 cppcheck (2.10-2)
src/wallet/test/spend_tests.cpp
[error] 13-13: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (10)
src/wallet/test/util.cpp (1)
27-28: Review lock ordering to prevent potential deadlocks.The
LOCK2macro is used to acquire multiple locks, but the order of lock acquisition could lead to deadlocks if other code acquires these locks in a different order.✅ Verification successful
Lock ordering is consistent across the codebase
Based on the search results, all instances of these locks being acquired together use the same order:
cs_walletfollowed bycs_KeyStore. This consistent ordering across the codebase helps prevent potential deadlocks. The current implementation insrc/wallet/test/util.cppfollows this established pattern.Examples of consistent lock ordering found:
src/wallet/rpcwallet.cpp:LOCK2(pwallet->cs_wallet, spk_man.cs_KeyStore)src/wallet/test/psbt_wallet_tests.cpp:LOCK2(m_wallet.cs_wallet, spk_man->cs_KeyStore)src/wallet/rpcdump.cpp:LOCK2(pwallet->cs_wallet, spk_man.cs_KeyStore)src/test/util/wallet.cpp:LOCK2(wallet.cs_wallet, spk_man->cs_KeyStore)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other instances of these locks being acquired in different orders rg -A 2 "cs_wallet.*cs_KeyStore|cs_KeyStore.*cs_wallet" --type cppLength of output: 2463
src/wallet/test/spend_tests.cpp (1)
26-26: Consider testing with more diverse recipient amounts.The test uses a fixed amount of 500 * COIN. Consider parameterizing the test to cover more scenarios, including edge cases.
Would you like me to generate additional test cases covering edge cases like dust amounts and maximum coin values?
src/wallet/coinselection.h (2)
60-97: Well-structured parameter encapsulation.The new
CoinSelectionParamsstruct effectively encapsulates related parameters with clear documentation. The inclusion of both a parameterized constructor and a default constructor provides good flexibility.
167-167: Function signature improvement.The removal of the
not_input_feesparameter simplifies the interface while maintaining functionality through the newCoinSelectionParamsstruct.src/wallet/coinselection.cpp (2)
401-404: Clean implementation of selection amount calculation.The
GetSelectionAmount()method provides a clear abstraction for handling fee subtraction logic.
123-123:⚠️ Potential issueAdd bounds check for array access.
The array access
utxo_pool.at(curr_selection.size())could potentially cause out-of-bounds access.-curr_available_value += utxo_pool.at(curr_selection.size()).GetSelectionAmount(); +if (curr_selection.size() < utxo_pool.size()) { + curr_available_value += utxo_pool.at(curr_selection.size()).GetSelectionAmount(); +} else { + // Handle error condition + return false; +}Likely invalid or redundant comment.
src/wallet/test/wallet_tests.cpp (1)
589-589: LGTM! Good refactoring of wallet setup logic.The change simplifies test setup by encapsulating multiple wallet initialization steps into a single
CreateSyncedWalletcall, improving code maintainability and readability.src/wallet/wallet.h (3)
104-105: LGTM! Well-documented constant for input size estimation.The addition of
DUMMY_NESTED_P2PKH_INPUT_SIZEwith clear documentation improves code readability and maintainability by centralizing the input size estimation constant.
960-960: Verify impact of removingbnb_usedparameter from SelectCoinsMinConf.Similar to SelectCoins, let's verify all call sites have been updated for the removed parameter.
✅ Verification successful
No references to
bnb_usedparameter found in SelectCoinsMinConf callsAfter checking all call sites of
SelectCoinsMinConfin the codebase, I can confirm that there are no remaining references to thebnb_usedparameter. All calls to the function have been properly updated to match the new signature which no longer includes this parameter. The function is consistently called with the correct number of parameters across wallet.cpp, coinselector_tests.cpp, and coin_selection.cpp.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to SelectCoinsMinConf to ensure they've been updated rg "SelectCoinsMinConf.*bnb_used"Length of output: 35
Script:
#!/bin/bash # Search for all calls to SelectCoinsMinConf to check their argument patterns rg "SelectCoinsMinConf\(" -A 2Length of output: 15013
846-846: Verify impact of removingbnb_usedparameter from SelectCoins.The removal of the
bnb_usedparameter suggests changes to the coin selection algorithm. Let's verify all call sites have been updated accordingly.
| LOCK(wallet->cs_wallet); | ||
| wallet->SetLastBlockProcessed(cchain.Height(), cchain.Tip()->GetBlockHash()); | ||
| } | ||
| wallet->LoadWallet(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for wallet loading.
The LoadWallet() call could fail, but the error is not checked. Consider handling potential failures to prevent silent errors in tests.
- wallet->LoadWallet();
+ bilingual_str error;
+ if (!wallet->LoadWallet(error)) {
+ throw std::runtime_error(strprintf("Wallet loading failed: %s", error.original));
+ }📝 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.
| wallet->LoadWallet(); | |
| bilingual_str error; | |
| if (!wallet->LoadWallet(error)) { | |
| throw std::runtime_error(strprintf("Wallet loading failed: %s", error.original)); | |
| } |
| BOOST_CHECK(wallet->CreateTransaction({recipient}, tx, fee, change_pos, error, coin_control, fee_calc)); | ||
| BOOST_CHECK_EQUAL(tx->vout.size(), 1); | ||
| BOOST_CHECK_EQUAL(tx->vout[0].nValue, recipient.nAmount + leftover_input_amount - fee); | ||
| BOOST_CHECK_GT(fee, 0); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add negative test cases for transaction creation.
The test only verifies successful scenarios. Consider adding test cases for invalid inputs, insufficient funds, or other error conditions.
+ // Test invalid scenarios
+ {
+ CRecipient invalid_recipient{GetScriptForRawPubKey({}), MAX_MONEY + 1, true};
+ CTransactionRef tx;
+ CAmount fee;
+ int change_pos = -1;
+ bilingual_str error;
+ CCoinControl coin_control;
+ FeeCalculation fee_calc;
+ BOOST_CHECK(!wallet->CreateTransaction({invalid_recipient}, tx, fee, change_pos, error, coin_control, fee_calc));
+ BOOST_CHECK(!error.empty());
+ }Committable suggestion skipped: line range outside the PR's diff.
Change needed so that next commit can extend check to include post-signature size.
Courtesy of 3e69939 from bitcoin#20536
Co-authored-by: UdjinM6 <[email protected]>
Not using BnB is a Dash-specific decision and has been commented accordingly.
They were removed as part of dash#3668 and omitted in related backports but as bitcoin#17331 will be a logical partial revert of the former, we need to add them back in preparation for it.
Adapted from 87e0ef9 in bitcoin#25647 and c1a84f1 in bitcoin#26643 Co-authored-by: UdjinM6 <[email protected]>
Additional Information
Dependency for backport: merge bitcoin#21207, #22008, #22686, #22742, #19101, #22183, #22009, #22938, #23288, #24592 (wallet backports: part 2) #6529
bitcoin#17331 logically partially reverts dash#3368 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).
coin_selection_params.tx_noinputs_sizeis said to have a size (sans output count) of9instead of10as we don't have a SegWit field (referred to as1 witness overhead (dummy, flag, stack size)in a code comment) on account of not having SegWit.To allow for backporting bitcoin#17331, portions of bitcoin#21083 (1a6a0b0) and bitcoin#20536 (3e69939) were backported.
bitcoin#17331 seems to have silently broken
CreateTransactionInternalas functional tests fail (see below) despite the backport not intending to change behavior. This was caught due to unit tests introduced in dash#3667.The aberration seems be remedied by portions of bitcoin#25647 and bitcoin#26643 and they have been incorporated into this pull request in a separate commit.
Special thanks to UdjinM6 for figuring this out! 🎉
Error log:
Breaking Changes
Checklist