backport: auto 20250821 #6827
backport: auto 20250821 #6827
Conversation
Co-authored-by: fanquake <fanquake@gmail.com>
Co-authored-by: fanquake <fanquake@gmail.com> Co-authored-by: pasta <pasta@pastas-Mac-Studio.local>
…and logging Co-authored-by: fanquake <fanquake@gmail.com> Co-authored-by: pasta <pasta@pastas-Mac-Studio.local>
… net(base).cpp Co-authored-by: fanquake <fanquake@gmail.com>
…back Co-authored-by: fanquake <fanquake@gmail.com>
…ntrol / sendcoins dialog Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
…our headers Co-authored-by: fanquake <fanquake@gmail.com>
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Co-authored-by: fanquake <fanquake@gmail.com>
Co-authored-by: glozow <gloriajzhao@gmail.com>
…achChain fails Co-authored-by: fanquake <fanquake@gmail.com>
Co-authored-by: fanquake <fanquake@gmail.com>
Co-authored-by: fanquake <fanquake@gmail.com> Co-authored-by: Claude Code <claude@anthropic.com>
Co-authored-by: fanquake <fanquake@gmail.com>
Co-authored-by: fanquake <fanquake@gmail.com>
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughDocumentation edits: require qemu in CI README; update OpenBSD note to 7.4; add brace-initialization guidance in developer notes; adjust policy README wording; tighten Linux MALLOC_ARENA_MAX guidance and example; update getmemoryinfo help text. Build change: quote per-package PATH assignments in depends/funcs.mk. Networking: remove some platform-specific include blocks in netbase/net.cpp and narrow libevent bufferevent read workaround to versions 2.1.6–2.1.9 in httpserver.cpp. Peer handling: remove the message parameter from MaybePunishNodeForTx and related logging. Wallet: reset m_chain_notifications_handler when AttachChain fails; forward-declare sqlite3 types in wallet/sqlite.h instead of including sqlite3.h. Qt GUI: remove Dust/LowOutput UI and clipboard features and add form label alignment properties. Tests/tooling: expanded URI parsing tests; rename EncodeDecimal→serialization_fallback and update imports; add test_runner CLI flag --skipunit to skip framework unit tests. CI scripts: remove --disable-gui-tests from s390x setup. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/functional/test_framework/authproxy.py (1)
62-66: Remove lingeringEncodeDecimaldefinition inutil.pyIt looks like the only remaining reference to
EncodeDecimalis its own definition intest/functional/test_framework/util.py(lines 229–233). To complete the rename and avoid dead code, please remove or replace this function with the newserialization_fallback.• File
test/functional/test_framework/util.py, remove or rename:-229- -230- -231-def EncodeDecimal(o): -232- if isinstance(o, Decimal): -233- return str(o)src/qt/coincontroldialog.cpp (1)
439-602: Remove leftover “Dust:” translation entriesThe code and UI no longer reference “Dust” or “LowOutput”, but the translation files still contain
<source>Dust:</source>entries that should be removed to avoid dead translations.• Affected files:
- All TS files under
src/qt/locale/(e.g.,dash_en.ts,dash_vi.ts,dash_ko.ts, etc.) containing<source>Dust:</source>message blocks.
• Action: remove each<message>block with<source>Dust:</source>and its corresponding<translation>entry.
🧹 Nitpick comments (20)
src/rpc/node.cpp (1)
891-896: LGTM with a tiny wording nitThe updated help text for getmemoryinfo clarifies availability without over-specifying glibc versions. Functionality remains gated correctly by HAVE_MALLOC_INFO at compile time.
Optional: To avoid edge-case confusion on very old or non-glibc libcs, consider slightly tightening the phrasing:
- " - \"mallocinfo\" returns an XML string describing low-level heap state (only available if compiled with glibc)."}, + " - \"mallocinfo\" returns an XML string describing low-level heap state (only available if compiled with glibc and malloc_info support)."},src/httpserver.cpp (2)
617-629: Mirror the same version-range handling here; consider using a shared helper.Re-enabling reads for the same [2.1.6, 2.1.9) range is consistent with the earlier disable. To keep things consistent and easier to maintain, prefer using a cached version or the proposed
LibeventHasBevReadBug()helper.Also, confirm that enabling
EV_READ | EV_WRITEis intentional (upstream does this). If we only ever disabledEV_READ, enablingEV_READalone would suffice, but matching upstream is typically safer during backports.Minimal in-place tweak (cache the version) within this block:
- if (event_get_version_number() >= 0x02010600 && event_get_version_number() < 0x02010900) { + const auto libevent_ver = event_get_version_number(); + if (libevent_ver >= 0x02010600 && libevent_ver < 0x02010900) {Or, if adopting the helper:
- if (event_get_version_number() >= 0x02010600 && event_get_version_number() < 0x02010900) { + if (LibeventHasBevReadBug()) {
222-233: Cacheevent_get_version_number()and centralize the workaroundWe’ve confirmed the same version-check block appears twice in
src/httpserver.cpp(around lines 225 and 621). To improve clarity, performance, and maintainability, you can:
- Cache the libevent version inside each block to avoid redundant calls:
- if (event_get_version_number() >= 0x02010600 && event_get_version_number() < 0x02010900) { + const uint32_t libevent_ver = event_get_version_number(); + if (libevent_ver >= 0x02010600 && libevent_ver < 0x02010900) {- Or better, extract a small helper into an anonymous namespace (e.g. near the top of
httpserver.cpp):Then simplify both locations to:namespace { constexpr uint32_t kLibeventBugStart = 0x02010600; // 2.1.6 constexpr uint32_t kLibeventBugFixed = 0x02010900; // 2.1.9 (exclusive) inline bool LibeventHasBevReadBug() { static const uint32_t v = event_get_version_number(); return v >= kLibeventBugStart && v < kLibeventBugFixed; } } // namespace- if (event_get_version_number() >= 0x02010600 && event_get_version_number() < 0x02010900) { + if (LibeventHasBevReadBug()) {- This ensures the magic constants and logic reside in one place, reducing future drift if the workaround range ever changes.
test/functional/test_runner.py (2)
434-435: Add --skipunit/-u option: good CLI surface, no conflicts detectedThe new flag is straightforward and consistent with existing options. No alias conflicts spotted (-u unused previously). Consider echoing when unit tests are skipped for clarity in CI logs.
573-581: Unit test gating works; minor exit-code consistency nit
- Gating on not skipunit is clear and uses unittest loader appropriately.
- sys.exit("Early exiting…") exits with code 1, but elsewhere the file tends to use numeric exit codes (e.g., sys.exit(not all_passed)). For consistency and easier tooling, prefer sys.exit(1).
Suggested tweak:
- sys.exit("Early exiting after failure in TestFramework unit tests") + sys.exit(1)Optionally log a one-liner when --skipunit is set so CI logs show that unit tests were intentionally skipped.
test/functional/test_framework/authproxy.py (2)
62-66: Rename to serialization_fallback: OK; add a thin compatibility alias to avoid downstream breakageThe new name is clearer and matches usage semantics. Given this is a backport batch, add a tiny alias to keep external/older tests from breaking if they still reference EncodeDecimal.
Apply:
def serialization_fallback(o): if isinstance(o, decimal.Decimal): return str(o) raise TypeError(repr(o) + " is not JSON serializable") + +# Back-compat shim (backported rename) +def EncodeDecimal(o): # deprecated + return serialization_fallback(o)This keeps behavior stable across backports and aligns with prior “move-only” backport preference.
188-191: Large-result logging nitDumping full results in logs can be heavy/noisy for large responses. Consider truncating or bounding the logged JSON (e.g., first N characters) to keep CI logs readable.
Example:
- log.debug("<-%s- [%.6f] %s" % (response["id"], elapsed, json.dumps(response["result"], default=serialization_fallback, ensure_ascii=self.ensure_ascii))) + result_str = json.dumps(response["result"], default=serialization_fallback, ensure_ascii=self.ensure_ascii) + log.debug("<-%s- [%.6f] %s" % (response["id"], elapsed, (result_str[:8192] + "…") if len(result_str) > 8192 else result_str))src/qt/forms/sendcoinsdialog.ui (2)
186-188: Label alignment addition improves visual consistency.Setting labelAlignment on formLayoutCoinControl1 to AlignLeft|AlignVCenter is sensible and matches the Coin Control dialog changes.
For full consistency in this dialog, consider applying the same labelAlignment to formLayoutCoinControl2 and formLayoutCoinControl3 as well, so all rows align identically regardless of translation length.
350-352: Mirror alignment for “After Fee/Change” block is good.This keeps the two form columns visually consistent.
If you decide to adopt the alignment in all Coin Control form layouts, update 2 and 3 here as well. Example XML to add under each QFormLayout:
<layout class="QFormLayout" name="formLayoutCoinControl2"> + <property name="labelAlignment"> + <set>Qt::AlignLeft|Qt::AlignVCenter</set> + </property> ... </layout> <layout class="QFormLayout" name="formLayoutCoinControl3"> + <property name="labelAlignment"> + <set>Qt::AlignLeft|Qt::AlignVCenter</set> + </property> ... </layout>src/qt/forms/coincontroldialog.ui (2)
27-29: Good: explicit label alignment on the first form column.AlignLeft|AlignVCenter reads well and matches Send Coins.
Consider applying the same alignment to formLayoutCoinControl2 and formLayoutCoinControl3 here as well for uniformity across all rows.
170-172: Good: alignment added to the “After Fee/Change” section.This matches the rest of the UI overhaul.
If you update 2 and 3 in this file (and in Send Coins) too, we’ll have consistent form row alignment across both dialogs.
src/qt/coincontroldialog.cpp (2)
445-447: Range-for loop is clear; consider minor tweaks.Current code is fine. Optional:
- Iterate by value; CAmount is a small integral type (int64_t), so “const &” has no benefit.
- Alternatively, std::accumulate expresses intent directly.
Here are two equivalent options:
Option A (by value):
- for (const CAmount &amount : CoinControlDialog::payAmounts) { + for (const CAmount amount : CoinControlDialog::payAmounts) { nPayAmount += amount; }Option B (accumulate; requires
<numeric>):+#include <numeric> // at top of file ... - CAmount nPayAmount = 0; - for (const CAmount &amount : CoinControlDialog::payAmounts) { - nPayAmount += amount; - } + CAmount nPayAmount = std::accumulate(CoinControlDialog::payAmounts.begin(), + CoinControlDialog::payAmounts.end(), + CAmount{0});Additionally (optional safety), you can assert/pay guard the money range if that’s a local convention:
// Optional: assert(MoneyRange(nPayAmount));
559-562: Enable/disable “Change” label wiring matches the UI default.UI sets Change labels disabled by default; enabling them when there’s a pay amount keeps behavior consistent.
To guard against future UI drift, consider resolving the pointers once and asserting presence:
- dialog->findChild<QLabel *>("labelCoinControlChangeText") ->setEnabled(nPayAmount > 0); - dialog->findChild<QLabel *>("labelCoinControlChange") ->setEnabled(nPayAmount > 0); + auto changeText = dialog->findChild<QLabel*>("labelCoinControlChangeText"); + auto changeValue = dialog->findChild<QLabel*>("labelCoinControlChange"); + Q_ASSERT(changeText && changeValue); + changeText->setEnabled(nPayAmount > 0); + changeValue->setEnabled(nPayAmount > 0);src/qt/test/uritests.cpp (3)
61-67: Rejecting comma-separated amounts is correct; add cases for locale/whitespace separators.Good negative coverage for commas. To harden against locale variants and encoded whitespace, consider adding NBSP/thin-space and percent-encoded space cases so parsers can’t bypass the check.
Apply this diff to extend the tests:
@@ uri.setUrl(QString("dash:XwnLY9Tf7Zsef8gMGL2fhWA9ZmMjt4KPwg?amount=1,000&label=Some Example")); QVERIFY(!GUIUtil::parseBitcoinURI(uri, &rv)); uri.setUrl(QString("dash:XwnLY9Tf7Zsef8gMGL2fhWA9ZmMjt4KPwg?amount=1,000.0&label=Some Example")); QVERIFY(!GUIUtil::parseBitcoinURI(uri, &rv)); + + // Disallow spaces and common Unicode thousands separators. + uri.setUrl(QString("dash:XwnLY9Tf7Zsef8gMGL2fhWA9ZmMjt4KPwg?amount=1%20000&label=Some Example")); // space + QVERIFY(!GUIUtil::parseBitcoinURI(uri, &rv)); + uri.setUrl(QString("dash:XwnLY9Tf7Zsef8gMGL2fhWA9ZmMjt4KPwg?amount=1%C2%A0000&label=Some Example")); // NBSP U+00A0 + QVERIFY(!GUIUtil::parseBitcoinURI(uri, &rv)); + uri.setUrl(QString::fromUtf8("dash:XwnLY9Tf7Zsef8gMGL2fhWA9ZmMjt4KPwg?amount=1%E2%80%AF000&label=Some Example")); // NNBSP U+202F + QVERIFY(!GUIUtil::parseBitcoinURI(uri, &rv));
68-74: Duplicate amount semantics: cover 3× duplicates and confirm “last wins” is intentional.The “last value wins” rule is explicit here—fine if that’s our intended policy. Add a 3-value case to lock behavior down, and please confirm this matches upstream/backport intent; some projects instead reject duplicates to avoid parameter-smuggling issues.
Proposed test addition:
@@ QVERIFY(rv.address == QString("XwnLY9Tf7Zsef8gMGL2fhWA9ZmMjt4KPwg")); QVERIFY(rv.amount == 20000000000LL); QVERIFY(rv.label == QString("Wikipedia Example")); + + // Three amounts; last still wins. + uri.setUrl(QString("dash:XwnLY9Tf7Zsef8gMGL2fhWA9ZmMjt4KPwg?amount=100&amount=200&amount=300&label=Wikipedia Example")); + QVERIFY(GUIUtil::parseBitcoinURI(uri, &rv)); + QVERIFY(rv.amount == 30000000000LL);If we decide duplicates should be rejected, I can propose a parser patch and flip these expectations accordingly.
79-85: Label with reserved characters: add coverage for ‘#’, ‘&’, and ‘=’ (require percent-encoding).Allowing raw “?” in a query value is fine, but “#” starts a fragment and “&”/“=” are query delimiters—these must be percent-encoded in URIs. Add tests to assert expectations and prevent accidental regressions.
Suggested additions (aligned with current “no decode” semantics in this file):
@@ QVERIFY(rv.amount == 10000000000LL); QVERIFY(rv.label == QString("?")); + // Label containing fragment/delimiter characters must be percent-encoded. + uri.setUrl(QString("dash:XwnLY9Tf7Zsef8gMGL2fhWA9ZmMjt4KPwg?amount=100&label=%23")); // '#' + QVERIFY(GUIUtil::parseBitcoinURI(uri, &rv)); + QVERIFY(rv.amount == 10000000000LL); + QVERIFY(rv.label == QString("%23")); + + uri.setUrl(QString("dash:XwnLY9Tf7Zsef8gMGL2fhWA9ZmMjt4KPwg?amount=100&label=%26")); // '&' + QVERIFY(GUIUtil::parseBitcoinURI(uri, &rv)); + QVERIFY(rv.label == QString("%26")); + + uri.setUrl(QString("dash:XwnLY9Tf7Zsef8gMGL2fhWA9ZmMjt4KPwg?amount=100&label=foo%3Dbar")); // '=' + QVERIFY(GUIUtil::parseBitcoinURI(uri, &rv)); + QVERIFY(rv.label == QString("foo%3Dbar"));ci/README.md (1)
18-22: Tighten wording and add language to code fence (markdownlint MD040)Minor doc polish: specify a language for the fenced block and tweak the sentence flow.
-requires `docker` to be installed. To run on different architectures than the host `qemu` is also required. To install all requirements on Ubuntu, run +requires `docker` to be installed. To run on different architectures than the host, `qemu` is also required. Install all requirements on Ubuntu with: - -``` +```bash sudo apt install docker.io bash qemu-user-static</blockquote></details> <details> <summary>doc/policy/README.md (1)</summary><blockquote> `5-6`: **Clarify how to access “Node relay options”** Slightly clarify that the options are shown in the binary’s help and add code formatting for the flag. ```diff -rules are local to the node and configurable, see "Node relay options" when running `-help`. +rules are local to the node and configurable; see “Node relay options” in `dashd -help`.doc/reduce-memory.md (1)
46-54: Tighten wording around glibc arena behavior; optional note for systemd users.The statement “glibc will create up to two heap arenas per core” is version- and arch-dependent; defaults have changed across glibc releases and can scale higher on 64‑bit. Consider softening to “glibc’s malloc may create multiple arenas per CPU (default upper bounds vary by version/arch).” The example and rationale are solid. Optionally add a brief note on setting MALLOC_ARENA_MAX in a systemd unit (Environment=MALLOC_ARENA_MAX=1) for users not launching via a wrapper script.
Would you like me to propose an exact wording update (diff) that accounts for glibc version differences and includes a short systemd snippet?
src/net_processing.cpp (1)
1875-1901: Preserve diagnostic context and drop unnecessary lock.Two small refinements:
- You can pass a concise reason to Misbehaving without re‑introducing the old method signature by using state.ToString(), keeping the useful context in NET logs.
- The cs_main lock here is no longer used; removing it reduces lock hold time and avoids needless contention.
Proposed diff:
bool PeerManagerImpl::MaybePunishNodeForTx(NodeId nodeid, const TxValidationState& state) { switch (state.GetResult()) { case TxValidationResult::TX_RESULT_UNSET: break; // The node is providing invalid data: case TxValidationResult::TX_CONSENSUS: - { - LOCK(cs_main); - Misbehaving(nodeid, 100); - return true; - } + { + Misbehaving(nodeid, 100, state.ToString()); + return true; + } // Conflicting (but not necessarily invalid) data or different policy: ... } return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (23)
ci/README.md(1 hunks)depends/funcs.mk(1 hunks)doc/build-openbsd.md(2 hunks)doc/developer-notes.md(3 hunks)doc/policy/README.md(1 hunks)doc/reduce-memory.md(1 hunks)src/httpserver.cpp(2 hunks)src/net.cpp(0 hunks)src/net_processing.cpp(2 hunks)src/netbase.cpp(0 hunks)src/qt/coincontroldialog.cpp(2 hunks)src/qt/coincontroldialog.h(0 hunks)src/qt/forms/coincontroldialog.ui(2 hunks)src/qt/forms/sendcoinsdialog.ui(2 hunks)src/qt/sendcoinsdialog.cpp(0 hunks)src/qt/sendcoinsdialog.h(0 hunks)src/qt/test/uritests.cpp(1 hunks)src/rpc/node.cpp(1 hunks)src/wallet/sqlite.h(1 hunks)src/wallet/wallet.cpp(1 hunks)test/functional/test_framework/authproxy.py(5 hunks)test/functional/test_framework/test_node.py(2 hunks)test/functional/test_runner.py(3 hunks)
💤 Files with no reviewable changes (5)
- src/netbase.cpp
- src/qt/coincontroldialog.h
- src/qt/sendcoinsdialog.h
- src/net.cpp
- src/qt/sendcoinsdialog.cpp
🧰 Additional context used
📓 Path-based instructions (6)
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/test_framework/authproxy.pytest/functional/test_runner.pytest/functional/test_framework/test_node.py
ci/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the ci directory (continuous integration)
Files:
ci/README.md
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/build-openbsd.mddoc/policy/README.mddoc/developer-notes.mddoc/reduce-memory.md
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/wallet/wallet.cppsrc/qt/test/uritests.cppsrc/httpserver.cppsrc/rpc/node.cppsrc/wallet/sqlite.hsrc/net_processing.cppsrc/qt/coincontroldialog.cpp
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/qt/test/uritests.cpp
depends/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the depends directory (dependency build system)
Files:
depends/funcs.mk
🧠 Learnings (1)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
🧬 Code graph analysis (2)
src/net_processing.cpp (2)
src/validation.h (1)
state(169-194)src/llmq/dkgsessionhandler.cpp (2)
Misbehaving(112-116)Misbehaving(112-112)
test/functional/test_framework/test_node.py (1)
test/functional/test_framework/authproxy.py (2)
JSONRPCException(51-59)serialization_fallback(62-65)
🪛 LanguageTool
ci/README.md
[style] ~18-~18: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... than the host qemu is also required. To install all requirements on Ubuntu, run...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
doc/policy/README.md
[grammar] ~5-~5: There might be a mistake here.
Context: ...ode relay options" when running -help. Policy may include restrictions on the t...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
ci/README.md
20-20: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (18)
test/functional/test_runner.py (2)
547-548: Plumbing skipunit through to run_tests is correctForwarding args.skipunit into run_tests keeps behavior explicit and keyworded. LGTM.
550-550: run_tests signature updated cleanly with a keyword-only skipunitKeeping use_term_control required and skipunit defaulting to False preserves backward compatibility of internal callers. Nice.
test/functional/test_framework/authproxy.py (3)
126-131: Logging args with serialization_fallback is correctSwitching json.dumps default to serialization_fallback preserves Decimal handling in debug logs. No behavior change otherwise. LGTM.
142-145: POST payload serialization updated: correct and consistentUsing serialization_fallback for request bodies is necessary for Decimal. Encoding to UTF-8 continues to be explicit. Good.
156-159: Batch POST serialization updated: correctConsistent with single-call path; preserves previous behavior for Decimal.
test/functional/test_framework/test_node.py (3)
25-28: Import switch to serialization_fallback is aligned with authproxy changeMatches the rename and keeps exceptions centralized. LGTM.
816-817: CLI arg JSON serialization uses serialization_fallback: correctThis keeps Decimal serialization behavior stable when passing dict/list args via CLI. No regressions expected.
25-28: No stale EncodeDecimal imports or usages detectedA ripgrep scan of the entire
test/functional/test_frameworkdirectory returned only the lone definition inutil.py(no other imports or calls toEncodeDecimal), confirming there are no leftover references. No further action is needed.src/qt/test/uritests.cpp (1)
75-78: LGTM: invalid second amount should invalidate the URI.Solid negative case; this guards against mixed-validity duplicates.
src/wallet/wallet.cpp (1)
3064-3066: Approved: Unsubscribe on AttachChain failure correctly handled; no additional call sites foundResetting m_chain_notifications_handler on AttachChain failure in wallet creation (src/wallet/wallet.cpp:3064–3066) prevents dangling callbacks and potential use-after-free. A repository-wide ripgrep scan across *.cpp and *.h files shows AttachChain is only called here (and defined immediately below), with no other call sites present. If future AttachChain usages are added, please apply the same unsubscribe-on-failure pattern.
• src/wallet/wallet.cpp:3064–3066
doc/build-openbsd.md (2)
3-3: Version bump looks goodUpdated reference to OpenBSD 7.4 is accurate and helpful. No further changes needed in this hunk.
43-44: Helpful cross-referencePointing readers to depends/README.md before showing db4 install is a nice touch and improves discoverability.
doc/developer-notes.md (3)
113-115: Endorse brace/list initialization guidanceRecommending list initialization aligns with modern C++ (and our C++20 baseline). Good addition.
140-140: Example updated to brace initUsing
int g_count{0};is consistent with the new guideline.
152-152: Loop-local initialization updated
int total_sum{0};matches the brace-init guidance; keeps examples consistent.src/wallet/sqlite.h (1)
13-15: Coverage Confirmed: All SQLite implementation files include sqlite3.hVerified that the only translation units which dereference sqlite3 or sqlite3_stmt are implementation files—and each directly includes <sqlite3.h>. No headers anywhere access these types beyond pointer declarations.
• src/wallet/sqlite.cpp: includes <sqlite3.h> on line 16
• src/leveldb/benchmarks/db_bench_sqlite3.cc: includes <sqlite3.h> on line 5Forward declarations in src/wallet/sqlite.h are therefore sufficient and safe.
depends/funcs.mk (1)
141-143: Quoting PATH assignments is a safe robustness win.Quoting prevents word-splitting when PATH contains spaces and ensures the whole assignment is exported as a single token in the shell block. Looks good.
src/net_processing.cpp (1)
698-700: No remaining 3-arg calls or declarations of MaybePunishNodeForTx
A repository-wide search confirms that all calls and definitions now use the 2-argument form (NodeId, TxValidationState), with no leftover references to a third “message” parameter.
• All 3-arg usages have been removed (no matches forMaybePunishNodeForTx(..., ..., ...)).
• Only the two-argument signature and calls remain in src/net_processing.cpp.
| // Escape sequences are not supported. | ||
| uri.setUrl(QString("dash:XwnLY9Tf7Zsef8gMGL2fhWA9ZmMjt4KPwg?amount=100&label=%3F")); | ||
| QVERIFY(GUIUtil::parseBitcoinURI(uri, &rv)); | ||
| QVERIFY(rv.address == QString("XwnLY9Tf7Zsef8gMGL2fhWA9ZmMjt4KPwg")); | ||
| QVERIFY(rv.amount == 10000000000LL); | ||
| QVERIFY(rv.label == QString("%3F")); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Percent-encoded label not decoded: verify against BIP-21; adjust test or document intentional deviation.
This test enshrines that “%3F” stays literal in the label. BIP-21 wallets typically percent-decode “label”/“message”. If Dash intentionally does not decode (e.g., to preserve raw text), keep this but document it. If standards compliance is desired, the expected value here should be “?”.
Two concrete options:
- Keep behavior (no decode): add a short comment noting this is intentional and deviates from BIP-21 to avoid ambiguity.
- Switch to decode: change the expectation to “?” as below.
Patch for the decode-aligned expectation:
- QVERIFY(rv.label == QString("%3F"));
+ QVERIFY(rv.label == QString("?"));To confirm parser behavior in-tree, run:
🏁 Script executed:
#!/bin/bash
# Inspect parseBitcoinURI implementation and how it retrieves/decodes query params.
rg -nP -C3 --type=cpp 'bool\s+GUIUtil::parseBitcoinURI\s*\(' src
rg -nP -C2 --type=cpp 'queryItemValue\s*\(\s*"label"|fromPercentEncoding|QUrlQuery' srcLength of output: 1411
🏁 Script executed:
#!/bin/bash
# Extract the implementation of GUIUtil::parseBitcoinURI to inspect how the "label" parameter is parsed
sed -n '350,450p' src/qt/guiutil.cppLength of output: 2675
Ensure BIP-21 compliance for percent-encoded labels
The current parseBitcoinURI loop assigns
rv.label = i->second;directly from QUrlQuery::queryItems(), which by default returns percent-encoded values (so %3F remains literal) (doc.qt.io). BIP-21 wallets, however, are expected to URL-decode the label (and similarly message) fields when parsing a URI (bips.xyz).
Two ways to address this:
- Decode on parse:
- After retrieving
i->second, call something likerv.label = QString::fromUtf8(QUrl::fromPercentEncoding(i->second.toUtf8()));
- Update the test in
uritests.cppto expect the decoded"?":- QVERIFY(rv.label == QString("%3F")); + QVERIFY(rv.label == QString("?"));
- After retrieving
- Document intentional deviation:
- If preserving the raw encoded form is a deliberate choice, insert a comment above the
rv.label = i->second;line (and in the test) explaining this is a known divergence from BIP-21.
- If preserving the raw encoded form is a deliberate choice, insert a comment above the
Locations requiring changes:
src/qt/guiutil.cpp, around lines 364–370 (theif (i->first == "label")block)src/qt/test/uritests.cpp, lines 86–92 (the label assertion)
🤖 Prompt for AI Agents
In src/qt/guiutil.cpp around lines 364-370 and src/qt/test/uritests.cpp lines
86-92, the parsing assigns percent-encoded label directly which leaves "%3F"
instead of "?"; update parseBitcoinURI to URL-decode i->second for the "label"
(and similarly "message") case using QUrl::fromPercentEncoding (or equivalent)
before assigning to rv.label, and update the unit test in uritests.cpp to expect
the decoded "?" instead of "%3F" (or alternatively add a clear comment in
guiutil.cpp if preserving raw encoding is intentional).
Co-authored-by: fanquake <fanquake@gmail.com>
Issue being fixed or feature implemented
Batch of automatically done backports by claude.
What was done?
See commits
How Has This Been Tested?
CI
Breaking Changes
None
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.