backport: bitcoin#16795, #23706, #24111, #24113, #24131, #24146, #24155, #24179, #24212, #24250, #24806 #6621
Conversation
knst
changed the title
WalkthroughThe changes span configuration, source code, documentation, and tests. In configuration files, the Docker image for a CentOS task was updated from a standard CentOS 9 image to a stream variant, and Docker-specific function calls were renamed to use a generic CI naming scheme. Documentation updates include modified instructions for Windows build dependencies and an updated REST interface schema that now includes a “desc” field. In the source code, type casts were added for clarity in chain-related functions, and error messages were altered in the initialization process. The RPC modules have been revised to adjust response structures, introduce “desc” fields in various JSON outputs, and alter error handling. Additionally, changes in fuzz testing introduce randomness in parameter selection, and JSON test files have been updated to include additional descriptive metadata. Minor removals include outdated compiler alternatives in a Dockerfile and several sanitizer suppression entries. Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/init.cpp (1)
1447-1448: Enhance Error Message Clarity for-maxuploadtargetParsing
The check for whetherParseByteUnitssucceeded is correct; however, the error message might be a bit terse for end users. Consider expanding the message to mention the expected format (e.g. a numeric value with an optional unit suffix, where the unit "M" indicates megabytes). This additional context could help users diagnose and correct misconfigurations more easily.doc/build-windows.md (1)
42-52: Documentation Formatting and Clarity in Cross-Compilation Instructions.
The updated instructions now clearly differentiate between modern and older systems for installing the mingw-w64 toolchain. However, static analysis hints suggest that the unordered list formatting could be standardized (e.g., using asterisks instead of dashes) and that using indented code blocks might better adhere to established markdown style guidelines (MD004, MD046). Consider revising these sections for consistency and improved readability.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
42-42: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
42-42: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
44-44: Code block style
Expected: indented; Actual: fenced(MD046, code-block-style)
48-48: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
48-48: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
50-50: Code block style
Expected: indented; Actual: fenced(MD046, code-block-style)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
.cirrus.yml(1 hunks)ci/test/04_install.sh(2 hunks)ci/test/05_before_script.sh(1 hunks)contrib/containers/ci/ci.Dockerfile(0 hunks)doc/REST-interface.md(1 hunks)doc/build-windows.md(1 hunks)share/setup.nsi.in(1 hunks)src/chain.cpp(1 hunks)src/chain.h(1 hunks)src/core_write.cpp(2 hunks)src/init.cpp(1 hunks)src/rpc/blockchain.cpp(3 hunks)src/rpc/rawtransaction.cpp(3 hunks)src/rpc/util.cpp(2 hunks)src/rpc/util.h(3 hunks)src/test/fuzz/script.cpp(1 hunks)test/functional/data/rpc_decodescript.json(2 hunks)test/functional/rpc_getblockfrompeer.py(1 hunks)test/functional/rpc_rawtransaction.py(1 hunks)test/sanitizer_suppressions/ubsan(0 hunks)test/util/data/tt-delin1-out.json(2 hunks)test/util/data/tt-delout1-out.json(1 hunks)test/util/data/tt-locktime317000-out.json(2 hunks)test/util/data/txcreate1.json(2 hunks)test/util/data/txcreate2.json(1 hunks)test/util/data/txcreatedata1.json(2 hunks)test/util/data/txcreatedata2.json(2 hunks)test/util/data/txcreatedata_seq0.json(1 hunks)test/util/data/txcreatedata_seq1.json(1 hunks)test/util/data/txcreatemultisig1.json(1 hunks)test/util/data/txcreatemultisig2.json(1 hunks)test/util/data/txcreateoutpubkey1.json(1 hunks)test/util/data/txcreatescript1.json(1 hunks)test/util/data/txcreatescript2.json(1 hunks)test/util/data/txcreatesignv1.json(1 hunks)
💤 Files with no reviewable changes (2)
- contrib/containers/ci/ci.Dockerfile
- test/sanitizer_suppressions/ubsan
🧰 Additional context used
🧬 Code Definitions (2)
ci/test/05_before_script.sh (1)
ci/test/04_install.sh (1)
CI_EXEC(53-55)
src/test/fuzz/script.cpp (2)
src/core_write.cpp (8)
FormatScript(45-79)FormatScript(45-45)ScriptToAsmStr(104-147)ScriptToAsmStr(104-104)ScriptPubKeyToUniv(161-176)ScriptPubKeyToUniv(161-161)ScriptToUniv(156-159)ScriptToUniv(156-156)src/core_io.h (4)
FormatScript(54-54)ScriptToAsmStr(36-36)ScriptPubKeyToUniv(57-57)ScriptToUniv(58-58)
🪛 markdownlint-cli2 (0.17.2)
doc/build-windows.md
42-42: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
42-42: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
44-44: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
48-48: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
48-48: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
50-50: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (47)
test/util/data/txcreate2.json (1)
16-16: Add "desc" Field for Enhanced Script Metadata
The new"desc": "raw()#58lrscpx"field enriches the metadata of thescriptPubKeyobject. This change aligns with similar updates in other test JSON files and improves clarity regarding the script details.test/util/data/txcreatescript1.json (1)
16-16: Introduce Descriptor for Script Details
Adding"desc": "raw(75)#ppey0zqj"provides clearer context for the script, which is consistent with the broader metadata enhancements across test fixtures.test/util/data/txcreatescript2.json (1)
16-16: Provide Descriptive Metadata for scripthash
The addition of"desc": "addr(7co3R3WSW8mHKMkFK2FrzQY2fLCBWsg56D)#tjdnllwj"gives a human-readable explanation for the output script. Please ensure that the format remains consistent with similar changes in other JSON test cases.test/util/data/txcreateoutpubkey1.json (1)
16-16: Add Descriptor for Public Key Output
The new"desc": "pk(02a5613bd857b7048924264d1e70e08fb2a7e6527d32b7ab1bb993ac59964ff397)#rk5v7uqw"field clarifies the public key usage in the output script. This descriptor is in line with the modifications made to other transaction output test files.test/util/data/txcreatesignv1.json (1)
25-25: Enhance Script Descriptor with Address Information
The newly introduced"desc": "addr(XijDvbYpPmznwgpWD3DkdYNfGmRP2KoVSk)#m8zt5egk"field effectively supplements thescriptPubKeyobject with additional context on the output. This change is consistent with the metadata improvements applied in other related JSON files.test/util/data/txcreatedata_seq0.json (1)
25-25: Added descriptive metadata to transaction outputThe addition of the
descfield enhances the transaction output metadata by providing an address descriptor. This is part of a broader set of changes across multiple test data files, bringing the format in line with the backported Bitcoin Core implementation.src/chain.cpp (1)
156-156: Explicit type casting improves code clarityAdding the explicit cast to
int64_tmakes the code more type-safe and prevents potential implicit conversion issues. This is a good practice and aligns with the function's declared return type.test/util/data/txcreatedata_seq1.json (1)
34-34: Added descriptive metadata to transaction outputThe addition of the
descfield enhances the transaction output metadata with an address descriptor. This change is consistent with similar additions in other test data files as part of the Bitcoin Core backport.test/util/data/txcreatemultisig2.json (1)
16-16: Added descriptive metadata to transaction outputThe addition of the
descfield provides additional context for the script hash address. This change follows the pattern implemented across other test data files in this PR, enhancing the metadata available for transaction outputs.share/setup.nsi.in (1)
56-56: Enhanced security for the installerThe change from
CRCCheck ontoCRCCheck forceimproves security by requiring the installer to abort if a CRC check fails, rather than just displaying a warning. This ensures that only files with verified integrity can be installed.test/util/data/txcreatemultisig1.json (1)
16-16: Added descriptive metadata for multisig outputThe new
descfield provides a structured descriptor for the multisig configuration, making it clearer that this requires 2-of-3 signatures from the specified public keys. This is a good addition that improves the JSON structure's informativeness without changing functionality..cirrus.yml (1)
91-91: Updated Docker image to CentOS Stream 9The change from
centos:9toquay.io/centos/centos:stream9updates the build environment to use the CentOS Stream variant, which is the upstream development platform for RHEL. This is appropriate as the standard CentOS Linux 9 was discontinued in favor of CentOS Stream.test/util/data/tt-delout1-out.json (1)
205-205: Added address descriptor to transaction outputThe new
descfield provides a structured address descriptor for the transaction output, enhancing the metadata available in the JSON. This aligns with similar changes in other transaction test data files and improves consistency across the codebase.doc/REST-interface.md (1)
112-117: Enhancement: Addition of the "desc" field in REST output.
The new"desc": "addr(mi7as51dvLJsizWnTMurtRmrP8hG2m1XvD)#gj9tznmy"field within thescriptPubKeyobject properly enriches the UTXO response without affecting existing fields. This change is consistent with similar enhancements elsewhere in the codebase.test/functional/data/rpc_decodescript.json (2)
4-9: Data Enrichment: "desc" field for scripthash Output.
The addition of"desc": "addr(2NF2b3KS8xXb9XHvbRMXdZh8s5g92rUZHtp)#ywfcpmh9"in the scripthash object significantly enhances the metadata and aligns with similar enhancements across RPC outputs.
12-18: Data Enrichment: "desc" field for nulldata Output.
Adding"desc": "raw(6a00)#ncfmkl43"to the nulldata object provides additional descriptive context, ensuring consistency across test data files.src/chain.h (1)
453-457: Robustness Improvement: Explicit Casting in Chain Height Calculation.
Changing the return expression toreturn int(vChain.size()) - 1;ensures that the unsigned size type is explicitly cast to anintbefore subtraction. This precaution helps prevent potential arithmetic issues related to type mismatches while determining the chain’s height.test/util/data/txcreate1.json (2)
41-45: Test Data Enhancement: New "desc" field in First Transaction Output.
The inclusion of"desc": "addr(Xdak8YsJz8tm1iHFmycfTyKeUvHgfbdpyw)#nr4se9ls"in the first output'sscriptPubKeyobject enriches the test metadata, aligning with the broader initiative to add descriptive information to transaction outputs.
53-57: Test Data Enhancement: New "desc" field in Second Transaction Output.
Similarly, the addition of"desc": "addr(XxppMBDQ6SiJrKcBrAy4WkkNdrxDBfzFdZ)#ack2ftlg"to the second output’sscriptPubKeyobject is consistent with other changes, thereby improving clarity and test coverage of descriptive metadata.src/core_write.cpp (2)
11-11: Appropriate header inclusion for descriptor support.The addition of the
<script/descriptor.h>include is necessary to support the new descriptor functionality added in line 166.
166-166: Good enhancement: Added output descriptor information.Adding the descriptor field to script outputs provides valuable context about how the script can be spent. This is consistent with similar improvements in Bitcoin Core and enhances the RPC interface by providing more structured and standardized information about script outputs.
The use of
InferDescriptorwithDUMMY_SIGNING_PROVIDERis appropriate since we're only generating a string representation and not actually signing anything.test/util/data/tt-delin1-out.json (2)
196-196: Test data updated to match new descriptor output.The test data has been correctly updated to include the new
descfield that was added to theScriptPubKeyToUnivfunction output. This ensures that tests will pass with the updated code.
208-208: Test data updated for second output descriptor.Properly added descriptor field for the second output to maintain consistency with the code changes in
ScriptPubKeyToUniv.src/rpc/util.h (2)
280-280: Good refactoring: Extracted inner document validation to a method.The code now calls
CheckInnerDoc()instead of having inline checks in the constructors. This is a good refactoring that improves code maintainability by centralizing the validation logic in one place.Also applies to: 304-304
322-324: Well-structured declaration of the new validation method.Added a private method
CheckInnerDoc() constwhich encapsulates the validation logic for inner result documents. This separates concerns and makes the constructor logic cleaner. The method is correctly marked asconstsince it doesn't modify the object's state.test/util/data/tt-locktime317000-out.json (2)
205-205: Enhanced transaction output metadata with descriptor fieldThis adds a new
descfield providing a standardized descriptor for the address, which enhances the metadata available for transaction outputs. This change is part of a broader effort to add descriptors throughout the codebase.
217-217: Added descriptor for second output addressSimilar to the first output, this adds a descriptor field for the second address, maintaining consistency in the transaction output representation.
src/rpc/util.cpp (2)
795-798: Improved handling of empty JSON objectsThe change properly handles empty objects by returning a
{}representation with a descriptive message rather than potentially causing an assertion failure. This makes the API more robust when dealing with empty structures.
853-862: Added validation method for inner document structuresThis new
CheckInnerDoc()method implements validation for inner document structures based on their types. It appropriately allowsType::OBJto have empty inner structures while enforcing proper structure for other types. This improves the overall robustness of the RPC response handling.ci/test/05_before_script.sh (1)
13-13: Refactored execution context from Docker-specific to generic CIThe changes replace
DOCKER_EXECwithCI_EXECacross multiple commands, making the CI scripts more generic and less tightly coupled to Docker. This aligns with the function definition inci/test/04_install.shwhich showsCI_EXECas a wrapper function using$DOCKER_CI_CMD_PREFIX.Also applies to: 16-16, 19-19, 22-22, 25-26, 30-30, 33-33
test/util/data/txcreatedata2.json (2)
25-25: Enhanced output metadata with address descriptorAdded a descriptor field for the first output address, providing standardized metadata that can be used to reconstruct the script.
37-37: Added raw script descriptor for OP_RETURN outputAdded a raw script descriptor for the OP_RETURN output, which is appropriate since this type of output doesn't have a standard address format. This ensures all outputs have consistent descriptor information regardless of their type.
test/util/data/txcreatedata1.json (2)
25-25: Added descriptive metadata to scriptPubKey objectThis change adds a
descfield to the standard address scriptPubKey, providing a machine-readable description of the output script. This enhances the test data to match the updated RPC response format.
37-37: Added descriptive metadata to OP_RETURN scriptPubKeyThis change adds a
descfield with raw output descriptor format to the OP_RETURN output, which matches the updated RPC output format. The descriptor includes a checksum (#zf2avljj), ensuring descriptor validity.test/functional/rpc_getblockfrompeer.py (2)
67-67: Updated test assertion to match new RPC behaviorThe test now expects an empty dictionary as the successful result from
getblockfrompeerinstead of checking for the absence of a "warnings" key, aligning with the updated RPC implementation inblockchain.cpp.
70-70: Error handling approach changed for already downloaded blocksThe test now expects an RPC error to be raised directly with the message "Block already downloaded" instead of checking for a warning field in the response. This is consistent with the updated implementation in
blockchain.cppthat throws an error rather than returning a warning message.ci/test/04_install.sh (2)
53-56: Renamed Docker-specific function to generic CI contextThe
DOCKER_EXECfunction has been renamed toCI_EXECto use a more generic naming scheme that's not specific to Docker. The functionality remains unchanged - it executes commands in the CI context with the proper PATH setup.
62-64: Updated all function calls to use the new nameAll occurrences of
DOCKER_EXEChave been replaced withCI_EXECthroughout the script, maintaining consistent usage of the renamed function. This change aligns with the broader shift to a more generic CI naming scheme.Also applies to: 66-67, 78-80, 82-83, 87-92, 97-97, 101-101, 107-107, 111-113
src/rpc/blockchain.cpp (4)
476-479: Improved documentation for getblockfrompeer RPCThe documentation now more clearly explains the behavior of subsequent calls for the same block and explicitly states that the method returns an empty JSON object on success. This provides better guidance to API consumers.
484-484: Simplified RPC result structure for getblockfrompeerThe result structure has been updated to return an empty object without any warnings field, simplifying the API response. This is consistent with the changes in the error handling approach.
511-517: Changed error handling approach for already downloaded blocksInstead of pushing a warning message into the result when a block is already downloaded, the function now throws an error directly. This ensures a consistent error-handling approach across the RPC interface and simplifies the response structure. The return statement also now explicitly returns an empty object to maintain consistency.
1253-1253: Added descriptor field to gettxout RPC resultA new
descfield has been added to thescriptPubKeyobject in the result of thegettxoutRPC call, providing an inferred descriptor for the output. This enhances the information available to clients about transaction outputs, consistent with the broader changes to add descriptors throughout the codebase.src/rpc/rawtransaction.cpp (2)
215-215: Enhancement: Added descriptors to RPC outputAdding an inferred descriptor for script outputs is a valuable addition that improves the usability of the RPC interface. Descriptors provide a standardized, human-readable way to express script semantics, making it easier for developers to understand the purpose of different outputs without having to manually decode script hex.
768-768: Consistent enhancement: Added descriptor field to decoderawtransactionThe addition of the "desc" field to the
decoderawtransactionRPC maintains consistency with the changes togetrawtransaction. This is good practice to ensure uniform behavior across related RPC methods.test/functional/rpc_rawtransaction.py (2)
101-105: Improved block generation and error message clarityThe change from
generatetogenerateblockwith explicit transaction inclusion is a good improvement that makes the test more precise about which transactions should be included in the block. The addition of a well-defined error message variable also improves test readability.
109-136: Better test structure for txindex vs. non-txindex configurationsThis restructuring significantly improves the test organization by clearly separating the behaviors expected with and without txindex enabled. The test now explicitly verifies that:
- With txindex, all getrawtransaction variants work correctly
- Without txindex, appropriate error messages are returned
This change makes the test more maintainable and the test logic easier to understand.
src/test/fuzz/script.cpp (1)
135-141: Enhanced fuzz testing with dynamic parameter selectionThe changes significantly improve the fuzzing coverage by using
fuzzed_data_provider.ConsumeBool()to randomize the boolean parameters passed to the script functions. Instead of always testing with hardcoded parameters, the fuzzer now explores different combinations of:
- Whether to attempt sighash decoding in
ScriptToAsmStr- Whether to include hex in
ScriptPubKeyToUnivThis allows the fuzzer to find potential edge cases that might only occur with specific parameter combinations.
src/rpc/rawtransaction.cpp
Outdated
| {RPCResult::Type::STR, "desc", "Inferred descriptor for the script"}, | ||
| {RPCResult::Type::STR, "type", "The output type (e.g. " + GetAllOutputTypes() + ")"}, | ||
| {RPCResult::Type::STR, "address", /* optional */ true, "The Dash address (only if a well-defined address exists)"}, | ||
| {RPCResult::Type::STR, "desc", "Inferred descriptor for the script"}, |
There was a problem hiding this comment.
Fix duplication of "desc" field in decodescript output
There appears to be a duplication where the "desc" field is included twice in the result structure for the decodescript RPC. This will cause the same information to appear twice in the JSON response, which is redundant and potentially confusing.
{RPCResult::Type::STR, "type", "The output type (e.g. " + GetAllOutputTypes() + ")"},
{RPCResult::Type::STR, "address", /* optional */ true, "The Dash address (only if a well-defined address exists)"},
-{RPCResult::Type::STR, "desc", "Inferred descriptor for the script"},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {RPCResult::Type::STR, "desc", "Inferred descriptor for the script"}, | |
| {RPCResult::Type::STR, "type", "The output type (e.g. " + GetAllOutputTypes() + ")"}, | |
| {RPCResult::Type::STR, "address", /* optional */ true, "The Dash address (only if a well-defined address exists)"}, | |
| {RPCResult::Type::STR, "desc", "Inferred descriptor for the script"}, | |
| {RPCResult::Type::STR, "desc", "Inferred descriptor for the script"}, | |
| {RPCResult::Type::STR, "type", "The output type (e.g. " + GetAllOutputTypes() + ")"}, | |
| {RPCResult::Type::STR, "address", /* optional */ true, "The Dash address (only if a well-defined address exists)"}, |
There was a problem hiding this comment.
@PastaPastaPasta Yes, this can and should be resolved by removing the duplicated "desc" field. The PR has accidentally added the same field twice in the RPCResult structure, which would cause redundant information in the JSON output.
To fix it, you need to remove the second occurrence of the "desc" field (the one after the "address" field). The correct order should be:
- "asm"
- "desc"
- "type"
- "address"
@@ -807,8 +809,9 @@ static RPCHelpMan decodescript()
RPCResult::Type::OBJ, "", "",
{
{RPCResult::Type::STR, "asm", "Script public key"},
+ {RPCResult::Type::STR, "desc", "Inferred descriptor for the script"},
{RPCResult::Type::STR, "type", "The output type (e.g. " + GetAllOutputTypes() + ")"},
{RPCResult::Type::STR, "address", /* optional */ true, "The Dash address (only if a well-defined address exists)"},
- {RPCResult::Type::STR, "desc", "Inferred descriptor for the script"},This will ensure the RPC output has the "desc" field in the proper position and only appears once.
There was a problem hiding this comment.
I think it was resolved in some force push. Is your suggestion still needed?
There was a problem hiding this comment.
@PastaPastaPasta You're right! After checking the current code version, I can see that the issue has been resolved. The duplicated "desc" field is no longer present - there's only a single occurrence of the "desc" field properly positioned after "asm" and before "type" in the decodescript RPC result structure. My suggestion is no longer needed since the fix was already implemented. Thanks for bringing this to my attention!
|
This pull request has conflicts, please rebase. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ci/test/05_before_script.sh (1)
19-19: Download logic in CI context:
The switch toCI_EXEC curl --location --failaligns with the new CI wrapper. Optionally, you might add a timeout (e.g.,--connect-timeout 15) or retry logic to guard against transient network failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
.cirrus.yml(1 hunks)ci/test/04_install.sh(2 hunks)ci/test/05_before_script.sh(1 hunks)contrib/containers/ci/ci.Dockerfile(0 hunks)doc/REST-interface.md(1 hunks)doc/build-windows.md(1 hunks)share/setup.nsi.in(1 hunks)src/chain.cpp(1 hunks)src/chain.h(1 hunks)src/core_write.cpp(2 hunks)src/init.cpp(1 hunks)src/rpc/blockchain.cpp(3 hunks)src/rpc/rawtransaction.cpp(3 hunks)src/rpc/util.cpp(2 hunks)src/rpc/util.h(3 hunks)src/test/fuzz/script.cpp(1 hunks)test/functional/data/rpc_decodescript.json(2 hunks)test/functional/rpc_getblockfrompeer.py(1 hunks)test/functional/rpc_rawtransaction.py(1 hunks)test/sanitizer_suppressions/ubsan(1 hunks)test/util/data/tt-delin1-out.json(2 hunks)test/util/data/tt-delout1-out.json(1 hunks)test/util/data/tt-locktime317000-out.json(2 hunks)test/util/data/txcreate1.json(2 hunks)test/util/data/txcreate2.json(1 hunks)test/util/data/txcreatedata1.json(2 hunks)test/util/data/txcreatedata2.json(2 hunks)test/util/data/txcreatedata_seq0.json(1 hunks)test/util/data/txcreatedata_seq1.json(1 hunks)test/util/data/txcreatemultisig1.json(1 hunks)test/util/data/txcreatemultisig2.json(1 hunks)test/util/data/txcreateoutpubkey1.json(1 hunks)test/util/data/txcreatescript1.json(1 hunks)test/util/data/txcreatescript2.json(1 hunks)test/util/data/txcreatesignv1.json(1 hunks)
💤 Files with no reviewable changes (1)
- contrib/containers/ci/ci.Dockerfile
✅ Files skipped from review due to trivial changes (6)
- share/setup.nsi.in
- test/util/data/tt-delin1-out.json
- src/init.cpp
- test/sanitizer_suppressions/ubsan
- src/rpc/rawtransaction.cpp
- test/util/data/txcreatedata2.json
🚧 Files skipped from review as they are similar to previous changes (27)
- test/util/data/txcreate2.json
- test/util/data/txcreatescript1.json
- src/chain.cpp
- test/util/data/txcreateoutpubkey1.json
- test/util/data/txcreatesignv1.json
- src/core_write.cpp
- test/util/data/txcreatescript2.json
- test/functional/rpc_getblockfrompeer.py
- test/util/data/txcreatemultisig1.json
- doc/REST-interface.md
- test/util/data/txcreatedata_seq0.json
- test/util/data/txcreatemultisig2.json
- doc/build-windows.md
- .cirrus.yml
- test/util/data/tt-delout1-out.json
- src/test/fuzz/script.cpp
- test/util/data/txcreatedata1.json
- src/rpc/blockchain.cpp
- src/chain.h
- src/rpc/util.h
- ci/test/04_install.sh
- test/util/data/tt-locktime317000-out.json
- test/functional/rpc_rawtransaction.py
- test/util/data/txcreate1.json
- test/functional/data/rpc_decodescript.json
- test/util/data/txcreatedata_seq1.json
- src/rpc/util.cpp
🧰 Additional context used
🧬 Code Graph Analysis (1)
ci/test/05_before_script.sh (1)
ci/test/04_install.sh (1)
CI_EXEC(53-55)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (6)
ci/test/05_before_script.sh (6)
13-13: Consistent CI execution wrapper:
Good replacement ofDOCKER_EXECwithCI_EXECfor echoing into the default data directory. The escaping of>and\$HOMEcorrectly defers the redirection and variable expansion to the inner (CI) shell.
16-16: Ensure directory creation runs in CI context:
Wrapping themkdir -pinvocation inCI_EXECcorrectly creates the SDKs directories inside the CI environment, and quoting${DEPENDS_DIR}safely handles any special characters or spaces.
22-22: Consistent extraction in CI container:
UsingCI_EXEC tar -C ... -xfmaintains the same extraction behavior under the CI wrapper. Ensure that the SDK archive format always matches tar’s expectations (e.g., consider-zor-Jflags if the archive type changes).Please verify that all SDK archives consumed here are uncompressed tar files; if you introduce gzip or xz compression in future, update this invocation accordingly.
25-27: Broaden CentOS tag matching and switch shell:
Updating the pattern to*centos*ensures thatquay.io/centos/centos:stream9is detected, and settingCONFIG_SHELL=/bin/dashmatches the.cirrus.ymlchange. This is necessary for consistentconfigurebehavior on CentOS-based images.
30-30: Passing shell option to make:
Injecting"$SHELL_OPTS"into theCI_EXEC makeinvocation correctly applies theCONFIG_SHELLoverride when needed. WhenSHELL_OPTSis empty, the extra empty argument is harmless.
33-33: Retrieve previous releases in CI:
Wrapping thetest/get_previous_releases.pycall inCI_EXECis consistent with the other commands and ensures downloads occur in the CI context.
923312f rpc: use peer_id, block_hash for FetchBlock (Sjors Provoost) 34d5399 rpc: more detailed errors for getblockfrompeer (Sjors Provoost) 60243ca rpc: turn already downloaded into error in getblockfrompeer (Sjors Provoost) 809d66b rpc: clarify getblockfrompeer behavior when called multiple times (Sjors Provoost) 0e3d7c5 refactor: drop redundant hash argument from FetchBlock (Sjors Provoost) 8d1a3e6 rpc: allow empty JSON object result (Sjors Provoost) bfbf91d test: fancier Python for getblockfrompeer (Sjors Provoost) Pull request description: Followups from bitcoin#20295. ACKs for top commit: jonatack: ACK 923312f 📦 fjahr: tested ACK 923312f Tree-SHA512: da9eca76e302e249409c9d7f0d16cca668ed981e2ab6ca2d1743dad0d830b94b1bc5ffb9028a00764b863201945c273cc8f4409a4c9ca3817830007dffa2bc20
…transaction_tests 449dffc test, bugfix: fix intermittent failure in getrawtransaction_tests (Jon Atack) Pull request description: Easiest to review with `colorMoved = dimmed-zebra` and `colorMovedWs = allow-indentation-change`. Resolves bitcoin#23991 Top commit has no ACKs. Tree-SHA512: a50c597857e45c52dae1b6fc5988ea077c09f2b32ffe3605f0c64bd780f5dd2635755cb26c4981233bf925c5ea9f7310163349a549239dc85657558f253ffb7b
fac8caa doc: Fix rpc docs (MarcoFalke) Pull request description: Broken in commit 39d9bbe. The fix removes the "type" `OBJ_EMPTY` added in commit 8d1a3e6, which isn't really a separate type and instead runs a check on `OBJ` whether it is empty or not. ACKs for top commit: Sjors: tACK fac8caa Tree-SHA512: dd978fe526a45095800249204afd26a239078e83b15124a5756ac078c473a677a3084b8f54e34d6dd5580abef7275c875a14bc9eb20d8feab066dfb0f0932967
b3ccf26 build: force CRCCheck in Windows installer (fanquake) Pull request description: Otherwise a user can pass `/NCRC` on the command line and bypass the CRC check, meaning they could use a corrupted installer. I can't think of a reason why we'd want to allow the use of corrupted installers. [NSIS docs](https://nsis.sourceforge.io/Docs/Chapter4.html#acrccheck): > Specifies whether or not the installer will perform a CRC on itself before allowing an install. Note that if the user uses /NCRC on the command line when executing the installer, and you didn't specify 'force', the CRC will not occur, and the user will be allowed to install a (potentially) corrupted installer. ACKs for top commit: hebasto: ACK b3ccf26, I have reviewed the code and it looks OK, I agree it can be merged. prusnak: utACK b3ccf26 Tree-SHA512: 6b3c151bfd896dbf1a4af26114aec1721e4d4e6dad89eef796cd754c988b1bb03fd9a3f9889be3c754ff5b1cd7bf5bbfc126876c76038205c17807daff029319
fa68429 fuzz: Speed up script fuzz target (MarcoFalke) Pull request description: Currently the script fuzz target takes the longest time (5000 seconds, aka 80 minutes, see https://cirrus-ci.com/task/5651378755338240?logs=ci#L4501). Fix this by making it twice as fast. Instead of running all possible combinations for all fuzz inputs, consume a bool and decide at runtime which path to take. I moved the new calls to the end to not invalidate existing fuzz inputs. ACKs for top commit: prusnak: ACK fa68429 Tree-SHA512: 5e408255f96f9e92e472f4e8a8a0f8d8814bad444ac0ff7d5db5ed84a59a861135ffe5e04d81f479b0695cb17e4d7af005734959dd4aa9328bdc5acc98f36665
fa83210 Avoid integer sanitizer warnings in chain.o (MarcoFalke) Pull request description: The two changes make the code more self-documenting and also allow to remove 5 file-wide suppressions for the module ACKs for top commit: PastaPastaPasta: utACK fa83210 jonatack: ACK fa83210 Tree-SHA512: d32a06099c56eed9f69130a3209f989872acc593f849528acd7746ee6caa96688cc32de37e8e59ad5d25dcb8912e341f1a43e50642dadeff6ca7624d0873ad10
fafc55a ci: Use dash when building depends in centos build (MarcoFalke) fa33236 scripted-diff: Rename DOCKER_EXEC to CI_EXEC (MarcoFalke) fa5457e ci: Bump CentOS 8 image (MarcoFalke) Pull request description: CentOS 8 was just shut down: ``` $ pob centos:8 [root_e606059da4d9 /]# dnf update Failed to set locale, defaulting to C.UTF-8 CentOS Linux 8 - AppStream Error: Failed to download metadata for repo 'appstream': Cannot prepare internal mirrorlist: No URLs in mirrorlist ``` Fix that by bumping the image. ACKs for top commit: Sjors: utACK fafc55a with the caveat that I've never used CentOS. Tree-SHA512: 741eb606c9f67cd218e41ce98460606d8594ff98148636c28ac20bae2809e3ee4a18c97aa3c849630f0a6ac30255c7423253b4309ae2d7cdd5c6624cc6836b95
BACKPORT NOTE: extra changes in build-windows.md due to missing bitcoin#11438 9796dca doc: Install only "-posix" MinGW compiler when possible (Hennadii Stepanov) 0bbae23 ci: Drop no longer needed `update-alternatives` (Hennadii Stepanov) 01d1845 build, qt: Specify QMAKE_CXX explicitly (Hennadii Stepanov) Pull request description: While changes introduced in bitcoin#22093 worked fine with Qt 5.12, after bumping Qt up to 5.15 the cross-compiling of `qt` package for Windows fails with `error: ‘mutex’ in namespace ‘std’ does not name a type`. The first commit fixes this bug. The second commit cleans up a related CI script. The third commit improves related docs (see bitcoin#22093 (comment)). ACKs for top commit: prusnak: ACK 9796dca Tree-SHA512: 0dc46c5dfab85bd6d2901052cd630e86f9b4e09c08ef87136b44ddecb1783cdf3cd0a6e67b95ac7a78da24cd7adedc88745f61f9a8d9993fbff26d33bf88d874
0425507 qt: Update source translations (laanwj) cf79c56 init: Remove confusing '(possible integer overflow?)' from error message (laanwj) d570a63 qt: Update transifex resource blob to 23.0 (laanwj) Pull request description: - Update translations for 0.23 string freeze - Update transifex resource blob to 23.0 This is necessary before a 23.0 resource can be created on Transifex. ACKs for top commit: hebasto: ACK 0425507 Tree-SHA512: ff886e92721f070e3c135cfec229c41848a67c02355b88f2a5a507241b545f4209167d83b561420c2a82f49a5994170b01dcfb95bfc3fe6b9c832abcc6547b14
…descriptors 6498ba1 transaction decoding infer output descriptors (Gregory Sanders) Pull request description: Following discussion in bitcoin#16725 this is complementary data to expose. All outputs are inferred. ACKs for top commit: achow101: ACK 6498ba1 meshcollider: utACK 6498ba1 Tree-SHA512: 36664117ddbe46d5fdde7ed6541ef2c9d8dfb7a3636b97f363bf1c325096fe00d9d2acea2d1917ea19fdb82f1ea296c12e440c5c703d6a9bfc1a02fba028bcd8
…ram name blockhash 88917f9 RPC: Switch getblockfrompeer back to standard param name blockhash (Luke Dashjr) Pull request description: This commit partially reverts 923312f. Portion of bitcoin#24294. ACKs for top commit: MarcoFalke: review ACK 88917f9 ajtowns: ACK 88917f9 jonatack: Review-and-grep-only ACK 88917f9 Tree-SHA512: e42497ea6162623e449c5e60b83a5abbef568f226edc022aa14bbc1f1921618255d593968cf43f7a6d2c0bfd84cdd4b05fbce5c724759b20035e6eead758d443
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
ci/test/05_before_script.sh (3)
13-13: Improve readability of escaped redirection
The current lineCI_EXEC echo \> \$HOME/.dashcorecorrectly defers the
>redirection into the CI shell, but the escaping is non-intuitive. Consider wrapping the command in a single-quoted bash invocation for clarity, for example:-CI_EXEC echo \> \$HOME/.dashcore +CI_EXEC bash -c 'echo > "$HOME/.dashcore"'
19-19: Add robustness to the download step
Consider enhancingCI_EXEC curl --location --fail "${SDK_URL}/${OSX_SDK_BASENAME}" -o "$OSX_SDK_PATH"with retry and timeout flags (e.g.,
--retry 3 --retry-delay 5 --connect-timeout 15 --show-error) to make it more resilient against transient network failures.
30-30: Clarify environment variable injection
UsingCI_EXEC "$SHELL_OPTS" make "$MAKEJOBS" -C depends HOST="$HOST" "$DEP_OPTS" LOG=1works, but embedding
CONFIG_SHELL=...as an unquoted$SHELL_OPTScan be subtle. You might exportCONFIG_SHELLbefore theCI_EXECcall to simplify the invocation.doc/build-windows.md (4)
42-42: Use consistent list bullet style
Markdown lint expects*bullets with no leading indent. Change:- - on modern systems (Ubuntu 21.04 Hirsute Hippo or newer, Debian 11 Bullseye or newer): +* on modern systems (Ubuntu 21.04 Hirsute Hippo or newer, Debian 11 Bullseye or newer):🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
42-42: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
42-42: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
44-46: Align code block style with project guidelines
The fenced blocksudo apt install g++-mingw-w64-x86-64-posixtriggers MD046. Either convert to an indented block:
-```sh -sudo apt install g++-mingw-w64-x86-64-posix -``` + sudo apt install g++-mingw-w64-x86-64-posixor update the linter configuration to allow fenced blocks.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
44-44: Code block style
Expected: indented; Actual: fenced(MD046, code-block-style)
48-48: Use consistent list bullet style for older systems
Similarly update:- - on older systems: +* on older systems:🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
48-48: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
48-48: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
50-52: Align secondary code block style
Convert the fenced block to indented syntax or permit fenced blocks in lint rules:-```sh -sudo apt install g++-mingw-w64-x86-64 -``` + sudo apt install g++-mingw-w64-x86-64🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
50-50: Code block style
Expected: indented; Actual: fenced(MD046, code-block-style)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
.cirrus.yml(1 hunks)ci/test/04_install.sh(2 hunks)ci/test/05_before_script.sh(1 hunks)contrib/containers/ci/ci.Dockerfile(0 hunks)doc/REST-interface.md(1 hunks)doc/build-windows.md(1 hunks)share/setup.nsi.in(1 hunks)src/chain.cpp(1 hunks)src/chain.h(1 hunks)src/core_write.cpp(2 hunks)src/init.cpp(1 hunks)src/rpc/blockchain.cpp(3 hunks)src/rpc/rawtransaction.cpp(3 hunks)src/rpc/util.cpp(2 hunks)src/rpc/util.h(3 hunks)src/test/fuzz/script.cpp(1 hunks)test/functional/data/rpc_decodescript.json(2 hunks)test/functional/rpc_getblockfrompeer.py(1 hunks)test/functional/rpc_rawtransaction.py(1 hunks)test/sanitizer_suppressions/ubsan(1 hunks)test/util/data/tt-delin1-out.json(2 hunks)test/util/data/tt-delout1-out.json(1 hunks)test/util/data/tt-locktime317000-out.json(2 hunks)test/util/data/txcreate1.json(2 hunks)test/util/data/txcreate2.json(1 hunks)test/util/data/txcreatedata1.json(2 hunks)test/util/data/txcreatedata2.json(2 hunks)test/util/data/txcreatedata_seq0.json(1 hunks)test/util/data/txcreatedata_seq1.json(1 hunks)test/util/data/txcreatemultisig1.json(1 hunks)test/util/data/txcreatemultisig2.json(1 hunks)test/util/data/txcreateoutpubkey1.json(1 hunks)test/util/data/txcreatescript1.json(1 hunks)test/util/data/txcreatescript2.json(1 hunks)test/util/data/txcreatesignv1.json(1 hunks)
💤 Files with no reviewable changes (1)
- contrib/containers/ci/ci.Dockerfile
✅ Files skipped from review due to trivial changes (4)
- .cirrus.yml
- doc/REST-interface.md
- test/util/data/tt-delin1-out.json
- test/util/data/txcreatedata2.json
🚧 Files skipped from review as they are similar to previous changes (28)
- test/util/data/txcreatemultisig1.json
- src/chain.h
- test/util/data/txcreateoutpubkey1.json
- test/util/data/txcreatedata_seq0.json
- share/setup.nsi.in
- test/util/data/txcreate2.json
- test/util/data/txcreatescript1.json
- test/util/data/txcreatescript2.json
- test/util/data/txcreatesignv1.json
- src/chain.cpp
- src/core_write.cpp
- src/rpc/blockchain.cpp
- test/util/data/txcreatedata_seq1.json
- test/util/data/tt-delout1-out.json
- test/util/data/txcreatemultisig2.json
- src/rpc/util.h
- src/test/fuzz/script.cpp
- test/functional/rpc_getblockfrompeer.py
- test/functional/data/rpc_decodescript.json
- test/util/data/txcreatedata1.json
- src/init.cpp
- src/rpc/util.cpp
- test/util/data/txcreate1.json
- ci/test/04_install.sh
- test/util/data/tt-locktime317000-out.json
- test/functional/rpc_rawtransaction.py
- test/sanitizer_suppressions/ubsan
- src/rpc/rawtransaction.cpp
🧰 Additional context used
🧬 Code Graph Analysis (1)
ci/test/05_before_script.sh (1)
ci/test/04_install.sh (1)
CI_EXEC(53-55)
🪛 markdownlint-cli2 (0.17.2)
doc/build-windows.md
42-42: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
42-42: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
44-44: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
48-48: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
48-48: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
50-50: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (4)
ci/test/05_before_script.sh (4)
16-16: Approve directory creation in CI
The invocationCI_EXEC mkdir -p "${DEPENDS_DIR}/SDKs" "${DEPENDS_DIR}/sdk-sources"correctly ensures the SDK directories exist in the CI environment.
22-22: Extraction command is correct
The tar invocationCI_EXEC tar -C "${DEPENDS_DIR}/SDKs" -xf "$OSX_SDK_PATH"properly extracts the SDK. If needed, you can add
--strip-componentsfor more control, otherwise this is good.
25-26: Consistent CentOS detection and shell choice
Switching to the patternif [[ $DOCKER_NAME_TAG == *centos* ]]; then SHELL_OPTS="CONFIG_SHELL=/bin/dash"broadens tag matching (e.g.,
centos:stream9) and aligns with the Dockerfile shell change. This matchesci/test/04_install.sh.
33-33: Approve retrieval of previous releases
The callCI_EXEC test/get_previous_releases.py -b -t "$PREVIOUS_RELEASES_DIR" "${PREVIOUS_RELEASES_TO_DOWNLOAD}"correctly downloads historical releases. Ensure the script has executable permissions and the expected shebang.
knst
changed the title
What was done?
Regular backports from Bitcoin Core v23
How Has This Been Tested?
Run unit & functional tests
Breaking Changes
N/A
Checklist: