[pull] master from bitcoin:master#1678
Merged
Merged
Conversation
Split out setup & tarball creation code, so that it can be re-used by build scripts.
This is a follow-up to commit faad08e. Hoisting the NodeClockContext above peer creation ensures m_connected is captured under mocktime, making the MINIMUM_CONNECT_TIME check deterministic regardless of which peer is selected for eviction. This is a prerequisite for the next commit, which removes the one-second advance from the NodeClockContext default constructor.
The increment was originally added so that mocked time would not appear to go backward relative to real-clock timestamps captured before construction, since Now<NodeSeconds>() rounds the current time down to a whole second. In practice the tests do not mix real and mocked timestamps in a way that exposes this, so the increment is unnecessary.
This refactor is a follow-up to commit faad08e and does not change any behavior. These call sites are clean mechanical swaps. The remaining ones require non-trivial test refactoring and are left for future follow-ups.
The previous name did not indicate the type was intended for testing. Renaming to FakeNodeClock makes this explicit and allows call sites to drop the ctx suffix on the variable name. Suggested in #34858 review feedback. -BEGIN VERIFY SCRIPT- s() { git grep -l "$1" -- src | xargs sed -i "s/$1/$2/g"; } s '\<NodeClockContext\>' 'FakeNodeClock' s '\<clock_ctx\>' 'clock' -END VERIFY SCRIPT-
SteadyClockContext and FakeNodeClock assume they are the only active instance. Overlapping them in the same scope would silently clobber each other. Add a CRTP base class, LimitOne, that asserts at construction if another instance already exists.
35a814a test: Limit clocks to one active instance (MarcoFalke) 55e402f scripted-diff: Rename NodeClockContext to FakeNodeClock (seduless) 1e9546f test: Use NodeClockContext in more call sites (seduless) 758fea5 test: Drop ++ from NodeClockContext default constructor (seduless) 7c2ec39 test: Enter mocktime before peer creation in block_relay_only_eviction (seduless) Pull request description: Follow-up to #34858 Updates remaining `SetMockTime` call sites that are clean, mechanical swaps fitting the spirit of the original PR (see: #34858 (review) and #34858 (comment)). Further updates to `SetMockTime` are more complex and deserve separate, isolated PRs. The default constructor for `NodeClockContext` increments to the next tick, which is a defensive measure to prevent time going backwards on construction. This has caused some confusion (see thread: #34858 (comment)) and can be safely removed after updating the only test where this is load-bearing (b3c9bd7) (see: #34858 (comment)). The removal also tightens the `addrman_tests/addrman_evictionworks` test to sit exactly on the `ADDRMAN_REPLACEMENT` boundary (4h), catching mutations such as: ```diff diff --git a/src/addrman.cpp b/src/addrman.cpp index d3dae59..d0929c62cb 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -920,3 +920,3 @@ void AddrManImpl::ResolveCollisions_() // Has successfully connected in last X hours - if (current_time - info_old.m_last_success < ADDRMAN_REPLACEMENT) { + if (current_time - info_old.m_last_success <= ADDRMAN_REPLACEMENT) { erase_collision = true; ``` The last follow-up item is updating `NodeClockContext` to `FakeNodeClock` to make it clear it is intended for testing (motivated by #34858 (review) and supported in #34858 (comment)). ACKs for top commit: maflcko: re-ACK 35a814a 🛒 sedited: ACK 35a814a Tree-SHA512: ade776e288a4b7bbc4c8855c14d61381b5b20329fe1e72fee87f773e47a9519975d58c277fbacda37dd73c0c1d4ce358c92dcdc4ca049d58cb3453ddf751b45b
54de023 guix: add setup.sh (fanquake) Pull request description: This is the first change in #25573, which splits out the setup & tarball generation code from `build.sh`, so that it can be re-used, from multiple (future) build scripts. ACKs for top commit: willcl-ark: ACK 54de023 hebasto: ACK 54de023. Tree-SHA512: 9a7f2fe322d281b9867414511af5243f4dd659ea42637f4eb8cc0c8629c94dab842669bb7c503f9fa67cab3fac65561364f07b5c0fda8e6d8c24e7bf161025ef
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 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.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )