Wallet: Don't backdate locktime rbf #36040

Bicaru20 commented at 3:28 PM on August 20, 2026: contributor

Currently, in Bitcoin Core, given an original transaction A and its replacement B, we refer to backdating when the locktime of A is higher than the locktime of B (A.locktime > B.locktime). This can happen because Bitcoin Core enables anti-fee-sniping by default, which sets the transaction's locktime to the current block height. For privacy, 10% of the time, it instead sets a different locktime, randomly chosen between the current height and the current height minus 100 blocks. This can lead into having a replacement of a transaction with a locktime older than its original transaction. This is unrealistics and can be used as a wallet fingerprint.

You can find a functional test to reproduce the behaviour mentioned here

The approach proposed in this PR adds a new parameter to CoinControl to keep track of the previous locktime in the case of a bumpfee. We then pass this parameter to the DiscourageFeeSniping function through a new parameter, minimum_height, whose default value is set to 0.

<details> <summary>Alternative approach:</summary> Since the locktime of the new transaction is 0, we also considered setting it to the previous locktime value and then, inside `DiscourageFeeSniping`, saving the previous locktime and setting the transaction's locktime to the block height before applying any of the backdating logic. This way, we could avoid passing a new parameter to the function. We ultimately decided not to go with this approach, as it makes the code more difficult to follow. </details>

The only RPC that is affected by this changes is bumpfee.

The pr also includes a functional test in wallet_bumpfee.py to test that when replacing a transation using bumpfee the locktime is not backdated.

<details> <summary>We also conducted a small analysis to see how many the backdating in RBF transactions actually happen.</summary> We have data on the replaced transactions from 2025-05-01 to 2026-06-01. With that we have been able to detect this many backdatings: <img width="1782" height="891" alt="image" src="https://github.com/user-attachments/assets/1d2a83d5-aa61-4850-82e5-cb434b9ee958" />

We took the date of the last transaction of the replacement chain (A replacement chain are all the transactions that replace themselves)

In total we have over 1,000,000 rbf transactions, but we see that on average there are only about 200-300 hundred replaccements backdating per week. Looking at the percentages we see that on average is less than 2% of all the transactions that have been replaced.

So, it is clear that backdating in replacement transactions is unusual. However, the few transactions that do exhibit backdating are easily fingerprintable as having been replaced using bumpfee in Bitcoin Core or Electrum.

</details>

DrahtBot added the label Wallet on Aug 20, 2026

DrahtBot commented at 3:28 PM on August 20, 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/36040.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Approach ACK	polespinasa
Stale ACK	nervana21, molnard

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

Conflicts

No conflicts as of last run.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

CreateTransaction(wallet, recipients, /change_pos=/std::nullopt, new_coin_control, false) in src/wallet/feebumper.cpp

2026-09-30 16:42:11

Bicaru20 force-pushed on Aug 20, 2026

DrahtBot added the label CI failed on Aug 20, 2026

DrahtBot removed the label CI failed on Aug 20, 2026

nervana21 commented at 12:34 AM on August 21, 2026: contributor

Concept ACK

in src/wallet/feebumper.cpp:317 in f26586b91d

 312 | @@ -313,6 +313,10 @@ Result CreateRateBumpTransaction(CWallet& wallet, const Txid& txid, const CCoinC
 313 |  
 314 |      // We cannot source new unconfirmed inputs(bip125 rule 2)
 315 |      new_coin_control.m_min_depth = 1;
 316 | +    // If no locktime is set, we save the previous one for anti fee sniping
 317 | +    if (!new_coin_control.m_locktime){

nervana21 commented at 5:07 PM on August 23, 2026:

b7935c708edc225e20dcab0820dff774f841736b: Wallet: Do not allow bumpfee to backdate the replacement transaction lockitme

    if (!new_coin_control.m_locktime.has_value()){

nit

polespinasa commented at 2:03 PM on August 25, 2026:

in b7935c708edc225e20dcab0820dff774f841736b Wallet: Do not allow bumpfee to backdate the replacement transaction lockitme

nit: missing space between ) and {

in src/wallet/spend.cpp:1034 in f26586b91d

1030 | +            if (static_cast<uint32_t>(block_height) >= minimum_height) {
1031 | +                int locktime_range = std::min(100, int(block_height - minimum_height));
1032 | +                if (locktime_range > 0){
1033 | +                    tx.nLockTime = std::max(int(minimum_height), int(tx.nLockTime) - int(rng_fast.randrange(locktime_range)));
1034 | +                }
1035 | +            }

nervana21 commented at 5:08 PM on August 23, 2026:

b7935c708edc225e20dcab0820dff774f841736b: Wallet: Do not allow bumpfee to backdate the replacement transaction lockitme

            const int tip_floor_distance = block_height - static_cast<int>(minimum_height);
            if (tip_floor_distance > 0) {
                const int bound = minimum_height > 0
                    ? tip_floor_distance + 1
                    : std::min(100, tip_floor_distance + 1);
                const int back = rng_fast.randrange(bound);
                tx.nLockTime = block_height - back;
            }

There's an OBO in this logic. In the inclusive range [minimum_height, block_height] there are (block_height - minimum_height) + 1 possible values. randrange(block_height - minimum_height) only draws block_height - minimum_height possible values and can never pick the minimum_height value.

Also, when minimum_height == 0, we retain the prior behavior and backdate at most 99 blocks from tip. When minimum_height > 0, we should use the full inclusive window without the 100 block cap.

Bicaru20 commented at 10:18 AM on August 28, 2026:

Also, when minimum_height == 0, we retain the prior behavior and backdate at most 99 blocks from tip. When minimum_height > 0, we should use the full inclusive window without the 100 block cap.

The idea here is to avoid backdating when using bumpfee, without making these changes distinguishable from other transactions that use antifee sniping. If we were to use only minimum_height as the inclusive window, we could end up with replacements with a locktime more than 100 blocks behind the current tip. That would be easily fingerprintable as coming from Core, which is exactly what we want to avoid.

in src/wallet/spend.cpp:1040 in f26586b91d

nervana21 commented at 5:10 PM on August 23, 2026:

b7935c708edc225e20dcab0820dff774f841736b: Wallet: Do not allow bumpfee to backdate the replacement transaction lockitme

        // the privacy of high-latency transactions. Use minimum_height so new
        // sends still get a constant 0 fingerprint, while bumpfee keeps the prior 
        // height and does not make the replacement older than the original.
        // If that height is ahead of the local tip, use 0 so the tx stays final
        // and we do not fingerprint the lagging tip.
        if (minimum_height <= static_cast<uint32_t>(block_height)) {
            tx.nLockTime = minimum_height;
        } else {
            tx.nLockTime = 0;

In the case where we're fee bumping an already-nLockTimed tx, we should not reset its nLockTime back below that established minimum_height. Otherwise we fingerprint the tx. minimum_height defaults to 0, so new sends on a stale chain still retain the prior behavior of nLockTime = 0.

In the case where the minimum_height is ahead of the local tip, set nLockTime = 0

molnard commented at 8:13 AM on August 26, 2026:

Suppose the original tx nLockTime is 96400. The node goes offline for more than 8 hours, and the user calls bumpfee. The PR passes 9640 into DiscourageFeeSniping() but the chain considered stale, so the execution goes directly to tx.nLockTime = 0;. Therefore:

original:    900,000
replacement:       0

Bicaru20 commented at 10:20 AM on August 28, 2026:

In the case where we're fee bumping an already-nLockTimed tx, we should not reset its nLockTime back below that established minimum_height. Otherwise we fingerprint the tx. minimum_height defaults to 0, so new sends on a stale chain still retain the prior behavior of nLockTime = 0.

The only time when this can happen is if the node is still downloading blocks or if the last block is from more than 8 horus ago. This males the block_height unrelaibale to compre and it could happen that minimum_height is greater than the block_height beacuse we don't have blockchain up to date. This could lead to setting a transaction with a locktime to 0 because even though minimum_height is less than the current block tip becasue the blockhain is not up to date. Example: We have the originstl transaction txA with locktime 12. The current tip is 15 but the node has only downloaded till 10. If we replace the transaction, that would set the locktime to 0 (since minimum_height is 12 and current_height is 10), and with that logic this would tell that the node is not up to date. Plus I do not belive that setting the rpelacement set to 0 fingerprints the transactions as there are lots of wallets than when bumping fee set the locktime to 0.

Bicaru20 commented at 10:21 AM on August 28, 2026:

In the case where the minimum_height is ahead of the local tip, set nLockTime = 0

~~But that would disable the antifee sniping completly, wouldn't it be beeter to leave the locktime in the current_tip? That way we are still antifee sniping.~~

Bicaru20 commented at 10:26 AM on August 28, 2026:

I think that is how it should be. See my previous answer: #36040 (review)

Bicaru20 commented at 10:29 AM on August 28, 2026:

In the case where the minimum_height is ahead of the local tip, set nLockTime = 0

You're right about this one, otherwise we would be setting a locktime older than the original transaction.

danielabrozzoni commented at 2:14 PM on August 28, 2026:

In the case where we're fee bumping an already-nLockTimed tx, we should not reset its nLockTime back below that established minimum_height. Otherwise we fingerprint the tx.

I think this makes sense. For example:

Current block height is 15, we create txA with locktime = 15
A few more blocks come in, txA is not included in any, but we fall behind the tip
We want to replace txA with txB; minimum_height = 15. Here we could either: a) set txB's locktime to 0, or b) set txB's locktime to minimum_height

If we go with a), an observer would say that we are either:

using a walelt that does anti-feesniping, but sets nlocktime to 0 when feebumping (assume there are any, I'm not sure)
using Bitcoin Core or equivalent, but we're not synced to the tip. In the imaginary future where we all use the same exact anti-fee sniping strategy, 1. doesn't exist, and the observer would notice we're not synced to the tip

If we go with b), an observer would say that we are either:

using a wallet that does anti-fee sniping on txs and replacements
using some protocol based on presigned transactions that had a locktime set (and there's no fee sniping at all)

The reason 2. wouldn't exist in case a) is that it's pretty weird that in a protocol that uses locktime, a txA has a locktime, but its replacement doesn't.

molnard commented at 11:26 AM on September 1, 2026:

I agree with the axiom of replacement.nLockTime >= original.nLockTime in any case. Good privacy strategy: do not reveal more than you already had, if possible.

Following that, the solution of setting replacement.nLockTime = original.nLockTime preserves the information already visible in the original, while other solutions introduce a new, unusual transition - since we cannot do better because our chain is stale.

in src/wallet/spend.h:193 in f26586b91d outdated

 187 | @@ -188,9 +188,11 @@ util::Result<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& av
 188 |  
 189 |  /**
 190 |   * Set a height-based locktime for new transactions (uses the height of the
 191 | - * current chain tip unless we are not synced with the current chain
 192 | + * current chain tip unless we are not synced with the current chain.
 193 | + * The locktime is occasionally backdated, but never by more than 100 blocks,
 194 | + * and never below minimum_height.

nervana21 commented at 5:12 PM on August 23, 2026:

b7935c708edc225e20dcab0820dff774f841736b: Wallet: Do not allow bumpfee to backdate the replacement transaction lockitme

 * current chain tip unless we are not synced with the current chain).
 * The locktime is occasionally backdated, but never below minimum_height.
 * When minimum_height is 0, backdating is capped at 99 blocks from tip.
 * When minimum_height is set (bumpfee), backdating is uniform on [minimum_height, tip].
 * If the chain is not current and minimum_height is above the local tip, locktime is 0.

nervana21 commented at 5:15 PM on August 23, 2026: contributor

b7935c708edc225e20dcab0820dff774f841736b: Wallet: Do not allow bumpfee to backdate the replacement transaction lockitme

nit: Commit subject and body say "lockitme". Should be "locktime".

Commit body says "This changes change the behavior". It should be "This changes the behavior".

in src/wallet/spend.cpp:1029 in b7935c708e

1023 | @@ -1024,7 +1024,14 @@ void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng_fast,
1024 |          // e.g. high-latency mix networks and some CoinJoin implementations, have
1025 |          // better privacy.
1026 |          if (rng_fast.randrange(10) == 0) {
1027 | -            tx.nLockTime = std::max(0, int(tx.nLockTime) - int(rng_fast.randrange(100)));
1028 | +            // If a previous locktime is passed (like in the bump fee case), the
1029 | +            // backdating is limited between the current height and the previous locktime
1030 | +            if (static_cast<uint32_t>(block_height) >= minimum_height) {

polespinasa commented at 10:52 AM on August 25, 2026:

in b7935c708edc225e20dcab0820dff774f841736b Wallet: Do not allow bumpfee to backdate the replacement transaction lockitme

Is this if needed? can we have a block_height smaller than the minimum_height? I think at least we will always be at the minimum_height, I can only think of a re-org, but then we must swich to a chain with more PoW so the height will likely be higher too.

Also I think this can be simplified into:

$ git diff
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 40eeb78f17..a34c95fcb3 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -1023,15 +1023,11 @@ void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng_fast,
         // that transactions that are delayed after signing for whatever reason,
         // e.g. high-latency mix networks and some CoinJoin implementations, have
         // better privacy.
-        if (rng_fast.randrange(10) == 0) {
+        if (rng_fast.randrange(10) == 0 && tx.nLockTime > minimum_height) {
             // If a previous locktime is passed (like in the bump fee case), the
             // backdating is limited between the current height and the previous locktime
-            if (static_cast<uint32_t>(block_height) >= minimum_height) {
-                int locktime_range = std::min(100, int(block_height - minimum_height));
-                if (locktime_range > 0){
-                    tx.nLockTime = std::max(int(minimum_height), int(tx.nLockTime) - int(rng_fast.randrange(locktime_range)));
-                }
-            }
+            int range = std::min(100, int(block_height - minimum_height + 1));
+            tx.nLockTime = std::max(int(minimum_height), int(tx.nLockTime) - int(rng_fast.randrange(range)));
         }
     } else {
         // If our chain is lagging behind, we can't discourage fee sniping nor help
sliv3r@sliv3r-tuxedo:~/Documentos/Projectes/BitcoinCore/bitcoin$

This version I propose has several improvements:

Removes some redundant if conditions.
Fixes a missing + 1 case that you are missing by just taking block_height - minimum_height.
Keeps the old behavior for the default case. Before this PR, short chains with less than a 100blocks would backdate locktime to 0, while this commit clamps it at 1.

molnard commented at 8:24 AM on August 26, 2026:

When minimum_height is greater, nothing happens. The transaction keeps the already assigned block_height,, which is below the declared minimum.

This can happen, for example, if:

The chain has moved backward after a reorganization or manual invalidateblock.
The original wallet transaction was dropped or never broadcast.
The original transaction was created with a future height locktime.

Bicaru20 commented at 10:22 AM on August 28, 2026:

Is this if needed? can we have a block_height smaller than the minimum_height? I think at least we will always be at the minimum_height, I can only think of a re-org, but then we must swich to a chain with more PoW so the height will likely be higher too.

If the original transaction has a final sequence number and a locktime greater than the current tip, the locktime is not enforced because of the sequence number, so the transaction is valid with a locktime far greater than the current tip. This would make minimum_height in the replacement greater than the current tip. This is the only case I can think of. The other solution would be to check the sequence number of the original transaction.

Bicaru20 commented at 10:29 AM on August 28, 2026:

Yes, you're rigth. I will fix it.

in src/wallet/spend.cpp:1031 in b7935c708e

1023 | @@ -1024,7 +1024,14 @@ void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng_fast,
1024 |          // e.g. high-latency mix networks and some CoinJoin implementations, have
1025 |          // better privacy.
1026 |          if (rng_fast.randrange(10) == 0) {
1027 | -            tx.nLockTime = std::max(0, int(tx.nLockTime) - int(rng_fast.randrange(100)));
1028 | +            // If a previous locktime is passed (like in the bump fee case), the
1029 | +            // backdating is limited between the current height and the previous locktime
1030 | +            if (static_cast<uint32_t>(block_height) >= minimum_height) {
1031 | +                int locktime_range = std::min(100, int(block_height - minimum_height));
1032 | +                if (locktime_range > 0){

polespinasa commented at 2:04 PM on August 25, 2026:

in b7935c7 Wallet: Do not allow bumpfee to backdate the replacement transaction lockitme

nit: again, space between ) and {

in src/wallet/spend.cpp:1336 in b7935c708e

1331 |      if (coin_control.m_locktime) {
1332 |          txNew.nLockTime = coin_control.m_locktime.value();
1333 |          // If we have a locktime set, we can't use anti-fee-sniping
1334 |          use_anti_fee_sniping = false;
1335 | +    } else if (coin_control.m_previous_locktime.has_value() && coin_control.m_previous_locktime < LOCKTIME_THRESHOLD) {
1336 | +            minimum_height = coin_control.m_previous_locktime.value();

polespinasa commented at 2:05 PM on August 25, 2026:

in b7935c7 Wallet: Do not allow bumpfee to backdate the replacement transaction lockitme

nit: this is over indented, there are 8 spaces instead of 4.

in test/functional/wallet_bumpfee.py:858 in f26586b91d

 853 | +
 854 | +        # Replacement with higher fee_rate
 855 | +        change_addr = get_change_address(tx["txid"], wallet)[0]
 856 | +        bumped = wallet.bumpfee(txid=tx["txid"], options={"fee_rate":5, "outputs": [{change_addr: 10}]})
 857 | +
 858 | +        replaced_locktime = rbf_node.getrawtransaction(bumped["txid"],True)["locktime"]

polespinasa commented at 2:07 PM on August 25, 2026:

in f26586b91de16a03d4c265a7e8ee9fefb728dd65 Test: bumpfee does not backdate the locktime

Missing commas between arguments, probably True con go with verbose=True so it is easier to understand.

in test/functional/wallet_bumpfee.py:847 in f26586b91d outdated

 842 | +    current_height = rbf_node.getblockchaininfo()["blocks"]
 843 | +    # The original tx has an older locktime so we can differentiate when the replacement backdates
 844 | +    replaced_locktime = current_height
 845 | +
 846 | +    # Exit the loop when the locktime backdates
 847 | +    while current_height == replaced_locktime:

polespinasa commented at 2:11 PM on August 25, 2026:

in f26586b Test: bumpfee does not backdate the locktime

The test is weak, if the backdating code is broken and it does not backdate, this loop would run forever and never fail. Probably should add a big max num of tries so if after N iterations it did not pass, we can assume the code is wrong.

I would suggest a unit test for the function anyway, that way we can just test the backdating function.

molnard commented at 8:56 AM on August 26, 2026:

Small chance to that the test can pass with the unpatched code, when the first nonzero random backdate is only one or two blocks.

polespinasa commented at 2:25 PM on August 25, 2026: member

Approach ACK

reviewed f26586b91de16a03d4c265a7e8ee9fefb728dd65

in f26586b91de16a03d4c265a7e8ee9fefb728dd65 there as a typo in the commit message. It ends with - should be a .

in src/wallet/spend.cpp:1032 in f26586b91d

1028 | +            // If a previous locktime is passed (like in the bump fee case), the
1029 | +            // backdating is limited between the current height and the previous locktime
1030 | +            if (static_cast<uint32_t>(block_height) >= minimum_height) {
1031 | +                int locktime_range = std::min(100, int(block_height - minimum_height));
1032 | +                if (locktime_range > 0){
1033 | +                    tx.nLockTime = std::max(int(minimum_height), int(tx.nLockTime) - int(rng_fast.randrange(locktime_range)));

molnard commented at 8:35 AM on August 26, 2026:

A true lower-bound implementation would retain the original distribution.

Example: rng_fast.randrange(50) => 0...49.

Therefore, the subtraction can never reach or cross minimum_height.

Bicaru20 commented at 10:24 AM on August 28, 2026:

Fixed in c72998bc4e

in src/wallet/coincontrol.h:120 in f26586b91d outdated

 115 | @@ -116,6 +116,8 @@ class CCoinControl
 116 |      uint32_t m_version = DEFAULT_WALLET_TX_VERSION;
 117 |      //! Locktime
 118 |      std::optional<uint32_t> m_locktime;
 119 | +    //! Save the previous locktime for replacements
 120 | +    std::optional<uint32_t> m_previous_locktime;

molnard commented at 8:46 AM on August 26, 2026:

CCoinControl describes how a new transaction should be constructed - but m_previous_locktime is different. A normal transaction has no previous transaction. The concept exists only because CreateRateBumpTransaction() is constructing an RBF replacement.

There is a hidden coupling now. The generic transaction builder must now understand a fee-bumping detail => maintenance complexity later.

What if we try to find better ownership for this fee-bump-specific improvement? The fee-bumper already possesses both values, so it could enforce the relationship locally:

mtx = CMutableTransaction(*txr.tx);

if (!coin_control.m_locktime &&
    tx->nLockTime < LOCKTIME_THRESHOLD) {
    mtx.nLockTime =
        std::max(mtx.nLockTime, tx->nLockTime);
}

The generic builder creates a normal transaction. The fee-bumper then applies the replacement-specific invariant before signing.

Bicaru20 commented at 10:23 AM on August 28, 2026:

I'm not sure if doing it like this is a good idea. When backdating, if we apply this, the Discuragefeesniping will give a locktime between the [current_height, current_height-100], and most of the times this locktime will be older than the previous locktime since most of replacements are done after a few blocks. That means that we will have either a replacement transaction with a locktime set on the current_height or on the previous lockitme most of the time. If we keep the current logic, we'll have a more diverse distribution of locktimes for replacement transactions.

molnard commented at 11:30 AM on September 1, 2026:

Fair point—the std::max() example may not be the right implementation.

My main point was architectural rather than about that exact implementation. What do you think about keeping this replacement-specific behavior in CreateRateBumpTransaction()? The fee-bumper owns both the original and replacement transactions and therefore knows the invariant it must enforce.

We could still generate the locktime using the desired distribution, possibly through a shared helper, while avoiding m_previous_locktime in the generic CCoinControl and keeping the generic transaction builder unaware of RBF history.

Bicaru20 commented at 9:18 PM on September 8, 2026:

I'd rather keep all the anti fee sniping logic inside the Discuragefeesniping. I think this way it is easier to follow the code and not delegate part of this logic outside the function because of the RBF case. I feel that otherwise we would be duplicating code when the current approach just needs the m_previous_locktime in CCoinControl.

molnard commented at 10:45 AM on September 9, 2026:

I looked more closely at the approach I suggested. Although it separates the replacement-specific behavior a bit more cleanly and avoids adding a field to CCoinControl, it introduces complexity elsewhere.

Your approach fits the existing code structure better, and I think the extra complexity of the separation outweighs its benefits here.

Thanks for taking the time to consider it.

molnard commented at 9:07 AM on August 26, 2026: none

Concept ACK.

I think this can be simplified by enforcing the replacement-specific invariant in CreateRateBumpTransaction() (feebumper.cpp), after creation and before signing. It avoids adding state to CCoinControl, preserves the existing random distribution, and covers stale and previous_locktime > block_height cases that the current implementation misses.

The test appears flaky: unpatched implementation still has about a chance of passing. Could this regression be tested deterministically, perhaps at the unit level with controlled randomness?

Bicaru20 force-pushed on Aug 28, 2026

Bicaru20 commented at 10:24 AM on August 28, 2026: contributor

Thanks for the reviews!

c72998bc4e: Fix the nits. Also applied Pol suggestion to simplify the code in Discuragefeesniping and at the same time fixing the OBO problem.
f911ae8345: Add verbose=True

For now putting the pr as a draft. Seeing some of the feedback I see now that the functional test is weak and that a unit test is better. Once the unit test is completed I will open the pr again. During this time, feel free to respond to my replies to your comments so we can agree on the best approach.

Bicaru20 marked this as a draft on Aug 28, 2026

Bicaru20 force-pushed on Sep 8, 2026

DrahtBot added the label CI failed on Sep 8, 2026

DrahtBot commented at 10:00 PM on September 8, 2026: contributor

🚧 At least one of the CI tasks failed. Task OpenBSD Cross: https://github.com/bitcoin/bitcoin/actions/runs/34280727979/job/102244563839 LLM reason (✨ experimental): CI failed due to a Clang build error from -Wthread-safety-analysis/-Werror in wallet/test/spend_tests.cpp (calling chainman->ActiveChain() without exclusively holding cs_main).

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

Bicaru20 commented at 10:06 PM on September 8, 2026: contributor

Changes made:

cc8f4784cf9ba77fb22209adbfa9c2176b82228a: I adpoted @nervana21 suggestion. Now we don't reset the nLocktime below the established minimum_height even if we are on a stal chain. The only case when we reset the nlocktime to 0 is if the minimum_height is higher than the current block_height. In this case, we set it to 0.
903d2bfa675f1acfc24e21fbbeff9ceb8b0e019e: I've eliminated the functional test and added a unit test in the spend_tests.cpp of the wallet. I added the following cases:
1. First, the case to check that the backdating when having a minimum_height occurs between the expected range ([minimum_height, block_height]).
2. Second, the case to check that the backdating can reach the minimum_height. This way we prove that the OBO error is not there and also check that when calculating the range in spend.cpp this is not 0 as this would cause an error later on rng_fast.randrange(range).
3. Third, the case where the minimum_height > block_height. In this case we set the locktime to 0.
4. Fourth, the case where we have a stale chain and minimum_height <= block_height, in this case we set the locktime to minimum_height
5. Finally, the case where we have a stale chain and minimum_height > block_height, in this case we set the locktime to 0.

Bicaru20 marked this as ready for review on Sep 8, 2026

DrahtBot removed the label CI failed on Sep 8, 2026

in src/wallet/test/spend_tests.cpp:129 in 903d2bfa67 outdated

 125 | @@ -120,5 +126,82 @@ BOOST_FIXTURE_TEST_CASE(wallet_duplicated_preset_inputs_test, TestChain100Setup)
 126 |      BOOST_CHECK(!CreateTransaction(*wallet, recipients, /*change_pos=*/std::nullopt, coin_control));
 127 |  }
 128 |  
 129 | +BOOST_FIXTURE_TEST_CASE(discourage_fee_sniping_backdating_bounds, TestChain100Setup)

molnard commented at 11:48 AM on September 9, 2026:

Could you add a test covering the full modified path, starting with passing the original locktime through CCoinControl? The current tests call DiscourageFeeSniping() directly, so they would still pass if the original locktime were no longer forwarded.

A wallet unit test that calls CreateRateBumpTransaction() should be enough. Using a stale tip and an original height-based locktime below the tip would make the check deterministic: the replacement should retain the original locktime rather than fall back to zero.

Bicaru20 commented at 5:34 PM on September 12, 2026:

Done! Added a test in feebumper_test.cpp which bumps a transaction directly through CreateRateBumpTransaction.

in src/wallet/test/spend_tests.cpp:160 in 903d2bfa67 outdated

 155 | +    }
 156 | +    BOOST_CHECK(backdated);
 157 | +
 158 | +    // Check that backdating can reach the minimum height.
 159 | +    backdated = false;
 160 | +    for (int i{0}; i < 200; ++i) {

molnard commented at 11:51 AM on September 9, 2026:

Could we use an explicitly fixed RNG seed here and verify that the resulting sequence reaches the minimum within the iteration limit? The test can fail even with correct code if none of the 200 attempts succeed (even though the chances are low - thinking long term).

Bicaru20 commented at 5:34 PM on September 12, 2026:

Yes, I think it makes sense to use a fixed rng seed. Thanks for pointing it out!

in src/wallet/test/spend_tests.cpp:126 in 903d2bfa67 outdated

 125 | @@ -120,5 +126,82 @@ BOOST_FIXTURE_TEST_CASE(wallet_duplicated_preset_inputs_test, TestChain100Setup)
 126 |      BOOST_CHECK(!CreateTransaction(*wallet, recipients, /*change_pos=*/std::nullopt, coin_control));

molnard commented at 11:54 AM on September 9, 2026:

Could you add a case with minimum_height greater than 99 blocks below the tip? The current tests use gaps of only 1 and 10 blocks, so they wouldn't catch the removal of the 100-value cap. On a current chain, the locktime should still stay within [block_height - 99, block_height].

Bicaru20 commented at 5:34 PM on September 12, 2026:

Done! I modified the discourage_fee_sniping_backdating_bounds first test so the minimum_height is greater than the 100 block limit.

in src/wallet/spend.cpp:1029 in 903d2bfa67 outdated

1022 | @@ -1023,13 +1023,22 @@ void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng_fast,
1023 |          // that transactions that are delayed after signing for whatever reason,
1024 |          // e.g. high-latency mix networks and some CoinJoin implementations, have
1025 |          // better privacy.
1026 | -        if (rng_fast.randrange(10) == 0) {
1027 | -            tx.nLockTime = std::max(0, int(tx.nLockTime) - int(rng_fast.randrange(100)));
1028 | +        if (rng_fast.randrange(10) == 0 && tx.nLockTime > minimum_height) {
1029 | +            // If a previous locktime is passed (like in the bump fee case), the
1030 | +            // backdating is limited between the current height and the previous locktime
1031 | +            int range = std::min(100, int(block_height - minimum_height + 1));

molnard commented at 12:04 PM on September 9, 2026:

nit:

With minimum_height == 0, this also changes the locktime distribution for ordinary sends when the chain height is below 99, short chains (like regtest).

For example, at height 2, the old code samples an offset from [0, 99] and clamps negative results to zero. Within the 10% random branch, offset 0 gives locktime 2, offset 1 gives locktime 1, and offsets 2–99 all give locktime 0. Zero therefore has a 98% probability within that branch.

The new code samples an offset from [0, 2], so locktimes 0, 1, and 2 each have a one-third probability within the random branch. The possible locktimes are the same, but their probabilities are different. The 90% branch that uses the current height is unchanged.

Is this intentional? I think this is good as it is, I just wanted to point out the change.

Bicaru20 commented at 5:33 PM on September 12, 2026:

Is this intentional? I think this is good as it is, I just wanted to point out the change.

I didn't though of that case, but I don't think it matters since in new chains you can start spending the UTXOs in block 101, and by then this is not a problem anymore.

molnard commented at 12:11 PM on September 9, 2026: none

Concept ACK.

I ran all new and modified tests locally, and they passed. I've left my test-related comments inline.

in src/wallet/test/spend_tests.cpp:14 in 903d2bfa67

   5 | @@ -6,6 +6,12 @@
   6 |  #include <key.h>
   7 |  #include <script/solver.h>
   8 |  #include <validation.h>
   9 | +#include <interfaces/chain.h>
  10 | +#include <primitives/transaction.h>
  11 | +#include <random.h>
  12 | +#include <uint256.h>
  13 | +#include <util/time.h>
  14 | +#include <test/util/setup_common.h>

nervana21 commented at 8:07 PM on September 9, 2026:

903d2bfa675f1acfc24e21fbbeff9ceb8b0e019e: Test: bumpfee does not backdate the locktime

nit: these should be alphabetized with the rest of the includes

in src/wallet/test/spend_tests.cpp:156 in 903d2bfa67 outdated

 151 | +        if (mtx.nLockTime < static_cast<uint32_t>(block_height)) {
 152 | +            backdated = true;
 153 | +            break;
 154 | +        }
 155 | +    }
 156 | +    BOOST_CHECK(backdated);

nervana21 commented at 8:20 PM on September 9, 2026:

903d2bfa675f1acfc24e21fbbeff9ceb8b0e019e: Test: bumpfee does not backdate the locktime

    bool backdated{false};
    bool saw_tip_locktime{false};
    for (int i{0}; i < 200; ++i) {
        DiscourageFeeSniping(mtx, rng_fast, chain, block_hash, block_height, block_height - 10);
        BOOST_CHECK_LE(mtx.nLockTime, static_cast<uint32_t>(block_height));
        BOOST_CHECK_GE(mtx.nLockTime, static_cast<uint32_t>(block_height - 10));
        if (mtx.nLockTime == static_cast<uint32_t>(block_height)) {
            saw_tip_locktime = true;
        } else {
            backdated = true;
        }
        if (backdated && saw_tip_locktime) break;
    }
    BOOST_CHECK(backdated);
    BOOST_CHECK(saw_tip_locktime);

Would it make sense to strengthen this test to check not only that we backdated but also check that we left the locktime unchanged?

Bicaru20 commented at 5:35 PM on September 12, 2026:

Yes, I think it doesnt hurt and makes the test more robust.

nervana21 commented at 8:23 PM on September 9, 2026: contributor

Thanks for taking the suggestions. I've left just a few more comments.

in src/wallet/test/spend_tests.cpp:180 in 903d2bfa67

 175 | +}
 176 | +
 177 | +BOOST_FIXTURE_TEST_CASE(discourage_fee_sniping_stale_tip, TestChain100Setup)
 178 | +{
 179 | +    // Verify that DiscourageFeeSniping works as expected when having a stale chain and called
 180 | +    // with the minimum_height parameter.

nervana21 commented at 10:14 PM on September 9, 2026:

903d2bfa675f1acfc24e21fbbeff9ceb8b0e019e: Test: bumpfee does not backdate the locktime

    // Verify DiscourageFeeSniping on a stale tip when minimum_height is set.

nit: wording

Bicaru20 commented at 5:35 PM on September 12, 2026:

Done!

Bicaru20 force-pushed on Sep 12, 2026

DrahtBot added the label CI failed on Sep 12, 2026

Bicaru20 commented at 5:39 PM on September 12, 2026: contributor

Thanks again for the reviews!

In b72a0533dac88b46755f9ee1051eb588b1934050:
- I added a test in feebumper_test.cpp (since we use feebumper logic) which bumps a transaction directly through CreateRateBumpTransaction. As suggested by @molnard, we do it on a stale chain so we can check if the previous lockitme is propagated to the new tx.
- In the test discourage_fee_sniping_backdating_bounds I added a fixed rng seed and modified the first test so the minimum_height is greater than the 100 block limit.
- Also added saw_tip_locktime check suggested by @nervana21

DrahtBot removed the label CI failed on Sep 12, 2026

in src/wallet/test/spend_tests.cpp:148 in b72a0533da

 143 | +
 144 | +    CMutableTransaction mtx;
 145 | +    mtx.vin.emplace_back(COutPoint{Txid::FromUint256(uint256::ONE), 0},
 146 | +                         CScript(), CTxIn::MAX_SEQUENCE_NONFINAL);
 147 | +
 148 | +    // Backdating is constrained to [block_height - 100, block_height].

molnard commented at 9:48 AM on September 14, 2026:

Shouldn't this be [block_height - 99, block_height]? randrange(100) returns values from 0 to 99, so the maximum backdate is 99 blocks. Including the current height, that gives 100 possible locktimes.

The BOOST_CHECK_GE below should also use block_height - 99; otherwise it allows a 100-block backdate.

Bicaru20 commented at 11:46 AM on September 15, 2026:

True, I got confussed when writing the comments. Thnaks!

in src/wallet/test/spend_tests.cpp:163 in b72a0533da

 158 | +        if (mtx.nLockTime == static_cast<uint32_t>(block_height)) {
 159 | +            saw_tip_locktime = true;
 160 | +        } else {
 161 | +            backdated = true;
 162 | +        }
 163 | +        if (backdated && saw_tip_locktime) break;

molnard commented at 9:58 AM on September 14, 2026:

Could we remove the break and run all 200 iterations? This would check more generated locktimes against both bounds and could catch regressions from future changes that the early exit would miss. We can still check backdated and saw_tip_locktime after the loop.

For example, with the current fixed seed, the first four locktimes are 111, 111, 111, 17, even if we remove the std::min(100, ...) cap. The loop exits there and misses the regression. In a standalone reproduction using Core's RNG, continuing the loop catches a violation of the 99-block bound on call 37.

Bicaru20 commented at 11:46 AM on September 15, 2026:

Agreed, it makes sense.

molnard commented at 10:00 AM on September 14, 2026: none

ACK b72a0533dac88b46755f9ee1051eb588b1934050 with nits.

The code looks good. I've left a couple of minor comments on the tests.

DrahtBot requested review from polespinasa on Sep 14, 2026

DrahtBot requested review from nervana21 on Sep 14, 2026

in src/wallet/test/feebumper_tests.cpp:75 in b72a0533da

  70 | +    CCoinControl coin_control;
  71 | +    coin_control.Select(coins[0].outpoint);
  72 | +    std::vector<CRecipient> recipients{{*Assert(wallet->GetNewDestination(OutputType::BECH32, "dummy")),
  73 | +                                        /*nAmount=*/40 * COIN, /*fSubtractFeeFromAmount=*/true}};
  74 | +
  75 | +    auto res = CreateTransaction(*wallet, recipients, /*change_pos=*/std::nullopt, coin_control);

nervana21 commented at 4:55 PM on September 14, 2026:

b72a0533dac88b46755f9ee1051eb588b1934050: Test: bumpfee does not backdate the locktime

    // Mine one more block so one coinbase is mature, then sync a spendable wallet.
    CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
    auto wallet = CreateSyncedWallet(*m_node.chain, WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return m_node.chainman->ActiveChain()), coinbaseKey);

    std::vector<CRecipient> recipients{{*Assert(wallet->GetNewDestination(OutputType::BECH32, "dummy")),
                                        /*nAmount=*/40 * COIN, /*fSubtractFeeFromAmount=*/true}};

    auto res = CreateTransaction(*wallet, recipients, /*change_pos=*/std::nullopt, CCoinControl{});

Since we're only dealing with a single coinbase coin, I think this can be simplified.

Bicaru20 commented at 11:58 AM on September 15, 2026:

True, thisway is better. Thanks!

DrahtBot requested review from nervana21 on Sep 14, 2026

in src/wallet/test/feebumper_tests.cpp:85 in b72a0533da

  80 | +    std::vector<bilingual_str> errors;
  81 | +    CAmount old_fee;
  82 | +    CAmount new_fee;
  83 | +    CMutableTransaction mtx;
  84 | +
  85 | +    // Rate bump with a stale tip: the lcoktime is set to the previous locktime.

nervana21 commented at 4:57 PM on September 14, 2026:

b72a0533dac88b46755f9ee1051eb588b1934050: Test: bumpfee does not backdate the locktime

    // Rate bump with a stale tip: keep the original transaction's locktime.

nit

Bicaru20 commented at 12:31 PM on September 15, 2026:

Done!

DrahtBot requested review from nervana21 on Sep 14, 2026

in src/wallet/test/feebumper_tests.cpp:89 in b72a0533da

  84 | +
  85 | +    // Rate bump with a stale tip: the lcoktime is set to the previous locktime.
  86 | +    const CBlockIndex* tip{WITH_LOCK(Assert(m_node.chainman)->GetMutex(),
  87 | +                                     return m_node.chainman->ActiveChain().Tip())};
  88 | +    SetMockTime(std::chrono::seconds{tip->GetBlockTime()} + std::chrono::hours{9});
  89 | +    auto bump_res = feebumper::CreateRateBumpTransaction(*wallet, txr.tx->GetHash(), coin_control, errors, old_fee, new_fee, mtx, /*require_mine=*/false, /*outputs=*/{});

nervana21 commented at 4:57 PM on September 14, 2026:

b72a0533dac88b46755f9ee1051eb588b1934050: Test: bumpfee does not backdate the locktime

    auto bump_res = feebumper::CreateRateBumpTransaction(*wallet, txr.tx->GetHash(), CCoinControl{}, errors, old_fee, new_fee, mtx, /*require_mine=*/false, /*outputs=*/{});

as above

Bicaru20 commented at 12:30 PM on September 15, 2026:

Done!

DrahtBot requested review from nervana21 on Sep 14, 2026

Bicaru20 force-pushed on Sep 15, 2026

Bicaru20 commented at 12:33 PM on September 15, 2026: contributor

Changes made:

Reduced the bump_transaction_fee_sniping_check test in feebumper_test as suggested by @nervana21
In discourage_fee_sniping_backdating_bounds I eliminated the breaks and change the 100 for a 99 as @molnard suggested.

molnard commented at 8:53 PM on September 15, 2026: none

ACK e34f9340d615976b1398e6ead0ab8d2bda24907c

Checked the code and ran the tests - all good.

in src/wallet/spend.h:193 in e34f9340d6

 186 | @@ -187,9 +187,13 @@ util::Result<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& av
 187 |  
 188 |  /**
 189 |   * Set a height-based locktime for new transactions (uses the height of the
 190 | - * current chain tip unless we are not synced with the current chain
 191 | + * current chain tip unless we are not synced with the current chain).
 192 | + * The locktime is occasionally backdated, but never below minimum_height.
 193 | + * When minimum_height is 0, backdating is capped at 99 blocks from tip.
 194 | + * When minimum_height is set (bumpfee), backdating is uniform on [minimum_height, tip].

nervana21 commented at 2:28 PM on September 16, 2026:

675448e26225d2d82b363a508048dc7819037219: Wallet: Do not allow bumpfee to backdate the replacement transaction locktime

 * The locktime is occasionally backdated, but never below minimum_height
 * and at most 99 blocks below the tip.

nit

DrahtBot requested review from nervana21 on Sep 16, 2026

nervana21 commented at 2:30 PM on September 16, 2026: contributor

675448e26225d2d82b363a508048dc7819037219: Wallet: Do not allow bumpfee to backdate the replacement transaction locktime

For this commit message: lockitme -> locktime greter -> greater

nervana21 commented at 2:30 PM on September 16, 2026: contributor

tACK e34f9340d615976b1398e6ead0ab8d2bda24907c

left minor, non-blocking nits

DrahtBot requested review from nervana21 on Sep 16, 2026

molnard commented at 7:26 PM on September 21, 2026: none

tACK e34f9340d615976b1398e6ead0ab8d2bda24907c

Reviewed the code and ran all tests locally.

Bicaru20 force-pushed on Sep 22, 2026

Bicaru20 commented at 9:04 AM on September 22, 2026: contributor

Addressed @nervana21 nits. Just minor changes:

Fixed the typos in the first commit message
Changed the comments in spend.h

nervana21 commented at 2:35 PM on September 22, 2026: contributor

ACK 8836a3643bca8fce1185b0420f509dc75dae0a4a

DrahtBot requested review from molnard on Sep 22, 2026

molnard commented at 7:21 PM on September 22, 2026: none

tACK https://github.com/bitcoin/bitcoin/commit/8836a3643bca8fce1185b0420f509dc75dae0a4a

in src/wallet/coincontrol.h:119 in 7049f9ba51

 114 | @@ -115,6 +115,8 @@ class CCoinControl
 115 |      uint32_t m_version = DEFAULT_WALLET_TX_VERSION;
 116 |      //! Locktime
 117 |      std::optional<uint32_t> m_locktime;
 118 | +    //! Save the previous locktime for replacements
 119 | +    std::optional<uint32_t> m_previous_locktime;

achow101 commented at 10:18 PM on September 25, 2026:

In 7049f9ba5175c278110984e4964a4c56b264f9fe "Wallet: Do not allow bumpfee to backdate the replacement transaction locktime"

I suggest calling this minimum_locktime rather than previous_locktime as it's use could be for more than just dealing with replacements.

in src/wallet/spend.cpp:1028 in 7049f9ba51

1022 | @@ -1023,13 +1023,22 @@ void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng_fast,
1023 |          // that transactions that are delayed after signing for whatever reason,
1024 |          // e.g. high-latency mix networks and some CoinJoin implementations, have
1025 |          // better privacy.
1026 | -        if (rng_fast.randrange(10) == 0) {
1027 | -            tx.nLockTime = std::max(0, int(tx.nLockTime) - int(rng_fast.randrange(100)));
1028 | +        if (rng_fast.randrange(10) == 0 && tx.nLockTime > minimum_height) {
1029 | +            // If a previous locktime is passed (like in the bump fee case), the
1030 | +            // backdating is limited between the current height and the previous locktime

achow101 commented at 10:19 PM on September 25, 2026:

In 7049f9ba5175c278110984e4964a4c56b264f9fe "Wallet: Do not allow bumpfee to backdate the replacement transaction locktime"

Discussing replacements is unnecessary in this function. This does not need to care about what the callers are.

in src/wallet/spend.cpp:1036 in 7049f9ba51

1035 |          // If our chain is lagging behind, we can't discourage fee sniping nor help
1036 | -        // the privacy of high-latency transactions. To avoid leaking a potentially
1037 | -        // unique "nLockTime fingerprint", set nLockTime to a constant.
1038 | +        // the privacy of high-latency transactions. Use minimum_height so new
1039 | +        // sends still get a constant 0 fingerprint, while bumpfee keeps the prior
1040 | +        // height and does not make the replacement older than the original.

achow101 commented at 10:19 PM on September 25, 2026:

In 7049f9ba5175c278110984e4964a4c56b264f9fe "Wallet: Do not allow bumpfee to backdate the replacement transaction locktime"

Same comment regarding referencing replacements.

in src/wallet/spend.cpp:1338 in 7049f9ba51

1333 |      if (coin_control.m_locktime) {
1334 |          txNew.nLockTime = coin_control.m_locktime.value();
1335 |          // If we have a locktime set, we can't use anti-fee-sniping
1336 |          use_anti_fee_sniping = false;
1337 | +    } else if (coin_control.m_previous_locktime.has_value() && coin_control.m_previous_locktime < LOCKTIME_THRESHOLD) {
1338 | +        minimum_height = coin_control.m_previous_locktime.value();

achow101 commented at 10:19 PM on September 25, 2026:

In 7049f9ba5175c278110984e4964a4c56b264f9fe "Wallet: Do not allow bumpfee to backdate the replacement transaction locktime"

This should be below inside of if (use_anti_fee_sniping).

in src/wallet/test/feebumper_tests.cpp:59 in 8836a3643b

  55 | @@ -51,6 +56,32 @@ BOOST_AUTO_TEST_CASE(external_max_weight_test)
  56 |      CheckMaxWeightComputation("", {"3042021f5c4c29e6b686aae5b6d0751e90208592ea96d26bc81d78b0d3871a94a21fa8021f74dc2f971e438ccece8699c8fd15704c41df219ab37b63264f2147d15c34d801", "01", "6321024cf55e52ec8af7866617dc4e7ff8433758e98799906d80e066c6f32033f685f967029000b275210214827893e2dcbe4ad6c20bd743288edad21100404eb7f52ccd6062fd0e7808f268ac"}, "002089e84892873c679b1129edea246e484fd914c2601f776d4f2f4a001eb8059703", 318);
  57 |  }
  58 |  
  59 | +BOOST_FIXTURE_TEST_CASE(bump_transaction_fee_sniping_check, TestChain100Setup)

achow101 commented at 10:20 PM on September 25, 2026:

In 8836a3643bca8fce1185b0420f509dc75dae0a4a "Test: bumpfee does not backdate the locktime"

This does not need to be a unit test. For wallet things, we prefer functional tests over unit tests wherever possible.

Bicaru20 commented at 4:43 PM on September 30, 2026:

Created a functional test in wallet_bumfee.py

Wallet: Do not allow bumpfee to backdate the replacement transaction locktime

This changes the behavior of bumpfee so when used, the replacement
transaction doesn't have an older locktime than the original transaction.
Now the replacement transaction will have a locktime between the block_height
and the locktime of the original transaction, except when the minimum_height
is greater than block_height. This could happen if the original transaction
has a high locktime and a sequence number that disable locktime validation.
In this case the tx is valid because the locktime is not used.

Co-Authored-By: danielabrozzoni <danielabrozzoni@protonmail.com>

fae0b2cb06

Bicaru20 force-pushed on Sep 30, 2026

Test: bumpfee does not backdate the locktime

Unit test to check that the DiscourageFeeSniping function behaves as expected
when the minimum_height parameter is passed.
We test that the backdating occurs between the block_height and minimum_height
range.
We test that when the transaction doesn't pass IsCurrentForAntiFeeSniping the
locktime is set to minimum_height if block_height > minimum_height,
or to 0 if block_height < minimum_height.

Co-Authored-By: danielabrozzoni <danielabrozzoni@protonmail.com>

bf80b5a711

Bicaru20 force-pushed on Sep 30, 2026

Bicaru20 commented at 4:42 PM on September 30, 2026: contributor

Addressed all the comments and suggestions by @achow101:

fae0b2cb06a99c427be034ac5c8351acef4f3357: As suggested, I changed the varible name from m_previous_locktime to m_minimum_locktime. Also changed the comments in spend.cpp to avoid disccusing replacements. In spend.cpp changed a condition so it is inside if (use_anti_fee_sniping).
bf80b5a7114216f29485bedf9c2358c6aa1947d3: Changed the bump_transaction_fee_sniping_check unit test to functional. I modified a little the approach: Create a transaction with a locktime older by 101 blocks and then replaced it with a stale tip so the lcoktime in the replacement is the same as the original. As explained in the test, I choose to create a tx with an explicit locktime 101 blocks below the current height beacuse antifee sniping normally sets the locktime to the current height, or it backdates it by a random 0-100 blocks. So a locktime 101 blocks older can't be produced by either, so if the bumped tx has this locktime, we know it was copied from the original transaction rather than recomputed by anti-fee-sniping.

DrahtBot added the label CI failed on Sep 30, 2026

DrahtBot removed the label CI failed on Sep 30, 2026

in src/wallet/spend.cpp:1028 in fae0b2cb06

1022 | @@ -1024,13 +1023,21 @@ void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng_fast,
1023 |          // that transactions that are delayed after signing for whatever reason,
1024 |          // e.g. high-latency mix networks and some CoinJoin implementations, have
1025 |          // better privacy.
1026 | -        if (rng_fast.randrange(10) == 0) {
1027 | -            tx.nLockTime = std::max(0, int(tx.nLockTime) - int(rng_fast.randrange(100)));
1028 | +        if (rng_fast.randrange(10) == 0 && tx.nLockTime > minimum_height) {
1029 | +            // If a previous minimum_height is passed, the backdating is limited
1030 | +            // between the current height and the previous locktime.

achow101 commented at 9:31 PM on September 30, 2026:

Stopped at fae0b2cb06a (Wallet: Do not allow bumpfee to backdate the replacement transaction locktime)

Still mentions previous locktime

in src/wallet/spend.cpp:1331 in fae0b2cb06

1325 | @@ -1319,13 +1326,19 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
1326 |              txNew.vin.back().scriptWitness = *scripts.second;
1327 |          }
1328 |      }
1329 | +
1330 | +    // Minimum height the DiscourageFeeSniping can backdate to
1331 | +    uint32_t minimum_height = 0;

achow101 commented at 9:31 PM on September 30, 2026:

Stopped at fae0b2cb06a (Wallet: Do not allow bumpfee to backdate the replacement transaction locktime)

Should also be inside of if (use_anti_fee_sniping)