backport: Bitcoin Core v0.26 backports (batch 2 rebuild)#7400
backport: Bitcoin Core v0.26 backports (batch 2 rebuild)#7400thepastaclaw wants to merge 5 commits into
Conversation
…equence` 272eb55 test: fix `include_immature_coinbase` logic in `get_utxos` (brunoerg) a951c34 test: fix `interface_usdt_mempool` by mining a block after each test (brunoerg) 1557bf1 test: fix mature utxos addition to wallet in `mempool_package_limits` (brunoerg) 60ced90 test: fix intermittent issue in `feature_bip68_sequence` (brunoerg) Pull request description: Fixes bitcoin#27129 To avoid `bad-txns-premature-spend-of-coinbase` error, when getting a utxo (using `get_utxo`) to create a new transaction `get_utxo` shouldn't return (if possible) by default immature coinbase. ACKs for top commit: achow101: ACK 272eb55 pinheadmz: re-ACK 272eb55 Tree-SHA512: eae821c7833bf084d8b907c94876ed010a7925d2177c3013a0c61b69d9571df006da83397a19487d93b0d1fa415951152f0b8ad0de2a55d86c39f6917934f050 (cherry picked from commit 0eb65f9)
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
|
✅ Review complete (commit 682e3f1) |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (27)
✅ Files skipped from review due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (20)
WalkthroughThis PR adds ignored-config startup handling with Estimated code review effort: 4 (Complex) | ~75 minutes Suggested reviewers: Possibly related PRs: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/functional/p2p_compactblocks.py (1)
715-764: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStale
getblocktxnmessage masks the second reconstruction check.
stalling_peeris reused for a secondannounce_cmpct_block()call (line 747) without clearing itslast_message["getblocktxn"]from the first block (or from earlier tests reusing the same connection, e.g.test_incorrect_blocktxn_response). Theassert "getblocktxn" in peer.last_messageinsideannounce_cmpct_block(line 728) will pass even if the node fails to send a fresh getblocktxn for the new block, silently weakening this check.🐛 Proposed fix
# Now test that delivering an invalid compact block won't break relay + stalling_peer.clear_getblocktxn() block, cmpct_block = announce_cmpct_block(node, stalling_peer)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/functional/p2p_compactblocks.py` around lines 715 - 764, The second use of stalling_peer in test_compactblock_reconstruction_stalling_peer can pass on a stale getblocktxn entry from a prior block, so the check in announce_cmpct_block no longer verifies that a new request was sent. Update announce_cmpct_block or the second call site to clear or snapshot peer.last_message before sending msg_cmpctblock, then assert the fresh getblocktxn appears for that specific compact block. Use the existing announce_cmpct_block helper and stalling_peer.last_message to keep the assertion tied to the current block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/functional/p2p_compactblocks.py`:
- Around line 793-863: The repeated compact block reconstruction test leaves
stale request state on stalling_peer, which can make later iterations pass
without proving a new getblocktxn was issued. Update
test_compactblock_reconstruction_parallel_reconstruction to clear
stalling_peer’s getblocktxn state at the end of each num_missing loop iteration
alongside delivery_peer, inbound_peer, and outbound_peer, so each pass through
announce_cmpct_block and the subsequent assertions verify fresh behavior.
---
Outside diff comments:
In `@test/functional/p2p_compactblocks.py`:
- Around line 715-764: The second use of stalling_peer in
test_compactblock_reconstruction_stalling_peer can pass on a stale getblocktxn
entry from a prior block, so the check in announce_cmpct_block no longer
verifies that a new request was sent. Update announce_cmpct_block or the second
call site to clear or snapshot peer.last_message before sending msg_cmpctblock,
then assert the fresh getblocktxn appears for that specific compact block. Use
the existing announce_cmpct_block helper and stalling_peer.last_message to keep
the assertion tied to the current block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e8a0be1e-3e91-4f16-8ccf-15400f575441
📒 Files selected for processing (28)
doc/release-notes-27302.mdsrc/Makefile.test_util.includesrc/bench/wallet_balance.cppsrc/bench/wallet_loading.cppsrc/init.cppsrc/init/common.cppsrc/net.hsrc/net_processing.cppsrc/net_processing.hsrc/qt/test/test_main.cppsrc/rpc/blockchain.cppsrc/rpc/client.cppsrc/rpc/rawtransaction.cppsrc/rpc/util.cppsrc/rpc/util.hsrc/test/fuzz/rpc.cppsrc/util/system.cppsrc/util/system.hsrc/wallet/test/load_util.cppsrc/wallet/test/util.hsrc/wallet/test/wallet_tests.cpptest/functional/feature_config_args.pytest/functional/mempool_package_limits.pytest/functional/p2p_compactblocks.pytest/functional/rpc_psbt.pytest/functional/test_framework/test_node.pytest/functional/test_framework/util.pytest/lint/run-lint-format-strings.py
There was a problem hiding this comment.
Code Review
Policy gate: an agent-reported missing upstream prerequisite was restored as a blocking finding. This PR is a full Bitcoin backport, and omitted upstream hunks require their prerequisite PRs unless the finding is explicitly allowlisted (intentional_exclusion / policy_override). The agent's original evidence is preserved in the finding(s) below.
Prior verifier summary (overridden by policy gate): One real blocking bug: the compact-block READ_STATUS_FAILED branch (net_processing.cpp:4934-4945) removes the in-flight request but falls through and can still send GETBLOCKTXN for a request it just removed. Upstream fixed this immediately in bitcoin#27743 (d972695) and it should be applied here as part of the bitcoin#27626 backport. Otherwise the backport is clean: five well-formed cherry-picks, Dash-specific adaptations verified, no missing prerequisites that materially matter.
Note: canonical verifier/policy action is REQUEST_CHANGES, but GitHub does not allow this account to request changes on its own PR, so the public review is posted as a COMMENT while preserving the blocking findings.
🔴 4 blocking | 💬 2 nitpick(s)
All verified findings
-
[BLOCKING]
src/net_processing.cpp:4934-4945: READ_STATUS_FAILED non-first-in-flight branch falls through instead of returning
WhenpartialBlock.InitData()returnsREAD_STATUS_FAILEDfor a non-first-in-flight parallel compact-block request, theelsebranch callsRemoveBlockRequest(pindex->GetBlockHash(), pfrom.GetId())but does notreturn. Execution falls through to theBlockTransactionsRequest req;block below, where the high-bandwidth branch at line 4963-4972 can still push aGETBLOCKTXNfor a block we just removed from our in-flight map. When the peer responds withBLOCKTXN, that message will be treated as unexpected (no matching in-flight entry) and dropped, wasting a slot in the parallel-download budget and hurting recovery time — exactly the scenario bitcoin#27626 was meant to improve. Upstream landed the fix in bitcoin#27743 (d972695797, "Unconditionally return when compact block status == READ_STATUS_FAILED") right after bitcoin#27626 and it should be pulled into this batch (or applied inline) bef... -
[NITPICK]
src/rpc/rawtransaction.cpp:2000-2018: descriptorprocesspsbt help references segwit, Taproot, and DEFAULT sighash that Dash does not support
The help text carries over upstream's Bitcoin-specific language: the description says "Update all segwit inputs..." andsighashtypeadvertises"DEFAULT"with a default of"DEFAULT for Taproot, ALL otherwise". Dash has no segwit and no Taproot, andParseSighashString(src/core_read.cpp) rejects"DEFAULT"— a user copyingsighashtype="DEFAULT"from this help will hit"DEFAULT is not a valid sighash parameter.". The RPC itself works correctly on legacy inputs because the null default resolves toSIGHASH_ALLin the C++ path, so this is docs-only, but the help should dropDEFAULTfrom the enumeration, reword the default to"ALL", and change the leading sentence to reflect Dash's legacy-inputs reality. -
[NITPICK]
test/functional/mempool_package_limits.py:1-3: bitcoin#27177 backport silently omits interface_usdt_mempool.py sub-fix
Upstream bitcoin#27177 bundles four sub-commits, one of which (a951c34f17) addsself.generate(self.wallet, 1)after each subtest intest/functional/interface_usdt_mempool.py. That file has never been backported to Dash (its parent bitcoin#26531 mempool-tracepoints stack is absent), so this Dash backport skips that hunk without a note. It is inert — there is nothing in Dash for the fix to apply to — but the PR advertises a full backport of bitcoin#27177, so the omission is worth recording explicitly (release notes, a comment innon-backported.txt, or the PR description) rather than silently dropping. -
[BLOCKING]
test/functional/mempool_package_limits.py:1-3: Squashed bitcoin#27177 omits interface_usdt_mempool.py sub-fix (file doesn't exist in Dash)
Upstream bitcoin#27177 (Merge bitcoin#27177) bundles four sub-commits: 272eb55 (get_utxos immature-coinbase fix), a951c34 (interface_usdt_mempool.py: mine a block after each test), 1557bf1 (mempool_package_limits.py mature-utxo fix), and 60ced90 (feature_bip68_sequence.py fix). This Dash backport (f454560) only touches test/functional/mempool_package_limits.py (3 lines) plus the shared get_utxos() helper in test_framework/wallet.py, and feature_bip68_sequence.py already correctly passes include_immature_coinbase=False. The a951c34 sub-fix for interface_usdt_mempool.py has no counterpart change in this PR because Dash has never backported that USDT mempool tracepoint test file — confirmed viagit merge-base --is-ancestor 9f55773a37 HEADreturning false (9f55773 being an earlier upstream commit that touches the same file). This is not a defect: the file genuinely do... -
[BLOCKING]
test/functional/interface_usdt_mempool.py:176-327: Missing prerequisite: bitcoin#26531
bitcoin#27177 includes commit a951c34, which addsself.generate(self.wallet, 1)after each mempool USDT tracepoint subtest intest/functional/interface_usdt_mempool.py. Dash's base does not have this file at all, so this part of the upstream PR is omitted. The missing file and tracepoint test stack come from bitcoin#26531 (Merge bitcoin/bitcoin#26531: mempool: Add mempool tracepoints). This is a soft prerequisite gap because the missing content is test-only and the absent USDT tracing subsystem is not used by the applied Dash hunk, but a claimed full backport of bitcoin#27177 is incomplete without the earlier tracepoint backport or an explicit exclusion. --- Policy gate (backport-prereq-restore): For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g.intentional_exclusion: trueor a matching entry in `polic... -
[BLOCKING]
src/util/system.cpp:1000-1013: Missing prerequisite: bitcoin#27419
bitcoin#27302 was merged upstream after bitcoin#27419 moved ArgsManager/config parsing fromsrc/util/system.*intosrc/common/args.*andsrc/common/config.cpp. Upstream bitcoin#27302 modifiesArgsManager::GetConfigFilePath()insrc/common/args.cpp, storesm_config_pathinsrc/common/args.h, and updates config reading insrc/common/config.cpp. Dash lacks those common args/config files, so this PR ports the same logic into the oldersrc/util/system.cpp/src/util/system.hlayout instead. That is a clean workaround, not a build blocker, but by origin the missing upstream content is bitcoin#27419 rather than a Dash-specific divergence. --- Policy gate (backport-prereq-restore): For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g.intentional_exclusion: trueor a matching entry inpolicy_overrides). The...
| } else if (status == READ_STATUS_FAILED) { | ||
| // Duplicate txindexes, the block is now in-flight, so just request it | ||
| std::vector<CInv> vInv(1); | ||
| vInv[0] = CInv(MSG_BLOCK, blockhash); | ||
| m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, vInv)); | ||
| return; | ||
| if (first_in_flight) { | ||
| // Duplicate txindexes, the block is now in-flight, so just request it | ||
| std::vector<CInv> vInv(1); | ||
| vInv[0] = CInv(MSG_BLOCK, blockhash); | ||
| m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, vInv)); | ||
| return; | ||
| } else { | ||
| // Give up for this peer and wait for other peer(s) | ||
| RemoveBlockRequest(pindex->GetBlockHash(), pfrom.GetId()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: READ_STATUS_FAILED non-first-in-flight branch falls through instead of returning
When partialBlock.InitData() returns READ_STATUS_FAILED for a non-first-in-flight parallel compact-block request, the else branch calls RemoveBlockRequest(pindex->GetBlockHash(), pfrom.GetId()) but does not return. Execution falls through to the BlockTransactionsRequest req; block below, where the high-bandwidth branch at line 4963-4972 can still push a GETBLOCKTXN for a block we just removed from our in-flight map. When the peer responds with BLOCKTXN, that message will be treated as unexpected (no matching in-flight entry) and dropped, wasting a slot in the parallel-download budget and hurting recovery time — exactly the scenario bitcoin#27626 was meant to improve. Upstream landed the fix in bitcoin#27743 (d972695797, "Unconditionally return when compact block status == READ_STATUS_FAILED") right after bitcoin#27626 and it should be pulled into this batch (or applied inline) before merge.
| } else if (status == READ_STATUS_FAILED) { | |
| // Duplicate txindexes, the block is now in-flight, so just request it | |
| std::vector<CInv> vInv(1); | |
| vInv[0] = CInv(MSG_BLOCK, blockhash); | |
| m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, vInv)); | |
| return; | |
| if (first_in_flight) { | |
| // Duplicate txindexes, the block is now in-flight, so just request it | |
| std::vector<CInv> vInv(1); | |
| vInv[0] = CInv(MSG_BLOCK, blockhash); | |
| m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, vInv)); | |
| return; | |
| } else { | |
| // Give up for this peer and wait for other peer(s) | |
| RemoveBlockRequest(pindex->GetBlockHash(), pfrom.GetId()); | |
| } | |
| } | |
| } else if (status == READ_STATUS_FAILED) { | |
| if (first_in_flight) { | |
| // Duplicate txindexes, the block is now in-flight, so just request it | |
| std::vector<CInv> vInv(1); | |
| vInv[0] = CInv(MSG_BLOCK, blockhash); | |
| m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETDATA, vInv)); | |
| } else { | |
| // Give up for this peer and wait for other peer(s) | |
| RemoveBlockRequest(pindex->GetBlockHash(), pfrom.GetId()); | |
| } | |
| return; | |
| } |
source: ['codex']
| "\nUpdate all segwit inputs in a PSBT with information from output descriptors, the UTXO set or the mempool. \n" | ||
| "Then, sign the inputs we are able to with information from the output descriptors. ", | ||
| { | ||
| {"psbt", RPCArg::Type::STR, RPCArg::Optional::NO, "The transaction base64 string"}, | ||
| {"descriptors", RPCArg::Type::ARR, RPCArg::Optional::NO, "An array of either strings or objects", { | ||
| {"", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "An output descriptor"}, | ||
| {"", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "An object with an output descriptor and extra information", { | ||
| {"desc", RPCArg::Type::STR, RPCArg::Optional::NO, "An output descriptor"}, | ||
| {"range", RPCArg::Type::RANGE, RPCArg::Default{1000}, "Up to what index HD chains should be explored (either end or [begin,end])"}, | ||
| }}, | ||
| }}, | ||
| {"sighashtype", RPCArg::Type::STR, RPCArg::Default{"DEFAULT for Taproot, ALL otherwise"}, "The signature hash type to sign with if not specified by the PSBT. Must be one of\n" | ||
| " \"DEFAULT\"\n" | ||
| " \"ALL\"\n" | ||
| " \"NONE\"\n" | ||
| " \"SINGLE\"\n" | ||
| " \"ALL|ANYONECANPAY\"\n" | ||
| " \"NONE|ANYONECANPAY\"\n" | ||
| " \"SINGLE|ANYONECANPAY\""}, |
There was a problem hiding this comment.
💬 Nitpick: descriptorprocesspsbt help references segwit, Taproot, and DEFAULT sighash that Dash does not support
The help text carries over upstream's Bitcoin-specific language: the description says "Update all segwit inputs..." and sighashtype advertises "DEFAULT" with a default of "DEFAULT for Taproot, ALL otherwise". Dash has no segwit and no Taproot, and ParseSighashString (src/core_read.cpp) rejects "DEFAULT" — a user copying sighashtype="DEFAULT" from this help will hit "DEFAULT is not a valid sighash parameter.". The RPC itself works correctly on legacy inputs because the null default resolves to SIGHASH_ALL in the C++ path, so this is docs-only, but the help should drop DEFAULT from the enumeration, reword the default to "ALL", and change the leading sentence to reflect Dash's legacy-inputs reality.
source: ['claude']
| @@ -1008,9 +1009,12 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys) | |||
| LOCK(cs_args); | |||
| m_settings.ro_config.clear(); | |||
| m_config_sections.clear(); | |||
| m_config_path = AbsPathForConfigVal(GetPathArg("-conf", BITCOIN_CONF_FILENAME), /*net_specific=*/false); | |||
| } | |||
There was a problem hiding this comment.
🔴 Blocking: Missing prerequisite: bitcoin#27419
bitcoin#27302 was merged upstream after bitcoin#27419 moved ArgsManager/config parsing from src/util/system.* into src/common/args.* and src/common/config.cpp. Upstream bitcoin#27302 modifies ArgsManager::GetConfigFilePath() in src/common/args.cpp, stores m_config_path in src/common/args.h, and updates config reading in src/common/config.cpp. Dash lacks those common args/config files, so this PR ports the same logic into the older src/util/system.cpp / src/util/system.h layout instead. That is a clean workaround, not a build blocker, but by origin the missing upstream content is bitcoin#27419 rather than a Dash-specific divergence.
Policy gate (backport-prereq-restore): For full upstream backport PRs, a missing prerequisite is blocking unless the finding is explicitly allowlisted (e.g. intentional_exclusion: true or a matching entry in policy_overrides). The agent's original evidence above is the basis for this block; either backport the prerequisite or annotate the intentional exclusion in the PR description.
source: ['codex-backport-reviewer']
|
Addressed CodeRabbit p2p_compactblocks.py request-state comments in be7930d. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest push be7930d adds the two missing stalling_peer.clear_getblocktxn() calls in test/functional/p2p_compactblocks.py; that latest delta looks correct and introduces no new code-defect blocker. However, every prior finding from 775b042 remains valid at the current head: the compact-block READ_STATUS_FAILED fallthrough is still present, bitcoin#27177 still omits the interface_usdt_mempool sub-fix because bitcoin#26531 is absent, bitcoin#27302 is still layered onto the pre-bitcoin#27419 args/config layout, and descriptorprocesspsbt help still carries Bitcoin-only segwit/Taproot/DEFAULT wording. New latest-delta finding: the sixth test-fix commit should be reflected in the PR body/commit provenance before merge.
🔴 3 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
Carried-forward prior findings
-
[BLOCKING]
src/net_processing.cpp:4934-4945: READ_STATUS_FAILED non-first-in-flight branch falls through instead of returning
Verified at HEAD (be7930d): whenpartialBlock.InitData()returnsREAD_STATUS_FAILEDfor a non-first-in-flight parallel compact-block request, theelsebranch at line 4941-4944 callsRemoveBlockRequest(pindex->GetBlockHash(), pfrom.GetId())but does notreturn. Execution falls through into theBlockTransactionsRequest req;construction at line 4947; because InitData failed the reconstructor cannot tell which txs are available soreq.indexesis typically non-empty, and withfirst_in_flightfalse control reaches the highbandwidth branch at 4963-4972 which will push aGETBLOCKTXNfor a hash just removed frommapBlocksInFlight. The peer's subsequentBLOCKTXNis then dropped as unexpected, wasting a slot in the parallel-download budget and defeating the recovery scenario bitcoin#27626 was intended to enable. Upstream fixed this in bitcoin#27743 (d972695797, "Unconditionally return when compact block status == READ_STATUS_FAILED"), which lives between the bitcoin#27626 and bitcoin#27666 commits already carried in this batch. Note: the latest push addsstalling_peer.clear_getblocktxn()calls in the functional test, which masks downstream test symptoms without applying the actual production fix. -
[BLOCKING]
test/functional/mempool_package_limits.py:1-3: Squashed bitcoin#27177 backport omits interface_usdt_mempool.py sub-fix (missing prerequisite bitcoin#26531)
Upstream bitcoin#27177 bundles four sub-commits, one of which (a951c34f17,test: fix interface_usdt_mempool by mining a block after each test) modifiestest/functional/interface_usdt_mempool.py. That file is confirmed absent from Dash at HEAD because the underlying USDT mempool-tracepoints stack (bitcoin#26531,mempool: Add mempool tracepoints) has never been backported. The Dash squashed commitf454560f83advertises a full backport of bitcoin#27177 and applies the other three sub-fixes, but silently drops the fourth with no PR-body annotation, release-note fragment, ortest/util/data/non-backported.txtentry. Per the backport-prereq policy, an omitted upstream hunk driven by an absent upstream prerequisite is a missing prerequisite (bitcoin#26531), not a Dash-native divergence. Either backport bitcoin#26531 (or the specific test file) or add an explicit intentional-exclusion note before merging. -
[BLOCKING]
src/util/system.cpp:1000-1013: bitcoin#27302 layered onto pre-bitcoin#27419 layout (missing prerequisite bitcoin#27419)
Upstream bitcoin#27302 was merged after bitcoin#27419 movedArgsManager/config parsing fromsrc/util/system.*intosrc/common/args.*andsrc/common/config.cpp, and bitcoin#27302's actual diff modifiesArgsManager::GetConfigFilePath()insrc/common/args.cpp, storesm_config_pathinsrc/common/args.h, and updates config reading insrc/common/config.cpp.src/common/args.cpp,src/common/args.h, andsrc/common/config.cppare confirmed absent from Dash at HEAD, so this PR ports the same logic into the pre-movesrc/util/system.{cpp,h}layout instead. The port is functionally equivalent and covered byfeature_config_args.py, but by upstream origin the missing content traces to bitcoin#27419, not to a Dash-intentional divergence. Per the backport-prereq policy for full-upstream backport PRs, either land bitcoin#27419 first or explicitly annotate the layout divergence in the PR body / non-backported tracking. -
[NITPICK]
src/rpc/rawtransaction.cpp:2000-2018: descriptorprocesspsbt help references segwit, Taproot, and DEFAULT sighash that Dash does not support
Help text carries over upstream's Bitcoin-specific wording verbatim: line 2000 opens with "Update all segwit inputs in a PSBT...", line 2011 defaultssighashtypeto"DEFAULT for Taproot, ALL otherwise", and line 2012 lists"DEFAULT"as a valid value. Dash has no segwit and no Taproot, andsrc/core_read.cpp's sighash parser does not handle the literal stringDEFAULT(verified via grep — noDEFAULThandling in ParseSighashString), so a user copyingsighashtype="DEFAULT"from the help will hit a parse error. The RPC itself works correctly on legacy inputs because the null default resolves toSIGHASH_ALLin C++, so this is docs-only. Suggest droppingDEFAULTfrom the enumeration, changing the default description to"ALL", and rewording the leading sentence to reflect Dash's legacy-inputs reality.
New finding in latest delta
- [SUGGESTION]
test/functional/p2p_compactblocks.py:747-863: Tip commit adapts an upstream-backported test in the same stack; message and PR body should reflect the sixth commit
Commitbe7930dc65 test: clear compact block request state between checksadds twostalling_peer.clear_getblocktxn()calls totest/functional/p2p_compactblocks.py, which was itself just added byd7f359b35e(part of the bitcoin#27626 backport, four commits earlier in the same stack). The commit has no upstream cherry-pick trailer, no body, and is not one of the five backport commits the PR description claims to replay (git rev-list --count develop..HEADreturns 6, not 5). The change itself is legitimate test hygiene, but as permanent Dash history it reads like an unattributed drive-by fix. Reword the commit message to note it is a Dash-authored adaptation of the bitcoin#27626 test (e.g. subjecttest: clear compact block request state between checks (adapt bitcoin#27626 backport)with a short body explaining the state-bleed being fixed), and update the PR description to acknowledge the sixth commit. Keeping it as a separate commit is fine — preserving the upstream backport byte-for-byte is a defensible trade-off.
Prior finding reconciliation
prior-1: STILL VALID — Verified src/net_processing.cpp:4934-4945 at be7930d — the else branch still lacks a return before the BlockTransactionsRequest construction at line 4947. Carried forward as findings[0].prior-2: STILL VALID — test/functional/interface_usdt_mempool.py confirmed absent at HEAD; bitcoin#27177 squashed backport still omits the a951c34 sub-fix. Consolidated with prior-3/prior-6 into findings[1].prior-3: STILL VALID — File absent, bitcoin#26531 (mempool tracepoints) not backported. Consolidated with prior-2/prior-6 into findings[1].prior-4: STILL VALID — src/common/args.cpp, src/common/args.h, and src/common/config.cpp confirmed absent at HEAD; bitcoin#27302 still layered onto pre-bitcoin#27419 ArgsManager layout. Carried forward as findings[2].prior-5: STILL VALID — Verified src/rpc/rawtransaction.cpp:2000-2018 unchanged at HEAD; core_read.cpp has no DEFAULT sighash handling. Carried forward as findings[3].prior-6: STILL VALID (consolidated) — Same underlying issue as prior-2/prior-3 at nitpick severity. Consolidated into the single blocking finding[1] to avoid duplication.
Note: GitHub does not allow me to submit REQUEST_CHANGES on my own PR, so this is posted as a COMMENT while preserving the verified blocking findings.
thepastaclaw
force-pushed
the
backport-0.26-batch-2-rebuild
branch
from
be7930d to
3191232
Compare
|
Force-pushed updated five-commit branch at Addressed the review items from the prior head:
Validation run locally: @coderabbitai review |
|
Triggering a fresh review of the updated branch at ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/wallet/test/load_util.cpp`:
- Around line 18-32: `TestLoadWallet` is using the result of `CWallet::Create`
without checking for failure, so it may register or initialize a null wallet.
Update this helper to mirror the production loader flow: verify the `wallet`
returned by `CWallet::Create` before calling `AddWallet`, `NotifyWalletLoaded`,
or `postInitProcess`, and return early if creation failed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 68d6719c-e2c7-4fda-9e49-6acb8c728c3d
📒 Files selected for processing (27)
doc/release-notes-27302.mdsrc/Makefile.test_util.includesrc/bench/wallet_balance.cppsrc/bench/wallet_loading.cppsrc/init.cppsrc/init/common.cppsrc/net.hsrc/net_processing.cppsrc/net_processing.hsrc/qt/test/test_main.cppsrc/rpc/blockchain.cppsrc/rpc/client.cppsrc/rpc/rawtransaction.cppsrc/rpc/util.cppsrc/rpc/util.hsrc/test/fuzz/rpc.cppsrc/util/system.cppsrc/util/system.hsrc/wallet/test/load_util.cppsrc/wallet/test/util.hsrc/wallet/test/wallet_tests.cpptest/functional/feature_config_args.pytest/functional/p2p_compactblocks.pytest/functional/rpc_psbt.pytest/functional/test_framework/test_node.pytest/functional/test_framework/util.pytest/lint/run-lint-format-strings.py
✅ Files skipped from review due to trivial changes (5)
- src/rpc/client.cpp
- src/net.h
- src/rpc/blockchain.cpp
- src/util/system.h
- doc/release-notes-27302.md
🚧 Files skipped from review as they are similar to previous changes (21)
- src/net_processing.h
- src/qt/test/test_main.cpp
- src/init/common.cpp
- src/test/fuzz/rpc.cpp
- src/wallet/test/util.h
- test/functional/rpc_psbt.py
- src/rpc/util.cpp
- src/Makefile.test_util.include
- test/functional/test_framework/test_node.py
- src/init.cpp
- test/lint/run-lint-format-strings.py
- src/bench/wallet_loading.cpp
- src/bench/wallet_balance.cpp
- src/rpc/util.h
- src/wallet/test/wallet_tests.cpp
- test/functional/test_framework/util.py
- test/functional/feature_config_args.py
- src/rpc/rawtransaction.cpp
- test/functional/p2p_compactblocks.py
- src/util/system.cpp
- src/net_processing.cpp
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta (be7930d..3191232) resolves the two prior blocking/nitpick findings on the code side: READ_STATUS_FAILED now returns unconditionally in both first-in-flight and give-up branches at src/net_processing.cpp:4934-4944, and the descriptorprocesspsbt help no longer references segwit/Taproot/DEFAULT sighash. The two prior missing-prerequisite blockers (bitcoin#26531 for interface_usdt_mempool.py; bitcoin#27419 for the args/config split) are now explicitly documented as intentional Dash-side exclusions/adaptations in the PR body and verified inert in the current tree. Two carried-over minor items remain: the descriptorprocesspsbt help still advertises 'the UTXO set' as a lookup source even though Dash's ProcessPSBT deliberately skips that fallback, and the folded bitcoin#27743 fix is not recorded in the c6c26cf commit message.
🟡 1 suggestion(s) | 💬 1 nitpick(s)
Findings not posted inline (2)
These findings could not be anchored to the current diff, but they are still part of this review.
- [NITPICK]
src/rpc/rawtransaction.cpp:2000-2001: descriptorprocesspsbt help still advertises UTXO set lookup that Dash does not perform — The Dash-adapted help text at line 2000 says descriptorprocesspsbt updates legacy inputs from output descriptors,the UTXO set or the mempool. But ProcessPSBT at src/rpc/rawtransaction.cpp:282-294 only fills legacynon_witness_utxodata fromg_txindexor the mempool, and the comment at line... - [SUGGESTION]
<commit:c6c26cf29e>:1: Folded bitcoin#27743 fix is not disclosed in the c6c26cf commit message — Commit c6c26cf ('Merge bitcoin#27626: Parallel compact block downloads, take 3') carries a single(cherry picked from commit 27033bcae2d52e6c4738106fc2276082891ae59d)trailer and lists only the six sub-commits of upstream bitcoin#27626, yet the diff also folds in bitcoin#2774...
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `<commit:c6c26cf29e>`:
- [SUGGESTION] <commit:c6c26cf29e>:1: Folded bitcoin#27743 fix is not disclosed in the c6c26cf29e commit message
Commit c6c26cf29e ('Merge bitcoin/bitcoin#27626: Parallel compact block downloads, take 3') carries a single `(cherry picked from commit 27033bcae2d52e6c4738106fc2276082891ae59d)` trailer and lists only the six sub-commits of upstream bitcoin#27626, yet the diff also folds in bitcoin/bitcoin#27743 (`d972695797`, the READ_STATUS_FAILED unconditional-return fix). That rationale lives only in the PR body, which does not persist into `git log` / `git blame` after merge. A future blame of the `return;` at src/net_processing.cpp:4944 will land here without provenance, and diffing this commit against `27033bcae2...` will show an unexplained extra hunk. Amend the commit body before merge to add a short note such as `Also includes bitcoin/bitcoin#27743 (d972695797): Unconditionally return when compact block status == READ_STATUS_FAILED.` so provenance is self-contained in permanent history.
Prior finding reconciliation
- READ_STATUS_FAILED non-first-in-flight branch falls through instead of returning — Fixed: delta moves
return;outside the if/else at src/net_processing.cpp:4941-4944, so both the first_in_flight GETDATA branch and the else RemoveBlockRequest branch fall through to a single unconditionalreturn;. Matches upstream bitcoin#27743. - Squashed bitcoin#27177 backport omits interface_usdt_mempool.py sub-fix (missing prerequisite bitcoin#26531) — Intentionally deferred and now documented in the PR body's Backport Review Notes. Verified no interface_usdt_mempool.py file exists in Dash's test tree, and bitcoin#26531 is not reachable from develop/HEAD. The applicable Dash hunks (get_utxos include_immature_coinbase, mempool_package_limits.py, feature_bip68_sequence.py-already-passing) are present. Marked as intentional_exclusion.
- bitcoin#27302 layered onto pre-bitcoin#27419 layout (missing prerequisite bitcoin#27419) — Intentionally deferred and now documented in the PR body. Verified src/common/args.* does not exist in Dash. bitcoin#27302 was correctly ported into the existing src/util/system.{cpp,h}: GetConfigFilePath() at system.cpp:1000, m_config_path, and -allowignoredconf detection at ~lines 1101-1127, with init/common.cpp:153 consuming the new accessor. Functionally equivalent to upstream. Marked as intentional_exclusion.
- Tip commit adapts an upstream-backported test in the same stack; message and PR body should reflect the sixth commit — Fixed: the prior tip test-adaptation commit is folded into the bitcoin#27626 backport commit c6c26cf as sub-commit d7f359b. Branch is now exactly 5 commits ahead of develop, matching the PR body. A narrower residual concern about the commit message not naming the folded-in bitcoin#27743 is captured as a new suggestion above.
- descriptorprocesspsbt help references segwit, Taproot, and DEFAULT sighash that Dash does not support — Fixed: delta rewrites line 2000 to 'Update all legacy inputs', drops the DEFAULT enum entry, and sets sighashtype Default to 'ALL' at line 2011 with only ALL/NONE/SINGLE (+ANYONECANPAY variants) listed. A separate but related help-text inaccuracy about UTXO-set lookup is captured as a new nitpick above.
Note: GitHub does not allow me to approve my own PR, so this is posted as a COMMENT review while preserving the verified nonblocking findings.
thepastaclaw
force-pushed
the
backport-0.26-batch-2-rebuild
branch
from
3191232 to
e4ea5c3
Compare
|
Force-pushed updated five-commit branch at Addressed CodeRabbit’s @coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/wallet/test/load_util.cpp (1)
18-32: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winNull check for
CWallet::Createstill missing.
walletfromCWallet::Createcan benullptron failure, but it's used unconditionally at lines 23/25/27/29 (AddWallet,NotifyWalletLoaded,wallet->postInitProcess()). This exact gap was flagged in a prior review round on this same line range. The PR discussion states a null guard forCWallet::Createwas folded into this commit, but the current file content still shows no such guard — please confirm the guard actually landed here.🐛 Proposed fix
bilingual_str error; std::vector<bilingual_str> warnings; auto wallet = CWallet::Create(context, "", std::move(database), create_flags, error, warnings); + if (!wallet) { + return nullptr; + } if (context.coinjoin_loader) { // TODO: see CreateWalletWithoutChain AddWallet(context, wallet); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/wallet/test/load_util.cpp` around lines 18 - 32, `TestLoadWallet` still uses the result of `CWallet::Create` without checking for null. Add a guard immediately after the `CWallet::Create` call in `TestLoadWallet` so failures are handled before `AddWallet`, `NotifyWalletLoaded`, or `wallet->postInitProcess()` are reached. Keep the existing control flow for the successful path, but ensure any early failure returns or otherwise avoids dereferencing a null `wallet`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/wallet/test/load_util.cpp`:
- Around line 18-32: `TestLoadWallet` still uses the result of `CWallet::Create`
without checking for null. Add a guard immediately after the `CWallet::Create`
call in `TestLoadWallet` so failures are handled before `AddWallet`,
`NotifyWalletLoaded`, or `wallet->postInitProcess()` are reached. Keep the
existing control flow for the successful path, but ensure any early failure
returns or otherwise avoids dereferencing a null `wallet`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fe5e0493-e8e4-42a7-94b9-fd614a481897
📒 Files selected for processing (6)
src/Makefile.test_util.includesrc/bench/wallet_balance.cppsrc/bench/wallet_loading.cppsrc/wallet/test/load_util.cppsrc/wallet/test/util.hsrc/wallet/test/wallet_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (5)
- src/bench/wallet_balance.cpp
- src/Makefile.test_util.include
- src/wallet/test/wallet_tests.cpp
- src/bench/wallet_loading.cpp
- src/wallet/test/util.h
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta 3191232..e4ea5c3 is tree-empty, so both prior nonblocking findings remain STILL VALID. A fresh current-head CodeRabbit review also correctly identified that the newly introduced shared TestLoadWallet helper still uses the CWallet::Create() result without a null guard before registration/notification/post-init, matching the current source at e4ea5c3.
🔴 1 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
All verified findings
-
[BLOCKING]
src/wallet/test/load_util.cpp:22: TestLoadWallet registers a null wallet when creation fails
The new TestLoadWallet helper stores the result of CWallet::Create() inwalletand then immediately passes it to AddWallet(), NotifyWalletLoaded(), and wallet->postInitProcess() without checking for failure. CWallet::Create() returns nullptr on wallet load/create errors, and the production loaders in src/wallet/load.cpp and src/wallet/wallet.cpp all guard that result before registration and post-init. This helper is introduced as shared test infrastructure in src/wallet/test/util.h, so any test or benchmark that hits a wallet creation failure now follows a null shared_ptr through the wallet notification/registration path and can crash instead of returning nullptr like the rest of the wallet-loading API. Return nullptr immediately after CWallet::Create() fails before AddWallet(), NotifyWalletLoaded(), or postInitProcess().Suggested replacement:
auto wallet = CWallet::Create(context, "", std::move(database), create_flags, error, warnings);
if (!wallet) {
return nullptr;
}
if (context.coinjoin_loader) {
// TODO: see CreateWalletWithoutChain
AddWallet(context, wallet);
}
NotifyWalletLoaded(context, wallet);
if (context.chain) {
wallet->postInitProcess();
}
-
[SUGGESTION]
<commit:c6c26cf29e>:1: Folded bitcoin#27743 fix is not disclosed in the c6c26cf commit message
Commit c6c26cf ('Merge bitcoin#27626: Parallel compact block downloads, take 3') carries a single(cherry picked from commit 27033bcae2d52e6c4738106fc2276082891ae59d)trailer and lists only the six sub-commits of upstream bitcoin#27626 (verified viagit log -1 --format=%B c6c26cf29eat head e4ea5c3). Its diff also folds in bitcoin#27743 (d972695, the unconditional-return fix when compact block status == READ_STATUS_FAILED, visible in the currentelse if (status == READ_STATUS_FAILED)branch of src/net_processing.cpp). Because Dash merges without squashing and the PR body does not persist intogit log/git blame, future readers bisecting or blaming the READ_STATUS_FAILED handling will land on a commit that appears to be a straight cherry-pick of bitcoin#27626 but actually contains an extra upstream hunk from bitcoin#27743, complicating backport audits. Amend the commit body before merge to add a short provenance note such asAlso includes bitcoin/bitcoin#27743 (d972695797): Unconditionally return when compact block status == READ_STATUS_FAILED. -
[NITPICK]
src/rpc/rawtransaction.cpp:2000: descriptorprocesspsbt help still advertises UTXO set lookup that Dash does not perform
The Dash-adapted help text at src/rpc/rawtransaction.cpp:2000 reads 'Update all legacy inputs in a PSBT with information from output descriptors, the UTXO set or the mempool.' Verified at current head e4ea5c3. However, ProcessPSBT in the same file only fills legacynon_witness_utxodata fromg_txindexor the mempool; the adjacent Dash-specific comment at lines 296-298 explicitly states the UTXO-set fallback is Bitcoin/segwit-specific and is not applicable in Dash. Users reading this help may expect confirmed legacy prevouts to be recovered from the UTXO set when txindex is disabled and the prevout transaction is no longer in the mempool, which the RPC does not do. Replace 'the UTXO set' with 'txindex' so the help matches the actual behavior.Suggested replacement:
"\nUpdate all legacy inputs in a PSBT with information from output descriptors, txindex or the mempool. \n"
"Then, sign the inputs we are able to with information from the output descriptors. "
Prior finding reconciliation
prior-1: STILL VALID — Folded bitcoin#27743 fix is not disclosed in the c6c26cf commit messageprior-2: STILL VALID — descriptorprocesspsbt help still advertises UTXO set lookup that Dash does not perform
Note: GitHub does not allow me to submit REQUEST_CHANGES on my own PR, so this is posted as a COMMENT while preserving the verified blocking finding.
thepastaclaw
force-pushed
the
backport-0.26-batch-2-rebuild
branch
from
e4ea5c3 to
818d9f2
Compare
|
Correction: the prior auto wallet = CWallet::Create(context, "", std::move(database), create_flags, error, warnings);
if (!wallet) return nullptr;Local validation: @coderabbitai review |
|
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The latest push at 818d9f2 fixes the prior blocking TestLoadWallet null-wallet issue by returning immediately after CWallet::Create() fails. Two non-blocking, in-scope findings remain: the permanent commit message for c6c26cf still omits the folded bitcoin#27743 provenance, and descriptorprocesspsbt help still advertises a UTXO-set lookup that Dash's ProcessPSBT does not perform. A codex-only missing-prerequisite claim for bitcoin#26531 was verified but dropped because the live PR body explicitly documents that omission as an intentional Dash adaptation.
🟡 1 suggestion(s) | 💬 1 nitpick(s)
All verified findings
-
[SUGGESTION]
<commit:c6c26cf29e>:1: Record the folded bitcoin#27743 fix in the commit message
Commit c6c26cf is presented as the bitcoin#27626 backport: its message lists only the six upstream bitcoin#27626 sub-commits and the cherry-pick trailer for 27033bc. The commit's diff also includes the bitcoin#27743 / d972695 READ_STATUS_FAILED unconditional-return fix, and the PR body documents that fold-in, but the PR body will not be available from git log or git blame after merge. Amend the c6c26cf commit body to record that bitcoin#27743 was also included, so future backport audits do not see an unexplained divergence from the cited bitcoin#27626 cherry-pick source. -
[NITPICK]
src/rpc/rawtransaction.cpp:2000-2001: descriptorprocesspsbt help advertises a UTXO-set lookup Dash skips
The descriptorprocesspsbt help says legacy inputs are updated from output descriptors, the UTXO set, or the mempool. In this Dash adaptation, ProcessPSBT only fills non_witness_utxo from txindex or the mempool, and the nearby comment explicitly says the upstream UTXO-set fallback is segwit-specific and not applicable to Dash. The help text should name txindex instead of the UTXO set so users do not expect confirmed legacy prevouts to be recovered without txindex or a mempool entry.Suggested replacement:
"\nUpdate all legacy inputs in a PSBT with information from output descriptors, txindex or the mempool. \n"
"Then, sign the inputs we are able to with information from the output descriptors. ",
Prior finding reconciliation
prior-1: FIXED — TestLoadWallet registers a null wallet when creation failsprior-2: STILL VALID — Folded bitcoin#27743 fix is not disclosed in the c6c26cf commit messageprior-3: STILL VALID — descriptorprocesspsbt help still advertises UTXO set lookup that Dash does not perform
Policy gate notes
Missing prerequisite: bitcoin#26531was not promoted because it is explicitly documented as an intentional Dash adaptation in the PR body.
d7f359b Add tests for parallel compact block downloads (Greg Sanders) 03423f8 Support up to 3 parallel compact block txn fetchings (Greg Sanders) 13f9b20 Only request full blocks from the peer we thought had the block in-flight (Greg Sanders) cce9618 Convert mapBlocksInFlight to a multimap (Greg Sanders) a905954 Remove nBlocksInFlight (Greg Sanders) 86cff8b alias BlockDownloadMap for mapBlocksInFlight (Greg Sanders) Pull request description: This is an attempt at mitigating bitcoin#25258 , which is a revival of bitcoin#10984, which is a revival of bitcoin#9447. This PR attempts to mitigate a single case, where high bandwidth peers can bail us out of a flakey peer not completing blocks for us. We allow up to 2 additional getblocktxns requests per unique block. This would hopefully allow the chance for an honest high bandwidth peer to hand us the transactions even if the first in flight peer stalls out. In contrast to previous effort: 1) it will not help if subsequent peers send block headers only, so only high-bandwidth peers this time. See: https://github.com/bitcoin/bitcoin/pull/10984/files#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R1411 2) `MAX_GETBLOCKTXN_TXN_AFTER_FIRST_IN_FLIGHT` is removed, in favor of aiding recovery during turbulent mempools 3) We require one of the 3 block fetching slots to be an outbound peer. This can be the original offering peer, or subsequent compact blocks given by high bandwidth peers. ACKs for top commit: sdaftuar: ACK d7f359b mzumsande: Code Review ACK d7f359b Tree-SHA512: 54980eac179e30f12a0bd49df147b2c3d63cd8f9401abb23c7baf02f76eeb59f2cfaaa155227990d0d39384de9fa38663f88774e891600a3837ae927f04f0db3 (cherry picked from commit 27033bc) Also includes bitcoin#27743 (d972695): Unconditionally return when compact block status == READ_STATUS_FAILED.
1bce12a test: add test for `descriptorprocesspsbt` RPC (ishaanam) fb2a3a7 rpc: add descriptorprocesspsbt rpc (ishaanam) Pull request description: This PR implements an RPC called `descriptorprocesspsbt`. This RPC is based off of `walletprocesspsbt`, but instead of interacting with the wallet to update, sign and finalize a psbt, it instead accepts an array of output descriptors and uses that information along with information from the mempool, txindex, and the utxo set to do so. `utxoupdatepsbt` also updates a psbt in this manner, but doesn't sign or finalize it. Because of this overlap, a helper function that is added in this PR is called by both `utxoupdatepsbt` and `descriptorprocesspsbt`. Whether or not the helper function signs a psbt is dictated by if the HidingSigningProvider passed to it contains any private information. There is also a test added in this PR for this new RPC that uses p2wsh, p2wpkh, and legacy outputs. Edit: see bitcoin#25796 (comment) ACKs for top commit: achow101: re-ACK 1bce12a instagibbs: reACK bitcoin@1bce12a Tree-SHA512: e1d0334739943e71f2ee68b4db7637ebe725da62e7aa4be071f71c7196d2a5970a31ece96d91e372d34454cde8509e95ab0eebd2c8edb94f7d5a781a84f8fc5d (cherry picked from commit ed99051)
… a bug (cherry picked from commit 9598089)
thepastaclaw
force-pushed
the
backport-0.26-batch-2-rebuild
branch
from
818d9f2 to
682e3f1
Compare
|
Force-pushed updated five-commit branch at Addressed the remaining self-review polish items:
Local validation: @coderabbitai review |
|
Triggering a fresh review of the updated branch. ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/rpc/rawtransaction.cpp (1)
257-321: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPropagate the caller-selected sighash into PSBT cleanup.
Line 311 signs with the caller-provided
sighash_type, but Line 319 still callsRemoveUnnecessaryTransactionsas if every signature usedSIGHASH_ALL. Fordescriptorprocesspsbtcallers usingNONE,SINGLE, orANYONECANPAY, cleanup can make decisions using the wrong signature mode.🐛 Proposed fix
- RemoveUnnecessaryTransactions(psbtx, /*sighash_type=*/1); + RemoveUnnecessaryTransactions(psbtx, sighash_type);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rpc/rawtransaction.cpp` around lines 257 - 321, The PSBT cleanup step in ProcessPSBT is still hardcoded to use SIGHASH_ALL, which can mismatch the caller’s chosen signature mode. Update the RemoveUnnecessaryTransactions call to use the sighash_type parameter that ProcessPSBT already receives, so descriptorprocesspsbt behaves correctly for NONE, SINGLE, and ANYONECANPAY. Refer to ProcessPSBT and RemoveUnnecessaryTransactions when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/rpc/rawtransaction.cpp`:
- Around line 257-321: The PSBT cleanup step in ProcessPSBT is still hardcoded
to use SIGHASH_ALL, which can mismatch the caller’s chosen signature mode.
Update the RemoveUnnecessaryTransactions call to use the sighash_type parameter
that ProcessPSBT already receives, so descriptorprocesspsbt behaves correctly
for NONE, SINGLE, and ANYONECANPAY. Refer to ProcessPSBT and
RemoveUnnecessaryTransactions when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6d184aa9-9a7b-4d65-854f-3da60d74d44c
📒 Files selected for processing (27)
doc/release-notes-27302.mdsrc/Makefile.test_util.includesrc/bench/wallet_balance.cppsrc/bench/wallet_loading.cppsrc/init.cppsrc/init/common.cppsrc/net.hsrc/net_processing.cppsrc/net_processing.hsrc/qt/test/test_main.cppsrc/rpc/blockchain.cppsrc/rpc/client.cppsrc/rpc/rawtransaction.cppsrc/rpc/util.cppsrc/rpc/util.hsrc/test/fuzz/rpc.cppsrc/util/system.cppsrc/util/system.hsrc/wallet/test/load_util.cppsrc/wallet/test/util.hsrc/wallet/test/wallet_tests.cpptest/functional/feature_config_args.pytest/functional/p2p_compactblocks.pytest/functional/rpc_psbt.pytest/functional/test_framework/test_node.pytest/functional/test_framework/util.pytest/lint/run-lint-format-strings.py
✅ Files skipped from review due to trivial changes (4)
- src/rpc/blockchain.cpp
- src/util/system.h
- doc/release-notes-27302.md
- src/net.h
🚧 Files skipped from review as they are similar to previous changes (21)
- src/Makefile.test_util.include
- src/test/fuzz/rpc.cpp
- src/init.cpp
- src/rpc/client.cpp
- src/net_processing.h
- test/functional/test_framework/util.py
- src/rpc/util.cpp
- src/init/common.cpp
- test/lint/run-lint-format-strings.py
- test/functional/rpc_psbt.py
- src/bench/wallet_balance.cpp
- src/wallet/test/wallet_tests.cpp
- test/functional/test_framework/test_node.py
- src/bench/wallet_loading.cpp
- src/wallet/test/util.h
- test/functional/p2p_compactblocks.py
- src/qt/test/test_main.cpp
- test/functional/feature_config_args.py
- src/rpc/util.h
- src/util/system.cpp
- src/net_processing.cpp
There was a problem hiding this comment.
Code Review
Current head 682e3f1 fixes both mandatory prior findings. The bitcoin#27743 fold-in is now recorded in the permanent c931a07 commit message, and descriptorprocesspsbt help now matches Dash's txindex/mempool behavior. CodeRabbit has no actionable findings to validate, and I found no in-scope issues to carry forward.
Canonical action: APPROVE. Findings: 0 findings.
Prior finding reconciliation
prior-818d9f28-1: FIXED — Verified with git log: c931a07 now documents the folded bitcoin#27743 fix in the commit body.prior-818d9f28-2: FIXED — Verified src/rpc/rawtransaction.cpp:2000 and ProcessPSBT: the help now names txindex/mempool, matching the implementation path that fills non_witness_utxo from txindex or mempool only.
Resolved prior findings
- Record the folded bitcoin#27743 fix in the commit message — Fixed in current head: commit c931a07 now includes the note
Also includes bitcoin/bitcoin#27743 (d972695797): Unconditionally return when compact block status == READ_STATUS_FAILED. - descriptorprocesspsbt help advertises a UTXO-set lookup Dash skips — Fixed in current head: src/rpc/rawtransaction.cpp:2000 now says descriptorprocesspsbt updates legacy inputs from output descriptors, txindex, or the mempool.
Note: GitHub does not allow me to submit APPROVE/REQUEST_CHANGES on my own PR, so this is posted as a COMMENT while preserving the canonical action.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
CI failure triage: the red Build source jobs are not caused by this PR's code changes. The failing jobs stop before compilation at the depends cache restore step, for example linux64-build in run 28571877624:
This is a fork-PR GitHub Actions cache permission/cache-miss issue, not a source build or test failure from the backport diff. I am leaving the branch untouched; maintainers can rerun/reseed CI with suitable permissions if they want a green source-build run. The remaining |
2 participants
backport: Bitcoin Core v0.26 backports (batch 2 rebuild)
Issue being fixed or feature implemented
This restores the automated Bitcoin Core v0.26 backport batch 2 after the
original PR, #7290, was closed during cleanup and the old staging
branch was found to contain fork PR merge commits.
The replacement branch replays the same five verified backport commits cleanly
on top of current
develop, preserving a linear reviewable history for theupstream Dash PR.
What was done?
Rebuilt batch 2 from the underlying backport commits without the GitHub fork PR
merge commits:
feature_bip68_sequencebitcoin/bitcoin#27177: test: fix intermittent issue infeature_bip68_sequencedescriptorprocesspsbtrpc bitcoin/bitcoin#25796: rpc: adddescriptorprocesspsbtrpctheir own file and fix a bug
This supersedes the closed PR #7290.
Backport Review Notes
d972695797, "Unconditionally return when compact block status == READ_STATUS_FAILED") into the Parallel compact block downloads, take 3 bitcoin/bitcoin#27626 backport commit because it fixes a fall-through introduced by the parallel compact-block download changes.feature_bip68_sequencebitcoin/bitcoin#27177 includes aninterface_usdt_mempool.pysub-fix from the mempool: Add mempool tracepoints bitcoin/bitcoin#26531 mempool tracepoint test stack. Dash does not have that tracepoint test file or its prerequisite stack, so there is no Dash hunk to apply; the relevant Dash hunks are theget_utxos()/mempool_package_limits.pychanges, whilefeature_bip68_sequence.pyalready passesinclude_immature_coinbase=False.src/util/system.*. Upstream applied the same logic after move-only: Extract common/args from util/system bitcoin/bitcoin#27419 split common args/config code intosrc/common/args.*andsrc/common/config.cpp; Dash has not made that split, so this PR intentionally keeps the change in the existing Dash files.descriptorprocesspsbtRPC help for Dash by removing Bitcoin-only segwit/Taproot/DEFAULTsighash wording and documenting Dash's acceptedALL/NONE/SINGLEsighash forms.How Has This Been Tested?
upstream/develop.upstream/developby exactly five commits.
git diff --check upstream/develop..HEAD.python3 -m py_compileon the changed functional/lint Python files.code-review dashpay/dash upstream/develop backport-0.26-batch-2-rebuildwith the rebuild recovery intent.
Result: no significant issues found; recommendation: ship.
Breaking Changes
None expected beyond the behavior of the upstream Bitcoin Core backports
themselves.
Checklist
(for repository code-owners and collaborators only)