Weekly cherry pick#41
Merged
Merged
Conversation
a7f9bdc611 Merge bitcoin-core/leveldb-subtree#52: Revert "Increase maximum read-only mmap()s used from 1000 to 4096 on 64-bit systems" a2f531d2d0 Merge bitcoin-core/leveldb-subtree#60: util: use [[fallthrough]] directly 1a166221cf Merge bitcoin-core/leveldb-subtree#61: Disable seek compaction 6bfdb6093b Disable seek compaction 42a5f29aa9 util: use [[fallthrough]] directly c274b50867 Merge bitcoin-core/leveldb-subtree#53: refactor: Delete unused `ScopedHandle:operator=(ScopedHandle&&)` 68740f586f Merge bitcoin-core/leveldb-subtree#59: Fix Clang `-Wconditional-uninitialized` warning 70142e186b Merge bitcoin-core/leveldb-subtree#55: build: Require C++17 d123cf5a83 Effectively, this change 31361bf339 Allow different C/C++ standards when this is used as a subproject. 0711e6d082 Fix Clang `-Wconditional-uninitialized` warning 85665f9547 refactor: Delete unused `ScopedHandle:operator=(ScopedHandle&&)` fd8f69657e Revert "Increase maximum read-only mmap()s used from 1000 to 4096 on 64-bit systems" git-subtree-dir: src/leveldb git-subtree-split: a7f9bdc6114fe6eeb848fda2980fe61d86ff2045
No-longer needed after bitcoin-core/leveldb-subtree#60.
…256 for GCC (matching Clang) fedeff7 crypto: disable ASan instrumentation of SSE4 SHA256 for GCC (deadmanoz) Pull request description: Fix the runtime crash described in bitcoin#34881. Upstream already disables ASan instrumentation for `sha256_sse4::Transform()` under Clang. This extends the same workaround to GCC by adding an `#elif` branch for `__GNUC__` / `__SANITIZE_ADDRESS__` that applies the same `no_sanitize("address")` attribute. Testing: - reproduced the crash before the fix with GCC 13, 14, and 15 on Haswell-class machines / guests without SHA-NI (including by forcing the SSE4 implementation on GitHub CI) - SEGV in debug builds regardless of optimization level (tested `-O0`, `-O1`, `-O2`, `-O3`) - verified that the GCC + ASan debug configurations that previously crashed pass with this change Issue bitcoin#34881 has more details about the issue. Note: the original Clang code placed the `__attribute__` between the function declarator and the opening brace. GCC's [Attribute Syntax](https://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html) documentation notes that this position in a function definition "may, in future, be permitted," so it is not currently supported. Placing the attribute at the start of the function definition is valid form for both GCC and Clang. ACKs for top commit: maflcko: lgtm ACK fedeff7 🏒 sedited: tACK fedeff7 Tree-SHA512: d8adda0df140b6c93d18f5ecd096b12012332bb640e678075e668122e596baddcb2182cbaeafe7908ada90b4b5cc776a59dcdfb488229ab6640058ccbbd7ea93 (cherry picked from commit 1ea532e)
…ements 5338877 guix: Remove redundant ShellCheck `source` directives (Hennadii Stepanov) 62cf7bc guix, refactor: Add `BASE` argument to `*_for_host` functions (Hennadii Stepanov) 5d46429 guix, refactor: Move `distsrc_for_host()` to `prelude.bash` (Hennadii Stepanov) cab65ea guix, refactor: Move duplicated `profiledir_for_host()` to `prelude.bash` (Hennadii Stepanov) faa9d43 guix, refactor: Move duplicated `outdir_for_host()` to `prelude.bash` (Hennadii Stepanov) 6b59fd6 guix, refactor: Remove `contains()` function (Hennadii Stepanov) d4c69a7 guix, refactor: Remove unused `out_name()` function (Hennadii Stepanov) Pull request description: While working on bitcoin#35098, I reviewed my notes regarding a few minor Guix script flaws and decided to address them here. This PR: 1. Removes unused code. 2. Reduces code duplication. 3. Improves consistency across function usage. 4. Minimizes ShellCheck directive usage. ACKs for top commit: fanquake: ACK 5338877 - the move-only deduplication, and dead code removal seem fine. The other (refactoring) changes seem less well motived Tree-SHA512: d565e31ba49300f3001b04d3143721aced305f41bca6d04c33dc479d568efb2c06d91758619378199f174a40f1306c9418ae80d5478013a65ad96341bea122b1 (cherry picked from commit fbe6287)
…btck_*_create` functions 570a627 kernel: assert invalid buffer preconditions in `btck_*_create` functions (stringintech) Pull request description: The kernel API appears to use `nullptr` returns to report failures that callers may reasonably want to recover from: malformed serialized input, object construction failures, chainstate/load failures, and similar runtime conditions. The raw create-function buffer checks seem to be a different case. A failure of `ptr == nullptr && len > 0` does not indicate malformed input data or a failure encountered while deserializing or constructing the requested object. Returning `nullptr` for these checks widens the recoverable error surface with cases that are better treated as programmer errors, similar to other asserted preconditions in this API such as invalid indices and impossible enum/flag states. This change switches those buffer argument checks from `nullptr` returns to assertions in `btck_transaction_create`, `btck_script_pubkey_create`, `btck_block_create`, `btck_block_header_create`, and `btck_chainstate_manager_options_create`. `btck_block_header_create` additionally asserts the pre-existing documented length contract (must be 80 bytes). These functions still return `nullptr` when the provided bytes cannot be parsed or when object creation fails during processing. I ended up looking at this while working on the `kernel-bindings-tests` spec/schema for `btck_script_pubkey_create`, where treating this path as a regular error did not seem like the right contract: stringintech/kernel-bindings-tests#14 (comment). ACKs for top commit: stickies-v: ACK 570a627 janb84: ACK 570a627 w0xlt: ACK 570a627 sedited: ACK 570a627 Tree-SHA512: 064d834abe0c27245a144e5290bbeeb510daf9e4d50bb3a8e50bd8a0bf897b3dcf6ad5acfcabf1d8110da120e5e014ee3aea0241c0f181a21c6f3c14dc452ade (cherry picked from commit 19e4533)
131fa57 test: Add test for BuildSkip() and skip heights (optout) Pull request description: The skip height values computed by the (internal) function `GetSkipHeight()`, and the `CBlockIndex::BuildSkip()` method are not tested directly, and the skip logic is not well documented. To improve test coverage, a new test is added, to verify `CBlockIndex::BuildSkip()` and the skip height bit-manipulation logic. Note: the original version contained a test for the complexity of the `GetAncestor()` algorithm, whose performance is greatly determined by the skip height logic. This test was out-scoped. (see: bitcoin#33661 (comment)) The motivation is to document the skip value computation through a test. (The issued was noticed while reviewing bitcoin#33515.) ACKs for top commit: l0rinc: ACK 131fa57 sipa: ACK 131fa57 Tree-SHA512: 468070946419d9d2891e43ed014b040348fc9d3b35dc21522487ac28e06b6d5d556ac02ebc4fa7761e202fc80f9e9ab7a7ec47c6e05ae55e33950ff5f8f3596f (cherry picked from commit 654a522)
8cb8653 fuzz: target concurrent leveldb reads (Andrew Toth) 6609088 fuzz: extract ConsumeDBParams helper (Andrew Toth) Pull request description: Inspired by bitcoin#31132 (comment). We currently do concurrent leveldb reads when accessing our indexes. 1. `txindex` - we call `FindTx()` from multiple RPC threads. 2. `blockfilterindex` - we call `LookupFilter/Header()` concurrently from `msghand` thread for p2p requests as well as RPC threads. 3. `coinstatsindex` - we call `LookUpStats()` from multiple RPC threads. 4. `txospenderindex` - we call `FindSpender()` from multiple RPC threads. We also read from our chainstate and blocks index while background compactions are writing. While OSS-Fuzz does cover leveldb (https://github.com/google/oss-fuzz/blob/master/projects/leveldb/fuzz_db.cc), it doesn't cover multi threaded access. Without a deterministic hypervisor this fuzz harness won't be deterministic, but we can at least run it with TSan to get a higher confidence that the synchronization code in leveldb is correct. Hopefully other reviewers find this useful. This harness creates a threadpool with 16 threads, and then creates an in-memory levelDB which it seeds with deterministically random values. It chooses a random set of keys to query. It first performs all queries on the db on a single thread to get a baseline, then synchronizes all threads on a latch so they hit the db at the same time. Each thread performs the same queries, and afterwards are all checked against the baseline. It uses a `DeterministicEnv` to capture background compaction work when seeding the db, which is also run immediately after the latch is released. This causes a race between compaction and reading, ensuring we exercise many thread synchronization code paths in leveldb. I ran both TSan and ASan/UBSan overnight with no issues. ACKs for top commit: fjahr: Code review ACK 8cb8653 l0rinc: ACK 8cb8653 Tree-SHA512: 2ca31a824715b92e258c84ecf0c762f43ee2a528e3a3192f94d8aaeddf6e99f820a0297ce9efcc95bc32c7ec74489f240a25bab856d724d768117a7d95a33974 (cherry picked from commit b28cf40)
…of regtest in unit tests 801e3bf chainparams: add overloads for RegTest and SigNet with no options (Antoine Poinsot) 4995c00 chainparams: make deployment configuration available on all test networks (Antoine Poinsot) df7ed5f chainparams: encapsulate deployment configuration logic (Antoine Poinsot) Pull request description: It's sometimes useful to test a deployment on other networks than regtest. This may be e.g. because regtest lacks a property relevant for the test, or simply because the test aims to be portable while regtest is Bitcoin Core specific. This PR makes it possible to set the `-vbparams` and `-testactivationheight` options on any network in unit tests, and on any **test** network as a startup option. This is preparatory work for a BIP 54 implementation, but may be useful separately. ACKs for top commit: edilmedeiros: utACK 801e3bf achow101: ACK 801e3bf sedited: ACK 801e3bf instagibbs: ACK 801e3bf Tree-SHA512: 10649dc9bbc70a830bb0c4b1c965de5bf2e6be1a6c2832bdf11e9248dacb4a1f60421b410d0284213e88de6c54218252ae5de423b4c6e659d10bab1cbe7e7e87 (cherry picked from commit 082bb1a)
3962138 test: add IPC submitBlock functional test (woltx) 5b60f69 mining: add submitBlock IPC method to Mining interface (woltx) 813b4a8 refactor: introduce SubmitBlock helper (w0xlt) Pull request description: This PR adds a `submitBlock` method to the IPC Mining interface, equivalent to the `submitblock` RPC. It accepts a serialized block over IPC, validates/processes it via the normal block-processing path. The method uses the same result shape as `checkBlock`: `bool` + `reason/debug out-params`. It reports duplicate, inconclusive, and invalid-block rejection details, and initializes reason/debug on every call. Closes bitcoin#34626 ACKs for top commit: Sjors: ACK 3962138 optout21: reACK 3962138 ryanofsky: Code review ACK 3962138. Just rebased since and made suggested changes since last review. Tree-SHA512: 705cbb89972a80b6ff0ab75a78f686983d6077c97f1758795efe5b8968f01065ebef664ac850eae2bc86af8964efa2a68e8dfc677209c312856650f9387ed006
…g started" section fa51f37 doc: Reword the Getting-Started section (MarcoFalke) fab5733 doc: Remove good_first_issue.yml (MarcoFalke) Pull request description: Fixes bitcoin#35399 IIUC, the good-first-issue label and template were meant to make it easier for completely new contributors to get started with something simple. However, I don't think the label and issue template are applicable anymore: * There are currently no issues with this label, and directing people toward an empty list seems pointless. * Historically the issue and label has been used rarely. 2026: once, 2025 twice, 2024 thrice. Source: https://github.com/bitcoin/bitcoin/issues?q=state%3Aclosed%20is%3Aissue%20label%3A%22good%20first%20issue%22 * The template has been mis-used, according to bitcoin#35399 Fix all issues by removing it, since it is clear that it is no longer actively used, nor applicable and possibly a net-negative overall. Of course, regular devs are still free to open issues of this kind as a normal issue, if they wish. However, having the template for this in this repo and tracking it via a label doesn't seem useful. Since removing the template and label requires rewriting the "Getting Started" section in `CONTRIBUTING.md`, I went ahead and also removed the mention of `Up for grabs` as good things for newcomers to work on. I don't recall the last new contributor that picked something up successfully. Also, `Up for grabs` is usually stuff that people lost interest in, or is no longer relevant. Instead I've added a sentence to encourage new contributors to help with critical and broad review, which will naturally guide them to good first follow-up issues to work on. Meta: I know this topic can be subjective and offer bike-shed potential, but I am happy to iterate a bit on this for a few days. ACKs for top commit: stickies-v: ACK fa51f37 danielabrozzoni: ACK fa51f37 sedited: ACK fa51f37 darosior: ACK fa51f37 furszy: ACK fa51f37 winterrdog: ACK fa51f37 Tree-SHA512: 9e6d7fe86262bee2df1e0af33ecfb5f77036da2d5d1832bb6afb08f4107d9313eec06d8b769966bf8ecaf8a4c574da5ff99509cc4b7fd9c53ea86788da29721c
4bdd46a ci: switch runners from cirrus to warpbuild (will) Pull request description: As cirrus is closing down, switch to warpbuild runners. Switch runner and provider names over. We now use GHA cache, so we don't need to switch that over here. ACKs for top commit: m3dwards: ACK 4bdd46a maflcko: review ACK 4bdd46a 🤾 hebasto: ACK 4bdd46a. Tree-SHA512: 47ed28a6cb7ab10a973af6aa24f4f7a632f59ed17e189ae4f658de37069d763c92cc0e32769693568db6d0e5d2543abcb77bb0977f0b3f296d80a254d6bb3833
…m as errors 6183942 ci, iwyu: Fix warnings in src/scripts and treat them as error (Brandon Odiwuor) Pull request description: This PR [continues](bitcoin#33725 (comment)) the ongoing effort to enforce IWYU warnings. See [Developer Notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#using-iwyu). ACKs for top commit: maflcko: review ACK 6183942 🚖 hebasto: ACK 6183942. Tree-SHA512: 75f36189953cae82e03a7a69c285969e3fbf896656f60c77ab44b80264f88a03cf78af5352b719567f80abc0d7c818b7e4ffff0c83cfbc6d08c2d5fcc2fb0c5b
5700a61 ci: use ubuntu-latest instead of ubuntu-24.04 (fanquake) 265563b doc: remove reference to cirrus (fanquake) Pull request description: Two followups to bitcoin#35378, mentioned here: bitcoin#35397 (comment). ACKs for top commit: maflcko: review ACK 5700a61 🥗 willcl-ark: ACK 5700a61 Tree-SHA512: 015cc2efd5f9c421c09c1d953e48e46c860deaa81302d00929b893a6cdcf759a8baac6a521ff80f8618adaf9d64800a4c0d921ca1e92ea32a6b83f241bd33657
…ge names into table fa78704 doc: Compress doc/build-unix.md dependency package names into table (MarcoFalke) Pull request description: Currently, `doc/build-unix.md` is tediously verbose, because for several Linux distros, it has exact duplicate sections with only the package names adjusted. This is hard to maintain, and review. Also, it is hard to read, and hard to use, because a one-line copy-paste does not work to fetch the list of packages. Fix all issues by compressing the 150+ lines into a small table and a short description. ACKs for top commit: achow101: ACK fa78704 darosior: ACK fa78704 sedited: ACK fa78704 hebasto: re-ACK fa78704. Tree-SHA512: a6d7f18392ab5a0d468387ffe4335f71ae9656a100ede3de4118e3ef28814e5a70202ffa55eb0218a07effcde220b680499ea9068fd54b91cac42fca8699febf
2ce4ae7 ci: Add dynamic cache switching to warp cache (will) Pull request description: The GHA cache is very slow, taking on the order of minutes to save and restore from. Use WarpBuild's cache instead as this is in the same region and should be much faster. WarpBuild cache action does not auto-fallback to GHA if not being run on Warp. To allow fork runs to fallback to GHA caching, whilst minimising duplication in the action files, create new "interal" actions which perform the switching logic, and use these in the (renamed) cache|save actions. Without this we would need the `if` logic in our prvious actions, 4 times in each of save and restore. ACKs for top commit: m3dwards: ACK 2ce4ae7 Tree-SHA512: 966ed85558e8ebeed20a6fdf065f0838c9bee95f7d0a2fd1c8a46de20b956f5eedb6bf5098d742da20ea80c56069b86ff46f3151936dc28d5e4afa70acc5d988
2ef6679 test: Check that MuSig2 signing does not reuse nonces (Ava Chow) bb05986 musig: Include pubnonce in session id (Ava Chow) Pull request description: It is safe to have multiple musig signing sessions over the same message so long as the nonces used are different. Including the pubnonce in the session id allows for multiple simultaneous signing sessions over the same message, rather than asserting when the user tries to do this. The second commit tests this behavior, both ensuring that there is no crash, and verifying that both sessions produce unique nonces and signatures to verify that no reuse is occurring. Lastly, the assertion in `SetMuSig2SecNonce` is retained as hitting it now would indicate that a nonce has been reused. We prefer to assert and crash rather than do something that is highly likely to leak a private key. Fixes bitcoin#35250 ACKs for top commit: rkrux: lgtm ACK 2ef6679 junbyjun1238: utACK 2ef6679 theStack: ACK 2ef6679 Tree-SHA512: 9fb60b68ebe0ea9656408afb65b9ec9f280632e1bb84a4821b074c8d8569847845f7c29da800c757b9ddf3aa31aa890dd9e3646cf119917a714e7daf20be2198
…e space 107d417 versionbits: update VersionBitsCache doc comment to match current behaviour (Antoine Poinsot) 94e3ac0 doc: release notes and bips doc update for bitcoin#34779 (Antoine Poinsot) 1d52405 qa: test we don't warn for ignored unknown version bits deployments (Antoine Poinsot) f802edf versionbits: Limit live activation params and activation warnings per BIP323 (Anthony Towns) Pull request description: This implements bitcoin/bips#2116, which repurposes 24 version bits as extra nonce space for miners rather than soft fork deployment coordination. 24 bits allows a miner to perform up to 72 PH before needing a fresh job from its controller. The current 16 bits in use by miners only allow up to 280 TH, which [apparently led some ASIC designers to start rolling the timestamp field](bitaxeorg/ESP-Miner#1553 (comment)) on their beefier machines. Mailing list discussion available [here](https://gnusha.org/pi/bitcoindev/6fa0cb45-37d6-4b41-9ff8-03730fd96d6e@mattcorallo.com/). A previous shot at this is bitcoin#13972 (with a smaller extranonce space). This change only affects the warning logic. ACKs for top commit: ajtowns: ACK 107d417 achow101: ACK 107d417 sedited: Re-ACK 107d417 optout21: ACK 107d417 Tree-SHA512: cfaf5d7de1e8c020a4d7f4b1096b6c3e0e3b41ea840a4652ebcdabc345c5c557161c8304f1d7d6de541a2bf1df3c855ad7b64e49dd8c8af3937876d134bb5aba
8290198 ci: use Warp cache for Docker layers (will) Pull request description: We switched to GH cache recently, but the performance is beyond terrible. We have already switched the main cache actions over to warp, and this completes the transfer with the docker buildkit cache to the Warp endpoint. ACKs for top commit: m3dwards: ACK 8290198 Tree-SHA512: caee053598c5082b1f0cc52a6abfe6314ac2d5d840238480f1706f55a892f08ca533ac3d5d4f9f20a84958df1c9100ae00518686072515a1011be994619b4df9
…1 reconnections bf0d257 net: un-default the OpenNetworkConnection()'s proxy_override argument (Eugene Siegel) 5a3756d test: add a regression test for private broadcast v1 retries (Vasil Dimov) ab35a02 test: make reusable filling of a node's addrman (Vasil Dimov) 2333be9 test: make reusable starting a standalone P2P listener (Vasil Dimov) 2ffa81f test: make reusable SOCKS5 server starting (Vasil Dimov) 32d072a doc: add release notes for bitcoin#35319 (Vasil Dimov) d01b461 net: ensure no direct private broadcast connections (Vasil Dimov) fd230f9 net: use the proxy if overriden when doing v2->v1 reconnections (Vasil Dimov) Pull request description: This PR includes bitcoin#35319 and on top of that adds a regression functional test. The functional test exercises the relevant code paths without modifying non-test code. To do that it does: * Add a bunch of IPv4 addresses to the node's addrman (they will be added without P2P_V2 flag). * Get them to report P2P_V2 in their service flags and connect to each one, so that the flags in addrman are updated to contain P2P_V2. * Get one successful connection to a Tor peer (.onion) so that bitcoind assumes the configured Tor proxy works and is indeed a proxy to the Tor network. This will make it open private broadcast connections also to IPv4 addresses via that proxy. * Start some private broadcast connections. * Remember the destination IPv4 address of the first connection and get it to fail the v2 transport. * Wait for a subsequent connection also through the Tor proxy to the same IPv4 and expect it to be v1, i.e. the v2->v1 downgrade retry. The test fails without the fix - the v1 retry never arrives to the Tor proxy. And passes with the fix. The fix is in the first commit here and in bitcoin#35319, can remove it by `git show fd230f9 | git apply -R`. ACKs for top commit: Crypt-iQ: reACK bf0d257 andrewtoth: ACK bf0d257 instagibbs: ACK bf0d257 sedited: utACK bf0d257 Tree-SHA512: 11e89be36577199e0312e5e63efeac04e295faaba1cf1c13a30e683d35f473c8dbb419d1897b0333c2e993c10637adecafcf90fe08c812065c793cbc903744c9
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
5 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.