Weekly cherry pick#46
Merged
Merged
Conversation
…QUE_NAME fba713a scripted-diff: Rename UNIQUE_NAME to BITCOIN_UNIQUE_NAME (Hennadii Stepanov) Pull request description: bitcoin#34454 (comment): > ... it is annoying that we keep running into the same bug over and over again (IIRC it happened in the past at least once for Bitcoin Core). Surely this is going to happen again in the future. And here we go again. --- The `nb30.h` Windows header [defines](https://github.com/mingw-w64/mingw-w64/blob/b536c4fdb038a9c59a7e5fb36e7d1293c4dc61d6/mingw-w64-headers/include/nb30.h#L78) `UNIQUE_NAME` as a macro. This introduces a fragile dependency on header inclusion order: if Windows headers happen to be included before `UNIQUE_NAME` is used, the preprocessor expands it into a numeric literal, causing syntax errors. Rename the macro to `BITCOIN_UNIQUE_NAME` to remove this fragility and avoid the collision entirely. --- Noticed while doing a Guix build of the [QML repo](https://github.com/bitcoin-core/gui-qml) for Windows. Recent similar PRs: bitcoin#34454 and bitcoin#34868. ACKs for top commit: maflcko: lgtm ACK fba713a sedited: ACK fba713a w0xlt: ACK fba713a Tree-SHA512: 7a63b99a754e797eb8fa5d6a598606150f47ae1130d1d26067c509830e6575f0378ce63fe0ca35c69dce9a394451a34ddadd8b3d5f6f9a7e4c529108af546fb6 (cherry picked from commit 9868e1b)
…adcastToAll fa2afba p2p: Release m_peer_mutex early in InitiateTxBroadcastToAll (MarcoFalke) Pull request description: The `InitiateTxBroadcastToAll` method holds the `m_peer_mutex` while updating the bloom filters for all peers. This is perfectly fine, because updating the bloom filters is fast. Though, from a style-perspective, the lock does not need to be held for the whole function. Also, holding the lock longer, may confuse Tsan into a lock-order inversion false-positive (ref: bitcoin#19303 (comment)). So "fix" both issues in this style-refactor. ACKs for top commit: xyzconstant: Code review ACK fa2afba shuv-amp: ACK fa2afba danielabrozzoni: Code Review ACK fa2afba sedited: ACK fa2afba Tree-SHA512: c47849a4c3cc11c74b61fec3425db8ec7f78db4ca43d7bf3145ce640f7b0872701c09495f0dfe77109d09d5716d920ad3d7308483fe41564c30867b3e80432e7 (cherry picked from commit 17ed7f5)
087f02c ci: skip libunwind runtime in LLVM build (fanquake) 6d47f7c ci: use llvm 22.1.7 (fanquake) Pull request description: Also document why we use `LIBCXXABI_USE_LLVM_UNWINDER=OFF`. Upstream issue is llvm/llvm-project#84348. ACKs for top commit: maflcko: lgtm ACK 087f02c sedited: ACK 087f02c Tree-SHA512: b93c798fd5a016cad40db9d24cb36cb72e531b284aee5458de41e062960514783e30c6f1413c0e62fa261758d783d0004a0973541cbb36bd34b77800c629bd7a (cherry picked from commit 543c00f)
54de023 guix: add setup.sh (fanquake) Pull request description: This is the first change in bitcoin#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 (cherry picked from commit 4b91316)
…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 (cherry picked from commit c85e04f)
… ChainCode 21a1380 key: cleanse ChainCode on destruction (Thomas) b3a3f88 crypto: cleanse HMAC stack buffers after use (Thomas) Pull request description: `CHMAC_SHA256` and `CHMAC_SHA512` leave two stack buffers populated on return: `rkey[]` holds `K' ⊕ ipad` after the constructor, and `temp[]` holds the inner-hash output after `Finalize()`. When the HMAC is keyed with sensitive material (chain code in `BIP32Hash()` in `hash.cpp` for BIP32 child key derivation; PRK in HKDF-Expand in `hkdf_sha256_32.cpp`, used for BIP324 transport keying), `rkey` is one constant XOR from that key, and `temp` is a one-way digest covering it. This PR cleanses both buffers with `memory_cleanse()`, matching the convention already used in `chacha20.cpp` and `chacha20poly1305.cpp`. No observable change for callers. Update: Cleansing the HMAC primitive's internal buffers still leaves a caller's `ChainCode` value populated in memory after use. The second commit promotes `ChainCode` from `typedef uint256` to a `base_blob<256>` subclass with a `memory_cleanse()` destructor, so chain codes in `CExtKey`, `CExtPubKey`, and local variables are cleansed on scope exit. `MUSIG_CHAINCODE` is retyped from `constexpr uint256` to `const ChainCode` to match its BIP328 semantic role; this also removes the GCC-14 consteval lambda workaround. ACKs for top commit: davidgumberg: crACK bitcoin@21a1380 optout21: ACK 21a1380 achow101: ACK 21a1380 winterrdog: ACK 21a1380 Tree-SHA512: 022c8372da3e2c9c269ef55b695d8415241acf64be04692f30da0e682dd1d05178f95601a3bd208573fd0630656b3dedcf6de34a2a3cf794515c0268e710af75 (cherry picked from commit 2880181)
…tionRef a9301cf refactor: disable default std::hash for CTransactionRef (Sjors Provoost) 47d68cd ci: backport iwyu PR 2013 std::hash mapping (Sjors Provoost) Pull request description: While working on bitcoin#33922 I initially forgot to add `CTransactionRefComp` to the `std::unordered_map<CTransactionRef` defined there. This PR turns that into a compiler error. See bitcoin#33922 (comment) This change triggers a false positive IWYU error, and an inconsistent one at that: our CI wants `<variant>`, while a manual build on Ubuntu (version 0.26 with clang version 22.1.1) wants `<string_view>`. Various workarounds were discussed in: - include-what-you-use/include-what-you-use#2007 - bitcoin#35073 - bitcoin#33922 (comment) Addressed by back-porting: - include-what-you-use/include-what-you-use#2013 ACKs for top commit: achow101: ACK a9301cf vasild: ACK a9301cf w0xlt: lgtm ACK a9301cf Tree-SHA512: 11b3f8698e66457a14d1c16f55cac3ee17a572e9c1c98f5aa9c8a8f1e8928246b1676f7544f2819bc16ec4dcf025585b9cbd60ab3f20839d3d9bcc65ee9e7c0f (cherry picked from commit 3bbc3c6)
…o::nScore 2189a6f p2p: Saturate LocalServiceInfo::nScore updates at INT_MAX (codeabysss) Pull request description: The overflow for signed arithmetic yields undefined behavior. This changes prevents undefined behavior in local address scoring by saturating `nScore` updates at `INT_MAX` in both `SeenLocal()` and `AddLocal()` update paths. Fixes: bitcoin#24049. ACKs for top commit: Crypt-iQ: ACK 2189a6f pending CI achow101: ACK 2189a6f sedited: ACK 2189a6f Tree-SHA512: b861e58ec9d6e18b17768f5cbee31ee825717e1a7216c332eb6fcbe63a7ac24e213ba638aea6f03cb710d9c2d8fe736cc626f11011ed66c3938acf6c38b0ef2a (cherry picked from commit 53b836c)
f6bdbcf lint: Grep for `AUTO` test suites in file names (rustaceanrob) Pull request description: Tests without a fixture did not have their file names linted because the grep matches on `BOOST_FIXTURE`. Updates to match `BOOST_FIXTURE` or `BOOST_TEST`. ACKs for top commit: l0rinc: ACK f6bdbcf achow101: ACK f6bdbcf hebasto: ACK f6bdbcf. Tree-SHA512: dd1763b6ac90fa87b7e0d2faa56d1c7beedb1e2d37d16367c60ebcadd155f5955113fff7cf5c0ce5eaa9e63aeeb67ffff2c8e081f7c23978cb072207f072f2ef (cherry picked from commit 809f909)
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 (cherry picked from commit e0fb41f)
…ance 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 (bitcoin#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 (cherry picked from commit d0b8d44)
… assignment 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 (cherry picked from commit 3b712b9)
3be1115 ci: Alpine 3.24 (fanquake) Pull request description: Switch to [Alpine 3.24](https://www.alpinelinux.org/posts/Alpine-3.24.0-released.html) in the Alpine CI job. ACKs for top commit: maflcko: lgtm ACK 3be1115 sedited: ACK 3be1115 Tree-SHA512: 735a193a3511da611ac3399a66917cf69d5f895c39e50667c1293f1e449795ae85bb5a9686ce28928f73547369b036d091e1cf7b523854652a10ec7cf529d1d3 (cherry picked from commit c117bbc)
…mutex ec6cf49 blockstorage: Remove cs_LastBlockFile recursive mutex (sedited) Pull request description: The `cs_LastBlockFile` mutex is redundant: all critical sections are already covered by cs_main. This is demonstrated in this patch by replacing all instances of locking `cs_LastBlockFile` with pairs of `AssertLockHeld(::cs_main)` and `EXCLUSIVE_LOCKS_REQUIRED(::cs_main)` annotations. No additional `::cs_main` LOCK(...)s are introduced (besides for test-only code). It is also not clear for which sections `cs_LastBlockFile` is responsible for. It is annotated for `m_blockfile_cursors`, but sporadically and inconsistently also covers `m_blockfile_info` (e.g. in `LoadBlockIndexDB`). Since it has no semantic meaning, and seems confusing to developers, remove it. An alternative to this patch would be expanding the scope of what `cs_LastBlockFile` covers and turning it into a non-recursive mutex. I prepared such a patch some time ago, but found it unsatisfactory. It was not clear to me if the lock was now covering too much or too little, and its purpose remained unclear. If this patch is accepted, I would expect the project to eventually implement a separate, narrowly-scoped block storage lock to allow for a more parallelizable block processing routine. ACKs for top commit: stickies-v: re-ACK ec6cf49 janb84: re- ACK ec6cf49 pablomartin4btc: ACK ec6cf49 Tree-SHA512: e5942bc87300b0db9a0b91d5fe26dab455049e6cef7c96bb12b28141fa04711d46c6af105c0e1a83a9f261edde2c8b8b43ecf577a27d54b4610d784676a85627
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 bitcoin#34858 Updates remaining `SetMockTime` call sites that are clean, mechanical swaps fitting the spirit of the original PR (see: bitcoin#34858 (review) and bitcoin#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: bitcoin#34858 (comment)) and can be safely removed after updating the only test where this is load-bearing (b3c9bd7) (see: bitcoin#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 bitcoin#34858 (review) and supported in bitcoin#34858 (comment)). ACKs for top commit: maflcko: re-ACK 35a814a 🛒 sedited: ACK 35a814a Tree-SHA512: ade776e288a4b7bbc4c8855c14d61381b5b20329fe1e72fee87f773e47a9519975d58c277fbacda37dd73c0c1d4ce358c92dcdc4ca049d58cb3453ddf751b45b
…_unlinked` on startup 3f44f9a test: Add coverage for m_blocks_unlinked invariant in LoadBlockIndex (marcofleon) 0e4b0ba validation: Don't add pruned blocks to m_blocks_unlinked on startup (marcofleon) Pull request description: Fixes bitcoin#35050 The `m_blocks_unlinked` map keeps track of blocks that have transactions but whose parent (or any ancestor) does not. This happens when a block is received before its parent, or during a reorg, when `FindMostWorkChain()` encounters a block whose ancestors were pruned. The bug this PR addresses is a rare interaction of these two cases, which happens on startup when `BlockManager::LoadBlockIndex()` rebuilds `m_blocks_unlinked`. The check there only considers whether a block has transactions, and pruned blocks keep `nTx > 0` but clear `BLOCK_HAVE_DATA`. So if there's a pruned block on a stale fork whose parent has no transactions, that block is added to `m_blocks_unlinked` without having data on disk. This violates an [assertion](https://github.com/bitcoin/bitcoin/blob/ad3f73862bdbee0aac106fa9e08c4181ce78ba47/src/validation.cpp#L5352) in `CheckBlockIndex()`. Get rid of this unintended case by gating on `BLOCK_HAVE_DATA` before adding to `m_blocks_unlinked`. ACKs for top commit: achow101: ACK 3f44f9a sedited: Re-ACK 3f44f9a stratospher: ACK 3f44f9a. nice! Tree-SHA512: 275d0f8588524c01c4e701c8635973cd4a086d31c10d252a498c1ef668bdb3895ba1cae265dbe88f8983ca7ddbe32247824753c7c1f49e59c8bce0df377b784c
da74ff9 test: Add functional test for BIP434 (Fabian Jahr) 01b8a11 test_framework: BIP 434 support (Anthony Towns) 6a12998 BIP434: FEATURE message support (Anthony Towns) 3210fc4 net: Add AdvertisedVersion() for protocol version advertised to a peer (Anthony Towns) 94ed454 serialize: add LimitedVectorFormatter (Anthony Towns) 1b3f776 serialize: string_view serialization (Anthony Towns) Pull request description: Adds support for [BIP 434](https://github.com/bitcoin/bips/blob/master/bip-0434.md). ACKs for top commit: fjahr: ACK da74ff9 pseudoramdom: ACK da74ff9 achow101: ACK da74ff9 darosior: ACK da74ff9 w0xlt: reACK da74ff9 sedited: ACK da74ff9 Tree-SHA512: 74aa01b9b296a1a498b3aa119af6db906453f0809ec7ae271fc26690491c3f5677bf2cd03817caf9e287f5b3bc977768cdfefbe74ed2dd0da1cd339e043fe010
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 bitcoin#35114. sedited: ACK fa03852 w0xlt: ACK fa03852 Tree-SHA512: 4d4f1ff170ce8cfa606a6dc0dc47ff8b89e2db0d0188daeabbbaa422df00143fd99426905adcff1fd152a00ebf8a9004e312cbc77a1d078e57895d5f1334a3b2
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
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
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.
No description provided.