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

pull ishaanam wants to merge 2 commits into bitcoin:master from ishaanam:sendall_anti_fee_sniping changing 6 files +55 −10
  1. ishaanam commented at 10:43 PM on November 26, 2023: contributor

    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.

  2. DrahtBot commented at 10:43 PM on November 26, 2023: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

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

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

    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]

    <sup>drahtbot_id_4_m</sup>

  3. DrahtBot added the label CI failed on Nov 26, 2023
  4. ishaanam force-pushed on Nov 27, 2023
  5. DrahtBot removed the label CI failed on Nov 27, 2023
  6. S3RK commented at 8:43 AM on November 27, 2023: contributor

    Concept ACK

  7. achow101 commented at 3:24 PM on November 28, 2023: member

    ACK 5c9ac1456b7b4512845270cd850e5c33b643f575

    A test for send would be nice as well.

  8. DrahtBot requested review from S3RK on Nov 28, 2023
  9. ishaanam force-pushed on Nov 29, 2023
  10. ishaanam commented at 5:05 PM on November 29, 2023: contributor

    A test for send would be nice as well.

    Done

  11. in test/functional/wallet_send.py:145 in 5101f054ce outdated 
     141 | @@ -142,7 +142,12 @@ def test_send(self, from_wallet, to_wallet=None, amount=None, data=None,
 142 |              return
 143 |  
 144 |          if locktime:
 145 | +            assert_equal(from_wallet.decoderawtransaction(from_wallet.gettransaction(res["txid"])["hex"])["locktime"], locktime)
    achow101 commented at 9:42 PM on December 4, 2023:

    In 5101f054ce435ac653a3f5e780fca066e2f25088 "test: test sendall and send do anti-fee-sniping"

    nit: gettransaction has a verbose argument that does the decoding so this doesn't need to call decoderawtransaction. Same below.

    ishaanam commented at 11:42 PM on December 4, 2023:

    Done

  12. in test/functional/wallet_sendall.py:392 in 5101f054ce outdated 
     387 | +
 388 | +        assert_greater_than(tx_from_wallet["decoded"]["locktime"], tx_from_wallet["blockheight"] - 100)
 389 | +
 390 | +        self.add_utxos([10,11])
 391 | +        txid = self.wallet.sendall(recipients=[self.remainder_target], options={"locktime":0})["txid"]
 392 | +        tx_from_wallet = self.wallet.decoderawtransaction(self.wallet.getrawtransaction(txid))
    achow101 commented at 9:43 PM on December 4, 2023:

    In 5101f054ce435ac653a3f5e780fca066e2f25088 "test: test sendall and send do anti-fee-sniping"

    nit: getrawtransaction has a verbosity option that will do the decoding, so no need to call decoderawtransaction here.

    ishaanam commented at 11:42 PM on December 4, 2023:

    Done

  13. in test/functional/wallet_send.py:150 in 5101f054ce outdated 
     141 | @@ -142,7 +142,12 @@ def test_send(self, from_wallet, to_wallet=None, amount=None, data=None,
 142 |              return
 143 |  
 144 |          if locktime:
 145 | +            assert_equal(from_wallet.decoderawtransaction(from_wallet.gettransaction(res["txid"])["hex"])["locktime"], locktime)
 146 |              return res
 147 | +        else:
 148 | +            if add_to_wallet:
 149 | +                decoded_tx = from_wallet.decoderawtransaction(from_wallet.gettransaction(res["txid"])["hex"])
 150 | +                assert_greater_than(decoded_tx["locktime"], 100)
    achow101 commented at 9:43 PM on December 4, 2023:

    In 5101f054ce435ac653a3f5e780fca066e2f25088 "test: test sendall and send do anti-fee-sniping"

    Shouldn't this check for getblockcount() - 100?

    ishaanam commented at 11:42 PM on December 4, 2023:

    Done

  14. ishaanam force-pushed on Dec 4, 2023
  15. in src/wallet/spend.h:225 in f348eadadd outdated 
     212 | @@ -213,6 +213,7 @@ struct CreatedTransactionResult
 213 |          : tx(_tx), fee(_fee), fee_calc(_fee_calc), change_pos(_change_pos) {}
 214 |  };
 215 |  
 216 | +void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng_fast, interfaces::Chain& chain, const uint256& block_hash, int block_height);
    luke-jr commented at 6:00 PM on December 6, 2023:

    nit: blank line under this; ideally docs above

    ishaanam commented at 4:09 AM on December 10, 2023:

    Done

  16. in test/functional/wallet_send.py:149 in f348eadadd outdated 
     141 | @@ -142,7 +142,12 @@ def test_send(self, from_wallet, to_wallet=None, amount=None, data=None,
 142 |              return
 143 |  
 144 |          if locktime:
 145 | +            assert_equal(from_wallet.gettransaction(txid=res["txid"], verbose=True)["decoded"]["locktime"], locktime)
 146 |              return res
 147 | +        else:
 148 | +            if add_to_wallet:
 149 | +                decoded_tx = from_wallet.decoderawtransaction(from_wallet.gettransaction(res["txid"])["hex"])
    achow101 commented at 9:56 PM on December 7, 2023:

    In f348eadadd6099e7763f04eb3d42cfd8bba33146 "test: test sendall and send do anti-fee-sniping"

    nit: verbose here too.

    ishaanam commented at 4:09 AM on December 10, 2023:

    Done

  17. achow101 commented at 9:56 PM on December 7, 2023: member

    ACK f348eadadd6099e7763f04eb3d42cfd8bba33146

  18. ishaanam force-pushed on Dec 9, 2023
  19. DrahtBot added the label CI failed on Dec 11, 2023
  20. ishaanam force-pushed on Dec 22, 2023
  21. murchandamus commented at 7:29 PM on December 27, 2023: contributor

    Concept ACK and almost crACK d76f88051524971def5137a07ce2940dce1c33a9 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.
  22. ishaanam force-pushed on Dec 27, 2023
  23. ishaanam commented at 8:19 PM on December 27, 2023: contributor

    Fixed silent merge conflict.

  24. DrahtBot removed the label CI failed on Dec 27, 2023
  25. murchandamus commented at 9:06 PM on December 27, 2023: contributor

    ACK c8de459de6d22e37164583f078747facaeae69c6

  26. DrahtBot requested review from achow101 on Dec 27, 2023
  27. DrahtBot added the label CI failed on Jan 16, 2024
  28. DrahtBot added the label Needs rebase on Jan 23, 2024
  29. ishaanam force-pushed on Jan 25, 2024
  30. DrahtBot removed the label Needs rebase on Jan 25, 2024
  31. ishaanam force-pushed on Jan 29, 2024
  32. luke-jr referenced this in commit 7c481a931c on Jan 30, 2024
  33. luke-jr referenced this in commit d036e53caa on Jan 30, 2024
  34. murchandamus commented at 8:04 PM on February 1, 2024: contributor

    I circled back to review, I think this has a merge conflict

  35. luke-jr changes_requested
  36. luke-jr commented at 5:38 PM on March 4, 2024: member 
    --- 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"]
  37. luke-jr referenced this in commit 4cf3a4a73b on Mar 5, 2024
  38. luke-jr referenced this in commit 2200dfd225 on Mar 5, 2024
  39. ishaanam force-pushed on Mar 6, 2024
  40. DrahtBot removed the label CI failed on Mar 6, 2024
  41. luke-jr referenced this in commit 13e36c822b on Mar 13, 2024
  42. luke-jr referenced this in commit a20609e70a on Mar 13, 2024
  43. in src/wallet/rpc/spend.cpp:97 in 8e5778a9fb outdated 
      93 |  {
  94 | +    bool can_anti_fee_snipe = !options.exists("locktime");
  95 | +
  96 | +    for (const CTxIn& tx_in : rawTx.vin) {
  97 | +        // Can not be FINAL for locktime to work
  98 | +        can_anti_fee_snipe &= tx_in.nSequence != CTxIn::SEQUENCE_FINAL;
    murchandamus commented at 8:08 PM on March 13, 2024:

    In 8e5778a9fb6fbcbddd4ba87cc3f3caf41118592a (wallet, rpc: add anti-fee-sniping to send and sendall):

    It seems to me that the nSequence check here is too restrictive. Locktime is disabled when all sequences are set to final, but it seems to me that can_anti_fee_snipe would be set to false here, even if only one input’s sequence is set to final.

    I’m not sure this would matter much in practice because most transactions would probably use the same sequence value for all inputs, but can_anti_fee_snipe should probably end up true if at least one nSequence is non-final?

    Could you perhaps add a test that a transaction with final nSequence values does not get its locktime set, but one with mixed values and one with all non-final nSequence values both do?

    achow101 commented at 9:48 PM on April 2, 2024:

    It seems to me that the nSequence check here is too restrictive. Locktime is disabled when all sequences are set to final, but it seems to me that can_anti_fee_snipe would be set to false here, even if only one input’s sequence is set to final.

    Currently DiscourageFeeSniping will assert if any sequence is final, so this check is appropriate for now. It also enforces that the sequences are either MAX_SEQUENCE_NONFINAL or MAX_BIP125_RBF_SEQUENCE, so this needs to be checking that as well otherwise an assertion will be hit. Or a commit could be added to relax those checks.

    Could you perhaps add a test that a transaction with final nSequence values does not get its locktime set, but one with mixed values and one with all non-final nSequence values both do?

    I would also like to see tests for other sequence numbers as there is a risk of reaching an assertion here.

    S3RK commented at 7:05 AM on June 19, 2024:

    It also enforces that the sequences are either MAX_SEQUENCE_NONFINAL or MAX_BIP125_RBF_SEQUENCE, so this needs to be checking that as well otherwise an assertion will be hit. Or a commit could be added to relax those checks.

    It seems like this part of the comment wasn't addressed

    ishaanam commented at 7:08 PM on June 27, 2024:

    It also enforces that the sequences are either MAX_SEQUENCE_NONFINAL or MAX_BIP125_RBF_SEQUENCE, so this needs to be checking that as well otherwise an assertion will be hit. Or a commit could be added to relax those checks

    I have added these checks to FinishTransaction.

    Could you perhaps add a test that a transaction with final nSequence values does not get its locktime set, but one with mixed values and one with all non-final nSequence values both do?

    I have added a test for this.

  44. ishaanam commented at 9:44 PM on March 14, 2024: contributor 
    --- 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

  45. achow101 removed review request from achow101 on Apr 16, 2024
  46. DrahtBot added the label Needs rebase on Jun 5, 2024
  47. murchandamus commented at 8:45 PM on June 11, 2024: contributor 
    --- 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?

  48. ishaanam commented at 9:24 PM on June 17, 2024: contributor 
    --- 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.

  49. in src/wallet/rpc/spend.cpp:105 in 8e5778a9fb outdated 
      97 | +        // Can not be FINAL for locktime to work
  98 | +        can_anti_fee_snipe &= tx_in.nSequence != CTxIn::SEQUENCE_FINAL;
  99 | +    }
 100 | +
 101 | +    if (can_anti_fee_snipe) {
 102 | +        LOCK(pwallet->cs_wallet);
    S3RK commented at 7:10 AM on June 19, 2024:

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

    ishaanam commented at 7:10 PM on June 27, 2024:

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

  50. ishaanam force-pushed on Jun 27, 2024
  51. DrahtBot removed the label Needs rebase on Jun 27, 2024
  52. luke-jr referenced this in commit a229d3e7b3 on Aug 1, 2024
  53. luke-jr referenced this in commit 095033167d on Aug 1, 2024
  54. in src/wallet/rpc/spend.cpp:101 in 6825ae0c78 outdated 
     100 |  {
 101 | +    bool can_anti_fee_snipe = !options.exists("locktime");
 102 | +
 103 | +    for (const CTxIn& tx_in : rawTx.vin) {
 104 | +        // Checks sequence values consistent with DiscourageFeeSniping
 105 | +        can_anti_fee_snipe &= (tx_in.nSequence == CTxIn::MAX_SEQUENCE_NONFINAL || tx_in.nSequence == MAX_BIP125_RBF_SEQUENCE);
    murchandamus commented at 6:27 PM on August 5, 2024:

    Nit: I think this is correct given that booleans in C++ are guaranteed to be true or false, but the bitwise operator did make me do a double take.

    I would consider this equivalent and more readable:

            can_anti_fee_snipe = can_anti_fee_snipe && (tx_in.nSequence == CTxIn::MAX_SEQUENCE_NONFINAL || tx_in.nSequence == MAX_BIP125_RBF_SEQUENCE);
    ishaanam commented at 3:58 PM on June 25, 2025:

    Done

  55. in src/wallet/rpc/spend.cpp:1343 in 6825ae0c78 outdated 
    1305 | @@ -1293,7 +1306,8 @@ RPCHelpMan send()
1306 |              rawTx.vout.clear();
1307 |              auto txr = FundTransaction(*pwallet, rawTx, recipients, options, coin_control, /*override_min_fee=*/false);
1308 |  
1309 | -            return FinishTransaction(pwallet, options, CMutableTransaction(*txr.tx));
1310 | +            CMutableTransaction tx = CMutableTransaction(*txr.tx);
1311 | +            return FinishTransaction(pwallet, options, tx);
    murchandamus commented at 6:34 PM on August 5, 2024:

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

    ishaanam commented at 3:52 PM on June 25, 2025:

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

  56. murchandamus commented at 6:48 PM on August 5, 2024: contributor

    crACK b11d00d54ed3315901de1f53885433d864a93019

  57. DrahtBot requested review from S3RK on Aug 5, 2024
  58. DrahtBot requested review from achow101 on Aug 5, 2024
  59. DrahtBot added the label CI failed on Sep 11, 2024
  60. DrahtBot removed the label CI failed on Sep 15, 2024
  61. DrahtBot added the label CI failed on Oct 24, 2024
  62. DrahtBot removed the label CI failed on Oct 25, 2024
  63. DrahtBot added the label CI failed on Mar 13, 2025
  64. DrahtBot commented at 7:54 PM on March 17, 2025: contributor

    Needs rebase, if still relevant.

  65. DrahtBot marked this as a draft on Apr 4, 2025
  66. DrahtBot added the label Needs rebase on Apr 25, 2025
  67. ishaanam force-pushed on Jun 11, 2025
  68. DrahtBot removed the label Needs rebase on Jun 11, 2025
  69. DrahtBot removed the label CI failed on Jun 11, 2025
  70. murchandamus commented at 7:36 PM on June 20, 2025: contributor

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

  71. wallet, rpc: add anti-fee-sniping to `send` and `sendall` 20802c7b65
  72. test: test sendall and send do anti-fee-sniping aac0b6dd79
  73. ishaanam force-pushed on Jun 25, 2025
  74. ishaanam marked this as ready for review on Jun 25, 2025
  75. Sjors commented at 7:18 AM on July 9, 2025: member

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

    I might be useful to borrow the idea from https://github.com/bitcoin/bitcoin/pull/32892/commits/642e4b6f37f5ceef2ac44b1b1c9527a660883d09 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).

  76. murchandamus commented at 9:23 PM on July 21, 2025: contributor

    ACK aac0b6dd79b0db1e9d42a6f466709a61cfd1f69f

    Compared to my prior ACK via range_diff, and only found this minor change and some changes due to the rebase.

    -        can_anti_fee_snipe &= (tx_in.nSequence == CTxIn::MAX_SEQUENCE_NONFINAL || tx_in.nSequence == MAX_BIP125_RBF_SEQUENCE);
+        can_anti_fee_snipe = can_anti_fee_snipe && (tx_in.nSequence == CTxIn::MAX_SEQUENCE_NONFINAL || tx_in.nSequence == MAX_BIP125_RBF_SEQUENCE);

    Tested that the new tests fail without the first commit. Looked over again from scratch, looks good to me.

    Thanks for the explanations.

    I think that @Sjors’s suggestions could perhaps be addressed in a follow-up.

  77. DrahtBot requested review from Sjors on Jul 21, 2025
  78. achow101 commented at 5:57 PM on July 23, 2025: member

    ACK aac0b6dd79b0db1e9d42a6f466709a61cfd1f69f

  79. glozow commented at 3:58 PM on July 29, 2025: member

    ACK aac0b6dd79b0db1e9d42a6f466709a61cfd1f69f

  80. glozow merged this on Jul 29, 2025
  81. glozow closed this on Jul 29, 2025
Contributors
ishaanamDrahtBotS3RKachow101luke-jrmurchandamusSjorsglozow


SjorsS3RKachow101

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-29 03:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me