backport: merge bitcoin#20039, #22363, #23079, #23120, #23118, #23866, #23558, #24035, #23873, #23978, #23836, #24223, #24533, #24605 (test backports: part 2) #6520
Conversation
The size in bytes of the three transactions used for testing reduces from 225 each to 85 each, so the size of memory pool will no longer be 674, it will be 255, which is 45 short of the minimum 300 expected by the test. So the minimum threshold has been reduced to 240 (assuming the transaction will at a minimum, consume 80 bytes).
tx2 will spend 20.999 tDASH instead of 20.99 tDASH as the remnant (0.01 tDASH) would result in a "Fee exceeds maximum configured by user" error. A 0.001 tDASH fee is permissible.
WalkthroughThe pull request introduces a comprehensive set of modifications across multiple test framework files, focusing on enhancing wallet functionality, script handling, and transaction management. The changes primarily involve:
The modifications span across various functional test files, test framework utilities, and core testing infrastructure. The changes aim to create a more modular, consistent, and maintainable approach to handling wallet interactions, transaction creation, and script generation in the Bitcoin test framework. By introducing utility functions and standardizing wallet operations, the pull request enhances the overall structure and clarity of the test suite. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (11)
test/functional/test_framework/wallet.py (2)
173-191: Implementation looks good; consider explicit error handling
The assertion checks the UTXO’s coverage. For clearer test feedback, raising an explicit error can be more descriptive than an assertion.
226-231: Consider exception handling
Any RPC errors fromsendrawtransactiongo unhandled here. Logging or intercepting the error may benefit debug scenarios.test/functional/test_framework/script_util.py (4)
42-42: Parameter shadowing
Consider renaminghashto a clearer identifier, e.g.keyhash, to avoid overshadowing built-in terminology.
47-47: Parameter naming
Same note as above: renamehashto avoid confusion with built-in functions.
64-64: Hex conversion
Wrapbytes.fromhex(key)in a try-except if you need better error reporting for invalid hex input.
72-72: Hex conversion incheck_script
Similar to above, consider a try-except for clearer error handling.test/functional/feature_maxuploadtarget.py (1)
15-20: Organize or remove unused imports as needed
It appears these imports are newly introduced or updated. Ensure they remain necessary for the updated logic. If any import is unused, consider removing it to keep the codebase clean.test/functional/feature_cltv.py (1)
60-60: Enhance comment clarity
Expanding the comment to reference(SEQUENCE_FINAL)is helpful. Ensure any other mention of non-final sequences is similarly updated for uniformity.test/functional/p2p_filter.py (1)
165-165: Block generation toward watch_script_pubkey
Generating a block to a watch-only script is a thorough test scenario. Confirm the wallet does not inadvertently recognize those outputs as spendable.test/functional/p2p_compactblocks.py (1)
60-61: Introduction of MiniWallet.Leveraging
MiniWalletstreamlines transaction generation in tests and reduces reliance on direct node interactions.test/functional/feature_block.py (1)
491-491: Redundant comments about redemption.There’s additional commentary explaining the script creation for P2SH in detail; ensure the multi-sig logic is thoroughly tested if further complexity is introduced.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
src/rest.cpp(9 hunks)test/functional/data/invalid_txs.py(6 hunks)test/functional/feature_addressindex.py(5 hunks)test/functional/feature_asset_locks.py(3 hunks)test/functional/feature_block.py(6 hunks)test/functional/feature_cltv.py(5 hunks)test/functional/feature_coinstatsindex.py(8 hunks)test/functional/feature_csv_activation.py(1 hunks)test/functional/feature_dersig.py(1 hunks)test/functional/feature_fee_estimation.py(2 hunks)test/functional/feature_maxuploadtarget.py(4 hunks)test/functional/feature_spentindex.py(3 hunks)test/functional/feature_txindex.py(2 hunks)test/functional/feature_utxo_set_hash.py(1 hunks)test/functional/interface_rest.py(7 hunks)test/functional/mempool_accept.py(10 hunks)test/functional/mempool_reorg.py(2 hunks)test/functional/mempool_spend_coinbase.py(1 hunks)test/functional/mining_basic.py(3 hunks)test/functional/p2p_blocksonly.py(1 hunks)test/functional/p2p_compactblocks.py(4 hunks)test/functional/p2p_filter.py(7 hunks)test/functional/rpc_generateblock.py(3 hunks)test/functional/rpc_scantxoutset.py(1 hunks)test/functional/rpc_signrawtransaction.py(1 hunks)test/functional/test_framework/blocktools.py(3 hunks)test/functional/test_framework/messages.py(2 hunks)test/functional/test_framework/script_util.py(2 hunks)test/functional/test_framework/util.py(1 hunks)test/functional/test_framework/wallet.py(6 hunks)test/functional/test_framework/wallet_util.py(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/functional/rpc_signrawtransaction.py
🧰 Additional context used
🪛 Ruff (0.8.2)
test/functional/test_framework/wallet.py
245-245: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
258-258: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
test/functional/test_framework/script_util.py
67-67: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
🔇 Additional comments (151)
test/functional/test_framework/wallet.py (5)
10-14: Imports look fine
No issues found with these import statements.
37-42: Script utility imports
All imported methods are relevant to script generation and usage inMiniWallet.
138-140: Newget_scriptPubKeymethod
Straightforward implementation returning the instance’s_scriptPubKey.
156-161: Handle potentialStopIteration
If no matching UTXO is found,next(utxo_filter)will raiseStopIteration. Consider catching and providing a descriptive error or fallback behavior.
192-194: Falling back toself._test_node
Usingfrom_node = from_node or self._test_nodeis concise. Ensure the docstring clarifies thatfrom_nodeis optional.test/functional/test_framework/util.py (1)
647-657: Logic for creating a large block
Creating multiple ~66 KB transactions to approach the 1 MB limit is a clear strategy. Implementation looks correct.test/functional/feature_txindex.py (2)
10-18: Import improvements
Imports are well organized for clarity.
56-56: Use ofkeyhash_to_p2pkh_script
Good choice for readability and consistency with other tests.test/functional/mempool_spend_coinbase.py (1)
43-43: Simplified call tocreate_self_transfer
Omitting thefrom_nodeparameter fits the updated default logic inMiniWallet.test/functional/test_framework/script_util.py (5)
6-14: Reformatted imports
No issues with the grouped imports.
37-40:key_to_p2pk_script
Implementation is straightforward and typical for P2PK scripts.
52-55:key_to_p2pkh_script
Useshash160(key)correctly before delegating tokeyhash_to_p2pkh_script. No issues here.
57-60:script_to_p2sh_script
Implementation is consistent and clear.
69-69: No further content
Nothing significant to address on this blank line.test/functional/feature_utxo_set_hash.py (1)
70-71: Confirm the updated expected UTXO set hash values.The new expected values for
hash_serialized_2andmuhashappear correct given the changes. However, please verify that the test environment is deterministic and the updated hashes match the final chain state across all relevant chain parameters.test/functional/test_framework/wallet_util.py (4)
19-22: Good use of utility functions for script creation.Importing and using
key_to_p2pkh_scriptandscript_to_p2sh_scriptis a welcome refactor, improving readability and reducing the likelihood of script-assembly errors.
44-44: Use ofkey_to_p2pkh_scriptis clear and concise.This matches the broader effort to standardize script creation, making the code easier to maintain. No issues noted.
57-57: Consistent refactor mirroring line 44.This reinforces the adoption of utility-based script creation, improving maintainability. Looks good.
73-73: Refactor toscript_to_p2sh_scriptis appropriate.Leveraging this helper function clarifies how the script is generated, reducing the risk of mistakes in manual script assembly.
test/functional/rpc_generateblock.py (8)
9-9: MiniWallet import improves consistency.Bringing in
MiniWalletaligns this file with the test framework’s standardized wallet approach. Good step towards unified transaction handling.
21-22: Initialize and rescan the MiniWallet.Creating and immediately rescanning the wallet is a sensible approach to ensure UTXOs are up to date. No issues found.
25-25: Usingminiwallet.get_address()to retrieve the destination.Switching from
node.getnewaddress()fosters consistency. This also centralizes wallet operations inMiniWallet.
47-47: Generating preemptive mempool transactions.Using
miniwallet.send_self_transferis straightforward. Ensure that enough UTXOs are available if more transactions are added in future expansions of the test.
50-50: Explicitly capturing thetxidfor block inclusion.Sending a self-transfer and passing the
txidensures clarity in verifying the correct transaction is mined. Well done.
57-59: Raw transaction creation is now simpler.Relying on
miniwallet.create_self_transfer()avoids manual raw transaction creation. Confirm no corner cases exist for large or custom scripts.
63-63: Asserting transaction retrieval bygetrawtransaction.This is a straightforward correctness check. No issues noted.
66-69: Verifying transaction ordering logic.The test ensures that an out-of-order transaction fails block validation. This looks correct and helps confirm proper consensus rules for test coverage.
test/functional/mempool_reorg.py (3)
48-50: Creating direct coinbase spends withcreate_self_transfer.This approach is simpler and consistent with the rest of the codebase. Confirm the node has enough coinbase UTXOs if test coverage expands.
73-73: Constructing a follow-up spend (spend_2_1).Ensuring the chain transaction is used as an input is correct. No issues spotted.
75-75: Similarly constructing spend_3_1.Maintains logical symmetry with
spend_2_1. This is reliable and helps ensure thorough coverage of reorg scenarios.test/functional/feature_spentindex.py (4)
12-18: Imports are consistent and aligned with project structure.These additions look good and ensure necessary classes (
COutPoint,CTransaction, etc.) are available. No issues found.
19-21: Clean and modular approach to script creation.Importing
keyhash_to_p2pkh_scripthelps maintain code clarity and reduce duplication. Good practice.
73-73: Use of keyhash_to_p2pkh_script for scriptPubKey.This change simplifies the creation of P2PKH scripts and is consistent with upstream modifications. No further action needed.
111-111: Consistent script creation for address2.Reusing
keyhash_to_p2pkh_scriptfor address2 ensures readability and maintainability. Looks good.test/functional/feature_dersig.py (1)
63-63: Removal of from_node parameter.Simplifies the method call by relying on default MiniWallet behavior. This is coherent with the other changes across the test suite.
test/functional/p2p_blocksonly.py (1)
118-118: Adoption of default node handling.Dropping
from_node=self.nodes[0]matches the approach of the other tests, making the code more uniform and concise.test/functional/data/invalid_txs.py (8)
31-31: Use of SEQUENCE_FINAL constant.Replacing hard-coded
0xffffffffimproves clarity and consistency. This is a good practice for readability.
34-39: Refined imports for script operations.Directly importing
OP_0,OP_CHECKSIG,OP_TRUEclarifies which opcodes are being used, improving readability.
40-42: Introduction of script_to_p2sh_script.Centralizing P2SH script creation is a maintainable approach. No issues identified.
66-66: SEQUENCE_FINAL used for valid_txin.Eliminates magic numbers and aligns with upstream coding style.
106-106: Replacing sc.CScript with a direct CScript import.Using
CScript([OP_TRUE])is consistent with the new import structure.
122-122: Nonexistent outpoint index practice remains intact.Using
SEQUENCE_FINALagain clarifies transaction finality semantics. Logical approach.
160-160: Nonexistent input refactor.Same beneficial shift from
0xfffffffftoSEQUENCE_FINALfor input creation. Good for consistency.
220-220: Use of OP_CHECKSIG list with direct imports.Enhances clarity, removing the previous
sc.alias. No issues found.test/functional/feature_maxuploadtarget.py (4)
23-27: Validate utility functions usage
You are importingmine_large_block,set_node_times, etc. Ensure they are used consistently and that the logic aligns with Dash’s chain parameters, as there could be differences compared to Bitcoin’s constants.
67-68: Good use of MiniWallet for testing
InstantiatingMiniWallethere and immediately generating blocks demonstrates a consistent approach to wallet-based block generation. Looks solid.
80-80: Mine large block usage
Callingmine_large_block(self, self.wallet, self.nodes[0])is convenient. Just confirm that these large blocks still meet the typical acceptance rules in Dash (e.g., block size limits).
91-91: Maintain consistency with test naming
Continuing to callmine_large_block(self, self.wallet, self.nodes[0])for the second block is consistent. Be sure that tests remain stable under varying node settings (like custom blockmaxsize).test/functional/feature_cltv.py (4)
16-16: Named constant usage
The introduction ofSEQUENCE_FINALclarifies the previously hardcoded 0xffffffff. This improves readability and aids in maintaining the code.
69-69: Explicit usage of SEQUENCE_FINAL
Replacing the literal0xffffffffwithSEQUENCE_FINALimproves maintainability. Check any other references to ensure consistency across the codebase.
123-123: Creating self-transfer transactions
Usingwallet.create_self_transfer()is a standard way to produce test transactions. Confirm that each call precisely matches your intended test scenario for invalid states.
158-158: Non-mandatory script verify checks
Here again, you’re constructing an invalid CLTV transaction. The approach is consistent with the earlier logic. Ensure each scenario covers unique code paths and properly triggers the intended error.test/functional/p2p_filter.py (12)
11-11: Double-check coin usage
UsingCOINas a multiplier for amounts is standard practice. Confirm that the amounts align with Dash’s fee and coinbase rules if they differ from Bitcoin.
32-35: Introduction of MiniWallet and getnewdestination
Importing and usingMiniWalletplusgetnewdestinationis a good approach. Confirm that the new methods handle all relevant address formats as expected for Dash.
40-40: Using bytes.fromhex for script
Switching to a bytes representation for the watch-only script is more explicit. Confirm that the script is recognized by the node as intended.
101-103: Adding generatetoscriptpubkey method
This helper method simplifies specialized block generation. Looks useful for any watch-only or custom scriptPubKey testing.
139-139: Replacing direct calls with MiniWallet's send_to
Usingwallet.send_tocentralizes transaction handling. Check that the logic for mempool confirmation and wallet UTXO updates remains robust.
150-150: No duplication in test coverage
The second usage ofwallet.send_toensures coverage for different states. Good approach for verifying relay behavior with fRelay set to false initially.
172-172: Check for different script destinations
Usinggetnewdestination()for block generation is helpful to confirm no watch output is matched by the filter.
179-179: Test coverage for non-matching mempool transaction
Confirming the filter doesn’t receive a merkleblock or a transaction is a valuable negative case.
186-186: Matching mempool transaction
Good job verifying that the watch-script triggers atx_receivedevent. Confirms the bloom filter test is working.
193-193: Transaction spamming
Looping multiple times ensures repeated coverage. Also ensures the filter is cleared, letting the transaction get relayed again.
200-200: Verify ignoring filtered blocks with no filter
The test checks that notxormerkleblockis sent. Ensure this logic remains consistent if code changes to fully rejectgetdataforMSG_FILTERED_BLOCKin such cases.
216-218: Initializing MiniWallet and rescanning
Setting up the wallet at test startup is consistent with other test patterns. This helps ensure the test environment is primed with scanned UTXOs.test/functional/mining_basic.py (3)
31-32: Adopting MiniWallet
ImportingMiniWalletis a clean approach for test-driven block creation. This aligns with the broader test suite changes to standardize transaction handling.
57-57: Generating blocks via the wallet
Switching toself.generate(self.wallet, ...)ensures block creation is always associated with test-driven UTXOs. Good synergy with the rest of the test.
76-77: Initialize wallet, then mine chain
Instantiating the wallet before mining is consistent, ensuring the wallet is ready. The straightforward call order makes the logic easy to follow.test/functional/test_framework/blocktools.py (5)
21-21: Use of a descriptive constant instead of a magic number is good practice
Replacing0xffffffffwithSEQUENCE_FINALimproves readability and helps avoid confusion down the line.
26-34: Consistent imports for script utilities
ImportingOP_TRUE,CScript,CScriptNum,CScriptOp, andkey_to_p2pk_scriptprovides clarity about dependencies and centralizes script-related functionality.
184-184: Further clarity withSEQUENCE_FINAL
UsingSEQUENCE_FINALin coinbase inputs clarifies the intent of a non-lock-timed input instead of relying on a magic number, aligning with best practices.
191-191: Improved readability through helper function
Replacing a script snippet withkey_to_p2pk_script(pubkey)properly encapsulates the creation logic. This approach aids maintainability.
212-212: Consistent application ofSEQUENCE_FINAL
It’s good to see the same descriptive constant used across the codebase, preventing mix-ups and ensuring uniform transaction creation semantics.test/functional/feature_coinstatsindex.py (8)
19-19: Clarity in coinbase maturity usage
ExportingCOINBASE_MATURITYfromblocktoolskeeps the value consistent. Ensuring tests rely on a single definition helps avoid mismatch errors.
35-39: Modular transaction creation via MiniWallet
IntroducingMiniWalletand generating addresses withgetnewdestinationprovides a tidier interface for sending transactions, reducing boilerplate.
85-86: Appropriate block generation and self-transfer
Generating blocks up toCOINBASE_MATURITY + 1and then usingsend_self_transferis consistent with test coverage for spendable coinbase outputs. The approach looks correct.
153-154: Testing coinbase amounts with decimals
Including decimal checks ensures precision is handled correctly. Verify that any future changes (e.g., halving logic) remain in sync with these test values.
165-169: Ensuring correct parameter usage forsend_to
Passing bothscriptPubKeyandamounttosend_topreserves clarity in test transactions. This pattern aids maintainability.
175-176: Manually setting outputs for test coverage
Manually constructing the transaction’svoutwith an OP_RETURN output is a robust way to test unspendable script behavior.
186-196: Verifying unspendable stats
Asserting the correctness oftotal_unspendable_amount,unclaimed_rewards, and other coin stats is key to ensuring the index captures maximum coverage. This is a well-structured test.
217-217: Accurate test of final unspendable total
Checking the updated total (Decimal('530.99900000')) after custom coinbase amounts and fees ensures correctness of indexing logic.test/functional/feature_fee_estimation.py (2)
22-24: Refactoring P2SH script creation
Usingscript_to_p2sh_scriptsimplifies code and removes the need for manual hashing logic. This approach is more readable and reduces room for errors.
40-41: Consistent application of the new functions
DefiningP2SH_1andP2SH_2viascript_to_p2sh_scripthelps centralize P2SH creation. The code becomes more maintainable and less error-prone.test/functional/rpc_scantxoutset.py (6)
6-9: Sensible imports for testing
Bringing inCOIN,BitcoinTestFramework,assert_equal, andassert_raises_rpc_errorensures the test has all necessary building blocks.
10-13: Moving to improved wallet APIs
AdoptingMiniWallet,address_to_scriptpubkey, andgetnewdestinationin a single place keeps code consistent and easier to follow.
25-29: Generic method for sending to either string addresses or scriptPubKey
sendtodestinationcleanly interprets whether the input is a string address or a raw scriptPubKey, avoiding code duplication and potential confusion.
32-34: Initializing MiniWallet
Creating and rescanning the wallet at the beginning of the test is consistent with how these functional tests are structured.
35-55: Systematic testing of multiple addresses
Sending various amounts to different addresses is beneficial for scanning coverage. Any break in derivations or address-specific logic is more likely to be caught.
68-72: Expanded coverage for different descriptor types
Scanning usingpkh,combo, and specificaddr(...)checks ensures broad coverage of address handling in the UTXO set. This thorough approach strengthens reliability.test/functional/feature_addressindex.py (6)
10-16: Imports look fine.
These lines cleanly group message-related classes and constants, aiding readability. No further issues noted.
19-22: No issues with these imports.
Loading script utility functions here appears consistent with the codebase's approach.
139-139: Appropriate utility usage.
scripthash_to_p2sh_scriptimproves clarity in P2SH script creation.
165-165: Correct utility call.
keyhash_to_p2pkh_scriptis used properly to create a standard P2PKH script.
257-257: Key-hash script creation is valid.
No concerns regarding the usage ofkeyhash_to_p2pkh_script.
259-259: P2SH script generation.
scripthash_to_p2sh_scriptfollows expected usage patterns.test/functional/mempool_accept.py (17)
7-7: Importingdeepcopy.
This import is standard and needed for duplicating transaction outputs.
21-21: Explicit import ofSEQUENCE_FINAL.
This constant clarifies BIP-68-related final sequence usage. No issues found.
33-35:script_to_p2sh_scriptfunction usage.
This utility function furthers clarity when generating P2SH scripts.
40-40: MiniWallet import.
IntroducingMiniWalletinto tests simplifies transaction flow and is consistent with the broader test refactor.
59-60: Instantiating and rescanning wallet.
Creating the MiniWallet instance and invokingrescan_utxos()ensures fresh unspent data.
74-78: Transaction creation from UTXOs.
Appending a duplicate output is valid for testing. Just confirm final outputs match fee logic.
89-92: UTXO selection for new transaction.
Extracting the 0.3 BTC UTXO is logically consistent with the subsequent fee deduction.
95-95:testmempoolacceptresult validation.
Ensures acceptance and correct fees. Nicely verifies the mempool logic for the newly created transaction.
100-106: Locktime usage & output value adjustment.
Settingsequence=SEQUENCE_FINALand customizing output matches the test scenario for finalizing transactions.
108-108: Computed fee validation.
fee_expected = Decimal('500.0') - output_amountclarifies the test's large input scenario.
110-110: Assertion on mempool acceptance.
Explicitly verifyingfees.baseensures correct fee calculation during acceptance.
129-129: Rejecting lower-fee replacement.
Since Dash defaults to no RBF, marking as mempool conflict is expected.
160-160: Spending previously valid outpoint.
Ensures coverage for a missing-input scenario.
163-169: Combining multiple inputs.
Creating a transaction to spend both UTXOs thoroughly tests the “missing-inputs” rejection logic for old TXIDs.
182-189: Constructing reference transaction.
Storing a baseline transaction for subsequent tests fosters clarity and code reuse.
Line range hint
302-308: Chained transactions in the mempool.
Loop logic is straightforward, verifying BIP125-like replacement handling in a Dash context (albeit RBF is absent).
Line range hint
314-315: Mempool size checks.
Ensures total bytes persist above an expected threshold. Useful for verifying changes to transaction size.test/functional/interface_rest.py (16)
9-9: HTTP client import.
Required for REST calls. No issues noted.
15-15: Blank line insertion.
Likely improves readability or separates logic.
16-19: Messages import.
Fetching constants likeBLOCK_HEADER_SIZEandCOIN. Consistent with the test usage.
26-29: MiniWallet and destination utils.
Highlights the shift to a wallet-based approach for transaction creation.
55-57: Appending whitelist arguments.
Ensures faster test relay in a local environment.
88-89: MiniWallet instantiation and rescan.
Ensuring we begin with known UTXOs in the test environment.
91-92: Sending test transaction & syncing.
Ensures coverage for both successful broadcast and REST-based inspection.
118-118: Generating blocks.
Used here to confirm chain tip correctness before further tests.
Line range hint
139-139: Spent TXO reference creation.
Capturing vin to verify subsequentgetutxoslogic.
162-162: Ensuring block count is 201.
This checks chain progression after coin issuance.
170-170: Unconfirmed transaction test.
Ensures mempool presence is selectively included in REST queries.
284-284: Fetching blockfilter headers.
Part of the index usage test. No concerns.
286-291: Comparisons with RPC blockfilter.
Cross-validates REST response with standard RPC. Great for consistency checks.
296-296: Number parsing validation.
Rejecting invalid or out-of-bounds header counts. Good user input sanity check.
302-308: Creating chained transactions & verifying mempool.
Tests multi-step transaction acceptance thoroughly.
314-315: Byte-size threshold check.
Verifies that the mempool holds enough total data for the chained transactions.test/functional/feature_csv_activation.py (1)
113-113: Simplified self-transfer call.
Removingfrom_nodealigns with the updated MiniWallet defaults. No issues.src/rest.cpp (10)
197-197: Improvement in error message clarity.Changing the phrase to “Header count is invalid or out of acceptable range” makes the error clearer to end users.
345-346: Consolidated warmup check into a single line.This is a minor refactor that slightly reduces vertical space and keeps the check more concise.
372-372: Error response refinement.Error response for an invalid header count remains consistent with similar checks elsewhere in the file, ensuring uniform user feedback.
375-375: Vector initialization clarity.Using reserve and a named variable (
parsed_count) for header count clarifies the allocation and iteration process, reducing potential confusion.
396-396: Index readiness check.This condition ensures filters are properly indexed before usage. Useful for preventing unexpected index corruption states.
414-414: Use of brace initialization for CDataStream.Adopting uniform brace initialization aligns with modern C++ style and improves code consistency.
425-425: Hex encoding consistency.Maintaining uniform usage of HexStr for block filter headers is consistent with the project’s coding patterns.
454-455: Flattened warmup check.The one-line check is consistent with the style used in similar functions; it also improves readability by reducing line breaks.
513-513: Binary block filter serialization style.Using braces
{SER_NETWORK, PROTOCOL_VERSION}consistently matches the style used throughout the file for stream initialization.
522-522: Hex block filter serialization.HexStr usage remains aligned with other hex-encoding sections, preserving consistency in REST responses.
test/functional/feature_asset_locks.py (3)
34-37: Refactor to modular script creation.Replacing
CScript([pubkey, OP_CHECKSIG])directly withkey_to_p2pk_script(pubkey)clarifies the script’s intent and ensures consistency with other script utility usage.
81-81: Output assignment logic.Assigning
remainingas the leftover after subtracting the asset lock amount is straightforward. Ensure the usage ofif remaining > 0 else ...is tested for edge cases (e.g., exact amounts).
98-98: Fee assignment protection.Adding the
feeparameter helps avoid magic constants in code and provides flexibility if future fee changes are needed.test/functional/p2p_compactblocks.py (2)
157-157: Shifting to generated UTXOs.Generating additional UTXOs with
self.walletis more robust and consistent with the new approach that usesMiniWallet.
284-284: Calls tosend_self_transfer.Switching from
node.sendtoaddressto the wallet’ssend_self_transfermethod fosters code cohesion and more centralized wallet logic.test/functional/feature_block.py (6)
24-24: Utilization ofSEQUENCE_FINAL.Replacing the hardcoded
0xffffffffwithSEQUENCE_FINALclarifies usage and improves maintainability.
47-49: Refinement of script utility imports.Replacing
hash160withscript_to_p2sh_scriptpromotes a more direct, descriptive approach to P2SH script construction.
52-55: Enhanced assertions.The addition of
assert_greater_thanimproves lucid checks relative toSEQUENCE_FINAL, increasing test clarity and coverage.
819-819: Sequence usage for invalid outpoints.Setting
SEQUENCE_FINALemphasizes finality. For clarity, consider confirming that the transaction is indeed being flagged due to a non-existent outpoint rather than sequence details.
894-894: Non-final locktime scenario.The explicit comparison of
nSequencewithSEQUENCE_FINALclarifies the test intent for non-final transactions. Ensure corner cases (e.g., partial final sequences) are also covered in tests.
1042-1042: Clarity in bogus transaction handling.Explicitly referencing a bogus transaction hash ensures the test scenario is unambiguous. This helps confirm the correct rejection path.
test/functional/test_framework/messages.py (2)
44-44: Use a standardized constant for final sequence.
DefiningSEQUENCE_FINAL = 0xffffffffis good for clarity and maintainability.
513-513: Ensure comprehensive coverage for the new default transaction version.
Changing the default transaction version to 2 is consistent with backports, but verify that tests and other code segments expectingnVersion=1are properly updated.Below is a script to locate any references to
CTransaction()ornVersion=1across the codebase for further validation:✅ Verification successful
Change to default transaction version 2 is safe and properly covered by tests
The codebase shows comprehensive version handling:
- Transaction validation explicitly allows versions 1-3
- Tests that require version 1 explicitly set it rather than relying on defaults
- No implicit dependencies on version 1 were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to CTransaction() and nVersion=1 across the project rg -A 3 -B 3 $'CTransaction\\(' rg -A 3 -B 3 $'nVersion=1'Length of output: 68339
Script:
#!/bin/bash # Search for transaction version comparisons and checks rg -A 3 -B 3 'nVersion [=<>!]' # Search for test assertions involving transaction versions rg -A 3 -B 3 'BOOST_CHECK.*version|assert.*version' test/Length of output: 66169
| def get_descriptor(self): | ||
| return descsum_create(f'raw({self._scriptPubKey.hex()})') | ||
|
|
||
| def get_address(self): | ||
| return self._address | ||
|
|
||
| def get_utxo(self, *, txid: Optional[str]='', mark_as_spent=True): | ||
| def get_utxo(self, *, txid: str = '', vout: Optional[int] = None, mark_as_spent=True): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add docstring for new vout parameter
Please update the function’s docstring to document the vout argument and its usage.
|
|
||
| def address_to_scriptpubkey(address): | ||
| """Converts a given address to the corresponding output script (scriptPubKey).""" | ||
| payload, version = base58_to_byte(address) | ||
| if version == 140: # testnet pubkey hash | ||
| return keyhash_to_p2pkh_script(payload) | ||
| elif version == 19: # testnet script hash | ||
| return scripthash_to_p2sh_script(payload) | ||
| # TODO: also support other address formats | ||
| else: | ||
| assert False | ||
|
|
There was a problem hiding this comment.
Don’t use assert False for unsupported address formats
Same rationale as above; replace it with an explicit exception.
- assert False
+ raise AssertionError(f"Unsupported address format: version={version}")📝 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.
| def address_to_scriptpubkey(address): | |
| """Converts a given address to the corresponding output script (scriptPubKey).""" | |
| payload, version = base58_to_byte(address) | |
| if version == 140: # testnet pubkey hash | |
| return keyhash_to_p2pkh_script(payload) | |
| elif version == 19: # testnet script hash | |
| return scripthash_to_p2sh_script(payload) | |
| # TODO: also support other address formats | |
| else: | |
| assert False | |
| def address_to_scriptpubkey(address): | |
| """Converts a given address to the corresponding output script (scriptPubKey).""" | |
| payload, version = base58_to_byte(address) | |
| if version == 140: # testnet pubkey hash | |
| return keyhash_to_p2pkh_script(payload) | |
| elif version == 19: # testnet script hash | |
| return scripthash_to_p2sh_script(payload) | |
| # TODO: also support other address formats | |
| else: | |
| raise AssertionError(f"Unsupported address format: version={version}") |
🧰 Tools
🪛 Ruff (0.8.2)
258-258: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
| def getnewdestination(address_type='legacy'): | ||
| """Generate a random destination of the specified type and return the | ||
| corresponding public key, scriptPubKey and address. Supported types are | ||
| 'legacy'. Can be used when a random destination is needed, but no | ||
| compiled wallet is available (e.g. as replacement to the | ||
| getnewaddress/getaddressinfo RPCs).""" | ||
| key = ECKey() | ||
| key.generate() | ||
| pubkey = key.get_pubkey().get_bytes() | ||
| if address_type == 'legacy': | ||
| scriptpubkey = key_to_p2pkh_script(pubkey) | ||
| address = key_to_p2pkh(pubkey) | ||
| else: | ||
| assert False | ||
| return pubkey, scriptpubkey, address | ||
|
|
There was a problem hiding this comment.
Avoid assert False
Replace it with a regular exception to handle unsupported address types.
- assert False
+ raise AssertionError(f"Unsupported address type: {address_type}")📝 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.
| def getnewdestination(address_type='legacy'): | |
| """Generate a random destination of the specified type and return the | |
| corresponding public key, scriptPubKey and address. Supported types are | |
| 'legacy'. Can be used when a random destination is needed, but no | |
| compiled wallet is available (e.g. as replacement to the | |
| getnewaddress/getaddressinfo RPCs).""" | |
| key = ECKey() | |
| key.generate() | |
| pubkey = key.get_pubkey().get_bytes() | |
| if address_type == 'legacy': | |
| scriptpubkey = key_to_p2pkh_script(pubkey) | |
| address = key_to_p2pkh(pubkey) | |
| else: | |
| assert False | |
| return pubkey, scriptpubkey, address | |
| def getnewdestination(address_type='legacy'): | |
| """Generate a random destination of the specified type and return the | |
| corresponding public key, scriptPubKey and address. Supported types are | |
| 'legacy'. Can be used when a random destination is needed, but no | |
| compiled wallet is available (e.g. as replacement to the | |
| getnewaddress/getaddressinfo RPCs).""" | |
| key = ECKey() | |
| key.generate() | |
| pubkey = key.get_pubkey().get_bytes() | |
| if address_type == 'legacy': | |
| scriptpubkey = key_to_p2pkh_script(pubkey) | |
| address = key_to_p2pkh(pubkey) | |
| else: | |
| raise AssertionError(f"Unsupported address type: {address_type}") | |
| return pubkey, scriptpubkey, address |
🧰 Tools
🪛 Ruff (0.8.2)
245-245: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
|
|
||
| def check_key(key): | ||
| if isinstance(key, str): | ||
| key = bytes.fromhex(key) # Assuming this is hex string | ||
| key = bytes.fromhex(key) # Assuming this is hex string | ||
| if isinstance(key, bytes) and (len(key) == 33 or len(key) == 65): | ||
| return key | ||
| assert False |
There was a problem hiding this comment.
Replace assert False
Raise an explicit AssertionError or custom exception with a descriptive message.
🧰 Tools
🪛 Ruff (0.8.2)
67-67: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
1646525 ci: add nowallet functional tests (Konstantin Akimov) Pull request description: Many functional tests can be run if Dash Core is built as no-wallet, but we currently do not test whether they actually work in no-wallet mode. Additionally, many functional tests can run with a simple dummy wallet, increasing the number of functional tests that can run without a wallet. However, we also do not test this scenario. Now that extra functional tests have become available to run with the dummy-wallet MiniWallet (see #6520), we should finally add a job to our CI pipeline to run functional tests in no-wallet mode. Though using MiniWallet for DashTestFramework to let register masternodes without using a real Dash Core wallet currently impossible due to limitation of MiniWallet: Note that this method fails if there is no single internal utxo available that can cover the cost for the amount and the fixed fee (the utxo with the largest value is taken). This limitation may be addressed in future PRs, but it is out of scope for this PR. ## What was done? Add new CI job to run functional tests for nowallet. ## How Has This Been Tested? See CI: https://gitlab.com/dashpay/dash/-/jobs/8805673945 ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 1646525 kwvg: utACK 1646525 PastaPastaPasta: utACK 1646525 Tree-SHA512: c93ef92670886d3a5cf835681b55c264c757005c4af3146355e78808ab7e22fee30e5559218808bd381eb0a59bc01cc0ac69ce89dfd2d659e533392c33ac0a6a
Additional Information
While bitcoin#22363 implements helpers for P2{WPKH,WSH}, this has been omitted on account of Dash not supporting SegWit. Likewise, logic related to RBF and Taproot have been likewise omitted in other backports (e.g. the changes extracted from bitcoin#22998 as 473620f).
In bitcoin#23866,
getnewdestinationwill only generatelegacyaddresses (also the default type of address) on account of not supportingbech32addresses or SegWit.address_to_scriptpubkey(), the version bits were taken from testnet chain parameters (source)The backport of bitcoin#23079 implements
random_p2pkh()(as opposed torandom_p2wpkh()upstream) on account of Dash not supporting SegWit.The trio of transactions generated in
interface_rest.pyare 225 bytes each, which helps clear the "mempool consumes at least 300 bytes" assertion (source).But bitcoin#24223 trims the transaction size down to 85 bytes each. To ensure the test passes, the assertion threshold has been reduced to 240 (assuming three transactions consume at least 80 bytes each).
In bitcoin#24605,
tx2will spend 20.999 tDASH instead of 2.99 tBTC expected upstream. This is due to Dash having a lower feerate and the test unmodified will trigger a "Fee exceeds maximum configured by user" error. Known-good values affected by this change have been updated accordingly.Breaking Changes
None expected.
Checklist