wallet: allow anti-fee-sniping in sendall RPC while not relying on RBF default #35404

pull rkrux wants to merge 2 commits into bitcoin:master from rkrux:antifeesniping changing 2 files +4 −4

rkrux commented at 2:15 PM on May 28, 2026: contributor

Partially fixes #32661. Prerequisite to #35405.

The test change in the second commit would fail without the presence of the first commit.
DrahtBot added the label Wallet on May 28, 2026
DrahtBot commented at 2:15 PM on May 28, 2026: contributor


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35404.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK maflcko, polespinasa

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #35405 (wallet: switch default optinrbf from true to false by rkrux)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
rkrux renamed this:
~~wallet: allow anti-fee-sniping in sendall RPC while not relying of RBF default~~
wallet: allow anti-fee-sniping in sendall RPC while not relying on RBF default
on May 28, 2026
rkrux commented at 2:57 PM on May 28, 2026: contributor

https://github.com/bitcoin/bitcoin/actions/runs/26580267905/job/78312103346?pr=35404#step:11:17035

test_framework.util.JSONRPCException: {'code': -8, 'message': 'PSBTs not compatible (different transactions)'} [http_status=200]

These failing jobs contain the same error that happened in #35381 and this commit a2a2b1745f0818364dd8149161e88cea1475d9b6 fixes it.

So, this PR is dependent on #35381, I'll update the PR description.
rkrux marked this as a draft on May 28, 2026
DrahtBot added the label CI failed on May 28, 2026
achow101 referenced this in commit d0a54dd8e0 on May 28, 2026
maflcko added the label Needs rebase on May 29, 2026
DrahtBot removed the label Needs rebase on May 29, 2026

Type	Reviewers
ACK	maflcko, polespinasa

wallet: allow anti-fee-sniping in sendall RPC while not relying on RBF default

In case locktime (and replaceable) not being specified in this RPC request,
the wallet sets the transaction replaceable due to the default value of opt-in
RBF set in the wallet.

This allowed the anti-fee-sniping flow to be executed but in case of
replaceable being set false in the request, anti-fee-sniping flow would be
missed.

This patch fixes it.

8877eec726

rkrux force-pushed on May 29, 2026
rkrux commented at 9:46 AM on May 29, 2026: contributor

Rebased over master to incorporate changes from #35381.
rkrux marked this as ready for review on May 29, 2026

in src/wallet/rpc/spend.cpp:1501 in f1cd91557f outdated

1497 | @@ -1498,7 +1498,7 @@ RPCMethod sendall()
1498 |                      if (output.depth == 0 && coin_control.m_version == TRUC_VERSION) {
1499 |                          coin_control.m_max_tx_weight = TRUC_CHILD_MAX_WEIGHT;
1500 |                      }
1501 | -                    CTxIn input(output.outpoint.hash, output.outpoint.n, CScript(), rbf ? MAX_BIP125_RBF_SEQUENCE : CTxIn::SEQUENCE_FINAL);
1502 | +                    CTxIn input(output.outpoint.hash, output.outpoint.n, CScript(), rbf ? MAX_BIP125_RBF_SEQUENCE : CTxIn::MAX_SEQUENCE_NONFINAL);

maflcko commented at 10:06 AM on May 29, 2026:

I don't understand how the prior code could have even worked? The docs say RPCArg::DefaultHint{"locktime close to block height to prevent fee sniping"} However, anti-fee-sniping has an assert:

src/wallet/spend.cpp:        assert(in.nSequence != CTxIn::SEQUENCE_FINAL);

So either:

The prior code was untested and will lead to an abort when called?
The new code is still wrong and doesn't use anti-fee-sniping?
The old code was setting a unique fingerprint on sendall transactions?
none/all of the above?

What am I missing?

rkrux commented at 10:18 AM on May 29, 2026:

I hadn't checked this assertion until now but it seems correct to be present there.

The earlier code was using MAX_BIP125_RBF_SEQUENCE as the nSequence because the test didn't pass replaceable as false and the default optin rbf was being used.

The new code in the test passes replaceable as false so that the second portion of the ternary condition is triggered now in which MAX_SEQUENCE_NONFINAL is used instead of SEQUENCE_FINAL.

That's why the second commit fails without the first commit.

maflcko commented at 12:14 PM on May 29, 2026:

Ah, if any input is SEQUENCE_FINAL, can_anti_fee_snipe becomes false and DiscourageFeeSniping() is never called.

I guess I was a bit confused by the test, because it is duplicating the helper and bloating the diff/code.

Why not just a simple diff:

diff --git a/test/functional/wallet_sendall.py b/test/functional/wallet_sendall.py
index 0524b5c82e..df22c920fc 100755
--- a/test/functional/wallet_sendall.py
+++ b/test/functional/wallet_sendall.py
@@ -61,4 +61,4 @@ class SendallTest(BitcoinTestFramework):
     # Helper schema for success cases
-    def test_sendall_success(self, sendall_args, remaining_balance = 0):
-        sendall_tx_receipt = self.wallet.sendall(sendall_args)
+    def test_sendall_success(self, sendall_args, remaining_balance = 0, *, options=None):
+        sendall_tx_receipt = self.wallet.sendall(sendall_args, options=options)
         self.generate(self.nodes[0], 1)
@@ -438,3 +438,3 @@ class SendallTest(BitcoinTestFramework):
         self.add_utxos([10,11])
-        tx_from_wallet = self.test_sendall_success(sendall_args = [self.remainder_target])
+        tx_from_wallet = self.test_sendall_success(sendall_args = [self.remainder_target],options={"replaceable":False})

rkrux commented at 1:08 PM on May 29, 2026:

Yes, it's simpler, added.

DrahtBot removed the label CI failed on May 29, 2026

wallet, test: fix sendall anti-fee-sniping when locktime is not specified

This particular test case only needs to ensure that locktime is not
specified in the RPC request, it doesn't need to rely on the wallet optin
RBF default that causes the test to pass coincidentally.

Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>

9c1fcaca5c

rkrux force-pushed on May 29, 2026

maflcko commented at 1:10 PM on May 29, 2026: member

review ACK 9c1fcaca5cb8481b2a46f42e0f43888311b97ef2 🍅

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 9c1fcaca5cb8481b2a46f42e0f43888311b97ef2 🍅
3ujWhAAEp/LuuNg9BFLB2nveMad8i9ra1rmRtbQKANQMBqK0RpOcw4fbAncGSBHRHdLsdokR/1bvmBGdJUpVDQ==

</details>

DrahtBot added the label CI failed on May 29, 2026
polespinasa approved
polespinasa commented at 10:23 AM on May 31, 2026: member

code review ACK 9c1fcaca5cb8481b2a46f42e0f43888311b97ef2

Nice catch, the PR corrects a bug that made sendall to not apply anti-fee sniping if rbf=false because of the user specifying it or because of wallet defaults.
polespinasa commented at 10:31 AM on May 31, 2026: member

Is the CI fail error identifies? Looks like feature_dbcrash.py fails because of gettxoutsetinfo which should be unrelated to this PR.

Contributors

Labels

Linked (view graph)

#32661 wallet, node: Redundant opt-in RBF wallet config and RPC parameters #35381 wallet, test: optinrbf deprecation followups #35405 wallet: switch default optinrbf from true to false

wallet: allow anti-fee-sniping in sendall RPC while not relying on RBF default #35404

Code Coverage & Benchmarks

Reviews

Conflicts