backport: bitcoin#22219, #22437, #22641, #22707, #22744, #22841, #22880, #22992, #23086, #24527 #6568
Conversation
|
This pull request has conflicts, please rebase. |
WalkthroughThe pull request applies a broad set of modifications across configuration, build, initialization, and test files. Changes include updates to the CI configuration scripts by adding comments and reordering commands, as well as adjustments in compiler flags for fuzz testing. Several source files have been modified: new source files have been added to build targets, method signatures and variable declarations in initialization, chain parameters, and wallet components have been updated, and the interfaces used to create nodes and chains have been restructured. Changes in RPC and QT files update how nodes are created and how transactions are decomposed. Additionally, the pull request revises multiple test scripts by adding new test methods, updating assertions, and refining error handling checks, particularly in raw transaction and message signing tests. These changes span documentation updates, source code modifications, and enhancements to testing procedures without altering the declared public entities. ✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
.cirrus.yml (1)
1-1:⚠️ Potential issueAddress the fast-forward merge failure.
The pipeline is failing because the branch cannot be fast-forwarded. This needs to be resolved before merging.
Options:
- Rebase your branch on the latest main branch
- Use merge with --no-ff flag
Run these commands to resolve the issue:
# Option 1: Rebase (recommended) git fetch origin main git rebase origin/main # Option 2: Merge with --no-ff git fetch origin main git merge --no-ff origin/main
🧹 Nitpick comments (4)
test/functional/wallet_signmessagewithaddress.py (1)
20-41: Consider adding more test cases for edge conditions.While the current test coverage is good, consider adding the following test cases:
- Empty message
- Unicode message
- Very long message
Here's a suggested implementation:
def run_test(self): message = 'This is just a test message' + edge_cases = { + 'empty': '', + 'unicode': '测试消息', + 'long': 'x' * 10000 + } + + # Test edge cases + for case_name, test_message in edge_cases.items(): + self.log.info(f'test signing {case_name} message') + address = self.nodes[0].getnewaddress() + signature = self.nodes[0].signmessage(address, test_message) + assert self.nodes[0].verifymessage(address, signature, test_message)test/functional/rpc_signmessagewithprivkey.py (1)
18-43: Consider improving test maintainability and coverage.Two suggestions for improvement:
- Move hardcoded test values to class-level constants or test fixtures
- Add test cases for different invalid private key formats
Here's a suggested implementation:
+ # Test constants + TEST_KEYS = { + 'valid': { + 'privkey': 'cU4zhap7nPJAWeMFu4j6jLrfPmqakDAzy8zn8Fhb3oEevdm4e5Lc', + 'address': 'yeMpGzMj3rhtnz48XsfpB8itPHhHtgxLc3', + 'signature': 'ICzMhjIUmmXcPWy2+9nw01zQMawo+s5FIy6F7VMkL+TmIeNq1j3AMEuw075os29kh5KYLbysKkDlDD+EAqERBd4=' + }, + 'invalid': [ + 'invalid_key', + '0123456789', + 'ABCDEF', + 'x' * 100 + ] + } + def run_test(self): message = 'This is just a test message' self.log.info('test signing with priv_key') - priv_key = 'cU4zhap7nPJAWeMFu4j6jLrfPmqakDAzy8zn8Fhb3oEevdm4e5Lc' - address = 'yeMpGzMj3rhtnz48XsfpB8itPHhHtgxLc3' - expected_signature = 'ICzMhjIUmmXcPWy2+9nw01zQMawo+s5FIy6F7VMkL+TmIeNq1j3AMEuw075os29kh5KYLbysKkDlDD+EAqERBd4=' + priv_key = self.TEST_KEYS['valid']['privkey'] + address = self.TEST_KEYS['valid']['address'] + expected_signature = self.TEST_KEYS['valid']['signature'] signature = self.nodes[0].signmessagewithprivkey(priv_key, message) assert_equal(expected_signature, signature) assert self.nodes[0].verifymessage(address, signature, message) + + self.log.info('test invalid private key formats') + for invalid_key in self.TEST_KEYS['invalid']: + assert_raises_rpc_error(-5, "Invalid private key", + self.nodes[0].signmessagewithprivkey, + invalid_key, message)src/qt/bitcoin.h (1)
56-57: Introducing createNode method
ReplacingsetNodewithcreateNodeclarifies the node ownership model. An optional error handling path could be added to address potential failures during node creation.ci/lint/06_script.sh (1)
31-32: Consider consolidating the duplicate git log commands.The same git log command is used to write to two different files. This could be simplified.
- git log HEAD~10 -1 --format='%H' > ./contrib/verify-commits/trusted-sha512-root-commit - git log HEAD~10 -1 --format='%H' > ./contrib/verify-commits/trusted-git-root + commit_hash=$(git log HEAD~10 -1 --format='%H') + echo "$commit_hash" > ./contrib/verify-commits/trusted-sha512-root-commit + echo "$commit_hash" > ./contrib/verify-commits/trusted-git-root
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
.cirrus.yml(1 hunks)INSTALL.md(1 hunks)ci/lint/06_script.sh(1 hunks)ci/test/00_setup_env_native_fuzz.sh(1 hunks)src/Makefile.qt.include(1 hunks)src/Makefile.qttest.include(1 hunks)src/chainparams.cpp(2 hunks)src/dummywallet.cpp(2 hunks)src/init.cpp(2 hunks)src/init/bitcoin-node.cpp(2 hunks)src/init/bitcoind.cpp(2 hunks)src/interfaces/init.cpp(1 hunks)src/interfaces/init.h(2 hunks)src/interfaces/node.h(1 hunks)src/node/interfaces.cpp(2 hunks)src/qt/bitcoin.cpp(4 hunks)src/qt/bitcoin.h(3 hunks)src/qt/test/addressbooktests.cpp(1 hunks)src/qt/test/test_main.cpp(3 hunks)src/qt/test/wallettests.cpp(1 hunks)src/qt/transactionrecord.cpp(2 hunks)src/qt/transactionrecord.h(1 hunks)src/qt/transactiontablemodel.cpp(2 hunks)src/rpc/misc.cpp(2 hunks)src/test/util/setup_common.cpp(0 hunks)src/wallet/init.cpp(2 hunks)test/functional/mempool_unbroadcast.py(1 hunks)test/functional/rpc_blockchain.py(2 hunks)test/functional/rpc_rawtransaction.py(10 hunks)test/functional/rpc_signmessagewithprivkey.py(2 hunks)test/functional/test_runner.py(1 hunks)test/functional/wallet_signmessagewithaddress.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/test/util/setup_common.cpp
✅ Files skipped from review due to trivial changes (1)
- INSTALL.md
🧰 Additional context used
🪛 GitHub Actions: Check Merge Fast-Forward Only
ci/test/00_setup_env_native_fuzz.sh
[error] 1-1: Diverging branches can't be fast-forwarded, you need to either merge with --no-ff or rebase.
[error] 1-1: Process completed with exit code 128.
ci/lint/06_script.sh
[error] 1-1: Not possible to fast-forward, aborting.
[warning] 1-1: Diverging branches can't be fast-forwarded, you need to either: git merge --no-ff or: git rebase.
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Build (win64, x86_64-w64-mingw32, win64)
- GitHub Check: Build (linux64_ubsan, x86_64-pc-linux-gnu, linux64)
- GitHub Check: Build (linux64_tsan, x86_64-pc-linux-gnu, linux64)
- GitHub Check: Build (linux64_sqlite, x86_64-pc-linux-gnu, linux64)
- GitHub Check: Build (linux64_nowallet, x86_64-pc-linux-gnu, linux64)
- GitHub Check: Build (linux64_fuzz, x86_64-pc-linux-gnu, linux64)
- GitHub Check: Build (linux64_cxx20, x86_64-pc-linux-gnu, linux64)
- GitHub Check: Build (arm-linux, arm-linux-gnueabihf, arm-linux)
🔇 Additional comments (64)
test/functional/wallet_signmessagewithaddress.py (1)
1-19: LGTM! Well-structured test setup.The test setup follows best practices with proper initialization and dependency checks.
test/functional/rpc_signmessagewithprivkey.py (1)
1-17: LGTM! Clear and focused test setup.The class name and docstring changes improve clarity by specifically indicating the private key focus.
test/functional/test_runner.py (1)
282-283: LGTM! Test runner updated correctly.The test runner has been properly updated to include the new test files for message signing functionality.
src/init/bitcoin-node.cpp (5)
5-5: Include Relevance
No major issues. The new dependency on<interfaces/chain.h>is consistent with the chain creation logic introduced below.
9-10: Consistent includes for node and wallet interfaces
These includes enable the new node and wallet interface factory methods used in the class.
30-30: Ensure safe usage of the node interface
Returning astd::unique_ptris appropriate for clear ownership. However, verify thatm_nodeis fully initialized before callinginterfaces::MakeNodeto avoid potential null references.
31-31: Chain creation approach
This method also follows a clear ownership model for the chain interface. The code is straightforward and consistent with themakeNodeapproach.
32-35: Enhanced wallet loader creation
UsingAssert(m_node.args)is acceptable, but ensurem_node.argscan never be null. Consider defensive checks wherem_nodeis constructed.src/qt/bitcoin.h (2)
29-31: Forward-declaration of interfaces::Init
Good practice to reduce header dependencies and improve build times.
109-109: Use of std::unique_ptr for node
Managing the node’s lifetime viastd::unique_ptrsimplifies memory handling. For stability, ensurenode()usage accounts for the possibility of a null state if creation did not succeed.test/functional/rpc_rawtransaction.py (13)
8-13: Updated test scope
Listing the RPCs under test in the docstring improves clarity for future maintainers.
31-32: Static TXID
Defining a constant for reuse across multiple tests helps avoid magic values and improves maintainability.
55-61: Increased node count & extra_args
Running four nodes with varied-txindexsettings broadens test coverage and ensures consistent behavior with and without indexing enabled.
76-82: Pre-test funding
Generating preliminary coins ensures each test has sufficient inputs. The approach here is straightforward and effective.
84-92: Method-based test structure
Invoking discrete test methods for each RPC improves readability and makes the suite easier to maintain.
93-179: Comprehensive getrawtransaction tests
This covers multiple verbose modes, block-hash usage, and edge cases related to invalid parameters. Excellent scope and detail.
180-257: Robust createrawtransaction tests
Ensuring that invalid inputs, duplicated addresses, negative amounts, and locktimes are handled correctly fortifies reliability.
258-306: Signrawtransactionwithwallet coverage
The tests scrutinize missing previous transaction info, partial signing, and user errors thoroughly.
307-314: Testing sendrawtransaction with missing inputs
Validates that transactions referencing unknown or already spent outputs are rejected. This prevents flawed broadcasts.
315-366: Max fee rate validations
Enforcing maximum fee constraints is crucial. The tests confirm that excessive fees are rejected, yet transactions can proceed when the fee is within limits.
367-375: Handling transactions already in chain
Confirming that a known transaction is rejected from mempool avoids duplicates. Solid check for normal network behavior.
376-396: Testing transaction version bounds
Ensuring the minimum and maximum transaction versions behave correctly helps avert potential numeric overflows or underflows.
398-513: Comprehensive raw multisig transaction tests (legacy)
Covers 2-of-2 and 2-of-3 workflows, partial signatures from multiple wallets, and final combination. Thorough coverage of legacy multisig complexities.src/qt/bitcoin.cpp (4)
16-16: No issues with added include.
The new include for<interfaces/init.h>is necessary to referenceinterfaces::Init.
273-279: Instantiation logic appears sound.
Creating the node viainit.makeNode()and assigning it tom_nodewithincreateNodeis clear and aligns with the new interface-based design. Theassert(!m_node);call helps prevent accidental re-creation.
485-486: Confirm setup sequence.
MakeNodeInitis invoked prior toSetupEnvironment(). Although it appears to work, ensure there are no environment-dependent tasks being carried out prematurely.
734-734: Consistent node creation call.
Callingapp.createNode(*init)fits the intended flow for node initialization and matches the updated design.src/interfaces/init.cpp (1)
14-14: Method signature updated without usage of parameter.
The extraCoinJoin::Loaderparameter is unutilized in this stub method. Ensure future or external code references validate usage, or remove for clarity if unnecessary.src/init/bitcoind.cpp (2)
5-9: Include additions are appropriate.
The newly added header files forchain,echo,node, andwalletare required for the expanded interfaces in this file. No issues found.
25-31: New interface method implementations look consistent.
These overrides properly create and return unique pointers toNode,Chain,WalletLoader, andEcho. Ensure concurrency or repeated usage is handled correctly, as calling these methods multiple times would each return new objects pointing to the sameNodeContext.src/interfaces/init.h (2)
19-22: LGTM! Clean namespace and class declaration.The new
CoinJoinnamespace andLoaderclass declaration follow proper C++ conventions.
37-37: Verify impact of makeWalletLoader signature change.The method signature has been updated to include a new
CoinJoin::Loaderparameter. Please ensure all implementations and call sites are updated accordingly.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for makeWalletLoader implementations and calls rg -A 5 "makeWalletLoader"Length of output: 2156
Impact of makeWalletLoader Signature Change Verified
- The interface declaration in
src/interfaces/init.hnow includes the newCoinJoin::Loaderparameter.- Implementations in
src/interfaces/init.cpp,src/init/bitcoind.cpp, andsrc/init/bitcoin-node.cppall correctly update the signature.- Call sites (e.g., in
src/wallet/init.cpp) have been updated accordingly.src/Makefile.qttest.include (1)
36-36: LGTM! Source file properly added to test target.The addition of
init/bitcoind.cpptoqt_test_test_dash_qt_SOURCESis correct and follows the Makefile's structure.src/dummywallet.cpp (2)
16-16: LGTM! Clean forward declaration.The
WalletClientforward declaration is properly placed with other interface declarations.
85-88: LGTM! Consistent error handling for non-wallet build.The
MakeWalletClientfunction follows the same pattern asMakeWallet, correctly throwing an error in non-wallet builds.src/qt/test/test_main.cpp (1)
87-87: LGTM! Node initialization updated to use Init interface.The change to use
createNode(*init)aligns with the new initialization pattern.test/functional/mempool_unbroadcast.py (1)
93-93: LGTM! Efficient use of getmempoolentry API.The change optimizes the unbroadcast status check by using
getmempoolentry()instead of fetching the entire mempool information.src/qt/transactionrecord.h (1)
132-132: Verify all callers of decomposeTransaction are updated.The method signature has been updated to include a node parameter. This is a breaking change that requires updates to all calling code.
Let's verify all callers have been updated:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for all calls to decomposeTransaction ast-grep --pattern 'decomposeTransaction($$$)'Length of output: 214
Let's search again for calls to
decomposeTransactionin the common C++ source files to ensure that no caller missed the updated signature.
🏁 Script executed:
#!/bin/bash # Search for calls to decomposeTransaction in C++ source and header files rg "decomposeTransaction\(" -g "*.cpp" -g "*.h" -g "*.hpp" || trueLength of output: 706
All callers of
decomposeTransactionhave been updated correctly.
- The search results show that the only callers in C++ source files are in
src/qt/transactiontablemodel.cpp, and both calls now pass the node parameter asparent->walletModel->node(), matching the updated signature.- No outdated call sites were found.
src/qt/test/addressbooktests.cpp (1)
66-67: LGTM! Proper wallet loader initialization.The wallet loader is correctly initialized and assigned to the node before setting up the node context, ensuring proper wallet setup for testing.
src/qt/test/wallettests.cpp (1)
111-112: LGTM! Consistent wallet loader initialization.The wallet loader initialization matches the pattern in addressbooktests.cpp, demonstrating a systematic approach to wallet setup in tests.
src/interfaces/node.h (1)
365-365: LGTM! The function signature change improves safety.The change from pointer to reference ensures that a valid
NodeContextmust be provided, preventing potential null pointer dereferences.Run the following script to verify the impact on callers:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Find all callers of MakeNode to ensure they handle the new signature. rg -A 5 $'MakeNode'Length of output: 4505
Verification of MakeNode Callers for Updated Signature
- The
rgsearch results show that all call sites invokeMakeNodewith a variable (e.g.,m_node) that satisfies the requiredNodeContext¶meter.- There are no instances of calls passing null pointers or invalid arguments.
- The updated reference parameter is applied consistently in both the declaration (in
src/interfaces/node.h) and the implementation (insrc/node/interfaces.cpp).src/qt/transactionrecord.cpp (2)
30-30: LGTM! The method signature change improves efficiency.The addition of
interfaces::Node& nodeparameter eliminates the need for creating a new node instance within the method.
39-39: LGTM! Using the passed node parameter directly.Accessing
coinJoinOptionsfrom the passed node parameter instead of creating a new node instance improves efficiency.src/Makefile.qt.include (1)
419-419: LGTM! Adding initialization code to Qt targets.Including
init/bitcoind.cppin both Qt targets ensures consistent initialization behavior.Also applies to: 426-426
src/wallet/init.cpp (2)
9-9: LGTM! Adding required include for initialization interface.Including
interfaces/init.his necessary for accessing the initialization interface.
191-193: LGTM! Using initialization interface for wallet loader creation.Using
node.init->makeWalletLoaderimproves modularity by delegating wallet loader creation to the initialization interface.test/functional/rpc_blockchain.py (3)
160-172: Test cases added for invalid-testactivationheightparameter formats.Good addition of test cases to verify error handling for invalid formats:
- Invalid name format
- Invalid height value
- Empty format
173-190: Test cases added for valid-testactivationheightparameter formats.Good addition of test cases to verify all supported activation heights:
- BIP34, DerSig, CLTV, CSV, BIP147
- DIP0001, DIP0003, DIP0008, DIP0024
- BRR, v19, v20, MN_RR
203-216: Updated softfork heights in assertions.The softfork heights have been updated to match the new activation heights set in the test cases above.
src/qt/transactiontablemodel.cpp (2)
117-117: Updated method call to include node parameter.The call to
TransactionRecord::decomposeTransactionhas been updated to include thenode()parameter from the wallet model.
177-177: Updated method call to include node parameter.The call to
TransactionRecord::decomposeTransactionhas been updated to include thenode()parameter from the wallet model.src/node/interfaces.cpp (2)
308-308: Updated constructor to use reference instead of pointer.Good change to use reference instead of pointer for required non-null parameter.
1029-1029: Updated MakeNode function to use reference instead of pointer.Good change to use reference instead of pointer for required non-null parameter.
src/rpc/misc.cpp (2)
1383-1385: Improved code readability by introducing local variable.Good change to store the init reference in a local variable to avoid redundant context access.
1403-1403: Updated method call to use local init reference.Good change to use the local init reference for consistency.
src/chainparams.cpp (2)
789-789: LGTM! Setting BIP147 active from genesis in regtest.Setting
BIP147Height = 0ensures BIP147 (signature verification mechanism) is active from the genesis block in regtest, which is appropriate for a testing environment.
1031-1032: LGTM! Synchronized activation of DIP0024 and its quorums.Setting both
DIP0024HeightandDIP0024QuorumsHeightto the same value ensures synchronized activation of DIP0024 and its quorums, which is the expected behavior.src/init.cpp (1)
1435-1435: LGTM! Good object-oriented design improvement.Moving from static
interfaces::MakeChain(node)to instance methodnode.init->makeChain()improves encapsulation and testability while maintaining consistent initialization patterns.ci/test/00_setup_env_native_fuzz.sh (2)
19-19: LGTM! Good security enhancement for fuzz testing.Adding
-ftrivial-auto-var-init=patternto both C and C++ compiler flags helps catch use-before-initialization bugs by initializing automatic variables with a pattern. This is particularly valuable for fuzz testing to identify potential memory-related security issues.
1-1: Resolve merge conflict before proceeding.The pipeline shows that this PR cannot be fast-forwarded due to diverging branches. Please either:
- Rebase your branch on the target branch, or
- Merge with --no-ff flag
🧰 Tools
🪛 GitHub Actions: Check Merge Fast-Forward Only
[error] 1-1: Diverging branches can't be fast-forwarded, you need to either merge with --no-ff or rebase.
[error] 1-1: Process completed with exit code 128.
ci/lint/06_script.sh (2)
26-32: Verify the commit range for sanity checks.The change from checking merge commits to checking the last 10 commits is a significant change in verification strategy. While this provides broader coverage, we should ensure it doesn't impact performance or cause false positives.
Run this script to analyze the impact:
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Compare the number of commits checked in both approaches # Check number of merge commits in last 2 days (old approach) echo "Old approach (merge commits in last 2 days):" git log --merges --since="2 days ago" --oneline | wc -l # Check number of commits in last 10 (new approach) echo "New approach (last 10 commits):" git log -10 --oneline | wc -lLength of output: 171
Verification of Commit Range for Sanity Checks
The test script did not output any commit counts for either approach. This is likely because the branch used for testing doesn’t have any merge commits from the last two days and might not have a sufficient history to show 10 commits. Therefore, we cannot conclusively determine whether switching from a merge-commit filter to a fixed “last 10 commits” check adversely impacts performance or increases the chance for false positives in a typical active branch.
- ci/lint/06_script.sh (lines 26-32): The change now uses a fixed commit range (via
HEAD~10 -1) rather than filtering merge commits.- Test Outcome: The shell script produced no numbers for either approach, indicating that the current test branch isn’t representative of normal activity.
- Recommendation: Verify the new strategy against a branch/environment with an active commit history to ensure it covers enough commits without performance issues or false positives.
34-35: Verify GPG key server availability and fallback options.The script uses keys.openpgp.org for key retrieval. Ensure there's proper error handling if the key server is unavailable.
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Test key server availability and response time # Test key server connection timeout 5 gpg --keyserver hkps://keys.openpgp.org --recv-keys 0x1234567890ABCDEF 2>/dev/null if [ $? -eq 124 ]; then echo "Warning: Key server connection timed out" fiLength of output: 104
GPG Key Server Fallback Verification Required
The test script did not trigger the timeout warning, and the output indicates that the command didn't hit the fallback error handling (exit code check 124). This means the current approach may not reliably detect when keys.openpgp.org is unavailable. Please manually verify in your environment whether a non-responsive key server is appropriately handled and consider enhancing error logging or fallbacks..cirrus.yml (2)
16-18: LGTM! Clear documentation of prerequisites.The added comments clearly explain why git is installed unconditionally and why the default git author is set.
21-22: Verify the order of operations in merge_base_script.The reordering of commands ensures git is available before any git operations, but we should verify this doesn't affect other parts of the CI pipeline.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Analyze the dependencies between CI steps # Check if any other scripts depend on the PR check rg -l "CIRRUS_PR" .Length of output: 42
I'll now search the repository for any occurrences of "merge_base_script" to review its implementation and confirm if the new ordering (checking for PR presence before fetching) introduces any side effects.
🏁 Script executed:
#!/bin/bash rg -l "merge_base_script" .Length of output: 27
I'll now inspect the first 30 lines of the
.cirrus.ymlfile to review the context and confirm whether the ordering of operations in the "merge_base_script" section might impact other parts of the CI pipeline.
🏁 Script executed:
#!/bin/bash sed -n '1,30p' .cirrus.ymlLength of output: 1707
Based on the analysis:
- The
.cirrus.ymlfile now clearly installs git (and configures it) before any git operations are performed.- The early exit when not running a PR (i.e. when
CIRRUS_PRis empty) means that the merge operations (fetch/merge) are skipped appropriately.- A search for other CI scripts referencing
CIRRUS_PR(e.g. inci/lint/06_script.sh) did not reveal any dependency on the merge base commands that would be negatively affected by this ordering.Thus, the ordering in
merge_base_scriptappears intentional and correct; installing and configuring git is ensured prior to any git operations, and non-PR runs exit early as expected. There is no evidence that this reordering adversely affects other parts of the CI pipeline.Action: No further changes are necessary.
| int unused_exit_status; | ||
| std::unique_ptr<interfaces::Init> init = interfaces::MakeNodeInit(node_context, argc, argv, unused_exit_status); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider handling the exit status.
The unused_exit_status variable is not checked. Consider handling it to properly manage test initialization failures.
🏁 Script executed:
#!/bin/bash
# Search for other MakeNodeInit calls to see how exit status is handled
rg -A 5 "MakeNodeInit.*exit_status"Length of output: 2413
Action Required: Handle the Exit Status in Test Initialization
The grep results confirm that in other parts of the codebase (e.g., in src/bitcoind.cpp), the return value of MakeNodeInit is checked and, if it fails, the associated exit status is returned. However, in src/qt/test/test_main.cpp the unused_exit_status is not handled after calling MakeNodeInit. This inconsistency may lead to improperly managed initialization failures in your test environment.
- Locations to address:
src/qt/test/test_main.cpp(Lines 66-67): Add exit status handling similar to:std::unique_ptr<interfaces::Init> init = interfaces::MakeNodeInit(node_context, argc, argv, exit_status); if (!init) { return exit_status; }
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/qt/test/test_main.cpp (1)
66-67:⚠️ Potential issueHandle the exit status from MakeNodeInit.
The
unused_exit_statusvariable should be checked to properly handle initialization failures.Apply this diff to handle initialization failures:
int unused_exit_status; std::unique_ptr<interfaces::Init> init = interfaces::MakeNodeInit(node_context, argc, argv, unused_exit_status); +if (!init) { + return unused_exit_status; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/Makefile.qt.include(1 hunks)src/Makefile.qttest.include(1 hunks)src/dummywallet.cpp(2 hunks)src/init.cpp(2 hunks)src/init/bitcoin-node.cpp(2 hunks)src/init/bitcoind.cpp(2 hunks)src/interfaces/init.cpp(1 hunks)src/interfaces/init.h(2 hunks)src/interfaces/node.h(1 hunks)src/node/interfaces.cpp(2 hunks)src/qt/bitcoin.cpp(4 hunks)src/qt/bitcoin.h(3 hunks)src/qt/test/addressbooktests.cpp(1 hunks)src/qt/test/test_main.cpp(3 hunks)src/qt/test/wallettests.cpp(1 hunks)src/qt/transactionrecord.cpp(2 hunks)src/qt/transactionrecord.h(1 hunks)src/qt/transactiontablemodel.cpp(2 hunks)src/rpc/misc.cpp(2 hunks)src/test/util/setup_common.cpp(0 hunks)src/wallet/init.cpp(2 hunks)
💤 Files with no reviewable changes (1)
- src/test/util/setup_common.cpp
🚧 Files skipped from review as they are similar to previous changes (13)
- src/qt/test/addressbooktests.cpp
- src/interfaces/init.cpp
- src/qt/test/wallettests.cpp
- src/wallet/init.cpp
- src/qt/transactionrecord.h
- src/node/interfaces.cpp
- src/rpc/misc.cpp
- src/Makefile.qt.include
- src/qt/transactiontablemodel.cpp
- src/init/bitcoin-node.cpp
- src/Makefile.qttest.include
- src/init/bitcoind.cpp
- src/init.cpp
🔇 Additional comments (17)
src/interfaces/init.h (2)
19-22: LGTM! New namespace for CoinJoin functionality.The addition of the
CoinJoinnamespace with theLoaderclass helps organize and encapsulate CoinJoin-related functionality.
37-37: Verify the impact of the updated method signature.The
makeWalletLoadermethod now requires aCoinJoin::Loaderparameter, which could affect existing callers.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for existing calls to makeWalletLoader rg -A 2 "makeWalletLoader\(" --type cppLength of output: 1482
Updated makeWalletLoader signature integration verified
- The abstract declaration in
src/interfaces/init.hnow requires aconst std::unique_ptr<CoinJoin::Loader>¶meter.- All implementations in
src/interfaces/init.cpp,src/init/bitcoin-node.cpp, andsrc/init/bitcoind.cpphave been updated accordingly.- The single call site in
src/wallet/init.cppcorrectly passesnode.coinjoin_loadertomakeWalletLoader.src/dummywallet.cpp (2)
16-16: LGTM! Forward declaration added.The forward declaration of
WalletClientmaintains clean separation of concerns.
85-88: LGTM! Consistent error handling for non-wallet builds.The implementation of
MakeWalletLoaderfollows the same pattern asMakeWallet, throwing an appropriate error for non-wallet builds.src/qt/bitcoin.h (3)
29-31: LGTM! Clean namespace organization.The forward declaration of
Initin theinterfacesnamespace maintains good code organization.
56-57: LGTM! Enhanced node initialization.The new
createNodemethod provides a more modular approach to node initialization.
109-109: LGTM! Improved memory management.Using
std::unique_ptrform_nodeensures proper RAII and ownership semantics.src/qt/test/test_main.cpp (2)
10-10: LGTM! Required include added.The addition of
interfaces/init.hsupports the new initialization approach.
87-87: LGTM! Updated node initialization.The use of
createNode(*init)aligns with the new modular initialization approach.src/qt/transactionrecord.cpp (2)
30-30: Method signature updated to improve dependency injection.The method now accepts
interfaces::Node& nodeas the first parameter, which is a better design as it:
- Makes dependencies explicit
- Improves testability
- Follows dependency injection principles
39-39: Direct access to coinJoinOptions through node reference.The code now directly accesses
coinJoinOptionsthrough the passed node reference, which is more efficient than creating a new node object.src/qt/bitcoin.cpp (5)
16-16: Added new header for initialization interface.The addition of
interfaces/init.hsupports the new initialization architecture.
273-279: Improved node creation with better ownership semantics.The new
createNodemethod:
- Uses smart pointers for automatic resource management
- Follows RAII principles
- Provides clearer ownership semantics
- Improves exception safety
484-486: Enhanced initialization process with better error handling.The new initialization process:
- Uses
interfaces::Initfor better modularity- Provides explicit error status handling
- Follows a more structured initialization pattern
487-488: Added environment setup and thread naming.Proper thread naming and environment setup improve debugging and monitoring capabilities.
734-734: Updated node creation call to use new initialization interface.The change from
setNodetocreateNodecompletes the transition to the new initialization architecture.src/interfaces/node.h (1)
365-365: Verify impact of breaking change.The removal of the default
nullptrvalue makes this a breaking change that could affect existing callers ofMakeNode.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for all calls to MakeNode to assess impact rg "MakeNode\(" --type cppLength of output: 481
Impact Analysis of Default Parameter Removal
Based on the search results, all existing calls toMakeNodeexplicitly pass theNodeContextargument (e.g.,m_node). There are no instances where callers rely on a defaultnullptr. Therefore, the removal of the default value does not impact any current usage.
- Call Sites Reviewed:
src/init/bitcoind.cppsrc/init/bitcoin-node.cpp
| @@ -362,7 +362,7 @@ class Node | |||
| }; | |||
|
|
|||
| //! Return implementation of Node interface. | |||
| std::unique_ptr<Node> MakeNode(NodeContext* context = nullptr); | |||
| std::unique_ptr<Node> MakeNode(NodeContext& context); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider maintaining consistency in context handling pattern.
The change from NodeContext* context = nullptr to NodeContext& context creates an inconsistency with other context-related methods in the file. All other methods (e.g., setContext in EVO, GOV, LLMQ interfaces) use pointer-based context handling.
Consider one of these approaches to maintain consistency:
- Keep using pointer but make it non-nullable:
-std::unique_ptr<Node> MakeNode(NodeContext& context);
+std::unique_ptr<Node> MakeNode(NodeContext* context /* not null */);- Update all context-related methods to use references:
-virtual void setContext(NodeContext* context) {}
+virtual void setContext(NodeContext& context) {}📝 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.
| std::unique_ptr<Node> MakeNode(NodeContext& context); | |
| std::unique_ptr<Node> MakeNode(NodeContext* context /* not null */); |
knst
changed the title
…ckchain fa4ca8d test: Add -testactivationheight tests to rpc_blockchain (MarcoFalke) Pull request description: Suggested: bitcoin#22818 (comment) ACKs for top commit: laanwj: Code review ACK fa4ca8d theStack: Concept and code-review ACK fa4ca8d Tree-SHA512: 41304db1a15c0c705a9cc2808c9f1d7831a321a8a7948a28ec5d3ee1ed3da6a0ce67cd50c99a33aaed86830c59608eb6ffadbeaba67d95245c490f9b6c277912
5ce3057 test: set segwit height back to 0 on regtest (Martin Zumsande) Pull request description: The change of `consensus.SegwitHeight` from 0 to 1 for regtest in bitcoin#22818 had the effect that if I create a regtest enviroment with current master (or 23.x), and then try to load this chain with an older version (22.x), I get an InitError `Witness data for blocks after height 0 requires validation. Please restart with -reindex` and have to reindex because `BLOCK_OPT_WITNESS` is no longer set for the Genesis block and `NeedsRedownload()` in validation returns `true` with an older version. That might be a bit annoying for tests that use a shared regtest dir with different versions. If people think this is enough of an issue to be worth fixing, I think it should also make it into 23.x ACKs for top commit: theStack: Concept and code-review ACK 5ce3057 Tree-SHA512: b0e89ff7fc953bc0ae929d2da44cde7149321d987fb4763934f6c9635d00d807129a50b459cc5e69e86bb1819e4b063b969486e8016a1cb8db8f905fa315653d
…l tests for efficiency 47c48b5 test: only use verbose for getrawmempool when necessary in functional tests (Michael Dietz) 7734971 test: use getmempoolentry instead of getrawmempool in functional tests when appropriate (Michael Dietz) 86dbd54 test: improve mempool_updatefrom efficiency by using getmempoolentry for specific txns (Michael Dietz) Pull request description: I don't think this changes the intention of the test. But it does shave ~30 seconds off the time it takes to run. From what I've seen our CI `macOS 11 native [gui] [no depends]` runs `mempool_updatefrom.py` in ~135 seconds. After this PR it should run in ~105 seconds I noticed this improvement should probably be made when testing performance/runtimes of bitcoin#22698. But I wanted to separate this out from that PR so the affects of each is decoupled Edit: The major change in this PR is improving mempool_updatefrom.py's runtime as this is a very long running test. Then made the same efficiency improvements across all the functional tests as it made since to do that here ACKs for top commit: theStack: Tested ACK 47c48b5 Tree-SHA512: 40f553715f3d4649dc18c2738554eafaca9ea800c4b028c099217896cc1c466ff457ae814d59cf8564c782a8964d8fac3eda60c1b6ffb08bbee1439b2d34434b
…llet a3b559c test: added test for disabled wallet (Shubhankar Gambhir) Pull request description: This PR enables a part of the non-wallet functional test (rpc_signmessage.py) to be run even with the Bitcoin Core wallet disabled, it is inspired by bitcoin#20078. Divided tests in rpc_signmessage.py into 2 files wallet_signmessagewithaddress.py and rpc_signmessagewithprivkey.py, latter one can run even when wallet is disabled that provides extra test which was not performed earlier. * we need bitcoincore wallet to run rpc_signmessage.py, but it is olny required for signing messages with address and not for signing messages wih private key, so latter one can be in a seperate test which can run without wallet * verifying message doesn't require wallet, so it can be used in both tests without any problem * 2 tests are named as wallet_signmessagewithaddress.py and rpc_signmessagewithprivkey.py to provide clarity of what they are testing. ACKs for top commit: vasild: ACK a3b559c theStack: Code-review ACK a3b559c Tree-SHA512: 1bfca3baf3123a02f0a2389e55e141d64430c3bed40ff5a5fb97ef2c66e2853c46e4b2dff62b948eb94dc574cb89d061769330f0535e2d5d1be76b60101136ac
fa00160 ci: Re-enable verify-commits.py check (MarcoFalke) fa880b1 ci: Unconditionally set the global git author name in cirrys.yml (MarcoFalke) Pull request description: Might be useful to detect bugs in the script itself or an accidentally missed signature. ACKs for top commit: josibake: ACK bitcoin@fa00160 Zero-1729: tACK fa00160 fanquake: untested ACK fa00160 Tree-SHA512: 8a13a67d325f2477f4088d1034f0d5e4e04937a01ee3c738435fe66394c02b9f33225529952ad331b0ba19b63ca4b2f26911cb5d264890159840cf3e09085969
…mprove rpc_rawtransaction 387355b test, refactor: rpc_rawtransaction PEP8 (Jon Atack) 7d5cec2 refactor: separate the rpc_rawtransaction tests into functions (Jon Atack) 409779d move-only: regroup similar rpc_rawtransaction tests together (Jon Atack) d861040 test: remove no longer needed (ASCII art) comments (Jon Atack) 14398b3 test: add and harmonize getrawtransaction logging (Jon Atack) 85d8869 test: run 2nd getrawtransaction section with/without -txindex (Jon Atack) 0097740 refactor: txid to constant in rpc_rawtransaction to isolate tests (Jon Atack) 8c19d13 refactor: dedup/reorg createrawtransaction sequence number tests (Jon Atack) 7f07359 Test src/node/transaction::GetTransaction() without -txindex (Jon Atack) Pull request description: Following up on bitcoin#22383 (review), this pull adds missing `src/node/transaction::GetTransaction()` test coverage for combinations of `-txindex` and `blockhash` and does some refactoring of the test file. ACKs for top commit: mjdietzx: reACK 387355b josibake: reACK bitcoin@387355b MarcoFalke: Approach ACK 387355b 🔆 Tree-SHA512: b47c4ff87d69c61434e5729c954b338bc13744eddaba0879ca9f5f42243ba2cb4640d94c5f74de9f2735a8bf5e66b3d1c3bd3b7c26cd7324da7d3270ce87c6fd
fa0a5fa ci: Fuzz with -ftrivial-auto-var-init=pattern (MarcoFalke) Pull request description: This makes memory bugs deterministic. `-ftrivial-auto-var-init=pattern` is incompatible with other memory sanitizers (like valgrind and msan), but that is irrelevant here, because the address sanitizer in this fuzz CI config is already incompatible with them. `-ftrivial-auto-var-init=pattern` goes well with `-fsanitize=bool` and `-fsanitize=enum`, but those are already enabled via `-fsanitize=undefined`. See https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#available-checks ACKs for top commit: practicalswift: cr ACK fa0a5fa Tree-SHA512: ed6be953cd99eadb1ba245ba30170747eff66be54d2773c8d26a3a6aee0fdcd6967c596f4f4ab1d238de6a6526623dac5211f0ba77f1986639395d7921bdc19f
67c85bd doc: Added hyperlink for doc/build (pradumnasaraf) Pull request description: Added hyperlink for the doc/build directory (https://github.com/bitcoin/bitcoin/tree/master/doc). This will be convenient for visitors to redirect.  Top commit has no ACKs. Tree-SHA512: e920487d5b5bc77ae4ad63f47c400834b8ca5b4b199392ff9b8f2166a620e12b842b7dcc4b04e9b2f65903dfbf421625aae41e7d59ae23306a94604106013eca
…in, etc methods e4709c7 Start using init makeNode, makeChain, etc methods (Russell Yanofsky) Pull request description: Use `interfaces::Init::make*` methods instead of `interfaces::Make*` functions, so interfaces can be constructed differently in different executable without having to change any code. (So for example `bitcoin-gui` can make an `interfaces::Node` pointer that communicates with a `bitcoin-node` subprocess, while `bitcoin-qt` can make an `interfaces::Node` pointer that controls node code in the same process.) --- This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). The commit was first part of larger PR bitcoin#10102. ACKs for top commit: jamesob: reACK bitcoin@e4709c7 achow101: ACK e4709c7 benthecarman: utACK e4709c7 Tree-SHA512: 580c1979dbb2ef444157c8e53041e70d15ddeee77e5cbdb34f70b6d228cc2d2fe3843825f172da84e506200c58f7e0932f7cd4c006bb5058c1f4e43259394834
865ee1a Fix Qt test broken by bitcoin#22219 (Russell Yanofsky) Pull request description: It looks like this should have been caught by CI but there might have been a conflict with a recently merged PR like bitcoin#19101. Failure was reported by fanquake bitcoin#22219 (comment) Fix avoids null WalletClient pointer dereference in address book test by adding MakeWalletClient call and making address book test initialization more consistent with wallet test initialization: https://github.com/bitcoin/bitcoin/blob/865ee1af201238f48671e3b79b8215f896db7a05/src/qt/test/addressbooktests.cpp#L63-L66 https://github.com/bitcoin/bitcoin/blob/865ee1af201238f48671e3b79b8215f896db7a05/src/qt/test/wallettests.cpp#L141-L144 ACKs for top commit: fanquake: ACK 865ee1a - I'm merging this now to unbreak the build. Tree-SHA512: 1f32b7fc79fa79fcf8600d23063896cbc7f8bbcff39d95747ecd546e754581f0f36ece3098ddecded175afccbb3709b4232da39a400dda23b7e550f361b515fb
PastaPastaPasta
force-pushed
the
bp-v23-p4
branch
from
b06c5a5 to
bc51716
Compare
There was a problem hiding this comment.
utACK bc51716; I rebased it on latest develop to ensure CI is happy still
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/qt/test/test_main.cpp (1)
66-67:⚠️ Potential issueHandle the Exit Status in Test Initialization
The
unused_exit_statusvariable is not checked after callingMakeNodeInit. This could lead to improperly managed initialization failures in the test environment.Apply this diff to handle initialization failures:
-int unused_exit_status; -std::unique_ptr<interfaces::Init> init = interfaces::MakeNodeInit(node_context, argc, argv, unused_exit_status); +int exit_status; +std::unique_ptr<interfaces::Init> init = interfaces::MakeNodeInit(node_context, argc, argv, exit_status); +if (!init) { + return exit_status; +}
🧹 Nitpick comments (2)
test/functional/wallet_signmessagewithaddress.py (1)
20-41: Consider adding more test cases for edge scenarios.While the current test cases cover basic functionality well, consider adding tests for:
- Empty message
- Unicode messages
- Very long messages
- Messages with special characters
def run_test(self): message = 'This is just a test message' + edge_messages = [ + '', # Empty message + 'Unicode: 你好', # Unicode message + 'a' * 10000, # Very long message + '!@#$%^&*()', # Special characters + ] + + for test_message in edge_messages: + self.log.info(f'test signing message: {test_message[:20]}...') + address = self.nodes[0].getnewaddress() + signature = self.nodes[0].signmessage(address, test_message) + assert self.nodes[0].verifymessage(address, signature, test_message) + # ... rest of the test cases ...test/functional/rpc_signmessagewithprivkey.py (1)
42-43: Consider adding more malformed signature test cases.While testing with an invalid signature is good, consider adding more variations of malformed signatures.
# malformed signature provided assert_raises_rpc_error(-3, "Malformed base64 encoding", self.nodes[0].verifymessage, 'yeMpGzMj3rhtnz48XsfpB8itPHhHtgxLc3', "invalid_sig", message) +# Additional malformed signature cases +malformed_sigs = [ + "=invalid_base64=", # Invalid base64 padding + "!@#$%^&*()", # Non-base64 characters + "a" * 100, # Too long signature + "", # Empty signature +] +for sig in malformed_sigs: + assert_raises_rpc_error(-3, "Malformed base64 encoding", self.nodes[0].verifymessage, 'yeMpGzMj3rhtnz48XsfpB8itPHhHtgxLc3', sig, message)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
.cirrus.yml(1 hunks)INSTALL.md(1 hunks)ci/lint/06_script.sh(1 hunks)ci/test/00_setup_env_native_fuzz.sh(1 hunks)src/Makefile.qt.include(1 hunks)src/Makefile.qttest.include(1 hunks)src/chainparams.cpp(2 hunks)src/dummywallet.cpp(2 hunks)src/init.cpp(2 hunks)src/init/bitcoin-node.cpp(2 hunks)src/init/bitcoind.cpp(2 hunks)src/interfaces/init.cpp(1 hunks)src/interfaces/init.h(2 hunks)src/interfaces/node.h(1 hunks)src/node/interfaces.cpp(2 hunks)src/qt/bitcoin.cpp(4 hunks)src/qt/bitcoin.h(3 hunks)src/qt/test/addressbooktests.cpp(1 hunks)src/qt/test/test_main.cpp(3 hunks)src/qt/test/wallettests.cpp(1 hunks)src/qt/transactionrecord.cpp(2 hunks)src/qt/transactionrecord.h(1 hunks)src/qt/transactiontablemodel.cpp(2 hunks)src/rpc/misc.cpp(2 hunks)src/test/util/setup_common.cpp(0 hunks)src/wallet/init.cpp(2 hunks)test/functional/mempool_unbroadcast.py(1 hunks)test/functional/rpc_blockchain.py(2 hunks)test/functional/rpc_rawtransaction.py(10 hunks)test/functional/rpc_signmessagewithprivkey.py(2 hunks)test/functional/test_runner.py(1 hunks)test/functional/wallet_signmessagewithaddress.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/test/util/setup_common.cpp
🚧 Files skipped from review as they are similar to previous changes (24)
- src/qt/test/wallettests.cpp
- INSTALL.md
- src/interfaces/init.h
- src/node/interfaces.cpp
- src/dummywallet.cpp
- src/interfaces/node.h
- src/qt/test/addressbooktests.cpp
- test/functional/mempool_unbroadcast.py
- src/interfaces/init.cpp
- src/wallet/init.cpp
- test/functional/test_runner.py
- .cirrus.yml
- src/qt/transactionrecord.h
- ci/test/00_setup_env_native_fuzz.sh
- src/qt/bitcoin.h
- src/init/bitcoind.cpp
- src/Makefile.qttest.include
- src/init/bitcoin-node.cpp
- ci/lint/06_script.sh
- src/qt/transactiontablemodel.cpp
- src/Makefile.qt.include
- src/init.cpp
- src/rpc/misc.cpp
- src/qt/transactionrecord.cpp
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
🔇 Additional comments (18)
test/functional/rpc_blockchain.py (3)
160-172: LGTM! Well-structured test cases for invalid parameter formats.The test cases thoroughly validate the
-testactivationheightparameter by checking:
- Invalid name format
- Invalid height value
- Empty parameter value
173-190: LGTM! Comprehensive test case for valid activation heights.The test case thoroughly validates the
-testactivationheightparameter by setting activation heights for all relevant BIPs and DIPs in a logical sequence.
203-216: LGTM! Softfork heights are correctly updated.The softfork heights in the assertions correctly match the activation heights set in the test case, maintaining proper validation of the RPC response structure.
test/functional/wallet_signmessagewithaddress.py (3)
1-11: LGTM! Well-structured file header and imports.The file has a clear purpose, proper copyright notice, and focused imports.
12-19: LGTM! Clean test setup.The test parameters and wallet module check are properly implemented.
43-45: LGTM! Standard test execution setup.The main block follows the standard pattern for test execution.
test/functional/rpc_signmessagewithprivkey.py (3)
5-5: LGTM! Clear and specific docstring.The updated docstring better reflects the focused purpose of testing RPC commands for signing messages with private keys.
13-13: LGTM! Descriptive class name.The renamed class
SignMessagesWithPrivTestbetter reflects its specific responsibility.
30-31: LGTM! Clear parameter requirement comment.The comment accurately describes the parameter requirements for
signmessagewithprivkey.src/qt/test/test_main.cpp (2)
10-10: LGTM!Added include for new initialization interface.
87-87: LGTM!Updated to use the new node creation method which aligns with the architectural changes.
test/functional/rpc_rawtransaction.py (3)
31-31: LGTM!Added constant
TXIDto avoid magic strings in test cases.
84-91: LGTM!Improved test structure by breaking down test cases into specific methods for better organization and maintainability.
93-174: LGTM!Comprehensive test coverage for
getrawtransactionRPC:
- Tests both with and without txindex
- Validates various parameter combinations
- Verifies error handling
- Checks blockhash parameter behavior
src/qt/bitcoin.cpp (2)
16-16: LGTM!Added include for new initialization interface.
273-279: LGTM!Improved node initialization with better ownership semantics:
- Uses unique_ptr for automatic resource management
- Validates node existence with assert
- Updates dependent components with new node instance
src/chainparams.cpp (2)
789-789: LGTM!Changed BIP147 height to 0 in regtest mode to ensure it's always active unless overridden.
1031-1032: LGTM!Added DIP0024 height configuration with both activation and quorum heights set to the same value for consistency.
What was done?
Regular backports from v23
How Has This Been Tested?
Run unit / functional test
Breaking Changes
N/A
Checklist: