backport: merge bitcoin#23732, #24636, #24625, #24349, #24673, #24732, #24919, #24925, #25246, #26275, #24927, #24804, #19888, #26308, #24672, #23072, #24216 (auxiliary backports: part 24) #6727
Conversation
Update to the "decodepsbt" description changes are not included as we don't return a "witness_utxo" object (no SegWit support)
WalkthroughThis update introduces extensive changes across the codebase, focusing on refactoring, modernization, and improved configurability. The random number generation API is overhauled, replacing Wallet database configuration is made more flexible: options such as shared memory, unsafe sync, and log size are now settable via command-line arguments and propagated through new or updated constructors and factory functions for BerkeleyDB and SQLite backends. The ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/wallet/test/db_tests.cpp (1)
65-66: Inconsistent argument usage - update remaining gArgs references.Lines 65-66 still use
gArgs.GetDataDirNet()while the rest of the file has been updated to usem_args.GetDataDirNet(). For consistency with the broader refactoring effort, these should also be updated.- fs::path datadir = gArgs.GetDataDirNet() / "1"; - fs::path datadir_2 = gArgs.GetDataDirNet() / "2"; + fs::path datadir = m_args.GetDataDirNet() / "1"; + fs::path datadir_2 = m_args.GetDataDirNet() / "2";
🧹 Nitpick comments (4)
src/test/dynamic_activation_thresholds_tests.cpp (1)
33-34: Fix code formatting to match project style.The explicit chain parameter addition is correct and aligns with the test setup standardization, but the formatting doesn't match the expected style.
Apply this formatting fix:
- TestChainDATSetup() : - TestChainSetup(window - 2, CBaseChainParams::REGTEST, {"-vbparams=testdummy:0:999999999999:0:100:80:60:5:0"}) {} + TestChainDATSetup() + : TestChainSetup(window - 2, CBaseChainParams::REGTEST, {"-vbparams=testdummy:0:999999999999:0:100:80:60:5:0"}) {}src/coinjoin/client.cpp (1)
1866-1869: Resolve clang-format errors onGetRand<int>line
CI is failing the diff format check around this line. Adjust indentation or break the expression to satisfy the project’s clang-format rules (e.g., place the comment on its own line or reflow parameters).src/validation.cpp (1)
2241-2241: Clean refactoring of BIP30 checksExtracting the hardcoded BIP30 checks into the
IsBIP30Repeat()helper function improves code readability and maintainability. However, ensure the helper function uses Dash-specific values as noted in the previous comment.src/test/util/setup_common.cpp (1)
594-638: Excellent test utility for realistic mempool scenarios.The
PopulateMempoolmethod creates a diverse transaction graph with:
- Random but bounded inputs/outputs (1-24)
- Proper UTXO tracking and chaining
- Deterministic randomness for reproducible tests
- Optional mempool submission
This will significantly improve mempool-related test coverage.
Consider adding a brief comment explaining the 1000 sat "fee" and 2000 sat threshold values for future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (85)
configure.ac(1 hunks)src/Makefile.test.include(1 hunks)src/addrdb.cpp(1 hunks)src/bench/bls.cpp(1 hunks)src/bench/bls_dkg.cpp(1 hunks)src/bench/mempool_stress.cpp(2 hunks)src/bitcoin-tx.cpp(1 hunks)src/blockencodings.cpp(1 hunks)src/coinjoin/client.cpp(3 hunks)src/coinjoin/server.cpp(4 hunks)src/common/bloom.cpp(1 hunks)src/consensus/tx_verify.cpp(2 hunks)src/core_io.h(1 hunks)src/core_write.cpp(5 hunks)src/evo/core_write.cpp(1 hunks)src/governance/governance.cpp(1 hunks)src/index/coinstatsindex.cpp(1 hunks)src/init.cpp(2 hunks)src/llmq/dkgsession.cpp(1 hunks)src/net_processing.cpp(1 hunks)src/netaddress.h(1 hunks)src/node/miner.cpp(0 hunks)src/node/miner.h(1 hunks)src/random.cpp(1 hunks)src/random.h(1 hunks)src/rest.cpp(3 hunks)src/rpc/blockchain.cpp(12 hunks)src/rpc/mining.cpp(4 hunks)src/rpc/misc.cpp(1 hunks)src/rpc/rawtransaction.cpp(7 hunks)src/rpc/txoutproof.cpp(1 hunks)src/saltedhasher.cpp(1 hunks)src/test/block_reward_reallocation_tests.cpp(1 hunks)src/test/blockfilter_index_tests.cpp(2 hunks)src/test/dynamic_activation_thresholds_tests.cpp(1 hunks)src/test/evo_deterministicmns_tests.cpp(1 hunks)src/test/fuzz/script.cpp(0 hunks)src/test/fuzz/script_format.cpp(1 hunks)src/test/fuzz/transaction.cpp(1 hunks)src/test/miner_tests.cpp(2 hunks)src/test/random_tests.cpp(2 hunks)src/test/txpackage_tests.cpp(1 hunks)src/test/txvalidation_tests.cpp(1 hunks)src/test/txvalidationcache_tests.cpp(1 hunks)src/test/util/setup_common.cpp(6 hunks)src/test/util/setup_common.h(3 hunks)src/txmempool.cpp(3 hunks)src/txmempool.h(3 hunks)src/util/bytevectorhash.cpp(1 hunks)src/util/hasher.cpp(1 hunks)src/validation.cpp(9 hunks)src/validation.h(1 hunks)src/versionbits.cpp(2 hunks)src/wallet/bdb.cpp(6 hunks)src/wallet/bdb.h(4 hunks)src/wallet/db.cpp(2 hunks)src/wallet/db.h(3 hunks)src/wallet/dump.cpp(4 hunks)src/wallet/dump.h(1 hunks)src/wallet/init.cpp(1 hunks)src/wallet/interfaces.cpp(2 hunks)src/wallet/load.cpp(3 hunks)src/wallet/rpc/transactions.cpp(1 hunks)src/wallet/rpc/wallet.cpp(2 hunks)src/wallet/salvage.cpp(1 hunks)src/wallet/salvage.h(1 hunks)src/wallet/sqlite.cpp(3 hunks)src/wallet/sqlite.h(2 hunks)src/wallet/test/coinjoin_tests.cpp(1 hunks)src/wallet/test/db_tests.cpp(3 hunks)src/wallet/test/init_test_fixture.cpp(2 hunks)src/wallet/test/init_tests.cpp(4 hunks)src/wallet/test/wallet_tests.cpp(8 hunks)src/wallet/wallet.cpp(2 hunks)src/wallet/walletdb.cpp(1 hunks)src/wallet/wallettool.cpp(4 hunks)src/zmq/zmqpublishnotifier.cpp(1 hunks)test/functional/README.md(1 hunks)test/functional/data/rpc_getblockstats.json(3 hunks)test/functional/feature_dip3_deterministicmns.py(1 hunks)test/functional/feature_index_prune.py(4 hunks)test/functional/feature_utxo_set_hash.py(1 hunks)test/functional/rpc_deriveaddresses.py(1 hunks)test/functional/rpc_dumptxoutset.py(1 hunks)test/functional/rpc_getblockstats.py(3 hunks)
💤 Files with no reviewable changes (2)
- src/test/fuzz/script.cpp
- src/node/miner.cpp
🧰 Additional context used
🧠 Learnings (1)
src/netaddress.h (1)
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.
🪛 GitHub Actions: Clang Diff Format Check
src/saltedhasher.cpp
[error] 7-11: Clang format differences found. Code formatting does not match the expected style.
src/test/block_reward_reallocation_tests.cpp
[error] 37-38: Clang format differences found. Code formatting does not match the expected style.
src/governance/governance.cpp
[error] 1260-1261: Clang format differences found. Code formatting does not match the expected style.
src/coinjoin/client.cpp
[error] 1865-1866: Clang format differences found. Code formatting does not match the expected style.
src/test/dynamic_activation_thresholds_tests.cpp
[error] 31-34: Clang format differences found. Code formatting does not match the expected style.
src/test/evo_deterministicmns_tests.cpp
[error] 866-868: Clang format differences found. Code formatting does not match the expected style.
🪛 Gitleaks (8.26.0)
test/functional/feature_dip3_deterministicmns.py
32-32: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: linux64_tsan-test / Test source
- GitHub Check: linux64_ubsan-test / Test source
- GitHub Check: linux64_sqlite-test / Test source
- GitHub Check: linux64-test / Test source
🔇 Additional comments (175)
configure.ac (1)
998-998: LGTM! Correct compiler flag detection method.The change from
AX_CHECK_LINK_FLAGtoAX_CHECK_COMPILE_FLAGis appropriate since-fcf-protection=fullis a compiler flag for Control Flow Integrity protection, not a linker flag. This simplifies the detection logic and aligns with how other compiler flags are checked in this section.src/Makefile.test.include (1)
339-339: LGTM! Properly adds new fuzz test source.The addition of
test/fuzz/script_format.cppto the fuzz test sources is correctly placed in alphabetical order and within the appropriate conditional compilation block for fuzz binary builds.test/functional/README.md (1)
27-27: LGTM! Corrects documentation link.The change from
.shto.pyextension correctly reflects that the Python lint script is actually a Python file, not a shell script. This helps developers find the right linting tool.test/functional/feature_utxo_set_hash.py (1)
70-71: LGTM! Updates deterministic hash test expectations.The updated expected hash values for
hash_serialized_2andmuhashare necessary to maintain test validity following the script serialization refactoring mentioned in the broader PR context. The test methodology remains sound - only the expected deterministic results have been updated to reflect the new serialization behavior.src/net_processing.cpp (1)
5642-5645: Standardize nonce generation with templated GetRand
The newdo–whileloop cleanly replaces the oldGetRandBytesapproach and ensures a non-zerouint64_tnonce as before.src/bitcoin-tx.cpp (1)
694-694: LGTM: Improved parameter clarityThe explicit named parameter comment
/*block_hash=*/enhances code readability by clearly indicating the purpose of theuint256()argument. This aligns well with the broader standardization effort across the codebase.test/functional/rpc_dumptxoutset.py (1)
40-40: LGTM: Updated test expectations for deterministic hash changesThe hash value updates correctly reflect the new deterministic outputs after the serialization and hashing improvements mentioned in the AI summary. These test updates are necessary to maintain test accuracy.
Also applies to: 46-46, 49-49
src/test/fuzz/transaction.cpp (1)
97-98: LGTM: Enhanced parameter documentationThe explicit named parameter comments
/*hashBlock=*/and/*entry=*/improve code clarity by documenting the purpose of each argument in theTxToUnivcalls. This is a valuable maintainability improvement consistent with the broader codebase standardization.src/wallet/test/init_tests.cpp (1)
22-22: LGTM: Improved test isolation with member argument accessThe transition from global
gArgsto memberm_argsimproves test isolation by localizing argument management within the test context. This reduces dependencies on global state and enhances test reliability.Also applies to: 35-35, 78-78, 91-91
src/rpc/txoutproof.cpp (1)
93-93: LGTM: Improved lock scope precisionThe scoped
WITH_LOCK(::cs_main, ...)provides better lock hygiene by precisely limiting the critical section to only theLookupBlockIndexcall. This reduces lock contention and improves code clarity compared to the broader lock acquisition that was presumably used before.src/wallet/rpc/transactions.cpp (1)
794-794: LGTM! Improved code clarity with named parameters.The change from positional arguments to explicit named parameters enhances readability and maintainability without altering functionality.
src/llmq/dkgsession.cpp (1)
1002-1002: LGTM! Modern templated random number generation API.The update from
GetRandInt(5)toGetRand<int>(/*nMax=*/5)improves type safety and code clarity through explicit template parameters and named argument comments.src/util/bytevectorhash.cpp (1)
11-14: LGTM! Efficient constructor with modern random API.The change to use member initializer list with
GetRand<uint64_t>()improves constructor efficiency and aligns with the templated random number generation API modernization.src/rpc/misc.cpp (1)
453-453: LGTM! Type consistency improvement.The change from
inttoint64_tfor the loop variable correctly aligns with therange_beginandrange_endvariables (declared asint64_ton lines 429-430), preventing potential overflow issues when deriving addresses across large index ranges.src/wallet/load.cpp (1)
62-62: Verify missing header include forReadDatabaseArgs.The
ReadDatabaseArgsfunction is being called but its declaration header (src/wallet/db.h) doesn't appear to be included. This would cause compilation errors.#!/bin/bash # Description: Check if ReadDatabaseArgs is declared and if the necessary header is included # Test: Search for ReadDatabaseArgs declaration in wallet headers echo "Searching for ReadDatabaseArgs declaration:" rg -A 3 "ReadDatabaseArgs" src/wallet/ # Test: Check if wallet/db.h is included in this file or through transitive includes echo "Checking includes that might provide ReadDatabaseArgs:" rg -n "#include.*db\.h" src/wallet/load.cpp rg -n "#include.*walletdb\.h" src/wallet/load.cpp # Test: Check if walletdb.h includes db.h echo "Checking if walletdb.h includes db.h:" rg -n "#include.*db\.h" src/wallet/walletdb.hAlso applies to: 90-90, 119-119
src/evo/core_write.cpp (1)
23-23: LGTM! Clean API migration.The replacement of
ScriptPubKeyToUnivwithScriptToUnivusing explicit named parameters improves code clarity and aligns with the broader script serialization API unification. The parameters maintain equivalent functionality.src/index/coinstatsindex.cpp (1)
140-140: LGTM! Improved code organization.The replacement of hardcoded BIP30 logic with the
IsBIP30Unspendable(*pindex)helper function enhances maintainability and ensures consistent BIP30 handling across the codebase.src/governance/governance.cpp (1)
1263-1263: LGTM! Random number generation API modernization.The replacement of
GetRandInt(999999)withGetRand<int>(/*nMax=*/999999)correctly modernizes the random number generation API while maintaining identical functionality. This aligns with the broader codebase refactor to improve type safety and clarity.src/wallet/test/coinjoin_tests.cpp (1)
136-136: LGTM! Improves test isolation by avoiding global state.The change from
&gArgsto&m_argscentralizes argument management within the test fixture, improving test isolation and configurability. This aligns with the broader effort to replace global argument object usage with member-scoped instances in wallet tests.src/wallet/walletdb.cpp (1)
1195-1199: LGTM: Clean integration with DatabaseOptions infrastructure.The addition of a local
DatabaseOptionsinstance and its consistent usage across both database constructors properly integrates the mock wallet database creation with the broader database options refactoring.test/functional/rpc_deriveaddresses.py (1)
47-52: Excellent regression test for edge case handling.This test properly validates that the
deriveaddressesRPC can handle the maximum derivation index (2147483647) without crashing, documenting the fix from PR bitcoin#26275. The test follows established patterns and provides important coverage for this edge case.src/wallet/rpc/wallet.cpp (2)
470-470: Proper integration of database arguments in loadwallet RPC.The call to
ReadDatabaseArgs(*context.args, options)correctly integrates command-line database arguments into the wallet loading process, ensuring configuration options are respected.
626-626: Consistent database arguments integration in createwallet RPC.The
ReadDatabaseArgs(*context.args, options)call properly applies database configuration from command-line arguments during wallet creation, maintaining consistency with the loadwallet implementation.src/consensus/tx_verify.cpp (2)
15-15: Required include for Assert() macro.The addition of
#include <util/check.h>is necessary to support the Assert() usage in the CalculateSequenceLocks function.
80-80: Good defensive programming with Assert() for null pointer protection.Wrapping the
block.GetAncestor()call withAssert()provides valuable protection against potential null pointer dereferences while documenting the expectation that the ancestor block should always exist. The change toconst int64_twith direct initialization is also an improvement.test/functional/data/rpc_getblockstats.json (3)
141-143: LGTM! Test data correctly updated for new RPC fields.The addition of
utxo_increase_actualandutxo_size_inc_actualfields aligns with the RPC enhancement to provide more accurate UTXO metrics by excluding unspendable outputs. The values correctly match the existing fields in this test data, which is expected when there are no unspendable outputs.
177-179: Consistent implementation across all test blocks.The new fields are systematically added to all block statistics entries, maintaining consistency with the RPC API changes.
212-214: Test data properly reflects the RPC enhancement.The consistent addition of the "actual" UTXO metrics fields throughout the test data ensures comprehensive testing of the new RPC functionality.
src/test/random_tests.cpp (2)
29-30: LGTM! Correct migration to templated random number generation.The replacement of
GetRand()andGetRandInt()with the templatedGetRand<uint64_t>()andGetRand<int>()functions aligns with the broader random number generation API refactoring. The type-specific templates provide better type safety.
70-71: Consistent update across both test scenarios.The non-deterministic test section correctly uses the same templated random functions, ensuring consistency in the API usage throughout the test suite.
src/wallet/interfaces.cpp (2)
616-616: LGTM! Proper integration of database arguments in wallet creation.The addition of
ReadDatabaseArgs(*m_context.args, options)correctly integrates command-line or RPC context arguments into the database options for wallet creation. This placement ensures configuration is applied before theCreateWalletcall.
626-626: Consistent database configuration for wallet loading.The same database argument reading pattern is correctly applied to wallet loading, ensuring consistent configuration handling across both wallet creation and loading workflows.
src/wallet/test/init_test_fixture.cpp (3)
18-18: LGTM! Improved test isolation with member argument management.Replacing the global
gArgswith the memberm_argsimproves test isolation and follows better testing practices by avoiding global state dependencies.
22-22: Consistent use of member arguments throughout the fixture.The systematic replacement of global argument references with member variables ensures consistent argument management within the test fixture.
49-49: Proper argument management in test configuration methods.Using
m_args.ForceSetArginstead of the global equivalent maintains the improved argument management pattern throughout all fixture methods.src/bench/bls_dkg.cpp (1)
98-98: LGTM! Correct migration to templated random generation with appropriate type.The replacement of
GetRandIntwithGetRand<size_t>aligns with the random number generation API refactoring. Usingsize_tfor theshareIdxvariable is appropriate for array indexing and improves type safety.src/coinjoin/server.cpp (1)
387-387: LGTM! Consistent modernization of random number generation.The changes consistently replace
GetRandIntcalls with the new templatedGetRand<int>()function. The logic and probability calculations remain unchanged, and the explicit parameter naming with/*nMax=*/improves code readability.Also applies to: 423-423, 455-455, 722-722
src/addrdb.cpp (1)
52-52: LGTM! Cleaner random number generation.The change simplifies the code by directly initializing the random value using the templated
GetRand<uint16_t>()function instead of usingGetRandBytesto fill a mutable variable. This is more concise and type-safe.src/zmq/zmqpublishnotifier.cpp (1)
313-317: Verify thread safety of block reading without lock.The removal of
LOCK(cs_main)aroundReadBlockFromDiskoperation creates an inconsistency with other methods in this file (CZMQPublishRawChainLockNotifier::NotifyChainLockandCZMQPublishRawChainLockSigNotifier::NotifyChainLock) that still acquire the lock. Ensure that block reading is thread-safe without this protection.#!/bin/bash # Description: Check if other block reading operations in ZMQ notifiers use locks # Expected: Find other ReadBlockFromDisk calls and their locking patterns # Search for ReadBlockFromDisk usage in ZMQ files rg -A 3 -B 3 "ReadBlockFromDisk" src/zmq/ # Search for LOCK(cs_main) usage in ZMQ files rg -A 5 -B 2 "LOCK.*cs_main" src/zmq/src/versionbits.cpp (1)
6-6: LGTM! Enhanced safety with runtime assertion.The addition of
Assert()around theGetAncestorcall provides a runtime safety check to ensure the ancestor block is non-null. The inclusion of<util/check.h>properly supports this assertion. This aligns with similar safety improvements across consensus-related code.Also applies to: 194-194
src/wallet/init.cpp (1)
109-109: LGTM! Dynamic defaults improve consistency.The changes make the default values for
-dblogsizeand-privdbdynamically derived fromDatabaseOptions()instead of hardcoded constants. This ensures consistency between the help text defaults and the actual configuration used by the wallet database system.Also applies to: 111-111
src/node/miner.h (1)
67-67: LGTM! Type consistency improvement for fee handling.The change from
int64_ttoCAmountfor the return type aligns with the standard fee amount type used throughout the codebase and improves type consistency.src/saltedhasher.cpp (1)
10-10: LGTM! Modernized random number generation with formatting fix needed.The change to use the templated
GetRand<uint64_t>()function improves type safety and aligns with the broader codebase refactoring. However, please address the formatting issue flagged by the pipeline.Please run the clang-format tool to fix the formatting:
#!/bin/bash # Fix formatting issues in saltedhasher.cpp clang-format -i src/saltedhasher.cppsrc/common/bloom.cpp (1)
338-338: LGTM! Consistent with templated random number generation refactoring.The change to use
GetRand<unsigned int>()improves type safety and aligns with the broader codebase modernization of random number generation functions.src/blockencodings.cpp (1)
22-22: LGTM! Improved type safety in random nonce generation.The change to use
GetRand<uint64_t>()provides better type safety and aligns with the templated random number generation refactoring across the codebase.src/random.cpp (1)
662-665: LGTM! Clean API refactoring for type-safe random number generation.The renaming of
GetRandtoGetRandInternalappropriately makes this an internal implementation detail, supporting the new templatedGetRand<T>()public interface. This improves type safety and API clarity.src/wallet/salvage.cpp (2)
25-25: LGTM: Function signature updated for explicit argument management.The addition of the
ArgsManager& argsparameter follows the established pattern of making database configuration explicit rather than relying on global state.
29-29: LGTM: Database options properly populated from arguments.The call to
ReadDatabaseArgs(args, options)ensures that database-related configuration options are correctly populated from the provided ArgsManager before proceeding with recovery operations.src/wallet/test/wallet_tests.cpp (3)
236-236: LGTM: Consistent migration from global to member argument management.The replacement of
gArgswithm_argsin wallet context initialization improves test isolation and follows the established pattern of explicit argument passing rather than relying on global state.Also applies to: 297-297, 325-325
292-292: LGTM: Proper argument usage in file path operations.Using
m_args.GetDataDirNet()instead ofgArgs.GetDataDirNet()ensures the test uses the fixture's controlled argument context for data directory operations.
730-730: LGTM: Systematic migration to fixture-scoped argument management.All instances of
gArgshave been consistently replaced withm_args, ensuring tests operate with controlled and isolated argument contexts throughout the wallet test suite.Also applies to: 733-733, 825-825, 834-834, 836-836, 885-885, 987-987
src/bench/bls.cpp (1)
218-222: LGTM: Improved type safety with templated random number generation.The replacement of
GetRandIntwithGetRand<size_t>provides better type consistency by directly generating asize_tindex instead of casting fromint. This aligns with the broader modernization of the random number generation API.src/test/blockfilter_index_tests.cpp (2)
7-7: LGTM: Required include for Merkle root calculation.The addition of
#include <consensus/merkle.h>is necessary for theBlockMerkleRootfunction used in the manual block creation logic.
77-82: LGTM: Correct manual implementation replacing IncrementExtraNonce.The explicit coinbase scriptSig update and Merkle root recalculation correctly replaces the functionality previously provided by
IncrementExtraNonce. The implementation:
- Creates a mutable copy of the coinbase transaction
- Updates the scriptSig with the block height
- Replaces the coinbase transaction in the block
- Recalculates the Merkle root
This aligns with the broader simplification of mining logic by removing the extra nonce manipulation.
src/wallet/test/db_tests.cpp (2)
21-21: LGTM: Updated GetBerkeleyEnv call with shared memory parameter.The addition of
falseas the second parameter toGetBerkeleyEnvcorrectly reflects the new function signature that requires ause_shared_memoryboolean parameter.
27-27: LGTM: Consistent migration to fixture-scoped argument management.The replacement of
gArgs.GetDataDirNet()withm_args.GetDataDirNet()follows the established pattern of using test fixture member variables instead of global state for better test isolation.Also applies to: 41-41, 51-52
src/test/txpackage_tests.cpp (1)
19-19: LGTM: Constructor standardization improves clarity.The explicit specification of
CBaseChainParams::REGTESTaligns with the broader standardization effort mentioned in the summary, making the test chain context explicit rather than relying on defaults.src/validation.h (1)
1063-1067: LGTM: Well-designed BIP30 helper function declarations.The new function declarations
IsBIP30RepeatandIsBIP30Unspendableprovide clear, descriptive names and appropriate const reference parameters. The comments effectively explain their purpose in BIP30 enforcement, and this centralization will improve code clarity and reuse.src/test/miner_tests.cpp (3)
468-470: LGTM: Named constant improves readability.The introduction of
SEQUENCE_LOCK_TIMEconstant makes the code more self-documenting and easier to maintain compared to the hardcoded512value.
476-479: LGTM: Safety improvements with Assert() calls.The addition of
Assert()calls before accessing ancestor blocks viaGetAncestor()provides runtime safety by ensuring the pointers are valid before dereferencing. This aligns with the broader safety improvements mentioned in the AI summary.
525-529: LGTM: Consistent safety pattern with explicit braces.The explicit braces and
Assert()calls follow the same safety pattern established earlier in the function, ensuring consistent error handling for ancestor block access.src/wallet/db.cpp (2)
9-9: LGTM: Required include for ArgsManager support.The addition of
util/system.his necessary to support the newReadDatabaseArgsfunction implementation.
141-147: LGTM: Well-designed argument reading function.The
ReadDatabaseArgsfunction properly centralizes database option configuration:
- Uses existing option values as defaults
- Correctly maps command-line arguments to options
- The negation logic for
use_shared_memorywith-privdbis intuitive (private DB = !shared memory)This centralization will improve consistency across wallet database handling.
src/wallet/salvage.h (2)
12-12: LGTM: Appropriate forward declaration.The forward declaration of
ArgsManageris the correct approach to avoid unnecessary header dependencies while enabling the function signature change.
15-15: LGTM: Function signature enhancement for database configuration.Adding the
ArgsManagerparameter toRecoverDatabaseFilealigns with the broader refactoring to explicitly pass argument context rather than relying on global state, improving testability and consistency.test/functional/feature_dip3_deterministicmns.py (1)
30-34: LGTM! Clean simplification of test arguments.The direct assignment of
extra_argsimproves readability and maintains the necessary test configuration parameters for DIP3 deterministic masternode testing.Regarding the static analysis hint about the spork key: This is a false positive. The hardcoded private key is intentionally used in test environments to ensure deterministic behavior and is not a security concern in test contexts.
src/test/txvalidation_tests.cpp (1)
20-20: LGTM! Improved clarity with explicit chain parameter.Making the chain parameter explicit (
CBaseChainParams::REGTEST) enhances code readability and eliminates ambiguity about the test chain context. This aligns with the broader modernization effort across the test suite.src/wallet/wallet.cpp (2)
377-377: LGTM! Proper integration of database arguments in wallet restoration.This addition ensures that command-line or configuration database arguments are properly applied during wallet restoration, maintaining consistency with other wallet operations mentioned in the AI summary.
3303-3303: LGTM! Consistent with Berkeley environment shared memory support.The addition of
use_shared_memory=trueparameter aligns with the described changes to Berkeley environment handling. This ensures backup operations use shared memory when interacting with the database environment.src/test/fuzz/script_format.cpp (2)
13-16: LGTM: Standard fuzz test initialization.The initialization function correctly sets up the REGTEST environment for fuzz testing, which is consistent with other fuzz tests in the codebase.
18-28: LGTM: Well-structured fuzz target.The fuzz target properly exercises the script formatting functions with randomized inputs. The use of
FuzzedDataProviderfor generating boolean flags and theConsumeScripthelper follows established patterns in the fuzz testing framework.test/functional/feature_index_prune.py (5)
81-81: Verify the updated prune height magic number.The expected prune height has been updated to 368. While the comment acknowledges these are magic numbers determined by block file wrapping thresholds, please ensure this value is correct for the current pruning logic.
98-99: LGTM: Coordinated test parameter adjustments.The changes to block generation (51 blocks) and sync height (751) appear to be coordinated adjustments that optimize the test setup while maintaining the test's integrity.
111-111: LGTM: Consistent test parameter update.The adjustment to mine 749 blocks is consistent with the other parameter changes in this test.
116-116: Verify the updated prune height magic number.The expected prune height has been updated to 736. Please verify this value aligns with the current pruning logic and block file wrapping behavior.
151-151: Verify the updated prune height magic number.The expected prune height has been updated to 2208. Please ensure this value is correct for the current pruning implementation.
src/test/txvalidationcache_tests.cpp (1)
17-17: LGTM: Standardized test chain initialization.The explicit specification of
CBaseChainParams::REGTESTin theDersig100Setupconstructor aligns with the broader standardization effort across the test suite and improves code clarity.src/netaddress.h (1)
582-583: LGTM! Clean modernization of random number generation API.The change from
GetRand(std::numeric_limits<uint64_t>::max())toGetRand<uint64_t>()improves type safety and code clarity while maintaining identical functionality. This aligns well with the broader templated random number generation API refactoring.src/txmempool.h (3)
117-117: Excellent type safety improvement.Changing
m_modified_feefromint64_ttoCAmountprovides better semantic meaning for fee-related data and improves consistency across the fee handling codebase.
146-146: Consistent with member variable type change.The return type change from
int64_ttoCAmountcorrectly matches the updatedm_modified_feemember type and maintains API consistency.
155-155: Well-coordinated parameter type update.Updating the
UpdateModifiedFee()parameter type fromint64_ttoCAmountcompletes the consistent adoption ofCAmountfor all fee-related operations in this class.src/init.cpp (2)
1622-1624: LGTM! Modern templated random number generation.The migration from the older
GetRandAPI to the templatedGetRand<uint64_t>()function improves type safety and code clarity. The CConnman constructor correctly receives two random uint64_t values as expected.
2165-2165: LGTM! Improved locking idiom with WITH_LOCK macro.The refactoring from manual
LOCKusage to theWITH_LOCKmacro makes the code more concise and idiomatic. The syntax is correct -WITH_LOCK(cs_main, return CheckLegacyTxindex(...))properly acquires the lock before executing the return statement.test/functional/rpc_getblockstats.py (4)
47-49: LGTM! Proper wallet setup for deterministic testing.The wallet creation and private key import provides the necessary setup for sending transactions in the test. Using
get_deterministic_priv_key()ensures consistent test behavior across runs.
59-60: LGTM! OP_RETURN transaction correctly tests unspendable output exclusion.The
send()call with{"data": "21"}creates an OP_RETURN output that should be excluded from the "actual" UTXO statistics, which is exactly what this test aims to validate.
169-175: LGTM! Genesis block test validates BIP30 special case handling.The test correctly validates that the genesis block coinbase:
- Creates 1 UTXO (
utxo_increase: 1)- But counts as 0 actual spendable UTXOs (
utxo_increase_actual: 0) due to BIP30 unspendable conditions- Uses the correct genesis block hash for regtest network
The size metrics follow the same pattern (117 vs 0 bytes).
177-182: LGTM! Tip block test confirms OP_RETURN exclusion works correctly.The test validates that despite having an OP_RETURN output in the block, both
utxo_increaseandutxo_increase_actualare equal (4), and size metrics are equal (300), confirming that OP_RETURN outputs are properly excluded from both total and actual counts as expected.src/wallet/dump.h (2)
16-16: LGTM! Proper forward declaration added.The forward declaration for
ArgsManageris correctly placed and necessary for the updated function signatures.
18-19: LGTM! Function signatures properly updated to accept ArgsManager.The addition of
const ArgsManager& argsas the first parameter in bothDumpWalletandCreateFromDumpfunctions aligns with the broader refactoring to eliminate globalgArgsusage and improve modularity.src/bench/mempool_stress.cpp (4)
89-89: LGTM! Correctly uses shared mempool from testing setup.Using the mempool reference from
testing_setup.get()->m_node.mempoolensures the benchmark operates on the same mempool instance managed by the test node, improving consistency.
103-104: LGTM! Proper setup upgrade for realistic benchmarking.The switch from
TestingSetupon mainnet toTestChain100Setupon regtest with-checkmempool=1provides:
- Pre-mined blocks for proper coinbase maturity
- Regtest network for consistent behavior
- Mempool checking enabled for validation
106-106: LGTM! PopulateMempool provides consistent test data.Using
PopulateMempool(det_rand, 400, true)replaces manual transaction creation with a standardized method that generates 400 randomized transactions, improving benchmark consistency and reducing code duplication.
110-111: LGTM! Proper spendheight parameter prevents coinbase maturity errors.Setting
spendheight=300in the mempool check ensures that coinbase outputs are considered mature, preventing premature spend errors during the benchmark. This is essential when using TestChain100Setup with pre-mined blocks.src/random.h (2)
75-75: LGTM! Proper function rename maintains backward compatibility.Renaming
GetRand(uint64_t nMax)toGetRandInternal(uint64_t nMax)makes the internal implementation clear while the new templated interface provides the public API.
76-85: LGTM! Excellent templated interface with proper type safety.The new templated
GetRand<T>()function provides several improvements:
- Type safety:
static_assertensures only integral types are accepted- Range safety: Limits to
uint64_tmaximum to prevent overflow- Convenience: Default parameter uses
std::numeric_limits<T>::max()- Consistency: Single interface replaces multiple functions
The implementation correctly delegates to
GetRandInternaland casts the result to the requested type.src/coinjoin/client.cpp (1)
1192-1197: EnsureGetRand<size_t>replicates original randomness logic
The switch fromGetRandInt(2)toGetRand<size_t>(/*nMax=*/2)appears correct, but please verify that the templatedGetRandstill returns a uniform value in[0, nMax-1]so the earlycontinuebehavior is unchanged.src/rest.cpp (3)
317-323: Limitcs_mainlock scope before disk read
The added braces correctly releasecs_mainbefore callingReadBlockFromDisk, avoiding holding the main lock during potentially slow disk I/O.
697-700: Use named parameters forTxToUnivcall
Switching to/*block_hash=*/hashBlockand/*entry=*/objTxenhances readability and aligns with the updated function signature.
881-884: Update to unified script serialization API
ReplacingScriptPubKeyToUnivwithScriptToUniv(…, /*include_hex=*/true, /*include_address=*/true)is consistent with the refactoring and ensures full script details in JSON.src/rpc/rawtransaction.cpp (6)
99-99: LGTM: Improved function call clarity with named parameters.The explicit naming of parameters makes this
TxToUnivcall much more readable and maintainable.
793-793: LGTM: Consistent parameter naming enhances readability.The explicit
/*entry=*/and/*include_hex=*/parameter names improve code clarity.
833-833: LGTM: Enhanced ScriptToUniv call with explicit parameters.The named parameters
/*out=*/,/*include_hex=*/, and/*include_address=*/clearly indicate the function's behavior, which is especially valuable given the refactoring that mergedScriptPubKeyToUnivfunctionality intoScriptToUniv.
1398-1398: LGTM: Consistent named parameter usage.Maintains consistency with other
TxToUnivcalls in the file.
1454-1454: LGTM: Clear parameter naming for PSBT transaction handling.The explicit parameter names improve readability in the PSBT context.
1487-1487: LGTM: Simplified ScriptToUniv calls with explicit output parameter.Both calls correctly use the
/*out=*/named parameter. These calls appear to use default values for the newinclude_hexandinclude_addressparameters introduced in the ScriptToUniv refactoring.Also applies to: 1585-1585
src/wallet/db.h (3)
19-19: LGTM: Appropriate forward declaration.Using a forward declaration of
ArgsManagerminimizes header dependencies and is the correct approach for the newReadDatabaseArgsfunction parameter.
213-217: LGTM: Well-designed database configuration options.The new options are thoughtfully designed with:
- Clear, descriptive names and inline documentation
- Sensible default values that maintain existing behavior
- Appropriate data types for each option
- Good separation of concerns (performance vs. safety vs. functionality)
The defaults maintain backward compatibility while allowing for optimization when needed.
237-237: LGTM: Clean function interface for argument parsing.The
ReadDatabaseArgsfunction signature follows a good pattern by takingconst ArgsManager&and modifyingDatabaseOptions&by reference, allowing callers to start with default options and overlay argument-based configuration.src/util/hasher.cpp (3)
15-16: LGTM: Clean migration to templated GetRand.The change from
GetRand(std::numeric_limits<uint64_t>::max())toGetRand<uint64_t>()simplifies the code while maintaining the same behavior - generating a random 64-bit unsigned integer.
19-19: LGTM: Consistent use of templated GetRand API.The updated
SaltedSipHasherconstructor properly uses the new templatedGetRand<uint64_t>()API, maintaining the random initialization of both keys.
12-12: Verify the intentional change to default initialization.The
SaltedTxidHasherconstructor changed from using random values to default initialization (k0(), k1()). This means the keys will be zero-initialized rather than randomized.Please confirm this behavioral change is intentional, as it could affect the security properties of the hasher if it was previously relying on random salt values.
#!/bin/bash # Description: Search for usage patterns of SaltedTxidHasher to understand if random initialization was required # Find all usages of SaltedTxidHasher rg -A 5 -B 5 "SaltedTxidHasher"src/wallet/sqlite.cpp (3)
106-107: LGTM: Clean dependency injection patternThe constructor changes properly accept
DatabaseOptionsand initialize the new member variable. This eliminates global state dependencies and improves testability.
285-289: LGTM: Consistent use of member variableThe conditional now uses
m_use_unsafe_syncmember instead of querying global arguments, which is consistent with the constructor changes and improves encapsulation.
574-574: LGTM: Factory function signature consistencyThe factory function correctly passes the
optionsparameter to the constructor, maintaining consistency with the updated constructor signature.src/wallet/dump.cpp (5)
21-21: LGTM: Explicit dependency injectionFunction signature updated to accept
ArgsManagerparameter, improving testability and removing global dependencies.
24-24: LGTM: Consistent argument accessUsing the passed
ArgsManagerparameter instead of globalgArgsis consistent with the function signature change.
116-116: LGTM: Function signature consistencyFunction signature updated to match the pattern established in
DumpWallet.
119-119: LGTM: Consistent argument usageAll argument access properly uses the passed
ArgsManagerparameter instead of global state.Also applies to: 173-173
195-195: LGTM: Proper database options initializationThe call to
ReadDatabaseArgs(args, options)correctly populates database options from the passedArgsManager, maintaining consistency with the overall refactoring pattern.src/wallet/sqlite.h (3)
70-70: LGTM: Constructor signature consistencyThe constructor signature correctly accepts
const DatabaseOptions& optionsparameter, matching the implementation in the .cpp file.
114-114: LGTM: Appropriate member variable additionThe
bool m_use_unsafe_syncmember variable is properly declared and will store the configuration option locally, eliminating the need to query global state.
117-117: LGTM: Factory function signature consistencyThe factory function signature correctly includes the
DatabaseOptionsparameter, maintaining consistency with the constructor changes.src/core_io.h (2)
57-57: LGTM: Enhanced function signature with backward compatibilityThe
ScriptToUnivfunction signature adds useful boolean parameters with sensible defaults (include_hex = true, include_address = false), providing flexibility while maintaining backward compatibility.
58-58: LGTM: Improved parameter namingRenaming the parameter from
hashBlocktoblock_hashimproves clarity and follows naming conventions without changing functionality.src/test/util/setup_common.h (4)
135-135: LGTM: More flexible test setup base classChanging the base class from
RegTestingSetuptoTestingSetupprovides more flexibility while the constructor's defaultchain_nameparameter maintains the regtest behavior.
137-139: LGTM: Backward-compatible constructor enhancementThe constructor signature adds a
chain_nameparameter with a sensible default (CBaseChainParams::REGTEST), maintaining backward compatibility while enabling chain configuration flexibility.
187-198: LGTM: Useful testing utility methodThe
PopulateMempoolmethod provides a valuable testing utility for generating randomized transactions with various characteristics. The documentation clearly explains its purpose for mempool consistency testing and the optional submission behavior.
208-209: LGTM: Consistent constructor signatureThe
TestChain100Setupconstructor signature is updated consistently withTestChainSetup, maintaining the same pattern and default values.src/rpc/mining.cpp (4)
11-11: LGTM: Added required include for Merkle root calculation.The addition of
consensus/merkle.his necessary for theBlockMerkleRootfunction used in the refactored mining logic.
125-128: LGTM: Clean refactoring of block generation logic.The removal of the
extra_nonceparameter and explicit calculation of the Merkle root usingBlockMerkleRoot(block)simplifies the mining logic while maintaining correctness. This centralizes nonce handling and eliminates the need for theIncrementExtraNoncefunction.
158-172: LGTM: Simplified block generation loop.The changes improve the loop logic:
- Using a decrementing counter (
nGenerate > 0and--nGenerate) is cleaner than tracking block heights- The updated
GenerateBlockcall aligns with the refactored function signature- Control flow remains identical while being more readable
391-391: LGTM: Consistent function call update.The
GenerateBlockcall is correctly updated to match the new signature, maintaining consistency across all usage sites.src/wallet/wallettool.cpp (6)
146-146: LGTM: Proper database argument handling.Adding
ReadDatabaseArgs(args, options)ensures database options are correctly populated from command-line arguments before wallet creation.
159-165: LGTM: Consistent database options setup for info command.The addition of
DatabaseOptionsinitialization,ReadDatabaseArgscall, and settingrequire_existing = truefollows the same pattern as other commands. This ensures consistent argument handling across all wallet operations.
170-170: LGTM: Updated function call signature.The
RecoverDatabaseFilecall is correctly updated to pass theArgsManagerparameter, aligning with the broader refactoring to improve argument handling.
214-214: LGTM: Database argument handling for dump command.Adding
ReadDatabaseArgs(args, options)maintains consistency with other commands and ensures proper database option configuration.
219-219: LGTM: Updated DumpWallet function call.The function call is correctly updated to pass the
ArgsManagerparameter, maintaining consistency with the refactored function signature.
229-229: LGTM: Updated CreateFromDump function call.The function call is correctly updated to pass the
ArgsManagerparameter, completing the consistent pattern of argument handling improvements across the file.src/txmempool.cpp (3)
70-75: LGTM! Type consistency improvement.The method signature change from
int64_ttoCAmountimproves type consistency across the fee handling codebase. The logic remains unchanged and correctly usesSaturatingAddfor safe arithmetic operations.
478-478: LGTM! Modernization with lambda functions.The replacement of the
update_fee_deltastruct with an inline lambda function modernizes the code while maintaining the same functionality. The lambda correctly capturesdeltaby reference and callsUpdateModifiedFee.
1442-1442: LGTM! Consistent refactoring pattern.The lambda function usage is consistent with the change at line 478, maintaining the same modernization approach throughout the file.
src/wallet/bdb.h (6)
32-34: LGTM! Standard includes for new functionality.The addition of standard library includes supports the new configuration options and is appropriately placed.
54-54: LGTM! Well-named configuration member.The
m_use_shared_memorymember follows the established naming convention and clearly indicates its purpose for controlling Berkeley DB shared memory usage.
56-56: LGTM! Constructor signature enhancement.The updated constructor signature appropriately accepts the
use_shared_memoryparameter to initialize the new member. The parameter type and naming are consistent.
84-84: LGTM! Factory function signature consistency.The
GetBerkeleyEnvfunction signature correctly includes theuse_shared_memoryparameter, maintaining consistency with the constructor changes.
97-99: LGTM! Improved constructor with options pattern.The constructor refactoring to accept
DatabaseOptionsfollows a good design pattern for extensible configuration. The member initializer list correctly initializesm_max_log_mbfrom the options.
159-159: LGTM! Appropriate member for log management.The
m_max_log_mbmember provides necessary configuration for Berkeley DB log size management with clear naming and appropriate type.src/core_write.cpp (6)
32-35: LGTM! Standard includes for updated functionality.The addition of standard library includes supports the enhanced script serialization functionality and is properly placed.
160-179: LGTM! Well-designed function unification.The enhanced
ScriptToUnivfunction successfully merges the functionality of the removedScriptPubKeyToUnivwith configurable parameters. The conditional logic forinclude_hexandinclude_addressis correct and maintains backward compatibility while providing flexibility.
181-181: LGTM! Parameter naming improvement.The rename from
hashBlocktoblock_hashimproves consistency with naming conventions used elsewhere in the codebase.
237-237: LGTM! Explicit parameter names for clarity.The use of explicit parameter names (
/*out=*/,/*include_hex=*/,/*include_address=*/) significantly improves code readability and makes the function call intent clear.
263-263: LGTM! Consistent explicit parameter usage.The function call maintains consistency with the previous update by using explicit parameter names for clarity.
339-341: LGTM! Consistent parameter name usage.The code correctly uses the renamed
block_hashparameter, maintaining consistency with the function signature change.src/rpc/blockchain.cpp (9)
173-175: Good improvement in thread safety patternMoving the pruning check inside a scoped lock is a better pattern than relying on callers to hold locks. This ensures thread-safe access to the block manager state.
181-181: LGTM - Named parameters improve readabilityUsing named parameters for
TxToUnivmakes the function call more self-documenting.
786-805: Well-structured handling of genesis block in GetUndoCheckedThe explicit handling of the genesis block (which has no undo data) by returning an empty undo object is a good defensive programming practice. The scoped locking pattern is consistent with GetBlockChecked.
1003-1004: Good use of const for immutabilityDeclaring the block as
constafter retrieval ensures it cannot be accidentally modified.
1886-1889: Safer code with references and CHECK_NONFATALConverting from pointers to references with
CHECK_NONFATALassertions is a good defensive programming practice that prevents null pointer dereferences.
2081-2091: Correct implementation of unspendable output filteringThe logic correctly excludes:
- Genesis block coinbase (line 2086)
- BIP30 repeat block coinbases (line 2086)
- Unspendable outputs like OP_RETURN (line 2088)
This provides accurate UTXO set statistics.
2121-2123: Size calculation correctly updated for actual UTXO trackingThe prevout size is now correctly subtracted from both the total and actual UTXO size counters.
766-784: ```shell
#!/bin/bashSearch for any callers of GetBlockChecked in the repository
rg -n "GetBlockChecked"
--- `1321-1321`: **API change for ScriptToUniv requires verification** The function signature change from `ScriptPubKeyToUniv` to `ScriptToUniv` with additional parameters needs to be verified across the codebase. ```shell #!/bin/bash # Verify that all uses of the old ScriptPubKeyToUniv have been updated rg "ScriptPubKeyToUniv" --type cpp # Check the new ScriptToUniv signature and usage ast-grep --pattern 'ScriptToUniv($$$)'src/validation.cpp (4)
226-231: Good defensive programming practiceAdding assertions and explanatory comments for
GetAncestoris a good safety measure. The logic correctly assumes thatmaxInputHeightwill always be less than the tip height, making a nullptr return impossible.
2119-2119: Useful performance instrumentationAdding timing metrics for undo data writing follows the established pattern and will help identify performance bottlenecks in block processing.
Also applies to: 2530-2537
2740-2741: Appropriate use of Assert() for safetyUsing
Assert()onGetAncestor()calls is good defensive programming. Since we're iterating within valid height ranges, these should never return nullptr. If they do, it indicates a serious inconsistency that warrants an immediate crash rather than undefined behavior.Also applies to: 4740-4744
2907-2918: Verify BIP30 block heights and hashes for DashThese helper functions contain Bitcoin-specific block heights and hashes. Since this is a Dash codebase, these values need to be verified and potentially replaced with Dash-specific equivalents.
The blocks referenced are:
- Height 91842:
0x00000000000a4d0a398161ffc163c503763b1f4360639393e0e4c8e300e0caec- Height 91880:
0x00000000000743f190a18c5577a3c2d2a1f610ae9601ac046a38084ccb7cd721- Height 91722:
0x00000000000271a2dc26e7667f8419f2e15416dc6955e5a6c6cdf3f2574dd08e- Height 91812:
0x00000000000af0aed4792b1acee3d966af36cf5def14935db8de83d6f9306f2f#!/bin/bash # Check if these Bitcoin-specific values are referenced elsewhere in the Dash codebase echo "Searching for Bitcoin BIP30 block hashes in the codebase..." rg -F "0x00000000000a4d0a398161ffc163c503763b1f4360639393e0e4c8e300e0caec" rg -F "0x00000000000743f190a18c5577a3c2d2a1f610ae9601ac046a38084ccb7cd721" rg -F "0x00000000000271a2dc26e7667f8419f2e15416dc6955e5a6c6cdf3f2574dd08e" rg -F "0x00000000000af0aed4792b1acee3d966af36cf5def14935db8de83d6f9306f2f" echo -e "\nSearching for BIP30 heights..." rg "91842|91880|91722|91812" --type cppsrc/wallet/bdb.cpp (5)
74-74: LGTM! Function signature properly updated.The addition of the
use_shared_memoryparameter aligns with the broader refactoring to use structured options instead of command-line arguments.
127-127: Constructor changes are well-implemented.Both the regular and mock constructors properly handle the new
use_shared_memoryparameter. The mock environment correctly defaults tofalsefor proper isolation.Also applies to: 203-203
159-161: Correct implementation of DB_PRIVATE flag logic.The inverted logic is appropriate:
DB_PRIVATEshould be set when shared memory is NOT used. This properly replaces the command-line argument dependency.
388-388: Good improvement to use database-specific log size.Using
m_database.m_max_log_mbinstead of a global command-line argument allows for more flexible per-database configuration.
847-847: Factory function properly updated for DatabaseOptions.The changes correctly propagate
use_shared_memorytoGetBerkeleyEnvand pass the completeoptionsobject to theBerkeleyDatabaseconstructor, completing the refactoring to structured options.Also applies to: 853-853
src/test/util/setup_common.cpp (4)
11-11: Required includes properly added.The new includes support the refactored block creation logic (
consensus/merkle.h) and the newPopulateMempoolfunctionality (algorithm,stdexcept).Also applies to: 71-71, 73-73
220-220: Good improvement for test flexibility.Using
GetIntArg("-checkmempool", 1)instead of a hardcoded value allows tests to customize mempool validation behavior while maintaining the same default.
369-370: Constructor updates enhance test flexibility.The explicit
chain_nameparameter allows tests to specify different chain configurations, improving test coverage capabilities.Also applies to: 374-375
507-515: Correct implementation of manual block finalization.The code properly replaces
IncrementExtraNonceby:
- Setting the previous block hash from chain tip
- Constructing a valid coinbase scriptSig with block height and a fixed nonce
- Explicitly recalculating the merkle root after all transaction modifications
The
LOCK(::cs_main)ensures thread-safe access to chain state.
| // 5 blocks earlier | ||
| TestChainV19BeforeActivationSetup::TestChainV19BeforeActivationSetup() : | ||
| TestChainSetup(494, {"-testactivationheight=v19@500", "-testactivationheight=v20@500", "-testactivationheight=mn_rr@500"}) | ||
| TestChainSetup(494, CBaseChainParams::REGTEST, {"-testactivationheight=v19@500", "-testactivationheight=v20@500", "-testactivationheight=mn_rr@500"}) |
There was a problem hiding this comment.
LGTM: Explicit chain parameter specification.
The change to explicitly pass CBaseChainParams::REGTEST to the base constructor standardizes test chain initialization and improves clarity across the test suite.
Fix formatting issues.
The pipeline indicates clang format differences on lines 866-868. Please run clang-format to align with the project's coding style.
🤖 Prompt for AI Agents
In src/test/evo_deterministicmns_tests.cpp around lines 866 to 868, there are
formatting issues detected by the pipeline's clang-format check. Run
clang-format on these lines to fix indentation, spacing, and other style
inconsistencies to align the code with the project's coding standards.
| {RPCResult::Type::NUM, "utxo_increase", /* optional */ true, "The increase/decrease in the number of unspent outputs (not discounting op_return and similar)"}, | ||
| {RPCResult::Type::NUM, "utxo_size_inc", /* optional */ true, "The increase/decrease in size for the utxo index (not discounting op_return and similar)"}, | ||
| {RPCResult::Type::NUM, "utxo_increase_actual", /*optional=*/true, "The increase/decrease in the number of unspent outputs, not counting unspendables"}, | ||
| {RPCResult::Type::NUM, "utxo_size_inc_actual", /*optional=*/true, "The increase/decrease in size for the utxo index, not counting unspendables"}, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
New UTXO statistics fields need documentation
The addition of utxo_increase_actual and utxo_size_inc_actual fields that exclude unspendable outputs is useful, but the RPC help text should clearly explain the difference between these and the original fields.
Consider updating the RPC help descriptions to be more explicit about what "unspendables" means:
-{RPCResult::Type::NUM, "utxo_increase_actual", /*optional=*/true, "The increase/decrease in the number of unspent outputs, not counting unspendables"},
-{RPCResult::Type::NUM, "utxo_size_inc_actual", /*optional=*/true, "The increase/decrease in size for the utxo index, not counting unspendables"},
+{RPCResult::Type::NUM, "utxo_increase_actual", /*optional=*/true, "The increase/decrease in the number of unspent outputs, excluding OP_RETURN and other provably unspendable outputs"},
+{RPCResult::Type::NUM, "utxo_size_inc_actual", /*optional=*/true, "The increase/decrease in size for the utxo index, excluding OP_RETURN and other provably unspendable outputs"},📝 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.
| {RPCResult::Type::NUM, "utxo_increase", /* optional */ true, "The increase/decrease in the number of unspent outputs (not discounting op_return and similar)"}, | |
| {RPCResult::Type::NUM, "utxo_size_inc", /* optional */ true, "The increase/decrease in size for the utxo index (not discounting op_return and similar)"}, | |
| {RPCResult::Type::NUM, "utxo_increase_actual", /*optional=*/true, "The increase/decrease in the number of unspent outputs, not counting unspendables"}, | |
| {RPCResult::Type::NUM, "utxo_size_inc_actual", /*optional=*/true, "The increase/decrease in size for the utxo index, not counting unspendables"}, | |
| {RPCResult::Type::NUM, "utxo_increase", /* optional */ true, "The increase/decrease in the number of unspent outputs (not discounting op_return and similar)"}, | |
| {RPCResult::Type::NUM, "utxo_size_inc", /* optional */ true, "The increase/decrease in size for the utxo index (not discounting op_return and similar)"}, | |
| {RPCResult::Type::NUM, "utxo_increase_actual", /*optional=*/true, "The increase/decrease in the number of unspent outputs, excluding OP_RETURN and other provably unspendable outputs"}, | |
| {RPCResult::Type::NUM, "utxo_size_inc_actual", /*optional=*/true, "The increase/decrease in size for the utxo index, excluding OP_RETURN and other provably unspendable outputs"}, |
🤖 Prompt for AI Agents
In src/rpc/blockchain.cpp around lines 2012 to 2015, the new RPC result fields
utxo_increase_actual and utxo_size_inc_actual lack clear documentation
distinguishing them from the original fields. Update the help text for these
fields to explicitly explain that "unspendables" refers to outputs that cannot
be spent, such as those marked by OP_RETURN or similar scripts, to clarify the
difference between the actual and original UTXO statistics.
`feature_blockfilterindex_prune.py` changes to `feature_index_prune.py` in bitcoin#21726 (backported as baf6e26, dash#6327), so changes are made there instead.
…uests per second
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/util/setup_common.cpp (1)
594-638: Consider refactoring the complex transaction generation logicThe
PopulateMempoolmethod provides valuable functionality for generating randomized test transactions, but the implementation is quite complex. Consider breaking it into smaller helper functions for better maintainability.The structured binding usage (line 607) requires C++17, which should be fine for this codebase.
Consider extracting helper functions like:
private: CMutableTransaction CreateRandomTransaction(FastRandomContext& det_rand, std::deque<std::pair<COutPoint, CAmount>>& unspent_prevouts); void AddTransactionOutputsToUnspent(const CTransactionRef& ptx, CAmount amount_per_output, std::deque<std::pair<COutPoint, CAmount>>& unspent_prevouts, FastRandomContext& det_rand);This would improve readability and make the logic easier to test independently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (58)
configure.ac(1 hunks)src/addrdb.cpp(1 hunks)src/bench/bls.cpp(1 hunks)src/bench/bls_dkg.cpp(1 hunks)src/bench/mempool_stress.cpp(2 hunks)src/bitcoin-tx.cpp(1 hunks)src/blockencodings.cpp(1 hunks)src/coinjoin/client.cpp(3 hunks)src/coinjoin/server.cpp(4 hunks)src/common/bloom.cpp(1 hunks)src/consensus/tx_verify.cpp(2 hunks)src/core_io.h(1 hunks)src/core_write.cpp(5 hunks)src/evo/core_write.cpp(1 hunks)src/governance/governance.cpp(1 hunks)src/index/coinstatsindex.cpp(1 hunks)src/init.cpp(2 hunks)src/llmq/dkgsession.cpp(1 hunks)src/net_processing.cpp(1 hunks)src/netaddress.h(1 hunks)src/node/miner.cpp(0 hunks)src/node/miner.h(1 hunks)src/random.cpp(1 hunks)src/random.h(1 hunks)src/rest.cpp(3 hunks)src/rpc/blockchain.cpp(12 hunks)src/rpc/mining.cpp(4 hunks)src/rpc/misc.cpp(1 hunks)src/rpc/rawtransaction.cpp(7 hunks)src/rpc/txoutproof.cpp(1 hunks)src/saltedhasher.cpp(1 hunks)src/test/block_reward_reallocation_tests.cpp(1 hunks)src/test/blockfilter_index_tests.cpp(2 hunks)src/test/dynamic_activation_thresholds_tests.cpp(1 hunks)src/test/evo_deterministicmns_tests.cpp(1 hunks)src/test/fuzz/script_format.cpp(1 hunks)src/test/fuzz/transaction.cpp(1 hunks)src/test/miner_tests.cpp(2 hunks)src/test/random_tests.cpp(2 hunks)src/test/txpackage_tests.cpp(1 hunks)src/test/txvalidation_tests.cpp(1 hunks)src/test/txvalidationcache_tests.cpp(1 hunks)src/test/util/setup_common.cpp(6 hunks)src/test/util/setup_common.h(3 hunks)src/util/bytevectorhash.cpp(1 hunks)src/util/hasher.cpp(1 hunks)src/validation.cpp(9 hunks)src/validation.h(1 hunks)src/versionbits.cpp(2 hunks)src/wallet/rpc/transactions.cpp(1 hunks)src/zmq/zmqpublishnotifier.cpp(1 hunks)test/functional/README.md(1 hunks)test/functional/data/rpc_getblockstats.json(3 hunks)test/functional/feature_index_prune.py(4 hunks)test/functional/feature_utxo_set_hash.py(1 hunks)test/functional/rpc_deriveaddresses.py(1 hunks)test/functional/rpc_dumptxoutset.py(1 hunks)test/functional/rpc_getblockstats.py(3 hunks)
💤 Files with no reviewable changes (1)
- src/node/miner.cpp
✅ Files skipped from review due to trivial changes (2)
- src/test/fuzz/transaction.cpp
- test/functional/feature_utxo_set_hash.py
🚧 Files skipped from review as they are similar to previous changes (54)
- src/versionbits.cpp
- test/functional/rpc_dumptxoutset.py
- test/functional/README.md
- src/consensus/tx_verify.cpp
- src/index/coinstatsindex.cpp
- src/llmq/dkgsession.cpp
- src/saltedhasher.cpp
- src/zmq/zmqpublishnotifier.cpp
- test/functional/data/rpc_getblockstats.json
- src/evo/core_write.cpp
- configure.ac
- src/test/blockfilter_index_tests.cpp
- src/node/miner.h
- src/wallet/rpc/transactions.cpp
- test/functional/rpc_deriveaddresses.py
- src/test/miner_tests.cpp
- src/bench/bls_dkg.cpp
- src/governance/governance.cpp
- src/rpc/txoutproof.cpp
- src/test/block_reward_reallocation_tests.cpp
- src/test/txpackage_tests.cpp
- src/common/bloom.cpp
- src/rest.cpp
- src/test/evo_deterministicmns_tests.cpp
- src/util/bytevectorhash.cpp
- src/test/fuzz/script_format.cpp
- src/addrdb.cpp
- src/bitcoin-tx.cpp
- src/rpc/rawtransaction.cpp
- test/functional/feature_index_prune.py
- src/coinjoin/server.cpp
- src/test/random_tests.cpp
- test/functional/rpc_getblockstats.py
- src/test/txvalidationcache_tests.cpp
- src/blockencodings.cpp
- src/test/txvalidation_tests.cpp
- src/test/dynamic_activation_thresholds_tests.cpp
- src/net_processing.cpp
- src/bench/mempool_stress.cpp
- src/util/hasher.cpp
- src/rpc/mining.cpp
- src/rpc/misc.cpp
- src/bench/bls.cpp
- src/init.cpp
- src/coinjoin/client.cpp
- src/test/util/setup_common.h
- src/random.h
- src/core_io.h
- src/core_write.cpp
- src/validation.cpp
- src/rpc/blockchain.cpp
- src/validation.h
- src/random.cpp
- src/netaddress.h
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (5)
src/test/util/setup_common.cpp (5)
11-11: LGTM: Include for merkle functionalityThe addition of
consensus/merkle.his appropriate for the new manual merkle root calculation in theCreateBlockmethod.
71-73: LGTM: Standard library includesThe additions of
algorithmandstdexceptheaders support the newPopulateMempoolmethod functionality.
220-220: LGTM: Configurable mempool checkingGood improvement to use the
-checkmempoolargument from node settings instead of a hardcoded value, while maintaining the same default behavior.
369-375: LGTM: Explicit chain name parametersThe constructor updates to accept explicit
chain_nameparameters improve configurability and make the test setup more explicit. The inheritance change fromRegTestingSetuptoTestingSetupaligns with the modernization goals.
507-515: LGTM: Manual coinbase construction replaces IncrementExtraNonceThe implementation correctly replaces the removed
IncrementExtraNoncefunction by manually constructing the coinbase scriptSig and recalculating the merkle root. The logic properly:
- Sets the coinbase scriptSig with height + 1 and nonce value of 1
- Recalculates the merkle root using
BlockMerkleRoot(block)- Maintains thread safety with the lock
3 participants
Additional Information
In bitcoin#23732, the automatic wallet backup logic (source) will ignore
-privdband use shared memory by default (this is the default behavior for-privdb) asCWallet::AutoBackupWallet()'s callers do not have access to database options or the context needed to read them (example).When backporting bitcoin#24636, the changes to the RPC description of
decodepsbtwere not applied as we don't support returning awitness_utxoobject on account of not supporting SegWit.The
-deprecatedrpc=addressesargument was removed in bitcoin#22650 (9faa63f in dash#6569), a leftover infeature_dip3_deterministicmns.pywas tidied up before backporting bitcoin#24673.In bitcoin#24732, the changes meant for
feature_blockfilterindex_prune.pyare applicable forfeature_index_prune.pydue to the OOO backport of bitcoin#21726 (baf6e26 in dash#6327).TestChainSetup's logic to replaceIncrementExtraNonce()is meant to replicate the behavior ofIncrementExtraNonce()to avoid invalidating the hardcoded checkpoints already generated (source).Breaking Changes
None expected.
Checklist