backport: Merge bitcoin#26642, 27344, gui#726, 27465, 26207, 27468 - #7390
Conversation
|
vijaydasmp
changed the title
|
🕓 Ready for review — 2 ahead in queue (commit 2b2fdd9) |
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
WalkthroughThis PR updates REST mempool contents query parameters and count parsing, adds URI parse failure handling with tests, removes legacy parsing checks from the string fuzz target, adjusts linting for that file, and adds vector preallocation changes across production and test code. It also includes minor documentation, clang-tidy, benchmark, and literal-search updates. Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant rest.cpp
participant MempoolToJSON
Client->>rest.cpp: GET /rest/mempool/contents.json?verbose&mempool_sequence
rest.cpp->>rest.cpp: parse/validate query params
alt invalid or conflicting params
rest.cpp-->>Client: HTTP_BAD_REQUEST
else valid params
rest.cpp->>MempoolToJSON: verbose, mempool_sequence
MempoolToJSON-->>rest.cpp: JSON result
rest.cpp-->>Client: JSON response
end
sequenceDiagram
participant Client
participant httpserver.cpp
participant evhttp_uri_parse
Client->>httpserver.cpp: request with invalid URI
httpserver.cpp->>evhttp_uri_parse: parse(uri)
evhttp_uri_parse-->>httpserver.cpp: nullptr
httpserver.cpp-->>Client: std::runtime_error / HTTP 400
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
🧹 Nitpick comments (1)
src/rest.cpp (1)
656-679: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate boolean-parsing boilerplate for
verbose/mempool_sequence.The two query-parameter blocks (fetch → validate
"true"/"false"→ 400 on error) are structurally identical aside from the parameter name and default. Extracting a small helper (e.g.ParseBoolQueryParam(req, name, default) -> std::optional<bool-or-error>) would remove duplication and make future boolean query params easier to add.♻️ Example helper sketch
+static util::Result<bool> GetBoolQueryParam(HTTPRequest* req, const std::string& name, bool default_value) +{ + std::string raw; + try { + raw = req->GetQueryParameter(name).value_or(default_value ? "true" : "false"); + } catch (const std::runtime_error& e) { + return util::Error{Untranslated(e.what())}; + } + if (raw != "true" && raw != "false") { + return util::Error{Untranslated(strprintf("The \"%s\" query parameter must be either \"true\" or \"false\".", name))}; + } + return raw == "true"; +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rest.cpp` around lines 656 - 679, The `verbose` and `mempool_sequence` query-parameter handling in `src/rest.cpp` duplicates the same fetch/validate/400-error flow twice. Refactor this into a small reusable helper near the request parsing logic used by `MempoolToJSON`, so both parameters are parsed through one path that accepts the parameter name and default, validates only "true"/"false", and returns the boolean value or error consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/rest.cpp`:
- Around line 656-679: The `verbose` and `mempool_sequence` query-parameter
handling in `src/rest.cpp` duplicates the same fetch/validate/400-error flow
twice. Refactor this into a small reusable helper near the request parsing logic
used by `MempoolToJSON`, so both parameters are parsed through one path that
accepts the parameter name and default, validates only "true"/"false", and
returns the boolean value or error consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 120e3388-19ce-4e35-8597-bf0c5d9aabb8
📥 Commits
Reviewing files that changed from the base of the PR and between 4af395d and 0e96447e240451b1b814b835edc3488c7e19cd2a.
📒 Files selected for processing (31)
ci/dash/lint-tidy.shdoc/REST-interface.mddoc/developer-notes.mdsrc/.clang-tidysrc/bench/lockedpool.cppsrc/bitcoin-cli.cppsrc/bitcoin-util.cppsrc/httpserver.cppsrc/node/interfaces.cppsrc/qt/trafficgraphwidget.cppsrc/rest.cppsrc/rpc/server.cppsrc/rpc/util.cppsrc/test/allocator_tests.cppsrc/test/fuzz/string.cppsrc/test/fuzz/tx_pool.cppsrc/test/getarg_tests.cppsrc/test/httpserver_tests.cppsrc/test/policyestimator_tests.cppsrc/test/scheduler_tests.cppsrc/test/script_p2sh_tests.cppsrc/test/util/net.cppsrc/test/util_threadnames_tests.cppsrc/test/validation_block_tests.cppsrc/util/bip32.cppsrc/wallet/bdb.cppsrc/wallet/rpc/backup.cppsrc/wallet/scriptpubkeyman.cppsrc/zmq/zmqpublishnotifier.cpptest/functional/interface_rest.pytest/lint/lint-locale-dependence.py
💤 Files with no reviewable changes (1)
- test/lint/lint-locale-dependence.py
There was a problem hiding this comment.
Code Review
I verified the two parsed Codex findings against the checked-out PR. The REST docs still carry Bitcoin Core's "25.0 and up" release note even though this Dash tree is versioned as 23.1.5, so that is a real in-scope documentation issue. The GUI provenance claim does not hold: the trafficgraphwidget change is part of the listed upstream bitcoin#26642 commit, while bitcoin-core/gui#726 is a different AddressPurpose change.
💬 1 nitpick(s)
Source: reviewers: gpt-5.5 (general, dash-core-commit-history); failed lanes: opus/claude-sonnet-5 (general, dash-core-commit-history, backport-reviewer), gpt-5.5 (backport-reviewer); verifier: gpt-5.5.
| Refer to the `getrawmempool` RPC help for details. Defaults to setting | ||
| `verbose=true` and `mempool_sequence=false`. | ||
|
|
||
| *Query parameters for `verbose` and `mempool_sequence` available in 25.0 and up.* |
There was a problem hiding this comment.
💬 Nitpick: Remove Bitcoin Core release number from Dash REST docs
This note was copied from the Bitcoin Core backport and says the new verbose and mempool_sequence query parameters are available in 25.0 and up. This is Dash Core documentation, and this PR is based on a Dash tree reporting version 23.1.5 in configure.ac, so the Bitcoin Core release number is misleading for Dash users trying to determine which Dash release contains /rest/mempool/contents.json?verbose=...&mempool_sequence=.... Adapt the sentence to the Dash release that will ship this backport, or remove the version-specific sentence.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Remove Bitcoin Core release number from Dash REST docs no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
…related fixes 03ec5b6 clang-tidy: Exclude `performance-*` checks rather including them (Hennadii Stepanov) 2400437 clang-tidy: Add `performance-type-promotion-in-math-fn` check (Hennadii Stepanov) 7e975e6 clang-tidy: Add `performance-inefficient-vector-operation` check (Hennadii Stepanov) 516b75f clang-tidy: Add `performance-faster-string-find` check (Hennadii Stepanov) Pull request description: ACKs for top commit: martinus: ACK 03ec5b6 TheCharlatan: re-ACK [03ec5b6](bitcoin@03ec5b6) Tree-SHA512: 2dfa52f9131da88826f32583bfd534a56a998477db9804b7333c0e7ac0b6b36141009755c7163b9f95d0ecbf5c2cb63f8a69ce4b114bb83423faed21b50cec67
faf8dc4 fuzz: Remove legacy int parse fuzz tests (MarcoFalke) Pull request description: The fuzz tests checked that the result of the new function was equal to the legacy function. (Side note: The checks were incomplete, as evident by the follow-up fix in commit b5c9bb5). Given that they haven't found any issues in years (beside missing the above issue, that they couldn't catch), it seems time to remove them. They may come in handy in the rare case that someone would want to modify `LocaleIndependentAtoi()` or `Parse*Int*()`, however that seems unlikely. Also, appropriate checks can be added then. ACKs for top commit: fanquake: ACK faf8dc4 dergoegge: ACK faf8dc4 Tree-SHA512: 4ec88b9fa8ba49a923b0604016f0f471b3c9b9e0ba6c5c3dc4e20503c6994789921e7221d9ec467a2a37a73f21a70ba51ba3370ed5ad311dee989e218290b29a
There was a problem hiding this comment.
Code Review
Source: reviewers: claude/opus general=failed, codex/gpt-5.5 general=ok, claude/opus dash-core-commit-history=failed, codex/gpt-5.5 dash-core-commit-history=ok, claude/opus backport-reviewer=failed, codex/gpt-5.5 backport-reviewer=ok; verifier: codex/gpt-5.5=ok.
Prior Reconciliation:
- prior-1: FIXED -
doc/REST-interface.mdline 135 now says23.1.5 and up, so the copied Bitcoin Core25.0 and upwording from the prior review is gone.
Carried-Forward Prior Findings: None.
New Findings In Latest Delta: 2 blocking backport-completeness findings.
The latest push fixes the prior REST documentation nitpick. Current-head review found two in-scope blockers: the advertised gui#726 backport is absent along with its prerequisite chain, and bitcoin#26642 omits upstream hunks because prerequisite upstream content is missing.
🔴 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/qt/bitcoin.cpp`:
- [BLOCKING] src/qt/bitcoin.cpp:89-91: Advertised gui#726 backport is missing its prerequisite chain
The PR title still advertises `bitcoin-core/gui#726`, but the current stack over `upstream/develop` contains only the five `bitcoin/bitcoin#...` merge commits and no gui#726 commit. Upstream gui#726 adds `#include <wallet/types.h>`, `Q_DECLARE_METATYPE(wallet::AddressPurpose)`, and `qRegisterMetaType<wallet::AddressPurpose>()` in this file; current `HEAD` has none of those lines, and `src/wallet/types.h` / `wallet::AddressPurpose` are absent. That type is introduced by upstream `bitcoin#27217`, so either the missing prerequisite chain and gui#726 need to be backported, or gui#726 must be removed from the PR title/description so the PR no longer claims to include it.
In `src/.clang-tidy`:
- [BLOCKING] src/.clang-tidy:9: bitcoin#26642 omits upstream hunks because prerequisites are missing
The PR claims a full `bitcoin#26642` backport and enables broad `performance-*` checks, but the upstream merge changed 25 files while the Dash merge changed 22. The omitted upstream fixes are `src/bench/wallet_create_tx.cpp`, `src/test/txrequest_tests.cpp`, and `src/script/descriptor.cpp`'s `MultiADescriptor::MakeScripts`; those hunks are absent because Dash is missing the prerequisite upstream content from `bitcoin#25234`, `bitcoin#19988`, and `bitcoin#24043`. Under the backport rules, omitted hunks caused by absent prerequisites are prerequisite gaps unless the PR explicitly excludes that upstream area, and I found no such exclusion in the PR metadata or `non-backported.txt`.
| performance-move-const-arg, | ||
| performance-no-automatic-move, | ||
| performance-unnecessary-copy-initialization, | ||
| performance-*, |
There was a problem hiding this comment.
🔴 Blocking: bitcoin#26642 omits upstream hunks because prerequisites are missing
The PR claims a full bitcoin#26642 backport and enables broad performance-* checks, but the upstream merge changed 25 files while the Dash merge changed 22. The omitted upstream fixes are src/bench/wallet_create_tx.cpp, src/test/txrequest_tests.cpp, and src/script/descriptor.cpp's MultiADescriptor::MakeScripts; those hunks are absent because Dash is missing the prerequisite upstream content from bitcoin#25234, bitcoin#19988, and bitcoin#24043. Under the backport rules, omitted hunks caused by absent prerequisites are prerequisite gaps unless the PR explicitly excludes that upstream area, and I found no such exclusion in the PR metadata or non-backported.txt.
source: ['codex']
There was a problem hiding this comment.
not relevant ; all performance-* relevant clang-tidy already done as prerequisites.
Any omitted changes in upstream will be highlighted by linter on CI immediately when they will be added to file.
| performance-move-const-arg, | ||
| performance-no-automatic-move, | ||
| performance-unnecessary-copy-initialization, | ||
| performance-*, |
There was a problem hiding this comment.
not relevant ; all performance-* relevant clang-tidy already done as prerequisites.
Any omitted changes in upstream will be highlighted by linter on CI immediately when they will be added to file.
knst
left a comment
knst
left a comment
There was a problem hiding this comment.
update doc/REST-interface.md version 23.1.5 -> 24.0
| Refer to the `getrawmempool` RPC help for details. Defaults to setting | ||
| `verbose=true` and `mempool_sequence=false`. | ||
|
|
||
| *Query parameters for `verbose` and `mempool_sequence` available in 23.1.5 and up.* |
There was a problem hiding this comment.
this code will be merged for 24.x branch and won't be backported to 23.x versionl
Please update here version to 24.x or 24.0
…ams for mempool/contents 1ff5d61 doc: add mempool/contents rest verbose and mempool_sequence args (Andrew Toth) 52a31dc tests: mempool/contents verbose and mempool_sequence query params tests (Andrew Toth) a518fff rest: add verbose and mempool_sequence query params for mempool/contents (Andrew Toth) Pull request description: The verbose mempool json response can get very large. This adds an option to return the non-verbose response of just the txids. It is identical to the rpc response so the diff here is minimal. This also adds the mempool_sequence parameter for rpc consistency. Verbose defaults to true to remain backwards compatible. It uses query parameters to be compatible with the efforts in bitcoin#25752. ACKs for top commit: achow101: ACK 1ff5d61 stickies-v: re-ACK [1ff5d61](bitcoin@1ff5d61) pablomartin4btc: tested ACK 1ff5d61. Tree-SHA512: 1bf08a7ffde2e7db14dc746e421feedf17d84c4b3f1141e79e36feb6014811dfde80e1d8dbc476c15ff705de2d3c967b3081dcd80536d76b7edf888f1a92e9d1
11422cc bugfix: rest: avoid segfault for invalid URI (pablomartin4btc) Pull request description: Minimal fix to get it promptly into 25.0 release (suggested by [stickies-v](bitcoin#27253 (review)) and supported by [vasild](bitcoin#27253 (review)) ) Please check bitcoin#27253 for reviewers comments and acks regarding this PR and read the commit comment message body for more details about the fix. ACKs for top commit: achow101: ACK 11422cc stickies-v: re-ACK 11422cc Tree-SHA512: 5af6b53fb266a12b463f960910556d5e97bc88b3c2a4f437ffa343886b38749e1eb058cf7bc64d62e82e1acf6232a186bddacd8f3b4500c87bf9e550a0153386
6 participants
bitcoin back ports