backport: bitcoin#24408, #24959, #25108, #25184, #25256, #25276, #25414, bitcoin-core/gui#583 #6878
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
Walkthrough
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/rpc/masternode.cpp (1)
681-704: Fix clang-format regression in command registration block.CI is currently failing with
Clang format differences detected in masternode.cpp. Please run the repository’s clang-format tooling (e.g../contrib/devtools/clang-format-diff.py -p1) on this hunk so the style check passes.src/test/fuzz/rpc.cpp (1)
128-141: Remove duplicate "gettxspendingprevout" entry.The RPC command "gettxspendingprevout" appears twice in the
RPC_COMMANDS_SAFE_FOR_FUZZINGlist at lines 128 and 141. This is a duplicate entry that should be removed.Apply this diff to remove the duplicate:
"getmempoolentry", - "gettxspendingprevout", "getmempoolinfo",src/rpc/net.cpp (2)
317-319: Fix RPC example commas in addnodeMissing comma between arguments in HelpExampleRpc.
Apply:
- + HelpExampleRpc("addnode", "\"192.168.0.6:9999\", \"onetry\" true") + + HelpExampleRpc("addnode", "\"192.168.0.6:9999\", \"onetry\", true")
380-382: Fix RPC example commas in addconnectionHelpExampleRpc is missing commas between arguments.
Apply:
- + HelpExampleRpc("addconnection", "\"192.168.0.6:8333\" \"outbound-full-relay\" true") + + HelpExampleRpc("addconnection", "\"192.168.0.6:8333\", \"outbound-full-relay\", true")
🧹 Nitpick comments (2)
src/rpc/net.cpp (1)
508-508: Remove stray semicolonMinor cleanup; no behavior change.
- const CConnman& connman = EnsureConnman(node);; + const CConnman& connman = EnsureConnman(node);src/sync.h (1)
323-341: Uniform MaybeCheckNotHeld helpers added
- Mutex: negative capability enforced (not already held).
- Global/Recursive/Shared: LOCKS_EXCLUDED applied.
Note: The comment for RecursiveMutex says “okay to already hold” but the annotation forbids holding; if intentional to surface reentrancy via TSA, fine. Otherwise, consider aligning the comment in a follow-up to avoid confusion. Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (85)
doc/build-unix.md(1 hunks)doc/developer-notes.md(2 hunks)doc/release-notes/release-notes-24408.md(1 hunks)src/.clang-tidy(1 hunks)src/base58.h(0 hunks)src/bench/checkqueue.cpp(1 hunks)src/bench/prevector.cpp(1 hunks)src/bitcoin-cli.cpp(2 hunks)src/core_io.h(0 hunks)src/httpserver.cpp(2 hunks)src/index/txindex.cpp(1 hunks)src/init.cpp(1 hunks)src/logging.cpp(1 hunks)src/net.cpp(2 hunks)src/net.h(1 hunks)src/net_processing.cpp(1 hunks)src/netaddress.cpp(1 hunks)src/netaddress.h(0 hunks)src/netbase.cpp(1 hunks)src/node/context.cpp(1 hunks)src/node/transaction.h(0 hunks)src/policy/fees.cpp(1 hunks)src/psbt.h(0 hunks)src/qt/addresstablemodel.cpp(1 hunks)src/qt/bantablemodel.cpp(1 hunks)src/qt/clientmodel.h(1 hunks)src/qt/notificator.cpp(1 hunks)src/qt/overviewpage.cpp(2 hunks)src/qt/paymentserver.cpp(1 hunks)src/qt/peertablemodel.cpp(1 hunks)src/qt/recentrequeststablemodel.cpp(1 hunks)src/qt/rpcconsole.cpp(2 hunks)src/qt/transactiondesc.cpp(1 hunks)src/qt/transactiontablemodel.cpp(1 hunks)src/qt/walletframe.cpp(1 hunks)src/qt/walletview.cpp(1 hunks)src/random.cpp(1 hunks)src/rpc/blockchain.cpp(3 hunks)src/rpc/client.cpp(1 hunks)src/rpc/coinjoin.cpp(1 hunks)src/rpc/evo.cpp(1 hunks)src/rpc/governance.cpp(1 hunks)src/rpc/masternode.cpp(1 hunks)src/rpc/mempool.cpp(2 hunks)src/rpc/mining.cpp(2 hunks)src/rpc/net.cpp(1 hunks)src/rpc/quorums.cpp(1 hunks)src/rpc/rawtransaction.cpp(1 hunks)src/rpc/server.cpp(2 hunks)src/rpc/txoutproof.cpp(0 hunks)src/rpc/util.cpp(1 hunks)src/scheduler.cpp(1 hunks)src/script/sign.cpp(1 hunks)src/script/standard.h(1 hunks)src/support/lockedpool.cpp(1 hunks)src/sync.h(4 hunks)src/test/checkqueue_tests.cpp(2 hunks)src/test/dbwrapper_tests.cpp(1 hunks)src/test/fuzz/rpc.cpp(1 hunks)src/test/fuzz/signature_checker.cpp(1 hunks)src/test/fuzz/util.h(0 hunks)src/test/script_tests.cpp(2 hunks)src/test/txvalidationcache_tests.cpp(1 hunks)src/test/util_tests.cpp(1 hunks)src/timedata.cpp(1 hunks)src/torcontrol.cpp(1 hunks)src/txdb.cpp(1 hunks)src/util/moneystr.h(0 hunks)src/util/strencodings.h(0 hunks)src/util/system.cpp(2 hunks)src/util/system.h(0 hunks)src/validation.cpp(4 hunks)src/validation.h(1 hunks)src/validationinterface.cpp(1 hunks)src/wallet/context.cpp(1 hunks)src/wallet/rpc/backup.cpp(1 hunks)src/wallet/rpc/transactions.cpp(1 hunks)src/wallet/rpc/wallet.cpp(1 hunks)src/wallet/wallet.cpp(1 hunks)src/wallet/walletdb.cpp(1 hunks)src/warnings.cpp(1 hunks)src/zmq/zmqrpc.cpp(1 hunks)test/functional/mempool_packages.py(1 hunks)test/functional/rpc_mempool_info.py(1 hunks)test/functional/test_runner.py(1 hunks)
💤 Files with no reviewable changes (10)
- src/base58.h
- src/netaddress.h
- src/rpc/txoutproof.cpp
- src/psbt.h
- src/util/system.h
- src/test/fuzz/util.h
- src/core_io.h
- src/node/transaction.h
- src/util/strencodings.h
- src/util/moneystr.h
🧰 Additional context used
📓 Path-based instructions (5)
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/walletview.cppsrc/bench/checkqueue.cppsrc/qt/recentrequeststablemodel.cppsrc/rpc/util.cppsrc/net.hsrc/policy/fees.cppsrc/qt/transactiondesc.cppsrc/qt/walletframe.cppsrc/wallet/context.cppsrc/qt/notificator.cppsrc/test/fuzz/signature_checker.cppsrc/qt/transactiontablemodel.cppsrc/net.cppsrc/qt/clientmodel.hsrc/support/lockedpool.cppsrc/bitcoin-cli.cppsrc/qt/peertablemodel.cppsrc/rpc/mempool.cppsrc/random.cppsrc/rpc/client.cppsrc/rpc/rawtransaction.cppsrc/validation.hsrc/index/txindex.cppsrc/test/txvalidationcache_tests.cppsrc/test/checkqueue_tests.cppsrc/scheduler.cppsrc/script/sign.cppsrc/node/context.cppsrc/netbase.cppsrc/init.cppsrc/qt/addresstablemodel.cppsrc/logging.cppsrc/qt/bantablemodel.cppsrc/wallet/rpc/transactions.cppsrc/sync.hsrc/script/standard.hsrc/wallet/walletdb.cppsrc/rpc/blockchain.cppsrc/rpc/coinjoin.cppsrc/rpc/server.cppsrc/rpc/governance.cppsrc/test/script_tests.cppsrc/validation.cppsrc/validationinterface.cppsrc/wallet/rpc/wallet.cppsrc/qt/rpcconsole.cppsrc/wallet/wallet.cppsrc/warnings.cppsrc/net_processing.cppsrc/test/dbwrapper_tests.cppsrc/rpc/evo.cppsrc/wallet/rpc/backup.cppsrc/txdb.cppsrc/test/fuzz/rpc.cppsrc/qt/paymentserver.cppsrc/netaddress.cppsrc/rpc/quorums.cppsrc/zmq/zmqrpc.cppsrc/rpc/net.cppsrc/torcontrol.cppsrc/test/util_tests.cppsrc/rpc/masternode.cppsrc/qt/overviewpage.cppsrc/bench/prevector.cppsrc/util/system.cppsrc/timedata.cppsrc/httpserver.cppsrc/rpc/mining.cpp
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/checkqueue.cppsrc/bench/prevector.cpp
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/test_runner.pytest/functional/rpc_mempool_info.pytest/functional/mempool_packages.py
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/test/fuzz/signature_checker.cppsrc/test/txvalidationcache_tests.cppsrc/test/checkqueue_tests.cppsrc/test/script_tests.cppsrc/test/dbwrapper_tests.cppsrc/test/fuzz/rpc.cppsrc/test/util_tests.cpp
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/release-notes/release-notes-24408.mddoc/build-unix.mddoc/developer-notes.md
🧠 Learnings (3)
📓 Common learnings
Learnt from: knst
PR: dashpay/dash#6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
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.
📚 Learning: 2025-10-03T11:20:37.475Z
Learnt from: kwvg
PR: dashpay/dash#6838
File: src/active/context.cpp:29-33
Timestamp: 2025-10-03T11:20:37.475Z
Learning: In Dash codebase, NodeContext (src/node/context.h) serves only as a container with trivial c/d-tors; member lifetime is explicitly managed via reset() calls in the shutdown sequence (src/init.cpp), not by declaration order. For example, active_ctx.reset() is called before DashChainstateSetupClose handles llmq_ctx teardown, ensuring proper destruction order regardless of declaration order in the struct.
Applied to files:
src/node/context.cpp
📚 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/backup.cpp
🧬 Code graph analysis (19)
src/qt/clientmodel.h (2)
src/qt/clientmodel.cpp (2)
TipChanged(258-279)TipChanged(258-258)src/net_processing.cpp (16)
EXCLUSIVE_LOCKS_REQUIRED(644-644)EXCLUSIVE_LOCKS_REQUIRED(645-645)EXCLUSIVE_LOCKS_REQUIRED(647-647)EXCLUSIVE_LOCKS_REQUIRED(661-661)EXCLUSIVE_LOCKS_REQUIRED(664-664)EXCLUSIVE_LOCKS_REQUIRED(670-670)EXCLUSIVE_LOCKS_REQUIRED(673-673)EXCLUSIVE_LOCKS_REQUIRED(755-755)EXCLUSIVE_LOCKS_REQUIRED(775-775)EXCLUSIVE_LOCKS_REQUIRED(835-835)EXCLUSIVE_LOCKS_REQUIRED(837-837)EXCLUSIVE_LOCKS_REQUIRED(842-842)EXCLUSIVE_LOCKS_REQUIRED(844-844)EXCLUSIVE_LOCKS_REQUIRED(845-845)EXCLUSIVE_LOCKS_REQUIRED(853-853)EXCLUSIVE_LOCKS_REQUIRED(854-854)
src/support/lockedpool.cpp (1)
src/support/lockedpool.h (1)
LockedPool(126-205)
src/rpc/mempool.cpp (1)
src/rpc/server_util.cpp (2)
EnsureAnyMemPool(55-58)EnsureAnyMemPool(55-55)
test/functional/rpc_mempool_info.py (4)
test/functional/test_framework/test_framework.py (1)
BitcoinTestFramework(101-1140)test/functional/test_framework/util.py (2)
assert_equal(69-74)assert_raises_rpc_error(132-148)test/functional/test_framework/wallet.py (4)
MiniWallet(75-314)rescan_utxos(100-118)get_utxo(181-199)send_self_transfer_multi(237-241)src/rpc/mempool.cpp (3)
tx(68-68)gettxspendingprevout(596-677)gettxspendingprevout(596-596)
src/rpc/rawtransaction.cpp (1)
src/wallet/interfaces.cpp (4)
commands(595-605)commands(595-595)commands(631-634)commands(631-631)
test/functional/mempool_packages.py (2)
src/rpc/mempool.cpp (2)
gettxspendingprevout(596-677)gettxspendingprevout(596-596)test/functional/test_framework/util.py (1)
assert_equal(69-74)
src/qt/bantablemodel.cpp (1)
src/qt/bantablemodel.h (1)
BanTableModel(43-80)
src/sync.h (5)
src/validation.cpp (17)
LOCKS_EXCLUDED(3349-3370)LOCKS_EXCLUDED(3372-3378)EXCLUSIVE_LOCKS_REQUIRED(370-381)EXCLUSIVE_LOCKS_REQUIRED(620-620)EXCLUSIVE_LOCKS_REQUIRED(627-627)EXCLUSIVE_LOCKS_REQUIRED(633-633)EXCLUSIVE_LOCKS_REQUIRED(667-667)EXCLUSIVE_LOCKS_REQUIRED(672-672)EXCLUSIVE_LOCKS_REQUIRED(676-676)EXCLUSIVE_LOCKS_REQUIRED(682-682)EXCLUSIVE_LOCKS_REQUIRED(687-687)EXCLUSIVE_LOCKS_REQUIRED(695-695)EXCLUSIVE_LOCKS_REQUIRED(698-711)EXCLUSIVE_LOCKS_REQUIRED(1332-1358)EXCLUSIVE_LOCKS_REQUIRED(2887-2901)EXCLUSIVE_LOCKS_REQUIRED(3991-4043)EXCLUSIVE_LOCKS_REQUIRED(4051-4117)src/net_processing.cpp (9)
LOCKS_EXCLUDED(327-333)LOCKS_EXCLUDED(340-344)LOCKS_EXCLUDED(345-349)LOCKS_EXCLUDED(1035-1035)LOCKS_EXCLUDED(1037-1037)EXCLUSIVE_LOCKS_REQUIRED(644-644)WITH_LOCK(335-338)WITH_LOCK(3172-3172)WITH_LOCK(3194-3194)src/sync.cpp (5)
AssertLockNotHeldInternal(291-296)AssertLockNotHeldInternal(291-291)AssertLockNotHeldInternal(297-297)AssertLockNotHeldInternal(298-298)AssertLockNotHeldInternal(299-299)src/httpserver.cpp (2)
LOCK(88-103)LOCK(128-133)src/validationinterface.cpp (1)
WAIT_LOCK(81-81)
src/rpc/blockchain.cpp (1)
src/wallet/interfaces.cpp (4)
commands(595-605)commands(595-595)commands(631-634)commands(631-631)
src/rpc/coinjoin.cpp (1)
src/wallet/interfaces.cpp (4)
commands(595-605)commands(595-595)commands(631-634)commands(631-631)
src/rpc/governance.cpp (1)
src/wallet/interfaces.cpp (4)
commands(595-605)commands(595-595)commands(631-634)commands(631-631)
src/validation.cpp (1)
src/net_processing.cpp (17)
tx(1065-1065)EXCLUSIVE_LOCKS_REQUIRED(644-644)EXCLUSIVE_LOCKS_REQUIRED(645-645)EXCLUSIVE_LOCKS_REQUIRED(647-647)EXCLUSIVE_LOCKS_REQUIRED(661-661)EXCLUSIVE_LOCKS_REQUIRED(664-664)EXCLUSIVE_LOCKS_REQUIRED(670-670)EXCLUSIVE_LOCKS_REQUIRED(673-673)EXCLUSIVE_LOCKS_REQUIRED(755-755)EXCLUSIVE_LOCKS_REQUIRED(775-775)EXCLUSIVE_LOCKS_REQUIRED(835-835)EXCLUSIVE_LOCKS_REQUIRED(837-837)EXCLUSIVE_LOCKS_REQUIRED(842-842)EXCLUSIVE_LOCKS_REQUIRED(844-844)EXCLUSIVE_LOCKS_REQUIRED(845-845)EXCLUSIVE_LOCKS_REQUIRED(853-853)EXCLUSIVE_LOCKS_REQUIRED(854-854)
src/wallet/rpc/wallet.cpp (1)
src/wallet/interfaces.cpp (4)
commands(595-605)commands(595-595)commands(631-634)commands(631-631)
src/rpc/evo.cpp (2)
src/wallet/interfaces.cpp (4)
commands(595-605)commands(595-595)commands(631-634)commands(631-631)src/node/interfaces.cpp (1)
tableRPC(601-601)
src/rpc/quorums.cpp (1)
src/wallet/interfaces.cpp (4)
commands(595-605)commands(595-595)commands(631-634)commands(631-631)
src/rpc/net.cpp (1)
src/wallet/interfaces.cpp (4)
commands(595-605)commands(595-595)commands(631-634)commands(631-631)
src/rpc/masternode.cpp (1)
src/wallet/interfaces.cpp (4)
commands(595-605)commands(595-595)commands(631-634)commands(631-631)
src/util/system.cpp (1)
src/util/system.h (1)
ArgsManager(177-593)
src/rpc/mining.cpp (1)
src/wallet/interfaces.cpp (4)
commands(595-605)commands(595-595)commands(631-634)commands(631-631)
🪛 GitHub Actions: Clang Diff Format Check
src/rpc/evo.cpp
[error] 1-1: Clang format differences detected in evo.cpp. Run './contrib/devtools/clang-format-diff.py -p1' to apply diffs or run clang-format to fix formatting.
src/rpc/quorums.cpp
[error] 1-1: Clang format differences detected in quorums.cpp. Run './contrib/devtools/clang-format-diff.py -p1' to apply diffs or run clang-format to fix formatting.
src/rpc/masternode.cpp
[error] 1-1: Clang format differences detected in masternode.cpp. Run './contrib/devtools/clang-format-diff.py -p1' to apply diffs or run clang-format to fix formatting.
⏰ 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). (10)
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (59)
src/wallet/context.cpp (1)
8-9: Defaulting WalletContext specials is fineSwitching to compiler-generated ctor/dtor matches upstream refactor and keeps behavior unchanged. No issues spotted.
src/script/standard.h (1)
9-9: Include looks good.Defining
CPubKeyinside this header warrants the direct include, aligning with upstream and eliminating transitive dependencies.src/torcontrol.cpp (1)
315-318: LGTM! Clean refactoring that removes redundant initialization.The removal of explicit
reconnect_evinitialization from the initializer list is correct. Sincereconnect_evis assigned immediately in the constructor body (Line 320) viaevent_new(), the explicitnullptrinitialization was redundant. This change follows modern C++ best practices and aligns with the PR's modernization objectives without altering observable behavior.src/wallet/rpc/transactions.cpp (1)
66-66: LGTM!Using
= defaultfor the constructor is the correct modern C++ idiom when all members have in-class initializers. This change maintains identical semantics while improving code clarity.src/logging.cpp (1)
465-467: LGTM! Good improvement to logging clarity.Caching the thread name in a local variable improves readability, and the "unknown" fallback ensures consistent log formatting when thread names are unavailable.
src/test/util_tests.cpp (1)
1275-1277: LGTM! Modern C++ best practice applied.The refactor moves the
copiesmember initialization from the constructor's initializer list to an in-class member initializer. This modernization:
- Improves code clarity by making the default value immediately visible at declaration
- Reduces duplication across constructors
- Maintains identical behavior (copies is still initialized to 0)
- Follows C++11+ best practices consistent with the C++20 codebase requirements
src/rpc/evo.cpp (1)
1871-1878: Runtime wallet‑free gating for protx_list/info is correct.Always-register core evo/bls commands; register wallet-free variants of protx_list/info only when wallet is unavailable or disabled. This avoids duplicate registration while preserving functionality.
Also applies to: 1879-1882
src/.clang-tidy (1)
4-4: LGTM! Enabling default member initialization check.Adding
modernize-use-default-member-initto both the checks and warnings-as-errors lists aligns with the broader modernization effort throughout this PR (defaulted constructors/destructors, in-class member initializers). This static analysis update will help maintain consistency in future code.Also applies to: 12-12
src/rpc/coinjoin.cpp (1)
506-517: LGTM! Formatting modernization.The RPC command registrations have been reformatted to use brace-initializer style, removing the need for clang-format pragmas. The functional mappings remain unchanged—this is purely a stylistic improvement consistent with similar changes across other RPC files in this PR.
Also applies to: 524-526
src/index/txindex.cpp (1)
55-55: LGTM! Defaulted destructor.Replacing the empty destructor with a defaulted implementation is a standard C++11 modernization. The compiler-generated destructor will exhibit identical behavior (properly destroying members in reverse declaration order). This change is consistent with the broader modernization effort throughout the PR.
src/netaddress.cpp (1)
102-102: LGTM! Defaulted default constructor.Replacing the empty constructor with a defaulted implementation follows modern C++ best practices. The compiler-generated default constructor will properly default-initialize all members, maintaining the same behavior as the empty body.
src/qt/recentrequeststablemodel.cpp (1)
37-37: LGTM! Defaulted destructor.The change to a defaulted destructor is consistent with the modernization pattern applied across multiple Qt model classes in this PR. The behavior remains identical to the previous empty destructor.
src/scheduler.cpp (1)
14-14: LGTM! Defaulted constructor.The defaulted constructor follows modern C++ idioms and maintains identical behavior to the previous empty-body implementation. This is part of the consistent modernization effort throughout the PR.
src/qt/walletview.cpp (1)
156-156: LGTM! Defaulted destructor.The change to a defaulted destructor is consistent with similar modifications across Qt UI components in this PR. The behavior is identical to the previous empty destructor.
src/rpc/rawtransaction.cpp (1)
1899-1924: LGTM!The refactoring of RPC command registration from multi-line to a compact brace-initialized inline array is a clean modernization. The function signature formatting change also improves consistency. Behavior remains identical.
src/txdb.cpp (1)
213-213: LGTM!Defaulting the destructor is appropriate here since
pcursoris aunique_ptrthat automatically handles cleanup. This modernization simplifies the code without changing behavior.src/qt/notificator.cpp (1)
73-73: LGTM!Defaulting the constructor is safe since all members have well-defined default initialization (primitives and
QByteArray). The explicit constructor on Line 74 handles non-default initialization when needed.src/qt/walletframe.cpp (1)
65-65: LGTM!Defaulting the destructor is appropriate since member cleanup is handled by Qt's parent-child ownership model and the destructors of member objects.
src/qt/peertablemodel.cpp (1)
30-30: LGTM!Defaulting the destructor is safe since the
timermember is a Qt child object (set vianew QTimer(this)on Line 22) and will be automatically deleted by Qt's parent-child mechanism.src/bench/prevector.cpp (1)
14-15: LGTM!The refactoring from constructor initialization list to in-class member initialization with a defaulted constructor is a clean modernization. Both approaches initialize
xto-1, preserving the non-trivial constructibility required by the static assertion on Line 20.src/bench/checkqueue.cpp (1)
32-32: LGTM!Defaulting the constructor is appropriate since the
prevectormember has a well-defined default constructor. The explicit constructor on Line 33 handles initialization with custom data when needed.src/node/context.cpp (1)
34-35: LGTM!Defaulting the constructor and destructor is safe for
NodeContext. As noted in previous reviews,NodeContextserves as a container with member lifetime explicitly managed viareset()calls in the shutdown sequence, not by declaration order. Based on learnings.src/timedata.cpp (1)
19-19: LGTM! Mutex type update aligns with GlobalMutex pattern.The change from
MutextoGlobalMutexforg_timeoffset_mutexis consistent with the broader refactor introducingGlobalMutexas the preferred type for global-scope mutexes across the codebase.src/net.h (1)
202-202: LGTM! Consistent GlobalMutex adoption.The type update from
MutextoGlobalMutexfor the external declarationg_maplocalhost_mutexaligns with the broader refactor pattern.src/warnings.cpp (1)
16-16: LGTM! GlobalMutex type update is correct.The change from
MutextoGlobalMutexforg_warnings_mutexis consistent with the codebase-wide refactor adoptingGlobalMutexfor global synchronization primitives.src/rpc/client.cpp (1)
202-202: LGTM! RPC parameter mapping is correct.The addition of the parameter mapping entry for "gettxspendingprevout" follows the established pattern and correctly maps the "outputs" parameter at index 0 for JSON conversion.
src/test/dbwrapper_tests.cpp (1)
325-325: LGTM! Defaulted constructor follows modern C++ practice.Replacing the explicit empty constructor with
= defaultis a best practice modernization that allows the compiler to generate the default constructor, improving code clarity without changing behavior.test/functional/test_runner.py (1)
396-396: LGTM! Test coverage for new RPC is appropriate.Adding 'rpc_mempool_info.py' to BASE_SCRIPTS ensures that the new gettxspendingprevout RPC functionality receives test coverage in the default test suite.
src/test/checkqueue_tests.cpp (1)
97-97: LGTM! Defaulted constructors are modern C++ best practice.The changes to use
= defaultfor bothMemoryCheckandFrozenCleanupCheckdefault constructors follow modern C++ guidelines and improve code clarity without changing behavior.Also applies to: 131-131
src/wallet/wallet.cpp (1)
200-201: LGTM! Type change aligns with broader GlobalMutex refactor.The conversion of
g_loading_wallet_mutexandg_wallet_release_mutexfromMutextoGlobalMutexis consistent with the repository-wide modernization. The usage patterns (LOCK, WITH_LOCK, WAIT_LOCK) remain unchanged, and the guarded data structures are unaffected.src/util/system.cpp (2)
100-100: LGTM! Consistent mutex type modernization.The change from
MutextoGlobalMutexforcs_dir_locksfollows the same pattern applied across the codebase. The guarded data structure (dir_locks) and usage remain unchanged.
267-268: LGTM! Good practice for reducing compile-time dependencies.Defaulting the constructor and destructor is a standard technique to avoid requiring complete type definitions for class members in headers. The comment at lines 264-266 clearly explains the rationale (avoiding univalue header inclusion for
m_settings).doc/release-notes/release-notes-24408.md (1)
1-5: LGTM! Clear documentation of new RPC feature.The release notes entry clearly documents the new
gettxspendingprevoutRPC, explaining its purpose (scanning mempool for transactions spending given outpoints) and referencing the relevant issue number.src/validation.h (1)
114-114: LGTM! Type upgrade aligns with broader synchronization refactoring.The change from
MutextoGlobalMutexis part of a systematic refactoring to improve thread-safety annotations for global synchronization primitives. This aligns with the PR objectives to backport Bitcoin Core v24 changes that introduce theGlobalMutextype.src/init.cpp (1)
802-802: LGTM! Static mutex type upgrade is consistent with global refactoring.The conversion from
static Mutextostatic GlobalMutexfollows the same pattern asg_best_block_mutexinsrc/validation.h. The existing usage patterns withLOCK(line 809) andWAIT_LOCK(line 2368) remain compatible, confirming thatGlobalMutexmaintains API compatibility with the existing locking mechanisms.test/functional/mempool_packages.py (1)
102-107: Nice addition covering gettxspendingprevoutThe per-input probe ties the new RPC back to existing mempool assertions, so the test will flag any drift between spending lookups and mempool state. Looks solid.
src/rpc/blockchain.cpp (2)
2363-2366: LGTM: cleaner member init and defaulted ctorBrace-initializing m_could_reserve and defaulting the ctor are idiomatic and maintain behavior.
2768-2801: LGTM: RPCTable registration refactorStatic in-function array + append loop aligns with Core v24 style; static storage makes handler pointers safe; categories preserved.
src/rpc/net.cpp (1)
1115-1134: Registration array refactor LGTMFormatting-only change; behavior unchanged.
src/httpserver.cpp (2)
75-87: WorkQueue init/destructor cleanup LGTMIn-class init and defaulted dtor are fine.
160-162: GlobalMutex adoption LGTMConsistent with sync.h changes; locking sites use LOCK correctly.
src/rpc/governance.cpp (2)
1055-1061: Wallet governance command array refactor LGTM
1067-1080: Governance command array refactor LGTMsrc/rpc/mempool.cpp (2)
596-677: New RPC gettxspendingprevout LGTMValidation, locking, and output shape match expectations; strict key checking is good.
774-775: Command registration LGTMsrc/rpc/server.cpp (3)
26-34: Global mutex migration LGTMMutexes now GlobalMutex; usage aligns with locking macros.
204-216: Add HelpExampleRpc for uptime LGTM
270-276: RPC command table init refactor LGTMtest/functional/rpc_mempool_info.py (1)
16-101: Test coverage LGTMGood exercise of success and error paths for gettxspendingprevout.
src/rpc/mining.cpp (2)
981-985: Default-init found flag LGTM
1097-1108: Mining RPC registration array refactor LGTMsrc/validation.cpp (4)
1800-1804: Definition matches the new contract in practiceAssertLockHeld(cs_main) in-body aligns with the declaration requirement.
3052-3052: Defaulted ctor is a clean modernizationNo behavior change; improves clarity.
164-169: New lock contract for CheckInputScripts (requires cs_main)
Good tightening. Ensure all call sites hold cs_main before invoking:
- src/validation.cpp:521, 969, 2477
- src/test/txvalidationcache_tests.cpp:20, 133, 143, 149, 211, 217, 279, 307
Add LOCK(cs_main) (or equivalent) around these calls as needed.
128-131: Verify condition_variable usage with GlobalMutexEnsure the
lockpassed tog_best_block_cv.wait_*insrc/rpc/mining.cpp(around line 778) is astd::unique_lock<GlobalMutex>wrapping the same underlyingMutex(std::mutex) so compatibility withstd::condition_variableis maintained.src/sync.h (4)
147-157: Introduce GlobalMutex to mark global-scope locksThis separation is useful for TSA and matches upstream patterns.
166-166: Add AssertLockNotHeld overload for GlobalMutexConsistent with the new type; no issues.
342-350: Locking macros now route through MaybeCheckNotHeldEnforces preconditions consistently; good safety improvement.
388-390: WITH_LOCK/READ_LOCK pre-checkPlacing MaybeCheckNotHeld before entering the lambda is correct and ensures TSA coverage.
| strTxStatus = tr("%1 confirmations").arg(depth); | ||
| if (fChainLocked) { | ||
| /*: Text explaining the current status of a transaction, shown in the | ||
| status field of the details window for this transaction. This status | ||
| represents a transaction confirmed in at least one block and has been locked by ChainLocks. */ | ||
| strTxStatus += QLatin1String(", ") + tr("locked via ChainLocks"); | ||
| return strTxStatus; | ||
| } | ||
| } | ||
|
|
||
| if (status.is_islocked) { | ||
| /*: Text explaining the current status of a transaction, shown in the | ||
| status field of the details window for this transaction. This status | ||
| represents an unconfirmed transaction that has been locked by InstantSend. */ | ||
| strTxStatus += QLatin1String(", ") + tr("verified via InstantSend"); | ||
| } | ||
|
|
There was a problem hiding this comment.
ChainLocks return prevents InstantSend status
By returning immediately in the ChainLocks branch we skip the status.is_islocked check below, so a transaction that is both ChainLocked and InstantSend-locked no longer displays the InstantSend suffix. Drop the early return and let the function append the InstantSend text as before.
🤖 Prompt for AI Agents
In src/qt/transactiondesc.cpp around lines 61 to 77, the early return inside the
ChainLocks branch prevents the subsequent status.is_islocked check from
appending the InstantSend text for transactions that are both ChainLocked and
InstantSend-locked; remove the immediate return so strTxStatus continues
processing after appending ", locked via ChainLocks" (i.e., drop the return
statement and let the function fall through to the is_islocked block so both
suffixes are appended).
src/rpc/blockchain.cpp
Outdated
| static GlobalMutex cs_blockchange; | ||
| static std::condition_variable cond_blockchange; | ||
| static CUpdatedBlock latestblock GUARDED_BY(cs_blockchange); | ||
|
|
There was a problem hiding this comment.
Fix: condition_variable incompatible with GlobalMutex (will not compile)
WAIT_LOCK on GlobalMutex yields std::unique_lock, but std::condition_variable::wait requires std::unique_lockstd::mutex. Use ConditionVariable (from sync.h) or std::condition_variable_any.
Apply this diff:
static GlobalMutex cs_blockchange;
-static std::condition_variable cond_blockchange;
+static ConditionVariable cond_blockchange;
static CUpdatedBlock latestblock GUARDED_BY(cs_blockchange);If ConditionVariable is unavailable in this branch, prefer std::condition_variable_any instead. Based on learnings
🤖 Prompt for AI Agents
In src/rpc/blockchain.cpp around lines 80 to 83, the code declares
std::condition_variable while the mutex type is GlobalMutex (WAIT_LOCK produces
std::unique_lock<GlobalMutex>), which is incompatible with
std::condition_variable::wait; replace std::condition_variable with either
ConditionVariable from sync.h (and include sync.h) so it accepts GlobalMutex
locks, or use std::condition_variable_any to accept
std::unique_lock<GlobalMutex>, and adjust any wait/notify calls accordingly so
waits use the existing std::unique_lock<GlobalMutex>.
src/test/txvalidationcache_tests.cpp
Outdated
| bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, | ||
| const CCoinsViewCache& inputs, unsigned int flags, bool cacheSigStore, | ||
| bool cacheFullScriptStore, PrecomputedTransactionData& txdata, | ||
| std::vector<CScriptCheck>* pvChecks) EXCLUSIVE_LOCKS_REQUIRED(cs_main); |
There was a problem hiding this comment.
Fix CheckInputScripts callsites to pass CCoinsViewCache by reference
Signature now expects const CCoinsViewCache&. Calls still pass pointers, which will not compile.
Apply:
@@
-bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
- const CCoinsViewCache& inputs, unsigned int flags, bool cacheSigStore,
- bool cacheFullScriptStore, PrecomputedTransactionData& txdata,
- std::vector<CScriptCheck>* pvChecks) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
+ const CCoinsViewCache& inputs, unsigned int flags, bool cacheSigStore,
+ bool cacheFullScriptStore, PrecomputedTransactionData& txdata,
+ std::vector<CScriptCheck>* pvChecks) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
@@
- bool ret = CheckInputScripts(tx, state, &active_coins_tip, test_flags, true, add_to_cache, txdata, nullptr);
+ bool ret = CheckInputScripts(tx, state, active_coins_tip, test_flags, true, add_to_cache, txdata, nullptr);
@@
- BOOST_CHECK(CheckInputScripts(tx, state, &active_coins_tip, test_flags, true, add_to_cache, txdata, &scriptchecks));
+ BOOST_CHECK(CheckInputScripts(tx, state, active_coins_tip, test_flags, true, add_to_cache, txdata, &scriptchecks));
@@
- BOOST_CHECK(CheckInputScripts(tx, state, &active_coins_tip, test_flags, true, add_to_cache, txdata, &scriptchecks));
+ BOOST_CHECK(CheckInputScripts(tx, state, active_coins_tip, test_flags, true, add_to_cache, txdata, &scriptchecks));
@@
- BOOST_CHECK(!CheckInputScripts(CTransaction(spend_tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, true, true, ptd_spend_tx, nullptr));
+ BOOST_CHECK(!CheckInputScripts(CTransaction(spend_tx), state, m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, true, true, ptd_spend_tx, nullptr));
@@
- BOOST_CHECK(CheckInputScripts(CTransaction(spend_tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, true, true, ptd_spend_tx, &scriptchecks));
+ BOOST_CHECK(CheckInputScripts(CTransaction(spend_tx), state, m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, true, true, ptd_spend_tx, &scriptchecks));
@@
- BOOST_CHECK(CheckInputScripts(CTransaction(invalid_with_cltv_tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY, true, true, txdata, nullptr));
+ BOOST_CHECK(CheckInputScripts(CTransaction(invalid_with_cltv_tx), state, m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY, true, true, txdata, nullptr));
@@
- BOOST_CHECK(CheckInputScripts(CTransaction(invalid_with_csv_tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_CHECKSEQUENCEVERIFY, true, true, txdata, nullptr));
+ BOOST_CHECK(CheckInputScripts(CTransaction(invalid_with_csv_tx), state, m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_CHECKSEQUENCEVERIFY, true, true, txdata, nullptr));As per coding guidelines
Also applies to: 133-151, 211-218, 279-280, 307-308
🤖 Prompt for AI Agents
In src/test/txvalidationcache_tests.cpp around lines 20-23 (and also at 133-151,
211-218, 279-280, 307-308), the CheckInputScripts prototype now takes a const
CCoinsViewCache& but the callsites still pass CCoinsViewCache* pointers; update
each call to pass a reference instead of a pointer (e.g. replace passing fooPtr
with *fooPtr or change the local variable to a const CCoinsViewCache&), ensure
the referenced object outlives the call, and adjust any temporary/nullable
usages accordingly so the code compiles with the new signature.
|
This pull request has conflicts, please rebase. |
fa870e3 Remove not needed clang-format off comments (MarcoFalke) Pull request description: It seems odd to disable clang-format and force manual formatting when there is no need for it. So remove the clang-format comments and other unneeded comments. Can be reviewed with `--word-diff-regex=. --ignore-all-space` Looks like this was initially added in commit d9d7957 to accommodate a linter that has since been removed and replaced by a functional test. ACKs for top commit: laanwj: Code review ACK fa870e3 fanquake: ACK fa870e3 Tree-SHA512: 0f8f97c12f5dbe517dd96c10b10ce1b8772d8daed33e6b41f73ea1040e89888cf3b8c0ad7b20319e366fe30c71e8b181c89098ae7f6a3deb8647e1b4731db815
71a8dbe refactor: Remove defunct attributes.h includes (Ben Woosley) Pull request description: Since the removal of NODISCARD in 81d5af4, the only attributes.h def is LIFETIMEBOUND, and it's included in many more places that it is used. This removes all includes which do not have an associated use of LIFETIMEBOUND, and adds it to the following files, due to their use of the same: * src/validationinterface.h * src/script/standard.h See also bitcoin#20499. Top commit has no ACKs. Tree-SHA512: f3e10a5cda5ab78371b77b702f4a241ff69d490a16cc6059f1a4202b97c584accdbc951cc7b6120eae94bee3b9249e9117b45cf6ed1a5228ca23b5638fcf7b7b
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/rpc/blockchain.cpp (1)
80-81: Known critical issue: condition_variable incompatible with GlobalMutex.This issue was previously flagged and remains unresolved.
std::condition_variable::waitrequiresstd::unique_lock<std::mutex>, butWAIT_LOCK(cs_blockchange, lock)producesstd::unique_lock<GlobalMutex>. UseConditionVariable(from sync.h) orstd::condition_variable_anyinstead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
doc/build-unix.md(1 hunks)doc/release-notes/release-notes-24408.md(1 hunks)src/base58.h(0 hunks)src/core_io.h(0 hunks)src/init.cpp(0 hunks)src/logging.cpp(1 hunks)src/netaddress.h(0 hunks)src/node/transaction.h(0 hunks)src/psbt.h(0 hunks)src/qt/transactiondesc.cpp(1 hunks)src/rpc/blockchain.cpp(1 hunks)src/rpc/client.cpp(1 hunks)src/rpc/coinjoin.cpp(1 hunks)src/rpc/evo.cpp(1 hunks)src/rpc/governance.cpp(1 hunks)src/rpc/masternode.cpp(1 hunks)src/rpc/mempool.cpp(2 hunks)src/rpc/mining.cpp(1 hunks)src/rpc/net.cpp(1 hunks)src/rpc/quorums.cpp(1 hunks)src/rpc/rawtransaction.cpp(1 hunks)src/rpc/server.cpp(1 hunks)src/rpc/txoutproof.cpp(0 hunks)src/script/standard.h(1 hunks)src/test/fuzz/rpc.cpp(1 hunks)src/test/fuzz/util.h(0 hunks)src/util/moneystr.h(0 hunks)src/util/strencodings.h(0 hunks)src/util/system.h(0 hunks)src/validationinterface.cpp(1 hunks)src/wallet/rpc/backup.cpp(1 hunks)src/wallet/rpc/wallet.cpp(1 hunks)src/zmq/zmqrpc.cpp(1 hunks)test/functional/mempool_packages.py(1 hunks)test/functional/rpc_mempool_info.py(1 hunks)test/functional/test_runner.py(1 hunks)
💤 Files with no reviewable changes (11)
- src/netaddress.h
- src/util/moneystr.h
- src/base58.h
- src/node/transaction.h
- src/init.cpp
- src/util/strencodings.h
- src/util/system.h
- src/rpc/txoutproof.cpp
- src/psbt.h
- src/core_io.h
- src/test/fuzz/util.h
🚧 Files skipped from review as they are similar to previous changes (15)
- doc/build-unix.md
- src/rpc/governance.cpp
- src/rpc/coinjoin.cpp
- src/rpc/mempool.cpp
- src/rpc/client.cpp
- src/rpc/quorums.cpp
- src/zmq/zmqrpc.cpp
- test/functional/test_runner.py
- src/rpc/rawtransaction.cpp
- src/rpc/net.cpp
- src/test/fuzz/rpc.cpp
- src/rpc/mining.cpp
- src/qt/transactiondesc.cpp
- src/validationinterface.cpp
- test/functional/mempool_packages.py
🧰 Additional context used
📓 Path-based instructions (3)
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/logging.cppsrc/rpc/evo.cppsrc/wallet/rpc/backup.cppsrc/rpc/masternode.cppsrc/rpc/blockchain.cppsrc/script/standard.hsrc/rpc/server.cppsrc/wallet/rpc/wallet.cpp
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/rpc_mempool_info.py
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/release-notes/release-notes-24408.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: knst
PR: dashpay/dash#6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
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 (5)
src/rpc/evo.cpp (2)
src/wallet/interfaces.cpp (4)
commands(595-605)commands(595-595)commands(631-634)commands(631-631)src/node/interfaces.cpp (1)
tableRPC(601-601)
src/rpc/masternode.cpp (1)
src/wallet/interfaces.cpp (4)
commands(595-605)commands(595-595)commands(631-634)commands(631-631)
src/rpc/blockchain.cpp (1)
src/wallet/interfaces.cpp (4)
commands(595-605)commands(595-595)commands(631-634)commands(631-631)
test/functional/rpc_mempool_info.py (4)
test/functional/test_framework/test_framework.py (1)
BitcoinTestFramework(101-1140)test/functional/test_framework/util.py (2)
assert_equal(69-74)assert_raises_rpc_error(132-148)test/functional/test_framework/wallet.py (4)
MiniWallet(75-314)rescan_utxos(100-118)get_utxo(181-199)send_self_transfer_multi(237-241)src/rpc/mempool.cpp (9)
tx(68-68)getrawmempool(370-422)getrawmempool(370-370)info(339-339)info(481-481)info(549-549)info(588-588)gettxspendingprevout(596-677)gettxspendingprevout(596-596)
src/wallet/rpc/wallet.cpp (1)
src/wallet/interfaces.cpp (4)
commands(595-605)commands(595-595)commands(631-634)commands(631-631)
🪛 GitHub Actions: Clang Diff Format Check
src/rpc/evo.cpp
[error] 1-1: Clang format differences found in src/rpc/evo.cpp. Run the clang-format-diff.py script (as shown in logs) to align formatting.
src/rpc/masternode.cpp
[error] 1-1: Clang format differences found in src/rpc/masternode.cpp. Run the clang-format-diff.py script (as shown in logs) to align formatting.
⏰ 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). (9)
- GitHub Check: mac-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (11)
src/logging.cpp (1)
465-467: LGTM: Thread name caching with fallback.The change correctly caches the thread name and provides a sensible "unknown" fallback for empty thread names, improving both readability and providing a minor performance optimization.
src/script/standard.h (1)
9-9: LGTM!Adding
#include <pubkey.h>provides direct access to pubkey-related types, aligning with the PR's broader header dependency cleanup effort.src/wallet/rpc/backup.cpp (1)
1851-1851: LGTM!Fixed typo in the help example string from "desccriptor" to "descriptor". This improves the quality of user-facing documentation.
src/rpc/server.cpp (2)
214-214: LGTM!Adding the RPC example for the uptime command improves documentation consistency with other RPC commands in the file.
270-276: LGTM!The array reformatting from multi-line brace-enclosed format to compact brace-initializer format aligns with the broader RPC registration style changes throughout this PR. No functional changes, just formatting.
src/rpc/evo.cpp (1)
1894-1902: Verify hybrid RPC registration logic is correct.The conditional registration of
commands_wallet(containingprotx_listandprotx_info) occurs when wallet support is NOT available or is disabled at runtime. This appears correct for hybrid RPCs that provide reduced functionality without wallet support, but please confirm this is the intended behavior.Based on learnings
src/rpc/blockchain.cpp (1)
2762-2799: LGTM! Registration refactor is correct.The change from a static command array to a brace-initialized inline array with loop-based registration preserves the same RPC entries and behavior while simplifying the registration pattern.
src/wallet/rpc/wallet.cpp (1)
1156-1226: LGTM! Registration refactor is correct.The change to a brace-initialized array preserves the same wallet RPC entries while aligning with the registration style refactor across other RPC modules.
doc/release-notes/release-notes-24408.md (1)
1-5: LGTM! Clear documentation of new RPC.The release note clearly describes the new
gettxspendingprevoutRPC and its purpose.src/rpc/masternode.cpp (1)
691-703: Registration refactor is correct, but formatting needs attention.The change to brace-initialized array with loop-based registration preserves behavior and aligns with the pattern across other RPC modules. However, address the clang-format issue flagged by the pipeline.
test/functional/rpc_mempool_info.py (1)
20-99: LGTM! Comprehensive test coverage for gettxspendingprevout RPC.The test creates a well-defined mempool transaction tree and exercises the
gettxspendingprevoutRPC with:
- Positive cases: spent outputs (lines 64-70)
- Negative cases: unspent outputs (lines 72-76)
- Mixed cases: spent and unspent outputs (lines 78-80)
- Error handling: invalid parameters (lines 82-98)
The test logic is sound and provides thorough coverage of the new RPC.
| static const CRPCCommand commands[]{ | ||
| {"evo", &protx_list}, | ||
| {"evo", &protx_info}, | ||
| {"evo", &protx_register}, | ||
| {"evo", &protx_register_evo}, | ||
| {"evo", &protx_register_fund}, | ||
| {"evo", &protx_register_fund_evo}, | ||
| {"evo", &protx_register_prepare}, | ||
| {"evo", &protx_register_prepare_evo}, | ||
| {"evo", &protx_update_service}, | ||
| {"evo", &protx_update_service_evo}, | ||
| {"evo", &protx_register_submit}, | ||
| {"evo", &protx_update_registrar}, | ||
| {"evo", &protx_revoke}, | ||
| {"hidden", &protx_register_legacy}, | ||
| {"hidden", &protx_register_fund_legacy}, | ||
| {"hidden", &protx_register_prepare_legacy}, | ||
| {"hidden", &protx_update_registrar_legacy}, | ||
| }; |
There was a problem hiding this comment.
Wallet-aware RPC registration looks correct; fix clang-format issues.
The split into separate arrays for wallet and non-wallet RPC commands is well-structured. However, the CI pipeline reports clang-format differences in this file that must be resolved.
Run the following command to fix formatting:
#!/bin/bash
# Fix clang-format issues in src/rpc/evo.cpp
./contrib/devtools/clang-format-diff.py -p1 src/rpc/evo.cpp🤖 Prompt for AI Agents
In src/rpc/evo.cpp around lines 1848 to 1866 the array formatting does not match
clang-format expectations; run the repo's clang-format-diff script
(./contrib/devtools/clang-format-diff.py -p1 src/rpc/evo.cpp) or apply the
project's clang-format style to that block so the entries line up and
spacing/commas match the rest of the file, then re-run CI to confirm formatting
is fixed.
| static const CRPCCommand commands[]{ | ||
| {"dash", &masternode_outputs}, | ||
| }; | ||
| return commands; | ||
| } |
There was a problem hiding this comment.
Run clang-format to fix formatting issues.
The pipeline detected formatting differences. Run clang-format-diff.py as indicated in the pipeline logs to align formatting.
🤖 Prompt for AI Agents
In src/rpc/masternode.cpp around lines 682 to 686 the file formatting does not
match project style; run the repository's clang-format step (e.g.,
clang-format-diff.py as referenced in pipeline logs or run clang-format with the
project style) and apply the suggested changes to reformat the static const
CRPCCommand commands[] block and surrounding code, then add the resulting
changes to the commit so the pipeline formatting check passes.
src/evo/netinfo.h:198:5: warning: Class 'NetInfoEntry' has a constructor with 1 argument that is not explicit. [noExplicitConstructor]
src/evo/netinfo.h:204:5: warning: Class 'NetInfoEntry' has a constructor with 1 argument that is not explicit. [noExplicitConstructor]
src/evo/netinfo.h:223:25: note: Shadowed declaration
src/evo/netinfo.h:226:32: warning: Local variable 'data_ptr' shadows outer variable [shadowVariable]
src/stacktraces.cpp:431:20: warning: Passing the result of c_str() to a function that takes std::string_view as argument no. 1 is slow and redundant. [stlcstrParam]
Advice not applicable in this specific case? Add an exception by updating
IGNORED_WARNINGS in /__w/dash/dash/test/lint/lint-cppcheck-dash.sh
…sc::FormatTxStatus` 8cfb562 qt, refactor: add translator comments in `TransactionDesc::FormatTxStatus()` (w0xlt) Pull request description: This PR adds translator comments to `TransactionDesc::FormatTxStatus` as suggested in bitcoin-core/gui#552 (comment) and bitcoin-core/gui#552 (comment). ACKs for top commit: hebasto: ACK 8cfb562 Tree-SHA512: 2c44b915e6309508f34fc22bb90e3d88ad32ed82fdb3a395f7c6716941edc1b311991140d28e838ad622a7484ed86aedd25e55674857fec8716d9575aed25fa0
…c prevouts 4185570 Add RPC to get mempool txs spending outputs (t-bast) Pull request description: We add an RPC to fetch mempool transactions spending any of the given outpoints. Without this RPC, application developers need to first call `getrawmempool` which returns a long list of `txid`, then fetch each of these transactions individually (`getrawtransaction`) to check whether they spend the given outpoints, which wastes a lot of bandwidth (in the worst case we need to transfer the whole mempool). For example in lightning, when we discover that one of our channel funding transactions has been spent, we need to find the spending transaction to claim our outputs from it. We are currently forced to fetch the whole mempool to do the analysis ourselves, which is quite costly. I believe that this RPC is also generally useful when doing some introspection on your mempool after one of your transactions failed to broadcast, for example when you implement RBF at the application level. Fetching and analyzing the conflicting transaction gives you more information to successfully replace it. ACKs for top commit: darosior: re-utACK 4185570 vincenzopalazzo: re-ACK bitcoin@4185570 danielabrozzoni: re-tACK 4185570 w0xlt: reACK bitcoin@4185570 Tree-SHA512: 206687efb720308b7e0b6cf16dd0a994006c0b5a290c8eb386917a80130973a6356d0d5cae1c63a01bb29e066dd721594969db106cba7249214fcac90d2c3dbc
3a171f7 logging: fix logging empty threadname (klementtan) Pull request description: Currently, `leveldb` background thread does not have a thread name and as a result, an empty thread name is logged. This PR fixes this by logging thread name as `"unknown"` if the thread name is empty On master: ```txt 2022-06-02T14:30:38Z [] [leveldb:debug] Generated table dashpay#281@0: 1862 keys, 138303 bytes ``` On this PR: ```txt 2022-06-02T14:30:38Z [unknown] [leveldb:debug] Generated table dashpay#281@0: 1862 keys, 138303 bytes ``` ACKs for top commit: laanwj: Code review ACK 3a171f7 hebasto: ACK 3a171f7 Tree-SHA512: 0af0fa5c4ddd3640c6dab9595fe9d97f74d0e0f4b41287a6630cf8ac5a21240250e0659ec4ac5a561e888d522f5304bf627104de2aba0fd0a86c1222de0897c2
210cd59 doc: Fix typo in importdescriptors (Kolby Moroz Liebl) Pull request description: ACKs for top commit: 1440000bytes: ACK bitcoin@210cd59 LarryRuane: ACK 210cd59 brunoerg: crACK 210cd59 Tree-SHA512: 39ff9777b05abc1a68c8c3e646e00b0672838696c567c582d0492baa753863231447fd8439bd41cd8a8b8ba752299b032e839c8862c02faa2bdc207a9a7a8540
f67b6fc Update Arch Linux build example (Igor Bubelov) Pull request description: The current build example has two issues: 1. The claim that the wallet functionality will be missing is obsolete since Bitcoin Core can use SQLite, which is a hard dependency of `pacman` so we can assume that it's always present. 2. Installing package groups such as `base-devel` adds some friction and uncertainty by forcing readers to choose which packages they need, interactively. Listing required deps explicitly speeds up the whole process, makes it more transparent and cuts the number of installed packages. ACKs for top commit: laanwj: re-ACK f67b6fc Tree-SHA512: c068dac5d244044827d5d94a4b48f239180301b6870dce31b003fa111a69f7e3a483681a7ea2b3d393d6791b40043685ce2fe62c338cce1b7e37a6db0f02b1a2
ac6fbf2 tidy: use modernize-use-default-member-init (fanquake) 7aa40f5 refactor: use C++11 default initializers (fanquake) Pull request description: Refactor and then enable [`modernize-use-default-member-init`](https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-default-member-init.html) in our `clang-tidy` job. Top commit has no ACKs. Tree-SHA512: 536b406f20639f8c588fe9e96175ec60c7bb825506b2670b562370b2f572801c24203c483443be3c199e1b958c0765d4532e57c57a4e78689162a1dd422d844f
|
Removing |
knst
changed the title
What was done?
Regular backports from bitcoin core v24
How Has This Been Tested?
Run unit & functional tests
Breaking Changes
N/A
Checklist: