backport: bitcoin#22981, #23348, #23462, #23473, #23504, #23510, #23520, #23525, #23546, #23600, partial #23540 #6590
Conversation
WalkthroughThe changes span multiple parts of the project. In CI configuration files, task names and container image references were updated for clarity and consistency. Several build auxiliary scripts and m4 macros received updates to their licensing comments, URLs, serial numbers, and syntax (standardizing the use of square brackets). In shell scripts used for building and testing, variable references were quoted to prevent word splitting issues. Modifications in the source code include renaming parameters in functions (for example, renaming "sigops" to "sigops_count" and "nIn" to "input_idx") and updating function return types to use 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 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: 0
🧹 Nitpick comments (8)
src/validation.cpp (4)
713-716: Consider using a constant for minimum transaction sizeThe minimum transaction size of 83 bytes is hardcoded. Consider extracting this to a named constant to improve maintainability and make the code more self-documenting.
+// A transaction with 1 empty scriptSig input and 1 P2SH output has size of 83 bytes +static constexpr size_t MIN_STANDARD_TX_SIZE = 83; if (::GetSerializeSize(tx, PROTOCOL_VERSION) < MIN_STANDARD_TX_SIZE) return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "tx-size-small");
754-786: Consider using a view stack pattern for coin view managementThe code switches between different coin views but the management of these views could be improved using a view stack pattern to make the transitions more explicit and safer.
+class CCoinsViewStack { + std::vector<CCoinsView*> m_views; +public: + void Push(CCoinsView* view) { + if (!m_views.empty()) { + view->SetBackend(m_views.back()); + } + m_views.push_back(view); + } + void Pop() { + assert(!m_views.empty()); + m_views.pop_back(); + } + CCoinsView* Top() { + return m_views.empty() ? nullptr : m_views.back(); + } +};
1738-1820: Consider batching script verification for performanceThe script verification could be batched to improve performance, especially for blocks with many transactions.
+class CScriptBatch { + std::vector<CScriptCheck> m_checks; + size_t m_batch_size; +public: + explicit CScriptBatch(size_t batch_size) : m_batch_size(batch_size) {} + void Add(CScriptCheck&& check) { + m_checks.push_back(std::move(check)); + if (m_checks.size() >= m_batch_size) { + Flush(); + } + } + void Flush() { + // Process batch in parallel + for (auto& check : m_checks) { + if (!check()) return false; + } + m_checks.clear(); + return true; + } +};
4713-4731: Consider adding progress tracking for reindexThe reindex progress tracking could be improved to provide more detailed feedback about the reindexing process.
+struct ReindexProgress { + int64_t start_time; + int64_t last_update; + int processed_blocks; + int total_blocks; + + void UpdateProgress() { + int64_t now = GetTime(); + if (now - last_update > 10) { + double progress = (double)processed_blocks / total_blocks; + double elapsed = now - start_time; + double eta = elapsed / progress - elapsed; + LogPrintf("Reindex progress: %.2f%% (ETA: %.0f seconds)\n", + progress * 100, eta); + last_update = now; + } + } +};ci/dash/test_integrationtests.sh (1)
45-46: Review of Shellcheck Disabling and Variable Expansion.
Theshellcheck disable=SC2086directive signals an intentional choice to allow word splitting for${TEST_RUNNER_EXTRA},$PASS_ARGS, and$EXTRA_ARGS. Please double-check that these variables are meant to expand into multiple arguments; if not, consider quoting them to prevent unintended splitting..cirrus.yml (3)
113-120: MSan Task Block – Merge Key FormattingIn the MSan task block (lines 113–120), static analysis flagged an issue on the merge key syntax at line 115. The extra space before the colon in:
<< : *GLOBAL_TASK_TEMPLATEshould be removed for better compliance with YAML standards.
Please change it to:- << : *GLOBAL_TASK_TEMPLATE + <<: *GLOBAL_TASK_TEMPLATE🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 113-113: duplication of key "task" in mapping
(key-duplicates)
[warning] 115-115: too many spaces before colon
(colons)
121-128: ASan Task Block – Merge Key ConsistencySimilar to the MSan block, the ASan task block (lines 121–128) exhibits extra spaces in the merge key (notably on line 123). For consistency and to keep the YAML clean, adjust the merge key by removing the extra space. For example:
- << : *GLOBAL_TASK_TEMPLATE + <<: *GLOBAL_TASK_TEMPLATE🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 121-121: duplication of key "task" in mapping
(key-duplicates)
[warning] 123-123: too many spaces before colon
(colons)
129-139: Fuzzer Task Block – Duplicate Key ConsiderationThe fuzzer task definition (lines 129–139) itself is correctly configured with proper resource adjustments. However, YAMLlint reports a "duplication of key 'task'" on line 129.
This warning arises because the file uses multiple top-leveltask:keys, which—although common in Cirrus CI configurations—may conflict with strict YAML validators. Please confirm that the CI parser accepts this structure. If necessary, consider reorganizing all task definitions under a sequence (for example, using a top-leveltasks:list) to resolve the duplication warning.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 129-129: duplication of key "task" in mapping
(key-duplicates)
[warning] 131-131: too many spaces before colon
(colons)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
.cirrus.yml(2 hunks)build-aux/m4/ax_check_compile_flag.m4(2 hunks)build-aux/m4/ax_check_link_flag.m4(2 hunks)build-aux/m4/ax_check_preproc_flag.m4(2 hunks)build-aux/m4/bitcoin_find_bdb48.m4(1 hunks)build-aux/m4/bitcoin_qt.m4(6 hunks)ci/dash/build_depends.sh(1 hunks)ci/dash/build_src.sh(3 hunks)ci/dash/test_integrationtests.sh(2 hunks)ci/dash/test_unittests.sh(2 hunks)ci/test/04_install.sh(1 hunks)configure.ac(43 hunks)contrib/auto_gdb/dash_dbg.sh(1 hunks)contrib/containers/deploy/docker-entrypoint.sh(1 hunks)doc/build-unix.md(1 hunks)src/bench/rpc_mempool.cpp(1 hunks)src/chain.cpp(0 hunks)src/net_processing.cpp(1 hunks)src/psbt.cpp(1 hunks)src/script/sign.cpp(1 hunks)src/script/sign.h(1 hunks)src/test/txpackage_tests.cpp(1 hunks)src/validation.cpp(1 hunks)src/wallet/rpcwallet.cpp(1 hunks)src/wallet/scriptpubkeyman.cpp(2 hunks)src/wallet/scriptpubkeyman.h(3 hunks)src/wallet/wallet.cpp(1 hunks)src/wallet/wallet.h(1 hunks)test/functional/rpc_decodescript.py(6 hunks)test/lint/lint-cppcheck-dash.sh(2 hunks)test/lint/lint-shell.py(0 hunks)
💤 Files with no reviewable changes (2)
- test/lint/lint-shell.py
- src/chain.cpp
✅ Files skipped from review due to trivial changes (13)
- ci/test/04_install.sh
- doc/build-unix.md
- contrib/containers/deploy/docker-entrypoint.sh
- build-aux/m4/ax_check_compile_flag.m4
- build-aux/m4/ax_check_link_flag.m4
- src/psbt.cpp
- ci/dash/build_depends.sh
- src/test/txpackage_tests.cpp
- build-aux/m4/bitcoin_find_bdb48.m4
- src/net_processing.cpp
- build-aux/m4/bitcoin_qt.m4
- src/script/sign.h
- configure.ac
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.cirrus.yml
[warning] 115-115: too many spaces before colon
(colons)
[error] 121-121: duplication of key "task" in mapping
(key-duplicates)
[warning] 123-123: too many spaces before colon
(colons)
[error] 129-129: duplication of key "task" in mapping
(key-duplicates)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (37)
build-aux/m4/ax_check_preproc_flag.m4 (3)
2-2: LGTM! Good security practice using HTTPS.The URL update to use HTTPS is a good security practice.
32-35: LGTM! License text simplified while maintaining legal requirements.The simplified license text preserves all necessary legal requirements including copyright notices and distribution terms.
37-37: LGTM! Serial number update maintains consistency.The serial number update to 6 maintains consistency with related m4 files in the build system.
src/validation.cpp (8)
743-751: LGTM: Mempool conflict check is properly implementedThe code correctly checks for transaction conflicts in the mempool and rejects transactions that would create double spends. The implementation is thread-safe due to the mempool lock.
837-839: LGTM: Special handling for MNHF_SIGNAL transactionsThe code correctly skips fee checks for MNHF_SIGNAL transactions since they don't have inputs, outputs, or fees. This is an important special case handling.
907-911: LGTM: Script validation is properly orderedThe code correctly performs script validation last to prevent CPU exhaustion DoS attacks, since script validation is computationally expensive.
3017-3084: LGTM: ConnectTip implementation is robustThe ConnectTip implementation properly handles all edge cases including:
- Verifying block validity
- Managing the mempool
- Updating indexes
- Handling chainstate updates
- Error recovery
5041-5048: LGTM: LoadMempool has proper error handlingThe LoadMempool function properly handles errors and continues operation even if the mempool loading fails, which is important for robustness.
799-802:✅ Verification successful
Verify sequence lock check optimization
The sequence lock check optimization assumes that passing with useExistingLockPoints=false implies validity of the lock points. This assumption should be verified.
🏁 Script executed:
#!/bin/bash # Search for other sequence lock validation code rg "CheckSequenceLocks|CalculateSequenceLocks"Length of output: 1830
Verification complete: Sequence lock check optimization is valid.
After examining the codebase, we found that the implementation in
src/validation.cpp(and related files such assrc/validation.handsrc/consensus/tx_verify.cpp) consistently uses the technique of passingm_chain.Height() + 1(or similar) when performing sequence lock checks. This pattern confirms that the optimization—that a transaction passing the check withuseExistingLockPoints=falseimplies valid lock points—is indeed the intended design and is verified in the broader context of the code.
- The search results show that
CheckSequenceLocksAtTipconsistently uses the next block height for evaluation.- The code in related test files (e.g., in
src/test/miner_tests.cpp) reinforces the usage of this optimization.No further changes appear necessary based on this review.
738-739:❓ Verification inconclusive
Verify potential race condition with chainlock check
The chainlock conflict check is done before mempool removal, which could potentially lead to a race condition if a new chainlock arrives between the check and removal.
🏁 Script executed:
#!/bin/bash # Search for other chainlock conflict checks to verify consistency rg -A 5 "HasConflictingChainLock|ConflictingISLockIfAny"Length of output: 6668
Potential Race Condition in Chainlock Check Around Mempool Removal
The check using
IsInstantSendWaitingForTx(hash)occurs immediately before callingm_pool.removeConflicts(tx). However, as similar chainlock conflict checks (using functions likeConflictingISLockIfAnyandHasConflictingChainLock) elsewhere in the code indicate, there is a possibility that a new chainlock could be registered in the narrow window between the check and the removal. This ordering may lead to a race condition where a newly arrived chainlock isn’t accounted for during mempool conflict resolution.
- File:
src/validation.cpp, Lines: 738-739- Concern: Verify that the interval between checking for an instant-send wait and the subsequent call to remove mempool conflicts is properly protected against concurrent updates (e.g., via a new chainlock arriving).
Please review the synchronization strategy here and consider reordering the operations or adding an additional check to mitigate any potential race condition.
2713-2716:✅ Verification successful
Verify disk space check is sufficient
The disk space check should verify not just the blocks directory but also the chainstate directory since both are critical for operation.
🏁 Script executed:
#!/bin/bash # Search for other disk space checks rg "CheckDiskSpace|GetDiskSpace"Length of output: 755
Disk Space Checks Adequate – Both Directories Are Covered
After verifying the repository, it’s clear that disk space is validated for both the blocks and chainstate directories. In addition to the check for the blocks directory via:
if (!CheckDiskSpace(gArgs.GetBlocksDirPath())) { return AbortNode(state, "Disk space is too low!", _("Disk space is too low!")); }the chainstate (data) directory is also checked later in the same file:
if (!CheckDiskSpace(gArgs.GetDataDirNet(), 48 * 2 * 2 * CoinsTip().GetCacheSize())) { ... }This confirms that both critical directories are covered. No further changes are required.
src/bench/rpc_mempool.cpp (1)
14-15: LGTM! Parameter rename improves clarity.The change from
sigopstosigops_costin theaddUncheckedmethod call better reflects the parameter's purpose, making the code more self-documenting.test/functional/rpc_decodescript.py (2)
29-30: LGTM! Enhanced test logging improves observability.The added logging statements provide better context for each test case, making it easier to understand test execution flow and debug failures.
Also applies to: 34-35, 38-39, 44-45, 51-52, 62-63, 67-68, 73-74, 80-81, 88-89, 121-122, 138-139, 144-145, 162-163, 172-173, 177-178, 184-185, 186-187, 188-189
70-71: LGTM! Added type assertions strengthen test coverage.The new assertions for script types (
pubkeyhash,scripthash,nulldata) enhance test coverage by validating not just the assembly output but also the script type classification.Also applies to: 85-86, 95-96
src/script/sign.cpp (2)
20-22: LGTM! Improved parameter naming and initialization style.The changes enhance code clarity through:
- More descriptive parameter names (e.g.,
input_idxinstead ofnInIn)- Modern C++ brace initialization
26-30: LGTM! Consistent parameter naming in overloaded constructor.The parameter renaming is consistently applied to the overloaded constructor, maintaining naming conventions across the codebase.
src/wallet/scriptpubkeyman.h (1)
196-196: LGTM! Improved error handling using std::optional.The change from
int64_ttostd::optional<int64_t>forGetOldestKeyPoolTimeis a better approach for representing the absence of a value, eliminating the need for magic numbers or sentinel values.Also applies to: 344-344, 569-569
src/wallet/wallet.h (1)
782-782: LGTM! Improved return type safety using std::optional.The change from
int64_ttostd::optional<int64_t>forGetOldestKeyPoolTimeis a good improvement that makes the absence of a valid keypool time more explicit and type-safe.src/wallet/scriptpubkeyman.cpp (1)
2109-2113: LGTM! Better handling of inapplicable functionality.The changes improve the implementation by:
- Using
std::nulloptinstead of a magic number to indicate inapplicability- Adding a clear comment explaining why this method is not relevant for descriptor wallets
src/wallet/wallet.cpp (2)
2879-2891: LGTM! Improved error handling using std::optional.The change from
int64_ttostd::optional<int64_t>forGetOldestKeyPoolTimeenhances error handling by properly indicating when no key pool time is available (when there are no script public key managers).
2882-2890: LGTM! Clean and efficient implementation.The implementation:
- Correctly handles the case where no script public key managers exist by returning std::nullopt
- Uses std::optional and std::min effectively to find the minimum key pool time across all managers
src/wallet/rpcwallet.cpp (1)
2479-2482: LGTM! Improved null safety using std::optionalThe change from direct assignment to using
std::optionalfor keypool oldest time improves safety by properly handling cases where no value exists. Thehas_value()check ensures we only access the value when it's present.contrib/auto_gdb/dash_dbg.sh (1)
8-9: Enhanced Variable Handling with Quotes.
The updated assignment ofdash_pidusingdash_pid="$(<~/.dashcore/testnet3/dashd.pid)"and the subsequent usage in thegdbcommand now correctly protects against word splitting or unintended behavior if the PID file output contains spaces or special characters.ci/dash/test_unittests.sh (3)
19-20: Proper Quoting for Environment Variables.
The exports forBOOST_TEST_RANDOMandLD_LIBRARY_PATHnow include quotes, ensuring that any potential whitespace or special characters in these variable values are handled safely.
25-25: Robust Directory Change with Quoted Path.
Using quotes in thecd "build-ci/dashcore-$BUILD_TARGET"command guarantees that the directory path is correctly interpreted even if$BUILD_TARGETcontains spaces or unexpected characters.
37-37: Safe Expansion in the make Command.
Enclosing$MAKEJOBSin quotes within themake "$MAKEJOBS" check VERBOSE=1command helps prevent issues due to word splitting, ensuring the passed parameter is treated as a single argument.ci/dash/test_integrationtests.sh (2)
25-28: Consistent Quoting for Improved Robustness.
By quoting directory paths and variable expansions (e.g., in the call to./test/get_previous_releases.pyand incd "build-ci/dashcore-$BUILD_TARGET"), the script minimizes risks associated with spaces or special characters in variable values or file paths.
51-69: Proper Log Collection with Quoted Variables.
The subsequent utilization of quotes (e.g., in the assignmentBASEDIR="$(ls testdatadirs)"and directory operations) ensures that file and directory names with unexpected characters are handled correctly.ci/dash/build_src.sh (6)
17-17: Improved Command Argument Quoting.
Passing"$COMMIT_RANGE"totest/lint/commit-script-check.shensures that any spaces or special characters in the commit range are handled correctly.
31-31: Secured ccache Parameter Passing.
Using quotes around$CCACHE_SIZEin the ccache command (ccache --max-size="$CCACHE_SIZE") helps avoid potential word splitting issues, ensuring the parameter is correctly interpreted.
49-49: Ensuring Version Parameter Safety.
Quoting theVERSION="$BUILD_TARGET"argument in the make command for creating the distribution directory ensures that the version string is interpreted as a single argument, which is especially important if it contains spaces.
51-51: Secure Directory Change with Quoted Target.
Changing to the build directory usingcd "dashcore-$BUILD_TARGET"ensures robust operation even if the build target string adds spaces or other special characters.
59-59: Safe Execution of Build Commands.
The invocation of the make command with quoted variables such as${MAKEJOBS}and${GOAL}reinforces safe handling of these parameters. This approach helps prevent subtle bugs that could arise from unintended word splitting.
66-66: Proper Quoting for Script Invocation.
Quoting"${BASE_ROOT_DIR}/ci/test/wrap-valgrind.sh"ensures that the script path is correctly handled, even if the base directory path contains spaces or special characters.test/lint/lint-cppcheck-dash.sh (2)
90-93: Securing Directory Handling with Quotes.
The modifications that check for the existence of"$CPPCHECK_DIR"and create it withmkdir -p "$CPPCHECK_DIR"enhance reliability by correctly handling directory names with spaces or special characters.
95-98: Robust Option Passing in cppcheck Command.
Encapsulating$CPPCHECK_DIRin quotes within the--cppcheck-build-diroption of the cppcheck command ensures that the build directory is passed as a single, correctly interpreted argument..cirrus.yml (1)
103-112: TSan Task Block – Configuration VerificationThe TSan task definition (lines 103–112) is clear and correct. The container image, resource assignments, and environment variables are appropriately specified.
🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 104-104: too many spaces before colon
(colons)
knst
force-pushed
the
bp-v23-p9
branch
2 times, most recently
from
26c19f6 to
d0e8dfc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
.cirrus.yml (1)
129-139: 🛠️ Refactor suggestion
⚠️ Potential issueFuzzer Task Configuration: Updated resource settings and naming.
- The task name
[fuzzer,address,undefined,integer, no depends] [jammy]clearly reflects its purpose.- The specified CPU (4 cores) and memory (16G) settings are appropriate for this job.
- Critical Issue: Static analysis reports a duplicate key error (at line 129) for
task:. To ensure YAML validity, please consider consolidating these tasks into a list (e.g., under atasks:key) rather than using multipletask:entries at the same level.- Additionally, check for any extra spaces before colons as noted by YAMLlint.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 129-129: duplication of key "task" in mapping
(key-duplicates)
[warning] 131-131: too many spaces before colon
(colons)
🧹 Nitpick comments (1)
.cirrus.yml (1)
113-120: MSan Task Configuration: Validate formatting and consistency.
- The new task name
[MSan, depends] [jammy]improves clarity.- Please verify the indentation and spacing; static analysis noted extra spaces before the colon on line 115.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 113-113: duplication of key "task" in mapping
(key-duplicates)
[warning] 115-115: too many spaces before colon
(colons)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
.cirrus.yml(2 hunks)ci/dash/build_depends.sh(1 hunks)ci/dash/build_src.sh(3 hunks)ci/dash/test_integrationtests.sh(2 hunks)ci/dash/test_unittests.sh(2 hunks)ci/test/04_install.sh(1 hunks)contrib/auto_gdb/dash_dbg.sh(1 hunks)contrib/containers/deploy/docker-entrypoint.sh(1 hunks)src/bench/rpc_mempool.cpp(1 hunks)src/chain.cpp(0 hunks)src/net_processing.cpp(1 hunks)src/psbt.cpp(1 hunks)src/script/sign.cpp(1 hunks)src/script/sign.h(1 hunks)src/test/txpackage_tests.cpp(1 hunks)src/validation.cpp(1 hunks)src/wallet/rpcwallet.cpp(1 hunks)src/wallet/scriptpubkeyman.cpp(2 hunks)src/wallet/scriptpubkeyman.h(3 hunks)src/wallet/wallet.cpp(1 hunks)src/wallet/wallet.h(1 hunks)test/functional/rpc_decodescript.py(6 hunks)test/lint/lint-cppcheck-dash.sh(2 hunks)test/lint/lint-shell.py(0 hunks)
💤 Files with no reviewable changes (2)
- test/lint/lint-shell.py
- src/chain.cpp
🚧 Files skipped from review as they are similar to previous changes (19)
- src/bench/rpc_mempool.cpp
- ci/dash/build_src.sh
- contrib/containers/deploy/docker-entrypoint.sh
- src/psbt.cpp
- ci/dash/test_unittests.sh
- src/test/txpackage_tests.cpp
- ci/test/04_install.sh
- contrib/auto_gdb/dash_dbg.sh
- ci/dash/build_depends.sh
- test/lint/lint-cppcheck-dash.sh
- ci/dash/test_integrationtests.sh
- test/functional/rpc_decodescript.py
- src/wallet/wallet.h
- src/validation.cpp
- src/wallet/scriptpubkeyman.h
- src/wallet/scriptpubkeyman.cpp
- src/script/sign.cpp
- src/script/sign.h
- src/net_processing.cpp
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.cirrus.yml
[warning] 115-115: too many spaces before colon
(colons)
[error] 121-121: duplication of key "task" in mapping
(key-duplicates)
[warning] 123-123: too many spaces before colon
(colons)
[error] 129-129: duplication of key "task" in mapping
(key-duplicates)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build container / Build container
🔇 Additional comments (3)
src/wallet/wallet.cpp (1)
2879-2891: LGTM! The changes improve error handling by using std::optional.The modification to return
std::optional<int64_t>instead ofint64_tis a good improvement as it:
- Explicitly handles the case when no script public key managers are available by returning
std::nullopt- Uses modern C++ features appropriately
- Maintains type safety by avoiding magic numbers or sentinel values
src/wallet/rpcwallet.cpp (1)
2479-2482: LGTM! Improved handling of keypool oldest time.The change from direct integer assignment to using
std::optionalimproves safety by explicitly handling cases where no keypool oldest time exists. The null check before pushing to JSON response prevents potential issues with undefined values..cirrus.yml (1)
103-112: TSan Task Configuration: Updated task name and resource allocation.
- The updated task name
[TSan, depends, no gui] [lunar]is clear and concise.- The increased CPU (6 cores) and memory (24G) settings are well justified to help avoid timeouts during TSan runs.
🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 104-104: too many spaces before colon
(colons)
| task: | ||
| name: '[no depends, sanitizers: address/leak (ASan + LSan) + undefined (UBSan) + integer] [hirsute]' | ||
| name: '[ASan + LSan + UBSan + integer, no depends] [jammy]' | ||
| << : *GLOBAL_TASK_TEMPLATE | ||
| container: | ||
| image: ubuntu:hirsute | ||
| image: ubuntu:jammy | ||
| env: | ||
| FILE_ENV: "./ci/test/00_setup_env_native_asan.sh" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
ASan Task Configuration: Check YAML structure and formatting.
- The task name
[ASan + LSan + UBSan + integer, no depends] [jammy]is descriptive and follows the naming convention. - Critical Issue: Static analysis flagged a duplicate key error (reported at line 121) for
task:; using multiple identical keys in a YAML mapping can lead to unexpected behavior. Consider restructuring these task definitions into a YAML sequence (for example, under a unifiedtasks:key). - Also, please review the spacing on line 123 as indicated by YAMLlint.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 121-121: duplication of key "task" in mapping
(key-duplicates)
[warning] 123-123: too many spaces before colon
(colons)
|
This pull request has conflicts, please rebase. |
34094af build: consistently quote AC_CHECK_LIB() arguments (fanquake) efd4fe1 build: consistently quote AC_MSG_* arguments (fanquake) c397326 build: consistently quote AC_CHECK_PROG() arguments (fanquake) 80762df build: consistently quote arguments in AC_ARG_VAR() (fanquake) e6749a4 build: consistently quote arguments in AM_CONDITIONAL() (fanquake) cdb47e1 build: consistently quote AC_DEFINE() arguments (fanquake) a17a3f9 build: consistently quote AC_MSG_CHECKING() arguments (fanquake) 50d99f2 build: consistently quote AC_PATH_TOOL arguments (fanquake) 05923e7 build: AC_PATH_PROG(S) consistently quote arguments (fanquake) 407f3a4 build: cleanup AX_CHECK_PREPROC_FLAG() usage (fanquake) b4dba0c build: AX_CHECK_PREPROC_FLAG() serial 6 (fanquake) 5ced925 build: cleanup AX_CHECK_COMPILE_FLAG() usage (fanquake) b3dd6c1 build: AX_CHECK_COMPILE_FLAG() serial 6 (fanquake) 5e6bc43 build: cleanup AX_CHECK_LINK_FLAG() usage (fanquake) a874637 build: AX_CHECK_LINK_FLAG serial 6 (fanquake) Pull request description: This is mostly just being consistent with how we do things, and migrating towards a style (we have already been doing so ad-hoc) that is clearer for anyone who cares to read `.m4`. For example: `master`: ```m4 AX_CHECK_COMPILE_FLAG( [-g3], [[DEBUG_CXXFLAGS="$DEBUG_CXXFLAGS -g3"]], [AX_CHECK_COMPILE_FLAG([-g],[[DEBUG_CXXFLAGS="$DEBUG_CXXFLAGS -g"]],,[[$CXXFLAG_WERROR]])], [[$CXXFLAG_WERROR]]) ``` This PR: ```m4 AX_CHECK_COMPILE_FLAG( [-g3], [DEBUG_CXXFLAGS="$DEBUG_CXXFLAGS -g3"], [AX_CHECK_COMPILE_FLAG([-g], [DEBUG_CXXFLAGS="$DEBUG_CXXFLAGS -g"], [], [$CXXFLAG_WERROR])], [$CXXFLAG_WERROR]) ``` Drop unneeded double-quoting (which we use inconsistently), use `[]` for empty arguments, space things out. There should be no functional change, before & after binaries identical. Very boring. Guix build: ```bash bash-5.1# find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum 22097cd621cd88348f827b916f4b4b120b40c3515a3752595347e36d57dc9158 guix-build-34094aff1348/output/aarch64-linux-gnu/SHA256SUMS.part 43f10bb857afa7ea52a8ed9beed36ff0e3ee02dba31100fc04c8e0b2443d94eb guix-build-34094aff1348/output/aarch64-linux-gnu/bitcoin-34094aff1348-aarch64-linux-gnu-debug.tar.gz 9095a8228376094065103f4adc9cdcb8189111fb9536ad88e4f8cecc3df6fc75 guix-build-34094aff1348/output/aarch64-linux-gnu/bitcoin-34094aff1348-aarch64-linux-gnu.tar.gz 9c73179059c6fe1f7643445ec5a530003fc41187aa0a94cb1f5c106097161e5b guix-build-34094aff1348/output/arm-linux-gnueabihf/SHA256SUMS.part 70ddd4dd0a06c7491937084125f690c1d62fa2647c16048fc1a4a9a09d8b10b4 guix-build-34094aff1348/output/arm-linux-gnueabihf/bitcoin-34094aff1348-arm-linux-gnueabihf-debug.tar.gz 0a338fdc9788c33a0d519b6c09fdf6271e3bd68846ee61eef0a06a2df6bab419 guix-build-34094aff1348/output/arm-linux-gnueabihf/bitcoin-34094aff1348-arm-linux-gnueabihf.tar.gz 25eda7fae2984b9dadf47420d1dc09b0224d425144233482602bd9e6d348255c guix-build-34094aff1348/output/dist-archive/bitcoin-34094aff1348.tar.gz d70d84e43ffa2d809063cda868f708539e9114b2d14edb6ddcf05fdf73f3187b guix-build-34094aff1348/output/powerpc64-linux-gnu/SHA256SUMS.part 8d1291e576d2b5f8f7120fe6e6ed4b23415249e22a657a350ccce68ff261e088 guix-build-34094aff1348/output/powerpc64-linux-gnu/bitcoin-34094aff1348-powerpc64-linux-gnu-debug.tar.gz eab448186aee18ac33c39eed4d24501208d10d257fe6e2739adf589b1d4b693a guix-build-34094aff1348/output/powerpc64-linux-gnu/bitcoin-34094aff1348-powerpc64-linux-gnu.tar.gz 4d28617b4d0ddb88c8b20d06ca21314ee40814043f92cabcd9ea3e3d8ee39183 guix-build-34094aff1348/output/powerpc64le-linux-gnu/SHA256SUMS.part dea02168e170e92600012f5806ec8b39209282c2270669f2040682f74bc3f320 guix-build-34094aff1348/output/powerpc64le-linux-gnu/bitcoin-34094aff1348-powerpc64le-linux-gnu-debug.tar.gz c7122e89d3186a183ac08e7f3020654722c98bf8acc8b790bb292b39f5ba8225 guix-build-34094aff1348/output/powerpc64le-linux-gnu/bitcoin-34094aff1348-powerpc64le-linux-gnu.tar.gz 0802a52720d2bec1264dc13f6554a9da347baa3d096242b29bb524f4b121eb10 guix-build-34094aff1348/output/riscv64-linux-gnu/SHA256SUMS.part 4aee9fc41f35d2adb2d0562902dd8584a8413a73c015ddcdcef00586779f63a7 guix-build-34094aff1348/output/riscv64-linux-gnu/bitcoin-34094aff1348-riscv64-linux-gnu-debug.tar.gz 3f0c4d6096ac7e08389e851c2d252632c044a700ce0174473ac4d7f66290e8cc guix-build-34094aff1348/output/riscv64-linux-gnu/bitcoin-34094aff1348-riscv64-linux-gnu.tar.gz c5149c46b9b7081d5715daf3e22fd30ffca23d333f664da1fabc8143ff8bf76c guix-build-34094aff1348/output/x86_64-apple-darwin19/SHA256SUMS.part 81848355751e55a8a60636e3ea2f03ca6abb78736a5431715cd51cebd46bb961 guix-build-34094aff1348/output/x86_64-apple-darwin19/bitcoin-34094aff1348-osx-unsigned.dmg d2f6f689cbedddd865f90dba9ddf21479c71c61b0350fda62804b2f233116a43 guix-build-34094aff1348/output/x86_64-apple-darwin19/bitcoin-34094aff1348-osx-unsigned.tar.gz 99d8fe428fcb67f9975e6b8d9a63d84946215a0a6b8f94967ce96cc3af4b7772 guix-build-34094aff1348/output/x86_64-apple-darwin19/bitcoin-34094aff1348-osx64.tar.gz 3dac13c7556d9a25ff5513bbb2638fe4fa74d8a88304bbdce52364df7832a3ab guix-build-34094aff1348/output/x86_64-linux-gnu/SHA256SUMS.part e4baa7da80fdabbb50953efaaa7b4867c7e575a7a156b728e8e197142df55697 guix-build-34094aff1348/output/x86_64-linux-gnu/bitcoin-34094aff1348-x86_64-linux-gnu-debug.tar.gz f82f5bcc7197c1741b106f62be7b468aadbdf5b3198091582026cd450bf13b3a guix-build-34094aff1348/output/x86_64-linux-gnu/bitcoin-34094aff1348-x86_64-linux-gnu.tar.gz db22b5f48783917f985920ddb26aa170b4d6cc65112406548847a883099505b8 guix-build-34094aff1348/output/x86_64-w64-mingw32/SHA256SUMS.part ead0809193ca1d462553a6f3f233cdbff7a3f8419100d825abfc10835508e485 guix-build-34094aff1348/output/x86_64-w64-mingw32/bitcoin-34094aff1348-win-unsigned.tar.gz e1006b6c114eaf33274144d8e9a20abc0cee01e26a4594063ee615bf09c1b344 guix-build-34094aff1348/output/x86_64-w64-mingw32/bitcoin-34094aff1348-win64-debug.zip 2d3fbc593b58d353a6859e02c0dd096d453cf5f1e3150c2a1c234bdfc97b4f24 guix-build-34094aff1348/output/x86_64-w64-mingw32/bitcoin-34094aff1348-win64-setup-unsigned.exe 25cfa8a3655727572593f100b7a70c1061fa6f3b017014ad7444059611c3ddda guix-build-34094aff1348/output/x86_64-w64-mingw32/bitcoin-34094aff1348-win64.zip ``` ACKs for top commit: hebasto: re-ACK 34094af Tree-SHA512: 7515e85b4dedddf430ddf0bf31f25fca8f73898cf2ba4b6a66b9f21feeaff4c2600fe24efdd2e81822f059827b5b35341b183ea8342fd689248d8c355bf5cb42
0bc1ce0 Fix Arch linux dead link (0xree) Pull request description: - Fixed dead link referencing an invalid link ACKs for top commit: benthecarman: ACK 0bc1ce0 Tree-SHA512: 330dd2f327458b10afb1ea920bf8fef459b8b2ad45cde4a0a754995fe0684e7f1e6dc8043b41c78cd83ff3b59745597873bdcd84f4b9255932f7425c0dd6a93a
fe0ff56 test: Enable SC2046 shellcheck rule (Hennadii Stepanov) 9a1ad7b test: Enable SC2086 shellcheck rule (Hennadii Stepanov) Pull request description: Closes bitcoin#20879. Replaces bitcoin#22695. **Note for reviewers**. Some touched shell scripts are not being run in CI, therefore they require more thorough reviewing: - `contrib/devtools/gen-manpages.sh` - `contrib/macdeploy/detached-sig-apply.sh` - `contrib/windeploy/detached-sig-create.sh` - `src/qt/res/animation/makespinner.sh` ACKs for top commit: laanwj: Code review re-ACK fe0ff56 Tree-SHA512: 73619b9a7bcb6cf0dfc4189a753ef550d40c82a3432bb9d8d8a994310d42594576038daac7e0c2fc004d716976bb1413b9a77848ecf088b25b69ed0773b77e8e
fafa66e ci: Replace soon EOL hirsute with jammy (MarcoFalke) Pull request description: `hirsute` will be EOL in about 1.5 months, at which point the package servers may be shut down. Avoid this by hopping to the next LTS release 22.04 (Jammy Jellyfish). While the release is currently in development, it seems unlikely that anything will break for us. I am doing the hirsute->jammy hop to avoid a hirsute->impish->jammy hop, but if anything does break, we can fall back to that "double hop". ACKs for top commit: fanquake: ACK fafa66e Zero-1729: crACK fafa66e Tree-SHA512: a3626b72519c7f54f260477d04265321af7aefe25cc2a7d653dba77f79caca10db3a6aa4249a934184dcdc99832f6b4c6e50330d7630e58720ab0aba3624ab8a
fa45922 ci: Rework ci task names (MarcoFalke) Pull request description: It is hard to find a specific sanitizer task in less than a second. Fix that by mentioning the sanitizer first in the task name. Less useful information (with or without depends) follows the sanitizer. Follow up to: * bitcoin#20545 * bitcoin#20572 ACKs for top commit: fanquake: ACK fa45922 Tree-SHA512: 87165caff4749cc415cef6ae36cac63fa448cec0a1c92cd6b691a0fbd22902466c8d861b0d922c3331e4bf36dbb4eba86d745ef135f391f7ff81461fe5159e05
…onSignatureCreator fa54a40 doc: Pick better named args for MutableTransactionSignatureCreator (MarcoFalke) Pull request description: Argument names of `nInIn` are not helpful. ACKs for top commit: shaavan: ACK fa54a40 achow101: ACK fa54a40 Tree-SHA512: 53a38588fdee07d7896a66339c1c2c2355638db95a95cad9844b60cd34e935bb726ab64d0c42dc414beb35375e56440f8a9cb3fbf5aec55c1eed066b7acad8c8
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.cirrus.yml (1)
124-131: ASan Task Configuration Update and Formatting Note
The ASan task now adopts the "ubuntu:jammy" image, consistent with other sanitizers. However, static analysis has flagged extra spacing in the merge operator (e.g.,<< : *GLOBAL_TASK_TEMPLATE). For improved readability and to avoid YAML formatting warnings, consider rewriting these lines without the extra space, for example:- << : *GLOBAL_TASK_TEMPLATE + <<: *GLOBAL_TASK_TEMPLATE🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 124-124: duplication of key "task" in mapping
(key-duplicates)
[warning] 126-126: too many spaces before colon
(colons)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
.cirrus.yml(2 hunks)build-aux/m4/ax_check_compile_flag.m4(2 hunks)build-aux/m4/ax_check_link_flag.m4(2 hunks)build-aux/m4/ax_check_preproc_flag.m4(2 hunks)build-aux/m4/bitcoin_find_bdb48.m4(1 hunks)build-aux/m4/bitcoin_qt.m4(6 hunks)ci/dash/build_depends.sh(1 hunks)ci/dash/build_src.sh(3 hunks)ci/dash/test_integrationtests.sh(2 hunks)ci/dash/test_unittests.sh(2 hunks)ci/test/04_install.sh(1 hunks)configure.ac(43 hunks)contrib/auto_gdb/dash_dbg.sh(1 hunks)contrib/containers/deploy/docker-entrypoint.sh(1 hunks)doc/build-unix.md(1 hunks)src/bench/rpc_mempool.cpp(1 hunks)src/chain.cpp(0 hunks)src/net_processing.cpp(1 hunks)src/psbt.cpp(1 hunks)src/script/sign.cpp(1 hunks)src/script/sign.h(1 hunks)src/test/txpackage_tests.cpp(1 hunks)src/validation.cpp(1 hunks)src/wallet/rpcwallet.cpp(1 hunks)src/wallet/scriptpubkeyman.cpp(2 hunks)src/wallet/scriptpubkeyman.h(3 hunks)src/wallet/wallet.cpp(1 hunks)src/wallet/wallet.h(1 hunks)test/functional/rpc_decodescript.py(6 hunks)test/lint/lint-cppcheck-dash.sh(2 hunks)test/lint/lint-shell.py(0 hunks)
💤 Files with no reviewable changes (2)
- src/chain.cpp
- test/lint/lint-shell.py
🚧 Files skipped from review as they are similar to previous changes (26)
- src/psbt.cpp
- ci/dash/build_src.sh
- src/bench/rpc_mempool.cpp
- src/test/txpackage_tests.cpp
- build-aux/m4/ax_check_preproc_flag.m4
- doc/build-unix.md
- ci/dash/test_unittests.sh
- ci/test/04_install.sh
- contrib/auto_gdb/dash_dbg.sh
- test/lint/lint-cppcheck-dash.sh
- ci/dash/build_depends.sh
- test/functional/rpc_decodescript.py
- build-aux/m4/ax_check_link_flag.m4
- src/wallet/wallet.h
- build-aux/m4/bitcoin_find_bdb48.m4
- src/script/sign.cpp
- src/wallet/scriptpubkeyman.cpp
- src/script/sign.h
- contrib/containers/deploy/docker-entrypoint.sh
- ci/dash/test_integrationtests.sh
- src/validation.cpp
- src/wallet/scriptpubkeyman.h
- build-aux/m4/ax_check_compile_flag.m4
- build-aux/m4/bitcoin_qt.m4
- src/wallet/rpcwallet.cpp
- src/net_processing.cpp
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.cirrus.yml
[warning] 118-118: too many spaces before colon
(colons)
[error] 124-124: duplication of key "task" in mapping
(key-duplicates)
[warning] 126-126: too many spaces before colon
(colons)
[error] 132-132: duplication of key "task" in mapping
(key-duplicates)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_sqlite-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: arm-linux-build / Build source
🔇 Additional comments (40)
.cirrus.yml (3)
105-115: TSan Task Configuration Update
The new TSan task is now configured with the "ubuntu:lunar" container image and increased CPU (6) and memory (24G) settings to help avoid timeouts. Please ensure that these resource allocations are available and consistent with your CI infrastructure expectations.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 105-105: duplication of key "task" in mapping
(key-duplicates)
[warning] 107-107: too many spaces before colon
(colons)
116-123: MSan Task Configuration Update
This MSan task block uses the "ubuntu:jammy" image via the global task template and correctly sets theFILE_ENVvariable. Verify that the environment script path (./ci/test/00_setup_env_native_msan.sh) exists and runs as expected.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 116-116: duplication of key "task" in mapping
(key-duplicates)
[warning] 118-118: too many spaces before colon
(colons)
132-142: Fuzzer Task Configuration and Duplicate Key Warning
The fuzzer task is appropriately set up with increased CPU and memory resources and specifies the correct environment script. Note that static analysis is reporting duplicate key errors fortask:(at lines 124 and 132 according to the hints). Although this pattern is common in some Cirrus CI configurations, YAML standards do not allow duplicate keys. If the CI parser permits this construct, you may choose to suppress the warning; otherwise, consider restructuring these tasks into a sequence (e.g. under a key liketasks:) to ensure consistency and avoid potential unexpected behavior.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 132-132: duplication of key "task" in mapping
(key-duplicates)
[warning] 134-134: too many spaces before colon
(colons)
src/wallet/wallet.cpp (1)
2878-2890: Well-designed API improvement using std::optionalThe change from returning
int64_ttostd::optional<int64_t>properly handles the case when no script public key managers are available by returningstd::nullopt. This is a more expressive and type-safe approach than returning a sentinel value.configure.ac (36)
33-33: Syntax standardization in autoconf macros for better consistency.The changes wrap various macro arguments in square brackets (e.g.,
AC_ARG_VAR,AC_PATH_TOOL,AC_PATH_PROG,AC_DEFINE, etc.) throughout the file. This is a standard autoconf best practice that helps prevent issues with whitespace and special characters in arguments.This standardizes the autoconf syntax and doesn't change any functional behavior, making the file conform to newer autoconf best practices. The changes are part of a larger effort to backport improvements from Bitcoin Core version 23.
Also applies to: 98-114, 244-244, 548-548, 815-1823
377-381: Consistent use of square brackets in debug flag checks.These
AX_CHECK_PREPROC_FLAGandAX_CHECK_COMPILE_FLAGcalls have been updated to use consistent square bracket syntax.
426-428: Consistent square bracket syntax in sanitizer flag checks.The sanitizer flag handling code now uses consistent square bracket syntax in all flag checks.
Also applies to: 436-438
471-488: Standardized warning flag check syntax.All compiler warning flag checks have been updated to use a consistent syntax with square brackets.
553-570: Consistent message checking syntax for SSE4.2 intrinsics.Square brackets are now consistently used in the message checking/result reporting for SSE4.2 intrinsic capability tests.
575-585: Standardized syntax in SSE4.1 intrinsics checks.The message checking and result reporting for SSE4.1 capabilities now follows the same square bracket syntax pattern.
590-600: Consistent AVX2 intrinsics check formatting.The message checking and result reporting for AVX2 intrinsics now use square brackets consistently.
605-617: Standardized x86 SHA-NI intrinsics check syntax.Message checking and result reporting for x86 SHA-NI intrinsics now use square brackets consistently.
626-640: Consistent ARM CRC32 intrinsics check syntax.The syntax for message checking and result reporting for ARM CRC32 intrinsics now follows the same square bracket pattern.
896-898: Standardized lcov flag checking syntax.The flag checks for lcov coverage testing now use square brackets consistently.
1047-1052: Consistent mallopt check syntax.The mallopt M_ARENA_MAX capability check now uses square brackets consistently in message checking and result reporting.
1055-1060: Standardized getmemoryinfo check syntax.The malloc_info function check now uses square brackets consistently in message checking and result reporting.
1063-1076: Consistent posix_fallocate check syntax.The posix_fallocate function availability check now uses square brackets consistently in message checking and result reporting.
1084-1089: Standardized visibility attribute check syntax.The default visibility attribute check now uses square brackets consistently in message checking and result reporting.
1101-1105: Consistent dllexport attribute check syntax.The dllexport attribute check now uses square brackets consistently in message checking and result reporting.
1148-1160: Standardized gmtime_r check syntax.The syntax for checking gmtime_r and gmtime_s availability now uses square brackets consistently.
1163-1170: Consistent Linux getrandom syscall check syntax.The Linux getrandom syscall check now uses square brackets consistently in message checking and result reporting.
1172-1178: Standardized getentropy check syntax.The getentropy check now uses square brackets consistently in message checking and result reporting.
1180-1201: Consistent sysctl check syntax.The sysctl and KERN_ARND checks now use square brackets consistently in message checking and result reporting.
1203-1209: Standardized type check syntax.The check for whether char equals int8_t now uses square brackets consistently in message checking and result reporting.
1220-1226: Consistent fdatasync check syntax.The fdatasync availability check now uses square brackets consistently in message checking and result reporting.
1228-1233: Standardized F_FULLFSYNC check syntax.The F_FULLFSYNC check now uses square brackets consistently in message checking and result reporting.
1235-1241: Consistent O_CLOEXEC check syntax.The O_CLOEXEC flag check now uses square brackets consistently in message checking and result reporting.
1244-1252: Standardized __builtin_prefetch check syntax.The __builtin_prefetch check now uses square brackets consistently in message checking and result reporting.
1254-1262: Consistent _mm_prefetch check syntax.The _mm_prefetch check now uses square brackets consistently in message checking and result reporting.
1264-1272: Standardized getauxval check syntax.The getauxval check now uses square brackets consistently in message checking and result reporting.
1275-1283: Consistent std::system check syntax.The std::system check now uses square brackets consistently in message checking and result reporting.
1285-1293: Standardized ::_wsystem check syntax.The ::_wsystem check now uses square brackets consistently in message checking and result reporting.
1637-1654: Consistent ccache check syntax.The ccache checking and enabling logic now uses square brackets consistently in message checking and result reporting.
1658-1665: Standardized wallet enabling check syntax.The wallet enabling check now uses square brackets consistently in message checking and result reporting.
1668-1685: Consistent UPnP support check syntax.The UPnP support check now uses square brackets consistently in message checking and result reporting.
1714-1717: Standardized D-Bus support check syntax.The D-Bus support check now uses square brackets consistently in message checking and result reporting.
1727-1727: Consistent QR code support syntax.The QR code support define now uses square brackets consistently.
1734-1735: Standardized xgettext warning message syntax.The xgettext warning now uses square brackets consistently.
1791-1811: Consistent conditional flag syntax.The AM_CONDITIONAL calls now use square brackets consistently.
1815-1823: Standardized version and copyright defines.The client version and copyright defines now use square brackets consistently.
knst
changed the title
| /* input_height */ 101, /* input_signing_key */ parent_key, | ||
| /* output_destination */ child_locking_script, | ||
| /* output_amount */ CAmount(48 * COIN), /* submit */ false); | ||
| auto mtx_child = CreateValidMempoolTransaction(/*input_transaction=*/ tx_parent, /*input_vout=*/0, |
There was a problem hiding this comment.
22981: ci complains about more cases
test/txpackage_tests.cpp:225:97: error: argument name 'vout' in comment does not match parameter name 'input_vout' [bugprone-argument-comment,-warnings-as-errors]
225 | auto mtx = CreateValidMempoolTransaction(/*input_transaction=*/m_coinbase_txns[i + 25], /*vout=*/0,
| ^
test/txpackage_tests.cpp:241:95: error: argument name 'vout' in comment does not match parameter name 'input_vout' [bugprone-argument-comment,-warnings-as-errors]
241 | auto mtx_parent = CreateValidMempoolTransaction(/*input_transaction=*/m_coinbase_txns[0], /*vout=*/0,
| ^
test/txpackage_tests.cpp:252:85: error: argument name 'vout' in comment does not match parameter name 'input_vout' [bugprone-argument-comment,-warnings-as-errors]
252 | auto mtx_child = CreateValidMempoolTransaction(/*input_transaction=*/tx_parent, /*vout=*/0,
| ^
test/txpackage_tests.cpp:263:89: error: argument name 'vout' in comment does not match parameter name 'input_vout' [bugprone-argument-comment,-warnings-as-errors]
263 | auto mtx_grandchild = CreateValidMempoolTransaction(/*input_transaction=*/tx_child, /*vout=*/0,
| ^
… output script BACKPORT NOTE: Skipped taproot related code 83f6c0f test: add decodescript RPC test for P2TR output type (Sebastian Falbesoner) 099c695 test: check for decodescript RPC 'type' results (Sebastian Falbesoner) 0d43525 test: add logging to rpc_decodescript.py (Sebastian Falbesoner) Pull request description: This PR adds a functional sub-test for calling `decodescript` with a P2TR / segwit v1 output script (`OP_1 <32-bytes push>`), expecting to return "witness_v1_taproot" as `type` result. In the first two commits, the test `rpc_decodescript.py` is also improved by adding logging (plus getting rid of the enumerations) and also adding missing checks `type` result checks for all other output script types. ACKs for top commit: MarcoFalke: ACK 83f6c0f Tree-SHA512: 5fbfa693f711f55022edbc26109b076610ba248bef5282822656f5a2289636a5da6e2c1a4d8ab16a599af5701dafb3452e8be653d0e5f09e59ed87b8144d46ef
…lank descriptor wallets ee03c78 wallet: Make GetOldestKeyPoolTime return nullopt for blank wallets (Hennadii Stepanov) 3e4f069 wallet, refactor: Make GetOldestKeyPoolTime return type std::optional (Hennadii Stepanov) Pull request description: The "keypoololdest" field in the `getwalletinfo` RPC response should be used for legacy wallets only. Th current implementation (04437ee) assumes that `CWallet::GetOldestKeyPoolTime()` always return `0` for descriptor wallets. This assumption is wrong for _blank_ descriptor wallets, when `m_spk_managers` is empty. As a result: ``` $ src/bitcoin-cli -signet -rpcwallet=211024-d-DPK getwalletinfo { "walletname": "211024-d-DPK", "walletversion": 169900, "format": "sqlite", "balance": 0.00000000, "unconfirmed_balance": 0.00000000, "immature_balance": 0.00000000, "txcount": 0, "keypoololdest": 9223372036854775807, "keypoolsize": 0, "keypoolsize_hd_internal": 0, "paytxfee": 0.00000000, "private_keys_enabled": false, "avoid_reuse": false, "scanning": false, "descriptors": true } ``` This PR fixes this issue with direct checking of the `WALLET_FLAG_DESCRIPTORS` flag. ACKs for top commit: lsilva01: re-ACK ee03c78 stratospher: ACK ee03c78. meshcollider: Code review ACK ee03c78 Tree-SHA512: 9852f9f8ed5c08c07507274d7714f039bbfda66da6df65cf98f67bf11a600167d0f7f872680c95775399477f4df9ba9fce80ec0cbe0adb7f2bb33c3bd65b15df
fa5a886 doc: Tidy up nMinDiskSpace comment (MarcoFalke) Pull request description: nMinDiskSpace was removed in commit 04cca33 Also, remove incorrect doxygen comment. See https://doxygen.bitcoincore.org/class_c_chain.html#aeb563751f7362d4308c7c2cb35b834a5 ACKs for top commit: theStack: ACK fa5a886 Tree-SHA512: d57a6a0f0a66615bebb3cca19dc831cca38be0f18a580bb88e774384c55ccc545279b6d115b86fda70528a86630065393fb692fc2997ef87f97eec2d162808bb
…ed arguments (tests only) fa00447 scripted-diff: Use clang-tidy syntax for C++ named arguments (MarcoFalke) fae13c3 doc: Use clang-tidy comments in crypto_tests (MarcoFalke) Pull request description: Incorrect named args are source of bugs, like bitcoin#22979. To allow them being checked by `clang-tidy`, use a format it can understand. ACKs for top commit: shaavan: ACK fa00447 rajarshimaitra: ACK fa00447 jonatack: ACK fa00447 fanquake: ACK fa00447
fac4947 doc: Fix incorrect C++ named args (MarcoFalke) Pull request description: Incorrect named args are source of bugs, like bitcoin#22979. Fix that by correcting them and adjust the format, so that clang-tidy can check it. ACKs for top commit: fanquake: ACK fac4947 - `run-clang-tidy` works for me now. Tree-SHA512: 2694e17a1586394baf30bbc479a913e4bad361221e8470b8739caf30aacea736befc73820f3fe56f6207d9f5d969323278d43a647f58c3497e8e44cad79f8934
What was done?
Regular backports from Bitcoin Core v23
How Has This Been Tested?
Run unit / functional tests
Breaking Changes
N/A
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.