Reviewing files that changed from the base of the PR and between e14a8bd and 0e33db2.

• src/bench/coin_selection.cpp (3 hunks)
• src/bench/wallet_balance.cpp (1 hunks)
• src/coinjoin/client.cpp (3 hunks)
• src/coinjoin/util.cpp (1 hunks)
• src/interfaces/chain.h (0 hunks)
• src/interfaces/wallet.h (0 hunks)
• src/node/interfaces.cpp (0 hunks)
• src/qt/test/addressbooktests.cpp (1 hunks)
• src/qt/test/wallettests.cpp (1 hunks)
• src/qt/transactiondesc.cpp (0 hunks)
• src/qt/transactionrecord.cpp (1 hunks)
• src/qt/transactionrecord.h (0 hunks)
• src/qt/transactiontablemodel.cpp (0 hunks)
• src/rpc/evo.cpp (1 hunks)
• src/rpc/masternode.cpp (1 hunks)
• src/wallet/coinjoin.cpp (5 hunks)
• src/wallet/coinselection.cpp (13 hunks)
• src/wallet/coinselection.h (6 hunks)
• src/wallet/dump.cpp (1 hunks)
• src/wallet/interfaces.cpp (2 hunks)
• src/wallet/receive.cpp (0 hunks)
• src/wallet/rpcwallet.cpp (4 hunks)
• src/wallet/salvage.cpp (1 hunks)
• src/wallet/spend.cpp (20 hunks)
• src/wallet/spend.h (3 hunks)
• src/wallet/test/coinjoin_tests.cpp (2 hunks)
• src/wallet/test/coinselector_tests.cpp (20 hunks)
• src/wallet/test/ismine_tests.cpp (10 hunks)
• src/wallet/test/scriptpubkeyman_tests.cpp (1 hunks)
• src/wallet/test/spend_tests.cpp (1 hunks)
• src/wallet/test/util.cpp (1 hunks)
• src/wallet/test/util.h (2 hunks)
• src/wallet/test/wallet_test_fixture.cpp (1 hunks)
• src/wallet/test/wallet_tests.cpp (18 hunks)
• src/wallet/wallet.cpp (5 hunks)
• src/wallet/wallet.h (3 hunks)
• src/wallet/wallettool.cpp (5 hunks)
• test/functional/rpc_fundrawtransaction.py (7 hunks)
• test/functional/test_runner.py (1 hunks)
• test/functional/wallet_basic.py (2 hunks)
• test/functional/wallet_timelock.py (1 hunks)
• test/functional/wallet_txn_clone.py (4 hunks)
• test/functional/wallet_txn_doublespend.py (4 hunks)

• src/interfaces/wallet.h
• src/interfaces/chain.h
• src/qt/transactionrecord.h
• src/qt/transactiontablemodel.cpp
• src/qt/transactiondesc.cpp
• src/node/interfaces.cpp
• src/wallet/receive.cpp

test/functional/test_runner.py (1)

376-376: Test suite expanded with wallet_timelock.py

The addition of 'wallet_timelock.py' to BASE_SCRIPTS ensures the new wallet timelock functionality is properly tested as part of the regular test suite, which is good for maintaining code quality and preventing regressions.

src/wallet/test/scriptpubkeyman_tests.cpp (1)

22-22: CWallet constructor updated with m_args parameter

This change updates the wallet construction to match the new constructor signature that takes an ArgsManager reference. This is part of a larger refactoring effort to reduce global state (gArgs) in favor of explicit dependency injection through constructor parameters, which improves testability and encapsulation.

src/qt/transactionrecord.cpp (1)

281-321: Transaction status handling simplified

The code correctly removes the transaction finality check logic that previously set statuses to OpenUntilBlock or OpenUntilDate. This simplification of the transaction status model makes the user experience more straightforward while maintaining all necessary status states for generated and regular transactions.

src/coinjoin/util.cpp (1)

127-130: Updated to use outpoints instead of vecInputCoins

The change properly updates the input selection loop to iterate over tallyItem.outpoints instead of the previous tallyItem.vecInputCoins. This aligns with the codebase-wide refactoring where CompactTallyItem now uses outpoints (vector of COutPoint) for more consistent output referencing.

src/wallet/test/spend_tests.cpp (1)

19-19: Update to add ArgsManager parameter

The change correctly adds the m_args parameter to the CreateSyncedWallet function call, aligning with the updated wallet construction pattern that now requires an ArgsManager reference.

src/qt/test/addressbooktests.cpp (1)

67-67: Updated wallet construction to include ArgsManager

The wallet constructor call now correctly includes gArgs as an additional parameter, consistent with the wallet construction updates throughout the codebase.

src/bench/wallet_balance.cpp (1)

22-22: Wallet constructor updated to include ArgsManager parameter

The CWallet constructor call now correctly includes gArgs as an additional parameter, matching the wallet construction updates across the codebase.

test/functional/wallet_basic.py (2)

18-18: Added utility function import for UTXO handling

Added import for find_vout_for_address function which will be used to improve the precision of UTXO handling in tests.

---

489-491: Improved UTXO locking with explicit vout discovery

The code now explicitly identifies the correct output index (vout) for the transaction before locking the unspent output, improving precision in test transaction handling.

The changes:

1. Added synchronization of mempools to ensure transaction visibility across nodes
2. Used find_vout_for_address to explicitly find the output index
3. Locked the unspent output using the specific txid and vout

src/rpc/evo.cpp (1)

264-265: Improved code clarity with direct property access.

The code now uses direct property access (out.txout.scriptPubKey and out.outpoint) instead of navigating through nested pointers (out.tx->tx->vout[out.i].scriptPubKey and creating a new COutPoint). This simplification is part of the wallet output handling refactoring and makes the code more readable and less prone to errors.

src/wallet/test/ismine_tests.cpp (10)

38-38: Updated CWallet constructor to include ArgsManager parameter.

The CWallet constructor now requires a reference to ArgsManager (m_args) as part of a broader refactor to localize argument access in the wallet code.

---

55-55: Updated CWallet constructor to include ArgsManager parameter.

Same change as above - providing required ArgsManager reference to CWallet constructor.

---

72-72: Updated CWallet constructor to include ArgsManager parameter.

Same change as above - providing required ArgsManager reference to CWallet constructor.

---

89-89: Updated CWallet constructor to include ArgsManager parameter.

Same change as above - providing required ArgsManager reference to CWallet constructor.

---

106-106: Updated CWallet constructor to include ArgsManager parameter.

Same change as above - providing required ArgsManager reference to CWallet constructor.

---

130-130: Updated CWallet constructor to include ArgsManager parameter.

Same change as above - providing required ArgsManager reference to CWallet constructor.

---

148-148: Updated CWallet constructor to include ArgsManager parameter.

Same change as above - providing required ArgsManager reference to CWallet constructor.

---

179-179: Updated CWallet constructor to include ArgsManager parameter.

Same change as above - providing required ArgsManager reference to CWallet constructor.

---

200-200: Updated CWallet constructor to include ArgsManager parameter.

Same change as above - providing required ArgsManager reference to CWallet constructor.

---

214-214: Updated CWallet constructor to include ArgsManager parameter.

Same change as above - providing required ArgsManager reference to CWallet constructor.

src/wallet/dump.cpp (1)

202-202: Updated CWallet constructor to include ArgsManager parameter.

The constructor call now includes gArgs to match the updated CWallet constructor signature, which now requires an ArgsManager reference. This is part of a broader refactor to localize argument access in the wallet code.

src/rpc/masternode.cpp (1)

143-143: Simplified transaction output access.

The code now directly uses out.outpoint instead of going through an intermediary method with out.GetInputCoin().outpoint. This change is part of the broader refactoring where wallet transaction outputs use the COutput class members directly, making the code cleaner and more efficient.

src/wallet/salvage.cpp (1)

136-136: Updated CWallet constructor call to include the gArgs parameter

This change correctly implements the wallet constructor signature update as part of the backporting effort, ensuring the wallet has explicit access to the global arguments.

src/wallet/test/wallet_test_fixture.cpp (1)

13-13: LGTM: Added m_args parameter to wallet constructor

This change correctly updates the test fixture to use the updated CWallet constructor signature, maintaining consistency with the refactored wallet code.

src/wallet/test/util.cpp (1)

18-18: Properly updated CreateSyncedWallet to use ArgsManager parameter

The function signature and implementation have been correctly modified to include and pass the ArgsManager reference to the wallet constructor, consistent with the broader refactoring pattern.

Also applies to: 20-20

src/wallet/test/util.h (1)

10-10: Added ArgsManager forward declaration and updated function signature

These changes correctly maintain consistency between the header and implementation by adding the necessary forward declaration and updating the function signature to match the implementation in util.cpp.

Also applies to: 21-21

src/qt/test/wallettests.cpp (1)

113-113: Constructor update looks good

The wallet constructor now receives gArgs as an additional parameter, aligning with the broader refactor to ensure wallet instances have access to argument management.

test/functional/wallet_txn_clone.py (4)

10-10: Appropriately importing helper function

Importing find_vout_for_address allows more precise identification of outputs for spending.

---

35-40: Well-implemented transaction helper

The spend_txid helper function provides explicit control over spending specific outputs, which aligns with the enhanced output management pattern being introduced in the PR.

---

53-53: Good use of explicit UTXO locking

The output is now explicitly locked before spending, providing better control over UTXO selection and reducing ambiguity in test behavior.

---

66-67: Improved transaction construction approach

Switching from simple wallet sends to explicitly spending identified outputs provides better test control and more accurately tests the transaction cloning behavior.

src/wallet/wallettool.cpp (2)

59-59: Function signature updated correctly

The MakeWallet function now accepts the ArgsManager parameter, consistent with other wallet-related functions.

---

70-70: Wallet constructor updated properly

The constructor call now passes the ArgsManager argument, ensuring wallet instances have access to configuration parameters.

src/wallet/interfaces.cpp (2)

132-141: Good addition of COutput-based constructor

This new overloaded function elegantly constructs a WalletTxOut directly from a COutput object, simplifying code and improving encapsulation by leveraging the richer COutput class.

---

472-473: Simplified coin listing code

Using the new MakeWalletTxOut overload with COutput directly streamlines the code, removing the need to extract components from the output.

test/functional/wallet_txn_doublespend.py (4)

12-12: Imported new utility function for improved UTXO handling.

The addition of find_vout_for_address import enhances the test's capability to locate specific outputs by address, which is needed for the new explicit UTXO handling approach.

---

33-38: Well-structured helper method for explicit UTXO spending.

This new spend_txid method implements a clear pattern for creating, funding, signing, and sending raw transactions that spend specific outputs. This improves test clarity and maintainability by encapsulating the raw transaction creation workflow.

---

58-58: Good use of explicit UTXO locking.

Explicitly locking the unspent output with lockunspent(False, ...) ensures deterministic UTXO selection. This change aligns with the modernized approach to wallet UTXO handling in the updated codebase.

---

89-90: Improved transaction control using explicit output selection.

Replacing wallet-level sendtoaddress calls with explicit spend_txid calls provides more precise control over which UTXOs are spent, making the test's double-spend scenario more deterministic and reliable.

src/wallet/test/coinjoin_tests.cpp (4)

133-134: Updated CWallet constructor call to include ArgsManager reference.

The constructor now passes m_args to the CWallet constructor, aligning with the broader change to include ArgsManager in wallet instantiation across the codebase.

---

203-203: Changed loop counter type for improved type safety.

The loop index was changed from size_t to uint32_t to better match the data type of transaction output indices, which are typically 32-bit unsigned integers.

---

208-208: Refactored to use COutPoint for output representation.

The code now uses outpoints.emplace_back(COutPoint{tx->GetHash(), n}) instead of the previous approach with vecInputCoins. This aligns with the broader refactoring to use COutPoint objects for representing transaction outputs throughout the wallet code.

---

212-212: Updated assertion to check outpoints size.

The assertion was updated to verify tallyItem.outpoints.size() matches vecAmounts.size(), reflecting the change from vecInputCoins to outpoints as the container for transaction outputs.

src/wallet/wallet.h (3)

131-131: Refactored CompactTallyItem to use COutPoint directly.

Changed std::vector<CInputCoin> vecInputCoins to std::vector<COutPoint> outpoints, simplifying the wallet's internal representation of outputs. This change is part of the broader refactoring to replace the legacy CInputCoin class with more focused data structures.

---

348-350: Added ArgsManager reference to CWallet class.

Added const ArgsManager& m_args as a private member to provide localized access to application arguments. This improves encapsulation by removing direct dependencies on global argument variables.

---

442-444: Updated CWallet constructor to accept ArgsManager.

Modified the constructor signature to include const ArgsManager& args parameter and initialize the new m_args member. This enables better testing and dependency injection by explicitly passing the arguments rather than relying on globals.

test/functional/wallet_timelock.py (5)

1-8: Well-structured test file with proper licensing and imports.

The file has appropriate licensing headers and imports only the necessary components from the test framework. This follows the established pattern for Bitcoin Core functional tests.

---

10-16: Clear test class definition with proper setup.

The WalletLocktimeTest class correctly inherits from BitcoinTestFramework and implements the required methods:

• set_test_params configures a single node for the test
• skip_test_if_missing_module ensures the test is skipped if wallet support is not available

---

17-32: Effective timelock transaction setup.

The test:

1. Gets the current block's median time past (MTP)
2. Creates a new address with a Unicode label
3. Sends funds with a locktime just before the current MTP
4. Confirms the transaction in a new block

This creates the necessary conditions to test locktime handling in the wallet.

---

33-46: Comprehensive verification of timelock behavior.

The test verifies that changing the node's mocktime to before the transaction's locktime does not affect the wallet's view of confirmed transactions. It checks multiple wallet functions:

• getreceivedbyaddress
• getreceivedbylabel
• listreceivedbyaddress
• listreceivedbylabel
• getbalances
• listunspent

This thoroughly validates that transaction finality in the wallet is properly based on confirmation status, not on the transaction's locktime relative to the current time.

---

49-51: Standard test execution boilerplate.

The file properly implements the standard Python pattern for allowing the test to be run directly or as part of the test suite.

src/coinjoin/client.cpp (4)

1471-1472: Refactor: Updated to use outpoints instead of vecInputCoins

This change is part of a broader refactoring where CompactTallyItem now uses outpoints instead of vecInputCoins to track inputs, maintaining the same logic for checking if this is a single input coin.

---

1565-1565: Code simplification: Direct access to txout property

This change simplifies the code by directly accessing the output's txout property instead of using the nested pointer access pattern output.tx->tx->vout[output.i], making the code more readable and less error-prone.

---

1568-1568: Code simplification: Direct access to outpoint properties

This change simplifies the code by directly accessing the output's outpoint properties instead of using output.tx->GetHash() and output.i, aligning with the broader refactoring to use the richer COutput class.

---

1639-1640: Refactor: Updated to use outpoints instead of vecInputCoins

Similar to previous changes, this replaces the check on vecInputCoins.size() with outpoints.size() to maintain compatibility with the refactored CompactTallyItem structure.

src/wallet/coinjoin.cpp (6)

71-72: Refactoring to direct field access looks cleaner.

The code now directly accesses the COutPoint and CTxOut fields instead of using nested accessors, making it more readable and maintainable.

---

77-78: Clean field access improvement.

Using direct field references from the COutput structure improves code clarity.

---

98-98: Good modernization using effective_value.

The comparison now uses the effective_value property of COutput objects which aligns with the refactoring from CInputCoin to COutput across the codebase.

---

117-117: Consistent direct field access pattern.

This follows the pattern established throughout the PR of directly accessing txout's nValue property.

