Skip to content

Conversation

@knst
Copy link
Collaborator

@knst knst commented Oct 26, 2025

What was done?

Trivial backports from Bitcoin Core v25

How Has This Been Tested?

Run & unit functional tests

Breaking Changes

N/A

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)

achow101 and others added 11 commits October 27, 2025 00:13
…cked wallet

2c03465 test: Test watchonly imports with passphrase-locked wallet (Aurèle Oulès)
1fcf9e6 rpc: Allow importmulti watchonly imports with locked wallet (Aurèle Oulès)

Pull request description:

  Allows watch-only imports on locked wallets with `importmulti`.
  Also adds a test.

  Fixes bitcoin#17867.

ACKs for top commit:
  achow101:
    ACK 2c03465
  kristapsk:
    re-ACK 2c03465
  theStack:
    re-ACK 2c03465

Tree-SHA512: 9978d6e59a230c0d160efd312c671cf59458797387d6622b6bf5c9e0681c1fcfebedb3d834fa9314dc5a1eda97e3295696352eacbeab9b43a46b942990087035
…er_tests

fa9436e test: Remove unused fCheckpointsEnabled from miner_tests (MacroFake)

Pull request description:

  The earliest checkpoint is at height 11111, so this can't possibly have any impact on this test.

ACKs for top commit:
  fanquake:
    ACK fa9436e - given the low number of blocks, having the additional check in `ContextualCheckBlockHeader()` enabled should be a no-op, so disabling and re-enabling is dead code.

Tree-SHA512: 7d1b952c297c915e9588761f82f5006cf5186b7ff30e8a1c702302e0b44afe536bde9eda8acf2995825ae01d2ad9d2393ae2feefb29f15676aaf71881941579b
303fb8f doc: mention BIP86 in doc/bips.md (Sebastian Falbesoner)

Pull request description:

ACKs for top commit:
  fanquake:
    ACK 303fb8f

Tree-SHA512: f30de5be3c789d315d0118594671e9f6a5c7b6e9ec7b7f1c9428f582eeff13946c37ebae26910a2134091e284f30499fcc62923f873418d0ba46a0322af998ad
…le blocks

f39d926 rpc: warn that nodes ignore requests for old stale blocks (Sjors Provoost)

Pull request description:

  Adds warning to RPC help that `getblockfrompeer` is of little use for stale blocks that are more than a month old.

  This is an anti-fingerprinting measure. See `BlockRequestAllowed` in `net_processing`.

  It's been in Bitcoin Core since 2014, introduced in dashpay#2910 and later improved to not rely on checkpoints.
  Older and alternative clients might still serve these blocks, so not throwing an error.

  Allowing whitelisted nodes to fetch these blocks anyway might be nice.

ACKs for top commit:
  fjahr:
    Code review ACK f39d926

Tree-SHA512: db88f9f7521289640c5e629c840dda1c2c3ab70d458e9e7136c60fbaeb02acfb36dc093502d83d4c098c331e22aab81bf8f4c4961d805e3bde0f8f3cfe68d968
…he virtual size instead of the transaction hex for feerate calculation.

6fb102c test: Changed small_txpuzzle_randfee to return the virtual size instead of the transaction hex for feerate calculation. (Randall Naar)

Pull request description:

  The fee rates used in feature_fee_estimation.py are calculated using the raw transaction size instead of the virtual transaction size (which is used in 'CBlockPolicyEstimator::processBlockTx' and 'CBlockPolicyEstimator::processBlock'). This leads to inconsistencies as the fee rates used in check_raw_estimates are incorrect and can cause assertions to fail.

  refs bitcoin#25179

ACKs for top commit:
  MarcoFalke:
    ACK 6fb102c

Tree-SHA512: b2bca85fffa605aeb17574f050736d4556506d782dd7fd969e165e48e108fd95ef4c4e2abbae83cce05ca777a00f4459cabfa0932694fa8bb93ddfba09d84357
…etwalletflag`

3666a06 test: add coverage for unknown wallet flag in `setwalletflag` (brunoerg)

Pull request description:

  This PR adds test coverage for the following error:
  https://github.com/bitcoin/bitcoin/blob/6d40a1a7e7f09ff2c32e53237f968adf8300d028/src/wallet/rpc/wallet.cpp#L275-L277

  https://marcofalke.github.io/btc_cov/total.coverage/src/wallet/rpc/wallet.cpp.gcov.html

ACKs for top commit:
  aureleoules:
    ACK 3666a06

Tree-SHA512: b6b2415442dfbc2404aed9720cc899995686007d6ba222dae461d064e4454d5af1326d3d527770b51b1005721ac42a49972f1eabf21108f656c30d3584790747
…get`

7a83aa0 test: add coverage for unparsable `-maxuploadtarget` (brunoerg)

Pull request description:

  This PR adds test coverage for the following error:
  https://github.com/bitcoin/bitcoin/blob/7386da7a0b08cd2df8ba88dae1fab9d36424b15c/src/init.cpp#L1096-L1099

Top commit has no ACKs.

Tree-SHA512: c115b2b4d2d0eb2316bf9fafd7e0046aa18c9650062779b3a82d6145d188765bff5317f4ca5f79607732fde6d83e1f67756ac20a12c98d060ee68d8acc20c76e
e846269 util: Remove duplicate include (Andrew Chow)

Pull request description:

  Duplicate `#include <utility>` is upsetting the linter.

ACKs for top commit:
  davidgumberg:
    ACK bitcoin@e846269
  theStack:
    ACK e846269
  john-moffett:
    ACK e846269

Tree-SHA512: 9e45d8f6a2dd5efcb8eb1c3c440d94b16490dbd63255784cb39863767fa07227e06da112a150ef337ef89e2e305b60b00d5b1c12ff7e1e9c02f6648ed97fac8c
…maintainers leaving

14fac80 verify-commits: Mention git v2.38.0 requirement (Andrew Chow)
bb86887 verify-commits: Skip checks for commits older than trusted roots (Andrew Chow)
5497c14 verify-commits: Use merge-tree in clean merge check (Andrew Chow)
76923bf verify-commits: Remove all allowed commit exceptions (Andrew Chow)
53b07b2 verify-commits: Move trusted-keys valid sig check into verify-commits itself (Andrew Chow)

Pull request description:

  Currently the `verify-commits.py` script does not work well with maintainers giving up their commit access. If a key is removed from `trusted-keys`, any commits it signed previously will fail to verify, however keys cannot be kept in the list as it would allow that person to continue to push new commits. Furthermore, the `trusted-keys` used depends on the working tree which `verify-commits.py` itself may be modifying. When the script is run, the `trusted-keys` may be the one that is intended to be used, but the script may change the tree to a different commit with a different `trusted-keys` and use that instead!

  To resolve these issues, I've updated `verify-commits.py` to load the `trusted-keys` file and check the keys itself rather than delegating that to `gpg.sh` (which previously read in `trusted-keys`). This avoids the issue with the tree changing.

  I've also updated the script so that it stops modifying the tree. It would do this for the clean merge check where it would checkout each individual commit and attempt to reapply the merges, and then checking out the commit given as a cli arg. `git merge-tree` lets us do basically that but without modifying the tree. It will give us the object id for the resulting tree which we can compare against the object id of the tree in the merge commit in question. This also appears to be quite a bit faster.

  Lastly I've removed all of the exception commits in `allow-revsig-commits`, `allow-incorrect-sha512-commits`, and `allow-unclean-merge-commits` since all of these predate the commits in `trusted-git-root` and `trusted-sha512-root`. I've also updated the script to skip verification of commits that predate `trusted-git-root`, and skip sha512 verification for those that predate `trusted-sha512-root`.

ACKs for top commit:
  Sjors:
    ACK 14fac80
  glozow:
    Concept ACK 14fac80

Tree-SHA512: f9b0c6e1f1aecb169cdd6c833b8871b15e31c2374dc589858df0523659b294220d327481cc36dd0f92e9040d868eee6a8a68502f3163e05fa751f9fc2fa8832a
…eveloper-notes

da347de doc: update broken links (pablomartin4btc)

Pull request description:

  References to `utilstrencodings` and `lint-locale-dependence.sh` where incorrect, updating them accordingly.

  Also, adding another reference to util function [`LocaleIndependentAtoi`](https://github.com/bitcoin/bitcoin/blob/master/src/util/strencodings.h#L108-L118), which is related with the updated section of the guide:

  ```
  // LocaleIndependentAtoi is provided for backwards compatibility reasons.
  //
  // New code should use ToIntegral or the ParseInt* functions
  // which provide parse error feedback.
  //
  // The goal of LocaleIndependentAtoi is to replicate the defined behaviour of
  // std::atoi as it behaves under the "C" locale, and remove some undefined
  // behavior. If the parsed value is bigger than the integer type's maximum
  // value, or smaller than the integer type's minimum value, std::atoi has
  // undefined behavior, while this function returns the maximum or minimum
  // values, respectively.
  ```

ACKs for top commit:
  MarcoFalke:
    lgtm ACK da347de

Tree-SHA512: c8f4cd9cff1fb3ea367ac9dbe5aa45dc187fc60114f2e2106e02e0e17fea4ee34d6e0c408fe920c2d8765e06b4dc30c231f0454fa35469c4399e0cadbcd341ba
…is too old.

1fefcf2 verify-commits: error and exit cleanly when git is too old. (Cory Fields)

Pull request description:

  Requested by fanquake. Rather than failing with a cryptic error with older git, fail gracefully and mention why.

  The new option semantics [are explained here](git/git@1f0c3a2).

  Note: my local git versions are currently too old to test the new functionality, so I've only verified the failure case.

ACKs for top commit:
  josibake:
    ACK bitcoin@1fefcf2
  achow101:
    ACK 1fefcf2

Tree-SHA512: f3dc583edf6ff6ff9bf06f33de967e10b8423ce62e7370912ffdca8a4ca4bfe4c2e783e9ad76281ce9e6748a4643d187aa5cb4a6b9ec4c1582910f02b94b6e3c
@knst knst added this to the 23.1 milestone Oct 26, 2025
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Overview

Greptile Summary

This PR backports 11 trivial changes from Bitcoin Core v25, consisting primarily of minor cleanups, documentation improvements, and test enhancements. The changes include removing an unused #include <utility> header from lockedpool.cpp, adding test cases for RPC error handling (setwalletflag validation, -maxuploadtarget parsing), improving RPC documentation (getblockfrompeer), simplifying GPG verification scripts, and optimizing importmulti to skip wallet unlocking for watchonly imports. The backports also update developer guidelines to recommend ToIntegral for number parsing, modernize the commit verification workflow to use git merge-tree instead of checkouts, and add clarifying comments about timeout behavior in block fetching. These changes integrate seamlessly with Dash Core's existing codebase as they primarily touch test infrastructure, documentation, and utility code rather than consensus-critical or Dash-specific features like masternodes or LLMQ.

PR Description Notes:

  • Several checklist items are unchecked (comments, tests, documentation updates), but this is acceptable for trivial backports that don't introduce new features.

Important Files Changed

Filename Score Overview
src/support/lockedpool.cpp 5/5 Removed unused #include <utility> header dependency with no functional impact
contrib/verify-commits/README.md 5/5 Added documentation for --clean-merge option requiring git v2.38.0+
test/functional/wallet_avoidreuse.py 5/5 Added test assertion to verify setwalletflag rejects unknown wallet flags
test/functional/feature_maxuploadtarget.py 5/5 Added test case for -maxuploadtarget parsing error handling
test/functional/wallet_importmulti.py 5/5 Added test verifying watchonly imports work on encrypted wallets without unlocking
src/net_processing.cpp 5/5 Added documentation comment explaining block request timeout behavior
src/rpc/blockchain.cpp 5/5 Enhanced RPC help text for getblockfrompeer with peer behavior clarifications
contrib/verify-commits/gpg.sh 4/5 Simplified GPG verification by delegating to GPG exit codes; has minor indentation inconsistencies
src/test/miner_tests.cpp 5/5 Removed unnecessary fCheckpointsEnabled global state manipulation from tests
test/functional/feature_fee_estimation.py 5/5 Refactored to return vsize directly instead of serialized hex for efficiency
doc/bips.md 2/5 Added BIP86 documentation for Taproot feature not implemented in Dash Core
doc/developer-notes.md 4/5 Updated number parsing guidelines and lint script references; contains duplicate section
src/wallet/rpc/backup.cpp 3/5 Optimized importmulti to skip unlocking for watchonly imports; lacks type validation
contrib/verify-commits/verify-commits.py 4/5 Modernized commit verification with direct GPG parsing and non-destructive merge checks

Confidence score: 3/5

  • This PR contains mostly safe changes but has several notable issues that prevent a higher score.
  • Score lowered due to: (1) BIP 86 documentation in doc/bips.md references Taproot functionality that doesn't exist in Dash Core, creating misleading documentation; (2) missing type validation in src/wallet/rpc/backup.cpp where request["watchonly"].get_bool() is called without verifying the field is a boolean; (3) duplicate section in doc/developer-notes.md; (4) inconsistent indentation in contrib/verify-commits/gpg.sh; (5) the modernized verify-commits.py changes GPG validation logic significantly and may require git v2.38.0+ in CI environments.
  • Pay close attention to doc/bips.md (Taproot documentation mismatch), src/wallet/rpc/backup.cpp (potential runtime exception), and contrib/verify-commits/verify-commits.py (verify git version compatibility in CI).

Additional Comments (1)

  1. doc/developer-notes.md, line 1313-1327 (link)

    syntax: Lines 1313-1327 duplicate the "Release notes" section (lines1299-1312). Remove the duplicate.

Context used:

  • Context from dashboard - CLAUDE.md (source)

14 files reviewed, 5 comments

Edit Code Review Agent Settings | Greptile

"gpg (GnuPG) 1.4.1"*|"gpg (GnuPG) 2.0."*)
echo "Please upgrade to at least gpg 2.1.10 to check for weak signatures" > /dev/stderr
GPG_RES="$(printf '%s\n' "$INPUT" | gpg --trust-model always "$@" 2>/dev/null)"
printf '%s\n' "$INPUT" | gpg --trust-model always "$@" 2>/dev/null
Copy link

Choose a reason for hiding this comment

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

style: Inconsistent indentation: spaces used instead of tabs (file uses tabs elsewhere)

Prompt To Fix With AI
This is a comment left during a code review.
Path: contrib/verify-commits/gpg.sh
Line: 24:24

Comment:
**style:** Inconsistent indentation: spaces used instead of tabs (file uses tabs elsewhere)

How can I resolve this? If you propose a fix, please make it concise.

echo "$GOODREVSIG"
else
printf '%s\n' "$INPUT" | gpg --trust-model always "$@" 2>/dev/null
printf '%s\n' "$INPUT" | gpg --trust-model always --weak-digest sha1 "$@" 2>/dev/null
Copy link

Choose a reason for hiding this comment

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

style: Inconsistent indentation: spaces used instead of tabs (file uses tabs elsewhere)

Prompt To Fix With AI
This is a comment left during a code review.
Path: contrib/verify-commits/gpg.sh
Line: 33:33

Comment:
**style:** Inconsistent indentation: spaces used instead of tabs (file uses tabs elsewhere)

How can I resolve this? If you propose a fix, please make it concise.

and it is disabled by default at build time since **v0.19.0** ([PR #15584](https://github.com/bitcoin/bitcoin/pull/15584)).
It has been removed as of **v0.20.0** ([PR 17165](https://github.com/bitcoin/bitcoin/pull/17165)).
* [`BIP 84`](https://github.com/bitcoin/bips/blob/master/bip-0084.mediawiki): The experimental descriptor wallets introduced in **v0.21.0** by default use the Hierarchical Deterministic Wallet derivation proposed by BIP 84. ([PR #16528](https://github.com/bitcoin/bitcoin/pull/16528))
* [`BIP 86`](https://github.com/bitcoin/bips/blob/master/bip-0086.mediawiki): Descriptor wallets by default use the Hierarchical Deterministic Wallet derivation proposed by BIP 86 since **v23.0** ([PR #22364](https://github.com/bitcoin/bitcoin/pull/22364)).
Copy link

Choose a reason for hiding this comment

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

logic: BIP 86 is Taproot-specific (HD derivation for P2TR outputs), but Dash Core doesn't implement Taproot. This entry may be misleading for Dash users since the referenced functionality doesn't exist in Dash. Does Dash Core actually implement BIP 86 derivation paths, or was this copied from Bitcoin without adjusting for Dash's lack of Taproot support?

Prompt To Fix With AI
This is a comment left during a code review.
Path: doc/bips.md
Line: 32:32

Comment:
**logic:** BIP 86 is Taproot-specific (HD derivation for P2TR outputs), but Dash Core doesn't implement Taproot. This entry may be misleading for Dash users since the referenced functionality doesn't exist in Dash. Does Dash Core actually implement BIP 86 derivation paths, or was this copied from Bitcoin without adjusting for Dash's lack of Taproot support?

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

see header of the file:

BIPs that are implemented by Bitcoin Core, some of them are relevant for Dash Core, some are just mentioned as a reference.
Versions and PRs are relevant to Bitcoin's core if not mentioned other.

}
}
// Wallet does not need to be unlocked if all requests are watchonly
if (!is_watchonly) EnsureWalletIsUnlocked(wallet);
Copy link

Choose a reason for hiding this comment

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

logic: The check incorrectly uses request["watchonly"].get_bool() without checking if the value exists first. If watchonly field exists but is null or not a bool, this will throw.

Suggested change
if (!is_watchonly) EnsureWalletIsUnlocked(wallet);
if (!is_watchonly) EnsureWalletIsUnlocked(*pwallet);

Should the logic check if the field is a bool type before calling get_bool(), or should we assume the field is always properly typed when it exists?

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/wallet/rpc/backup.cpp
Line: 1596:1596

Comment:
**logic:** The check incorrectly uses `request["watchonly"].get_bool()` without checking if the value exists first. If `watchonly` field exists but is null or not a bool, this will throw.

```suggestion
        if (!is_watchonly) EnsureWalletIsUnlocked(*pwallet);
```

 Should the logic check if the field is a bool type before calling get_bool(), or should we assume the field is always properly typed when it exists?

How can I resolve this? If you propose a fix, please make it concise.

@github-actions
Copy link

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

@coderabbitai
Copy link

coderabbitai bot commented Oct 26, 2025

Walkthrough

This pull request encompasses a diverse set of improvements and updates across Bitcoin Core components. Primary changes include: refactoring GPG verification logic in verify-commits scripts to simplify state tracking and add trusted-keys validation; updating documentation for BIP 86 and number parsing functions; adding explanatory comments to network processing code; enhancing RPC help text for getblockfrompeer; adjusting wallet unlock logic in importmulti to be conditional on watchonly status; removing an unnecessary include from lockedpool; disabling explicit checkpoint toggles in miner tests; and adding functional test coverage for unparsable command-line arguments, unknown wallet flags, and locked wallet import scenarios.

Sequence Diagram

sequenceDiagram
    participant User
    participant verify-commits.py
    participant gpg.sh
    participant Git
    participant trusted_keys

    User->>verify-commits.py: Run with commit range
    verify-commits.py->>verify-commits.py: Read trusted_keys file
    
    rect rgb(220, 240, 255)
    Note over verify-commits.py: Age Check Phase
    verify-commits.py->>verify-commits.py: Check if commit predates trusted root
    alt Commit too old
        verify-commits.py->>User: Exit with error
    end
    end
    
    rect rgb(220, 255, 220)
    Note over verify-commits.py,gpg.sh: Signature Verification Phase
    verify-commits.py->>gpg.sh: Request signature verification
    alt SHA1 allowed
        gpg.sh->>gpg.sh: Execute gpg with trust-model always
    else SHA1 not allowed
        gpg.sh->>gpg.sh: Execute gpg with --weak-digest sha1
    end
    gpg.sh->>verify-commits.py: Return status and VALIDSIG
    end
    
    rect rgb(255, 240, 220)
    Note over verify-commits.py,trusted_keys: Trusted Keys Validation
    verify-commits.py->>trusted_keys: Check signing key
    alt Key in trusted_keys
        verify-commits.py->>verify-commits.py: Continue verification
    else Key not trusted
        verify-commits.py->>User: Reject signature
    end
    end
    
    rect rgb(240, 220, 255)
    Note over verify-commits.py,Git: Tree Verification Phase (git v2.38+)
    alt Merge commit && clean-merge enabled
        verify-commits.py->>Git: Run git merge-tree --write-tree
        Git->>verify-commits.py: Return recreated tree
        verify-commits.py->>verify-commits.py: Compare with current tree
    end
    end
    
    verify-commits.py->>User: Report verification result
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

This diff spans multiple domains with varied complexity levels. While several changes are straightforward (documentation updates, test additions, comments), others require contextual understanding:

  • contrib/verify-commits/gpg.sh and verify-commits.py: Significant refactoring of cryptographic verification logic introducing trusted-keys validation, age checks, and git merge-tree integration. These changes are security-sensitive and demand careful verification of the simplified flow and new branching logic.
  • src/wallet/rpc/backup.cpp: Conditional wallet unlock based on watchonly status alters control flow in transaction import, requiring verification that this change doesn't inadvertently skip necessary security steps.
  • src/support/lockedpool.cpp: Removal of <utility> include while std::move is still used elsewhere in the code may have subtle implications depending on transitive includes.
  • Test additions across multiple files: New test paths for edge cases (unparsable arguments, locked wallet imports, unknown flags) should be validated for correctness and integration with existing test logic.

Human review should focus particularly on the verify-commits changes and the wallet unlock conditional logic to ensure security invariants are preserved.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 30.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "backport: bitcoin#24226, bitcoin#26116, bitcoin#26243, bitcoin#26443, bitcoin#26517, bitcoin#26714, bitcoin#26738, bitcoin#27058, bitcoin#27151, bitcoin#27220, bitcoin#27461" directly and accurately reflects the changeset. The raw summary confirms that multiple files contain changes related to backports from Bitcoin Core (verify-commits improvements, documentation updates, RPC help text updates, test modifications, and wallet functionality changes). The title clearly conveys that this is a backport effort from specific Bitcoin Core PRs and is fully related to the main objective of the pull request. While the title is lengthy due to listing multiple PR numbers, this specificity aids in understanding the scope of the backport rather than detracting from clarity.
Description Check ✅ Passed The pull request description states "Trivial backports from Bitcoin Core v25" and indicates the changes have been tested with unit and functional tests. This description is directly related to the changeset, as the raw summary confirms the pull request contains multiple modifications across various files that are backports from Bitcoin Core, including updates to verification scripts, documentation, RPC help text, and test files. The description may be minimal in detail, but it accurately conveys the nature of the changes and satisfies the lenient criteria by being clearly related to the actual changeset content.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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

@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: 0

Caution

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

⚠️ Outside diff range comments (1)
test/functional/feature_fee_estimation.py (1)

32-32: Update the docstring to reflect the actual return value.

The docstring states "Returns (raw transaction, fee)" but the function now returns (tx.get_vsize(), fee). The documentation should be updated to accurately describe the return value.

Apply this diff to update the docstring:

-    Returns (raw transaction, fee)."""
+    Returns (transaction vsize, fee)."""

Also applies to: 64-64

🧹 Nitpick comments (5)
test/functional/feature_fee_estimation.py (1)

151-151: Consider renaming tx_bytes to tx_vsize for clarity.

The variable tx_bytes receives the virtual size (a numeric value) from small_txpuzzle_randfee, not raw bytes. The name tx_bytes is misleading and could confuse readers. Consider renaming it to tx_vsize or tx_size to better reflect its actual content.

Apply this diff to improve variable naming:

-                (tx_bytes, fee) = small_txpuzzle_randfee(
+                (tx_vsize, fee) = small_txpuzzle_randfee(
                     self.wallet,
                     self.nodes[from_index],
                     self.confutxo,
                     self.memutxo,
                     Decimal("0.005"),
                     min_fee,
                     min_fee,
                 )
-                tx_kbytes = tx_bytes / 1000.0
+                tx_kbytes = tx_vsize / 1000.0

Also applies to: 160-160

test/functional/wallet_importmulti.py (1)

711-717: Good negative-path coverage; silence the linter for test-only passphrase.

The locked-wallet failure path is correct and asserts the expected RPC_WALLET_UNLOCK_NEEDED (-13). Add a one-line noqa to avoid Ruff S106 on the hardcoded test passphrase.

-        self.nodes[1].createwallet(wallet_name='w1', blank=True, passphrase='pass')
+        self.nodes[1].createwallet(wallet_name='w1', blank=True, passphrase='pass')  # noqa: S106 - test-only passphrase
src/wallet/rpc/backup.cpp (1)

1586-1596: Conditional unlock is correct; guard against non-boolean watchonly to keep per-request error semantics.

As written, request["watchonly"].get_bool() will throw if a caller passes a non-bool (e.g., "true"), aborting the whole RPC before per-item error handling. Consider a defensive check that treats non-bool or missing values as not watchonly, so malformed entries fail per-request inside ProcessImport*.

-        // Check all requests are watchonly
-        bool is_watchonly{true};
+        // Check all requests are explicitly watchonly
+        bool is_watchonly{true};
         for (size_t i = 0; i < requests.size(); ++i) {
             const UniValue& request = requests[i];
-            if (!request.exists("watchonly") || !request["watchonly"].get_bool()) {
+            if (!request.exists("watchonly")) {
+                is_watchonly = false;
+                break;
+            }
+            const UniValue& wo = request["watchonly"];
+            if (!(wo.isBool() && wo.get_bool())) {
                 is_watchonly = false;
                 break;
             }
         }
         // Wallet does not need to be unlocked if all requests are watchonly
         if (!is_watchonly) EnsureWalletIsUnlocked(wallet);

Based on learnings

contrib/verify-commits/gpg.sh (1)

1-1: Optional: add strict mode for robustness.

Consider adding set -eu after the shebang (portable in POSIX sh) for early exit on errors/unset vars.

 #!/bin/sh
+set -eu
contrib/verify-commits/verify-commits.py (1)

182-194: merge-tree --write-tree guard and error handling LGTM; optional exception rethrow tweak.

Git version check and error message are good. Optionally prefer raise (bare) to preserve original traceback.

-            except subprocess.CalledProcessError as e:
+            except subprocess.CalledProcessError as e:
                 if e.returncode == 128:
                     print("git v2.38+ is required for this functionality.", file=sys.stderr)
                     sys.exit(1)
                 else:
-                    raise e
+                    raise
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b82450a and 8ba79e5.

📒 Files selected for processing (14)
  • contrib/verify-commits/README.md (1 hunks)
  • contrib/verify-commits/gpg.sh (2 hunks)
  • contrib/verify-commits/verify-commits.py (3 hunks)
  • doc/bips.md (1 hunks)
  • doc/developer-notes.md (1 hunks)
  • src/net_processing.cpp (1 hunks)
  • src/rpc/blockchain.cpp (1 hunks)
  • src/support/lockedpool.cpp (0 hunks)
  • src/test/miner_tests.cpp (0 hunks)
  • src/wallet/rpc/backup.cpp (1 hunks)
  • test/functional/feature_fee_estimation.py (3 hunks)
  • test/functional/feature_maxuploadtarget.py (1 hunks)
  • test/functional/wallet_avoidreuse.py (1 hunks)
  • test/functional/wallet_importmulti.py (1 hunks)
💤 Files with no reviewable changes (2)
  • src/support/lockedpool.cpp
  • src/test/miner_tests.cpp
🧰 Additional context used
📓 Path-based instructions (4)
doc/**

📄 CodeRabbit inference engine (CLAUDE.md)

Unless specifically prompted, avoid making changes to the doc directory (documentation)

Files:

  • doc/bips.md
  • doc/developer-notes.md
src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

  • src/wallet/rpc/backup.cpp
  • src/rpc/blockchain.cpp
  • src/net_processing.cpp
test/functional/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

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

Files:

  • test/functional/wallet_avoidreuse.py
  • test/functional/wallet_importmulti.py
  • test/functional/feature_maxuploadtarget.py
  • test/functional/feature_fee_estimation.py
contrib/**

📄 CodeRabbit inference engine (CLAUDE.md)

Unless specifically prompted, avoid making changes to the contrib directory (contributed scripts)

Files:

  • contrib/verify-commits/README.md
  • contrib/verify-commits/gpg.sh
  • contrib/verify-commits/verify-commits.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: knst
PR: dashpay/dash#6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
🧬 Code graph analysis (4)
src/wallet/rpc/backup.cpp (2)
src/wallet/rpc/util.cpp (2)
  • EnsureWalletIsUnlocked (79-84)
  • EnsureWalletIsUnlocked (79-79)
src/wallet/rpc/util.h (1)
  • wallet (18-45)
test/functional/wallet_avoidreuse.py (2)
test/functional/test_framework/util.py (1)
  • assert_raises_rpc_error (132-148)
src/wallet/rpc/wallet.cpp (2)
  • setwalletflag (545-611)
  • setwalletflag (545-545)
test/functional/wallet_importmulti.py (3)
test/functional/test_framework/test_node.py (1)
  • get_wallet_rpc (363-369)
test/functional/test_framework/util.py (1)
  • assert_raises_rpc_error (132-148)
src/wallet/rpc/backup.cpp (4)
  • importmulti (1472-1673)
  • importmulti (1472-1472)
  • result (1385-1385)
  • result (1678-1678)
test/functional/feature_maxuploadtarget.py (1)
test/functional/test_framework/test_node.py (1)
  • assert_start_raises_init_error (606-649)
🪛 LanguageTool
doc/developer-notes.md

[grammar] ~873-~873: Use a hyphen to join words.
Context: ...sky locale issues. - Avoid using locale dependent functions if possible. You can...

(QB_NEW_EN_HYPHEN)

🪛 Ruff (0.14.1)
test/functional/wallet_importmulti.py

713-713: Possible hardcoded password assigned to argument: "passphrase"

(S106)

contrib/verify-commits/verify-commits.py

119-119: subprocess call: check for execution of untrusted input

(S603)


131-131: subprocess call: check for execution of untrusted input

(S603)


143-143: subprocess call: check for execution of untrusted input

(S603)


187-187: subprocess call: check for execution of untrusted input

(S603)


193-193: Use raise without specifying exception name

Remove exception name

(TRY201)


197-197: subprocess call: check for execution of untrusted input

(S603)

🔇 Additional comments (14)
doc/bips.md (1)

32-32: Formatting and content are consistent with existing BIP entries.

The BIP 86 entry follows the established pattern used throughout the file, with proper markdown formatting, version number, and Bitcoin Core PR link. The description accurately reflects descriptor wallet support via BIP 86 since v23.0.

test/functional/wallet_avoidreuse.py (1)

118-119: LGTM! Good test coverage addition.

The test correctly validates that setting an unknown wallet flag raises the expected RPC error (-8) with the appropriate message. This improves edge-case coverage and is consistent with the implementation in src/wallet/rpc/wallet.cpp.

src/net_processing.cpp (1)

1936-1938: LGTM: clarifies expected stall/timeout and BLOCKTXN drop behavior.

Accurate, non-functional, and matches current logic in BlockRequested()/RemoveBlockRequest() and stalling/timeouts. Keeping upstream wording is good for future backports.

Based on learnings

src/rpc/blockchain.cpp (1)

486-488: Help text improvements approved.

The three new sentences clarify peer behavior for the getblockfrompeer RPC:

  • Explain that subsequent calls override previous peer requests
  • Document stale block filtering by peers (age ≥ ~1 month)
  • Clarify disconnect behavior on non-response

These additions are appropriate and improve API documentation for users.

test/functional/feature_maxuploadtarget.py (1)

174-176: LGTM! Good test coverage for invalid command-line arguments.

This test case correctly validates that the node rejects unparsable values for -maxuploadtarget with an appropriate error message. The implementation properly stops the node before attempting to restart it with invalid arguments, and the error message format is consistent with Bitcoin Core's argument parsing.

test/functional/wallet_importmulti.py (1)

721-729: LGTM: watchonly import on locked wallet succeeds.

This validates the new conditional-unlock behavior without introducing non-upstream logic. Nice addition.

contrib/verify-commits/verify-commits.py (5)

117-123: Early exit on commits outside trusted-root ancestry — confirm intended CI semantics.

Exiting with code 0 on “predates the trusted root” avoids false failures when history diverges. Confirm this is desired (e.g., for shallow or topic branches), and that CI treats it as success only when the path from the initial commit to trusted root is fully validated.


124-136: Tree-SHA512 gating logic LGTM.

Cleanly disabling tree checks once hitting or predating the trusted SHA512 root matches the intent.


197-199: Helpful diff on unclean merge — good addition.

Emitting git diff between recreated and current trees provides actionable context.


95-97: Trusted keys file verified and valid.

✓ File exists and contains proper primary key fingerprints (40-hex format, one per line). Code looks good.


139-151: Review comment is incorrect; code has parsing bug for primary key signatures.

The review claims that extracting the last VALIDSIG field correctly matches primary key fingerprints, but this is false. GnuPG VALIDSIG format includes an optional 10th field (primary-key fingerprint) only when a subkey made the signature. When a primary key signs directly, only 9 fields are present, making split(" ")[-1] extract field 9 (a numeric signature class), not the fingerprint.

The README states trusted-keys should contain "PGP fingerprints of authorized commit signers (primary, not subkeys)", meaning the code should validate primary key signatures—yet the current parsing only works for subkey signatures. This is a parsing bug that contradicts the intended design.

Likely an incorrect or invalid review comment.

doc/developer-notes.md (1)

869-875: Documentation path verified—no changes needed.

The linter path test/lint/lint-locale-dependence.py exists in the repository as documented. The changes are accurate.

contrib/verify-commits/README.md (1)

30-33: Git ≥ 2.38 requirement verified — documentation is accurate.

The verification confirms the current environment runs git 2.39.5, which satisfies the ≥ 2.38 requirement stated in lines 30-33. The documentation correctly reflects the actual dependency for merge-tree --write-tree functionality in verify-commits.py.

contrib/verify-commits/gpg.sh (1)

9-11: Simplified gpg path LGTM; confirm raw status lines remain visible to verify-commits.py.

We still redirect gpg stderr to /dev/null. Given git --raw typically surfaces [GNUPG:] status lines itself, this should be fine, but please verify the Python script still sees VALIDSIG lines.

Also applies to: 33-35

#include <iomanip>
#include <iostream>
#endif
#include <utility>
Copy link

Choose a reason for hiding this comment

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

27151: DNM, not a duplicate in our case

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