Merge bitcoin/bitcoin#27279: Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet #1129
Conversation
…s already there dfcef53 blockstorage: do not flush block to disk if it is already there (Matthew Zipkin) Pull request description: Closes bitcoin#2039 When reindexing from flat-file block storage there is no need to write anything back to disk, since the block data is already there. This PR skips flushing to disk those blocks that already have a known position in the datastore. Skipping this means that users can write-protect the `blk` files on disk which may be useful for security or even safely sharing that data between multiple bitcoind instances. `FindBlockPos()` may also flush the undo data file, but again this is skipped if the corresponding block position is known, like during the initial stage of a reindex when block data is being indexed. Once the block index is complete the validation mechanism will call `ConnectBlock()` which will save undo data at that time. The call stack looks like this: ``` init() ThreadImport() <-- process fReindex flag LoadExternalBlockFile() AcceptBlock() SaveBlockToDisk() FindBlockPos() FlushBlockFile() <-- unnecessary if block is already on disk ``` A larger refactor of this part of the code was started by mzumsande here: https://github.com/mzumsande/bitcoin/tree/202207_refactor_findblockpos including this fix, reviewers can let me know if the changes should be combined. ACKs for top commit: sipa: utACK dfcef53 mzumsande: re-ACK dfcef53 achow101: ACK dfcef53 furszy: Rebase diff ACK dfcef53. Tree-SHA512: 385c5ac1288b325135398d0ddd3ab788fa98cc0ca19bd2474c74039f2ce70d5088c1d1c9d4dd10aefcbd4c757767ec5805d07ba8cee9289a66f96e6f9eaa5279
…oad,unload,restore}wallet 7ccdd74 test: fix importmulti/importdescriptors assertion (Jon Atack) 19d888c rpc: move WALLET_FLAG_CAVEATS to the compilation unit of its caller (Jon Atack) 01df011 doc: release note for wallet RPCs "warning" field deprecation (Jon Atack) 9ea8b37 test: createwallet "warning" field deprecation test (Jon Atack) 645d7f7 rpc: deprecate "warning" field in {create,load,unload,restore}wallet (Jon Atack) 2f4a926 test: add test coverage for "warnings" field in createwallet (Jon Atack) 4a1e479 rpc: add "warnings" field to RPCs {create,load,unload,restore}wallet (Jon Atack) 079d8cd rpc: extract wallet "warnings" fields to a util helper (Jon Atack) f73782a doc: fix/improve warning helps in {create,load,unload,restore}wallet (Jon Atack) Pull request description: Based on discussion and concept ACKed in bitcoin#27138, add a `warnings` field to RPCs createwallet, loadwallet, unloadwallet, and restorewallet as a JSON array of strings to replace the `warning` string field in these 4 RPCs. The idea is to more gracefully handle multiple warning messages and for consistency with other wallet RPCs. Then, deprecate the latter fields, which represent all the remaining RPC `warning` fields. The first commit bitcoin@f73782a implements bitcoin#27138 (comment) as an alternative to bitcoin#27138. One of those two could potentially be backported to our currently supported releases. ACKs for top commit: achow101: ACK 7ccdd74 1440000bytes: utACK bitcoin@7ccdd74 vasild: ACK 7ccdd74 pinheadmz: re-ACK 7ccdd74 Tree-SHA512: 314e0a4c41fa383d95e2817bfacf359d449e460529d235c3eb902851e2f4eacbabe646d9a5a4beabc4964cdfabf6397ed8301366a58d344a2f787f83b75e9d64
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on October 7. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
WalkthroughIntroduces a new RPC warnings array and deprecates the legacy single warning field across wallet RPCs. Adds PushWarnings helpers, updates wallet RPC implementations and tests accordingly, adjusts block file flushing to reference a specific file index, removes a wallet flag caveats symbol, and adds a reindex-with-readonly-blockfile functional test. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Ruff (0.13.1)test/functional/feature_reindex_readonly.py�[1;31mruff failed�[0m test/functional/wallet_createwallet.py�[1;31mruff failed�[0m 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: 3
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_reindex_readonly.py (1)
1-91: VALIDATION CONCERN: File appears unrelated to PR bitcoin#27279 scope.This test file (
feature_reindex_readonly.py) tests reindexing behavior with read-only block files, which is unrelated to the stated PR objective of adding a "warnings" array field and deprecating the "warning" string field in wallet RPCs.Bitcoin Core PR bitcoin#27279 focuses on wallet RPC warning handling changes. This reindex test appears to be from a different Bitcoin commit or PR mixed into this backport batch.
Please verify:
- Is this file part of the original Bitcoin PR bitcoin#27279, or was it accidentally included from a different PR in the backport batch?
- If this is intentional (part of batch-412), the PR description should clarify which Bitcoin commit this test originates from.
Based on learnings about scope creep detection in Dash backports.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
doc/release-notes-27279.md(1 hunks)src/node/blockstorage.cpp(1 hunks)src/rpc/util.cpp(1 hunks)src/rpc/util.h(1 hunks)src/wallet/rpc/backup.cpp(4 hunks)src/wallet/rpc/wallet.cpp(8 hunks)src/wallet/wallet.cpp(0 hunks)src/wallet/wallet.h(0 hunks)test/functional/feature_reindex_readonly.py(1 hunks)test/functional/wallet_createwallet.py(4 hunks)
💤 Files with no reviewable changes (2)
- src/wallet/wallet.cpp
- src/wallet/wallet.h
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
C++20 codebase should be placed under src/
Files:
src/rpc/util.hsrc/wallet/rpc/backup.cppsrc/rpc/util.cppsrc/node/blockstorage.cppsrc/wallet/rpc/wallet.cpp
**
⚙️ CodeRabbit configuration file
**: # CodeRabbit AI Review Instructions for Dash BackportsYour 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
bitcoin→dashin strings/pathsBitcoin→Dashin 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
- First: Check file operations match exactly
- Second: Calculate size ratio
- Third: Scan for scope creep patterns
- Fourth: Detect Bitcoin-specific code
- Fifth: Verify all changes are minimal adaptations
Output Format
VALIDATION: [PASS/FAIL] File Operatio...
Files:
src/rpc/util.hsrc/wallet/rpc/backup.cppdoc/release-notes-27279.mdtest/functional/wallet_createwallet.pysrc/rpc/util.cppsrc/node/blockstorage.cppsrc/wallet/rpc/wallet.cpptest/functional/feature_reindex_readonly.py
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/release-notes-27279.md
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be placed in test/functional/ and written in Python
Files:
test/functional/wallet_createwallet.pytest/functional/feature_reindex_readonly.py
🧠 Learnings (1)
📓 Common learnings
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-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.
🧬 Code graph analysis (6)
src/rpc/util.h (1)
src/rpc/util.cpp (6)
PushWarnings(1054-1058)PushWarnings(1054-1054)PushWarnings(1060-1064)PushWarnings(1060-1060)obj(306-306)obj(312-312)
src/wallet/rpc/backup.cpp (2)
src/rpc/util.cpp (6)
PushWarnings(1054-1058)PushWarnings(1054-1054)PushWarnings(1060-1064)PushWarnings(1060-1060)obj(306-306)obj(312-312)src/util/translation.h (1)
Untranslated(48-48)
test/functional/wallet_createwallet.py (2)
test/functional/test_framework/test_framework.py (2)
BitcoinTestFramework(101-1137)is_bdb_compiled(1135-1137)src/wallet/rpc/wallet.cpp (2)
createwallet(618-715)createwallet(618-618)
src/rpc/util.cpp (1)
src/wallet/rpc/backup.cpp (4)
warnings(1162-1162)warnings(1297-1297)warnings(1383-1383)warnings(1665-1665)
src/wallet/rpc/wallet.cpp (2)
src/rpc/util.cpp (8)
HelpExampleCli(204-207)HelpExampleCli(204-204)HelpExampleRpc(222-227)HelpExampleRpc(222-222)PushWarnings(1054-1058)PushWarnings(1054-1054)PushWarnings(1060-1064)PushWarnings(1060-1060)src/util/translation.h (1)
Untranslated(48-48)
test/functional/feature_reindex_readonly.py (2)
test/functional/test_framework/test_framework.py (1)
start_node(646-655)test/functional/test_framework/test_node.py (2)
chain_path(429-430)wait_for_debug_log(473-506)
🪛 Flake8 (7.2.0)
test/functional/feature_reindex_readonly.py
[error] 37-37: do not assign a lambda expression, use a def
(E731)
[error] 43-43: do not assign a lambda expression, use a def
(E731)
[error] 62-62: do not assign a lambda expression, use a def
(E731)
🪛 markdownlint-cli2 (0.18.1)
doc/release-notes-27279.md
1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (15)
test/functional/wallet_createwallet.py (6)
18-20: LGTM! Constants defined for warning message validation.These constants centralize the expected warning messages for easier maintenance and consistent validation across tests.
25-25: LGTM! Deprecated RPC flag enables backward compatibility testing.Adding
-deprecatedrpc=walletwarningfieldallows the test to verify both the deprecated "warning" string field and the new "warnings" array field are present during the transition period.
60-60: LGTM! Updated to check new "warnings" array field.Correctly validates that the "warnings" array field is not present in error responses when private keys are disabled, consistent with the new API structure.
165-167: LGTM! Validates both deprecated and new warning formats.The test correctly verifies:
- The deprecated "warning" string field (with newline-delimited messages)
- The new "warnings" array field (with separate array elements)
This ensures backward compatibility during the deprecation transition.
180-193: LGTM! Legacy wallet deprecation test properly guarded.The test block correctly:
- Guards legacy wallet testing with
is_bdb_compiled()check- Tests legacy wallet creation warning messages
- Validates both single and combined warning scenarios
- Verifies both "warning" (deprecated) and "warnings" (array) formats
195-199: LGTM! Verifies deprecated field removal.Correctly tests that the deprecated "warning" field is not present after restarting the node without the
-deprecatedrpc=walletwarningfieldflag, ensuring the deprecation mechanism works as intended.src/wallet/rpc/wallet.cpp (9)
27-27: LGTM! Required include added.The
wallet/walletutil.hinclude is necessary for wallet utility functions used in the warning handling implementation.
39-44: LGTM! Wallet flag caveats map introduced.This static map provides warning messages for specific wallet flags, currently containing the AVOID_REUSE flag caveat about rescanning. This centralizes warning logic and supports the new warnings array field.
500-521: LGTM! loadwallet help text updated with warnings fields.The help text correctly documents:
- The deprecated "warning" string field (with deprecation note)
- The new "warnings" array field
- The
-deprecatedrpc=walletwarningfieldconfiguration requirementHelp examples also updated appropriately. Note: "dashd" is correctly used instead of "bitcoind".
540-543: LGTM! loadwallet implementation adds conditional warnings.The implementation correctly:
- Checks
rpcEnableDeprecated("walletwarningfield")before adding deprecated "warning" field- Uses
Join()with newline delimiter for backward compatibility- Calls
PushWarnings()to add the new "warnings" array field
609-611: LGTM! setwalletflag uses new caveats map.Correctly references
WALLET_FLAG_CAVEATSto retrieve warning messages for wallet flags, maintaining consistency with the new warning infrastructure.
636-640: LGTM! createwallet help text updated with warnings fields.Documentation correctly describes both the deprecated "warning" string field and new "warnings" array field, consistent with the pattern established in loadwallet.
707-710: LGTM! createwallet implementation adds conditional warnings.Follows the same correct pattern as loadwallet:
- Conditional deprecated "warning" field based on
rpcEnableDeprecated("walletwarningfield")- Newline-delimited warning string for backward compatibility
PushWarnings()call for new "warnings" array
720-736: LGTM! unloadwallet help updated with object result and warnings.Help text correctly updated to:
- Improve parameter descriptions
- Change result type from null to object
- Document both deprecated "warning" and new "warnings" fields
- Update examples
769-776: LGTM! unloadwallet returns object with warnings.Implementation correctly:
- Creates UniValue object result instead of returning null
- Adds conditional deprecated "warning" field
- Calls
PushWarnings()for new "warnings" array- Returns the result object after unloading wallet
| deprecation period by launching bitcoind with the configuration option | ||
| `-deprecatedrpc=walletwarningfield`. (#27279) |
There was a problem hiding this comment.
Replace upstream daemon name with Dash variant.
Line 9 still tells users to launch “bitcoind”, but in Dash this should read “dashd”. Please adjust the release note so Dash operators follow the correct instructions.
🤖 Prompt for AI Agents
In doc/release-notes-27279.md around lines 9 to 10, the release note references
the upstream daemon name "bitcoind" but Dash operators should be instructed to
run "dashd"; update the wording to replace "bitcoind" with "dashd" (e.g.,
"launch dashd with the configuration option
`-deprecatedrpc=walletwarningfield`") so the correct daemon name appears in the
release note.
| if (!FlushBlockFile(m_last_blockfile, !fKnown, finalize_undo)) { | ||
| LogPrintLevel(BCLog::BLOCKSTORE, BCLog::Level::Warning, | ||
| "Failed to flush previous block file %05i (finalize=%i, finalize_undo=%i) before opening new block file %05i\n", | ||
| m_last_blockfile, !fKnown, finalize_undo, nFile); | ||
| } |
There was a problem hiding this comment.
FlushBlockFile call uses a non-existent signature.
Line 652 invokes FlushBlockFile(m_last_blockfile, !fKnown, finalize_undo) and checks its return value, but in this tree BlockManager::FlushBlockFile is still declared as void FlushBlockFile(bool fFinalize, bool finalize_undo) (see Line 557). This won’t compile—the extra m_last_blockfile argument doesn’t match any overload, and there’s no boolean return to test. Please port the corresponding signature/body change from the upstream commit (accepting the file index and returning a status) or revert the call form so the build succeeds.
🤖 Prompt for AI Agents
In src/node/blockstorage.cpp around lines 652-656, the call
FlushBlockFile(m_last_blockfile, !fKnown, finalize_undo) does not match the
current BlockManager::FlushBlockFile(bool fFinalize, bool finalize_undo)
signature and incorrectly checks a return value; fix by porting the upstream
signature change or reverting the call: either (A) update
BlockManager::FlushBlockFile declaration and definition to accept the file index
(e.g. int file) and return a status/boolean (update all callers to handle the
return), then call FlushBlockFile(m_last_blockfile, !fKnown, finalize_undo) and
check the returned status; or (B) change this site to call the existing API
(FlushBlockFile(!fKnown, finalize_undo)) and remove the boolean return check,
leaving logging based on void behavior — apply the corresponding header/source
edits and update any other callers to match the chosen API.
| filename = self.nodes[0].chain_path / "blocks" / "blk00000.dat" | ||
| filename.chmod(stat.S_IREAD) | ||
|
|
||
| undo_immutable = lambda: None |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Replace lambda assignments with def statements.
Flake8 correctly flags these lambda assignments as violating PEP 8 style (E731). Use named functions instead for better readability and debuggability.
Apply these changes:
- undo_immutable = lambda: None
+ def undo_immutable():
+ pass- subprocess.run(['chattr', '+i', filename], capture_output=True, check=True)
- undo_immutable = lambda: subprocess.check_call(['chattr', '-i', filename])
+ def undo_immutable_chattr():
+ subprocess.check_call(['chattr', '-i', filename])
+ subprocess.run(['chattr', '+i', filename], capture_output=True, check=True)
+ undo_immutable = undo_immutable_chattr- subprocess.run(['chflags', 'uchg', filename], capture_output=True, check=True)
- undo_immutable = lambda: subprocess.check_call(['chflags', 'nouchg', filename])
+ def undo_immutable_chflags():
+ subprocess.check_call(['chflags', 'nouchg', filename])
+ subprocess.run(['chflags', 'uchg', filename], capture_output=True, check=True)
+ undo_immutable = undo_immutable_chflagsAs per static analysis hints.
Also applies to: 43-43, 62-62
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 37-37: do not assign a lambda expression, use a def
(E731)
🤖 Prompt for AI Agents
In test/functional/feature_reindex_readonly.py around lines 37, 43 and 62,
replace the inline lambda assignments (e.g. undo_immutable = lambda: None) with
properly named def functions to satisfy PEP8 E731 and improve
readability/debuggability; create simple def undo_immutable(): return None (and
similar named defs for the other two lambda assignments), then assign or use
those function names where the lambdas were used.
Summary
Backport of Bitcoin Core PR bitcoin#27279 (commit 6a16732).
This PR adds a new
warningsfield (as a JSON array of strings) to wallet RPCscreatewallet,loadwallet,unloadwallet, andrestorewallet, while deprecating the existingwarningstring field. This improves handling of multiple warning messages and provides consistency with other wallet RPCs.Key Changes:
PushWarnings()helper functions insrc/rpc/util.cppandsrc/rpc/util.hto standardize warning outputwarning(deprecated) andwarnings(new array format) fields-deprecatedrpc=walletwarningfieldflag to control whether the deprecatedwarningfield is returnedDash-Specific Adaptations:
wallet_backwards_compatibility.pytest which doesn't exist in DashOriginal Bitcoin PR:
bitcoin#27279
Bitcoin commit: 6a16732
Test plan
test/functional/wallet_createwallet.pywarningsarray fieldwarningfield when-deprecatedrpc=walletwarningfieldis setGenerated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests