backport: merge bitcoin#24587, #24637, #24623, #23075, #24817, #25087, #24839, #20456, #26640, #26892, #26280, #26657, #26923, partial bitcoin#19937 (test backports: part 3) #6726
Conversation
, bitcoin#25435, bitcoin#23017, bitcoin#25522, bitcoin#26414, partial bitcoin#24587, bitcoin#24637, bitcoin#24623, bitcoin#25445 (implement better tx capabilities in `MiniWallet`) cfa906e merge bitcoin#26414: Move tx creation to create_self_transfer_multi (Kittywhiskers Van Gogh) e167162 merge bitcoin#25522: Remove -acceptnonstdtxn=1 from feature_rbf.py (Kittywhiskers Van Gogh) a5cb9b4 partial bitcoin#25445: Return new_utxo from create_self_transfer in MiniWallet (Kittywhiskers Van Gogh) b28d90f merge bitcoin#23017: Replace MiniWallet scan_blocks with rescan_utxos (Kittywhiskers Van Gogh) ebfb239 merge bitcoin#25435: Remove from_node from create_self_transfer* MiniWallet helpers (Kittywhiskers Van Gogh) 144228b merge bitcoin#25356: Remove MiniWallet mempool_valid option (Kittywhiskers Van Gogh) 0622788 merge bitcoin#24941: support skipping mempool checks (feature_fee_estimation.py performance fix) (Kittywhiskers Van Gogh) 9c93e3a merge bitcoin#25228: add BIP-125 rule 5 testcase with default mempool (Kittywhiskers Van Gogh) b84bcad partial bitcoin#24623: Add diamond-shape prioritisetransaction test (Kittywhiskers Van Gogh) 4621320 partial bitcoin#24637: use MiniWallet for mempool_package_onemore.py (Kittywhiskers Van Gogh) a21dfd2 partial bitcoin#24587: use MiniWallet for rpc_createmultisig.py (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependency for #6726 * Since [bitcoin#15891](bitcoin#15891) (8ca90f3), we enforce standard transactions on regtest and regrettably, the logic introduced in [bitcoin#24637](bitcoin#24637) (`*self_transfer*()`) had a tendency to create transactions that were non-standard (see below) <details> <summary>Test failure:</summary> ``` dash@b90780fda2d1:/src/dash$ ./test/functional/mempool_package_onemore.py 2025-06-18T17:50:19.492000Z TestFramework (INFO): PRNG seed is: 8344907287561424619 2025-06-18T17:50:19.492000Z TestFramework (INFO): Initializing test directory /tmp/dash_func_test_emt8bu6u 2025-06-18T17:50:19.765000Z TestFramework (ERROR): JSONRPC error Traceback (most recent call last): File "/src/dash/test/functional/test_framework/test_framework.py", line 162, in main self.run_test() File "/src/dash/./test/functional/mempool_package_onemore.py", line 41, in run_test utxo, utxo2 = self.chain_tx([utxo], num_outputs=2) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/src/dash/./test/functional/mempool_package_onemore.py", line 28, in chain_tx return self.wallet.send_self_transfer_multi( ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/src/dash/test/functional/test_framework/wallet.py", line 207, in send_self_transfer_multi txid = self.sendrawtransaction(from_node=kwargs['from_node'], tx_hex=tx.serialize().hex()) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/src/dash/test/functional/test_framework/wallet.py", line 269, in sendrawtransaction txid = from_node.sendrawtransaction(hexstring=tx_hex, maxfeerate=maxfeerate, **kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/src/dash/test/functional/test_framework/coverage.py", line 49, in __call__ return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/src/dash/test/functional/test_framework/authproxy.py", line 143, in __call__ raise JSONRPCException(response['error'], status) test_framework.authproxy.JSONRPCException: bad-txns-nonstandard-inputs (-26) 2025-06-18T17:50:20.266000Z TestFramework (INFO): Stopping nodes ``` </details> Two options presented themselves, either temporary relaxing `fRequireStandard` on regtest or backporting all the changes needed to make `*self_transfer*()` create standard transactions and then backporting `MiniWallet` conversions in a separate pull request. The latter approach was taken. * The correctness of this pull request can be validated by observing the CI results of [dash#6726](#6726) * The backport for [bitcoin#23017](bitcoin#23017) does not include changes to `rpc_blockchain.py` as the relevant `scan_blocks()` call was removed in the partial backport of [bitcoin#23371](bitcoin#23371) (081897c). ## Breaking Changes None expected. Affects only functional tests. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK cfa906e UdjinM6: utACK cfa906e Tree-SHA512: 81107b6352cc673cd4d4358da0eba23f971546fae9df0f34bea5b584bfe1a3f820872444314ec65bf181158aeca1cc3880a0b6925101c953db19ecd45d28aa38
includes: - 2726b60 (changes excl. wallet.py) continuation of a21dfd2 checkbalances() has to be modified as the balances are now predominantly generated by MiniWallet and the nine[1] blocks mined by the test leave behind immature balances that we must discount. [1] - 8 blocks mined by 4 calls to do_multisig() and 1 block mined by 1 call to checkbalances())
includes: - 95d5d5e needed for backports affecting mining_prioritisetransaction.py so we don't need to manually reconnect (or keep around) nodes.
includes changes to feature_fee_estimation.py from bitcoin#25445 (fa04ff6) due to OOO backport needed for correctness
includes changes to feature_dbcrash.py from bitcoin#25445 (fa04ff6) due to OOO backport needed for correctness
WalkthroughThe changes span various source and test files, introducing several targeted updates. In the core code, the ✨ 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)
test/functional/rpc_scantxoutset.py (1)
35-36: Verify the hardcoded coinbase count expectation.The test asserts that exactly 49 coinbase outputs should be found, but this hardcoded value might be brittle if the test setup changes. Consider adding a comment explaining how this number is derived (e.g., number of blocks generated + any pre-existing coinbase outputs).
+ # Expect 49 coinbase outputs: X blocks from setup + Y from previous operations self.log.info("Test if we find coinbase outputs.") assert_equal(sum(u["coinbase"] for u in self.nodes[0].scantxoutset("start", [self.wallet.get_descriptor()])["unspents"]), 49)test/functional/feature_dbcrash.py (1)
189-191: Minor optimization: Use generator expression in sum()Consider using a generator expression for better memory efficiency as suggested by static analysis.
- input_amount = int(sum([utxo['value'] for utxo in utxos_to_spend]) * COIN) + input_amount = int(sum(utxo['value'] for utxo in utxos_to_spend) * COIN)test/functional/mining_prioritisetransaction.py (1)
105-133: Consider refactoring to reduce method complexity.While the MiniWallet integration is correct, the
run_testmethod has grown complex with 20 local variables and 64 statements (pylint warnings). Consider extracting some test logic into helper methods, such as:
- Initial parameter validation tests (lines 109-124)
- UTXO setup and transaction batch creation logic
test/functional/test_framework/util.py (1)
617-632: Consider making some parameters keyword-only to improve API clarity.The function has 6 positional arguments, which exceeds the recommended limit. Consider making less frequently used parameters keyword-only to improve the API:
-def create_lots_of_big_transactions(mini_wallet, node, fee, tx_batch_size, txouts, utxos=None): +def create_lots_of_big_transactions(mini_wallet, node, fee, tx_batch_size, txouts, *, utxos=None):This would require callers to explicitly name the
utxosparameter when provided, making the code more readable.test/functional/feature_fee_estimation.py (1)
24-63: Consider refactoring function signature to use keyword arguments.The function has 7 positional arguments. Consider grouping related parameters or making some keyword-only:
-def small_txpuzzle_randfee( - wallet, from_node, conflist, unconflist, amount, min_fee, fee_increment -): +def small_txpuzzle_randfee( + wallet, from_node, conflist, unconflist, *, amount, min_fee, fee_increment +):This would make the fee-related parameters more explicit at call sites.
test/functional/test_framework/wallet.py (3)
94-95: Consider using a dataclass for UTXO representation.While the current implementation works, consider using a dataclass for better type safety and maintainability:
from dataclasses import dataclass @dataclass class UTXO: txid: str vout: int value: Decimal height: int coinbase: bool confirmations: intThis would make the code more maintainable and provide better IDE support.
135-156: Fix code style issues in sign_tx method.The refactoring correctly handles all wallet modes, but has minor style issues:
- while not len(der_sig) == 71: + while len(der_sig) != 71: der_sig = self._priv_key.sign_ecdsa(sighash)else: - assert False + raise AssertionError(f"Unknown wallet mode: {self._mode}")
201-210: Fix Yoda condition in get_utxos method.The method implementation is correct, but has a style issue:
- utxo_filter = filter(lambda utxo: not utxo['coinbase'] or COINBASE_MATURITY <= utxo['confirmations'], self._utxos) + utxo_filter = filter(lambda utxo: not utxo['coinbase'] or utxo['confirmations'] >= COINBASE_MATURITY, self._utxos)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/rpc/blockchain.cpp(2 hunks)src/rpc/mining.cpp(1 hunks)test/functional/feature_bip68_sequence.py(16 hunks)test/functional/feature_dbcrash.py(3 hunks)test/functional/feature_fee_estimation.py(10 hunks)test/functional/mempool_compatibility.py(1 hunks)test/functional/mempool_limit.py(2 hunks)test/functional/mempool_package_onemore.py(1 hunks)test/functional/mining_prioritisetransaction.py(4 hunks)test/functional/p2p_eviction.py(5 hunks)test/functional/p2p_permissions.py(2 hunks)test/functional/p2p_tx_download.py(4 hunks)test/functional/rpc_createmultisig.py(4 hunks)test/functional/rpc_scantxoutset.py(1 hunks)test/functional/test_framework/util.py(1 hunks)test/functional/test_framework/wallet.py(6 hunks)test/functional/test_runner.py(1 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
test/functional/feature_dbcrash.py
[refactor] 190-190: Consider using a generator instead 'sum(utxo['value'] for utxo in utxos_to_spend)'
(R1728)
test/functional/mining_prioritisetransaction.py
[refactor] 105-105: Too many local variables (20/15)
(R0914)
[refactor] 105-105: Too many statements (64/50)
(R0915)
test/functional/feature_fee_estimation.py
[refactor] 24-24: Too many arguments (7/5)
(R0913)
[refactor] 24-24: Too many positional arguments (7/5)
(R0917)
test/functional/test_framework/util.py
[refactor] 617-617: Too many arguments (6/5)
(R0913)
[refactor] 617-617: Too many positional arguments (6/5)
(R0917)
test/functional/test_framework/wallet.py
[refactor] 94-94: Too many arguments (7/5)
(R0913)
test/functional/rpc_createmultisig.py
[refactor] 25-25: Too many instance attributes (11/7)
(R0902)
[refactor] 47-47: Too many local variables (18/15)
(R0914)
[refactor] 125-125: Too many local variables (28/15)
(R0914)
[refactor] 125-125: Too many statements (55/50)
(R0915)
🪛 Ruff (0.11.9)
test/functional/test_framework/wallet.py
143-143: Use len(der_sig) != 71 instead of not len(der_sig) == 71
Replace with != operator
(SIM201)
156-156: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
204-204: Yoda condition detected
Rewrite as utxo['confirmations'] >= COINBASE_MATURITY
(SIM300)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: linux64-test / Test source
🔇 Additional comments (33)
src/rpc/blockchain.cpp (2)
2425-2425: LGTM: Well-documented new featureThe documentation correctly describes the new "coinbase" field that indicates whether each unspent output is from a coinbase transaction. This enhances the API by providing useful metadata to callers.
2529-2529: LGTM: Clean implementation of coinbase fieldThe implementation correctly uses
coin.IsCoinBase()to determine and expose the coinbase status of each unspent output. This is a backward-compatible enhancement that adds valuable information without affecting existing functionality.test/functional/test_runner.py (1)
271-271: LGTM! Test consolidation aligns with refactoring objectives.The consolidation of the two separate
rpc_createmultisig.pyentries into a single entry without wallet mode flags is consistent with the PR's objective to make the test handle different wallet modes conditionally rather than through separate test runs.src/rpc/mining.cpp (2)
731-739: LGTM! Proper isolation of test chain behavior.The conditional bypass of peer connection and IBD checks for test chains is well-implemented. It enables testing of
getblocktemplatewithout requiring complex network setup while maintaining all safety checks for production chains.
742-742: Good placement of the sporkman assertion.Moving the
CHECK_NONFATAL(node.sporkman);outside the conditional ensures it executes regardless of chain type, which is the correct behavior since sporkman is needed for all chains.test/functional/mempool_compatibility.py (1)
52-52: LGTM! Improved transaction handling with MiniWallet.The replacement of manual wallet RPC calls with
MiniWallet.send_self_transfer()improves test reliability and aligns with the broader test framework modernization. The test logic remains the same while benefiting from standardized UTXO and transaction management.test/functional/mempool_limit.py (2)
15-15: Good addition of shared utility import.Importing
create_lots_of_big_transactionsfrom the shared test framework utilities promotes code reuse and standardization across mempool-related tests.
64-64: LGTM! Improved code reuse with shared utility function.Replacing the local
send_large_txsmethod with the sharedcreate_lots_of_big_transactionsutility function eliminates code duplication and benefits from the enhanced MiniWallet-based implementation with better fee handling and mempool acceptance checks.test/functional/p2p_eviction.py (2)
58-59: LGTM: Clean MiniWallet integrationThe MiniWallet initialization and UTXO rescanning is correctly implemented, replacing the manual transaction setup.
85-86: LGTM: Simplified transaction creationThe replacement of manual raw transaction construction with
self.wallet.create_self_transfer()['tx']is correct and maintains the same functionality while improving code clarity.test/functional/p2p_permissions.py (2)
28-30: LGTM: Proper MiniWallet setupThe MiniWallet initialization and UTXO rescanning is correctly implemented at the start of the test.
102-103: LGTM: Correct transaction creation with sequencingThe use of
create_self_transfer(sequence=SEQUENCE_FINAL)properly replaces the manual transaction construction while maintaining the required transaction properties for the permission test.test/functional/p2p_tx_download.py (2)
146-148: LGTM: Proper wallet initializationThe MiniWallet setup at the beginning of
run_testis correctly implemented.
82-83: LGTM: Simplified transaction handlingThe replacement of manual transaction creation with
self.wallet.create_self_transfer()and proper txid extraction from the transaction dictionary is correct and maintains the test functionality.test/functional/feature_dbcrash.py (3)
205-207: LGTM: Proper MiniWallet integrationThe MiniWallet initialization and UTXO rescanning correctly replaces the manual wallet setup, improving test maintainability.
195-200: LGTM: Simplified transaction creationThe use of
send_self_transfer_multiwith proper parameters effectively replaces the manual transaction construction while maintaining the same test behavior.
264-266: LGTM: Consistent UTXO managementThe wallet rescanning and UTXO retrieval properly replaces the manual UTXO list management, maintaining test correctness while improving code clarity.
test/functional/mempool_package_onemore.py (4)
27-31: LGTM: Well-designed helper methodThe
chain_txmethod provides a clean abstraction for transaction chaining using MiniWallet, improving code readability and maintainability.
34-35: LGTM: Proper wallet initializationThe MiniWallet setup correctly replaces the manual transaction and UTXO management infrastructure.
52-59: LGTM: Consistent error handling updatesAll error assertions have been properly updated to use the new
chain_txmethod while maintaining the same test validation logic for mempool ancestor and descendant limits.
39-46: LGTM: Simplified transaction chain constructionThe refactored chain building logic using
chain_txandwallet.get_utxo()is much cleaner than the previous manual approach while maintaining identical test behavior.test/functional/mining_prioritisetransaction.py (3)
7-20: LGTM! Imports are well-organized and necessary.The added imports support the refactoring to use MiniWallet and are properly organized by module.
32-104: Well-designed test for diamond-shaped transaction prioritization.The test comprehensively covers prioritization behavior for a diamond-shaped dependency graph, properly testing both in-mempool and post-restart scenarios. Good use of Decimal for precise fee calculations.
144-150: Clean refactoring of transaction creation.The transaction creation is now much simpler and more maintainable using MiniWallet methods. The fee handling is consistent and the code is more readable.
Also applies to: 209-211
test/functional/feature_bip68_sequence.py (3)
27-27: Correct MiniWallet integration.The MiniWallet is properly initialized and used throughout the test, simplifying UTXO management and transaction creation.
Also applies to: 53-56, 83-84
95-96: Transaction handling properly migrated to MiniWallet.The signing and sending of transactions now use MiniWallet methods while preserving the original test logic and error handling.
Also applies to: 108-108, 114-114
397-401: Proper wallet isolation for multi-node test.Creating a separate MiniWallet instance for node 1 is the correct approach to maintain wallet isolation between nodes.
test/functional/test_framework/util.py (1)
635-641: Clean integration with refactored transaction creation.The function properly delegates to the refactored
create_lots_of_big_transactionswith appropriate fee calculation.test/functional/feature_fee_estimation.py (2)
121-124: Clean test setup with MiniWallet.The removal of
-acceptnonstdtxn=1is appropriate since MiniWallet creates standard transactions. The initial UTXO splitting is much cleaner usingsend_self_transfer_multi.Also applies to: 174-177, 223-224
150-158: Correct integration with refactored transaction creation.The method properly passes the MiniWallet instance to the refactored
small_txpuzzle_randfeefunction.test/functional/test_framework/wallet.py (1)
100-118: Excellent enhancement to support mempool-aware UTXO rescanning.The implementation correctly:
- Calculates confirmations as
height - utxo_height + 1- Sorts mempool transactions by ancestor count to match mining behavior
- Maintains backward compatibility with the optional parameter
test/functional/rpc_createmultisig.py (2)
20-23: Well-implemented MiniWallet integrationThe refactoring to use MiniWallet for wallet-independent operations is clean and follows the established patterns from other test files in this PR.
Also applies to: 49-49, 55-55, 161-162
30-31: Proper isolation of wallet-dependent functionalityThe conditional logic using
is_bdb_compiled()effectively isolates wallet-dependent operations, allowing the test to run in environments without BDB wallet support while maintaining full functionality when available.Also applies to: 42-45, 51-52, 62-63, 75-84, 128-159
includes changes to util.py, mining_prioritisetransaction.py from bitcoin#25435 (fa8421b) and changes to util.py from bitcoin#25356 (fa779de) due to OOO backport needed for correctness
getmempoolinfo["loaded"] was introduced in bitcoin#15323 (dash#4609), shipped as part of v18.x but this version is also needed by bitcoin#26640 as v0.15 does not recognize "maxfeerate", which MiniWallet needs. We also need to delete the datadir of the v18.2.2 node to avoid problems that could arise from the EvoDb, the files needed for the test are managed by the test.
…MiniWallet Running `rescan_utxos()` is necessary to ensure that funds are visible during the tests.
kwvg
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/functional/test_framework/wallet.py (2)
135-156: Address static analysis suggestion and improve code clarity.The
sign_txmethod refactoring properly handles all MiniWallet modes explicitly. However, there are a couple of minor improvements suggested by static analysis:- while not len(der_sig) == 71: + while len(der_sig) != 71:- else: - assert False + else: + raise AssertionError("Unknown MiniWallet mode")The explicit mode handling is much better than the previous implementation and ensures all cases are covered.
201-210: Address static analysis suggestion in coinbase maturity check.The
get_utxosmethod is well-designed for bulk UTXO retrieval with proper filtering and optional spending. However, there's a minor style improvement:- if not include_immature_coinbase: - utxo_filter = filter(lambda utxo: not utxo['coinbase'] or COINBASE_MATURITY <= utxo['confirmations'], self._utxos) + if not include_immature_coinbase: + utxo_filter = filter(lambda utxo: not utxo['coinbase'] or utxo['confirmations'] >= COINBASE_MATURITY, self._utxos)This addresses the "Yoda condition" flagged by static analysis for better readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/rpc/blockchain.cpp(2 hunks)test/functional/feature_bip68_sequence.py(16 hunks)test/functional/mempool_compatibility.py(3 hunks)test/functional/mempool_limit.py(2 hunks)test/functional/mining_prioritisetransaction.py(4 hunks)test/functional/p2p_eviction.py(5 hunks)test/functional/p2p_permissions.py(2 hunks)test/functional/p2p_tx_download.py(4 hunks)test/functional/rpc_scantxoutset.py(1 hunks)test/functional/test_framework/util.py(1 hunks)test/functional/test_framework/wallet.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- test/functional/rpc_scantxoutset.py
- test/functional/p2p_eviction.py
- src/rpc/blockchain.cpp
- test/functional/p2p_permissions.py
- test/functional/mempool_limit.py
- test/functional/mempool_compatibility.py
- test/functional/p2p_tx_download.py
- test/functional/feature_bip68_sequence.py
🧰 Additional context used
🪛 Pylint (3.3.7)
test/functional/mining_prioritisetransaction.py
[refactor] 105-105: Too many local variables (20/15)
(R0914)
[refactor] 105-105: Too many statements (64/50)
(R0915)
test/functional/test_framework/util.py
[refactor] 617-617: Too many arguments (6/5)
(R0913)
[refactor] 617-617: Too many positional arguments (6/5)
(R0917)
test/functional/test_framework/wallet.py
[refactor] 94-94: Too many arguments (7/5)
(R0913)
🪛 Ruff (0.11.9)
test/functional/test_framework/wallet.py
143-143: Use len(der_sig) != 71 instead of not len(der_sig) == 71
Replace with != operator
(SIM201)
156-156: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
204-204: Yoda condition detected
Rewrite as utxo['confirmations'] >= COINBASE_MATURITY
(SIM300)
🔇 Additional comments (15)
test/functional/mining_prioritisetransaction.py (7)
7-21: LGTM: Import changes support MiniWallet refactoring.The import additions for
Decimal,COIN,MAX_BLOCK_SIZE,create_lots_of_big_transactions,gen_return_txouts, andMiniWalletare appropriate for the test modernization.
32-104: LGTM: Well-structured diamond transaction dependency test.The
test_diamondmethod correctly implements a diamond-shaped transaction dependency graph test:
- Creates transactions in proper dependency order (tx_a → tx_b/tx_c → tx_d)
- Tests prioritization both with transactions in mempool and after restart
- Properly verifies fee delta effects on ancestor/descendant relationships
- Uses appropriate cleanup with mempool clearing and node restart
The logic for testing prioritization behavior across node restarts with
-nopersistmempoolis particularly valuable for ensuring consistent behavior.
105-108: LGTM: Proper MiniWallet initialization and UTXO rescan.The initialization of
MiniWalletand callingrescan_utxos()follows the expected pattern for test setup when using the MiniWallet utility.
125-125: LGTM: Diamond test integration.Appropriate placement of the diamond test call within the test flow.
131-134: LGTM: MiniWallet UTXO creation and block generation.The pattern of using
send_self_transfer_multifollowed bygeneratecorrectly creates confirmed UTXOs for the test while maintaining proper test isolation.
144-151: LGTM: Updated utility function call with MiniWallet.The call to
create_lots_of_big_transactionscorrectly passes theMiniWalletinstance as the first parameter, matching the updated function signature inutil.py.
209-212: LGTM: MiniWallet transaction creation for fee testing.Using
create_self_transferwithfee_rate=0appropriately creates a zero-fee transaction for testing prioritization of free transactions.test/functional/test_framework/util.py (2)
617-632: LGTM: Well-refactored function using MiniWallet.The refactored
create_lots_of_big_transactionsfunction properly:
- Accepts
mini_walletas the first parameter for consistency with MiniWallet patterns- Uses
mini_wallet.create_self_transferinstead of manual transaction construction- Correctly handles fee conversion from float to satoshis
- Maintains the transaction size by extending with
txouts- Validates fees through
testmempoolacceptbefore broadcasting- Handles optional UTXO parameter with fallback to internal wallet UTXOs
The static analysis warning about too many arguments is acceptable for this utility function given its comprehensive parameter set.
639-641: LGTM: Simplified mine_large_block using refactored utility.The
mine_large_blockfunction correctly delegates tocreate_lots_of_big_transactionswith the updated signature, simplifying the implementation while maintaining the same functionality.test/functional/test_framework/wallet.py (6)
48-48: LGTM: Added import for coinbase maturity constant.The import of
COINBASE_MATURITYfromblocktoolsis necessary for the new coinbase UTXO filtering logic.
94-95: LGTM: Enhanced UTXO metadata tracking.The
_create_utxomethod now correctly trackscoinbaseandconfirmationsfields, which are essential for proper UTXO management and coinbase maturity handling.
100-118: LGTM: Enhanced rescan with mempool support.The
rescan_utxosmethod improvements are well-implemented:
- Optional
include_mempoolparameter with sensible default- Proper sorting of mempool transactions by ancestor count (matching Bitcoin Core's block assembly logic)
- Correct calculation of confirmations based on block height
- Integration of coinbase field from scan results
The mempool handling logic correctly processes transactions in dependency order.
133-133: LGTM: Consistent UTXO creation in scan_tx.Creating UTXOs with
coinbase=Falseandconfirmations=0for mempool transactions is correct since these are not coinbase transactions and are unconfirmed.
273-273: LGTM: Simplified transaction signing.Replacing manual mode-specific scriptSig handling with a call to the unified
sign_txmethod improves code maintainability and consistency.
282-284: LGTM: Consistent metadata in created UTXOs.Adding
coinbase=Falseandconfirmations=0to newly created UTXOs is correct since these are manually created transactions that are unconfirmed.
, bitcoin#25616, bitcoin#26005, bitcoin#24855, bitcoin#18554, bitcoin#17204, bitcoin#20562, bitcoin#21166, bitcoin#25044, bitcoin#25364, bitcoin#25525, bitcoin#25512, bitcoin#24678, bitcoin#26747, partial bitcoin#22154, bitcoin#24584 (wallet backports: part 8) c0f9225 merge bitcoin#26747: fix confusing error / GUI crash on cross-chain legacy wallet restore (Kittywhiskers Van Gogh) d63ca8a merge bitcoin#24678: Prevent wallet unload on GetWalletForJSONRPCRequest (Kittywhiskers Van Gogh) 2a880d8 merge bitcoin#25512: remove wallet dependency and refactor rpc_signrawtransaction.py (Kittywhiskers Van Gogh) f405790 merge bitcoin#25525: remove wallet dependency from mempool_updatefromblock.py (Kittywhiskers Van Gogh) c24c9ea merge bitcoin#25364: remove wallet dependency from feature_nulldummy.py (Kittywhiskers Van Gogh) a118fe1 test: reduce `num_nodes` in `feature_nulldummy.py` to 1 (Kittywhiskers Van Gogh) c6eabc1 merge bitcoin#25044: Use MiniWallet in rpc_rawtransaction.py (Kittywhiskers Van Gogh) 4e0725e merge bitcoin#21166: Introduce DeferredSignatureChecker and have SignatureExtractorClass subclass it (Kittywhiskers Van Gogh) f883122 merge bitcoin#20562: Test that a fully signed tx given to signrawtx is unchanged (Kittywhiskers Van Gogh) c349dad merge bitcoin#17204: Do not turn OP_1NEGATE in scriptSig into 0x0181 in signing code (Kittywhiskers Van Gogh) c5e8bd6 merge bitcoin#18554: ensure wallet files are not reused across chains (Kittywhiskers Van Gogh) 0fca914 merge bitcoin#24855: Fix `setwalletflag` disabling of flags (Kittywhiskers Van Gogh) f15a1b9 merge bitcoin#26005: Fix error handling (copy_file failure in RestoreWallet, and in general via interfaces) (Kittywhiskers Van Gogh) 240765e merge bitcoin#25616: Return `util::Result` from WalletLoader methods (Kittywhiskers Van Gogh) bd897fa merge bitcoin#25656: return util::Result from `GetReservedDestination` methods (Kittywhiskers Van Gogh) 56accfe merge bitcoin#25721: Replace BResult with util::Result (Kittywhiskers Van Gogh) 03939f2 partial bitcoin#24584: avoid mixing different `OutputTypes` during coin selection (Kittywhiskers Van Gogh) 50ca8cf refactor: remove `CKey` overload for `Create{,AndProcess}Block()` (Kittywhiskers Van Gogh) 9e23b11 merge bitcoin#25218: introduce generic 'Result' class and connect it to CreateTransaction and GetNewDestination (Kittywhiskers Van Gogh) ec764fd partial bitcoin#22154: Add OutputType::BECH32M and related wallet support for fetching bech32m addresses (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * The `CKey` overload for `Create{,AndProcess}Block()` was introduced as a convenience function in [dash#2264](#2264). To allow for the easier backport of [bitcoin#24584](bitcoin#24584) (by resolving an ambiguous overload error) and because `GetScriptForRawPubKey` makes the same construction, it was opted to realign with upstream and drop the overload. * Even though we don't actively support any address types that utilize Bech32m, [bitcoin#25656](bitcoin#25656) assumes that `GetReservedDestination` modifies a `bilingual_str` for its error case in order to encapsulate it in a `util::Result`. In order to complete that backport, [bitcoin#22154](bitcoin#22154) has been partially backported. * `CKeyHolder` has always assumed that `GetReservedDestination` will succeed without validation of this assumption ([source](https://github.com/dashpay/dash/blob/develop/src/coinjoin/util.cpp#L29-L33)). As [bitcoin#25656](bitcoin#25656) forces us to unwrap the returned value, this assumption has been documented with an `assert` ([source](https://github.com/dashpay/dash/blob/bd897fa7097d3d68d19b8a9af14b23f4007645ca/src/coinjoin/util.cpp#L32-L34)) * When backporting [bitcoin#21166](bitcoin#21166), we don't have the luxury of a `scriptWitness` to include the redemption script, so `rpc_signrawtransaction.py` must import the script in order to spend it. This creates an additional complication as descriptor wallets do not permit wallets to mix descriptors with and without private keys ([source](https://github.com/dashpay/dash/blob/cc9da5d9e28bf9a7b411dc390523ac6d2dd09de6/src/wallet/rpc/backup.cpp#L1758)) and as P2SH descriptors don't involve a private key, those specific subtests need to be skipped for descriptor wallets. * `OP_TRUE` has been appended to the script, this mirrors the `OP_TRUE` in the `scriptWitness` ([source](https://github.com/bitcoin/bitcoin/blob/a97a9298cea085858e1a65a5e9b20d7a9e0f7303/test/functional/rpc_signrawtransaction.py#L270)) * The subtest `test_signing_with_missing_prevtx_info()` introduced in [bitcoin#25044](bitcoin#25044) requires an extra `walletpassphrase` as the preceding tests that unlock the wallet are not called for descriptor wallets, making the call necessary. It is redundant but harmless for legacy wallets. * With the backport of [bitcoin#19937](bitcoin#19937) in [dash#6726](#6726) (as ae7e4cb), we can remove a workaround in `feature_nulldummy.py` where we used two nodes to satisfy `getblocktemplate`'s requirement of a connected node as that requirement was lifted on test chains. ## Breaking Changes None expected. ## How Has This Been Tested? A full CoinJoin session run on c0f9225  ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK c0f9225 UdjinM6: utACK c0f9225 Tree-SHA512: 2217281ea75afe3eb6061e1acbd42afb66ff2ab28c15a3ad03bdb528ef4e40ea30bf1adad8d0ba93310a3d561d6c3f2ad95780b0b972af238523c6f2950a39c7
3 participants
Additional Information
Depends on backport: merge bitcoin#25228, #24941, #25356, #25435, #23017, #25522, #26414, partial bitcoin#24587, #24637, #24623, #25445 (implement better tx capabilities in
MiniWallet) #6725In bitcoin#24587,
checkbalances()inrpc_createmultisig.pyhad to be reworked as we have to exclude the 9 blocks generated by the test itself (excluding the original mining of 149 blocks), i.e. 2 blocks generated bydo_multisig()(source, source) (itself called 4 times, source) and 1 block generated bycheckbalances()(source)bitcoin#19937 is partially backported to avoid the need for reconnections (or nodes altogether) just to be able to call
getblocktemplate, as is done inmining_prioritisetransaction.py.Portions of bitcoin#25445 (fa04ff6) have been included when backporting bitcoin#24817 and bitcoin#25087 for correctness.
Portions of bitcoin#25435 (fa8421b) and bitcoin#25356 (fa779de) have been included when backporting bitcoin#24839 for correctness.
In bitcoin#26923,
rescan_utxos()has to be manually called (instead of it being called implicitly) as bitcoin#26886 has not been backported yet and is not included here due to its longer list of dependent PRs.In bitcoin#20456, v18 has been used to replace v0.15 as
getmempoolinfo["loaded"]was implemented in bitcoin#15323 (71e38b9 in dash#4609), which is included in v18.Additionally, bitcoin#26640 needs a more recent version of Dash Core to be tested against as v0.15 does not recognize
maxfeerateas an argument (build), which theMiniWalletchanges introduced by bitcoin#26640 rely on.The deletion of the default data directory to avoid problematic migration logic has been documented in the description on dash#6327
Breaking Changes
None expected. New field added in
scantxoutsetand changes ingetblocktemplatebehavior on test chains are not breaking.Checklist