Skip to content

backport: #22764 #28597 #7238

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
d0wn3d wants to merge 2 commits into
base: develop
Choose a base branch
from
+16 −4
Open

backport: #22764 #28597 #7238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c352c19
Backport #22764: build: Include qt sources for parsing with extract_s…
d0wn3d Mar 20, 2026
6bcfb67
Merge #28597: wallet: No BDB creation, unless -deprecatedrpc=create_bdb
d0wn3d Mar 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/Makefile.qt.include
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,11 +489,14 @@ SECONDARY: $(QT_QM)

$(srcdir)/qt/dashstrings.cpp: FORCE
@test -n $(XGETTEXT) || echo "xgettext is required for updating translations"
$(AM_V_GEN) cd $(srcdir); XGETTEXT=$(XGETTEXT) COPYRIGHT_HOLDERS="$(COPYRIGHT_HOLDERS)" $(PYTHON) ../share/qt/extract_strings_qt.py $(libbitcoin_node_a_SOURCES) $(libbitcoin_wallet_a_SOURCES) $(libbitcoin_common_a_SOURCES) $(libbitcoin_zmq_a_SOURCES) $(libbitcoin_consensus_a_SOURCES) $(libbitcoin_util_a_SOURCES)
$(AM_V_GEN) cd $(srcdir); XGETTEXT=$(XGETTEXT) COPYRIGHT_HOLDERS="$(COPYRIGHT_HOLDERS)" $(PYTHON) ../share/qt/extract_strings_qt.py \
$(libbitcoin_node_a_SOURCES) $(libbitcoin_wallet_a_SOURCES) $(libbitcoin_common_a_SOURCES) \
$(libbitcoin_zmq_a_SOURCES) $(libbitcoin_consensus_a_SOURCES) $(libbitcoin_util_a_SOURCES) \
$(BITCOIN_QT_BASE_CPP) $(BITCOIN_QT_WINDOWS_CPP) $(BITCOIN_QT_WALLET_CPP) $(BITCOIN_QT_H) $(BITCOIN_MM)

# The resulted dash_en.xlf source file should follow Transifex requirements.
# See: https://docs.transifex.com/formats/xliff#how-to-distinguish-between-a-source-file-and-a-translation-file
translate: $(srcdir)/qt/dashstrings.cpp $(QT_FORMS_UI) $(QT_FORMS_UI) $(BITCOIN_QT_BASE_CPP) qt/bitcoin.cpp $(BITCOIN_QT_WINDOWS_CPP) $(BITCOIN_QT_WALLET_CPP) $(BITCOIN_QT_H) $(BITCOIN_MM)
translate: $(srcdir)/qt/dashstrings.cpp $(QT_FORMS_UI) $(QT_FORMS_UI) $(BITCOIN_QT_BASE_CPP) $(BITCOIN_QT_WINDOWS_CPP) $(BITCOIN_QT_WALLET_CPP) $(BITCOIN_QT_H) $(BITCOIN_MM)
@test -n $(LUPDATE) || echo "lupdate is required for updating translations"
$(AM_V_GEN) QT_SELECT=$(QT_SELECT) $(LUPDATE) -no-obsolete -I $(srcdir) -locations relative $^ -ts $(srcdir)/qt/locale/dash_en.ts
@test -n $(LCONVERT) || echo "lconvert is required for updating translations"
Expand Down
12 changes: 10 additions & 2 deletions src/wallet/rpc/wallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,7 +634,9 @@ static RPCHelpMan createwallet()
{"blank", RPCArg::Type::BOOL, RPCArg::Default{false}, "Create a blank wallet. A blank wallet has no keys or HD seed. One can be set using upgradetohd (by mnemonic) or sethdseed (WIF private key)."},
{"passphrase", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "Encrypt the wallet with this passphrase."},
{"avoid_reuse", RPCArg::Type::BOOL, RPCArg::Default{false}, "Keep track of coin reuse, and treat dirty and clean coins differently with privacy considerations in mind."},
{"descriptors", RPCArg::Type::BOOL, RPCArg::Default{true}, "Create a native descriptor wallet. The wallet will use descriptors internally to handle address creation."},
{"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."},
{"load_on_startup", RPCArg::Type::BOOL, RPCArg::Optional::OMITTED_NAMED_ARG, "Save wallet name to persistent settings and load on startup. True to add wallet to startup list, false to remove, null to leave unchanged."},
{"external_signer", RPCArg::Type::BOOL, RPCArg::Default{false}, "Use an external signer such as a hardware wallet. Requires -signer to be configured. Wallet creation will fail if keys cannot be fetched. Requires disable_private_keys and descriptors set to true."},
},
Expand Down Expand Up @@ -679,11 +681,17 @@ static RPCHelpMan createwallet()
if (!request.params[5].isNull() && request.params[6].isNull()) {
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"))
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Mar 21, 2026

Choose a reason for hiding this comment

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

P1 Badge Replace unsupported RPCHelpMan Arg helper

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 👍 / 👎.

Copy link

@thepastaclaw thepastaclaw Mar 22, 2026

Choose a reason for hiding this comment

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

🔴 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
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.

Copy link

@thepastaclaw thepastaclaw Mar 24, 2026

Choose a reason for hiding this comment

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

🛑 Blocking — self.Arg("descriptors") does not exist in Dash (4/4 agents converged)

Dash's RPCHelpMan class (src/rpc/util.h:367-391) has no Arg<T>() template method. This method was introduced upstream in bitcoin#28230 (index-based) and bitcoin#29277 (name-based), neither of which has been backported. This is the only call site in the tree (grep confirms no other uses). The PR description states the author intended to revert to request.params[5] handling, but the reversion was not applied. This is a hard compilation failure.

Suggested change
if (self.Arg<bool>("descriptors"))
if (request.params[5].isNull() || request.params[5].get_bool())
{

{
#ifndef USE_SQLITE
throw JSONRPCError(RPC_WALLET_ERROR, "Compiled without sqlite support (required for descriptor wallets)");
#endif
flags |= WALLET_FLAG_DESCRIPTORS;
} else {
if (!context.chain->rpcEnableDeprecated("create_bdb")) {
throw JSONRPCError(RPC_WALLET_ERROR, "BDB wallet creation is deprecated and will be removed in a future release."
" In this release it can be re-enabled temporarily with the -deprecatedrpc=create_bdb setting.");
}
}
if (!request.params[7].isNull() && request.params[7].get_bool()) {
#ifdef ENABLE_EXTERNAL_SIGNER
Expand Down
1 change: 1 addition & 0 deletions test/functional/test_framework/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,7 @@ def write_config(config_path, *, n, chain, extra_config="", disable_autoconnect=
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
Copy link

@coderabbitai coderabbitai bot Mar 20, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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.

Copy link

@thepastaclaw thepastaclaw Mar 24, 2026

Choose a reason for hiding this comment

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

💡 Suggestion — Global deprecatedrpc=create_bdb prevents testing the new default behavior

Adding deprecatedrpc=create_bdb to every functional test node's config means no test exercises the new default where createwallet(..., descriptors=false) rejects BDB creation without the flag. Consider adding at least one test that verifies the deprecation error fires when the flag is absent, then enable deprecatedrpc=create_bdb only for tests that need legacy wallets.

# To reduce IO and consumed disk storage use tiny size for allocated blk and rev files
f.write("tinyblk=1\n")
# To improve SQLite wallet performance so that the tests don't timeout, use -unsafesqlitesync
Expand Down
Loading