[pull] master from bitcoin:master#1681
Merged
Merged
Conversation
Currently, all test cases using TestChain100Setup or a derived class like BuildChainTestingSetup are using mocktime by default due to the SetMockTime call in the TestChain100Setup ctor. This is confusing, because test cases using mocktime explicitly seem to imply that before they set the mocktime, real time was used. E.g. index_reorg_crash claimed in a comment to "Enable mock time". Fix this issue by adding a FakeNodeClock m_clock field to TestChain100Setup. Then, use the m_clock instead of explicit calls to SetMockTime or to a (now) shadowing local FakeNodeClock variable.
The context is easier to reason about: E.g., * in TestBasicMining it allows to drop manual SetMockTime(0) calls, * in connections_desirable_service_flags it allows to drop manual calls to SetMockTime(GetTime<std::chrono::seconds>() + _n_) and replace them by operator+=(_n_) * in wallet_tests it clarifies that the mocktime does not persist outside the AddTx function
This is easier to reason about, because it will automatically take care of properly setting INITIAL_MOCK_TIME in the ctor. Also, it allows to drop the ElapseTime and replace it with a call to operator+=()
Their clang version is pinned, so the only relevant change should be a more recent cmake, more recent C++ stdlib, and more recent Python version.
Now that the C++ std-lib was bumped in the CI task in the prior commit, some unused includes can (and must) be dropped.
Also assert that the availability of the satisfaction weight estimate does not depend on the signature-size assumption, and that assuming non-max-size signatures never increases the estimate.
fa03852 test: Use SteadyClockContext in pcp_tests (MarcoFalke) fa3716c test: Use FakeNodeClock in more places (MarcoFalke) fae9623 test: Add FakeNodeClock m_clock to TestChain100Setup (MarcoFalke) Pull request description: This switches the remaining cases in the unit tests from `SetMockTime` to `FakeNodeClock` for clarity, as explained in the commit messages. ACKs for top commit: frankomosh: crACK fa03852. This PR continues the FakeNodeClock migration from #35114. sedited: ACK fa03852 w0xlt: ACK fa03852 Tree-SHA512: 4d4f1ff170ce8cfa606a6dc0dc47ff8b89e2db0d0188daeabbbaa422df00143fd99426905adcff1fd152a00ebf8a9004e312cbc77a1d078e57895d5f1334a3b2
526aae3 fuzz: test non-max descriptor satisfaction weight (woltx) Pull request description: The descriptor fuzz target is intended to exercise descriptor satisfaction-size estimation for solvable descriptors. It currently calls `MaxSatisfactionWeight(true)` twice, so the `false` branch is never exercised. This PR changes `max_sat_nonmaxsig` to call `MaxSatisfactionWeight(false)`, so fuzzing covers both branches. ACKs for top commit: brunoerg: reACK 526aae3 sedited: ACK 526aae3 Tree-SHA512: 029750d76c1d50f5c6a008b826a0a2dc187feb420be96401d2e15747b44901341d32ac75e86a5e10585919d419c607800d8e117a1cbce50b1db40121d3610f9c
1ce9e26 fuzz: improve dbwrapper_concurrent_reads performance (Andrew Toth) Pull request description: The recently merged fuzz harness targeting concurrent reads suffers from poor performance and memory leaks (#34866 (comment)). Fix this by - using a global thread pool instead of a local one per iteration - reduce thread count to 8 from 16 - use a std::map oracle to check results inline instead of reading from the db to get a baseline and storing results ACKs for top commit: marcofleon: reACK 1ce9e26 l0rinc: ACK 1ce9e26 sedited: ACK 1ce9e26 Tree-SHA512: 2e532caf246f389105e4a9b487496386d1fe9add7b27fba9ecbbf51a432ef493765ad7095288dd7e0a896860ff150d89ecb6afb8baf311a4af94d8e01b77dba5
fab5228 refactor: Drop unused includes after iwyu CI bump (MarcoFalke) fa4774d ci: Bump APT_LLVM_V-based task configs to Ubuntu 26.04 (MarcoFalke) fa1414a ci: Debian Trixie -> Ubuntu 26.04 (MarcoFalke) Pull request description: This is for the upcoming 32.x, because I presume users and devs are more likely using a later distro. This comes with tool bumps, such as: * GCC 14 -> 15 (https://packages.debian.org/trixie/g++ -> https://packages.ubuntu.com/resolute/g++) * Clang 19 -> 21 * Cmake 3.31 -> 4.2 * Valgrind 3.24 -> 3.26 ACKs for top commit: l0rinc: code review ACK fab5228 hebasto: re-ACK fab5228. Tree-SHA512: 9d5be2f5b15cf7904c50687ce5e8cceeb2f740c7d5180190d6a10e751998ce2c2156098f89352eac49f24c8cd9ab55b78321e310240ac829dcbe48b576b6240c
The defaulted move assignment overwrites m_conn without disconnecting
it first, so the previous callback stays registered with the signal and
keeps firing, violating the RAII contract:
btcsignals::scoped_connection sc0 = sig.connect(IncrementCallback);
btcsignals::scoped_connection sc1 = sig.connect(SquareCallback);
sc0 = std::move(sc1);
val = 3; sig(val); // both callbacks fire: 16 instead of 9
Move assignment is unused in the codebase, so delete it rather than
fixing it. It can be implemented properly if a use case arises.
…ment b83a999 btcsignals: delete broken scoped_connection move assignment (Thomas) Pull request description: The defaulted move-assignment operator for `scoped_connection` overwrites `m_conn` without first calling `disconnect()`. Since disconnection is signaled via the liveness flag (which is never cleared) the old callback remains registered in the signal and keeps firing, violating the RAII contract: ```cpp int val{0}; btcsignals::scoped_connection sc0 = sig.connect(IncrementCallback); btcsignals::scoped_connection sc1 = sig.connect(SquareCallback); sc0 = std::move(sc1); val = 3; sig(val); // Expected: 9 (only SquareCallback) // Actual: 16 (both callbacks fire, old connection leaked) ``` Move assignment is unused in the codebase, so following the review discussion this deletes the broken operator instead of fixing it. A correct implementation can be added if a use case arises. Earlier versions of this PR fixed the operator and added a default constructor to enable the member-variable assignment pattern; that was dropped in favor of removing the unused operation. ACKs for top commit: maflcko: lgtm ACK b83a999 sedited: ACK b83a999 Tree-SHA512: 794347e9cb868d50957ea298f7df6eac5b9f55b9d35ab09e41be269923c45e0709194431dea66b7977c74f802150ba53cb2d12d35937f4966ec302bffb9c95f8
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.
6 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 : )