backport: 0.24 batch 212 #6747
backport: 0.24 batch 212 #6747
Conversation
|
This pull request has conflicts, please rebase. |
PastaPastaPasta
force-pushed
the
backport-0.24-batch-212
branch
from
0129f91 to
3f6cd9b
Compare
WalkthroughThis set of changes introduces a new wallet RPC command, 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (61)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (15)
🚧 Files skipped from review as they are similar to previous changes (43)
🧰 Additional context used📓 Path-based instructions (1)test/functional/**/*.pyInstructions used from: Sources:
🧠 Learnings (2)📓 Common learningstest/functional/rpc_net.py (1)🧬 Code Graph Analysis (1)test/functional/rpc_net.py (1)
🪛 Ruff (0.12.2)test/functional/rpc_net.py202-202: Function definition does not bind loop variable (B023) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ 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: 0
🧹 Nitpick comments (1)
src/wallet/wallet.cpp (1)
2755-2760: Fix grammatical error in error message.The error handling logic for
DBErrors::UNKNOWN_DESCRIPTORis correctly implemented and follows the established pattern. However, there's a grammatical error in the error message.- error = strprintf(_("Unrecognized descriptor found. Loading wallet %s\n\n" - "The wallet might had been created on a newer version.\n" - "Please try running the latest software version.\n"), walletFile); + error = strprintf(_("Unrecognized descriptor found. Loading wallet %s\n\n" + "The wallet might have been created on a newer version.\n" + "Please try running the latest software version.\n"), walletFile);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/rpc/client.cpp(1 hunks)src/rpc/mining.cpp(0 hunks)src/rpc/util.cpp(1 hunks)src/wallet/load.cpp(1 hunks)src/wallet/rpc/wallet.cpp(2 hunks)src/wallet/wallet.cpp(1 hunks)src/wallet/walletdb.cpp(5 hunks)src/wallet/walletdb.h(1 hunks)test/functional/mempool_accept.py(1 hunks)test/functional/p2p_leak.py(2 hunks)test/functional/rpc_estimatefee.py(1 hunks)test/functional/rpc_getblockfrompeer.py(1 hunks)test/functional/rpc_getdescriptorinfo.py(1 hunks)test/functional/rpc_rawtransaction.py(2 hunks)test/functional/test_framework/util.py(1 hunks)test/functional/test_runner.py(4 hunks)test/functional/wallet_resendwallettransactions.py(2 hunks)test/functional/wallet_simulaterawtx.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/rpc/mining.cpp
🧰 Additional context used
🧠 Learnings (15)
📓 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.
Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Learnt from: kwvg
PR: dashpay/dash#6726
File: test/functional/rpc_createmultisig.py:120-120
Timestamp: 2025-06-20T23:32:16.225Z
Learning: In the rpc_createmultisig.py test, the checkbalances() function correctly excludes 9 blocks from the balance calculation: 8 blocks from do_multisig() calls (2 blocks per call × 4 calls) plus 1 block from checkbalances() itself.
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
PR: dashpay/dash#6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Learnt from: kwvg
PR: dashpay/dash#6729
File: src/evo/deterministicmns.cpp:1313-1316
Timestamp: 2025-07-09T15:02:26.853Z
Learning: In Dash's masternode transaction validation, `IsVersionChangeValid()` is only called by transaction types that update existing masternode entries (like `ProUpServTx`, `ProUpRegTx`, `ProUpRevTx`), not by `ProRegTx` which creates new entries. This means validation logic in `IsVersionChangeValid()` only applies to the subset of transaction types that actually call it, not all masternode transaction types.
test/functional/mempool_accept.py (1)
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
test/functional/rpc_getdescriptorinfo.py (1)
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
test/functional/rpc_getblockfrompeer.py (4)
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
PR: dashpay/dash#6726
File: test/functional/rpc_createmultisig.py:120-120
Timestamp: 2025-06-20T23:32:16.225Z
Learning: In the rpc_createmultisig.py test, the checkbalances() function correctly excludes 9 blocks from the balance calculation: 8 blocks from do_multisig() calls (2 blocks per call × 4 calls) plus 1 block from checkbalances() itself.
Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Learnt from: UdjinM6
PR: dashpay/dash#6553
File: src/chainparams.cpp:221-225
Timestamp: 2025-02-04T14:35:54.261Z
Learning: Block hashes should be verified without the '0x' prefix when using block explorer APIs.
src/wallet/load.cpp (2)
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.
Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
test/functional/rpc_estimatefee.py (1)
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
test/functional/wallet_resendwallettransactions.py (1)
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
test/functional/rpc_rawtransaction.py (2)
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
PR: dashpay/dash#6726
File: test/functional/rpc_createmultisig.py:120-120
Timestamp: 2025-06-20T23:32:16.225Z
Learning: In the rpc_createmultisig.py test, the checkbalances() function correctly excludes 9 blocks from the balance calculation: 8 blocks from do_multisig() calls (2 blocks per call × 4 calls) plus 1 block from checkbalances() itself.
src/rpc/client.cpp (3)
Learnt from: kwvg
PR: dashpay/dash#6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Learnt from: kwvg
PR: dashpay/dash#6530
File: src/validation.cpp:360-362
Timestamp: 2025-01-14T08:37:16.955Z
Learning: The UpdateTransactionsFromBlock() method in txmempool.cpp takes parameters in the order: vHashUpdate, ancestor_size_limit, ancestor_count_limit. The size limit comes before the count limit.
src/wallet/wallet.cpp (2)
undefined
<retrieved_learning>
Learnt from: kwvg
PR: #6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The GetWallet() function calls in src/wallet/rpcwallet.cpp are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
</retrieved_learning>
<retrieved_learning>
Learnt from: kwvg
PR: #6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
</retrieved_learning>
src/wallet/rpc/wallet.cpp (1)
Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
src/rpc/util.cpp (1)
Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
test/functional/test_runner.py (1)
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
test/functional/wallet_simulaterawtx.py (4)
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Learnt from: kwvg
PR: dashpay/dash#6726
File: test/functional/rpc_createmultisig.py:120-120
Timestamp: 2025-06-20T23:32:16.225Z
Learning: In the rpc_createmultisig.py test, the checkbalances() function correctly excludes 9 blocks from the balance calculation: 8 blocks from do_multisig() calls (2 blocks per call × 4 calls) plus 1 block from checkbalances() itself.
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.
src/wallet/walletdb.cpp (3)
Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Learnt from: kwvg
PR: dashpay/dash#6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Learnt from: kwvg
PR: dashpay/dash#6530
File: src/validation.cpp:4514-4516
Timestamp: 2025-01-14T08:40:00.994Z
Learning: In Dash Core, error() is used (instead of state.Invalid()) to report critical failures that require immediate user intervention, particularly for database/disk errors and data corruption scenarios. This is an established pattern seen in validation.cpp.
🧬 Code Graph Analysis (8)
test/functional/mempool_accept.py (1)
test/functional/test_framework/util.py (1)
assert_raises_rpc_error(131-147)
test/functional/rpc_getblockfrompeer.py (2)
test/functional/test_framework/util.py (1)
assert_raises_rpc_error(131-147)src/rpc/blockchain.cpp (2)
getblockfrompeer(481-528)getblockfrompeer(481-481)
src/wallet/load.cpp (2)
src/wallet/interfaces.cpp (2)
scheduler(602-602)scheduler(602-602)src/wallet/wallet.cpp (2)
MaybeResendWalletTxs(1898-1903)MaybeResendWalletTxs(1898-1898)
test/functional/rpc_estimatefee.py (1)
test/functional/test_framework/util.py (1)
assert_raises_rpc_error(131-147)
test/functional/rpc_rawtransaction.py (1)
test/functional/test_framework/util.py (1)
assert_raises_rpc_error(131-147)
src/wallet/rpc/wallet.cpp (4)
src/wallet/rpc/util.cpp (4)
GetWalletForJSONRPCRequest(55-78)GetWalletForJSONRPCRequest(55-55)ParseIncludeWatchonly(34-43)ParseIncludeWatchonly(34-34)src/wallet/wallet.h (5)
wallet(404-404)LOCK(818-818)filter(761-761)txin(790-790)txout(791-791)src/wallet/interfaces.cpp (14)
LOCK(241-250)LOCK(251-254)LOCK(277-281)LOCK(282-286)LOCK(342-351)LOCK(469-481)txin(449-453)txin(449-449)txin(459-463)txin(459-459)txout(454-458)txout(454-454)txout(464-468)txout(464-464)src/core_io.h (1)
DecodeHexTx(37-37)
src/rpc/util.cpp (1)
src/univalue/include/univalue.h (1)
uvTypeName(211-211)
test/functional/wallet_simulaterawtx.py (4)
test/functional/test_framework/test_framework.py (3)
skip_if_no_wallet(1055-1063)no_op(795-796)sync_all(868-870)test/functional/test_framework/util.py (3)
assert_approx(31-40)assert_equal(68-73)assert_raises_rpc_error(131-147)src/wallet/rpc/wallet.cpp (2)
simulaterawtransaction(912-1021)simulaterawtransaction(912-912)test/functional/test_framework/test_node.py (1)
get_wallet_rpc(360-366)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: arm-linux-build / Build source
🔇 Additional comments (38)
src/wallet/load.cpp (1)
155-155: LGTM! Performance optimization for wallet transaction rebroadcasting.Changing the interval from 1 second to 1 minute is a sensible optimization that reduces unnecessary resource usage while maintaining adequate rebroadcast functionality.
test/functional/test_framework/util.py (1)
33-36: LGTM! Good enhancement for decimal precision in tests.Adding Decimal type handling ensures consistent high-precision arithmetic when comparing decimal values, which is important for financial calculations in wallet tests.
test/functional/wallet_resendwallettransactions.py (3)
31-35: LGTM! Test correctly updated for new rebroadcast timing.The comments and mockscheduler call are properly updated to reflect the change from 1 second to 60 seconds for wallet transaction rebroadcasting.
64-64: LGTM! Mockscheduler call updated for new timing.Correctly changed from 1 second to 60 seconds to match the new rebroadcast interval.
72-73: LGTM! Comments and timing updated consistently.Both the comment and mockscheduler call are properly updated to reflect the new 60-second interval for wallet transaction rebroadcasting.
test/functional/mempool_accept.py (1)
66-66: LGTM! Error message updated for new RPC format.The test correctly updates the expected error message to match the new, more explicit JSON type error format in the RPC system.
test/functional/rpc_getdescriptorinfo.py (1)
32-32: LGTM! Error message updated for new RPC format.The test correctly updates the expected error message to match the new, more explicit JSON type error format that specifies both the actual type (number) and expected type (string).
src/wallet/walletdb.h (1)
57-60: LGTM: Clean enum extension for improved error handling.The addition of
NEED_RESCANandUNKNOWN_DESCRIPTORerror codes provides better granularity for wallet database error handling, particularly for cases involving unknown wallet descriptors during wallet loading.src/rpc/util.cpp (1)
53-54: LGTM: Improved RPC error message clarity.The error message format change from
"Expected type %s, got %s"to"JSON value of type %s is not of expected type %s"provides more explicit and user-friendly error reporting. The new format clearly identifies the nature of the error (JSON value type mismatch) and presents the actual type before the expected type, which is more intuitive for debugging.test/functional/rpc_getblockfrompeer.py (1)
54-55: LGTM: Test assertions updated to match new RPC error format.The error message assertions have been correctly updated to reflect the new RPC error format from
src/rpc/util.cpp. The test logic remains unchanged while properly validating the improved error messages.test/functional/rpc_rawtransaction.py (2)
198-198: LGTM: Test assertion updated for new RPC error format.The error message assertion has been correctly updated to reflect the new RPC error format for invalid
inputsparameter type validation increaterawtransaction.
239-239: LGTM: Test assertion updated for new RPC error format.The error message assertion has been correctly updated to reflect the new RPC error format for invalid
locktimeparameter type validation increaterawtransaction.test/functional/rpc_estimatefee.py (3)
25-26: LGTM: Test assertions updated for new RPC error format.The error message assertions for
conf_targetparameter validation have been correctly updated to reflect the new RPC error format for bothestimatesmartfeeandestimaterawfeeRPCs.
29-29: LGTM: Test assertion updated for new RPC error format.The error message assertion for
estimate_modeparameter validation has been correctly updated to reflect the new RPC error format forestimatesmartfee.
33-33: LGTM: Test assertion updated for new RPC error format.The error message assertion for
thresholdparameter validation has been correctly updated to reflect the new RPC error format forestimaterawfee.src/rpc/client.cpp (1)
167-168: LGTM! Parameter conversion entries added correctly.The new entries for
simulaterawtransactionfollow the established pattern and are properly integrated into the conversion table. The parameter names "rawtxs" and "options" are descriptive and the indices are correct.test/functional/p2p_leak.py (2)
135-137: Good defensive testing practice.Adding a P2P connection to ensure the network thread executes disconnect logic at least once improves test reliability. The log message provides clear context for this action.
162-162: More robust assertion check.Removing the explicit peer identifier from the debug log assertion makes the test more resilient to changes in peer ID formatting while still validating the core disconnect functionality.
test/functional/test_runner.py (4)
308-309: New test integration looks good.The addition of
wallet_simulaterawtx.pytests for both legacy and descriptor wallets provides comprehensive coverage for the new RPC command. The placement in the test list follows the established pattern.
610-610: Enhanced skip reason handling improves test debugging.The addition of
skip_reasonto the unpacking tuple aligns with the enhanced logging functionality that helps developers understand why tests are skipped.
616-616: Skip reason logging enhancement is helpful.The logging format clearly indicates the skip reason, which will improve debugging when tests are skipped due to missing features or other conditions.
744-785: Skip reason extraction logic is well implemented.The implementation correctly:
- Initializes
skip_reasontoNone- Extracts the reason from test output using regex when a test is skipped
- Includes the skip reason in the returned tuple for logging
The regex pattern
r"Test Skipped: (.*)"matches the expected test framework output format.src/wallet/rpc/wallet.cpp (5)
912-937: RPC help documentation is comprehensive.The help text clearly describes the function, parameters, and return values. The parameter documentation correctly specifies the array of raw transaction hex strings and optional include_watchonly setting.
940-963: Parameter validation and setup is robust.The implementation correctly:
- Validates parameter types using
RPCTypeCheck- Handles optional parameters gracefully
- Uses
ParseIncludeWatchonlyutility for consistent watch-only logic- Applies appropriate wallet locking
The type checking and parameter parsing follow established patterns in the codebase.
965-1000: Input processing logic is well designed.The transaction input processing correctly:
- Decodes hex transactions with proper error handling
- Fetches coins for all inputs upfront
- Implements double-spend detection within the batch
- Handles both new UTXOs from the batch and existing UTXOs
- Uses
GetDebitfor wallet-specific debit calculationThe logic for tracking
spentoutputs andnew_utxosprevents double-spending and enables chained transactions within the batch.
1002-1021: Output processing and result formatting is correct.The implementation properly:
- Determines wallet ownership of outputs using
IsMinewith the appropriate filter- Tracks new UTXOs for potential spending in subsequent transactions
- Calculates balance changes correctly (positive for received, negative for spent)
- Returns the result in the expected format
The logic correctly handles both spendable and watch-only outputs based on the filter.
1156-1156: Command registration is properly placed.The new RPC command is correctly registered in the wallet command table under the "wallet" category, following the established pattern.
test/functional/wallet_simulaterawtx.py (7)
1-27: Test setup and imports are appropriate.The test properly imports necessary modules and sets up a clean chain with a single node. The class structure follows the standard test framework pattern.
28-42: Wallet setup provides good test coverage.The test creates three wallets with different characteristics:
w0: Normal wallet with fundsw1: Normal wallet without fundsw2: Watch-only walletThis setup enables comprehensive testing of the RPC across different wallet types. The generation of coinbase maturity blocks ensures proper funding.
43-66: Basic functionality tests are comprehensive.The tests correctly verify:
- Unfunded transactions show expected balance changes
- Watch-only wallets properly detect owned outputs
- Multiple transaction simulation works correctly
- Different wallets see appropriate balance changes
The assertions use exact equality checks which are appropriate for these unfunded transaction scenarios.
67-79: Funded transaction testing is thorough.The test properly:
- Uses
fundrawtransactionto create a realistic funded transaction- Accounts for transaction fees in balance calculations
- Uses
assert_approxfor fee-sensitive comparisons- Verifies behavior across different wallet types
The use of
Decimalfor fee calculations and approximate assertions prevents floating-point precision issues.
80-95: Error condition testing is excellent.The test correctly verifies error conditions:
- Double-spending detection within the same batch
- Missing input validation for dependent transactions
- Proper error messages match the RPC implementation
The use of
assert_raises_rpc_errorwith specific error codes and messages ensures robust error handling.
96-114: Chained transaction testing is sophisticated.The test validates complex scenarios:
- Transactions that depend on outputs from previous transactions in the batch
- Correct balance calculations for chained transactions
- Conflict detection when multiple transactions spend the same output
The comments clearly explain expected behavior, making the test maintainable. The transaction broadcasting and confirmation setup proper state for subsequent tests.
115-129: Post-confirmation testing validates real-world scenarios.The test verifies that:
- Simulation correctly handles already-confirmed transactions
- Error detection works when attempting to simulate spent inputs
- The RPC behavior remains consistent after blockchain state changes
This ensures the RPC properly interacts with the blockchain state and doesn't just work with unconfirmed transactions.
src/wallet/walletdb.cpp (4)
632-638: Good error handling for descriptor deserialization.The try-catch block properly handles deserialization failures that could occur when encountering descriptors from newer wallet versions or corrupted data. This follows the established error handling pattern in the codebase.
779-784: Improved version handling and logging.Moving the client version reading earlier in the loading process enables better error messages and consolidated logging. The implementation correctly tracks whether the version was read successfully.
843-849: Well-designed error handling for unknown descriptors.The implementation provides clear, actionable error messages that help users understand whether the issue is due to a newer wallet version or potential corruption. The use of a specific error code (
DBErrors::UNKNOWN_DESCRIPTOR) enables proper handling at the wallet creation level.
903-904: Improved wallet loading summary.The updated log message is more concise and includes the metadata count, which is useful for debugging wallet loading issues.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/functional/test_framework/test_framework.py(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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.
Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
PR: dashpay/dash#6726
File: test/functional/rpc_createmultisig.py:120-120
Timestamp: 2025-06-20T23:32:16.225Z
Learning: In the rpc_createmultisig.py test, the checkbalances() function correctly excludes 9 blocks from the balance calculation: 8 blocks from do_multisig() calls (2 blocks per call × 4 calls) plus 1 block from checkbalances() itself.
Learnt from: kwvg
PR: dashpay/dash#6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Learnt from: kwvg
PR: dashpay/dash#6729
File: src/evo/deterministicmns.cpp:1313-1316
Timestamp: 2025-07-09T15:02:26.853Z
Learning: In Dash's masternode transaction validation, `IsVersionChangeValid()` is only called by transaction types that update existing masternode entries (like `ProUpServTx`, `ProUpRegTx`, `ProUpRevTx`), not by `ProRegTx` which creates new entries. This means validation logic in `IsVersionChangeValid()` only applies to the subset of transaction types that actually call it, not all masternode transaction types.
test/functional/test_framework/test_framework.py (1)
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
🔇 Additional comments (1)
test/functional/test_framework/test_framework.py (1)
126-126: LGTM: Clean instance variable initialization.The addition of
self.extra_args = Nonefollows the established pattern of initializing instance variables in the constructor.
| if hasattr(self, "extra_args"): | ||
| extra_args = self.extra_args | ||
| self.add_nodes(self.num_nodes, extra_args) | ||
| self.add_nodes(self.num_nodes, self.extra_args) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify this interface change is appropriate for a backport.
The change removes fallback logic from setup_nodes and now requires self.extra_args to be set externally. While add_nodes properly handles None values (lines 518-519), this changes the interface contract of setup_nodes.
Given this is a backport PR, ensure this change doesn't break existing tests that may rely on the previous behavior.
Run the following script to verify no tests directly call setup_nodes without setting extra_args:
🏁 Script executed:
#!/bin/bash
# Search for direct calls to setup_nodes in test files
rg -A 3 -B 3 "\.setup_nodes\(" test/functional/
# Search for classes that override setup_nodes
rg -A 5 "def setup_nodes" test/functional/Length of output: 16554
#!/bin/bash
Identify functional tests that call setup_nodes() but never set self.extra_args
for file in $(rg --files-with-matches ".setup_nodes" test/functional/); do
if ! grep -q "self.extra_args" "$file"; then
echo "$file"
fi
done
🏁 Script executed:
#!/usr/bin/env bash
# Identify tests that call self.setup_nodes() without preceding self.extra_args assignment
for file in $(rg --files-with-matches "self.setup_nodes" test/functional/); do
while IFS=: read -r ln _; do
if ! head -n "$ln" "$file" | grep -q "self.extra_args"; then
echo "$file:$ln"
fi
done < <(grep -n "self.setup_nodes" "$file")
doneLength of output: 5872
Tests broken by missing self.extra_args after backport
Several functional tests override setup_network() and call self.setup_nodes() without defining self.extra_args, which will now raise AttributeError. Please either restore the default‐fallback logic in TestFramework.setup_nodes, or update these tests to set self.extra_args before invoking setup_nodes().
Files to update:
- test/functional/wallet_importmulti.py (in
setup_network) - test/functional/wallet_backup.py (in
setup_network) - test/functional/wallet_basic.py (in
setup_network) - test/functional/rpc_getblockfrompeer.py (in
setup_network) - test/functional/wallet_simulaterawtx.py (in
setup_network) - test/functional/interface_http.py (in
setup_network)
Suggested fix: at the top of each setup_network() add:
self.extra_args = [[]] * self.num_nodes…or reintroduce the pre-backport fallback in TestFramework.setup_nodes.
🤖 Prompt for AI Agents
In test/functional/test_framework/test_framework.py at line 452, the call to
self.add_nodes uses self.extra_args which may not be defined in some tests,
causing AttributeError. Fix this by either restoring the default fallback for
self.extra_args in TestFramework.setup_nodes to set it to [[]] * self.num_nodes
if not already defined, or update each affected test's setup_network method (in
the listed test files) to define self.extra_args as [[]] * self.num_nodes before
calling setup_nodes.
92fc65a to
e9c106e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/functional/test_runner.py (1)
744-785: Skip reason extraction logic properly implemented.The implementation correctly:
- Initializes
skip_reasontoNonefor non-skipped tests- Extracts skip reasons from test stdout using regex for skipped tests
- Includes
skip_reasonin the returned tuple structureThe regex pattern
r"Test Skipped: (.*)"appropriately captures skip reason text from test output.Consider adding error handling for cases where the regex might not match:
elif proc.returncode == TEST_EXIT_SKIPPED: status = "Skipped" - skip_reason = re.search(r"Test Skipped: (.*)", stdout).group(1) + match = re.search(r"Test Skipped: (.*)", stdout) + skip_reason = match.group(1) if match else "Unknown reason"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (54)
ci/test/04_install.sh(2 hunks)ci/test/05_before_script.sh(1 hunks)configure.ac(0 hunks)contrib/devtools/test-security-check.py(1 hunks)contrib/devtools/test-symbol-check.py(1 hunks)src/Makefile.am(2 hunks)src/chain.h(1 hunks)src/i2p.cpp(1 hunks)src/i2p.h(1 hunks)src/index/base.h(1 hunks)src/llmq/instantsend.h(1 hunks)src/llmq/quorums.h(1 hunks)src/llmq/signing.h(1 hunks)src/llmq/signing_shares.h(1 hunks)src/mapport.cpp(1 hunks)src/net.cpp(1 hunks)src/net.h(1 hunks)src/qt/transactionrecord.h(1 hunks)src/rpc/client.cpp(1 hunks)src/rpc/mining.cpp(0 hunks)src/rpc/util.cpp(1 hunks)src/stats/rawsender.h(1 hunks)src/test/fuzz/pow.cpp(2 hunks)src/test/i2p_tests.cpp(1 hunks)src/test/mempool_tests.cpp(4 hunks)src/test/miner_tests.cpp(13 hunks)src/test/policyestimator_tests.cpp(3 hunks)src/test/sock_tests.cpp(1 hunks)src/test/util/setup_common.cpp(1 hunks)src/test/util/setup_common.h(2 hunks)src/util/check.h(2 hunks)src/util/sock.cpp(1 hunks)src/util/sock.h(1 hunks)src/util/threadinterrupt.cpp(1 hunks)src/util/threadinterrupt.h(2 hunks)src/wallet/load.cpp(1 hunks)src/wallet/rpc/wallet.cpp(2 hunks)src/wallet/wallet.cpp(1 hunks)src/wallet/walletdb.cpp(5 hunks)src/wallet/walletdb.h(1 hunks)test/functional/feature_cltv.py(2 hunks)test/functional/mempool_accept.py(1 hunks)test/functional/p2p_leak.py(2 hunks)test/functional/rpc_estimatefee.py(1 hunks)test/functional/rpc_getblockfrompeer.py(1 hunks)test/functional/rpc_getdescriptorinfo.py(1 hunks)test/functional/rpc_net.py(1 hunks)test/functional/rpc_rawtransaction.py(2 hunks)test/functional/test_framework/test_framework.py(2 hunks)test/functional/test_framework/util.py(1 hunks)test/functional/test_runner.py(4 hunks)test/functional/wallet_listdescriptors.py(5 hunks)test/functional/wallet_resendwallettransactions.py(2 hunks)test/functional/wallet_simulaterawtx.py(1 hunks)
💤 Files with no reviewable changes (2)
- configure.ac
- src/rpc/mining.cpp
✅ Files skipped from review due to trivial changes (24)
- src/util/threadinterrupt.h
- src/util/sock.cpp
- src/test/i2p_tests.cpp
- src/llmq/signing_shares.h
- contrib/devtools/test-security-check.py
- test/functional/rpc_rawtransaction.py
- src/util/threadinterrupt.cpp
- src/stats/rawsender.h
- src/llmq/instantsend.h
- src/index/base.h
- src/llmq/signing.h
- src/util/sock.h
- src/test/sock_tests.cpp
- src/llmq/quorums.h
- src/i2p.cpp
- src/mapport.cpp
- src/i2p.h
- test/functional/wallet_listdescriptors.py
- src/test/policyestimator_tests.cpp
- src/net.cpp
- src/net.h
- src/test/util/setup_common.cpp
- src/Makefile.am
- src/test/mempool_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (17)
- src/util/check.h
- test/functional/rpc_getdescriptorinfo.py
- src/wallet/load.cpp
- test/functional/rpc_getblockfrompeer.py
- test/functional/wallet_resendwallettransactions.py
- test/functional/rpc_estimatefee.py
- test/functional/test_framework/util.py
- src/wallet/wallet.cpp
- test/functional/mempool_accept.py
- src/wallet/walletdb.h
- src/wallet/walletdb.cpp
- src/rpc/util.cpp
- src/rpc/client.cpp
- test/functional/p2p_leak.py
- test/functional/wallet_simulaterawtx.py
- test/functional/test_framework/test_framework.py
- src/wallet/rpc/wallet.cpp
🧰 Additional context used
🧠 Learnings (10)
📓 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.
Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
PR: dashpay/dash#6726
File: test/functional/rpc_createmultisig.py:120-120
Timestamp: 2025-06-20T23:32:16.225Z
Learning: In the rpc_createmultisig.py test, the checkbalances() function correctly excludes 9 blocks from the balance calculation: 8 blocks from do_multisig() calls (2 blocks per call × 4 calls) plus 1 block from checkbalances() itself.
Learnt from: kwvg
PR: dashpay/dash#6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Learnt from: kwvg
PR: dashpay/dash#6729
File: src/evo/deterministicmns.cpp:1313-1316
Timestamp: 2025-07-09T15:02:26.853Z
Learning: In Dash's masternode transaction validation, `IsVersionChangeValid()` is only called by transaction types that update existing masternode entries (like `ProUpServTx`, `ProUpRegTx`, `ProUpRevTx`), not by `ProRegTx` which creates new entries. This means validation logic in `IsVersionChangeValid()` only applies to the subset of transaction types that actually call it, not all masternode transaction types.
test/functional/feature_cltv.py (1)
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
src/test/util/setup_common.h (1)
Learnt from: kwvg
PR: dashpay/dash#6530
File: src/validation.cpp:360-362
Timestamp: 2025-01-14T08:37:16.955Z
Learning: The UpdateTransactionsFromBlock() method in txmempool.cpp takes parameters in the order: vHashUpdate, ancestor_size_limit, ancestor_count_limit. The size limit comes before the count limit.
src/chain.h (1)
Learnt from: knst
PR: dashpay/dash#6658
File: src/evo/creditpool.cpp:177-185
Timestamp: 2025-05-05T12:45:44.781Z
Learning: The GetAncestor() function in CBlockIndex safely handles negative heights by returning nullptr rather than asserting, making it safe to call with potentially negative values.
src/test/miner_tests.cpp (6)
Learnt from: knst
PR: dashpay/dash#6658
File: src/evo/creditpool.cpp:177-185
Timestamp: 2025-05-05T12:45:44.781Z
Learning: The GetAncestor() function in CBlockIndex safely handles negative heights by returning nullptr rather than asserting, making it safe to call with potentially negative values.
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
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.
Learnt from: kwvg
PR: dashpay/dash#6726
File: test/functional/rpc_createmultisig.py:120-120
Timestamp: 2025-06-20T23:32:16.225Z
Learning: In the rpc_createmultisig.py test, the checkbalances() function correctly excludes 9 blocks from the balance calculation: 8 blocks from do_multisig() calls (2 blocks per call × 4 calls) plus 1 block from checkbalances() itself.
Learnt from: kwvg
PR: dashpay/dash#6530
File: src/validation.cpp:360-362
Timestamp: 2025-01-14T08:37:16.955Z
Learning: The UpdateTransactionsFromBlock() method in txmempool.cpp takes parameters in the order: vHashUpdate, ancestor_size_limit, ancestor_count_limit. The size limit comes before the count limit.
ci/test/05_before_script.sh (1)
Learnt from: PastaPastaPasta
PR: dashpay/dash#6564
File: contrib/containers/deploy/Dockerfile:1-1
Timestamp: 2025-02-11T19:25:05.487Z
Learning: The deploy container for Dash Core is designed to be minimal, requiring only basic container functionality without complex init systems or service management.
test/functional/rpc_net.py (1)
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
ci/test/04_install.sh (1)
Learnt from: PastaPastaPasta
PR: dashpay/dash#6564
File: contrib/containers/deploy/Dockerfile:1-1
Timestamp: 2025-02-11T19:25:05.487Z
Learning: When migrating from Phusion baseimage to vanilla Ubuntu in Docker containers, it's recommended to take an iterative approach: first change the base image while maintaining existing functionality, then add any missing packages based on testing results.
src/test/fuzz/pow.cpp (3)
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
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.
Learnt from: kwvg
PR: dashpay/dash#6532
File: src/test/fuzz/netaddress.cpp:83-84
Timestamp: 2025-01-14T09:06:19.717Z
Learning: In fuzzer harness tests, CServiceHash can be used with both default constructor (CServiceHash()) and parameterized constructor (CServiceHash(salt_k0, salt_k1)) to test different variants. The usage pattern CServiceHash()(service) and CServiceHash(0, 0)(service) is valid and intentional in such tests.
test/functional/test_runner.py (1)
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
🧬 Code Graph Analysis (1)
ci/test/05_before_script.sh (1)
ci/test/04_install.sh (1)
CI_EXEC_ROOT(70-72)
🪛 Ruff (0.11.9)
test/functional/rpc_net.py
202-202: Function definition does not bind loop variable n
(B023)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (20)
src/qt/transactionrecord.h (2)
94-98: LGTM: Redundant initialization removed.The explicit initialization of
strAddressto""is redundant sincestd::stringdefaults to empty. The behavior remains identical asDecodeDestination(strAddress)is still called with an empty string.
100-105: LGTM: Redundant initialization removed.Similar to the default constructor, removing the explicit
strAddress("")initialization is correct sincestd::stringdefaults to empty. The functionality is preserved.contrib/devtools/test-symbol-check.py (1)
26-26: LGTM! Improved script robustness by using absolute path construction.This change enhances the reliability of the script by using
os.path.join(os.path.dirname(__file__), 'symbol-check.py')instead of a relative path. This approach ensures the script can locatesymbol-check.pyregardless of the current working directory, which is a Python best practice for script-relative file references.test/functional/feature_cltv.py (2)
11-11: Good addition of the named constant import.The import of
TIME_GENESIS_BLOCKfromtest_framework.blocktoolsproperly supports the standardization effort across test files.
68-68: Excellent replacement of magic number with named constant.Replacing the hardcoded genesis block timestamp (1296688602) with
TIME_GENESIS_BLOCKsignificantly improves code readability and maintainability. The functionality remains identical - this still creates the intended lock-time type mismatch (height vs timestamp) for testing CLTV failure scenario #3.src/test/fuzz/pow.cpp (1)
28-28: Correct adaptation to CBlockIndex ownership restrictions.The changes properly adapt the fuzz test to use
std::unique_ptr<CBlockIndex>instead of value objects, which aligns with the copy/move restrictions introduced insrc/chain.h. The ownership semantics are clear and prevent potential issues with shallow copies.Also applies to: 36-40, 70-72
src/test/util/setup_common.h (1)
20-20: Good improvement in time type safety.The migration from
int64_ttoNodeSecondsfor time handling improves type safety and prevents unit confusion. The default initialization and method signature updates are consistent with the broader effort to use strongly-typed time representations.Also applies to: 235-235, 242-242, 250-250
src/test/miner_tests.cpp (3)
87-93: Proper adaptation of CreateBlockIndex to ownership model.The function correctly returns
std::unique_ptr<CBlockIndex>and the implementation properly allocates and initializes the block index. This aligns with the CBlockIndex copy restrictions.
117-117: Consistent migration to NodeSeconds time API.All
Time(GetTime())calls have been correctly updated toTime(Now<NodeSeconds>()), which provides better type safety and is consistent with the TestMemPoolEntryHelper changes.Also applies to: 123-123, 129-129, 290-290, 305-305, 325-325, 333-333, 342-342, 350-350, 361-361, 372-372, 375-375, 414-414, 419-419, 453-453, 467-467, 490-490, 501-501
459-459: Correct dereferencing of unique_ptr for SequenceLocks calls.The dereferencing of
CreateBlockIndex()results properly passes references to theCBlockIndexobjects as required bySequenceLocks().Also applies to: 476-476
src/chain.h (1)
333-349: Well-designed copy/move restrictions for CBlockIndex.The changes effectively prevent shallow copying issues while maintaining necessary functionality:
- Protected copy constructor allows
CDiskBlockIndexinheritance- Deleted assignment operators and move constructor prevent accidental misuse
- The detailed comment clearly explains the rationale
This defensive design prevents pointer equality issues and dangling pointer risks from
pprev/pskipmembers.test/functional/test_runner.py (3)
308-309: LGTM! New wallet simulation test properly configured.The addition of
wallet_simulaterawtx.pywith both--legacy-walletand--descriptorsoptions follows the established pattern for wallet tests in the test suite.
610-610: Skip reason unpacking correctly implemented.The additional
skip_reasonparameter in the tuple unpacking aligns with the TestHandler.get_next() method modifications that include skip_reason in the returned tuples.
616-616: Enhanced skip reason logging improves debugging.The inclusion of
skip_reasonin the debug logging for skipped tests provides valuable diagnostic information for understanding why tests were skipped.ci/test/05_before_script.sh (1)
14-14: Dual-user context file creation properly implemented.The addition of
CI_EXEC_ROOT echo \> \$HOME/.dashcorealongside the existingCI_EXECcommand ensures the dummy.dashcorefile is created in both root and non-root user contexts, aligning with the Docker container setup that creates both user environments.ci/test/04_install.sh (5)
36-40: Local user context capture properly implemented.The retrieval of local UID/GID using
id -uandid -gcommands is correct, and the descriptive username "nonroot" clearly indicates the purpose of this user creation.
54-62: Docker container user setup correctly implemented.The implementation properly addresses Docker volume permission issues by:
- Creating a non-root user with matching UID/GID to the local user
- Setting up separate command prefixes for root and non-root execution
- Ensuring proper ownership of the base directory
This follows Docker best practices for handling file permissions on mounted volumes.
70-74: CI_EXEC_ROOT function properly defined.The function correctly mirrors
CI_EXECbut usesDOCKER_CI_CMD_PREFIX_ROOTfor root execution. The export ensures availability to child processes.
77-77: Package management commands correctly updated for root execution.The updates to use
CI_EXEC_ROOTfor package management commands (dpkg,yum,apt-get) are appropriate since these operations require root privileges for system package installation.Also applies to: 81-85
88-88: Correct privilege separation for different package managers.The PIP package installation correctly continues to use the regular
pip3 install --usercommand without root privileges, maintaining appropriate security separation between system-level and user-level package management.
| for n in self.nodes: | ||
| self.wait_until(lambda: n.getnetworkinfo()['connections'] == 0, timeout=3) |
There was a problem hiding this comment.
Fix the lambda closure issue to properly capture loop variable.
The lambda function captures the loop variable n by reference, not by value. When wait_until executes the lambda, the loop may have finished and n will always refer to the last node in self.nodes, not the intended node for each iteration.
Apply this diff to fix the closure issue:
- for n in self.nodes:
- self.wait_until(lambda: n.getnetworkinfo()['connections'] == 0, timeout=3)
+ for n in self.nodes:
+ self.wait_until(lambda node=n: node.getnetworkinfo()['connections'] == 0, timeout=3)Alternatively, you can use a helper function approach:
- for n in self.nodes:
- self.wait_until(lambda: n.getnetworkinfo()['connections'] == 0, timeout=3)
+ for n in self.nodes:
+ self.wait_until(lambda node=n: node.getnetworkinfo()['connections'] == 0, timeout=3)📝 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.
| for n in self.nodes: | |
| self.wait_until(lambda: n.getnetworkinfo()['connections'] == 0, timeout=3) | |
| for n in self.nodes: | |
| self.wait_until(lambda node=n: node.getnetworkinfo()['connections'] == 0, timeout=3) |
🧰 Tools
🪛 Ruff (0.11.9)
202-202: Function definition does not bind loop variable n
(B023)
🤖 Prompt for AI Agents
In test/functional/rpc_net.py around lines 201 to 202, the lambda function
inside the loop captures the loop variable `n` by reference, causing it to
always refer to the last node when executed. To fix this, modify the lambda to
capture the current value of `n` by using a default argument like `lambda n=n:
...` so each lambda retains its own node reference during the wait_until call.
280c295 to
52a691e
Compare
PastaPastaPasta
force-pushed
the
backport-0.24-batch-212
branch
from
9d9cd86 to
26af25d
Compare
db10cf8 rpc/wallet: add simulaterawtransaction RPC (Karl-Johan Alm) 701a64f test: add support for Decimal to assert_approx (Karl-Johan Alm) Pull request description: (note: this was originally titled "add analyzerawtransaction RPC") This command iterates over the inputs and outputs of the given transactions, and tallies up the balance change for the given wallet. This can be useful e.g. when verifying that a coin join like transaction doesn't contain unexpected inputs that the wallet will then sign for unintentionally. I originally proposed this to Elements (ElementsProject/elements#1016) and it was suggested that I propose this upstream. There is an alternative bitcoin#22776 to instead add this info to `getbalances` when providing an optional transaction as argument. ACKs for top commit: jonatack: ACK db10cf8 achow101: re-ACK db10cf8 Tree-SHA512: adf222ec7dcdc068d007ae6f465dbc35b692dc7bb2db337be25340ad0c2f9c64cfab4124df23400995c700f41c83c29a2c34812121782c26063b100c7969b89d
…lity code 90a5dfa RPC/Mining: Clean out pre-Segwit miner compatibility code (Luke Dashjr) Pull request description: This is dead code post-Segwit. ACKs for top commit: achow101: ACK 90a5dfa Tree-SHA512: 5970aa3548d2a7da7c6e83fb9b910529faab10251b115122cec833bb7d3a54c7cb0714c1a873807be04c7817bb827c7ece1e20e8fa4c907aa58688487d0ec44d
07b6e74 test: Display skipped tests reason (Aurèle Oulès) Pull request description: Attempt to fix bitcoin#26023. ACKs for top commit: brunoerg: ACK 07b6e74 Tree-SHA512: 5d8f7fbd8d65772000a5da8c01276948b157d93d359203c6442cf2681cdcc2426b1fee7ec62cee100019c59a486a96ad98d5e819bffe1fd37624dcd28f42aed2
5ef8c2c test: fix typo for MaybeResendWalletTxs (stickies-v) fbba4a1 wallet: trigger MaybeResendWalletTxs() every minute (stickies-v) Pull request description: ResendWalletTransactions() only executes every [12-36h (24h average)](https://github.com/bitcoin/bitcoin/blob/1420547ec30a24fc82ba3ae5ac18374e8e5af5e5/src/wallet/wallet.cpp#L1947). Triggering it every second is excessive, once per minute should be plenty. The goal of this PR is to reduce the amount of (unnecessary) schedule executions by ~60x without meaningfully altering transaction rebroadcast logic/assumptions which would require more significant review. ACKs for top commit: achow101: ACK 5ef8c2c 1440000bytes: ACK bitcoin@5ef8c2c Tree-SHA512: 4a077e3579b289c11c347eaa0d3601ef2dbb9fee66ab918d56b4a0c2e08222560a0e6be295297a74831836e001a997ecc143adb0c132faaba96a669dac1cd9e6
fa2aae5 test: Fix intermittent issue in p2p_leak.py (MacroFake) Pull request description: Diff to reproduce: ```diff diff --git a/src/net.cpp b/src/net.cpp index 865ce2e..ccf289d77b 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1150,6 +1150,7 @@ bool CConnman::InactivityCheck(const CNode& node) const if (last_recv.count() == 0 || last_send.count() == 0) { LogPrint(BCLog::NET, "socket no message in first %i seconds, %d %d peer=%d\n", count_seconds(m_peer_connect_timeout), last_recv.count() != 0, last_send.count() != 0, node.GetId()); + UninterruptibleSleep(6s); return true; } ``` Example in CI: ``` node0 2022-08-12T09:51:56.015288Z [net] [net.cpp:1152] [InactivityCheck] [net] socket no message in first 3 seconds, 0 0 peer=0 test 2022-08-12T09:51:57.658000Z TestFramework (ERROR): Assertion failed Traceback (most recent call last): File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 133, in main self.run_test() File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/p2p_leak.py", line 155, in run_test assert not no_version_idle_peer.is_connected AssertionError ``` https://cirrus-ci.com/task/5346634421764096?logs=ci#L3683 ACKs for top commit: satsie: ACK fa2aae5 luke-jr: tACK fa2aae5 Tree-SHA512: e6ddf5b985f7da365b18b699ff8d0719b71b44e4e6bc5576d4099d1bad2c702495afd85f69f4edba89a883e13756a340946db2e7f4be41b1ac0e3c4f515ca4fd
* Merge bitcoin#26069: rpc: unify arg type error message 2870a97 RPC: unify arg type error message (furszy) Pull request description: Decoupled from bitcoin#25737 per request. We are throwing two different error descriptions for the same problematic: 1) "Expected type {expected], got {type}" --> `RPCTypeCheckArgument()` 2) "JSON value of type {type} is not of expected type {expected}" --> `UniValue::checkType()` ACKs for top commit: MarcoFalke: review ACK 2870a97 fanquake: ACK 2870a97 Tree-SHA512: 9ac863243b6b7687986c0394611b5cad8b35424ec49d82195d536f2a5e64c60327b25f0dc7336189f86fd71122689c7309da49adfa93805d2e345693fa8efa9b * fix: remove replaceable parameter test from rpc_rawtransaction The backport incorrectly included a test for the 'replaceable' parameter in createrawtransaction, but Dash doesn't support this parameter. Bitcoin added RBF (Replace-By-Fee) support which includes the replaceable parameter, but Dash uses InstantSend instead and doesn't have RBF. This was causing test failures in the CI. The fix removes the test for the non-existent parameter while keeping all the other valid error message updates. Auto-generated by backport verification system. Verification-fix: bitcoin-backport-invalid-parameter-test * fix: correct RPC type error message expectations in tests The Bitcoin commit bitcoin#26069 unified RPC argument type error messages to use 'JSON value of type X is not of expected type Y' format. However, two test assertions in rpc_rawtransaction.py were incorrectly changed. These specific errors come from UniValue's internal type checking, not from RPCTypeCheckArgument, so they retain their original format: - 'JSON value is not an object as expected' - 'JSON value is not a string as expected' This fix reverts those two test expectations to match the actual error messages while keeping all other RPC type error unification changes. --------- Co-authored-by: fanquake <[email protected]> Co-authored-by: Claude Code <[email protected]>
* Fix bitcoin#26645: Include full version id in bug reports - Modified format_internal_error macro to include PACKAGE_NAME and FormatFullVersion() - Added clientversion.h include to util/check.h - Reverted util/check.cpp to original state (no changes needed) The previous implementation incorrectly added entire functions to the .cpp file when the change should only modify the existing macro in the header file to match Bitcoin's functionality while preserving Dash's macro-based structure. Original commit: fa825bd * Remove unrelated change in src/util/check.cpp The change in src/util/check.cpp was unrelated to the Bitcoin commit. Bitcoin's commit modifies the StrFormatInternalBug function, but Dash uses a macro-based approach in the header file instead. The only required change is the format_internal_error macro in src/util/check.h which correctly includes PACKAGE_NAME and FormatFullVersion(). 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]> --------- Co-authored-by: Claude Code <[email protected]> Co-authored-by: PastaPastaPasta <[email protected]> Co-authored-by: Claude <[email protected]>
150340a test: remove unneeded extra_args code (josibake) 989a52e test: add extra_args to BTF class (josibake) Pull request description: ## problem If you try to add `extra_args` when using `TestShell`, you will get the following error: ```python >>> import sys >>> >>> sys.path.insert(0, "/home/josibake/bitcoin/test/functional") >>> >>> from test_framework.test_shell import TestShell >>> test = TestShell().setup(num_nodes=2, extra_args=[[],['-fallbackfee=0.0002']]) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/home/josibake/bitcoin/test/functional/test_framework/test_shell.py", line 41, in setup raise KeyError(key + " not a valid parameter key!") KeyError: 'extra_args not a valid parameter key!' >>> ``` ## solution add `self.extra_args = None` so that `extra_args` is recognized as a valid parameter to be passed to `BitcoinTestFramework` ```python >>> import sys >>> >>> sys.path.insert(0, "/home/josibake/bitcoin/test/functional") >>> >>> from test_framework.test_shell import TestShell >>> test = TestShell().setup(num_nodes=2, extra_args=[[],['-fallbackfee=0.0002']]) 2022-12-01T11:23:23.765000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_sbwthbb_ ``` ACKs for top commit: willcl-ark: re-ACK 150340a Tree-SHA512: e6fa2a780a8f2d3472c322e8cdb00ec35cb220c3b4d6ca02291eb8b41c0d8676a635fbc79c6be80e3bb71d700a2501a4b73f762478f533ae453d492d449307bb Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^[email protected]>
849f20a ci: create and use non-root user for docker image (josibake) Pull request description: Previously, everything in the ci docker image ran as the root user. This would lead to certain directories (`ci/scratch`, `depends`) being owned by `root` after running the ci locally which would lead to annoying behavior such as subsequent guix builds failing due to `depends/` being owned by root. This PR adds a non-root user in the container and chowns the mounted working directory. All the `docker exec` commands now run as the non-root user, except for the few that still need to run as root (mainly, installing packages). To test this I checked out a fresh copy of the repo, applied my changes, ran the CI, and verified all the local file permissions were unchanged after the CI was finished running. ACKs for top commit: hebasto: ACK 849f20a, tested on Ubuntu 22.04 by running commands as follows: Tree-SHA512: 734dca0f36157fce5fab243b4ff657fc17ba980e8e4e4644305f41002ff21bd5cef02c306ea1e0b5c841d4c07c095e8e4be16722e6a38c890717c60a3f5ec62a Co-authored-by: MarcoFalke <[email protected]>
b895304 util: move threadinterrupt into util (fanquake) Pull request description: Alongside thread and threadnames. It's part of libbitcoin_util. ACKs for top commit: ryanofsky: Code review ACK b895304. No changes since last review other than rebase theuni: ACK b895304. Tree-SHA512: 0421f4d1881ec295272446804b27d16bf63e6b62b272f8bb52bfecde9ae6605e8109ed16294690d3e3ce4b15cc5e7c4046f99442df73adb10bdf069d3fb165aa Co-authored-by: fanquake <[email protected]>
…ntryHelper * Merge bitcoin#26376: test: Use type-safe NodeSeconds for TestMemPoolEntryHelper 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> * validation: Fix TestMemPoolEntryHelper time conversion to match Bitcoin Use TicksSinceEpoch<std::chrono::seconds>(time) instead of count_seconds(time.time_since_epoch()) to match the exact implementation from Bitcoin commit fa2d014. Also apply formatting changes to match Bitcoin's code style. --------- Co-authored-by: Claude Code <[email protected]> Co-authored-by: Claude <[email protected]>
fadf7b8 test: Fix intermittent issue in rpc_net.py (MarcoFalke) Pull request description: Both nodes must be aware of the closed connections before re-connecting, otherwise the test will fail. Fixes bitcoin#25741 ACKs for top commit: jarolrod: ACK fadf7b8 Tree-SHA512: 0492dd59a46516007e903fe8f6288982e305d4fd152a0297dd60e10ac663efc9bdc0bc6d1d2c6ec5f239196dac08319f37b079401fe057e4864b117b0fd9dcbc Co-authored-by: fanquake <[email protected]>
…p magic numbers dbed289 test: refactor: eliminate genesis block timestamp magic numbers (Sebastian Falbesoner) Pull request description: This tiny PR replaces all occurences of the regtest/testnet genesis block timestamp (found via `git grep 1296688602`) with the constant `TIME_GENESIS_BLOCK` to increase the readability. ACKs for top commit: aureleoules: ACK dbed289 Tree-SHA512: be39d5c2631ad20eb775c2a077b1b1f056a1a4930aa44e6fdec73b974fd4bdf8da0103a3a38e3514b68fcf6a6316e007a371c523da5076a315545c9bf3091aee Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^[email protected]>
…s/` scripts * Merge bitcoin#26446: build: Drop unneeded linking of `contrib/devtools/` scripts 29ef26a build: Drop unneeded linking of `contrib/devtools/` scripts (Hennadii Stepanov) 77779c3 script: Improve `test-{security,symbol}-check.py` robustness (Hennadii Stepanov) Pull request description: The build system targets `make test-security-check`, `make -C src check-security` and `make -C src check-symbols` run `contrib/devtools/{test-,}{security,symbol}-check.py` scripts from the top source directory, i.e. `$(top_srcdir)` in the current Autotools-based build system. This renders needless of linking of those scripts into the build directory. Both build systems, the current Autotools-based and the future CMake-based, benefit from this simplification. ACKs for top commit: fanquake: ACK 29ef26a Tree-SHA512: 442b6aa116615d01eabc58b6ded67d9c6993033a071bb7008afdb956c468b65bb2b51705aeaed60fd68211dd2b9c8b8e2234babd45abd022daff391c00091165 * Apply suggestions from code review --------- Co-authored-by: fanquake <[email protected]> Co-authored-by: PastaPastaPasta <[email protected]>
36c201f remove CBlockIndex copy construction (James O'Beirne) Pull request description: Copy construction of CBlockIndex objects is a footgun because of the wide use of equality-by-pointer comparison in the code base. There are also potential lifetime confusions of using copied instances, since there are recursive pointer members (e.g. pprev). (See also bitcoin#24008 (comment)) We can't just delete the copy constructors because they are used for derived classes (CDiskBlockIndex), so we mark them protected. ACKs for top commit: ajtowns: ACK 36c201f - code review only MarcoFalke: re-ACK 36c201f 🏻 Tree-SHA512: b1cf9a1cb992464a4377dad609713eea63cc099435df374e4553bfe62d362a4eb5e3c6c6649177832f38c0905b23841caf9d62196cef8e3084bfea0bfc26374b Co-authored-by: fanquake <[email protected]>
…ring-init` in headers c39619e clang-tidy: Fix `readability-redundant-string-init` in headers (Hennadii Stepanov) Pull request description: Split from bitcoin#26705 as was requested in bitcoin#26705 (comment). To test this PR, consider applying a diff as follows: ```diff --- a/src/.clang-tidy +++ b/src/.clang-tidy @@ -12,17 +12,9 @@ readability-redundant-declaration, readability-redundant-string-init, ' WarningsAsErrors: ' -bugprone-argument-comment, -bugprone-use-after-move, -misc-unused-using-decls, -modernize-use-default-member-init, -modernize-use-nullptr, -performance-for-range-copy, -performance-move-const-arg, -performance-unnecessary-copy-initialization, -readability-redundant-declaration, readability-redundant-string-init, ' CheckOptions: - key: performance-move-const-arg.CheckTriviallyCopyableMove value: false +HeaderFilterRegex: '.' ``` ACKs for top commit: MarcoFalke: review ACK c39619e Tree-SHA512: d7b61be17737f68b8bb40b084cf03f89eae86f4951da2aa000fde0c5245491a01dbb83e5d6e870c6bab4de2dbb5c0eb0dd6613da71592b3a27cf2000a45eaeeb Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^[email protected]>
…nterface * Merge bitcoin#26419: log: mempool: log removal reason in validation interface 25ef049 log: mempool: log removal reason in validation interface (James O'Beirne) Pull request description: Currently the exact reason a transaction is removed from the mempool isn't logged. It is sometimes detectable from context, but adding the `reason` to the validation interface logs (where it is already passed) seems like an easy way to disambiguate. For example in the case of mempool expiry, the logs look like this: ``` [validationinterface.cpp:220] [TransactionRemovedFromMempool] [validation] Enqueuing TransactionRemovedFromMempool: txid=<txid> wtxid=<wtxid> [txmempool.cpp:1050] [RemoveUnbroadcastTx] [mempool] Removed <txid> from set of unbroadcast txns before confirmation that txn was sent out [validationinterface.cpp:220] [operator()] [validation] TransactionRemovedFromMempool: txid=<txid> wtxid=<wtxid> [validation.cpp:267] [LimitMempoolSize] [mempool] Expired 1 transactions from the memory pool ``` There is no context-free way to know $txid was evicted on the basis of expiry. This change will make that case (and probably others) clear. ACKs for top commit: 0xB10C: ACK 25ef049 Tree-SHA512: 9890f9fa16f66c8a9296798d8c28993e1b81da17cf592946f2abc22041f0b30b0911ab86a0c48d4aa46b9a8b3f7f5de67778649ac48c97740b0a09aa6816e0af * Fix compilation errors in mempool logging backport - Fix MemPoolRemovalReason enum mismatch: REPLACED → MANUAL to match Dash's enum - Remove GetWitnessHash() call as Dash doesn't support Segwit - Revert scope creep: MainSignalsImpl → MainSignalsInstance to match original - Restore original comment style (//\! instead of /**) These changes ensure faithful backporting of bitcoin#26419 with proper Dash adaptations. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> --------- Co-authored-by: MacroFake <[email protected]> Co-authored-by: Claude Code <[email protected]> Co-authored-by: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Claude Code <[email protected]> Co-authored-by: Claude <[email protected]>
* Merge bitcoin#28740: refactor: Add LIFETIMEBOUND to all (w)txid getters faec889 refactor: Add LIFETIMEBOUND to all (w)txid getters (MarcoFalke) Pull request description: Currently some getters return a reference, some don't. Fix this by returning a reference everywhere. Also, add `LIFETIMEBOUND` to all. Then, use the compiler warnings to create copies only where needed. Also, fix iwyu includes while touching the includes. ACKs for top commit: dergoegge: Code review ACK faec889 stickies-v: ACK faec889 pablomartin4btc: cr ACK faec889 Tree-SHA512: 0c2a151f39d0e007b4d33b0b85ad578cc220f3e9dd94890e812b3181c3901545b039325707731cc39a5e89557f59c1154c6320525f78f5de95f119a514d2d23f * Fix backport: Remove non-existent includes and adapt for Dash - Remove wallet/types.h include (file doesn't exist in Dash) - Remove unused includes that were added in Bitcoin but not needed - Fix tx_pool.cpp to not call ToUint256() on uint256 objects - Adapt GetHash() calls to Dash's return type (uint256 not transaction_identifier) * Remove backup file --------- Co-authored-by: fanquake <[email protected]> Co-authored-by: Claude Code <[email protected]>
…face.md bdb2e8d Update JSON-RPC-interface.md (iamcarlos94) Pull request description: clarifying when the .cookie file is generated ACKs for top commit: maflcko: lgtm ACK bdb2e8d achow101: ACK bdb2e8d Tree-SHA512: 5a19b9892917126980bd3260f6035a8b2c5c9a9cfd16261d5364713ffa4816f1604e8bd3298fcf7ca7be072f33cd5a9f4e0a89cfee77ae90dc0b201e4abc0f3f Co-authored-by: Andrew Chow <[email protected]>
…nly legacy wallets * Merge bitcoin#25680: rpc, docs: Add note for commands that supports only legacy wallets 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> * Fix missing changes from Bitcoin commit 0713088 Add legacy wallet notes to addmultisigaddress and sethdseed commands that were missing from the original backport. - Add note to addmultisigaddress in addresses.cpp - Add note to sethdseed in wallet.cpp 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> --------- Co-authored-by: Claude Code <[email protected]> Co-authored-by: Claude <[email protected]>
Removes unnecessary local variable usage in favor of directly using self.extra_args in DashTestFramework.setup_nodes(), making it consistent with the implementation in BitcoinTestFramework.setup_nodes() as suggested in PR review.
PastaPastaPasta
force-pushed
the
backport-0.24-batch-212
branch
from
26af25d to
5a1b908
Compare
| "\nImports keys from a wallet dump file (see dumpwallet). Requires a new wallet backup to include imported keys.\n" | ||
| "Note: Use \"getwalletinfo\" to query the scanning progress.\n", | ||
| "Note: Use \"getwalletinfo\" to query the scanning progress.\n" | ||
| "Note: This command is only compatible with legacy wallets.\n", |
There was a problem hiding this comment.
several RPCs are missing. Here's fix: #6774
…eypool, dumpprivkey, importelectrumwallet b27ab96 fix: follow-up bitcoin#25680 for several Dash rpcs: newkeypool, dumpprivkey, importelectrumwallet (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented These Dash RPCs are legacy-only, but note is missing in #6747 ## What was done? Added note for `newkeypool`, `dumpprivkey`, `importelectrumwallet` ## How Has This Been Tested? N/A ## 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 ACKs for top commit: UdjinM6: utACK b27ab96 Tree-SHA512: a0b9c179e691be217511f669e93cf56a9a0182a536d54dcc0f54f3ba436c2b0cff6e078da5c72e39b10a5298ace1640488ac2e0955767ab767ea275b13ffd536
7 participants
Issue being fixed or feature implemented
What was done?
How Has This Been Tested?
Breaking Changes
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.