backport: bitcoin#26257, #26480, #26600, #26601, #26660, #26689, #26802 #7089
Conversation
fa10f19 test: Set default in add_wallet_options if only one type can be chosen (MacroFake) 555519d test: Remove wallet option from non-wallet tests (MacroFake) fac8d59 test: Set -disablewallet when no wallet has been compiled (MacroFake) fa68937 test: Make requires_wallet private (MacroFake) Pull request description: The tests have several issues: * Some tests that are wallet-type specific offer the option to run the test with the incompatible type For example, `wallet_dump.py` offers `--descriptors` and on current master fails with `JSONRPCException: Invalid public key`. After the changes here, it fails with a clear error: `unrecognized arguments: --descriptors`. * Tests that don't use the wallet at all offer the option to run it with a wallet type. This is confusing and wastes developers time if they are "tricked" into running the test for both wallet types, even though no wallet code is executed at all. For example, `feature_addrman.py` will happily accept and run with `--descriptors` or `--legacy-wallet`. After the changes here, it no longer silently ignores the flag, but reports a clear error: `unrecognized arguments`. ACKs for top commit: achow101: ACK fa10f19 Tree-SHA512: a5784da7305f4ec58c0013f433289000d94fc3d434b00fc329ffa37b812e2cd1da0071e34c3462bf79d904808564f2ae6d3d582f6b86b26215f9b07391b58460
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThis pull request centralizes wallet option handling in the functional test framework by adding BitcoinTestFramework.add_wallet_options(...) and renaming the public requires_wallet attribute to an internal _requires_wallet. Parsing of wallet-type (descriptors vs legacy) is now configurable per-test via new add_options(self, parser) hooks added to many tests (~130 files). Test invocation scripts and the test runner were updated to use wallet-specific flags. Additionally, there are dependency version bumps in CI tooling (flake8, lief, codespell) and widespread stylistic assertion/parentheses formatting changes across tests. No new exported library APIs beyond test-framework changes. Sequence Diagram(s)sequenceDiagram
%% Styling for clarity
rect rgb(235,245,255)
participant Runner as Test Runner
participant Parser as ArgParser
participant TF as BitcoinTestFramework
participant Test as Functional Test
participant Node as TestNode
end
Runner->>Parser: start and collect test args
Parser->>Test: call add_options(parser) if present
Note right of Test: Many tests now implement\nadd_options -> add_wallet_options(...)
Test-->>TF: delegate to add_wallet_options(parser,...)
TF->>Parser: register mutually exclusive wallet flags\n(--descriptors / --legacy-wallet / --nohd etc.)
Parser-->>Runner: parsed options (descriptors/legacy)
Runner->>TF: instantiate test with parsed options
TF->>Node: create TestNode(args including descriptors flag or -disablewallet)
Node-->>TF: node started with wallet configuration
TF->>Test: run_test() using node(s) with chosen wallet mode
Note left of TF: Internal flag _requires_wallet governs\nwallet-related setup steps
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRsSuggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @ci/lint/04_install.sh:
- Around line 38-39: Replace the vulnerable pinned lief==0.13.2 install with a
safe version >=0.15.0: update the line that uses ${CI_RETRY_EXE} pip3 install
lief==0.13.2 to use ${CI_RETRY_EXE} pip3 install "lief>=0.15.0" (or pin to a
specific secure release >=0.15.0) so the CI installation command referenced by
CI_RETRY_EXE installs a non-vulnerable lief.
In @test/functional/test_framework/crypto/siphash.py:
- Line 34: Replace equality-based type checks with identity checks for the two
assertions in siphash.py: change the assertion that verifies the type of the
variable `data` to use `is` (i.e., assert type(data) is bytes) and similarly
change the assertion for `num` to use `is` (i.e., assert type(num) is int);
update the two `assert type(...) == ...` occurrences accordingly to satisfy the
E721 lint rule.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/lint/all-lint.py (1)
20-29: Update the parallel branch to consistently use the Python executable.The non-parallel branch (line 32) invokes lint scripts with
[executable, lint]to ensure they use the same Python interpreter as all-lint.py. However, the parallel branch (line 22) passes lint scripts directly to theparallelcommand, which executes them via their shebangs.This inconsistency contradicts the PR's objective. Update line 22 to invoke lint scripts with the Python executable for consistency:
Example approach
command = ["parallel", "--jobs", "100%", "--will-cite", "--joblog", logfile, ":::", executable] + [f"{executable} {lint}" for lint in lints]Or adjust the command construction to ensure lint scripts are invoked through the same Python executable as the non-parallel branch.
🧹 Nitpick comments (2)
ci/lint/04_install.sh (1)
36-36: Minor typo in path reference.The comment references
contrib/containers/ci//ci-slim.Dockerfilewith a double slash. The correct path should becontrib/containers/ci/ci-slim.Dockerfile.🔎 Proposed fix
-# NOTE: BUMP ALSO contrib/containers/ci//ci-slim.Dockerfile +# NOTE: BUMP ALSO contrib/containers/ci/ci-slim.Dockerfiletest/functional/wallet_importdescriptors.py (1)
461-462: LGTM: Assertion syntax improved.The assertions have been updated to use more direct and Pythonic membership checks, improving code readability without changing functionality.
| ${CI_RETRY_EXE} pip3 install flake8==5.0.4 | ||
| ${CI_RETRY_EXE} pip3 install lief==0.13.2 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the current state of the file
head -50 ci/lint/04_install.sh | tail -15
Repository: dashpay/dash
Length of output: 673
🌐 Web query:
flake8 5.0.4 security vulnerabilities CVE
💡 Result:
I found no public CVE entries for flake8 5.0.4. Flake8's own release notes for 5.0.4 list only a bugfix (importlib‑metadata bound) and no security advisory, and major vulnerability trackers (Snyk, Safety DB) show no direct CVE for that version. [1][2][3]
If you want, I can run a targeted CVE/NVD search (or check GitHub/security advisories) and return exact CVE database results.
🌐 Web query:
lief 0.13.2 security vulnerabilities CVE
💡 Result:
Yes — LIEF 0.13.2 is affected by CVE-2024-31636 (information disclosure via an uninitialized variable in machd_reader.c; affects versions < 0.15.0). Upgrade to >= 0.15.0 to obtain the fix. [1][2]
Sources:
[1] NVD entry for CVE-2024-31636.
[2] SafetyDB package page for lief (shows affected versions <0.15.0).
Upgrade lief to ≥0.15.0 to fix CVE-2024-31636.
lief 0.13.2 is vulnerable to CVE-2024-31636 (information disclosure via uninitialized variable in machd_reader.c). This affects all versions < 0.15.0. flake8 5.0.4 is secure with no known vulnerabilities.
🤖 Prompt for AI Agents
In @ci/lint/04_install.sh around lines 38-39, Replace the vulnerable pinned
lief==0.13.2 install with a safe version >=0.15.0: update the line that uses
${CI_RETRY_EXE} pip3 install lief==0.13.2 to use ${CI_RETRY_EXE} pip3 install
"lief>=0.15.0" (or pin to a specific secure release >=0.15.0) so the CI
installation command referenced by CI_RETRY_EXE installs a non-vulnerable lief.
|
|
||
| def siphash(k0, k1, data): | ||
| assert(type(data) == bytes) | ||
| assert type(data) == bytes |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the actual code at lines 34 and 64
cat -n test/functional/test_framework/crypto/siphash.py | sed -n '30,70p'
Repository: dashpay/dash
Length of output: 1336
🏁 Script executed:
# Check if there are any linter configuration files that define how this is treated
fd -type f "pyproject.toml|setup.cfg|tox.ini|.flake8|.ruff.toml|ruff.toml" | head -20
Repository: dashpay/dash
Length of output: 226
🏁 Script executed:
# Find linter configuration files
find . -maxdepth 3 -type f \( -name "pyproject.toml" -o -name "setup.cfg" -o -name "tox.ini" -o -name ".flake8" -o -name ".ruff*" \) 2>/dev/null
Repository: dashpay/dash
Length of output: 106
🏁 Script executed:
# Search for upstream Bitcoin Core v25 siphash.py on GitHub to verify the pattern
# Also check git history to see if this file was backported
git log --oneline --all test/functional/test_framework/crypto/siphash.py | head -10
Repository: dashpay/dash
Length of output: 384
🏁 Script executed:
# Check if there's any reference to Bitcoin Core upstream in commits
git log --all --grep="Bitcoin Core\|backport\|upstream" -- test/functional/test_framework/crypto/siphash.py | head -20
Repository: dashpay/dash
Length of output: 606
🏁 Script executed:
# Check if there's a main linter config that applies to test files
find . -maxdepth 2 -type f \( -name "pyproject.toml" -o -name ".flake8" -o -name "ruff.toml" \) | grep -v dashbls
Repository: dashpay/dash
Length of output: 67
🏁 Script executed:
# Check git log more carefully for the specific file origin
git log --all --oneline --follow -- test/functional/test_framework/crypto/siphash.py | head -20
Repository: dashpay/dash
Length of output: 918
🌐 Web query:
Bitcoin Core v25 siphash.py test framework crypto
💡 Result:
Do you want (A) the location/path of siphash.py in the Bitcoin Core v25 repo, (B) the file contents, or (C) how to run the siphash-related tests?
🏁 Script executed:
# Show what the E721 update commit changed
git show 38c3fd846b --stat | head -50
Repository: dashpay/dash
Length of output: 360
🏁 Script executed:
# Show the actual changes for siphash-related files in that commit
git show 38c3fd846b -- test/functional/test_framework/crypto/siphash.py 2>/dev/null || echo "Not in this commit"
Repository: dashpay/dash
Length of output: 38
🏁 Script executed:
# Check ruff.toml to see E721 settings
cat -n ruff.toml
Repository: dashpay/dash
Length of output: 1172
🏁 Script executed:
# Show the actual diff from the E721 update commit
git show 38c3fd846b
Repository: dashpay/dash
Length of output: 1684
🏁 Script executed:
# Check if there's a siphash.py in the old location (not in crypto/)
git ls-files test/functional/test_framework/ | grep -i siphash
Repository: dashpay/dash
Length of output: 104
Use is instead of == for type comparisons to comply with E721 linter rule.
Lines 34 and 64 should use the is operator instead of == for type comparisons:
- Line 34:
assert type(data) is bytes - Line 64:
assert type(num) is int
This matches the E721 fix that was previously applied to this code in commit 38c3fd8 and aligns with the active ruff.toml linter configuration.
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 34-34: do not compare types, for exact checks use is / is not, for instance checks use isinstance()
(E721)
🪛 Ruff (0.14.10)
34-34: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
🤖 Prompt for AI Agents
In @test/functional/test_framework/crypto/siphash.py around line 34, Replace
equality-based type checks with identity checks for the two assertions in
siphash.py: change the assertion that verifies the type of the variable `data`
to use `is` (i.e., assert type(data) is bytes) and similarly change the
assertion for `num` to use `is` (i.e., assert type(num) is int); update the two
`assert type(...) == ...` occurrences accordingly to satisfy the E721 lint rule.
…n only one type is allowed BACKPORT NOTE: some tests doesn't exist yet, such as wallet_taproot.py fadb869 test: Set wallet type in test_runner when only one type is allowed (MarcoFalke) Pull request description: Currently devs are free to set or not set the wallet type in the test_runner when only one type is allowed to be set. This is inconsistent and causes review comments such as: * bitcoin#24865 (comment) ACKs for top commit: achow101: ACK fadb869 Tree-SHA512: 1ca0946df07b5bf6778fea957d74393757781c324d554fec2f7d03bf1915033e644d9a4c3d77e0b24090ab593d7ed3cb3c9169666bc39fff423706fceaa1af80
fabb24c test: Use last release in compatibility tests (MarcoFalke) Pull request description: In compatibility tests it makes sense to always use the last release without the new feature, as it is likely more in use than any even older previous release. ACKs for top commit: Sjors: utACK fabb24c Tree-SHA512: beb854f4d28ba313282e1e0303abb0e09377828b138bde5a3e209337210b6b4c24855ab90a68f8789387001e4ca33b15cc37dbc9b7809929f4e7d1b69833a527
…tuted by v19.3.0)
fa7d71a test: Move rpc_fundrawtransaction.py to wallet_fundrawtransaction.py (MarcoFalke) fa933d6 test: Move feature_backwards_compatibility.py to wallet_backwards_compatibility.py (MarcoFalke) Pull request description: The tests only tests the wallet and it doesn't make sense to extend it for other stuff, so clarify that. ACKs for top commit: fanquake: ACK fa7d71a pablomartin4btc: re-ACK bitcoin@fa7d71a Tree-SHA512: 9dc131ed8ff119bd6d43d04ecc5c96d02f2d6fdc3d0805492774a414c0fbe6a984da7631154122a080c54ddd25fc5a5bdd52b282c918fce4932057ba72c2bf71
bcb7123 test: add add_wallet_options to TestShell (josibake) Pull request description: following bitcoin@555519d, `TestShell` now always runs with `-disablewallet`. simple fix is to add `add_wallet_options` to `add_options`; looks like testshell was overlooked when adding in the `add_wallet_options` call to the functional tests in bitcoin#26480 ACKs for top commit: amitiuttarwar: ACK bcb7123 Tree-SHA512: db554a8b3c8ff5bd10cab9592b316035a92f86a0a0ae8ff914de9687bbbb6fc2235bdf82c4cd40e4071782f8b6edf91aad372e82ed3b826c9d8ae39dbe3dbf57
…pdate dependencies 1e5e87c script: update python linter dependencies (Jon Atack) 459cb63 script, test: fix python linter E275 errors with flake8 5.0.4 (Jon Atack) Pull request description: It is helpful to be able to run the python linter locally to review PRs and check local diffs and work. Fix the errors raised by `./test/lint/lint-python.py` when run locally with flake8 5.0.4, which enforces rule E275 more strictly than previous versions, and update our python linter CI dependencies. ACKs for top commit: kristapsk: ACK 1e5e87c Tree-SHA512: c7da09396144b9fed4ce6405c0763b2e3e5651d49a5220b053da465aceca09f754fb70a8ab9647278ce2028dde803bea461a3cd93fb39868ead39d06187cd8af
…s as for `all-lint.py` f6eadaa Use same Python executable for subprocesses as for all-lint.py (Kristaps Kaupe) Pull request description: Before this all linters were ran by `/usr/bin/env python3`, no matter what was used to run `test/lint/all-lint.py`. This change allows to use non-default Python executable for `test/lint/all-lint.py` and then all subprocesses will also use same Python interpreter (for example, `python3.10 ./test/lint/all-lint.py`). See bitcoin#26792 (comment) as use case. ACKs for top commit: fanquake: ACK f6eadaa - did not test Tree-SHA512: 4da3b5581a0dd8ab9a6387829495019091a93a7ceaf2135d65d40a1983fd11a0b92b20891ef30d2a132abb0a690cd9b2f7eb5fcc38df06a340394ef449d640af
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/lint/all-lint.py (1)
20-35: Addexecutableto parallel command for consistency with PR#26802 intent.The parallel branch (line 22) should explicitly pass the Python interpreter via
executable, matching the non-parallel branch (line 32). The upstream Bitcoin Core PR#26802 specifically aims to "use same Python executable for subprocesses" to avoid virtualenv/PATH mismatches. The parallel optimization in Dash should follow this same principle:Suggested change
command = ["parallel", "--jobs", "100%", "--will-cite", "--joblog", logfile, executable, ":::"] + lints
♻️ Duplicate comments (1)
test/functional/test_framework/crypto/siphash.py (1)
34-34: Useisinstead of==for type comparisons to satisfy E721 linter rule.Both assertions use
==for type comparisons, which violates the E721 linter rule enforced by the active ruff configuration. This issue was previously flagged and should be addressed.🔎 Proposed fix
- assert type(data) == bytes + assert type(data) is bytes- assert type(num) == int + assert type(num) is intAlso applies to: 64-64
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (73)
ci/lint/04_install.shcontrib/containers/ci/ci-slim.Dockerfilecontrib/linearize/linearize-hashes.pycontrib/seeds/generate-seeds.pytest/functional/feature_addressindex.pytest/functional/feature_asset_locks.pytest/functional/feature_coinstatsindex.pytest/functional/feature_dip3_deterministicmns.pytest/functional/feature_dip3_v19.pytest/functional/feature_dip4_coinbasemerkleroots.pytest/functional/feature_governance.pytest/functional/feature_governance_cl.pytest/functional/feature_governance_objects.pytest/functional/feature_index_prune.pytest/functional/feature_llmq_chainlocks.pytest/functional/feature_llmq_connections.pytest/functional/feature_llmq_data_recovery.pytest/functional/feature_llmq_dkgerrors.pytest/functional/feature_llmq_evo.pytest/functional/feature_llmq_is_cl_conflicts.pytest/functional/feature_llmq_is_retroactive.pytest/functional/feature_llmq_rotation.pytest/functional/feature_llmq_signing.pytest/functional/feature_llmq_simplepose.pytest/functional/feature_llmq_singlenode.pytest/functional/feature_masternode_params.pytest/functional/feature_mnehf.pytest/functional/feature_spentindex.pytest/functional/feature_txindex.pytest/functional/feature_unsupported_utxo_db.pytest/functional/interface_usdt_utxocache.pytest/functional/interface_usdt_validation.pytest/functional/interface_zmq_dash.pytest/functional/mempool_compatibility.pytest/functional/mocks/invalid_signer.pytest/functional/mocks/signer.pytest/functional/p2p_addr_relay.pytest/functional/p2p_blocksonly.pytest/functional/p2p_getaddr_caching.pytest/functional/p2p_instantsend.pytest/functional/p2p_message_capture.pytest/functional/p2p_platform_ban.pytest/functional/p2p_quorum_data.pytest/functional/rpc_blockchain.pytest/functional/rpc_coinjoin.pytest/functional/rpc_masternode.pytest/functional/rpc_mnauth.pytest/functional/rpc_net.pytest/functional/rpc_netinfo.pytest/functional/rpc_quorum.pytest/functional/rpc_verifychainlock.pytest/functional/rpc_verifyislock.pytest/functional/rpc_wipewallettxes.pytest/functional/test_framework/crypto/siphash.pytest/functional/test_framework/key.pytest/functional/test_framework/test_framework.pytest/functional/test_framework/test_node.pytest/functional/test_framework/test_shell.pytest/functional/test_runner.pytest/functional/wallet_avoidreuse.pytest/functional/wallet_backwards_compatibility.pytest/functional/wallet_change_address.pytest/functional/wallet_conflicts.pytest/functional/wallet_fundrawtransaction.pytest/functional/wallet_importdescriptors.pytest/functional/wallet_keypool_hd.pytest/functional/wallet_mnemonicbits.pytest/functional/wallet_reorgsrestore.pytest/functional/wallet_send.pytest/functional/wallet_signer.pytest/functional/wallet_upgradetohd.pytest/get_previous_releases.pytest/lint/all-lint.py
🚧 Files skipped from review as they are similar to previous changes (51)
- test/functional/feature_llmq_chainlocks.py
- test/functional/feature_governance_cl.py
- test/functional/wallet_send.py
- test/functional/rpc_mnauth.py
- ci/lint/04_install.sh
- test/functional/wallet_backwards_compatibility.py
- test/functional/feature_llmq_data_recovery.py
- test/functional/test_framework/test_shell.py
- test/functional/feature_asset_locks.py
- test/functional/feature_governance_objects.py
- test/functional/feature_mnehf.py
- test/functional/feature_dip4_coinbasemerkleroots.py
- test/functional/interface_zmq_dash.py
- test/functional/wallet_keypool_hd.py
- test/functional/feature_llmq_evo.py
- test/functional/mempool_compatibility.py
- test/functional/mocks/signer.py
- test/functional/wallet_change_address.py
- test/functional/feature_llmq_rotation.py
- test/functional/rpc_netinfo.py
- test/functional/rpc_quorum.py
- test/functional/p2p_quorum_data.py
- test/functional/p2p_platform_ban.py
- test/functional/interface_usdt_utxocache.py
- test/functional/feature_masternode_params.py
- test/functional/test_framework/test_node.py
- test/functional/feature_index_prune.py
- test/functional/wallet_avoidreuse.py
- test/functional/mocks/invalid_signer.py
- test/functional/p2p_instantsend.py
- test/functional/feature_llmq_singlenode.py
- test/functional/feature_llmq_simplepose.py
- test/functional/p2p_addr_relay.py
- test/functional/p2p_getaddr_caching.py
- test/functional/wallet_importdescriptors.py
- test/functional/rpc_masternode.py
- test/functional/feature_governance.py
- test/functional/feature_dip3_deterministicmns.py
- test/functional/feature_unsupported_utxo_db.py
- contrib/linearize/linearize-hashes.py
- test/functional/feature_coinstatsindex.py
- test/functional/feature_llmq_signing.py
- test/functional/feature_addressindex.py
- test/functional/wallet_fundrawtransaction.py
- test/functional/feature_llmq_dkgerrors.py
- test/functional/feature_dip3_v19.py
- test/functional/interface_usdt_validation.py
- test/functional/wallet_reorgsrestore.py
- test/functional/p2p_blocksonly.py
- test/get_previous_releases.py
- test/functional/rpc_verifychainlock.py
🧰 Additional context used
📓 Path-based instructions (2)
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/wallet_upgradetohd.pytest/functional/wallet_signer.pytest/functional/rpc_wipewallettxes.pytest/functional/test_framework/test_framework.pytest/functional/test_framework/key.pytest/functional/test_framework/crypto/siphash.pytest/functional/wallet_mnemonicbits.pytest/functional/feature_llmq_is_cl_conflicts.pytest/functional/feature_llmq_connections.pytest/functional/feature_spentindex.pytest/functional/rpc_blockchain.pytest/functional/rpc_net.pytest/functional/test_runner.pytest/functional/feature_llmq_is_retroactive.pytest/functional/rpc_verifyislock.pytest/functional/rpc_coinjoin.pytest/functional/feature_txindex.pytest/functional/p2p_message_capture.pytest/functional/wallet_conflicts.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/containers/ci/ci-slim.Dockerfilecontrib/seeds/generate-seeds.py
🧠 Learnings (8)
📓 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: 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: 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-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:
test/functional/wallet_upgradetohd.pytest/functional/test_framework/test_framework.pytest/functional/test_runner.py
📚 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/test_framework.pytest/functional/rpc_blockchain.pytest/functional/test_runner.pytest/functional/p2p_message_capture.pycontrib/seeds/generate-seeds.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/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: Always run linting before commits using test/lint/all-lint.py
Applied to files:
test/lint/all-lint.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/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Applied to files:
test/functional/rpc_coinjoin.py
📚 Learning: 2025-10-13T12:37:12.357Z
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.
Applied to files:
contrib/seeds/generate-seeds.py
📚 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:
contrib/seeds/generate-seeds.py
🧬 Code graph analysis (6)
test/functional/wallet_upgradetohd.py (1)
test/functional/test_framework/test_framework.py (2)
add_options(420-422)add_wallet_options(501-514)
test/functional/wallet_signer.py (5)
test/functional/wallet_avoidreuse.py (1)
add_options(66-67)test/functional/wallet_importdescriptors.py (1)
add_options(31-32)test/functional/wallet_send.py (1)
add_options(23-24)test/functional/test_framework/test_framework.py (2)
add_options(420-422)add_wallet_options(501-514)test/functional/mempool_persist.py (1)
add_options(51-52)
test/functional/wallet_mnemonicbits.py (1)
test/functional/test_framework/test_framework.py (2)
add_options(420-422)add_wallet_options(501-514)
test/functional/feature_llmq_connections.py (1)
test/functional/test_framework/test_framework.py (2)
add_options(420-422)add_wallet_options(501-514)
test/functional/rpc_coinjoin.py (16)
test/functional/test_framework/test_framework.py (2)
add_options(420-422)add_wallet_options(501-514)test/functional/feature_addressindex.py (1)
add_options(26-27)test/functional/feature_asset_locks.py (1)
add_options(53-54)test/functional/feature_dip3_deterministicmns.py (1)
add_options(23-24)test/functional/feature_dip3_v19.py (1)
add_options(48-49)test/functional/feature_dip4_coinbasemerkleroots.py (1)
add_options(50-51)test/functional/feature_governance.py (1)
add_options(20-21)test/functional/feature_governance_cl.py (1)
add_options(15-16)test/functional/feature_governance_objects.py (1)
add_options(25-26)test/functional/feature_llmq_chainlocks.py (1)
add_options(22-23)test/functional/feature_llmq_connections.py (1)
add_options(22-23)test/functional/feature_llmq_data_recovery.py (1)
add_options(30-31)test/functional/feature_llmq_dkgerrors.py (1)
add_options(25-26)test/functional/feature_llmq_evo.py (1)
add_options(51-52)test/functional/feature_llmq_is_cl_conflicts.py (1)
add_options(51-52)test/functional/feature_llmq_is_retroactive.py (1)
add_options(23-24)
test/functional/feature_txindex.py (16)
test/functional/test_framework/test_framework.py (2)
add_options(420-422)add_wallet_options(501-514)test/functional/feature_addressindex.py (1)
add_options(26-27)test/functional/feature_asset_locks.py (1)
add_options(53-54)test/functional/feature_dip3_deterministicmns.py (1)
add_options(23-24)test/functional/feature_dip3_v19.py (1)
add_options(48-49)test/functional/feature_dip4_coinbasemerkleroots.py (1)
add_options(50-51)test/functional/feature_governance.py (1)
add_options(20-21)test/functional/feature_governance_cl.py (1)
add_options(15-16)test/functional/feature_governance_objects.py (1)
add_options(25-26)test/functional/feature_llmq_chainlocks.py (1)
add_options(22-23)test/functional/feature_llmq_connections.py (1)
add_options(22-23)test/functional/feature_llmq_data_recovery.py (1)
add_options(30-31)test/functional/feature_llmq_dkgerrors.py (1)
add_options(25-26)test/functional/feature_llmq_evo.py (1)
add_options(51-52)test/functional/feature_llmq_is_cl_conflicts.py (1)
add_options(51-52)test/functional/feature_llmq_is_retroactive.py (1)
add_options(23-24)
🪛 Flake8 (7.3.0)
test/functional/test_framework/crypto/siphash.py
[error] 34-34: do not compare types, for exact checks use is / is not, for instance checks use isinstance()
(E721)
[error] 64-64: do not compare types, for exact checks use is / is not, for instance checks use isinstance()
(E721)
🪛 Ruff (0.14.10)
test/lint/all-lint.py
32-32: subprocess call: check for execution of untrusted input
(S603)
test/functional/test_framework/crypto/siphash.py
34-34: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
64-64: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
⏰ 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_ubsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (31)
test/functional/p2p_message_capture.py (1)
39-39: LGTM! Stylistic improvement to assert statements.The removal of unnecessary parentheses from the assert statements is a valid stylistic cleanup backported from Bitcoin Core. The assertions remain functionally identical.
Also applies to: 47-47
test/functional/rpc_blockchain.py (1)
366-366: LGTM! Stylistic improvement.Removing unnecessary parentheses from the assert statement follows Python best practices, as
assertis a statement rather than a function call.test/functional/test_framework/key.py (1)
132-132: LGTM! Stylistic improvement to assert statements.The removal of unnecessary parentheses from
assertstatements follows Python best practices, asassertis a statement rather than a function. These changes are purely stylistic with no functional impact.Also applies to: 145-145, 158-158, 169-169
test/functional/feature_llmq_is_retroactive.py (1)
23-24: LGTM! Wallet option handling correctly added.The new
add_optionsmethod appropriately delegates toself.add_wallet_options(parser), which is necessary since this test uses wallet functionality extensively (e.g.,getnewaddress(),signrawtransactionwithwallet(),sendtoaddress()). The implementation aligns with the Bitcoin Core backport pattern for centralizing wallet option handling across the test framework.test/functional/rpc_coinjoin.py (1)
28-30: LGTM!The addition of
add_optionsthat delegates toself.add_wallet_options(parser)is consistent with the pattern established across the test suite and aligns with the PR objective of centralizing wallet option handling. Since this test creates and manages wallets, exposing wallet-type CLI options is appropriate.test/functional/wallet_mnemonicbits.py (1)
14-16: LGTM! Wallet option integration is correct.The
add_optionsmethod properly delegates toself.add_wallet_options(parser)with default arguments that allow both descriptor and legacy wallet types. This correctly integrates with the test's existing use ofself.options.descriptors(line 37) and its conditional logic for both wallet types (lines 37-64).contrib/seeds/generate-seeds.py (1)
66-66: LGTM! Stylistic assertion formatting improvements.The assertion formatting changes improve Python style consistency by removing unnecessary parentheses and following Python best practices where
assertis a statement, not a function call. The changes are semantically equivalent to the original code.Also applies to: 72-72
contrib/containers/ci/ci-slim.Dockerfile (1)
78-81: Both package versions are valid and have no security advisories.The versions
codespell 2.2.1andflake8 5.0.4are confirmed on PyPI with no associated CVEs or security advisories. The added synchronization note on line 78 is a helpful maintainability improvement. The version bumps are consistent with the backport from Bitcoin Core v25.test/functional/feature_spentindex.py (1)
28-29: LGTM! Wallet options wiring matches backport pattern.The addition of
add_optionsdelegating toadd_wallet_options(parser)is consistent with the broader PR objective of centralizing wallet option handling across functional tests.test/functional/test_runner.py (5)
30-31: LGTM! Environment variable enables explicit wallet type specification.The
REQUIRE_WALLET_TYPE_SETenvironment variable is referenced intest_framework.py(line 508) to enforce explicit wallet type flags in test invocations, ensuring consistency across test runs.
108-110: LGTM! Test invocations updated for wallet-specific configurations.The test invocations properly specify wallet types (legacy with/without HD, and descriptors), aligning with the centralized wallet option handling introduced by this backport.
190-190: LGTM! Explicit wallet type specification added.The
--descriptorsflag explicitly specifies the wallet type for this test invocation.
270-270: LGTM! Test entry follows wallet specification pattern.
272-273: LGTM! Backwards compatibility test updated for both wallet types.The test invocations correctly specify both legacy and descriptor wallet variants, ensuring comprehensive coverage.
test/functional/test_framework/test_framework.py (6)
144-144: LGTM! Attribute renamed to indicate internal use.The single underscore prefix (
_requires_wallet) follows Python convention for internal/non-public attributes.
251-267: LGTM! Wallet backend initialization logic handles multiple scenarios.The logic appropriately:
- Sets
descriptors=Nonewhen wallet is not required (lines 251-255)- Selects BDB (legacy) by default when wallet is needed and available (lines 259-260)
- Falls back to SQLite for descriptors if BDB unavailable (lines 261-262)
- Sets None if neither backend is compiled (lines 266-267)
501-514: LGTM! Centralized wallet options API introduced.The
add_wallet_optionsmethod provides a unified interface for tests to expose wallet-related CLI options. The mutually exclusive group ensures only one wallet type is selected per test run, with appropriate defaults when only one type is supported.
465-465: LGTM! Reference updated to use internal attribute.
1067-1067: LGTM! Internal attribute updated in skip_if_no_wallet().
1816-1816: LGTM! DashTestFramework reference updated consistently.test/functional/feature_llmq_connections.py (1)
22-23: LGTM! Wallet options wiring added to LLMQ test.The change is consistent with the backport pattern and appropriate for this DashTestFramework-based test.
test/functional/feature_txindex.py (1)
24-25: LGTM! Wallet options wiring added to txindex test.The change follows the established pattern for this backport PR.
test/functional/rpc_net.py (1)
46-47: LGTM! Wallet options integration added correctly.The
add_optionsmethod properly delegates toadd_wallet_options(parser), enabling wallet-related CLI options for this test. This is appropriate since the test usesMiniWalletfor transaction creation.test/functional/feature_llmq_is_cl_conflicts.py (1)
51-52: LGTM! Wallet options properly integrated.The
add_optionsmethod correctly delegates toadd_wallet_options(parser), which is necessary for this test's wallet operations (transaction creation, signing, address generation).test/functional/wallet_upgradetohd.py (1)
23-24: LGTM! Wallet options correctly added.The
add_optionsmethod properly integrates wallet-related CLI options, which are essential for this HD wallet upgrade test.test/functional/wallet_signer.py (2)
21-22: LGTM! Wallet options correctly configured for external signer.The
add_optionsmethod properly specifieslegacy=False, which is correct since external signers require descriptor wallets (as validated by the test assertions at lines 76-80).
153-153: LGTM! Assertion formatting improved.The removal of unnecessary parentheses from
assertstatements aligns with Python best practices and improves readability without changing functionality.Also applies to: 182-182, 190-190
test/functional/rpc_wipewallettxes.py (1)
12-13: LGTM! Wallet options properly added.The
add_optionsmethod correctly integrates wallet-related CLI options, which are required for this wallet transaction management test.test/functional/rpc_verifyislock.py (1)
18-20: LGTM! Wallet option integration aligns with test requirements.The
add_optionsmethod correctly delegates toself.add_wallet_options(parser), enabling wallet CLI option parsing for this test. Since the test already uses wallet RPCs (e.g.,sendtoaddress,getnewaddress,signrawtransactionwithwallet), this change properly integrates wallet configuration support.test/functional/wallet_conflicts.py (1)
18-20: LGTM! Wallet option integration is appropriate for this wallet test.The
add_optionsmethod correctly delegates toself.add_wallet_options(parser), enabling wallet CLI option parsing. This is consistent with the test's existing wallet dependency (seeskip_if_no_wallet()at line 25) and its extensive use of wallet functionality throughout.test/lint/all-lint.py (1)
15-15: LGTM: Import addition is correct.The import of
executablefromsysis necessary to ensure lint scripts run with the same Python interpreter as the parent script.
2 participants
What was done?
Regular backports from Bitcoin Core v25; only python code is changed here.
How Has This Been Tested?
Run unit & functional tests.
Breaking Changes
N/A
Checklist: