Skip to content

Merge bitcoin-core/gui#771: Avoid error-prone leading whitespace in translatable strings#974

Closed
DashCoreAutoGuix wants to merge 28 commits into
backport-0.27-batch-679from
backport-0.27-batch-679-pr-771
Closed

Merge bitcoin-core/gui#771: Avoid error-prone leading whitespace in translatable strings#974
DashCoreAutoGuix wants to merge 28 commits into
backport-0.27-batch-679from
backport-0.27-batch-679-pr-771

Conversation

@DashCoreAutoGuix
Copy link
Copy Markdown
Owner

@DashCoreAutoGuix DashCoreAutoGuix commented Aug 5, 2025

Backports bitcoin#771

Original commit: 64879f4

Backported from Bitcoin Core v0.27

Summary by CodeRabbit

  • New Features

    • Added a new fuzz test for orphan transaction handling.
    • Introduced a script to lint Qt translatable strings for leading whitespace.
  • Bug Fixes

    • Improved error handling and validation in wallet upgrade and RPC commands.
    • Enhanced wallet transaction addition to prevent inconsistent state on database write failures.
    • Updated tests to ensure correct error codes and messages for wallet passphrase errors and invalid port values in RPC calls.
  • Refactor

    • Centralized orphan transaction synchronization and processing, replacing global locks with internal mutexes and per-peer work sets.
    • Simplified and clarified HD wallet upgrade logic.
    • Updated coin selection and transaction rendering for better code clarity and consistency.
  • Tests

    • Added and updated test cases for orphan transaction handling, wallet upgrades, database write failures, and RPC validation.
    • Removed a utility function for generating confirmed UTXOs in test framework.
  • Documentation

    • Improved and clarified method documentation in wallet and system modules.
  • Chores

    • Updated build scripts and CI to include new test targets and linting steps.
    • Adjusted debug logging configuration for test nodes.

kwvg and others added 28 commits July 30, 2025 07:52
Going through the vin and then adding the vin index as a vout position
shouldn't work, especially if there are an unequal number of inputs and
outputs but the test transactions tend to have spend patterns that allow
the code to inadvertently work, evading detection. A new test has been
added that uses a spend pattern that would've netted unexpected behavior
in the previous logic.

Also, we can just use `AddChildrenToWorkSet` for this because it
effectively implements the same logic (going through vout, finding and
inserting).
Both functions that manipulate it, `AddTx()` and `LimitOrphans()` hold
the mutex.
This commit will not compile as-is as block based orphan reprocessing, a
Dash-ism, has not been adapted to work with this refactor. That has been
segmented out into a separate commit to aid in review.
…ate datadir

fb0dbe9 docs: GetDataDirNet and GetDataDirBase don't create datadir (stickies-v)

Pull request description:

  Since bitcoin#27073, the behaviour of `GetDataDir()` [changed](https://github.com/bitcoin/bitcoin/pull/27073/files#diff-19427b0dd1a791adc728c82e88f267751ba4f1c751e19262cac03cccd2822216L435-L443) to only return the datadir path, but not create it if non-existent. This also changed the behaviour of `GetDataDirNet()` and `GetDataDirBase()` but the docs do not yet reflect that.

ACKs for top commit:
  TheCharlatan:
    ACK fb0dbe9
  theStack:
    ACK fb0dbe9
  willcl-ark:
    ACK fb0dbe9

Tree-SHA512: 3f10f4871df59882f3649c6d3b2362cae2f8a01ad0bd0c636c5608b0d177d279a2e8712930b819d6d3912e91fa6447b9e54507c33d8afe427f7f39002b013bfb
…coin#25641, bitcoin#25683, bitcoin#23679, bitcoin#26216, bitcoin#26295, bitcoin#26513, bitcoin#26515, bitcoin#26727, bitcoin#25374, bitcoin#25272, bitcoin#26408, bitcoin#25112, bitcoin#26702, bitcoin#25932, bitcoin#26822 (auxiliary backports: part 27)

f45de22 merge bitcoin#26822: don't allow past absolute timestamp in `setban` (Kittywhiskers Van Gogh)
920e85d test: adjust mocktime bump to match upstream in `p2p_disconnect_ban` (Kittywhiskers Van Gogh)
b1a95d8 merge bitcoin#25932: Simplify backtrack logic (Kittywhiskers Van Gogh)
eff27a4 merge bitcoin#26702: drop unused `FindWalletTx` parameter and rename (Kittywhiskers Van Gogh)
62998f3 merge bitcoin#25112: Move error message formatting of NonFatalCheckError to cpp (Kittywhiskers Van Gogh)
27db044 merge bitcoin#26408: Remove spam from debug log (Kittywhiskers Van Gogh)
12acde7 merge bitcoin#25272: guard and alert about a wallet invalid state during chain sync (Kittywhiskers Van Gogh)
385306d merge bitcoin#25374: remove unused `create_confirmed_utxos` helper (Kittywhiskers Van Gogh)
a8d29f9 merge bitcoin#26727: remove optional from fStateStats fields (Kittywhiskers Van Gogh)
a0701e9 merge bitcoin#26515: skip getpeerinfo for a peer without CNodeStateStats (Kittywhiskers Van Gogh)
c9eedc8 merge bitcoin#26513: Make static nLastFlush and nLastWrite Chainstate members (Kittywhiskers Van Gogh)
859d64e txorphanage: adapt block-based orphan reprocessing to refactor (Kittywhiskers Van Gogh)
4172102 merge bitcoin#26295: Replace global g_cs_orphans lock with local (Kittywhiskers Van Gogh)
3665919 fix: protect `m_orphan_tx_size` with `g_cs_orphans` (Kittywhiskers Van Gogh)
f12e6cc fix: scan outputs in `GetCandidatesForBlock`, add regression test (Kittywhiskers Van Gogh)
42aa8a7 merge bitcoin#26216: Limit outpoints.size in txorphan target to avoid OOM (Kittywhiskers Van Gogh)
b376c09 merge bitcoin#23679: Sanitize port in `addpeeraddress()` (Kittywhiskers Van Gogh)
cd6c2b5 merge bitcoin#25683: log `nEvicted` message in LimitOrphans then return void (Kittywhiskers Van Gogh)
6cd09ef merge bitcoin#25641: Fix `-Wparentheses` gcc warning (Kittywhiskers Van Gogh)
bc69f80 merge bitcoin#25624: Fix assert bug in txorphan target (Kittywhiskers Van Gogh)
fc68188 merge bitcoin#25447: add low-level target for txorphanage (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional information

  * Since [dash#3121](dashpay#3121), Dash has limited the orphan transaction map by the accumulated size of all transactions in the map instead of transaction count (like upstream). [bitcoin#25447](bitcoin#25447) introduces a new fuzzing harness that operates on upstream assumptions.

    To bridge the gap, `DEFAULT_MAX_ORPHAN_TRANSACTIONS` has been defined in the harness to mean the highest transaction count _assuming_ each transaction is of maximum size, which means the effective worst-case count is 100 transactions. This is already documented in `net_processing.h` ([source](https://github.com/dashpay/dash/blob/bd589dd6f434c0c6cc8f0c3e2cf299785bad6d07/src/net_processing.h#L31-L32)) but has also been explicitly documented for the harness ([source](https://github.com/dashpay/dash/blob/fc68188e3227dc0912ef3c508f6d45ff1d7614ee/src/test/fuzz/txorphan.cpp#L27-L28)).

  * Dash introduced an additional trigger for orphan reprocessing in [dash#3139](dashpay#3139) to allow for faster processing when parents appear in a block. Currently, the implementation scans through the _inputs_ of each transaction to check against the outpoint map ([source](https://github.com/dashpay/dash/blob/86071f9df4d052de6f1df64a5d269a67a6756d29/src/txorphanage.cpp#L186-L192)).

    This does not seem to be intended behavior as it's using the input index to search through outpoints when it should be iterating through outpoints like `AddChildrenToWorkSet()` ([source](https://github.com/dashpay/dash/blob/86071f9df4d052de6f1df64a5d269a67a6756d29/src/txorphanage.cpp#L148C19-L159)). Though despite this, the code still worked _if_ either of the following conditions were met.

    * There are an even number of inputs and outputs (in which case a match is found as if iterating through the set of outputs); or
    * There are more inputs than outputs (in which case a match is found even if all `find()`s `i > vout.size()` are wasted operations)

    The test case introduced in [dash#3139](dashpay#3139) met this criteria and passed though the code would _not_ work as expected if

    * There are more outputs than inputs (in which case the loop will bail prematurely as `vin.size() > vout.size()` and we're scanning `vins` for outpoint positions)

    This has been rectified by modifying `GetCandidatesForBlock()` to use `AddChildrenToWorkSet()` and adding a unit test to document this behavior.

  * [bitcoin#26295](bitcoin#26295) does a substantial refactor of the transaction "orphanage" and part of that refactor is solidifying `ProcessOrphanTx()` as dealing with `Peer`s (as it's located in `net_processing` and not available through the interface). This conflicts with the capability introduced in [dash#3139](dashpay#3139) as there is no peer to attribute it to, which is relevant as the newer code introduces a map between peer ID and the orphan work map ([source](https://github.com/bitcoin/bitcoin/blob/7082ce3e88d77456d60a5a66bd38807fbd66f311/src/txorphanage.h#L75-L76)).

    This has been worked around by using the ID `-1` to mean block connection triggered orphan work ([source](https://github.com/dashpay/dash/blob/859d64e98fc1b5bf613f526ef76e5fc66851fe13/src/txorphanage.cpp#L204-L209), [source](https://github.com/dashpay/dash/blob/859d64e98fc1b5bf613f526ef76e5fc66851fe13/src/net_processing.cpp#L2015-L2024)). This is because no valid peer can be assigned a negative ID as it starts from 0 ([source](https://github.com/dashpay/dash/blob/bd589dd6f434c0c6cc8f0c3e2cf299785bad6d07/src/net.h#L1735)) and increments ([source](https://github.com/dashpay/dash/blob/bd589dd6f434c0c6cc8f0c3e2cf299785bad6d07/src/net.cpp#L3897-L3900)). An example of this kind of usage is InstantSend code ([source](https://github.com/dashpay/dash/blob/859d64e98fc1b5bf613f526ef76e5fc66851fe13/src/instantsend/signing.cpp#L252)).

    * This change, alongside relevant changes to `ProcessOrphanTx()` arguments and other Dash-specific code has been segmented out as a distinct commit to allow for ease of review. This also means that the commit that backports [bitcoin#26295](bitcoin#26295) cannot be compiled on its own due to build errors from not-yet adapted code.

    * Additionally, `ProcessOrphanTx()` assumes `Peer`-triggered orphan resolution and doesn't account for the possibility that the processing of a transaction may further populate the orphan set. This is a key assumption in block-based orphan processing and to retain this intended functionality, `TxOrphanage::HaveMoreWork()` has been defined to influence the return value of `ProcessOrphanTx()` to affect the resolution loop ([source](https://github.com/dashpay/dash/blob/859d64e98fc1b5bf613f526ef76e5fc66851fe13/src/net_processing.cpp#L2015-L2024)) to exhaust all work before continuing ahead.

  ## Breaking changes

  None expected.

  ## Checklist

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  knst:
    utACK f45de22
  UdjinM6:
    utACK f45de22

Tree-SHA512: 8f6a46984091d6f43d4fb97d13edc9dfa7affa1eb79ab37e14f49078b5d738308c6044f9b2b3f5e0d1fe2efde27b7bae14727e14e1ccd5d21eb695162d9c4615
…GetDataDirBase don't create datadir

c5b325a Merge bitcoin#27176: docs: GetDataDirNet and GetDataDirBase don't create datadir (fanquake)

Pull request description:

  ## Issue being fixed or feature implemented

  Since bitcoin#27073 (backported via dashpay#6780), the behaviour of `GetDataDir()` [changed](https://github.com/bitcoin/bitcoin/pull/27073/files#diff-19427b0dd1a791adc728c82e88f267751ba4f1c751e19262cac03cccd2822216L435-L443) to only return the datadir path, but not create it if non-existent. This also changed the behaviour of `GetDataDirNet()` and `GetDataDirBase()` but the docs do not yet reflect that.

  ## How Has This Been Tested?
  N/A

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK c5b325a
  PastaPastaPasta:
    utACK c5b325a

Tree-SHA512: ff3fa0daea16ddc2d4140e65b7a02926fa2be9adebd6f756868ca985c49a9963950ba1ced1117b3ca289cdda6c7d44e72f51158ced1d3abb5e2f965b81f00896
…ohd` rpc

746e5f8 fix: always leave the wallet in a locked state afer `upgradetohd` (UdjinM6)
b19f463 fix(rpc): return correct error codes in `upgradetohd` rpc (UdjinM6)

Pull request description:

  ## Issue being fixed or feature implemented
  `upgradetohd` rpc returns wrong error codes

  ## What was done?
  Move wallet "upgrade to HD" code used in rpc only to `upgradetohd` rpc, return correct error codes. We also don't need all that logic to extract wallet master encryption key because it's already known when wallet is unlocked.

  ## How Has This Been Tested?
  Run tests

  ## Breaking Changes
  `upgradetohd` should return correct error codes now which could probably break things if you relied on them, see tests

  ## Checklist:
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  knst:
    utACK 746e5f8

Tree-SHA512: 829d1f9d805f7a87f628a9c7f4ae93cb545c8113cea97f430639165dae124db17d389c2f5c663214b074da8ae541435ab7df2b672f78370fe883ac77b3cfed42
…ranslatable strings

856325f lint: Add `lint-qt-translation.py` (Hennadii Stepanov)
294a018 qt: Avoid error prone leading spaces in translatable strings (Hennadii Stepanov)
d8298e7 qt, refactor: Drop superfluous type conversions (Hennadii Stepanov)

Pull request description:

  While working on the GUI translation via Transifex web interface, I found it error-prone to have leading whitespace in translatable strings. This is because it is very easy to unintentionally drop them in translations unnoticed.

  Fixed all current cases. Added a linter to prevent similar cases in the future.

ACKs for top commit:
  furszy:
    utACK 856325f

Tree-SHA512: b1ca5effb2db6649e1e99382de79acf3a9f81cc9dad434db5623338489e597897e8addd60c1ab3dcc7506ae62753a7a4ad5a41d7a865f8fcdf94348b54baa7e7
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Aug 5, 2025

Walkthrough

This update introduces significant refactoring and testing improvements to orphan transaction handling, wallet HD upgrade logic, and error reporting. The orphanage logic is overhauled for per-peer work sets and internal mutex locking, with corresponding changes in processing, tests, and fuzz targets. Wallet code removes legacy HD upgrade methods, improving error handling and test coverage for transaction addition and upgrade failures. Additional changes include enhancements to RPC error checks, new and revised tests, a new translation lint script, and minor documentation and formatting updates.

Changes

Cohort / File(s) Change Summary
Orphanage Refactor
src/txorphanage.cpp, src/txorphanage.h, src/net_processing.cpp, src/init.cpp, src/test/fuzz/process_message.cpp, src/test/fuzz/process_messages.cpp, src/test/orphanage_tests.cpp, src/test/fuzz/txorphan.cpp, src/Makefile.test.include
Refactored orphan transaction management: replaced global mutex with internal class mutex, introduced per-peer work sets, updated APIs, simplified locking, and added a new fuzz target and test case for orphanage logic.
Wallet HD Upgrade and Error Handling
src/wallet/wallet.cpp, src/wallet/wallet.h, src/wallet/rpc/wallet.cpp, src/wallet/test/wallet_tests.cpp
Removed legacy HD upgrade methods, refactored wallet creation and upgrade logic, improved error handling for transaction addition, and added a new test for database write failures.
Wallet Database API Update
src/wallet/walletdb.cpp, src/wallet/walletdb.h
Renamed and simplified wallet database method: FindWalletTxFindWalletTxHashes, removing deserialization of wallet transactions.
Error Reporting Refactor
src/util/check.cpp, src/util/check.h
Simplified error reporting: removed formatted error messages and dependencies, introduced explicit exception construction.
Validation State Tracking
src/validation.cpp, src/validation.h
Replaced static local variables for last write/flush times with CChainState member variables.
Qt Transaction Rendering
src/qt/psbtoperationsdialog.cpp, src/qt/psbtoperationsdialog.h
Changed transaction rendering to use QString and HTML formatting for improved UI integration.
Qt Send Dialog Formatting
src/qt/sendcoinsdialog.cpp
Improved wallet name formatting in send confirmation dialog, ensuring HTML escaping.
RPC and Test Enhancements
src/rpc/net.cpp, test/functional/p2p_disconnect_ban.py, test/functional/rpc_net.py, test/functional/wallet_upgradetohd.py
Improved RPC error handling and documentation, added/updated tests for ban timestamp validation, invalid peer address ports, and wallet upgrade error codes.
Test Framework and Linting
test/functional/test_framework/test_node.py, test/functional/test_framework/util.py, test/lint/lint-qt-translation.py
Added debug exclusion for randomness, removed UTXO creation helper, and introduced a new lint script for Qt translation string whitespace.
Documentation and Comments
src/util/system.h
Removed postcondition comments from ArgsManager methods.

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~70 minutes

  • Complexity: High. The orphanage refactor and wallet HD upgrade changes are intricate, affecting core logic, locking, and interfaces, and require careful review for concurrency and correctness. Multiple new/altered tests and fuzz targets increase review breadth. Other changes are moderate to low in complexity but numerous.
  • Volume: High, with changes spanning many files and touching core, wallet, RPC, Qt, and test code.
  • Estimated review time: ~70 minutes, accounting for the need to verify concurrency, state transitions, error handling, and test coverage.
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch backport-0.27-batch-679-pr-771

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
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

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: 9

🔭 Outside diff range comments (5)
src/rpc/net.cpp (1)

243-270: Mandatory fields implemented correctly

All peer state fields are now included unconditionally, implementing the API consistency improvement.

The TODO comment on line 256 mentions that banscore removal may be blocked by p2p_quorum_data.py usage. Would you like me to create an issue to track the removal of this deprecated field?

src/test/orphanage_tests.cpp (1)

23-185: VALIDATION: FAIL - Unrelated orphan transaction changes

These orphan transaction handling changes are completely unrelated to the stated PR objective of fixing whitespace in translatable strings from bitcoin-core/gui#771.

Expected: Changes to GUI files removing leading whitespace from translatable strings
Actual: Orphan transaction mutex refactoring and new test cases

This confirms the validation failure - wrong changes are being backported under an incorrect PR title.

src/net_processing.cpp (1)

3355-3365: Potential message processing starvation from orphan handling

The early return when has_more_orphans is true (line 3364) could potentially starve other message processing if a peer has many orphans queued. This might allow a malicious peer to prevent processing of other messages by flooding orphan transactions.

Consider implementing a limit on consecutive orphan processing or processing other messages before returning to orphans.

+    static constexpr size_t MAX_ORPHANS_PER_CYCLE = 10;
+    size_t orphans_processed = 0;
     bool has_more_orphans;
     {
         LOCK(cs_main);
-        has_more_orphans = ProcessOrphanTx(peer->m_id);
+        // Process up to MAX_ORPHANS_PER_CYCLE to prevent starvation
+        while (orphans_processed < MAX_ORPHANS_PER_CYCLE) {
+            has_more_orphans = ProcessOrphanTx(peer->m_id);
+            if (!has_more_orphans) break;
+            orphans_processed++;
+        }
     }

     if (pfrom->fDisconnect)
         return false;

-    if (has_more_orphans) return true;
+    // Continue with other message processing even if more orphans exist
src/txorphanage.cpp (1)

1-249: VALIDATION: FAIL - Complete mismatch between PR objectives and actual changes

The PR claims to backport bitcoin-core/gui#771 about "Avoid error-prone leading whitespace in translatable strings", but the actual changes are a massive refactoring of orphan transaction handling with:

  • Global mutex g_cs_orphans → internal m_mutex
  • New per-peer work sets (m_peer_work_set)
  • Complete API changes (e.g., GetTxGetTxToReconsider)
  • New methods like SetCandidatesByBlock, HaveMoreWork

This appears to be a completely different Bitcoin Core PR. Please verify:

  1. Are these the correct files for PR #974?
  2. Is this actually backporting a different Bitcoin commit related to orphanage refactoring?
  3. Was the wrong PR objective provided?
src/txorphanage.h (1)

1-111: File changes don't match stated PR objective

This header file shows the same orphanage refactoring that doesn't align with the stated objective of fixing "leading whitespace in translatable strings" from bitcoin-core/gui#771.

The changes here are consistent with the implementation file but completely unrelated to GUI translation strings.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7b83b48 and cb4d0e2.

📒 Files selected for processing (32)
  • ci/dash/lint-tidy.sh (1 hunks)
  • src/Makefile.test.include (1 hunks)
  • src/init.cpp (0 hunks)
  • src/net_processing.cpp (13 hunks)
  • src/qt/psbtoperationsdialog.cpp (3 hunks)
  • src/qt/psbtoperationsdialog.h (2 hunks)
  • src/qt/sendcoinsdialog.cpp (1 hunks)
  • src/rpc/net.cpp (8 hunks)
  • src/test/fuzz/process_message.cpp (0 hunks)
  • src/test/fuzz/process_messages.cpp (0 hunks)
  • src/test/fuzz/txorphan.cpp (1 hunks)
  • src/test/orphanage_tests.cpp (2 hunks)
  • src/txorphanage.cpp (8 hunks)
  • src/txorphanage.h (4 hunks)
  • src/util/check.cpp (1 hunks)
  • src/util/check.h (2 hunks)
  • src/util/system.h (0 hunks)
  • src/validation.cpp (4 hunks)
  • src/validation.h (2 hunks)
  • src/wallet/coinselection.cpp (1 hunks)
  • src/wallet/rpc/wallet.cpp (1 hunks)
  • src/wallet/test/wallet_tests.cpp (1 hunks)
  • src/wallet/wallet.cpp (2 hunks)
  • src/wallet/wallet.h (1 hunks)
  • src/wallet/walletdb.cpp (3 hunks)
  • src/wallet/walletdb.h (1 hunks)
  • test/functional/p2p_disconnect_ban.py (2 hunks)
  • test/functional/rpc_net.py (1 hunks)
  • test/functional/test_framework/test_node.py (1 hunks)
  • test/functional/test_framework/util.py (0 hunks)
  • test/functional/wallet_upgradetohd.py (1 hunks)
  • test/lint/lint-qt-translation.py (1 hunks)
💤 Files with no reviewable changes (5)
  • src/test/fuzz/process_messages.cpp
  • src/test/fuzz/process_message.cpp
  • src/util/system.h
  • src/init.cpp
  • test/functional/test_framework/util.py
🧰 Additional context used
📓 Path-based instructions (6)
**

⚙️ CodeRabbit Configuration File

**: # CodeRabbit AI Review Instructions for Dash Backports

Your Role

You are reviewing Bitcoin Core backports to Dash Core. Your ONLY job is to validate that the Dash commit faithfully represents the original Bitcoin commit with minimal, necessary adaptations.

Critical Validation Rules

1. File Operations Must Match (AUTO-REJECT if violated)

  • If Bitcoin modifies an existing file → Dash MUST modify (not create new)
  • If Bitcoin creates a new file → Dash creates
  • If Bitcoin deletes a file → Dash deletes
  • Common failure: Bitcoin modifies keys.txt, Dash creates new file with 58 keys

2. Size Ratio Check (80-150% of Bitcoin)

  • Count functional lines changed (exclude comments/whitespace)
  • Dash changes should be 80-150% of Bitcoin's size
  • Red flag: 2-line Bitcoin fix becoming 150+ lines in Dash

3. No Scope Creep

  • Reject if you see: "TODO:", "FIXME:", "while we're here", "also fix"
  • No unrelated refactoring or style changes
  • Only Bitcoin's intended changes + minimal Dash adaptations

4. Bitcoin-Specific Code Detection

  • Auto-reject witness/segwit code: msg_wtxidrelay, MSG_WTX, witness imports
  • Auto-reject RBF (replace-by-fee) functionality
  • Note: PSBT is supported in Dash (don't flag)

5. Mandatory Adaptations Only

  • bitcoindash in strings/paths
  • BitcoinDash in user-facing text
  • Port numbers: 8332→9998 (RPC), 8333→9999 (P2P)
  • Hardcoded test values specific to Dash
  • No other changes unless absolutely required

6. Completeness Check

  • All files changed in Bitcoin must be present
  • Extra files need clear justification (Dash-specific compatibility)
  • Missing files = incomplete backport

Review Process

  1. First: Check file operations match exactly
  2. Second: Calculate size ratio
  3. Third: Scan for scope creep patterns
  4. Fourth: Detect Bitcoin-specific code
  5. Fifth: Verify all changes are minimal adaptations

Output Format

VALIDATION: [PASS/FAIL]

File Operatio...

Files:

  • src/Makefile.test.include
  • src/wallet/coinselection.cpp
  • test/functional/test_framework/test_node.py
  • src/qt/sendcoinsdialog.cpp
  • test/functional/wallet_upgradetohd.py
  • test/functional/p2p_disconnect_ban.py
  • src/util/check.cpp
  • test/lint/lint-qt-translation.py
  • ci/dash/lint-tidy.sh
  • src/wallet/walletdb.h
  • src/validation.h
  • src/qt/psbtoperationsdialog.h
  • src/qt/psbtoperationsdialog.cpp
  • src/test/orphanage_tests.cpp
  • src/test/fuzz/txorphan.cpp
  • src/wallet/wallet.h
  • src/rpc/net.cpp
  • src/wallet/walletdb.cpp
  • src/wallet/test/wallet_tests.cpp
  • src/util/check.h
  • src/validation.cpp
  • src/wallet/wallet.cpp
  • src/net_processing.cpp
  • src/wallet/rpc/wallet.cpp
  • test/functional/rpc_net.py
  • src/txorphanage.h
  • src/txorphanage.cpp
src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit Inference Engine (CLAUDE.md)

C++20 codebase should be placed under src/

Files:

  • src/wallet/coinselection.cpp
  • src/qt/sendcoinsdialog.cpp
  • src/util/check.cpp
  • src/wallet/walletdb.h
  • src/validation.h
  • src/qt/psbtoperationsdialog.h
  • src/qt/psbtoperationsdialog.cpp
  • src/test/orphanage_tests.cpp
  • src/test/fuzz/txorphan.cpp
  • src/wallet/wallet.h
  • src/rpc/net.cpp
  • src/wallet/walletdb.cpp
  • src/wallet/test/wallet_tests.cpp
  • src/util/check.h
  • src/validation.cpp
  • src/wallet/wallet.cpp
  • src/net_processing.cpp
  • src/wallet/rpc/wallet.cpp
  • src/txorphanage.h
  • src/txorphanage.cpp
test/functional/**/*.py

📄 CodeRabbit Inference Engine (CLAUDE.md)

Functional tests should be placed in test/functional/ and written in Python

Files:

  • test/functional/test_framework/test_node.py
  • test/functional/wallet_upgradetohd.py
  • test/functional/p2p_disconnect_ban.py
  • test/functional/rpc_net.py
src/util/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit Inference Engine (CLAUDE.md)

Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

  • src/util/check.cpp
  • src/util/check.h
ci/**

📄 CodeRabbit Inference Engine (CLAUDE.md)

Unless specifically prompted, avoid making changes to the ci directory (continuous integration)

Files:

  • ci/dash/lint-tidy.sh
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit Inference Engine (CLAUDE.md)

Unit tests should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests

Files:

  • src/test/orphanage_tests.cpp
  • src/test/fuzz/txorphan.cpp
  • src/wallet/test/wallet_tests.cpp
🧠 Learnings (15)
📓 Common learnings
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-28T22:03:12.364Z
Learning: During multiple verification attempts of Bitcoin Core commit 06d469c26b backport to Dash PR #566, DashCoreAutoGuix consistently identified scope creep in interface_usdt_utxocache.py where additional pruning test functionality was added beyond the original Bitcoin commit. The user provided comprehensive fixes including both scope creep removal and missing mempool test file additions, but couldn't push due to authentication restrictions. The scope creep fix was identified as the priority to resolve CI failures.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-28T20:34:29.061Z
Learning: During Dash backport verification of Bitcoin Core commit 06d469c26b, scope creep was detected when additional pruning test functionality was added to interface_usdt_utxocache.py beyond what was in the original Bitcoin commit. The fix involved removing the extra test block while maintaining the core compiler flag fixes for USDT compilation errors.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-31T01:14:55.631Z
Learning: DashCoreAutoGuix successfully completed a complex Bitcoin Core backport (PR #29412) for block mutation detection by implementing the IsBlockMutated function, adding net processing integration, creating comprehensive unit tests, and properly adapting all Bitcoin-specific witness code for Dash compatibility. The backport maintains full security functionality while respecting Dash's non-witness transaction architecture.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-29T21:29:32.827Z
Learning: DashCoreAutoGuix successfully fixed scope creep in Bitcoin Core commit fcdb39d3ee backport by removing the parse test case from src/test/uint256_tests.cpp that was not part of the original Bitcoin commit. The fix was implemented in commit 16748115ce and verified through range-diff analysis.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-28T19:54:21.426Z
Learning: In Dash backports from Bitcoin Core, including necessary compilation fixes (such as API compatibility changes like UniValue get_int() → getInt<int>()) alongside the core backport is standard and expected practice. These compatibility fixes ensure the backported code compiles in Dash's evolved codebase while preserving Bitcoin's original functionality and intent.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-29T17:13:35.087Z
Learning: When backporting Bitcoin Core commits that use Python's textwrap.dedent() function in test files, the textwrap import statement needs to be explicitly added if it's missing in the Dash test file.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-27T22:35:10.176Z
Learning: In Dash backports, src/dashbls files are vendored dependencies that should not be modified during Bitcoin Core backports unless there is specific justification. Unauthorized modifications to vendored dependencies should be removed to maintain code integrity.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-29T22:13:54.797Z
Learning: In Dash backports from Bitcoin Core, witness transaction-related code (MSG_WTX, wtxid) should be replaced with regular transaction handling (MSG_TX, txid) for compatibility, as demonstrated in the p2p_filter.py test fix where MSG_WTX was replaced with MSG_TX and irr_wtxid usage was replaced with irr_txid.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-30T14:45:15.700Z
Learning: In Dash backports from Bitcoin Core test files, the `address_to_scriptpubkey` function should be imported from `test_framework.wallet` instead of `test_framework.address` as the import location differs between Bitcoin and Dash test frameworks.
Learnt from: knst
PR: DashCoreAutoGuix/dash#436
File: src/univalue/include/univalue.h:86-86
Timestamp: 2025-08-04T18:16:45.145Z
Learning: UniValue (src/univalue) is no longer a vendored dependency in Dash as of PR #6775, so modifications to UniValue files are permitted when needed for Bitcoin Core backports and other legitimate changes.
📚 Learning: applies to src/fuzz/**/*.{cpp,h,cc,cxx,hpp} : fuzzing harnesses should be placed in src/fuzz/...
Learnt from: CR
PR: DashCoreAutoGuix/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T23:09:09.522Z
Learning: Applies to src/fuzz/**/*.{cpp,h,cc,cxx,hpp} : Fuzzing harnesses should be placed in src/fuzz/

Applied to files:

  • src/Makefile.test.include
  • ci/dash/lint-tidy.sh
  • src/test/fuzz/txorphan.cpp
📚 Learning: applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : unit tests should be placed in s...
Learnt from: CR
PR: DashCoreAutoGuix/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T23:09:09.522Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests

Applied to files:

  • src/Makefile.test.include
  • test/lint/lint-qt-translation.py
  • ci/dash/lint-tidy.sh
  • src/test/orphanage_tests.cpp
  • src/wallet/test/wallet_tests.cpp
📚 Learning: during dash backport verification of bitcoin core commit 06d469c, scope creep was detected when a...
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-28T20:34:29.061Z
Learning: During Dash backport verification of Bitcoin Core commit 06d469c26b, scope creep was detected when additional pruning test functionality was added to interface_usdt_utxocache.py beyond what was in the original Bitcoin commit. The fix involved removing the extra test block while maintaining the core compiler flag fixes for USDT compilation errors.

Applied to files:

  • src/Makefile.test.include
  • src/wallet/coinselection.cpp
  • test/functional/test_framework/test_node.py
  • test/functional/wallet_upgradetohd.py
  • ci/dash/lint-tidy.sh
  • src/test/orphanage_tests.cpp
  • src/test/fuzz/txorphan.cpp
  • src/wallet/test/wallet_tests.cpp
  • src/wallet/rpc/wallet.cpp
📚 Learning: dashcoreautoguix successfully fixed scope creep in bitcoin core commit fcdb39d backport by removi...
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-29T21:29:32.827Z
Learning: DashCoreAutoGuix successfully fixed scope creep in Bitcoin Core commit fcdb39d3ee backport by removing the parse test case from src/test/uint256_tests.cpp that was not part of the original Bitcoin commit. The fix was implemented in commit 16748115ce and verified through range-diff analysis.

Applied to files:

  • src/Makefile.test.include
  • src/wallet/coinselection.cpp
  • test/functional/wallet_upgradetohd.py
  • ci/dash/lint-tidy.sh
  • src/test/orphanage_tests.cpp
  • src/wallet/walletdb.cpp
  • src/wallet/test/wallet_tests.cpp
  • src/wallet/wallet.cpp
  • src/wallet/rpc/wallet.cpp
📚 Learning: in dash backports from bitcoin core, witness transaction-related code (msg_wtx, wtxid) should be rep...
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-29T22:13:54.797Z
Learning: In Dash backports from Bitcoin Core, witness transaction-related code (MSG_WTX, wtxid) should be replaced with regular transaction handling (MSG_TX, txid) for compatibility, as demonstrated in the p2p_filter.py test fix where MSG_WTX was replaced with MSG_TX and irr_wtxid usage was replaced with irr_txid.

Applied to files:

  • src/Makefile.test.include
  • test/functional/wallet_upgradetohd.py
  • src/wallet/walletdb.h
  • src/wallet/walletdb.cpp
  • src/wallet/test/wallet_tests.cpp
  • src/txorphanage.h
📚 Learning: during multiple verification attempts of bitcoin core commit 06d469c backport to dash pr #566, da...
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-28T22:03:12.364Z
Learning: During multiple verification attempts of Bitcoin Core commit 06d469c26b backport to Dash PR #566, DashCoreAutoGuix consistently identified scope creep in interface_usdt_utxocache.py where additional pruning test functionality was added beyond the original Bitcoin commit. The user provided comprehensive fixes including both scope creep removal and missing mempool test file additions, but couldn't push due to authentication restrictions. The scope creep fix was identified as the priority to resolve CI failures.

Applied to files:

  • src/Makefile.test.include
  • src/wallet/coinselection.cpp
  • test/functional/wallet_upgradetohd.py
  • ci/dash/lint-tidy.sh
  • src/test/orphanage_tests.cpp
  • src/test/fuzz/txorphan.cpp
  • src/wallet/test/wallet_tests.cpp
  • src/wallet/rpc/wallet.cpp
  • src/txorphanage.h
  • src/txorphanage.cpp
📚 Learning: when backporting bitcoin core commits that use python's textwrap.dedent() function in test files, th...
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-29T17:13:35.087Z
Learning: When backporting Bitcoin Core commits that use Python's textwrap.dedent() function in test files, the textwrap import statement needs to be explicitly added if it's missing in the Dash test file.

Applied to files:

  • test/functional/test_framework/test_node.py
  • test/functional/wallet_upgradetohd.py
  • test/lint/lint-qt-translation.py
  • ci/dash/lint-tidy.sh
📚 Learning: in dash backports from bitcoin core test files, the `address_to_scriptpubkey` function should be imp...
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-30T14:45:15.700Z
Learning: In Dash backports from Bitcoin Core test files, the `address_to_scriptpubkey` function should be imported from `test_framework.wallet` instead of `test_framework.address` as the import location differs between Bitcoin and Dash test frameworks.

Applied to files:

  • test/functional/wallet_upgradetohd.py
  • src/wallet/walletdb.h
  • src/wallet/walletdb.cpp
  • src/wallet/test/wallet_tests.cpp
  • src/wallet/wallet.cpp
  • src/wallet/rpc/wallet.cpp
📚 Learning: the `add_wallet_options` method is not available in dash's test framework and should be removed when...
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-30T14:45:15.700Z
Learning: The `add_wallet_options` method is not available in Dash's test framework and should be removed when backporting Bitcoin Core tests that use this method, as demonstrated in wallet_rescan_unconfirmed.py.

Applied to files:

  • test/functional/wallet_upgradetohd.py
  • src/wallet/walletdb.h
  • src/wallet/wallet.h
  • src/wallet/test/wallet_tests.cpp
  • src/wallet/wallet.cpp
  • src/wallet/rpc/wallet.cpp
📚 Learning: always run linting before commits using test/lint/all-lint.py...
Learnt from: CR
PR: DashCoreAutoGuix/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T23:09:09.522Z
Learning: Always run linting before commits using test/lint/all-lint.py

Applied to files:

  • test/lint/lint-qt-translation.py
📚 Learning: dashcoreautoguix successfully completed a complex bitcoin core backport (pr bitcoin#29412) for block mutati...
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-31T01:14:55.631Z
Learning: DashCoreAutoGuix successfully completed a complex Bitcoin Core backport (PR #29412) for block mutation detection by implementing the IsBlockMutated function, adding net processing integration, creating comprehensive unit tests, and properly adapting all Bitcoin-specific witness code for Dash compatibility. The backport maintains full security functionality while respecting Dash's non-witness transaction architecture.

Applied to files:

  • ci/dash/lint-tidy.sh
  • src/test/orphanage_tests.cpp
  • src/wallet/test/wallet_tests.cpp
  • src/txorphanage.h
📚 Learning: applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : masternode lists use immutable data struct...
Learnt from: CR
PR: DashCoreAutoGuix/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T23:09:09.522Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists use immutable data structures from the Immer library for thread safety

Applied to files:

  • ci/dash/lint-tidy.sh
  • src/validation.h
  • src/txorphanage.h
📚 Learning: applies to src/util/**/*.{cpp,h,cc,cxx,hpp} : dash uses unordered_lru_cache for efficient caching wi...
Learnt from: CR
PR: DashCoreAutoGuix/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T23:09:09.522Z
Learning: Applies to src/util/**/*.{cpp,h,cc,cxx,hpp} : Dash uses unordered_lru_cache for efficient caching with LRU eviction

Applied to files:

  • src/validation.h
📚 Learning: applies to src/evo/specialtx.h : special transactions use payload extensions as defined in src/evo/s...
Learnt from: CR
PR: DashCoreAutoGuix/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T23:09:09.522Z
Learning: Applies to src/evo/specialtx.h : Special transactions use payload extensions as defined in src/evo/specialtx.h

Applied to files:

  • src/rpc/net.cpp
  • src/net_processing.cpp
🪛 Ruff (0.12.2)
test/functional/p2p_disconnect_ban.py

61-61: Boolean positional value in function call

(FBT003)


82-82: Boolean positional value in function call

(FBT003)


106-106: Boolean positional value in function call

(FBT003)

test/lint/lint-qt-translation.py

13-13: Missing return type annotation for public function main

Add return type annotation: None

(ANN201)


14-14: Starting a process with a partial executable path

(S607)

🔇 Additional comments (40)
test/functional/test_framework/test_node.py (1)

109-109: No action needed: -debugexclude=rand matches upstream Bitcoin Core
Confirmed that the upstream test_node.py already includes "-debugexclude=rand" alongside libevent and leveldb. This change is part of the original Bitcoin Core commit.

src/Makefile.test.include (1)

369-369: LGTM! Verify this is part of the original Bitcoin Core backport.

The addition of the new fuzz test source file is correctly placed in alphabetical order and follows proper makefile conventions.

Please confirm this new fuzz test was included in the original Bitcoin Core commit being backported, as it seems unrelated to the stated PR objective of fixing translatable string whitespace issues.

ci/dash/lint-tidy.sh (2)

23-23: LGTM! Properly maintains CI analysis coverage.

The addition correctly includes the new fuzz test file in the include-what-you-use analysis.


26-26: LGTM! Properly maintains CI analysis coverage.

The addition correctly includes the new utility file in the include-what-you-use analysis.

test/functional/p2p_disconnect_ban.py (2)

92-98: LGTM! Proper assertion updates for new ban entries.

The time remaining calculations correctly account for the mock time advancement and include the new absolute timestamp ban.


105-106: LGTM! Proper persistence test updates.

The assertions correctly include the new ban entry in the expected order after node restart.

test/functional/wallet_upgradetohd.py (1)

206-206: Verify RPC behavior for upgradetohd change

Please confirm that the upgradetohd RPC in this backport now intentionally returns error -14 with the message
"Error: The wallet passphrase entered was incorrect"
and that this change comes from Bitcoin Core GUI commit #771 rather than unintended scope creep. If the underlying RPC wasn’t updated here (or isn’t part of the GUI translation-whitespace fixes), revert the test change.

• test/functional/wallet_upgradetohd.py: line 206 [assert_raises_rpc_error(-14, …)]

src/qt/psbtoperationsdialog.h (1)

9-9: LGTM! Proper Qt string handling improvement

These changes improve Qt GUI string consistency by:

  1. Adding the necessary QString header include
  2. Changing renderTransaction method to return QString instead of std::string

This aligns perfectly with the PR objective of improving GUI translatable string handling and avoiding error-prone string conversions in Qt components.

Also applies to: 50-50

src/qt/sendcoinsdialog.cpp (1)

345-345: LGTM: Proper Qt translation string formatting

The change uses proper Qt translation formatting with placeholders and HTML escaping, which helps avoid whitespace issues in translatable strings. This aligns with the PR objective to prevent error-prone leading whitespace.

src/wallet/walletdb.h (1)

236-236: Unexpected walletdb.h method signature change

I wasn’t able to find any reference to FindWalletTxHashes (or the rename from FindWalletTx) in Bitcoin Core commit 64879f4. This change in src/wallet/walletdb.h:236 appears unrelated to the GUI translation objective and may be scope creep.

• Location to review:

  • src/wallet/walletdb.h, line 236:
    DBErrors FindWalletTxHashes(std::vector<uint256>& tx_hashes);

Please verify whether this rename should be part of this PR or if it needs to be reverted.

test/lint/lint-qt-translation.py (1)

13-19: LGTM: Effective linting for translation string whitespace

The script correctly implements detection of leading whitespace in Qt translatable strings using git grep with an appropriate pattern. The logic and error reporting are sound.

src/validation.cpp (5)

2747-2751: LGTM: Clean refactoring from static locals to member variables.

The initialization logic for timing variables has been properly converted from static locals to member variables while preserving the original behavior.


2760-2762: LGTM: Consistent use of member variables in timing conditions.

The periodic write and flush condition checks correctly use the new member variables while maintaining identical timing logic.


2793-2793: LGTM: Proper member variable update after write operations.

The timing state is correctly updated using the member variable after write operations complete.


2819-2819: LGTM: Proper member variable update after flush operations.

The timing state is correctly updated using the member variable after flush operations complete.


2747-2819: Inconsistency between PR title and actual changes.

The PR title mentions "Avoid error-prone leading whitespace in translatable strings" from bitcoin-core/gui#771, but the actual changes are in src/validation.cpp replacing static local variables with member variables for timing state management. This appears to be either:

  1. Incorrect PR title/description
  2. Missing context about how these validation changes relate to GUI translation strings
  3. Part of a larger changeset not fully represented

While the code changes themselves appear technically sound (clean refactoring from static locals to member variables), they don't align with the stated PR objective.

Please verify this is the correct changeset for bitcoin-core/gui#771 or provide clarification on how these validation timing changes relate to GUI translation string handling.

Likely an incorrect or invalid review comment.

src/wallet/walletdb.cpp (2)

977-1024: LGTM! Good refactoring to simplify the transaction retrieval interface.

The rename from FindWalletTx to FindWalletTxHashes and the simplified signature that only collects transaction hashes is a clean improvement. This avoids unnecessary deserialization of CWalletTx objects when only the hashes are needed.


1028-1030: LGTM! Consistent update to use the new FindWalletTxHashes method.

The call site has been properly updated to match the new method signature.

src/util/check.h (3)

11-17: LGTM! Clean separation of interface and implementation.

Moving the error formatting logic out of the header into the implementation file is a good design choice. This reduces header dependencies and improves compilation times.


20-27: LGTM! Simplified error throwing without inline formatting.

The helper function now simply forwards the raw parameters to the exception constructor, keeping the header lean.


84-86: LGTM! Cleaner macro definition.

The NONFATAL_UNREACHABLE macro is now simpler and more maintainable without embedded formatting logic.

src/qt/psbtoperationsdialog.cpp (2)

170-173: LGTM! Cleaner method call without unnecessary string conversion.

The direct use of the QString return value eliminates the need for conversion.


175-223: LGTM! Excellent Qt-idiomatic refactoring.

The changes properly utilize Qt's string handling:

  • Native QString usage throughout
  • Proper HTML formatting for Qt widget display
  • Efficient string construction without unnecessary conversions

This results in cleaner, more maintainable Qt code.

src/wallet/wallet.h (1)

620-624: LGTM! Important documentation clarification for error handling.

The updated documentation clearly specifies that AddToWallet returns nullptr on database write errors, which is crucial for proper error handling by callers.

src/rpc/net.cpp (6)

121-126: Documentation improvement for masternode fields

The updated documentation clearly explains when these masternode-specific fields are present in the RPC response.


146-156: Good API improvement: Making peer info fields mandatory

Converting these fields from optional to mandatory improves API consistency and simplifies client implementations.


199-207: Excellent race condition handling

The added check prevents reporting stale peer data when a disconnect occurs between the two API calls. The comment clearly documents the rationale.


219-222: Consistent peer state reporting

Using statestats.their_services ensures accurate reporting of peer-advertised services, and unconditionally including relaytxes aligns with the mandatory field approach.


785-788: Good input validation for ban timestamps

Rejecting past timestamps for absolute bans prevents ineffective ban entries and provides clear user feedback.


1012-1012: Correct type usage for port parameter

Using uint16_t for the port parameter ensures valid port range enforcement at the type level.

src/test/fuzz/txorphan.cpp (4)

1-31: Well-structured fuzz test setup

The includes and constant definitions are appropriate for testing orphan transaction handling.


32-50: Good fuzz test initialization

Proper setup with mock time control and configurable transaction generation parameters.


51-83: Well-designed transaction construction for fuzzing

The logic properly handles duplicate inputs, manages the outpoint pool, and creates realistic transaction patterns for thorough orphanage testing.


84-146: Comprehensive orphanage operation coverage

Excellent coverage of all TxOrphanage methods with appropriate assertions for state consistency and edge cases.

src/wallet/rpc/wallet.cpp (2)

375-419: Improved atomic wallet upgrade with proper locking

The expanded lock scope ensures the entire HD upgrade operation is atomic, preventing race conditions. Good validation checks for HD status and private keys.


420-465: Clean HD chain generation with proper encryption handling

The refactored implementation properly separates descriptor and legacy wallet paths, uses WithEncryptionKey for secure HD generation, and correctly manages wallet lock state.

src/net_processing.cpp (2)

4503-4513: LGTM: Orphan processing correctly integrated with transaction acceptance

The orphan processing is properly integrated after successful transaction acceptance, with appropriate locking and the new peer-based API.


2015-2023: Consider using a named constant for the special node ID

The use of -1 as a special node ID to indicate "no specific peer" works but could be clearer with a named constant.

+    static constexpr NodeId NO_PEER = -1;
     // Candidates are sourced from a block and therefore cannot be attributed to a peer, we use -1 as the identifier
     bool have_candidates{true};
     {
         LOCK(::cs_main);
         m_orphanage.SetCandidatesByBlock(*pblock);
         // Keep processing as valid orphans may enable processing of their descendants
         while (have_candidates) {
-            have_candidates = ProcessOrphanTx(/*node_id=*/-1);
+            have_candidates = ProcessOrphanTx(/*node_id=*/NO_PEER);
         }
     }

Likely an incorrect or invalid review comment.

src/wallet/wallet.cpp (2)

356-367: Inconsistency between PR objectives and actual changes detected.

The PR objectives state this backport is about "avoiding error-prone leading whitespace in translatable strings" from bitcoin-core/gui#771, but these code changes are refactoring HD chain generation logic in the core wallet functionality, not GUI translation strings.

This HD chain generation refactoring looks like a legitimate Bitcoin Core pattern (using ScriptPubKeyMan managers instead of direct method calls), but it doesn't match the stated PR purpose.

Please verify what Bitcoin Core commit this actually backports and whether it matches the stated PR objectives. The changes appear to be from a different Bitcoin commit than the GUI translation string improvements mentioned in the PR description.

Likely an incorrect or invalid review comment.


1115-1122: Improved error handling for database write failures.

The change correctly adds null checking for AddToWallet return value and throws a runtime exception on DB write failure instead of silently returning false. This prevents inconsistent wallet state and makes errors visible to callers.

However, this change also doesn't relate to the stated PR objectives about GUI translation strings. Please verify this is the correct backport as described in the PR title and objectives.

Likely an incorrect or invalid review comment.

Comment thread src/net_processing.cpp
Comment on lines +711 to 722
/**
* Reconsider orphan transactions after a parent has been accepted to the mempool.
*
* @param[in] node_id The peer whose orphan transactions we will reconsider. Generally only one
* orphan will be reconsidered on each call of this function. This set
* may be added to if accepting an orphan causes its children to be
* reconsidered.
* @return True if there are still orphans in this peer's work set.
*/
bool ProcessOrphanTx(NodeId node_id)
EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, cs_main);
/** Process a single headers message from a peer. */
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Verify the impact of processing multiple orphans in a single call

The new implementation processes orphans in a loop until none are available, which differs from the previous per-transaction approach. This could potentially cause performance issues if many orphans are queued.

Consider adding a limit to the number of orphans processed per call to prevent potential DoS scenarios where a peer could cause excessive CPU usage.


🏁 Script executed:

#!/bin/bash
# Check if there are any limits on orphan processing in the codebase
rg -A 5 -B 5 "ProcessOrphanTx|orphan.*limit|MAX.*ORPHAN" --type cpp

Length of output: 11095


Limit orphan reprocessing per ProcessOrphanTx invocation

The new ProcessOrphanTx(NodeId) loops until no more orphans remain, which means a malicious peer could enqueue a large batch and tie up your node’s CPU in one call. There is no existing cap on “orphans processed per call”—the only limits today are overall orphan‐cache size (-maxorphantxsize) and eviction in LimitOrphans().

Please add a per‐invocation limit around line 711 in src/net_processing.cpp, for example:

  • Introduce a new constant or config option (e.g. -maxreprocorphans)
  • Track a counter inside the loop and break out once the limit is reached

This will prevent a single ProcessOrphanTx call from processing unbounded orphans and hardening against DoS.

🤖 Prompt for AI Agents
In src/net_processing.cpp around lines 711 to 722, the ProcessOrphanTx function
currently processes orphans in a loop without a limit, allowing a malicious peer
to cause high CPU usage. To fix this, introduce a new constant or configuration
option like -maxreprocorphans to define the maximum orphans processed per call.
Then, add a counter inside the processing loop to track how many orphans have
been processed and break out of the loop once this limit is reached, preventing
unbounded processing in a single invocation.

Comment thread src/util/check.cpp
Comment on lines +7 to +26
#if defined(HAVE_CONFIG_H)
#include <config/bitcoin-config.h>
#endif

#include <clientversion.h>
#include <tinyformat.h>

#include <cstdio>
#include <cstdlib>
#include <string>


NonFatalCheckError::NonFatalCheckError(const char* msg, const char* file, int line, const char* func)
: std::runtime_error{
strprintf("Internal bug detected: \"%s\"\n%s:%d (%s)\n"
"%s %s\n"
"Please report this issue here: %s\n",
msg, file, line, func, PACKAGE_NAME, FormatFullVersion(), PACKAGE_BUGREPORT)}
{
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Verify this change belongs in the GUI translation PR

This new NonFatalCheckError constructor and related includes appear completely unrelated to the PR objective of avoiding leading whitespace in translatable strings. This suggests potential scope creep.


🏁 Script executed:

#!/bin/bash
# Verify if check.cpp changes are part of Bitcoin Core commit 64879f4c03
echo "Checking Bitcoin Core commit 64879f4c03 for check.cpp changes..."
git show 64879f4c03 --name-only | grep -E "(check\.cpp|check\.h)"
echo ""
echo "Searching for related Bitcoin commits with NonFatalCheckError..."
git log --oneline --grep="NonFatalCheckError" --all | head -5

Length of output: 547


Remove scope creep in src/util/check.cpp

The new NonFatalCheckError constructor and its related includes weren’t introduced by the upstream Bitcoin Core commit (no 64879f4c03 reference and no history of NonFatalCheckError) and have nothing to do with GUI translation whitespace. Please remove these changes or move them into a separate PR focused on error-reporting improvements.

• src/util/check.cpp: lines 7–26

--- a/src/util/check.cpp
+++ b/src/util/check.cpp
@@ lines 7–26
- #if defined(HAVE_CONFIG_H)
- #include <config/bitcoin-config.h>
- #endif
-
- #include <clientversion.h>
-
- #include <tinyformat.h>
-
- #include <cstdio>
- #include <cstdlib>
- #include <string>
-
-
- NonFatalCheckError::NonFatalCheckError(const char* msg, const char* file, int line, const char* func)
-     : std::runtime_error{
-           strprintf("Internal bug detected: \"%s\"\n%s:%d (%s)\n"
-                     "%s %s\n"
-                     "Please report this issue here: %s\n",
-                     msg, file, line, func, PACKAGE_NAME, FormatFullVersion(), PACKAGE_BUGREPORT)}
- {
- }
📝 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
#if defined(HAVE_CONFIG_H)
#include <config/bitcoin-config.h>
#endif
#include <clientversion.h>
#include <tinyformat.h>
#include <cstdio>
#include <cstdlib>
#include <string>
NonFatalCheckError::NonFatalCheckError(const char* msg, const char* file, int line, const char* func)
: std::runtime_error{
strprintf("Internal bug detected: \"%s\"\n%s:%d (%s)\n"
"%s %s\n"
"Please report this issue here: %s\n",
msg, file, line, func, PACKAGE_NAME, FormatFullVersion(), PACKAGE_BUGREPORT)}
{
}
🤖 Prompt for AI Agents
In src/util/check.cpp between lines 7 and 26, the newly added NonFatalCheckError
constructor and its related includes were not part of the original upstream
Bitcoin Core commit and are unrelated to the GUI translation whitespace changes.
To fix this, remove the entire NonFatalCheckError constructor implementation and
any associated includes introduced in these lines, or alternatively move these
changes into a separate pull request dedicated to error-reporting improvements.

Comment thread src/validation.h
#include <util/translation.h>

#include <atomic>
#include <chrono>
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

VALIDATION: FAIL - Major scope creep detected

These changes add blockchain validation timing infrastructure by including <chrono> and adding m_last_write/m_last_flush timing member variables to the CChainState class.

Scope Creep: This blockchain validation timing functionality is completely unrelated to the stated PR objective of "avoiding error-prone leading whitespace in translatable strings" from Bitcoin Core GUI commit #771.

This represents major scope creep that violates backport integrity. Remove these unrelated blockchain timing changes:

-#include <chrono>
-    std::chrono::microseconds m_last_write{0};
-    std::chrono::microseconds m_last_flush{0};
-

The validation timing infrastructure should be part of a separate, properly scoped commit if needed.

Also applies to: 813-814

🤖 Prompt for AI Agents
In src/validation.h at line 34 and also lines 813-814, remove the inclusion of
<chrono> and the added timing member variables m_last_write and m_last_flush
from the CChainState class. These changes introduce unrelated blockchain
validation timing functionality that causes major scope creep and violate the
PR's focus on fixing leading whitespace in translatable strings. Keep the PR
strictly limited to the whitespace fix and move the timing additions to a
separate, properly scoped commit.

Comment on lines +93 to +94
bool is_feerate_high = utxo_pool.at(0).fee > utxo_pool.at(0).long_term_fee;

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

VALIDATION: FAIL - Scope creep detected

This code refactoring introduces a local boolean variable is_feerate_high to improve readability of the fee rate comparison logic. However, this change is completely unrelated to the stated PR objective of "avoiding error-prone leading whitespace in translatable strings" from Bitcoin Core GUI commit #771.

Scope Creep: This is a code readability optimization that has no connection to GUI translation string fixes. According to the coding guidelines, backports should only include the original Bitcoin changes plus minimal Dash adaptations.

Remove this unrelated refactoring to maintain backport integrity:

-    bool is_feerate_high = utxo_pool.at(0).fee > utxo_pool.at(0).long_term_fee;
-
 
     // Depth First search loop for choosing the UTXOs
     for (size_t curr_try = 0, utxo_pool_index = 0; curr_try < TOTAL_TRIES; ++curr_try, ++utxo_pool_index) {
         // Conditions for starting a backtrack
         bool backtrack = false;
         if (curr_value + curr_available_value < selection_target || // Cannot possibly reach target with the amount remaining in the curr_available_value.
             curr_value > selection_target + cost_of_change || // Selected value is out of range, go back and try other branch
-            (curr_waste > best_waste && is_feerate_high)) { // Don't select things which we know will be more wasteful if the waste is increasing
+            (curr_waste > best_waste && (utxo_pool.at(0).fee > utxo_pool.at(0).long_term_fee))) { // Don't select things which we know will be more wasteful if the waste is increasing

Also applies to: 101-101

🤖 Prompt for AI Agents
In src/wallet/coinselection.cpp at lines 93 to 94, remove the newly introduced
local boolean variable is_feerate_high and revert the code to directly compare
utxo_pool.at(0).fee and utxo_pool.at(0).long_term_fee without assigning the
result to a separate variable. This eliminates the unrelated code refactoring
and keeps the backport focused solely on the original GUI translation string
fixes.

Comment on lines +1490 to +1595
/** RAII class that provides access to a FailDatabase. Which fails if needed. */
class FailBatch : public DatabaseBatch
{
private:
bool m_pass{true};
bool ReadKey(CDataStream&& key, CDataStream& value) override { return m_pass; }
bool WriteKey(CDataStream&& key, CDataStream&& value, bool overwrite=true) override { return m_pass; }
bool EraseKey(CDataStream&& key) override { return m_pass; }
bool HasKey(CDataStream&& key) override { return m_pass; }

public:
explicit FailBatch(bool pass) : m_pass(pass) {}
void Flush() override {}
void Close() override {}

bool StartCursor() override { return true; }
bool ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool& complete) override { return false; }
void CloseCursor() override {}
bool TxnBegin() override { return false; }
bool TxnCommit() override { return false; }
bool TxnAbort() override { return false; }
};

/** A dummy WalletDatabase that does nothing, only fails if needed.**/
class FailDatabase : public WalletDatabase
{
public:
bool m_pass{true}; // false when this db should fail

void Open() override {};
void AddRef() override {}
void RemoveRef() override {}
bool Rewrite(const char* pszSkip=nullptr) override { return true; }
bool Backup(const std::string& strDest) const override { return true; }
void Close() override {}
void Flush() override {}
bool PeriodicFlush() override { return true; }
void IncrementUpdateCounter() override { ++nUpdateCounter; }
void ReloadDbEnv() override {}
std::string Filename() override { return "faildb"; }
std::string Format() override { return "faildb"; }
std::unique_ptr<DatabaseBatch> MakeBatch(bool flush_on_close = true) override { return std::make_unique<FailBatch>(m_pass); }
};

/**
* Checks a wallet invalid state where the inputs (prev-txs) of a new arriving transaction are not marked dirty,
* while the transaction that spends them exist inside the in-memory wallet tx map (not stored on db due a db write failure).
*/
BOOST_FIXTURE_TEST_CASE(wallet_sync_tx_invalid_state_test, TestChain100Setup)
{
CWallet wallet(m_node.chain.get(), m_node.coinjoin_loader.get(), "", m_args, std::make_unique<FailDatabase>());
{
LOCK(wallet.cs_wallet);
wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
wallet.SetupDescriptorScriptPubKeyMans("", "");
}

// Add tx to wallet
CTxDestination dest;
bilingual_str error;
BOOST_ASSERT(wallet.GetNewDestination("", dest, error));

CMutableTransaction mtx;
mtx.vout.push_back({COIN, GetScriptForDestination(dest)});
mtx.vin.push_back(CTxIn(g_insecure_rand_ctx.rand256(), 0));
const auto& tx_id_to_spend = wallet.AddToWallet(MakeTransactionRef(mtx), TxStateInMempool{})->GetHash();

{
// Cache and verify available balance for the wtx
LOCK(wallet.cs_wallet);
const CWalletTx* wtx_to_spend = wallet.GetWalletTx(tx_id_to_spend);
BOOST_CHECK_EQUAL(CachedTxGetAvailableCredit(wallet, *wtx_to_spend), 1 * COIN);
}

// Now the good case:
// 1) Add a transaction that spends the previously created transaction
// 2) Verify that the available balance of this new tx and the old one is updated (prev tx is marked dirty)

mtx.vin.clear();
mtx.vin.push_back(CTxIn(tx_id_to_spend, 0));
wallet.transactionAddedToMempool(MakeTransactionRef(mtx), 0, 0);
const uint256& good_tx_id = mtx.GetHash();

{
// Verify balance update for the new tx and the old one
LOCK(wallet.cs_wallet);
const CWalletTx* new_wtx = wallet.GetWalletTx(good_tx_id);
BOOST_CHECK_EQUAL(CachedTxGetAvailableCredit(wallet, *new_wtx), 1 * COIN);

// Now the old wtx
const CWalletTx* wtx_to_spend = wallet.GetWalletTx(tx_id_to_spend);
BOOST_CHECK_EQUAL(CachedTxGetAvailableCredit(wallet, *wtx_to_spend), 0 * COIN);
}

// Now the bad case:
// 1) Make db always fail
// 2) Try to add a transaction that spends the previously created transaction and
// verify that we are not moving forward if the wallet cannot store it
static_cast<FailDatabase&>(wallet.GetDatabase()).m_pass = false;
mtx.vin.clear();
mtx.vin.push_back(CTxIn(good_tx_id, 0));
BOOST_CHECK_EXCEPTION(wallet.transactionAddedToMempool(MakeTransactionRef(mtx), 0, 0),
std::runtime_error,
HasReason("DB error adding transaction to wallet, write failed"));
}

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

VALIDATION: FAIL - Complete mismatch between PR objectives and actual changes

This PR claims to backport bitcoin-core/gui#771 "Avoid error-prone leading whitespace in translatable strings", but these changes add wallet database failure tests instead. This is not related to GUI translation strings at all.

File Operations: MISMATCH - Expected GUI/translation files, got wallet test files
Bitcoin-Specific Code: NONE
Scope Creep: DETECTED - Adding unrelated wallet test functionality

The actual changes should be modifying translatable strings to remove leading whitespace, likely in GUI-related files.

🤖 Prompt for AI Agents
In src/wallet/test/wallet_tests.cpp lines 1490 to 1595, the changes introduce
wallet database failure tests unrelated to the PR's stated goal of removing
leading whitespace in translatable GUI strings. To fix this, revert these wallet
test additions and instead focus on modifying GUI-related files to remove
leading whitespace from translatable strings as originally intended by the PR.

Comment on lines +60 to +62
self.log.info("setban: fail to ban with past absolute timestamp")
assert_raises_rpc_error(-8, "Error: Absolute timestamp is in the past", self.nodes[1].setban, "127.27.0.1", "add", 123, True)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM! Good validation of past timestamp rejection.

The test correctly verifies that attempting to ban with a past absolute timestamp fails with the expected error.

Consider using named parameters for better readability:

-        assert_raises_rpc_error(-8, "Error: Absolute timestamp is in the past", self.nodes[1].setban, "127.27.0.1", "add", 123, True)
+        assert_raises_rpc_error(-8, "Error: Absolute timestamp is in the past", self.nodes[1].setban, "127.27.0.1", "add", bantime=123, absolute=True)
📝 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.log.info("setban: fail to ban with past absolute timestamp")
assert_raises_rpc_error(-8, "Error: Absolute timestamp is in the past", self.nodes[1].setban, "127.27.0.1", "add", 123, True)
self.log.info("setban: fail to ban with past absolute timestamp")
assert_raises_rpc_error(-8, "Error: Absolute timestamp is in the past", self.nodes[1].setban, "127.27.0.1", "add", bantime=123, absolute=True)
🧰 Tools
🪛 Ruff (0.12.2)

61-61: Boolean positional value in function call

(FBT003)

🤖 Prompt for AI Agents
In test/functional/p2p_disconnect_ban.py around lines 60 to 62, the test
correctly checks banning with a past timestamp but uses positional arguments in
the setban call. Improve readability by changing the setban call to use named
parameters for the IP, command, ban time, and absolute flag.

Comment on lines +81 to +86
self.log.info("setban: test banning with absolute timestamp")
self.nodes[1].setban("192.168.0.2", "add", self.mocktime + 120, True)

# Move time forward by 3 seconds so the third ban has expired
self.bump_mocktime(3)
self.wait_until(lambda: len(self.nodes[1].listbanned()) == 4, timeout=10)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM! Good test coverage for absolute timestamp banning.

The test correctly validates banning with future absolute timestamps and verifies the ban list updates appropriately.

Consider using named parameters for better readability:

-        self.nodes[1].setban("192.168.0.2", "add", self.mocktime + 120, True)
+        self.nodes[1].setban("192.168.0.2", "add", bantime=self.mocktime + 120, absolute=True)
📝 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.log.info("setban: test banning with absolute timestamp")
self.nodes[1].setban("192.168.0.2", "add", self.mocktime + 120, True)
# Move time forward by 3 seconds so the third ban has expired
self.bump_mocktime(3)
self.wait_until(lambda: len(self.nodes[1].listbanned()) == 4, timeout=10)
self.log.info("setban: test banning with absolute timestamp")
- self.nodes[1].setban("192.168.0.2", "add", self.mocktime + 120, True)
+ self.nodes[1].setban("192.168.0.2", "add", bantime=self.mocktime + 120, absolute=True)
# Move time forward by 3 seconds so the third ban has expired
self.bump_mocktime(3)
self.wait_until(lambda: len(self.nodes[1].listbanned()) == 4, timeout=10)
🧰 Tools
🪛 Ruff (0.12.2)

82-82: Boolean positional value in function call

(FBT003)

🤖 Prompt for AI Agents
In test/functional/p2p_disconnect_ban.py around lines 81 to 86, improve
readability by using named parameters in the setban call. Replace positional
arguments with explicit parameter names such as address=, command=, ban_time=,
and absolute= to clarify the purpose of each argument in the method call.

Comment on lines +344 to +346
self.log.debug("Test that adding an address with invalid port fails")
assert_raises_rpc_error(-1, "JSON integer out of range", self.nodes[0].addpeeraddress, address="1.2.3.4", port=-1)
assert_raises_rpc_error(-1, "JSON integer out of range", self.nodes[0].addpeeraddress,address="1.2.3.4", port=65536)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Fix syntax error and verify RPC error handling.

The port validation logic is correct, but there's a syntax error on line 346.

-        assert_raises_rpc_error(-1, "JSON integer out of range", self.nodes[0].addpeeraddress,address="1.2.3.4", port=65536)
+        assert_raises_rpc_error(-1, "JSON integer out of range", self.nodes[0].addpeeraddress, address="1.2.3.4", port=65536)

Also verify that the RPC implementation correctly validates uint16_t port ranges and throws the expected error code -1.

📝 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.log.debug("Test that adding an address with invalid port fails")
assert_raises_rpc_error(-1, "JSON integer out of range", self.nodes[0].addpeeraddress, address="1.2.3.4", port=-1)
assert_raises_rpc_error(-1, "JSON integer out of range", self.nodes[0].addpeeraddress,address="1.2.3.4", port=65536)
self.log.debug("Test that adding an address with invalid port fails")
assert_raises_rpc_error(-1, "JSON integer out of range", self.nodes[0].addpeeraddress, address="1.2.3.4", port=-1)
assert_raises_rpc_error(-1, "JSON integer out of range", self.nodes[0].addpeeraddress, address="1.2.3.4", port=65536)
🤖 Prompt for AI Agents
In test/functional/rpc_net.py around lines 344 to 346, fix the syntax error on
line 346 by adding a space after the comma between the function argument
'self.nodes[0].addpeeraddress' and the keyword argument 'address="1.2.3.4"'.
Also, verify that the RPC implementation properly validates port numbers as
uint16_t and raises the expected error code -1 with the message "JSON integer
out of range" for out-of-range port values.

import sys


def main():
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Minor: Add return type annotation

Consider adding the return type annotation to address the static analysis hint.

-def main():
+def main() -> None:
📝 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
def main():
-def main():
+def main() -> None:
🧰 Tools
🪛 Ruff (0.12.2)

13-13: Missing return type annotation for public function main

Add return type annotation: None

(ANN201)

🤖 Prompt for AI Agents
In test/lint/lint-qt-translation.py at line 13, the main function lacks a return
type annotation. Add the appropriate return type annotation, such as -> None, to
the main function definition to satisfy static analysis requirements.

@DashCoreAutoGuix
Copy link
Copy Markdown
Owner Author

❌ Backport Verification - CATASTROPHIC FAILURE

Original Bitcoin commit: 64879f4c03b (bitcoin-core/gui#771: Avoid error-prone leading whitespace in translatable strings)
Reviewed commit hash: cb4d0e2e357-verify-1754378653

CRITICAL VIOLATIONS DETECTED:

🚨 Scope Explosion Violation

  • Bitcoin commit: 6 files, 47 changes (36 additions, 11 deletions)
  • Your PR: 32 files, 1084 changes (695 additions, 389 deletions)
  • Size ratio: 2306.4% (should be 80-150%)

🚨 File Operation Mismatch

  • 28 EXTRA FILES modified that don't exist in Bitcoin commit
  • Bitcoin only touched: GUI translation files (src/qt/, test/lint/)
  • Your PR modifies entire Bitcoin infrastructure (wallet, networking, validation, testing)

🚨 Intent Violation

What Bitcoin Did: Fix leading whitespace in 6 GUI translation strings
What Your PR Does:

  • Import entire wallet infrastructure changes
  • Add orphan transaction processing systems
  • Modify core validation and networking code
  • Add massive wallet testing infrastructure
  • Import unrelated build system changes

🚨 CI Status: NORMAL

  • 0 build failures (not relevant due to catastrophic scope violations)
  • CI status irrelevant when fundamental backport principles are violated

🚨 Reviewer Feedback Analysis

Human reviewer comments reviewed: 9 critical violations identified

  • Scope creep detected: Unrelated wallet timing infrastructure
  • Intent violation: Adding NonFatalCheckError constructor unrelated to GUI strings
  • Dependency explosion: Wallet database failure tests completely unrelated to translation strings
  • File operation mismatch: Expected GUI files, got wallet/validation/network files

ALL comments indicate fundamental scope violations - this is not a faithful Bitcoin backport.

🚨 Auto-Reject Conditions Met

  1. ✅ File operation mismatch (missing 2 Bitcoin GUI files, adding 28 unrelated files)
  2. ✅ Severe scope explosion (2306.4% vs expected 80-150%)
  3. ✅ Dependency explosion (GUI strings → entire Bitcoin infrastructure)
  4. ✅ Infrastructure import (wallet, networking, validation systems)
  5. ✅ Intent violation (translation strings → wholesale Bitcoin feature import)

Required Action: COMPLETE REWRITE

This PR violates EVERY fundamental principle of faithful backporting:

  1. Start over: Discard current changes completely
  2. Study Bitcoin commit: Only implement the GUI translation string whitespace fixes
  3. Scope discipline: Zero wallet/networking/validation changes
  4. Size validation: Aim for ~47 lines total, not 1084

Example of correct scope:

// Bitcoin commit 64879f4c03b changes ONLY these files:
src/qt/psbtoperationsdialog.cpp
src/qt/psbtoperationsdialog.h  
src/qt/sendcoinsdialog.cpp
src/qt/transactiondesc.cpp
src/qt/walletcontroller.cpp
test/lint/lint-qt-translation.py

This PR represents a fundamental misunderstanding of Bitcoin commit 64879f4c03b. Please review the actual Bitcoin changes and implement ONLY the GUI translation string whitespace fixes.

Status: REJECTED - Requires complete rewrite


🚫 This PR has been automatically closed due to catastrophic validation failures. Please create a new PR with a proper backport implementation.
EOF < /dev/null

@DashCoreAutoGuix
Copy link
Copy Markdown
Owner Author

Automatically closed due to catastrophic validation failures. Please see the detailed analysis above and create a new PR with a proper backport implementation.

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.

6 participants