backport: bitcoin#22235, #23635, #23662, #24147, #24152, #24538, #24786, #24796, #24906, #25043, #25067 #6871
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughAdds a full Miniscript front-end (public miniscript.h API and internal miniscript.cpp implementation), new Miniscript unit and fuzz tests, and exposes miniscript sources/headers in build files. Implements package-level feerate policy and propagates package_feerate through mempool acceptance paths (ATMPArgs changes, SingleInPackageAccept, PackageMempoolAcceptResult gains optional m_package_feerate). Moves minimal-push validation into src/script (adds CheckMinimalPush in script.cpp/h and removes the old interpreter declaration). Tightens -rpcauth parsing and adds functional tests. Also includes CI/Docker/version bumps, docs and tooling additions, and various small formatting/refactor edits. Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/validationinterface.cpp (1)
282-320: Forwardidto validation callbacks
NotifyChainLock,NotifyGovernanceVote,NotifyGovernanceObject, andNotifyRecoveredSignow accept anid, but the lambdas never capture or forward it. Callers relying on the new identifier won’t see it, and onceCValidationInterfaceis updated this will miscompile. Please propagateidthrough the lambdas.- auto event = [pindex, clsig, this] { - m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.NotifyChainLock(pindex, clsig); }); + auto event = [pindex, clsig, id, this] { + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.NotifyChainLock(pindex, clsig, id); }); }; @@ - auto event = [vote, tip_mn_list, this] { - m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.NotifyGovernanceVote(tip_mn_list, vote); }); + auto event = [vote, tip_mn_list, id, this] { + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.NotifyGovernanceVote(tip_mn_list, vote, id); }); }; @@ - auto event = [object, this] { - m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.NotifyGovernanceObject(object); }); + auto event = [object, id, this] { + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.NotifyGovernanceObject(object, id); }); }; @@ - auto event = [sig, this] { - m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.NotifyRecoveredSig(sig); }); + auto event = [sig, id, this] { + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.NotifyRecoveredSig(sig, id); }); };src/validationinterface.h (1)
235-239: Include<string>for the new id parameters.
std::stringis now used in this header but<string>is not included, so any translation unit that includesvalidationinterface.hbefore pulling in<string>will fail to compile. Please add the missing include.#include <functional> #include <memory> +#include <string>
🧹 Nitpick comments (2)
contrib/devtools/README.md (1)
93-106: Consider using fenced code blocks for consistency.The documentation for
gen-dash-conf.shis clear and helpful. However, the code blocks on lines 97 and 103 use indented style rather than fenced (```) style, which is flagged by markdownlint and is less consistent with modern markdown conventions.Based on static analysis hints.
Consider applying this formatting adjustment:
-Generates a dash.conf file in `share/examples/` by parsing the output from `dashd--help`. This script is run during the +Generates a dash.conf file in `share/examples/` by parsing the output from `dashd --help`. This script is run during the release process to include a dash.conf with the release binaries and can also be run by users to generate a file locally. When generating a file as part of the release process, make sure to commit the changes after running the script. With in-tree builds this tool can be run from any directory within the repository. To use this tool with out-of-tree builds set `BUILDDIR`. For example: +```bash - - BUILDDIR=$PWD/build contrib/devtools/gen-dash-conf.sh +BUILDDIR=$PWD/build contrib/devtools/gen-dash-conf.sh +``` -Note: Also fixed a typo:
dashd--help→dashd --helpon line 96.doc/policy/packages.md (1)
64-98: LGTM: Package fees documentation is clear and well-reasoned.The new "Package Fees and Feerate" section effectively documents the package-level feerate policy. The explanations of:
- Feerate calculation excluding mempool transactions
- CPFP within packages
- Individual-first validation approach
- Rationale for policy decisions
are all clear and align with the implementation changes in
src/validation.hand related files.Minor: Consider using fenced code blocks for consistency.
The static analysis tool suggests using fenced code blocks instead of indented blocks for better markdown consistency. While this is a minor formatting preference, fenced blocks (```) are more explicit and portable across markdown renderers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (49)
CONTRIBUTING.md(2 hunks)README.md(1 hunks)ci/lint/04_install.sh(1 hunks)contrib/containers/ci/ci-slim.Dockerfile(1 hunks)contrib/debian/examples/dash.conf(1 hunks)contrib/devtools/README.md(1 hunks)contrib/devtools/gen-dash-conf.sh(1 hunks)contrib/guix/libexec/build.sh(1 hunks)doc/build-openbsd.md(1 hunks)doc/dash-conf.md(1 hunks)doc/policy/packages.md(1 hunks)doc/release-process.md(1 hunks)src/Makefile.am(2 hunks)src/Makefile.test.include(2 hunks)src/bitcoin-tx.cpp(1 hunks)src/httprpc.cpp(2 hunks)src/init.cpp(1 hunks)src/masternode/node.h(1 hunks)src/node/chainstate.cpp(1 hunks)src/node/miner.h(1 hunks)src/psbt.cpp(2 hunks)src/rest.cpp(1 hunks)src/rpc/blockchain.cpp(1 hunks)src/rpc/rawtransaction.cpp(1 hunks)src/rpc/util.cpp(2 hunks)src/script/interpreter.cpp(0 hunks)src/script/interpreter.h(0 hunks)src/script/miniscript.cpp(1 hunks)src/script/miniscript.h(1 hunks)src/script/script.cpp(1 hunks)src/script/script.h(2 hunks)src/script/sign.cpp(4 hunks)src/script/sign.h(2 hunks)src/script/standard.cpp(0 hunks)src/script/standard.h(1 hunks)src/test/fuzz/miniscript_decode.cpp(1 hunks)src/test/fuzz/script_sign.cpp(1 hunks)src/test/fuzz/tx_pool.cpp(1 hunks)src/test/miner_tests.cpp(5 hunks)src/test/miniscript_tests.cpp(1 hunks)src/test/script_tests.cpp(1 hunks)src/test/txpackage_tests.cpp(9 hunks)src/validation.cpp(12 hunks)src/validation.h(2 hunks)src/validationinterface.cpp(3 hunks)src/validationinterface.h(1 hunks)src/wallet/rpc/backup.cpp(2 hunks)src/wallet/rpc/coins.cpp(3 hunks)test/functional/rpc_users.py(1 hunks)
💤 Files with no reviewable changes (3)
- src/script/interpreter.cpp
- src/script/interpreter.h
- src/script/standard.cpp
🧰 Additional context used
📓 Path-based instructions (7)
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/release-process.mddoc/dash-conf.mddoc/policy/packages.mddoc/build-openbsd.md
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/rpc_users.py
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/node/chainstate.cppsrc/rest.cppsrc/rpc/util.cppsrc/test/script_tests.cppsrc/script/standard.hsrc/script/script.hsrc/test/miner_tests.cppsrc/script/script.cppsrc/test/txpackage_tests.cppsrc/init.cppsrc/masternode/node.hsrc/script/sign.cppsrc/script/miniscript.cppsrc/node/miner.hsrc/psbt.cppsrc/wallet/rpc/coins.cppsrc/validation.hsrc/script/sign.hsrc/wallet/rpc/backup.cppsrc/rpc/blockchain.cppsrc/httprpc.cppsrc/test/fuzz/tx_pool.cppsrc/test/miniscript_tests.cppsrc/validationinterface.hsrc/rpc/rawtransaction.cppsrc/validation.cppsrc/test/fuzz/script_sign.cppsrc/test/fuzz/miniscript_decode.cppsrc/bitcoin-tx.cppsrc/validationinterface.cppsrc/script/miniscript.h
contrib/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the contrib directory (contributed scripts)
Files:
contrib/devtools/gen-dash-conf.shcontrib/guix/libexec/build.shcontrib/devtools/README.mdcontrib/debian/examples/dash.confcontrib/containers/ci/ci-slim.Dockerfile
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/script_tests.cppsrc/test/miner_tests.cppsrc/test/txpackage_tests.cppsrc/test/fuzz/tx_pool.cppsrc/test/miniscript_tests.cppsrc/test/fuzz/script_sign.cppsrc/test/fuzz/miniscript_decode.cpp
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/masternode/node.h
ci/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the ci directory (continuous integration)
Files:
ci/lint/04_install.sh
🧠 Learnings (1)
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
PR: dashpay/dash#6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/wallet/rpc/backup.cpp
🧬 Code graph analysis (19)
test/functional/rpc_users.py (1)
test/functional/test_framework/test_node.py (1)
assert_start_raises_init_error(606-649)
src/rest.cpp (1)
src/node/transaction.cpp (2)
GetTransaction(120-151)GetTransaction(120-120)
src/test/script_tests.cpp (1)
src/script/sign.cpp (4)
ProduceSignature(189-218)ProduceSignature(189-189)MutableTransactionSignatureCreator(20-24)MutableTransactionSignatureCreator(26-32)
src/script/script.h (1)
src/script/script.cpp (2)
CheckMinimalPush(386-409)CheckMinimalPush(386-386)
src/test/txpackage_tests.cpp (2)
src/test/util/setup_common.cpp (4)
mineBlocks(447-461)mineBlocks(447-447)CreateValidMempoolTransaction(544-591)CreateValidMempoolTransaction(544-550)src/validation.cpp (2)
ProcessNewPackage(1420-1438)ProcessNewPackage(1420-1421)
src/script/miniscript.cpp (3)
src/script/script.h (4)
DecodeOP_N(488-494)vector(330-361)CScriptNum(211-226)CScriptNum(230-240)src/script/standard.h (1)
IsPushdataOp(111-114)src/script/script.cpp (2)
CheckMinimalPush(386-409)CheckMinimalPush(386-386)
src/psbt.cpp (1)
src/script/sign.cpp (1)
creator(327-327)
src/wallet/rpc/coins.cpp (2)
src/wallet/rpc/util.cpp (2)
LabelFromValue(117-123)LabelFromValue(117-117)src/wallet/scriptpubkeyman.h (1)
wallet(28-227)
src/validation.h (1)
src/node/psbt.cpp (1)
feerate(140-140)
src/script/sign.h (1)
src/script/sign.cpp (2)
MutableTransactionSignatureCreator(20-24)MutableTransactionSignatureCreator(26-32)
src/test/fuzz/tx_pool.cpp (1)
src/validation.cpp (2)
ProcessNewPackage(1420-1438)ProcessNewPackage(1420-1421)
src/test/miniscript_tests.cpp (2)
src/test/fuzz/miniscript_decode.cpp (8)
key(24-27)key(24-24)key(28-30)key(28-28)key(31-34)key(31-31)FromString(37-41)FromString(37-37)src/crypto/ripemd160.h (1)
CRIPEMD160(12-26)
src/validationinterface.h (1)
src/validationinterface.cpp (2)
MainSignalsImpl(47-47)MainSignalsImpl(47-47)
src/rpc/rawtransaction.cpp (1)
src/script/sign.cpp (4)
ProduceSignature(189-218)ProduceSignature(189-189)MutableTransactionSignatureCreator(20-24)MutableTransactionSignatureCreator(26-32)
src/validation.cpp (1)
src/validation.h (1)
state(169-194)
src/test/fuzz/miniscript_decode.cpp (1)
src/test/miniscript_tests.cpp (6)
key(75-75)key(75-75)key(78-83)key(78-78)FromString(87-91)FromString(87-87)
src/bitcoin-tx.cpp (2)
src/script/sign.cpp (4)
ProduceSignature(189-218)ProduceSignature(189-189)MutableTransactionSignatureCreator(20-24)MutableTransactionSignatureCreator(26-32)src/rpc/rawtransaction.cpp (1)
mergedTx(863-863)
src/validationinterface.cpp (1)
src/net_processing.cpp (2)
scheduler(625-625)scheduler(676-676)
src/script/miniscript.h (2)
src/script/miniscript.cpp (12)
ComputeType(38-251)ComputeType(38-38)ComputeScriptLen(253-284)ComputeScriptLen(253-253)SanitizeType(16-36)SanitizeType(16-16)FindNextChar(338-346)FindNextChar(338-338)DecomposeScript(286-321)DecomposeScript(286-286)ParseScriptNumber(323-336)ParseScriptNumber(323-323)src/test/miniscript_tests.cpp (6)
key(75-75)key(75-75)key(78-83)key(78-78)FromString(87-91)FromString(87-87)
🪛 markdownlint-cli2 (0.18.1)
contrib/devtools/README.md
97-97: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
103-103: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
doc/policy/packages.md
97-97: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build container / Build container
🔇 Additional comments (22)
src/test/fuzz/tx_pool.cpp (1)
236-247: LGTM! Proper handling of package-policy results.The conditional logic correctly guards per-transaction result verification when a package-policy issue occurs. This prevents potential assertion failures when
ProcessNewPackagedoesn't return per-transaction results due to package-level policy violations. The implementation aligns with the package-feerate policy changes introduced in this PR.src/node/miner.h (1)
138-138: LGTM! Correctness improvement for fee accounting.Switching from
GetFee()toGetModifiedFee()ensures consistency with hownModFeesWithAncestorsis initialized (Line 63 usesGetModFeesWithAncestors(), which tracks modified fees). When a parent is included in the block, subtracting its modified fee correctly updates the child's ancestor fee total, accounting for any fee prioritization.doc/release-process.md (1)
4-6: LGTM! Branch reference updates are consistent.The checklist item URLs have been correctly updated to reference the
developbranch instead ofmaster, aligning with the repository's branch structure. The addition of thegen-dash-conf.shreference is consistent with the new script documented incontrib/devtools/README.md.src/masternode/node.h (1)
14-14: LGTM! Appropriate forward declaration for header decoupling.Adding the
CConnmanforward declaration here reduces header dependencies and follows best practices for modular design. This aligns with the broader effort noted in the AI summary to decouple components across the codebase.src/node/chainstate.cpp (1)
151-153: LGTM! Minor formatting improvement.Removing spaces around parameter names in the inline comments improves consistency with the codebase's comment style. No functional changes.
src/script/standard.h (1)
111-114: LGTM! Appropriate constexpr function for pushdata identification.The
IsPushdataOpfunction is correctly implemented as aconstexprfunction, enabling compile-time evaluation. The logic correctly identifies pushdata opcodes (OP_FALSE < opcode ≤ OP_PUSHDATA4), and moving it to the header improves its accessibility across the codebase.doc/build-openbsd.md (1)
13-15: LGTM! Improved build instructions for OpenBSD.Consolidating the package installation into a single command and adding the clarifying comment about selecting the newest versions improves the documentation's clarity and usability.
src/rpc/blockchain.cpp (1)
1301-1301: LGTM! Type safety and modern C++ style improvement.The change from
inttouint32_timproves type safety, asCOutPoint::nis typically an unsigned 32-bit value. Using braced initialization aligns with modern C++20 style and makes the code more concise by eliminating the intermediate variable.CONTRIBUTING.md (2)
13-15: LGTM! Simplified governance structure description.The updated wording clarifies the maintainer role by removing the explicit "lead maintainer" concept and simplifying to "repository maintainers." This provides a clearer and more general description of responsibilities without changing the underlying process.
274-274: LGTM! Consistent governance terminology.The simplification to "maintainers" aligns with the earlier changes in the document and maintains consistency throughout the contribution guidelines.
src/validation.h (2)
234-237: LGTM: Package feerate field is well-documented.The new
m_package_feeratefield correctly captures the package-level feerate with clear documentation explaining its definition (aggregated modified fees divided by total size) and caveats about availability. The use ofstd::optional<CFeeRate>appropriately models the possibility that the field may be unavailable in certain scenarios.
243-246: LGTM: Constructor overload properly initializes package feerate.The new constructor correctly accepts a
CFeeRate feerateparameter and initializesm_package_feeratewith it, alongside the existingstateandresultsparameters. This follows the established pattern of the existing constructor while extending functionality.src/script/script.cpp (1)
386-409: LGTM: Minimal push check correctly enforces consensus rules.The
CheckMinimalPushfunction properly validates that data is pushed using the minimal encoding required by script consensus rules. The logic correctly handles all size ranges and special cases (empty data, OP_1..OP_16, OP_1NEGATE). The assertion on line 388 ensures the opcode is within the valid push range [0, OP_PUSHDATA4].src/script/script.h (2)
323-323: LGTM: Accessor provides direct access to internal value.The
GetInt64()method correctly exposes the internalm_valueasint64_t, following the existing pattern of accessor methods inCScriptNum.
547-547: LGTM: Declaration matches implementation.The
CheckMinimalPushdeclaration correctly matches the implementation insrc/script/script.cpp(lines 386-409).src/psbt.cpp (2)
208-208: LGTM: Constructor invocation updated to use reference.The change from
&txtotxcorrectly updates theMutableTransactionSignatureCreatorconstructor invocation to pass a reference instead of a pointer, aligning with the updated API insrc/script/sign.h.
270-270: LGTM: Constructor invocation updated to use reference.The change from
&txtotxcorrectly updates theMutableTransactionSignatureCreatorconstructor invocation to pass a reference instead of a pointer, consistent with the API change.src/wallet/rpc/coins.cpp (2)
20-42: LGTM: Script-based filtering is correctly implemented.The refactoring from destination-based to script-based filtering is sound. The code now:
- Converts addresses to scripts via
GetScriptForDestination- Filters scripts by ownership (
IsMine) at collection time- Stores scripts directly in
output_scriptsThis approach maintains correctness while simplifying the filtering logic in the balance calculation loop. Early ownership validation at collection time is a good practice.
74-74: LGTM: Direct scriptPubKey comparison is efficient.The change to compare
txout.scriptPubKeydirectly againstoutput_scriptsis more efficient than the previous destination-based approach, as it avoids the overhead of extracting destinations from scripts during the balance tallying loop.src/test/fuzz/miniscript_decode.cpp (2)
21-52: LGTM: Converter implementation is correct for fuzz testing.The
Converterstruct properly implements all required methods for Miniscript fuzz testing:
ToStringandFromStringcorrectly handle hex encoding/decodingToPKBytesandToPKHBytesproduce appropriate byte representationsFromPKBytesvalidates key constructionFromPKHBytescorrectly returns false for the unsupported PKH-to-key path with an assertion to catch test logic errors
56-72: LGTM: Fuzz target correctly validates Miniscript round-tripping.The fuzz target properly:
- Deserializes a CScript from fuzz input
- Parses it as Miniscript
- Validates round-trip via string representation
- Verifies script round-trip consistency
This provides good coverage for Miniscript parsing and serialization.
contrib/devtools/gen-dash-conf.sh (1)
1-80: Script appears functional but review is limited per coding guidelines.The script structure looks reasonable for generating a dash.conf file from
dashd --helpoutput. The sed/awk pipeline transforms help text into configuration format, and path handling uses standard bash patterns. However, per the coding guidelines: "Unless specifically prompted, avoid making changes to the contrib directory."Note: This is a new contributed script for tooling purposes. While the implementation appears sound, changes to contrib/ are generally outside typical review scope unless issues are critical.
| #include <string> | ||
| #include <vector> | ||
| #include <script/script.h> | ||
| #include <script/standard.h> | ||
| #include <script/miniscript.h> | ||
|
|
||
| #include <assert.h> | ||
|
|
There was a problem hiding this comment.
Add the missing <algorithm> include
std::reverse is used (Line 319) but <algorithm> is not included. This file won't compile on standard-compliant toolchains without it. Please add the header.
#include <string>
#include <vector>
+#include <algorithm>📝 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.
| #include <string> | |
| #include <vector> | |
| #include <script/script.h> | |
| #include <script/standard.h> | |
| #include <script/miniscript.h> | |
| #include <assert.h> | |
| #include <string> | |
| #include <vector> | |
| #include <algorithm> | |
| #include <script/script.h> | |
| #include <script/standard.h> | |
| #include <script/miniscript.h> | |
| #include <assert.h> |
🤖 Prompt for AI Agents
In src/script/miniscript.cpp around lines 5 to 12, the file uses std::reverse
later but does not include <algorithm>; add the missing #include <algorithm> to
the existing include block near the top of the file so the translation unit
compiles on standard-compliant toolchains.
| #include <algorithm> | ||
| #include <numeric> | ||
| #include <memory> | ||
| #include <optional> | ||
| #include <string> | ||
| #include <vector> | ||
|
|
||
| #include <stdlib.h> | ||
| #include <assert.h> | ||
|
|
||
| #include <policy/policy.h> | ||
| #include <primitives/transaction.h> | ||
| #include <script/script.h> | ||
| #include <span.h> | ||
| #include <util/spanparsing.h> | ||
| #include <util/strencodings.h> | ||
| #include <util/string.h> | ||
| #include <util/vector.h> | ||
|
|
There was a problem hiding this comment.
Add the missing standard headers
This header uses std::logic_error (Line 175) and std::tuple (Line 892), but neither <stdexcept> nor <tuple> is included. Under C++20 with Clang 16/GCC 11, this leads to hard compilation errors (std::logic_error not declared / incomplete type for std::tuple). Please add the explicit includes.
#include <optional>
#include <string>
#include <vector>
+#include <stdexcept>
+#include <tuple>
+
#include <stdlib.h>
#include <assert.h>📝 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.
| #include <algorithm> | |
| #include <numeric> | |
| #include <memory> | |
| #include <optional> | |
| #include <string> | |
| #include <vector> | |
| #include <stdlib.h> | |
| #include <assert.h> | |
| #include <policy/policy.h> | |
| #include <primitives/transaction.h> | |
| #include <script/script.h> | |
| #include <span.h> | |
| #include <util/spanparsing.h> | |
| #include <util/strencodings.h> | |
| #include <util/string.h> | |
| #include <util/vector.h> | |
| #include <algorithm> | |
| #include <numeric> | |
| #include <memory> | |
| #include <optional> | |
| #include <string> | |
| #include <vector> | |
| #include <stdexcept> | |
| #include <tuple> | |
| #include <stdlib.h> | |
| #include <assert.h> | |
| #include <policy/policy.h> | |
| #include <primitives/transaction.h> | |
| #include <script/script.h> | |
| #include <span.h> | |
| #include <util/spanparsing.h> | |
| #include <util/strencodings.h> | |
| #include <util/string.h> | |
| #include <util/vector.h> |
🤖 Prompt for AI Agents
In src/script/miniscript.h around lines 8 to 26, the header uses
std::logic_error (line ~175) and std::tuple (line ~892) but does not include
<stdexcept> or <tuple>; add explicit #include <stdexcept> and #include <tuple>
alongside the other standard headers near the top of the file (with <algorithm>,
<numeric>, etc.) so the types are declared and compilation errors are resolved.
| /** Build a script by concatenating other scripts, or any argument accepted by CScript::operator<<. */ | ||
| template<typename... Ts> | ||
| CScript BuildScript(Ts&&... inputs) | ||
| { | ||
| CScript ret; | ||
| int cnt{0}; | ||
|
|
||
| ([&ret, &cnt] (Ts&& input) { | ||
| cnt++; | ||
| if constexpr (std::is_same_v<std::remove_cv_t<std::remove_reference_t<Ts>>, CScript>) { | ||
| // If it is a CScript, extend ret with it. Move or copy the first element instead. | ||
| if (cnt == 0) { | ||
| ret = std::forward<Ts>(input); | ||
| } else { | ||
| ret.insert(ret.end(), input.begin(), input.end()); | ||
| } | ||
| } else { | ||
| // Otherwise invoke CScript::operator<<. | ||
| ret << input; | ||
| } | ||
| } (std::forward<Ts>(inputs)), ...); | ||
|
|
||
| return ret; | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Verify the counter initialization in BuildScript.
The cnt variable is initialized to 0 but the condition if (cnt == 0) on line 560 will never be true because cnt is incremented to 1 before the check. This appears to be a logic error.
Generate a verification script to confirm the intended behavior:
🏁 Script executed:
#!/bin/bash
# Description: Search for usage patterns of BuildScript to understand intended behavior
# Expected: Find call sites that demonstrate whether the counter logic is correct
# Search for BuildScript usage
rg -n -C 5 'BuildScript\s*\(' --type=cpp
# Search for related test cases
rg -n -C 3 'BuildScript' src/test/ --type=cppLength of output: 7449
Correct first-element handling in BuildScript
Incrementing cnt before if(cnt == 0) makes the initial branch unreachable; change to if(cnt == 1) or move the increment after the check.
🤖 Prompt for AI Agents
In src/script/script.h around lines 549 to 573, the lambda increments cnt before
checking if it's the first element so the cnt==0 branch is never taken; adjust
logic so the first-element branch runs correctly by either moving the increment
to after the check or changing the comparison to if(cnt == 1). Ensure you still
use perfect forwarding for the first-element assignment (ret =
std::forward<Ts>(input)) and keep the subsequent-element behavior (insert or
operator<<) unchanged.
knst
changed the title
knst
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/script/miniscript.h (1)
8-26: Add the missing standard headers.This header uses
std::logic_error(Line 175) andstd::tuple(Line 892), but neither<stdexcept>nor<tuple>is included. This will cause compilation errors in C++20 with Clang 16/GCC 11.1 as per the coding guidelines.Apply this diff to add the required headers:
#include <optional> #include <string> +#include <stdexcept> +#include <tuple> #include <vector> #include <stdlib.h>Based on past review comments.
🧹 Nitpick comments (4)
src/script/script.cpp (1)
386-409: Simplify the redundant condition in the assert.The condition
0 <= opcodeis always true becauseopcodeis of typeopcodetype, which is an unsigned integer. This part of the assertion is redundant and can be removed for clarity.Apply this diff to simplify the assertion:
- assert(0 <= opcode && opcode <= OP_PUSHDATA4); + assert(opcode <= OP_PUSHDATA4);src/wallet/rpc/coins.cpp (1)
74-74: Consider usingstd::setfor faster lookups.The linear search with
std::findinside nested loops could become slow for large labels or wallets with many transactions. Usingstd::set<CScript>foroutput_scriptswould reduce lookup complexity from O(n) to O(log n).Apply this diff to use
std::set:- std::vector<CScript> output_scripts; + std::set<CScript> output_scripts;And update line 74:
- if (std::find(output_scripts.begin(), output_scripts.end(), txout.scriptPubKey) != output_scripts.end()) { + if (output_scripts.count(txout.scriptPubKey)) { amount += txout.nValue; }src/test/miner_tests.cpp (1)
641-641: Verify necessity of direct height manipulation.Directly decrementing
nHeighton the active chain tip is unusual and potentially fragile. This manual state manipulation bypasses normal blockchain state management and could lead to inconsistencies.Consider whether this adjustment is necessary or if there's a cleaner approach using standard blockchain operations (e.g., proper invalidation/rollback). Since this is a backport from Bitcoin Core, the logic is likely correct, but it may be worth verifying that this state manipulation is intentional and necessary for the test to function properly.
Would you like me to investigate whether there's a cleaner alternative to this direct height manipulation?
src/validation.h (1)
183-183: Use consistent “virtual size” terminology
Update the comment in src/validation.h (line 183) to refer to “virtual size” instead of just “size,” e.g.:- /** Size as used by the mempool, calculated using serialized size and sigops. */ + /** Virtual size as used by the mempool, calculated using serialized size and sigops. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
ci/lint/04_install.sh(1 hunks)contrib/containers/ci/ci-slim.Dockerfile(2 hunks)doc/build-openbsd.md(1 hunks)doc/policy/packages.md(1 hunks)src/Makefile.am(2 hunks)src/Makefile.test.include(2 hunks)src/httprpc.cpp(2 hunks)src/masternode/node.h(1 hunks)src/node/chainstate.cpp(1 hunks)src/node/miner.h(1 hunks)src/rest.cpp(1 hunks)src/rpc/util.cpp(2 hunks)src/script/interpreter.cpp(0 hunks)src/script/interpreter.h(0 hunks)src/script/miniscript.cpp(1 hunks)src/script/miniscript.h(1 hunks)src/script/script.cpp(1 hunks)src/script/script.h(2 hunks)src/script/standard.cpp(0 hunks)src/script/standard.h(1 hunks)src/test/fuzz/miniscript_decode.cpp(1 hunks)src/test/fuzz/tx_pool.cpp(1 hunks)src/test/miner_tests.cpp(5 hunks)src/test/miniscript_tests.cpp(1 hunks)src/test/txpackage_tests.cpp(9 hunks)src/validation.cpp(12 hunks)src/validation.h(2 hunks)src/validationinterface.cpp(3 hunks)src/validationinterface.h(1 hunks)src/wallet/rpc/backup.cpp(2 hunks)src/wallet/rpc/coins.cpp(3 hunks)test/functional/rpc_users.py(1 hunks)
💤 Files with no reviewable changes (3)
- src/script/interpreter.cpp
- src/script/interpreter.h
- src/script/standard.cpp
✅ Files skipped from review due to trivial changes (1)
- src/node/chainstate.cpp
🚧 Files skipped from review as they are similar to previous changes (11)
- doc/build-openbsd.md
- src/rest.cpp
- src/script/standard.h
- src/rpc/util.cpp
- src/wallet/rpc/backup.cpp
- src/script/script.h
- src/Makefile.test.include
- src/validationinterface.cpp
- src/test/fuzz/tx_pool.cpp
- src/node/miner.h
- src/script/miniscript.cpp
🧰 Additional context used
📓 Path-based instructions (7)
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/rpc_users.py
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/test/txpackage_tests.cppsrc/test/fuzz/miniscript_decode.cppsrc/script/script.cppsrc/wallet/rpc/coins.cppsrc/test/miniscript_tests.cppsrc/validation.hsrc/validation.cppsrc/script/miniscript.hsrc/httprpc.cppsrc/masternode/node.hsrc/validationinterface.hsrc/test/miner_tests.cpp
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/txpackage_tests.cppsrc/test/fuzz/miniscript_decode.cppsrc/test/miniscript_tests.cppsrc/test/miner_tests.cpp
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/policy/packages.md
ci/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the ci directory (continuous integration)
Files:
ci/lint/04_install.sh
contrib/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the contrib directory (contributed scripts)
Files:
contrib/containers/ci/ci-slim.Dockerfile
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/masternode/node.h
🧬 Code graph analysis (9)
test/functional/rpc_users.py (1)
test/functional/test_framework/test_node.py (1)
assert_start_raises_init_error(606-649)
src/test/txpackage_tests.cpp (2)
src/test/util/setup_common.cpp (4)
mineBlocks(447-461)mineBlocks(447-447)CreateValidMempoolTransaction(544-591)CreateValidMempoolTransaction(544-550)src/validation.cpp (2)
ProcessNewPackage(1420-1438)ProcessNewPackage(1420-1421)
src/test/fuzz/miniscript_decode.cpp (1)
src/test/miniscript_tests.cpp (6)
key(75-75)key(75-75)key(78-83)key(78-78)FromString(87-91)FromString(87-87)
src/wallet/rpc/coins.cpp (3)
src/wallet/interfaces.cpp (6)
label(184-188)label(184-184)txout(459-463)txout(459-459)txout(469-473)txout(469-469)src/wallet/rpc/util.cpp (2)
LabelFromValue(117-123)LabelFromValue(117-117)src/wallet/scriptpubkeyman.h (1)
wallet(28-227)
src/test/miniscript_tests.cpp (2)
src/test/fuzz/miniscript_decode.cpp (6)
key(24-27)key(24-24)key(28-30)key(28-28)key(31-34)key(31-31)src/crypto/ripemd160.h (1)
CRIPEMD160(12-26)
src/validation.cpp (1)
src/validation.h (1)
state(169-194)
src/script/miniscript.h (1)
src/script/miniscript.cpp (12)
ComputeType(38-251)ComputeType(38-38)ComputeScriptLen(253-284)ComputeScriptLen(253-253)SanitizeType(16-36)SanitizeType(16-16)FindNextChar(338-346)FindNextChar(338-338)DecomposeScript(286-321)DecomposeScript(286-286)ParseScriptNumber(323-336)ParseScriptNumber(323-323)
src/httprpc.cpp (1)
src/bitcoin-cli.cpp (1)
gArgs(445-451)
src/validationinterface.h (1)
src/validationinterface.cpp (2)
MainSignalsImpl(47-47)MainSignalsImpl(47-47)
🔇 Additional comments (20)
src/Makefile.am (1)
348-348: LGTM!The Miniscript header and source files are correctly integrated into the build system.
Also applies to: 926-926
src/test/fuzz/miniscript_decode.cpp (1)
48-51: Intentionally limited FromPKHBytes implementation.The
FromPKHBytesmethod asserts the input is 20 bytes but always returnsfalse, indicating this conversion path is intentionally unsupported in this fuzz test. This is acceptable for testing round-trips throughFromScriptandToString, as the test focuses on other conversion paths.src/test/miniscript_tests.cpp (1)
148-291: LGTM! Comprehensive test coverage.The test suite thoroughly exercises:
- Type system validation and constraints
- Script round-tripping through Miniscript parsing
- Malleability properties
- Timelock semantics
- Threshold and multi-signature constructs
- Edge cases like non-minimal pushes
The singleton test data pattern with proper cleanup (line 290) ensures test isolation.
src/wallet/rpc/coins.cpp (1)
20-42: LGTM! Script-based filtering is more robust.The refactor from destination-based to script-based filtering is sound:
- Directly matches against
txout.scriptPubKeywithout intermediate conversions- Handles non-standard scripts that may not have valid destinations
- Preserves wallet ownership validation in both paths
src/test/miner_tests.cpp (3)
40-41: LGTM! Clean extraction of test helper methods.The new method declarations properly capture the extracted test logic with appropriate lock annotations. The
baseheightparameter inTestBasicMiningcorrectly propagates the block height context needed for sequence lock testing.
210-482: LGTM! Comprehensive basic mining test extraction.The extracted
TestBasicMininghelper correctly tests various block creation scenarios including sigops limits, size constraints, orphan handling, double spends, and all lock time variations (relative height, relative time, absolute height, absolute time). Thebaseheightparameter is properly used for calculating sequence lock prevheights.
484-557: LGTM! Well-structured prioritisation test extraction.The extracted
TestPrioritisedMininghelper thoroughly tests transaction prioritisation, including edge cases where prioritised transactions have prioritised ancestors. The test correctly verifies that parent prioritisation doesn't incorrectly propagate to descendants when calculating ancestor fees.src/validation.h (2)
234-237: LGTM!The new
m_package_feeratefield is well-designed with clear documentation. Usingstd::optional<CFeeRate>appropriately models the case where package feerate may be unavailable (e.g., missing inputs or early termination).
243-245: LGTM!The new constructor correctly initializes the
m_package_feeratefield alongside existing fields. The parameter order and initialization are consistent with the struct's design.src/test/txpackage_tests.cpp (11)
84-84: LGTM!The increased output amounts (499 COIN for parent, 498 COIN for child) result in higher total fees (~2 COIN) for the package, which aligns with the updated assertions in lines 106-108. This ensures the test package has sufficient fees for validation.
Also applies to: 93-93
106-108: LGTM!The test correctly validates that
m_package_feerateis present and equals the expected aggregated feerate computed from total fees (2 COIN) divided by total virtual size.
120-120: LGTM!Correctly validates that
m_package_feerateisstd::nulloptwhen validation fails due to oversized transaction. This aligns with the field's documented behavior.
241-241: LGTM!The assertions correctly verify that
m_package_feerateisstd::nulloptin policy violation scenarios (unrelated transactions, 3-generation package, missing parent), where package validation doesn't proceed to feerate computation.Also applies to: 283-283, 299-299
319-322: LGTM!The comment clearly explains why
m_package_feerateisstd::nulloptin this case: when both transactions have sufficient individual feerates, package-level validation is unnecessary. Good documentation of optimization behavior.
344-345: LGTM!Correctly validates that
m_package_feerateisstd::nulloptwhen transactions are already in the mempool (deduplicated case), as package validation is bypassed.
348-398: LGTM!The test setup and prioritisation scenario are well-structured:
- Lines 360-378: Creates a zero-fee parent and 1 COIN fee child
- Lines 383-398: Correctly tests that negative prioritisation (-1 COIN on parent) brings package feerate to 0, causing validation to fail with "package-fee-too-low"
The assertions properly validate both the failure state and the computed package feerate (0 sat/vbyte).
400-432: LGTM!The CPFP success case is correctly tested:
- Lines 405-432: After clearing prioritisation, the zero-fee parent + 1 COIN child package succeeds
- Assertions verify both transactions accepted with correct fees (0 for parent, COIN for child)
- Package feerate validation correctly computes expected value from total fees and sizes
- The check
expected_feerate.GetFeePerK() > 1000ensures the package meets minimum feerateGood validation of the core CPFP functionality.
434-470: LGTM!The package-fee-too-low scenario is well-tested:
- Lines 437-470: Creates a package paying only 200 satoshis total
- Child alone has >1000 sat/kvB feerate, but combined package falls below 1000 sat/kvB threshold
- Assertions correctly verify failure with "package-fee-too-low" and validate the computed package feerate
Good coverage of the edge case where individual transaction fees aren't sufficient for the entire package.
472-488: LGTM!The prioritisation-enabled-package scenario is correct:
- Lines 474-488: Adds 1 COIN prioritisation to child, making the previously-failing package succeed
- Correctly validates that modified fees (including prioritisation delta) are used in package feerate calculation
- Expected feerate computation includes the 1 COIN prioritisation:
CFeeRate(1 * COIN + 200, ...)Good test of prioritisation interaction with package validation.
490-537: LGTM!The rich-parent scenario correctly tests topology independence:
- Lines 495-537: Creates parent paying 1 COIN fee and zero-fee child
- Parent is accepted individually (sufficient feerate)
- Child is then validated as a "package of 1" and fails with zero feerate
- Package feerate reflects only the child's feerate (0 sat/vbyte)
This validates that parents can't unfairly subsidize children - each transaction is validated individually first, and only insufficient-feerate transactions trigger package validation. Good test case.
knst
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
doc/dash-conf.md (1)
66-74: Add language specifier to code block.The code block at lines 71-73 should specify
shellorbashas the language for proper syntax highlighting and to satisfy Markdown linting rules.Apply this diff to add the language specifier:
-``` +```shell # example copy command for linux user cp share/examples/dash.conf ~/.dashcore</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 22c416aba45672c9cf5e916fa3e6359bf6dfec12 and 6738517c8e736cc6d528aa71e9f9338fe44a037f. </details> <details> <summary>📒 Files selected for processing (7)</summary> * `contrib/debian/examples/dash.conf` (1 hunks) * `contrib/devtools/README.md` (1 hunks) * `contrib/devtools/gen-dash-conf.sh` (1 hunks) * `contrib/guix/libexec/build.sh` (1 hunks) * `doc/dash-conf.md` (1 hunks) * `doc/release-process.md` (1 hunks) * `src/init.cpp` (1 hunks) </details> <details> <summary>✅ Files skipped from review due to trivial changes (1)</summary> * src/init.cpp </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (2)</summary> <details> <summary>contrib/**</summary> **📄 CodeRabbit inference engine (CLAUDE.md)** > Unless specifically prompted, avoid making changes to the contrib directory (contributed scripts) Files: - `contrib/devtools/README.md` - `contrib/guix/libexec/build.sh` - `contrib/debian/examples/dash.conf` - `contrib/devtools/gen-dash-conf.sh` </details> <details> <summary>doc/**</summary> **📄 CodeRabbit inference engine (CLAUDE.md)** > Unless specifically prompted, avoid making changes to the doc directory (documentation) Files: - `doc/dash-conf.md` - `doc/release-process.md` </details> </details><details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>doc/dash-conf.md</summary> 71-71: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)</summary> * GitHub Check: Build container / Build container </details> <details> <summary>🔇 Additional comments (4)</summary><blockquote> <details> <summary>doc/release-process.md (1)</summary><blockquote> `4-6`: **LGTM!** The documentation updates correctly reflect the new workflow for generating dash.conf and appropriately reference the develop branch for translations, manpages, and configuration generation. </blockquote></details> <details> <summary>contrib/debian/examples/dash.conf (1)</summary><blockquote> `1-1`: **LGTM!** Replacing the static example with a placeholder aligns with the new dynamic generation workflow. The placeholder appropriately directs users to the generation script documentation. </blockquote></details> <details> <summary>contrib/devtools/gen-dash-conf.sh (1)</summary><blockquote> `1-80`: **LGTM with path fix required!** The script logic is well-structured with proper error checking, dirty build warnings, and comprehensive help text parsing. The sed/awk pipeline effectively transforms the `dashd --help` output into a commented configuration format, and the footer correctly includes network-specific section headers. However, this approval is contingent on fixing the critical path issue flagged in the previous comment. </blockquote></details> <details> <summary>contrib/devtools/README.md (1)</summary><blockquote> `93-106`: **Fix typo in script description.** Line 96 contains a typo: "dashd--help" should be "dashd --help" (missing space between command and flag). Apply this diff to fix the typo: ```diff -Generates a dash.conf file in `share/examples/` by parsing the output from `dashd--help`. This script is run during the +Generates a dash.conf file in `share/examples/` by parsing the output from `dashd --help`. This script is run during the release process to include a dash.conf with the release binaries and can also be run by users to generate a file locally.Likely an incorrect or invalid review comment.
contrib/guix/libexec/build.sh
Outdated
| # copy over the example dash.conf file. if contrib/devtools/gen-dash-conf.sh | ||
| # has not been run before buildling, this file will be a stub | ||
| cp "${DISTSRC}/share/examples/dash.conf" "${DISTNAME}/" |
There was a problem hiding this comment.
Fix typo in comment.
Line 359 contains a typo: "buildling" should be "building".
Apply this diff to fix the typo:
- # copy over the example dash.conf file. if contrib/devtools/gen-dash-conf.sh
- # has not been run before buildling, this file will be a stub
+ # copy over the example dash.conf file. if contrib/devtools/gen-dash-conf.sh
+ # has not been run before building, this file will be a stub
cp "${DISTSRC}/share/examples/dash.conf" "${DISTNAME}/"📝 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.
| # copy over the example dash.conf file. if contrib/devtools/gen-dash-conf.sh | |
| # has not been run before buildling, this file will be a stub | |
| cp "${DISTSRC}/share/examples/dash.conf" "${DISTNAME}/" | |
| # copy over the example dash.conf file. if contrib/devtools/gen-dash-conf.sh | |
| # has not been run before building, this file will be a stub | |
| cp "${DISTSRC}/share/examples/dash.conf" "${DISTNAME}/" |
🤖 Prompt for AI Agents
contrib/guix/libexec/build.sh around lines 358 to 360: the comment contains a
typo "buildling" — update the comment to "building" so the line reads "... has
not been run before building, this file will be a stub". Make only that spelling
fix in the comment and keep the surrounding code unchanged.
There was a problem hiding this comment.
it's backported typo, let's keep it to reduce conflicts in the future.
p.s. still not fixed in the master of bitcoin core
There was a problem hiding this comment.
@knst, understood! That makes sense—maintaining consistency with the upstream Bitcoin Core code to minimize merge conflicts is a practical approach, especially when the issue hasn't been fixed upstream yet.
✏️ Learnings added
Learnt from: knst
PR: dashpay/dash#6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.442Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/validation.cpp (1)
1222-1268: Package feerate still omits already‑accepted parentsWhen
AcceptPackage()individually admits a parent (or finds it already in the mempool), that tx is removed fromtxns_new; yetAcceptMultipleTransactions()still computesm_total_vsize/m_total_modified_feesonly from theworkspacesbuilt fromtxns_new. Consequently the package feerate check sees only the remaining child, so a standard CPFP (high-fee parent already present, low-fee child pending) is rejected as “package-fee-too-low”. The regression flagged earlier is therefore still present.Please carry the size/fee contributions of the accepted/already-present parents into the totals before calling
CheckFeeRate(e.g. accumulate them inAcceptPackage()and pass alongsidetxns_new, or extendworkspaceswith synthetic entries for those parents). Until those contributions are included, valid packages continue to fail admission.
♻️ Duplicate comments (3)
contrib/devtools/gen-dash-conf.sh (1)
11-12: Critical: Fix incorrect default output path.The default path for
SHARE_EXAMPLES_DIRis inconsistent with the documentation and the build process:
- This script generates to:
$TOPDIR/contrib/debian/examples/(line 11 default)- Documentation references:
share/examples/dash.conf(doc/dash-conf.md line 68)- Build script expects:
${DISTSRC}/share/examples/dash.conf(contrib/guix/libexec/build.sh line 360)This path mismatch will break the release process because the build script won't find the generated file at the expected location.
Apply this diff to fix the default path:
-SHARE_EXAMPLES_DIR=${SHARE_EXAMPLES_DIR:-$TOPDIR/contrib/debian/examples/} +SHARE_EXAMPLES_DIR=${SHARE_EXAMPLES_DIR:-$TOPDIR/share/examples/} EXAMPLE_CONF_FILE=${EXAMPLE_CONF_FILE:-$SHARE_EXAMPLES_DIR/dash.conf}src/script/miniscript.cpp (1)
5-12:<algorithm>include still missing
std::reverse(Line 319) is still used without including<algorithm>, so the TU will fail to build on standard-compliant compilers. Please add the header next to the other includes.src/script/miniscript.h (1)
8-26: Add the missing standard headers.This header uses
std::logic_error(Line 175) andstd::tuple(Line 892), but neither<stdexcept>nor<tuple>is included. Under C++20 with Clang 16/GCC 11, this leads to hard compilation errors (std::logic_errornot declared / incomplete type forstd::tuple).Apply this diff to add the explicit includes:
#include <optional> #include <string> +#include <stdexcept> +#include <tuple> #include <vector> #include <stdlib.h>
🧹 Nitpick comments (3)
test/functional/rpc_users.py (1)
109-111: LGTM! Comprehensive coverage of malformed rpcauth formats.The three new test cases properly validate rejection of distinct malformed
-rpcauthformats:
foo:bar:baz: too many colonsfoo$bar:baz: wrong delimiter between username and saltfoo$bar$baz: missing colon separator entirelyThese tests follow the existing pattern and align with the tightened parsing implemented in
src/httprpc.cpp.Optional: Consider adding inline comments above each assertion to document which malformed format is being tested, similar to line 104's comment for the empty case.
Apply this diff to add clarifying comments:
+ # Too many colons (three parts) self.nodes[0].assert_start_raises_init_error(expected_msg=init_error, extra_args=['-rpcauth=foo:bar:baz']) + # Wrong delimiter between username and salt self.nodes[0].assert_start_raises_init_error(expected_msg=init_error, extra_args=['-rpcauth=foo$bar:baz']) + # Missing colon separator self.nodes[0].assert_start_raises_init_error(expected_msg=init_error, extra_args=['-rpcauth=foo$bar$baz'])doc/dash-conf.md (1)
66-74: Add language identifier to the code block.The documentation clearly explains the new generation workflow. Consider adding a language identifier to the fenced code block for better syntax highlighting and accessibility.
Apply this diff:
-``` +```bash # example copy command for linux user cp share/examples/dash.conf ~/.dashcore</blockquote></details> <details> <summary>src/wallet/rpc/coins.cpp (1)</summary><blockquote> `20-20`: **Consider using `std::set<CScript>` for better lookup performance.** Using `std::vector<CScript>` results in O(n) lookup complexity when checking membership at line 74, which is executed for every output of every transaction in the wallet. For labels with many addresses or wallets with many transactions, this could cause noticeable slowdown. Apply this diff: ```diff - std::vector<CScript> output_scripts; + std::set<CScript> output_scripts;Then update line 28 and line 41 to use
insertinstead ofemplace_back:- output_scripts.emplace_back(output_script); + output_scripts.insert(output_script);- output_scripts.emplace_back(script_pub_key); + output_scripts.insert(script_pub_key);And update line 74:
- if (std::find(output_scripts.begin(), output_scripts.end(), txout.scriptPubKey) != output_scripts.end()) { + if (output_scripts.count(txout.scriptPubKey)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
ci/lint/04_install.sh(1 hunks)contrib/containers/ci/ci-slim.Dockerfile(2 hunks)contrib/debian/examples/dash.conf(1 hunks)contrib/devtools/README.md(1 hunks)contrib/devtools/gen-dash-conf.sh(1 hunks)contrib/guix/libexec/build.sh(1 hunks)doc/build-openbsd.md(1 hunks)doc/dash-conf.md(1 hunks)doc/policy/packages.md(1 hunks)doc/release-process.md(1 hunks)src/Makefile.am(2 hunks)src/Makefile.test.include(2 hunks)src/httprpc.cpp(2 hunks)src/init.cpp(1 hunks)src/masternode/node.h(1 hunks)src/node/chainstate.cpp(1 hunks)src/node/miner.h(1 hunks)src/rest.cpp(1 hunks)src/rpc/util.cpp(2 hunks)src/script/interpreter.cpp(0 hunks)src/script/interpreter.h(0 hunks)src/script/miniscript.cpp(1 hunks)src/script/miniscript.h(1 hunks)src/script/script.cpp(1 hunks)src/script/script.h(2 hunks)src/script/standard.cpp(0 hunks)src/script/standard.h(1 hunks)src/test/fuzz/miniscript_decode.cpp(1 hunks)src/test/fuzz/tx_pool.cpp(1 hunks)src/test/miner_tests.cpp(5 hunks)src/test/miniscript_tests.cpp(1 hunks)src/test/txpackage_tests.cpp(9 hunks)src/validation.cpp(12 hunks)src/validation.h(2 hunks)src/validationinterface.cpp(3 hunks)src/validationinterface.h(1 hunks)src/wallet/rpc/backup.cpp(2 hunks)src/wallet/rpc/coins.cpp(3 hunks)test/functional/rpc_users.py(1 hunks)
💤 Files with no reviewable changes (3)
- src/script/interpreter.h
- src/script/standard.cpp
- src/script/interpreter.cpp
✅ Files skipped from review due to trivial changes (1)
- src/wallet/rpc/backup.cpp
🚧 Files skipped from review as they are similar to previous changes (15)
- contrib/devtools/README.md
- src/Makefile.test.include
- src/test/fuzz/miniscript_decode.cpp
- src/script/standard.h
- src/script/script.cpp
- src/rest.cpp
- src/init.cpp
- src/masternode/node.h
- doc/build-openbsd.md
- src/validationinterface.h
- src/httprpc.cpp
- ci/lint/04_install.sh
- contrib/containers/ci/ci-slim.Dockerfile
- src/node/chainstate.cpp
- src/script/script.h
🧰 Additional context used
📓 Path-based instructions (5)
contrib/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the contrib directory (contributed scripts)
Files:
contrib/debian/examples/dash.confcontrib/guix/libexec/build.shcontrib/devtools/gen-dash-conf.sh
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/validationinterface.cppsrc/rpc/util.cppsrc/test/txpackage_tests.cppsrc/node/miner.hsrc/test/miner_tests.cppsrc/test/miniscript_tests.cppsrc/script/miniscript.cppsrc/test/fuzz/tx_pool.cppsrc/validation.cppsrc/script/miniscript.hsrc/validation.hsrc/wallet/rpc/coins.cpp
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/release-process.mddoc/dash-conf.mddoc/policy/packages.md
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/txpackage_tests.cppsrc/test/miner_tests.cppsrc/test/miniscript_tests.cppsrc/test/fuzz/tx_pool.cpp
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/rpc_users.py
🧠 Learnings (1)
📚 Learning: 2025-10-05T20:38:28.442Z
Learnt from: knst
PR: dashpay/dash#6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.442Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Applied to files:
contrib/guix/libexec/build.sh
🧬 Code graph analysis (8)
src/test/txpackage_tests.cpp (2)
src/test/util/setup_common.cpp (4)
mineBlocks(453-467)mineBlocks(453-453)CreateValidMempoolTransaction(550-597)CreateValidMempoolTransaction(550-556)src/validation.cpp (2)
ProcessNewPackage(1420-1438)ProcessNewPackage(1420-1421)
src/test/miniscript_tests.cpp (2)
src/test/fuzz/miniscript_decode.cpp (8)
key(24-27)key(24-24)key(28-30)key(28-28)key(31-34)key(31-31)FromString(37-41)FromString(37-37)src/crypto/ripemd160.h (1)
CRIPEMD160(12-26)
src/script/miniscript.cpp (3)
src/script/script.h (4)
DecodeOP_N(488-494)vector(330-361)CScriptNum(211-226)CScriptNum(230-240)src/script/standard.h (1)
IsPushdataOp(111-114)src/script/script.cpp (2)
CheckMinimalPush(386-409)CheckMinimalPush(386-386)
src/test/fuzz/tx_pool.cpp (1)
src/validation.cpp (2)
ProcessNewPackage(1420-1438)ProcessNewPackage(1420-1421)
src/validation.cpp (1)
src/validation.h (1)
state(169-194)
test/functional/rpc_users.py (1)
test/functional/test_framework/test_node.py (1)
assert_start_raises_init_error(606-649)
src/script/miniscript.h (2)
src/script/miniscript.cpp (12)
ComputeType(38-251)ComputeType(38-38)ComputeScriptLen(253-284)ComputeScriptLen(253-253)SanitizeType(16-36)SanitizeType(16-16)FindNextChar(338-346)FindNextChar(338-338)DecomposeScript(286-321)DecomposeScript(286-286)ParseScriptNumber(323-336)ParseScriptNumber(323-323)src/test/fuzz/miniscript_decode.cpp (8)
key(24-27)key(24-24)key(28-30)key(28-28)key(31-34)key(31-31)FromString(37-41)FromString(37-37)
src/wallet/rpc/coins.cpp (2)
src/wallet/rpc/util.cpp (2)
LabelFromValue(117-123)LabelFromValue(117-117)src/wallet/scriptpubkeyman.h (1)
wallet(28-241)
🪛 markdownlint-cli2 (0.18.1)
doc/dash-conf.md
71-71: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
doc/policy/packages.md
71-71: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build container / Build container
🔇 Additional comments (24)
src/rpc/util.cpp (2)
379-379: LGTM!The named-argument comment
/*oneline=*/falseimproves code readability by clarifying the boolean parameter's purpose without changing functionality.
582-582: LGTM!The named-argument comment
/*oneline=*/trueimproves code readability by clarifying the boolean parameter's purpose without changing functionality.doc/release-process.md (1)
4-6: LGTM!The updated branch references (from master to develop) and the new dash.conf checklist item align with the PR objectives and the new configuration generation workflow.
contrib/debian/examples/dash.conf (1)
1-1: LGTM!Replacing the static example with a placeholder that references the generation script is a reasonable approach for maintaining an up-to-date configuration example.
contrib/guix/libexec/build.sh (1)
358-360: Verify the source path matches the generation script output.The copy command expects the dash.conf at
${DISTSRC}/share/examples/dash.conf. However, the generation scriptgen-dash-conf.shhas a default path that points tocontrib/debian/examples/(see my comment on that file). Ensure the generation script is updated to output to the correct location, or this copy will fail during the build process.Based on learnings: keeping the typo "buildling" to reduce merge conflicts with upstream Bitcoin Core.
src/test/miner_tests.cpp (3)
40-41: LGTM!The new helper function declarations are well-designed with appropriate lock annotations and parameters. The
baseheightparameter inTestBasicMiningis correctly threaded through to support relative locktime tests.
210-482: LGTM!The
TestBasicMininghelper effectively consolidates mining test scenarios (sigops limits, block size, orphans, fee rates, and various locktime conditions) into a cohesive function. Thebaseheightparameter is correctly utilized for relative locktime calculations.
484-557: LGTM!The
TestPrioritisedMininghelper correctly tests transaction prioritisation logic, including edge cases with ancestor fee calculations and de-prioritised transactions.src/wallet/rpc/coins.cpp (3)
22-30: LGTM! Correct filtering logic.The by_label path correctly iterates through addresses for the label, converts them to scripts, and filters by wallet ownership using
IsMine(). This properly handles watch-only or other non-owned addresses.
31-42: LGTM! Proper validation and error handling.The single-address path correctly validates the destination early, converts it to a script, checks wallet ownership, and provides clear error messages for invalid addresses (
RPC_INVALID_ADDRESS_OR_KEY) and non-owned addresses (RPC_WALLET_ERROR).
74-74: Logic improvement: filtering by scriptPubKey is more accurate.Switching from destination-based filtering to scriptPubKey-based filtering is a positive change, as it correctly handles scriptPubKeys that cannot be converted to destinations. The logic is correct.
Note: The performance concern with
std::findon a vector has been flagged at line 20.src/node/miner.h (1)
138-138: LGTM: Modified fee correctly used for parent inclusion accounting.The change from
iter->GetFee()toiter->GetModifiedFee()ensures prioritisation deltas are included when adjusting ancestor fee totals during package assembly.src/test/fuzz/tx_pool.cpp (1)
236-247: LGTM: Conditional validation correctly handles PCKG_POLICY state.The added conditional check prevents accessing
m_tx_resultswhen the package state isPCKG_POLICY, which can occur when package-specific policy violations prevent individual transaction validation. This correctly aligns with the package validation flow.src/validationinterface.cpp (1)
21-99: LGTM: Type rename is consistent and does not affect behavior.The renaming of
MainSignalsInstancetoMainSignalsImplis a pure refactoring change with no functional impact. All references are updated consistently.src/Makefile.am (2)
348-348: LGTM: Miniscript header correctly added to public API.The addition of
script/miniscript.hto the exported headers list enables Miniscript front-end functionality for the codebase.
926-926: LGTM: Miniscript implementation correctly added to common library.The addition of
script/miniscript.cpptolibbitcoin_common_a_SOURCESensures the Miniscript implementation is available to both dashd and dash-qt.doc/policy/packages.md (1)
64-98: LGTM: Package fees documentation is comprehensive and accurate.The new "Package Fees and Feerate" section clearly explains:
- Package feerate calculation (excluding deduplicated mempool transactions)
- How package feerate satisfies fee requirements
- CPFP semantics and rationales
- Implementation details about individual-first validation
The documentation aligns well with the code changes throughout the PR.
src/test/txpackage_tests.cpp (4)
84-108: LGTM: Updated amounts and package feerate validation.The increased output amounts (499/498 COIN) ensure sufficient fees for individual transaction validation, and the new assertions correctly verify that
m_package_feerateis present with the expected value.
120-120: LGTM: Correct nullopt assertion for oversized transaction.Single oversized transactions fail validation early without computing package feerate, so
std::nulloptis the correct expectation.
241-344: LGTM: Appropriate nullopt checks for non-package scenarios.The assertions correctly verify that
m_package_feerateisstd::nulloptwhen:
- Package structure is invalid (unrelated transactions, missing parents, etc.)
- Individual transactions pass validation without requiring package-level checks
348-537: LGTM: Comprehensive CPFP test coverage.The new
package_cpfp_testssuite thoroughly validates:
- Zero-fee parent with fee-paying child
- Package feerate calculation with prioritisation deltas
- Low-feerate package rejection
- High-fee parent with low-fee child (parent-only acceptance)
The tests correctly exercise package-fee policy semantics and edge cases.
src/validation.h (2)
183-183: Minor documentation clarification: "Size" vs "Virtual size".The comment change from "Virtual size" to "Size" is a minor clarification. The field continues to represent the same mempool-used size metric.
234-245: LGTM: Package feerate field and constructor correctly added.The new
m_package_feeratefield and constructor enable package-level feerate to be propagated through validation results. The field documentation clearly explains when it may be unavailable, and the addition is backward compatible.src/test/miniscript_tests.cpp (1)
1-291: LGTM: Comprehensive and well-structured Miniscript test suite.The new test file provides excellent coverage of Miniscript functionality:
Test infrastructure:
TestDataprecomputes necessary test data (pubkeys, hashes)KeyConverterhandles CPubKey conversions for Miniscript integrationTesthelper validates properties: IsValid, IsValidTopLevel, malleability, signature requirements, timelock semantics, script roundtripsTest coverage:
- Validity rules for wrappers and fragments (andor, and_v, or_b, etc.)
- Threshold edge cases (k=0, k=n, k>n)
- Timelock mixing detection
- Non-minimal push and VERIFY detection
- Ops and stack size limits
- Extensive randomized type/node combinations
The test suite properly initializes and cleans up global state.
| const uint32_t k = 0; | ||
| //! The keys used by this expression (only for PK_K/PK_H/MULTI) | ||
| const std::vector<Key> keys; | ||
| //! The data bytes in this expression (only for HASH160/HASH256/SHA256/RIPEMD10). |
There was a problem hiding this comment.
Fix the typo in the comment.
"RIPEMD10" should be "RIPEMD160".
Apply this diff:
- //! The data bytes in this expression (only for HASH160/HASH256/SHA256/RIPEMD10).
+ //! The data bytes in this expression (only for HASH160/HASH256/SHA256/RIPEMD160).📝 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.
| //! The data bytes in this expression (only for HASH160/HASH256/SHA256/RIPEMD10). | |
| //! The data bytes in this expression (only for HASH160/HASH256/SHA256/RIPEMD160). |
🤖 Prompt for AI Agents
In src/script/miniscript.h around line 287, the comment contains a typo:
"RIPEMD10" should be "RIPEMD160"; update the comment to read "RIPEMD160" so it
correctly lists HASH160/HASH256/SHA256/RIPEMD160 as the supported hash types.
| LOCK2(cs_main, m_node.mempool->cs); | ||
| TestPackageSelection(chainparams, scriptPubKey, txFirst); | ||
|
|
||
| m_node.chainman->ActiveChain().Tip()->nHeight--; |
There was a problem hiding this comment.
Direct chain state manipulation bypasses normal APIs and risks corruption.
Directly decrementing nHeight on the chain tip bypasses proper chain management (e.g., InvalidateBlock) and can leave the blockchain in an inconsistent state. This could break invariants in CChainState, corrupt metadata like cumulative work, or cause subtle test failures.
Consider using InvalidateBlock to properly rewind the chain, or investigate why TestPackageSelection might be advancing the height and prevent that if unintended. If this decrement is a workaround for test state management, document why normal APIs cannot be used and what invariants are being maintained.
🤖 Prompt for AI Agents
In src/test/miner_tests.cpp around line 641, directly decrementing
m_node.chainman->ActiveChain().Tip()->nHeight bypasses chain management APIs and
can corrupt chain state; replace this direct mutation by rewinding via the
proper API (call InvalidateBlock on the current tip or use
CChainState::DisconnectBlock/DeactivateBestChain as appropriate) so the chain
tip is rolled back cleanly, or modify the test to avoid advancing the height in
the first place; if you cannot use normal APIs for test isolation, add a clear
comment explaining why and document the invariants preserved.
doc/build-openbsd.md
Outdated
| pkg_add bash git gmake libevent libtool boost | ||
| # Select the newest version of the following packages: | ||
| pkg_add autoconf automake python |
There was a problem hiding this comment.
24796: this should be reverted
EDIT: typo fix should be applied after 24652 from #6874
|
This pull request has conflicts, please rebase. |
…a class, drop unused forward declarations ca1ac1f scripted-diff: Rename MainSignalsInstance() class to MainSignalsImpl() (Jon Atack) 2aaec23 refactor: remove unused forward declarations in validationinterface.h (Jon Atack) 23854f8 refactor: make MainSignalsInstance() a class (Jon Atack) Pull request description: Make MainSignalsInstance a class, rename it to MainSignalsImpl, use Doxygen documentation for it, and remove no longer used forward declarations in src/validationinterface.h. ---- MainSignalsInstance was created in 3a19fed and originally was a collection of boost::signals methods moved to validationinterface.cpp, in order to no longer need to include boost/signals in validationinterface.h. MainSignalsInstance then evolved in d6815a2 to become class-like: - [C.8: Use class rather than struct if any member is non-public](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-class) - [C.2: Use class if the class has an invariant; use struct if the data members can vary independently](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c2-use-class-if-the-class-has-an-invariant-use-struct-if-the-data-members-can-vary-independently) - A class has the advantage of default private access, as opposed to public for a struct. ACKs for top commit: furszy: Code ACK ca1ac1f promag: Code review ACK ca1ac1f. danielabrozzoni: Code review ACK ca1ac1f Tree-SHA512: 25f85e2b582fe8c269e6cf384a4aa29350b97ea6477edf3c63591e4f68a97364f7fb2fc4ad2764f61ff86b27353e31d2f12eed7a23368a247e4cf271c7e133ea
…ormance f336ff7 rpc: avoid expensive `IsMine` calls in `GetReceived` tally (Sebastian Falbesoner) a7b65af rpc: avoid scriptPubKey<->CTxDestination conversions in `GetReceived` tally (Sebastian Falbesoner) Pull request description: The RPC calls `getreceivedbyaddress`/`getreceivedbylabel` both use the internal helper function `GetReceived` which was introduced in PR bitcoin#17579 to deduplicate tallying code. For every wallet-related transaction output, the following unnecessary operations are currently performed in the tally loop, leading to a quite bad performance (as reported in bitcoin#23645): - converting from CScript -> TxDestination (`ExtractDestination(...)`), converting from TxDestination -> CScript (`CWallet::IsMine(const CTxDestination& dest)`); this can be avoided by directly using output scripts in the search set instead of addresses (first commit) - checking if the iterated output script belongs to the wallet by calling `IsMine`; this can be avoided by only adding addresses to the search set which fulfil `IsMine` in the first place (second commit) ### Benchmark results The functional test [wallet_pr23662.py](https://github.com/theStack/bitcoin/blob/pr23662_benchmarks/test/functional/wallet_pr23662.py) (not part of this PR) creates transactions with 15000 different addresses: - 5000 outputs (500 txs with 10 outputs each) with label set, IsMine set (received) - 5000 outputs (500 txs with 10 outputs each) with label set, IsMine not set (sent) - 5000 outputs (500 txs with 10 outputs each) without label set, IsMine not set (sent) Then, the time is measured for calling `getreceivedbyaddress` and `getreceivedbylabel`, the latter with two variants. Results on my machine: | branch | `getreceivedbyaddress` (single) | `getreceivedbylabel` (single) | `getreceivedbylabel` (10000) | |--------------------|---------------------------------|-------------------------------|------------------------------| | master | 406.13ms | 425.33ms | 446.58ms | | PR (first commit) | 367.18ms | 365.81ms | 426.33ms | | PR (second commit) | 3.96ms | 4.83ms | 339.69ms | Fixes bitcoin#23645. ACKs for top commit: achow101: ACK f336ff7 w0xlt: ACK bitcoin@f336ff7 furszy: Code ACK f336ff7 Tree-SHA512: 9cbf402b9e269713bd3feda9e31719d9dca8a0dfd526de12fd3d561711589195d0c50143432c65dae279c4eab90a4fc3f99e29fbc0452fefe05113e92d129b8f
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
src/test/miner_tests.cpp (1)
641-641: Don’t mutate nHeight directly; rewind via chainstate APIs.Directly decrementing ActiveChain().Tip()->nHeight risks corrupting chain state and violates invariants. Use InvalidateBlock (as you do above) to roll back cleanly.
- m_node.chainman->ActiveChain().Tip()->nHeight--; + { + BlockValidationState rewind_state; + m_node.chainman->ActiveChainstate().InvalidateBlock( + rewind_state, + WITH_LOCK(cs_main, return m_node.chainman->ActiveChain().Tip()) + ); + BOOST_CHECK(rewind_state.IsValid()); + }src/script/miniscript.h (2)
287-288: Fix typo in comment: RIPEMD160 (not RIPEMD10).Update the comment to avoid confusion.
- //! The data bytes in this expression (only for HASH160/HASH256/SHA256/RIPEMD10). + //! The data bytes in this expression (only for HASH160/HASH256/SHA256/RIPEMD160).
8-17: Add missing standard headers to fix compile errors.This header uses std::logic_error and std::tuple but doesn’t include or . Add them explicitly.
#include <memory> #include <optional> #include <string> #include <vector> +#include <stdexcept> +#include <tuple> + #include <stdlib.h> #include <assert.h>src/validation.cpp (1)
1226-1238: Package feerate still drops already-admitted parents
m_total_vsize/m_total_modified_feesonly sumworkspaces, butAcceptPackage()has already filtered out any parent tx that made it into the mempool (or got accepted individually just above). The new package-feerate guard therefore evaluates the remaining child(ren) in isolation, so a normal CPFP pair (high-fee parent already in, low-fee child under test) still failsCheckFeeRate(...). This regresses package relay exactly like the earlier report.Please carry the accepted parents’ vsize/modified-fee contributions into the totals before calling
CheckFeeRate(e.g., track cumulative parent metrics inAcceptPackage()and add them here, or extendworkspaceswith synthetic entries).
🧹 Nitpick comments (4)
src/wallet/rpc/coins.cpp (1)
74-74: Consider usingstd::setfor better lookup performance.The
std::findcall on a vector has O(n) complexity and executes inside nested loops (once per output in every wallet transaction). For users with many addresses or large wallets, this could impact performance noticeably.Apply this diff to improve lookup performance:
- std::vector<CScript> output_scripts; + std::set<CScript> output_scripts;Then update insertions to use
insertinstead ofemplace_back:- output_scripts.emplace_back(output_script); + output_scripts.insert(output_script);- output_scripts.emplace_back(script_pub_key); + output_scripts.insert(script_pub_key);And replace the
std::findcheck with a direct membership test:- if (std::find(output_scripts.begin(), output_scripts.end(), txout.scriptPubKey) != output_scripts.end()) { + if (output_scripts.count(txout.scriptPubKey)) {doc/policy/packages.md (1)
64-87: Clarify deduplication scope in the definition.Optional: explicitly say “exclude duplicate transactions already in mempool from both numerator (fees) and denominator (sizes)” to make the calculation unambiguous.
src/test/txpackage_tests.cpp (1)
316-344: Optional: lock mempool for read-heavy checks.To avoid future TSAN noise, consider guarding mempool->size()/exists() reads with mempool->cs.
- BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); - BOOST_CHECK(m_node.mempool->exists(tx_parent->GetHash())); - BOOST_CHECK(m_node.mempool->exists(tx_child->GetHash())); + WITH_LOCK(m_node.mempool->cs, { + BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); + BOOST_CHECK(m_node.mempool->exists(tx_parent->GetHash())); + BOOST_CHECK(m_node.mempool->exists(tx_child->GetHash())); + });Also applies to: 340-344
src/test/miniscript_tests.cpp (1)
6-9: Include and to avoid transitive-include dependency.Prevents brittle builds if indirect includes change.
-#include <string> +#include <string> +#include <map> +#include <memory>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
ci/lint/04_install.sh(1 hunks)contrib/containers/ci/ci-slim.Dockerfile(2 hunks)contrib/debian/examples/dash.conf(1 hunks)contrib/devtools/README.md(1 hunks)contrib/devtools/gen-dash-conf.sh(1 hunks)contrib/guix/libexec/build.sh(1 hunks)doc/build-openbsd.md(1 hunks)doc/dash-conf.md(1 hunks)doc/policy/packages.md(1 hunks)doc/release-process.md(1 hunks)src/Makefile.am(2 hunks)src/Makefile.test.include(2 hunks)src/httprpc.cpp(2 hunks)src/init.cpp(1 hunks)src/masternode/node.h(1 hunks)src/node/chainstate.cpp(1 hunks)src/node/miner.h(1 hunks)src/rest.cpp(1 hunks)src/rpc/util.cpp(2 hunks)src/script/interpreter.cpp(0 hunks)src/script/interpreter.h(0 hunks)src/script/miniscript.cpp(1 hunks)src/script/miniscript.h(1 hunks)src/script/script.cpp(1 hunks)src/script/script.h(2 hunks)src/script/standard.cpp(0 hunks)src/script/standard.h(1 hunks)src/test/fuzz/miniscript_decode.cpp(1 hunks)src/test/fuzz/tx_pool.cpp(1 hunks)src/test/miner_tests.cpp(5 hunks)src/test/miniscript_tests.cpp(1 hunks)src/test/txpackage_tests.cpp(9 hunks)src/validation.cpp(12 hunks)src/validation.h(2 hunks)src/validationinterface.cpp(3 hunks)src/validationinterface.h(1 hunks)src/wallet/rpc/backup.cpp(2 hunks)src/wallet/rpc/coins.cpp(3 hunks)test/functional/rpc_users.py(1 hunks)
💤 Files with no reviewable changes (3)
- src/script/interpreter.h
- src/script/interpreter.cpp
- src/script/standard.cpp
✅ Files skipped from review due to trivial changes (1)
- src/masternode/node.h
🚧 Files skipped from review as they are similar to previous changes (24)
- doc/build-openbsd.md
- doc/release-process.md
- contrib/debian/examples/dash.conf
- test/functional/rpc_users.py
- src/test/fuzz/tx_pool.cpp
- src/node/miner.h
- src/Makefile.am
- contrib/containers/ci/ci-slim.Dockerfile
- src/node/chainstate.cpp
- src/validationinterface.h
- src/rest.cpp
- src/script/script.h
- src/script/script.cpp
- src/script/standard.h
- src/test/fuzz/miniscript_decode.cpp
- ci/lint/04_install.sh
- contrib/guix/libexec/build.sh
- src/init.cpp
- src/script/miniscript.cpp
- contrib/devtools/gen-dash-conf.sh
- src/validationinterface.cpp
- src/Makefile.test.include
- contrib/devtools/README.md
- src/validation.h
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/wallet/rpc/backup.cppsrc/test/txpackage_tests.cppsrc/httprpc.cppsrc/test/miner_tests.cppsrc/wallet/rpc/coins.cppsrc/test/miniscript_tests.cppsrc/rpc/util.cppsrc/validation.cppsrc/script/miniscript.h
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/policy/packages.mddoc/dash-conf.md
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/txpackage_tests.cppsrc/test/miner_tests.cppsrc/test/miniscript_tests.cpp
🧬 Code graph analysis (6)
src/test/txpackage_tests.cpp (2)
src/test/util/setup_common.cpp (4)
mineBlocks(453-467)mineBlocks(453-453)CreateValidMempoolTransaction(550-597)CreateValidMempoolTransaction(550-556)src/validation.cpp (2)
ProcessNewPackage(1424-1442)ProcessNewPackage(1424-1425)
src/httprpc.cpp (1)
src/bitcoin-cli.cpp (1)
gArgs(445-451)
src/wallet/rpc/coins.cpp (3)
src/wallet/interfaces.cpp (6)
label(184-188)label(184-184)txout(459-463)txout(459-459)txout(469-473)txout(469-469)src/wallet/rpc/util.cpp (2)
LabelFromValue(117-123)LabelFromValue(117-117)src/wallet/scriptpubkeyman.h (1)
wallet(28-241)
src/test/miniscript_tests.cpp (2)
src/test/fuzz/miniscript_decode.cpp (8)
key(24-27)key(24-24)key(28-30)key(28-28)key(31-34)key(31-31)FromString(37-41)FromString(37-37)src/crypto/ripemd160.h (1)
CRIPEMD160(12-26)
src/validation.cpp (1)
src/validation.h (1)
state(169-194)
src/script/miniscript.h (1)
src/script/miniscript.cpp (12)
ComputeType(38-251)ComputeType(38-38)ComputeScriptLen(253-284)ComputeScriptLen(253-253)SanitizeType(16-36)SanitizeType(16-16)FindNextChar(338-346)FindNextChar(338-338)DecomposeScript(286-321)DecomposeScript(286-286)ParseScriptNumber(323-336)ParseScriptNumber(323-323)
🪛 LanguageTool
doc/policy/packages.md
[grammar] ~72-~72: Ensure spelling is correct
Context: ...ransactions are allowed to be below the feerate requirements if the package meets the fe...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~73-~73: Ensure spelling is correct
Context: ...e requirements if the package meets the feerate requirements. For example, the parent(s...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~84-~84: Ensure spelling is correct
Context: ...transactions that failed. Since package feerate is only calculated using transactions t...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~88-~88: Ensure spelling is correct
Context: .... Rationale: We must not allow a low-feerate child to prevent its parent from being ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~94-~94: Consider a more concise word here.
Context: ...o avoid accidentally restricting policy in order to be backward-compatible for users and ap...
(IN_ORDER_TO_PREMIUM)
🪛 markdownlint-cli2 (0.18.1)
doc/dash-conf.md
71-71: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (10)
doc/dash-conf.md (1)
66-73: Verify the output path for the generated dash.conf.Line 68 states the output is placed under
contrib/debian/examples/dash.conf, but the PR summary and AI-generated summary referenceshare/examples/dash.confas the intended output location. The path incontrib/debian/examples/appears specific to Debian packaging rather than the general build output location. Please verify which path is correct and update the documentation accordingly to avoid user confusion.src/wallet/rpc/backup.cpp (1)
1489-1489: LGTM! Formatting improvements for consistency.The changes update comment syntax from
/* oneline_description */to/*oneline_description=*/, adopting a named-argument style that improves code readability. These are non-functional changes with no impact on behavior.Also applies to: 1497-1497, 1823-1823
src/rpc/util.cpp (1)
378-378: LGTM! Improved parameter clarity.The changes add explicit named-parameter style comments (
/*oneline=*/) to boolean function arguments, making the code more self-documenting and easier to understand at the call site. These are non-functional improvements that enhance readability.Also applies to: 581-581
src/wallet/rpc/coins.cpp (1)
20-42: LGTM! Refactor from CTxDestination to CScript is sound.The refactor correctly shifts the filtering mechanism from destinations to scripts, which is a more direct representation of transaction outputs. Ownership validation via
wallet.IsMine()is properly performed in both the by_label and single-address paths before adding scripts to the collection.src/httprpc.cpp (1)
22-22: Include is correct and necessary.std::vector is used for g_rpcauth; include is appropriate.
src/test/miner_tests.cpp (1)
484-557: Prioritisation logic looks good; coverage is solid.Good use of PrioritiseTransaction and checks ensuring deprioritised entries stay out while prioritised ones are included.
src/test/txpackage_tests.cpp (3)
84-109: Package feerate expectation is correct.Parent (1 COIN fee) + child (1 COIN fee) => total 2 COIN over sum of vsizes. Assertion matches calculation.
348-433: CPFP package tests are well-constructed.Good coverage for negative/positive prioritisation impact, zero-fee parent with fee-paying child, and feerate assertions.
452-488: “Package fee too low” path validates policy correctly.Accurate use of per-tx feerate vs package feerate; assertions match expected thresholds.
src/test/miniscript_tests.cpp (1)
148-291: Miniscript round-trip and limits coverage LGTM.Good breadth: validity, non-malleability, timelock mixing, ops/stack limits, and non-minimality checks.
| ``` | ||
| # example copy command for linux user | ||
| cp contrib/debian/examples/dash.conf ~/.dashcore |
There was a problem hiding this comment.
Specify language for fenced code block.
The code block lacks a language identifier, which violates markdown linting rules (MD040). Add a language specifier (e.g., bash or sh) to the opening fence.
Apply this diff:
-```
+```bash
# example copy command for linux user
cp contrib/debian/examples/dash.conf ~/.dashcore
-```
+```🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
71-71: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In doc/dash-conf.md around lines 71 to 73, the fenced code block is missing a
language identifier which triggers MD040; update the opening backtick fence to
include a language (e.g., bash or sh) so the block reads with a language
specifier and leave the closing fence unchanged.
| if (gArgs.GetArg("-rpcauth", "") != "") { | ||
| LogPrintf("Using rpcauth authentication.\n"); |
There was a problem hiding this comment.
Use IsArgSet() instead of GetArg() to gate -rpcauth parsing.
GetArg("-rpcauth","") checks only the last value; if multiple -rpcauth entries are present and the last is empty, valid earlier entries are skipped. IsArgSet() correctly detects presence.
Apply this small change:
- if (gArgs.GetArg("-rpcauth", "") != "") {
+ if (gArgs.IsArgSet("-rpcauth")) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (gArgs.GetArg("-rpcauth", "") != "") { | |
| LogPrintf("Using rpcauth authentication.\n"); | |
| if (gArgs.IsArgSet("-rpcauth")) { | |
| LogPrintf("Using rpcauth authentication.\n"); |
🤖 Prompt for AI Agents
In src/httprpc.cpp around lines 290-291, the current check uses
GetArg("-rpcauth","") to decide whether to parse rpcauth which misses earlier
entries if the last -rpcauth is empty; replace that guard with a presence check
using gArgs.IsArgSet("-rpcauth") so any -rpcauth occurrence triggers parsing.
Update the conditional to call IsArgSet and leave the subsequent parsing logic
unchanged.
| std::vector<std::string> fields{SplitString(rpcauth, ':')}; | ||
| const std::vector<std::string> salt_hmac{SplitString(fields.back(), '$')}; | ||
| if (fields.size() == 2 && salt_hmac.size() == 2) { | ||
| fields.pop_back(); | ||
| fields.insert(fields.end(), salt_hmac.begin(), salt_hmac.end()); |
There was a problem hiding this comment.
Fix potential crash: fields.back() accessed before validating size.
If an entry is empty or lacks “:”, SplitString can return an empty/single-element vector, and fields.back() is undefined behavior. Validate sizes first, then index, and reject empties.
Apply this safer parsing:
- for (const std::string& rpcauth : gArgs.GetArgs("-rpcauth")) {
- std::vector<std::string> fields{SplitString(rpcauth, ':')};
- const std::vector<std::string> salt_hmac{SplitString(fields.back(), '$')};
- if (fields.size() == 2 && salt_hmac.size() == 2) {
- fields.pop_back();
- fields.insert(fields.end(), salt_hmac.begin(), salt_hmac.end());
- g_rpcauth.push_back(fields);
- } else {
- LogPrintf("Invalid -rpcauth argument.\n");
- return false;
- }
- }
+ for (const std::string& rpcauth : gArgs.GetArgs("-rpcauth")) {
+ const std::vector<std::string> fields{SplitString(rpcauth, ':')};
+ if (fields.size() != 2) {
+ LogPrintf("Invalid -rpcauth argument (expected user:salt$hmac).\n");
+ return false;
+ }
+ const std::vector<std::string> salt_hmac{SplitString(fields[1], '$')};
+ if (salt_hmac.size() != 2 || fields[0].empty() || salt_hmac[0].empty() || salt_hmac[1].empty()) {
+ LogPrintf("Invalid -rpcauth argument (expected user:salt$hmac).\n");
+ return false;
+ }
+ g_rpcauth.push_back({fields[0], salt_hmac[0], salt_hmac[1]});
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/httprpc.cpp around lines 293-297, the code calls fields.back() before
verifying fields is non-empty which can crash; instead, after
SplitString(rpcauth, ':'), first check fields.empty() and reject/skip that
entry, then compute salt_hmac from fields.back(); additionally validate that
neither the username/parts nor the salt_hmac parts are empty (e.g. ensure
fields.size()==2 and !fields[0].empty() and salt_hmac.size()==2 and
!salt_hmac[0].empty() and !salt_hmac[1].empty()) before mutating fields
(pop_back/insert), otherwise treat the rpcauth as invalid.
| TestPrioritisedMining(chainparams, scriptPubKey, txFirst); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Acquire cs_main and mempool->cs before calling TestPrioritisedMining.
The function requires EXCLUSIVE_LOCKS_REQUIRED(::cs_main, m_node.mempool->cs) but is invoked without locks here. Wrap the call with LOCK2 to satisfy annotations and avoid races.
- TestPrioritisedMining(chainparams, scriptPubKey, txFirst);
+ {
+ LOCK2(cs_main, m_node.mempool->cs);
+ TestPrioritisedMining(chainparams, scriptPubKey, txFirst);
+ }🤖 Prompt for AI Agents
In src/test/miner_tests.cpp around lines 645-646, the call to
TestPrioritisedMining is missing the required locks; wrap the invocation in
LOCK2(::cs_main, m_node.mempool->cs) so cs_main and m_node.mempool->cs are held
before calling TestPrioritisedMining to satisfy EXCLUSIVE_LOCKS_REQUIRED
annotations and prevent races.
…kages BACKPORT NOTE: This PR is backported to minimize conflicts in future even if Dash Core has no bumpfee feature 9bebf35 [validation] don't package validate if not policy or missing inputs (glozow) 51edcff [unit test] package feerate and package cpfp (glozow) 1b93748 [validation] try individual validation before package validation (glozow) 17a8ffd [packages/policy] use package feerate in package validation (glozow) 09f32cf [docs] package feerate (glozow) Pull request description: Part of bitcoin#22290, aka [Package Mempool Accept](https://gist.github.com/glozow/dc4e9d5c5b14ade7cdfac40f43adb18a). This enables CPFP fee bumping in child-with-unconfirmed-parents packages by introducing [package feerate](https://gist.github.com/glozow/dc4e9d5c5b14ade7cdfac40f43adb18a#fee-related-checks-use-package-feerate) (total modified fees divided by total virtual size) and using it in place of individual feerate. We also always [validate individual transactions first](https://gist.github.com/glozow/dc4e9d5c5b14ade7cdfac40f43adb18a#always-try-individual-submission-first) to avoid incentive-incompatible policies like "parents pay for children" or "siblings pay for siblings" behavior. ACKs for top commit: instagibbs: reACK bitcoin@9bebf35 mzumsande: Code review ACK 9bebf35 t-bast: ACK bitcoin@9bebf35 Tree-SHA512: 5117cfcc3ce55c00384d9e8003a0589ceac1e6f738b1c299007d9cd9cdd2d7c530d31cfd23658b041a6604d39073bcc6e81f0639a300082a92097682a6ea8c8f
2da94a4 fuzz: add a fuzz target for Miniscript decoding from Script (Antoine Poinsot) f836999 Miniscript: ops limit and stack size computation (Pieter Wuille) 2e55e88 Miniscript: conversion from script (Pieter Wuille) 1ddaa66 Miniscript: type system, script creation, text notation, tests (Pieter Wuille) 4fe2936 script: expose getter for CScriptNum, add a BuildScript helper (Antoine Poinsot) f4e289f script: move CheckMinimalPush from interpreter to script.h (Antoine Poinsot) 31ec6ae script: make IsPushdataOp non-static (Antoine Poinsot) Pull request description: Miniscript is a language for writing (a subset of) Bitcoin Scripts in a structured way. Miniscript permits: - To safely extend the Output Descriptor language to many more scripting features thanks to the typing system (composition). - Statical analysis of spending conditions, maximum spending cost of each branch, security properties, third-party malleability. - General satisfaction of any correctly typed ("valid" [0]) Miniscript. The satisfaction itself is also analyzable. - To extend the possibilities of external signers, because of all of the above and since it carries enough metadata. Miniscript guarantees: - That for any statically-analyzed as "safe" [0] Script, a witness can be constructed in the bounds of the consensus and standardness rules (standardness complete). - That unless the conditions of the Miniscript are met, no witness can be created for the Script (consensus sound). - Third-party malleability protection for the satisfaction of a sane Miniscript, which is too complex to summarize here. For more details around Miniscript (including the specifications), please refer to the [website](https://bitcoin.sipa.be/miniscript/). Miniscript was designed by Pieter Wuille, Andrew Poelstra and Sanket Kanjalkar. This PR is an updated and rebased version of bitcoin#16800. See [the commit history of the Miniscript repository](https://github.com/sipa/miniscript/commits/master) for details about the changes made since September 2019 (TL;DR: bugfixes, introduction of timelock conflicts in the type system, `pk()` and `pkh()` aliases, `thresh_m` renamed to `multi`, all recursive algorithms were made non-recursive). This PR is also the first in a series of 3: - The first one (here) integrates the backbone of Miniscript. - The second one (bitcoin#24148) introduces support for Miniscript in Output Descriptors, allowing for watch-only support of Miniscript Descriptors in the wallet. - The third one (bitcoin#24149) implements signing for these Miniscript Descriptors, using Miniscript's satisfaction algorithm. Note to reviewers: - Miniscript is currently defined only for P2WSH. No Taproot yet. - Miniscript is different from the policy language (a high-level logical representation of a spending policy). A policy->Miniscript compiler is not included here. - The fuzz target included here is more interestingly extended in the 3rd PR to check a script's satisfaction against `VerifyScript`. I think it could be further improved by having custom mutators as we now have for multisig (see bitcoin#23105). A minified corpus of Miniscript Scripts is available at bitcoin-core/qa-assets#85. [0] We call "valid" any correctly-typed Miniscript. And "safe" any sane Miniscript, ie one whose satisfaction isn't malleable, which requires a key for any spending path, etc.. ACKs for top commit: jb55: ACK 2da94a4 laanwj: Light code review ACK 2da94a4 (mostly reviewed the changes to the existing code and build system) Tree-SHA512: d3ef558436cfcc699a50ad13caf1e776f7d0addddb433ee28ef38f66ea5c3e581382d8c748ccac9b51768e4b95712ed7a6112b0e3281a6551e0f325331de9167
…rmat ffffb7a doc: Convert remaining comments to clang-tidy format (MarcoFalke) Pull request description: This is a follow-up to commit 0da559e, hopefully the last one. ACKs for top commit: Empact: Code review ACK ffffb7a vincenzopalazzo: ACK bitcoin@ffffb7a Tree-SHA512: 541f5e69aeee53815e77e63b9012d3ed24856eec5f7e28cc764b3ce29f897563e56ade07db9891324f2809dcf064ce10333b1a1e4a7490af9bd97dcf6ff4e4f7
f87f259 refactor: fixup named args in txpackage tests (fanquake) 864772c lint: mypy 0.942 (fanquake) 38031ad lint: flake8 4.0.1 (fanquake) eaf712c lint: codespell 2.1.0 (fanquake) Pull request description: Use newer versions of our lint packages. Fix all the outstanding typos. Fix the failing CI. ACKs for top commit: MarcoFalke: review ACK f87f259 Tree-SHA512: 2c1471c13e088d5b9cdd436ebde40180516ae1dd2a2ea4d2f54214762a0712557d899e73c4cfd32992fe45fce6fdba50ba39254f2c79cd5d1180e873a79b9b22
5202bd1 test: Bump shellcheck version to 0.8.0 (Hennadii Stepanov) Pull request description: Among [added](https://github.com/koalaman/shellcheck/blob/master/CHANGELOG.md#v080---2021-11-06) rules, SC2295 could be [useful](bitcoin#23506 (comment)) for us. ACKs for top commit: dongcarl: Code Review ACK 5202bd1 fanquake: ACK 5202bd1 - would have rather this just been a part of bitcoin#23506 to avoid another PR and pointless rebasing. Tree-SHA512: fd7ff801c71af03c5a5b2823b7daba25a430b3ead5e5e50a3663961ee2223e55d322aec91d79999814cd35bd7ed6e9415a0b797718ceb8c0b1dbdbb40c336b82
7417594 miniscript: the 'd:' wrapper must not be 'u' (Antoine Poinsot) Pull request description: The type system was incorrectly relying on a standardness rule to be sound. This bug was found and reported by Andrew Poelstra [based on a question from Aman Kumar Kashyap](rust-bitcoin/rust-miniscript#341). ACKs for top commit: sipa: ACK 7417594 apoelstra: utACK 7417594 achow101: ACK 7417594 Tree-SHA512: af68c1df1c40e40dd105ef54544c226f560524dd8e35248fa0305dbef966e96ec1fa6ff2fe50fb8f2792ac310761a29c55ea81dd7b6d122a0de0a68b135e5aaa
…ing modified fees, not base e4303c3 [unit test] prioritisation in mining (glozow) 7a8d606 [miner] bug fix: update for parent inclusion using modified fee (glozow) 0f9a444 MOVEONLY: group miner tests into MinerTestingSetup functions (glozow) Pull request description: Came up while reviewing bitcoin#24364, where some of us incorrectly assumed that we use the same fee deduction in `CTxMemPoolModifiedEntry::nModFeesWithAncestors` when first constructing an entry and in `update_for_parent_inclusion`. Actually, the behavior is this: when a mempool entry's ancestor is included in the block template, we create a `CTxMemPoolModifiedEntry` for it, subtracting the ancestor's modified fees from `nModFeesWithAncestors`. If another ancestor is included, we update it again, but use the ancestor's _base_ fees instead. I can't explain why we use `GetFee` in one place and `GetModifiedFee` in the other, but I'm quite certain we should be using the same one for both. And should it be base or modified fees? Modified, otherwise the child inherits the prioritisation of the parent, but only until the parent gets mined. If we want prioritisation to cascade down to current in-mempool descendants, we should probably document that in the `prioritsetransaction` helpstring and implement it in `CTxMemPool::mapDeltas`, not as a quirk in the mining code? Wrote a test in which a mempool entry has 2 ancestors, both prioritised, and both included in a block template individually. This test should fail without the s/GetFee/GetModifiedFee commit. ACKs for top commit: ccdle12: tested ACK e4303c3 MarcoFalke: ACK e4303c3 🚗 Tree-SHA512: 4cd94106fbc9353e9f9b6d5af268ecda5aec7539245298c940ca220606dd0737264505bfaae1f83d94765cc2d9e1a6e913a765048fe6c19292482241761a6762
fa12706 Reject invalid rpcauth formats (MacroFake) Pull request description: This was added in commit 438ee59, but I couldn't determine if it was intentional. One reason to accept `foo:bar:baz` over `foo:bar$baz` is that `$` may be eaten by the shell. Though, I don't think many users pass `rpcauth` via the shell. Also it should be easy to avoid by passing `'-rpcauth=foo:bar$baz'` or `"-rpcauth=foo:bar\$baz"`. Can be tested with the added test. ACKs for top commit: pk-b2: ACK fa12706 Tree-SHA512: 9998cbb295c79f7b0342bf86e1d3e5b5ab90851c627662ad6495b699a65a9035998173cf1debfd94325387faba184de683407b609fe86acdd8f6749157644441
b42643c doc: update init.cpp -conf help text (josibake) 970b998 doc: update devtools, release-process readmes (josibake) 50635d2 build: include bitcoin.conf in build outputs (josibake) 6aac946 doc: update bitcoin-conf.md (Josiah Baker) 1c7e820 script: add script to generate example bitcoin.conf (josibake) b483084 doc: replace bitcoin.conf with placeholder file (josibake) Pull request description: create a script for parsing the output from `bitcoind --help` to create an example conf file for new users ## problem per bitcoin#10746 , `bitcoin.conf` not being put into the data directory during installation causes some confusion for users when running bitcoin. in the discussion on the issue, one proposed solution was to have an example config file and instruct users to `cp` it into their data directory after startup. in addition to bitcoin#10746 , there have been other requests for a "skeleton config file" (bitcoin#19641) to help users get started with configuring bitcoind. the main issue with an example config file is that it creates a second source of truth regarding what options are available for configuring bitcoind. this means any changes to the options (including the addition or removal of options) would have to be updated for the command line and also updated in the example file. this PR addresses this issue by providing a script to generate an example file directly from the `bitcoind --help` on-demand by running `contrib/devtools/gen-bitcoin-conf.sh`. this solution was originally proposed on bitcoin#10746 and would also solve bitcoin#19641 . this guarantees any changes made to the command-line options or the command-line options help would also be reflected in the example file after compiling and running the script. the main purpose of this script is to generate a config file to be included with releases, same as `gen-manpages.sh`. this ensures every release also includes an up-to-date, full example config file for users to edit. the script is also available for users who compile from source for generating an example config for their compiled binary. ## special considerations this removes the `bitcoin.conf` example file from the repo as it is now generated by this script. the original example file did contain extra text related to how to use certain options but going forward all option help docs should be moved into `init.cpp` this also edits `init.cpp` to have the option help indicate that `-conf` is not usable from the config file. this is similar to how `-includeconf` 's help indicates it cannot be used from the command line ACKs for top commit: laanwj: Tested and code review ACK b42643c Tree-SHA512: 4546e0cef92aa1398da553294ce4712d02e616dd72dcbe0b921af474e54f24750464ec813661f1283802472d1e8774e634dd1cc26fbf1f13286d3e0406c02c09
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/miner_tests.cpp (1)
422-423: Direct chain state manipulation risks test reliability.Similar to the issue at line 641 (flagged in previous reviews), directly manipulating
nTimeon chain tip ancestors bypasses proper chain management APIs. While these appear to be test-specific "tricks" to adjustMedianTimePastwithout mining blocks, they can:
- Leave chain state inconsistent
- Break invariants in
CChainState- Cause subtle test failures if other code paths validate chain metadata
Consider using helper methods that properly maintain chain invariants, or document why direct manipulation is safe in this test context and what invariants are preserved.
Also applies to: 429-432, 478-481
♻️ Duplicate comments (5)
src/test/miner_tests.cpp (1)
638-646: Past review comment about missing locks is incorrect.The past review comment stating that
TestPrioritisedMiningat line 645 is missing locks is incorrect. TheLOCK2(cs_main, m_node.mempool->cs)at line 638 remains in scope through line 648 (function end), so both required locks are properly held whenTestPrioritisedMiningis called.However, the concern about line 641's direct
nHeightmanipulation (from the past review) remains valid and should be addressed.src/script/script.h (1)
549-572: Fix the unreachable first-element branch in BuildScript.The past review comment correctly identified a critical logic error:
cntis incremented on line 557 before theif (cnt == 0)check on line 560, making that branch unreachable. This breaks the intended optimization of moving/assigning the first CScript argument instead of inserting it.Execution trace:
- First call: cnt=0 → increment to 1 → check if(1==0) → FALSE → else branch
- All subsequent calls: cnt keeps incrementing, condition never true
Apply this diff to fix the logic:
([&ret, &cnt] (Ts&& input) { cnt++; if constexpr (std::is_same_v<std::remove_cv_t<std::remove_reference_t<Ts>>, CScript>) { // If it is a CScript, extend ret with it. Move or copy the first element instead. - if (cnt == 0) { + if (cnt == 1) { ret = std::forward<Ts>(input); } else { ret.insert(ret.end(), input.begin(), input.end());src/script/miniscript.cpp (1)
5-12: Add missing include (build will fail on strict toolchains)std::reverse is used at Line 319; include .
Apply:
#include <string> #include <vector> +#include <algorithm> #include <script/script.h> #include <script/standard.h> #include <script/miniscript.h>src/script/miniscript.h (2)
8-26: Add the missing standard headers (already flagged)This issue was previously identified:
std::logic_error(Line 175) andstd::tuple(Line 892) require<stdexcept>and<tuple>headers respectively, but these are not included. This will cause compilation failures.
287-287: Fix the typo in the comment (already flagged)This issue was previously identified: "RIPEMD10" should be "RIPEMD160".
🧹 Nitpick comments (13)
contrib/devtools/gen-dash-conf.sh (1)
14-14: Consider using explicit if-statement for clarity.While the chained
&&operators work correctly, an explicit if-statement would be more readable:if [ ! -x "$DASHD" ]; then echo "$DASHD not found or not executable." exit 1 fiHowever, the current pattern is functional and commonly used in shell scripts.
ci/lint/04_install.sh (1)
36-40: Use python3 -m pip to bind installs to the pyenv interpreterAvoids picking up system pip3 when PATHs are mixed; no behavior change.
Apply this small tweak:
-${CI_RETRY_EXE} pip3 install codespell==2.1.0 -${CI_RETRY_EXE} pip3 install flake8==4.0.1 +${CI_RETRY_EXE} python3 -m pip install codespell==2.1.0 +${CI_RETRY_EXE} python3 -m pip install flake8==4.0.1 ${CI_RETRY_EXE} pip3 install lief==0.13.1 -${CI_RETRY_EXE} pip3 install mypy==0.942 +${CI_RETRY_EXE} python3 -m pip install mypy==0.942 ${CI_RETRY_EXE} pip3 install pyzmq==22.3.0 ${CI_RETRY_EXE} pip3 install vulture==2.3Optional follow-up: consider a pinned requirements/constraints file to keep these in sync with the Docker image.
src/test/txpackage_tests.cpp (6)
75-75: Use size_t for mempool sizesmempool->size() returns a size_t. Prefer size_t to avoid narrowing. Apply similarly where expected_pool_size is declared below.
- unsigned int initialPoolSize = m_node.mempool->size(); + size_t initialPoolSize = m_node.mempool->size();
221-221: Same here: prefer size_t for pool sizeKeep type consistent with mempool->size().
- unsigned int expected_pool_size = m_node.mempool->size(); + size_t expected_pool_size = m_node.mempool->size();
96-105: Gate critical preconditions with BOOST_REQUIRESubsequent lookups depend on success. Use BOOST_REQUIRE to short‑circuit on failure and avoid cascading assertions.
106-109: DRY: factor package vsize onceCompute package_vsize once for readability and to avoid recomputation.
- BOOST_CHECK(result_parent_child.m_package_feerate.has_value()); - BOOST_CHECK(result_parent_child.m_package_feerate.value() == - CFeeRate(2 * COIN, GetVirtualTransactionSize(*tx_parent) + GetVirtualTransactionSize(*tx_child))); + BOOST_CHECK(result_parent_child.m_package_feerate.has_value()); + const auto package_vsize = + GetVirtualTransactionSize(*tx_parent) + GetVirtualTransactionSize(*tx_child); + BOOST_CHECK(result_parent_child.m_package_feerate.value() == + CFeeRate(2 * COIN, package_vsize));
120-120: Prefer has_value() over comparison with std::nulloptSlight readability improvement.
- BOOST_CHECK(result_single_large.m_package_feerate == std::nullopt); + BOOST_CHECK(!result_single_large.m_package_feerate.has_value());(Apply similarly to the other occurrences.)
Also applies to: 241-241, 283-283, 299-299, 323-323, 345-345
361-364: Avoid hardcoded subsidy; derive from coinbaseDerive coinbase_value from the mined coinbase to de‑flake tests if subsidy changes in params.
- const CAmount coinbase_value{500 * COIN}; + const CAmount coinbase_value{m_coinbase_txns[0]->vout[0].nValue};src/test/miniscript_tests.cpp (3)
51-63: Make crypto API usage explicit and consistentPass explicit lengths to Write() and use .data() for Finalize() to avoid overload ambiguity.
- std::vector<unsigned char> hash; - hash.resize(32); - CSHA256().Write(keydata, 32).Finalize(hash.data()); + std::vector<unsigned char> hash(32); + CSHA256().Write(keydata, 32).Finalize(hash.data()); sha256.push_back(hash); - CHash256().Write(keydata).Finalize(hash); + CHash256().Write(keydata, 32).Finalize(hash.data()); hash256.push_back(hash); hash.resize(20); - CRIPEMD160().Write(keydata, 32).Finalize(hash.data()); + CRIPEMD160().Write(keydata, 32).Finalize(hash.data()); ripemd160.push_back(hash); - CHash160().Write(keydata).Finalize(hash); + CHash160().Write(keydata, 32).Finalize(hash.data()); hash160.push_back(hash);
34-37: Reserve capacity for test vectorsAvoid reallocations in tight loop.
TestData() { - // We generate 255 public keys and 255 hashes of each type. + // We generate 255 public keys and 255 hashes of each type. + pubkeys.reserve(255); + sha256.reserve(255); + hash256.reserve(255); + ripemd160.reserve(255); + hash160.reserve(255);
80-83: Defensive check for g_testdataAdd a fast assert to catch accidental use before initialization.
std::vector<unsigned char> ToPKHBytes(const CPubKey& key) const { + assert(g_testdata); auto it = g_testdata->pkhashes.find(key); assert(it != g_testdata->pkhashes.end()); return {it->second.begin(), it->second.end()}; } @@ bool FromPKHBytes(I first, I last, CPubKey& key) const { + assert(g_testdata); assert(last - first == 20); CKeyID keyid; std::copy(first, last, keyid.begin()); auto it = g_testdata->pkmap.find(keyid); assert(it != g_testdata->pkmap.end()); key = it->second; return true; }Also applies to: 101-108
src/test/fuzz/miniscript_decode.cpp (2)
18-19: Remove unused literal importoperator""_mst isn’t used here.
-using miniscript::operator""_mst; -
48-51: Consider supporting PKH→Key mapping to increase decode coverageReturning false skips P2PKH miniscript decoding paths. If feasible, add a small in‑fuzz dictionary to map some 20‑byte hashes to canned pubkeys, otherwise add a brief comment explaining why it’s intentionally unsupported.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
ci/lint/04_install.sh(1 hunks)contrib/containers/ci/ci-slim.Dockerfile(2 hunks)contrib/debian/examples/dash.conf(1 hunks)contrib/devtools/README.md(1 hunks)contrib/devtools/gen-dash-conf.sh(1 hunks)contrib/guix/libexec/build.sh(1 hunks)doc/build-openbsd.md(1 hunks)doc/dash-conf.md(1 hunks)doc/policy/packages.md(1 hunks)doc/release-process.md(1 hunks)src/Makefile.am(2 hunks)src/Makefile.test.include(2 hunks)src/httprpc.cpp(2 hunks)src/init.cpp(1 hunks)src/node/chainstate.cpp(1 hunks)src/node/miner.h(1 hunks)src/rest.cpp(1 hunks)src/rpc/util.cpp(2 hunks)src/script/interpreter.cpp(0 hunks)src/script/interpreter.h(0 hunks)src/script/miniscript.cpp(1 hunks)src/script/miniscript.h(1 hunks)src/script/script.cpp(1 hunks)src/script/script.h(2 hunks)src/script/standard.cpp(0 hunks)src/script/standard.h(1 hunks)src/test/fuzz/miniscript_decode.cpp(1 hunks)src/test/fuzz/tx_pool.cpp(1 hunks)src/test/miner_tests.cpp(5 hunks)src/test/miniscript_tests.cpp(1 hunks)src/test/txpackage_tests.cpp(9 hunks)src/validation.cpp(12 hunks)src/validation.h(2 hunks)src/wallet/rpc/backup.cpp(2 hunks)test/functional/rpc_users.py(1 hunks)
💤 Files with no reviewable changes (3)
- src/script/interpreter.h
- src/script/interpreter.cpp
- src/script/standard.cpp
🚧 Files skipped from review as they are similar to previous changes (14)
- doc/dash-conf.md
- test/functional/rpc_users.py
- src/node/miner.h
- src/Makefile.test.include
- contrib/devtools/README.md
- src/httprpc.cpp
- src/rpc/util.cpp
- src/test/fuzz/tx_pool.cpp
- doc/build-openbsd.md
- src/node/chainstate.cpp
- src/script/script.cpp
- src/init.cpp
- src/wallet/rpc/backup.cpp
- contrib/containers/ci/ci-slim.Dockerfile
🧰 Additional context used
📓 Path-based instructions (5)
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/release-process.mddoc/policy/packages.md
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/test/txpackage_tests.cppsrc/script/script.hsrc/rest.cppsrc/test/miniscript_tests.cppsrc/test/miner_tests.cppsrc/validation.hsrc/script/standard.hsrc/script/miniscript.cppsrc/validation.cppsrc/test/fuzz/miniscript_decode.cppsrc/script/miniscript.h
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/txpackage_tests.cppsrc/test/miniscript_tests.cppsrc/test/miner_tests.cppsrc/test/fuzz/miniscript_decode.cpp
contrib/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the contrib directory (contributed scripts)
Files:
contrib/devtools/gen-dash-conf.shcontrib/debian/examples/dash.confcontrib/guix/libexec/build.sh
ci/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the ci directory (continuous integration)
Files:
ci/lint/04_install.sh
🧠 Learnings (1)
📚 Learning: 2025-10-05T20:38:28.457Z
Learnt from: knst
PR: dashpay/dash#6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Applied to files:
contrib/guix/libexec/build.sh
🧬 Code graph analysis (8)
src/test/txpackage_tests.cpp (2)
src/test/util/setup_common.cpp (4)
mineBlocks(453-467)mineBlocks(453-453)CreateValidMempoolTransaction(550-597)CreateValidMempoolTransaction(550-556)src/validation.cpp (2)
ProcessNewPackage(1424-1442)ProcessNewPackage(1424-1425)
src/script/script.h (1)
src/script/script.cpp (2)
CheckMinimalPush(386-409)CheckMinimalPush(386-386)
src/rest.cpp (2)
src/validation.cpp (1)
GetTransaction(143-143)src/node/transaction.cpp (2)
GetTransaction(120-151)GetTransaction(120-120)
src/test/miniscript_tests.cpp (1)
src/test/fuzz/miniscript_decode.cpp (8)
key(24-27)key(24-24)key(28-30)key(28-28)key(31-34)key(31-31)FromString(37-41)FromString(37-37)
src/script/miniscript.cpp (3)
src/script/script.h (4)
DecodeOP_N(488-494)vector(330-361)CScriptNum(211-226)CScriptNum(230-240)src/script/standard.h (1)
IsPushdataOp(112-115)src/script/script.cpp (2)
CheckMinimalPush(386-409)CheckMinimalPush(386-386)
src/validation.cpp (1)
src/validation.h (1)
state(169-194)
src/test/fuzz/miniscript_decode.cpp (1)
src/test/miniscript_tests.cpp (6)
key(75-75)key(75-75)key(78-83)key(78-78)FromString(87-91)FromString(87-87)
src/script/miniscript.h (1)
src/script/miniscript.cpp (12)
ComputeType(38-251)ComputeType(38-38)ComputeScriptLen(253-284)ComputeScriptLen(253-253)SanitizeType(16-36)SanitizeType(16-16)FindNextChar(338-346)FindNextChar(338-338)DecomposeScript(286-321)DecomposeScript(286-286)ParseScriptNumber(323-336)ParseScriptNumber(323-323)
🪛 LanguageTool
doc/policy/packages.md
[grammar] ~73-~73: Ensure spelling is correct
Context: ...ransactions are allowed to be below the feerate requirements if the package meets the fe...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~74-~74: Ensure spelling is correct
Context: ...e requirements if the package meets the feerate requirements. For example, the parent(s...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~85-~85: Ensure spelling is correct
Context: ...transactions that failed. Since package feerate is only calculated using transactions t...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~89-~89: Ensure spelling is correct
Context: .... Rationale: We must not allow a low-feerate child to prevent its parent from being ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~95-~95: Consider a more concise word here.
Context: ...o avoid accidentally restricting policy in order to be backward-compatible for users and ap...
(IN_ORDER_TO_PREMIUM)
🔇 Additional comments (22)
src/rest.cpp (1)
694-694: LGTM! Improved parameter clarity.The change from
/* block_index */to/*block_index=*/improves readability by making it clearer that this is a named parameter comment. This style is already used consistently at line 722 for theTxToUnivcall.src/test/miner_tests.cpp (2)
40-41: LGTM: Well-annotated function declarations.The new helper function declarations are properly annotated with
EXCLUSIVE_LOCKS_REQUIREDfor bothcs_mainandm_node.mempool->cs, which correctly documents the locking requirements.
484-557: LGTM: Thorough prioritisation testing.The
TestPrioritisedMiningimplementation correctly tests complex scenarios including:
- Zero-fee transactions with prioritisation
- Ancestor fee calculations with prioritised parents
- De-prioritised transactions exclusion
The logic properly validates that parent prioritisations don't incorrectly propagate to descendants.
doc/release-process.md (1)
4-6: LGTM: Documentation correctly updated for new dash.conf workflow.The release process checklist has been appropriately updated to reference the
developbranch and include the new dash.conf generation step. The links are consistent and point to the correct documentation.contrib/debian/examples/dash.conf (1)
1-1: LGTM: Placeholder appropriately directs users to generation script.The placeholder correctly instructs users to generate the dash.conf file using the new script, maintaining consistency with the updated workflow where this file is auto-generated rather than manually maintained.
contrib/guix/libexec/build.sh (1)
358-361: LGTM: Correctly integrates dash.conf into distribution packaging.The copy step properly includes the generated (or placeholder) dash.conf file from
contrib/debian/examples/into the distribution package, ensuring users receive a configuration example regardless of whether the generation script was run during the build.contrib/devtools/gen-dash-conf.sh (5)
1-12: LGTM: Environment setup correctly configured.The script header and environment variables are properly configured with sensible defaults that can be overridden. The output path at line 11 (
contrib/debian/examples/) correctly matches the location expected by the build script atcontrib/guix/libexec/build.sh:360.
16-26: LGTM: Dirty build detection appropriately warns users.The script helpfully detects and warns when dashd is built from a dirty tree, prompting users to commit changes and rebuild before generating the configuration file. This prevents config drift from uncommitted code changes.
28-47: LGTM: Configuration header properly generated.The script creates the output directory and writes a clear, well-documented header to the configuration file. The header appropriately explains the file's purpose and usage.
49-60: LGTM: Help text parsing transforms options into config format.The sed/awk pipeline systematically transforms
dashd --helpoutput into a properly formatted, commented configuration file. The transformations handle option formatting, section headers, and boolean option defaults appropriately.
62-80: LGTM: Network sections properly configured.The footer appropriately adds network-specific sections with clear documentation explaining how options are scoped to different networks (mainnet, testnet, regtest). The note about network-specific option behavior is particularly helpful.
src/script/script.h (2)
323-323: LGTM!The accessor correctly exposes the internal
m_valueand is appropriately const-qualified.
547-547: LGTM!The function declaration is clear and the implementation (visible in relevant snippets) correctly validates minimal push encoding per Bitcoin script rules.
src/script/standard.h (1)
112-115: Code change verified — no issues found.All call sites resolve correctly after the move. The
IsPushdataOpfunction is properly declared asconstexprin the header and called successfully fromsrc/script/standard.cpp(withinGetMultisigKeyCount) andsrc/script/miniscript.cpp. The function logic correctly identifies pushdata opcodes (range 0x01–0x4e). No unresolved references are present.src/validation.h (2)
183-187: Comment update LGTMClarifies vsize meaning; no API or ABI impact.
234-246: Verified: package feerate is populated correctly by all callersThe implementation is correct. Call sites consistently use the 2-param constructor (with feerate) after
package_feerateis computed, and the 1-param constructor (without feerate) for early returns. The assertion at line 1388 (if (submission_result.m_state.IsValid()) assert(submission_result.m_package_feerate.has_value())) enforces the invariant that valid package states have a feerate value. Tests verify both populated and unpopulated cases as expected.src/script/miniscript.cpp (2)
286-321: DecomposeScript: minimal VERIFY handling and decomposition LGTM
- Correctly rejects non-minimal X OP_VERIFY sequences and decomposes XVERIFY forms.
- Minimal-push check via CheckMinimalPush is used appropriately.
323-336: ParseScriptNumber: strict minimal encoding with safe failureException from CScriptNum is caught and returns false; behavior matches policy. LGTM.
src/Makefile.am (2)
348-349: Expose script/miniscript.h in public headers — LGTMHeader is now exported alongside descriptor.h; consistent with upstream layout.
927-928: Wire script/miniscript.cpp into libbitcoin_common — LGTM; confirm linkagecommon now depends on symbols from script/script.cpp (CheckMinimalPush). Binaries link COMMON before CONSENSUS, so static link order should resolve. Please confirm at least one target including both libs still links cleanly.
Run a quick symbol check after a local build to ensure no unresolved references to CheckMinimalPush from miniscript.o.
doc/policy/packages.md (1)
64-99: Package Feerate section reads clearly and matches implementation intentDefinition excludes deduped mempool txs and clarifies use against minRelayTxFee and dynamic floor; consistent with code changes exposing m_package_feerate. LGTM.
src/test/fuzz/miniscript_decode.cpp (1)
56-72: LGTM: fuzz round‑trip checksRound‑tripping Miniscript⇄string and Miniscript⇄script looks correct and side‑effect free.
| // Transactions must meet two minimum feerates: the mempool minimum fee and min relay fee. | ||
| // For transactions consisting of exactly one child and its parents, it suffices to use the | ||
| // package feerate (total modified fees / total size) to check this requirement. | ||
| const auto m_total_vsize = std::accumulate(workspaces.cbegin(), workspaces.cend(), int64_t{0}, | ||
| [](int64_t sum, auto& ws) { return sum + ws.m_vsize; }); | ||
| const auto m_total_modified_fees = std::accumulate(workspaces.cbegin(), workspaces.cend(), CAmount{0}, | ||
| [](CAmount sum, auto& ws) { return sum + ws.m_modified_fees; }); | ||
| const CFeeRate package_feerate(m_total_modified_fees, m_total_vsize); | ||
| TxValidationState placeholder_state; | ||
| if (args.m_package_feerates && | ||
| !CheckFeeRate(m_total_vsize, m_total_modified_fees, placeholder_state)) { | ||
| package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-fee-too-low"); | ||
| return PackageMempoolAcceptResult(package_state, package_feerate, {}); | ||
| } |
There was a problem hiding this comment.
Package feerate still excludes already-admitted parents
m_total_vsize/m_total_modified_fees are still derived only from the workspaces vector. In AcceptPackage() we now admit any parent that passes AcceptSingleTransaction() before building txns_new, so a standard CPFP case (high-fee parent already in the pool, low-fee child remaining) reaches this block with only the child in workspaces. The new CheckFeeRate(...) call therefore evaluates the child in isolation and rejects the package even though the full parent+child package would easily clear the target feerate — the exact regression we previously flagged. Please include the size and modified fees of parents that were already in (or newly accepted into) the mempool when computing the package totals, e.g. by tracking their contributions in AcceptPackage() and adding them here before calling CheckFeeRate and constructing package_feerate.
🤖 Prompt for AI Agents
In src/validation.cpp around lines 1226 to 1239, the package feerate is computed
only from the workspaces vector which excludes parents that were already
admitted (or newly accepted) into the mempool, causing CheckFeeRate to evaluate
only the child and wrongly reject valid parent+child packages; update the logic
so that m_total_vsize and m_total_modified_fees include the vsize and
modified-fee contributions of any already-admitted or newly-accepted parent
transactions tracked by AcceptPackage() (i.e., have AcceptPackage accumulate
parent_vsize and parent_modified_fees and add them to the m_total_* totals here
before constructing package_feerate and calling CheckFeeRate), ensuring
package_feerate and the CheckFeeRate call use the combined parent+workspaces
totals.
6 participants
What was done?
Regular backports from Bitcoin V24
How Has This Been Tested?
Run unit & functional tests
Breaking Changes
There is a new way to prepare default
dash.conf, the docs for release process are updated accordinglyChecklist: