wallet: don’t back-date locktime below input locktime #34480

pull danielabrozzoni wants to merge 6 commits into bitcoin:master from danielabrozzoni:issue/26527-dont-backtime-nlocktime-unconf changing 7 files +158 −8
  1. danielabrozzoni commented at 12:47 pm on February 2, 2026: member

    Fixes #26527

    The Bitcoin Core wallet implements an anti-fee-sniping mechanism that sets a transaction’s nLockTime to the current block height, and occasionally to an earlier height.

    Previously, when creating a transaction, the wallet could choose an nLockTime that was lower than the nLockTime of one of its inputs. This creates an unrealistic “transaction signed earlier but only broadcast now” scenario and may act as a wallet fingerprint.

    This PR fixes the issue by adding a min_allowed_locktime parameter to DiscourageFeeSniping. When creating a transaction, the wallet now sets this minimum to the highest locktime among all parent transactions. This ensures that while the locktime can still be backdated for anti-fee-sniping, it won’t go below any input’s locktime.

    DiscourageFeeSniping is invoked by the following RPCs:

    • sendall
    • send
    • sendtoaddress
    • sendmany
    • fundrawtransaction
    • walletcreatefundedpsbt

    Tests are added for sendall, send, sendtoaddress and sendmany.

    fundrawtransaction and walletcreatefundedpsbt can’t hit this edge case, because they don’t use the anti-fee-sniping logic and instead always set nLockTime to 0 unless explicitly provided by the user.

  2. DrahtBot added the label Wallet on Feb 2, 2026
  3. DrahtBot commented at 12:48 pm on February 2, 2026: contributor

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

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept NACK gmaxwell
    Concept ACK rkrux

    If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

  4. danielabrozzoni commented at 12:52 pm on February 2, 2026: member
    @ishaanam: I put you as a co-author for the first commit, because I started working from your code here #26902#pullrequestreview-1522310111 :)
  5. in src/wallet/spend.cpp:1301 in c4325985a0
    1297+        // Find the highest nLockTime of all unconfirmed inputs so that
    1298+        // this transaction does not use a lower value.
    1299+        // Setting a lower nLockTime would make the "signed earlier but broadcast later"
    1300+        // explanation unrealistic, and may act as a wallet fingerprint.
    1301+        unsigned int max_unconfirmed_input_locktime{0}; // If there aren't any unconfirmed inputs, the minimum locktime is 0
    1302+        for (const auto& coin: selected_coins) {
    


    achow101 commented at 10:03 pm on February 2, 2026:

    In c4325985a0a6f82d482f9c1f9c0276336b2476c8 “wallet: don’t back-date locktime below unconfirmed input locktime”

    nit:

    0        for (const auto& coin : selected_coins) {
    
  6. in test/functional/wallet_send.py:572 in d4b45ac6fb
    563@@ -563,6 +564,25 @@ def test_weight_limits(self):
    564 
    565         self.nodes[1].unloadwallet("test_weight_limits")
    566 
    567+    def test_unconfirmed_input_fee_sniping(self):
    568+        self.log.info("Test that send sets the transaction locktime no lower than any unconfirmed input locktime")
    569+        self.nodes[1].createwallet("test_fee_sniping")
    570+        # Create parent tx with anti-fee-sniping nlocktime
    571+        wallet = self.nodes[1].get_wallet_rpc("test_fee_sniping")
    572+        unconfirmed_parent_txid = self.nodes[0].sendtoaddress(wallet.getnewaddress(), 10)
    


    achow101 commented at 10:08 pm on February 2, 2026:

    In d4b45ac6fb09ac85579724497ec9ba639235fe0a “test: check send RPC locktime is not below unconfirmed input locktime”

    This relies on the anti-fee-sniping to randomly backdate the locktime. I think it would be better if this could set the locktime to a specific value that would be in range so that the range is not often just a single value.

  7. in test/functional/wallet_send.py:579 in d4b45ac6fb
    574+        # Parent nlocktime should be within 100 blocks of tip
    575+        assert_greater_than_or_equal(unconfirmed_parent_nlocktime, self.nodes[0].getblockcount() - 100)
    576+        self.sync_mempools()
    577+        # Create child tx spending the unconfirmed parent
    578+        utxos = wallet.listunspent(0, 0)
    579+        child_txid = wallet.send([{wallet.getnewaddress(): 8}], inputs=utxos)["txid"]
    


    achow101 commented at 10:09 pm on February 2, 2026:

    In d4b45ac6fb09ac85579724497ec9ba639235fe0a “test: check send RPC locktime is not below unconfirmed input locktime”

    Since backdating anti-fee-sniping is random, we should do this multiple times to be reasonably sure that we get a backdated nlocktime.

  8. achow101 commented at 10:12 pm on February 2, 2026: member

    The sendtoaddress and sendmany RPCs don’t spend unconfirmed inputs

    They can spend unconfirmed change, and I would prefer for these to be tested as well since the calling of DiscourageFeeSniping is different between these RPCs and send and sendall.

  9. wallet: don't back-date locktime below input locktime
    When creating a transaction with anti fee sniping enabled, the wallet
    could previously select an nLockTime that was lower than the nLockTime
    of its input transactions. This can break the "transaction signed earlier
    but broadcast later" explanation and may act as a wallet fingerprint.
    
    This is fixed by constraining the anti fee sniping logic to never select
    an nLockTime lower than the maximum nLockTime of the wallet-owned input
    transactions. The minimum allowed value is passed to
    DiscourageFeeSniping().
    
    Co-Authored-By: ishaanam <ishaana.misra@gmail.com>
    8c537a1a26
  10. test: assert anti-fee-sniping locktime is within 100 blocks
    The previous assertion only verified that nLockTime was non-zero.
    Tighten the check to ensure the locktime is within the allowed range,
    up to 100 blocks in the past.
    8299967664
  11. danielabrozzoni force-pushed on Feb 5, 2026
  12. danielabrozzoni renamed this:
    wallet: don't back-date locktime below unconfirmed input locktime
    wallet: don't back-date locktime below input locktime
    on Feb 5, 2026
  13. danielabrozzoni commented at 10:17 am on February 5, 2026: member

    Thanks for your review Ava! I picked up your suggestions, and the idea in #26527 (comment).

    Changes since last push:

    • Now we avoid backdating the nlocktime below any input nLocktime, not just unconfirmed inputs.
    • I added tests for sendtoaddress and sendmany. These use confirmed parent transactions instead of unconfirmed change, which was easier to implement and still tests the same behavior.
    • In the tests, I explicitly set the parent nLockTime and retry creating a child transaction until I get one with a backdated nLockTime (nLockTime != tip_height).

    One concern I have, which I just realized: since we cannot know the nLockTime for external inputs, we might still end up backdating below those inputs’ nLockTime. This could be problematic, since it may leak some information about which inputs are wallet-owned. Is this acceptable?

    I thought about a couple of ways to mitigate this but could not implement either. Let me know if I missed something:

    • Getting external unconfirmed inputs from the mempool. Unfortunately, I could use wallet.chain to check whether a tx is in the mempool (isInMempool), but couldn’t retrieve the whole transaction
    • Getting external inputs from the txindex, if enabled. Unfortunately I could not access g_txindex from the wallet code.

    Otherwise, we could disallow backdating if there’s at least one external input, by setting min_allowed_locktime to the tip height.

  14. in test/functional/wallet_send.py:585 in 4033a5ccf9
    580+        child_nlocktime = tip_height
    581+        # We keep track of the amount to send, so that at each loop we send a bit less, paying more in fees
    582+        # and replacing the previous transaction
    583+        amount_to_send = 99
    584+        while child_nlocktime == tip_height:
    585+            child_txid = wallet.send([{wallet.getnewaddress(): amount_to_send}], inputs=utxos)["txid"]
    


    achow101 commented at 0:08 am on February 6, 2026:

    In 4033a5ccf924a22bb9236f67052dc05f1b14c446 “test: check send respects input locktimes with anti fee sniping”

    Setting add_to_wallet=False will prevent the transaction from being added to the wallet or broadcast. That will allow this loop to run without needing to change the amount to send.

    Same for the sendall test.

  15. test: check send respects input locktimes with anti fee sniping
    Add a test to verify that when the anti fee sniping logic randomly
    backdates a transaction locktime, the send RPC never selects a value
    lower than the nLockTime of its input transactions.
    4100ee8aa0
  16. test: check sendall respects input locktimes with anti fee sniping
    Add a test to verify that when the anti fee sniping logic randomly
    backdates a transaction locktime, the send RPC never selects a value
    lower than the nLockTime of its input transactions.
    c409d99daa
  17. test: check sendtoaddress respects input locktimes with anti fee sniping
    Add a test to verify that when the anti fee sniping logic randomly
    backdates a transaction locktime, the send RPC never selects a value
    lower than the nLockTime of its input transactions.
    4d8bedb2de
  18. test: check sendmany respects input locktimes with anti fee sniping
    Add a test to verify that when the anti fee sniping logic randomly
    backdates a transaction locktime, the send RPC never selects a value
    lower than the nLockTime of its input transactions.
    8bd6857eb8
  19. danielabrozzoni force-pushed on Feb 9, 2026
  20. danielabrozzoni commented at 7:24 pm on February 9, 2026: member

    Thanks!

    Last push (8bd6857eb8dc720f8a0883d2c587021b2e8fd0e6):

  21. rkrux commented at 12:35 pm on February 10, 2026: contributor
    Concept ACK 8bd6857eb8dc720f8a0883d2c587021b2e8fd0e6, reviewing at the moment.
  22. in src/wallet/spend.h:219 in 8c537a1a26
    215@@ -200,8 +216,9 @@ struct CreatedTransactionResult
    216 /**
    217  * Set a height-based locktime for new transactions (uses the height of the
    218  * current chain tip unless we are not synced with the current chain
    219+ *
    


    rkrux commented at 12:36 pm on February 10, 2026:

    In 8c537a1a26aedca65717437e1e3b2cb8a732d30a “wallet: don’t back-date locktime below input locktime”

    Nit: Might as well close the unclosed parenthesis in the comment.

  23. in src/wallet/spend.h:221 in 8c537a1a26
    215@@ -200,8 +216,9 @@ struct CreatedTransactionResult
    216 /**
    217  * Set a height-based locktime for new transactions (uses the height of the
    218  * current chain tip unless we are not synced with the current chain
    219+ *
    220  */
    221-void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng_fast, interfaces::Chain& chain, const uint256& block_hash, int block_height);
    222+void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng_fast, interfaces::Chain& chain, const uint256& block_hash, int block_height, unsigned int min_allowed_locktime);
    


    rkrux commented at 12:38 pm on February 10, 2026:

    In 8c537a1 “wallet: don’t back-date locktime below input locktime”

    0- void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng_fast, interfaces::Chain& chain, const uint256& block_hash, int block_height, unsigned int min_allowed_locktime);
    1+ void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng_fast, interfaces::Chain& chain, const uint256& block_hash, int block_height, uint32_t min_allowed_locktime);
    

    Reference: https://github.com/bitcoin/bitcoin/blob/64294c89094d5ab10d87236729cc267fde0a24ca/src/primitives/transaction.h#L294

  24. in src/wallet/spend.cpp:1308 in 8c537a1a26
    1304+        unsigned int max_input_locktime{0};
    1305+        for (const auto& coin : selected_coins) {
    1306+            if (const CWalletTx* coin_wtx{wallet.GetWalletTx(coin->outpoint.hash)}) {
    1307+                max_input_locktime = std::max(max_input_locktime, coin_wtx->tx->nLockTime);
    1308+            }
    1309+        }
    


    rkrux commented at 12:42 pm on February 10, 2026:

    In 8c537a1 “wallet: don’t back-date locktime below input locktime”

    Can we loop this over txNew.vin similar to the implementation in rpc/spend? Then, we would be able to extract it out in a helper function and could avoid the code duplication.

    The verbose comment is helpful but doesn’t need to be duplicated imo.


    rkrux commented at 12:05 pm on February 11, 2026:
    This helpful function, which would be extracted, most likely could be reused for fixing the related issue for replacement transactions later: #26526.
  25. in src/wallet/spend.cpp:987 in 8c537a1a26
    982@@ -989,7 +983,8 @@ static bool IsCurrentForAntiFeeSniping(interfaces::Chain& chain, const uint256&
    983 }
    984 
    985 void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng_fast,
    986-                                 interfaces::Chain& chain, const uint256& block_hash, int block_height)
    987+                                 interfaces::Chain& chain, const uint256& block_hash, int block_height,
    988+                                 unsigned int min_allowed_locktime)
    


    rkrux commented at 12:42 pm on February 10, 2026:
    Same comment: #34480 (review)
  26. in test/functional/wallet_sendmany.py:1 in 8bd6857eb8


    rkrux commented at 12:45 pm on February 10, 2026:

    In 8bd6857eb8dc720f8a0883d2c587021b2e8fd0e6 “test: check sendmany respects input locktimes with anti fee sniping”

    For the commit message:

    locktime, the send RPC never selects

    Either the last 4 test commits could be squashed into one, or the correct RPC name should be mentioned in the commit description. send is mentioned even for other RPCs.

  27. in test/functional/wallet_create_tx.py:77 in 4d8bedb2de
    72+        num_tries = 0
    73+        while child_nlocktime == tip_height:
    74+            child_txid = test_wallet.sendtoaddress(wallet_rpc.getnewaddress(), 9)
    75+            child_nlocktime = test_wallet.gettransaction(txid=child_txid, verbose=True)["decoded"]["locktime"]
    76+            num_tries += 1
    77+            if num_tries >= num_parents:
    


    rkrux commented at 1:01 pm on February 10, 2026:

    In 4d8bedb “test: check sendtoaddress respects input locktimes with anti fee sniping”

    In agreement with using multiple unspents, the previous 2 test commits didn’t. However, few points:

    1. This doesn’t test spending multiple utxos in one transaction, so we never fully test the logic that the max among them is picked up.
    2. In order to test the above point, the parent transactions should have different locktimes.
    3. I don’t see why 100 unspents need to be tested and I’m unable to see the correlation between num_tries and num_parents. There’s a wait_until helper that waits for the passed predicate to be true until a configurable timeout is reached. We want to avoid the assertion error that’s one line below as much as possible. Reference: https://github.com/bitcoin/bitcoin/blob/64294c89094d5ab10d87236729cc267fde0a24ca/test/functional/test_framework/test_framework.py#L745-L746

    danielabrozzoni commented at 2:37 pm on February 11, 2026:

    In agreement with using multiple unspents, the previous 2 test commits didn’t.

    Yes, sorry if it wasn’t clear in the commit message. What I’m doing here is:

    • creating 100 parent transactions
    • creating children transactions over and over again, each one of them spending exactly one parent (child_tx i spends parent_tx i), until I find one child transaction that has a backdated nlocktime. At that point, I check that the nlocktime is >= the one of the parent.

    The reason why only the sendtoaddress and sendmany tests do this is because with send/sendall I can set add_to_wallet=False, not adding the tx to the wallet or broadcasting it, and so I can keep re-using the same parent transaction. (See #34480 (review))

    I can absolutely modify the test to have multiple parents, and update the variable names/comments to make everything clearer!

    There’s a wait_until helper that waits for the passed predicate to be true until a configurable timeout is reached.

    Ah, thanks! I’ll try refactoring the test in order to use wait_until :)


    rkrux commented at 10:57 am on February 12, 2026:

    The reason why only the sendtoaddress and sendmany tests do this is because with send/sendall I can set add_to_wallet=False, not adding the tx to the wallet or broadcasting it, and so I can keep re-using the same parent transaction.

    I see, add_to_wallet does seem quite handy but unfortunate that it’s not available to sendtoaddress/sendmany. I guess the current idea seems alright but would suggest the following:

    1. The transactions should spend 2 inputs with different nlocktimes instead of one.
    2. If somehow in all these tries, the backdated nlocktime is not generated, we should not raise the below assertion error - it would make the test flaky and might fail CI of unrelated PRs later. I don’t believe the wait_until approach would be foolproof as well. The only complete solution I see is to fund the wallet with more inputs and keep retrying until the backdate nlocktime is generated.
    0if num_tries >= num_parents:
    1   raise AssertionError("RPC call sendtoaddress() did not produce a backdated nLockTime")
    
  28. in test/functional/wallet_send.py:585 in 4100ee8aa0
    580+        child_nlocktime = tip_height
    581+        while child_nlocktime == tip_height:
    582+            # We set add_to_wallet=False, so that the transaction won't get added to the wallet or broadcasted.
    583+            # This way we can continue creating transactions spending the same input, until we find one with
    584+            # a backdated nlocktime
    585+            child_hex = wallet.send([{wallet.getnewaddress(): 9}], inputs=utxos, add_to_wallet=False)["hex"]
    


    rkrux commented at 1:05 pm on February 10, 2026:

    In 4100ee8aa007141b59d990bec95064ed42e04b9a “test: check send respects input locktimes with anti fee sniping”

    From this PR comment #34480 (comment):

    One concern I have, which I just realized: since we cannot know the nLockTime for external inputs, we might still end up backdating below those inputs’ nLockTime. This could be problematic, since it may leak some information about which inputs are wallet-owned. Is this acceptable? I thought about a couple of ways to mitigate this but could not implement either. Let me know if I missed something: Getting external unconfirmed inputs from the mempool. Unfortunately, I could use wallet.chain to check whether a tx is in the mempool (isInMempool), but couldn’t retrieve the whole transaction

    Is this concern still valid? IIUC, this test successfully tests the external unconfirmed inputs. The wallet in node0 sends some funds to the wallet in node1 that are unconfirmed at the moment.


    danielabrozzoni commented at 8:00 pm on February 11, 2026:

    I think I misused the word “external”, I meant “inputs that are not owned by the wallet”. In this test case, node1 receives the funds and spends them, so the input is owned by the wallet.

    Before calling DiscourageFeeSniping, I’m finding the highest input nlocktime using GetWalletTx, but this means that if the transaction input is not owned by the wallet, I won’t be able to see the nlocktime.

    I think I can hit this edge case in a couple of ways… For example, using send + add_to_wallet=False + inputs:

    0❯ ./build/bin/bitcoin-cli -datadir=signet-datadir -named send outputs='[{"tb1p4tp4l6glyr2gs94neqcpr5gha7344nfyznfkc8szkreflscsdkgqsdent4": 0.00001}]' add_to_wallet=false fee_rate=1 inputs='[{"txid": "fb20af6c84b3fc1baff2f915140d0445b3479549093de35bdbe9e7d7d1008333", "vout": 0}]'
    1{
    2  "psbt": "cHNidP8BAIkCAAAAATODANHX5+nbW+M9CUmVR7NFBA0UFfnyrxv8s4RsryD7AAAAAAD9////Am5QPJMYAAAAIlEgR/Kst32gSmc8pi48WvNByPA4RzliEXhWybc25kgAUpnoAwAAAAAAACJRIKrDX+kfINSIFrPIMBHRF++jWs0kFNNsHgKw8p/DEG2Q0XAEAAAAAQUgtEB0iqlfBOJ0PlRRM3ubBOQxa5Kq4XSqozDCQ/cjgDUhB7RAdIqpXwTidD5UUTN7mwTkMWuSquF0qqMwwkP3I4A1GQAIf5iUVgAAgAEAAIAAAACAAQAAAH0AAAAAAA==",
    3  "complete": false
    4}
    5        
    

    I picked these addresses and txid from a block explorer, and they are not in my wallet. The resulting psbt was signed at block height 291092, but the nlocktime was backdated to 291025. If I understand correctly the core wallet would have had no way of knowing whether it was backlocking before the input tx nlocktime, because it didn’t know the input tx.


    rkrux commented at 10:19 am on February 12, 2026:

    I meant “inputs that are not owned by the wallet”.

    When I read this statement, the first things that pops up in my mind is that wallet can’t spend the inputs that it doesn’t own, which seems fine.

    For example, using send + add_to_wallet=False + inputs I picked these addresses and txid from a block explorer, and they are not in my wallet.

    I think I now get what you mean. However, this seems like quite a contrived example, can you explain how this can happen in a real world scenario - as in what would motivate the user go about using this approach?

    0{"add_to_wallet", RPCArg::Type::BOOL, RPCArg::Default{true}, "When false, returns the serialized transaction without broadcasting or adding it to the wallet"},
    

    If add_to_wallet is used, then the user is responsible for storing the serialized transaction because there is no other way for the wallet to get it. If the user intends to create a child transaction of such a transaction, then the full transaction hex must be provided by the user, an option that I could not find in these RPCs. All I can see are the following arguments for inputs in these RPCs that allude to the assumption that the wallet can find the hex of the transaction pointed to by txid - something that doesn’t hold true for the above example.

     0{"inputs", RPCArg::Type::ARR, RPCArg::Default{UniValue::VARR}, "Specify inputs instead of adding them automatically.",
     1                        {
     2                          {"", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "", {
     3                            {"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The transaction id"},
     4                            {"vout", RPCArg::Type::NUM, RPCArg::Optional::NO, "The output number"},
     5                            {"sequence", RPCArg::Type::NUM, RPCArg::DefaultHint{"depends on the value of the 'replaceable' and 'locktime' arguments"}, "The sequence number"},
     6                            {"weight", RPCArg::Type::NUM, RPCArg::DefaultHint{"Calculated from wallet and solving data"}, "The maximum weight for this input, "
     7                                        "including the weight of the outpoint and sequence number. "
     8                                        "Note that signature sizes are not guaranteed to be consistent, "
     9                                        "so the maximum DER signatures size of 73 bytes should be used when considering ECDSA signatures."
    10                                        "Remember to convert serialized sizes to weight units when necessary."},
    11                          }},
    12                        },
    13                    },
    
  29. rkrux commented at 1:06 pm on February 10, 2026: contributor
    I have shared few points.
  30. in src/wallet/spend.cpp:1024 in 8c537a1a26
    1021+        // current block height; otherwise, we already picked the earliest possible nLockTime.
    1022+        if (static_cast<int>(min_allowed_locktime) < block_height && rng_fast.randrange(10) == 0) {
    1023+            // Calculate how far back we can set nLockTime: at most 100 blocks, but constrained
    1024+            // by min_allowed_locktime to make sure we don't go below the minimum value
    1025+            int locktime_range = std::min(100, int(block_height - min_allowed_locktime));
    1026+            tx.nLockTime = std::max(int(min_allowed_locktime), int(tx.nLockTime) - int(rng_fast.randrange(locktime_range)));
    


    rkrux commented at 1:30 pm on February 10, 2026:

    In 8c537a1a26aedca65717437e1e3b2cb8a732d30a “wallet: don’t back-date locktime below input locktime”

    Building up on this previous comment for the nLockTime data type #34480 (review): I find these intermediate int conversions unnecessary because the tx.nLockTime type is already uint32_t. These casts also make the code difficult to read and promotes the tendency in the reader to skip reading altogether.

    The following diff removes all of them except the block_height one. Though it can be argued that its type can also be changed but that would be an unrelated change, so to keep the diff minimal:

    0-            int locktime_range = std::min(100, int(block_height - min_allowed_locktime));
    1-            tx.nLockTime = std::max(int(min_allowed_locktime), int(tx.nLockTime) - int(rng_fast.randrange(locktime_range)));
    2+            uint32_t locktime_range = std::min(100U, uint32_t(block_height) - min_allowed_locktime);
    3+            tx.nLockTime = std::max(min_allowed_locktime, tx.nLockTime - rng_fast.randrange(locktime_range));
    
  31. rkrux changes_requested
  32. gmaxwell commented at 7:31 pm on February 14, 2026: contributor

    NACK.

    On its face this is kind of a pointless change: A transaction can have any locktime before where was created, it’s not a “time the transaction was created” except as a side effect of anti-fee sniping and then only if it was anti-fee-sniping that set the lock time… and so it’s not weird for a transaction to have a locktime after one of its inputs. It leaks no information to do this, so it’s harmless if semantically meaningless since you can’t confirm a transaction before its inputs.

    The practice is not, however, necessarily semantically meaningless. It is if SIGHASH_ALL is used, but if the transaction uses ANYONECANPAY or just doesn’t have signatures, then it may be completely semantically sensible to have the locktime of a transaction lower than an input it uses when it does get confirmed.

    On these basis I believe #26527 was just mistaken, it’s not a problem.

    All this seemed like pointless churn to me, and harmless within the context of core’s wallet and anti-fee-sniping where the simplified understanding (e.g. no ANYONECANPAY) usually applies… I wasn’t going to comment at all, other than to maybe mention it to the authors so they didn’t think they could make these assumptions outside of the wallet. But then I realized this PR has more serious problems:

    Applying the clamping only to wallet transactions is a distinguisher which identifies inputs as belonging to the wallet or not, and would potentially leak which inputs belong to the author (e.g. in a payjoin/coinjoin transactions). It doesn’t follow the causality argument (for non-anyonecanpay inputs) that a tx cannot be confirmed before its inputs, which doesn’t care who authored the inputs. I think this is a fatal privacy vulnerability. Even though nothing in the wallet’s UI creates payjoins or coinjoins right now AFAIK people do it via the RPC, and future features could implement stuff like payjoin support without noticing this leak.

    The approach here is also incorrect in that even among wallet transactions it would apply deeply and not just to the immediate inputs: We cannot assume that all transactions in the wallet were authored with the same code we’re currently. So if C spends B which spends A then C’s locktime would need to be checked against A too and not just B.

    Correctly applying that causality limit would require applying it to all transactions in the causative transaction history, regardless of who owns them. – This data isn’t tracked at all right now not even in the chainstate, so there is no trivial fix. Even making it just go one deep (which still doesn’t completely ‘fix’ the behavior) is also not trivial due to needing non-wallet data.

    The code also doesn’t appear to correctly (or at least intentionally) handle the fact that locktime is logically a union type between a lock height and a lock time. How does one construct a maximum between inputs that are lock heights and lock times? (I believe the current code just blindly changes things to locktimes if any input is a time– probably not the intended behavior!). It also doesn’t appear to check if the locktime is actually active through non-maximal sequence.

    So I think to implement the intent of this would require tracking in the UTXO set a maximum locktime and lock height for the casual history of every output in the change (taking care to inspect sequences to only include active locks), and then apply it. That would be conceptually correct absent the existence of ANYONECANPAY (which I think can’t ever be made to make sense with this behavior). Sensibly handling mixed locktimes also seems hard even once you’ve correctly tracked them, as you may not always be able to convert between them until very close to locked in (when heights are convertible to MTP times).

    But I think ultimately pointless because it’s an attempt to fix a non-issue, and would be a complex, invasive, and runtime expensive change, and the half measure should not be done due to the privacy issue.

  33. achow101 commented at 8:59 pm on February 14, 2026: member

    Even though nothing in the wallet’s UI creates payjoins or coinjoins right now AFAIK people do it via the RPC

    Currently doing those via the walletcreatefundedpsbt and *rawtransaction RPCs results in no anti-fee-sniping being applied at all. I think send does apply anti-fee-sniping, but that can also easily be changed to not do it if any inputs are not known to the wallet.

    future features could implement stuff like payjoin support without noticing this leak.

    This probably needs a recommendation in the BIPs as well.

    The code also doesn’t appear to correctly (or at least intentionally) handle the fact that locktime is logically a union type between a lock height and a lock time. How does one construct a maximum between inputs that are lock heights and lock times? (I believe the current code just blindly changes things to locktimes if any input is a time– probably not the intended behavior!).

    Hmm, good point. It looks like it would change to timestamps, and then immediately assert.

    It also doesn’t appear to check if the locktime is actually active through non-maximal sequence.

    There are checks prior to calling DiscourageFeeSniping to ensure that sequences are non-final so that anti-fee-sniping will work in general.

  34. gmaxwell commented at 10:32 pm on February 14, 2026: contributor

    There are checks prior to calling DiscourageFeeSniping to ensure that sequences are non-final so that anti-fee-sniping will work in general.

    That helps only if you can presume all in-wallet transactions (including potentially ones created before anti-fee-sniping was invented) were created by code that makes them final! :)

  35. danielabrozzoni commented at 11:48 am on February 19, 2026: member

    Thanks for the feedback @gmaxwell!

    I agree with you that this change is a half measure, and your message helped me understand that it’s a smaller improvement than what I initially thought. I still think it might be worth pursuing, since the privacy issue seems solvable. However, I’m not sure if this particular half measure is worth the review time and slight increase in code complexity.

    The goal of this PR is to make anti-fee-sniping less obvious in certain (very) specific cases. The benefit is that an external observer would not be able to clearly distinguish between a wallet that implements anti-fee-sniping and one that simply supports nLockTime where the user selected it manually.

    That said, there are a couple of issues, so thanks for pointing them out and taking the time to explain!

    Applying the clamping only to wallet transactions is a distinguisher which identifies inputs as belonging to the wallet or not…

    As far as I can see, there are two ways to create a multiparty transaction:

    1. You ask all your cosigners their inputs’ outpoints, create a transaction with all the inputs, sign it (without need of using ANYONECANPAY), pass it on.
    2. You create a transaction with your inputs, sign with ANYONECANPAY, pass it on to your cosigners, which will add their inputs and sign

    Case 1) can be hit at the moment, using sendall or send. You can solve the privacy issue by not backdating when there are external inputs. Since anti fee sniping only backdates 10 percent of the time, an observer cannot easily distinguish between “not backdated because it is multiparty” and “not backdated due to randomness.”

    We can’t hit 2) at the moment, since we don’t have a way to create a transaction and sign it with ANYONECANPAY with any RPCs affected by this change (sendall, send, sendtoaddress, sendmany), and I think it’s safe to assume that we never will.

    The approach here is also incorrect in that even among wallet transactions it would apply deeply and not just to the immediate inputs: We cannot assume that all transactions in the wallet were authored with the same code we’re currently. So if C spends B which spends A then C’s locktime would need to be checked against A too and not just B.

    Yes, true. I agree with you that implementing the full fix wouldn’t be worth the effort, and without implementing it, this is just a half measure.

    The code also doesn’t appear to correctly (or at least intentionally) handle the fact that locktime is logically a union type between a lock height and a lock time.

    Yes, thanks for catching this. I didn’t consider time-based locktime when writing the current code, which currently would simply not backdate at all if any parent had a time-based nlocktime. (It doesn’t crash, but that’s not because I handled time-based locktimes properly, just because it happens to work out :))

    I think there could be two ways to go about this:

    1. Don’t backdate at all if any input transaction has a time-based locktime, or
    2. Only consider height based locktimes when calculating min_allowed_locktime. If a parent has a time based locktime, I will simply ignore it. This means that if I create B, spending A, and A has a time-based locktime, I ignore it, and B might or might not have a locktime that is <= A’s locktime, and I might or might not show that I’m backdating.

    I don’t have a strong preference either way.

    It also doesn’t appear to check if the locktime is actually active through non-maximal sequence.

    There are checks prior to calling DiscourageFeeSniping to ensure that sequences are non-final so that anti-fee-sniping will work in general.

    That helps only if you can presume all in-wallet transactions (including potentially ones created before anti-fee-sniping was invented) were created by code that makes them final! :)

    Yes, thank you. All code paths that lead to calling DiscourageFeeSniping already check that the sequence is non-final so anti-fee-sniping can work. However, when calculating min_allowed_locktime, I consider each input’s locktime without checking whether the sequence actually made that locktime active in the previous transaction.

    Where this puts us

    One approach for this PR could be:

    a) Only consider the immediate parents of a transaction, instead of all the ancestor history. As you pointed out, considering the whole history would be extremely complex and not worth it.

    b) Check that the previous locktime is active, ofc :) We can check each inputs’ nSequence, ignore the transaction nlocktime if all are SEQUENCE_FINAL

    Then, regarding time based locktimes, one of the following:

    c1) Only consider height based timelocks from the parents, and ignore time based ones. c2) Or, if one of my parents has a time-based locktime, disable backdating altogether.

    And finally

    d) Disable backdating if there are non-wallet inputs

    The result would be:

    In the specific case this PR targets, if I create B spending A and A has a height-based locktime, then B’s locktime would not be set below A’s.

    However, there are still cases this does not address:

    • As you pointed out, if I create C spending B, which spends A, and A’s locktime is greater than or equal to B’s, then C is only clamped against B. That means C could still end up with a locktime lower than A’s, which would reveal backdating.
    • For time-based locktimes, it depends on whether we go with c1 or c2. If we choose c1 and I create B spending A where A has a time-based locktime, B could still be backdated below A’s locktime, again revealing anti-fee-sniping. If we choose c2 instead, then backdating would be disabled in that situation, so B would not be backdated at all.

    Overall, this would be a small improvement over the current behavior, but I am still unsure whether it justifies the effort.


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-03-03 21:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me