Skip to content

backport: Merge bitcoin#28725, 28771, 28825, 28814, 28546, 28076, 28741, 28727, 22764#7280

Open
vijaydasmp wants to merge 9 commits intodashpay:developfrom
vijaydasmp:Apr_2026_5
Open

backport: Merge bitcoin#28725, 28771, 28825, 28814, 28546, 28076, 28741, 28727, 22764#7280
vijaydasmp wants to merge 9 commits intodashpay:developfrom
vijaydasmp:Apr_2026_5

Conversation

@vijaydasmp
Copy link
Copy Markdown

No description provided.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 11, 2026

⚠️ Potential Merge Conflicts Detected

This PR has potential conflicts with the following open PRs:

Please coordinate with the authors of these PRs to avoid merge conflicts.

@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#28725, 28771, 28825, 28814, 28546, 28076, 28741, 28727 backport: Merge bitcoin#28725, 28771, 28825, 28814, 28546, 28076, 28741, 28727, 22764 Apr 11, 2026
@vijaydasmp vijaydasmp marked this pull request as ready for review April 11, 2026 02:52
@thepastaclaw
Copy link
Copy Markdown

thepastaclaw commented Apr 11, 2026

✅ Review complete (commit 63494e8)

fanquake and others added 9 commits April 11, 2026 08:24
…r type hints (Python 3.9 / PEP 585)

a478c81 test: replace `Callable`/`Iterable` with their `collections.abc` alternative (PEP 585) (stickies-v)
4b9afb1 scripted-diff: use PEP 585 built-in collection types for verify-binary script (Sebastian Falbesoner)
d516cf8 test: use built-in collection types for type hints (Python 3.9 / PEP 585) (Sebastian Falbesoner)

Pull request description:

  With Python 3.9 / [PEP 585](https://peps.python.org/pep-0585/), [type hinting has become a little less awkward](https://docs.python.org/3.9/whatsnew/3.9.html#type-hinting-generics-in-standard-collections), as for collection types one doesn't need to import the corresponding capitalized types (`Dict`, `List`, `Set`, `Tuple`, ...) anymore, but can use the built-in types directly (see  https://peps.python.org/pep-0585/#implementation for the full list).

  This PR applies the replacement for all Python scripts (i.e. in the contrib and test folders) for the basic types, i.e.:

  - typing.Dict -> dict
  - typing.List -> list
  - typing.Set  -> set
  - typing.Tuple -> tuple

  For an additional check, I ran mypy 1.6.1 on both master and the PR branch via
  ```
  $ mypy --ignore-missing-imports --explicit-package-bases $(git ls-files "*.py")
  ```
  and verified that the output is identical -- (from the 22 identified problems, most look like false-positives, it's probably worth it to go deeper here and address them in a follow-up though).

ACKs for top commit:
  stickies-v:
    ACK a478c81
  fanquake:
    ACK a478c81

Tree-SHA512: 6948c905f6abd644d84f09fcb3661d7edb2742e8f2b28560008697d251d77a61a1146ab4b070e65b0d27acede7a5256703da7bf6eb1c7c3a897755478c76c6e8

Replacing List and Dict with list and dict
88e09ac tests: Fix LCOV_OPTS to be in the correct position (Andrew Chow)

Pull request description:

  `lcov`'s `-a` option takes an argument. With `LCOV_OPTS` immediately after `-a`, the first additional argument becomes the argument to `-a` which is incorrect.

  Also add `LCOV_OPTS` to more `lcov` calls.

ACKs for top commit:
  fanquake:
    ACK 88e09ac

Tree-SHA512: 1ed657c96395bfe882041ded883cb5fa4d04d6ede91f66c319b5bbdd1f88468f8abb2a741dd7898904a78ed7e6c844316f7958ce9e4ccf2dbe666ebec308b7fb
6a91791 fuzz: allow fake and duplicate inputs in tx_package_eval target (Greg Sanders)
a0626cc fuzz: allow reaching MempoolAcceptResult::ResultType::DIFFERENT_WITNESS in tx_package_eval target (Greg Sanders)

Pull request description:

  Exercises `DIFFERENT_WITNESS` by using "blank" WSH() and allowing witness to determine wtxid, and attempts to make invalid/duplicate inputs.

ACKs for top commit:
  dergoegge:
    Coverage looks good to me ACK 6a91791

Tree-SHA512: db894f5f5b81c6b454874baf11f296462832285f41ccb09f23c0db92b9abc98f8ecacd72fc8f60dc92cb7947f543a2e55bed2fd210b0e8ca7c7d5389d90b14af
49d9532 fuzz: explicitly specify llvm-symbolizer path in runner (fanquake)

Pull request description:

  It's not completely clear to me why this needs to be explicitly specified in some environments, and not in others, while at the same time that `llvm-symbolizer` is already in PATH, but this has fixed the 2 issues outlined in bitcoin#28147.

  Use `LLVM_SYMBOLIZER_PATH` as the env var, as that is somewhat also used inside LLVM, but not consistently, i.e it's checked for in the asan_symbolize script, but not in in the ubsan_symbolize script, or from in compiler-rt.

  Alternative to bitcoin#28804.

ACKs for top commit:
  maflcko:
    lgtm ACK 49d9532

Tree-SHA512: c3d5bf1c3629793b342c70754a419b3c7a3cd39f800b9aa69ce3395cc2bf83b4d46f2b329974337b94b99573cd0b8600d3f147ed5c21387bf3812316570d1ee3
…ights with asserts, comments, and refactoring

f06016d wallet: Add asserts to detect unset transaction height values (Ryan Ofsky)
262a78b wallet, refactor: Add CWalletTx::updateState function (Ryan Ofsky)

Pull request description:

  Originally, this PR fixed a wallet migration bug that could cause the watchonly wallet created by legacy wallet migration to have incorrect transaction height values. A different fix for the bug was implemented in bitcoin#28609, but that PR did not add any test coverage that would have caught the bug, and didn't include other changes from this PR intended to prevent problems from invalid transaction heights.

  This PR adds new asserts to catch invalid transaction heights, which would trigger test failures without bugfix in bitcoin#28609. This PR also refactors code and adds comments to clarify assumptions and make it less likely a bug from invalid transaction height values would be introduced.

ACKs for top commit:
  achow101:
    ACK f06016d
  Sjors:
    utACK f06016d
  furszy:
    Code review ACK f06016d

Tree-SHA512: 82657c403724d60354f7676b53bcfcc95bdc5864e051a2eb8bfad09d8ad35615393b2d6b432b46f908def9be37bebded3a55ec9ae19e19371d35897fe842c92e
bbbbdb0 ci: Add filesystem lint check (MarcoFalke)
fada2f9 refactor: Replace <filesystem> with <util/fs.h> (MarcoFalke)

Pull request description:

  Using `std::filesystem` is problematic:

  * There is a `fs` namespace wrapper for it. So having two ways to achieve the same is confusing.
  * Not using the `fs` wrapper is dangerous and buggy, because it disables known bugs by deleting problematic functions.

  Fix all issues by removing use of it and adding a linter to avoid using it again in the future.

ACKs for top commit:
  TheCharlatan:
    ACK  bbbbdb0
  fanquake:
    ACK bbbbdb0 🦀

Tree-SHA512: 0e2d49742b08eb2635e6fce41485277cb9c40fe20b81017c391d3472a43787db1278a236825714ca1e41c9d2f59913865cfb0c649e3c8ab1fb598c849f80c660
fa56067 refactor: Fix bugprone-string-constructor warning (MarcoFalke)

Pull request description:

  String literals in C++ have a trailing null character, so the current code is fine to rely on that implicitly. However,
  * the sqlite documentation explicitly mentions the null character
  * code readers may wonder if the code is intentional
  * clang-tidy warns about the code via `bugprone-string-constructor`

  Address the points by putting the null character into the code and enable the clang-tidy `bugprone-string-constructor` check.

ACKs for top commit:
  stickies-v:
    ACK fa56067

Tree-SHA512: da519184d792a885a8151ffc44c8da5781f5aaae12ef768a187cc6d9e542ca8952aebc2ec6c1a05f673f29a86ef44902ee96e7b491af7b4705ad38e14624882e
fe3ac37 test: replace random_bytes with randbytes bitcoin#28720 (ns-xvrn)

Pull request description:

  With Python upgraded to 3.9 replaced the `random_bytes` function in util of functional tests and replaced it's usage with `random.randbytes`.

  Closes bitcoin#28720.

ACKs for top commit:
  maflcko:
    lgtm ACK fe3ac37
  BrandonOdiwuor:
    ACK fe3ac37
  stickies-v:
    ACK fe3ac37, thanks for picking this up
  kristapsk:
    utACK fe3ac37

Tree-SHA512: f65a75e73ebd840c2936eb133d42bccd552f25b717c8ca25c18d06e0593e12f292389cfcc0a0b0759004b67a46ea0c8ac237973ef90f246139778230be1e64e1

Adding random back to utils.py as it is used in other places

Replacing random_bytes with random.randbytes
…ct_strings.py

b59b31a build: Drop redundant qt/bitcoin.cpp (Hennadii Stepanov)
d90ad5a build: Include qt sources for parsing with extract_strings.py (Hennadii Stepanov)

Pull request description:

  On master (4fc15d1) some strings are still untranslated.

  This PR fixes this issue.

  To verify:
  1) `./autogen.sh && ./configure && make -C src translate` _before_ applying this change
  2) apply this change
  3) `./autogen.sh && ./configure && make -C src translate` _after_ applying this change

  The result of `git diff src/qt/bitcoinstrings.cpp`:
  ```diff
  --- a/src/qt/bitcoinstrings.cpp
  +++ b/src/qt/bitcoinstrings.cpp
  @@ -126,6 +126,7 @@ QT_TRANSLATE_NOOP("bitcoin-core", ""
   "You need to rebuild the database using -reindex to go back to unpruned "
   "mode.  This will redownload the entire blockchain"),
   QT_TRANSLATE_NOOP("bitcoin-core", "%s is set very high!"),
  +QT_TRANSLATE_NOOP("bitcoin-core", "(press q to shutdown and continue later)"),
   QT_TRANSLATE_NOOP("bitcoin-core", "-maxmempool must be at least %d MB"),
   QT_TRANSLATE_NOOP("bitcoin-core", "A fatal internal error occurred, see debug.log for details"),
   QT_TRANSLATE_NOOP("bitcoin-core", "Cannot resolve -%s address: '%s'"),
  @@ -204,6 +205,8 @@ QT_TRANSLATE_NOOP("bitcoin-core", "SQLiteDatabase: Failed to prepare statement t
   QT_TRANSLATE_NOOP("bitcoin-core", "SQLiteDatabase: Failed to read database verification error: %s"),
   QT_TRANSLATE_NOOP("bitcoin-core", "SQLiteDatabase: Unexpected application id. Expected %u, got %u"),
   QT_TRANSLATE_NOOP("bitcoin-core", "Section [%s] is not recognized."),
  +QT_TRANSLATE_NOOP("bitcoin-core", "Settings file could not be read"),
  +QT_TRANSLATE_NOOP("bitcoin-core", "Settings file could not be written"),
   QT_TRANSLATE_NOOP("bitcoin-core", "Signing transaction failed"),
   QT_TRANSLATE_NOOP("bitcoin-core", "Specified -walletdir \"%s\" does not exist"),
   QT_TRANSLATE_NOOP("bitcoin-core", "Specified -walletdir \"%s\" is a relative path"),
  @@ -242,4 +245,5 @@ QT_TRANSLATE_NOOP("bitcoin-core", "User Agent comment (%s) contains unsafe chara
   QT_TRANSLATE_NOOP("bitcoin-core", "Verifying blocks…"),
   QT_TRANSLATE_NOOP("bitcoin-core", "Verifying wallet(s)…"),
   QT_TRANSLATE_NOOP("bitcoin-core", "Wallet needed to be rewritten: restart %s to complete"),
  +QT_TRANSLATE_NOOP("bitcoin-core", "press q to shutdown"),
   };
  ```

ACKs for top commit:
  ryanofsky:
    Code review ACK b59b31a. Being able to use `_()` macro in qt would allow simplifying some code, for example replacing repetitive:
  TheCharlatan:
    ACK b59b31a

Tree-SHA512: 13d9d86b487a1b6e718ae96c198a0a927c881bf33df318412793ec9efba3a7e59cfa836204f73f5b53ff4c99edce778c11bffaa88138b80e37b71e36df6b816f
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 659d7c7eb2

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

"std::filesystem",
"--",
"./src/",
":(exclude)src/util/fs.h",
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Exclude src/fs.h from std::filesystem lint

The new Rust lint runner hardcodes :(exclude)src/util/fs.h, but this repository’s safe wrapper lives at src/fs.h, so git grep always matches the intentional std::filesystem usage in that wrapper and the lint fails every run. In the CI flow (ci/lint/06_script.sh) this makes the lint stage fail even on clean trees; reproducing with cargo run --manifest-path test/lint/test_runner/Cargo.toml reports matches from src/fs.h and exits non-zero.

Useful? React with 👍 / 👎.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 11, 2026

Walkthrough

The PR modernizes Python type hints to use built-in generics (list/dict), replaces a custom random_bytes helper with random.randbytes across tests, and adds a Rust-based CI lint test runner (Cargo manifest and main.rs) plus build/invocation steps in lint scripts and Docker entrypoint. It adds LLVM symbolizer env vars for fuzzing and test runner integration, refines Qt translation source lists in Makefiles, tweaks LCOV invocations, adds test script constants, enables a clang-tidy check, and introduces wallet changes: a new CWalletTx::updateState(interfaces::Chain&) and calling code that delegates state reconciliation to it.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant CWallet as CWallet
participant CWalletTx as CWalletTx
participant Chain as interfaces::Chain
CWallet->>CWalletTx: LoadToWallet -> for each wtx, call updateState(chain())
CWalletTx->>Chain: query stored block (findBlock / inActiveChain)
alt block found and active
Chain-->>CWalletTx: return block hash + height (active)
CWalletTx-->>CWalletTx: keep TxStateConfirmed/TxStateConflicted with height
else block missing or not active
Chain-->>CWalletTx: not found / not active
CWalletTx-->>CWalletTx: set TxStateInactive{} (clear block-related fields)
end

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive No pull request description was provided by the author, making this check inconclusive as no content exists to evaluate. Add a brief description summarizing the purpose and impact of these backported changes, or reference the upstream Bitcoin PRs being merged.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title references a Bitcoin backport (PR numbers) which corresponds to the substantial changeset including LCOV options, Rust lint runner, Python type annotations, test utilities, and wallet transaction state updates.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 12

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
contrib/seeds/makeseeds.py (1)

16-174: ⚠️ Potential issue | 🟠 Major

contrib/** directory change needs explicit justification/approval.

This PR modifies contrib/seeds/makeseeds.py, which conflicts with the repository rule that contrib/** should not be changed unless specifically prompted. Please confirm this exception is intentionally approved for this backport.

As per coding guidelines: "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".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@contrib/seeds/makeseeds.py` around lines 16 - 174, The change touches
contrib/seeds/makeseeds.py which per repo policy requires explicit approval;
update the PR to include a clear justification and approval for modifying
contrib (mention it's a backport or bugfix), list the affected symbols (e.g.
parseip, resolveasn, filterbyasn), reference the original issue/PR and target
branch, and obtain an explicit maintainer approval comment or label before
merging. Ensure the PR description includes the reason, risk assessment, and a
short note that contributors were notified/approved so reviewers can accept the
contrib/** change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@ci/lint/04_install.sh`:
- Around line 37-44: The script currently tests only if the LINT_RUNNER_PATH
directory exists, which misses the case where the test_runner binary is absent;
update the guard to check the executable itself (use -x on the binary path, e.g.
"${LINT_RUNNER_PATH}/test_runner" or ensure LINT_RUNNER_PATH points to the
binary) so the cargo build and mv steps run when the binary is missing; adjust
surrounding logic that creates the directory (mkdir -p) and the subsequent mv
target/debug/test_runner to work with the new executable-path check and keep
using CI_RETRY_EXE for apt-get as before.

In `@ci/test/00_setup_env_native_fuzz.sh`:
- Line 19: The export of LLVM_SYMBOLIZER_PATH points to llvm-symbolizer-17 while
the build uses clang-19 (mismatched major versions), and changes to ci/**
require explicit scope approval; update the symbolizer to match the compiler
(e.g., set LLVM_SYMBOLIZER_PATH to llvm-symbolizer-19) or make the clang
invocation use clang-17 so versions align, and add a brief CI note or PR
description confirming that modifying files under ci/** is authorized by the
team/CI owners; locate the assignment to LLVM_SYMBOLIZER_PATH and the clang-19
invocation in the script and adjust one of them (or add an approval comment) to
resolve the mismatch and policy concern.

In `@contrib/devtools/utils.py`:
- Line 13: This change touches contrib/devtools/utils.py (function
determine_wellknown_cmd) which per guidelines requires explicit scope approval;
update the PR description or backport metadata to state that modifications under
contrib/ are intentional and approved, and include a short justification and the
approver's identifier (e.g., maintainer or release manager) so reviewers can
verify the exception, and optionally add a one-line comment above
determine_wellknown_cmd noting the accepted contrib-path change for future
readers.

In `@contrib/message-capture/message-capture-parser.py`:
- Line 14: The file uses PEP 585 generics (e.g., list[Any]) which breaks Python
3.7/3.8 compatibility and touches the contrib directory; either add "from
__future__ import annotations" as the first import in the module to allow
built-in generics on older Pythons, or revert to typing generics by importing
List/Dict/etc. from typing and replace occurrences of list[Any] at the locations
referenced (around the existing "from typing import Any, Optional" import and
usages near lines 95 and 192) with typing.List/typing.Dict forms; also undo any
unnecessary edits to the contrib/ directory unless this change was explicitly
requested.

In `@src/Makefile.qt.include`:
- Line 499: Remove the duplicate prerequisite by editing the translate target to
list $(QT_FORMS_UI) only once; locate the translate target definition (the line
starting with "translate:") and remove the extra $(QT_FORMS_UI) token so the
prerequisite list contains a single reference to $(QT_FORMS_UI), leaving other
prerequisites (e.g., $(srcdir)/qt/dashstrings.cpp, $(BITCOIN_QT_BASE_CPP),
$(BITCOIN_QT_WINDOWS_CPP), $(BITCOIN_QT_WALLET_CPP), $(BITCOIN_QT_H),
$(BITCOIN_MM)) unchanged.

In `@src/wallet/wallet.h`:
- Around line 531-537: Edit the precondition comment in src/wallet/wallet.h to
remove the duplicated word "able" and clarify the awkward sentence about block
references: change "it won't be able able to determine the the height of the
last block processed, or the heights of blocks referenced in transaction, and
might cause assert failures." to a clean phrasing such as "it won't be able to
determine the height of the last processed block or the heights of blocks
referenced by transactions, which may cause assert failures." Update the comment
block near the wallet online/block index precondition so it reads clearly and
contains no duplicated words.

In `@test/functional/interface_rest.py`:
- Line 67: The parameter annotation for query_params in the function signature
is incorrect because it defaults to None but is typed as dict[str, typing.Any];
update the annotation to an explicit optional type such as
typing.Optional[dict[str, typing.Any]] (or from typing import Optional and use
Optional[dict[str, Any]]) so that query_params: Optional[dict[str, typing.Any]]
= None (reference the query_params parameter in the function signature in
test/functional/interface_rest.py).

In `@test/functional/mempool_datacarrier.py`:
- Around line 51-55: The five test-only uses of randbytes (assignments to
default_size_data, too_long_data, small_data, one_byte, zero_bytes) trigger
S311; update each line to append a noqa suppression comment like "# noqa: S311"
and include a brief inline note such as "# test-only pseudo-random data,
non-crypto" so the linter ignores these non-cryptographic PRNG warnings while
documenting intent.

In `@test/functional/p2p_v2_misbehaving.py`:
- Line 93: The test uses non-cryptographic randomness in the assignment to
self.transport_version (random.randbytes(5)); add a Ruff S311 suppression
comment on that line to indicate this is test-only pseudo-random data (not used
for crypto) so the linter ignores it. Locate the assignment to
self.transport_version and append a comment like "# noqa: S311 -- test-only
non-crypto randomness" (or equivalent project linter suppression) to suppress
the warning.

In `@test/functional/rpc_psbt.py`:
- Around line 601-604: The test uses randbytes() to generate pseudo-random test
data for variables hash_ripemd160, preimage_ripemd160, hash_sha256,
preimage_sha256, hash_hash160, preimage_hash160, hash_hash256 and
preimage_hash256; add inline suppression comments "noqa: S311" to those
randbytes() call lines to silence the insecure randomness warning (S311) from
Ruff/flake8-bandit since this is non-cryptographic test data, keeping the calls
and variable names (randbytes, hash_ripemd160, preimage_ripemd160, etc.)
unchanged.

In `@test/functional/test_framework/v2_p2p.py`:
- Line 158: The linter flags use of the random module as cryptographically
unsafe; add inline suppressions by appending the comment "# noqa: S311 -
test-only pseudo-random data" to each test-only pseudo-random call: the call to
random.randrange(...), the assignment using random.randbytes(garbage_len)
(self.sent_garbage = random.randbytes(garbage_len)), and the two
random.randint(...) usages so each line ends with that exact noqa comment to
silence S311 for these test-only data generators.

In `@test/lint/test_runner/src/main.rs`:
- Around line 35-52: The git grep invocation in the git().args([...]) call
incorrectly excludes "src/util/fs.h" instead of the actual wrapper "src/fs.h",
causing false positives, and it blindly interprets .status().success() which
treats exit code 1 (no matches) same as exit code 2 (command error); update the
args to exclude "src/fs.h" and change the handling of the Command status
produced by git().status() so you explicitly match on the exit code or use
status.code() to treat 0 as found, 1 as not found (returning Ok/false), and any
other nonzero code as an error (returning Err with the command failure details)
for the git() call used here.

---

Outside diff comments:
In `@contrib/seeds/makeseeds.py`:
- Around line 16-174: The change touches contrib/seeds/makeseeds.py which per
repo policy requires explicit approval; update the PR to include a clear
justification and approval for modifying contrib (mention it's a backport or
bugfix), list the affected symbols (e.g. parseip, resolveasn, filterbyasn),
reference the original issue/PR and target branch, and obtain an explicit
maintainer approval comment or label before merging. Ensure the PR description
includes the reason, risk assessment, and a short note that contributors were
notified/approved so reviewers can accept the contrib/** change.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: bb475e86-4fbe-4b62-ba4b-20e62a937774

📥 Commits

Reviewing files that changed from the base of the PR and between 56bceb4 and 659d7c7.

⛔ Files ignored due to path filters (1)
  • test/lint/test_runner/Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (38)
  • Makefile.am
  • ci/lint/04_install.sh
  • ci/lint/06_script.sh
  • ci/lint/docker-entrypoint.sh
  • ci/test/00_setup_env_native_fuzz.sh
  • contrib/devtools/circular-dependencies.py
  • contrib/devtools/utils.py
  • contrib/guix/security-check.py
  • contrib/guix/symbol-check.py
  • contrib/macdeploy/macdeployqtplus
  • contrib/message-capture/message-capture-parser.py
  • contrib/seeds/makeseeds.py
  • src/.clang-tidy
  • src/Makefile.qt.include
  • src/fs.h
  • src/test/util/script.h
  • src/wallet/db.cpp
  • src/wallet/transaction.cpp
  • src/wallet/transaction.h
  • src/wallet/wallet.cpp
  • src/wallet/wallet.h
  • test/functional/interface_rest.py
  • test/functional/mempool_datacarrier.py
  • test/functional/p2p_net_deadlock.py
  • test/functional/p2p_v2_misbehaving.py
  • test/functional/rpc_psbt.py
  • test/functional/test_framework/script.py
  • test/functional/test_framework/test_framework.py
  • test/functional/test_framework/util.py
  • test/functional/test_framework/v2_p2p.py
  • test/functional/test_framework/wallet.py
  • test/functional/wallet_fast_rescan.py
  • test/fuzz/test_runner.py
  • test/lint/README.md
  • test/lint/lint-files.py
  • test/lint/lint-include-guards.py
  • test/lint/test_runner/Cargo.toml
  • test/lint/test_runner/src/main.rs

Comment on lines +37 to +44
if [ ! -d "${LINT_RUNNER_PATH}" ]; then
${CI_RETRY_EXE} apt-get install -y cargo
(
cd ./test/lint/test_runner || exit 1
cargo build
mkdir -p "${LINT_RUNNER_PATH}"
mv target/debug/test_runner "${LINT_RUNNER_PATH}"
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Check for the executable, not just the directory.

Line 37 only checks -d "${LINT_RUNNER_PATH}". If the directory exists but test_runner is missing (e.g., prior partial run), the build is skipped and downstream execution fails. Guard on the binary path (-x) instead.

Suggested fix
 export LINT_RUNNER_PATH="/lint_test_runner"
-if [ ! -d "${LINT_RUNNER_PATH}" ]; then
+if [ ! -x "${LINT_RUNNER_PATH}/test_runner" ]; then
   ${CI_RETRY_EXE} apt-get install -y cargo
   (
     cd ./test/lint/test_runner || exit 1
     cargo build
     mkdir -p "${LINT_RUNNER_PATH}"
-    mv target/debug/test_runner "${LINT_RUNNER_PATH}"
+    mv target/debug/test_runner "${LINT_RUNNER_PATH}/test_runner"
   )
 fi
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ci/lint/04_install.sh` around lines 37 - 44, The script currently tests only
if the LINT_RUNNER_PATH directory exists, which misses the case where the
test_runner binary is absent; update the guard to check the executable itself
(use -x on the binary path, e.g. "${LINT_RUNNER_PATH}/test_runner" or ensure
LINT_RUNNER_PATH points to the binary) so the cargo build and mv steps run when
the binary is missing; adjust surrounding logic that creates the directory
(mkdir -p) and the subsequent mv target/debug/test_runner to work with the new
executable-path check and keep using CI_RETRY_EXE for apt-get as before.

export RUN_FUZZ_TESTS=true
export GOAL="install"
export BITCOIN_CONFIG="--enable-zmq --enable-fuzz --with-sanitizers=fuzzer,address,undefined,integer CC='clang-19 -ftrivial-auto-var-init=pattern' CXX='clang++-19 -ftrivial-auto-var-init=pattern'"
export LLVM_SYMBOLIZER_PATH="/usr/bin/llvm-symbolizer-17"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect compiler and symbolizer pins in the changed CI setup script.
rg -n 'BITCOIN_CONFIG|LLVM_SYMBOLIZER_PATH|clang-[0-9]+|llvm-symbolizer-[0-9]+' ci/test/00_setup_env_native_fuzz.sh -C1

# Extract and compare major versions (read-only check).
python - <<'PY'
import re
from pathlib import Path

p = Path("ci/test/00_setup_env_native_fuzz.sh").read_text()
clang = re.search(r"clang-(\d+)", p)
symbolizer = re.search(r"llvm-symbolizer-(\d+)", p)

print("clang_major =", clang.group(1) if clang else "not found")
print("symbolizer_major =", symbolizer.group(1) if symbolizer else "not found")

if clang and symbolizer and clang.group(1) != symbolizer.group(1):
    print("RESULT: version mismatch detected (needs explicit justification).")
else:
    print("RESULT: no major-version mismatch detected in this file.")
PY

Repository: dashpay/dash

Length of output: 446


Address LLVM version mismatch and verify CI-scope approval.

Line 19 pins llvm-symbolizer-17 while line 18 compiles with clang-19. This major-version mismatch between the compiler and symbolizer can degrade stack trace quality during fuzzing and sanitizer reporting. Additionally, this change modifies the ci/** directory, which per coding guidelines should not be changed unless explicitly prompted. Confirm that this CI modification is in scope and that the symbolizer version mismatch is intentional.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ci/test/00_setup_env_native_fuzz.sh` at line 19, The export of
LLVM_SYMBOLIZER_PATH points to llvm-symbolizer-17 while the build uses clang-19
(mismatched major versions), and changes to ci/** require explicit scope
approval; update the symbolizer to match the compiler (e.g., set
LLVM_SYMBOLIZER_PATH to llvm-symbolizer-19) or make the clang invocation use
clang-17 so versions align, and add a brief CI note or PR description confirming
that modifying files under ci/** is authorized by the team/CI owners; locate the
assignment to LLVM_SYMBOLIZER_PATH and the clang-19 invocation in the script and
adjust one of them (or add an approval comment) to resolve the mismatch and
policy concern.



def determine_wellknown_cmd(envvar, progname) -> List[str]:
def determine_wellknown_cmd(envvar, progname) -> list[str]:
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Contrib-path modification needs explicit scope approval.

Line 13 changes contrib/devtools/utils.py, but this PR context does not explicitly document an exception for contrib/** edits. Please confirm and document that this directory is intentionally in-scope for this backport.

As per coding guidelines, "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".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@contrib/devtools/utils.py` at line 13, This change touches
contrib/devtools/utils.py (function determine_wellknown_cmd) which per
guidelines requires explicit scope approval; update the PR description or
backport metadata to state that modifications under contrib/ are intentional and
approved, and include a short justification and the approver's identifier (e.g.,
maintainer or release manager) so reviewers can verify the exception, and
optionally add a one-line comment above determine_wellknown_cmd noting the
accepted contrib-path change for future readers.

import json
from pathlib import Path
from typing import Any, List, Optional
from typing import Any, Optional
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check Python version requirements across the project

# Check for python_requires in setup files
fd -e py -e cfg -e toml -e txt 'setup|pyproject|requirements|README' --exec rg -n 'python_requires|Python [0-9]|python.*3\.[0-9]+' {}

# Check shebang lines for version hints
rg -n '^#!/usr/bin/env python3\.[0-9]+' --type py

# Check for other uses of built-in generics (list[], dict[])
rg -n 'def.*:\s*(list|dict)\[' --type py | head -20

Repository: dashpay/dash

Length of output: 814


🌐 Web query:

Python 3.9 PEP 585 built-in generic types support

💡 Result:

PEP 585 introduces support for using built-in collection types like list, dict, tuple, set, frozenset, and type as generics directly in type hints in Python 3.9, without needing to import equivalents from the typing module (e.g., list[str] instead of typing.List[str]). This uses class_getitem for parameterization, available at runtime. Importing the typing versions is deprecated for Python 3.9+, with removal planned after Python 3.9's end-of-life in October 2025. Other collections like collections.deque and collections.defaultdict also gain generic support. Type checkers must recognize these, and with from future import annotations (since 3.7), this works in annotations earlier. Example: def greet_all(names: list[str]) -> None: ...

Citations:


Remove built-in generic syntax or add future annotations import to maintain Python 3.7+ compatibility.

The changes use PEP 585 syntax (list[Any]) which requires Python 3.9+, but the project minimum version is Python 3.7+ per setup.py. This breaks compatibility with Python 3.7 and 3.8. Either revert to typing.List, or add from __future__ import annotations at the top of the file to enable this syntax on earlier versions.

Additionally, changes to contrib/ directory should be avoided unless specifically prompted per coding guidelines.

Also applies to: 95-95, 192-192

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@contrib/message-capture/message-capture-parser.py` at line 14, The file uses
PEP 585 generics (e.g., list[Any]) which breaks Python 3.7/3.8 compatibility and
touches the contrib directory; either add "from __future__ import annotations"
as the first import in the module to allow built-in generics on older Pythons,
or revert to typing generics by importing List/Dict/etc. from typing and replace
occurrences of list[Any] at the locations referenced (around the existing "from
typing import Any, Optional" import and usages near lines 95 and 192) with
typing.List/typing.Dict forms; also undo any unnecessary edits to the contrib/
directory unless this change was explicitly requested.

# The resulted dash_en.xlf source file should follow Transifex requirements.
# See: https://docs.transifex.com/formats/xliff#how-to-distinguish-between-a-source-file-and-a-translation-file
translate: $(srcdir)/qt/dashstrings.cpp $(QT_FORMS_UI) $(QT_FORMS_UI) $(BITCOIN_QT_BASE_CPP) qt/bitcoin.cpp $(BITCOIN_QT_WINDOWS_CPP) $(BITCOIN_QT_WALLET_CPP) $(BITCOIN_QT_H) $(BITCOIN_MM)
translate: $(srcdir)/qt/dashstrings.cpp $(QT_FORMS_UI) $(QT_FORMS_UI) $(BITCOIN_QT_BASE_CPP) $(BITCOIN_QT_WINDOWS_CPP) $(BITCOIN_QT_WALLET_CPP) $(BITCOIN_QT_H) $(BITCOIN_MM)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Duplicate $(QT_FORMS_UI) in prerequisites.

$(QT_FORMS_UI) appears twice in the prerequisites list. While Make handles duplicates gracefully, this appears to be an oversight and should be cleaned up.

🔧 Proposed fix
-translate: $(srcdir)/qt/dashstrings.cpp $(QT_FORMS_UI) $(QT_FORMS_UI) $(BITCOIN_QT_BASE_CPP) $(BITCOIN_QT_WINDOWS_CPP) $(BITCOIN_QT_WALLET_CPP) $(BITCOIN_QT_H) $(BITCOIN_MM)
+translate: $(srcdir)/qt/dashstrings.cpp $(QT_FORMS_UI) $(BITCOIN_QT_BASE_CPP) $(BITCOIN_QT_WINDOWS_CPP) $(BITCOIN_QT_WALLET_CPP) $(BITCOIN_QT_H) $(BITCOIN_MM)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
translate: $(srcdir)/qt/dashstrings.cpp $(QT_FORMS_UI) $(QT_FORMS_UI) $(BITCOIN_QT_BASE_CPP) $(BITCOIN_QT_WINDOWS_CPP) $(BITCOIN_QT_WALLET_CPP) $(BITCOIN_QT_H) $(BITCOIN_MM)
translate: $(srcdir)/qt/dashstrings.cpp $(QT_FORMS_UI) $(BITCOIN_QT_BASE_CPP) $(BITCOIN_QT_WINDOWS_CPP) $(BITCOIN_QT_WALLET_CPP) $(BITCOIN_QT_H) $(BITCOIN_MM)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/Makefile.qt.include` at line 499, Remove the duplicate prerequisite by
editing the translate target to list $(QT_FORMS_UI) only once; locate the
translate target definition (the line starting with "translate:") and remove the
extra $(QT_FORMS_UI) token so the prerequisite list contains a single reference
to $(QT_FORMS_UI), leaving other prerequisites (e.g.,
$(srcdir)/qt/dashstrings.cpp, $(BITCOIN_QT_BASE_CPP), $(BITCOIN_QT_WINDOWS_CPP),
$(BITCOIN_QT_WALLET_CPP), $(BITCOIN_QT_H), $(BITCOIN_MM)) unchanged.

Comment on lines +51 to +55
default_size_data = randbytes(MAX_OP_RETURN_RELAY - 3)
too_long_data = randbytes(MAX_OP_RETURN_RELAY - 2)
small_data = randbytes(MAX_OP_RETURN_RELAY - 4)
one_byte = randbytes(1)
zero_bytes = randbytes(0)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n test/functional/mempool_datacarrier.py | sed -n '45,60p'

Repository: dashpay/dash

Length of output: 949


🏁 Script executed:

# Check if there's a linting configuration file that specifies S311 rules
fd -type f -name ".*lint*" -o -name "*lint*.yml" -o -name "*lint*.yaml" -o -name ".pylintrc" -o -name "pyproject.toml" -o -name "setup.cfg" | head -20

Repository: dashpay/dash

Length of output: 226


🌐 Web query:

S311 Bandit randbytes pseudo-random security issue

💡 Result:

S311 is a linter rule in Ruff (derived from flake8-bandit and Bandit's B311) that flags the use of non-cryptographic pseudo-random number generators from Python's random module, such as random.randbytes, random.random, random.randrange, random.choice, random.sample, random.getrandbits, and random.Random. These are unsuitable for security-sensitive operations like generating tokens, keys, or nonces because they are predictable and can be brute-forced or reverse-engineered by attackers. The "security issue" is not a vulnerability or exploit in the random module itself, but a potential security risk if developers mistakenly use these functions in cryptographic contexts. Python documentation explicitly warns against this and recommends the secrets module (e.g., secrets.token_bytes for random.randbytes equivalent) for cryptographically secure randomness. This rule was expanded over time: - Added random.Random support (Bandit PR #940, 2023). - Added random.randbytes (Bandit PR #1096, 2024). - Added random.sample, randrange, getrandbits (Bandit PR #1235, 2025). - Ruff synced these updates (PR #10313, 2024). No evidence of a specific "S311 Bandit randbytes pseudo-random security issue" beyond this linting rule. "Bandit" refers to the PyCQA/bandit security linter, not a product or vulnerability named Bandit. There are no associated CVEs; it's a best-practice warning (CWE-330: Use of Insufficiently Random Values).

Citations:


🏁 Script executed:

head -30 test/functional/mempool_datacarrier.py

Repository: dashpay/dash

Length of output: 1014


🏁 Script executed:

# Search for linting configuration files
find . -maxdepth 2 -type f \( -name "ruff.toml" -o -name "pyproject.toml" -o -name ".ruff.toml" -o -name "setup.cfg" \)

Repository: dashpay/dash

Length of output: 67


🏁 Script executed:

# Check if there's a linting config in the root or common locations
cat pyproject.toml 2>/dev/null | grep -A 20 "\[tool.ruff\]" || echo "No ruff config in pyproject.toml or section not found"

Repository: dashpay/dash

Length of output: 109


🏁 Script executed:

cat ruff.toml

Repository: dashpay/dash

Length of output: 1160


🏁 Script executed:

# Search for other uses of randbytes in test files to verify if noqa suppression is the standard approach
rg "randbytes" test/functional/ -B 2 -A 2 | head -50

Repository: dashpay/dash

Length of output: 3425


Add noqa suppression comments for S311 randbytes warnings.

All five randbytes calls at lines 51-55 trigger S311 (non-cryptographic pseudo-random generator) because the linting config enforces flake8-bandit rules. Add # noqa: S311 comments to each line with a note explaining these are test-only pseudo-random initializations.

Proposed minimal fix
-        default_size_data = randbytes(MAX_OP_RETURN_RELAY - 3)
-        too_long_data = randbytes(MAX_OP_RETURN_RELAY - 2)
-        small_data = randbytes(MAX_OP_RETURN_RELAY - 4)
-        one_byte = randbytes(1)
-        zero_bytes = randbytes(0)
+        default_size_data = randbytes(MAX_OP_RETURN_RELAY - 3)  # noqa: S311 - test-only pseudo-random data
+        too_long_data = randbytes(MAX_OP_RETURN_RELAY - 2)  # noqa: S311 - test-only pseudo-random data
+        small_data = randbytes(MAX_OP_RETURN_RELAY - 4)  # noqa: S311 - test-only pseudo-random data
+        one_byte = randbytes(1)  # noqa: S311 - test-only pseudo-random data
+        zero_bytes = randbytes(0)  # noqa: S311 - test-only pseudo-random data
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
default_size_data = randbytes(MAX_OP_RETURN_RELAY - 3)
too_long_data = randbytes(MAX_OP_RETURN_RELAY - 2)
small_data = randbytes(MAX_OP_RETURN_RELAY - 4)
one_byte = randbytes(1)
zero_bytes = randbytes(0)
default_size_data = randbytes(MAX_OP_RETURN_RELAY - 3) # noqa: S311 - test-only pseudo-random data
too_long_data = randbytes(MAX_OP_RETURN_RELAY - 2) # noqa: S311 - test-only pseudo-random data
small_data = randbytes(MAX_OP_RETURN_RELAY - 4) # noqa: S311 - test-only pseudo-random data
one_byte = randbytes(1) # noqa: S311 - test-only pseudo-random data
zero_bytes = randbytes(0) # noqa: S311 - test-only pseudo-random data
🧰 Tools
🪛 Ruff (0.15.9)

[error] 51-51: Standard pseudo-random generators are not suitable for cryptographic purposes

(S311)


[error] 52-52: Standard pseudo-random generators are not suitable for cryptographic purposes

(S311)


[error] 53-53: Standard pseudo-random generators are not suitable for cryptographic purposes

(S311)


[error] 54-54: Standard pseudo-random generators are not suitable for cryptographic purposes

(S311)


[error] 55-55: Standard pseudo-random generators are not suitable for cryptographic purposes

(S311)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/functional/mempool_datacarrier.py` around lines 51 - 55, The five
test-only uses of randbytes (assignments to default_size_data, too_long_data,
small_data, one_byte, zero_bytes) trigger S311; update each line to append a
noqa suppression comment like "# noqa: S311" and include a brief inline note
such as "# test-only pseudo-random data, non-crypto" so the linter ignores these
non-cryptographic PRNG warnings while documenting intent.

""""Add option for sending non-empty transport version packet."""
def complete_handshake(self, response):
self.transport_version = random_bytes(5)
self.transport_version = random.randbytes(5)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Check if the file exists and read the relevant section
find . -type f -name "p2p_v2_misbehaving.py" | head -5

Repository: dashpay/dash

Length of output: 95


🏁 Script executed:

# Read the file around line 93 to see the context
sed -n '85,100p' test/functional/p2p_v2_misbehaving.py

Repository: dashpay/dash

Length of output: 725


🏁 Script executed:

# Check for Ruff configuration in the project
find . -type f \( -name "ruff.toml" -o -name "pyproject.toml" -o -name ".ruff.toml" \) | head -10

Repository: dashpay/dash

Length of output: 96


🏁 Script executed:

# Search for other uses of randbytes in test files to see how they're handled
rg "randbytes" test/ -n

Repository: dashpay/dash

Length of output: 1417


🏁 Script executed:

# Check the Ruff configuration
cat ruff.toml

Repository: dashpay/dash

Length of output: 1160


🏁 Script executed:

# Search for existing noqa suppressions in test files for S311
rg "noqa.*S311" test/ -n

Repository: dashpay/dash

Length of output: 38


🏁 Script executed:

# Check if S311 is configured in pyproject.toml for src/dashbls
cat ./src/dashbls/pyproject.toml | head -50

Repository: dashpay/dash

Length of output: 249


🏁 Script executed:

# Check if there are per-file ignores for test/functional in ruff.toml
grep -A 10 'per-file-ignores' ruff.toml

Repository: dashpay/dash

Length of output: 135


Add S311 suppression comment for test-only randomness.

The random.randbytes(5) call on line 93 triggers Ruff's S311 security warning. Since this is test-only pseudo-random data for P2P protocol testing (not cryptographic use), add a suppression comment:

-        self.transport_version = random.randbytes(5)
+        self.transport_version = random.randbytes(5)  # noqa: S311
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
self.transport_version = random.randbytes(5)
self.transport_version = random.randbytes(5) # noqa: S311
🧰 Tools
🪛 Ruff (0.15.9)

[error] 93-93: Standard pseudo-random generators are not suitable for cryptographic purposes

(S311)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/functional/p2p_v2_misbehaving.py` at line 93, The test uses
non-cryptographic randomness in the assignment to self.transport_version
(random.randbytes(5)); add a Ruff S311 suppression comment on that line to
indicate this is test-only pseudo-random data (not used for crypto) so the
linter ignores it. Locate the assignment to self.transport_version and append a
comment like "# noqa: S311 -- test-only non-crypto randomness" (or equivalent
project linter suppression) to suppress the warning.

Comment on lines +601 to +604
hash_ripemd160, preimage_ripemd160 = randbytes(20), randbytes(50)
hash_sha256, preimage_sha256 = randbytes(32), randbytes(50)
hash_hash160, preimage_hash160 = randbytes(20), randbytes(50)
hash_hash256, preimage_hash256 = randbytes(32), randbytes(50)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Check if the file exists and read lines 601-604
head -604 test/functional/rpc_psbt.py | tail -10

Repository: dashpay/dash

Length of output: 852


🏁 Script executed:

# Also check surrounding context to understand the code better
sed -n '595,610p' test/functional/rpc_psbt.py

Repository: dashpay/dash

Length of output: 1241


🌐 Web query:

Ruff S311 linter rule insecure randomness

💡 Result:

Ruff's S311 linter rule, named "suspicious-non-cryptographic-random-usage", detects uses of cryptographically weak pseudo-random number generators like the standard 'random' module. It flags these because they are predictable and insecure for security-sensitive randomness, recommending cryptographically secure alternatives such as the 'secrets' module. Added in Ruff version 0.0.258, it originates from the flake8-bandit linter (Bandit's B311). The rule triggers on calls like random.randrange, random.randint, etc., with the message: "Standard pseudo-random generators are not suitable for cryptographic purposes." Example violation: import random random.randrange(10) Recommended fix: import secrets secrets.randbelow(10) The rule applies broadly via static analysis, as determining cryptographic context is challenging, leading to occasional false positives (e.g., non-security uses of random). It is part of Ruff's S3xx ruleset for security issues and remains active as of 2026.

Citations:


🏁 Script executed:

# Check imports in the file to see where randbytes comes from
head -100 test/functional/rpc_psbt.py | grep -E "^from|^import|randbytes"

Repository: dashpay/dash

Length of output: 476


🏁 Script executed:

# Search for randbytes definition or import in the file
grep -n "randbytes\|from.*import\|^import" test/functional/rpc_psbt.py | head -20

Repository: dashpay/dash

Length of output: 808


🏁 Script executed:

# Check what Python version is specified for this project
cat .python-version

Repository: dashpay/dash

Length of output: 63


🏁 Script executed:

# Check if there's a ruff configuration file or pyproject.toml with linting rules
find . -maxdepth 2 -name "pyproject.toml" -o -name "ruff.toml" -o -name ".ruff.toml" | head -5

Repository: dashpay/dash

Length of output: 67


🏁 Script executed:

# Look for existing ruff configurations or suppressions
grep -r "S311\|noqa" test/functional/rpc_psbt.py | head -10

Repository: dashpay/dash

Length of output: 38


🌐 Web query:

Python random.randbytes cryptographically secure os.urandom

💡 Result:

No, random.randbytes is not cryptographically secure. It uses the default pseudo-random number generator (Mersenne Twister) from the random module, which is unsuitable for cryptographic purposes. os.urandom is cryptographically secure. It returns random bytes from the operating system's secure randomness source (e.g., /dev/urandom on Unix, CryptGenRandom on Windows). For cryptographic random bytes, prefer: - secrets.token_bytes(n) (high-level, recommended) - os.urandom(n) (low-level) - random.SystemRandom.randbytes(n) (uses os.urandom under the hood) random.randbytes(n) explicitly warns: "This method should not be used for generating security tokens. Use secrets.token_bytes instead." (Added in Python 3.9)

Citations:


🏁 Script executed:

# Check the ruff configuration to see if S311 is enabled
cat ruff.toml

Repository: dashpay/dash

Length of output: 1160


🏁 Script executed:

# Also check for any pyproject.toml with ruff config
cat pyproject.toml 2>/dev/null | grep -A 50 "\[tool.ruff"

Repository: dashpay/dash

Length of output: 38


🏁 Script executed:

# Try to run ruff on the specific lines (if ruff is available in the repo context)
# First check if ruff config includes S311
grep -i "S311\|select\|extend-select" ruff.toml

Repository: dashpay/dash

Length of output: 66


Add noqa: S311 comments to suppress insecure randomness warning in test code.

The randbytes() calls at lines 601-604 import from Python's random module, which Ruff's S311 rule (flake8-bandit) flags as unsuitable for cryptographic purposes. Since this is test code generating pseudo-random test data (not actual security-sensitive material), suppressing S311 with inline comments is appropriate.

Proposed minimal fix
-        hash_ripemd160, preimage_ripemd160 = randbytes(20), randbytes(50)
-        hash_sha256, preimage_sha256 = randbytes(32), randbytes(50)
-        hash_hash160, preimage_hash160 = randbytes(20), randbytes(50)
-        hash_hash256, preimage_hash256 = randbytes(32), randbytes(50)
+        hash_ripemd160, preimage_ripemd160 = randbytes(20), randbytes(50)  # noqa: S311 - test-only pseudo-random data
+        hash_sha256, preimage_sha256 = randbytes(32), randbytes(50)  # noqa: S311 - test-only pseudo-random data
+        hash_hash160, preimage_hash160 = randbytes(20), randbytes(50)  # noqa: S311 - test-only pseudo-random data
+        hash_hash256, preimage_hash256 = randbytes(32), randbytes(50)  # noqa: S311 - test-only pseudo-random data
🧰 Tools
🪛 Ruff (0.15.9)

[error] 601-601: Standard pseudo-random generators are not suitable for cryptographic purposes

(S311)


[error] 601-601: Standard pseudo-random generators are not suitable for cryptographic purposes

(S311)


[error] 602-602: Standard pseudo-random generators are not suitable for cryptographic purposes

(S311)


[error] 602-602: Standard pseudo-random generators are not suitable for cryptographic purposes

(S311)


[error] 603-603: Standard pseudo-random generators are not suitable for cryptographic purposes

(S311)


[error] 603-603: Standard pseudo-random generators are not suitable for cryptographic purposes

(S311)


[error] 604-604: Standard pseudo-random generators are not suitable for cryptographic purposes

(S311)


[error] 604-604: Standard pseudo-random generators are not suitable for cryptographic purposes

(S311)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/functional/rpc_psbt.py` around lines 601 - 604, The test uses
randbytes() to generate pseudo-random test data for variables hash_ripemd160,
preimage_ripemd160, hash_sha256, preimage_sha256, hash_hash160,
preimage_hash160, hash_hash256 and preimage_hash256; add inline suppression
comments "noqa: S311" to those randbytes() call lines to silence the insecure
randomness warning (S311) from Ruff/flake8-bandit since this is
non-cryptographic test data, keeping the calls and variable names (randbytes,
hash_ripemd160, preimage_ripemd160, etc.) unchanged.

if garbage_len is None:
garbage_len = random.randrange(MAX_GARBAGE_LEN + 1)
self.sent_garbage = random_bytes(garbage_len)
self.sent_garbage = random.randbytes(garbage_len)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

ruff check test/functional/test_framework/v2_p2p.py --select S311

Repository: dashpay/dash

Length of output: 2787


Multiple S311 violations require suppression in this file.

Lines 157, 158, and 210 all use random module functions that Ruff flags as unsuitable for cryptographic purposes. While intentional for pseudo-random test data, all occurrences need explicit suppression to avoid lint failure:

  • Line 157: random.randrange()
  • Line 158: random.randbytes()
  • Line 210: Two calls to random.randint()

Add # noqa: S311 - test-only pseudo-random data comments to each occurrence.

🧰 Tools
🪛 Ruff (0.15.9)

[error] 158-158: Standard pseudo-random generators are not suitable for cryptographic purposes

(S311)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/functional/test_framework/v2_p2p.py` at line 158, The linter flags use
of the random module as cryptographically unsafe; add inline suppressions by
appending the comment "# noqa: S311 - test-only pseudo-random data" to each
test-only pseudo-random call: the call to random.randrange(...), the assignment
using random.randbytes(garbage_len) (self.sent_garbage =
random.randbytes(garbage_len)), and the two random.randint(...) usages so each
line ends with that exact noqa comment to silence S311 for these test-only data
generators.

Comment on lines +35 to +52
let found = git()
.args([
"grep",
"std::filesystem",
"--",
"./src/",
":(exclude)src/util/fs.h",
])
.status()
.expect("command error")
.success();
if found {
Err(r#"
^^^
Direct use of std::filesystem may be dangerous and buggy. Please include <util/fs.h> and use the
fs:: namespace, which has unsafe filesystem functions marked as deleted.
"#
.to_string())
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/usr/bin/env bash
set -u

echo "Checking wrapper header candidates:"
fd -i '^fs\.h$' src

for exclude in ":(exclude)src/util/fs.h" ":(exclude)src/fs.h"; do
  echo
  echo "=== git grep with $exclude ==="
  git grep -n "std::filesystem" -- ./src/ "$exclude"
  status=$?
  echo "exit_status=$status"
done

Repository: dashpay/dash

Length of output: 2462


Fix the excluded wrapper path and handle git grep exit codes explicitly.

Line 41 excludes src/util/fs.h, but the actual wrapper file is src/fs.h, causing false positives on legitimate wrapper usage.
Additionally, .status().success() on lines 43–45 conflates exit code 1 (no match) with exit code 2 (command error), silently passing when git grep fails rather than reporting the error.

Proposed fix
 fn lint_std_filesystem() -> Result<(), LintError> {
-    let found = git()
+    let out = git()
         .args([
             "grep",
             "std::filesystem",
             "--",
             "./src/",
-            ":(exclude)src/util/fs.h",
+            ":(exclude)src/fs.h",
         ])
-        .status()
-        .expect("command error")
-        .success();
-    if found {
-        Err(r#"
+        .output()
+        .expect("command error");
+
+    match out.status.code() {
+        Some(0) => Err(r#"
 ^^^
-Direct use of std::filesystem may be dangerous and buggy. Please include <util/fs.h> and use the
+Direct use of std::filesystem may be dangerous and buggy. Please include <fs.h> and use the
 fs:: namespace, which has unsafe filesystem functions marked as deleted.
             "#
-        .to_string())
-    } else {
-        Ok(())
+        .to_string()),
+        Some(1) => Ok(()), // no matches
+        _ => Err(String::from_utf8_lossy(&out.stderr).to_string()),
     }
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/lint/test_runner/src/main.rs` around lines 35 - 52, The git grep
invocation in the git().args([...]) call incorrectly excludes "src/util/fs.h"
instead of the actual wrapper "src/fs.h", causing false positives, and it
blindly interprets .status().success() which treats exit code 1 (no matches)
same as exit code 2 (command error); update the args to exclude "src/fs.h" and
change the handling of the Command status produced by git().status() so you
explicitly match on the exit code or use status.code() to treat 0 as found, 1 as
not found (returning Ok/false), and any other nonzero code as an error
(returning Err with the command failure details) for the git() call used here.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (6)
test/functional/interface_rest.py (1)

67-67: ⚠️ Potential issue | 🟡 Minor

Use an explicit optional type for query_params.

Line 67 uses a non-optional type with a None default, which violates RUF013 and will fail linting.

Proposed fix
-            query_params: dict[str, typing.Any] = None,
+            query_params: dict[str, typing.Any] | None = None,
#!/bin/bash
set -euo pipefail

# Verify the exact annotation at the changed line
cat -n test/functional/interface_rest.py | sed -n '62,70p'

# Verify lint rule is configured/enforced
rg -n "RUF013|target-version|select|extend-select" ruff.toml pyproject.toml setup.cfg tox.ini 2>/dev/null || true
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/functional/interface_rest.py` at line 67, The parameter annotation for
query_params in the function signature in test/functional/interface_rest.py is
declared as dict[str, typing.Any] with a default of None which violates RUF013;
update the annotation to an explicit optional type (e.g., use
typing.Optional[dict[str, typing.Any]] or dict[str, typing.Any] | None depending
on project Python target) and ensure the corresponding import (typing.Optional)
or syntax is present so the default None is type-correct for query_params.
src/Makefile.qt.include (1)

499-499: ⚠️ Potential issue | 🟡 Minor

Remove duplicated prerequisite in translate target.

Line 499 lists $(QT_FORMS_UI) twice. It won’t break Make, but it should be deduplicated to avoid redundant inputs to lupdate.

Suggested patch
-translate: $(srcdir)/qt/dashstrings.cpp $(QT_FORMS_UI) $(QT_FORMS_UI) $(BITCOIN_QT_BASE_CPP) $(BITCOIN_QT_WINDOWS_CPP) $(BITCOIN_QT_WALLET_CPP) $(BITCOIN_QT_H) $(BITCOIN_MM)
+translate: $(srcdir)/qt/dashstrings.cpp $(QT_FORMS_UI) $(BITCOIN_QT_BASE_CPP) $(BITCOIN_QT_WINDOWS_CPP) $(BITCOIN_QT_WALLET_CPP) $(BITCOIN_QT_H) $(BITCOIN_MM)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/Makefile.qt.include` at line 499, The translate target lists
$(QT_FORMS_UI) twice causing a duplicated prerequisite; edit the translate rule
(the line starting with "translate:") and remove the repeated $(QT_FORMS_UI) so
each prerequisite appears only once, ensuring the translate target and its
prerequisites (e.g., $(srcdir)/qt/dashstrings.cpp, $(QT_FORMS_UI),
$(BITCOIN_QT_BASE_CPP), $(BITCOIN_QT_WINDOWS_CPP), $(BITCOIN_QT_WALLET_CPP),
$(BITCOIN_QT_H), $(BITCOIN_MM)) are deduplicated.
test/functional/p2p_v2_misbehaving.py (1)

93-93: ⚠️ Potential issue | 🟠 Major

Suppress S311 for test-only randbytes use.

Line 93 uses non-crypto randomness in functional-test scaffolding; Ruff flags this as S311 and can fail lint unless explicitly suppressed.

Proposed fix
-        self.transport_version = random.randbytes(5)
+        self.transport_version = random.randbytes(5)  # noqa: S311 - test-only pseudo-random data
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/functional/p2p_v2_misbehaving.py` at line 93, Suppress the S311 lint for
this test-only use of non-crypto randomness by adding an inline noqa for the
offending assignment; update the line that sets self.transport_version =
random.randbytes(5) to include a per-line suppression like `# noqa: S311` so
Ruff/linters ignore the S311 warning for the test scaffold where cryptographic
randomness isn't required.
test/functional/mempool_datacarrier.py (1)

51-55: ⚠️ Potential issue | 🟠 Major

Suppress S311 on test-only random payload fixtures.

Lines 51–55 intentionally generate pseudo-random test payloads, but Ruff S311 will keep flagging these calls.

Proposed fix
-        default_size_data = randbytes(MAX_OP_RETURN_RELAY - 3)
-        too_long_data = randbytes(MAX_OP_RETURN_RELAY - 2)
-        small_data = randbytes(MAX_OP_RETURN_RELAY - 4)
-        one_byte = randbytes(1)
-        zero_bytes = randbytes(0)
+        default_size_data = randbytes(MAX_OP_RETURN_RELAY - 3)  # noqa: S311 - test-only pseudo-random data
+        too_long_data = randbytes(MAX_OP_RETURN_RELAY - 2)  # noqa: S311 - test-only pseudo-random data
+        small_data = randbytes(MAX_OP_RETURN_RELAY - 4)  # noqa: S311 - test-only pseudo-random data
+        one_byte = randbytes(1)  # noqa: S311 - test-only pseudo-random data
+        zero_bytes = randbytes(0)  # noqa: S311 - test-only pseudo-random data
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/functional/mempool_datacarrier.py` around lines 51 - 55, The test
generates pseudo-random payloads using randbytes (assignments default_size_data,
too_long_data, small_data, one_byte, zero_bytes) which intentionally triggers
Ruff S311; suppress this false positive by appending a per-line noqa for S311 to
each assignment (or add a file-level ruff directive if preferred), e.g. add "#
noqa: S311" to the lines creating these fixtures so the linter ignores these
test-only random bytes.
test/functional/test_framework/v2_p2p.py (1)

158-158: ⚠️ Potential issue | 🟠 Major

Add inline S311 suppression for handshake garbage generation.

Line 158 is test-only pseudo-random data generation, but Ruff still flags it as S311.

Proposed fix
-        self.sent_garbage = random.randbytes(garbage_len)
+        self.sent_garbage = random.randbytes(garbage_len)  # noqa: S311 - test-only pseudo-random data
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/functional/test_framework/v2_p2p.py` at line 158, Ruff flags the
test-only pseudo-random data generation as S311; suppress it inline by adding a
per-line noqa for S311 to the assignment to self.sent_garbage (the call to
random.randbytes(garbage_len)) — append "# noqa: S311" to that line so the test
remains unchanged but the linter no longer reports S311 for this intentional
test-only random byte generation.
test/functional/rpc_psbt.py (1)

601-604: ⚠️ Potential issue | 🟠 Major

Add S311 suppression to PSBT preimage/hash random fixtures.

Lines 601–604 are test-only random byte fixtures; Ruff flags each randbytes() call as S311.

Proposed fix
-        hash_ripemd160, preimage_ripemd160 = randbytes(20), randbytes(50)
-        hash_sha256, preimage_sha256 = randbytes(32), randbytes(50)
-        hash_hash160, preimage_hash160 = randbytes(20), randbytes(50)
-        hash_hash256, preimage_hash256 = randbytes(32), randbytes(50)
+        hash_ripemd160, preimage_ripemd160 = randbytes(20), randbytes(50)  # noqa: S311 - test-only pseudo-random data
+        hash_sha256, preimage_sha256 = randbytes(32), randbytes(50)  # noqa: S311 - test-only pseudo-random data
+        hash_hash160, preimage_hash160 = randbytes(20), randbytes(50)  # noqa: S311 - test-only pseudo-random data
+        hash_hash256, preimage_hash256 = randbytes(32), randbytes(50)  # noqa: S311 - test-only pseudo-random data
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/functional/rpc_psbt.py` around lines 601 - 604, The test-only random
byte fixtures (variables hash_ripemd160, preimage_ripemd160, hash_sha256,
preimage_sha256, hash_hash160, preimage_hash160, hash_hash256, preimage_hash256
in rpc_psbt.py) are triggering Ruff S311; silence it by adding a per-expression
suppression, e.g. append "# noqa: S311" to each randbytes(...) call or add a
single inline ruff noqa that covers those assignments so the test remains
unchanged but Ruff no longer flags the lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/functional/p2p_net_deadlock.py`:
- Line 21: The test uses non-cryptographic random generation via randbytes to
create protocol payloads (rand_msg =
randbytes(MAX_PROTOCOL_MESSAGE_LENGTH).hex()), which triggers Ruff S311;
suppress this warning for the test-only usage by adding a per-line exemption
such as appending "# noqa: S311" to the rand_msg line (or an equivalent ruff
noqa comment referencing S311) so Ruff ignores this security rule for that
specific test-only pseudo-random payload generation.

---

Duplicate comments:
In `@src/Makefile.qt.include`:
- Line 499: The translate target lists $(QT_FORMS_UI) twice causing a duplicated
prerequisite; edit the translate rule (the line starting with "translate:") and
remove the repeated $(QT_FORMS_UI) so each prerequisite appears only once,
ensuring the translate target and its prerequisites (e.g.,
$(srcdir)/qt/dashstrings.cpp, $(QT_FORMS_UI), $(BITCOIN_QT_BASE_CPP),
$(BITCOIN_QT_WINDOWS_CPP), $(BITCOIN_QT_WALLET_CPP), $(BITCOIN_QT_H),
$(BITCOIN_MM)) are deduplicated.

In `@test/functional/interface_rest.py`:
- Line 67: The parameter annotation for query_params in the function signature
in test/functional/interface_rest.py is declared as dict[str, typing.Any] with a
default of None which violates RUF013; update the annotation to an explicit
optional type (e.g., use typing.Optional[dict[str, typing.Any]] or dict[str,
typing.Any] | None depending on project Python target) and ensure the
corresponding import (typing.Optional) or syntax is present so the default None
is type-correct for query_params.

In `@test/functional/mempool_datacarrier.py`:
- Around line 51-55: The test generates pseudo-random payloads using randbytes
(assignments default_size_data, too_long_data, small_data, one_byte, zero_bytes)
which intentionally triggers Ruff S311; suppress this false positive by
appending a per-line noqa for S311 to each assignment (or add a file-level ruff
directive if preferred), e.g. add "# noqa: S311" to the lines creating these
fixtures so the linter ignores these test-only random bytes.

In `@test/functional/p2p_v2_misbehaving.py`:
- Line 93: Suppress the S311 lint for this test-only use of non-crypto
randomness by adding an inline noqa for the offending assignment; update the
line that sets self.transport_version = random.randbytes(5) to include a
per-line suppression like `# noqa: S311` so Ruff/linters ignore the S311 warning
for the test scaffold where cryptographic randomness isn't required.

In `@test/functional/rpc_psbt.py`:
- Around line 601-604: The test-only random byte fixtures (variables
hash_ripemd160, preimage_ripemd160, hash_sha256, preimage_sha256, hash_hash160,
preimage_hash160, hash_hash256, preimage_hash256 in rpc_psbt.py) are triggering
Ruff S311; silence it by adding a per-expression suppression, e.g. append "#
noqa: S311" to each randbytes(...) call or add a single inline ruff noqa that
covers those assignments so the test remains unchanged but Ruff no longer flags
the lines.

In `@test/functional/test_framework/v2_p2p.py`:
- Line 158: Ruff flags the test-only pseudo-random data generation as S311;
suppress it inline by adding a per-line noqa for S311 to the assignment to
self.sent_garbage (the call to random.randbytes(garbage_len)) — append "# noqa:
S311" to that line so the test remains unchanged but the linter no longer
reports S311 for this intentional test-only random byte generation.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 0ce535bc-9487-43e4-a8af-990c97886c46

📥 Commits

Reviewing files that changed from the base of the PR and between 659d7c7 and 63494e8.

⛔ Files ignored due to path filters (1)
  • test/lint/test_runner/Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (38)
  • Makefile.am
  • ci/lint/04_install.sh
  • ci/lint/06_script.sh
  • ci/lint/docker-entrypoint.sh
  • ci/test/00_setup_env_native_fuzz.sh
  • contrib/devtools/circular-dependencies.py
  • contrib/devtools/utils.py
  • contrib/guix/security-check.py
  • contrib/guix/symbol-check.py
  • contrib/macdeploy/macdeployqtplus
  • contrib/message-capture/message-capture-parser.py
  • contrib/seeds/makeseeds.py
  • src/.clang-tidy
  • src/Makefile.qt.include
  • src/fs.h
  • src/test/util/script.h
  • src/wallet/db.cpp
  • src/wallet/transaction.cpp
  • src/wallet/transaction.h
  • src/wallet/wallet.cpp
  • src/wallet/wallet.h
  • test/functional/interface_rest.py
  • test/functional/mempool_datacarrier.py
  • test/functional/p2p_net_deadlock.py
  • test/functional/p2p_v2_misbehaving.py
  • test/functional/rpc_psbt.py
  • test/functional/test_framework/script.py
  • test/functional/test_framework/test_framework.py
  • test/functional/test_framework/util.py
  • test/functional/test_framework/v2_p2p.py
  • test/functional/test_framework/wallet.py
  • test/functional/wallet_fast_rescan.py
  • test/fuzz/test_runner.py
  • test/lint/README.md
  • test/lint/lint-files.py
  • test/lint/lint-include-guards.py
  • test/lint/test_runner/Cargo.toml
  • test/lint/test_runner/src/main.rs
✅ Files skipped from review due to trivial changes (20)
  • contrib/devtools/circular-dependencies.py
  • test/functional/test_framework/script.py
  • contrib/guix/security-check.py
  • src/fs.h
  • src/.clang-tidy
  • ci/test/00_setup_env_native_fuzz.sh
  • test/lint/test_runner/Cargo.toml
  • contrib/devtools/utils.py
  • test/lint/lint-files.py
  • src/wallet/wallet.h
  • test/functional/test_framework/wallet.py
  • test/functional/wallet_fast_rescan.py
  • src/test/util/script.h
  • contrib/guix/symbol-check.py
  • contrib/message-capture/message-capture-parser.py
  • contrib/macdeploy/macdeployqtplus
  • test/lint/lint-include-guards.py
  • test/lint/README.md
  • contrib/seeds/makeseeds.py
  • test/functional/test_framework/test_framework.py
🚧 Files skipped from review as they are similar to previous changes (7)
  • ci/lint/docker-entrypoint.sh
  • ci/lint/06_script.sh
  • test/functional/test_framework/util.py
  • ci/lint/04_install.sh
  • test/fuzz/test_runner.py
  • Makefile.am
  • test/lint/test_runner/src/main.rs


self.log.info("Simultaneously send a large message on both sides")
rand_msg = random_bytes(MAX_PROTOCOL_MESSAGE_LENGTH).hex()
rand_msg = randbytes(MAX_PROTOCOL_MESSAGE_LENGTH).hex()
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Find and examine the file
find . -path "*/test/functional/p2p_net_deadlock.py" -type f

Repository: dashpay/dash

Length of output: 93


🏁 Script executed:

# Check for Ruff configuration files
find . -maxdepth 3 -type f \( -name "pyproject.toml" -o -name "ruff.toml" -o -name ".flake8" -o -name "setup.cfg" \) | head -10

Repository: dashpay/dash

Length of output: 118


🏁 Script executed:

# Search for S311 references in the codebase to understand how it's configured
rg "S311|noqa.*S311" --type py | head -20

Repository: dashpay/dash

Length of output: 38


🏁 Script executed:

# Examine the file around line 21
sed -n '1,30p' ./test/functional/p2p_net_deadlock.py | cat -n

Repository: dashpay/dash

Length of output: 1308


🏁 Script executed:

# Check the imports at the top of the file
head -30 ./test/functional/p2p_net_deadlock.py

Repository: dashpay/dash

Length of output: 1098


🏁 Script executed:

# Read the ruff.toml configuration
cat ./ruff.toml

Repository: dashpay/dash

Length of output: 1160


Add S311 security rule exemption for test-only pseudo-random payload generation.

Line 21 uses randbytes from the random module (non-cryptographic), which Ruff S311 flags. Since this is test-only payload generation for protocol messages, the suppression is appropriate.

Proposed fix
-        rand_msg = randbytes(MAX_PROTOCOL_MESSAGE_LENGTH).hex()
+        rand_msg = randbytes(MAX_PROTOCOL_MESSAGE_LENGTH).hex()  # noqa: S311 - test-only pseudo-random data
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
rand_msg = randbytes(MAX_PROTOCOL_MESSAGE_LENGTH).hex()
rand_msg = randbytes(MAX_PROTOCOL_MESSAGE_LENGTH).hex() # noqa: S311 - test-only pseudo-random data
🧰 Tools
🪛 Ruff (0.15.9)

[error] 21-21: Standard pseudo-random generators are not suitable for cryptographic purposes

(S311)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/functional/p2p_net_deadlock.py` at line 21, The test uses
non-cryptographic random generation via randbytes to create protocol payloads
(rand_msg = randbytes(MAX_PROTOCOL_MESSAGE_LENGTH).hex()), which triggers Ruff
S311; suppress this warning for the test-only usage by adding a per-line
exemption such as appending "# noqa: S311" to the rand_msg line (or an
equivalent ruff noqa comment referencing S311) so Ruff ignores this security
rule for that specific test-only pseudo-random payload generation.

Copy link
Copy Markdown

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Validated the full PR range against upstream/develop...HEAD. One blocking finding is actionable: the new Rust lint runner is wired into CI and currently fails in this tree because it excludes Bitcoin Core's src/util/fs.h instead of Dash's src/fs.h. The indentation note is not review-worthy, and the src/validation.cpp item is out of scope.

Reviewed commit: 63494e8

🔴 1 blocking

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `test/lint/test_runner/src/main.rs`:
- [BLOCKING] line 41: Rust lint runner excludes the wrong filesystem wrapper header for Dash
  This new lint is now executed by `ci/lint/06_script.sh`, but the pathspec still excludes `src/util/fs.h`, which does not exist in this tree. Dash's wrapper lives at `src/fs.h`, so `git grep "std::filesystem" -- ./src/ ':(exclude)src/util/fs.h'` still matches the intentional `std::filesystem` usages in `src/fs.h` and makes the lint fail every time. I reproduced this by running `cargo run` from `test/lint/test_runner/`, which exits with the `std::filesystem check` failure in this checkout. Please exclude `src/fs.h` here so lint CI can pass.

"std::filesystem",
"--",
"./src/",
":(exclude)src/util/fs.h",
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking: Rust lint runner excludes the wrong filesystem wrapper header for Dash

This new lint is now executed by ci/lint/06_script.sh, but the pathspec still excludes src/util/fs.h, which does not exist in this tree. Dash's wrapper lives at src/fs.h, so git grep "std::filesystem" -- ./src/ ':(exclude)src/util/fs.h' still matches the intentional std::filesystem usages in src/fs.h and makes the lint fail every time. I reproduced this by running cargo run from test/lint/test_runner/, which exits with the std::filesystem check failure in this checkout. Please exclude src/fs.h here so lint CI can pass.

💡 Suggested change
Suggested change
":(exclude)src/util/fs.h",
:(exclude)src/fs.h

source: ['claude-general']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `test/lint/test_runner/src/main.rs`:
- [BLOCKING] line 41: Rust lint runner excludes the wrong filesystem wrapper header for Dash
  This new lint is now executed by `ci/lint/06_script.sh`, but the pathspec still excludes `src/util/fs.h`, which does not exist in this tree. Dash's wrapper lives at `src/fs.h`, so `git grep "std::filesystem" -- ./src/ ':(exclude)src/util/fs.h'` still matches the intentional `std::filesystem` usages in `src/fs.h` and makes the lint fail every time. I reproduced this by running `cargo run` from `test/lint/test_runner/`, which exits with the `std::filesystem check` failure in this checkout. Please exclude `src/fs.h` here so lint CI can pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants