2026 dont backdate locktime rbf#3
Conversation
This changes change the behavior of bumpfee so when used, the replacement transaction doesn't have an older lockitme than the original transaction. Now the replacement transaction will have either the block_height as the locktime or the locktime of the original transaction.
Test to check that the replacement transaction from bumpfee does not have an older locktime than the original transaction-
Bicaru20
force-pushed
the
2026-dont-backdate-locktime-RBF
branch
from
78a0e0f to
b4602c0
Compare
| // backdating is limited betwen the current height and the previous lockitme | ||
| if (previous_locktime > 0){ | ||
| // To avoid issues with the randrange, locktime_range cannot be 0 | ||
| int locktime_range = std::max(1, std::min(100, int(block_height - previous_locktime))); |
There was a problem hiding this comment.
Instead of using max, I think it would be clearer to calculate locktime_range, and if it's 0, skip the next step (we can't backdate anyways). Like this:
int locktime_range = std::min(100, int(block_height - previous_locktime));
if (locktime_range > 0) ...
Otherwise right now it's a bit convoluted: what happens if block_height == previous_locktime? Then locktime_range = 1, and below tx.nLockTime is the max between previous_locktime and tx.nLockTime - 1, meaning that it will be previous_locktime. It is still correct, but a bit harder to follow
| // If we have a locktime set, we can't use anti-fee-sniping | ||
| use_anti_fee_sniping = false; | ||
| } else if (coin_control.m_previous_locktime.has_value()) { | ||
| txNew.nLockTime = coin_control.m_previous_locktime.value(); |
There was a problem hiding this comment.
I'm a bit unsure between passing the previous locktime to the function by setting txNew.nLockTime as you're doing now, or by simply adding a new parameter for the minimum height (which should be set to zero if there's no minimum).
I know I pushed a bit against adding a lot of parameters throughout the code, but I feel like here it might make the code easier to follow. Otherwise people inside DiscourageFeeSniping need to have the context of "if there was a nlocktime in the previous transaction, it is now in nLockTime".
| // To avoid issues with the randrange, locktime_range cannot be 0 | ||
| int locktime_range = std::max(1, std::min(100, int(block_height - previous_locktime))); | ||
| tx.nLockTime = std::max(int(previous_locktime), int(tx.nLockTime) - int(rng_fast.randrange(locktime_range))); | ||
| }else{ |
There was a problem hiding this comment.
Don't forget to run clang-tidy before opening PR on bitcoin/bitcoin :) https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#running-clang-tidy
2 participants
When doing a replacement, avoid backdate the locktime to an older value than the original transaction.