backport: merge bitcoin#22976, #23242, #23155, #22787, #23602, #12315, #22805, #23640, #23879, #24154, #23975, #12677, #23408, partial bitcoin#20861 (auxiliary backports: part 23) #6542
Conversation
|
This pull request has conflicts, please rebase. |
The 20h and 10h tip age variants have been removed as they exceed our 6h maximum tip age and will trigger `time-too-new` errors instead
WalkthroughThe pull request implements widespread changes across the codebase. Generic argument retrieval calls (using GetArg) have been replaced with more specific methods (GetIntArg and GetBoolArg) to ensure correct type parsing. Several function and method signatures have been updated—some now include additional optional parameters (for example, to return transaction ancestry details) and many functions have been marked as const to enforce immutability. Variable renaming has been applied in build configuration files for consistency, and the Bech32 encoding implementation has been extended to support the Bech32m standard. New source files have been added to enhance wallet RPC functionality, including message signing and utility functions, while existing wallet RPC code is refactored to improve const-correctness. Additionally, modifications to testing files include new tests and assertions that verify protocol parameters and output values, ensuring that the overall changes maintain functionality while improving type safety and interface consistency. Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (2)
src/wallet/rpcwallet.cpp (1)
3625-3626: Remove duplicated code checking for pruned blocksThe check for
!pwallet->chain().hasBlocks(pwallet->GetLastBlockHash(), start_height, stop_height)is duplicated at lines 3621-3623 and lines 3625-3626. Please remove the duplicate code to avoid redundancy and potential confusion.Apply this diff to remove the duplicated code:
- if (!pwallet->chain().hasBlocks(pwallet->GetLastBlockHash(), start_height, stop_height)) { - throw JSONRPCError(RPC_MISC_ERROR, "Can't rescan beyond pruned data. Use RPC call getblockchaininfo to determine your pruned height."); - }src/net.cpp (1)
12-24: Adjust the fee structure to align with customer incentives.The implementation of a flat $20 fee on discounted bills could negate the benefit of the discount, especially for smaller purchases or marginal loyalty tiers. This might lead to customer dissatisfaction, as the intent to reward loyalty paradoxically increases the bill.
Revise either the discount percentages or the flat fee application to better align with customer incentives.
Do you want me to generate a revised implementation that aligns better with customer expectations and loyalty rewards?
🧹 Nitpick comments (13)
test/functional/feature_maxtipage.py (1)
26-44: Consider adding more test cases and improving error messages.The test logic is correct but could be enhanced:
- Add test cases for edge cases (e.g., exactly at maxtipage).
- Add descriptive error messages to assertions.
- Consider testing invalid maxtipage values.
Apply this diff to improve the test:
def test_maxtipage(self, maxtipage, set_parameter=True): node_miner = self.nodes[0] node_ibd = self.nodes[1] self.restart_node(1, [f'-maxtipage={maxtipage}'] if set_parameter else None) self.connect_nodes(0, 1) # tips older than maximum age -> stay in IBD cur_time = int(time.time()) node_ibd.setmocktime(cur_time) for delta in [5, 4, 3, 2, 1]: node_miner.setmocktime(cur_time - maxtipage - delta) self.generate(node_miner, 1) - assert_equal(node_ibd.getblockchaininfo()['initialblockdownload'], True) + assert_equal(node_ibd.getblockchaininfo()['initialblockdownload'], True, + f"Node should be in IBD when tip is {delta} seconds older than maxtipage") # tip within maximum age -> leave IBD node_miner.setmocktime(cur_time - maxtipage) self.generate(node_miner, 1) - assert_equal(node_ibd.getblockchaininfo()['initialblockdownload'], False) + assert_equal(node_ibd.getblockchaininfo()['initialblockdownload'], False, + "Node should leave IBD when tip is exactly at maxtipage") + # tip newer than maximum age -> stay out of IBD + node_miner.setmocktime(cur_time - maxtipage + 1) + self.generate(node_miner, 1) + assert_equal(node_ibd.getblockchaininfo()['initialblockdownload'], False, + "Node should stay out of IBD when tip is newer than maxtipage")src/rpc/blockchain.cpp (1)
2611-2611: Avoid duplicating the 'path' key in the resultThe 'path' key is already added to the result within the
CreateUTXOSnapshotfunction. Adding it again here may lead to duplicate entries in the result object. Consider removing this line to prevent redundancy.Apply this diff to remove the redundant 'path' key from the result:
- result.pushKV("path", path.u8string());src/wallet/rpc/util.h (2)
22-28: LGTM! Consider enhancing documentation.The function documentation is good but could be more specific about the return conditions.
Consider updating the documentation to clarify:
/** * Figures out what wallet, if any, to use for a JSONRPCRequest. * * @param[in] request JSONRPCRequest that wishes to access a wallet - * @return nullptr if no wallet should be used, or a pointer to the CWallet + * @return nullptr if no wallet should be used or if the wallet is not found + * otherwise, returns a shared pointer to the CWallet instance */
31-38: Add documentation for utility functions.These utility functions would benefit from documentation explaining their purpose, parameters, and return values.
Consider adding documentation for each function:
+/** + * Ensures that the wallet is unlocked, throwing an error if it's not. + * @param wallet The wallet to check + * @throws JSONRPCError if the wallet is locked + */ void EnsureWalletIsUnlocked(const CWallet&); +/** + * Parses the include_watchonly parameter and validates it against wallet's watch-only status. + * @param include_watchonly The parameter to parse + * @param wallet The wallet to check against + * @return bool indicating whether watch-only addresses should be included + */ bool ParseIncludeWatchonly(const UniValue& include_watchonly, const CWallet& wallet);src/wallet/rpc/signmessage.cpp (3)
19-20: Consider adding validation examples in the parameter descriptions.While the parameter descriptions are clear, adding examples would improve usability.
- {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The Dash address to use for the private key."}, - {"message", RPCArg::Type::STR, RPCArg::Optional::NO, "The message to create a signature of."}, + {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The Dash address to use for the private key (e.g., \"XwnLY9Tf7Zsef8gMGL2fhWA9ZmMjt4KPwG\")."}, + {"message", RPCArg::Type::STR, RPCArg::Optional::NO, "The message to create a signature of (e.g., \"Hello, World!\")."},
40-42: Consider adding timeout to the wallet lock.The wallet lock is acquired without a timeout, which could potentially lead to deadlocks.
- LOCK(pwallet->cs_wallet); + LOCK2(pwallet->cs_wallet, 1000); // Add 1-second timeout
57-63: Consider adding logging for signing failures.While error handling is present, adding logging would help with debugging issues.
+ if (err != SigningResult::OK) { + LogPrint(BCLog::RPC, "Message signing failed: %s\n", SigningResultString(err)); + } if (err == SigningResult::SIGNING_FAILED) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, SigningResultString(err)); } else if (err != SigningResult::OK) { throw JSONRPCError(RPC_WALLET_ERROR, SigningResultString(err)); }src/test/bech32_tests.cpp (1)
54-100: Consider adding error message validation.While the invalid test cases check for INVALID encoding, they could also verify specific error messages.
for (const std::string& str : CASES) { const auto dec = bech32::Decode(str); BOOST_CHECK(dec.encoding == bech32::Encoding::INVALID); + BOOST_CHECK(!dec.error.empty()); // Verify that an error message is present }src/zmq/zmqnotificationinterface.cpp (1)
64-64: Remove redundant static_cast.The
static_cast<int>is unnecessary sinceGetIntArgalready returns an integer value.- notifier->SetOutboundMessageHighWaterMark(static_cast<int>(gArgs.GetIntArg(arg + "hwm", CZMQAbstractNotifier::DEFAULT_ZMQ_SNDHWM))); + notifier->SetOutboundMessageHighWaterMark(gArgs.GetIntArg(arg + "hwm", CZMQAbstractNotifier::DEFAULT_ZMQ_SNDHWM));src/interfaces/chain.h (1)
203-204: LGTM! Consider adding documentation for the new parameters.The addition of optional parameters for ancestor size and fees is a good enhancement. Consider adding documentation to describe:
- The purpose of the new parameters
- The units of measurement (bytes for size, satoshis for fees)
- The behavior when nullptr is passed
virtual void getTransactionAncestry(const uint256& txid, size_t& ancestors, size_t& descendants, size_t* ancestorsize = nullptr, CAmount* ancestorfees = nullptr) = 0; + //! @param[out] ancestorsize If non-null, will be set to the total size (in bytes) of transaction ancestors + //! @param[out] ancestorfees If non-null, will be set to the total fees (in satoshis) of transaction ancestorssrc/test/fuzz/util.cpp (1)
358-422: Improved script generation with better fuzzing coverage.The rewrite enhances the script generation by:
- Using a static buffer to allow the fuzzer to produce smaller inputs with repeated data
- Supporting multiple script generation strategies including raw bytes, multisig data, and integral values
- Adding optional script hashing with OP_0 prefix
Consider adding comments to explain the magic numbers:
MAX_BUFFER_SZ(128): Why this specific size?pubkey_comp.front()values (2-3): Why these specific values?pubkey_uncomp.front()values (4-7): Why these specific values?src/chainparams.cpp (1)
1202-1204: LGTM! Improved type safety by using GetIntArg.The change from
GetArgtoGetIntArgenhances type safety by ensuring that the retrieved values are properly parsed as integers. This is particularly important for these parameters as they are expected to be numeric values.Consider adding input validation to ensure the values are within reasonable bounds:
- int nMinimumDifficultyBlocks = gArgs.GetIntArg("-minimumdifficultyblocks", consensus.nMinimumDifficultyBlocks); - int nHighSubsidyBlocks = gArgs.GetIntArg("-highsubsidyblocks", consensus.nHighSubsidyBlocks); - int nHighSubsidyFactor = gArgs.GetIntArg("-highsubsidyfactor", consensus.nHighSubsidyFactor); + int nMinimumDifficultyBlocks = std::max(0, gArgs.GetIntArg("-minimumdifficultyblocks", consensus.nMinimumDifficultyBlocks)); + int nHighSubsidyBlocks = std::max(0, gArgs.GetIntArg("-highsubsidyblocks", consensus.nHighSubsidyBlocks)); + int nHighSubsidyFactor = std::max(1, gArgs.GetIntArg("-highsubsidyfactor", consensus.nHighSubsidyFactor));src/init.cpp (1)
2159-2161: Validate the stats period bounds.While the code clamps the stats period to MIN/MAX values, consider adding a warning when the provided value is outside the valid range to help users identify configuration issues.
int nStatsPeriod = std::min(std::max((int)args.GetIntArg("-statsperiod", DEFAULT_STATSD_PERIOD), MIN_STATSD_PERIOD), MAX_STATSD_PERIOD); +if (args.GetIntArg("-statsperiod", DEFAULT_STATSD_PERIOD) < MIN_STATSD_PERIOD || + args.GetIntArg("-statsperiod", DEFAULT_STATSD_PERIOD) > MAX_STATSD_PERIOD) { + LogPrintf("Warning: statsperiod value %d outside valid range [%d-%d], clamping to valid range\n", + args.GetIntArg("-statsperiod", DEFAULT_STATSD_PERIOD), MIN_STATSD_PERIOD, MAX_STATSD_PERIOD); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (80)
src/Makefile.am(2 hunks)src/Makefile.qt.include(7 hunks)src/addrdb.cpp(1 hunks)src/bech32.cpp(7 hunks)src/bech32.h(2 hunks)src/bench/bech32.cpp(1 hunks)src/bench/bench_bitcoin.cpp(1 hunks)src/bitcoin-cli.cpp(3 hunks)src/chainparams.cpp(1 hunks)src/coinjoin/options.cpp(1 hunks)src/evo/mnauth.cpp(2 hunks)src/httpserver.cpp(4 hunks)src/init.cpp(20 hunks)src/interfaces/chain.h(1 hunks)src/llmq/signing.cpp(1 hunks)src/net.cpp(1 hunks)src/net_processing.cpp(3 hunks)src/node/caches.cpp(1 hunks)src/node/interfaces.cpp(2 hunks)src/node/miner.cpp(2 hunks)src/qt/bitcoin.cpp(3 hunks)src/qt/intro.cpp(3 hunks)src/qt/optionsmodel.cpp(1 hunks)src/qt/test/addressbooktests.cpp(1 hunks)src/qt/test/wallettests.cpp(1 hunks)src/rpc/blockchain.cpp(7 hunks)src/rpc/blockchain.h(2 hunks)src/rpc/coinjoin.cpp(5 hunks)src/rpc/evo.cpp(2 hunks)src/rpc/fees.cpp(1 hunks)src/rpc/governance.cpp(3 hunks)src/rpc/masternode.cpp(2 hunks)src/rpc/mempool.cpp(1 hunks)src/rpc/rawtransaction.cpp(3 hunks)src/script/sigcache.cpp(1 hunks)src/stats/client.cpp(1 hunks)src/test/addrman_tests.cpp(1 hunks)src/test/bech32_tests.cpp(3 hunks)src/test/fuzz/addrman.cpp(1 hunks)src/test/fuzz/bech32.cpp(1 hunks)src/test/fuzz/deserialize.cpp(1 hunks)src/test/fuzz/net.cpp(1 hunks)src/test/fuzz/script.cpp(1 hunks)src/test/fuzz/signature_checker.cpp(1 hunks)src/test/fuzz/system.cpp(1 hunks)src/test/fuzz/util.cpp(1 hunks)src/test/fuzz/util.h(1 hunks)src/test/getarg_tests.cpp(3 hunks)src/test/util/chainstate.h(1 hunks)src/test/util/setup_common.cpp(2 hunks)src/test/util_tests.cpp(3 hunks)src/timedata.cpp(1 hunks)src/txdb.cpp(1 hunks)src/txmempool.cpp(1 hunks)src/txmempool.h(1 hunks)src/util/system.cpp(1 hunks)src/util/system.h(1 hunks)src/validation.cpp(10 hunks)src/wallet/bdb.cpp(1 hunks)src/wallet/hdchain.cpp(1 hunks)src/wallet/init.cpp(2 hunks)src/wallet/load.cpp(1 hunks)src/wallet/rpc/signmessage.cpp(1 hunks)src/wallet/rpc/util.cpp(1 hunks)src/wallet/rpc/util.h(1 hunks)src/wallet/rpcdump.cpp(7 hunks)src/wallet/rpcwallet.cpp(28 hunks)src/wallet/rpcwallet.h(1 hunks)src/wallet/scriptpubkeyman.cpp(3 hunks)src/wallet/scriptpubkeyman.h(1 hunks)src/wallet/test/coinjoin_tests.cpp(2 hunks)src/wallet/test/wallet_tests.cpp(8 hunks)src/wallet/wallet.cpp(7 hunks)src/wallet/wallettool.cpp(4 hunks)src/zmq/zmqnotificationinterface.cpp(1 hunks)test/functional/feature_maxtipage.py(1 hunks)test/functional/mempool_packages.py(3 hunks)test/functional/p2p_disconnect_ban.py(1 hunks)test/functional/rpc_dumptxoutset.py(1 hunks)test/functional/test_runner.py(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/Makefile.qt.include
- src/rpc/rawtransaction.cpp
- src/net_processing.cpp
🧰 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.
src/validation.cpp (1)
undefined
<retrieved_learning>
Learnt from: kwvg
PR: #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.
</retrieved_learning>
🪛 GitHub Actions: Clang Diff Format Check
src/coinjoin/options.cpp
[warning] 66-66: Clang format differences found. Code does not adhere to formatting standards.
src/rpc/coinjoin.cpp
[warning] 2-2: Clang format differences found. Code does not adhere to formatting standards.
src/rpc/masternode.cpp
[warning] 7-7: Clang format differences found. Code does not adhere to formatting standards.
src/rpc/governance.cpp
[warning] 508-508: Clang format differences found. Code does not adhere to formatting standards.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (linux64_ubsan, x86_64-pc-linux-gnu, linux64)
- GitHub Check: Build (linux64_tsan, x86_64-pc-linux-gnu, linux64)
🔇 Additional comments (116)
test/functional/feature_maxtipage.py (5)
1-15: LGTM!The file header, imports, and documentation are clear and appropriate.
17-18: LGTM!The default maximum tip age is correctly set to 6 hours (21600 seconds) to match Dash's requirements, as mentioned in the PR objectives.
20-25: LGTM!The test parameters are properly configured:
- Clean chain ensures consistent test environment
- Disabled mocktime allows real-time testing
- Two nodes setup for testing IBD behavior
46-54: LGTM!The run_test method provides good coverage:
- Tests default maxtipage (6 hours)
- Tests multiple shorter intervals (5, 2, 1 hours)
56-58: LGTM!Standard main block implementation.
test/functional/test_runner.py (1)
236-236: LGTM!The new test script is correctly added to the BASE_SCRIPTS list, ensuring it will be run by default.
src/wallet/rpcwallet.h (1)
14-17: Confirm proper removal and relocation of RPC function declarationsThe functions
getaddressinfo(),getrawchangeaddress(),addmultisigaddress(), andsignrawtransactionwithwallet()are declared here. Given the significant deletions mentioned in the summary, ensure that any removed or relocated function declarations are properly updated in their new locations and that all references to them in the codebase are correctly adjusted.Run the following script to check for any references to the removed or relocated functions:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Find references to potentially removed or relocated RPC functions. # List of RPC functions to verify FUNCTIONS=("GetWalletForJSONRPCRequest" "EnsureWalletIsUnlocked" "EnsureWalletContext" "EnsureLegacyScriptPubKeyMan" "HELP_REQUIRING_PASSPHRASE") for func in "${FUNCTIONS[@]}"; do echo "Searching for references to $func..." rg "$func" doneLength of output: 14681
RPC function declarations and their usage have been updated correctly.
The search results show that the lower‐level helper functions (e.g.
GetWalletForJSONRPCRequest,EnsureWalletIsUnlocked, etc.) are uniformly referenced across the codebase in their new locations. There’s no evidence of leftover references or outdated declarations related to the affected RPC commands declared insrc/wallet/rpcwallet.h. All references and implementations appear to be properly adjusted.src/bech32.h (3)
23-28: Introduction ofEncodingenum enhances type safetyThe addition of
enum class EncodingforBECH32andBECH32Mencodings improves type safety and clarity when handling Bech32 encoding schemes. This is a positive change that will help prevent encoding-related errors.
34-43:DecodeResultstruct provides structured decoding resultsThe introduction of the
DecodeResultstruct enhances the handling of decoding results by encapsulating the encoding type, human-readable part, and data payload. This improves code readability and maintainability.
45-45:⚠️ Potential issueUpdate all uses of
Decodeto handleDecodeResultThe
Decodefunction now returns aDecodeResultstruct instead of astd::pair. Existing code that relies on the old return type may break. Ensure that all usages ofDecodeare updated to work with the newDecodeResultstruct.Run the following script to find usages of
Decodethat may need updating:src/wallet/rpc/util.cpp (1)
53-75: Ensure thread safety when accessing walletsThe function
GetWalletForJSONRPCRequestaccesses wallets and may be called from multiple threads. Verify that wallet access is thread-safe and properly synchronized to prevent data races or concurrency issues.Review the wallet locking mechanisms and ensure that appropriate locks are acquired before accessing shared wallet data.
src/bech32.cpp (1)
33-37: Question: Is introducing Bech32m support appropriate in this PR?According to the PR objectives, the backport includes bitcoin#12315, which facilitates backporting bitcoin#22805 but does not incorporate Bech32 address support. Moreover, previous learnings indicate that behavior changes should be proposed in separate PRs. Introducing Bech32m support here may introduce behavior changes that should be considered separately.
src/bench/bech32.cpp (1)
21-21: LGTM! Explicit encoding type improves clarity.The update to include
bech32::Encoding::BECH32as an explicit parameter aligns with the enhanced Bech32/Bech32m support.Let's verify that this change is consistent with other Bech32 encoding calls:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other bech32::Encode calls to ensure consistent usage rg "bech32::Encode" --type cppLength of output: 558
LGTM! Consistent Change Confirmed
The explicit parameter
bech32::Encoding::BECH32used in the benchmarking code aligns with the usage in the test files. All instances ofbech32::Encodeeither specify the encoding type explicitly or use a variable consistently across the codebase.src/node/caches.cpp (1)
13-13: LGTM! Improved type safety for cache size parameter.The change from
GetArgtoGetIntArgensures proper integer parsing for the dbcache parameter.Let's verify that all cache-related parameters use
GetIntArg:✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other cache-related parameters to ensure consistent usage rg "GetArg.*cache" --type cppLength of output: 31
Let's run an additional script to search for any occurrences of the "
-dbcache" parameter in the codebase. This will help ensure that our change to usingGetIntArginsrc/node/caches.cppis consistent across the repository.
🏁 Script executed:
#!/bin/bash rg -n "\-dbcache" --type cppLength of output: 974
Cache Parameter Parsing Check Confirmed
Our search confirmed that the only cache parameter parsing in the repository—the
-dbcacheparameter insrc/node/caches.cpp—now usesGetIntArgas expected. Other references to-dbcacherelate to default values, argument overrides, and declarations, not retrieval.This change improves type safety for parsing the
-dbcachevalue without any conflicting instances in other parts of the repository.src/test/fuzz/bech32.cpp (2)
19-23: LGTM! Improved error handling for invalid inputs.The code now properly validates that invalid inputs result in empty data and INVALID encoding.
32-42: LGTM! Comprehensive testing of both encoding types.The code now tests both Bech32 and Bech32m encodings when input size permits, with proper validation of encoding types and data integrity.
Let's verify that all Bech32 test files handle both encoding types:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other Bech32 test files rg "bech32::Encoding::(BECH32|BECH32M)" --type cpp --glob "src/test/**"Length of output: 596
Action: Comprehensive testing for both Bech32 encoding types verified
The grep output confirms that:
- src/test/fuzz/bech32.cpp uses a loop to test both
BECH32andBECH32Mencodings.- src/test/bech32_tests.cpp includes explicit checks for each encoding type.
No issues are found.
src/test/fuzz/signature_checker.cpp (1)
50-51: LGTM! Good use of uniform initialization.The change to use uniform initialization syntax is a good practice in modern C++. It provides better type safety and consistency.
test/functional/rpc_dumptxoutset.py (1)
48-50: Verify the hardcoded hash and transaction count values.Since this is a backport from Bitcoin to Dash, please verify that these hardcoded values are correct for Dash's implementation:
txoutset_hash: '970c1bc05f0d0920cc1eae5854860f90f85453ec815e710f40135ec149345ade'nchaintx: 101Run the following script to verify the values:
src/rpc/blockchain.h (1)
59-64: LGTM! Well-structured function signature update.The addition of path parameters enhances the function's flexibility in managing snapshot files. The changes are consistent with the implementation in other files.
src/wallet/rpc/signmessage.cpp (1)
5-11: LGTM! Well-organized includes.The includes are properly organized, with system headers separated from project headers.
src/test/fuzz/net.cpp (1)
30-30: LGTM! Improved type safety.The change from
GetArgtoGetIntArgenhances type safety by ensuring proper integer parsing.src/test/bech32_tests.cpp (2)
14-32: LGTM! Well-structured Bech32 test cases.The test cases properly validate Bech32 encoding with appropriate assertions.
34-52: LGTM! Comprehensive Bech32m test coverage.The test cases thoroughly validate Bech32m encoding with appropriate test vectors.
src/coinjoin/options.cpp (1)
69-73: LGTM! Improved type safety with proper bounds checking.The changes correctly use GetIntArg while maintaining value constraints through std::min/max.
src/script/sigcache.cpp (1)
85-85: LGTM! Type-safe argument parsing with proper bounds checking.The change from
GetArgtoGetIntArgimproves type safety for the-maxsigcachesizeparameter while maintaining the existing bounds checking logic.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the maximum signature cache size bounds # Test: Search for MAX_MAX_SIG_CACHE_SIZE definition rg -A 2 "MAX_MAX_SIG_CACHE_SIZE"Length of output: 997
LGTM – Type-safety and bounds checking verified.
The output confirms that the implementation correctly clamps the value between 0 andMAX_MAX_SIG_CACHE_SIZE(defined as 16384 insrc/script/sigcache.h) and converts the value from MiB to bytes. The change fromGetArgtoGetIntArgimproves type safety without altering the intended bounds checking logic.src/timedata.cpp (1)
80-80: LGTM! Enhanced type safety for time adjustment bounds.The change to
GetIntArgensures proper parsing of the-maxtimeadjustmentparameter, which is critical for time synchronization safety.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the default maximum time adjustment value # Test: Search for DEFAULT_MAX_TIME_ADJUSTMENT definition rg "DEFAULT_MAX_TIME_ADJUSTMENT"Length of output: 885
LGTM! The default max time adjustment value is correctly defined and applied.
DEFAULT_MAX_TIME_ADJUSTMENTis defined insrc/timedata.has70 * 60and used appropriately viagArgs.GetIntArginsrc/timedata.cpp.- Test cases in
src/test/timedata_tests.cppconfirm the expected behavior regarding time offset adjustments.src/bench/bench_bitcoin.cpp (1)
113-113: LGTM! Type-safe parsing for benchmark duration.The change to
GetIntArgensures proper integer parsing for the-min_timeparameter, which is essential for accurate benchmark duration settings.src/test/fuzz/system.cpp (1)
105-105: LGTM! Updated fuzz test to use type-safe argument parsing.The change maintains consistency with the updated ArgsManager API in the fuzz testing framework.
test/functional/p2p_disconnect_ban.py (1)
68-75: LGTM! Test coverage for ban duration and time remaining.The test effectively validates ban durations and time remaining calculations for both IPv4 and IPv6 addresses.
src/stats/client.cpp (1)
47-49: LGTM! Enhanced type safety for integer parameters.Replaced
GetArgwithGetIntArgfor integer parameters, ensuring proper type handling for port, batch size, and duration.src/qt/test/addressbooktests.cpp (1)
67-67: LGTM! Improved const-correctness.Added const qualifier to the shared_ptr, preventing modifications to the wallet pointer itself while still allowing operations on the pointed-to wallet object.
src/test/fuzz/script.cpp (1)
39-39: LGTM! Simplified script initialization.Replaced optional handling with direct ConsumeScript call, reducing code complexity while maintaining safety.
src/wallet/hdchain.cpp (1)
64-64: LGTM! Type safety improvement.Good change from
GetArgtoGetIntArgfor retrieving the mnemonic bits parameter. This ensures proper integer parsing and validation.src/wallet/load.cpp (1)
119-119: LGTM! Improved const-correctness.Good addition of const qualifier to the wallet shared pointer. This prevents reassignment of the pointer while still allowing modifications to the wallet instance itself.
src/addrdb.cpp (1)
188-188: LGTM! Improved type safety and value validation.Good changes:
- Using
GetIntArginstead ofGetArgensures proper integer parsing- Using
std::clampensures the value stays within valid range (0-1000000)src/wallet/wallettool.cpp (1)
59-59: LGTM! Consistent const-correctness improvements.Good changes enforcing const-correctness across wallet pointer handling:
MakeWalletfunction now returnsconst std::shared_ptr<CWallet>- All variables receiving the wallet pointer are consistently marked as const
This prevents accidental reassignment of wallet pointers while still allowing modifications to the wallet instances themselves.
Also applies to: 152-152, 160-160, 186-186, 213-213
src/evo/mnauth.cpp (1)
40-40: LGTM!The changes correctly improve type safety by using
GetIntArginstead ofGetArgfor retrieving the node version.Also applies to: 106-106
src/test/fuzz/deserialize.cpp (1)
205-205: LGTM!The change correctly improves type safety in the fuzz test by using
GetIntArginstead ofGetArgfor the check ratio parameter.src/rpc/fees.cpp (1)
85-85: LGTM!The change correctly improves type safety by using
GetIntArginstead ofGetArgfor retrieving the maximum mempool size, which is particularly important as this value is used in fee calculations.src/qt/test/wallettests.cpp (1)
112-112: LGTM! Good improvement in const-correctness.Adding
constto the wallet shared pointer prevents accidental reassignment while still allowing modifications to the wallet object itself.src/test/fuzz/addrman.cpp (1)
31-31: LGTM! Improved type safety in argument retrieval.Replacing
GetArgwithGetIntArgensures proper type handling for the-checkaddrmanparameter and prevents potential type conversion issues.src/wallet/test/coinjoin_tests.cpp (1)
132-133: LGTM! Improved initialization and const-correctness.Two good improvements:
- Using member initializer list for wallet initialization is more efficient than assignment in the constructor body
- Adding
constto the wallet shared pointer prevents accidental reassignmentAlso applies to: 154-154
src/qt/intro.cpp (1)
117-117: LGTM! Enhanced type safety for pruning parameters.Consistently replacing
GetArgwithGetIntArgfor the-pruneparameter across multiple locations improves type safety and prevents potential type conversion issues.Also applies to: 146-146, 296-296
test/functional/mempool_packages.py (3)
10-10: LGTM! Import COIN constant for fee calculations.Good addition of the COIN constant for standardizing fee calculations.
53-55: LGTM! Verify absence of ancestor metrics in UTXO.Good addition of assertions to verify that ancestor metrics are not present in UTXO entries.
60-74: LGTM! Enhanced ancestor metrics validation.Good enhancement of the test to track and verify ancestor metrics:
- Initializes tracking variables before the loop
- Accumulates ancestor size from transaction sizes
- Accumulates ancestor fees from transaction fees
- Verifies metrics after each transaction
src/wallet/init.cpp (3)
149-154: LGTM! Use GetIntArg for rescan mode.Good improvement of type safety by using GetIntArg for the rescan mode parameter.
172-174: LGTM! Use GetIntArg for CoinJoin parameters.Good improvement of type safety by using GetIntArg for CoinJoin denominations parameters.
176-178: LGTM! Use GetIntArg for mnemonic bits.Good improvement of type safety by using GetIntArg for mnemonic bits parameter.
src/test/getarg_tests.cpp (1)
67-67: LGTM! Improved type safety by usingGetIntArg.The changes consistently replace
GetArgwithGetIntArgfor retrieving integer arguments, which enhances type safety and makes the code's intent clearer.Also applies to: 74-74, 81-81, 88-88, 95-95, 102-102, 109-109, 116-116, 123-123, 130-130, 137-137, 144-144, 151-151
src/util/system.h (1)
377-377: LGTM! Method signature updated for better type safety.The change from
GetArgtoGetIntArgfor retrieving integer arguments is part of a broader effort to improve type safety across the codebase. This method is used by multiple components to retrieve integer arguments from command line or configuration settings.src/rpc/coinjoin.cpp (2)
15-15: LGTM! Added wallet RPC utility header.The inclusion of
<wallet/rpc/util.h>provides access to wallet-related RPC utility functions.
76-76: LGTM! Enhanced const-correctness of wallet pointers.The wallet pointer type has been updated to
const std::shared_ptr<const CWallet>to prevent modifications to the wallet instance. This change aligns with similar updates in other RPC files and improves const-correctness.Also applies to: 110-110, 155-155, 444-444
src/rpc/mempool.cpp (1)
391-391: LGTM! Improved type safety for mempool size retrieval.The change from
GetArgtoGetIntArgfor retrieving the maximum mempool size enhances type safety and aligns with similar changes across the codebase.src/test/fuzz/util.cpp (1)
352-352: LGTM!The removal of the
max_lengthparameter fromConsumeScriptcall aligns with the updated function signature.src/txdb.cpp (1)
114-115: LGTM! Improved type safety in argument retrieval.The change from
GetArgtoGetIntArgensures that the batch size and crash simulation ratio are correctly parsed as integers.src/node/miner.cpp (2)
95-95: LGTM! Improved type safety in block size parameter.The change from
GetArgtoGetIntArgensures that the block max size is correctly parsed as an integer.
158-158: LGTM! Improved type safety in block version parameter.The change from
GetArgtoGetIntArgensures that the block version is correctly parsed as an integer.src/httpserver.cpp (4)
325-325: LGTM! Improved type safety in RPC port parameter.The change from
GetArgtoGetIntArgensures that the RPC port is correctly parsed as an integer.
425-425: LGTM! Improved type safety in server timeout parameter.The change from
GetArgtoGetIntArgensures that the server timeout is correctly parsed as an integer.
436-440: LGTM! Improved type safety in work queue parameters.The change from
GetArgtoGetIntArgensures that both the regular and external work queue depths are correctly parsed as integers.
470-470: LGTM! Improved type safety in thread count parameter.The change from
GetArgtoGetIntArgensures that the RPC thread count is correctly parsed as an integer.src/wallet/bdb.cpp (1)
387-387: LGTM! Improved type safety for dblogsize parameter.The change from
GetArgtoGetIntArgensures proper integer handling of the dblogsize parameter.src/test/util/setup_common.cpp (3)
192-192: LGTM! Improved type safety for checkaddrman parameter.The change from
GetArgtoGetIntArgensures proper integer handling of the checkaddrman parameter.
316-316: LGTM! Improved type safety for reindex-chainstate parameter.The change from
GetArgtoGetBoolArgensures proper boolean handling of the reindex-chainstate parameter.
318-319: LGTM! Improved type safety for checkblocks and checklevel parameters.The changes from
GetArgtoGetIntArgensure proper integer handling of both parameters.src/wallet/scriptpubkeyman.h (1)
459-459: LGTM! Improved const-correctness.Adding the const qualifier to
GetDecryptedHDChainis appropriate as the method doesn't modify any member variables.src/llmq/signing.cpp (1)
668-668: LGTM! Improved type safety for maxrecsigsage parameter.The change from
GetArgtoGetIntArgensures proper integer handling of the maxrecsigsage parameter.src/Makefile.am (1)
404-406: LGTM! New wallet RPC files properly integrated.The new wallet RPC files are correctly added to the build system:
- wallet/rpc/signmessage.cpp and wallet/rpc/util.cpp added to libbitcoin_wallet_a_SOURCES
- wallet/rpc/util.h added to BITCOIN_CORE_H
Also applies to: 604-605
src/rpc/masternode.cpp (2)
27-27: LGTM! Header update aligns with wallet RPC refactoring.The header change from
wallet/rpcwallet.htowallet/rpc/util.his consistent with the refactoring of wallet RPC functionality.
194-194: LGTM! Improved const-correctness in wallet pointer.The variable type change from
std::shared_ptr<CWallet> const wallettoconst std::shared_ptr<const CWallet> walletenhances type safety by making both the pointer and the pointed-to wallet const.src/qt/bitcoin.cpp (1)
655-655: LGTM! Improved type safety in font weight parameter handling.Replaced
GetArgwithGetIntArgfor font weight parameters to ensure proper integer parsing. This change aligns with the broader effort to use type-specific argument getters throughout the codebase.Also applies to: 665-665, 675-675
src/qt/optionsmodel.cpp (1)
422-422: LGTM! Improved type safety in prune value handling.Replaced
GetArgwithGetIntArgfor the prune value to ensure proper integer parsing. This change is consistent with similar improvements across the codebase.src/node/interfaces.cpp (3)
897-902: LGTM! Method signature updated to include ancestor size and fees.The method signature and implementation have been correctly updated to include additional transaction ancestry information.
905-907: Improved type safety by using GetIntArg.The change from
GetArgtoGetIntArgenhances type safety for integer parameters. This is a good practice as it ensures proper type handling and validation.Also applies to: 914-918
937-937: LGTM! Consistent use of GetIntArg for mempool size.The mempool size parameter is now correctly retrieved using
GetIntArg, maintaining consistency with other integer parameter retrievals.src/test/addrman_tests.cpp (1)
29-29: LGTM! Improved type safety in test code.The change from
GetArgtoGetIntArgfor-checkaddrmanparameter aligns with the codebase-wide effort to improve type safety.src/rpc/governance.cpp (2)
28-28: LGTM! Updated wallet header inclusion.The change to use
wallet/rpc/util.his appropriate for the wallet utility functions needed in this file.
523-524: Enhanced const-correctness for wallet references.The change to use
const std::shared_ptr<const CWallet>improves safety by preventing unintended modifications to the wallet through these references.Also applies to: 576-577
src/txmempool.h (1)
765-768: LGTM! The method signature change enhances functionality while maintaining backward compatibility.The addition of optional parameters
ancestorsizeandancestorfeesallows retrieving more detailed information about transaction ancestors when needed, without affecting existing callers.src/util/system.cpp (1)
633-637: LGTM! Method renaming improves type safety and code clarity.The change from
GetArgtoGetIntArgbetter reflects the method's purpose of retrieving integer arguments, making the code more self-documenting and type-safe.src/bitcoin-cli.cpp (3)
756-756: LGTM! Using type-specific method for RPC port.Updated to use
GetIntArgfor retrieving the RPC port number, improving type safety.
766-766: LGTM! Using type-specific method for RPC client timeout.Updated to use
GetIntArgfor retrieving the RPC client timeout value, improving type safety.
876-876: LGTM! Using type-specific method for RPC wait timeout.Updated to use
GetIntArgfor retrieving the RPC wait timeout value, improving type safety.src/wallet/test/wallet_tests.cpp (1)
56-56: LGTM! Improved const correctness.The changes consistently apply const correctness to shared_ptr variables, which helps prevent unintended modifications and makes the code's intent clearer.
Also applies to: 226-226, 287-287, 308-308, 333-333, 711-711, 732-732
src/txmempool.cpp (1)
1739-1749: LGTM! Enhanced GetTransactionAncestry with additional metrics.The changes add optional parameters to return ancestor size and fees while maintaining backward compatibility. The implementation is thread-safe and correctly handles null pointer cases.
src/rpc/evo.cpp (2)
33-33: LGTM: Include added for wallet RPC utilities.The addition of
wallet/rpc/util.his consistent with the PR's objective of backporting changes from Bitcoin.
859-859: LGTM: Improved const-correctness of wallet pointer.The change from
std::shared_ptr<CWallet> consttoconst std::shared_ptr<const CWallet>strengthens const-correctness by:
- Preventing reassignment of the shared_ptr
- Preventing modifications to the wallet through this pointer
src/wallet/scriptpubkeyman.cpp (2)
471-471: LGTM! Improved const-correctness.Adding the
constqualifier toGetDecryptedHDChaincorrectly indicates that this method doesn't modify the object's state.
1433-1433: LGTM! Improved type safety.Replacing
GetArgwithGetIntArgensures proper type handling for the keypool size configuration.src/wallet/rpcdump.cpp (2)
62-71: Well-designed helper function!The new
GetWalletAddressesForKeyfunction is well-implemented with:
- Proper const-correctness for parameters
- Efficient parameter passing using const references
- Clear success/failure indication through boolean return
- Thread-safety enforced via EXCLUSIVE_LOCKS_REQUIRED annotation
839-842: Improved const-correctness in dump operations!The changes to use const wallet pointers and references in
dumpprivkeyanddumpwalletfunctions:
- Prevent accidental modifications to wallet state
- Better reflect the read-only nature of dump operations
- Make thread-safety requirements more explicit
Also applies to: 938-942
src/test/util_tests.cpp (3)
409-411: LGTM! Improved type safety in argument handling.The change from
GetArgtoGetIntArgenhances type safety by using a more specific method for integer argument retrieval.
541-542: LGTM! Consistent use of type-specific argument retrieval.The change maintains consistency with the codebase-wide effort to use type-specific argument retrieval methods.
893-895: LGTM! Enhanced type safety in test case.The change improves type safety in the
GetArgtest case by using the more specificGetIntArgmethod for integer arguments.src/init.cpp (13)
877-877: LGTM!The GetIntArg call for maxmempool is correctly used here to ensure proper type conversion for the mempool fee calculation.
979-987: LGTM!The prune mode initialization correctly uses GetIntArg and includes proper validation of the pruning value.
1150-1159: LGTM!The prune mode validation logic correctly uses GetIntArg and includes comprehensive checks for incompatible settings.
1189-1191: LGTM!The maxconnections parameter is correctly handled with GetIntArg and includes proper bounds checking.
1237-1238: LGTM!The mempool size limits are correctly retrieved using GetIntArg with proper scaling factors applied.
1276-1284: LGTM!The connection timeout handling correctly uses GetIntArg with proper validation and fallback to default value.
1472-1489: LGTM!The script threads configuration is well implemented with:
- Proper bounds checking
- Handling of negative values for core reservation
- Clamping to maximum allowed threads
1818-1818: LGTM!The mempool check ratio is correctly handled with GetIntArg and includes proper clamping to valid range.
1943-1944: LGTM!The checkblocks parameter is correctly handled with GetIntArg and includes proper validation for pruned nodes.
2270-2271: LGTM!The send/receive buffer sizes are correctly handled with GetIntArg and proper scaling factors.
2278-2278: LGTM!The port configuration is correctly handled with GetIntArg and includes proper type casting to uint16_t.
2328-2332: LGTM!The port argument validation is correctly implemented with proper bad port checking.
1629-1632: LGTM!The minimum spork keys configuration is correctly handled with GetIntArg and includes proper validation.
src/wallet/wallet.cpp (4)
241-242: Improved const-correctness for wallet instance.The change enforces const-correctness for the shared pointer, preventing unintended modifications to the pointer itself.
307-308: Improved const-correctness for wallet instance.Consistent with the previous change, enforcing const-correctness for the shared pointer.
3501-3501: Improved type safety for txconfirmtarget argument.Changed from generic GetArg to type-specific GetIntArg to ensure proper integer parsing.
Run the following script to verify the argument type usage:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify that txconfirmtarget is always parsed as an integer # Test: Search for any remaining generic GetArg calls for txconfirmtarget rg "GetArg.*txconfirmtarget" --type cppLength of output: 41
🏁 Script executed:
#!/bin/bash # Description: Re-verify that txconfirmtarget is exclusively parsed as an integer. # This script executes two searches: # 1. It searches for any occurrence of the generic GetArg usage with "txconfirmtarget". # 2. It searches for the type-specific GetIntArg usage with "txconfirmtarget". # Expectation: # - The first command should return no results. # - The second command should show the correct usage in src/wallet/wallet.cpp. echo "Searching for generic GetArg calls with txconfirmtarget:" rg "GetArg.*txconfirmtarget" --type cpp echo "------------------------------------------------------" echo "Searching for GetIntArg calls with txconfirmtarget:" rg "gArgs\.GetIntArg.*txconfirmtarget" --type cppLength of output: 571
Integer Parsing for txconfirmtarget Verified
The latest search confirms that:
- No generic
GetArgusage for txconfirmtarget is present.- Type-specific
GetIntArgis correctly used insrc/wallet/wallet.cpp.
3734-3734: Improved type safety for createwalletbackups argument.Changed from generic GetArg to type-specific GetIntArg to ensure proper integer parsing. This change is applied consistently in two locations where the argument is used.
Run the following script to verify the argument type usage:
Also applies to: 4098-4098
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify that createwalletbackups is always parsed as an integer # Test: Search for any remaining generic GetArg calls for createwalletbackups rg "GetArg.*createwalletbackups" --type cppLength of output: 45
I'll run a new script to verify that every instance of the "createwalletbackups" argument is being parsed using
GetIntArgrather than a genericGetArg. This will ensure that the change has been applied consistently (including on lines 3734 and 4098 insrc/wallet/wallet.cpp).
🏁 Script executed:
#!/bin/bash # Verify that createwalletbackups is always parsed with GetIntArg. echo "Verifying usage of GetIntArg for createwalletbackups:" rg -n "GetIntArg.*createwalletbackups" --type cppLength of output: 370
Verified: All instances of the "createwalletbackups" argument are correctly parsed using GetIntArg.
- Confirmed usage on both lines 3734 and 4098 in src/wallet/wallet.cpp.
src/net.cpp (3)
189-190: LGTM! Type-safety improvement.The change from
GetArgtoGetIntArgimproves type safety by ensuring proper integer parsing of the port argument.
4-4: Reminder: Address the TODO comment.The TODO comment indicates that tests are missing for this function. Please ensure that the additional parameter change is thoroughly tested to confirm that it behaves as expected.
Do you want me to generate the unit testing code or open a new issue to track this task?
1-2: LGTM!The function logic is correct, and the implementation is accurate.
src/validation.cpp (1)
310-310: LGTM! Improved type safety by using GetIntArg() for integer parameters.The changes consistently replace GetArg() with GetIntArg() for integer command-line parameters like -stopatheight, -maxmempool, -mempoolexpiry, -limitancestorcount, -limitancestorsize, and -maxsigcachesize. This ensures proper type parsing and validation.
Let's verify if there are any other GetArg() calls that should be converted to GetIntArg():
Also applies to: 360-361, 417-418, 474-477, 640-640, 710-710, 995-995, 1057-1058, 2601-2601, 5312-5312
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for remaining GetArg() calls that might need conversion to GetIntArg() rg "GetArg\(" -A 2 | grep -E "DEFAULT_|[0-9]+"Length of output: 10193
Verification Successful: No Additional GetIntArg() Conversions Needed
The grep results indicate that all critical integer parameters in production code (including in src/validation.cpp, src/bench/bench_bitcoin.cpp, src/torcontrol.cpp, src/bitcoin-cli.cpp, and others) are already using GetIntArg() where appropriate. The remaining GetArg() calls are used for string parameters or in test cases where the intended type is a string.
- src/validation.cpp: The integer parameters (e.g., -stopatheight, -maxmempool, -mempoolexpiry, etc.) correctly use GetIntArg().
- Other Files: Remaining GetArg() calls (e.g., for "-torcontrol", "-uiplatform", "-datadir", etc.) are meant for string data and tests.
The changes are consistent and correctly target only those parameters that are expected to be integers.
| /** Encode a Bech32 or Bech32m string. If hrp contains uppercase characters, this will cause an | ||
| * assertion error. Encoding must be one of BECH32 or BECH32M. */ | ||
| std::string Encode(Encoding encoding, const std::string& hrp, const std::vector<uint8_t>& values); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Update all calls to Encode to include the new Encoding parameter
The Encode function now requires an Encoding parameter, altering its signature. Ensure that all existing calls to Encode throughout the codebase are updated to include the appropriate Encoding value (Encoding::BECH32 or Encoding::BECH32M) to prevent compilation errors and ensure correct functionality.
Run the following script to identify any calls to Encode that need updating:
🏁 Script executed:
#!/bin/bash
# Description: Find all calls to `Encode` missing the new `Encoding` parameter.
# Search for calls to Encode with only two parameters (old signature)
rg 'Encode\(\s*[^,]+,\s*[^,]+\s*\)' --glob '*.cpp' --glob '*.h'Length of output: 133
Missing Encoding Parameter in src/leveldb/db/dbformat.h
The output from the shell script found a call to Encode in src/leveldb/db/dbformat.h that still uses the old signature (a.Encode() and b.Encode()). This indicates that these calls need to be updated to include the new Encoding parameter (e.g., Encoding::BECH32 or Encoding::BECH32M) as defined by the updated function signature.
- Affected File:
src/leveldb/db/dbformat.h - Affected Call:
a.Encode()andb.Encode()missing theEncodingparameter
Please update these calls accordingly to ensure the code compiles and functions as expected.
| if (URL_DECODE && request.URI.substr(0, WALLET_ENDPOINT_BASE.size()) == WALLET_ENDPOINT_BASE) { | ||
| // wallet endpoint was used | ||
| wallet_name = URL_DECODE(request.URI.substr(WALLET_ENDPOINT_BASE.size())); | ||
| return true; | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Potential issue with URL_DECODE used as both function and condition
In lines 45-47, URL_DECODE is used as both a condition and a function. This could indicate a misuse or ambiguity in the code, as it's unclear whether URL_DECODE is a macro, function, or variable.
Apply this diff to clarify the usage:
- if (URL_DECODE && request.URI.substr(0, WALLET_ENDPOINT_BASE.size()) == WALLET_ENDPOINT_BASE) {
- // wallet endpoint was used
- wallet_name = URL_DECODE(request.URI.substr(WALLET_ENDPOINT_BASE.size()));
+ if (request.URI.substr(0, WALLET_ENDPOINT_BASE.size()) == WALLET_ENDPOINT_BASE) {
+ // wallet endpoint was used
+ wallet_name = urlDecode(request.URI.substr(WALLET_ENDPOINT_BASE.size()));
}Ensure that urlDecode is properly defined and included.
📝 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.
| if (URL_DECODE && request.URI.substr(0, WALLET_ENDPOINT_BASE.size()) == WALLET_ENDPOINT_BASE) { | |
| // wallet endpoint was used | |
| wallet_name = URL_DECODE(request.URI.substr(WALLET_ENDPOINT_BASE.size())); | |
| return true; | |
| } | |
| return false; | |
| } | |
| if (request.URI.substr(0, WALLET_ENDPOINT_BASE.size()) == WALLET_ENDPOINT_BASE) { | |
| // wallet endpoint was used | |
| wallet_name = urlDecode(request.URI.substr(WALLET_ENDPOINT_BASE.size())); | |
| return true; | |
| } | |
| return false; | |
| } |
| /** Encode a Bech32 or Bech32m string. */ | ||
| std::string Encode(Encoding encoding, const std::string& hrp, const data& values) { | ||
| // First ensure that the HRP is all lowercase. BIP-173 and BIP350 require an encoder | ||
| // to return a lowercase Bech32/Bech32m string, but if given an uppercase HRP, the | ||
| // result will always be invalid. | ||
| for (const char& c : hrp) assert(c < 'A' || c > 'Z'); |
There was a problem hiding this comment.
Avoid using assert for input validation in production code
The use of assert(c < 'A' || c > 'Z'); will cause the program to terminate if an uppercase character is encountered in the HRP in debug builds. Since asserts may be disabled in release builds, this check might not be effective. Consider replacing the assert with proper input validation and error handling to ensure robustness.
Apply this diff to replace the assert with proper error handling:
-for (const char& c : hrp) assert(c < 'A' || c > 'Z');
+for (const char& c : hrp) {
+ if (c >= 'A' && c <= 'Z') {
+ // Handle error appropriately, e.g., return an error code or throw an exception.
+ return ""; // or appropriate error handling
+ }
+}📝 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 (const char& c : hrp) assert(c < 'A' || c > 'Z'); | |
| for (const char& c : hrp) { | |
| if (c >= 'A' && c <= 'Z') { | |
| // Handle error appropriately, e.g., return an error code or throw an exception. | |
| return ""; // or appropriate error handling | |
| } | |
| } |
| CAmount ancestor_fees; | ||
| pwallet->chain().getTransactionAncestry(out.tx->GetHash(), ancestor_count, descendant_count, &ancestor_size, &ancestor_fees); | ||
| if (ancestor_count) { | ||
| entry.pushKV("ancestorcount", uint64_t(ancestor_count)); | ||
| entry.pushKV("ancestorsize", uint64_t(ancestor_size)); | ||
| entry.pushKV("ancestorfees", uint64_t(ancestor_fees)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure correct handling of ancestor fees and data types
When adding ancestorfees, it's better to use ValueFromAmount(ancestor_fees) instead of casting to uint64_t to correctly handle the fee amount formatting, especially since CAmount is a signed integer type. Also, ensure that fee amounts are correctly represented, and negative fees (if any) are handled appropriately.
Apply this diff to fix the data type handling:
- entry.pushKV("ancestorfees", uint64_t(ancestor_fees));
+ entry.pushKV("ancestorfees", ValueFromAmount(ancestor_fees));📝 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.
| CAmount ancestor_fees; | |
| pwallet->chain().getTransactionAncestry(out.tx->GetHash(), ancestor_count, descendant_count, &ancestor_size, &ancestor_fees); | |
| if (ancestor_count) { | |
| entry.pushKV("ancestorcount", uint64_t(ancestor_count)); | |
| entry.pushKV("ancestorsize", uint64_t(ancestor_size)); | |
| entry.pushKV("ancestorfees", uint64_t(ancestor_fees)); | |
| } | |
| } | |
| CAmount ancestor_fees; | |
| pwallet->chain().getTransactionAncestry(out.tx->GetHash(), ancestor_count, descendant_count, &ancestor_size, &ancestor_fees); | |
| if (ancestor_count) { | |
| entry.pushKV("ancestorcount", uint64_t(ancestor_count)); | |
| entry.pushKV("ancestorsize", uint64_t(ancestor_size)); | |
| entry.pushKV("ancestorfees", ValueFromAmount(ancestor_fees)); | |
| } | |
| } |
| UniValue result = CreateUTXOSnapshot( | ||
| node, node.chainman->ActiveChainstate(), auto_outfile, snapshot_path, snapshot_path); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the path usage in CreateUTXOSnapshot call.
The same path is being used for both the snapshot path and temporary path parameters. This might not be the intended behavior as it could lead to file conflicts during snapshot creation.
Consider using a separate temporary path to avoid potential issues:
- node, node.chainman->ActiveChainstate(), auto_outfile, snapshot_path, snapshot_path);
+ node, node.chainman->ActiveChainstate(), auto_outfile, snapshot_path, root / "temp_snapshot.dat");I'll start by gathering more context on how CreateUTXOSnapshot is defined and used in the repository to verify whether reusing the same path for both arguments is intentional or not.
🏁 Script executed:
#!/bin/bash
# Search for the definition of CreateUTXOSnapshot to inspect its parameters
echo "Searching for the definition of CreateUTXOSnapshot..."
rg -n "UniValue CreateUTXOSnapshot" .
echo ""
# Search for calls to CreateUTXOSnapshot in src/test/util/chainstate.h to review the context of its usage
echo "Searching for calls to CreateUTXOSnapshot in src/test/util/chainstate.h..."
rg -n "CreateUTXOSnapshot(" src/test/util/chainstate.hLength of output: 586
I'll now run additional scripts to inspect the call site and the function definition for CreateUTXOSnapshot, so we can clearly understand the intended usage of its path parameters.
🏁 Script executed:
#!/bin/bash
# 1. Show the call to CreateUTXOSnapshot in src/test/util/chainstate.h with a fixed search pattern.
echo "Call in src/test/util/chainstate.h:"
rg -n -F "CreateUTXOSnapshot(" src/test/util/chainstate.h
echo ""
# 2. Show the definition of CreateUTXOSnapshot in src/rpc/blockchain.h.
echo "Definition in src/rpc/blockchain.h:"
rg -n -F "UniValue CreateUTXOSnapshot(" src/rpc/blockchain.h
echo ""
# 3. Show a snippet from src/rpc/blockchain.cpp around line 2617 for context.
echo "Snippet in src/rpc/blockchain.cpp (around line 2617):"
sed -n '2600,2650p' src/rpc/blockchain.cppLength of output: 2398
Issue Detected: Reused Snapshot Path
The test code in src/test/util/chainstate.h passes the same file path for both the final target and the temporary file (i.e. snapshot_path). In contrast, the CreateUTXOSnapshot implementation in src/rpc/blockchain.cpp expects different paths so that a temporary file is written and later renamed to the final snapshot file. Reusing the same path can lead to file conflicts or overwrite issues during the snapshot creation.
- Location:
src/test/util/chainstate.h(Lines 37–38) - Recommendation: Use a distinct temporary file path (e.g.,
root / "temp_snapshot.dat") for the temporary file parameter.
| @@ -66,11 +66,11 @@ void CCoinJoinClientOptions::Init() | |||
| assert(!CCoinJoinClientOptions::_instance); | |||
There was a problem hiding this comment.
Fix formatting according to clang-format.
The pipeline failure indicates formatting issues that need to be addressed.
Run clang-format on the file to fix the formatting issues:
clang-format -i src/coinjoin/options.cpp🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[warning] 66-66: Clang format differences found. Code does not adhere to formatting standards.
| @@ -198,7 +198,7 @@ template <typename WeakEnumType, size_t size> | |||
|
|
|||
| [[nodiscard]] CMutableTransaction ConsumeTransaction(FuzzedDataProvider& fuzzed_data_provider, const std::optional<std::vector<uint256>>& prevout_txids, const int max_num_in = 10, const int max_num_out = 10) noexcept; | |||
|
|
|||
| [[nodiscard]] CScript ConsumeScript(FuzzedDataProvider& fuzzed_data_provider, const std::optional<size_t>& max_length = std::nullopt) noexcept; | |||
| [[nodiscard]] CScript ConsumeScript(FuzzedDataProvider& fuzzed_data_provider) noexcept; | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Breaking change: ConsumeScript signature modified.
The removal of the optional max_length parameter is a breaking change that could affect existing tests. Please:
- Verify that all callers of ConsumeScript have been updated
- Document the rationale for removing the parameter
Run this script to find all callers of ConsumeScript:
🏁 Script executed:
#!/bin/bash
# Description: Find all callers of ConsumeScript to verify they've been updated
# Search for ConsumeScript function calls
rg -A 5 'ConsumeScript\('Length of output: 8137
ConsumeScript Callers Updated – Missing Rationale Documentation
- All instances in the repository call
ConsumeScriptwith a single argument (as confirmed in fuzz tests across multiple files), so the API change does not break existing callers. - However, there is no accompanying documentation or inline comment explaining the rationale for removing the optional
max_lengthparameter. This rationale should be documented (e.g., in the commit message, header comments, or changelog) to clarify the breaking change.
| @@ -1675,7 +1688,7 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c | |||
| } else { | |||
| warnings.push_back("Range not given, using default keypool range"); | |||
| range_start = 0; | |||
| range_end = gArgs.GetArg("-keypool", DEFAULT_KEYPOOL_SIZE); | |||
| range_end = gArgs.GetIntArg("-keypool", DEFAULT_KEYPOOL_SIZE); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use wallet's configuration instead of global args.
Direct usage of the global gArgs makes the code harder to test and maintain. Consider getting the keypool size through the wallet's configuration instead.
Replace the global args usage with wallet's configuration:
- range_end = gArgs.GetIntArg("-keypool", DEFAULT_KEYPOOL_SIZE);
+ range_end = wallet.GetDefaultKeyPoolSize();📝 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.
| range_end = gArgs.GetIntArg("-keypool", DEFAULT_KEYPOOL_SIZE); | |
| range_end = wallet.GetDefaultKeyPoolSize(); |
Additional Information
bitcoin#12315 was backported to allow for backporting bitcoin#22805 and does not include Bech32 address support.
In bitcoin#24154,
feature_maxtipage.pywas adjusted to account for Dash's shorter maximum tip age (6 hours, source vs 24 hours upstream, source). Attempting to use tip ages greater than that will result intime-too-newerrors instead.Breaking Changes
None expected.
Checklist