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
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The blocking findings about missing skip_type_check on Dash-specific RPCs are confirmed and represent real regressions for JSON-RPC clients sending numeric block heights. The ParseBlockIndex catch-all error masking is also confirmed. Most other findings are either upstream code (not reviewable in a backport) or false positives.
Reviewed commit: 8facd0e
🔴 3 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
4 additional findings
🔴 blocking: protx diff baseBlock/block args reject numeric heights over JSON-RPC
src/rpc/evo.cpp (lines 1625-1626)
Both baseBlock and block are declared as RPCArg::Type::STR without skip_type_check. The new automatic type checking in HandleRequest() (line 516-526) maps Type::STR to UniValue::VSTR and rejects VNUM values before ParseBlock() is ever called. ParseBlock() (line 1591) explicitly handles v.isNum(), confirming numeric values are intended to work. JSON-RPC clients sending {"params": ["diff", 1000, 2000]} will get a type error. CLI clients are unaffected because baseBlock/block have no entry in client.cpp's conversion table, so the CLI sends them as strings. Compare with getblockstats (blockchain.cpp:2022-2026) and gettxoutsetinfo (blockchain.cpp:1145-1149) which correctly use skip_type_check = true for their mixed hash-or-height args.
💡 Suggested change
{"baseBlock", RPCArg::Type::STR, RPCArg::Optional::NO, "The starting block hash or height.", RPCArgOptions{.skip_type_check = true}},
{"block", RPCArg::Type::STR, RPCArg::Optional::NO, "The ending block hash or height.", RPCArgOptions{.skip_type_check = true}},
🔴 blocking: protx listdiff has same missing skip_type_check as protx diff
src/rpc/evo.cpp (lines 1666-1667)
Same issue as protx diff. baseBlock and block declared as RPCArg::Type::STR without skip_type_check. JSON-RPC clients sending numeric heights will be rejected by automatic type validation before ParseBlockIndex() → ParseBlock() can process them.
💡 Suggested change
{"baseBlock", RPCArg::Type::STR, RPCArg::Optional::NO, "The starting block hash or height.", RPCArgOptions{.skip_type_check = true}},
{"block", RPCArg::Type::STR, RPCArg::Optional::NO, "The ending block hash or height.", RPCArgOptions{.skip_type_check = true}},
🔴 blocking: evodb verify/repair reject numeric heights despite help examples showing them
src/rpc/evo.cpp (lines 1842-1843)
Both startBlock and stopBlock are declared as RPCArg::Type::STR without skip_type_check. The RPC help examples (line 1863) explicitly demonstrate numeric usage: HelpExampleRpc("evodb", "\"verify\", 1000, 2000"). JSON-RPC clients following the documented examples will hit a type error. Same root cause as protx diff/listdiff — automatic type checking rejects VNUM when VSTR is expected, before ParseBlock() can handle the numeric value. This applies to both evodb verify and evodb repair (line 1880-1881).
💡 Suggested change
{"startBlock", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "The starting block hash or height (defaults to DIP0003 activation height).", RPCArgOptions{.skip_type_check = true}},
{"stopBlock", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "The ending block hash or height (defaults to current chain tip).", RPCArgOptions{.skip_type_check = true}},
🟡 suggestion: ParseBlockIndex catch-all swallows specific 'Block not found' error
src/rpc/evo.cpp (lines 1607-1617)
The catch(...) on line 1614 catches all exceptions, including the specific "Block %s with hash %s not found" error thrown on line 1611 when LookupBlockIndex returns null. A user who supplies a syntactically valid block hash that isn't in the index gets the generic "must be a block hash or chain height" message instead of the specific "Block not found" message. Fix: move the LookupBlockIndex call and null check outside the try/catch, or narrow the catch to only wrap ParseBlock().
🤖 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/evo.cpp`:
- [BLOCKING] lines 1625-1626: protx diff baseBlock/block args reject numeric heights over JSON-RPC
Both `baseBlock` and `block` are declared as `RPCArg::Type::STR` without `skip_type_check`. The new automatic type checking in `HandleRequest()` (line 516-526) maps `Type::STR` to `UniValue::VSTR` and rejects `VNUM` values before `ParseBlock()` is ever called. `ParseBlock()` (line 1591) explicitly handles `v.isNum()`, confirming numeric values are intended to work. JSON-RPC clients sending `{"params": ["diff", 1000, 2000]}` will get a type error. CLI clients are unaffected because `baseBlock`/`block` have no entry in `client.cpp`'s conversion table, so the CLI sends them as strings. Compare with `getblockstats` (blockchain.cpp:2022-2026) and `gettxoutsetinfo` (blockchain.cpp:1145-1149) which correctly use `skip_type_check = true` for their mixed hash-or-height args.
- [BLOCKING] lines 1666-1667: protx listdiff has same missing skip_type_check as protx diff
Same issue as protx diff. `baseBlock` and `block` declared as `RPCArg::Type::STR` without `skip_type_check`. JSON-RPC clients sending numeric heights will be rejected by automatic type validation before `ParseBlockIndex()` → `ParseBlock()` can process them.
- [BLOCKING] lines 1842-1843: evodb verify/repair reject numeric heights despite help examples showing them
Both `startBlock` and `stopBlock` are declared as `RPCArg::Type::STR` without `skip_type_check`. The RPC help examples (line 1863) explicitly demonstrate numeric usage: `HelpExampleRpc("evodb", "\"verify\", 1000, 2000")`. JSON-RPC clients following the documented examples will hit a type error. Same root cause as protx diff/listdiff — automatic type checking rejects `VNUM` when `VSTR` is expected, before `ParseBlock()` can handle the numeric value. This applies to both `evodb verify` and `evodb repair` (line 1880-1881).
- [SUGGESTION] lines 1607-1617: ParseBlockIndex catch-all swallows specific 'Block not found' error
The `catch(...)` on line 1614 catches all exceptions, including the specific `"Block %s with hash %s not found"` error thrown on line 1611 when `LookupBlockIndex` returns null. A user who supplies a syntactically valid block hash that isn't in the index gets the generic `"must be a block hash or chain height"` message instead of the specific `"Block not found"` message. Fix: move the `LookupBlockIndex` call and null check outside the try/catch, or narrow the catch to only wrap `ParseBlock()`.
| 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.
💬 Nitpick: Global deprecatedrpc=create_bdb prevents testing the deprecation error path
Setting deprecatedrpc=create_bdb globally for all test nodes means no functional test will exercise the code path that returns the deprecation error when creating BDB wallets without the flag. This is a pragmatic choice to avoid breaking existing legacy wallet tests, but it means the deprecation enforcement logic is untested in the functional test suite. Consider adding a dedicated test that verifies the deprecation error fires when the flag is absent.
source: ['claude']
- Remove const from RPCArg constructor parameters to fix clang-tidy performance-move-const-arg warnings (std::move on const has no effect) - Fix rpc_rawtransaction.py: use bool (False/True) instead of int (0/1) for verbose parameter, matching RPCArg::Type::BOOL - Fix test_framework.py: convert operator_reward to string for protx register/register_fund RPC calls, matching RPCArg::Type::STR
The new automatic type checking from bitcoin#26039 rejects numeric heights sent by JSON-RPC clients for RPCArg::Type::STR parameters. Add skip_type_check to all evo.cpp RPCs where parameters accept both block hashes (strings) and heights (numbers): - protx diff: baseBlock, block - protx listdiff: baseBlock, block - evodb verify: startBlock, stopBlock - evodb repair: startBlock, stopBlock Also simplify ParseBlockIndex by removing the catch-all that was swallowing the specific 'Block not found' error. ParseBlock already throws a clear error on bad input, so the catch-all was redundant and masked useful error messages.
The RPC type checking backported in this PR (bitcoin#26039) enforces strict type checking for RPC parameters. The getrawtransaction 'verbose' parameter is declared as RPCArg::Type::BOOL, so passing integer 1 instead of True now raises a type error. Fix all 29 occurrences across 13 test files where getrawtransaction was called with integer 1 instead of boolean True.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The 4 incremental commits are correct. skip_type_check additions for evo RPCs are properly applied, getrawtransaction bool migration is complete, ParseBlockIndex catch-all removal is safe, and the RPCArg const removal is a clean upstream backport. No blocking issues found — only minor test hygiene suggestions.
Reviewed commit: 04e96e7
🟡 2 suggestion(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/test_framework/test_framework.py`:
- [SUGGESTION] line 1276: Pre-existing: collateral_vout has the same falsy-value bug that was fixed for operator_reward
The operator_reward fix here correctly changed `operator_reward or self.operator_reward` to `operator_reward if operator_reward is not None else self.operator_reward`. However, `collateral_vout` at line 1271 still uses `collateral_vout or self.collateral_vout` — vout index 0 is valid but falsy. Worth fixing in a follow-up since the pattern bug was already identified and fixed for operator_reward in this same function.
In `test/functional/rpc_rawtransaction.py`:
- [SUGGESTION] lines 116-119: Duplicate assertion and missing negative tests after integer verbose removal
Cases 3 and 4 now have identical first assertions (`getrawtransaction(txId, True)["hex"]`). Case 4's only unique value is testing `getrawtransactionmulti` with named args — consider removing the duplicate `getrawtransaction` call or updating the comment to clarify the intent.
Also, since type checking now rejects integers for BOOL params, the removed integer tests (0, 1) could be converted to negative test cases (asserting they raise type errors), similar to the existing string tests at lines 127-130.
| pubKeyOperator or self.pubKeyOperator, | ||
| votingAddr or self.votingAddr, | ||
| operator_reward or self.operator_reward, | ||
| str(operator_reward if operator_reward is not None else self.operator_reward), |
There was a problem hiding this comment.
🟡 Suggestion: Pre-existing: collateral_vout has the same falsy-value bug that was fixed for operator_reward
The operator_reward fix here correctly changed operator_reward or self.operator_reward to operator_reward if operator_reward is not None else self.operator_reward. However, collateral_vout at line 1271 still uses collateral_vout or self.collateral_vout — vout index 0 is valid but falsy. Worth fixing in a follow-up since the pattern bug was already identified and fixed for operator_reward in this same function.
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 `test/functional/test_framework/test_framework.py`:
- [SUGGESTION] line 1276: Pre-existing: collateral_vout has the same falsy-value bug that was fixed for operator_reward
The operator_reward fix here correctly changed `operator_reward or self.operator_reward` to `operator_reward if operator_reward is not None else self.operator_reward`. However, `collateral_vout` at line 1271 still uses `collateral_vout or self.collateral_vout` — vout index 0 is valid but falsy. Worth fixing in a follow-up since the pattern bug was already identified and fixed for operator_reward in this same function.
|
|
||
| # 5. valid parameters - supply txid and True for non-verbose | ||
| # 4. valid parameters - supply txid and True for verbose (named args) | ||
| assert_equal(self.nodes[n].getrawtransaction(txId, True)["hex"], tx['hex']) | ||
| assert_equal(self.nodes[n].getrawtransactionmulti(verbose=True, transactions={"0":[txId]})[txId]['hex'], tx['hex']) |
There was a problem hiding this comment.
🟡 Suggestion: Duplicate assertion and missing negative tests after integer verbose removal
Cases 3 and 4 now have identical first assertions (getrawtransaction(txId, True)["hex"]). Case 4's only unique value is testing getrawtransactionmulti with named args — consider removing the duplicate getrawtransaction call or updating the comment to clarify the intent.
Also, since type checking now rejects integers for BOOL params, the removed integer tests (0, 1) could be converted to negative test cases (asserting they raise type errors), similar to the existing string tests at lines 127-130.
source: ['claude', 'claude-backport']
🤖 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/rpc_rawtransaction.py`:
- [SUGGESTION] lines 116-119: Duplicate assertion and missing negative tests after integer verbose removal
Cases 3 and 4 now have identical first assertions (`getrawtransaction(txId, True)["hex"]`). Case 4's only unique value is testing `getrawtransactionmulti` with named args — consider removing the duplicate `getrawtransaction` call or updating the comment to clarify the intent.
Also, since type checking now rejects integers for BOOL params, the removed integer tests (0, 1) could be converted to negative test cases (asserting they raise type errors), similar to the existing string tests at lines 127-130.
…action tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Both findings are correctly identified issues but neither warrants a PR comment. The ParseBlock h<1 finding is pre-existing (not introduced in this delta) and must be dropped per incremental review policy. The positional-integer test suggestion exercises the same RPC type-checking codepath as the existing named-arg test, providing negligible additional coverage.
Reviewed commit: 4a0dab8
…e types The protx register/update RPCs accept multiple types (array, string, number) for address parameters like coreP2PAddrs, platformP2PAddrs, and platformHTTPSAddrs. The RPC type checking introduced by the bitcoin#26039 backport enforces strict type matching against the declared RPCArg::Type::ARR, rejecting valid integer port inputs. Add skip_type_check to these 6 address parameter definitions since their handlers (ProcessNetInfoCore, ProcessNetInfoPlatform) explicitly support NUM, STR, and ARR inputs for backwards compatibility. Also applies clang-format to the GetRpcArg map entries.
…nput The address-index RPCs (getaddressbalance, getaddresstxids, getaddressdeltas, getaddressutxos, getaddressmempool) declare their first parameter as ARR but actually accept either a bare string (single address) or an object with an addresses array. The stricter type checking from bitcoin#29277 now validates parameter types before the handler runs, causing these RPCs to reject string inputs with 'JSON value of type string is not of expected type array'. Add skip_type_check to these RPCs since they intentionally handle polymorphic input in getAddressesFromParams().
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.