2026 dont backdate locktime rbf#2
Closed
Bicaru20 wants to merge 6 commits into
Closed
Conversation
CheckGlobalsImpl's constructor runs at the start of every fuzz iteration and already resets the global RNG flags and the mockable NodeClock via SetMockTime(0s), but it never reset the mockable steady clock. A value written to g_mock_steady_time by one input therefore leaked into the next one. For example, FuzzedSock's constructor calls SetMockTime(INITIAL_MOCK_TIME) and never clears it, so the mocked steady time stays set for all subsequent iterations. Reset MockableSteadyClock symmetrically with NodeClock so each input starts from an unmocked steady clock. This also brings the steady clock under the same discipline as the system clock: a target that reads MockableSteadyClock::now() without first mocking it is now caught by the existing g_used_system_time check instead of silently reusing a leaked value.
…erations 19b32a2 fuzz: reset the mockable steady clock between iterations (Hao Xu) Pull request description: Fix the issue mentioned by bitcoin#29018 (comment) And this is my investigation on it: bitcoin#29018 (comment) `CheckGlobalsImpl`'s constructor runs at the start of every fuzz iteration and already resets the global RNG flags and the mockable `NodeClock` (`SetMockTime(0s)`), but it never reset the mockable steady clock. A value written to `g_mock_steady_time` by one input therefore leaks into the next iteration. The most common source is `FuzzedSock`'s constructor, which calls `SetMockTime(INITIAL_MOCK_TIME)` (through `ElapseTime(0s)`) and never clears it: once any input constructs a `FuzzedSock`, the steady clock stays mocked for every subsequent iteration in the same process. This is one of the global-state leaks tracked in bitcoin#29018. ### Fix Reset `MockableSteadyClock` symmetrically with `NodeClock`: ```diff g_used_system_time = false; SetMockTime(0s); +MockableSteadyClock::ClearMockTime(); ``` Besides removing the leak, this puts the steady clock under the same discipline as the system clock: a target that reads `MockableSteadyClock::now()` without first mocking it (via `FuzzedSock`, `SteadyClockContext`, …) is now caught by the existing `g_used_system_time` check at the end of the iteration, instead of silently reusing a value left over from a previous input. Clearing in `~FuzzedSock()` would be wrong: several `FuzzedSock`s can be alive simultaneously (e.g. `process_messages` adds 1–3 peers), so clearing in one destructor would corrupt the mock observed by the others. Resetting at the iteration boundary keeps it decoupled from socket lifetimes. ### Testing Verified with the global-state-detector approach from bitcoin#29018 (snapshotting/diffing the writable globals around each iteration): - **Before:** a single empty input to `process_message` reports `g_mock_steady_time` changing `00 → 01` (`0` → `INITIAL_MOCK_TIME`). - **After:** that report is gone; the only remaining diffs are the benign one-time initialization of `ConsumeTime`'s function-local statics. `p2p_headers_presync` (uses `SteadyClockContext`) and `pcp_request_port_map` (uses `FuzzedSock`) still run to `succeeded` without aborting, confirming existing steady-clock readers are unaffected. This leak is invisible to coverage-based checks such as `deterministic-fuzz-coverage`, because `g_mock_steady_time` is only consumed through coarse time comparisons (e.g. the 250 ms presync rate-limiter): a changed value doesn't change the executed branches, so only a memory-diffing detector can see it. ACKs for top commit: maflcko: lgtm ACK 19b32a2 marcofleon: Nice catch, ACK 19b32a2 Tree-SHA512: b875795addb2914eae489adc703438483f8e464b9a210bd5d76189f13266dae5843c8749590d59e78bf171f19aa7cee21ca678cd311843d8a88cbe9831f20b6a
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-
If when replacing a transaction, the original has a time-based locktime, do not anti-fee snipe as the locktime type would change, making the anti-fee sniping very identifiable.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Dont backadate the locktime when replacing a transaction