Wallet: Don't backdate locktime rbf #36040

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

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

nit

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

nit: missing space between ) and {

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.

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.

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

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

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.

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.

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

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.

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:

1. using a walelt that does anti-feesniping, but sets nlocktime to 0 when feebumping (assume there are any, I'm not sure)
2. 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:

1. using a wallet that does anti-fee sniping on txs and replacements
2. 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.

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.

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.

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:

1. Removes some redundant if conditions.
2. Fixes a missing + 1 case that you are missing by just taking block_height - minimum_height.
3. 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.

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.

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.

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

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

nit: again, space between ) and {

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

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

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.

Fixed in c72998bc4e

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.

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.

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.

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.