backport: Merge bitcoin/bitcoin#26733 by cryptotura · Pull Request #7119 · dashpay/dash

cryptotura · 2026-01-29T10:53:01Z

backporting

…tractfeefrom` parameter 057057a Add test for `sendmany` rpc that uses `subtractfeefrom` parameter (Yusuf Sahin HAMZA) Pull request description: This PR adds test that uses `sendmany` rpc to send **BTC** to multiple addresses using `subtractfeefrom` parameter, then checks receiver addresses balances to make sure fees are subtracted correctly. ACKs for top commit: achow101: ACK 057057a Tree-SHA512: 51167120d489f0ff7b8b9855424d07cb55a8965984f904643cddf45e7a08c350eaded498c350ec9c660edf72c2f128ec142347c9c79d5043d9f6cd481b15cd7e

github-actions · 2026-01-29T10:53:34Z

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

coderabbitai · 2026-01-29T10:55:32Z

Walkthrough

The changes add two new test scenarios to the wallet functionality tests. The first scenario tests the sendmany operation with an explicit fee_rate parameter, where transaction fees are subtracted from multiple recipients. It creates addresses, sends DASH to them with fee subtraction enabled, advances the network, and verifies the received amounts reflect the fee deductions. The second scenario introduces a parallel test path that exercises explicit fee_rate specification alongside existing fee-rate tests, validating outcomes for sendtoaddress and sendmany operations.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	❓ Inconclusive	The description 'backporting' is vague and generic, using a non-descriptive term that doesn't convey meaningful information about what is being backported or why.	Expand the description to include what feature or test is being backported and its purpose, e.g., 'Backport Bitcoin PR `#26733`: Add test for sendmany with subtractfeefrom parameter to verify fee deduction from multiple recipients.'

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'backport: Merge bitcoin#26733' accurately describes the main change—backporting a specific Bitcoin commit into the Dash repository.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

thepastaclaw · 2026-03-30T14:47:48Z

🕓 Review not started yet because this PR is a draft.

Request normal review — click when the PR is ready for review.
Request priority review — click to move this review to the front of the queue.

Commit 5e32a3e. Normal review starts when eligible; priority review starts as soon as a slot is available.

thepastaclaw

Code Review

Clean backport of bitcoin#26733 — adds a test for sendmany RPC with subtractfeefrom parameter. The test sends 5 DASH to two addresses with fee subtracted from both, then verifies balances. No behavioral changes, test-only addition. The adaptation from BTC to DASH naming (sat/vB → duff/B in the log message that follows) is consistent with existing conventions.

Reviewed commit: 5e32a3e

thepastaclaw

Code Review

Small test backport of bitcoin#26733. One blocking issue: the exact-equality fee-split assertion does not match Dash wallet behavior (first SFFO recipient absorbs the remainder when fee is not evenly divisible). A follow-up adaptation commit already exists on a parallel branch (9b8ed10) confirming this failure mode. One merge-resolution nitpick: the cherry-pick produced two near-duplicate log headers around the new SFFO block.

Reviewed commit: 5e32a3e

🔴 1 blocking | 💬 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 `test/functional/wallet_basic.py`:
- [BLOCKING] lines 290-292: Exact-equality SFFO assertion does not match Dash fee-splitting (test will fail spuriously on odd fees)
  These assertions require both recipients to receive exactly `5 + tx['fee'] / 2`, but `src/wallet/spend.cpp:1024-1029` divides the fee with integer arithmetic and makes the *first* SFFO recipient pay the remainder (`to_reduce % outputs_to_subtract_fee_from`). With two outputs and an odd `to_reduce`, the two recipients differ by one duff and at least one of these `assert_equal`s will fail for a perfectly valid transaction. This is not theoretical: a Dash-side follow-up commit (`9b8ed10e40` "fix: adapt sendmany subtractfeefrom test for Dash fee precision") already exists on a parallel branch reworking exactly these three lines into a sum-and-tolerance check, confirming the upstream assertion was observed to fail in Dash CI. The same idiom is used in `rpc_fundrawtransaction.py`, which already encodes the tolerance invariant. The backport should fold that adaptation in here rather than landing the upstream form unchanged.

thepastaclaw · 2026-05-09T10:01:13Z

+        expected_bal = Decimal('5') + (tx['fee'] / 2)
+        assert_equal(self.nodes[0].getreceivedbyaddress(a0), expected_bal)
+        assert_equal(self.nodes[0].getreceivedbyaddress(a1), expected_bal)


