Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
d0wn3d
changed the title
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughChanges touch three areas: the Makefile.qt.include generation of qt/dashstrings.cpp was refactored to use a multi-line extract_strings_qt.py invocation and additional Qt source/header variables were added to the inputs and translate prerequisites; the createwallet RPC help text and parameter handling were updated so descriptors are parsed explicitly and the legacy (BDB) creation path requires rpcEnableDeprecated("create_bdb"), returning RPC_WALLET_ERROR if not enabled; the functional test write_config() now appends deprecatedrpc=create_bdb to generated dash.conf. Sequence Diagram(s)sequenceDiagram
participant Client
participant RPC as createwallet RPC
participant Chain as ChainContext
participant Wallet as WalletCreator
Client->>RPC: createwallet(..., descriptors)
RPC->>RPC: parse params (explicit bool)
alt descriptors == true
RPC->>Wallet: set WALLET_FLAG_DESCRIPTORS / create descriptors wallet
Wallet-->>RPC: success
RPC-->>Client: wallet-created response
else descriptors == false
RPC->>Chain: rpcEnableDeprecated("create_bdb")?
alt deprecated enabled
Chain-->>RPC: enabled
RPC->>Wallet: create legacy (BDB) wallet
Wallet-->>RPC: success
RPC-->>Client: wallet-created response
else deprecated not enabled
Chain-->>RPC: not-enabled
RPC-->>Client: RPC_WALLET_ERROR (deprecated disabled)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Makefile.qt.include`:
- Line 499: The translate target lists $(QT_FORMS_UI) twice in its prerequisites
(translate: $(srcdir)/qt/dashstrings.cpp $(QT_FORMS_UI) $(QT_FORMS_UI) ...);
remove the duplicate $(QT_FORMS_UI) so the prerequisites include it only once,
keeping the rest of the listed symbols (e.g., $(srcdir)/qt/dashstrings.cpp,
$(BITCOIN_QT_BASE_CPP), $(BITCOIN_QT_WINDOWS_CPP), $(BITCOIN_QT_WALLET_CPP),
$(BITCOIN_QT_H), $(BITCOIN_MM)) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5cca096e-6b80-49cc-8235-0eacd805fb0a
📒 Files selected for processing (1)
src/Makefile.qt.include
d0wn3d
changed the title
d0wn3d
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/wallet/rpc/wallet.cpp (1)
637-639: Tighten thedescriptorshelp text phrasing.Line 638 has awkward punctuation/capitalization (
"; This"and"because,"), which hurts readability in RPC help output.✍️ Proposed wording cleanup
- {"descriptors", RPCArg::Type::BOOL, RPCArg::Default{true}, "Create a native descriptor wallet. The wallet will use descriptors internally to handle address creation." - " Setting to \"false\" will create a legacy wallet; This is only possible with the -deprecatedrpc=create_bdb setting because, the legacy wallet type is being deprecated and" - " support for creating and opening legacy wallets will be removed in the future."}, + {"descriptors", RPCArg::Type::BOOL, RPCArg::Default{true}, "Create a native descriptor wallet. The wallet will use descriptors internally to handle address creation." + " Setting this to \"false\" creates a legacy wallet. This is only possible with -deprecatedrpc=create_bdb, because legacy wallet creation is deprecated and" + " support for creating and opening legacy wallets will be removed in a future release."},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/rpc/wallet.cpp` around lines 637 - 639, The help text for the RPCArg named "descriptors" in wallet.cpp contains awkward punctuation/capitalization ("; This" and "because,") — update the string passed to the RPCArg constructor (the descriptor help message where RPCArg::Type::BOOL and RPCArg::Default{true} are used) to use clearer phrasing and correct punctuation/capitalization (e.g., replace the semicolon+capitalized "This" with a period or comma and remove the stray comma after "because"), ensuring the message reads smoothly and maintains the same information about creating native descriptor vs legacy wallets and the -deprecatedrpc=create_bdb caveat.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/functional/test_framework/util.py`:
- Line 422: Remove the global "deprecatedrpc=create_bdb" write in
test/functional/test_framework/util.py so functional nodes don't enable the
legacy create_bdb RPC by default; instead add a per-node opt-in (e.g., extend
the node startup helper such as start_node/start_nodes or the node's
extra_args/bitcoin_conf construction to accept a deprecatedrpc flag) and update
tests that require the legacy behavior (like
test/functional/wallet_descriptor.py) to pass that flag or set
"deprecatedrpc=create_bdb" only for the specific node datadir they control
during setup.
---
Nitpick comments:
In `@src/wallet/rpc/wallet.cpp`:
- Around line 637-639: The help text for the RPCArg named "descriptors" in
wallet.cpp contains awkward punctuation/capitalization ("; This" and "because,")
— update the string passed to the RPCArg constructor (the descriptor help
message where RPCArg::Type::BOOL and RPCArg::Default{true} are used) to use
clearer phrasing and correct punctuation/capitalization (e.g., replace the
semicolon+capitalized "This" with a period or comma and remove the stray comma
after "because"), ensuring the message reads smoothly and maintains the same
information about creating native descriptor vs legacy wallets and the
-deprecatedrpc=create_bdb caveat.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aac9c63f-ac2a-4867-9a07-ea72122793f3
📒 Files selected for processing (2)
src/wallet/rpc/wallet.cpptest/functional/test_framework/util.py
| f.write("upnp=0\n") | ||
| f.write("natpmp=0\n") | ||
| f.write("shrinkdebugfile=0\n") | ||
| f.write("deprecatedrpc=create_bdb\n") # Required to run the tests |
There was a problem hiding this comment.
Avoid enabling create_bdb deprecated RPC globally in framework config.
Line 422 makes every functional node allow deprecated legacy wallet creation, which can hide failures in tests that should validate the default rejection path (e.g., test/functional/wallet_descriptor.py legacy createwallet flow).
🧪 Suggested fix (make deprecated RPC opt-in per test)
-def write_config(config_path, *, n, chain, extra_config="", disable_autoconnect=True):
+def write_config(config_path, *, n, chain, extra_config="", disable_autoconnect=True, enable_create_bdb_deprecatedrpc=False):
@@
- f.write("deprecatedrpc=create_bdb\n") # Required to run the tests
+ if enable_create_bdb_deprecatedrpc:
+ f.write("deprecatedrpc=create_bdb\n")Then enable it only in tests that intentionally exercise legacy wallet creation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| f.write("deprecatedrpc=create_bdb\n") # Required to run the tests | |
| def write_config(config_path, *, n, chain, extra_config="", disable_autoconnect=True, enable_create_bdb_deprecatedrpc=False): | |
| # ... other code ... | |
| if enable_create_bdb_deprecatedrpc: | |
| f.write("deprecatedrpc=create_bdb\n") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/functional/test_framework/util.py` at line 422, Remove the global
"deprecatedrpc=create_bdb" write in test/functional/test_framework/util.py so
functional nodes don't enable the legacy create_bdb RPC by default; instead add
a per-node opt-in (e.g., extend the node startup helper such as
start_node/start_nodes or the node's extra_args/bitcoin_conf construction to
accept a deprecatedrpc flag) and update tests that require the legacy behavior
(like test/functional/wallet_descriptor.py) to pass that flag or set
"deprecatedrpc=create_bdb" only for the specific node datadir they control
during setup.
f316a36 to
b7f36d8
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7f36d8a71
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/wallet/rpc/wallet.cpp`:
- Around line 684-693: The code calls request.params[5].get_bool() unguarded
which breaks the documented default of descriptors=true when the argument is
omitted; change the logic around request.params[5] so a null param preserves the
default (treat null as true). Specifically, update the conditional that sets
WALLET_FLAG_DESCRIPTORS (and the else branch that checks
rpcEnableDeprecated("create_bdb")) to use: if (request.params[5].isNull() ||
request.params[5].get_bool()) { flags |= WALLET_FLAG_DESCRIPTORS; } else { /*
existing BDB deprecation check/throw */ } so the omitted case keeps descriptors
enabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 49934e17-fc1d-4b12-a41b-25b5bd67c41d
📒 Files selected for processing (2)
src/wallet/rpc/wallet.cpptest/functional/test_framework/util.py
🚧 Files skipped from review as they are similar to previous changes (1)
- test/functional/test_framework/util.py
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean backport of bitcoin#22764 and bitcoin#28597. The Makefile change correctly adds Qt sources for string extraction. The wallet change correctly gates BDB wallet creation behind -deprecatedrpc=create_bdb, using request.params[5] instead of upstream's self.Arg(5) since Dash's RPCHelpMan lacks the Arg() method — consistent with all surrounding param access in this function. The test framework change to enable create_bdb in test configs is necessary and correct.
Reviewed commit: 23ac20e
…reate_bdb fa071ae wallet: No BDB creation, unless -deprecatedrpc=create_bdb (MarcoFalke) Bitcoin Pull request description: With BDB being removed soon, it seems confusing and harmful to allow users to create fresh BDB wallets going forward, as it would load them with an additional burden of having to migrate them soon after. Also, it would be good to allow for one release for test (and external) scripts to adapt. Fix all issues by introducing the `-deprecatedrpc=create_bdb` setting.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bcfb6753f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| throw JSONRPCError(RPC_INVALID_PARAMETER, "The createwallet RPC requires specifying the 'load_on_startup' flag when param 'descriptors' is specified. Dash Core v21 introduced this requirement due to breaking changes in the createwallet RPC."); | ||
| } | ||
| if (request.params[5].isNull() || request.params[5].get_bool()) { | ||
| if (self.Arg<bool>("descriptors")) |
There was a problem hiding this comment.
This line calls self.Arg<bool>("descriptors"), but Dash's RPCHelpMan interface (see src/rpc/util.h) does not define an Arg accessor, so wallet-enabled builds fail when compiling wallet.cpp. In practice this blocks the backport from building at all unless this check is rewritten using request.params[5] (with the null/default-true behavior).
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Both agents independently identified the same blocking build break: self.Arg<bool>("descriptors") at src/wallet/rpc/wallet.cpp:684 calls a template method that does not exist on Dash's RPCHelpMan class. The previous review on SHA 23ac20e had already approved the request.params[5] workaround, but this push regressed it back to the upstream Bitcoin Core API.
Reviewed commit: 6bcfb67
🔴 1 blocking
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/wallet/rpc/wallet.cpp`:
- [BLOCKING] line 684: Build break: `RPCHelpMan::Arg<T>()` does not exist in Dash
Dash's `RPCHelpMan` (src/rpc/util.h:367-391) has no `Arg<T>()` template method — it only has constructors, `HandleRequest`, `ToString`, `GetArgMap`, `IsValidNumArgs`, and `GetArgNames`. A repo-wide search confirms this is the **only** `self.Arg<` call site in the entire codebase.
This is a compile-time failure. The previous revision (23ac20e5) correctly worked around this by using `request.params[5]` directly, which was explicitly approved in the prior review. This push appears to have regressed that fix.
| throw JSONRPCError(RPC_INVALID_PARAMETER, "The createwallet RPC requires specifying the 'load_on_startup' flag when param 'descriptors' is specified. Dash Core v21 introduced this requirement due to breaking changes in the createwallet RPC."); | ||
| } | ||
| if (request.params[5].isNull() || request.params[5].get_bool()) { | ||
| if (self.Arg<bool>("descriptors")) |
There was a problem hiding this comment.
🔴 Blocking: Build break: RPCHelpMan::Arg<T>() does not exist in Dash
Dash's RPCHelpMan (src/rpc/util.h:367-391) has no Arg<T>() template method — it only has constructors, HandleRequest, ToString, GetArgMap, IsValidNumArgs, and GetArgNames. A repo-wide search confirms this is the only self.Arg< call site in the entire codebase.
This is a compile-time failure. The previous revision (23ac20e) correctly worked around this by using request.params[5] directly, which was explicitly approved in the prior review. This push appears to have regressed that fix.
💡 Suggested change
| if (self.Arg<bool>("descriptors")) | |
| if (request.params[5].isNull() || request.params[5].get_bool()) |
source: ['claude', 'codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/wallet/rpc/wallet.cpp`:
- [BLOCKING] line 684: Build break: `RPCHelpMan::Arg<T>()` does not exist in Dash
Dash's `RPCHelpMan` (src/rpc/util.h:367-391) has no `Arg<T>()` template method — it only has constructors, `HandleRequest`, `ToString`, `GetArgMap`, `IsValidNumArgs`, and `GetArgNames`. A repo-wide search confirms this is the **only** `self.Arg<` call site in the entire codebase.
This is a compile-time failure. The previous revision (23ac20e5) correctly worked around this by using `request.params[5]` directly, which was explicitly approved in the prior review. This push appears to have regressed that fix.
2 participants
Issue being fixed or feature implemented
Backports Bitcoin Merge
Merge bitcoin#22764: build: Include qt sources for parsing with extract_strings.py
Merge bitcoin#28597: wallet: No BDB creation, unless -deprecatedrpc=create_bdb
What was done?
bitcoin@b59b31a build: Drop redundant qt/bitcoin.cpp (Hennadii Stepanov)
bitcoin@d90ad5a build: Include qt sources for parsing with extract_strings.py (Hennadii Stepanov)
bitcoin@fa071ae wallet: No BDB creation, unless -deprecatedrpc=create_bdb (MarcoFalke)
How Has This Been Tested?
Not tested
Breaking Changes
Dash's
RPCHelpMandoesn't have theArg<T>()template method, so I reverted torequest.params[5].isNull() || request.params[5].get_bool()Checklist:
Go over all the following points, and put an
xin all the boxes that apply.