backport: merge bitcoin#23334, #21206, #24196, #24530, #24635, #24502, #24494, #24677, #24602, #24644, #24820, #25507, #25825, #29042, partial bitcoin#23725 (wallet backports: part 5)#6685
Conversation
WalkthroughThis update introduces a comprehensive refactor of wallet transaction state management, shifting from legacy status enums and confirmation structs to a modern, type-safe variant-based system. The new system uses explicit state structs (such as ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
🧹 Nitpick comments (8)
src/validation.cpp (1)
1946-1948: Prefersize_t(or a reverse iterator) instead ofunsigned intThe new loop is clearer than the previous post-decrement trick ✅.
However,tx.vin.size()returnssize_t; storing it inunsigned intrisks truncation if a (malformed) transaction ever contains > 4 Gi inputs.-for (unsigned int j = tx.vin.size(); j > 0;) { +for (size_t j = tx.vin.size(); j > 0;) { --j;Even cleaner (and bounds-safe):
for (auto it = tx.vin.rbegin(); it != tx.vin.rend(); ++it) { const COutPoint& out = it->prevout; ... }Not critical, but worth tightening while touching this code.
doc/release-notes-6685.md (1)
4-5: Bullet formatting consistency
The release note bullet is split across two lines, which may hinder readability and markdown rendering. Consider merging into a single line or wrapping at natural sentence boundaries. For example:- To help prevent fingerprinting transactions created by the Dash Core wallet, change output amounts are now randomized.src/wallet/transaction.cpp (1)
18-18: Consider explicit boolean conversion for clarity.The
state<TxStateInMempool>()method returns a pointer, which will implicitly convert to bool (nullptr → false, non-null → true). While this works correctly, consider making the intent explicit for better readability:- return state<TxStateInMempool>(); + return state<TxStateInMempool>() != nullptr;This makes it clear that we're checking for the presence of the mempool state rather than relying on implicit pointer-to-bool conversion.
src/wallet/test/wallet_transaction_tests.cpp (1)
1-25: Excellent test coverage for transaction state serialization.The roundtrip test properly validates the
TxStateserialization/deserialization logic introduced in the wallet refactoring. The test structure is well-designed using appropriate fixtures and covers various combinations of hash and index values.Consider expanding test coverage to include edge cases:
BOOST_AUTO_TEST_CASE(roundtrip) { - for (uint8_t hash = 0; hash < 5; ++hash) { - for (int index = -2; index < 3; ++index) { + // Test basic range + for (uint8_t hash = 0; hash < 5; ++hash) { + for (int index = -2; index < 3; ++index) { TxState state = TxStateInterpretSerialized(TxStateUnrecognized{uint256{hash}, index}); BOOST_CHECK_EQUAL(TxStateSerializedBlockHash(state), uint256{hash}); BOOST_CHECK_EQUAL(TxStateSerializedIndex(state), index); } } + + // Test edge cases + std::vector<std::pair<uint256, int>> edge_cases = { + {uint256::ZERO, 0}, {uint256::ONE, -1}, + {uint256::ZERO, INT_MAX}, {uint256::ONE, INT_MIN} + }; + for (const auto& [hash, index] : edge_cases) { + TxState state = TxStateInterpretSerialized(TxStateUnrecognized{hash, index}); + BOOST_CHECK_EQUAL(TxStateSerializedBlockHash(state), hash); + BOOST_CHECK_EQUAL(TxStateSerializedIndex(state), index); + } }doc/tracing.md (1)
171-213: Comprehensive tracepoint documentation with minor grammatical improvements needed.The new
coin_selectioncontext documentation is well-structured and provides valuable observability into wallet coin selection processes. The documentation follows the established pattern and clearly explains each tracepoint's purpose and arguments.Consider these minor grammatical improvements for clarity:
-#### Tracepoint `coin_selection:selected_coins` - -Is called when `SelectCoins` completes. +#### Tracepoint `coin_selection:selected_coins` + +This is called when `SelectCoins` completes.-#### Tracepoint `coin_selection:normal_create_tx_internal` - -Is called when the first `CreateTransactionInternal` completes. +#### Tracepoint `coin_selection:normal_create_tx_internal` + +This is called when the first `CreateTransactionInternal` completes.-3. Whether `CreateTransactionInternal` succeeded as` bool` +3. Whether `CreateTransactionInternal` succeeded as `bool`-4. The expected transaction fee as an `int64` +4. The expected transaction fee as `int64`🧰 Tools
🪛 LanguageTool
[uncategorized] ~174-~174: Possible missing preposition found.
Context: ...epointcoin_selection:selected_coinsIs called whenSelectCoinscompletes. A...(AI_HYDRA_LEO_MISSING_IT)
[uncategorized] ~185-~185: Possible missing preposition found.
Context: ...n_selection:normal_create_tx_internalIs called when the firstCreateTransactio...(AI_HYDRA_LEO_MISSING_IT)
[misspelling] ~191-~191: Did you mean “and” or “any”?
Context: ...ool3. The expected transaction fee as anint64` 4. The position of the change o...(AN_AND)
[grammar] ~195-~195: This sentence seems to be incomplete. Insert a noun before ‘Is’ to make the sentence complete.
Context: ...in_selection:attempting_aps_create_txIs called whenCreateTransactionInternal`...(MISSING_SUBJECT)
[uncategorized] ~204-~204: Possible missing preposition found.
Context: ...coin_selection:aps_create_tx_internalIs called when the secondCreateTransacti...(AI_HYDRA_LEO_MISSING_IT)
[misspelling] ~211-~211: Did you mean “and” or “any”?
Context: ...ool4. The expected transaction fee as anint64` 5. The position of the change o...(AN_AND)
🪛 markdownlint-cli2 (0.17.2)
210-210: Spaces inside code span elements
null(MD038, no-space-in-code)
src/wallet/test/fuzz/coinselection.cpp (1)
1-101: Excellent comprehensive fuzz test for coin selection algorithms.This is a well-designed fuzz test that effectively exercises the coin selection logic with diverse scenarios:
- Comprehensive coverage: Tests all three main algorithms (BnB, SRD, Knapsack)
- Proper randomization: Uses seeded FastRandomContext for reproducible results
- Good assertions: Line 96-98 provides a meaningful correctness check
- Safe iteration: Uses
LIMITED_WHILEto prevent infinite loops- Realistic parameters: Generates varied coin values, input sizes, and fee rates
The test structure follows established patterns and integrates well with the enhanced coin selection algorithms introduced in this PR.
Consider extracting the coin generation logic into a separate helper function for better readability:
static void GenerateRandomCoins(FuzzedDataProvider& fuzzed_data_provider, std::vector<COutput>& utxo_pool, CAmount& total_balance) { int next_locktime{0}; LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) { const int n_input{fuzzed_data_provider.ConsumeIntegralInRange<int>(0, 10)}; const int n_input_bytes{fuzzed_data_provider.ConsumeIntegralInRange<int>(100, 10000)}; const CAmount amount{fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(1, MAX_MONEY)}; if (total_balance + amount >= MAX_MONEY) break; AddCoin(amount, n_input, n_input_bytes, ++next_locktime, utxo_pool); total_balance += amount; } }test/functional/interface_usdt_coinselection.py (1)
11-14: Consider using contextlib.suppress for cleaner code.While the current implementation works, using
contextlib.suppresswould be more Pythonic.-try: - from bcc import BPF, USDT # type: ignore[import] -except ImportError: - pass +from contextlib import suppress +with suppress(ImportError): + from bcc import BPF, USDT # type: ignore[import]🧰 Tools
🪛 Ruff (0.11.9)
11-14: Use
contextlib.suppress(ImportError)instead oftry-except-pass(SIM105)
src/wallet/wallet.cpp (1)
1817-1836: Important API change: Parameter is now non-const.The removal of
constfrom thewtxparameter is necessary to update the transaction state. This is a breaking change that may require updates to callers of this function.The implementation correctly sets
TxStateInMempoolto prevent race conditions as explained in the comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between da8a475 and dc09ed354f5d274b6dc0575995c65a8886f4d61d.
📒 Files selected for processing (34)
configure.ac(0 hunks)doc/release-notes-6685.md(1 hunks)doc/tracing.md(1 hunks)src/Makefile.am(1 hunks)src/Makefile.test.include(4 hunks)src/bench/coin_selection.cpp(2 hunks)src/qt/coincontroldialog.cpp(1 hunks)src/test/fuzz/fuzz.cpp(1 hunks)src/util/overloaded.h(1 hunks)src/util/types.h(1 hunks)src/validation.cpp(1 hunks)src/wallet/coinselection.cpp(10 hunks)src/wallet/coinselection.h(5 hunks)src/wallet/interfaces.cpp(1 hunks)src/wallet/rpcdump.cpp(1 hunks)src/wallet/rpcwallet.cpp(2 hunks)src/wallet/spend.cpp(11 hunks)src/wallet/test/coinjoin_tests.cpp(1 hunks)src/wallet/test/coinselector_tests.cpp(15 hunks)src/wallet/test/fuzz/coinselection.cpp(1 hunks)src/wallet/test/fuzz/notifications.cpp(1 hunks)src/wallet/test/psbt_wallet_tests.cpp(1 hunks)src/wallet/test/wallet_tests.cpp(5 hunks)src/wallet/test/wallet_transaction_tests.cpp(1 hunks)src/wallet/transaction.cpp(1 hunks)src/wallet/transaction.h(6 hunks)src/wallet/wallet.cpp(30 hunks)src/wallet/wallet.h(5 hunks)src/wallet/walletdb.cpp(1 hunks)test/functional/interface_usdt_coinselection.py(1 hunks)test/functional/test_runner.py(2 hunks)test/functional/wallet_balance.py(1 hunks)test/functional/wallet_basic.py(3 hunks)test/sanitizer_suppressions/ubsan(0 hunks)
💤 Files with no reviewable changes (2)
- test/sanitizer_suppressions/ubsan
- configure.ac
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
🧬 Code Graph Analysis (5)
src/wallet/transaction.cpp (1)
src/wallet/transaction.h (7)
state(296-296)state(296-296)state(297-297)state(297-297)state(299-299)state(300-300)state(302-302)
src/wallet/test/wallet_transaction_tests.cpp (1)
src/wallet/transaction.h (13)
state(296-296)state(296-296)state(297-297)state(297-297)state(299-299)state(300-300)state(302-302)TxStateInterpretSerialized(71-83)TxStateInterpretSerialized(71-71)TxStateSerializedBlockHash(86-95)TxStateSerializedBlockHash(86-86)TxStateSerializedIndex(98-107)TxStateSerializedIndex(98-98)
test/functional/wallet_basic.py (2)
test/functional/feature_dbcrash.py (1)
restart_node(78-105)test/functional/test_framework/test_framework.py (1)
start_node(642-651)
src/wallet/test/wallet_tests.cpp (2)
src/wallet/wallet.h (28)
wtx(292-292)wtx(503-503)wtx(511-511)wtx(512-512)wtx(518-518)wtx(519-519)wtx(526-526)wtx(527-527)wtx(598-598)wtx(695-695)wtx(697-697)block(620-620)block(621-621)wallet(402-402)tx(300-300)tx(319-319)tx(325-325)tx(329-329)tx(617-617)tx(619-619)tx(641-641)tx(650-650)tx(652-652)tx(692-692)tx(790-790)tx(792-792)tx(793-793)tx(915-915)src/wallet/transaction.h (7)
state(296-296)state(296-296)state(297-297)state(297-297)state(299-299)state(300-300)state(302-302)
src/wallet/wallet.cpp (4)
src/wallet/wallet.h (46)
tx(300-300)tx(319-319)tx(325-325)tx(329-329)tx(617-617)tx(619-619)tx(641-641)tx(650-650)tx(652-652)tx(692-692)tx(790-790)tx(792-792)tx(793-793)tx(915-915)hash(501-501)hash(551-551)hash(554-554)hash(557-557)hash(559-559)hash(618-618)hash(658-658)wtx(292-292)wtx(503-503)wtx(511-511)wtx(512-512)wtx(518-518)wtx(519-519)wtx(526-526)wtx(527-527)wtx(598-598)wtx(695-695)wtx(697-697)batch(334-334)batch(337-337)batch(340-340)batch(555-555)batch(604-604)batch(805-805)batch(808-808)WalletLogPrintf(960-962)WalletLogPrintf(960-960)hashBlock(322-322)block(620-620)block(621-621)block_height(1016-1016)txout(789-789)src/wallet/transaction.h (15)
tx(290-290)tx(303-303)TxStateInactive(50-50)TxStateInactive(50-50)state(296-296)state(296-296)state(297-297)state(297-297)state(299-299)state(300-300)state(302-302)TxStateSerializedIndex(98-107)TxStateSerializedIndex(98-98)TxStateSerializedBlockHash(86-95)TxStateSerializedBlockHash(86-86)src/interfaces/chain.h (14)
height(56-56)height(56-56)height(114-114)height(118-118)height(140-140)block(260-260)block(260-260)block(261-261)block(261-261)block_hash(156-156)block_hash(160-162)block_hash(179-179)block_hash(184-184)coins(175-175)src/wallet/scriptpubkeyman.cpp (2)
GetAffectedKeys(1640-1650)GetAffectedKeys(1640-1640)
🪛 LanguageTool
doc/tracing.md
[uncategorized] ~174-~174: Possible missing preposition found.
Context: ...epoint coin_selection:selected_coins Is called when SelectCoins completes. A...
(AI_HYDRA_LEO_MISSING_IT)
[uncategorized] ~185-~185: Possible missing preposition found.
Context: ...n_selection:normal_create_tx_internal Is called when the firstCreateTransactio...
(AI_HYDRA_LEO_MISSING_IT)
[misspelling] ~191-~191: Did you mean “and” or “any”?
Context: ...ool3. The expected transaction fee as anint64` 4. The position of the change o...
(AN_AND)
[grammar] ~195-~195: This sentence seems to be incomplete. Insert a noun before ‘Is’ to make the sentence complete.
Context: ...in_selection:attempting_aps_create_tx Is called whenCreateTransactionInternal`...
(MISSING_SUBJECT)
[uncategorized] ~204-~204: Possible missing preposition found.
Context: ...coin_selection:aps_create_tx_internal Is called when the secondCreateTransacti...
(AI_HYDRA_LEO_MISSING_IT)
[misspelling] ~211-~211: Did you mean “and” or “any”?
Context: ...ool4. The expected transaction fee as anint64` 5. The position of the change o...
(AN_AND)
🪛 markdownlint-cli2 (0.17.2)
doc/tracing.md
210-210: Spaces inside code span elements
null
(MD038, no-space-in-code)
🪛 Ruff (0.11.9)
test/functional/interface_usdt_coinselection.py
11-14: Use contextlib.suppress(ImportError) instead of try-except-pass
(SIM105)
122-122: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
🔇 Additional comments (75)
src/util/overloaded.h (1)
5-20: Utility headerOverloadedimplementation looks correct
The include guard, template inheritance (struct Overloaded : Ts...), and explicit deduction guide are all defined properly and consistent with the backported Bitcoin Core pattern. No missing includes or namespace issues detected.src/util/types.h (1)
8-8: Clarifying comment onALWAYS_FALSE
The added comment referencing the C++23 CWG issue (DR 2518) clearly documents why this workaround exists and when it can be removed.src/test/fuzz/fuzz.cpp (1)
72-72: Updated compiler compatibility note
The inlined comment correctly reflects that the temporary wrapper can be dropped after Clang 16, aligning with the minimum supported compiler versions.src/Makefile.am (1)
367-367: Include new header inBITCOIN_CORE_H
Addingutil/overloaded.hto the core headers list ensures the backported utility is installed and available across all build targets that rely on variant visitation helpers.src/wallet/test/psbt_wallet_tests.cpp (1)
35-35: LGTM! Test correctly updated for new TxState system.The changes properly update the test to use the new
TxStatevariant system by explicitly setting transactions toTxStateInactive{}when inserting intomapWallet. This aligns with the broader refactoring away from the legacy confirmation system.Also applies to: 40-40
test/functional/test_runner.py (2)
198-198: LGTM! Addition of USDT coinselection test is appropriate.The new
interface_usdt_coinselection.pytest aligns with the wallet backports mentioned in the PR objectives, particularly the addition of USDT tracepoints for coin selection observability.
227-227: LGTM! Test configuration consolidation looks good.The consolidation of
wallet_disable.pyentries into a single entry without flags simplifies the test configuration while maintaining test coverage.src/wallet/interfaces.cpp (1)
104-107: LGTM! Block height logic correctly updated for TxState system.The new logic properly handles different transaction states using the variant system:
- Uses confirmed block height for confirmed transactions
- Uses conflicting block height for conflicted transactions
- Falls back to max int for other states
This maintains the same semantic behavior as the legacy confirmation system while leveraging the new type-safe TxState approach.
src/wallet/rpcdump.cpp (1)
349-349: LGTM! Correct modernization of transaction state handling.This change correctly replaces the legacy
CWalletTx::Confirmationobject construction with the newTxStateConfirmedvariant, maintaining the same parameters (block hash, height, transaction index). This aligns with the broader wallet refactoring to modernize transaction state management using theTxStatevariant system.src/wallet/walletdb.cpp (1)
971-971: LGTM: Correct migration to TxState variant system.The change correctly updates
CWalletTxconstruction to use the newTxStatevariant system, replacing the legacyConfirmationstruct. UsingTxStateInactive{}as the default state for database-loaded transactions is appropriate, as the actual transaction state will be determined during the loading process.src/wallet/test/coinjoin_tests.cpp (1)
177-177: LGTM: Test code correctly migrated to TxState system.The change properly updates the test helper to use the new
TxStateConfirmedinstead of the legacyConfirmationstruct. The parameters (block hash, height, and index) correctly represent the confirmed transaction state being simulated in the test.test/functional/wallet_basic.py (3)
31-31: LGTM: Appropriate test configuration for new default behavior.The addition of
-walletrejectlongchains=0to both descriptor and non-descriptor test configurations correctly disables the new default wallet behavior of rejecting long mempool chains. This ensures test stability and consistent behavior across different test scenarios.Also applies to: 35-35
155-155: LGTM: Consistent node restart configuration.Adding
-walletrejectlongchains=0when restarting node 2 maintains consistency with the test setup configuration, ensuring the node behavior remains predictable during persistent output lock tests.
611-611: LGTM: Maintenance test configuration updated appropriately.Adding
-walletrejectlongchains=0to the maintenance tests (alongside-limitancestorcount) ensures consistent wallet behavior during-rescanand-reindexoperations, maintaining test reliability with the new default settings.test/functional/wallet_balance.py (1)
53-57: LGTM! Test configuration properly updated for new wallet behavior.The addition of
-walletrejectlongchains=0correctly maintains test compatibility with the new default behavior whereDEFAULT_WALLET_REJECT_LONG_CHAINSwas changed fromfalsetotrue. The comment clearly explains the purpose and the change ensures the wallet can still create transactions despite mempool limits during testing.src/qt/coincontroldialog.cpp (1)
542-553: Excellent refactoring of dust detection logic.The simplified approach improves code maintainability by:
- Removing MIN_CHANGE dependency: No longer relies on a fixed constant for dust detection
- Centralizing logic: Uses the
IsDust()function directly for consistent dust threshold evaluation- Improved readability: The linear flow is clearer than the previous nested conditions
The refactored logic correctly handles dust outputs by adding them to the fee instead of creating dust change, which aligns with the wallet's enhanced coin selection and change output randomization features introduced in this PR.
src/wallet/rpcwallet.cpp (2)
83-90: LGTM: Correctly implements new transaction state interfaceThe change from accessing
wtx.m_confirmto usingwtx.state<TxStateConfirmed>()properly implements the newTxStatevariant system. The functionality remains identical - extracting block hash, height, index, and block time for confirmed transactions. This is a clean migration from the legacy confirmation struct to the modern state-based design.
749-749: LGTM: Correctly migrated to new confirmation check methodThe change from checking the old confirmation status enum to calling
wtx.isConfirmed()properly implements the new transaction state interface. The logic remains the same - skipping confirmed transactions whenkeep_confirmedis true in thewipewallettxescommand. This aligns with the broader refactor to theTxStatevariant system.src/Makefile.test.include (3)
204-204: LGTM: Proper integration of new wallet transaction tests.The addition of
wallet_transaction_tests.cppto the test suite is correctly placed within theENABLE_WALLETconditional block.
219-225: Well-structured conditional fuzz test integration.The
FUZZ_WALLET_SRCvariable correctly conditionally includesnotifications.cppbased onUSE_SQLITE, which aligns with the dependency requirements mentioned in the AI summary.
258-260: Clean refactor: Inlining FUZZ_SUITE_LDFLAGS_COMMON.The removal of the intermediate variable and direct assignment to
test_fuzz_fuzz_LDFLAGSis a good cleanup that reduces indirection without affecting functionality.src/bench/coin_selection.cpp (2)
21-21: Correct integration with new transaction state system.The addition of
TxStateInactive{}parameter aligns with the refactor from the legacyConfirmationsystem to the newTxStatevariant-based representation mentioned in the PR objectives.
54-62: Proper update of CoinSelectionParams with new parameters.The addition of
min_change_targetparameter and the structured initialization format improves code readability and aligns with the enhanced coin selection algorithms introduced in this PR.src/wallet/test/wallet_tests.cpp (4)
354-354: LGTM: Proper usage of new TxState system.The transition from the deprecated
CWalletTx::Confirmationstruct toTxStateConfirmedis correctly implemented. The constructor properly provides the block hash, height, and transaction index parameters.
376-396: LGTM: Well-implemented test helper with proper documentation.The
AddTxfunction correctly implements the newTxStatesystem:
- Appropriately starts with
TxStateInactivefor unconfirmed transactions- Transitions to
TxStateConfirmedwhen a block is provided- The explicit assignment of
wtx.m_statein the callback is well-documented as a test simplification to avoid simulating reorg eventsThe comment clearly explains the rationale for the explicit state assignment.
555-555: LGTM: Consistent implementation of TxState system.The direct assignment of
TxStateConfirmedto the wallet transaction state is consistent with the broader test infrastructure changes. The parameters (block hash, height, and index) are correctly provided.
1103-1103: LGTM: Consistent with TxState refactoring pattern.Another correct implementation of the
TxStateConfirmedassignment, maintaining consistency with the wallet transaction state management refactoring throughout the test suite.src/wallet/test/fuzz/notifications.cpp (4)
34-80: LGTM: Well-designed RAII wrapper for fuzz testing.The
FuzzedWalletstruct properly implements the RAII pattern:
- Constructor correctly creates descriptor wallets with appropriate flags
- Thorough error checking with assertions ensures wallet creation succeeds
- Destructor properly cleans up wallet and database files
GetScriptPubKeymethod provides good coverage by testing both new destinations and change destinationsThe resource management is solid and appropriate for fuzz testing.
82-103: LGTM: Well-structured fuzz test initialization.The fuzz target setup is properly designed:
- Correctly initializes with the setup function
- Two-wallet approach provides good coverage for coin transfer scenarios
- Appropriate data structures (
Coinstype,chainvector) for tracking blockchain state- Initial coin state is properly established with deterministic starting conditions
The setup provides a solid foundation for comprehensive wallet notification testing.
108-150: LGTM: Excellent transaction generation logic with proper invariants.The transaction creation logic is well-designed:
- Smart no-fee approach: Ensures total balance remains constant by spending exact input amounts
- Comprehensive randomization: Uses fuzz data to vary input/output counts and wallet selection
- Proper resource management: Consumes all coins from the previous block to maintain clean state
- Correct notifications: Both wallets are properly notified with
blockConnectedand correct block height- State tracking: Coin tracking is correctly updated for the next iteration
The design maintains important test invariants while providing good coverage.
151-168: LGTM: Robust block disconnection logic with excellent invariant verification.The block disconnection and balance verification logic is well-implemented:
- Proper preconditions: Correctly prevents disconnecting the initial entry or empty blocks
- Accurate notifications: Wallets are properly notified with
blockDisconnectedand correct height parameters- Strong invariant checking: Balance verification ensures the core property that wallet balances always sum to the total amount
- Appropriate balance type: Uses
m_mine_trustedwhich is correct for confirmed transactions- Conditional verification: Only checks balances when meaningful (after at least one block is submitted)
The invariant verification provides excellent coverage for detecting wallet notification bugs.
src/wallet/test/coinselector_tests.cpp (5)
89-89: LGTM!The update to construct
CWalletTxwithTxStateInactive{}correctly aligns with the wallet's transition to using theTxStatevariant for transaction state representation.
147-150: LGTM!The wrapper function correctly forwards the new
change_targetparameter to the underlyingKnapsackSolverimplementation.
156-165: LGTM!The
CoinSelectionParamsinitialization correctly includes all new fields, withmin_change_targetappropriately set toCENT.
382-384: Acknowledged: Intentionally commented test due to Dash's BnB disablement.As noted in the PR objectives, this check is commented out because Dash disables BnB coin selection for mixing awareness. The comment clearly explains this design decision.
401-726: LGTM!All
KnapsackSolvercalls are consistently updated to includeCENTas thechange_targetparameter, maintaining test uniformity across the suite.src/wallet/coinselection.cpp (5)
168-169: Critical: Waste computation prevents crashes with tracepoints.As noted in the PR objectives, computing waste by calling
ComputeAndSetWaste()is essential because the tracepoints introduced in bitcoin#24644 will cause the client to crash if waste is not calculated whenGetWaste()is called. The assertion ensures this invariant is maintained.
198-209: LGTM!The enhanced documentation clearly explains the function's parameters and behavior, improving code maintainability.
284-296: LGTM!The
KnapsackSolvercorrectly integrates thechange_targetparameter throughout its logic, with appropriate handling for thefFullyMixedOnlycase where no change is allowed.Also applies to: 308-308, 327-327, 370-377
475-485: LGTM!The
GenerateChangeTargetfunction implements a sensible strategy for randomizing change amounts, which can enhance transaction privacy. The handling of small payments (≤ 25k sats) by returning a fixed lower bound is appropriate.
538-550: LGTM!The
GetAlgorithmNamefunction is well-implemented with proper enum-to-string conversion. The absence of a default case enables compiler warnings for unhandled enum values, and the assertion provides runtime safety.test/functional/interface_usdt_coinselection.py (1)
99-208: LGTM! Comprehensive test coverage for coin selection tracepoints.The test effectively validates all four new tracepoints (
selected_coins,normal_create_tx_internal,attempting_aps_create_tx,aps_create_tx_internal) with appropriate scenarios including successful transactions, failures, and APS usage. The BPF program correctly captures and decodes the tracepoint data.🧰 Tools
🪛 Ruff (0.11.9)
122-122: Do not
assert False(python -Oremoves these calls), raiseAssertionError()Replace
assert False(B011)
src/wallet/coinselection.h (5)
15-18: LGTM! Good choice of change target bounds.The range of 50,000 to 1,000,000 satoshis provides a reasonable balance between privacy (through output value randomization) and efficiency (avoiding dust outputs).
96-124: LGTM! Well-integrated change target parameter.The
m_min_change_targetmember is properly documented and consistently integrated into the constructor. The parameter ordering in the constructor maintains logical grouping of related parameters.
224-236: Excellent documentation for the change target generation strategy.The documentation clearly explains the privacy benefits and the rationale behind using double the payment value to break analysis heuristics. This is a well-thought-out approach to improving transaction privacy.
238-246: LGTM! Good addition for algorithm tracking.The
SelectionAlgorithmenum andGetAlgorithmNamehelper will improve observability of coin selection behavior, especially useful with the new USDT tracepoints mentioned in the PR summary.
259-301: LGTM! Consistent updates to track algorithm and change target.The
SelectionResultproperly captures which algorithm was used and the target amount, while theKnapsackSolversignature update aligns with the new change target mechanism. These changes support the enhanced tracing capabilities mentioned in the PR objectives.src/wallet/wallet.h (3)
97-97: Verify the impact of changing the default for wallet long chain rejection.Changing
DEFAULT_WALLET_REJECT_LONG_CHAINSfromfalsetotrueis a significant behavioral change that will affect how wallets handle transactions with long unconfirmed ancestor chains. This aligns with Bitcoin Core's approach but may impact users who rely on the previous behavior.Please ensure this change is documented in release notes and that any necessary migration guidance is provided for users who may need to adjust their configurations.
319-319: LGTM! Consistent migration to the new transaction state system.The method signatures have been properly updated to use the new
TxStateandSyncTxStatetypes, replacing the legacyCWalletTx::Confirmationsystem. This improves type safety and extensibility.Also applies to: 329-329, 617-617
697-697: Note: API change from const to non-const reference.The change from
const CWalletTx&toCWalletTx&inSubmitTxMemoryPoolAndRelayindicates that the method now modifies the transaction object, likely updating its state. This is a breaking API change that may affect code calling this method.Please verify that all callers of this method have been updated to handle the non-const requirement and that the state modifications are properly documented.
src/wallet/transaction.h (4)
22-69: Excellent design for the new transaction state system.The variant-based approach with distinct state structs is a significant improvement over the legacy enum/struct system:
- Type safety through
std::variant- Clear separation of concerns with state-specific data
- Forward compatibility via
TxStateUnrecognized- Clean subset definition with
SyncTxStateThis is a well-architected solution that improves maintainability and extensibility.
71-108: Well-designed serialization with backward compatibility.The serialization helper functions cleverly maintain backward compatibility while supporting the new state system:
- Sentinel values (ZERO/ONE) for inactive states
- Index-based disambiguation between states
- Graceful handling of unrecognized states
- Clean implementation using the visitor pattern
This ensures wallet files remain compatible across versions.
202-258: LGTM! Clean integration of TxState into CWalletTx.The constructor now properly requires an explicit
TxState, ensuring transactions always have a well-defined state. The serialization/deserialization logic correctly uses the helper functions to maintain backward compatibility while supporting the new state system.
296-302: LGTM! Type-safe state access methods.The template-based state accessors and boolean query methods provide a clean, type-safe interface for checking transaction states. The implementation correctly handles the nuanced case of abandoned transactions being a sub-state of inactive.
src/wallet/spend.cpp (7)
14-14: LGTM! Necessary includes for new functionality.The includes are properly added to support the new tracing capabilities and mathematical operations used in the change target calculation.
Also applies to: 23-23
391-393: Correct implementation of the new change target parameter.The KnapsackSolver call properly includes the new
m_min_change_targetparameter, which is part of the randomized change target mechanism introduced in this PR.
398-402: Good optimization for SRD target calculation.The change to use
CHANGE_LOWERinstead of the randomized change target for SRD is sensible, as the comment clearly explains. Since SRD inherently produces random change amounts, there's no need to add randomness to the target value.
432-432: Critical fix: Proper waste calculation for manual selections.These changes ensure that waste is calculated for all manual selection paths, which is essential to prevent crashes when tracepoints call
GetWaste(). This directly addresses the PR objective stating that "every non-std::nulloptreturn path calculates waste."Also applies to: 442-442, 447-447, 520-520, 574-576
688-688: Well-implemented randomized change target generation.The use of
GenerateChangeTargetwith the average recipient amount provides a reasonable basis for the minimum change target, enhancing privacy by adding variability to change outputs.
798-798: Comprehensive tracing for coin selection results.The TRACE5 call provides detailed logging of coin selection outcomes, including the algorithm used, target value, waste, and selected value. This aligns with the new USDT tracepoints documented in the PR.
1007-1007: Effective tracing for transaction creation flow.The trace calls provide excellent visibility into both the normal and APS (avoid partial spends) transaction creation paths, which will be valuable for debugging and performance analysis.
Also applies to: 1010-1010, 1028-1028
src/wallet/wallet.cpp (14)
108-115: LGTM! Correct implementation of the new transaction state system.The function properly updates transaction state based on mempool status, transitioning between
TxStateInMempoolandTxStateInactiveas appropriate.
886-932: LGTM! Proper migration to TxState in AddToWallet.The function correctly uses the new
TxStatevariant system with appropriate state comparisons using the serialization helper functions.
957-964: LGTM! Correct state checking for wallet notifications.The code properly extracts confirmation details from
TxStateConfirmedand provides appropriate fallback values for unconfirmed transactions.
986-1026: LGTM! Robust state management in LoadToWallet.The function correctly handles transaction state during wallet loading, including proper reorg detection and conflict propagation using the new state system.
1043-1093: LGTM! Proper use of SyncTxState with correct state conversions.The function correctly handles transaction synchronization with appropriate conflict detection and state type conversion.
1152-1152: LGTM! Correct abandoned transaction state.Properly sets
TxStateInactivewith the abandoned flag.
1219-1219: LGTM! Proper conflicted state with block details.Correctly creates
TxStateConflictedwith the conflicting block hash and height.
1254-1337: LGTM! Correct state management in blockchain callbacks.All mempool and block event handlers properly update transaction states:
- Mempool addition →
TxStateInMempool- Mempool removal →
TxStateInactive- Block connection →
TxStateConfirmedwith position- Block disconnection →
TxStateInactive
1942-1943: LGTM! Correct height extraction from confirmed state.Properly retrieves the confirmed block height when available, defaulting to 0 for unconfirmed transactions.
2048-2071: LGTM! Proper initial state for new transactions.Correctly initializes new wallet transactions with
TxStateInactivestate. The updated comment accurately describes the mempool state caching behavior.
2496-2547: LGTM! Correct migration to TxStateConfirmed in key birth time calculation.The function properly uses the new state system to track the earliest confirmed transactions for each key.
2576-2622: LGTM! Enhanced to handle both confirmed and conflicted states.The function now correctly extracts block information from both
TxStateConfirmedandTxStateConflicted, which is more comprehensive than the previous implementation.
3440-3447: LGTM! Clean state-based depth calculation.The function elegantly handles all transaction states with appropriate depth values:
- Confirmed: positive depth
- Conflicted: negative depth
- Other states: 0
3461-3472: LGTM! Correct chain lock verification.Properly extracts confirmation details from
TxStateConfirmedto verify chain locks.
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace assert False with explicit exception.
Using assert False can be problematic as assertions are removed when Python is run with the -O flag. Consider raising an explicit exception instead.
- else:
- # If the loop exits successfully instead of throwing a KeyError, then we have had
- # more events than expected. There should be no more than len(expected_types) events.
- assert False
+ else:
+ # If the loop exits successfully instead of throwing a KeyError, then we have had
+ # more events than expected. There should be no more than len(expected_types) events.
+ raise AssertionError(f"Too many events: expected {len(expected_types)}, but got more")📝 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.
| else: | |
| # If the loop exits successfully instead of throwing a KeyError, then we have had | |
| # more events than expected. There should be no more than len(expected_types) events. | |
| assert False | |
| else: | |
| # If the loop exits successfully instead of throwing a KeyError, then we have had | |
| # more events than expected. There should be no more than len(expected_types) events. | |
| raise AssertionError(f"Too many events: expected {len(expected_types)}, but got more") |
🧰 Tools
🪛 Ruff (0.11.9)
122-122: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
🤖 Prompt for AI Agents
In test/functional/interface_usdt_coinselection.py around lines 119 to 122,
replace the use of 'assert False' with raising an explicit exception such as
RuntimeError or ValueError with a descriptive message. This ensures the error is
always raised regardless of Python optimization flags.
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
Seems to be working correctly.
Light ACK dc09ed354f5d274b6dc0575995c65a8886f4d61d
|
This pull request has conflicts, please rebase. |
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
utACK dc09ed354f5d274b6dc0575995c65a8886f4d61d; want clarification on my one comment
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/functional/interface_usdt_coinselection.py (1)
111-126: Replace assert False with explicit exception.Using
assert Falsecan be problematic as assertions are removed when Python is run with the-Oflag.- else: - # If the loop exits successfully instead of throwing a KeyError, then we have had - # more events than expected. There should be no more than len(expected_types) events. - assert False + else: + # If the loop exits successfully instead of throwing a KeyError, then we have had + # more events than expected. There should be no more than len(expected_types) events. + raise AssertionError(f"Too many events: expected {len(expected_types)}, but got more")🧰 Tools
🪛 Ruff (0.11.9)
122-122: Do not
assert False(python -Oremoves these calls), raiseAssertionError()Replace
assert False(B011)
🧹 Nitpick comments (1)
test/functional/interface_usdt_coinselection.py (1)
11-14: Consider using contextlib.suppress for cleaner import handling.The current try-except-pass pattern can be simplified using
contextlib.suppress.-try: - from bcc import BPF, USDT # type: ignore[import] -except ImportError: - pass +from contextlib import suppress + +with suppress(ImportError): + from bcc import BPF, USDT # type: ignore[import]🧰 Tools
🪛 Ruff (0.11.9)
11-14: Use
contextlib.suppress(ImportError)instead oftry-except-pass(SIM105)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between dc09ed354f5d274b6dc0575995c65a8886f4d61d and 13637df.
📒 Files selected for processing (35)
configure.ac(0 hunks)doc/release-notes-6685.md(1 hunks)doc/tracing.md(1 hunks)src/Makefile.am(1 hunks)src/Makefile.test.include(4 hunks)src/bench/coin_selection.cpp(2 hunks)src/qt/coincontroldialog.cpp(1 hunks)src/test/fuzz/fuzz.cpp(1 hunks)src/util/overloaded.h(1 hunks)src/util/types.h(1 hunks)src/validation.cpp(1 hunks)src/wallet/coinselection.cpp(10 hunks)src/wallet/coinselection.h(5 hunks)src/wallet/interfaces.cpp(1 hunks)src/wallet/rpc/backup.cpp(1 hunks)src/wallet/rpc/transactions.cpp(1 hunks)src/wallet/rpc/wallet.cpp(1 hunks)src/wallet/spend.cpp(11 hunks)src/wallet/test/coinjoin_tests.cpp(1 hunks)src/wallet/test/coinselector_tests.cpp(15 hunks)src/wallet/test/fuzz/coinselection.cpp(1 hunks)src/wallet/test/fuzz/notifications.cpp(1 hunks)src/wallet/test/psbt_wallet_tests.cpp(1 hunks)src/wallet/test/wallet_tests.cpp(5 hunks)src/wallet/test/wallet_transaction_tests.cpp(1 hunks)src/wallet/transaction.cpp(1 hunks)src/wallet/transaction.h(6 hunks)src/wallet/wallet.cpp(30 hunks)src/wallet/wallet.h(5 hunks)src/wallet/walletdb.cpp(1 hunks)test/functional/interface_usdt_coinselection.py(1 hunks)test/functional/test_runner.py(2 hunks)test/functional/wallet_balance.py(1 hunks)test/functional/wallet_basic.py(3 hunks)test/sanitizer_suppressions/ubsan(0 hunks)
💤 Files with no reviewable changes (2)
- test/sanitizer_suppressions/ubsan
- configure.ac
✅ Files skipped from review due to trivial changes (5)
- src/wallet/walletdb.cpp
- src/test/fuzz/fuzz.cpp
- src/wallet/rpc/backup.cpp
- src/Makefile.am
- src/qt/coincontroldialog.cpp
🚧 Files skipped from review as they are similar to previous changes (21)
- src/util/overloaded.h
- src/util/types.h
- src/wallet/test/coinjoin_tests.cpp
- src/validation.cpp
- test/functional/test_runner.py
- src/wallet/coinselection.h
- src/wallet/transaction.cpp
- test/functional/wallet_balance.py
- doc/release-notes-6685.md
- src/wallet/test/psbt_wallet_tests.cpp
- src/bench/coin_selection.cpp
- src/wallet/interfaces.cpp
- src/wallet/test/fuzz/coinselection.cpp
- test/functional/wallet_basic.py
- src/wallet/test/wallet_transaction_tests.cpp
- src/Makefile.test.include
- src/wallet/test/wallet_tests.cpp
- src/wallet/spend.cpp
- src/wallet/test/coinselector_tests.cpp
- src/wallet/wallet.cpp
- src/wallet/wallet.h
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
🧬 Code Graph Analysis (2)
src/wallet/coinselection.cpp (1)
src/wallet/coinselection.h (1)
group(274-274)
src/wallet/transaction.h (1)
src/wallet/wallet.h (14)
tx(300-300)tx(319-319)tx(325-325)tx(329-329)tx(617-617)tx(619-619)tx(641-641)tx(650-650)tx(652-652)tx(692-692)tx(790-790)tx(792-792)tx(793-793)tx(915-915)
🪛 LanguageTool
doc/tracing.md
[uncategorized] ~174-~174: Possible missing preposition found.
Context: ...epoint coin_selection:selected_coins Is called when SelectCoins completes. A...
(AI_HYDRA_LEO_MISSING_IT)
[grammar] ~185-~185: This sentence seems to be incomplete. Insert a noun before ‘Is’ to make the sentence complete.
Context: ...n_selection:normal_create_tx_internal Is called when the firstCreateTransactio...
(MISSING_SUBJECT)
[misspelling] ~191-~191: Did you mean “and” or “any”?
Context: ...ool3. The expected transaction fee as anint64` 4. The position of the change o...
(AN_AND)
[grammar] ~195-~195: This sentence seems to be incomplete. Insert a noun before ‘Is’ to make the sentence complete.
Context: ...in_selection:attempting_aps_create_tx Is called whenCreateTransactionInternal`...
(MISSING_SUBJECT)
[uncategorized] ~204-~204: Possible missing preposition found.
Context: ...coin_selection:aps_create_tx_internal Is called when the secondCreateTransacti...
(AI_HYDRA_LEO_MISSING_IT)
[misspelling] ~211-~211: Did you mean “and” or “any”?
Context: ...ool4. The expected transaction fee as anint64` 5. The position of the change o...
(AN_AND)
🪛 markdownlint-cli2 (0.17.2)
doc/tracing.md
210-210: Spaces inside code span elements
null
(MD038, no-space-in-code)
🪛 Ruff (0.11.9)
test/functional/interface_usdt_coinselection.py
11-14: Use contextlib.suppress(ImportError) instead of try-except-pass
(SIM105)
122-122: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: build (x86_64-apple-darwin)
- GitHub Check: build (arm64-apple-darwin)
- GitHub Check: build (x86_64-w64-mingw32)
- GitHub Check: build (aarch64-linux-gnu)
- GitHub Check: build (x86_64-linux-gnu)
- GitHub Check: build (powerpc64-linux-gnu)
- GitHub Check: build (riscv64-linux-gnu)
- GitHub Check: build (arm-linux-gnueabihf)
🔇 Additional comments (28)
src/wallet/rpc/wallet.cpp (1)
741-741: LGTM! Clean transition to the new transaction state API.The change from direct field access to the
isConfirmed()method properly encapsulates the transaction state logic and aligns with the broader refactor to the variant-based transaction state system.src/wallet/rpc/transactions.cpp (1)
35-42: Excellent implementation of the new variant-based transaction state pattern.The code correctly uses
std::get_ifthrough thestate<TxStateConfirmed>()template method to safely access the confirmed transaction state. The field access is properly updated to use the newTxStateConfirmedstruct members (confirmed_block_hash,confirmed_block_height,position_in_block) instead of the legacy confirmation struct.src/wallet/transaction.h (8)
14-19: Required includes for the variant-based state system.The addition of
util/overloaded.handvariantheaders is necessary for the newstd::variant-based transaction state implementation.
22-68: Well-designed transaction state structs with clear semantics.The state structs provide excellent separation of concerns:
TxStateConfirmed: Contains block hash, height, and position for confirmed transactionsTxStateInMempool: Simple marker for mempool transactionsTxStateConflicted: Tracks conflicting block informationTxStateInactive: Handles abandoned and other inactive statesTxStateUnrecognized: Graceful handling of unknown serialization dataThe explicit constructors and member initialization ensure proper state management.
71-83: Robust deserialization with backward compatibility.The
TxStateInterpretSerializedfunction correctly handles the legacy serialization format by mapping special values:
uint256::ZEROwith index 0 →TxStateInactive{}uint256::ONEwith index -1 →TxStateInactive{abandoned=true}- Valid block hash with index ≥ 0 →
TxStateConfirmed- Valid block hash with index -1 →
TxStateConflictedThis ensures seamless migration from the old confirmation struct format.
86-107: Consistent serialization functions that maintain format compatibility.The
TxStateSerializedBlockHashandTxStateSerializedIndexfunctions correctly implement the inverse mapping ofTxStateInterpretSerialized, ensuring:
- Consistent round-trip serialization
- Proper use of
std::visitwithutil::Overloaded- Maintenance of the legacy encoding scheme
The implementation handles all state variants appropriately.
202-205: Constructor updated to require explicit transaction state.The constructor now mandates a
TxStateparameter, preventing creation of wallet transactions with undefined state. This is a significant improvement in API safety and clarity.
223-223: Core state member replaces legacy confirmation system.The
TxState m_statemember is the heart of the new variant-based system, replacing the previous confirmation struct and boolean flags with a type-safe, extensible state representation.
241-258: Backward-compatible serialization maintains wallet database format.The serialization logic correctly:
- Uses the new helper functions to convert
TxStateto the legacy format- Preserves the existing field order in the serialized data
- Handles deserialization by interpreting legacy data through
TxStateInterpretSerializedThis ensures existing wallet.dat files can be read without migration.
296-302: Clean and intuitive state query API.The new methods provide excellent encapsulation:
state<T>()template methods for direct variant accessisAbandoned(),isConflicted(),isUnconfirmed(),isConfirmed()for common queriesThe logic correctly implements the state relationships, particularly
isUnconfirmed()which excludes abandoned, conflicted, and confirmed states.src/wallet/test/fuzz/notifications.cpp (4)
1-20: Headers and includes look appropriate.The included headers are comprehensive and suitable for a wallet notification fuzz test, covering fuzz testing utilities, wallet functionality, and standard library components.
21-28: Global setup follows fuzz test best practices.The initialization pattern with a global testing setup pointer and no-log-file context is appropriate for fuzz testing scenarios.
30-80: Well-structured wallet wrapper with proper resource management.The
FuzzedWalletstruct correctly manages wallet lifecycle with appropriate setup, cleanup, and randomized script generation for fuzzing scenarios.
160-167: Excellent balance consistency validation.The balance check ensures the fundamental invariant that total wallet balances equal the initial amount, providing strong validation of wallet notification correctness during block operations.
src/wallet/coinselection.cpp (10)
69-69: Good addition of explicit algorithm specification.This change correctly specifies the BNB algorithm when constructing the SelectionResult, enabling proper tracking of which algorithm was used for coin selection.
168-169: Ensure waste computation is always called.The addition of
ComputeAndSetWasteis critical as noted in the PR objectives - tracepoints depend on this being calculated to prevent crashes whenGetWaste()is called. The assertion verifies consistency between the local waste calculation and the SelectionResult's waste.
178-178: Consistent algorithm specification for SRD.Good consistency with the BNB changes, properly specifying the SRD algorithm.
198-209: Improved documentation for ApproximateBestSubset.The enhanced documentation clearly explains the function's approximation behavior and parameter meanings, improving code maintainability.
289-289: Consistent algorithm specification for Knapsack.Good consistency with other coin selection algorithms, properly specifying the KNAPSACK algorithm.
308-308: Appropriate change target for fully mixed mode.Setting
change_target = 0for fully mixed mode is correct since no change is allowed in CoinJoin transactions.
327-327: Consistent use of change_target parameter.The logic correctly uses the
change_targetparameter instead of hardcoded minimum change values, enabling the randomized change targeting feature.Also applies to: 370-372, 377-377
538-549: Well-implemented algorithm name utility.The
GetAlgorithmNamefunction properly covers all SelectionAlgorithm enum values with appropriate string representations. The assert(false) at the end ensures compilation warnings if new algorithms are added without updating this function.
475-484:Details
✅ Verification successful
Verify change target constants are properly defined.
The
GenerateChangeTargetfunction referencesCHANGE_LOWERandCHANGE_UPPERconstants. Ensure these are properly defined in the header file.
🏁 Script executed:
#!/bin/bash # Description: Verify CHANGE_LOWER and CHANGE_UPPER constants are defined # Expected: Both constants should be defined in coinselection.h rg -A 2 -B 2 "CHANGE_LOWER|CHANGE_UPPER" src/wallet/coinselection.hLength of output: 351
CHANGE_LOWER and CHANGE_UPPER confirmed defined
Both constants are declared insrc/wallet/coinselection.has:static constexpr CAmount CHANGE_LOWER{50000}; static constexpr CAmount CHANGE_UPPER{1000000};No further action needed.
284-285:Details
✅ Verification successful
Verify all callers updated for new signature.
The KnapsackSolver signature change adds the
change_targetparameter. Ensure all call sites have been updated to pass this new parameter.
🏁 Script executed:
#!/bin/bash # Description: Find all calls to KnapsackSolver to verify they use the new signature # Expected: All calls should include the change_target parameter ast-grep --pattern 'KnapsackSolver($$$)'Length of output: 6822
All call sites updated to new signature
I searched for every invocation of
KnapsackSolverand confirmed that all calls now supply thechange_targetargument in the correct position. No remaining sites use the old 3- or 4-parameter form.— src/wallet/spend.cpp
— src/wallet/test/fuzz/coinselection.cpp
— src/wallet/test/coinselector_tests.cppdoc/tracing.md (1)
171-213: Comprehensive tracepoint documentation.The new
coin_selectioncontext documentation is well-structured and follows the established format. It clearly describes:
- selected_coins: Captures coin selection completion with algorithm, target, waste, and selected value
- normal_create_tx_internal: Tracks first transaction creation attempt
- attempting_aps_create_tx: Signals APS (Avoid Partial Spends) attempt
- aps_create_tx_internal: Tracks APS transaction creation completion
The argument types and order are clearly specified, which is important for the semi-stable API requirement mentioned in the guidelines.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~174-~174: Possible missing preposition found.
Context: ...epointcoin_selection:selected_coinsIs called whenSelectCoinscompletes. A...(AI_HYDRA_LEO_MISSING_IT)
[grammar] ~185-~185: This sentence seems to be incomplete. Insert a noun before ‘Is’ to make the sentence complete.
Context: ...n_selection:normal_create_tx_internalIs called when the firstCreateTransactio...(MISSING_SUBJECT)
[misspelling] ~191-~191: Did you mean “and” or “any”?
Context: ...ool3. The expected transaction fee as anint64` 4. The position of the change o...(AN_AND)
[grammar] ~195-~195: This sentence seems to be incomplete. Insert a noun before ‘Is’ to make the sentence complete.
Context: ...in_selection:attempting_aps_create_txIs called whenCreateTransactionInternal`...(MISSING_SUBJECT)
[uncategorized] ~204-~204: Possible missing preposition found.
Context: ...coin_selection:aps_create_tx_internalIs called when the secondCreateTransacti...(AI_HYDRA_LEO_MISSING_IT)
[misspelling] ~211-~211: Did you mean “and” or “any”?
Context: ...ool4. The expected transaction fee as anint64` 5. The position of the change o...(AN_AND)
🪛 markdownlint-cli2 (0.17.2)
210-210: Spaces inside code span elements
null(MD038, no-space-in-code)
test/functional/interface_usdt_coinselection.py (3)
22-96: Well-structured BPF program for tracepoint capture.The eBPF program correctly defines event structures and capture functions for all four coin selection tracepoints. The event queue and data extraction logic properly handle the different argument types and counts for each tracepoint.
128-157: Comprehensive event parsing logic.The
determine_selection_from_usdtfunction correctly parses the event sequence to extract coin selection metrics, handling both normal and APS (Avoid Partial Spends) scenarios. The logic properly tracks which algorithm and waste values to use based on the APS decision.
159-204: Thorough test coverage of tracepoint scenarios.The test comprehensively validates:
- Successful transactions: Verifies all 5 expected tracepoints fire in correct order
- Failed transactions: Ensures only failure tracepoint fires
- APS behavior: Tests avoid-reuse flag produces expected tracepoint sequence
This provides good coverage of the tracepoint API under different wallet operation scenarios.
| LIMITED_WHILE(in > 0 && fuzzed_data_provider.ConsumeBool(), 100) | ||
| { | ||
| const auto out_value{ConsumeMoney(fuzzed_data_provider, in)}; | ||
| in -= out_value; | ||
| auto& wallet{fuzzed_data_provider.ConsumeBool() ? a : b}; | ||
| tx.vout.emplace_back(out_value, wallet.GetScriptPubKey(fuzzed_data_provider)); | ||
| } | ||
| // Spend the remaining input value, if any | ||
| auto& wallet{fuzzed_data_provider.ConsumeBool() ? a : b}; | ||
| tx.vout.emplace_back(in, wallet.GetScriptPubKey(fuzzed_data_provider)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential issue with zero-value outputs.
The transaction creation logic could produce zero-value outputs when the LIMITED_WHILE loop consumes all input value. Zero-value outputs are typically considered dust and may result in invalid or non-standard transactions.
Consider adding a check to avoid creating zero-value outputs:
// Spend the remaining input value, if any
-auto& wallet{fuzzed_data_provider.ConsumeBool() ? a : b};
-tx.vout.emplace_back(in, wallet.GetScriptPubKey(fuzzed_data_provider));
+if (in > 0) {
+ auto& wallet{fuzzed_data_provider.ConsumeBool() ? a : b};
+ tx.vout.emplace_back(in, wallet.GetScriptPubKey(fuzzed_data_provider));
+}📝 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.
| LIMITED_WHILE(in > 0 && fuzzed_data_provider.ConsumeBool(), 100) | |
| { | |
| const auto out_value{ConsumeMoney(fuzzed_data_provider, in)}; | |
| in -= out_value; | |
| auto& wallet{fuzzed_data_provider.ConsumeBool() ? a : b}; | |
| tx.vout.emplace_back(out_value, wallet.GetScriptPubKey(fuzzed_data_provider)); | |
| } | |
| // Spend the remaining input value, if any | |
| auto& wallet{fuzzed_data_provider.ConsumeBool() ? a : b}; | |
| tx.vout.emplace_back(in, wallet.GetScriptPubKey(fuzzed_data_provider)); | |
| LIMITED_WHILE(in > 0 && fuzzed_data_provider.ConsumeBool(), 100) | |
| { | |
| const auto out_value{ConsumeMoney(fuzzed_data_provider, in)}; | |
| in -= out_value; | |
| auto& wallet{fuzzed_data_provider.ConsumeBool() ? a : b}; | |
| tx.vout.emplace_back(out_value, wallet.GetScriptPubKey(fuzzed_data_provider)); | |
| } | |
| // Spend the remaining input value, if any | |
| if (in > 0) { | |
| auto& wallet{fuzzed_data_provider.ConsumeBool() ? a : b}; | |
| tx.vout.emplace_back(in, wallet.GetScriptPubKey(fuzzed_data_provider)); | |
| } |
🤖 Prompt for AI Agents
In src/wallet/test/fuzz/notifications.cpp around lines 125 to 134, the code may
create zero-value outputs if the input value is fully consumed in the
LIMITED_WHILE loop, which can lead to invalid or non-standard transactions. Add
a condition before creating the final output to check if the remaining input
value is greater than zero, and only then create the output to avoid zero-value
outputs.
3 participants
Additional Information
In bitcoin#24820, one of the
BOOST_CHECKs has been commented out as it relies on the usage of BnB coin selection, which is disabled for Dash for not being mixing aware. Regardless, when testing, that disablement (source) was commented out and the check commented back in and it passes.In
SelectCoins()(source), care is needed to ensure that every non-std::nulloptreturn path calculates waste (callsSelectionResult::ComputeAndSetWaste()). Due to the tracepoints introduced in bitcoin#24644, a missing waste calculation will result in the client crashing (specifically due to aGetWaste()call here).How Has This Been Tested?
A full CoinJoin session run on dc09ed354f5d
Breaking Changes
None expected.
Checklist