backport: bitcoin#18815, #22490, #23858, #24137, #24409, #24453, #24510, #24524, #24600, #24601, #24608, #24658, bitcoin-core/gui#441 #6853
Conversation
BACKPORT NOTE: other changes are reverted by bitcoin#23866 fafe4de test: Fix pep8 of touched file (MarcoFalke) fa0ac9d test: Fix rpc_scantxoutset intermittent issue (MarcoFalke) Pull request description: I fail to see how this could have ever worked, since there is nothing that prevents the wallet from spending the coins in the utxo set. Fixes bitcoin#23847 Longer term it would be nice to remove the wallet and use MiniWallet here. ACKs for top commit: brunoerg: tACK fafe4de theStack: Code-review ACK fafe4de Tree-SHA512: d92b7be9a81eb62f496488dd15b8e564e9b7a64b55634af2714b53f985e6a35fdae788323833ff59c40ae7c6a0cf54fe069db8eb3be03686f7c100379f54a292 test: Fix pep8 of touched file Can be reviewed with --word-diff-regex=. --ignore-all-space
fa30e62 doc: Rework generate* doc (MarcoFalke) Pull request description: Hide the test-only calls and clarify the short description ACKs for top commit: 0xB10C: reACK fa30e62. changes since fa3bb58 are: dropping the `immediately` + formatting the touched line and a rebase Tree-SHA512: 07439f39660bbf144c2cc406b6010b64dcdd27150d78654fe04a36a982a519f837a0cf0f030c9f30af69c451ccf7a3b7287a275637aa81904c202029b9efc661
…in the functional tests 8ca51af test: Disable automatic connections by default (Martin Zumsande) Pull request description: A node normally doesn't make automatic connections to peers in the functional tests because neither DNS seeds nor hardcoded peers are available on regtest. However, when random entries are inserted into addrman as part of a functional test (e.g. while testing addr relay), `ThreadOpenConnections` will periodically try to connect to them, resulting in log entries such as: `[opencon] [net.cpp:400] [ConnectNode] trying connection 18.166.1.1:8333 lastseen=0.0hrs` I don't think it's desirable that functional tests try to connect to random computers on the internet, aside from the possibility that at some point in time someone out there might actually answer in a way to ruin a test. This PR fixes this problem by disabling `ThreadOpenConnections` by adding `-connect=0` to the default args, and adding exceptions only when needed for the test to pass. ACKs for top commit: tryphe: Concept ACK, light code review ACK 8ca51af Tree-SHA512: bcfb2de610e6c35a97a2bd7ad6267e968b1ac7529638d99276993cd5bc93ce9919d54e22d6dc84e1b02ecd626ab6554e201693552ea065c29794eece38c43f7d
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughUpdates across docs, build files, benchmarks, CLI, RPC, wallet, Qt UI, test framework, and tooling. Key changes: doc clarifications for FreeBSD/NetBSD/Unix and .gitignore entry; expanded bench sources and new logging benchmark; CLI/version output behavior changed with new LicenseInfo/CopyrightHolders in clientversion and removals from init/system; mining RPC help text updated and three mining RPCs always registered hidden; wallet RPC change-address arg types adjusted; Qt adds an "Enable PSBT controls" option and refactors SendCoinsDialog for PSBT paths; tests add disable_autoconnect plumbing and new importprunedfunds cases; lint/gen-manpages tweaks. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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.
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 (3)
doc/build-freebsd.md (1)
108-112: Use --disable-wallet (not --without-wallet) in docsconfigure.ac defines AC_ARG_ENABLE([wallet]) — configure accepts --enable/--disable flags; --without-wallet is not recognized. Replace the occurrences in doc/build-freebsd.md (lines 108–112) and doc/build-osx.md (lines 239–242) and grep the repo for any other
--without-walletusages to normalize to--disable-wallet(consistent with doc/build-unix.md).src/bitcoin-cli.cpp (1)
556-561: Fix reversed condition causing out-of-bounds read and incorrect displaystarts_with returns bool; comparing to 0 inverts the logic. This can index [1] on non-“v*” strings and yields wrong output for “vN”. Use a direct bool check and guard length.
Apply this diff:
- peer.transport_protocol_type.starts_with('v') == 0 ? peer.transport_protocol_type[1] : ' ', + (peer.transport_protocol_type.starts_with('v') && peer.transport_protocol_type.size() > 1) ? peer.transport_protocol_type[1] : ' ',src/qt/sendcoinsdialog.cpp (1)
500-503: PSBT Save path: handle failures, fix i18n, and restore state on cancel.
- Don’t assert on fillPSBT errors; show a user-facing error and bail.
- Wrap user-facing strings in tr() for translation.
- If the user cancels the Save dialog (empty filename), restore fNewRecipientAllowed before returning to avoid locking out new recipients.
Apply this diff:
- bool complete = false; - const TransactionError err = model->wallet().fillPSBT(SIGHASH_ALL, false /* sign */, true /* bip32derivs */, nullptr, psbtx, complete); - assert(!complete); - assert(err == TransactionError::OK); + bool complete = false; + const TransactionError err = model->wallet().fillPSBT(SIGHASH_ALL, /* sign */ false, /* bip32derivs */ true, nullptr, psbtx, complete); + if (err != TransactionError::OK || complete) { + Q_EMIT message(tr("PSBT creation failed"), tr("Unable to create a PSBT."), CClientUIInterface::MSG_ERROR); + fNewRecipientAllowed = true; + return; + }- msgBox.setText("Unsigned Transaction"); - msgBox.setInformativeText("The PSBT has been copied to the clipboard. You can also save it."); + msgBox.setText(tr("Unsigned Transaction")); + msgBox.setInformativeText(tr("The PSBT has been copied to the clipboard. You can also save it."));- if (filename.isEmpty()) { - return; - } + if (filename.isEmpty()) { + fNewRecipientAllowed = true; + return; + }- Q_EMIT message(tr("PSBT saved"), "PSBT saved to disk", CClientUIInterface::MSG_INFORMATION); + Q_EMIT message(tr("PSBT saved"), tr("PSBT saved to disk"), CClientUIInterface::MSG_INFORMATION);Optional (non-blocking): prefer QSaveFile for atomic writes and check failures.
Example:
QSaveFile out(filename); if (!out.open(QIODevice::WriteOnly)) { Q_EMIT message(tr("PSBT save failed"), tr("Cannot open %1 for writing.").arg(filename), CClientUIInterface::MSG_ERROR); fNewRecipientAllowed = true; return; } out.write(ssTx.str().data(), static_cast<qint64>(ssTx.str().size())); out.commit();Also applies to: 508-511, 531-533, 537-537
🧹 Nitpick comments (12)
test/functional/wallet_importprunedfunds.py (1)
132-135: Add one more negative case for invalid txoutproof hex.Consider adding a case where txoutproof is non‑hex or truncated to assert RPC_INVALID_PARAMETER is raised. It complements the invalid tx hex check and the mismatched-proof case.
src/wallet/rpc/spend.cpp (1)
1066-1066: Docs consistency: consider mentioning the underscore aliaswalletcreatefundedpsbt documents changeAddress (camelCase), while send documents change_address (underscore) and rejects changeAddress. Since parsing accepts both in FundTransaction, consider noting change_address as an accepted alias here to reduce confusion. If mirroring upstream exactly, feel free to keep as-is.
doc/build-unix.md (1)
258-258: Use fenced code blocks instead of indentation (MD046).The indented command under disable‑wallet triggers markdownlint MD046. Prefer fenced blocks for consistency with the rest of the doc.
doc/build-netbsd.md (2)
55-60: Fix grammar and pkg system reference.
- “BerkeleyDB is use for legacy wallet functionality.” → “Berkeley DB is used for legacy wallet functionality.”
- “from ports.” is FreeBSD terminology; NetBSD uses pkgsrc. Suggest “from pkgsrc packages.”
33-33: Heading style consistency (MD003).Mixed setext and atx heading styles. Consider aligning to one style or disabling the rule to avoid lint churn.
Also applies to: 37-37, 53-53, 81-81
doc/build-freebsd.md (1)
90-96: Fix heading level increment (MD001) and minor wording.
- Heading jumps from h3 to h5; use h4 to satisfy MD001.
- Minor: “assuming sqlite and qt are installed” → “sqlite3 and Qt5” for consistency with package names above.
test/functional/test_framework/test_framework.py (1)
461-461: Replace stray triple‑quoted string with a comment.It’s currently a no‑op string literal inside the function body. Prefer a regular comment.
Apply this diff:
- """ NOTE! If this method is updated - backport changes to DashTestFramework.setup_nodes""" + # NOTE: If this method is updated, backport changes to DashTestFramework.setup_nodessrc/bench/logging.cpp (1)
20-48: Clarify benchmark naming (“Yo” → “Yes”/“With”).Names like “YoThreadNames/YoCategory” read like typos. Prefer “Yes/With” for readability in bench output.
-static void LoggingYoThreadNames(benchmark::Bench& bench) +static void LoggingYesThreadNames(benchmark::Bench& bench) { - Logging(bench, {"-logthreadnames=1"}, [] { LogPrintf("%s\n", "test"); }); + Logging(bench, {"-logthreadnames=1"}, [] { LogPrintf("%s\n", "test"); }); } -static void LoggingYoCategory(benchmark::Bench& bench) +static void LoggingYesCategory(benchmark::Bench& bench) { - Logging(bench, {"-logthreadnames=0", "-debug=net"}, [] { LogPrint(BCLog::NET, "%s\n", "test"); }); + Logging(bench, {"-logthreadnames=0", "-debug=net"}, [] { LogPrint(BCLog::NET, "%s\n", "test"); }); } -BENCHMARK(LoggingYoThreadNames); +BENCHMARK(LoggingYesThreadNames); BENCHMARK(LoggingNoThreadNames); -BENCHMARK(LoggingYoCategory); +BENCHMARK(LoggingYesCategory); BENCHMARK(LoggingNoCategory); BENCHMARK(LoggingNoFile);test/functional/rpc_scantxoutset.py (2)
49-61: Avoid float-to-int rounding in amount construction (possible off-by-one sats).Using float literals (e.g., 1.024) with int(COIN * amount) risks flooring due to FP rounding. Prefer Decimal to build satoshis deterministically.
Minimal change (outside this hunk) to harden sendtodestination:
def sendtodestination(self, destination, amount_btc): if isinstance(destination, str): destination = address_to_scriptpubkey(destination) sat = int(Decimal(str(amount_btc)) * COIN) self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=destination, amount=sat)
72-78: Prefer f-strings for readability in descriptor/address concatenations.No behavior change; improves clarity.
- assert_equal(self.nodes[0].scantxoutset("start", ["pkh(" + pubk1.hex() + ")", "pkh(" + pubk2.hex() + ")", "pkh(" + pubk3.hex() + ")"])['total_amount'], Decimal("0.007")) - assert_equal(self.nodes[0].scantxoutset("start", ["combo(" + pubk1.hex() + ")", "combo(" + pubk2.hex() + ")", "combo(" + pubk3.hex() + ")"])['total_amount'], Decimal("0.007")) - assert_equal(self.nodes[0].scantxoutset("start", ["addr(" + addr1 + ")", "addr(" + addr2 + ")", "combo(" + pubk3.hex() + ")"])['total_amount'], Decimal("0.007")) - assert_equal(self.nodes[0].scantxoutset("start", ["addr(" + addr1 + ")", "addr(" + addr2 + ")", "addr(" + addr3 + ")"])['total_amount'], Decimal("0.007")) - assert_equal(self.nodes[0].scantxoutset("start", ["addr(" + addr1 + ")", "addr(" + addr2 + ")", "pkh(" + pubk3.hex() + ")"])['total_amount'], Decimal("0.007")) + assert_equal(self.nodes[0].scantxoutset("start", [f"pkh({pubk1.hex()})", f"pkh({pubk2.hex()})", f"pkh({pubk3.hex()})"])['total_amount'], Decimal("0.007")) + assert_equal(self.nodes[0].scantxoutset("start", [f"combo({pubk1.hex()})", f"combo({pubk2.hex()})", f"combo({pubk3.hex()})"])['total_amount'], Decimal("0.007")) + assert_equal(self.nodes[0].scantxoutset("start", [f"addr({addr1})", f"addr({addr2})", f"combo({pubk3.hex()})"])['total_amount'], Decimal("0.007")) + assert_equal(self.nodes[0].scantxoutset("start", [f"addr({addr1})", f"addr({addr2})", f"addr({addr3})"])['total_amount'], Decimal("0.007")) + assert_equal(self.nodes[0].scantxoutset("start", [f"addr({addr1})", f"addr({addr2})", f"pkh({pubk3.hex()})"])['total_amount'], Decimal("0.007"))src/qt/sendcoinsdialog.h (1)
115-115: Prefer constexpr over macro (optional)Use a scoped constant for type safety.
Apply this diff:
-#define SEND_CONFIRM_DELAY 3 +static constexpr int kSendConfirmDelay = 3;And update the default arg to use kSendConfirmDelay.
src/qt/sendcoinsdialog.cpp (1)
367-386: PSBT messaging: avoid “Bitcoin” branding and show PSBT notice when send is disabled.
- Use “Partially Signed Transaction (PSBT)” to avoid brand mismatch.
- When private keys are disabled (send is impossible) but PSBT controls are off, the Save button is still shown; add PSBT guidance text for that case.
Apply this diff:
- // if (model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner()) { + // if (model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner()) { if (false) { /*: Text to inform a user attempting to create a transaction of their current options. At this stage, a user can only create a PSBT. This string is displayed when private keys are disabled and an external signer is not available. */ - question_string.append(tr("Please, review your transaction proposal. This will produce a Partially Signed Bitcoin Transaction (PSBT) which you can save or copy and then sign with e.g. an offline %1 wallet, or a PSBT-compatible hardware wallet.").arg(PACKAGE_NAME)); - } else if (model->getOptionsModel()->getEnablePSBTControls()) { + question_string.append(tr("Please, review your transaction proposal. This will produce a Partially Signed Transaction (PSBT) which you can save or copy and then sign with, e.g., an offline %1 wallet, or a PSBT-compatible hardware wallet.").arg(PACKAGE_NAME)); + } else if (model->wallet().privateKeysDisabled()) { + // Save-only flow (no private keys): inform about PSBT even if PSBT controls are disabled + question_string.append(tr("Please, review your transaction proposal. This will produce a Partially Signed Transaction (PSBT) which you can save or copy and then sign with, e.g., an offline %1 wallet, or a PSBT-compatible hardware wallet.").arg(PACKAGE_NAME)); + } else if (model->getOptionsModel()->getEnablePSBTControls()) { /*: Text to inform a user attempting to create a transaction of their current options. At this stage, a user can send their transaction or create a PSBT. This string is displayed when both private keys and PSBT controls are enabled. */ - question_string.append(tr("Please, review your transaction. You can create and send this transaction or create a Partially Signed Bitcoin Transaction (PSBT), which you can save or copy and then sign with, e.g., an offline %1 wallet, or a PSBT-compatible hardware wallet.").arg(PACKAGE_NAME)); + question_string.append(tr("Please, review your transaction. You can create and send this transaction or create a Partially Signed Transaction (PSBT), which you can save or copy and then sign with, e.g., an offline %1 wallet, or a PSBT-compatible hardware wallet.").arg(PACKAGE_NAME)); } else { /*: Text to prompt a user to review the details of the transaction they are attempting to send. */ question_string.append(tr("Please, review your transaction.")); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.gitignore(1 hunks)doc/build-freebsd.md(3 hunks)doc/build-netbsd.md(2 hunks)doc/build-unix.md(1 hunks)src/Makefile.bench.include(1 hunks)src/bench/logging.cpp(1 hunks)src/bitcoin-cli.cpp(1 hunks)src/qt/forms/optionsdialog.ui(1 hunks)src/qt/optionsdialog.cpp(1 hunks)src/qt/optionsmodel.cpp(3 hunks)src/qt/optionsmodel.h(3 hunks)src/qt/sendcoinsdialog.cpp(5 hunks)src/qt/sendcoinsdialog.h(1 hunks)src/rpc/mining.cpp(3 hunks)src/wallet/rpc/spend.cpp(3 hunks)test/functional/feature_anchors.py(1 hunks)test/functional/feature_config_args.py(1 hunks)test/functional/rpc_help.py(1 hunks)test/functional/rpc_scantxoutset.py(4 hunks)test/functional/test_framework/test_framework.py(5 hunks)test/functional/test_framework/util.py(2 hunks)test/functional/wallet_importprunedfunds.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/feature_anchors.pytest/functional/rpc_help.pytest/functional/rpc_scantxoutset.pytest/functional/feature_config_args.pytest/functional/wallet_importprunedfunds.pytest/functional/test_framework/test_framework.pytest/functional/test_framework/util.py
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/bitcoin-cli.cppsrc/qt/optionsmodel.cppsrc/qt/optionsdialog.cppsrc/qt/optionsmodel.hsrc/wallet/rpc/spend.cppsrc/qt/sendcoinsdialog.hsrc/bench/logging.cppsrc/qt/sendcoinsdialog.cppsrc/rpc/mining.cpp
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/build-unix.mddoc/build-freebsd.mddoc/build-netbsd.md
src/bench/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Performance benchmarks should be placed in src/bench/ and use nanobench
Files:
src/bench/logging.cpp
🧠 Learnings (6)
📓 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.
Learnt from: knst
PR: dashpay/dash#6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
PR: dashpay/dash#6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/wallet/rpc/spend.cpp
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to guix-build* : Do not make changes under any circumstances to build system files in guix-build*
Applied to files:
.gitignore
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/bench/**/*.{cpp,h,cc,cxx,hpp} : Performance benchmarks should be placed in src/bench/ and use nanobench
Applied to files:
src/bench/logging.cppsrc/Makefile.bench.include
📚 Learning: 2025-08-10T13:52:46.289Z
Learnt from: kwvg
PR: dashpay/dash#6666
File: test/functional/rpc_netinfo.py:73-81
Timestamp: 2025-08-10T13:52:46.289Z
Learning: In the Bitcoin/Dash test framework, `self.node.extra_args` contains the original startup arguments for a TestNode and is not modified by `test.restart_node()`. Each restart with new `extra_args` doesn't update the stored `self.node.extra_args`, so copying from it always provides a clean slate without accumulated modifications from previous restarts.
Applied to files:
test/functional/test_framework/test_framework.py
📚 Learning: 2025-08-11T17:16:36.654Z
Learnt from: PastaPastaPasta
PR: dashpay/dash#6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Applied to files:
src/Makefile.bench.include
🧬 Code graph analysis (5)
test/functional/rpc_scantxoutset.py (2)
src/rpc/blockchain.cpp (3)
out(1302-1302)scantxoutset(2385-2553)scantxoutset(2385-2385)test/functional/test_framework/util.py (2)
assert_equal(69-74)assert_raises_rpc_error(132-148)
test/functional/wallet_importprunedfunds.py (2)
test/functional/test_framework/messages.py (2)
CMerkleBlock(1112-1136)from_hex(220-227)test/functional/test_framework/util.py (1)
assert_raises_rpc_error(132-148)
src/qt/sendcoinsdialog.h (1)
src/qt/sendcoinsdialog.cpp (7)
SendConfirmationDialog(1061-1080)exec(1082-1087)exec(1082-1082)countDown(1089-1098)countDown(1089-1089)updateButtons(1100-1120)updateButtons(1100-1100)
src/qt/sendcoinsdialog.cpp (1)
src/qt/sendcoinsdialog.h (1)
m_psbt_button_text(136-137)
test/functional/test_framework/test_framework.py (1)
test/functional/test_framework/util.py (1)
initialize_datadir(374-381)
🪛 markdownlint-cli2 (0.18.1)
doc/build-unix.md
258-258: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
doc/build-freebsd.md
90-90: Heading levels should only increment by one level at a time
Expected: h4; Actual: h5
(MD001, heading-increment)
doc/build-netbsd.md
33-33: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
37-37: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
53-53: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
81-81: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
⏰ 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). (6)
- 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: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (49)
.gitignore (1)
166-167: LGTM — .gitignore correctly ignores top-level ci/scratch/ci/test/00_setup_env.sh:31 defaults BASE_SCRATCH_DIR to $BASE_ROOT_DIR/ci/scratch; git ls-files shows no tracked files under ci/scratch.
test/functional/wallet_importprunedfunds.py (3)
11-14: LGTM: Reuse framework helpers for Merkle proofs.Importing CMerkleBlock and from_hex from test_framework.messages is correct and avoids ad‑hoc parsing.
136-139: LGTM: Merkle root/header mismatch path covered.Flipping header.hashMerkleRoot to trigger the “Something wrong with merkleblock” path is appropriate.
140-143: LGTM: Block-not-found path exercised.Bumping nTime to alter the block hash (while keeping the merkle root consistent) is a clean way to hit “Block not found in chain”.
src/wallet/rpc/spend.cpp (2)
551-551: Correct changeAddress type/hint in fundrawtransaction — LGTMSwitching to Type::STR with DefaultHint "automatic" matches DecodeDestination parsing and the VSTR checks in FundTransaction. Accurate and less misleading than STR_HEX.
765-765: Correct change_address type/hint in send — LGTMType::STR with "automatic" aligns with the underscore option used in send and with DecodeDestination/VSTR handling.
doc/build-unix.md (1)
251-261: Clarify legacy-only BDB note and disable-wallet deps (LGTM).The “legacy wallet only needs BDB” note and the disable‑wallet section correctly state there’s no SQLite/Berkeley DB dependency when wallet is disabled. Anchor references resolve within this doc.
Please verify the “Disable-wallet mode” anchor renders correctly in your Markdown viewer.
doc/build-netbsd.md (3)
33-36: Good: Explicitly calling out gmake.Noting the requirement to use gmake will save users a common pitfall on NetBSD.
39-52: Descriptor wallet build steps look correct.sqlite3 prerequisite and configure flags are consistent with other OS docs.
81-89: Without‑wallet section is clear and consistent.The flags mirror the descriptor/legacy split and align with other platform guides.
doc/build-freebsd.md (4)
3-3: FreeBSD version bump note (LGTM).Clear update marker; helpful for readers tracking doc freshness.
33-33: Good clarification.Stating wallet functionality isn’t required to run dashd/dash‑qt reduces confusion.
37-41: Descriptor wallet deps and guidance (LGTM).sqlite3 requirement and link to descriptors are accurate.
43-49: Legacy wallet deps (LGTM).db5 as a legacy‑only dependency is correct; the note prevents unnecessary installs.
test/functional/feature_anchors.py (1)
23-23: Good: explicitly enabling autoconnect for this test.This aligns with the anchors reconnection behavior being tested and avoids the framework’s default connect=0 suppression.
test/functional/feature_config_args.py (1)
20-20: Good: autoconnect enabled for config/args networking tests.Required so open-connection and seeding behaviors (and related logs) occur as expected.
test/functional/test_framework/util.py (2)
374-379: Propagate disable_autoconnect into datadir initialization.Signature and usage look correct; default keeps previous behavior (autoconnect disabled) unless overridden.
384-384: Writing connect=0 when autoconnect is disabled is appropriate.
This reliably suppresses ThreadOpenConnections so tests can control topology deterministically.
Quick grep shows disable_autoconnect = False in test/functional/feature_config_args.py:20 and test/functional/feature_anchors.py:23.
Confirm previous-release binaries in the test matrix honor connect=0 the same way.test/functional/test_framework/test_framework.py (3)
145-147: Sane default: disable_autoconnect=True.Matches upstream intent to avoid incidental outbound connections in tests.
953-953: Correctly plumbs disable_autoconnect to datadir creation in all init paths.Cache creation, cache copy overwrite, and clean chain init all pass the flag consistently.
Also applies to: 1018-1018, 1026-1026
1525-1527: Good: autoconnect enabled for DashTestFramework masternode flows.Required for quorum formation and MN networking threads.
src/bench/logging.cpp (2)
1-49: Solid addition; compiles cleanly and aligns with bench harness.Uses TestingSetup on REGTEST and registers focused logging benchmarks. Matches placement/use of nanobench-backed bench framework.
10-18: Confirm log volume won’t bloat debug.log during benches.Variants other than LoggingNoFile will write many lines; depending on iterations this can balloon I/O and skew results. If the intent is to isolate formatting/category cost, consider also providing “NoFile” counterparts or capping iterations.
Please run benches locally and check debug.log size and runtime, or confirm that intended comparisons rely on file I/O being part of the measurement.
src/Makefile.bench.include (2)
39-39: Confirmed: src/bench/logging.cpp is an upstream backport — no non-backported entry requiredgit log shows a merge of bitcoin#18815 (commit 843ed5d), so the file originates from upstream Bitcoin and does not need to be added to test/util/data/non-backported.txt.
15-58: Missing bench sources in src/Makefile.bench.include — add files or remove entries to avoid build break.Missing files:
- bench/base58.cpp
- bench/bech32.cpp
- bench/bench_bitcoin.cpp
- bench/ccoins_caching.cpp
- bench/chacha20.cpp
- bench/crypto_hash.cpp
- bench/data.h
- bench/logging.cpp
- bench/lockedpool.cpp
- bench/merkle_root.cpp
- bench/nanobench.h
- bench/poly1305.cpp
- bench/prevector.cpp
- bench/rollingbloom.cpp
- bench/util_time.cpp
⛔ Skipped due to learnings
Learnt from: CR PR: dashpay/dash#0 File: CLAUDE.md:0-0 Timestamp: 2025-07-20T18:42:49.794Z Learning: Applies to src/bench/**/*.{cpp,h,cc,cxx,hpp} : Performance benchmarks should be placed in src/bench/ and use nanobenchtest/functional/rpc_scantxoutset.py (3)
17-17: LGTM (formatting-only changes).No behavioral impact.
Also applies to: 21-21, 134-135
89-114: LGTM (descriptor coverage and sums).Descriptor cases and totals align with expected UTXO tagging; no issues spotted.
116-119: LGTM (expected descriptors).Expected descriptor strings and checksums match.
src/bitcoin-cli.cpp (1)
82-87: Help text tweak LGTMAccurately clarifies -generate semantics and aligns with RPC arg names.
src/rpc/mining.cpp (2)
219-224: RPC help text updates LGTMClearer wording for generatetodescriptor, generatetoaddress, and generateblock; arg descriptions match behavior.
Also applies to: 256-261, 296-299
1108-1111: Confirm intent: always register as hiddenThese RPCs are now unconditionally in the “hidden” category. This removes the “Generating” help section. Ensure this matches desired regtest-only visibility and that help/tests are aligned (rpc_help.py updated).
test/functional/rpc_help.py (1)
103-111: Updated expected help categories LGTMRemoval of “Generating” matches the RPCs being registered as hidden.
src/qt/optionsdialog.cpp (1)
333-333: Wiring PSBT controls option LGTMMapper correctly binds m_enable_psbt_controls to OptionsModel::EnablePSBTControls.
src/qt/forms/optionsdialog.ui (1)
351-359: UI checkbox for PSBT controls LGTMGood label and tooltip; consistent with OptionsModel wiring.
src/qt/optionsmodel.h (3)
48-93: New OptionID and getter LGTMEnablePSBTControls added without reordering existing IDs; public getter provided.
112-112: Getter naming/placement LGTMConsistent with existing getters.
141-142: Private member for PSBT flag LGTMBacks the new option.
src/qt/optionsmodel.cpp (3)
160-165: Initialize and persist PSBT controls setting LGTMDefaults to false; respects ENABLE_WALLET.
559-561: Expose PSBT controls via data() LGTMReturns QSettings value; consistent with other settings.
810-813: Persist PSBT controls in setData() LGTMUpdates cached member and QSettings.
src/qt/sendcoinsdialog.h (2)
122-123: Constructor overload and defaults LGTMSupports PSBT flow with delay and explicit send/unsigned controls.
Please confirm all call sites were updated to the new overload where needed and that default delay (now 3s) is acceptable UX-wise.
127-137: Button state rename and new members LGTMupdateButtons reflects both Send/PSBT states; default texts set with tr().
src/qt/sendcoinsdialog.cpp (7)
483-485: Constructor wiring LGTM.The new SendConfirmationDialog signature is used correctly; enable_send and PSBT controls are passed as intended.
489-493: Early-exit handling LGTM.Cancel path restores state via fNewRecipientAllowed and returns cleanly.
546-550: Send path assertion LGTM.The Yes path is correctly gated by private keys being enabled.
1061-1081: Dialog initialization LGTM.Button selection logic and Save button opt-in are coherent.
1092-1093: Countdown update LGTM.Refreshing button states per tick is correct.
1100-1121: Button gating LGTM.Enable/disable and label updates for Yes/Save align with the new flow.
1084-1086: Fix compile/runtime: avoid 1s chrono literal and Qt version pitfalls.1s requires std::chrono_literals and Qt >= 5.14 overloads; this can break builds. Use milliseconds to be safe.
Apply this diff:
- countDownTimer.start(1s); + countDownTimer.start(1000);⛔ Skipped due to learnings
Learnt from: kwvg PR: dashpay/dash#6815 File: src/chainlock/chainlock.h:68-69 Timestamp: 2025-08-13T16:10:32.972Z Learning: The Dash codebase extensively uses std::chrono_literals throughout, including in header files. Chrono literals like 0s, 1min, etc. are acceptable and preferred over explicit constructors like std::chrono::seconds::zero().
| assert_raises_rpc_error(-8, "End of range is too high", self.nodes[0].scantxoutset, "start", [{"desc": "desc", "range": -1}]) | ||
| assert_raises_rpc_error(-8, "Range should be greater or equal than 0", self.nodes[0].scantxoutset, "start", [{"desc": "desc", "range": [-1, 10]}]) | ||
| assert_raises_rpc_error(-8, "End of range is too high", self.nodes[0].scantxoutset, "start", [{"desc": "desc", "range": [(2 << 31 + 1) - 1000000, (2 << 31 + 1)]}]) | ||
| assert_raises_rpc_error(-8, "Range specified as [begin,end] must not have begin after end", self.nodes[0].scantxoutset, "start", [{"desc": "desc", "range": [2, 1]}]) | ||
| assert_raises_rpc_error(-8, "Range is too large", self.nodes[0].scantxoutset, "start", [{"desc": "desc", "range": [0, 1000001]}]) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Fix operator precedence bug in range limit expression.
In Python, + binds tighter than <<, so 2 << 31 + 1 == 2 << 32. Use 2**31 + 1 (or (1 << 31) + 1) as intended.
- assert_raises_rpc_error(-8, "End of range is too high", self.nodes[0].scantxoutset, "start", [{"desc": "desc", "range": [(2 << 31 + 1) - 1000000, (2 << 31 + 1)]}])
+ assert_raises_rpc_error(-8, "End of range is too high", self.nodes[0].scantxoutset, "start", [{"desc": "desc", "range": [(2**31 + 1) - 1000000, (2**31 + 1)]}])🏁 Script executed:
#!/bin/bash
# Scan repo for similar precedence patterns to fix consistently
rg -nP -C2 '\b\d+\s*<<\s*31\s*\+\s*1\b'Length of output: 2130
Fix operator-precedence bug in range limit expression
In Python, + binds tighter than <<; replace 2 << 31 + 1 with 2**31 + 1 (or (1 << 31) + 1).
- assert_raises_rpc_error(-8, "End of range is too high", self.nodes[0].scantxoutset, "start", [{"desc": "desc", "range": [(2 << 31 + 1) - 1000000, (2 << 31 + 1)]}])
+ assert_raises_rpc_error(-8, "End of range is too high", self.nodes[0].scantxoutset, "start", [{"desc": "desc", "range": [(2**31 + 1) - 1000000, (2**31 + 1)]}])Also update the same expression in:
- test/functional/wallet_importmulti.py:461
- test/functional/wallet_importdescriptors.py:246
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert_raises_rpc_error(-8, "End of range is too high", self.nodes[0].scantxoutset, "start", [{"desc": "desc", "range": -1}]) | |
| assert_raises_rpc_error(-8, "Range should be greater or equal than 0", self.nodes[0].scantxoutset, "start", [{"desc": "desc", "range": [-1, 10]}]) | |
| assert_raises_rpc_error(-8, "End of range is too high", self.nodes[0].scantxoutset, "start", [{"desc": "desc", "range": [(2 << 31 + 1) - 1000000, (2 << 31 + 1)]}]) | |
| assert_raises_rpc_error(-8, "Range specified as [begin,end] must not have begin after end", self.nodes[0].scantxoutset, "start", [{"desc": "desc", "range": [2, 1]}]) | |
| assert_raises_rpc_error(-8, "Range is too large", self.nodes[0].scantxoutset, "start", [{"desc": "desc", "range": [0, 1000001]}]) | |
| assert_raises_rpc_error(-8, "End of range is too high", self.nodes[0].scantxoutset, "start", [{"desc": "desc", "range": -1}]) | |
| assert_raises_rpc_error(-8, "Range should be greater or equal than 0", self.nodes[0].scantxoutset, "start", [{"desc": "desc", "range": [-1, 10]}]) | |
| assert_raises_rpc_error(-8, "End of range is too high", self.nodes[0].scantxoutset, "start", [{"desc": "desc", "range": [(2**31 + 1) - 1000000, (2**31 + 1)]}]) | |
| assert_raises_rpc_error(-8, "Range specified as [begin,end] must not have begin after end", self.nodes[0].scantxoutset, "start", [{"desc": "desc", "range": [2, 1]}]) | |
| assert_raises_rpc_error(-8, "Range is too large", self.nodes[0].scantxoutset, "start", [{"desc": "desc", "range": [0, 1000001]}]) |
🤖 Prompt for AI Agents
In test/functional/rpc_scantxoutset.py around lines 80-85, the expression (2 <<
31 + 1) suffers from operator-precedence: + binds tighter than <<, producing the
wrong value; change it to 2**31 + 1 (or (1 << 31) + 1) to compute the intended
large integer, and make the identical replacement in
test/functional/wallet_importmulti.py at line ~461 and
test/functional/wallet_importdescriptors.py at line ~246 so all range-limit
checks use the correct value.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/lint/lint-format-strings.py (1)
80-81: Escape regex dots to prevent unintended matchesThe unescaped “.” in
strprintf.cppandclientversion.cppwill match any character. Make it a raw string and escape the dots to avoid accidental overmatching.- if not re.search('^src/(dashbls|leveldb|secp256k1|minisketch|tinyformat|test/fuzz/strprintf.cpp|clientversion.cpp)', matching_file): + if not re.search(r'^src/(dashbls|leveldb|secp256k1|minisketch|tinyformat|test/fuzz/strprintf\.cpp|clientversion\.cpp)', matching_file):contrib/devtools/gen-manpages.py (1)
49-53: Harden copyright extractionUse splitlines() here too and guard against empty output to avoid IndexError on unexpected binary output.
Apply:
- # remaining lines are copyright - copyright = r.stdout.split('\n')[1:] - assert copyright[0].startswith('Copyright (C)') + # remaining lines are copyright + copyright = r.stdout.splitlines()[1:] + assert copyright and copyright[0].startswith('Copyright (C)')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
contrib/devtools/gen-manpages.py(1 hunks)src/bitcoin-cli.cpp(2 hunks)src/bitcoin-tx.cpp(1 hunks)src/bitcoin-wallet.cpp(1 hunks)src/bitcoind.cpp(1 hunks)src/clientversion.cpp(2 hunks)src/clientversion.h(1 hunks)src/init.cpp(0 hunks)src/init.h(0 hunks)src/util/system.cpp(0 hunks)src/util/system.h(0 hunks)test/lint/lint-format-strings.py(1 hunks)
💤 Files with no reviewable changes (4)
- src/util/system.h
- src/init.h
- src/init.cpp
- src/util/system.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/bitcoin-cli.cpp
🧰 Additional context used
📓 Path-based instructions (2)
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/bitcoind.cppsrc/clientversion.hsrc/clientversion.cppsrc/bitcoin-tx.cppsrc/bitcoin-wallet.cpp
contrib/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the contrib directory (contributed scripts)
Files:
contrib/devtools/gen-manpages.py
🧠 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.
Learnt from: knst
PR: dashpay/dash#6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
🧬 Code graph analysis (4)
src/bitcoind.cpp (1)
src/clientversion.cpp (2)
LicenseInfo(97-112)LicenseInfo(97-97)
src/clientversion.h (1)
src/clientversion.cpp (4)
CopyrightHolders(81-95)CopyrightHolders(81-81)LicenseInfo(97-112)LicenseInfo(97-97)
src/bitcoin-tx.cpp (2)
src/util/strencodings.cpp (2)
FormatParagraph(284-324)FormatParagraph(284-284)src/clientversion.cpp (2)
LicenseInfo(97-112)LicenseInfo(97-97)
src/bitcoin-wallet.cpp (1)
src/clientversion.cpp (2)
LicenseInfo(97-112)LicenseInfo(97-97)
⏰ 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: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (10)
contrib/devtools/gen-manpages.py (4)
44-44: Good: use splitlines() for robust version-line parsingUsing splitlines() avoids platform newline issues.
54-55: Dirty tree check update LGTMTuple-unpacking change is consistent with versions now holding three elements.
62-65: Footer generation with first binary’s copyright is acceptableReusing versions[0][2] is fine assuming uniform license output across binaries.
68-68: 3‑tuple unpacking across generation loop is consistentMatches the versions structure established above.
src/clientversion.cpp (2)
81-95: CopyrightHolders(): translation-aware and defensiveRuntime checks ensure Dash/Bitcoin credits aren’t lost in translation; implementation is straightforward.
97-112: LicenseInfo(): centralized, translatable license blockContent and formatting match the intended -version output across tools.
src/bitcoin-tx.cpp (1)
110-112: Correct -version behavior: FormatParagraph supports default args so single-arg call is validHeader (src/util/strencodings.h:246) declares
std::string FormatParagraph(std::string_view in, size_t width = 79, size_t indent = 0);, soFormatParagraph(LicenseInfo())is valid and no change needed.src/bitcoind.cpp (1)
139-145: Version/help flow aligned with centralized LicenseInfo — LGTMFormatParagraph is declared with default args (src/util/strencodings.h:246) and implemented without defaults (src/util/strencodings.cpp:284); usage is valid — no action required.
src/bitcoin-wallet.cpp (1)
63-65: Wallet -version path uses LicenseInfo; FormatParagraph defaults verifiedFormatParagraph is declared with defaults (width=79, indent=0) in src/util/strencodings.h and implemented in src/util/strencodings.cpp; calling FormatParagraph(LicenseInfo()) is valid.
src/clientversion.h (1)
44-48: Approve — license/copyright declarations centralized; no duplicates foundSearch shows the declarations only in src/clientversion.h and implementations only in src/clientversion.cpp; no other matches (e.g., init.* or util/system.*).
…ationDialog BACKPORT NOTE: Formatting in src/qt/optionsmodel.h has been changed in bitcoin#3267. These changes has not been backported on time; done with this backport 742918c qt: hide Create Unsigned button behind an expert mode option (Andrew Chow) 5c3b800 qt: Add Create Unsigned button to SendConfirmationDialog (Andrew Chow) Pull request description: Instead of having different buttons or changing button behavior for making a PSBT, just have SendConfirmationDialog return whether the user wants a PSBT or a broadcasted transaction. Since this dialog is used by both the bumpFeeAction and the SendCoinsDialog, changes to both to support the different behavior is needed. They will check the return value of the SendConfirmationDialog for whether a PSBT needs to be created instead of checking whether private keys are disabled. Strings used in this dialog are being slightly modified to work with both private keys enabled and disabled wallets. Moved from bitcoin#18789 ACKs for top commit: jarolrod: ACK 742918c ryanofsky: Code review ACK 742918c. Just suggested changes since last review. Looks great! hebasto: ACK 742918c, tested on Linux Mint 20.2 (Qt 5.12.8). Tree-SHA512: dd29f4364c7b4f15befe8fe63257b26187918786b005e0f8336183270b1a162680b93f6ced60f0285c6e607c084cc0d24950fc68a8f9c056521ede614041be66
8336a06 doc: remove Boost LDFLAGS from netBSD build docs (fanquake) Pull request description: We no-longer link against any Boost libs, so we shouldn't need to use any Boost linker flags. ACKs for top commit: hebasto: ACK 8336a06, I have reviewed the code and it looks OK, I agree it can be merged. Also verified that there is no other usage of `BOOST_LDFLAGS` in our codebase or documentation. Tree-SHA512: b7814d10cee789903cb3c613631e184a72f5766cda85261b5f99f9ac207348a2a49c92494c8c1d50163494f6b755c503cf51bf083b31f564dae1b0f493c54c2e
…s in wallet RPC help e827202 doc: Use human-friendly DefaultHint for change_address/changeAddress in wallet RPC help (Luke Dashjr) 9d5e693 Bugfix: doc: Correct type of change_address/changeAddress in wallet RPC help (STR, not STR_HEX) (Luke Dashjr) Pull request description: ACKs for top commit: achow101: ACK e827202 Tree-SHA512: da4db2b241160c93ea66f8c572c69d4688f52a5fd8c32b66b1192925fcb360baf91be9771eaca178f5b08e1920559174260ed57caddcffade48156ec0c83c0bc
7573789 test: check for importprunedfunds RPC errors (Sebastian Falbesoner) Pull request description: This PR adds missing test coverage for the following errors of the `importprunedfunds` RPC: https://github.com/bitcoin/bitcoin/blob/7003b6ab24f6adfffd71d7b7d4182afde52ff859/src/wallet/rpc/backup.cpp#L320-L322 https://github.com/bitcoin/bitcoin/blob/7003b6ab24f6adfffd71d7b7d4182afde52ff859/src/wallet/rpc/backup.cpp#L332-L334 https://github.com/bitcoin/bitcoin/blob/7003b6ab24f6adfffd71d7b7d4182afde52ff859/src/wallet/rpc/backup.cpp#L338-L340 https://github.com/bitcoin/bitcoin/blob/7003b6ab24f6adfffd71d7b7d4182afde52ff859/src/wallet/rpc/backup.cpp#L343-L345 ACKs for top commit: MarcoFalke: review ACK 7573789 Tree-SHA512: b054520d102e5940bdeed2456ca644e91afb187d169b751b1262ce34480e4e9fbe1616ab184a78777c184350dced23508c3d367ed5825cab78bb5ad687fd7dac
fafe06c bench: Sort bench_bench_bitcoin_SOURCES (MarcoFalke) fa31dc9 bench: Add logging benchmark (MarcoFalke) Pull request description: Might make finding performance bottlenecks or regressions (bitcoin#17218) easier. For example, fuzzing relies on disabled logging to be as fast as possible. ACKs for top commit: dergoegge: ACK fafe06c Tree-SHA512: dd858f3234a4dfb00bd7dec4398eb076370a4b9746aa24eecee7da86f6882398a2d086e5ab0b7c9f7321abcb135e7ffc54cc78e60d18b90379c6dba6d613b3f7
…egacy wallet in build-unix.md 307215b doc: clarify that BDB is only required for the legacy wallet (fanquake) Pull request description: ACKs for top commit: gruve-p: ACK bitcoin@307215b darosior: ACK 307215b Tree-SHA512: d77d013831e3e76a596603fbea80958c1cf4d3e65591debd66cd4f5ff77300dae7e81df8e7d79f3f4d2e561bb3e8090434b704586e2568ca8e89ba8196de173c
7027595 add ci/scratch dir to gitignore (josibake) Pull request description: Not sure if I'm missing some context as to why this isn't already ignored? ACKs for top commit: hebasto: re-ACK 7027595, tested on Ubuntu 22.04. Tree-SHA512: 1f13041cb27cd3687619105ac1bb3af4c31d000fcd98e5f84160c34649de532fcd8b98cb8a5bed0ba68e25b3bb344f669ea3567b9c9d86cf73386ddf276f292e
…n build-netbsd.md 7ac7198 doc: mention that BDB is for the legacy wallet in build-netbsd.md (fanquake) Pull request description: Re-order legacy and descriptor wallet section. Add an additional configure example. NetBSD version of bitcoin#23446. ACKs for top commit: shaavan: ACK 7ac7198 Tree-SHA512: 5c8218424a6b12e9eee00b44dd93f9fe95fd9afa468563167feb255663720a84b55e75850985cfae3ca288a6a76e17c00ccce60b8180f92875eeaee2c9afa843
…n build-freebsd.md b5ba3b5 doc: mention that BDB is for the legacy wallet in build-freebsd.md (fanquake) Pull request description: Re-order legacy and descriptor wallet section. Remove prelude that pointlessly repeats the same info. Cleanup configure examples. FreeBSD version of bitcoin#23446. ACKs for top commit: shaavan: ACK b5ba3b5 Tree-SHA512: ed85acee5c1b933b57d038d144768da1c4b1f1cfd441d9191353b82d50af16adb10aabb3b4661dc9aee54405ab3af2b800e39f9973261a041b2cf0db8675b5c4
…ion` BACKPORT NOTE: Please, notice, that there are added a new exception for clientversion.cpp It is temporary measure so far as it will be removed anyway by bitcoin-31174 5a89bed contrib: address gen-manpages feedback from bitcoin#24263 (fanquake) 2618fb8 Output license info when binaries are passed -version (fanquake) 4c3e3c5 refactor: shift CopyrightHolders() and LicenseInfo() to clientversion.cpp (fanquake) Pull request description: Addresses a review comment from bitcoin#24263, and addresses the [comment](bitcoin#24263 (comment)) where it was pointed out that we are inconsistent with emitting our copyright. After this change, the copyright is always emitted with `-version`, rather than `-help`, i.e: ```bash bitcoind -version Bitcoin Core version v22.99.0-fc1f355913f6-dirty Copyright (C) 2009-2022 The Bitcoin Core developers Please contribute if you find Bitcoin Core useful. Visit <https://bitcoincore.org/> for further information about the software. The source code is available from <https://github.com/bitcoin/bitcoin>. This is experimental software. Distributed under the MIT software license, see the accompanying file COPYING or <https://opensource.org/licenses/MIT> ``` The info is also added to binaries other than `bitcoind`/`bitcoin-qt`. This change also prevents duplicate copyright info appearing in the `bitcoind` man page. ACKs for top commit: laanwj: Tested ACK 5a89bed Tree-SHA512: 0ac2a1adf9e9de0c3206f35837008e3f93eaf15b193736203d71609273f0887cca20b8a90972cb9f941ebd62b330d61a0cbb5fb1b7a7f2dbc715ed8a0c1569d9
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/qt/sendcoinsdialog.cpp (1)
531-535: Cancel-save early return leaves the dialog in a locked stateIf the user cancels the file dialog, fNewRecipientAllowed remains false, blocking further recipient edits until the dialog is closed. Set it back before returning.
- if (filename.isEmpty()) { - return; - } + if (filename.isEmpty()) { + fNewRecipientAllowed = true; + return; + }
🧹 Nitpick comments (10)
doc/build-unix.md (1)
255-261: Use fenced code block for the configure line (MD046).Switch the indented code line to fenced to satisfy markdownlint.
Apply this diff:
- ./configure --disable-wallet +```sh +./configure --disable-wallet +```doc/build-netbsd.md (2)
55-60: Fix grammar and NetBSD packaging term (“ports” → “pkgsrc”).Minor textual corrections.
Apply this diff:
-BerkeleyDB is use for legacy wallet functionality. +Berkeley DB is used for legacy wallet functionality. @@ -from ports. +from pkgsrc (binary packages).
81-89: “Without wallet” section LGTM; consider heading style normalization.markdownlint MD003 flagged heading style elsewhere in this file; consider converting these atx headings to setext or updating the lint config for consistency.
src/clientversion.cpp (1)
81-95: Avoid showing a redundant year range when start == endIf nStartYear == nEndYear, this prints "YYYY-YYYY". Prefer a single year for polish.
Apply this diff:
- const auto copyright_devs = strprintf(_(COPYRIGHT_HOLDERS).translated, COPYRIGHT_HOLDERS_SUBSTITUTION); - std::string strCopyrightHolders = strPrefix + strprintf(" %u-%u ", nStartYear, nEndYear) + copyright_devs; + const auto copyright_devs = strprintf(_(COPYRIGHT_HOLDERS).translated, COPYRIGHT_HOLDERS_SUBSTITUTION); + const bool single_year = nStartYear == nEndYear; + const std::string year_span = single_year + ? strprintf(" %u ", nEndYear) + : strprintf(" %u-%u ", nStartYear, nEndYear); + std::string strCopyrightHolders = strPrefix + year_span + copyright_devs;src/qt/optionsmodel.cpp (1)
559-561: Use cached member in data() for consistencyReturn the cached member to mirror other settings (e.g., SubFeeFromAmount) and avoid redundant QSettings reads.
- case EnablePSBTControls: - return settings.value("enable_psbt_controls"); + case EnablePSBTControls: + return m_enable_psbt_controls;src/qt/sendcoinsdialog.cpp (3)
367-387: Fix PSBT-only messaging condition when private keys are disabledThe current stub and condition produce misleading text (talks about both sending and PSBT even when send is disabled). Gate on privateKeysDisabled() now; external signer can be added later without wrong messaging.
- // TODO: re-enable it when external signer will be backported - // if (model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner()) { - const bool external_signer_available{false}; - if (external_signer_available) { - /*: Text to inform a user attempting to create a transaction of their current options. At this stage, - a user can only create a PSBT. This string is displayed when private keys are disabled and an external - signer is not available. */ - question_string.append(tr("Please, review your transaction proposal. This will produce a Partially Signed Bitcoin Transaction (PSBT) which you can save or copy and then sign with e.g. an offline %1 wallet, or a PSBT-compatible hardware wallet.").arg(PACKAGE_NAME)); - } else if (model->getOptionsModel()->getEnablePSBTControls()) { + if (model->wallet().privateKeysDisabled()) { + /*: Private keys are disabled: user can only create a PSBT. */ + question_string.append(tr("Please, review your transaction proposal. This will produce a Partially Signed Transaction (PSBT) which you can save or copy and then sign with e.g. an offline %1 wallet, or a PSBT-compatible hardware wallet.").arg(PACKAGE_NAME)); + } else if (model->getOptionsModel()->getEnablePSBTControls()) { /*: Text to inform a user attempting to create a transaction of their current options. At this stage, a user can send their transaction or create a PSBT. This string is displayed when both private keys and PSBT controls are enabled. */ - question_string.append(tr("Please, review your transaction. You can create and send this transaction or create a Partially Signed Bitcoin Transaction (PSBT), which you can save or copy and then sign with, e.g., an offline %1 wallet, or a PSBT-compatible hardware wallet.").arg(PACKAGE_NAME)); + question_string.append(tr("Please, review your transaction. You can create and send this transaction or create a Partially Signed Transaction (PSBT), which you can save or copy and then sign with, e.g., an offline %1 wallet, or a PSBT-compatible hardware wallet.").arg(PACKAGE_NAME)); } else { /*: Text to prompt a user to review the details of the transaction they are attempting to send. */ question_string.append(tr("Please, review your transaction.")); }
509-513: Wrap user-facing strings with tr() for i18nLocalize the PSBT clipboard/save messages.
- msgBox.setText("Unsigned Transaction"); - msgBox.setInformativeText("The PSBT has been copied to the clipboard. You can also save it."); + msgBox.setText(tr("Unsigned transaction")); + msgBox.setInformativeText(tr("The PSBT has been copied to the clipboard. You can also save it.")); @@ - Q_EMIT message(tr("PSBT saved"), "PSBT saved to disk", CClientUIInterface::MSG_INFORMATION); + Q_EMIT message(tr("PSBT saved"), tr("PSBT saved to disk"), CClientUIInterface::MSG_INFORMATION);Also applies to: 538-539
501-504: Avoid asserting on a user path; handle PSBT creation failure gracefullyAsserting here can terminate the GUI. Prefer an error message and a safe exit path.
- bool complete = false; - const TransactionError err = model->wallet().fillPSBT(SIGHASH_ALL, false /* sign */, true /* bip32derivs */, nullptr, psbtx, complete); - assert(!complete); - assert(err == TransactionError::OK); + bool complete = false; + const TransactionError err = model->wallet().fillPSBT(SIGHASH_ALL, /*sign=*/false, /*bip32derivs=*/true, nullptr, psbtx, complete); + if (complete || err != TransactionError::OK) { + Q_EMIT message(tr("PSBT creation failed"), tr("Unable to create a PSBT for this transaction."), CClientUIInterface::MSG_ERROR); + fNewRecipientAllowed = true; + return; + }contrib/devtools/gen-manpages.py (2)
49-53: Use splitlines() consistently and guard empty output.Minor robustness tweak to avoid CRLF pitfalls and IndexError when no second line.
Apply:
- # remaining lines are copyright - copyright = r.stdout.split('\n')[1:] - assert copyright[0].startswith('Copyright (C)') + # remaining lines are copyright + copyright = r.stdout.splitlines()[1:] + assert copyright and copyright[0].startswith('Copyright (C)') - versions.append((abspath, verstr, copyright)) + versions.append((abspath, verstr, copyright))
62-65: Verify that all binaries share the same COPYRIGHT footer and append a final newlinecontrib/devtools/gen-manpages.py unconditionally uses versions[0][2] for the footer and writes it with .strip(), so it does not verify other binaries and removes the trailing newline.
Optional small fix (keep):
- footer.write('\n'.join(versions[0][2]).strip()) + footer.write('\n'.join(versions[0][2]).strip() + '\n')Also add a runtime check after populating versions to warn/exit if any versions[*][2] != versions[0][2] to avoid generating manpages with incorrect footers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
.gitignore(1 hunks)contrib/devtools/gen-manpages.py(1 hunks)doc/build-freebsd.md(3 hunks)doc/build-netbsd.md(2 hunks)doc/build-unix.md(1 hunks)src/Makefile.bench.include(1 hunks)src/bench/logging.cpp(1 hunks)src/bitcoin-cli.cpp(2 hunks)src/bitcoin-tx.cpp(1 hunks)src/bitcoin-wallet.cpp(1 hunks)src/bitcoind.cpp(1 hunks)src/clientversion.cpp(2 hunks)src/clientversion.h(1 hunks)src/init.cpp(0 hunks)src/init.h(0 hunks)src/qt/forms/optionsdialog.ui(1 hunks)src/qt/optionsdialog.cpp(1 hunks)src/qt/optionsmodel.cpp(3 hunks)src/qt/optionsmodel.h(3 hunks)src/qt/sendcoinsdialog.cpp(5 hunks)src/qt/sendcoinsdialog.h(1 hunks)src/util/system.cpp(0 hunks)src/util/system.h(0 hunks)src/wallet/rpc/spend.cpp(3 hunks)test/functional/wallet_importprunedfunds.py(2 hunks)test/lint/lint-format-strings.py(1 hunks)
💤 Files with no reviewable changes (4)
- src/init.h
- src/init.cpp
- src/util/system.h
- src/util/system.cpp
🚧 Files skipped from review as they are similar to previous changes (14)
- src/qt/optionsdialog.cpp
- src/wallet/rpc/spend.cpp
- src/qt/optionsmodel.h
- src/bench/logging.cpp
- src/bitcoin-wallet.cpp
- src/bitcoin-tx.cpp
- src/bitcoin-cli.cpp
- src/bitcoind.cpp
- src/clientversion.h
- src/qt/forms/optionsdialog.ui
- .gitignore
- test/lint/lint-format-strings.py
- test/functional/wallet_importprunedfunds.py
- src/qt/sendcoinsdialog.h
🧰 Additional context used
📓 Path-based instructions (3)
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/build-freebsd.mddoc/build-netbsd.mddoc/build-unix.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/qt/optionsmodel.cppsrc/clientversion.cppsrc/qt/sendcoinsdialog.cpp
contrib/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the contrib directory (contributed scripts)
Files:
contrib/devtools/gen-manpages.py
🧠 Learnings (3)
📓 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.
Learnt from: knst
PR: dashpay/dash#6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
📚 Learning: 2025-08-11T17:16:36.654Z
Learnt from: PastaPastaPasta
PR: dashpay/dash#6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Applied to files:
src/Makefile.bench.include
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/bench/**/*.{cpp,h,cc,cxx,hpp} : Performance benchmarks should be placed in src/bench/ and use nanobench
Applied to files:
src/Makefile.bench.include
🧬 Code graph analysis (1)
src/qt/sendcoinsdialog.cpp (1)
src/qt/sendcoinsdialog.h (1)
m_psbt_button_text(136-137)
🪛 markdownlint-cli2 (0.18.1)
doc/build-freebsd.md
33-33: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
37-37: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
doc/build-netbsd.md
33-33: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
37-37: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
53-53: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
81-81: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
doc/build-unix.md
258-258: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
⏰ 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). (6)
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
🔇 Additional comments (24)
doc/build-freebsd.md (7)
3-3: Confirm target FreeBSD release scope (12.3 is EOL).If this doc intentionally targets 12.3 for historical reproducibility, please note that; otherwise consider updating to a currently supported series (13.x/14.x).
33-33: Good clarification: wallet not required to run node/GUI.
37-41: Package name check for FreeBSD.Verify
sqlite3is the correct pkg name on supported FreeBSD versions (sometimes names vary by Python ABI or port flavor).
43-49: Legacy wallet dependency note LGTM.Clear that db5 is only needed for legacy wallets.
88-96: Descriptor+GUI configure example LGTM.Flags are consistent with descriptor-only wallet builds and GUI enabled.
97-106: Verify BDB include/lib paths and lib name on FreeBSD.Confirm
-ldb_cxx-5and-I/usr/local/include/db5match FreeBSD’s db5 pkg on supported releases.
35-49: markdownlint MD003 (heading style) reported nearby.The linter flagged heading style inconsistencies around these sections. Either normalize to setext where required or adjust the rule. Please run markdownlint to confirm.
doc/build-unix.md (2)
251-251: Note reads well and is accurate.
260-260: Dependency clarification LGTM.doc/build-netbsd.md (3)
33-36: gmake emphasis LGTM.
37-51: Descriptor wallet steps LGTM; verify pkg name.
pkgin install sqlite3is likely correct; please confirm against supported NetBSD versions.
68-69: BDB_PREFIX export example LGTM.src/clientversion.cpp (3)
6-6: LGTM: include for translationsIncluding util/translation.h is correct for _ and bilingual_str usage.
10-12: LGTM: required STL headerssstream/string/vector are needed for FormatSubVersion and related uses.
97-112: LicenseInfo relocation looks good — manual verification requiredAutomated check failed (rg: "src/**/clientversion.h: No such file or directory"); string assembly/i18n look fine.
- Confirm declaration was moved to clientversion.h and removed from init headers (init.h / init*).
- Ensure all callers of LicenseInfo include clientversion.h (not init headers).
- Verify a single implementation remains in clientversion.cpp.
src/Makefile.bench.include (1)
14-58: Bench sources expanded: LGTM — verified present, no duplicatesAll 44 listed bench files (after expanding RAW_BENCH_FILES) were found in the repo (mostly under src/bench); no missing files or duplicate entries detected.
src/qt/optionsmodel.cpp (2)
160-164: Initialize enable_psbt_controls — LGTMDefaults and caching look correct and match the rest of the Init pattern.
810-813: Persist + cache enable_psbt_controls in setData — LGTMSetter updates both the cache and QSettings coherently.
src/qt/sendcoinsdialog.cpp (3)
484-486: Constructor call updated for SendConfirmationDialog — LGTMNew parameters (enable_send, always_show_unsigned) are used correctly with wallet state and options.
1062-1081: PSBT-aware confirmation dialog wiring — LGTMConstructor updates (enable_send, always_show_unsigned) and Save button integration are correct.
1101-1121: Countdown and button state handling — LGTMDisables both actions during countdown and correctly enables only valid actions afterward.
contrib/devtools/gen-manpages.py (3)
44-44: Good fix: splitlines() is more robust.Using splitlines()[0] avoids platform-specific newline issues and trailing blanks.
54-54: LGTM: dirty-tree check with tuple unpacking.Clear and correct.
68-68: Nice: tuple unpacking keeps the loop clear.Ignoring the copyright payload here improves readability.
Issue being fixed or feature implemented
Regular backports from bitcoin v23, v24
What was done?
See commits
How Has This Been Tested?
Run unit & functional tests
Breaking Changes
RPCs
generatetoaddress,generatetodescriptor,generateblockare hidden now so far as they are available only for RegTestChecklist: