Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
d0wn3d
changed the title
WalkthroughUpdated three areas: Makefile.qt.include changes how 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
…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.
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
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.