[pull] master from bitcoin:master#1679
Merged
Merged
Conversation
Cherry-pick include-what-you-use commit 52f85e1f4d99 onto the clang_22 branch tracked by ci/test/00_setup_env_native_iwyu.sh, fixing the false positive where std::hash was mapped to <string_view>/<variant> instead of its real provider (<functional>, <memory>, etc).
The default std::hash for shared_ptr compares by pointer. CTransactionRefHash or a custom hasher should be used instead. Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
LoadBlockIndex() adds to m_blocks_unlinked based only on nTx > 0, without checking BLOCK_HAVE_DATA. Pruning preserves nTx but clears BLOCK_HAVE_DATA, so a pruned block whose parent was header-only gets re-added on every restart, causing the CheckBlockIndex() assertion that entries must have data on disk to fail. Check that BLOCK_HAVE_DATA is set before inserting into m_blocks_unlinked. Fixes #35050.
CHMAC_SHA256 and CHMAC_SHA512 leave two stack buffers populated on return: rkey[] holds K' XOR 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. Cleanse both buffers with memory_cleanse(), matching the convention in chacha20.cpp and chacha20poly1305.cpp. No observable change for callers.
A pruned stale-fork block whose parent doesn't have any transactions shouldn't be added to m_blocks_unlinked when starting up a node.
HMAC primitives cleanse their internal stack buffers, but a caller's ChainCode remains populated in memory after use. Promote 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. Retype MUSIG_CHAINCODE from `constexpr uint256` to `const ChainCode` to match its BIP328 semantic role. Dropping `constexpr` (ChainCode is no longer a literal type) also removes the GCC-14 consteval lambda workaround. Remove the duplicate typedef in pubkey.h (which includes hash.h transitively). Two fuzz-test call sites in test/fuzz/key.cpp now construct the chain-code argument explicitly rather than relying on the typedef.
Signed overflow on nScore updates is undefined behavior. Use SaturatingAdd in AddLocal() and SeenLocal() so increments saturate at INT_MAX instead of overflowing. Add unit test coverage for saturation in both code paths.
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.
19b32a2 fuzz: reset the mockable steady clock between iterations (Hao Xu) Pull request description: Fix the issue mentioned by #29018 (comment) And this is my investigation on it: #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 #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 #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
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 21a1380 optout21: ACK 21a1380 achow101: ACK 21a1380 winterrdog: ACK 21a1380 Tree-SHA512: 022c8372da3e2c9c269ef55b695d8415241acf64be04692f30da0e682dd1d05178f95601a3bd208573fd0630656b3dedcf6de34a2a3cf794515c0268e710af75
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 #33922 I initially forgot to add `CTransactionRefComp` to the `std::unordered_map<CTransactionRef` defined there. This PR turns that into a compiler error. See #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 - #35073 - #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
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: #24049. ACKs for top commit: Crypt-iQ: ACK 2189a6f pending CI achow101: ACK 2189a6f sedited: ACK 2189a6f Tree-SHA512: b861e58ec9d6e18b17768f5cbee31ee825717e1a7216c332eb6fcbe63a7ac24e213ba638aea6f03cb710d9c2d8fe736cc626f11011ed66c3938acf6c38b0ef2a
…ed` 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 #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
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.
7 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 : )