Merge bitcoin-core/gui#742: Exit and show error if unrecognized command line args are present#973
Conversation
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.
…ate datadir fb0dbe9 docs: GetDataDirNet and GetDataDirBase don't create datadir (stickies-v) Pull request description: Since bitcoin#27073, the behaviour of `GetDataDir()` [changed](https://github.com/bitcoin/bitcoin/pull/27073/files#diff-19427b0dd1a791adc728c82e88f267751ba4f1c751e19262cac03cccd2822216L435-L443) to only return the datadir path, but not create it if non-existent. This also changed the behaviour of `GetDataDirNet()` and `GetDataDirBase()` but the docs do not yet reflect that. ACKs for top commit: TheCharlatan: ACK fb0dbe9 theStack: ACK fb0dbe9 willcl-ark: ACK fb0dbe9 Tree-SHA512: 3f10f4871df59882f3649c6d3b2362cae2f8a01ad0bd0c636c5608b0d177d279a2e8712930b819d6d3912e91fa6447b9e54507c33d8afe427f7f39002b013bfb
…coin#25641, bitcoin#25683, bitcoin#23679, bitcoin#26216, bitcoin#26295, bitcoin#26513, bitcoin#26515, bitcoin#26727, bitcoin#25374, bitcoin#25272, bitcoin#26408, bitcoin#25112, bitcoin#26702, bitcoin#25932, bitcoin#26822 (auxiliary backports: part 27) f45de22 merge bitcoin#26822: don't allow past absolute timestamp in `setban` (Kittywhiskers Van Gogh) 920e85d test: adjust mocktime bump to match upstream in `p2p_disconnect_ban` (Kittywhiskers Van Gogh) b1a95d8 merge bitcoin#25932: Simplify backtrack logic (Kittywhiskers Van Gogh) eff27a4 merge bitcoin#26702: drop unused `FindWalletTx` parameter and rename (Kittywhiskers Van Gogh) 62998f3 merge bitcoin#25112: Move error message formatting of NonFatalCheckError to cpp (Kittywhiskers Van Gogh) 27db044 merge bitcoin#26408: Remove spam from debug log (Kittywhiskers Van Gogh) 12acde7 merge bitcoin#25272: guard and alert about a wallet invalid state during chain sync (Kittywhiskers Van Gogh) 385306d merge bitcoin#25374: remove unused `create_confirmed_utxos` helper (Kittywhiskers Van Gogh) a8d29f9 merge bitcoin#26727: remove optional from fStateStats fields (Kittywhiskers Van Gogh) a0701e9 merge bitcoin#26515: skip getpeerinfo for a peer without CNodeStateStats (Kittywhiskers Van Gogh) c9eedc8 merge bitcoin#26513: Make static nLastFlush and nLastWrite Chainstate members (Kittywhiskers Van Gogh) 859d64e txorphanage: adapt block-based orphan reprocessing to refactor (Kittywhiskers Van Gogh) 4172102 merge bitcoin#26295: Replace global g_cs_orphans lock with local (Kittywhiskers Van Gogh) 3665919 fix: protect `m_orphan_tx_size` with `g_cs_orphans` (Kittywhiskers Van Gogh) f12e6cc fix: scan outputs in `GetCandidatesForBlock`, add regression test (Kittywhiskers Van Gogh) 42aa8a7 merge bitcoin#26216: Limit outpoints.size in txorphan target to avoid OOM (Kittywhiskers Van Gogh) b376c09 merge bitcoin#23679: Sanitize port in `addpeeraddress()` (Kittywhiskers Van Gogh) cd6c2b5 merge bitcoin#25683: log `nEvicted` message in LimitOrphans then return void (Kittywhiskers Van Gogh) 6cd09ef merge bitcoin#25641: Fix `-Wparentheses` gcc warning (Kittywhiskers Van Gogh) bc69f80 merge bitcoin#25624: Fix assert bug in txorphan target (Kittywhiskers Van Gogh) fc68188 merge bitcoin#25447: add low-level target for txorphanage (Kittywhiskers Van Gogh) Pull request description: ## Additional information * Since [dash#3121](dashpay#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](bitcoin#25447) introduces a new fuzzing harness that operates on upstream assumptions. To bridge the gap, `DEFAULT_MAX_ORPHAN_TRANSACTIONS` has 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 in `net_processing.h` ([source](https://github.com/dashpay/dash/blob/bd589dd6f434c0c6cc8f0c3e2cf299785bad6d07/src/net_processing.h#L31-L32)) but has also been explicitly documented for the harness ([source](https://github.com/dashpay/dash/blob/fc68188e3227dc0912ef3c508f6d45ff1d7614ee/src/test/fuzz/txorphan.cpp#L27-L28)). * Dash introduced an additional trigger for orphan reprocessing in [dash#3139](dashpay#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](https://github.com/dashpay/dash/blob/86071f9df4d052de6f1df64a5d269a67a6756d29/src/txorphanage.cpp#L186-L192)). 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](https://github.com/dashpay/dash/blob/86071f9df4d052de6f1df64a5d269a67a6756d29/src/txorphanage.cpp#L148C19-L159)). Though despite this, the code still worked _if_ either of the following conditions were met. * There are an even number of inputs and outputs (in which case a match is found as if iterating through the set of outputs); or * There are more inputs than outputs (in which case a match is found even if all `find()`s `i > vout.size()` are wasted operations) The test case introduced in [dash#3139](dashpay#3139) met this criteria and passed though the code would _not_ work as expected if * There are more outputs than inputs (in which case the loop will bail prematurely as `vin.size() > vout.size()` and we're scanning `vins` for outpoint positions) This has been rectified by modifying `GetCandidatesForBlock()` to use `AddChildrenToWorkSet()` and adding a unit test to document this behavior. * [bitcoin#26295](bitcoin#26295) does a substantial refactor of the transaction "orphanage" and part of that refactor is solidifying `ProcessOrphanTx()` as dealing with `Peer`s (as it's located in `net_processing` and not available through the interface). This conflicts with the capability introduced in [dash#3139](dashpay#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](https://github.com/bitcoin/bitcoin/blob/7082ce3e88d77456d60a5a66bd38807fbd66f311/src/txorphanage.h#L75-L76)). This has been worked around by using the ID `-1` to mean block connection triggered orphan work ([source](https://github.com/dashpay/dash/blob/859d64e98fc1b5bf613f526ef76e5fc66851fe13/src/txorphanage.cpp#L204-L209), [source](https://github.com/dashpay/dash/blob/859d64e98fc1b5bf613f526ef76e5fc66851fe13/src/net_processing.cpp#L2015-L2024)). This is because no valid peer can be assigned a negative ID as it starts from 0 ([source](https://github.com/dashpay/dash/blob/bd589dd6f434c0c6cc8f0c3e2cf299785bad6d07/src/net.h#L1735)) and increments ([source](https://github.com/dashpay/dash/blob/bd589dd6f434c0c6cc8f0c3e2cf299785bad6d07/src/net.cpp#L3897-L3900)). An example of this kind of usage is InstantSend code ([source](https://github.com/dashpay/dash/blob/859d64e98fc1b5bf613f526ef76e5fc66851fe13/src/instantsend/signing.cpp#L252)). * 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](bitcoin#26295) cannot be compiled on its own due to build errors from not-yet adapted code. * Additionally, `ProcessOrphanTx()` assumes `Peer`-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 of `ProcessOrphanTx()` to affect the resolution loop ([source](https://github.com/dashpay/dash/blob/859d64e98fc1b5bf613f526ef76e5fc66851fe13/src/net_processing.cpp#L2015-L2024)) to exhaust all work before continuing ahead. ## Breaking changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: knst: utACK f45de22 UdjinM6: utACK f45de22 Tree-SHA512: 8f6a46984091d6f43d4fb97d13edc9dfa7affa1eb79ab37e14f49078b5d738308c6044f9b2b3f5e0d1fe2efde27b7bae14727e14e1ccd5d21eb695162d9c4615
…GetDataDirBase don't create datadir c5b325a Merge bitcoin#27176: docs: GetDataDirNet and GetDataDirBase don't create datadir (fanquake) Pull request description: ## Issue being fixed or feature implemented Since bitcoin#27073 (backported via dashpay#6780), the behaviour of `GetDataDir()` [changed](https://github.com/bitcoin/bitcoin/pull/27073/files#diff-19427b0dd1a791adc728c82e88f267751ba4f1c751e19262cac03cccd2822216L435-L443) to only return the datadir path, but not create it if non-existent. This also changed the behaviour of `GetDataDirNet()` and `GetDataDirBase()` but the docs do not yet reflect that. ## How Has This Been Tested? N/A ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK c5b325a PastaPastaPasta: utACK c5b325a Tree-SHA512: ff3fa0daea16ddc2d4140e65b7a02926fa2be9adebd6f756868ca985c49a9963950ba1ced1117b3ca289cdda6c7d44e72f51158ced1d3abb5e2f965b81f00896
…ohd` rpc 746e5f8 fix: always leave the wallet in a locked state afer `upgradetohd` (UdjinM6) b19f463 fix(rpc): return correct error codes in `upgradetohd` rpc (UdjinM6) Pull request description: ## Issue being fixed or feature implemented `upgradetohd` rpc returns wrong error codes ## What was done? Move wallet "upgrade to HD" code used in rpc only to `upgradetohd` rpc, return correct error codes. We also don't need all that logic to extract wallet master encryption key because it's already known when wallet is unlocked. ## How Has This Been Tested? Run tests ## Breaking Changes `upgradetohd` should return correct error codes now which could probably break things if you relied on them, see tests ## Checklist: - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: knst: utACK 746e5f8 Tree-SHA512: 829d1f9d805f7a87f628a9c7f4ae93cb545c8113cea97f430639165dae124db17d389c2f5c663214b074da8ae541435ab7df2b672f78370fe883ac77b3cfed42
Complete backport of bitcoin#28643 by adding CI_RETRY_EXE to the qa-assets git clone in Dash's CI structure. This matches Bitcoin's intent to add retry capability to all git clone operations. Resolves validation issues while preserving Bitcoin intent.
WalkthroughThis update introduces significant refactoring and enhancements across core, wallet, test, and utility components. Major changes include internalizing orphan transaction synchronization with per-peer work sets and mutex-protected access, refactoring HD wallet upgrade logic, improving error handling and test coverage, and updating documentation and test scripts. Several method signatures and class interfaces are updated, and new tests and fuzz targets are added. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 14
🔭 Outside diff range comments (2)
src/wallet/walletdb.cpp (1)
977-1024: Critical mismatch: walletdb.cpp changes are not part of the “unrecognized command line args” PRThe merge commit afa081a only modifies src/qt/bitcoin.cpp; there are no changes to src/wallet/walletdb.cpp. The diff in walletdb.cpp is unrelated to the stated objective and must be removed or moved to a separate PR.
Files needing attention:
- src/wallet/walletdb.cpp (remove unintended edits)
- Ensure only src/qt/bitcoin.cpp contains the backport for CLI-argument validation
src/net_processing.cpp (1)
1-6369: Critical: PR objectives don't match the code changesThe PR is titled "Exit and show error if unrecognized command line args are present" but all the changes in this file are related to orphan transaction processing refactoring. This is a complete mismatch.
Please verify:
- Is this the correct code for PR #973?
- Should the PR title/description be updated to reflect orphan transaction changes?
- Are there missing files that implement the command line argument validation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (30)
ci/dash/lint-tidy.sh(1 hunks)ci/lint/04_install.sh(1 hunks)ci/test/04_install.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/util/system.h(0 hunks)src/validation.cpp(4 hunks)src/validation.h(2 hunks)src/wallet/coinselection.cpp(1 hunks)src/wallet/rpc/wallet.cpp(1 hunks)src/wallet/test/wallet_tests.cpp(1 hunks)src/wallet/wallet.cpp(2 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(2 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)test/functional/wallet_upgradetohd.py(1 hunks)
💤 Files with no reviewable changes (5)
- src/init.cpp
- src/util/system.h
- test/functional/test_framework/util.py
- src/test/fuzz/process_message.cpp
- src/test/fuzz/process_messages.cpp
🧰 Additional context used
📓 Path-based instructions (6)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
C++20 codebase should be placed under src/
Files:
src/wallet/coinselection.cppsrc/util/check.hsrc/wallet/walletdb.hsrc/test/fuzz/txorphan.cppsrc/wallet/test/wallet_tests.cppsrc/wallet/walletdb.cppsrc/validation.hsrc/validation.cppsrc/util/check.cppsrc/wallet/rpc/wallet.cppsrc/wallet/wallet.cppsrc/rpc/net.cppsrc/net_processing.cppsrc/wallet/wallet.hsrc/txorphanage.hsrc/txorphanage.cppsrc/test/orphanage_tests.cpp
**
⚙️ CodeRabbit Configuration File
**: # CodeRabbit AI Review Instructions for Dash BackportsYour Role
You are reviewing Bitcoin Core backports to Dash Core. Your ONLY job is to validate that the Dash commit faithfully represents the original Bitcoin commit with minimal, necessary adaptations.
Critical Validation Rules
1. File Operations Must Match (AUTO-REJECT if violated)
- If Bitcoin modifies an existing file → Dash MUST modify (not create new)
- If Bitcoin creates a new file → Dash creates
- If Bitcoin deletes a file → Dash deletes
- Common failure: Bitcoin modifies keys.txt, Dash creates new file with 58 keys
2. Size Ratio Check (80-150% of Bitcoin)
- Count functional lines changed (exclude comments/whitespace)
- Dash changes should be 80-150% of Bitcoin's size
- Red flag: 2-line Bitcoin fix becoming 150+ lines in Dash
3. No Scope Creep
- Reject if you see: "TODO:", "FIXME:", "while we're here", "also fix"
- No unrelated refactoring or style changes
- Only Bitcoin's intended changes + minimal Dash adaptations
4. Bitcoin-Specific Code Detection
- Auto-reject witness/segwit code:
msg_wtxidrelay,MSG_WTX, witness imports- Auto-reject RBF (replace-by-fee) functionality
- Note: PSBT is supported in Dash (don't flag)
5. Mandatory Adaptations Only
bitcoin→dashin strings/pathsBitcoin→Dashin user-facing text- Port numbers: 8332→9998 (RPC), 8333→9999 (P2P)
- Hardcoded test values specific to Dash
- No other changes unless absolutely required
6. Completeness Check
- All files changed in Bitcoin must be present
- Extra files need clear justification (Dash-specific compatibility)
- Missing files = incomplete backport
Review Process
- First: Check file operations match exactly
- Second: Calculate size ratio
- Third: Scan for scope creep patterns
- Fourth: Detect Bitcoin-specific code
- Fifth: Verify all changes are minimal adaptations
Output Format
VALIDATION: [PASS/FAIL] File Operatio...
Files:
src/wallet/coinselection.cppci/dash/lint-tidy.shtest/functional/p2p_disconnect_ban.pysrc/util/check.hci/lint/04_install.shtest/functional/test_framework/test_node.pysrc/wallet/walletdb.htest/functional/wallet_upgradetohd.pysrc/test/fuzz/txorphan.cppsrc/wallet/test/wallet_tests.cppsrc/wallet/walletdb.cppsrc/Makefile.test.includetest/functional/rpc_net.pysrc/validation.hsrc/validation.cppsrc/util/check.cppsrc/wallet/rpc/wallet.cppsrc/wallet/wallet.cppsrc/rpc/net.cppci/test/04_install.shsrc/net_processing.cppsrc/wallet/wallet.hsrc/txorphanage.hsrc/txorphanage.cppsrc/test/orphanage_tests.cpp
ci/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the ci directory (continuous integration)
Files:
ci/dash/lint-tidy.shci/lint/04_install.shci/test/04_install.sh
test/functional/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Functional tests should be placed in test/functional/ and written in Python
Files:
test/functional/p2p_disconnect_ban.pytest/functional/test_framework/test_node.pytest/functional/wallet_upgradetohd.pytest/functional/rpc_net.py
src/util/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/util/check.hsrc/util/check.cpp
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Unit tests 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 (15)
📓 Common learnings
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-28T22:03:12.364Z
Learning: During multiple verification attempts of Bitcoin Core commit 06d469c26b backport to Dash PR #566, DashCoreAutoGuix consistently identified scope creep in interface_usdt_utxocache.py where additional pruning test functionality was added beyond the original Bitcoin commit. The user provided comprehensive fixes including both scope creep removal and missing mempool test file additions, but couldn't push due to authentication restrictions. The scope creep fix was identified as the priority to resolve CI failures.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-28T20:34:29.061Z
Learning: During Dash backport verification of Bitcoin Core commit 06d469c26b, scope creep was detected when additional pruning test functionality was added to interface_usdt_utxocache.py beyond what was in the original Bitcoin commit. The fix involved removing the extra test block while maintaining the core compiler flag fixes for USDT compilation errors.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-31T01:14:55.631Z
Learning: DashCoreAutoGuix successfully completed a complex Bitcoin Core backport (PR #29412) for block mutation detection by implementing the IsBlockMutated function, adding net processing integration, creating comprehensive unit tests, and properly adapting all Bitcoin-specific witness code for Dash compatibility. The backport maintains full security functionality while respecting Dash's non-witness transaction architecture.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-29T21:29:32.827Z
Learning: DashCoreAutoGuix successfully fixed scope creep in Bitcoin Core commit fcdb39d3ee backport by removing the parse test case from src/test/uint256_tests.cpp that was not part of the original Bitcoin commit. The fix was implemented in commit 16748115ce and verified through range-diff analysis.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-28T19:54:21.426Z
Learning: In Dash backports from Bitcoin Core, including necessary compilation fixes (such as API compatibility changes like UniValue get_int() → getInt<int>()) alongside the core backport is standard and expected practice. These compatibility fixes ensure the backported code compiles in Dash's evolved codebase while preserving Bitcoin's original functionality and intent.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-27T22:35:10.176Z
Learning: In Dash backports, src/dashbls files are vendored dependencies that should not be modified during Bitcoin Core backports unless there is specific justification. Unauthorized modifications to vendored dependencies should be removed to maintain code integrity.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-29T22:13:54.797Z
Learning: In Dash backports from Bitcoin Core, witness transaction-related code (MSG_WTX, wtxid) should be replaced with regular transaction handling (MSG_TX, txid) for compatibility, as demonstrated in the p2p_filter.py test fix where MSG_WTX was replaced with MSG_TX and irr_wtxid usage was replaced with irr_txid.
Learnt from: knst
PR: DashCoreAutoGuix/dash#436
File: src/univalue/include/univalue.h:86-86
Timestamp: 2025-08-04T18:16:45.145Z
Learning: UniValue (src/univalue) is no longer a vendored dependency in Dash as of PR #6775, so modifications to UniValue files are permitted when needed for Bitcoin Core backports and other legitimate changes.
Learnt from: CR
PR: DashCoreAutoGuix/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T23:09:09.522Z
Learning: Extend Bitcoin Core functionality through composition and extension, not modification
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-30T14:45:15.700Z
Learning: In Dash backports from Bitcoin Core test files, the `address_to_scriptpubkey` function should be imported from `test_framework.wallet` instead of `test_framework.address` as the import location differs between Bitcoin and Dash test frameworks.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-30T14:45:15.700Z
Learning: The `add_wallet_options` method is not available in Dash's test framework and should be removed when backporting Bitcoin Core tests that use this method, as demonstrated in wallet_rescan_unconfirmed.py.
📚 Learning: during dash backport verification of bitcoin core commit 06d469c, scope creep was detected when a...
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-28T20:34:29.061Z
Learning: During Dash backport verification of Bitcoin Core commit 06d469c26b, scope creep was detected when additional pruning test functionality was added to interface_usdt_utxocache.py beyond what was in the original Bitcoin commit. The fix involved removing the extra test block while maintaining the core compiler flag fixes for USDT compilation errors.
Applied to files:
src/wallet/coinselection.cppci/dash/lint-tidy.shtest/functional/p2p_disconnect_ban.pytest/functional/test_framework/test_node.pytest/functional/wallet_upgradetohd.pysrc/test/fuzz/txorphan.cppsrc/wallet/test/wallet_tests.cppsrc/Makefile.test.includesrc/wallet/rpc/wallet.cppsrc/wallet/wallet.cppsrc/test/orphanage_tests.cpp
📚 Learning: dashcoreautoguix successfully fixed scope creep in bitcoin core commit fcdb39d backport by removi...
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-29T21:29:32.827Z
Learning: DashCoreAutoGuix successfully fixed scope creep in Bitcoin Core commit fcdb39d3ee backport by removing the parse test case from src/test/uint256_tests.cpp that was not part of the original Bitcoin commit. The fix was implemented in commit 16748115ce and verified through range-diff analysis.
Applied to files:
src/wallet/coinselection.cppci/dash/lint-tidy.shtest/functional/wallet_upgradetohd.pysrc/wallet/test/wallet_tests.cppsrc/wallet/walletdb.cppsrc/Makefile.test.includesrc/wallet/rpc/wallet.cppsrc/wallet/wallet.cppsrc/test/orphanage_tests.cpp
📚 Learning: during multiple verification attempts of bitcoin core commit 06d469c backport to dash pr #566, da...
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-28T22:03:12.364Z
Learning: During multiple verification attempts of Bitcoin Core commit 06d469c26b backport to Dash PR #566, DashCoreAutoGuix consistently identified scope creep in interface_usdt_utxocache.py where additional pruning test functionality was added beyond the original Bitcoin commit. The user provided comprehensive fixes including both scope creep removal and missing mempool test file additions, but couldn't push due to authentication restrictions. The scope creep fix was identified as the priority to resolve CI failures.
Applied to files:
src/wallet/coinselection.cppci/dash/lint-tidy.shtest/functional/wallet_upgradetohd.pysrc/test/fuzz/txorphan.cppsrc/wallet/test/wallet_tests.cppsrc/Makefile.test.includesrc/wallet/rpc/wallet.cppsrc/wallet/wallet.cppci/test/04_install.shsrc/txorphanage.cppsrc/test/orphanage_tests.cpp
📚 Learning: applies to src/fuzz/**/*.{cpp,h,cc,cxx,hpp} : fuzzing harnesses should be placed in src/fuzz/...
Learnt from: CR
PR: DashCoreAutoGuix/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T23:09:09.522Z
Learning: Applies to src/fuzz/**/*.{cpp,h,cc,cxx,hpp} : Fuzzing harnesses should be placed in src/fuzz/
Applied to files:
ci/dash/lint-tidy.shsrc/test/fuzz/txorphan.cppsrc/Makefile.test.includeci/test/04_install.sh
📚 Learning: when backporting bitcoin core commits that use python's textwrap.dedent() function in test files, th...
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-29T17:13:35.087Z
Learning: When backporting Bitcoin Core commits that use Python's textwrap.dedent() function in test files, the textwrap import statement needs to be explicitly added if it's missing in the Dash test file.
Applied to files:
ci/dash/lint-tidy.shtest/functional/test_framework/test_node.pytest/functional/wallet_upgradetohd.py
📚 Learning: applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : unit tests should be placed in s...
Learnt from: CR
PR: DashCoreAutoGuix/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T23:09:09.522Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Applied to files:
ci/dash/lint-tidy.shsrc/wallet/test/wallet_tests.cppsrc/Makefile.test.includesrc/test/orphanage_tests.cpp
📚 Learning: dashcoreautoguix successfully completed a complex bitcoin core backport (pr bitcoin#29412) for block mutati...
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-31T01:14:55.631Z
Learning: DashCoreAutoGuix successfully completed a complex Bitcoin Core backport (PR #29412) for block mutation detection by implementing the IsBlockMutated function, adding net processing integration, creating comprehensive unit tests, and properly adapting all Bitcoin-specific witness code for Dash compatibility. The backport maintains full security functionality while respecting Dash's non-witness transaction architecture.
Applied to files:
ci/dash/lint-tidy.shsrc/wallet/test/wallet_tests.cppsrc/wallet/rpc/wallet.cppsrc/wallet/wallet.cppsrc/test/orphanage_tests.cpp
📚 Learning: applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : masternode lists use immutable data struct...
Learnt from: CR
PR: DashCoreAutoGuix/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T23:09:09.522Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists use immutable data structures from the Immer library for thread safety
Applied to files:
ci/dash/lint-tidy.shsrc/validation.hsrc/txorphanage.h
📚 Learning: the `add_wallet_options` method is not available in dash's test framework and should be removed when...
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-30T14:45:15.700Z
Learning: The `add_wallet_options` method is not available in Dash's test framework and should be removed when backporting Bitcoin Core tests that use this method, as demonstrated in wallet_rescan_unconfirmed.py.
Applied to files:
test/functional/test_framework/test_node.pytest/functional/wallet_upgradetohd.pysrc/wallet/test/wallet_tests.cppsrc/wallet/wallet.cppsrc/wallet/wallet.h
📚 Learning: in dash backports from bitcoin core, witness transaction-related code (msg_wtx, wtxid) should be rep...
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-29T22:13:54.797Z
Learning: In Dash backports from Bitcoin Core, witness transaction-related code (MSG_WTX, wtxid) should be replaced with regular transaction handling (MSG_TX, txid) for compatibility, as demonstrated in the p2p_filter.py test fix where MSG_WTX was replaced with MSG_TX and irr_wtxid usage was replaced with irr_txid.
Applied to files:
src/wallet/walletdb.htest/functional/wallet_upgradetohd.pysrc/wallet/test/wallet_tests.cppsrc/wallet/walletdb.cppsrc/Makefile.test.includesrc/wallet/wallet.cpp
📚 Learning: in dash backports from bitcoin core test files, the `address_to_scriptpubkey` function should be imp...
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-30T14:45:15.700Z
Learning: In Dash backports from Bitcoin Core test files, the `address_to_scriptpubkey` function should be imported from `test_framework.wallet` instead of `test_framework.address` as the import location differs between Bitcoin and Dash test frameworks.
Applied to files:
src/wallet/walletdb.htest/functional/wallet_upgradetohd.pysrc/wallet/test/wallet_tests.cppsrc/wallet/walletdb.cppsrc/wallet/rpc/wallet.cppsrc/wallet/wallet.cpp
📚 Learning: applies to src/util/**/*.{cpp,h,cc,cxx,hpp} : dash uses unordered_lru_cache for efficient caching wi...
Learnt from: CR
PR: DashCoreAutoGuix/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T23:09:09.522Z
Learning: Applies to src/util/**/*.{cpp,h,cc,cxx,hpp} : Dash uses unordered_lru_cache for efficient caching with LRU eviction
Applied to files:
src/validation.h
📚 Learning: applies to src/evo/specialtx.h : special transactions use payload extensions as defined in src/evo/s...
Learnt from: CR
PR: DashCoreAutoGuix/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T23:09:09.522Z
Learning: Applies to src/evo/specialtx.h : Special transactions use payload extensions as defined in src/evo/specialtx.h
Applied to files:
src/rpc/net.cppsrc/net_processing.cpp
📚 Learning: applies to ci/** : unless specifically prompted, avoid making changes to the ci directory (continuou...
Learnt from: CR
PR: DashCoreAutoGuix/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T23:09:09.522Z
Learning: Applies to ci/** : Unless specifically prompted, avoid making changes to the ci directory (continuous integration)
Applied to files:
ci/test/04_install.sh
🪛 Ruff (0.12.2)
test/functional/p2p_disconnect_ban.py
61-61: Boolean positional value in function call
(FBT003)
82-82: Boolean positional value in function call
(FBT003)
106-106: Boolean positional value in function call
(FBT003)
🪛 Shellcheck (0.10.0)
ci/test/04_install.sh
[info] 109-109: Double quote to prevent globbing and word splitting.
(SC2086)
🔇 Additional comments (35)
src/validation.h (2)
34-34: Header inclusion looks correct.
<chrono>is required for the newstd::chrono::microsecondsmembers and the insertion point keeps include blocks alphabetically ordered (atomic→chrono→map). No issues spotted.
813-815: Refine validation‐timer cleanup scopeThe wallet’s
nLastFlushed/nLastWalletUpdateinwalletdb.cpp/db.hare unrelated to this change and can be ignored. Focus the search on the validation layer only:rg -n --no-heading 'nLastWrite|nLastFlush' src/validation* src/consensus* src/validation.hIt should return no hits. If you do find any remaining
nLastWrite/nLastFlushin those files, replace them with the new members (m_last_write/m_last_flush). Also double-check that all accesses remain protected by::cs_mainto maintain thread safety.Likely an incorrect or invalid review comment.
src/validation.cpp (4)
2747-2752: Member variable initialization logic looks correct.The initialization check
m_last_write.count() == 0properly handles the first-time setup, and both variables are initialized to the current time to avoid immediate writes/flushes after startup.
2760-2762: Periodic timing logic correctly updated to use member variables.The conditions for
fPeriodicWriteandfPeriodicFlushproperly use the member variablesm_last_writeandm_last_flushinstead of static locals, maintaining the same timing logic.
2793-2793: Write timestamp update correctly placed.The
m_last_writeupdate occurs after successful block/index writes, maintaining proper timing for the next periodic write cycle.
2819-2819: Flush timestamp update correctly placed.The
m_last_flushupdate occurs after successful full flush operations, maintaining proper timing for the next periodic flush cycle.ci/lint/04_install.sh (1)
18-22: CI directory modified – confirm necessity vs. “no-CI-changes” policyOur internal guideline warns against touching
ci/**unless the change is required for the specific backport.
Please double-check that upstream Bitcoin commit afa081a actually changes this line; otherwise, drop it to avoid scope creep.
(If it is in the original commit, no action needed.)ci/test/04_install.sh (1)
106-111: CI directory change—ensure it belongs to the backportSimilar to
ci/lint/04_install.sh, confirm this modification is present in the original Bitcoin commit; otherwise it violates the “no CI changes unless required” rule.src/wallet/coinselection.cpp (1)
101-101: LGTM - Good refactoring practice.The use of the pre-computed
is_feerate_highvariable improves code readability compared to the inline expression. The logic remains identical.test/functional/rpc_net.py (1)
344-346: LGTM! Proper input validation testing.The test correctly validates that invalid port numbers (negative and out of range) are rejected by the
addpeeraddressRPC with appropriate error messages.test/functional/wallet_upgradetohd.py (1)
206-206: LGTM! Error handling unification.The test correctly unifies the expected error code to -14 for incorrect wallet passphrases, removing conditional branching based on wallet type. This aligns with the refactored RPC error handling.
src/wallet/walletdb.h (1)
236-236: LGTM! Interface simplification.Renaming
FindWalletTxtoFindWalletTxHashesand removing the wallet transaction output parameter appropriately focuses the method on hash extraction only, avoiding unnecessary transaction deserialization.test/functional/p2p_disconnect_ban.py (1)
105-106: LGTM! Correct ban list verification.The test correctly validates the ban list persistence and ordering after node restart.
src/util/check.cpp (1)
7-26: LGTM! Well-implemented exception class.The
NonFatalCheckErrorconstructor properly formats detailed error messages with source location, version information, and bug report instructions. The conditional config header inclusion and explicit standard library includes are appropriate.src/wallet/test/wallet_tests.cpp (1)
1490-1595: VALIDATION: FAIL - Scope mismatch with Bitcoin commitThis PR claims to backport Bitcoin Core commit afa081a which adds error handling for unrecognized command line arguments. However, this change adds a wallet database failure test which is completely unrelated to command line argument parsing.
File Operations: MISMATCH - Bitcoin commit likely modifies command line parsing code, not wallet tests
Scope Creep: DETECTED - Adding unrelated wallet database failure tests
Completeness: UNKNOWN - Cannot verify without seeing the actual Bitcoin commit changesPlease provide the correct files that implement command line argument validation, or explain why wallet database tests are being added in a command line argument handling backport.
Likely an incorrect or invalid review comment.
src/test/fuzz/txorphan.cpp (1)
1-149: VALIDATION: FAIL - Unrelated to Bitcoin commit objectiveThis is a new fuzz test for orphan transaction handling, which is completely unrelated to the stated objective of "Exit and show error if unrecognized command line args are present".
Expected changes: Command line argument validation in files like
src/init.cpp,src/qt/dash.cpp, or similar
Actual changes: Orphan transaction fuzz testsThis appears to be either:
- Wrong files provided for review
- Wrong PR metadata (incorrect Bitcoin commit reference)
- Massive scope creep combining multiple unrelated changes
Please clarify what Bitcoin commit is actually being backported and provide the relevant files.
Likely an incorrect or invalid review comment.
src/wallet/wallet.cpp (2)
356-366: Good refactoring of HD chain generation!The refactoring properly encapsulates HD chain generation within the ScriptPubKeyMan and uses the secure
WithEncryptionKeypattern for handling encryption keys. The error handling for missing Legacy ScriptPubKeyMan is appropriate.
1115-1121: Improved error handling for DB write failures.Throwing a runtime error on DB write failure is the correct approach here, as it prevents the wallet from continuing with an inconsistent state between memory and disk. The error message clearly indicates the failure point.
src/test/orphanage_tests.cpp (4)
1-186: Critical mismatch between PR description and actual changesThe PR is described as "Exit and show error if unrecognized command line args are present" (backport of bitcoin-core/gui#742), but this file contains orphan transaction management test updates, not command line argument validation.
This suggests either:
- Wrong files provided for review
- Incorrect PR description
- Mixed changes in single PR
Without the original Bitcoin commit afa081a, I cannot validate if this is a faithful backport.
#!/bin/bash # Check if the original Bitcoin commit matches the described functionality echo "Searching for Bitcoin Core commit afa081a39bde77d3959aa35b6e6c75a2fe988d68" curl -s "https://api.github.com/repos/bitcoin/bitcoin/commits/afa081a39bde77d3959aa35b6e6c75a2fe988d68" | jq -r '.commit.message'Likely an incorrect or invalid review comment.
23-27: LGTM - Internal locking implementationThe CountOrphans() method correctly implements internal locking with m_mutex instead of requiring external lock management. The EXCLUSIVE_LOCKS_REQUIRED annotation properly indicates the method manages its own locking.
29-37: LGTM - Consistent internal locking patternRandomOrphan() follows the same internal locking pattern as CountOrphans(), properly acquiring m_mutex internally. The implementation is thread-safe and consistent.
137-184: LGTM - Comprehensive test for SetCandidatesByBlockThe new test case properly validates the SetCandidatesByBlock functionality by:
- Creating a transaction with more outputs than inputs
- Adding an orphan that spends from the last output
- Verifying the orphan is correctly reconsidered after block inclusion
The test addresses the specific edge case mentioned in the comments about cycling through vouts instead of vins.
src/rpc/net.cpp (5)
1-1143: Critical mismatch - RPC changes don't match PR descriptionSimilar to the test file, these RPC improvements have no relation to "Exit and show error if unrecognized command line args are present". This file contains networking RPC enhancements, not command line argument validation.
Likely an incorrect or invalid review comment.
199-206: LGTM - Proper handling of disconnected peersThe addition of the state stats check prevents reporting peers that were disconnected between GetNodeStats() and GetNodeStateStats() calls. This eliminates potential race conditions and ensures only valid peer data is returned.
219-222: LGTM - Simplified service flags handlingThe direct construction of ServiceFlags from statestats.their_services and early placement of relaytxes field improves code clarity and organization.
785-787: LGTM - Important validation for absolute timestampsAdding validation to reject absolute ban timestamps in the past prevents logical errors where bans would be immediately expired. This is a sensible security improvement.
1012-1012: LGTM - Explicit port type castingUsing getInt<uint16_t>() for port parameter provides explicit type safety and validates the port is within valid range (0-65535).
src/net_processing.cpp (6)
711-721: LGTM - Orphan transaction processing refactored to use NodeIdThe signature change from accepting a work set to accepting a NodeId with bool return is a clean refactoring that centralizes orphan work set management in TxOrphanage.
2015-2025: LGTM - Block-sourced orphan processing with special NodeIdUsing -1 as the NodeId for block-sourced orphans is appropriate since they don't originate from a specific peer. The loop correctly processes all candidates.
3207-3247: LGTM - ProcessOrphanTx implementation updated for per-peer trackingThe implementation correctly uses the new TxOrphanage API with NodeId-based orphan tracking. The loop processes orphans individually and properly returns whether more work remains.
3223-3223: LGTM - AddChildrenToWorkSet calls updated with NodeId parameterAll calls to AddChildrenToWorkSet correctly include the node_id parameter, maintaining consistency with the new per-peer orphan tracking design.
Also applies to: 3503-3503, 4503-4503
2018-2019: LGTM - Proper locking for ProcessOrphanTx callsAll ProcessOrphanTx calls correctly hold cs_main lock as required. The orphan processing maintains thread safety.
Also applies to: 3209-3209, 5357-5358
5355-5365: LGTM - Message processing correctly handles orphan workThe orphan processing logic correctly uses the new API and returns early when more orphans remain, ensuring orphans are processed before new messages.
src/txorphanage.cpp (1)
1-249: VALIDATION: CANNOT COMPLETEI cannot properly validate this Dash backport without the original Bitcoin Core commit reference. The PR description mentions "bitcoin#742" but that PR is about command line arguments, not orphan transaction handling.
To perform a proper backport validation, I need:
- The correct Bitcoin Core commit hash or PR number
- Access to the original Bitcoin changes to compare file operations, size ratios, and ensure no scope creep
The changes shown here appear to be a significant refactoring of orphan transaction handling:
- Replacing global mutex
g_cs_orphanswith class memberm_mutex- Adding per-peer work sets (
m_peer_work_set)- Refactoring method signatures and adding new methods
Without the Bitcoin reference, I cannot verify if:
- These are the same changes Bitcoin made
- The size ratio is appropriate (80-150%)
- There's any scope creep beyond necessary Dash adaptations
- All Bitcoin changes are included
Please provide the correct Bitcoin Core commit hash or PR number for this orphan transaction refactoring.
src/txorphanage.h (1)
1-111: Backport validation requires Bitcoin referenceThis file shows corresponding header changes for the orphan transaction refactoring, but validation cannot proceed without the original Bitcoin commit.
The changes include:
- New private mutex
m_mutexreplacing global synchronization- New per-peer work set map
m_peer_work_set- Method signature changes and new methods like
GetTxToReconsider,SetCandidatesByBlock,HaveMoreWork- Private
_EraseTxhelper method
| "src/init" \ | ||
| "src/rpc/fees.cpp" \ | ||
| "src/rpc/signmessage.cpp" \ | ||
| "src/test/fuzz/txorphan.cpp" \ |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify these files are part of the original Bitcoin commit.
Adding src/test/fuzz/txorphan.cpp and src/util/check.cpp to the linting scope suggests new source files that need quality checking. However, these appear unrelated to the stated PR objective of handling unrecognized command line arguments from bitcoin-core/gui#742.
Also applies to: 26-26
🏁 Script executed:
#!/bin/bash
# Description: Check if these files were added in the original Bitcoin commit
# Expected: Should not find these files if they're scope creep
echo "Checking for txorphan.cpp and check.cpp in original Bitcoin commit..."
curl -s "https://api.github.com/repos/bitcoin/bitcoin/commits/afa081a39bde77d3959aa35b6e6c75a2fe988d68" | jq -r '.files[] | select(.filename | contains("txorphan") or contains("check.cpp")) | .filename' 2>/dev/null || echo "No matching files found in original commit"Length of output: 351
Remove unrelated lint targets from this PR
The additions of src/test/fuzz/txorphan.cpp and src/util/check.cpp to ci/dash/lint-tidy.sh aren’t part of the original Bitcoin commit and fall outside the GUI CLI‐validation scope. Please remove or isolate these entries:
• ci/dash/lint-tidy.sh line 23: "src/test/fuzz/txorphan.cpp"
• ci/dash/lint-tidy.sh line 26: "src/util/check.cpp"
- "src/test/fuzz/txorphan.cpp" \
…
- "src/util/check.cpp" \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "src/test/fuzz/txorphan.cpp" \ | |
| lint_targets=( | |
| "src/app/gui.cpp" \ | |
| "src/app/cli.cpp" \ | |
| - "src/test/fuzz/txorphan.cpp" \ | |
| - "src/util/check.cpp" \ | |
| "src/main.cpp" \ | |
| ) |
🤖 Prompt for AI Agents
In ci/dash/lint-tidy.sh at lines 23 and 26, remove the entries
"src/test/fuzz/txorphan.cpp" and "src/util/check.cpp" from the lint targets list
because they are unrelated to the original Bitcoin commit and outside the GUI
CLI-validation scope. This will ensure the linting only covers relevant files.
| if [ "$RUN_FUZZ_TESTS" = "true" ]; then | ||
| export DIR_FUZZ_IN=${DIR_QA_ASSETS}/fuzz_seed_corpus/ | ||
| if [ ! -d "$DIR_FUZZ_IN" ]; then | ||
| CI_EXEC git clone --depth=1 https://github.com/bitcoin-core/qa-assets "${DIR_QA_ASSETS}" | ||
| CI_EXEC ${CI_RETRY_EXE} git clone --depth=1 https://github.com/bitcoin-core/qa-assets "${DIR_QA_ASSETS}" | ||
| fi | ||
| ( |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Quote ${CI_RETRY_EXE} to avoid word-splitting (SC2086)
ShellCheck flags this line; the variable may contain spaces or options (e.g. timeout 120s).
Quoting preserves it as a single argv element:
- CI_EXEC ${CI_RETRY_EXE} git clone --depth=1 https://github.com/bitcoin-core/qa-assets "${DIR_QA_ASSETS}"
+ CI_EXEC "${CI_RETRY_EXE}" git clone --depth=1 https://github.com/bitcoin-core/qa-assets "${DIR_QA_ASSETS}"[nitpick]
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ "$RUN_FUZZ_TESTS" = "true" ]; then | |
| export DIR_FUZZ_IN=${DIR_QA_ASSETS}/fuzz_seed_corpus/ | |
| if [ ! -d "$DIR_FUZZ_IN" ]; then | |
| CI_EXEC git clone --depth=1 https://github.com/bitcoin-core/qa-assets "${DIR_QA_ASSETS}" | |
| CI_EXEC ${CI_RETRY_EXE} git clone --depth=1 https://github.com/bitcoin-core/qa-assets "${DIR_QA_ASSETS}" | |
| fi | |
| ( | |
| if [ "$RUN_FUZZ_TESTS" = "true" ]; then | |
| export DIR_FUZZ_IN=${DIR_QA_ASSETS}/fuzz_seed_corpus/ | |
| if [ ! -d "$DIR_FUZZ_IN" ]; then | |
| CI_EXEC "${CI_RETRY_EXE}" git clone --depth=1 https://github.com/bitcoin-core/qa-assets "${DIR_QA_ASSETS}" | |
| fi | |
| ( |
🧰 Tools
🪛 Shellcheck (0.10.0)
[info] 109-109: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 Prompt for AI Agents
In ci/test/04_install.sh around lines 106 to 111, the variable ${CI_RETRY_EXE}
is used without quotes, which can cause word-splitting issues if it contains
spaces or multiple options. To fix this, enclose ${CI_RETRY_EXE} in double
quotes in the git clone command to ensure it is treated as a single argument and
prevent unintended splitting.
| test/fuzz/tx_in.cpp \ | ||
| test/fuzz/tx_out.cpp \ | ||
| test/fuzz/tx_pool.cpp \ | ||
| test/fuzz/txorphan.cpp \ |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify this new fuzz test is part of the original Bitcoin commit.
Adding test/fuzz/txorphan.cpp to the build system suggests a new fuzz test for orphan transaction handling. This appears unrelated to the stated PR objective of handling unrecognized command line arguments from bitcoin-core/gui#742.
🏁 Script executed:
#!/bin/bash
# Description: Check if txorphan.cpp fuzz test was added in the original Bitcoin commit
# Expected: Should not find this fuzz test if it's scope creep
echo "Checking for txorphan fuzz test in original Bitcoin commit..."
curl -s "https://api.github.com/repos/bitcoin/bitcoin/commits/afa081a39bde77d3959aa35b6e6c75a2fe988d68" | jq -r '.files[] | select(.filename | contains("txorphan")) | .filename' 2>/dev/null || echo "No txorphan files found in original commit"
echo "Checking if Makefile changes were in original commit..."
curl -s "https://api.github.com/repos/bitcoin/bitcoin/commits/afa081a39bde77d3959aa35b6e6c75a2fe988d68" | jq -r '.files[] | select(.filename | contains("Makefile")) | .filename' 2>/dev/null || echo "No Makefile changes found in original commit"Length of output: 609
Remove the unintended txorphan.cpp fuzz test entry
The test/fuzz/txorphan.cpp addition in src/Makefile.test.include (line 369) is not part of the original Bitcoin commit (afa081a…) and introduces scope creep unrelated to the GUI argument‐validation work. Please remove this line so that only fuzz tests present in the upstream commit remain.
- File to update:
- src/Makefile.test.include, remove
test/fuzz/txorphan.cpp \fromtest_fuzz_fuzz_SOURCES
- src/Makefile.test.include, remove
🤖 Prompt for AI Agents
In src/Makefile.test.include at line 369, remove the line containing
'test/fuzz/txorphan.cpp \' from the test_fuzz_fuzz_SOURCES list to eliminate the
unintended fuzz test addition and keep the file consistent with the original
upstream commit.
| #include <tinyformat.h> | ||
|
|
||
| #include <stdexcept> | ||
| #include <utility> |
There was a problem hiding this comment.
Scope mismatch: Utility header changes unrelated to command line validation
Adding #include <utility> and other header refactoring changes have no relation to "Exit and show error if unrecognized command line args are present". This appears to be infrastructure refactoring that should not be part of a focused command line argument validation backport.
🤖 Prompt for AI Agents
In src/util/check.h at line 11, the addition of #include <utility> is unrelated
to the command line argument validation changes. Remove this include and any
other unrelated header refactoring from this commit to keep the backport focused
solely on exiting and showing errors for unrecognized command line arguments.
| public: | ||
| NonFatalCheckError(const char* msg, const char* file, int line, const char* func); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Technical implementation correct but wrong scope
The NonFatalCheckError constructor refactoring appears technically sound, but error handling infrastructure changes are unrelated to command line argument validation. This represents scope creep beyond the stated Bitcoin Core backport.
Remove these utility header changes and focus only on the command line argument validation logic from the original Bitcoin commit.
🤖 Prompt for AI Agents
In src/util/check.h at lines 15-16, remove the NonFatalCheckError constructor
declaration and any related error handling infrastructure changes, as they are
unrelated to the command line argument validation logic. Focus the changes
solely on the argument validation code from the original Bitcoin Core backport
to avoid scope creep.
| /** | ||
| * Add the transaction to the wallet, wrapping it up inside a CWalletTx | ||
| * @return the recently added wtx pointer or nullptr if there was a db write error. | ||
| */ | ||
| CWalletTx* AddToWallet(CTransactionRef tx, const TxState& state, const UpdateWalletTxFn& update_wtx=nullptr, bool fFlushOnClose=true, bool rescanning_old_block = false); |
There was a problem hiding this comment.
Additional evidence of wrong backport
The addition of Doxygen documentation to AddToWallet and the removal of GenerateNewHDChain/UpgradeToHD methods (mentioned in AI summary but not visible as they were removed) further confirms this is a wallet refactoring backport, not a command line argument validation fix.
A command line argument fix would never require:
- Removing wallet HD chain generation methods
- Updating wallet transaction documentation
- Modifying core wallet interfaces
This solidifies that the wrong Bitcoin Core commit is being backported under an incorrect PR title.
🤖 Prompt for AI Agents
In src/wallet/wallet.h around lines 620 to 624, the issue is that the changes
made, including adding Doxygen comments to AddToWallet and removing HD chain
generation methods, indicate a wallet refactoring backport rather than a command
line argument validation fix. To fix this, revert the changes related to wallet
HD chain methods and core wallet interface modifications, and ensure only the
intended command line argument validation fixes are backported to keep the PR
focused and accurate.
| // build list of wallet TX hashes | ||
| std::vector<uint256> vTxHash; | ||
| std::list<CWalletTx> vWtx; | ||
| DBErrors err = FindWalletTx(vTxHash, vWtx); | ||
| DBErrors err = FindWalletTxHashes(vTxHash); |
There was a problem hiding this comment.
Scope creep: Wallet database refactoring unrelated to command line arguments
The changes to FindWalletTxHashes and ZapSelectTx represent significant wallet database refactoring that has no connection to command line argument validation. This violates the backport validation rule against scope creep.
🤖 Prompt for AI Agents
In src/wallet/walletdb.cpp around lines 1028 to 1030, the modifications to
FindWalletTxHashes and ZapSelectTx introduce wallet database refactoring
unrelated to command line argument validation. To fix this, revert these changes
and isolate wallet database refactoring into a separate commit or pull request,
ensuring that the current change only addresses command line argument validation
without scope creep.
| self.log.info("setban: fail to ban with past absolute timestamp") | ||
| assert_raises_rpc_error(-8, "Error: Absolute timestamp is in the past", self.nodes[1].setban, "127.27.0.1", "add", 123, True) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Comprehensive timestamp validation testing.
The test correctly validates that past absolute timestamps are rejected with an appropriate error message.
Consider using named arguments for better readability:
-assert_raises_rpc_error(-8, "Error: Absolute timestamp is in the past", self.nodes[1].setban, "127.27.0.1", "add", 123, True)
+assert_raises_rpc_error(-8, "Error: Absolute timestamp is in the past", self.nodes[1].setban, subnet="127.27.0.1", command="add", bantime=123, absolute=True)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.log.info("setban: fail to ban with past absolute timestamp") | |
| assert_raises_rpc_error(-8, "Error: Absolute timestamp is in the past", self.nodes[1].setban, "127.27.0.1", "add", 123, True) | |
| self.log.info("setban: fail to ban with past absolute timestamp") | |
| - assert_raises_rpc_error(-8, "Error: Absolute timestamp is in the past", self.nodes[1].setban, "127.27.0.1", "add", 123, True) | |
| + assert_raises_rpc_error(-8, "Error: Absolute timestamp is in the past", self.nodes[1].setban, subnet="127.27.0.1", command="add", bantime=123, absolute=True) |
🧰 Tools
🪛 Ruff (0.12.2)
61-61: Boolean positional value in function call
(FBT003)
🤖 Prompt for AI Agents
In test/functional/p2p_disconnect_ban.py at lines 60 to 61, the call to setban
uses positional arguments which reduces readability. Refactor the setban call to
use named arguments for the IP address, command, ban time, and absolute flag to
make the test clearer and easier to understand.
| self.log.info("setban: test banning with absolute timestamp") | ||
| self.nodes[1].setban("192.168.0.2", "add", self.mocktime + 120, True) | ||
|
|
||
| # Move time forward by 3 seconds so the third ban has expired | ||
| self.bump_mocktime(3) | ||
| self.wait_until(lambda: len(self.nodes[1].listbanned()) == 4, timeout=10) | ||
|
|
||
| self.log.info("Test ban_duration and time_remaining") | ||
| for ban in self.nodes[1].listbanned(): | ||
| if ban["address"] in ["127.0.0.0/32", "127.0.0.0/24"]: | ||
| assert_equal(ban["ban_duration"], 86400) | ||
| assert_equal(ban["time_remaining"], 86398) | ||
| assert_equal(ban["time_remaining"], 86397) | ||
| elif ban["address"] == "2001:4d48:ac57:400:cacf:e9ff:fe1d:9c63/19": | ||
| assert_equal(ban["ban_duration"], 1000) | ||
| assert_equal(ban["time_remaining"], 997) | ||
| elif ban["address"] == "192.168.0.2/32": | ||
| assert_equal(ban["ban_duration"], 120) | ||
| assert_equal(ban["time_remaining"], 117) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Thorough absolute timestamp banning test.
The test properly validates absolute timestamp banning functionality, including time advancement and ban duration calculations.
Consider using named arguments for the boolean parameter:
-self.nodes[1].setban("192.168.0.2", "add", self.mocktime + 120, True)
+self.nodes[1].setban(subnet="192.168.0.2", command="add", bantime=self.mocktime + 120, absolute=True)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.log.info("setban: test banning with absolute timestamp") | |
| self.nodes[1].setban("192.168.0.2", "add", self.mocktime + 120, True) | |
| # Move time forward by 3 seconds so the third ban has expired | |
| self.bump_mocktime(3) | |
| self.wait_until(lambda: len(self.nodes[1].listbanned()) == 4, timeout=10) | |
| self.log.info("Test ban_duration and time_remaining") | |
| for ban in self.nodes[1].listbanned(): | |
| if ban["address"] in ["127.0.0.0/32", "127.0.0.0/24"]: | |
| assert_equal(ban["ban_duration"], 86400) | |
| assert_equal(ban["time_remaining"], 86398) | |
| assert_equal(ban["time_remaining"], 86397) | |
| elif ban["address"] == "2001:4d48:ac57:400:cacf:e9ff:fe1d:9c63/19": | |
| assert_equal(ban["ban_duration"], 1000) | |
| assert_equal(ban["time_remaining"], 997) | |
| elif ban["address"] == "192.168.0.2/32": | |
| assert_equal(ban["ban_duration"], 120) | |
| assert_equal(ban["time_remaining"], 117) | |
| self.log.info("setban: test banning with absolute timestamp") | |
| - self.nodes[1].setban("192.168.0.2", "add", self.mocktime + 120, True) | |
| + self.nodes[1].setban(subnet="192.168.0.2", command="add", bantime=self.mocktime + 120, absolute=True) | |
| # Move time forward by 3 seconds so the third ban has expired | |
| self.bump_mocktime(3) | |
| self.wait_until(lambda: len(self.nodes[1].listbanned()) == 4, timeout=10) | |
| self.log.info("Test ban_duration and time_remaining") | |
| for ban in self.nodes[1].listbanned(): | |
| if ban["address"] in ["127.0.0.0/32", "127.0.0.0/24"]: | |
| assert_equal(ban["ban_duration"], 86400) | |
| assert_equal(ban["time_remaining"], 86397) | |
| elif ban["address"] == "2001:4d48:ac57:400:cacf:e9ff:fe1d:9c63/19": | |
| assert_equal(ban["ban_duration"], 1000) | |
| assert_equal(ban["time_remaining"], 997) | |
| elif ban["address"] == "192.168.0.2/32": | |
| assert_equal(ban["ban_duration"], 120) | |
| assert_equal(ban["time_remaining"], 117) |
🧰 Tools
🪛 Ruff (0.12.2)
82-82: Boolean positional value in function call
(FBT003)
🤖 Prompt for AI Agents
In test/functional/p2p_disconnect_ban.py around lines 81 to 98, the call to
setban uses a boolean positional argument for the absolute timestamp flag. To
improve code clarity, change the call to use a named argument for the boolean
parameter, for example, setban("192.168.0.2", "add", self.mocktime + 120,
absolute=True).
| "-debug", | ||
| "-debugexclude=libevent", | ||
| "-debugexclude=leveldb", | ||
| "-debugexclude=rand", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify this change is part of the original Bitcoin commit.
This debug exclusion addition appears unrelated to the stated PR objective of handling unrecognized command line arguments from bitcoin-core/gui#742. This looks like scope creep - adding test framework improvements beyond the original Bitcoin commit.
🏁 Script executed:
#!/bin/bash
# Description: Verify if adding "-debugexclude=rand" was part of the original Bitcoin commit afa081a39bde77d3959aa35b6e6c75a2fe988d68
# Expected: Should not find this change in the original Bitcoin commit if this is scope creep
echo "Searching for the original Bitcoin commit details..."
curl -s "https://api.github.com/repos/bitcoin/bitcoin/commits/afa081a39bde77d3959aa35b6e6c75a2fe988d68" | jq -r '.commit.message, .files[].filename' 2>/dev/null || echo "Unable to fetch Bitcoin commit details"
echo "Searching for debugexclude=rand in Bitcoin Core repository..."
curl -s "https://api.github.com/search/code?q=debugexclude%3Drand+repo:bitcoin/bitcoin" | jq -r '.items[].html_url' 2>/dev/null || echo "Unable to search Bitcoin repository"Length of output: 2014
Remove unrelated “-debugexclude=rand” test exclusion
The addition of "-debugexclude=rand" in the functional test framework isn’t part of the original Bitcoin commit (afa081a…) and is unrelated to the PR objective of exiting on unrecognized GUI command-line args. Please remove this line or move it to a separate PR focused on test-framework optimizations.
• test/functional/test_framework/test_node.py: line 109
🤖 Prompt for AI Agents
In test/functional/test_framework/test_node.py at line 109, remove the line
containing "-debugexclude=rand" as it is unrelated to the current PR's objective
and was not part of the original Bitcoin commit. This will keep the PR focused
and avoid mixing unrelated test exclusions.
❌ Backport Verification - CATASTROPHIC FAILUREOriginal Bitcoin commit: CRITICAL VIOLATIONS DETECTED: 🚨 Complete File Operation Mismatch
🚨 Scope Explosion Violation
🚨 Intent ViolationWhat Bitcoin Did: Added GUI command line argument validation to exit with error on unrecognized arguments
🚨 Reviewer Feedback AnalysisHuman comments reviewed: 14 critical violations identified
🚨 Auto-Reject Conditions Met
Required Action: COMPLETE REWRITEThis PR violates EVERY fundamental principle of faithful backporting:
Example of correct scope: // In src/qt/bitcoin.cpp - add argument validation
if (!unrecognized_args.empty()) {
QMessageBox::critical(nullptr, PACKAGE_NAME,
QObject::tr("Error: Unrecognized command line arguments"));
return EXIT_FAILURE;
}This PR represents a fundamental misunderstanding of Bitcoin commit Status: REJECTED - Requires complete rewrite 🚫 This PR has been automatically closed due to catastrophic validation failures. Please create a new PR with a proper backport implementation. |
|
Automatically closed due to catastrophic validation failures. Please see the detailed analysis above and create a new PR with a proper backport implementation. |
Backports bitcoin#742
Original commit: afa081a
Backported from Bitcoin Core v0.27
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores