backport: Bitcoin Core v0.26 backports (batch 2 rebuild)#7400
backport: Bitcoin Core v0.26 backports (batch 2 rebuild)#7400thepastaclaw wants to merge 16 commits into
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
|
✅ Review complete (commit 8045006) |
WalkthroughThis PR refactors wallet coin selection around cached output groups, shared-pointer UTXOs, explicit preset inputs, effective-value accounting, and detailed errors. It adds descriptor-based PSBT processing with configurable sighash and finalization, centralizes wallet test utilities, updates wallet benchmarks and GUI transaction handling, adds unknown-descriptor wallet errors, and expands regression coverage. Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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...
|
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.
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.
|
files share the same purpose, and we should not have wallet code inside the test directory. This later is needed to use wallet util functions in the bench and test binaries without being forced to duplicate them. (cherry picked from commit ee7a984)
…lid" set 13d9760 test: load wallet, coverage for crypted keys (furszy) 373c996 refactor: move DuplicateMockDatabase to wallet/test/util.h (furszy) ee7a984 refactor: unify test/util/wallet.h with wallet/test/util.h (furszy) cc5a5e8 wallet: bugfix, invalid crypted key "checksum_valid" set (furszy) Pull request description: At wallet load time, the crypted key "checksum_valid" variable is always set to false. Which, on every wallet decryption call, forces the process to re-write all the ckeys to db when it's not needed. Note: The first commit fixes the issue, the two commits in the middle are cleanups so `DuplicateMockDatabase` can be used without duplicating code. And, the last one is pure test coverage for the crypted keys loading process. Includes test coverage for the following scenarios: 1) "All ckeys checksums valid" test: Loads an encrypted wallet with all the crypted keys with a valid checksum and verifies that 'CWallet::Unlock' doesn't force an entire crypted keys re-write. (we force a complete ckeys re-write if we find any missing crypted key checksum during the wallet loading process) 2) "Missing checksum in one ckey" test: Verifies that loading up a wallet with, at least one, 'ckey' with no checksum triggers a complete re-write of the crypted keys. 3) "Invalid ckey checksum error" test: Verifies that loading up a ckey with an invalid checksum stops the wallet loading process with a corruption error. 4) "Invalid ckey pubkey error" test: Verifies that loading up a ckey with an invalid pubkey stops the wallet loading process with a corruption error. ACKs for top commit: achow101: ACK 13d9760 aureleoules: ACK 13d9760 Tree-SHA512: 9ea630ee4a355282fbeee61ca04737294382577bb4b2631f50e732568fdab8f72491930807fbda58206446c4f26200cdc34d8afa14dbe1241aec713887d06a0b Dash notes: - ee7a984 ("refactor: unify test/util/wallet.h with wallet/test/util.h", listed above) was already backported separately as commit 77e4504f67b, before this branch's bitcoin#26021 insertion; this commit only carries the remaining three upstream commits from bitcoin#26532 (the checksum_valid fix, the DuplicateMockDatabase move, and the crypted-key test coverage). - cc5a5e8 (the "checksum_valid" fix itself) was already present in this tree's src/wallet/walletdb.cpp before this commit, inherited from an earlier partial backport of bitcoin#26532 predating this branch's base; it is listed above for provenance but contributes no diff here. - wallet_load_verif_crypted_key_checksum adapts the upstream test to Dash's legacy-wallet key-generation API: LegacyScriptPubKeyMan::SetupGeneration() and GetNewDestination(OutputType) have not been backported to Dash (src/wallet/scriptpubkeyman.cpp still has a "TODO: unify with bitcoin after backporting SetupGeneration" note), so the test adds its test key directly via LegacyScriptPubKeyMan::AddKeyPubKey() instead of through SetupGeneration. This preserves the test's intent (an encrypted legacy wallet with one known crypted key) without requiring that unrelated, unbackported prerequisite. (cherry picked from commit 5690848)
… amount 7362f8e refactor: make CoinsResult total amounts members private (furszy) 3282fad wallet: add assert to SelectionResult::Merge for safety (S3RK) c4e3b7d wallet: SelectCoins, return early if wallet's UTXOs cannot cover the target (furszy) cac2725 test: bugfix, coinselector_test, use 'CoinsResult::Erase/Add' instead of direct member access (furszy) cf79384 test: Coin Selection, duplicated preset inputs selection (furszy) 341ba7f test: wallet, coverage for CoinsResult::Erase function (furszy) f930aef wallet: bugfix, 'CoinsResult::Erase' is erasing only one output of the set (furszy) Pull request description: This comes with bitcoin#26559. Solving few bugs inside the wallet's transaction creation process and adding test coverage for them. Plus, making use of the `CoinsResult::total_amount` cached value inside the Coin Selection process to return early if we don't have enough funds to cover the target amount. ### Bugs 1) The `CoinsResult::Erase` method removes only one output from the available coins vector (there is a [loop break](https://github.com/bitcoin/bitcoin/blob/c1061be14a515b0ed4f4d646fcd0378c62e6ded3/src/wallet/spend.cpp#L112) that should have never been there) and not all the preset inputs. Which on master is not a problem, because since [bitcoin#25685](bitcoin#25685) we are no longer using the method. But, it's a bug on v24 (check [bitcoin#26559](bitcoin#26559)). This method it's being fixed and not removed because I'm later using it to solve another bug inside this PR. 2) As we update the total cached amount of the `CoinsResult` object inside `AvailableCoins` and we don't use such function inside the coin selection tests (we manually load up the `CoinsResult` object), there is a discrepancy between the outputs that we add/erase and the total amount cached value. ### Improvements * This makes use of the `CoinsResult` total amount field to early return with an "Insufficient funds" error inside Coin Selection if the tx target amount is greater than the sum of all the wallet available coins plus the preset inputs amounts (we don't need to perform the entire coin selection process if we already know that there aren't enough funds inside our wallet). ### Test Coverage 1) Adds test coverage for the duplicated preset input selection bug that we have in v24. Where the wallet invalidly selects the preset inputs twice during the Coin Selection process. Which ends up with a "good" Coin Selection result that does not cover the total tx target amount. Which, alone, crashes the wallet due an insane fee. But.. to make it worst, adding the subtract fee from output functionality to this mix ends up with the wallet by-passing the "insane" fee assertion, decreasing the output amount to fulfill the insane fee, and.. sadly, broadcasting the tx to the network. 2) Adds test coverage for the `CoinsResult::Erase` method. ------------------------------------ TO DO: * [ ] Update [bitcoin#26559 ](bitcoin#26559) description. ACKs for top commit: achow101: ACK 7362f8e glozow: ACK 7362f8e, I assume there will be a followup PR to add coin selection sanity checks and we can discuss the best way to do that there. josibake: ACK [7362f8e](bitcoin@7362f8e) Tree-SHA512: 37a6828ea10d8d36c8d5873ceede7c8bef72ae4c34bef21721fa9dad83ad6dba93711c3170a26ab6e05bdbc267bb17433da08ccb83b82956d05fb16090328cba Dash notes: This is a prerequisite for the bitcoin#26661 backport (Coin Selection accurate error messages): bitcoin#26661 upstream builds on top of bitcoin#26560's CoinsResult total-amount caching and expects GetTotalAmount()/GetEffectiveTotalAmount() to exist. It was flagged as a soft gap by an independent backport-reviewer gate after bitcoin#26661 had already been backported without it, and is added here, in its correct topological position (after bitcoin#26532, before bitcoin#26661), to close that gap. Dash's CoinsResult never adopted upstream's OutputType-keyed map (it was already simplified to plain `legacy`/`other` vectors well before this point in the backport chain, since Dash only supports OutputType::LEGACY), so the following upstream sub-commits do not apply and are intentionally omitted: - 7362f8e (private total_amount/total_effective_amount + CoinsResult::Add()): adapted, not ported verbatim. total_amount/total_effective_amount stay public fields (matching Dash's existing simpler CoinsResult style) with GetTotalAmount()/GetEffectiveTotalAmount() accessors added alongside, and COutput::HasEffectiveValue() added to coinselection.h. AvailableCoins() accumulates total_effective_amount the same way it already accumulated total_amount. Dash's test-only `add_coin` helper (coinselector_tests.cpp) is updated to keep both cached totals in sync, mirroring what CoinsResult::Add does upstream. - f930aef / 341ba7f / cac2725 (CoinsResult::Erase bugfix + coverage): not applicable. Dash never uses CoinsResult::Erase (removed along with the rest of the OutputType map when Dash's bitcoin#25685 backport landed); the method does not exist in Dash's CoinsResult. - 3282fad (SelectionResult::Merge duplicate-input assert): not ported here. Dash's bitcoin#26661 backport already replaces Merge's raw util::insert with an InsertInputs helper that throws STR_INTERNAL_BUG on any duplicate, which is strictly stronger than this commit's assert. Porting the assert here would just be immediately superseded by the very next commit in the stack. Ported as Dash adaptations: - c4e3b7d (SelectCoins early return when wallet UTXOs can't cover the target): ported directly, using the same GetTotalAmount()/ GetEffectiveTotalAmount() accessors described above. - cf79384 (duplicated preset inputs selection test coverage): ported as a C++ unit test (wallet_duplicated_preset_inputs_test in spend_tests.cpp) using Dash's CreateSyncedWallet test helper and single-output-type GetNewDestination(label) signature, with amounts scaled 10x to match Dash's regtest block subsidy (500 DASH/block vs Bitcoin's 50 BTC/block). The upstream functional-test duplicate of this same coverage (rpc_fundrawtransaction.py's test_preset_inputs_selection) is not separately ported, since it exercises the identical SelectCoins code path already covered by the unit test. (cherry picked from commit f0c4807, partial: omits CoinsResult::Erase/Add and the SelectionResult::Merge assert as described above)
…ssages 76dc547 gui: create tx, launch error dialog if backend throws runtime_error (furszy) f4d7947 wallet: coin selection, add duplicated inputs checks (furszy) 0aa065b wallet: return accurate error messages from Coin Selection (furszy) 7e8340a wallet: make SelectCoins flow return util::Result (furszy) e5e147f wallet: refactor eight consecutive 'AttemptSelection' calls into a loop (furszy) Pull request description: Work decoupled from bitcoin#25806, which cleanup and improves the Coin Selection flow further. Adding the capability to propagate specific error messages from the Coin Selection process to the user. Instead of always returning the general "Insufficient funds" message which is not always accurate to what happened internally. Letting us instruct the user how to proceed under certain circumstances. The following error messages were added: 1) If the selection result exceeds the maximum transaction weight, we now will return: -> "The inputs size exceeds the maximum weight. Please try sending a smaller amount or manually consolidating your wallet's UTXOs". 2) If the user pre-selected inputs and disallowed the automatic coin selection process (no other inputs are allowed), we now will return: -> "The preselected coins total amount does not cover the transaction target. Please allow other inputs to be automatically selected or include more coins manually". 3) The double-counted preset inputs during Coin Selection error will now throw an "internal bug detected" message instead of crashing the node. The essence of this work comes from several comments: 1. bitcoin#26560 (comment) 2. bitcoin#25729 (comment) 3. bitcoin#25269 (review) 4. bitcoin#23144 (which is connected to bitcoin#24845) ACKs for top commit: ishaanam: crACK 76dc547 achow101: ACK 76dc547 aureleoules: ACK 76dc547 theStack: ACK 76dc547 🌇 Tree-SHA512: 9de30792d7a5849cae77747aa978e70390b66ee9d082779a56088a024f82e725b0af050e6603aece0ac8229f6d73bc471ba97b4ab69dc7eddf419f5f56ae89a5 Dash notes: - This is a gate-flagged prerequisite for the bitcoin#25806 backport in this branch: bitcoin#25806's own starting commit (e5e147f) is the first commit of this PR. Without it, AutomaticCoinSelection() still used eight hardcoded sequential AttemptSelection() calls instead of a data-driven loop over ordered_filters, which the bitcoin#25806 rework builds on. - bitcoin#26560 (CoinsResult::GetTotalAmount()/GetEffectiveTotalAmount() and the SelectCoins early-exit that uses them) is now backported as its own commit immediately before this one, in its correct upstream topological position (after bitcoin#26532, before this commit). With it in place, both content gaps that an earlier version of this commit had documented as intentional omissions are now closed instead: 1) The "max transaction weight exceeded" specific error message. Dash's coin-selection algorithms (SelectCoinsBnB/SelectCoinsSRD/KnapsackSolver, already backported via bitcoin#26720 earlier in Dash's history) already return a specific ErrorMaxWeightExceeded() util::Result when a candidate selection would exceed the max input weight; ChooseSelectionResult previously consumed those results as plain booleans (`if (auto x{...})`), discarding that specific error and falling through to a bare util::Error() whenever no algorithm produced a usable result. ChooseSelectionResult (both the OutputType-keyed overload used here and, in the later bitcoin#25806 commit, the Groups-based overload) now collects every algorithm's util::Result, and if none of them succeed, returns the first one that actually carried an error message instead of a generic empty one. AttemptSelection's mixed-output-types fallback had the identical discard-on-failure bug (`if (auto result{ChooseSelectionResult(...)}) { return result; }`, silently dropping ChooseSelectionResult's error on the else path) and is fixed the same way: it now returns ChooseSelectionResult's result directly, success or error, instead of gating on it as a boolean. Coverage: coinselector_tests.cpp's check_max_weight test now asserts the exact message text end-to-end through SelectCoins() (previously it only asserted `!result`), and the upstream functional test this message is for, test_transaction_too_large, is restored in wallet_fundrawtransaction.py (amounts scaled 10x for Dash's regtest block subsidy) after having been dropped by an earlier, now-superseded version of this backport that predated this fix. 2) SelectCoins()'s early-exit using CoinsResult::GetTotalAmount()/ GetEffectiveTotalAmount() is included as originally written upstream, now that bitcoin#26560 provides those accessors. - Dash-specific CoinType/CoinJoin behavior is preserved throughout: nCoinType is threaded through AttemptSelection/ChooseSelectionResult exactly as before, ONLY_NONDENOMINATED/ONLY_FULLY_MIXED get their existing dedicated error messages ahead of upstream's new generic-vs-specific SelectCoins error propagation, and ChooseSelectionResult's pre-existing "BnB/SRD are disabled because they're unaware of mixed CoinJoin coins" behavior (clearing positive_groups before calling SelectCoinsBnB/SelectCoinsSRD) is unchanged. (cherry picked from commit 3f8591d)
…m Coin Selection 6a302d4 wallet: single output groups filtering and grouping process (furszy) bd91ed1 wallet: unify outputs grouping process (furszy) 5596200 test: coinselector_tests refactor, use CoinsResult instead of plain std::vector (furszy) 34f54a0 wallet: decouple outputs grouping process from each ChooseSelectionResult (furszy) 461f082 refactor: make OutputGroup::m_outputs field a vector of shared_ptr (furszy) d8e749b test: wallet, add coverage for outputs grouping process (furszy) 06ec8f9 wallet: make OutputGroup "positive_only" filter explicit (furszy) Pull request description: The idea originates from bitcoin#24845 (comment). Note: For clarity, it's recommended to start reviewing from the end result to understand the structure of the flow. #### GroupOutputs function rationale: If "Avoid Partial Spends" is enabled, the function gathers outputs with the same script together inside a container. So Coin Selection can treats them as if them were just one possible input and either select them all or not select them. #### How the Inputs Fetch + Selection process roughly works: ``` 1. Fetch user’s manually selected inputs. 2. Fetch wallet available coins (walks through the entire wallet txes map) and insert them into a set of vectors (each vector store outputs from a single type). 3. Coin Selection Process: Call `AttemptSelection` 8 times. Each of them expands the coin eligibility filter (accepting a larger subset of coins in the calculation) until it founds a solutions or completely fails if no solutions gets founds after the 8 rounds. Each `AttemptSelection` call performs the following actions: - For each output type supported by the wallet (P2SH, P2PK, P2WPKH, P2WSH and a combination of all of them): Call ‘ChooseSelectionResult’ providing the respective, filtered by type, coins vector. Which: I. Groups the outputs vector twice (one for positive only and a second one who includes the negative ones as well). - GroupOutputs walks-through the entire inputted coins vector one time at least, + more if we are avoiding partial spends, to generate a vector of OutputGroups. II. Then performs every coin selection algorithm using the recently created vector of OutputGroup: (1) BnB, (2) knapsack and (3) SRD. III. Then returns the best solution out of them. ``` We perform the general operation of gathering outputs, with the same script, into a single container inside: Each coins selection attempt (8 times —> each coin eligibility filter), for each of the outputs vector who were filtered by type (plus another one joining all the outputs as well if needed), twice (one for the positive only outputs effective value and a second one for all of them). So, in the worst case scenario where no solution is found after the 8 Coin Selection attempts, the `GroupOutputs` function is called 80 times (8 * 5 * 2). #### Improvements: This proposal streamlines the process so that the output groups, filtered by coin eligibility and type, are created in a single loop outside of the Coin Selection Process. The new process is as follows: ``` 1. Fetch user’s manually selected inputs. 2. Fetch wallet available coins. 3. Group outputs by each coin eligibility filter and each different output type found. 4. Coin Selection Process: Call AttemptSelection 8 times. Each of them expands the coin eligibility filter (accepting different output groups) until it founds a solutions or completely fails if no solutions gets founds after the 8 rounds. Each ‘AttemptSelection’ call performs the following actions: - For each output type supported by the wallet (P2SH, P2PK, P2WPKH, P2WSH and all of them): A. Call ‘ChooseSelectionResult’ providing the respective, filtered by type, output group. Which: I. Performs every coin selection algorithm using the provided vector of OutputGroup: (1) BnB, (2) knapsack and (3) SRD. II. Then returns the best solution out of them. ``` Extra Note: The next steps after this PR will be to: 1) Merge `AvailableCoins` and `GroupOutputs` processes. 2) Skip entire coin selection rounds if no new coins are added into the subsequent round. 3) Remove global feerates from the OutputGroup class. 4) Remove secondary "grouped" tx creation from `CreateTransactionInternal` by running Coin Selection results over the aps grouped outputs vs non-aps ones. ACKs for top commit: S3RK: ReACK 6a302d4 achow101: ACK 6a302d4 theStack: re-ACK 6a302d4 🥥 Tree-SHA512: dff849063be328e7d9c358ec80239a6db2cd6131963b511b83699b95b337d3106263507eaba0119eaac63e6ac21c6c42d187ae23d79d9220b90c323d44b01d24 Dash notes: This commit applies all seven upstream subcommits, including the final one, 6a302d4 ("wallet: single output groups filtering and grouping process"), which an earlier version of this backport had omitted. That commit's purpose upstream is to batch-compute GroupOutputs() results across every CoinEligibilityFilter AutomaticCoinSelection walks, in one pass, instead of recomputing them once per filter inside AttemptSelection -- introducing FilteredOutputGroups/SelectionFilter on top of upstream's OutputType-keyed OutputGroupTypeMap. That upstream OutputType map itself is still not ported (see below), but the CORE optimization -- computing every filter's groups once, ahead of time, so AttemptSelection performs zero GroupOutputs() calls -- is fully adapted here: - GroupOutputs() gains a second overload taking a `const std::vector<SelectionFilter>&` and returning `FilteredOutputGroups` (a `std::map<CoinEligibilityFilter, Groups>`, requiring the new CoinEligibilityFilter::operator< in coinselection.h). It walks every output exactly once, checking eligibility against each filter in an inner loop and inserting into that filter's Groups entry, instead of looping over outputs once per filter as before. The original single-filter GroupOutputs() overload is preserved as a thin wrapper around this one, so existing callers (tests, bench) are unaffected. - AutomaticCoinSelection() now computes TWO such caches once, ahead of its ordered_filters walk: `filtered_groups` over available_coins.legacy (the privacy-preserving, single-OutputType-equivalent pass) and `filtered_mixed_groups` over available_coins.all() (legacy + other, used by the mixed-output-types fallback). AttemptSelection() no longer takes an eligibility_filter or CoinsResult at all; it takes both pre-computed Groups& references directly and calls GroupOutputs() zero times. A filter with no eligible coins in one (or both) passes has no entry in the corresponding cache; a shared empty Groups sentinel is substituted for that pass rather than skipping AttemptSelection entirely, so a filter that only has eligible coins in available_coins.other (nothing in .legacy) can still be resolved via the mixed-groups cache. An earlier iteration of this adaptation instead skipped AttemptSelection outright whenever the legacy-only cache had no entry for a filter, which broke any wallet whose only spendable coin is a non-legacy-typed output living in CoinsResult::other (e.g. a P2PK coinbase output, the common case exercised by availablecoins_tests.cpp's BasicOutputTypesTest) -- that regression is fixed here, before this commit existed as a separate patch. - src/wallet/test/group_outputs_tests.cpp gains filtered_output_groups_match_per_filter_calls, proving the filter-list GroupOutputs() overload produces, for every filter in a representative ordered_filters-shaped list and both avoid_partial_spends settings, the exact same positive/mixed groups the single-filter overload would compute for that filter alone -- i.e. that the batched cache is a faithful, reusable stand-in for N individual calls. - src/bench/coin_selection.cpp's CoinSelection benchmark is updated to precompute both Groups once outside the bench.run() loop and pass them into AttemptSelection(), matching production's call shape and no longer timing repeated grouping work the real code path no longer does either. Upstream's OutputGroupTypeMap (introduced by bd91ed1, used to fan groups out by OutputType) is still not ported: Dash's CoinsResult only ever has one meaningful bucket (CoinsResult::legacy, with CoinsResult::all() as the mixed-type fallback used by m_allow_other_inputs-style callers), so GroupOutputs() here returns a plain Groups{positive_group, mixed_group} directly rather than an OutputType-keyed structure, and FilteredOutputGroups is `map<CoinEligibilityFilter, Groups>` rather than upstream's `map<CoinEligibilityFilter, OutputGroupTypeMap>`. AttemptSelection()/ ChooseSelectionResult() keep Dash's existing CoinType/CoinJoin-aware signatures (isISLocked eligibility override, ONLY_FULLY_MIXED-aware KnapsackSolver dispatch, and the pre-existing "BnB/SRD are disabled because they're unaware of mixed CoinJoin coins" behavior) unchanged in spirit; those hooks are threaded through the new Groups-based call shape rather than being dropped. src/wallet/test/group_outputs_tests.cpp's original coverage (from d8e749b) is adapted the same way: no OutputType parameter, and coins are always inserted into CoinsResult::legacy since LegacyScriptPubKeyMan::GetNewDestination() only ever returns OutputType::LEGACY destinations here. src/wallet/test/walletload_tests.cpp's crypted-key coverage introduced by bitcoin#26532 (backported separately, see the "Merge bitcoin#26532" commit in this branch) predates this commit and is unaffected by it. (cherry picked from commit 4ea3a8b)
…elected coins 68eed5d test,gui: add coverage for PSBT creation on legacy watch-only wallets (furszy) 306aab5 test,gui: decouple widgets and model into a MiniGui struct (furszy) 2f76ac0 test,gui: decouple chain and wallet initialization from test case (furszy) cd98b71 gui: 'getAvailableBalance', include watch only balance (furszy) 74eac3a test: add coverage for 'useAvailableBalance' functionality (furszy) dc1cc1c gui: bugfix, getAvailableBalance skips selected coins (furszy) Pull request description: Fixes bitcoin-core/gui#688 and bitcoin#26687. First Issue Description (bitcoin-core/gui#688): The previous behavior for `getAvailableBalance`, when the coin control had selected coins, was to return the sum of them. Instead, we are currently returning the wallet's available total balance minus the selected coins total amount. Reason: Missed to update the `GetAvailableBalance` function to include the coin control selected coins on bitcoin#25685. Context: Since bitcoin#25685 we skip the selected coins inside `AvailableCoins`, the reason is that there is no need to waste resources walking through the entire wallet's txes map just to get coins that could have gotten by just doing a simple `mapWallet.find`). Places Where This Generates Issues (only when the user manually select coins via coin control): 1) The GUI balance check prior the transaction creation process. 2) The GUI "useAvailableBalance" functionality. Note 1: As the GUI uses a balance cache since bitcoin-core/gui#598, this issue does not affect the regular spending process. Only arises when the user manually select coins. Note 2: Added test coverage for the `useAvailableBalance` functionality. ---------------------------------- Second Issue Description (bitcoin#26687): As we are using a cached balance on `WalletModel::getAvailableBalance`, the function needs to include the watch-only available balance for wallets with private keys disabled. ACKs for top commit: Sjors: tACK 68eed5d achow101: ACK 68eed5d theStack: ACK 68eed5d Tree-SHA512: 674f3e050024dabda2ff4a04b9ed3750cf54a040527204c920e1e38bd3d7f5fd4d096e4fd08a0fea84ee6abb5070f022b5c0d450c58fd30202ef05ebfd7af6d3 (cherry picked from commit 27dcc07)
33e2b82 wallet, bench: Remove unused database options from WalletBenchLoading (Andrew Chow) 80ace04 tests: Modify records directly in wallet ckey loading test (Andrew Chow) b3bb17d tests: Update DuplicateMockDatabase for MockableDatabase (Andrew Chow) f0eecf5 scripted-diff: Replace CreateMockWalletDB with CreateMockableWalletDB (Andrew Chow) 075962b wallet, tests: Include wallet/test/util.h (Andrew Chow) 14aa4cb wallet: Move DummyDatabase to salvage (Andrew Chow) f67a385 wallet, tests: Replace usage of dummy db with mockable db (Andrew Chow) 33c6245 Introduce MockableDatabase for wallet unit tests (Andrew Chow) Pull request description: For the wallet's unit tests, we currently use either `DummyDatabase` or memory-only versions of either BDB or SQLite. The tests that use `DummyDatabase` just need a `WalletDatabase` so that the `CWallet` can be constructed, while the tests using the memory-only databases just need a backing data store. There is also a `FailDatabase` that is similar to `DummyDatabase` except it fails be default or can have a configured return value. Having all of these different database types can make it difficult to write tests, particularly tests that work when either BDB or SQLite is disabled. This PR unifies all of these different unit test database classes into a single `MockableDatabase`. Like `DummyDatabase`, most functions do nothing and just return true. Like `FailDatabase`, the return value of some functions can be configured on the fly to test various failure cases. Like the memory-only databases, records can actually be written to the `MockableDatabase` and be retrieved later, but all of this is still held in memory. Using `MockableDatabase` completely removes the need for having BDB or SQLite backed wallets in the unit tests for the tests that are not actually testing specific database behaviors. Because `MockableDatabase`s can be created by each unit test, we can also control what records are stored in the database. Records can be added and removed externally from the typical database modification functions. This will give us greater ability to test failure conditions, particularly those involving corrupted records. Possible alternative to bitcoin#26644 ACKs for top commit: furszy: ACK 33e2b82 TheCharlatan: ACK 33e2b82 Tree-SHA512: c2b09eff9728d063d2d4aea28a0f0e64e40b76483e75dc53f08667df23bd25834d52656cd4eafb02e552db0b9e619cfdb1b1c65b26b5436ee2c971d804768bcc (cherry picked from commit d02df7d)
0282b21 walletdb: Remove unused CreateMockWalletDatabase (Andrew Chow) Pull request description: This has been superseded by the MockableDatabase. Remove to avoid confusion as to which type of mock database to use for testing. I thought this was included in bitcoin#26715, maybe it got lost in a rebase. ACKs for top commit: furszy: utACK 0282b21 brunoerg: crACK 0282b21 Tree-SHA512: 18445c4d8a4e2609ef7471c6d75297f43694b79e768f6c993a5addb1dc0e88bd12bac263c9d8e903d828ddf6bf50434f9e2f72048f4fc528e98fed8ee65123ca (cherry picked from commit b34e19a)
…heir own file and fix a bug 7379a54 bench: Remove incorrect LoadWallet call in WalletBalance (Andrew Chow) 846b2fe tests: Move ADDRESS_BCRT1_UNSPENDABLE to wallet/test/util.h (Andrew Chow) c61d3f0 tests, bench: Consolidate {Test,Bench}Un/LoadWallet helper (Andrew Chow) Pull request description: I have a few PRs and branches that use these two commits, probably makes sense to split them into a separate PR to be merged sooner. The first commit contains some things that end up being commonly used in new wallet benchmarks. These are moved into `wallet_common.{h/cpp}`. The second commit contains a bugfix for the wallet_balance benchmark where it calls `LoadWallet` in the wrong place. It's unnecessary to call that function in this benchmark. Although this does not cause any issues currently, it ends up causing issues in some PRs and branches that I'm working on. ACKs for top commit: Sjors: utACK 7379a54 furszy: ACK 7379a54 Tree-SHA512: 47773887a16c69ac7121c699d3446a8c399bd792a6a31714998b7b7a19fea179c6d3b29cb898b04397b2962c1b4120d57009352b8460b8283e188d4cb480c9ba (cherry picked from commit 05ec664)
…tionInternal Dash-specific follow-up to the bitcoin#25647 backport (wallet: return change from SelectionResult). Transactions that end up needing 253 or more inputs could fail transaction creation with "Fee needed > fee paid", even though 252-input transactions with otherwise identical parameters succeeded. Root cause: coin_selection_params.tx_noinputs_size (set in CreateTransactionInternal, src/wallet/spend.cpp) hardcodes the vin-count CompactSize prefix as 1 byte, because the final input count isn't known until after coin selection runs. This estimate feeds SelectionResult::m_target (introduced by bitcoin#25647) and therefore change_amount/nFeeRet. Once the final input count crosses a CompactSize size class (253, or 65536), the real prefix is wider (3 or 9 bytes) than assumed, so nFeeRet ends up a few duffs short of fee_needed, which is computed from the accurately-measured final transaction size. The existing STR_INTERNAL_BUG("Fee needed > fee paid") check then unconditionally aborted an otherwise perfectly valid transaction. Fix: when this shortfall is detected and a change output exists, recover it from the change output instead of failing outright: reduce change by the shortfall, or if that would push change below the viable-change floor, drop the change output entirely (its value goes to the fee) and recompute the transaction size/fee. This keeps the fix scoped to the rare boundary-crossing case instead of globally over-reserving bytes for every transaction. Upstream Bitcoin Core still hardcodes tx_noinputs_size's input-count prefix to 1 byte in the same way as of this writing, so this defect is expected to still affect current Bitcoin Core; this commit only fixes Dash Core. Adds a regression test (src/wallet/test/spend_tests.cpp) that forces exactly 252 and then 253 wallet UTXOs as required inputs via CCoinControl::fRequireAllInputs, confirming 252 succeeds unconditionally and 253 fails on the pre-fix code with the exact "Fee needed > fee paid" error before passing with this fix. CoinJoin does not exercise this path directly (CoinJoin fully-mixed sends never create change, per the existing comment in CreateTransactionInternal), so the test is left decoupled from CoinJoin internals.
thepastaclaw
force-pushed
the
backport-0.26-batch-2-rebuild
branch
from
be8a7bf to
859e53d
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 859e53d1fb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Return early if we cannot cover the target with the wallet's UTXO. | ||
| // We use the total effective value if we are not subtracting fee from outputs and 'available_coins' contains the data. | ||
| CAmount available_coins_total_amount = coin_selection_params.m_subtract_fee_outputs ? available_coins.GetTotalAmount() : | ||
| (available_coins.GetEffectiveTotalAmount().has_value() ? *available_coins.GetEffectiveTotalAmount() : 0); |
There was a problem hiding this comment.
When fees are not subtracted from outputs, this precheck uses the sum of every available coin's effective value, including negative-effective dust. A wallet with one larger spendable UTXO and enough small negative-effective UTXOs can have this total fall below selection_target, so CreateTransaction returns “Insufficient funds” before coin selection has a chance to pick the larger UTXO by itself; the check should ignore negative effective values or defer to the selector.
Useful? React with 👍 / 👎.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/functional/wallet_fundrawtransaction.py (1)
385-387: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove this non-functional backport refactor.
The
txid/voutlocals do not change behavior and add divergence from the upstream functional test. Keep this file limited to correctness, compatibility, or failing-test fixes.Based on learnings, Bitcoin Core backport functional tests should remain as close to upstream as possible; only necessary changes should be retained.
Proposed minimal diff
- txid = "1c7f966dab21119bac53213a2bc7532bff1fa844c124fd750a7d0b1332440bd1" - vout = 0 - inputs = [ {'txid' : txid, 'vout' : vout} ] `#invalid` vin! + inputs = [ {'txid' : "1c7f966dab21119bac53213a2bc7532bff1fa844c124fd750a7d0b1332440bd1", 'vout' : 0} ] `#invalid` vin!🤖 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/wallet_fundrawtransaction.py` around lines 385 - 387, Remove the unnecessary txid and vout local variables in the invalid-vin setup of the fundrawtransaction test, restoring the upstream-style inline input definition while preserving the same invalid transaction values and test behavior.Source: Learnings
🤖 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/walletdb.cpp`:
- Around line 898-904: Update the descriptor_unknown branch in the
cursor-iteration logic to call m_batch->CloseCursor() immediately before
returning DBErrors::UNKNOWN_DESCRIPTOR, matching the sibling early-return
cleanup pattern. Preserve the existing error message and return value.
---
Nitpick comments:
In `@test/functional/wallet_fundrawtransaction.py`:
- Around line 385-387: Remove the unnecessary txid and vout local variables in
the invalid-vin setup of the fundrawtransaction test, restoring the
upstream-style inline input definition while preserving the same invalid
transaction values and test behavior.
🪄 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: 44e806a8-c42f-4120-92b8-f37762ed5d7a
📒 Files selected for processing (47)
src/Makefile.bench.includesrc/Makefile.test.includesrc/Makefile.test_util.includesrc/bench/block_assemble.cppsrc/bench/coin_selection.cppsrc/bench/wallet_balance.cppsrc/bench/wallet_create_tx.cppsrc/bench/wallet_loading.cppsrc/qt/sendcoinsdialog.cppsrc/qt/sendcoinsdialog.hsrc/qt/test/addressbooktests.cppsrc/qt/test/wallettests.cppsrc/qt/walletmodel.cppsrc/rpc/client.cppsrc/rpc/evo.cppsrc/rpc/rawtransaction.cppsrc/rpc/util.cppsrc/rpc/util.hsrc/test/fuzz/rpc.cppsrc/test/util/wallet.cppsrc/test/util/wallet.hsrc/wallet/coincontrol.hsrc/wallet/coinselection.cppsrc/wallet/coinselection.hsrc/wallet/db.hsrc/wallet/interfaces.cppsrc/wallet/salvage.cppsrc/wallet/spend.cppsrc/wallet/spend.hsrc/wallet/test/coinjoin_tests.cppsrc/wallet/test/coinselector_tests.cppsrc/wallet/test/fuzz/coinselection.cppsrc/wallet/test/group_outputs_tests.cppsrc/wallet/test/ismine_tests.cppsrc/wallet/test/scriptpubkeyman_tests.cppsrc/wallet/test/spend_tests.cppsrc/wallet/test/util.cppsrc/wallet/test/util.hsrc/wallet/test/wallet_test_fixture.cppsrc/wallet/test/wallet_tests.cppsrc/wallet/test/walletload_tests.cppsrc/wallet/wallet.cppsrc/wallet/walletdb.cppsrc/wallet/walletdb.htest/functional/rpc_psbt.pytest/functional/wallet_fundrawtransaction.pytest/functional/wallet_send.py
💤 Files with no reviewable changes (4)
- src/test/util/wallet.h
- src/test/util/wallet.cpp
- src/bench/block_assemble.cpp
- src/wallet/db.h
🚧 Files skipped from review as they are similar to previous changes (7)
- src/test/fuzz/rpc.cpp
- src/rpc/util.h
- src/rpc/util.cpp
- src/Makefile.test_util.include
- src/rpc/client.cpp
- src/rpc/rawtransaction.cpp
- src/bench/wallet_balance.cpp
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Policy gate: three unallowlisted backport-prerequisite findings were restored as blocking, so the canonical action is REQUEST_CHANGES. The explicitly documented bitcoin#17304 exclusion and the verifier-refuted bitcoin#26560 assert claim were allowlisted.
Prior reconciliation: prior-1 (bitcoin#25685) FIXED; prior-2 (bitcoin#26715) FIXED; prior-3 (legacy-input wording) INTENTIONALLY DEFERRED and carried forward without a fresh comment; prior-4 (unused script/standard.h include) FIXED because the rebuilt header now declares CTxDestination getNewDestination(CWallet&).
Carried-forward prior findings: prior-3 only, intentionally deferred.
New findings in the rebuilt/latest delta: stale bitcoin#26532 prerequisite SHA/order note; undocumented gui#598 prerequisite/omitted bitcoin#26699 GUI hunks; and incomplete adoption of the bitcoin#26715 m_records-based wallet-load test shape. The verifier also confirmed two wallet-selection concerns are inherited unchanged from upstream and therefore out of scope for this adaptation review.
Source: orchestrator — openai/gpt-5.6-sol (high, orchestration-only); reviewers — gpt-5.6-sol (high) and claude-sonnet-5 (high effort) for general, dash-core-commit-history, and backport-reviewer; verifier — claude-sonnet-5. Experiment: sonnet-primary-opus-quarter-sample-20260710, sonnet_primary bucket 3 (no Opus reviewer).
🔴 3 blocking | 🟡 3 suggestion(s)
3 additional finding(s) omitted (not in diff).
🤖 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:81a5d253573>`:
- [SUGGESTION] <commit:81a5d253573>:1: Dash notes in the #26532 commit message cite a stale prerequisite SHA/ordering
The commit message for 81a5d253573 (Merge bitcoin/bitcoin#26532) states the ee7a984f8501 prerequisite was 'already backported separately as commit 77e4504f67b, before this branch's #26021 insertion.' Verified directly: `git merge-base --is-ancestor 77e4504f67b 81a5d253573` fails (77e4504f67b is not an ancestor of this branch at all — it belongs to a different branch line). The actual immediate prerequisite in the rebuilt stack is 95db517836a ('refactor: unify test/util/wallet.h with wallet/test/util.h', cherry-picked from ee7a984f8501), whose parent is 79cb403d3e3 (Merge bitcoin/bitcoin#26021) — i.e. the real order is #26021 THEN 95db517836a, immediately before 81a5d253573. This is a documentation-accuracy issue in a durable Dash note, not a functional defect, but it will mislead future readers tracing prerequisite provenance.
In `src/qt/walletmodel.cpp`:
- [SUGGESTION] src/qt/walletmodel.cpp:261: bitcoin-core/gui#598 balance-cache prerequisite for #26699 remains unported
Upstream #26699 ('gui: bugfix, getAvailableBalance skips selected coins') is written against WalletModel's cached getAvailableBalance introduced by gui#598. Dash never backported that cache; src/qt/walletmodel.cpp:261 and src/qt/sendcoinsdialog.cpp:895-906 call `model->wallet().getAvailableBalance(coin_control)` directly on every invocation instead of going through a cached wrapper. Verified this is a real, undocumented gap — no 'Dash notes' section in 579d5d8e462 acknowledges the omission, unlike other prerequisite decisions in this stack (e.g. 81a5d253573, bb2e6c40885). However, verified impact is low: both bugs #26699 fixes (selected-coins-skipped, and missing watch-only balance) are specific to the WalletModel *cache* path; Dash's uncached direct call to wallet().getAvailableBalance() in src/wallet/interfaces.cpp:526-550 already computes the correct value on every call (FetchSelectedInputs() for selected coins, AvailableCoins() otherwise), so neither upstream bug can manifest here. Document this omission explicitly (a one-line Dash note) rather than leaving it silent, since per project policy an undocumented missing prerequisite is otherwise treated as blocking regardless of current build/runtime safety.
- [BLOCKING] src/qt/walletmodel.cpp:261: bitcoin#26699's GUI-side hunks depend on bitcoin-core/gui#598, which Dash has not backported
The cached wrapper prerequisite is absent. Dash's direct uncached call is operationally safe, but the omitted prerequisite and GUI hunks are undocumented.
---
**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.
In `src/wallet/test/walletload_tests.cpp`:
- [SUGGESTION] src/wallet/test/walletload_tests.cpp:1-148: walletload_tests.cpp keeps the old batch/cursor test shape instead of MockableDatabase::m_records
Verified: walletload_tests.cpp (187 lines total) still drives its crypted-key checksum test through manual `db->MakeBatch()` / `StartCursor` / `ReadAtCursor` calls rather than directly manipulating `MockableDatabase::m_records` (declared in wallet/test/util.h since a080d63cb53's #26715 backport). The invariant under test (encrypted legacy wallet with one known crypted key, checksum validity) is preserved and the mechanical helper rename (CreateMockWalletDatabase -> CreateMockableWalletDatabase) compiles and passes, so this is not a functional bug — just missed adoption of the cleaner test-authoring shape #26715 was meant to enable. Low priority, but worth picking up alongside any future #26715-adjacent test work.
- [BLOCKING] src/wallet/test/walletload_tests.cpp:1-90: bitcoin#26715's rewritten wallet_load_ckey test content not adopted; old test structure kept
Dash retains the old batch/cursor test shape with mechanical MockableDatabase helper renames rather than adopting the upstream m_records-based test structure. The invariant remains covered, but the upstream test-content hunk is omitted without a documented 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: 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.
In `src/qt/sendcoinsdialog.cpp`:
- [BLOCKING] src/qt/sendcoinsdialog.cpp:895-906: Missing prerequisite: bitcoin-core/gui#598
Upstream #26699 starts with WalletModel::getAvailableBalance from gui#598 and modifies its cached balance behavior. Dash lacks that helper, omits the walletmodel hunk/watch-only test, and adapts SendCoinsDialog to call the wallet interface directly; the omission is not documented.
---
**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.
Note: GitHub does not allow me to submit REQUEST_CHANGES on my own PR, so this is posted as a COMMENT while preserving the canonical blocking findings.
WalletBatch::LoadWallet opens a DB cursor via m_batch->StartCursor() and every other early return from inside the cursor-iteration loop closes it first (see the sibling !ret path that calls m_batch->CloseCursor() before returning DBErrors::CORRUPT). The descriptor_unknown branch, added when backporting bitcoin#26021, returned DBErrors::UNKNOWN_DESCRIPTOR directly and left the cursor open until the enclosing batch was destroyed. Dash still uses the explicit StartCursor()/CloseCursor() form here (upstream later moved to RAII DatabaseCursor handles, so its own early return is safe). Leaving the cursor open is inconsistent with the sibling paths and is a latent hazard because StartCursor() asserts !m_cursor, so any reuse of the batch would abort. Close the cursor before the early return to match the surrounding pattern. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Re: the CodeRabbit nitpick on The txid = "1c7f966dab21119bac53213a2bc7532bff1fa844c124fd750a7d0b1332440bd1"
vout = 0
inputs = [ {'txid' : txid, 'vout' : vout} ] #invalid vin!Reverting to the inline form would create divergence from upstream, not remove it. It's also behavior-neutral and out of scope for this cursor-cleanup fix, so I'm leaving it as-is. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80450061d0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // GroupOutputs' filter-list overload. AttemptSelection itself never calls GroupOutputs. | ||
| auto result{ChooseSelectionResult(nTargetValue, groups, coin_selection_params, nCoinType, wallet.m_default_max_tx_fee)}; | ||
| // If any specific error message appears here, then something particularly wrong happened. | ||
| if (HasErrorMsg(result)) return result; // So let's return the specific error. |
There was a problem hiding this comment.
When allow_mixed_output_types is true, a detailed failure from the legacy-only pass, such as max-weight exceeded after considering many small legacy UTXOs, returns here before the mixed_groups fallback is attempted below. In a wallet that also has spendable non-legacy/other outputs large enough to fund the target, transaction creation fails with the legacy error instead of selecting the mixed/all-coins solution; keep this error only if the mixed fallback also fails.
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The latest one-line delta correctly closes the wallet DB cursor before the UNKNOWN_DESCRIPTOR early return. All six findings from 859e53d remain STILL VALID and are carried forward: the undocumented gui#598/bitcoin#26699 GUI omission, the undocumented bitcoin#26715 m_records test omission, and the stale bitcoin#26532 prerequisite note.
Carried-forward prior findings: all six prior IDs, consolidated by root cause in the verifier and retained/restored by the deterministic prerequisite policy.
New findings in the latest delta: the existing mock test cannot observe cursor closure, and the PR body still says the stack ends at 859e53d. The proposed squash into the upstream bitcoin#26021 commit was rejected because a separate Dash repair preserves backport fidelity.
Source: experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_primary, bucket 1; reviewers gpt-5.6-sol (high) and claude-sonnet-5 (high effort) for general, dash-core-commit-history, and backport-reviewer; primary verifier claude-sonnet-5 was unparseable, final fallback verifier gpt-5.6-sol (high); orchestrator openai/gpt-5.6-sol (high, orchestration-only and not reviewer/verifier evidence).
🔴 5 blocking | 🟡 2 suggestion(s) | 💬 1 nitpick(s)
3 additional finding(s) omitted (not in diff).
🤖 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 `src/qt/sendcoinsdialog.cpp`:
- [BLOCKING] src/qt/sendcoinsdialog.cpp:895-906: Document or backport the gui#598 prerequisite and omitted #26699 GUI hunks
Upstream bitcoin#26699 modifies WalletModel::getAvailableBalance, a cached wrapper introduced by bitcoin-core/gui#598. Dash lacks that prerequisite, omits the WalletModel/watch-only hunks, and adapts SendCoinsDialog to call the wallet interface directly without documenting the exclusion.
- [BLOCKING] src/qt/sendcoinsdialog.cpp:895-906: Missing prerequisite: bitcoin-core/gui#598
Upstream bitcoin#26699 modifies WalletModel::getAvailableBalance, a cached wrapper introduced by bitcoin-core/gui#598. Dash lacks that prerequisite, omits the WalletModel/watch-only hunks, and adapts SendCoinsDialog to call the wallet interface directly without documenting the 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: 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.
In `src/wallet/test/walletload_tests.cpp`:
- [BLOCKING] src/wallet/test/walletload_tests.cpp:1-148: Adopt or document the omitted #26715 m_records test rewrite
Dash mechanically adopts CreateMockableWalletDatabase but retains the old batch/cursor wallet-load test instead of upstream's m_records rewrite. The invariant remains covered, but the omitted upstream test-content hunk is undocumented.
- [BLOCKING] src/wallet/test/walletload_tests.cpp:1-90: bitcoin#26715's rewritten wallet_load_ckey test content not adopted; old test structure kept
Dash mechanically adopts CreateMockableWalletDatabase but retains the old batch/cursor wallet-load test instead of upstream's m_records rewrite. The invariant remains covered, but the omitted upstream test-content hunk is undocumented.
---
**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.
In `<commit:81a5d253573>`:
- [SUGGESTION] <commit:81a5d253573>:1: Correct the stale #26532 prerequisite SHA and ordering
The Dash notes cite unreachable commit 77e4504f67b and claim it preceded #26021. The actual prerequisite is 95db517836a immediately after the #26021 backport.
In `src/wallet/walletdb.cpp`:
- [SUGGESTION] src/wallet/walletdb.cpp:904: Add coverage that observes cursor closure on UNKNOWN_DESCRIPTOR
The new CloseCursor call is correct, but wallet_load_unknown_descriptor cannot protect it because MockableBatch::CloseCursor is a no-op and MockableBatch::StartCursor does not model the real backends' open-cursor assertion. Model cursor-open state in the mock and exercise reuse after LoadWallet returns.
In `src/qt/walletmodel.cpp`:
- [BLOCKING] src/qt/walletmodel.cpp:261: bitcoin#26699's GUI-side hunks depend on bitcoin-core/gui#598, which Dash has not backported
The cached WalletModel wrapper prerequisite is absent. Dash's direct uncached call is operationally safe, but the omitted prerequisite and GUI hunks are undocumented.
---
**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.
Note: GitHub does not allow me to submit REQUEST_CHANGES on my own PR, so this is posted as a COMMENT while preserving the canonical blocking findings.
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
✅ Action performedReviews resumed. |
3 participants
backport: Bitcoin Core v0.26 wallet transaction creation fixes and prerequisites
Issue being fixed or feature implemented
This rebuilds Bitcoin Core v0.26 backport batch 2 on current
developwith the complete prerequisite chain required by the wallet transaction-creation changes.It also adds a separate Dash-only follow-up for a regression exposed by the
bitcoin/bitcoin#25647backport: transactions requiring 253 or more inputs could fail withFee needed > fee paidwhen the serialized input-count CompactSize prefix grew from 1 byte to 3 bytes.This supersedes the closed #7290 and replaces the earlier incomplete history on this PR.
What was done?
The branch contains this linear, reviewable stack:
SelectionResultdescriptorprocesspsbtrpc bitcoin/bitcoin#25796: rpc: adddescriptorprocesspsbttest/util/wallet.hwithwallet/test/util.hchecksum_validstateCoinsResultMockableDatabasefor wallet unit tests bitcoin/bitcoin#26715: introduceMockableDatabasefor wallet unit testsCreateMockWalletDatabaseCreateTransactionInternalThe stack is based on
f343d585eba2cfcf233956b68c9865daf3e66e5aand ends at859e53d1fb17ef128b76f245e718bbe47a3f06d9.Backport review notes
bitcoin/bitcoin#26560is included in its upstream position between wallet: bugfix, invalid crypted key "checksum_valid" set bitcoin/bitcoin#26532 and wallet: Coin Selection, return accurate error messages bitcoin/bitcoin#26661.ErrorMaxWeightExceeded()instead of being collapsed to generic “Insufficient funds”; unit and RPC regression coverage are included.6a302d40. Dash precomputes both the legacy-only groups and the all-coins fallback groups across all ordered eligibility filters.AttemptSelectionperforms noGroupOutputscalls, while wallets whose eligible coins exist only inCoinsResult::otherremain supported.descriptorprocesspsbthelp is adapted to Dash’s accepted sighash forms and does not retain Bitcoin-only SegWit/Taproot wording.Dash-only CompactSize regression fix
CreateTransactionInternalestimates the non-input transaction size before coin selection, when the final input count is unknown. The estimate assumes a 1-byte CompactSize input-count prefix. At 253 inputs, the real prefix becomes 3 bytes; at 65,536 inputs it becomes 9 bytes.After bitcoin#25647, that estimate feeds
SelectionResult::m_target, change calculation, and the collected fee. The final fee requirement is calculated from the accurately serialized transaction, so crossing a CompactSize boundary can leave the collected fee a few duffs short and trigger the internalFee needed > fee paidfailure.The separate final commit recovers that small shortfall from the change output. If reducing change would make it nonviable, it drops the change output and recomputes transaction size and required fee.
Regression tests force exactly 252 and 253 required wallet inputs with
CCoinControl::fRequireAllInputs: the below-boundary case remains valid, and the at-boundary case now succeeds instead of hitting the internal error.Bitcoin Core still has the same 1-byte input-count estimate and failure path as of this backport; this commit fixes Dash Core only.
How has this been tested?
On macOS arm64 against the final rebased head:
test_dash: 754 tests, zero failureswallet_fundrawtransaction.py(descriptor, legacy, legacy/no-HD)rpc_psbt.py(descriptor and legacy)wallet_send.py(descriptor and legacy)wallet_basic.py(descriptor and legacy)wallet_avoidreuse.py(descriptor and legacy)git diff --check859e53d1fb17ef128b76f245e718bbe47a3f06d9: PASS, no findingsBreaking changes
None expected beyond the intended behavior of the upstream Bitcoin Core backports. The Dash-only follow-up turns a transaction-creation internal error into successful fee recovery from change.
Checklist