backport: merge bitcoin#25447, #25624, #25641, #25683, #23679, #26216, #26295, #26513, #26515, #26727, #25374, #25272, #26408, #25112, #26702, #25932, #26822 (auxiliary backports: part 27) #6788
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
Going through the vin and then adding the vin index as a vout position shouldn't work, especially if there are an unequal number of inputs and outputs but the test transactions tend to have spend patterns that allow the code to inadvertently work, evading detection. A new test has been added that uses a spend pattern that would've netted unexpected behavior in the previous logic. Also, we can just use `AddChildrenToWorkSet` for this because it effectively implements the same logic (going through vout, finding and inserting).
Both functions that manipulate it, `AddTx()` and `LimitOrphans()` hold the mutex.
This commit will not compile as-is as block based orphan reprocessing, a Dash-ism, has not been adapted to work with this refactor. That has been segmented out into a separate commit to aid in review.
WalkthroughThis set of changes implements a major refactor of the orphan transaction handling infrastructure in the codebase. The global recursive orphan mutex ( Estimated code review effort🎯 5 (Critical) | ⏱️ ~80–100 minutes Complexity label: Extensive refactor and interface changes This review requires careful attention to thread safety, correctness of new peer-driven orphan handling logic, interface changes across several modules, and thorough validation of new and updated tests and fuzz targets. The scope spans consensus, networking, wallet, RPC, and test code, affecting both core logic and public interfaces. Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (15)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (8)
🧰 Additional context used📓 Path-based instructions (3)test/functional/**/*.py📄 CodeRabbit Inference Engine (CLAUDE.md)
Files:
src/**/*.{cpp,h,cc,cxx,hpp}📄 CodeRabbit Inference Engine (CLAUDE.md)
Files:
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}📄 CodeRabbit Inference Engine (CLAUDE.md)
Files:
🧠 Learnings (4)📓 Common learnings📚 Learning: in the test framework consolidation pr (#6718), user kwvg prefers to limit functional changes to tho...Applied to files:
📚 Learning: applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : unit tests for c++ code should b...Applied to files:
📚 Learning: the `getwallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null chec...Applied to files:
🧬 Code Graph Analysis (1)test/functional/p2p_disconnect_ban.py (3)
⏰ 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)
🔇 Additional comments (8)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/test/fuzz/txorphan.cpp (1)
29-30: Document the orphan transaction limit calculationThe calculation correctly adapts Bitcoin's count-based limit to Dash's size-based approach by assuming maximum-sized transactions. Consider adding a comment explaining this adaptation aligns with Dash's orphan limiting strategy and effectively caps at 100 transactions.
-//! Default maximum number of maximum size orphan transactions that can be kept in memory -static const uint32_t DEFAULT_MAX_ORPHAN_TRANSACTIONS{(DEFAULT_MAX_ORPHAN_TRANSACTIONS_SIZE * 1000000) / MAX_STANDARD_TX_SIZE}; +//! Default maximum number of maximum size orphan transactions that can be kept in memory +//! Dash limits orphans by total size, so we calculate the count assuming max-sized transactions +static const uint32_t DEFAULT_MAX_ORPHAN_TRANSACTIONS{(DEFAULT_MAX_ORPHAN_TRANSACTIONS_SIZE * 1000000) / MAX_STANDARD_TX_SIZE};src/wallet/test/wallet_tests.cpp (1)
1550-1551: Consider using BOOST_CHECK instead of BOOST_ASSERT.
BOOST_ASSERTwill terminate the test execution if the condition fails, whileBOOST_CHECKallows the test to continue and report the failure. For unit tests,BOOST_CHECKis generally preferred to get more comprehensive test results.- BOOST_ASSERT(wallet.GetNewDestination("", dest, error)); + BOOST_CHECK(wallet.GetNewDestination("", dest, error));src/net_processing.cpp (1)
3211-3215: Remove redundant variable declarationThe initial declaration of
porphanTxon line 3211 is unnecessary since it's immediately reassigned in the while loop condition.- CTransactionRef porphanTx = nullptr; NodeId from_peer = -1; bool more = false; - while (CTransactionRef porphanTx = m_orphanage.GetTxToReconsider(node_id, from_peer, more)) { + CTransactionRef porphanTx; + while ((porphanTx = m_orphanage.GetTxToReconsider(node_id, from_peer, more))) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
ci/dash/lint-tidy.sh(1 hunks)src/Makefile.test.include(1 hunks)src/init.cpp(0 hunks)src/net_processing.cpp(13 hunks)src/rpc/net.cpp(8 hunks)src/test/fuzz/process_message.cpp(0 hunks)src/test/fuzz/process_messages.cpp(0 hunks)src/test/fuzz/txorphan.cpp(1 hunks)src/test/orphanage_tests.cpp(2 hunks)src/txorphanage.cpp(8 hunks)src/txorphanage.h(4 hunks)src/util/check.cpp(1 hunks)src/util/check.h(2 hunks)src/validation.cpp(4 hunks)src/validation.h(1 hunks)src/wallet/coinselection.cpp(1 hunks)src/wallet/test/wallet_tests.cpp(1 hunks)src/wallet/wallet.cpp(1 hunks)src/wallet/wallet.h(1 hunks)src/wallet/walletdb.cpp(3 hunks)src/wallet/walletdb.h(1 hunks)test/functional/p2p_disconnect_ban.py(3 hunks)test/functional/rpc_net.py(1 hunks)test/functional/test_framework/test_node.py(1 hunks)test/functional/test_framework/util.py(0 hunks)
💤 Files with no reviewable changes (4)
- src/test/fuzz/process_messages.cpp
- src/test/fuzz/process_message.cpp
- test/functional/test_framework/util.py
- src/init.cpp
🧰 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/rpc_net.pytest/functional/test_framework/test_node.pytest/functional/p2p_disconnect_ban.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/validation.hsrc/util/check.cppsrc/wallet/wallet.hsrc/wallet/walletdb.cppsrc/wallet/coinselection.cppsrc/util/check.hsrc/wallet/walletdb.hsrc/test/fuzz/txorphan.cppsrc/wallet/wallet.cppsrc/wallet/test/wallet_tests.cppsrc/rpc/net.cppsrc/test/orphanage_tests.cppsrc/validation.cppsrc/txorphanage.hsrc/net_processing.cppsrc/txorphanage.cpp
ci/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the ci directory (continuous integration)
Files:
ci/dash/lint-tidy.sh
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/txorphan.cppsrc/wallet/test/wallet_tests.cppsrc/test/orphanage_tests.cpp
🧠 Learnings (19)
📓 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: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.631Z
Learning: Dash Core uses BITCOIN_ prefix for header guards as the standard convention, inherited from Bitcoin Core. Only a few BLS-specific files in src/bls/ use DASH_ prefix. The vast majority of files (385+) use BITCOIN_ prefix.
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/wallet/wallet.cpp:0-0
Timestamp: 2025-07-29T14:33:01.040Z
Learning: In refactoring PRs like #6761, kwvg acknowledges code safety improvements (like null pointer checks and unused parameter warnings) but prefers to defer them to follow-up PRs to maintain focus on the primary refactoring objectives, avoiding scope creep.
Learnt from: kwvg
PR: dashpay/dash#6752
File: src/wallet/load.cpp:164-164
Timestamp: 2025-07-17T15:48:29.418Z
Learning: In Dash Core, kwvg prefers using assert() statements to document expected behavior in code paths, especially when the called function also has internal asserts. This is used as a defensive programming pattern rather than requiring conditional error handling.
src/Makefile.test.include (2)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/fuzz/**/*.{cpp,h,cc,cxx,hpp} : Fuzzing harnesses should be placed in src/fuzz/
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : 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
src/util/check.cpp (1)
Learnt from: kwvg
PR: #6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
src/wallet/wallet.h (2)
Learnt from: kwvg
PR: #6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The GetWallet() function calls in src/wallet/rpcwallet.cpp are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/evo/specialtx.h : Special transactions use payload extensions defined in src/evo/specialtx.h
ci/dash/lint-tidy.sh (6)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to 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
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{crc32c,dashbls,gsl,immer,leveldb,minisketch,secp256k1,univalue}/** : Do not make changes under any circumstances to vendored dependencies in src/crc32c, src/dashbls, src/gsl, src/immer, src/leveldb, src/minisketch, src/secp256k1, src/univalue
Learnt from: kwvg
PR: #6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : 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
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/fuzz/**/*.{cpp,h,cc,cxx,hpp} : Fuzzing harnesses should be placed in src/fuzz/
Learnt from: kwvg
PR: #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.
test/functional/test_framework/test_node.py (1)
Learnt from: kwvg
PR: #6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
src/wallet/walletdb.cpp (2)
Learnt from: kwvg
PR: #6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The GetWallet() function calls in src/wallet/rpcwallet.cpp are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Learnt from: kwvg
PR: #6530
File: src/validation.cpp:360-362
Timestamp: 2025-01-14T08:37:16.955Z
Learning: The UpdateTransactionsFromBlock() method in txmempool.cpp takes parameters in the order: vHashUpdate, ancestor_size_limit, ancestor_count_limit. The size limit comes before the count limit.
src/wallet/coinselection.cpp (1)
Learnt from: kwvg
PR: #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.
src/util/check.h (1)
Learnt from: kwvg
PR: #6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
src/wallet/walletdb.h (2)
Learnt from: kwvg
PR: #6530
File: src/validation.cpp:360-362
Timestamp: 2025-01-14T08:37:16.955Z
Learning: The UpdateTransactionsFromBlock() method in txmempool.cpp takes parameters in the order: vHashUpdate, ancestor_size_limit, ancestor_count_limit. The size limit comes before the count limit.
Learnt from: kwvg
PR: #6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The GetWallet() function calls in src/wallet/rpcwallet.cpp are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
src/test/fuzz/txorphan.cpp (1)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/fuzz/**/*.{cpp,h,cc,cxx,hpp} : Fuzzing harnesses should be placed in src/fuzz/
src/wallet/wallet.cpp (2)
Learnt from: kwvg
PR: #6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The GetWallet() function calls in src/wallet/rpcwallet.cpp are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Learnt from: kwvg
PR: #6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
src/wallet/test/wallet_tests.cpp (3)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : 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
Learnt from: kwvg
PR: #6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The GetWallet() function calls in src/wallet/rpcwallet.cpp are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Learnt from: kwvg
PR: #6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
src/rpc/net.cpp (4)
Learnt from: kwvg
PR: #6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/is_basic_scheme_active=/true, /is_extended_addr=/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Learnt from: kwvg
PR: #6665
File: src/evo/deterministicmns.cpp:1284-1287
Timestamp: 2025-06-16T17:59:55.669Z
Learning: In Dash masternode ProRegTx validation, platform ports (platformHTTPPort and platformP2PPort) are mandatory and must be non-zero, while netInfo (ipAndPort) is optional. This means that even if an empty netInfo returns 0 from GetPrimary().GetPort(), it won't cause false positives in port duplication checks since platform ports cannot be 0.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/evo/specialtx.h : Special transactions use payload extensions defined in src/evo/specialtx.h
Learnt from: kwvg
PR: #6629
File: src/evo/deterministicmns.cpp:473-485
Timestamp: 2025-05-10T00:54:15.597Z
Learning: In the current implementation of NetInfoEntry, only CService is a valid type and std::monostate represents an invalid state. Entries that don't provide a valid CService through GetAddrPort() should be rejected with an exception, not silently skipped.
src/test/orphanage_tests.cpp (4)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : 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
Learnt from: kwvg
PR: #6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
PR: #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: kwvg
PR: #6530
File: src/validation.cpp:360-362
Timestamp: 2025-01-14T08:37:16.955Z
Learning: The UpdateTransactionsFromBlock() method in txmempool.cpp takes parameters in the order: vHashUpdate, ancestor_size_limit, ancestor_count_limit. The size limit comes before the count limit.
src/validation.cpp (1)
Learnt from: knst
PR: #6511
File: src/evo/deterministicmns.cpp:1369-1373
Timestamp: 2025-01-07T18:50:44.838Z
Learning: The functions MigrateDBIfNeeded and MigrateDBIfNeeded2 in src/evo/deterministicmns.cpp are temporary and will be removed in a future version. Refactoring suggestions for these functions should be avoided.
src/txorphanage.h (4)
Learnt from: kwvg
PR: #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: kwvg
PR: #6530
File: src/validation.cpp:360-362
Timestamp: 2025-01-14T08:37:16.955Z
Learning: The UpdateTransactionsFromBlock() method in txmempool.cpp takes parameters in the order: vHashUpdate, ancestor_size_limit, ancestor_count_limit. The size limit comes before the count limit.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
Learnt from: kwvg
PR: #6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
src/net_processing.cpp (4)
Learnt from: kwvg
PR: #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: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/evo/specialtx.h : Special transactions use payload extensions defined in src/evo/specialtx.h
Learnt from: kwvg
PR: #6530
File: src/validation.cpp:360-362
Timestamp: 2025-01-14T08:37:16.955Z
Learning: The UpdateTransactionsFromBlock() method in txmempool.cpp takes parameters in the order: vHashUpdate, ancestor_size_limit, ancestor_count_limit. The size limit comes before the count limit.
Learnt from: knst
PR: #6658
File: src/evo/creditpool.cpp:177-185
Timestamp: 2025-05-05T12:45:44.781Z
Learning: The GetAncestor() function in CBlockIndex safely handles negative heights by returning nullptr rather than asserting, making it safe to call with potentially negative values.
src/txorphanage.cpp (2)
Learnt from: kwvg
PR: #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: kwvg
PR: #6530
File: src/validation.cpp:360-362
Timestamp: 2025-01-14T08:37:16.955Z
Learning: The UpdateTransactionsFromBlock() method in txmempool.cpp takes parameters in the order: vHashUpdate, ancestor_size_limit, ancestor_count_limit. The size limit comes before the count limit.
🧬 Code Graph Analysis (8)
test/functional/rpc_net.py (2)
test/functional/test_framework/util.py (1)
assert_raises_rpc_error(132-148)src/rpc/net.cpp (3)
addpeeraddress(988-1042)addpeeraddress(988-988)address(538-538)
src/util/check.cpp (1)
src/util/check.h (2)
NonFatalCheckError(13-17)NonFatalCheckError(24-25)
src/util/check.h (3)
src/util/check.cpp (1)
NonFatalCheckError(19-26)src/net.cpp (1)
msg(829-829)src/txmempool.h (1)
T(72-84)
src/wallet/walletdb.h (1)
src/wallet/walletdb.cpp (2)
FindWalletTxHashes(977-1024)FindWalletTxHashes(977-977)
test/functional/p2p_disconnect_ban.py (3)
src/rpc/net.cpp (5)
info(422-422)setban(729-814)setban(729-729)listbanned(816-861)listbanned(816-816)test/functional/test_framework/util.py (2)
assert_raises_rpc_error(132-148)assert_equal(69-74)test/functional/test_framework/test_framework.py (1)
bump_mocktime(870-887)
src/test/orphanage_tests.cpp (2)
src/txorphanage.h (1)
m_mutex(104-104)src/test/util/setup_common.h (1)
InsecureRand256(80-80)
src/txorphanage.h (2)
src/txorphanage.cpp (22)
AddTx(20-57)AddTx(20-20)HaveTx(172-176)HaveTx(172-172)GetTxToReconsider(178-199)GetTxToReconsider(178-178)SetCandidatesByBlock(201-210)SetCandidatesByBlock(201-201)EraseTx(59-63)EraseTx(59-59)EraseForPeer(100-117)EraseForPeer(100-100)EraseForBlock(212-241)EraseForBlock(212-212)HaveMoreWork(243-248)HaveMoreWork(243-243)LimitOrphans(119-153)LimitOrphans(119-119)AddChildrenToWorkSet(155-170)AddChildrenToWorkSet(155-155)_EraseTx(65-98)_EraseTx(65-65)src/test/orphanage_tests.cpp (2)
LOCK(23-27)LOCK(29-37)
src/txorphanage.cpp (2)
src/test/orphanage_tests.cpp (2)
LOCK(23-27)LOCK(29-37)src/txorphanage.h (1)
m_mutex(104-104)
🔇 Additional comments (40)
src/validation.cpp (4)
2708-2713: LGTM! Proper initialization of member variables.The initialization logic correctly maintains the same behavior as the original static local variables, using
.count() == 0to check for first-time initialization.
2721-2723: LGTM! Timing comparisons correctly updated.The periodic write and flush timing checks properly use the new member variables while maintaining identical logic and behavior.
2754-2754: LGTM! Write timestamp correctly updated.The member variable is properly updated after a successful write operation, maintaining the same tracking behavior as the original static variable.
2780-2780: LGTM! Flush timestamp correctly updated.The member variable is properly updated after a successful flush operation, maintaining the same tracking behavior as the original static variable.
test/functional/test_framework/test_node.py (1)
109-109: LGTM: Appropriate debug exclusion for testing.Adding
"-debugexclude=rand"is a reasonable addition to reduce noise from random-related debug logs during functional tests, especially given the extensive fuzzing infrastructure changes in this PR.src/Makefile.test.include (1)
366-366: LGTM: Correctly adds new txorphan fuzz test to build.The addition of
test/fuzz/txorphan.cppto the fuzz test sources is correctly placed and necessary for building the new orphan transaction fuzz test that exercises the refactoredTxOrphanageclass.ci/dash/lint-tidy.sh (1)
23-23: LGTM: Correctly adds new files to IWYU analysis.Adding
src/test/fuzz/txorphan.cppandsrc/util/check.cppto the include-what-you-use tool ensures proper analysis of include directives for the new fuzz test and updated utility code.Also applies to: 26-26
test/functional/rpc_net.py (1)
344-347: LGTM: Comprehensive port validation tests.The test cases appropriately verify that the
addpeeraddressRPC rejects invalid port values (-1 and 65536) with the expected error code (-1) and message ("JSON integer out of range"). This complements the RPC implementation changes that parse ports asuint16_t.src/wallet/coinselection.cpp (1)
93-94: LGTM: Good refactoring for code clarity.Extracting the fee rate comparison into a local boolean variable
is_feerate_highimproves code readability without changing the algorithm's logic. This makes the backtracking condition in the Branch and Bound algorithm easier to understand.Also applies to: 101-101
src/wallet/wallet.h (1)
620-623: LGTM! Clear documentation for AddToWallet contract.The added documentation clearly explains the method's behavior and return value semantics, particularly the important detail that it returns
nullptron database write failures. This documentation aligns well with the improved error handling mentioned in the PR objectives.src/wallet/walletdb.h (1)
236-236: LGTM! Interface simplification improves efficiency.The method rename from
FindWalletTxtoFindWalletTxHasheswith the simplified signature accurately reflects its purpose - extracting only transaction hashes from the database without deserializing full wallet transactions. This is a good optimization when only hashes are needed.src/util/check.cpp (2)
7-16: LGTM! Proper includes for enhanced error reporting.The conditional inclusion of
bitcoin-config.hand additional standard headers (cstdio,cstdlib,string) are necessary for the enhanced error formatting functionality implemented below.
19-26: LGTM! Comprehensive error reporting for internal bugs.The new
NonFatalCheckErrorconstructor creates detailed error messages that include:
- The assertion message and source location (file, line, function)
- Package name and full version information
- Bug report URL for user guidance
This significantly improves debugging experience by providing all relevant context when internal bugs are detected.
test/functional/p2p_disconnect_ban.py (4)
60-61: LGTM! Test coverage for past absolute timestamp validation.The test correctly validates that attempting to ban with a past absolute timestamp (123 seconds since epoch) throws the expected RPC error (-8) with the appropriate error message. This directly tests the validation logic added to the
setbanRPC command.
81-83: LGTM! Test coverage for absolute timestamp banning.The test adds a ban with an absolute timestamp (current mock time + 120 seconds) using the absolute flag, which exercises the new absolute timestamp functionality in the
setbanRPC command.
85-99: LGTM! Comprehensive ban duration and time remaining validation.The test correctly verifies that ban duration and time remaining calculations work properly after advancing mock time by 2 seconds. The assertions cover all ban types including the new absolute timestamp ban, ensuring the timing calculations are accurate.
105-106: LGTM! Persistence verification after restart.The test correctly verifies that the absolute timestamp ban (
192.168.0.2/32) persists after node restart and appears in the expected position in the ban list, ensuring the ban database correctly handles absolute timestamp bans.src/wallet/walletdb.cpp (2)
977-1024: LGTM! Efficient implementation that avoids unnecessary deserialization.The refactored method correctly extracts only transaction hashes without deserializing full wallet transactions, which should improve performance. The error handling and cursor management are properly implemented.
1028-1030: LGTM! Correctly updated to use the new method signature.The changes properly update the caller to use
FindWalletTxHashesinstead of the old method, with appropriate comment and variable naming updates that reflect the focus on transaction hashes rather than full transactions.src/wallet/wallet.cpp (1)
1115-1121: LGTM! Excellent error handling improvement.The change properly handles database write failures by checking for nullptr return from
AddToWalletand throwing a descriptive runtime error. This prevents silent failures and provides clear diagnostic information when database issues occur. The implementation correctly maintains the function's contract while making error conditions explicit.src/util/check.h (1)
10-27: Good refactoring to reduce header dependenciesMoving the error message formatting from the header to the implementation file is a clean improvement. This reduces compile-time dependencies and keeps implementation details properly encapsulated.
src/test/fuzz/txorphan.cpp (1)
37-148: Comprehensive fuzz test coverageThe fuzz test thoroughly exercises the refactored TxOrphanage interface, including the new peer-based orphan work set management. The transaction construction logic properly handles input/output generation and the test assertions verify expected behavior.
src/rpc/net.cpp (3)
199-206: Good improvement to handle disconnected peersSkipping peers when
GetNodeStateStats()fails prevents reporting stale data for disconnected peers. The comment clearly explains the race condition scenario.
789-792: Good validation for ban timestampsValidating that absolute ban timestamps aren't in the past prevents invalid ban entries and provides clear error feedback.
1016-1016: Appropriate type change for port validationUsing
uint16_tfor the port parameter ensures automatic validation of the port range (0-65535) and throws a clear error for out-of-range values.src/wallet/test/wallet_tests.cpp (2)
1490-1533: Well-designed test helper classes for database failure simulation.The
FailBatchandFailDatabaseclasses provide a clean and controlled way to simulate database write failures in unit tests. The design follows good testing practices by allowing fine-grained control over when operations should succeed or fail via them_passboolean flag.
1534-1595: Comprehensive test for database write failure scenarios.This test effectively validates the wallet's error handling when database writes fail. The test structure is well-organized:
- Setup phase: Creates wallet with controllable failure database
- Good case: Verifies normal transaction processing with balance updates
- Bad case: Verifies runtime error is thrown when database write fails
The test ensures that wallet state remains consistent and prevents silent failures when the database cannot persist transactions.
src/test/orphanage_tests.cpp (3)
23-31: Correct locking pattern for thread-safe class methods.The changes properly implement the internal locking pattern:
- Methods require that the mutex is NOT held when called (
EXCLUSIVE_LOCKS_REQUIRED(!m_mutex))- Methods acquire the mutex internally using
LOCK(m_mutex)This follows best practices for thread-safe class design and aligns with the refactoring from global to internal mutex management.
137-185: Excellent test for orphan reconsideration edge case.This test validates a critical scenario where orphan transactions spend from parent transactions that have more outputs than inputs. The test structure is well-designed:
- Setup: Creates parent transaction (1 input → 3 outputs) and orphan spending the last output
- Action: Calls
SetCandidatesByBlockto trigger orphan reconsideration- Verification: Confirms orphan is properly queued for reconsideration
The comment explaining the historical bug (cycling through
vininstead ofvout) provides valuable context for why this specific test case is important.
179-183: Good defensive programming with assertion check.The use of
Assert(ref)before dereferencing the transaction reference is a good practice that helps catch potential null pointer issues during testing. The test logic correctly verifies that the orphan transaction is available for reconsideration and matches the expected transaction hash.src/net_processing.cpp (2)
2015-2023: Correct implementation of block-triggered orphan processingThe implementation correctly uses the special peer ID
-1for block-triggered orphan processing as documented in the PR objectives. The loop ensures all orphan candidates triggered by the block are fully processed before continuing.
1049-1056: Thread safety of compact extra transactions verifiedAll calls to
AddToCompactExtraTransactionsoccur inside methods that require and assertg_msgproc_mutex(for example, withinPeerManagerImpl::ProcessMessage, which begins withAssertLockHeld(g_msgproc_mutex)). This satisfies theEXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex)annotation onAddToCompactExtraTransactions. No further locking changes are needed.src/txorphanage.cpp (6)
59-67: Good refactoring pattern for lock management.The separation of
EraseTx(public interface) and_EraseTx(internal implementation) avoids redundant locking when the method is called from other member functions that already hold the mutex.
100-117: Correct cleanup of peer work sets.The method properly cleans up the peer's work set before erasing their orphan transactions, preventing memory leaks when peers disconnect.
155-170: Good encapsulation of work set management.The method now manages peer work sets internally rather than requiring callers to pass external sets, improving encapsulation and thread safety.
178-199: Well-designed interface for peer-based orphan processing.The new
GetTxToReconsidermethod provides a clean interface for incremental orphan processing per peer, returning the originator information and signaling when more work remains.
243-248: Useful helper method for checking pending work.The
HaveMoreWorkmethod provides an efficient way to check if orphan processing is complete for a peer without modifying state.
201-210: Sentinel NodeId(-1) usage is safe and consistent
- NodeId is defined as a signed int64_t (using/typedef in multiple headers).
- The
-1sentinel is already used throughout the codebase (net_processing.cpp, orphanage tests, etc.) to represent “no peer.”- No conflicting uses of negative NodeId values were found.
This aligns with existing conventions—approving the change.
src/txorphanage.h (2)
60-64: Thread-safe Size() implementation.Good practice to include the lock acquisition within the
Size()method rather than requiring callers to hold the lock.
66-83: Proper thread safety annotations.The
mutablemutex allows locking in const methods, andGUARDED_BYannotations enable static analysis verification of correct locking discipline.
knst
left a comment
knst
left a comment
There was a problem hiding this comment.
check comments, otherwise LGTM
src/txorphanage.cpp
Outdated
| } | ||
| std::set<uint256> orphan_work_set; | ||
| for (const auto& ptx : block.vtx) { | ||
| AddChildrenToWorkSet(*ptx, orphan_work_set); |
4 participants
Additional information
Since dash#3121, Dash has limited the orphan transaction map by the accumulated size of all transactions in the map instead of transaction count (like upstream). bitcoin#25447 introduces a new fuzzing harness that operates on upstream assumptions.
To bridge the gap,
DEFAULT_MAX_ORPHAN_TRANSACTIONShas been defined in the harness to mean the highest transaction count assuming each transaction is of maximum size, which means the effective worst-case count is 100 transactions. This is already documented innet_processing.h(source) but has also been explicitly documented for the harness (source).Dash introduced an additional trigger for orphan reprocessing in dash#3139 to allow for faster processing when parents appear in a block. Currently, the implementation scans through the inputs of each transaction to check against the outpoint map (source).
This does not seem to be intended behavior as it's using the input index to search through outpoints when it should be iterating through outpoints like
AddChildrenToWorkSet()(source). Though despite this, the code still worked if either of the following conditions were met.find()si > vout.size()are wasted operations)The test case introduced in dash#3139 met this criteria and passed though the code would not work as expected if
vin.size() > vout.size()and we're scanningvinsfor outpoint positions)This has been rectified by modifying
GetCandidatesForBlock()to useAddChildrenToWorkSet()and adding a unit test to document this behavior.bitcoin#26295 does a substantial refactor of the transaction "orphanage" and part of that refactor is solidifying
ProcessOrphanTx()as dealing withPeers (as it's located innet_processingand not available through the interface). This conflicts with the capability introduced in dash#3139 as there is no peer to attribute it to, which is relevant as the newer code introduces a map between peer ID and the orphan work map (source).This has been worked around by using the ID
-1to mean block connection triggered orphan work (source, source). This is because no valid peer can be assigned a negative ID as it starts from 0 (source) and increments (source). An example of this kind of usage is InstantSend code (source).This change, alongside relevant changes to
ProcessOrphanTx()arguments and other Dash-specific code has been segmented out as a distinct commit to allow for ease of review. This also means that the commit that backports bitcoin#26295 cannot be compiled on its own due to build errors from not-yet adapted code.Additionally,
ProcessOrphanTx()assumesPeer-triggered orphan resolution and doesn't account for the possibility that the processing of a transaction may further populate the orphan set. This is a key assumption in block-based orphan processing and to retain this intended functionality,TxOrphanage::HaveMoreWork()has been defined to influence the return value ofProcessOrphanTx()to affect the resolution loop (source) to exhaust all work before continuing ahead.Breaking changes
None expected.
Checklist