🔴 Blocking: Exact-equality SFFO assertion does not match Dash fee-splitting (test will fail spuriously on odd fees)

These assertions require both recipients to receive exactly 5 + tx['fee'] / 2, but src/wallet/spend.cpp:1024-1029 divides the fee with integer arithmetic and makes the first SFFO recipient pay the remainder (to_reduce % outputs_to_subtract_fee_from). With two outputs and an odd to_reduce, the two recipients differ by one duff and at least one of these assert_equals will fail for a perfectly valid transaction. This is not theoretical: a Dash-side follow-up commit (9b8ed10e40 "fix: adapt sendmany subtractfeefrom test for Dash fee precision") already exists on a parallel branch reworking exactly these three lines into a sum-and-tolerance check, confirming the upstream assertion was observed to fail in Dash CI. The same idiom is used in rpc_fundrawtransaction.py, which already encodes the tolerance invariant. The backport should fold that adaptation in here rather than landing the upstream form unchanged.

💡 Suggested change

Suggested change

expected_bal = Decimal('5') + (tx['fee'] / 2)

assert_equal(self.nodes[0].getreceivedbyaddress(a0), expected_bal)

assert_equal(self.nodes[0].getreceivedbyaddress(a1), expected_bal)

a0_received = self.nodes[0].getreceivedbyaddress(a0)

a1_received = self.nodes[0].getreceivedbyaddress(a1)

# Fee subtraction is rounded to whole duffs, so the split across equal

# recipients can differ by one duff.

assert_equal(a0_received + a1_received, Decimal('10') + tx['fee'])

assert_greater_than(Decimal('5'), a0_received)

assert_greater_than(Decimal('5'), a1_received)

assert_greater_than(Decimal('0.00000002'), abs(a0_received - a1_received))

_{source: ['codex']}

🤖 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 `test/functional/wallet_basic.py`: - [BLOCKING] lines 290-292: Exact-equality SFFO assertion does not match Dash fee-splitting (test will fail spuriously on odd fees) These assertions require both recipients to receive exactly `5 + tx['fee'] / 2`, but `src/wallet/spend.cpp:1024-1029` divides the fee with integer arithmetic and makes the *first* SFFO recipient pay the remainder (`to_reduce % outputs_to_subtract_fee_from`). With two outputs and an odd `to_reduce`, the two recipients differ by one duff and at least one of these `assert_equal`s will fail for a perfectly valid transaction. This is not theoretical: a Dash-side follow-up commit (`9b8ed10e40` "fix: adapt sendmany subtractfeefrom test for Dash fee precision") already exists on a parallel branch reworking exactly these three lines into a sum-and-tolerance check, confirming the upstream assertion was observed to fail in Dash CI. The same idiom is used in `rpc_fundrawtransaction.py`, which already encodes the tolerance invariant. The backport should fold that adaptation in here rather than landing the upstream form unchanged.

thepastaclaw · 2026-05-09T10:01:13Z

+        assert_equal(self.nodes[0].getreceivedbyaddress(a0), expected_bal)
+        assert_equal(self.nodes[0].getreceivedbyaddress(a1), expected_bal)
+
+        self.log.info("Test sendmany with fee_rate param (explicit fee rate in sat/vB)")


💬 Nitpick: Duplicate/inconsistent log header introduced by the merge resolution

Upstream bitcoin#26733 inserted the new SFFO block immediately above its own existing log line "...explicit fee rate in sat/vB". Dash had previously renamed that line to duff/B at line 279. The merge resolution kept the renamed duff/B line at 279 and re-added the upstream sat/vB line at 294, leaving two near-duplicate headers bracketing the SFFO block. Two consequences: (a) line 294 uses upstream sat/vB terminology, inconsistent with every other fee-rate string in this file (lines 322, 327, 334, 509, 540, 545, 552 all say duff/B); (b) the pre-existing line-279 header now sits above the SFFO block instead of the fee_rate block it was written to describe, so it mislabels what runs next. Simplest fix: drop the re-added line 294 entirely so the existing duff/B line at 279 once again prefaces the fee_rate test block, and add a dedicated self.log.info("Test sendmany with subtractfeefrom") for the new block.

_{source: ['claude']}

Suggested change

self.log.info("Test sendmany with fee_rate param (explicit fee rate in sat/vB)")

+ self.log.info("Test sendmany with fee_rate param (explicit fee rate in duff/B)")

- self.log.info("Test sendmany with fee_rate param (explicit fee rate in sat/vB)")

knst

comment has been de-dashified, need to revert this change

knst · 2026-07-23T15:32:22Z

+        assert_equal(self.nodes[0].getreceivedbyaddress(a0), expected_bal)
+        assert_equal(self.nodes[0].getreceivedbyaddress(a1), expected_bal)
+
+        self.log.info("Test sendmany with fee_rate param (explicit fee rate in sat/vB)")


Suggested change

self.log.info("Test sendmany with fee_rate param (explicit fee rate in sat/vB)")

+ self.log.info("Test sendmany with fee_rate param (explicit fee rate in duff/B)")

- self.log.info("Test sendmany with fee_rate param (explicit fee rate in sat/vB)")

knst · 2026-07-23T15:32:56Z

        node_0_bal = self.check_fee_amount(self.nodes[0].getbalance(), node_0_bal + Decimal('100'), fee_per_byte, count_bytes(self.nodes[2].gettransaction(txid)['hex']))

        self.log.info("Test sendmany with fee_rate param (explicit fee rate in duff/B)")
+        # Sendmany 5 DASH to two addresses with subtracting fee from both addresses


- self.log.info("Test sendmany with fee_rate param (explicit fee rate in duff/B)")

knst · 2026-10-02T20:22:31Z

Converted to draft: multiple comments with changes-requests + CI failures.

github-actions Bot mentioned this pull request Feb 26, 2026

backport: Merge bitcoin/bitcoin#26733 #7150

Closed

thepastaclaw reviewed Mar 31, 2026

View reviewed changes

thepastaclaw suggested changes May 9, 2026

View reviewed changes

knst reviewed Jul 23, 2026

View reviewed changes

PastaPastaPasta requested review from PastaPastaPasta and UdjinM6 August 29, 2026 13:17

knst marked this pull request as draft October 2, 2026 20:22

Navigation Menu

backport: Merge bitcoin/bitcoin#26733 - #7119

backport: Merge bitcoin/bitcoin#26733#7119
cryptotura wants to merge 1 commit into
dashpay:developfrom
CryptoTuraTeam:Feb

cryptotura commented Jan 29, 2026

Uh oh!

github-actions Bot commented Jan 29, 2026

Uh oh!

coderabbitai Bot commented Jan 29, 2026

Uh oh!

thepastaclaw commented Mar 30, 2026 •

edited

Loading

Uh oh!

thepastaclaw left a comment

Uh oh!

thepastaclaw left a comment

Uh oh!

thepastaclaw May 9, 2026

Uh oh!

thepastaclaw May 9, 2026

Uh oh!

knst Jul 23, 2026

Uh oh!

knst left a comment

Uh oh!

knst Jul 23, 2026

Uh oh!

knst Jul 23, 2026

Uh oh!

knst commented Oct 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

-        expected_bal = Decimal('5') + (tx['fee'] / 2)
-        assert_equal(self.nodes[0].getreceivedbyaddress(a0), expected_bal)
-        assert_equal(self.nodes[0].getreceivedbyaddress(a1), expected_bal)
+        a0_received = self.nodes[0].getreceivedbyaddress(a0)
+        a1_received = self.nodes[0].getreceivedbyaddress(a1)
+        # Fee subtraction is rounded to whole duffs, so the split across equal
+        # recipients can differ by one duff.
+        assert_equal(a0_received + a1_received, Decimal('10') + tx['fee'])
+        assert_greater_than(Decimal('5'), a0_received)
+        assert_greater_than(Decimal('5'), a1_received)
+        assert_greater_than(Decimal('0.00000002'), abs(a0_received - a1_received))

	self.log.info("Test sendmany with fee_rate param (explicit fee rate in sat/vB)")
	+ self.log.info("Test sendmany with fee_rate param (explicit fee rate in duff/B)")
	- self.log.info("Test sendmany with fee_rate param (explicit fee rate in sat/vB)")

Conversation

cryptotura commented Jan 29, 2026

Uh oh!

github-actions Bot commented Jan 29, 2026

✅ No Merge Conflicts Detected

Uh oh!

coderabbitai Bot commented Jan 29, 2026

Walkthrough

Estimated code review effort

Uh oh!

thepastaclaw commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

thepastaclaw May 9, 2026

Choose a reason for hiding this comment

Uh oh!

thepastaclaw May 9, 2026

Choose a reason for hiding this comment

Uh oh!

knst Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

knst Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

knst Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

knst commented Oct 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thepastaclaw commented Mar 30, 2026 •

edited

Loading