backport: bitcoin#24226, #26116, #26243, #26443, #26517, #26714, #26738, #27058, #27220, #27461 #6922
Conversation
…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
…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
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.mdreferences Taproot functionality that doesn't exist in Dash Core, creating misleading documentation; (2) missing type validation insrc/wallet/rpc/backup.cppwhererequest["watchonly"].get_bool()is called without verifying the field is a boolean; (3) duplicate section indoc/developer-notes.md; (4) inconsistent indentation incontrib/verify-commits/gpg.sh; (5) the modernizedverify-commits.pychanges 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), andcontrib/verify-commits/verify-commits.py(verify git version compatibility in CI).
Additional Comments (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
| "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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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)). |
There was a problem hiding this 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?
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.There was a problem hiding this comment.
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); |
There was a problem hiding this 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.
| 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.
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR comprises a broad set of updates across documentation, core verification utilities, wallet functionality, and tests. Key changes include: simplification of GPG verification logic in Sequence DiagramsequenceDiagram
participant User
participant verify-commits.py
participant gpg.sh
participant Git
participant trusted_keys
User->>verify-commits.py: Verify commit range
verify-commits.py->>Git: Get commits to verify
loop For each commit
verify-commits.py->>Git: Verify commit predates trusted root
alt Commit too old
verify-commits.py->>User: Exit (predates trusted root)
else Commit within range
verify-commits.py->>gpg.sh: Verify GPG signature
gpg.sh->>Git: Run gpg with trust-model always
Git-->>gpg.sh: VALIDSIG/REVKEYSIG output
gpg.sh-->>verify-commits.py: Exit status + output
verify-commits.py->>trusted_keys: Check key in trusted list
alt Key trusted
alt Tree verification enabled
verify-commits.py->>Git: Run merge-tree --write-tree
Git-->>verify-commits.py: Merge result (Git 2.38+)
alt Clean merge
verify-commits.py->>User: Commit verified
else Merge conflict
verify-commits.py->>User: Show merge diff
end
else Tree verification disabled
verify-commits.py->>User: Signature verified only
end
else Key not trusted
verify-commits.py->>User: Invalid signature
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes Key areas requiring careful attention:
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (2)contrib/**📄 CodeRabbit inference engine (CLAUDE.md)
Files:
doc/**📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (1)📓 Common learnings🪛 LanguageTooldoc/developer-notes.md[grammar] ~873-~873: Use a hyphen to join words. (QB_NEW_EN_HYPHEN) 🪛 Ruff (0.14.2)contrib/verify-commits/verify-commits.py119-119: (S603) 131-131: (S603) 143-143: (S603) 187-187: (S603) 193-193: Use Remove exception name (TRY201) 197-197: (S603) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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 renamingtx_bytestotx_vsizefor clarity.The variable
tx_bytesreceives the virtual size (a numeric value) fromsmall_txpuzzle_randfee, not raw bytes. The nametx_bytesis misleading and could confuse readers. Consider renaming it totx_vsizeortx_sizeto 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.0Also 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 passphrasesrc/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 -euafter the shebang (portable in POSIX sh) for early exit on errors/unset vars.#!/bin/sh +set -eucontrib/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
📒 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.mddoc/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.cppsrc/rpc/blockchain.cppsrc/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.pytest/functional/wallet_importmulti.pytest/functional/feature_maxuploadtarget.pytest/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.mdcontrib/verify-commits/gpg.shcontrib/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
getblockfrompeerRPC:
- 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
-maxuploadtargetwith 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 diffbetween 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.pyexists 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-treefunctionality 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
…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
force-pushed
the
bp-v25-trivial-1
branch
from
8ba79e5 to
916aafb
Compare
knst
changed the title
8 participants
What was done?
Trivial backports from Bitcoin Core v25
How Has This Been Tested?
Run & unit functional tests
Breaking Changes
N/A
Checklist: