backport: bitcoin#22764, #24505, #26074, #26039, #28230, #29277, #28597 (wallet deprecation + RPC type checking)#7242
Conversation
|
thepastaclaw
force-pushed
the
backport-28597-with-prereqs
branch
from
f632694 to
6a6cf89
Compare
There was a problem hiding this comment.
Code Review — Updated
This PR is missing a critical prerequisite: bitcoin#28230. Resolved — the PR was restructured after the initial review and now includes bitcoin#28230 as a proper squash merge (commit 3435e53). The ArgValue<R>() template specializations, m_req assignment in HandleRequest, and all supporting infrastructure are present in the final tree.
The suggestion about test coverage for the create_bdb rejection path remains valid — see inline comment.
Originally reviewed commit: f632694 — PR has since been updated to 4ec22b0
🟡 1 suggestion(s)
| f.write("shrinkdebugfile=0\n") | ||
| # To reduce IO and consumed disk storage use tiny size for allocated blk and rev files | ||
| f.write("tinyblk=1\n") | ||
| f.write("deprecatedrpc=create_bdb\n") # Required to run the tests |
There was a problem hiding this comment.
🟡 Suggestion: Global deprecatedrpc=create_bdb bypasses the new BDB rejection gate in all tests
Adding deprecatedrpc=create_bdb to every test node's default config is necessary to keep existing tests that create legacy wallets working. However, this means the new default rejection path (wallet.cpp:690-693) is never exercised in the test suite. The wallet_createwallet.py test only covers the opt-in success case (creating a legacy wallet with the deprecation flag enabled) but never verifies that BDB creation is properly rejected when the flag is absent.
Consider adding a test case in wallet_createwallet.py that starts a node without -deprecatedrpc=create_bdb and asserts that createwallet(descriptors=False) returns the expected RPC error.
source: ['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 `test/functional/test_framework/util.py`:
- [SUGGESTION] line 424: Global deprecatedrpc=create_bdb bypasses the new BDB rejection gate in all tests
Adding `deprecatedrpc=create_bdb` to every test node's default config is necessary to keep existing tests that create legacy wallets working. However, this means the new default rejection path (wallet.cpp:690-693) is never exercised in the test suite. The `wallet_createwallet.py` test only covers the opt-in success case (creating a legacy wallet with the deprecation flag enabled) but never verifies that BDB creation is properly rejected when the flag is absent.
Consider adding a test case in `wallet_createwallet.py` that starts a node without `-deprecatedrpc=create_bdb` and asserts that `createwallet(descriptors=False)` returns the expected RPC error.
6a6cf89 to
4ec22b0
Compare
thepastaclaw
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Two build-breaking issues from the cherry-pick of bitcoin#28230. CI confirms all build jobs are failing. The first issue (RPCArg::MatchesType) is a compile error visible in CI logs. The second issue (missing ArgValue instantiation) is a linker error that would surface once the compile error is fixed. The prior blocking finding (missing prerequisites bitcoin#24505, bitcoin#28230, bitcoin#29277) IS resolved — all three are now included.
Reviewed commit: 4ec22b0
🔴 2 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/rpc/util.cpp`:
- [BLOCKING] lines 533-543: Compilation error: RPCArg::MatchesType() does not exist
The cherry-pick of bitcoin#28230 included an `arg_mismatch` type-checking block (lines 533–543) that calls `arg.MatchesType(request.params[i])` on an `RPCArg` object. However, `MatchesType` is only defined on `RPCResult` (util.h:346, util.cpp:879), not on `RPCArg`. CI confirms all build jobs fail with: `rpc/util.cpp:536:28: error: 'const struct RPCArg' has no member named 'MatchesType'`. This block was added upstream in a separate PR and should not have been included in this backport.
- [BLOCKING] lines 593-595: Linker error: missing ArgValue<bool> template instantiation
The `TMPL_INST` macro instantiates `ArgValue` for `int`, `uint64_t`, `const std::string&`, `std::optional<double>`, `std::optional<bool>`, and `const std::string*` — but NOT for `bool`. Since `std::is_integral_v<bool>` is `true`, `Arg<bool>(i)` (util.h:416) dispatches to `ArgValue<bool>(i)`, which has no explicit instantiation. This is called from `src/wallet/rpc/wallet.cpp:684` (`self.Arg<bool>("descriptors")`) and `src/test/rpc_tests.cpp:741,764`. This will produce an unresolved symbol linker error. Currently masked by Finding 1's compile error.
| UniValue arg_mismatch{UniValue::VOBJ}; | ||
| for (size_t i{0}; i < m_args.size(); ++i) { | ||
| const auto& arg{m_args.at(i)}; | ||
| UniValue match{arg.MatchesType(request.params[i])}; | ||
| if (!match.isTrue()) { | ||
| arg_mismatch.pushKV(strprintf("Position %s (%s)", i + 1, arg.m_names), std::move(match)); | ||
| } | ||
| } | ||
| if (!arg_mismatch.empty()) { | ||
| throw JSONRPCError(RPC_TYPE_ERROR, strprintf("Wrong type passed:\n%s", arg_mismatch.write(4))); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Compilation error: RPCArg::MatchesType() does not exist
The cherry-pick of bitcoin#28230 included an arg_mismatch type-checking block (lines 533–543) that calls arg.MatchesType(request.params[i]) on an RPCArg object. However, MatchesType is only defined on RPCResult (util.h:346, util.cpp:879), not on RPCArg. CI confirms all build jobs fail with: rpc/util.cpp:536:28: error: 'const struct RPCArg' has no member named 'MatchesType'. This block was added upstream in a separate PR and should not have been included in this backport.
source: ['claude']
🤖 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/rpc/util.cpp`:
- [BLOCKING] lines 533-543: Compilation error: RPCArg::MatchesType() does not exist
The cherry-pick of bitcoin#28230 included an `arg_mismatch` type-checking block (lines 533–543) that calls `arg.MatchesType(request.params[i])` on an `RPCArg` object. However, `MatchesType` is only defined on `RPCResult` (util.h:346, util.cpp:879), not on `RPCArg`. CI confirms all build jobs fail with: `rpc/util.cpp:536:28: error: 'const struct RPCArg' has no member named 'MatchesType'`. This block was added upstream in a separate PR and should not have been included in this backport.
| TMPL_INST(CheckRequiredOrDefault, int, CHECK_NONFATAL(maybe_arg)->getInt<int>();); | ||
| TMPL_INST(CheckRequiredOrDefault, uint64_t, CHECK_NONFATAL(maybe_arg)->getInt<uint64_t>();); | ||
| TMPL_INST(CheckRequiredOrDefault, const std::string&, CHECK_NONFATAL(maybe_arg)->get_str();); |
There was a problem hiding this comment.
🔴 Blocking: Linker error: missing ArgValue template instantiation
The TMPL_INST macro instantiates ArgValue for int, uint64_t, const std::string&, std::optional<double>, std::optional<bool>, and const std::string* — but NOT for bool. Since std::is_integral_v<bool> is true, Arg<bool>(i) (util.h:416) dispatches to ArgValue<bool>(i), which has no explicit instantiation. This is called from src/wallet/rpc/wallet.cpp:684 (self.Arg<bool>("descriptors")) and src/test/rpc_tests.cpp:741,764. This will produce an unresolved symbol linker error. Currently masked by Finding 1's compile error.
💡 Suggested change
| TMPL_INST(CheckRequiredOrDefault, int, CHECK_NONFATAL(maybe_arg)->getInt<int>();); | |
| TMPL_INST(CheckRequiredOrDefault, uint64_t, CHECK_NONFATAL(maybe_arg)->getInt<uint64_t>();); | |
| TMPL_INST(CheckRequiredOrDefault, const std::string&, CHECK_NONFATAL(maybe_arg)->get_str();); | |
| TMPL_INST(CheckRequiredOrDefault, bool, CHECK_NONFATAL(maybe_arg)->get_bool();); |
source: ['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/rpc/util.cpp`:
- [BLOCKING] lines 593-595: Linker error: missing ArgValue<bool> template instantiation
The `TMPL_INST` macro instantiates `ArgValue` for `int`, `uint64_t`, `const std::string&`, `std::optional<double>`, `std::optional<bool>`, and `const std::string*` — but NOT for `bool`. Since `std::is_integral_v<bool>` is `true`, `Arg<bool>(i)` (util.h:416) dispatches to `ArgValue<bool>(i)`, which has no explicit instantiation. This is called from `src/wallet/rpc/wallet.cpp:684` (`self.Arg<bool>("descriptors")`) and `src/test/rpc_tests.cpp:741,764`. This will produce an unresolved symbol linker error. Currently masked by Finding 1's compile error.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Push resolves 1 of 2 prior blocking findings: the RPCArg::MatchesType compile error is fixed by removing the cherry-pick artifact from bitcoin#26039. However, the missing TMPL_INST(CheckRequiredOrDefault, bool, ...) linker error persists — Arg<bool>("descriptors") in wallet.cpp dispatches to ArgValue<bool> which has no explicit template instantiation. Upstream Bitcoin Core added this line but it wasn't included in this backport.
Reviewed commit: 6713543
🔴 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/rpc/util.cpp`:
- [BLOCKING] line 584: Missing TMPL_INST for `bool` — linker error from `Arg<bool>()` usage
The prior review flagged this: `Arg<bool>("descriptors")` (wallet.cpp:684) dispatches through `std::is_integral_v<bool>` to `ArgValue<bool>(i)`, but there is no explicit template instantiation for `bool`. Only `std::optional<bool>` is instantiated (line 578). Upstream Bitcoin Core added `TMPL_INST(CheckRequiredOrDefault, bool, CHECK_NONFATAL(maybe_arg)->get_bool(););` alongside bitcoin#28597 — it needs to be included here too. Without it, the linker will produce an unresolved symbol error for `RPCHelpMan::ArgValue<bool>`.
| // Required arg or optional arg with default value. | ||
| TMPL_INST(CheckRequiredOrDefault, int, CHECK_NONFATAL(maybe_arg)->getInt<int>();); | ||
| TMPL_INST(CheckRequiredOrDefault, uint64_t, CHECK_NONFATAL(maybe_arg)->getInt<uint64_t>();); | ||
| TMPL_INST(CheckRequiredOrDefault, const std::string&, CHECK_NONFATAL(maybe_arg)->get_str();); |
There was a problem hiding this comment.
🔴 Blocking: Missing TMPL_INST for bool — linker error from Arg<bool>() usage
The prior review flagged this: Arg<bool>("descriptors") (wallet.cpp:684) dispatches through std::is_integral_v<bool> to ArgValue<bool>(i), but there is no explicit template instantiation for bool. Only std::optional<bool> is instantiated (line 578). Upstream Bitcoin Core added TMPL_INST(CheckRequiredOrDefault, bool, CHECK_NONFATAL(maybe_arg)->get_bool();); alongside bitcoin#28597 — it needs to be included here too. Without it, the linker will produce an unresolved symbol error for RPCHelpMan::ArgValue<bool>.
💡 Suggested change
| TMPL_INST(CheckRequiredOrDefault, const std::string&, CHECK_NONFATAL(maybe_arg)->get_str();); | |
| TMPL_INST(CheckRequiredOrDefault, const std::string&, CHECK_NONFATAL(maybe_arg)->get_str();); | |
| TMPL_INST(CheckRequiredOrDefault, bool, CHECK_NONFATAL(maybe_arg)->get_bool();); |
source: unknown
🤖 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/rpc/util.cpp`:
- [BLOCKING] line 584: Missing TMPL_INST for `bool` — linker error from `Arg<bool>()` usage
The prior review flagged this: `Arg<bool>("descriptors")` (wallet.cpp:684) dispatches through `std::is_integral_v<bool>` to `ArgValue<bool>(i)`, but there is no explicit template instantiation for `bool`. Only `std::optional<bool>` is instantiated (line 578). Upstream Bitcoin Core added `TMPL_INST(CheckRequiredOrDefault, bool, CHECK_NONFATAL(maybe_arg)->get_bool(););` alongside bitcoin#28597 — it needs to be included here too. Without it, the linker will produce an unresolved symbol error for `RPCHelpMan::ArgValue<bool>`.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
The missing TMPL_INST(CheckRequiredOrDefault, bool, ...) needs to be added to src/rpc/util.cpp. Without it, Arg<bool>("descriptors") in wallet.cpp:684 will cause a linker error.
Fix: add this line before the existing CheckRequiredOrDefault, int line (around line 582):
TMPL_INST(CheckRequiredOrDefault, bool, CHECK_NONFATAL(maybe_arg)->get_bool(););This matches upstream Bitcoin Core, which added this instantiation in commit c73cd42 (bitcoin#28331). The fix is on branch fix/7242-missing-bool-tmpl-inst — can be cherry-picked or squashed into the backport.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Single-line addition of the missing TMPL_INST(CheckRequiredOrDefault, bool, ...) template instantiation in src/rpc/util.cpp. The new line follows the established pattern exactly (CHECK_NONFATAL + get_bool()) and is placed before int, matching upstream ordering. Both review agents correctly identified this as a clean, necessary fix for the linker error caused by the bitcoin#28597 backport.
Reviewed commit: 1e7166e
thepastaclaw
force-pushed
the
backport-28597-with-prereqs
branch
2 times, most recently
from
5ca644d to
4def8f3
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
All four incremental commits are sound. The prior TMPL_INST blocker is resolved. The LLMQ signing test rewrite correctly de-flakes the test but drops coverage of the submit=false → P2P recovery pipeline. One misleading comment in the test. No blocking issues.
Reviewed commit: 4def8f3
🟡 1 suggestion(s) | 💬 1 nitpick(s)
🤖 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 `test/functional/feature_llmq_signing.py`:
- [SUGGESTION] lines 88-94: P2P round-trip coverage for submit=false sig shares removed without replacement
The original test validated the full pipeline: get a sig share via `submit=false`, construct a `CSigShare` P2P message from the returned fields, send it to the recovery member via `msg_qsigshare`, and verify signature recovery. This tested that the RPC's serialized output was compatible with the P2P transport format.
The new code only checks that the expected field names exist (lines 89-90), then submits a fresh share via the normal `submit=true` path (line 94), which takes the `AsyncSignIfMember` code path and never exercises the `submit=false` return value beyond shape validation.
The de-flaking tradeoff is reasonable, but the field validation could be strengthened to partially compensate — e.g., asserting value types and format (int for llmqType/quorumMember, 64-char hex for hashes, 192-char hex for BLS signature).
| # 3. Verify the returned sigShare has the expected fields | ||
| for field in ["llmqType", "quorumHash", "quorumMember", "id", "msgHash", "signHash", "signature"]: | ||
| assert field in sig_share_rpc_1, f"Missing field {field} in sigShare" | ||
| # 4. Now submit the third share normally so that recovery can proceed | ||
| # reliably via the concentrated send path (the submit=false RPC was | ||
| # already validated above) | ||
| self.mninfo[2].get_node(self).quorum("sign", q_type, id, msgHash) |
There was a problem hiding this comment.
🟡 Suggestion: P2P round-trip coverage for submit=false sig shares removed without replacement
The original test validated the full pipeline: get a sig share via submit=false, construct a CSigShare P2P message from the returned fields, send it to the recovery member via msg_qsigshare, and verify signature recovery. This tested that the RPC's serialized output was compatible with the P2P transport format.
The new code only checks that the expected field names exist (lines 89-90), then submits a fresh share via the normal submit=true path (line 94), which takes the AsyncSignIfMember code path and never exercises the submit=false return value beyond shape validation.
The de-flaking tradeoff is reasonable, but the field validation could be strengthened to partially compensate — e.g., asserting value types and format (int for llmqType/quorumMember, 64-char hex for hashes, 192-char hex for BLS signature).
💡 Suggested change
| # 3. Verify the returned sigShare has the expected fields | |
| for field in ["llmqType", "quorumHash", "quorumMember", "id", "msgHash", "signHash", "signature"]: | |
| assert field in sig_share_rpc_1, f"Missing field {field} in sigShare" | |
| # 4. Now submit the third share normally so that recovery can proceed | |
| # reliably via the concentrated send path (the submit=false RPC was | |
| # already validated above) | |
| self.mninfo[2].get_node(self).quorum("sign", q_type, id, msgHash) | |
| # 3. Verify the returned sigShare has the expected fields and plausible values | |
| for field in ["llmqType", "quorumHash", "quorumMember", "id", "msgHash", "signHash", "signature"]: | |
| assert field in sig_share_rpc_1, f"Missing field {field} in sigShare" | |
| assert isinstance(sig_share_rpc_1["llmqType"], int) | |
| assert len(sig_share_rpc_1["quorumHash"]) == 64 | |
| assert isinstance(sig_share_rpc_1["quorumMember"], int) | |
| assert len(sig_share_rpc_1["signature"]) == 192 # 96-byte BLS sig as hex |
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 `test/functional/feature_llmq_signing.py`:
- [SUGGESTION] lines 88-94: P2P round-trip coverage for submit=false sig shares removed without replacement
The original test validated the full pipeline: get a sig share via `submit=false`, construct a `CSigShare` P2P message from the returned fields, send it to the recovery member via `msg_qsigshare`, and verify signature recovery. This tested that the RPC's serialized output was compatible with the P2P transport format.
The new code only checks that the expected field names exist (lines 89-90), then submits a fresh share via the normal `submit=true` path (line 94), which takes the `AsyncSignIfMember` code path and never exercises the `submit=false` return value beyond shape validation.
The de-flaking tradeoff is reasonable, but the field validation could be strengthened to partially compensate — e.g., asserting value types and format (int for llmqType/quorumMember, 64-char hex for hashes, 192-char hex for BLS signature).
| # Advance mocktime past the daemon's 5-second signing session | ||
| # cleanup cadence so recovery responsibility rotates to the next | ||
| # member. Use 10s to guarantee at least one full cycle completes. | ||
| self.bump_mocktime(10) |
There was a problem hiding this comment.
💬 Nitpick: Comment incorrectly references '5-second cleanup cadence' — the relevant constant is the recovery rotation timeout
The comment says 'Advance mocktime past the daemon's 5-second signing session cleanup cadence'. The relevant constants in signing_shares.h are:
SIG_SHARE_REQUEST_TIMEOUT = 5(sig share re-request interval, not cleanup)SESSION_NEW_SHARES_TIMEOUT = 60(actual session cleanup)EXP_SEND_FOR_RECOVERY_TIMEOUT = 2000ms/MAX_SEND_FOR_RECOVERY_TIMEOUT = 10000ms(recovery rotation)
The purpose of the 10-second bump is to exceed MAX_SEND_FOR_RECOVERY_TIMEOUT so recovery responsibility rotates to the next member. The '5-second cleanup cadence' description conflates the wrong constant with the wrong mechanism.
💡 Suggested change
| # Advance mocktime past the daemon's 5-second signing session | |
| # cleanup cadence so recovery responsibility rotates to the next | |
| # member. Use 10s to guarantee at least one full cycle completes. | |
| self.bump_mocktime(10) | |
| # Advance mocktime past the daemon's MAX_SEND_FOR_RECOVERY_TIMEOUT | |
| # (10 s) so that recovery responsibility rotates to the next member. | |
| self.bump_mocktime(10) |
source: ['claude']
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Trivial incremental: 5 lines adding wait-for-propagation before stability assertion in LLMQ signing test — ensures recovered sig propagation completes under sanitizer load before checking assert_sigs_nochange. No production code changes.
Reviewed commit: 2b59080
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Single-commit incremental: restores shareman->RegisterRecoveryInterface() in ActiveContext::Start() (accidentally removed by the f8cd67e85d refactor that moved NotifyRecoveredSig to NetSigning). Without this registration, CSigSharesManager::HandleNewRecoveredSig() never fires, leaving sig-share sessions uncleaned after threshold recovery. The register/unregister pair follows proper LIFO ordering consistent with cl_signer and is_signer. Test helpers (wait_for_instantlock_with_bumping, wait_for_sigs_bumping) are well-designed with a 30s mocktime cap staying safely under SESSION_NEW_SHARES_TIMEOUT = 60s.
Reviewed commit: a80c46c
thepastaclaw
force-pushed
the
backport-28597-with-prereqs
branch
from
a80c46c to
5952472
Compare
|
Force-pushed to remove 5 commits that were unrelated to this backport (LLMQ test stability fixes and a Removed commits:
Where they went:
Remaining: 9 commits (5 upstream cherry-picks + 4 Dash adaptation fixes), all wallet/RPC scope. |
thepastaclaw
force-pushed
the
backport-28597-with-prereqs
branch
from
5952472 to
5207f5a
Compare
|
Force-pushed: squashed the 4 adaptation commits (remove MatchesType, add bool TMPL_INST, fix test assertions, restore bool TMPL_INST) into a single clean commit: Branch is now 5 upstream cherry-picks + 1 adaptation commit (6 total, down from 9). |
thepastaclaw
force-pushed
the
backport-28597-with-prereqs
branch
from
5207f5a to
fe1e763
Compare
|
Force-pushed again: folded all adaptations into their respective merge commits.
Branch is now 5 clean cherry-picks with no separate fix commits. Same tree as before ( |
…ct_strings.py b59b31a build: Drop redundant qt/bitcoin.cpp (Hennadii Stepanov) d90ad5a build: Include qt sources for parsing with extract_strings.py (Hennadii Stepanov) Pull request description: On master (4fc15d1) some strings are still untranslated. This PR fixes this issue. To verify: 1) `./autogen.sh && ./configure && make -C src translate` _before_ applying this change 2) apply this change 3) `./autogen.sh && ./configure && make -C src translate` _after_ applying this change The result of `git diff src/qt/bitcoinstrings.cpp`: ```diff --- a/src/qt/bitcoinstrings.cpp +++ b/src/qt/bitcoinstrings.cpp @@ -126,6 +126,7 @@ QT_TRANSLATE_NOOP("bitcoin-core", "" "You need to rebuild the database using -reindex to go back to unpruned " "mode. This will redownload the entire blockchain"), QT_TRANSLATE_NOOP("bitcoin-core", "%s is set very high!"), +QT_TRANSLATE_NOOP("bitcoin-core", "(press q to shutdown and continue later)"), QT_TRANSLATE_NOOP("bitcoin-core", "-maxmempool must be at least %d MB"), QT_TRANSLATE_NOOP("bitcoin-core", "A fatal internal error occurred, see debug.log for details"), QT_TRANSLATE_NOOP("bitcoin-core", "Cannot resolve -%s address: '%s'"), @@ -204,6 +205,8 @@ QT_TRANSLATE_NOOP("bitcoin-core", "SQLiteDatabase: Failed to prepare statement t QT_TRANSLATE_NOOP("bitcoin-core", "SQLiteDatabase: Failed to read database verification error: %s"), QT_TRANSLATE_NOOP("bitcoin-core", "SQLiteDatabase: Unexpected application id. Expected %u, got %u"), QT_TRANSLATE_NOOP("bitcoin-core", "Section [%s] is not recognized."), +QT_TRANSLATE_NOOP("bitcoin-core", "Settings file could not be read"), +QT_TRANSLATE_NOOP("bitcoin-core", "Settings file could not be written"), QT_TRANSLATE_NOOP("bitcoin-core", "Signing transaction failed"), QT_TRANSLATE_NOOP("bitcoin-core", "Specified -walletdir \"%s\" does not exist"), QT_TRANSLATE_NOOP("bitcoin-core", "Specified -walletdir \"%s\" is a relative path"), @@ -242,4 +245,5 @@ QT_TRANSLATE_NOOP("bitcoin-core", "User Agent comment (%s) contains unsafe chara QT_TRANSLATE_NOOP("bitcoin-core", "Verifying blocks…"), QT_TRANSLATE_NOOP("bitcoin-core", "Verifying wallet(s)…"), QT_TRANSLATE_NOOP("bitcoin-core", "Wallet needed to be rewritten: restart %s to complete"), +QT_TRANSLATE_NOOP("bitcoin-core", "press q to shutdown"), }; ``` ACKs for top commit: ryanofsky: Code review ACK b59b31a. Being able to use `_()` macro in qt would allow simplifying some code, for example replacing repetitive: TheCharlatan: ACK b59b31a Tree-SHA512: 13d9d86b487a1b6e718ae96c198a0a927c881bf33df318412793ec9efba3a7e59cfa836204f73f5b53ff4c99edce778c11bffaa88138b80e37b71e36df6b816f
…ted legacy wallets 6115218 wallet: Add a deprecation warning for newly created legacy wallets (Andrew Chow) Pull request description: As we slowly deprecate legacy wallets, we need to warn users that are making new legacy wallets that their wallet type is going to be unsupported in the future. ACKs for top commit: jonatack: ACK 6115218 S3RK: reACK 6115218 theStack: ACK 6115218 Tree-SHA512: e89bfb8168869542498958f0c9a2ab302dfd43287f8a49e7d9e09f60438a567bb8b7219a4e569797ee819b30b624f532fcc0b70c6aa0edcb392a301b8ce8b541
c00000d rpc: Add MaybeArg() and Arg() default helper (MarcoFalke) Pull request description: Currently the RPC method implementations have many issues: * Default RPC argument values (and their optionality state) are duplicated in the documentation and the C++ code, with no checks to prevent them from going out of sync. * Getting an optional RPC argument is verbose, using a ternary operator, or worse, a multi-line `if`. Fix all issues by adding default helper that can be called via `self.Arg<int>(0)`. The helper needs a few lines of code in the `src/rpc/util.h` header file. Everything else will be implemented in the cpp file once and if an RPC method needs it. There is also an `self.MaybeArg<int>(0)` helper that works on any arg to return the argument, the default, or a falsy value. ACKs for top commit: ajtowns: reACK c00000d stickies-v: re-ACK c00000d TheCharlatan: re-ACK c00000d Tree-SHA512: e7ddcab3faa319bc53edbdf3f89ce83389d2c4e571d5db42401620ff105e522a4a0669dad08e08cde5fd05c790aec3b806f63261a9100c2778865a489e57381e
30a6c99 rpc: access some args by name (stickies-v) bbb3126 rpc: add named arg helper (stickies-v) 13525e0 rpc: add arg helper unit test (stickies-v) Pull request description: Adds string overloads for the `RPCHelpMan::Arg` and `RPCHelpMan::MaybeArg` helpers to be able to access RPC arguments by name instead of index number. Especially in RPCs with a large number of parameters, this can be quite helpful. Example usage: ```cpp const auto action{self.Arg<std::string>("action")}; ``` Most of the LoC is adding test coverage and documentation updates. No behaviour change. An alternative approach to bitcoin#27788 with significantly less overhaul. ACKs for top commit: fjahr: Code review ACK 30a6c99 maflcko: ACK 30a6c99 🥑 ryanofsky: Code review ACK 30a6c99. Nice change! Implementation is surprisingly simple and additional unit test coverage is welcome, too. Tree-SHA512: 4904f5f914fe1d421d32f60edb7c5a028c8ea0f140a2f207a106b4752d441164e073066a6bf2e17693f859fe847815a96609d3cf521e0ac4178d8cd09362ea3d
…reate_bdb fa071ae wallet: No BDB creation, unless -deprecatedrpc=create_bdb (MarcoFalke) 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. ACKs for top commit: Sjors: tACK fa071ae achow101: ACK fa071ae furszy: utACK fa071ae Tree-SHA512: 37a4c3e4ba659e0ebe2382e71d9c80e42a895d9ad743f5dda7c110fbbb7d2a36f46769982552a9ac0c3a57203379ef164be97aa8033eb7674d6b4da030ba8f9b
thepastaclaw
force-pushed
the
backport-28597-with-prereqs
branch
from
fe1e763 to
8facd0e
Compare
|
Force-pushed: complete rebuild with proper prerequisite chain. New commit order (8 commits):
What changed: Instead of deleting the
Adaptations folded into merge commits (obvious/clear):
Build verified: |
thepastaclaw
changed the title
4 participants
Backported PRs
New prerequisite PRs (for bitcoin#28230)
Original scope
Adaptations
RPCArgOptions{}structRPCTypeCheck()calls from Dash-specific RPCs (type checking now automatic viaRPCArg::MatchesType())TMPL_INST(from unbackported BIP324 integration bitcoin/bitcoin#28331) + wallet_createwallet test passphrase fixNotes
Supersedes d0wn3d's #7238. Originally opened with 5 commits + hack fixes; rebuilt with proper prerequisite chain after review.