wallet, rpc: add anti-fee-sniping to `send` and `sendall` #28944

ishaanam · 2023-11-26T22:43:31Z

Currently, send and sendall don't do anti-fee-sniping because they don't use CreateTransaction. This PR adds anti-fee-sniping to these RPCs by calling DiscourageFeeSniping from FinishTransaction when the user does not specify a locktime.

DrahtBot · 2023-11-26T22:43:34Z

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/28944.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	S3RK, Sjors
Stale ACK	achow101, murchandamus

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32857 (wallet: allow skipping script paths by Sjors)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

In spend.h doc comment: missing closing parenthesis
Original: “…unless we are not synced with the current chain”
Should be: “…unless we are not synced with the current chain)” [closes the parenthetical]

^{drahtbot_id_4_m}

S3RK · 2023-11-27T08:43:45Z

Concept ACK

achow101 · 2023-11-28T15:24:35Z

ACK 5c9ac14

A test for send would be nice as well.

ishaanam · 2023-11-29T17:05:16Z

A test for send would be nice as well.

Done

test/functional/wallet_send.py

test/functional/wallet_sendall.py

test/functional/wallet_send.py

src/wallet/spend.h

achow101

ACK f348ead

test/functional/wallet_send.py

murchandamus

Concept ACK and almost crACK d76f880 except that there seems to be a type mix-up:

 wallet/rpc/spend.cpp:1299:20: error: no matching function for call to 'FinishTransaction'
            return FinishTransaction(pwallet, options, CMutableTransaction(*txr.tx));
                   ^~~~~~~~~~~~~~~~~
wallet/rpc/spend.cpp:79:17: note: candidate function not viable: expects an lvalue for 3rd argument
static UniValue FinishTransaction(const std::shared_ptr<CWallet> pwallet, const UniValue& options, CMutableTransaction& rawTx)
                ^
1 error generated.

ishaanam · 2023-12-27T20:19:55Z

Fixed silent merge conflict.

murchandamus

ACK c8de459

ishaanam · 2024-06-17T21:24:05Z

--- a/test/functional/wallet_import_rescan.py
+++ b/test/functional/wallet_import_rescan.py
@@ -293,6 +293,7 @@ class ImportRescanTest(BitcoinTestFramework):
                 add_to_wallet=False,
                 inputs=[unspent_txid_map[variant.initial_txid]],
                 outputs=[{ADDRESS_BCRT1_UNSPENDABLE : variant.initial_amount}],
+                locktime=0,
                 subtract_fee_from_outputs=[0]
             )
             variant.child_txid = child["txid"]

Done

It seems that you may have been working on an update, but it might not have been pushed. Is that possible?

I pushed this change, but I had forgotten to comment until later.

S3RK · 2024-06-19T07:10:43Z

src/wallet/rpc/spend.cpp

+    }
+
+    if (can_anti_fee_snipe) {
+        LOCK(pwallet->cs_wallet);


I'm not clear why do we need a wallet lock here?

Here the wallet is locked because it runs CWallet::GetLastBlockHash and CWallet::GetLastBlockHeight which both require pwallet->cs_wallet to be locked.

…niping is compatible with transaction Currently DiscourageFeeSniping will assert unless sequences are all either MAX_SEQUENCE_NONFINAL or MAX_BIP125_RBF_SEQUENCE. Github-Pull: bitcoin#28944 Rebased-From: 6825ae0

…sequences is final Github-Pull: bitcoin#28944 Rebased-From: b11d00d

murchandamus

crACK b11d00d

src/wallet/rpc/spend.cpp

murchandamus · 2024-08-05T18:34:57Z

src/wallet/rpc/spend.cpp

+            CMutableTransaction tx = CMutableTransaction(*txr.tx);
+            return FinishTransaction(pwallet, options, tx);


It’s not clear to me why this change was made. It does not seem to be affecting function, is this just for readability?

Because FinishTransaction now takes a CMutableTransaction& instead of a const CMutableTransaction&, we can't give it an rvalue.

DrahtBot · 2025-03-17T19:54:18Z

Needs rebase, if still relevant.

murchandamus · 2025-06-20T19:36:55Z

Hey @ishaanam, I noticed that this PR was rebased, but it is still set to Draft. Should this be marked as ready for review?

Sjors · 2025-07-09T07:18:27Z

Concept ACK. Discovered this after I tried to implement it myself in #32892 :-)

I might be useful to borrow the idea from 642e4b6 of renaming DiscourageFeeSniping to MaybeDiscourageFeeSniping. Right now the code that decides whether to do this is duplicated between FinishTransaction and CreateTransactionInternal. Both places look at coin_control.m_locktime and loop through the inputs, but implemented in slightly different ways.

In wallet_send.py it would be useful to check that a generated PSBT has the correct lock time (you could use either set psbt or use a watch-only wallet which always produces a PSBT).

DrahtBot added the CI failed label Nov 26, 2023

ishaanam force-pushed the sendall_anti_fee_sniping branch from b8e489a to 5c9ac14 Compare November 27, 2023 00:07

DrahtBot removed the CI failed label Nov 27, 2023

This was referenced Nov 27, 2023

wallet: do not backdate locktime if it may lead to fingerprinting #26902

Closed

wallet: BIP 326 sequence based anti-fee-snipe for taproot inputs #24128

Closed

DrahtBot requested a review from S3RK November 28, 2023 15:24

ishaanam force-pushed the sendall_anti_fee_sniping branch from 5c9ac14 to 5101f05 Compare November 29, 2023 15:12

DrahtBot mentioned this pull request Dec 1, 2023

wallet, rpc: document and update sendall behavior around unconfirmed inputs #28979

Merged

achow101 reviewed Dec 4, 2023

View reviewed changes

test/functional/wallet_send.py Outdated Show resolved Hide resolved

test/functional/wallet_sendall.py Outdated Show resolved Hide resolved

test/functional/wallet_send.py Outdated Show resolved Hide resolved

ishaanam force-pushed the sendall_anti_fee_sniping branch from 5101f05 to f348ead Compare December 4, 2023 23:41

luke-jr reviewed Dec 6, 2023

View reviewed changes

src/wallet/spend.h Show resolved Hide resolved

achow101 reviewed Dec 7, 2023

View reviewed changes

test/functional/wallet_send.py Outdated Show resolved Hide resolved

ishaanam force-pushed the sendall_anti_fee_sniping branch from f348ead to a5ef4e2 Compare December 9, 2023 00:02

DrahtBot added the CI failed label Dec 11, 2023

ishaanam force-pushed the sendall_anti_fee_sniping branch from a5ef4e2 to d76f880 Compare December 22, 2023 04:24

DrahtBot mentioned this pull request Dec 22, 2023

wallet, rpc: FundTransaction refactor #28560

Merged

murchandamus reviewed Dec 27, 2023

View reviewed changes

ishaanam force-pushed the sendall_anti_fee_sniping branch from d76f880 to c8de459 Compare December 27, 2023 20:19

DrahtBot removed the CI failed label Dec 27, 2023

murchandamus reviewed Dec 27, 2023

View reviewed changes

DrahtBot requested a review from achow101 December 27, 2023 21:06

DrahtBot mentioned this pull request Jan 16, 2024

Silent Payments: sending #28201

Draft

DrahtBot added the CI failed label Jan 16, 2024

DrahtBot added the Needs rebase label Jan 23, 2024

ishaanam force-pushed the sendall_anti_fee_sniping branch from c8de459 to ed5cc12 Compare January 25, 2024 01:53

S3RK reviewed Jun 19, 2024

View reviewed changes

ishaanam force-pushed the sendall_anti_fee_sniping branch from fa1fa35 to b11d00d Compare June 27, 2024 19:06

DrahtBot removed the Needs rebase label Jun 27, 2024

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Aug 1, 2024

test: test sendall does not do anti-fee-sniping when even one of the …

0950331

…sequences is final Github-Pull: bitcoin#28944 Rebased-From: b11d00d

murchandamus reviewed Aug 5, 2024

View reviewed changes

DrahtBot requested review from S3RK and achow101 August 5, 2024 18:48

DrahtBot added CI failed and removed CI failed labels Sep 11, 2024

DrahtBot added CI failed and removed CI failed labels Oct 24, 2024

DrahtBot mentioned this pull request Nov 7, 2024

wallet: Disable creating and loading legacy wallets #31250

Merged

DrahtBot added the CI failed label Mar 13, 2025

DrahtBot marked this pull request as draft April 4, 2025 06:02

DrahtBot added the Needs rebase label Apr 25, 2025

DrahtBot mentioned this pull request Apr 25, 2025

test: Increase stack size for "Debug" builds with MSVC #32349

Closed

ishaanam force-pushed the sendall_anti_fee_sniping branch from b11d00d to 6f30f22 Compare June 11, 2025 15:51

DrahtBot removed Needs rebase CI failed labels Jun 11, 2025

ishaanam added 2 commits June 25, 2025 11:55

wallet, rpc: add anti-fee-sniping to send and sendall

20802c7

test: test sendall and send do anti-fee-sniping

aac0b6d

ishaanam force-pushed the sendall_anti_fee_sniping branch from 6f30f22 to aac0b6d Compare June 25, 2025 15:57

ishaanam marked this pull request as ready for review June 25, 2025 17:38

This was referenced Jul 2, 2025

wallet: allow skipping script paths #32857

Draft

rpc: use anti-fee-sniping in send, sendall and walletcreatefundedpsbt #32892

Closed

		CMutableTransaction tx = CMutableTransaction(*txr.tx);
		return FinishTransaction(pwallet, options, tx);

wallet, rpc: add anti-fee-sniping to send and sendall #28944

Are you sure you want to change the base?

wallet, rpc: add anti-fee-sniping to send and sendall #28944

Conversation

ishaanam commented Nov 26, 2023

Uh oh!

DrahtBot commented Nov 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)

Uh oh!

S3RK commented Nov 27, 2023

Uh oh!

achow101 commented Nov 28, 2023

Uh oh!

ishaanam commented Nov 29, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

murchandamus left a comment

Choose a reason for hiding this comment

Uh oh!

ishaanam commented Dec 27, 2023

Uh oh!

murchandamus left a comment

Choose a reason for hiding this comment

Uh oh!

ishaanam commented Jun 17, 2024

Uh oh!

S3RK Jun 19, 2024

Choose a reason for hiding this comment

Uh oh!

ishaanam Jun 27, 2024

Choose a reason for hiding this comment

Uh oh!

murchandamus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

murchandamus Aug 5, 2024

Choose a reason for hiding this comment

Uh oh!

ishaanam Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Mar 17, 2025

Uh oh!

murchandamus commented Jun 20, 2025

Uh oh!

Sjors commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

wallet, rpc: add anti-fee-sniping to `send` and `sendall` #28944

wallet, rpc: add anti-fee-sniping to `send` and `sendall` #28944

DrahtBot commented Nov 26, 2023 •

edited

Loading

Sjors commented Jul 9, 2025 •

edited

Loading