---

172-172: Updated interface using outpoints collection.

The code now checks against outpoints.size() instead of the previous vecInputCoins implementation, maintaining the same functionality but using the modernized data structure.

---

194-194: Consistent data structure update.

Using outpoints vector instead of the previous vecInputCoins collection, which aligns with the broader refactoring of wallet output handling.

src/wallet/wallet.cpp (5)

953-953: Replaced global argument access with instance member.

Now using m_args instance member instead of global gArgs for accessing command-line arguments, improving encapsulation.

---

2721-2721: Updated wallet instantiation to use ArgsManager parameter.

The wallet constructor now accepts an ArgsManager reference which gets stored as a member variable, enabling the removal of global gArgs references throughout the code.

---

2957-2961: Consistent local args parameter usage.

Using the local args parameter instead of global gArgs for accessing the consolidate fee rate setting, following the pattern established throughout the PR.

---

3374-3374: Localized instantsendnotify argument access.

Replaced global argument access with wallet instance member m_args, consistent with other similar changes in this PR.

---

3551-3551: Improved wallet backups configuration access.

Using the instance member m_args instead of global gArgs for accessing the wallet backup configuration setting, completing the consistent pattern of localized argument access.

src/wallet/test/coinselector_tests.cpp (1)

32-38: Potential overflow / concurrency issue with nextLockTime

nextLockTime is a non-atomic signed int.
After ~2 billion coins have been added across all tests the counter will wrap, re-creating txids.
While that seems far-fetched, switching to an unsigned 64-bit (or even a monotonic std::atomic_uint64_t) costs nothing and removes the edge-case entirely.

-static int nextLockTime = 0;
+static std::atomic<uint64_t> nextLockTime{0};

(You may need to #include <atomic>.)

src/wallet/coinselection.cpp (1)

424-445: Assertion may trigger for subtract_fee_outputs selections

When change_cost == 0 the function asserts
selected_effective_value >= target.

If any input came from a group with m_subtract_fee_outputs==true, its
effective_value equals the real value, while target still embeds
fees. In that borderline case the inequality can be violated, firing
the assert in valid scenarios.

Consider replacing the assert with a runtime check (and early return of
MAX_MONEY) or using selected_real_value when !use_effective_value.

src/wallet/test/wallet_tests.cpp (3)

116-116: Constructor parameter update looks good

All CWallet constructors now properly include the m_args parameter, which is consistent with the wallet refactoring to explicitly pass ArgsManager instances rather than relying on global arguments.

Also applies to: 137-137, 166-166, 193-193, 232-232, 298-298, 320-320, 353-353, 528-528, 630-630, 640-640, 892-892, 989-989

---

607-607: Improved COutPoint usage

Replaced construction of new COutPoint objects with direct usage of existing coin.outpoint members. This is more efficient and aligns with the wallet refactoring that favors direct COutPoint usage.

Also applies to: 1297-1298, 1355-1355

---

1423-1423: Updated member variable reference

The member vecInputCoins in CompactTallyItem was properly renamed to outpoints to reflect the shift from coin objects to outpoints, matching changes in related wallet code.

Also applies to: 1466-1467

Reviewing files that changed from the base of the PR and between 0e33db2 and e901183.

• src/bench/coin_selection.cpp (3 hunks)
• src/bench/wallet_balance.cpp (1 hunks)
• src/coinjoin/client.cpp (3 hunks)
• src/coinjoin/util.cpp (1 hunks)
• src/interfaces/chain.h (0 hunks)
• src/interfaces/wallet.h (0 hunks)
• src/node/interfaces.cpp (0 hunks)
• src/qt/test/addressbooktests.cpp (1 hunks)
• src/qt/test/wallettests.cpp (1 hunks)
• src/qt/transactiondesc.cpp (0 hunks)
• src/qt/transactionrecord.cpp (1 hunks)
• src/qt/transactionrecord.h (0 hunks)
• src/qt/transactiontablemodel.cpp (0 hunks)
• src/rpc/evo.cpp (1 hunks)
• src/rpc/masternode.cpp (1 hunks)
• src/wallet/coinjoin.cpp (5 hunks)
• src/wallet/coinselection.cpp (13 hunks)
• src/wallet/coinselection.h (6 hunks)
• src/wallet/dump.cpp (1 hunks)
• src/wallet/interfaces.cpp (2 hunks)
• src/wallet/receive.cpp (0 hunks)
• src/wallet/rpcwallet.cpp (4 hunks)
• src/wallet/salvage.cpp (1 hunks)
• src/wallet/spend.cpp (20 hunks)
• src/wallet/spend.h (3 hunks)
• src/wallet/test/coinjoin_tests.cpp (2 hunks)
• src/wallet/test/coinselector_tests.cpp (20 hunks)
• src/wallet/test/ismine_tests.cpp (10 hunks)
• src/wallet/test/scriptpubkeyman_tests.cpp (1 hunks)
• src/wallet/test/spend_tests.cpp (1 hunks)
• src/wallet/test/util.cpp (1 hunks)
• src/wallet/test/util.h (2 hunks)
• src/wallet/test/wallet_test_fixture.cpp (1 hunks)
• src/wallet/test/wallet_tests.cpp (18 hunks)
• src/wallet/wallet.cpp (5 hunks)
• src/wallet/wallet.h (3 hunks)
• src/wallet/wallettool.cpp (5 hunks)
• test/functional/rpc_fundrawtransaction.py (7 hunks)
• test/functional/test_runner.py (1 hunks)
• test/functional/wallet_basic.py (2 hunks)
• test/functional/wallet_timelock.py (1 hunks)
• test/functional/wallet_txn_clone.py (4 hunks)
• test/functional/wallet_txn_doublespend.py (4 hunks)

• src/node/interfaces.cpp
• src/interfaces/wallet.h
• src/interfaces/chain.h
• src/qt/transactionrecord.h
• src/wallet/receive.cpp
• src/qt/transactiondesc.cpp
• src/qt/transactiontablemodel.cpp

• test/functional/test_runner.py

• src/rpc/masternode.cpp
• src/wallet/dump.cpp
• src/wallet/test/ismine_tests.cpp
• src/wallet/test/spend_tests.cpp
• src/wallet/test/scriptpubkeyman_tests.cpp
• src/bench/wallet_balance.cpp
• src/wallet/test/util.cpp
• src/wallet/salvage.cpp
• src/qt/transactionrecord.cpp
• src/qt/test/wallettests.cpp
• src/coinjoin/util.cpp
• src/wallet/test/wallet_test_fixture.cpp
• test/functional/wallet_basic.py
• src/wallet/wallettool.cpp
• src/qt/test/addressbooktests.cpp
• src/wallet/interfaces.cpp
• test/functional/wallet_txn_doublespend.py
• test/functional/wallet_timelock.py
• src/rpc/evo.cpp
• test/functional/wallet_txn_clone.py
• src/wallet/test/coinjoin_tests.cpp
• src/wallet/test/util.h
• src/bench/coin_selection.cpp
• src/wallet/wallet.h
• src/wallet/coinjoin.cpp
• src/wallet/rpcwallet.cpp
• src/wallet/spend.h
• src/coinjoin/client.cpp
• src/wallet/wallet.cpp
• src/wallet/test/wallet_tests.cpp

• GitHub Check: arm-linux-build / Build source
• GitHub Check: mac-build / Build source
• GitHub Check: linux64_tsan-build / Build source
• GitHub Check: linux64_multiprocess-build / Build source
• GitHub Check: linux64_fuzz-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: win64-build / Build source
• GitHub Check: linux64_nowallet-build / Build source

test/functional/rpc_fundrawtransaction.py (10)

54-70: LGTM: New lock_outputs_type helper function

This helper function improves code organization by centralizing the logic for locking specific output types, helping to restrict coin selection to certain UTXO types during fee tests.

🧰 Tools

🪛 Ruff (0.8.2)

61-61: Do not assert False (python -O removes these calls), raise AssertionError()

Replace assert False

(B011)

---

71-74: LGTM: New unlock_utxos helper function

This helper function centralizes the unlocking logic, making it consistent across the test cases while preserving watchonly UTXOs.

---

82-84: LGTM: Initialize watchonly tracking variables

Good initialization of tracking variables for watchonly outputs at the start of the test.

---

410-411: LGTM: Proper UTXO locking and unlocking in fee test

The test now appropriately uses the new helper functions to restrict coin selection to specific UTXO types, improving test isolation and preventing interference between test cases.

Also applies to: 431-432

---

435-436: LGTM: Proper UTXO locking and unlocking in p2sh fee test

Appropriately using the new helpers for coin selection restriction.

Also applies to: 458-459

---

463-464: LGTM: Proper UTXO locking and unlocking in 4of5 fee test

Appropriately using the helpers for coin selection control.

Also applies to: 502-503

---

570-574: LGTM: Improved input selection in locked wallet test

The changes improve how inputs and total value are calculated, providing a more robust approach by using all available inputs and calculating the total value dynamically.

---

949-955: LGTM: Improved unsafe input creation and testing

Better approach to creating and tracking unsafe inputs:

1. Creates two 5 DASH inputs in a loop
2. Finds the correct output index for each input
3. Stores inputs in an array for later verification

🧰 Tools

🪛 Ruff (0.8.2)

950-950: Loop control variable i not used within loop body

Rename unused i to _i

(B007)

---

957-958: LGTM: Better verification of unsafe inputs in transactions

The test now properly verifies that specific inputs are included in the transaction and uses testmempoolaccept to check validity without broadcasting, which is more efficient.

Also applies to: 963-966

---

969-974: LGTM: Improved post-confirmation test for previously unsafe inputs

The test now properly checks that previously unsafe inputs are included in the transaction after confirmation and verifies acceptance with testmempoolaccept.

src/wallet/test/coinselector_tests.cpp (16)

32-33: LGTM: Updated CoinSet typedef to use COutput

Properly updated the typedef to use COutput instead of CInputCoin, in line with the API modernization.

---

37-38: LGTM: Added nextLockTime to ensure unique transaction hashes

Good addition of a static variable to ensure each test transaction gets a unique hash, preventing collisions during testing.

---

39-46: LGTM: Updated add_coin function to use COutput

The function now properly constructs COutput objects instead of CInputCoin, consistent with the updated coin selection API.

---

48-58: LGTM: New add_coin overload for SelectionResult

This helper function makes it easy to add coins directly to a SelectionResult, improving test code organization.

---

60-71: LGTM: Updated add_coin for CoinSet

Properly updated to use COutput instead of CInputCoin with correct initialization of fee fields.

---

73-93: LGTM: Updated add_coin for wallet tests

Now correctly uses the new COutput constructor with additional metadata when adding coins to the wallet.

---

95-112: LGTM: New EquivalentResult function

Good addition of a helper function to check if SelectionResult objects contain inputs with equivalent values, which is useful for testing selection algorithms.

---

114-122: LGTM: New EqualResult function

Good addition of a helper function to check if SelectionResult objects contain exactly the same inputs, useful for testing determinism.

---

147-150: LGTM: Updated KnapsackSolver wrapper

Properly updated to return std::optional and take a FastRandomContext for randomness.

---

152-168: LGTM: Updated KnapsackGroupOutputs

Now correctly creates and passes a CoinSelectionParams object with the required FastRandomContext.

---

172-219: LGTM: Updated BnB tests to use optional return type

Tests now properly check for the presence of a result and verify selection properties through the SelectionResult interface.

---

309-330: LGTM: Updated effective value tests to use ArgsManager

The test now properly passes m_args to the CWallet constructor to match the updated wallet API.

---

332-351: LGTM: Updated coin control tests

The wallet creation and testing code now properly uses the ArgsManager parameter and the updated SelectCoins API.

---

353-663: LGTM: Comprehensive update of Knapsack tests

All the tests have been properly updated to use std::optional results and verify selection properties through the new interfaces.

---

665-688: LGTM: Updated ApproximateBestSubset test

The test now properly uses FastRandomContext and verifies the optional result.

---

687-738: LGTM: Updated SelectCoins test

The test now uses the ArgsManager and updated coin selection parameters with FastRandomContext.

src/wallet/coinselection.h (6)

21-86: LGTM: Rich COutput class replacing CInputCoin

Excellent refactoring to replace the limited CInputCoin with a more comprehensive COutput class that includes:

• Transaction metadata (depth, safety, etc.)
• Spendability and solvability flags
• Ownership information
• Fee calculation fields

This provides a more complete picture of each output for better coin selection decisions.

---

89-129: LGTM: Updated CoinSelectionParams with explicit randomness

Good improvement to pass FastRandomContext explicitly, ensuring deterministic behavior in tests while allowing genuine randomness in production.

---

216-255: LGTM: New SelectionResult class with comprehensive API

Excellent addition of a dedicated class to encapsulate selection results, providing methods to:

• Track and retrieve selected inputs
• Calculate and retrieve waste
• Get shuffled inputs for transaction creation
• Support comparison operations

This improves encapsulation and code organization.

---

226-227: Potential issue with m_use_effective flag

The m_use_effective flag could be inconsistent when groups with different m_subtract_fee_outputs flags are combined. In the corresponding implementation (SelectionResult::AddInput), this flag is blindly overwritten by each group added, so the last group's setting prevails.

Consider updating the logic to only allow disabling effective value mode, not re-enabling it:

// In cpp file:
-    m_use_effective = !group.m_subtract_fee_outputs;
+    if (group.m_subtract_fee_outputs) {
+        m_use_effective = false;
+    }

Can you verify whether the implementation of AddInput in the cpp file has already addressed this concern?

---

256-265: LGTM: New SelectCoinsSRD algorithm

Good addition of the Single Random Draw coin selection algorithm, which provides a simple alternative that might be useful in specific cases.

---

267-268: LGTM: Updated KnapsackSolver signature

Properly updated to return std::optional for consistent error handling across all selection algorithms.

src/wallet/coinselection.cpp (8)

63-170: LGTM: Updated SelectCoinsBnB implementation

The function has been properly refactored to:

1. Return std::optional instead of modifying output parameters
2. Use the new SelectionResult class to track selections
3. Return std::nullopt on failure cases
4. Directly return the result on success

This creates a more robust and easier to use API.

---

172-194: LGTM: New SelectCoinsSRD implementation

Clean implementation of the Single Random Draw algorithm that:

1. Shuffles indexes to avoid bias
2. Adds inputs until target is reached
3. Returns std::nullopt if insufficient funds
4. Uses Assume() for checks that should always succeed

This provides a simple alternative selection algorithm.

---

196-197: LGTM: Updated ApproximateBestSubset signature

Properly updated to take a FastRandomContext reference for randomness.

---

267-377: LGTM: Updated KnapsackSolver implementation

The function has been properly refactored to:

1. Return std::optional instead of modifying output parameters
2. Use the SelectionResult class to track selections
3. Return early with the result when a solution is found
4. Return std::nullopt on failure cases

This creates a more robust API.

---

385-415: LGTM: Updated OutputGroup::Insert implementation

The function now properly works with COutput objects and correctly:

1. Calculates effective values
2. Updates group properties
3. Handles fee calculations

This aligns with the updated data structure.

---

429-454: LGTM: Updated GetSelectionWaste function

The function now operates on std::set instead of CInputCoin and correctly calculates waste based on effective values and fees.

---

456-501: LGTM: SelectionResult method implementations

Good implementation of all SelectionResult methods with proper:

1. Waste calculation
2. Input management
3. Value aggregation
4. Comparison operations
5. Input shuffling

This completes the API needed for comprehensive coin selection.

---

503-506: LGTM: COutput::ToString implementation

Good addition of a debug formatting method for COutput objects.