backport: bitcoin#17127, #23810, #25867, #26226, #26705, #26904, #26956, #27081, bitcoin-core/gui#653, partial bitcoin#15294 #7100
Conversation
fa8fe5b scripted-diff: Use new python 3.7 keywords (MarcoFalke) fa2a235 Revert "contrib: Fix capture_output in getcoins.py" (MarcoFalke) dddd462 Bump minimum python version to 3.7 (MarcoFalke) Pull request description: While there is nothing that requires a bump, it may require less maintenance to drop python3.6 support. Python3.7 is available through the package manager on all currently supported operating systems. ACKs for top commit: jamesob: ACK bitcoin@fa8fe5b hebasto: ACK fa8fe5b Tree-SHA512: f6e080d8751948bb0e01c87be601363158f345e8037b70ce7e1bc507c611eb61600e4f24f1d2f8a6e7e44877ab09319302869e33ce8118c4c4f71fc89c0a1198
BACKPORT NOTE: we have no usages for a new caller RipeMd160 as it is used only by witness's related code. It may be useful in the future, but DNM atm
…lper b530d96 test: refactor: introduce `replace_in_config` helper (Sebastian Falbesoner) Pull request description: Currently two functional tests (p2p_permissions.py and wallet_crosschain.py) include quite similar code for substituting strings in a TestNode's bitcoind configuration file, so refactoring that out to a dedicated helper method seems to make sense (probably other tests could need that too in the future). ACKs for top commit: kouloumos: ACK b530d96 Tree-SHA512: 5ca65a2ef3292460e5720d5c6acf7326335001858e8f71ab054560117f9479dbadb1dacd8c9235f67f3fcfd08dbc761b62704f379cbf619bba8804f16a25bc7b
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThis PR contains wide-ranging code quality and infrastructure improvements across development scripts, test frameworks, and core application code. Major changes include: replacing deprecated subprocess parameter Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/functional/test_runner.py (1)
731-741: LGTM, but there's an inconsistency in the retry logic.The change to
text=Truehere is correct. However, the retry logic at line 784 still usesuniversal_newlines=True:self.jobs.append((name, time.time(), subprocess.Popen([sys.executable, self.tests_dir + test_argv[0]] + test_argv[1:] + self.flags + portseed_arg + tmpdir_arg, universal_newlines=True, # <-- inconsistent stdout=log_stdout, stderr=log_stderr),For consistency, line 784 should also use
text=True.♻️ Suggested fix for consistency
@@ -781,7 +781,7 @@ class TestHandler: self.jobs.append((name, time.time(), subprocess.Popen([sys.executable, self.tests_dir + test_argv[0]] + test_argv[1:] + self.flags + portseed_arg + tmpdir_arg, - universal_newlines=True, + text=True, stdout=log_stdout, stderr=log_stderr),
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (83)
ci/test/04_install.shcontrib/devtools/clang-format-diff.pycontrib/devtools/gen-manpages.pycontrib/devtools/github-merge.pycontrib/devtools/optimize-pngs.pycontrib/devtools/security-check.pycontrib/devtools/test-security-check.pycontrib/devtools/test-symbol-check.pycontrib/devtools/update-translations.pycontrib/macdeploy/macdeployqtplusdoc/developer-notes.mddoc/init.mdshare/rpcauth/rpcauth.pysrc/.clang-tidysrc/Makefile.amsrc/crypto/ripemd160.cppsrc/crypto/ripemd160.hsrc/crypto/sha1.cppsrc/crypto/sha1.hsrc/crypto/sha256.cppsrc/crypto/sha256.hsrc/crypto/sha512.cppsrc/crypto/sha512.hsrc/cuckoocache.hsrc/dbwrapper.hsrc/i2p.cppsrc/init.cppsrc/key.hsrc/policy/fees.cppsrc/policy/fees.hsrc/qt/sendcoinsdialog.cppsrc/rpc/request.cppsrc/script/sign.cppsrc/serialize.hsrc/span.hsrc/streams.hsrc/support/lockedpool.cppsrc/support/lockedpool.hsrc/test/util/net.hsrc/tinyformat.hsrc/torcontrol.cppsrc/torcontrol.hsrc/univalue/include/univalue_utffilter.hsrc/util/system.cppsrc/wallet/bdb.cppsrc/wallet/bdb.hsrc/wallet/db.hsrc/wallet/init.cppsrc/wallet/wallet.hsrc/zmq/zmqabstractnotifier.hsrc/zmq/zmqnotificationinterface.cppsrc/zmq/zmqnotificationinterface.htest/functional/feature_dbcrash.pytest/functional/feature_llmq_connections.pytest/functional/feature_llmq_is_retroactive.pytest/functional/feature_posix_fs_permissions.pytest/functional/p2p_node_network_limited.pytest/functional/p2p_permissions.pytest/functional/rpc_preciousblock.pytest/functional/rpc_users.pytest/functional/test_framework/p2p.pytest/functional/test_framework/test_framework.pytest/functional/test_framework/test_node.pytest/functional/test_framework/util.pytest/functional/test_runner.pytest/functional/tool_wallet.pytest/functional/wallet_crosschain.pytest/fuzz/test_runner.pytest/lint/lint-assertions.pytest/lint/lint-circular-dependencies.pytest/lint/lint-git-commit-check.pytest/lint/lint-includes.pytest/lint/lint-locale-dependence.pytest/lint/lint-logs.pytest/lint/lint-python-mutable-default-parameters.pytest/lint/lint-python-utf8-encoding.pytest/lint/lint-python.pytest/lint/lint-shell.pytest/lint/lint-submodule.pytest/lint/lint-tests.pytest/lint/lint-whitespace.pytest/util/rpcauth-test.pytest/util/test_runner.py
💤 Files with no reviewable changes (3)
- src/policy/fees.cpp
- src/wallet/init.cpp
- src/init.cpp
🧰 Additional context used
📓 Path-based instructions (7)
src/**/*.{cpp,h,hpp,cc}
📄 CodeRabbit inference engine (CLAUDE.md)
Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1
Files:
src/support/lockedpool.hsrc/script/sign.cppsrc/rpc/request.cppsrc/crypto/ripemd160.hsrc/wallet/bdb.hsrc/util/system.cppsrc/qt/sendcoinsdialog.cppsrc/zmq/zmqnotificationinterface.hsrc/span.hsrc/crypto/sha1.hsrc/i2p.cppsrc/torcontrol.hsrc/streams.hsrc/crypto/sha256.hsrc/wallet/bdb.cppsrc/key.hsrc/crypto/sha512.hsrc/dbwrapper.hsrc/crypto/ripemd160.cppsrc/wallet/wallet.hsrc/cuckoocache.hsrc/policy/fees.hsrc/tinyformat.hsrc/test/util/net.hsrc/support/lockedpool.cppsrc/serialize.hsrc/torcontrol.cppsrc/crypto/sha512.cppsrc/crypto/sha256.cppsrc/zmq/zmqabstractnotifier.hsrc/zmq/zmqnotificationinterface.cppsrc/crypto/sha1.cppsrc/wallet/db.hsrc/univalue/include/univalue_utffilter.h
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests in test/functional/ must be written in Python (minimum version specified in .python-version) and depend on dashd and dash-node
Files:
test/functional/rpc_preciousblock.pytest/functional/test_framework/util.pytest/functional/feature_llmq_connections.pytest/functional/feature_dbcrash.pytest/functional/test_framework/test_node.pytest/functional/tool_wallet.pytest/functional/test_framework/test_framework.pytest/functional/wallet_crosschain.pytest/functional/rpc_users.pytest/functional/p2p_node_network_limited.pytest/functional/p2p_permissions.pytest/functional/feature_llmq_is_retroactive.pytest/functional/test_framework/p2p.pytest/functional/feature_posix_fs_permissions.pytest/functional/test_runner.py
{guix-build*,releases,**/guix-build*,releases/**,.github/**,depends/**,ci/**,contrib/**,doc/**}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not make changes to build system files (guix-build*), release artifacts, or avoid changes to .github, depends, ci, contrib, and doc directories unless specifically prompted
Files:
contrib/macdeploy/macdeployqtplusdoc/developer-notes.mdci/test/04_install.shcontrib/devtools/test-security-check.pycontrib/devtools/update-translations.pycontrib/devtools/github-merge.pycontrib/devtools/security-check.pydoc/init.mdcontrib/devtools/gen-manpages.pycontrib/devtools/clang-format-diff.pycontrib/devtools/test-symbol-check.pycontrib/devtools/optimize-pngs.py
src/wallet/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Wallet implementation must use Berkeley DB and SQLite
Files:
src/wallet/bdb.hsrc/wallet/bdb.cppsrc/wallet/wallet.hsrc/wallet/db.h
src/qt/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
GUI implementation in src/qt/ must use Qt 5
Files:
src/qt/sendcoinsdialog.cpp
src/{test,wallet/test}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in src/test/ and src/wallet/test/ must use Boost::Test framework
Files:
src/test/util/net.h
src/{crc32c,dashbls,gsl,immer,leveldb,minisketch,secp256k1,univalue,crypto/{ctaes,x11}}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Do not make changes to vendored dependencies: src/{crc32c,dashbls,gsl,immer,leveldb,minisketch,secp256k1,univalue} or src/crypto/{ctaes,x11}
Files:
src/univalue/include/univalue_utffilter.h
🧠 Learnings (28)
📓 Common learnings
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: knst
Repo: dashpay/dash PR: 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.
Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.
Learnt from: kwvg
Repo: dashpay/dash PR: 7072
File: src/qt/walletcontroller.cpp:520-528
Timestamp: 2025-12-17T13:58:26.891Z
Learning: In Dash Qt wallet code, when leveraging upstream Bitcoin Core wallet capabilities (especially for operations like rescanning), prefer using the inherited signal-based mechanisms (e.g., ShowProgress signals) over adding explicit Qt progress dialogs to minimize upstream deviation and simplify future backports.
📚 Learning: 2025-12-22T15:42:48.595Z
Learnt from: kwvg
Repo: dashpay/dash PR: 7068
File: src/qt/guiutil_font.h:68-77
Timestamp: 2025-12-22T15:42:48.595Z
Learning: In C++/Qt codebases, use fail-fast asserts in setters to enforce invariants (e.g., ensuring internal maps contain necessary keys). Prefer assert() for programmer errors that should be caught in development and debugging, rather than defensive runtime checks with fallbacks. This helps catch invariant violations early during development. Apply to header and source files where invariant-driven state mutations occur, especially in setters like SetWeightBold/SetWeightNormal that assume established relationships or preconditions.
Applied to files:
src/support/lockedpool.hsrc/crypto/ripemd160.hsrc/wallet/bdb.hsrc/zmq/zmqnotificationinterface.hsrc/span.hsrc/crypto/sha1.hsrc/torcontrol.hsrc/streams.hsrc/crypto/sha256.hsrc/key.hsrc/crypto/sha512.hsrc/dbwrapper.hsrc/wallet/wallet.hsrc/cuckoocache.hsrc/policy/fees.hsrc/tinyformat.hsrc/test/util/net.hsrc/serialize.hsrc/zmq/zmqabstractnotifier.hsrc/wallet/db.hsrc/univalue/include/univalue_utffilter.h
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Applied to files:
test/functional/rpc_preciousblock.pytest/functional/test_framework/util.pytest/functional/feature_llmq_connections.pytest/util/test_runner.pytest/functional/feature_dbcrash.pytest/functional/test_framework/test_framework.pytest/functional/p2p_node_network_limited.pytest/functional/feature_llmq_is_retroactive.py
📚 Learning: 2025-02-14T15:15:58.165Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/rpc/governance.cpp:1074-1089
Timestamp: 2025-02-14T15:15:58.165Z
Learning: Code blocks marked with `// clang-format off` and `// clang-format on` directives should be excluded from clang-format suggestions as they are intentionally formatted manually for better readability.
Applied to files:
doc/developer-notes.md
📚 Learning: 2025-11-14T23:17:08.495Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6976
File: src/stacktraces.cpp:475-482
Timestamp: 2025-11-14T23:17:08.495Z
Learning: In C++ code reviews, always check for opportunities to use standard algorithms (std::any_of, std::all_of, std::none_of, std::find_if, std::transform, std::for_each, etc.) instead of manual for-loops, especially when the loop body performs simple checks, transformations, or accumulations.
Applied to files:
doc/developer-notes.md
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/wallet/**/*.{cpp,h} : Wallet implementation must use Berkeley DB and SQLite
Applied to files:
src/wallet/bdb.hsrc/wallet/bdb.cppsrc/wallet/wallet.hsrc/wallet/db.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h} : Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Applied to files:
src/wallet/bdb.hsrc/Makefile.am
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 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/bdb.hsrc/qt/sendcoinsdialog.cppsrc/wallet/wallet.h
📚 Learning: 2025-12-17T13:58:19.813Z
Learnt from: kwvg
Repo: dashpay/dash PR: 7072
File: src/qt/walletcontroller.cpp:520-528
Timestamp: 2025-12-17T13:58:19.813Z
Learning: In Dash Qt wallet code, prefer using existing Qt signals/slots (e.g., ShowProgress signals) for long-running operations instead of adding explicit QProgressDialog dialogs. This minimizes upstream deviations and simplifies backports. Apply this pattern to wallet-related Qt components (e.g., operations like rescanning) and rely on signal-based progress updates rather than bespoke dialogs.
Applied to files:
src/qt/sendcoinsdialog.cpp
📚 Learning: 2025-12-17T13:58:26.891Z
Learnt from: kwvg
Repo: dashpay/dash PR: 7072
File: src/qt/walletcontroller.cpp:520-528
Timestamp: 2025-12-17T13:58:26.891Z
Learning: In Dash Qt wallet code, when leveraging upstream Bitcoin Core wallet capabilities (especially for operations like rescanning), prefer using the inherited signal-based mechanisms (e.g., ShowProgress signals) over adding explicit Qt progress dialogs to minimize upstream deviation and simplify future backports.
Applied to files:
test/functional/tool_wallet.py
📚 Learning: 2025-08-10T13:52:46.289Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6666
File: test/functional/rpc_netinfo.py:73-81
Timestamp: 2025-08-10T13:52:46.289Z
Learning: In the Bitcoin/Dash test framework, `self.node.extra_args` contains the original startup arguments for a TestNode and is not modified by `test.restart_node()`. Each restart with new `extra_args` doesn't update the stored `self.node.extra_args`, so copying from it always provides a clean slate without accumulated modifications from previous restarts.
Applied to files:
test/functional/test_framework/test_framework.pytest/functional/wallet_crosschain.py
📚 Learning: 2025-07-14T10:11:05.011Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6754
File: contrib/containers/guix/docker-compose.yml:18-19
Timestamp: 2025-07-14T10:11:05.011Z
Learning: In the Guix build process for Dash Core, the `guix.sigs` directory requires write access as signatures are written to it during the build process, and `dash-detached-sigs` may be updated with `git pull` operations, so both directories need rw permissions in the Docker volume mounts.
Applied to files:
doc/init.md
📚 Learning: 2025-02-14T15:19:17.218Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Applied to files:
src/wallet/wallet.h
📚 Learning: 2025-08-19T18:53:02.863Z
Learnt from: knst
Repo: dashpay/dash PR: 6824
File: test/functional/feature_llmq_is_retroactive.py:34-37
Timestamp: 2025-08-19T18:53:02.863Z
Learning: The `check_no_is` method in `test/functional/feature_llmq_is_retroactive.py` uses assertions for validation and doesn't return any value - it either succeeds silently or raises an AssertionError when the InstantLock assertion fails.
Applied to files:
test/functional/feature_llmq_is_retroactive.py
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,llmq,evo,coinjoin,governance}/**/*.{cpp,h} : Use unordered_lru_cache for efficient caching with LRU eviction in Dash-specific data structures
Applied to files:
src/cuckoocache.h
📚 Learning: 2025-08-11T17:16:36.654Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Applied to files:
src/Makefile.am
📚 Learning: 2025-07-23T09:30:34.631Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.631Z
Learning: Dash Core uses BITCOIN_ prefix for header guards as the standard convention, inherited from Bitcoin Core. Only a few BLS-specific files in src/bls/ use DASH_ prefix. The vast majority of files (385+) use BITCOIN_ prefix.
Applied to files:
src/Makefile.am
📚 Learning: 2025-10-25T07:08:51.918Z
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: The univalue library (in src/univalue/) is no longer a vendored external dependency but is now part of the Bitcoin Core codebase and can be modified as needed during backports.
Applied to files:
src/Makefile.am
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,txmempool}/**/*.{cpp,h} : Block validation and mempool handling must use extensions to Bitcoin Core mechanisms for special transaction validation and enhanced transaction relay
Applied to files:
src/Makefile.am
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{crc32c,dashbls,gsl,immer,leveldb,minisketch,secp256k1,univalue,crypto/{ctaes,x11}}/** : Do not make changes to vendored dependencies: src/{crc32c,dashbls,gsl,immer,leveldb,minisketch,secp256k1,univalue} or src/crypto/{ctaes,x11}
Applied to files:
src/Makefile.am
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Dash extends Bitcoin Core through composition with minimal changes to the Bitcoin Core foundation
Applied to files:
src/Makefile.am
📚 Learning: 2025-02-06T14:34:30.466Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Applied to files:
src/Makefile.am
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/evo/**/*.{cpp,h} : Special transactions use payload serialization routines defined in src/evo/specialtx.h and must include appropriate special transaction types (ProRegTx, ProUpServTx, ProUpRegTx, ProUpRevTx)
Applied to files:
src/Makefile.am
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,consensus,net_processing}/**/*.{cpp,h} : ValidationInterface callbacks must be used for event-driven architecture to coordinate subsystems during block/transaction processing
Applied to files:
src/Makefile.am
📚 Learning: 2025-01-14T09:05:12.032Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6532
File: src/test/util/net.h:204-204
Timestamp: 2025-01-14T09:05:12.032Z
Learning: The SetNonBlocking() method in Sock and its derived classes is correctly marked const as it doesn't modify the socket's internal state, only its blocking mode configuration.
Applied to files:
src/test/util/net.h
📚 Learning: 2025-01-14T09:06:19.717Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6532
File: src/test/fuzz/netaddress.cpp:83-84
Timestamp: 2025-01-14T09:06:19.717Z
Learning: In fuzzer harness tests, CServiceHash can be used with both default constructor (CServiceHash()) and parameterized constructor (CServiceHash(salt_k0, salt_k1)) to test different variants. The usage pattern CServiceHash()(service) and CServiceHash(0, 0)(service) is valid and intentional in such tests.
Applied to files:
src/crypto/sha512.cppsrc/crypto/sha256.cppsrc/crypto/sha1.cpp
📚 Learning: 2025-09-09T21:36:31.504Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:31.504Z
Learning: In RawSender class (src/stats/rawsender.cpp), m_sock access is protected by the cs_net mutex. This mutex coordinates between the TCP send path in SendDirectly() and the reconnection operations in Connect()/Reconnect() methods, ensuring proper synchronization of socket state.
Applied to files:
src/zmq/zmqabstractnotifier.h
📚 Learning: 2025-09-09T21:36:58.969Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:223-232
Timestamp: 2025-09-09T21:36:58.969Z
Learning: In RawSender class (src/stats/rawsender.cpp), socket operations are properly synchronized using the cs_net mutex. The m_sock and m_server members are GUARDED_BY(cs_net), and methods like Connect(), SendDirectly(), and ReconnectThread() use appropriate locking with cs_net to prevent race conditions on socket access.
Applied to files:
src/zmq/zmqabstractnotifier.h
🧬 Code graph analysis (19)
src/crypto/ripemd160.h (3)
src/crypto/sha1.h (1)
bytes(17-17)src/crypto/sha256.h (1)
bytes(18-18)src/crypto/sha512.h (1)
bytes(17-17)
test/functional/test_framework/test_node.py (1)
test/functional/test_framework/util.py (1)
assert_equal(69-74)
test/functional/test_framework/test_framework.py (1)
test/functional/test_framework/test_node.py (1)
replace_in_config(430-443)
src/crypto/sha1.h (3)
src/crypto/ripemd160.h (1)
bytes(17-17)src/crypto/sha256.h (1)
bytes(18-18)src/crypto/sha512.h (1)
bytes(17-17)
test/functional/wallet_crosschain.py (1)
test/functional/test_framework/test_node.py (1)
replace_in_config(430-443)
src/crypto/sha256.h (3)
src/crypto/ripemd160.h (1)
bytes(17-17)src/crypto/sha1.h (1)
bytes(17-17)src/crypto/sha512.h (1)
bytes(17-17)
src/crypto/sha512.h (3)
src/crypto/ripemd160.h (1)
bytes(17-17)src/crypto/sha1.h (1)
bytes(17-17)src/crypto/sha256.h (1)
bytes(18-18)
test/functional/p2p_permissions.py (1)
test/functional/test_framework/test_node.py (1)
replace_in_config(430-443)
test/util/rpcauth-test.py (1)
share/rpcauth/rpcauth.py (3)
generate_password(15-17)generate_salt(11-13)password_to_hmac(19-21)
src/dbwrapper.h (1)
src/wallet/bdb.cpp (2)
ssKey(492-492)ssValue(493-493)
src/crypto/ripemd160.cpp (1)
src/crypto/ripemd160.h (1)
CRIPEMD160(12-26)
test/functional/feature_posix_fs_permissions.py (1)
test/functional/test_framework/test_framework.py (6)
set_test_params(415-417)set_test_params(1469-1471)skip_if_platform_not_posix(1040-1043)run_test(494-496)run_test(1476-1478)main(156-187)
src/support/lockedpool.cpp (1)
src/support/lockedpool.h (2)
LockedPool(126-205)LockedPageAllocator(19-43)
src/serialize.h (1)
src/chain.h (1)
nSize(44-44)
src/crypto/sha512.cpp (1)
src/crypto/sha512.h (1)
CSHA512(12-27)
src/crypto/sha256.cpp (1)
src/crypto/sha256.h (1)
CSHA256(13-27)
src/zmq/zmqabstractnotifier.h (1)
src/zmq/zmqabstractnotifier.cpp (1)
CZMQAbstractNotifier(11-14)
src/crypto/sha1.cpp (1)
src/crypto/sha1.h (1)
CSHA1(12-26)
src/wallet/db.h (1)
src/wallet/test/wallet_tests.cpp (1)
nUpdateCounter(1517-1517)
🪛 Flake8 (7.3.0)
test/lint/lint-submodule.py
[error] 16-16: continuation line over-indented for visual indent
(E127)
🪛 Ruff (0.14.10)
test/functional/rpc_preciousblock.py
19-19: Do not catch blind exception: Exception
(BLE001)
contrib/devtools/test-security-check.py
44-44: subprocess call: check for execution of untrusted input
(S603)
contrib/devtools/update-translations.py
59-59: Do not catch blind exception: Exception
(BLE001)
test/functional/test_framework/util.py
477-478: try-except-pass detected, consider logging the exception
(S110)
477-477: Do not catch blind exception: Exception
(BLE001)
499-500: try-except-pass detected, consider logging the exception
(S110)
499-499: Do not catch blind exception: Exception
(BLE001)
test/functional/feature_llmq_connections.py
94-95: try-except-pass detected, consider logging the exception
(S110)
94-94: Do not catch blind exception: Exception
(BLE001)
108-109: try-except-pass detected, consider logging the exception
(S110)
108-108: Do not catch blind exception: Exception
(BLE001)
test/lint/lint-circular-dependencies.py
82-82: subprocess call: check for execution of untrusted input
(S603)
test/lint/lint-git-commit-check.py
45-45: Starting a process with a partial executable path
(S607)
52-52: subprocess call: check for execution of untrusted input
(S603)
52-52: Starting a process with a partial executable path
(S607)
55-55: subprocess call: check for execution of untrusted input
(S603)
55-55: Starting a process with a partial executable path
(S607)
test/util/test_runner.py
57-57: Do not catch blind exception: Exception
(BLE001)
115-115: subprocess call: check for execution of untrusted input
(S603)
contrib/devtools/github-merge.py
309-310: try-except-pass detected, consider logging the exception
(S110)
309-309: Do not catch blind exception: Exception
(BLE001)
test/functional/feature_dbcrash.py
88-88: Do not catch blind exception: Exception
(BLE001)
contrib/devtools/security-check.py
37-37: Do not catch blind exception: Exception
(BLE001)
test/lint/lint-assertions.py
15-15: subprocess call: check for execution of untrusted input
(S603)
15-15: Starting a process with a partial executable path
(S607)
test/functional/test_framework/test_node.py
875-875: subprocess call: check for execution of untrusted input
(S603)
958-958: Do not catch blind exception: Exception
(BLE001)
test/functional/tool_wallet.py
35-35: subprocess call: check for execution of untrusted input
(S603)
35-35: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
test/lint/lint-includes.py
46-46: Starting a process with a partial executable path
(S607)
52-52: subprocess call: check for execution of untrusted input
(S603)
52-52: Consider ["git", "ls-files", "src", *exclude_args] instead of concatenation
Replace with ["git", "ls-files", "src", *exclude_args]
(RUF005)
74-74: Starting a process with a partial executable path
(S607)
89-89: subprocess call: check for execution of untrusted input
(S603)
89-89: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
112-112: subprocess call: check for execution of untrusted input
(S603)
112-112: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
155-155: subprocess call: check for execution of untrusted input
(S603)
155-155: Starting a process with a partial executable path
(S607)
161-161: subprocess call: check for execution of untrusted input
(S603)
161-161: Starting a process with a partial executable path
(S607)
test/functional/test_framework/test_framework.py
1043-1043: Avoid specifying long messages outside the exception class
(TRY003)
1608-1608: Do not catch blind exception: Exception
(BLE001)
1691-1691: Do not catch blind exception: Exception
(BLE001)
1939-1939: Do not catch blind exception: Exception
(BLE001)
1950-1950: Do not catch blind exception: Exception
(BLE001)
test/lint/lint-shell.py
28-28: subprocess call: check for execution of untrusted input
(S603)
contrib/devtools/gen-manpages.py
26-26: subprocess call: check for execution of untrusted input
(S603)
39-39: subprocess call: check for execution of untrusted input
(S603)
test/functional/rpc_users.py
55-55: subprocess call: check for execution of untrusted input
(S603)
61-61: subprocess call: check for execution of untrusted input
(S603)
contrib/devtools/test-symbol-check.py
26-26: subprocess call: check for execution of untrusted input
(S603)
test/functional/p2p_node_network_limited.py
86-87: try-except-pass detected, consider logging the exception
(S110)
86-86: Do not catch blind exception: Exception
(BLE001)
test/lint/lint-whitespace.py
86-86: subprocess call: check for execution of untrusted input
(S603)
86-86: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
99-99: Starting a process with a partial executable path
(S607)
test/lint/lint-python-mutable-default-parameters.py
24-24: subprocess call: check for execution of untrusted input
(S603)
test/functional/feature_llmq_is_retroactive.py
40-40: Do not catch blind exception: Exception
(BLE001)
test/lint/lint-locale-dependence.py
230-230: subprocess call: check for execution of untrusted input
(S603)
contrib/devtools/optimize-pngs.py
53-53: Do not catch blind exception: Exception
(BLE001)
test/lint/lint-python-utf8-encoding.py
27-27: subprocess call: check for execution of untrusted input
(S603)
27-27: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
41-41: subprocess call: check for execution of untrusted input
(S603)
41-41: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
44-44: Use raise without specifying exception name
Remove exception name
(TRY201)
test/lint/lint-logs.py
19-19: Starting a process with a partial executable path
(S607)
test/functional/test_runner.py
647-647: subprocess call: check for execution of untrusted input
(S603)
test/lint/lint-tests.py
26-26: subprocess call: check for execution of untrusted input
(S603)
⏰ 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 / Create multi-arch manifest
…llets fdb8dc8 gui: Show watchonly balance only for Legacy wallets (Andrew Chow) Pull request description: Descriptor wallets do not have a watchonly balance as wallets are designated watchonly or not. Thus we should not be displaying the empty watchonly balance for descriptor wallets. The result is that instead of the send page showing "Watch-only balance: 0.00000000 BTC" for watchonly descriptor wallets, we see the actual balance as "Balance: 10.00000000 BTC" ACKs for top commit: johnny9: tACK fdb8dc8 furszy: ACK fdb8dc8 hebasto: ACK fdb8dc8 Tree-SHA512: e5c0703a62d25c881c8dadfb9cffd482791f3d437a4ec5ae0088ce1a2069c2455ad6d3ec6c95a4404a3b55fbd727f92694529c35052236951553ca90c4ed31b5
…d `wallets/` subdir c9ba4f9 test: Add test for file system permissions (Hennadii Stepanov) 581f16e Apply default umask in `SetupEnvironment()` (Hennadii Stepanov) 8a6219e Remove `-sysperms` option (Hennadii Stepanov) Pull request description: On master (1e7564e) docs say: ``` $ ./src/bitcoind -help | grep -A 3 sysperms -sysperms Create new files with system default permissions, instead of umask 077 (only effective with disabled wallet functionality) ``` Basing on that, one could expect that running `bitcoind` first time will create data directory and `wallets/` subdirectory with safe 0700 permissions. But that is not the case: ``` $ stat .bitcoin | grep id Access: (0775/drwxrwxr-x) Uid: ( 1000/ hebasto) Gid: ( 1000/ hebasto) $ stat .bitcoin/wallets | grep id Access: (0775/drwxrwxr-x) Uid: ( 1000/ hebasto) Gid: ( 1000/ hebasto) ``` Both directories, in fact, are created with system default permissions. With this PR: ``` $ stat .bitcoin/wallets | grep id Access: (0700/drwx------) Uid: ( 1000/ hebasto) Gid: ( 1000/ hebasto) $ stat .bitcoin/wallets | grep id Access: (0700/drwx------) Uid: ( 1000/ hebasto) Gid: ( 1000/ hebasto) ``` --- This PR: - is alternative to bitcoin#13389 - fixes bitcoin#15902 - fixes bitcoin#22595 - closes bitcoin#13371 - reverts bitcoin#4286 Changes in behavior: removed `-sysperms` command-line argument / configure option. The related discussions are here: - bitcoin#13389 (comment) - bitcoin#13389 (comment) - bitcoin#13389 (comment) If users rely on non-default access permissions, they could use `chmod`. ACKs for top commit: john-moffett: ACK c9ba4f9 willcl-ark: ACK c9ba4f9 Tree-SHA512: 96c745339e6bd0e4d7bf65daf9a721e2e1945b2b0ab74ca0f66576d0dc358b5de8eb8cdb89fe2160f3b19c39d2798bb8b291784316085dc73a27102d3415bd57
7534723 docs: document c-style cast prohibition (Pasta) Pull request description: In the words of practicalswift: ``` A C-style cast is equivalent to try casting in the following order: const_cast(...) static_cast(...) const_cast(static_cast(...)) reinterpret_cast(...) const_cast(reinterpret_cast(...)) By using static_cast<T>(...) explicitly we avoid the possibility of an unintentional and dangerous reinterpret_cast. Furthermore static_cast<T>(...) allows for easier grepping of casts. For a more thorough discussion, see "ES.49: If you must use a cast, use a named cast" in the C++ Core Guidelines (Stroustrup & Sutter). ``` Modern tooling, specifically `-Wold-style-cast` can enable us to enforce never using C-style casts. I believe this is especially important due to the number of C-style casts the codebase is currently being used as a reinterpret_cast. reinterpret_casts are especially dangerous, and should never be done via C-style casts. Update the docs to suggest the use of named cast or functional casts. Top commit has no ACKs. Tree-SHA512: 29a98de396f0c78e32d8a1831319162203c4405a670da5add5da956fcc7df200a1cec162ef1cfac4ddfb02714b66406081d40ed435c7f0f28581cfa24d94fac1
e4e1790 Modernize rpcauth.py and its tests (Pieter Wuille) Pull request description: Use Python3 constructions, and f-strings. ACKs for top commit: jamesob: Github ACK bitcoin@e4e1790 Tree-SHA512: 005573d967e04400fec727f45739f138879be703e692745c0a639272d37d221d230f388de23f2615cb954bb47179fb46e53da0410ae9f0865319b91bb2dc01f4
61bb4e7 lint: enable E722 do not use bare except (Leonardo Lazzaro) Pull request description: Improve test code and enable E722 lint check. If you want to catch all exceptions that signal program errors, use except Exception: (bare except is equivalent to except BaseException:). Reference: https://peps.python.org/pep-0008/#programming-recommendations ACKs for top commit: MarcoFalke: lgtm ACK 61bb4e7 Tree-SHA512: c7497769d5745fa02c78a20f4a0e555d8d3996d64af6faf1ce28e22ac1d8be415b98e967294679007b7bda2a9fd04031a9d140b24201e00257ceadeb5c5d7665
87a08cb build: move rpc/request from util lib to common (fanquake) Pull request description: This is JSON RPC related code that doesn't need to be in util, and should not be required by the kernel. ACKs for top commit: TheCharlatan: ACK 87a08cb Tree-SHA512: 5f335be9f0f9ff02eff073af47558ecf505c1392c05f18ca24a065b12b8d92529ec3942d84978cc5028c38369c496ed0243653e1fa26d4db2fae26dfe55c3d65
To prevent linkage error
function `ConnectAndCallRPC(BaseRequestHandler*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, std::optional<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > const&)':
DASH/src/bitcoin-cli.cpp:788:(.text+0xf23): undefined reference to `GetAuthCookie(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*)'
/usr/bin/ld: dash_cli-bitcoin-cli.o: in function `GenerateToAddressRequestHandler::PrepareRequest(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&)':
DASH/src/bitcoin-cli.cpp:710:(.text._ZN31GenerateToAddressRequestHandler14PrepareRequestERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEERKSt6vectorIS5_SaIS5_EE[_ZN31GenerateToAddressRequestHandler14PrepareRequestERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEERKSt6vectorIS5_SaIS5_EE]+0x15d): undefined reference to `JSONRPCRequestObj(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, UniValue const&, UniValue const&)'
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/functional/test_runner.py (1)
731-741: Modernization looks good, but there's an inconsistency in the retry path.Line 734 correctly uses the modern
text=True, but the retry code path at line 784 still usesuniversal_newlines=True. Consider updating line 784 as well for consistency across the file.♻️ Suggested fix for consistency
self.jobs.append((name, time.time(), subprocess.Popen([sys.executable, self.tests_dir + test_argv[0]] + test_argv[1:] + self.flags + portseed_arg + tmpdir_arg, - universal_newlines=True, + text=True, stdout=log_stdout, stderr=log_stderr),
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
contrib/devtools/github-merge.pycontrib/devtools/optimize-pngs.pycontrib/devtools/security-check.pycontrib/devtools/update-translations.pydoc/developer-notes.mddoc/init.mdshare/rpcauth/rpcauth.pysrc/Makefile.amsrc/i2p.cppsrc/init.cppsrc/qt/sendcoinsdialog.cppsrc/rpc/request.cppsrc/util/system.cppsrc/wallet/init.cpptest/functional/feature_dbcrash.pytest/functional/feature_llmq_connections.pytest/functional/feature_llmq_is_retroactive.pytest/functional/feature_posix_fs_permissions.pytest/functional/p2p_node_network_limited.pytest/functional/rpc_preciousblock.pytest/functional/test_framework/p2p.pytest/functional/test_framework/test_framework.pytest/functional/test_framework/test_node.pytest/functional/test_framework/util.pytest/functional/test_runner.pytest/lint/lint-python.pytest/util/rpcauth-test.pytest/util/test_runner.py
💤 Files with no reviewable changes (2)
- src/init.cpp
- src/wallet/init.cpp
✅ Files skipped from review due to trivial changes (1)
- src/rpc/request.cpp
🚧 Files skipped from review as they are similar to previous changes (9)
- test/util/rpcauth-test.py
- test/lint/lint-python.py
- test/functional/test_framework/p2p.py
- doc/developer-notes.md
- src/util/system.cpp
- src/Makefile.am
- test/functional/feature_posix_fs_permissions.py
- doc/init.md
- src/i2p.cpp
🧰 Additional context used
📓 Path-based instructions (4)
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests in test/functional/ must be written in Python (minimum version specified in .python-version) and depend on dashd and dash-node
Files:
test/functional/test_framework/test_node.pytest/functional/test_framework/util.pytest/functional/feature_llmq_connections.pytest/functional/test_runner.pytest/functional/test_framework/test_framework.pytest/functional/feature_dbcrash.pytest/functional/feature_llmq_is_retroactive.pytest/functional/p2p_node_network_limited.pytest/functional/rpc_preciousblock.py
src/**/*.{cpp,h,hpp,cc}
📄 CodeRabbit inference engine (CLAUDE.md)
Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1
Files:
src/qt/sendcoinsdialog.cpp
src/qt/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
GUI implementation in src/qt/ must use Qt 5
Files:
src/qt/sendcoinsdialog.cpp
{guix-build*,releases,**/guix-build*,releases/**,.github/**,depends/**,ci/**,contrib/**,doc/**}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not make changes to build system files (guix-build*), release artifacts, or avoid changes to .github, depends, ci, contrib, and doc directories unless specifically prompted
Files:
contrib/devtools/optimize-pngs.pycontrib/devtools/security-check.pycontrib/devtools/github-merge.pycontrib/devtools/update-translations.py
🧠 Learnings (5)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.
Learnt from: knst
Repo: dashpay/dash PR: 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.
Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Learnt from: kwvg
Repo: dashpay/dash PR: 7072
File: src/qt/walletcontroller.cpp:520-528
Timestamp: 2025-12-17T13:58:26.891Z
Learning: In Dash Qt wallet code, when leveraging upstream Bitcoin Core wallet capabilities (especially for operations like rescanning), prefer using the inherited signal-based mechanisms (e.g., ShowProgress signals) over adding explicit Qt progress dialogs to minimize upstream deviation and simplify future backports.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
📚 Learning: 2025-12-17T13:58:19.813Z
Learnt from: kwvg
Repo: dashpay/dash PR: 7072
File: src/qt/walletcontroller.cpp:520-528
Timestamp: 2025-12-17T13:58:19.813Z
Learning: In Dash Qt wallet code, prefer using existing Qt signals/slots (e.g., ShowProgress signals) for long-running operations instead of adding explicit QProgressDialog dialogs. This minimizes upstream deviations and simplifies backports. Apply this pattern to wallet-related Qt components (e.g., operations like rescanning) and rely on signal-based progress updates rather than bespoke dialogs.
Applied to files:
src/qt/sendcoinsdialog.cpp
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Applied to files:
test/functional/test_framework/util.pytest/functional/feature_llmq_connections.pytest/functional/test_framework/test_framework.pytest/functional/feature_dbcrash.pytest/functional/feature_llmq_is_retroactive.pytest/functional/p2p_node_network_limited.pytest/functional/rpc_preciousblock.py
📚 Learning: 2025-08-10T13:52:46.289Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6666
File: test/functional/rpc_netinfo.py:73-81
Timestamp: 2025-08-10T13:52:46.289Z
Learning: In the Bitcoin/Dash test framework, `self.node.extra_args` contains the original startup arguments for a TestNode and is not modified by `test.restart_node()`. Each restart with new `extra_args` doesn't update the stored `self.node.extra_args`, so copying from it always provides a clean slate without accumulated modifications from previous restarts.
Applied to files:
test/functional/test_framework/test_framework.py
📚 Learning: 2025-08-19T18:53:02.863Z
Learnt from: knst
Repo: dashpay/dash PR: 6824
File: test/functional/feature_llmq_is_retroactive.py:34-37
Timestamp: 2025-08-19T18:53:02.863Z
Learning: The `check_no_is` method in `test/functional/feature_llmq_is_retroactive.py` uses assertions for validation and doesn't return any value - it either succeeds silently or raises an AssertionError when the InstantLock assertion fails.
Applied to files:
test/functional/feature_llmq_is_retroactive.py
🧬 Code graph analysis (2)
test/functional/test_framework/test_node.py (1)
test/functional/test_framework/util.py (1)
assert_equal(69-74)
test/functional/test_framework/test_framework.py (1)
test/functional/test_framework/test_node.py (1)
replace_in_config(430-443)
🪛 Ruff (0.14.10)
test/functional/test_framework/test_node.py
875-875: subprocess call: check for execution of untrusted input
(S603)
958-958: Do not catch blind exception: Exception
(BLE001)
test/util/test_runner.py
57-57: Do not catch blind exception: Exception
(BLE001)
115-115: subprocess call: check for execution of untrusted input
(S603)
test/functional/test_framework/util.py
477-478: try-except-pass detected, consider logging the exception
(S110)
477-477: Do not catch blind exception: Exception
(BLE001)
499-500: try-except-pass detected, consider logging the exception
(S110)
499-499: Do not catch blind exception: Exception
(BLE001)
contrib/devtools/optimize-pngs.py
53-53: Do not catch blind exception: Exception
(BLE001)
test/functional/feature_llmq_connections.py
94-95: try-except-pass detected, consider logging the exception
(S110)
94-94: Do not catch blind exception: Exception
(BLE001)
108-109: try-except-pass detected, consider logging the exception
(S110)
108-108: Do not catch blind exception: Exception
(BLE001)
contrib/devtools/security-check.py
37-37: Do not catch blind exception: Exception
(BLE001)
test/functional/test_runner.py
647-647: subprocess call: check for execution of untrusted input
(S603)
test/functional/test_framework/test_framework.py
1043-1043: Avoid specifying long messages outside the exception class
(TRY003)
1608-1608: Do not catch blind exception: Exception
(BLE001)
1691-1691: Do not catch blind exception: Exception
(BLE001)
1939-1939: Do not catch blind exception: Exception
(BLE001)
1950-1950: Do not catch blind exception: Exception
(BLE001)
contrib/devtools/github-merge.py
309-310: try-except-pass detected, consider logging the exception
(S110)
309-309: Do not catch blind exception: Exception
(BLE001)
contrib/devtools/update-translations.py
59-59: Do not catch blind exception: Exception
(BLE001)
test/functional/feature_dbcrash.py
88-88: Do not catch blind exception: Exception
(BLE001)
test/functional/feature_llmq_is_retroactive.py
40-40: Do not catch blind exception: Exception
(BLE001)
test/functional/p2p_node_network_limited.py
86-87: try-except-pass detected, consider logging the exception
(S110)
86-86: Do not catch blind exception: Exception
(BLE001)
test/functional/rpc_preciousblock.py
19-19: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (10)
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (33)
contrib/devtools/update-translations.py (1)
57-61: LGTM - Exception handling improvement aligns with upstream backport.The change from bare
except:toexcept Exception:is an appropriate narrowing that prevents catchingSystemExit,KeyboardInterrupt, andGeneratorExit. TheIndexErrorthat could occur when the string ends with%is properly caught.Regarding the Ruff BLE001 warning: while a more specific
except IndexError:would be ideal, this matches the upstream Bitcoin Core backport and should be preserved for backport fidelity. Based on learnings, backported changes should match the original upstream PRs.contrib/devtools/github-merge.py (1)
307-310: LGTM - Narrowing exception handling is an improvement.The change from bare
except:toexcept Exception:is appropriate. It still catches standard exceptions while allowingKeyboardInterruptandSystemExitto propagate correctly, which is better behavior.Static analysis flags this as a silent exception (try-except-pass), but the context justifies it: this is an optional display step that shouldn't abort the workflow if it fails. Since this is a backport from Bitcoin Core, the implementation matches upstream.
src/qt/sendcoinsdialog.cpp (1)
788-797: LGTM! Correct fix for descriptor wallet watch-only balance display.The added
isLegacy()check ensures watch-only balance is only shown for Legacy wallets. Descriptor wallets handle watch-only status differently, and without this guard they would incorrectly displaywatch_only_balanceinstead of the regularbalance. This correctly backports bitcoin-core/gui#653.contrib/devtools/optimize-pngs.py (1)
53-55: LGTM - Appropriate exception handling improvement.Replacing bare
except:withexcept Exception:is correct modernization—it avoids inadvertently catchingSystemExit,KeyboardInterrupt, andGeneratorExit. The static analysis hint (BLE001) suggests catching more specific exceptions, but for a subprocess call that can fail in various ways, catchingExceptionis pragmatic here and matches the upstream backport.contrib/devtools/security-check.py (1)
33-38: LGTM - Valid exception handling modernization.The change from bare
except:toexcept Exception:is a correct backport improvement. When querying LIEF binary properties, various exceptions can occur, and falling back tohave_bindnow = Falseis the appropriate behavior. The Ruff BLE001 warning is noted, but catchingExceptionhere is reasonable given LIEF's API can raise different exception types, and this matches the upstream change.share/rpcauth/rpcauth.py (4)
8-8: LGTM: Secure randomness viasecretsmodule.Using
secretsis the recommended approach for cryptographic purposes per PEP 506, and aligns with the Python 3.7+ minimum requirement.
11-17: LGTM: Salt and password generation improvements.Both functions now use the
secretsmodule's purpose-built functions for cryptographic tokens.token_hexandtoken_urlsafeprovide equivalent security guarantees to the previousos.urandom+ encoding approach with cleaner semantics.
19-21: LGTM: Cleaner HMAC construction.Using
.encode('utf-8')is functionally equivalent tobytearray(str, 'utf-8')and is the idiomatic Python approach for string-to-bytes conversion.
39-40: LGTM: f-string formatting.Modern Python formatting that improves readability.
test/functional/feature_llmq_connections.py (2)
94-95: LGTM – narrowed exception handling improves correctness.Replacing bare
except:withexcept Exception:is the right fix: it avoids catchingSystemExit/KeyboardInterrupt, aligning with the E722 lint rule enabled in this backport.Regarding the Ruff hints (S110, BLE001): the
try-except-passpattern is intentional here—the test tolerates occasional failures as long as at least one iteration succeeds, and the pre-existing pattern is outside the scope of this backport.
108-109: LGTM – consistent with the earlier change.Same rationale applies: narrowing the exception clause to
except Exception:is the correct modernization per E722, and the intentionalpasspattern is unchanged.test/util/test_runner.py (3)
53-59: LGTM!The change from bare
except:toexcept Exception:correctly narrows exception handling. This allows system-exiting exceptions (SystemExit,KeyboardInterrupt) to propagate while still catching test failures. The Ruff BLE001 warning is acceptable here—catching broadExceptionis intentional for test frameworks that need to continue running after individual test failures.
101-106: LGTM!Same correct modernization as above—narrowing from bare
except:toexcept Exception:while preserving the log-and-reraise behavior for file I/O errors.
114-115: LGTM!The
text=Trueparameter is the modern (Python 3.7+) alias foruniversal_newlines=True—functionally identical but more descriptive. The S603 static analysis warning is a false positive; this test runner is designed to execute locally-built test binaries.test/functional/rpc_preciousblock.py (1)
19-19: LGTM!Narrowing from bare
except:toexcept Exception:is correct. This prevents inadvertently catchingSystemExit,KeyboardInterrupt, andGeneratorExit, while still handling the expected case wheregetblockraises an exception for a block not present on the destination node.The static analysis hint (BLE001) is a false positive here—
except Exception:is the appropriate pattern for this test helper.test/functional/p2p_node_network_limited.py (1)
84-89: LGTM!The narrowing to
except Exception:is appropriate. Thetry-except-passpattern here is intentional test logic: the test deliberately expectssync_blocksto fail (node 2 is in IBD, node 0 isNODE_NETWORK_LIMITED), and the subsequent assertion verifies that node 2 remains at height 0.The static analysis hints (S110, BLE001) are false positives for this test scenario.
test/functional/feature_dbcrash.py (1)
88-94: LGTM!The change to
except Exception:is correct for this crash-recovery test scenario. Therestart_nodemethod deliberately catches exceptions from RPC calls that may fail due to the node crashing (triggered by-dbcrashratio), then retries. Narrowing from bareexcept:appropriately excludesSystemExit/KeyboardInterruptwhile still handling the expected crash-induced exceptions.test/functional/feature_llmq_is_retroactive.py (1)
37-41: LGTM!The narrowing to
except Exception:is appropriate for this helper function. Thecheck_txinner function checks whether a transaction exists via RPC, returningFalseon any exception (e.g., whengetrawtransactionraises because the tx isn't found). This correctly excludesKeyboardInterruptand similar non-error exceptions.test/functional/test_runner.py (2)
228-228: LGTM!The new
feature_posix_fs_permissions.pytest is correctly added toBASE_SCRIPTS, ensuring it runs as part of the default test suite. This aligns with the PR's addition of POSIX filesystem permissions enforcement.
647-647: LGTM!
text=Trueis the modern, more readable alias foruniversal_newlines=True(available since Python 3.7). This is a straightforward modernization with identical behavior.test/functional/test_framework/util.py (3)
285-287: LGTM - Exception handling modernization.Replacing bare
except:withexcept Exception:aligns with Python best practices (PEP 8) and the upstream Bitcoin Core backport. This maintains the existing behavior while being more explicit and avoiding catching system-exiting exceptions likeKeyboardInterruptandSystemExit.
477-478: Backported exception handling update.The change from bare
except:toexcept Exception:is consistent with the upstream backport. The static analysis hint abouttry-except-pass(S110) and blind exception (BLE001) can be safely ignored here - this pattern is intentional for silently handling copy failures incopy_datadir, preserving the original behavior while modernizing the exception handling syntax.
499-500: Backported exception handling update.Consistent with the other changes in this file - modernizing from bare
except:toexcept Exception:per upstream Bitcoin Core backport. The existing pass-on-error behavior is preserved.test/functional/test_framework/test_node.py (3)
430-443: LGTM - Newreplace_in_confighelper method.Clean implementation of a configuration file modification helper. The method:
- Uses
assert_equalfor tuple validation, consistent with the test framework patterns- Performs in-place string replacements in the dash.conf file
- Properly handles file encoding
This provides a cleaner API for tests that need to modify node configuration compared to direct file manipulation.
875-875: LGTM - Modernized subprocess parameter.The change from
universal_newlines=Truetotext=Trueuses the modern Python 3.7+ parameter name. Both are functionally equivalent, buttext=Trueis the preferred naming convention in modern Python. This aligns with the PR objective of bumping the minimum Python version to 3.7 and modernizing Python code.
958-959: Backported exception handling update.Consistent with the pattern across this PR - replacing bare
except:withexcept Exception:. The existing behavior of falling back toaddr()descriptor on non-hex address input is preserved.test/functional/test_framework/test_framework.py (7)
568-568: LGTM - Refactored to use newreplace_in_configAPI.Good improvement that replaces direct file manipulation with the new centralized
replace_in_configmethod fromTestNode. This makes the pre-v16 configuration adjustment cleaner and more maintainable. The relevant code snippet fromtest_node.py(lines 429-442) confirms this method handles the replacement properly.
1040-1043: LGTM - New POSIX platform skip helper.The new
skip_if_platform_not_posixmethod follows the established pattern of otherskip_if_*methods in this class. Usingos.name != 'posix'is the correct approach for detecting POSIX platforms, which will be used by the new filesystem permissions tests added in this backport PR.
653-656: Backported exception handling update.Replacing bare
except:withexcept Exception:in thestart_nodesfailure handling path. This preserves the existing cleanup behavior (stopping nodes on failure) while being more explicit about the exception types caught.
1608-1609: Backported exception handling update.Consistent pattern for catching exceptions during masternode preparation. The exception is logged and the success flag is used to control test flow.
1691-1692: Backported exception handling update.Same pattern as above for EvoNode service updates - catch and log exceptions, then validate success based on expectations.
1939-1940: Backported exception handling update.The
except Exception:incheck_instantlockinner function handles cases wheregetrawtransactionmight fail (e.g., transaction not yet in mempool). ReturningFalseallows thewait_untilloop to continue polling.
1950-1951: Backported exception handling update.Similar to the instantlock check - handles transient failures when checking for chainlocked blocks. The pattern of catching exceptions and returning
Falseto continue polling is appropriate for these wait helper functions.
What was done?
Regular backports from Bitcoin Core v25
Please, note that partial Merge bitcoin#26294 is included due to fix bitcoin#26904 but if #7085 will get merged before this one, this PR should be rebased to exclude duplicated changes.
How Has This Been Tested?
Run unit & functional tests
Breaking Changes
N/A
Checklist: