#36040 Wallet: Don't backdate locktime rbf
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36040 Wallet: Don't backdate locktime rbf | Ready | 0 (+2) | Strong | 28 + 137 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Prevents wallet fee-bumped transactions from receiving an nLockTime earlier than the transaction they replace. When replacing a transaction via bumpfee, the wallet records the original locktime in CCoinControl and uses it as a floor in DiscourageFeeSniping so random anti-fee-sniping backdating cannot decrease the locktime.
Problem: Bitcoin Core's anti-fee-sniping mechanism randomly backdates transaction locktimes by up to 99 blocks approximately 10% of the time. When bumping fees on an existing transaction, this random roll can result in a replacement transaction with an older locktime than the original, creating an unnatural on-chain pattern that fingerprints the wallet as Bitcoin Core or Electrum.
Evidence of importance:
- Addresses issue #26526.
- Author's empirical survey in PR description: across over 1,000,000 RBF transactions between May 2025 and June 2026, roughly 200 to 300 replacements backdated locktimes per week (under 2% of replacements), uniquely fingerprinting them.
Scope: Touches transaction generation in the wallet (spend.cpp and feebumper.cpp), adding a minimum_height parameter to DiscourageFeeSniping.
Risk of a mistake: medium (wallet). A flaw in anti-fee-sniping locktime bounds could set an invalid locktime or lock a transaction to a future height, causing fee bumps to fail or transactions to remain unbroadcastable.
Changed since the previous assessment: The implementation approach remains the same; handling for edge cases where the chain tip is stale or lagging behind minimum_height was refined and tested.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- author: addressing open suggestions from polespinasa (r:5426214913, rc:4193517172, rc:4193685626) and contested approach item from achow101 (rc:4149545215) 2026-09-30
- reviewer: polespinasa to review author's reply and mocktime handling in functional test 2026-10-14
Agreement
Strong
substantive support, no open objection (molnard, molnard, molnard)
Thread notes: The author replied to polespinasa's mocktime objection (rc:4193748472) explaining the use of int(time.time()), moving the objection to answered, and rebased the branch.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| nervana21 (contributor) | objection, correctness | An off-by-one error prevents backdating from ever selecting the minimum_height value | fixed · fix pushed · addressed in push and confirmed by reviewer | answered | yes | 2026-08-23: '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 p' Status from: 2026-09-09: nervana21: 'Thanks for taking the suggestions.' |
| nervana21 (contributor) | objection, safety | Resetting locktime below minimum_height on stale chains fingerprints the transaction | fixed · fix pushed · addressed in push and confirmed by reviewer | answered | yes | 2026-08-23: '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.' Status from: 2026-09-09: nervana21: 'Thanks for taking the suggestions.' |
| polespinasa (member) | suggestion, approach | Simplify DiscourageFeeSniping logic and range calculation | fixed · fix pushed · Author applied simplification in c72998bc4e | answered | yes | 2026-08-25: 'Also I think this can be simplified into:' Status from: 2026-08-28: Bicaru20: 'Also applied Pol suggestion to simplify the code in Discuragefeesniping and at the same time fixing the OBO problem.' |
| polespinasa (member) | objection, correctness | The functional test loop can run indefinitely without failing if backdating is broken | fixed · fix pushed · replaced with unit tests | answered | yes | 2026-08-25: 'The test is weak, if the backdating code is broken and it does not backdate, this loop would run forever and never fail.' Status from: 2026-08-28: Bicaru20: 'Seeing some of the feedback I see now that the functional test is weak and that a unit test is better.' |
| molnard (none) | objection, correctness | On stale chains, bumpfee resets locktime to 0, backdating an already-locktimed transaction | fixed · fix pushed · fixed in push and acknowledged by reviewer | answered | yes | 2026-08-26: '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 =' Status from: 2026-09-15: molnard: 'Checked the code and ran the tests - all good.' |
| molnard (none) | objection, correctness | When minimum_height exceeds block_height during reorgs, locktime remains at block_height below minimum | fixed · fix pushed · addressed in push and confirmed | answered | yes | 2026-08-26: 'When minimum_height is greater, nothing happens. The transaction keeps the already assigned block_height,, which is below the declared minimum.' Status from: 2026-09-15: molnard: 'Checked the code and ran the tests - all good.' |
| molnard (none) | objection, maintenance | Adding fee-bumping state to generic CCoinControl creates hidden coupling | accepted · reviewer conceded after author explanation | answered | yes | 2026-08-26: 'There is a hidden coupling now. The generic transaction builder must now understand a fee-bumping detail => maintenance complexity later.' Status from: 2026-09-09: molnard: 'Your approach fits the existing code structure better, and I think the extra complexity of the separation outweighs its ' |
| molnard (none) | objection, correctness | The functional test is flaky and can pass on unpatched code | fixed · fix pushed · replaced with unit tests | answered | yes | 2026-08-26: 'Small chance to that the test can pass with the unpatched code, when the first nonzero random backdate is only one or two blocks.' Status from: 2026-09-15: molnard: 'Checked the code and ran the tests - all good.' |
| molnard (none) | suggestion, approach | Add a test covering the full modified path starting with passing original locktime through CCoinControl | fixed · fix pushed · author added test in feebumper_tests.cpp and reviewer ACKed | answered | yes | 2026-09-09: 'Could you add a test covering the full modified path, starting with passing the original locktime through `CCoinControl`?' Status from: 2026-09-15: molnard: 'Checked the code and ran the tests - all good.' |
| molnard (none) | suggestion, correctness | Use an explicitly fixed RNG seed in the unit test | fixed · fix pushed · fixed seed added in push and acknowledged by reviewer | answered | yes | 2026-09-09: 'Could we use an explicitly fixed RNG seed here and verify that the resulting sequence reaches the minimum within the iteration limit?' Status from: 2026-09-15: molnard: 'Checked the code and ran the tests - all good.' |
| molnard (none) | suggestion, correctness | Add a test case with minimum_height greater than 99 blocks below the tip | fixed · fix pushed · case added in push and acknowledged by reviewer | answered | yes | 2026-09-09: 'Could you add a case with `minimum_height` greater than 99 blocks below the tip?' Status from: 2026-09-15: molnard: 'Checked the code and ran the tests - all good.' |
| nervana21 (contributor) | suggestion, correctness | Check that the test also verifies leaving locktime unchanged at the tip | fixed · fix pushed · author added saw_tip_locktime check | answered | yes | 2026-09-09: 'Would it make sense to strengthen this test to check not only that we backdated but also check that we left the locktime unchanged?' Status from: 2026-09-09: nervana21: 'Thanks for taking the suggestions.' |
| molnard (none) | objection, correctness | Test assertion allows a 100-block backdate instead of capping at 99 blocks | fixed · fix pushed · fixed in push and acknowledged by reviewer | answered | yes | 2026-09-14: 'The BOOST_CHECK_GE below should also use block_height - 99; otherwise it allows a 100-block backdate.' Status from: 2026-09-15: molnard: 'Checked the code and ran the tests - all good.' |
| molnard (none) | suggestion, correctness | Remove the break statement and run all 200 iterations in the test | fixed · fix pushed · author removed break in push and reviewer ACKed | answered | yes | 2026-09-14: '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.' Status from: 2026-09-15: molnard: 'Checked the code and ran the tests - all good.' |
| achow101 (member) | suggestion, interface | Rename previous_locktime to minimum_locktime in CCoinControl | fixed · fix pushed · renamed in push | answered | yes | 2026-09-25: 'I suggest calling this `minimum_locktime` rather than `previous_locktime` as it's use could be for more than just dealing with replacements.' Status from: 2026-09-30: Bicaru20: 'As suggested, I changed the varible name from `m_previous_locktime` to `m_minimum_locktime`.' |
| achow101 (member) | suggestion, maintenance | Avoid discussing or referencing transaction replacements in DiscourageFeeSniping | fixed · fix pushed · Author updated comments to remove references to previous locktime in a6bc933b3f | answered | yes | 2026-09-25: 'Discussing replacements is unnecessary in this function. This does not need to care about what the callers are.' Status from: 2026-10-02: Bicaru20: 'In a6bc933b3f changed the commets so previous locktime is not metioned.' |
| achow101 (member) | suggestion, approach | Place the locktime logic inside the if (use_anti_fee_sniping) block | contested · achow101 pushed back that the logic should still be inside if (use_anti_fee_sniping) | answered | yes | 2026-09-25: 'This should be below inside of `if (use_anti_fee_sniping)`.' Status from: 2026-09-30: achow101: 'Should also be inside of `if (use_anti_fee_sniping)`' |
| achow101 (member) | suggestion, approach | Use functional tests instead of unit tests for wallet testing | fixed · fix pushed · author replaced unit test with functional test in wallet_bumpfee.py | answered | yes | 2026-09-25: 'This does not need to be a unit test. For wallet things, we prefer functional tests over unit tests wherever possible.' Status from: 2026-09-30: Bicaru20: 'Changed the `bump_transaction_fee_sniping_check` unit test to functional.' |
| polespinasa (member) | suggestion, correctness | Add functional test verifying a normal replacement never falls below its original height locktime | open | answered | no | 2026-10-06: 'Functional tests only test the stale tip branch, we should also test that a normal replacement never falls below its original height locktime.' |
| polespinasa (member) | suggestion, correctness | Add unit test cases for minimum_height == block_height for current and stale tips | open | answered | no | 2026-10-06: 'For unit tests it is testing a minimum one block below the tip, but what about `minimum_height == block_height`? That case can be added for current and stale tips.' |
| polespinasa (member) | suggestion, approach | Replace the if statement with an assert in feebumper.cpp since manual locktime is never set when replacing | open | answered | no | 2026-10-06: 'This if statement is unnecessary as we never set a manual locktime when replacing. So we should always store the locktime as the minimum one.' |
| polespinasa (member) | suggestion, maintenance | Clarify that m_minimum_locktime in CCoinControl is an optional anti-fee-sniping minimum that can be overwritten by explicit locktime | answered · author replied that std::optional already indicates it is optional and reviewers preferred not referencing anti-fee-sniping/replacements | answered | yes | 2026-10-06: 'This is not true at all, it is only an optional anti-fee-sniping minimum timestamp value. The comment seems to indicate it is always taken into account, but it can be overwritten by an explicit lockti' Status from: 2026-10-13: Bicaru20: 'I think it self explanatory that is an optional parametet given that it is a `std__optional`.' |
| polespinasa (member) | suggestion, maintenance | Update spend.h comment to specify best known tip since stale tips can backdate more than 99 blocks from the real tip | open | answered | no | 2026-10-06: 'Maybe we don't know the tip or we are stale and we backdate more than 99blocks from the real tip.' |
| polespinasa (member) | suggestion, safety | Use consistent types for block_height and minimum_height instead of mixing signed and unsigned values before casting | answered · author replied that static_cast to int is now used | answered | yes | 2026-10-06: 'block_height and minimum_height mixes signed and unsiged values before casting the result to int. I think using consistent types make that safety easier to verify.' Status from: 2026-10-13: Bicaru20: 'Now we use `static_cast` to pass the variable to `int` in the line 1029.' |
| polespinasa (member) | suggestion, maintenance | Fix comment range in wallet_bumpfee.py to 'a random 0-99 blocks' | answered · fix pushed · author updated the comment to 0-99 in push 76a55ec1d0 | answered | yes | 2026-10-06: 'The range in the comment is wrong, it should be `a random 0-99 blocks`.' Status from: 2026-10-13: Bicaru20: 'In the functional test I updeated the comment so the range is correct (0-99).' |
| polespinasa (member) | objection, correctness | Functional test fails to properly restore mocktime by using tip timestamp instead of node mocktime | answered · author explained saving time using int(time.time()) | answered | yes | 2026-10-06: 'This is not restoring the mocktime. You should not use the tip timestamp but the node mocktime.' Status from: 2026-10-14: Bicaru20: 'I couldn't do it with rbf_node.mocktime, so following examples from other tests I saved the time using `int(time.time())`' |
| vicjuma (contributor) | suggestion, style | Define a named constant for extra blocks created instead of magic number 11 in spend_tests.cpp | answered · author added comments explaining the 11 blocks instead of declaring a new variable | answered | yes | 2026-10-08: 'What is 11? :-) It might be even better if we know that 11 blocks are created on top of the current 100-block tip' Status from: 2026-10-13: Bicaru20: 'Instead of declaring anew variable, I updeted the code with some comments that explain the reason of the extra 11 blocks' |
Support:
- nervana21 (contributor) (Concept ACK, 2026-08-21): Concept ACK [not substantive]
- polespinasa (member) (Approach ACK, 2026-08-25): Approach ACK [not substantive]
- molnard (none) (Concept ACK, 2026-08-26): Concept ACK. [not substantive]
- molnard (none) (Concept ACK, 2026-09-09) [tested]: I ran all new and modified tests locally, and they passed.
- molnard (none) (ACK, 2026-09-14): ACK b72a0533dac88b46755f9ee1051eb588b1934050 with nits. The code looks good. [not substantive]
- molnard (none) (ACK, 2026-09-15) [both]: Checked the code and ran the tests - all good.
- nervana21 (contributor) (Tested ACK, 2026-09-16) [tested]: left minor, non-blocking nits [not substantive]
- molnard (none) (Tested ACK, 2026-09-21) [both]: Reviewed the code and ran all tests locally.
- nervana21 (contributor) (ACK, 2026-09-22): ACK 8836a3643bca8fce1185b0420f509dc75dae0a4a [not substantive]
- molnard (none) (Tested ACK, 2026-09-22) [tested]: tACK https://github.com/bitcoin/bitcoin/commit/8836a3643bca8fce1185b0420f509dc75dae0a4a [not substantive]
- vicjuma (contributor) (Concept ACK, 2026-10-08): Concept ACK [not substantive]
Participants:
- nervana21 (contributor, objection, 17 statements 2026-08-21 to 2026-09-22): Raised off-by-one and stale chain locktime concerns, later ACKed
- polespinasa (member, objection, 16 statements 2026-08-25 to 2026-10-06): Suggested code simplifications, test mocktime fix, and test coverage improvements; gave Approach ACK
- molnard (none, objection, 20 statements 2026-08-26 to 2026-09-22): Raised stale chain locktime, reorg handling, CCoinControl coupling, and test flakiness concerns, later tested ACKed
- danielabrozzoni (member, neutral, 1 statements 2026-08-28 to 2026-08-28): Discussed stale chain locktime privacy trade-offs
- achow101 (member, neutral, 7 statements 2026-09-25 to 2026-09-30): Requested moving logic inside use_anti_fee_sniping, switching to functional tests, and removing replacement references
- vicjuma (contributor, support, 2 statements 2026-10-08 to 2026-10-08): Concept ACKed and suggested defining a named constant for extra blocks in spend_tests.cpp
Files
137 lines under test/bench/ci.
- src/wallet/test/spend_tests.cpp +91/-0
- test/functional/wallet_bumpfee.py +46/-0
- src/wallet/spend.cpp +18/-6
- src/wallet/spend.h +5/-2
- src/wallet/feebumper.cpp +3/-0
- src/wallet/coincontrol.h +2/-0
Card
The short text the ranking pass compares across PRs.
This PR modifies wallet fee bumping to ensure a replacement transaction does not receive an nLockTime earlier than the transaction it replaces. Anti-fee-sniping randomly backdates locktimes by up to 99 blocks roughly 10% of the time, which can cause bumpfee to produce a replacement with a lower locktime than the original. This regression acts as an on-chain fingerprint identifying Bitcoin Core and Electrum wallets. An empirical survey cited in the PR found 200 to 300 backdated replacements per week out of over one million RBF transactions.
Data
dossier JSON · extract JSON · model ledger, generated 2026-10-15T01:18, confidence high, input hash 445bd610093734aa
thread read through 2026-10-14 (99 statements), head ec9199610b
labels: Wallet
thread read through 2026-10-14 (99 statements), head ec9199610b
labels: Wallet
Categories
Wallet
P3 · bug fix
Prevents unnatural locktime backdating on fee-bumped transactions that leaks an on-chain wallet fingerprint. When bumping fees, anti-fee-sniping roll can randomly set a locktime earlier than the original transaction (estimated at 200–300 occurrences per week), an edge-case privacy flaw noted in #26526.
Membership: Matches 'Wallet' label and modifies wallet transaction creation and fee-bumping logic in `src/wallet/spend.cpp` and `src/wallet/feebumper.cpp`.
Factors: security/stability 1, bug 1, performance 0, user value 1, leverage 0