backport: bitcoin/bitcoin#24118: Add 'sendall' RPC née sweep and related backports#7131
Merged
PastaPastaPasta merged 8 commits intodashpay:developfrom Mar 30, 2026
Merged
backport: bitcoin/bitcoin#24118: Add 'sendall' RPC née sweep and related backports#7131PastaPastaPasta merged 8 commits intodashpay:developfrom
PastaPastaPasta merged 8 commits intodashpay:developfrom
Conversation
|
|
This pull request has conflicts, please rebase. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
WalkthroughAdds a new wallet RPC method, sendall, that spends specified UTXOs to one or more recipients without creating change. Introduces helpers for fee-estimation and option validation (InterpretFeeEstimationInstructions, PreventOutdatedOptions) and centralizes PSBT finalization/commit logic (FinishTransaction). Extends RPC parameter conversion for sendall, registers the sendall RPC, and implements extensive functional tests and test-runner integration. Updates release notes to document the new RPC. Sequence DiagramsequenceDiagram
participant Client as User/Client
participant RPC as RPC Handler
participant Wallet as Wallet
participant Builder as Tx Builder
participant Network as P2P/Chain
Client->>RPC: sendall(recipients, inputs?, options...)
RPC->>RPC: Validate recipients, inputs, options
RPC->>RPC: InterpretFeeEstimationInstructions()
RPC->>RPC: PreventOutdatedOptions()
RPC->>Wallet: Select/verify UTXOs (lock if requested)
Wallet-->>RPC: Selected inputs
RPC->>Builder: Build transaction / estimate fee / create PSBT
Builder-->>RPC: PSBT / raw tx
RPC->>Wallet: FillPSBT, Sign, Finalize (FinishTransaction)
Wallet-->>RPC: Finalized tx, psbt, hex
RPC->>Network: Broadcast tx (if requested)
Network-->>RPC: Broadcast result / txid
RPC-->>Client: Return txid, psbt/hex, complete status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
🤖 Fix all issues with AI agents
In `@src/wallet/rpc/spend.cpp`:
- Around line 1156-1160: The check incorrectly treats presence of the send_max
option as the conflict; change the guard to only reject when send_max is true.
Replace the condition that uses options.exists("send_max") (in the block around
total_input_value and send_max) with a check that verifies send_max itself (the
bool variable) when deciding to throw for combining inputs and send_max, i.e.
use if (options.exists("inputs") && send_max) and leave the subsequent inputs
handling unchanged.
- Around line 1135-1140: The AvailableCoins call in this sendall path may ignore
the wallet's avoid_reuse setting because coin_control.m_avoid_address_reuse is
not set; update the sendall flow to set coin_control.m_avoid_address_reuse from
pwallet->GetAvoidReuse() (or equivalent accessor) before calling
AvailableCoins(), mirroring other spend paths that set
CCoinControl::m_avoid_address_reuse; ensure this assignment occurs after
creating CCoinControl and before AvailableCoins() so the avoid-reuse behavior is
respected.
In `@test/functional/wallet_sendall.py`:
- Around line 45-49: The condition in assert_tx_has_output incorrectly uses
"and" / "or" precedence, causing outputs with a matching value but wrong address
to pass; update the if in assert_tx_has_output to require addr ==
output["scriptPubKey"]["address"] AND (value is None OR value ==
output["value"]) so the address must match and the value check is grouped, i.e.,
add parentheses around the value checks when evaluating each output in
tx["decoded"]["vout"] before returning.
🧹 Nitpick comments (2)
test/functional/wallet_sendall.py (2)
149-167:Decimal(0.00000010)uses a float literal — consider string literal for precision.Line 167 passes a float to
Decimal(), which can introduce subtle floating-point representation issues.Decimal("0.00000010")would be safer. Flagged by Ruff (RUF032). However, since this is backported upstream code, this is informational only.
296-320: Note: the watchonly wallet is not unloaded after use.Unlike the
dustwalletinsendall_negative_effective_value(line 187), the"watching"wallet created on line 301 is never unloaded. This is unlikely to cause problems since it's near the end of the test, but for symmetry and resource cleanup, consider unloading it. That said, this matches upstream behavior, so no action needed.
Comment on lines
+1135
to
1140
| CCoinControl coin_control; | ||
|
|
||
| SetFeeEstimateMode(*pwallet, coin_control, options["conf_target"], options["estimate_mode"], options["fee_rate"], /*override_min_fee=*/false); | ||
|
|
||
| coin_control.fAllowWatchOnly = ParseIncludeWatchonly(options["include_watching"], *pwallet); | ||
|
|
There was a problem hiding this comment.
sendall does not explicitly apply the avoid_reuse wallet flag.
The help text says sendall respects avoid_reuse, but coin_control.m_avoid_address_reuse isn’t set before AvailableCoins(), so the wallet flag may be ignored. Consider wiring it the same way as other spend paths.
💡 Proposed fix
coin_control.fAllowWatchOnly = ParseIncludeWatchonly(options["include_watching"], *pwallet);
+ coin_control.m_avoid_address_reuse = GetAvoidReuseFlag(*pwallet, NullUniValue);
+ // Enable partial-spend avoidance when reuse avoidance is active.
+ coin_control.m_avoid_partial_spends |= coin_control.m_avoid_address_reuse;🤖 Prompt for AI Agents
In `@src/wallet/rpc/spend.cpp` around lines 1135 - 1140, The AvailableCoins call
in this sendall path may ignore the wallet's avoid_reuse setting because
coin_control.m_avoid_address_reuse is not set; update the sendall flow to set
coin_control.m_avoid_address_reuse from pwallet->GetAvoidReuse() (or equivalent
accessor) before calling AvailableCoins(), mirroring other spend paths that set
CCoinControl::m_avoid_address_reuse; ensure this assignment occurs after
creating CCoinControl and before AvailableCoins() so the avoid-reuse behavior is
respected.
Comment on lines
+1156
to
+1160
| CAmount total_input_value(0); | ||
| bool send_max{options.exists("send_max") ? options["send_max"].get_bool() : false}; | ||
| if (options.exists("inputs") && options.exists("send_max")) { | ||
| throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot combine send_max with specific inputs."); | ||
| } else if (options.exists("inputs")) { |
There was a problem hiding this comment.
send_max incompatibility check is too strict.
The current guard rejects inputs even when send_max is explicitly set to false. That makes “default false” options objects fail unnecessarily.
🐛 Proposed fix
- if (options.exists("inputs") && options.exists("send_max")) {
+ if (options.exists("inputs") && send_max) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot combine send_max with specific inputs.");
} else if (options.exists("inputs")) {🤖 Prompt for AI Agents
In `@src/wallet/rpc/spend.cpp` around lines 1156 - 1160, The check incorrectly
treats presence of the send_max option as the conflict; change the guard to only
reject when send_max is true. Replace the condition that uses
options.exists("send_max") (in the block around total_input_value and send_max)
with a check that verifies send_max itself (the bool variable) when deciding to
throw for combining inputs and send_max, i.e. use if (options.exists("inputs")
&& send_max) and leave the subsequent inputs handling unchanged.
Comment on lines
+45
to
+49
| def assert_tx_has_output(self, tx, addr, value=None): | ||
| for output in tx["decoded"]["vout"]: | ||
| if addr == output["scriptPubKey"]["address"] and value is None or value == output["value"]: | ||
| return | ||
| raise AssertionError("Output to {} not present or wrong amount".format(addr)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the actual call sites to verify the usage patterns mentioned
rg -n "assert_tx_has_output" test/functional/wallet_sendall.py | head -20Repository: dashpay/dash
Length of output: 626
🏁 Script executed:
#!/bin/bash
# Extract the context around the problematic line and call sites
sed -n '45,55p' test/functional/wallet_sendall.py
echo "---"
sed -n '210,220p' test/functional/wallet_sendall.pyRepository: dashpay/dash
Length of output: 1309
🏁 Script executed:
#!/bin/bash
# Check if there's any git history showing this is from Bitcoin Core
cd test/functional && git log --oneline -n 20 wallet_sendall.py 2>/dev/null || echo "File is new in this PR"Repository: dashpay/dash
Length of output: 182
🏁 Script executed:
#!/bin/bash
# Verify Python operator precedence: and vs or
python3 << 'EOF'
# Test operator precedence
# In Python: and has higher precedence than or
# So "A and B or C" is evaluated as "(A and B) or C"
# Simulate the logic with concrete values
addr_matches = True
value_is_none = False
value_matches = True
# Current code: addr == ... and value is None or value == ...
current_logic = (addr_matches and value_is_none) or (value_matches)
print(f"Current logic (addr_matches=T, value_is_none=F, value_matches=T): {current_logic}")
print(f"Evaluated as: (True and False) or True = {current_logic}")
# With mismatched address but matching value (the bug scenario)
addr_matches = False
value_is_none = False
value_matches = True
current_logic = (addr_matches and value_is_none) or (value_matches)
print(f"\nBug scenario (addr_matches=F, value_is_none=F, value_matches=T): {current_logic}")
print(f"Evaluated as: (False and False) or True = {current_logic}")
# Intended logic with parentheses: addr == ... and (value is None or value == ...)
addr_matches = False
value_is_none = False
value_matches = True
intended_logic = (addr_matches) and (value_is_none or value_matches)
print(f"\nIntended logic with fix (addr_matches=F, value_is_none=F, value_matches=T): {intended_logic}")
print(f"Evaluated as: False and (False or True) = {intended_logic}")
EOFRepository: dashpay/dash
Length of output: 421
🏁 Script executed:
#!/bin/bash
# Check if bitcoin/bitcoin upstream has the same code
# Look for any references to Bitcoin Core upstream in the PR or file
grep -r "26722" . 2>/dev/null | head -5Repository: dashpay/dash
Length of output: 1010
🏁 Script executed:
#!/bin/bash
# Verify the exact logic at lines 51-54 to confirm the assert_tx_has_outputs usage
sed -n '51,54p' test/functional/wallet_sendall.pyRepository: dashpay/dash
Length of output: 292
Fix operator precedence: parenthesize the or clause to match intended logic.
Line 47 contains a genuine operator precedence bug. Because and binds tighter than or, the condition evaluates as (addr == output["scriptPubKey"]["address"] and value is None) or (value == output["value"]). This means a matching value on a wrong address would return success.
The intended logic requires parentheses:
if addr == output["scriptPubKey"]["address"] and (value is None or value == output["value"]):This bug doesn't affect the current test suite because:
- Line 54 (via
assert_tx_has_outputs) calls with both address and value provided, so the address must match for the function to succeed anyway - Line 217 calls with
value=None, so the secondorclause is irrelevant
However, a future test or code change that calls this function with a mismatched address but matching value would silently pass incorrectly.
Proposed fix
def assert_tx_has_output(self, tx, addr, value=None):
for output in tx["decoded"]["vout"]:
- if addr == output["scriptPubKey"]["address"] and value is None or value == output["value"]:
+ if addr == output["scriptPubKey"]["address"] and (value is None or value == output["value"]):
return
raise AssertionError("Output to {} not present or wrong amount".format(addr))🧰 Tools
🪛 Ruff (0.14.14)
[warning] 47-47: Parenthesize a and b expressions when chaining and and or together, to make the precedence clear
Parenthesize the and subexpression
(RUF021)
🤖 Prompt for AI Agents
In `@test/functional/wallet_sendall.py` around lines 45 - 49, The condition in
assert_tx_has_output incorrectly uses "and" / "or" precedence, causing outputs
with a matching value but wrong address to pass; update the if in
assert_tx_has_output to require addr == output["scriptPubKey"]["address"] AND
(value is None OR value == output["value"]) so the address must match and the
value check is grouped, i.e., add parentheses around the value checks when
evaluating each output in tx["decoded"]["vout"] before returning.
knst
changed the title
knst
pushed a commit
to knst/dash
that referenced
this pull request
Mar 2, 2026
…ated initializers Missing changes for `sendall` rpc, depends on dashpay#7131 fa2c72d rpc: Set RPCArg options with designated initializers (MacroFake) Pull request description: For optional constructor arguments, use a new struct. This comes with two benefits: * Earlier unused optional arguments can be omitted * Designated initializers can be used ACKs for top commit: stickies-v: re-ACK fa2c72d Tree-SHA512: 2a0619548187cc7437fee2466ac4780746490622f202659f53641be01bc2a1fea4416d1a77f3e963bf7c4cce62899b61fab0b9683440cf82f68be44f63826658
UdjinM6
reviewed
Mar 11, 2026
| create a changeless payment from select UTXOs. When creating a payment | ||
| from a specific amount for which the recipient incurs the transaction | ||
| fee, continue to use the `subtractfeefromamount` option via the | ||
| `send`, `sendtoaddress`, or `sendmany` RPCs. (#24118) |
There was a problem hiding this comment.
Should dashify: rename the file and update the ref to 7131
Collaborator
Author
There was a problem hiding this comment.
dashified text, the file name is untouched because it's anyway will be merged in release-notes.md at some point.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
test/functional/wallet_sendall.py (1)
45-49:⚠️ Potential issue | 🟡 MinorParenthesize the value check in
assert_tx_has_output.This condition currently evaluates as
(addr_match and value_is_none) or value_match, so a matching amount on the wrong address can still pass.Suggested fix
def assert_tx_has_output(self, tx, addr, value=None): for output in tx["decoded"]["vout"]: - if addr == output["scriptPubKey"]["address"] and value is None or value == output["value"]: + if addr == output["scriptPubKey"]["address"] and (value is None or value == output["value"]): return raise AssertionError("Output to {} not present or wrong amount".format(addr))#!/bin/bash python3 - <<'PY' addr_matches = False value_is_none = False value_matches = True current = (addr_matches and value_is_none) or value_matches fixed = addr_matches and (value_is_none or value_matches) print(f"current={current}") print(f"fixed={fixed}") PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/functional/wallet_sendall.py` around lines 45 - 49, The conditional in assert_tx_has_output currently evaluates as (addr_match and value_is_none) or value_match which allows a matching value at the wrong address; change the condition to require the address match first and then check the value, i.e. in assert_tx_has_output iterate outputs and use addr == output["scriptPubKey"]["address"] and (value is None or value == output["value"]) so an amount match alone cannot pass for a different address.src/wallet/rpc/spend.cpp (1)
1135-1140:⚠️ Potential issue | 🟠 MajorApply
avoid_reusetosendallcoin selection.
sendallstill advertises that it respectsavoid_reuse, butcoin_control.m_avoid_address_reuseis never set beforeAvailableCoins()runs. That means dirty outputs can still be selected here.Suggested fix
CCoinControl coin_control; SetFeeEstimateMode(*pwallet, coin_control, options["conf_target"], options["estimate_mode"], options["fee_rate"], /*override_min_fee=*/false); coin_control.fAllowWatchOnly = ParseIncludeWatchonly(options["include_watching"], *pwallet); + coin_control.m_avoid_address_reuse = GetAvoidReuseFlag(*pwallet, NullUniValue); + coin_control.m_avoid_partial_spends |= coin_control.m_avoid_address_reuse;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/rpc/spend.cpp` around lines 1135 - 1140, The sendall path currently prepares CCoinControl and calls AvailableCoins/coin selection without applying the avoid_reuse setting, so m_avoid_address_reuse is never set and reused (dirty) outputs may be selected; before any call that enumerates/selects coins (e.g., before AvailableCoins() or the selection logic in sendall), set coin_control.m_avoid_address_reuse from the RPC option (use the same parsing used elsewhere, e.g. read options["avoid_reuse"] into a bool via the existing parsing helper or wallet API) so that CCoinControl respects avoid_reuse; adjust the code around SetFeeEstimateMode/coin_control initialization to assign coin_control.m_avoid_address_reuse prior to coin enumeration/selection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/wallet/rpc/spend.cpp`:
- Around line 1244-1251: The code currently locks inputs (pwallet->LockCoin on
rawTx.vin) and then calls FinishTransaction(pwallet, options, rawTx), which can
fail and leave coins locked; update the flow to ensure coins are always unlocked
on failure by wrapping the locking + FinishTransaction call in a scope that
guarantees cleanup—either use an RAII guard that stores the locked outpoints and
calls pwallet->UnlockCoin(prevout) in its destructor, or catch
exceptions/inspect FinishTransaction's failure return and explicitly iterate
rawTx.vin to call pwallet->UnlockCoin(txin.prevout) before returning/erroring;
reference LockCoin, UnlockCoin, rawTx.vin, FinishTransaction and pwallet in your
changes.
In `@test/functional/wallet_sendall.py`:
- Around line 164-167: The test is using Decimal(0.00000010) which constructs
from a float and is imprecise; update the second assertion in
test/functional/wallet_sendall.py to use an exact Decimal constructed from a
string (e.g. Decimal("0.00000010") or Decimal("1e-7")) when calling
self.wallet.sendall with [{self.recipient: pre_sendall_balance - fee -
Decimal(...)} , self.remainder_target], so the dust-boundary value is exact.
---
Duplicate comments:
In `@src/wallet/rpc/spend.cpp`:
- Around line 1135-1140: The sendall path currently prepares CCoinControl and
calls AvailableCoins/coin selection without applying the avoid_reuse setting, so
m_avoid_address_reuse is never set and reused (dirty) outputs may be selected;
before any call that enumerates/selects coins (e.g., before AvailableCoins() or
the selection logic in sendall), set coin_control.m_avoid_address_reuse from the
RPC option (use the same parsing used elsewhere, e.g. read
options["avoid_reuse"] into a bool via the existing parsing helper or wallet
API) so that CCoinControl respects avoid_reuse; adjust the code around
SetFeeEstimateMode/coin_control initialization to assign
coin_control.m_avoid_address_reuse prior to coin enumeration/selection.
In `@test/functional/wallet_sendall.py`:
- Around line 45-49: The conditional in assert_tx_has_output currently evaluates
as (addr_match and value_is_none) or value_match which allows a matching value
at the wrong address; change the condition to require the address match first
and then check the value, i.e. in assert_tx_has_output iterate outputs and use
addr == output["scriptPubKey"]["address"] and (value is None or value ==
output["value"]) so an amount match alone cannot pass for a different address.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 48dd5180-9774-468c-a063-b8742b0a6147
📒 Files selected for processing (10)
doc/release-notes-24118.mdsrc/rpc/client.cppsrc/wallet/rpc/spend.cppsrc/wallet/rpc/wallet.cpptest/functional/test_runner.pytest/functional/wallet_basic.pytest/functional/wallet_fundrawtransaction.pytest/functional/wallet_groups.pytest/functional/wallet_sendall.pytest/functional/wallet_signer.py
🚧 Files skipped from review as they are similar to previous changes (2)
- test/functional/wallet_signer.py
- test/functional/wallet_fundrawtransaction.py
Comment on lines
+1244
to
+1251
| const bool lock_unspents{options.exists("lock_unspents") ? options["lock_unspents"].get_bool() : false}; | ||
| if (lock_unspents) { | ||
| for (const CTxIn& txin : rawTx.vin) { | ||
| pwallet->LockCoin(txin.prevout); | ||
| } | ||
| } | ||
|
|
||
| return FinishTransaction(pwallet, options, rawTx); |
There was a problem hiding this comment.
Avoid leaving coins locked when finalization fails.
These outpoints are locked before FinishTransaction() signs/commits. If signing fails afterwards, the RPC errors out but the inputs remain in setLockedCoins, which is a surprising persistent side effect.
Suggested fix
const bool lock_unspents{options.exists("lock_unspents") ? options["lock_unspents"].get_bool() : false};
if (lock_unspents) {
for (const CTxIn& txin : rawTx.vin) {
pwallet->LockCoin(txin.prevout);
}
}
-
- return FinishTransaction(pwallet, options, rawTx);
+ try {
+ return FinishTransaction(pwallet, options, rawTx);
+ } catch (...) {
+ if (lock_unspents) {
+ for (const CTxIn& txin : rawTx.vin) {
+ pwallet->UnlockCoin(txin.prevout);
+ }
+ }
+ throw;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/wallet/rpc/spend.cpp` around lines 1244 - 1251, The code currently locks
inputs (pwallet->LockCoin on rawTx.vin) and then calls
FinishTransaction(pwallet, options, rawTx), which can fail and leave coins
locked; update the flow to ensure coins are always unlocked on failure by
wrapping the locking + FinishTransaction call in a scope that guarantees
cleanup—either use an RAII guard that stores the locked outpoints and calls
pwallet->UnlockCoin(prevout) in its destructor, or catch exceptions/inspect
FinishTransaction's failure return and explicitly iterate rawTx.vin to call
pwallet->UnlockCoin(txin.prevout) before returning/erroring; reference LockCoin,
UnlockCoin, rawTx.vin, FinishTransaction and pwallet in your changes.
Collaborator
Author
There was a problem hiding this comment.
backported code; fix is out-of-scope
There was a problem hiding this comment.
@knst, understood — since this is backported code, keeping the change minimal makes sense. Would you like me to open a follow-up GitHub issue to track this lock_unspents cleanup-on-failure improvement for a future PR?
🧠 Learnings used
Learnt from: d0wn3d
Repo: dashpay/dash PR: 7238
File: src/wallet/rpc/wallet.cpp:0-0
Timestamp: 2026-03-20T19:03:15.360Z
Learning: In Dash Core/DashPay code, RPCHelpMan does not provide the Bitcoin Core-style `Arg<T>(...)` template. When handling optional boolean RPC parameters, avoid `self.Arg<bool>(X)`. Instead, use the dashp2p/UniValue null-check pattern on the request param, e.g. `request.params[X].isNull() || request.params[X].get_bool()` (or `!request.params[X].isNull() && request.params[X].get_bool()` if the default semantics require it), so you correctly distinguish between an omitted/null value and an explicit boolean.
Comment on lines
+164
to
+167
| assert_raises_rpc_error(-6, "Dynamically assigned remainder results in dust output.", self.wallet.sendall, | ||
| [{self.recipient: pre_sendall_balance - fee}, self.remainder_target]) | ||
| assert_raises_rpc_error(-6, "Dynamically assigned remainder results in dust output.", self.wallet.sendall, | ||
| [{self.recipient: pre_sendall_balance - fee - Decimal(0.00000010)}, self.remainder_target]) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
from decimal import Decimal
print(Decimal(0.00000010))
print(Decimal("0.00000010"))
PYRepository: dashpay/dash
Length of output: 133
🏁 Script executed:
sed -n '160,170p' test/functional/wallet_sendall.pyRepository: dashpay/dash
Length of output: 994
Use an exact Decimal literal in this dust-boundary test.
Decimal(0.00000010) is built from a binary float, producing an imprecise value (approximately 9.99999999999999954748111825886258685613938723690807819366455078125E-8) instead of the exact 1e-7. Use Decimal("0.00000010") to construct the decimal from a string.
Suggested fix
assert_raises_rpc_error(-6, "Dynamically assigned remainder results in dust output.", self.wallet.sendall,
- [{self.recipient: pre_sendall_balance - fee - Decimal(0.00000010)}, self.remainder_target])
+ [{self.recipient: pre_sendall_balance - fee - Decimal("0.00000010")}, self.remainder_target])🧰 Tools
🪛 Flake8 (7.3.0)
[error] 165-165: continuation line under-indented for visual indent
(E128)
[error] 167-167: continuation line under-indented for visual indent
(E128)
🪛 Ruff (0.15.5)
[warning] 167-167: Decimal() called with float literal argument
Replace with string literal
(RUF032)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/functional/wallet_sendall.py` around lines 164 - 167, The test is using
Decimal(0.00000010) which constructs from a float and is imprecise; update the
second assertion in test/functional/wallet_sendall.py to use an exact Decimal
constructed from a string (e.g. Decimal("0.00000010") or Decimal("1e-7")) when
calling self.wallet.sendall with [{self.recipient: pre_sendall_balance - fee -
Decimal(...)} , self.remainder_target], so the dust-boundary value is exact.
Collaborator
Author
There was a problem hiding this comment.
backported code; fix is out-of-scope
There was a problem hiding this comment.
@knst, understood — since this is backported code, keeping it aligned with upstream is reasonable. I'll note this for future reference.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The backport is well-executed with correct Dash-specific adaptations (MAX_STANDARD_TX_SIZE, duff/B fees, RBF removal, dust thresholds). Two instances of 'bitcoin address' in the new sendall RPC help text were missed during Dash adaptation — these are legitimate findings but are help-text-only issues that don't affect runtime behavior, so severity is downgraded from blocking to suggestion. One minor grammar nitpick is also valid. The codex-general agent's clean report is consistent with the code — no additional issues found.
Reviewed commit: 18441b2
🟡 2 suggestion(s) | 💬 1 nitpick(s)
🤖 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/wallet/rpc/spend.cpp`:
- [SUGGESTION] line 1031: RPC help text says 'bitcoin address' instead of 'Dash address'
The sendall RPC help text says `"A bitcoin address which receives an equal share of the unspecified amount."` — this should say "Dash address" for consistency with `send()` (line 911) and `sendmany()` (line 321), which both correctly reference "Dash address". Missed Dash adaptation from the upstream backport.
- [SUGGESTION] line 1034: RPC help text says 'bitcoin address' instead of 'Dash address' (second instance)
Same issue: `"the bitcoin address"` in the key-value pair description should say `"the Dash address"` for consistency with the rest of the wallet RPCs.
| {"recipients", RPCArg::Type::ARR, RPCArg::Optional::NO, "The sendall destinations. Each address may only appear once.\n" | ||
| "Optionally some recipients can be specified with an amount to perform payments, but at least one address must appear without a specified amount.\n", | ||
| { | ||
| {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "A bitcoin address which receives an equal share of the unspecified amount."}, |
There was a problem hiding this comment.
🟡 Suggestion: RPC help text says 'bitcoin address' instead of 'Dash address'
The sendall RPC help text says "A bitcoin address which receives an equal share of the unspecified amount." — this should say "Dash address" for consistency with send() (line 911) and sendmany() (line 321), which both correctly reference "Dash address". Missed Dash adaptation from the upstream backport.
💡 Suggested change
Suggested change
| {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "A bitcoin address which receives an equal share of the unspecified amount."}, | |
| {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "A Dash address which receives an equal share of the unspecified amount."}, |
source: ['claude']
🤖 Fix this 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/wallet/rpc/spend.cpp`:
- [SUGGESTION] line 1031: RPC help text says 'bitcoin address' instead of 'Dash address'
The sendall RPC help text says `"A bitcoin address which receives an equal share of the unspecified amount."` — this should say "Dash address" for consistency with `send()` (line 911) and `sendmany()` (line 321), which both correctly reference "Dash address". Missed Dash adaptation from the upstream backport.
| {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "A bitcoin address which receives an equal share of the unspecified amount."}, | ||
| {"", RPCArg::Type::OBJ_USER_KEYS, RPCArg::Optional::OMITTED, "", | ||
| { | ||
| {"address", RPCArg::Type::AMOUNT, RPCArg::Optional::NO, "A key-value pair. The key (string) is the bitcoin address, the value (float or string) is the amount in " + CURRENCY_UNIT + ""}, |
There was a problem hiding this comment.
🟡 Suggestion: RPC help text says 'bitcoin address' instead of 'Dash address' (second instance)
Same issue: "the bitcoin address" in the key-value pair description should say "the Dash address" for consistency with the rest of the wallet RPCs.
💡 Suggested change
Suggested change
| {"address", RPCArg::Type::AMOUNT, RPCArg::Optional::NO, "A key-value pair. The key (string) is the bitcoin address, the value (float or string) is the amount in " + CURRENCY_UNIT + ""}, | |
| {"address", RPCArg::Type::AMOUNT, RPCArg::Optional::NO, "A key-value pair. The key (string) is the Dash address, the value (float or string) is the amount in " + CURRENCY_UNIT + ""}, |
source: ['claude']
🤖 Fix this 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/wallet/rpc/spend.cpp`:
- [SUGGESTION] line 1034: RPC help text says 'bitcoin address' instead of 'Dash address' (second instance)
Same issue: `"the bitcoin address"` in the key-value pair description should say `"the Dash address"` for consistency with the rest of the wallet RPCs.
| { | ||
| {"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The transaction id"}, | ||
| {"vout", RPCArg::Type::NUM, RPCArg::Optional::NO, "The output number"}, | ||
| {"sequence", RPCArg::Type::NUM, RPCArg::DefaultHint{"depends on the value of the 'locktime' arguments"}, "The sequence number"}, |
There was a problem hiding this comment.
💬 Nitpick: Plural 'arguments' should be singular 'argument' after RBF removal
The Dash adaptation correctly removed the 'replaceable' reference (no RBF), but the plural 'arguments' wasn't updated to singular 'argument' now that only 'locktime' is referenced. Very minor grammar issue.
source: ['claude']
UdjinM6
reviewed
Mar 23, 2026
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
Rebases/fixes look good, one small issue: we use capital D in "Dash address" throughout the code
added 2 commits
March 24, 2026 13:02
BACKPORT NOTE: This commit also includes missing changes for RPC sendall from: - bitcoin#25083 - bitcoin#25005 - bitcoin#24584 Also there's an extra fix to dashify dust value for sendall to avoid test failure ---------- bb84b71 add tests for no recipient and using send_max while inputs are specified (ishaanam) 49090ec Add sendall RPC née sweep (Murch) 902793c Extract FinishTransaction from send() (Murch) 6d2208a Extract interpretation of fee estimation arguments (Murch) a31d75e Elaborate error messages for outdated options (Murch) 35ed094 Extract prevention of outdated option names (Murch) Pull request description: Add sendall RPC née sweep _Motivation_ Currently, the wallet uses a fSubtractFeeAmount (SFFO) flag on the recipients objects for all forms of sending calls. According to the commit discussion, this flag was chiefly introduced to permit sweeping without manually calculating the fees of transactions. However, the flag leads to unintuitive behavior and makes it more complicated to test many wallet RPCs exhaustively. We proposed to introduce a dedicated `sendall` RPC with the intention to cover this functionality. Since the proposal, it was discovered in further discussion that our proposed `sendall` rpc and SFFO have subtly different scopes of operation. • sendall: Use _given UTXOs_ to pay a destination the remainder after fees. • SFFO: Use a _given budget_ to pay an address the remainder after fees. While `sendall` will simplify cases of spending a given set of UTXOs such as paying the value from one or more specific UTXOs, emptying a wallet, or burning dust, we realized that there are some cases in which SFFO is used to pay other parties from a limited budget, which can often lead to the creation of change outputs. This cannot be easily replicated using `sendall` as it would require manual computation of the appropriate change amount. As such, sendall cannot replace all uses of SFFO, but it still has a different use case and will aid in simplifying some wallet calls and numerous wallet tests. _Sendall call details_ The proposed sendall call builds a transaction from a specific subset of the wallet's UTXO pool (by default all of them) and assigns the funds to one or more receivers. Receivers can either be specified with a given amount or receive an equal share of the remaining unassigned funds. At least one recipient must be provided without assigned amount to collect the remainder. The `sendall` call will never create change. The call has a `send_max` option that changes the default behavior of spending all UTXOs ("no UTXO left behind"), to maximizing the output amount of the transaction by skipping uneconomic UTXOs. The `send_max` option is incompatible with providing a specific set of inputs. --- Edit: Replaced OP with latest commit message to reflect my updated motivation of the proposal. ACKs for top commit: achow101: re-ACK bb84b71 Tree-SHA512: 20aaf75d268cb4b144f5d6437d33ec7b5f989256b3daeeb768ae1e7f39dc6b962af8223c5cb42ecc72dc38cecd921c53c077bc0ec300b994e902412213dd2cc3
28ea4c7 test: simplify splitment with `sendall` in wallet_basic (brunoerg) 923d245 test: use `sendall` when emptying wallet (brunoerg) Pull request description: In some tests they have used `sendtoaddress` in order to empty a wallet. With the addition of `sendall`, it makes sense to use it for that. ACKs for top commit: achow101: ACK 28ea4c7 ishaanam: utACK 28ea4c7 w0xlt: ACK bitcoin@28ea4c7 Tree-SHA512: 903136d7df5c65d3c02310d5a84241c9fd11070f69d932b4e188b8ad45c38ab5bc1bd5a9242b3e52d2576665ead14be0a03971a9ad8c00431fed442eba4ca48f
5182940 RPC: fix sendall docs (Anthony Towns) Pull request description: Updates the documentation for the "inputs" entry in the "options" parameter of the sendall RPC to match the documentation for createrawtransaction. ACKs for top commit: achow101: ACK 5182940 Xekyo: ACK 5182940 Tree-SHA512: fe78e17b2f36190939b645d7f4653d025bbac110e4a7285b49e7f1da27adac8c4d03fd5b770e3a74351066b1ab87fde36fc796f42b03897e4e2ebef4b6b6081c
6f8e381 sendall: check if the maxtxfee has been exceeded (ishaanam) Pull request description: Previously the `sendall` RPC didn't check whether the fees of the transaction it creates exceed the set `maxtxfee`. This PR adds this check to `sendall` and a test case for it. ACKs for top commit: achow101: ACK 6f8e381 Xekyo: ACK 6f8e381 glozow: Concept ACK 6f8e381. The high feerate is unlikely but sendall should respect the existing wallet options. Tree-SHA512: 6ef0961937091293d49be16f17e4451cff3159d901c0c7c6e508883999dfe0c20ed4d7126bf74bfea8150d4c1eef961a45f0c28ef64562e6cb817fede2319f1a
…e check cc434cb wallet: fix sendall creates tx that fails tx-size check (kouloumos) Pull request description: Fixes bitcoin#26011 The `sendall` RPC doesn't use `CreateTransactionInternal` as the rest of the wallet RPCs. [This has already been discussed in the original PR](bitcoin#24118 (comment)). By not going through that path, it never checks the transaction's weight against the maximum tx weight for transactions we're willing to relay. https://github.com/bitcoin/bitcoin/blob/447f50e4aed9a8b1d80e1891cda85801aeb80b4e/src/wallet/spend.cpp#L1013-L1018 This PR adds a check for tx-size as well as test coverage for that case. _Note: It seems that the test takes a bit of time on slower machines, I'm not sure if dropping it might be for the better._ ACKs for top commit: glozow: re ACK cc434cb via range-diff. Changes were addressing bitcoin#26024 (comment) and bitcoin#26024 (comment). achow101: ACK cc434cb w0xlt: reACK bitcoin@cc434cb Tree-SHA512: 64a1d8f2c737b39f3ccee90689eda1dd9c1b65f11b2c7bc0ec8bfe72f0202ce90ab4033bb0ecfc6080af8c947575059588a00938fe48e7fd553f7fb5ee03b3cc
…pecified inputs 315fd4d test: Test for out of bounds vout in sendall (Andrew Chow) b132c85 wallet: Check utxo prevout index out of bounds in sendall (Andrew Chow) 708b72b test: Test that sendall works with watchonly spending specific utxos (Andrew Chow) 6bcd7e2 wallet: Correctly check ismine for sendall (Andrew Chow) Pull request description: The `sendall` RPC would previously fail when used with a watchonly wallet and specified inputs. This failure was caused by checking isminetype equality with ISMINE_ALL rather than a bitwise AND as IsMine can never return ISMINE_ALL. Also added a test. ACKs for top commit: w0xlt: ACK bitcoin@315fd4d furszy: ACK 315fd4d Tree-SHA512: fb55cf6524e789964770b803f401027319f0351433ea084ffa7c5e6f1797567a608c956b7f7c5bd542aa172c4b7b38b07d0976f5ec587569efead27266e8664c
cb44c59 test: Add sendall test for min-fee setting (Aurèle Oulès) Pull request description: While experimenting with mutation testing it appeared that the minimum fee-rate check was not tested for the `sendall` RPC. https://bcm-ui.aureleoules.com/mutations/3581479318544ea6b97f788cec6e6ef1 ACKs for top commit: 0xB10C: ACK cb44c59 ishaanam: ACK cb44c59 stickies-v: re-ACK [cb44c59](bitcoin@cb44c59) Tree-SHA512: 31978436e1f01cc6abf44addc62b6887e65611e9a7ae7dc72e6a73cdfdb3a6a4f0a6c53043b47ecd1b10fc902385a172921e68818a7f5061c96e5e1ef5280b48
…l tests by 18-35% via `keypoolrefill()` BACKPORT NOTE: we have missing changes for wallet_fundrawtransaction.py due to DNM bitcoin#20536 31fdc54 test: speed up wallet_fundrawtransaction.py and wallet_sendall.py (kdmukai) Pull request description: ## Problem `wallet_fundrawtransaction.py` and `wallet_sendall.py` are the two slowest functional tests *when running without a RAM disk*. ``` # M1 MacBook Pro timings wallet_fundrawtransaction.py --descriptors | ✓ Passed | 55 s wallet_fundrawtransaction.py --legacy-wallet | ✓ Passed | 381 s wallet_sendall.py --descriptors | ✓ Passed | 43 s wallet_sendall.py --legacy-wallet | ✓ Passed | 327 s ``` In each case, the majority of the time is spent iterating through 1500 to 1600 `getnewaddress()` calls. This is particularly slow in the `--legacy-wallet` runs. see: https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_fundrawtransaction.py#L986-L987 see: https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_sendall.py#L324 ## Solution Pre-fill the keypool before iterating through those `getnewaddress()` calls. With this change, the execution time drops to: ``` wallet_fundrawtransaction.py --descriptors | ✓ Passed | 52 s # -3s diff wallet_fundrawtransaction.py --legacy-wallet | ✓ Passed | 291 s # -90s diff wallet_sendall.py --descriptors | ✓ Passed | 27 s # -16s diff wallet_sendall.py --legacy-wallet | ✓ Passed | 228 s # -99s diff ``` --- Tagging @ Sjors as he had encouraged me to take a look at speeding up the tests. ACKs for top commit: achow101: ACK 31fdc54 Tree-SHA512: e8dd89323551779832a407d068977c827c09dff55c1079d3c19aab39fcce6957df22b1da797ed7aa3bc2f6dd22fdf9e6f5e1a9a0200fdb16ed6042fc5f6dd992
thepastaclaw
approved these changes
Mar 24, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Re-reviewed after the Mar 24 force push addressing UdjinM6 feedback. All prior thepastaclaw review comments (bitcoin→Dash address text on lines 1031/1034) have been addressed.
Backport verification — reviewed all 8 cherry-picks against their upstream Bitcoin counterparts:
bitcoin#24118(sendall core): All Dash adaptations correct —SIGHASH_ALL(no taproot),/Bnot/vB(no vsize),DUFF_BnotSAT_VB,SEQUENCE_FINAL(no RBF),MAX_STANDARD_TX_SIZE(not weight),AvailableCoinsAPI matches Dash's interface,ConstructTransactionwithout RBF param.bitcoin#25955(test: use sendall): Clean adaptation.bitcoin#26010(fix docs): Clean.bitcoin#26084(maxtxfee check): Clean.bitcoin#26024(tx-size check): Correctly usesMAX_STANDARD_TX_SIZEnotMAX_STANDARD_TX_WEIGHT.bitcoin#26344(watchonly fix): Clean.bitcoin#26622(min-fee test): Fee rate error message correctly referencesduff/B.bitcoin#26722(partial — test speedup): Partial merge noted in commit message, wallet_fundrawtransaction.py excluded due to DNM — correct.
Dust threshold: Test amounts correctly adapted (700/600 duffs vs Bitcoin's 400/300 sats).
Known upstream issues retained (matching upstream, not in scope):
assert_tx_has_outputoperator precedence (and/or without parens) — matches upstreamlock_unspentsnot cleaned up onFinishTransactionfailure — matches upstreamDecimal(0.00000010)float precision — matches upstream
utACK 7710093
UdjinM6
approved these changes
Mar 24, 2026
Review GateCommit:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
This PR includes implementation of
sendallRPC.What was done?
sendallwhen emptying wallet bitcoin/bitcoin#25955keypoolrefill()bitcoin/bitcoin#26722Even bitcoin#25375 does not fully fit Dash's conception of instant-send, because specifying "minimum confirmation" is not really relevant since Dash Core have chainlocks and instant-send, but it maybe useful for sack of sync with Bitcoin Core's RPC interfaces. Though, it's skipped for this PR as out-of-scope at the moment.
How Has This Been Tested?
Run unit & functional tests
Breaking Changes
N/A
Checklist: