build: Drop ZeroMQ patch for glibc < 2.12 (Hennadii Stepanov) 079df96 build: Drop ZeroMQ patch for Mingw-w64 < 4.0 (Hennadii Stepanov)#3
Open
ryihan wants to merge 10000 commits intoNetboxGlobal:masterfrom
Open
Conversation
65379bb ci: add FreeBSD cross CI job (fanquake) f44191f depends: build qrencode for Freebsd (fanquake) 7f70187 depends: FreeBSD cross with Clang (fanquake) 6464f14 depends: disable inotify in Freebsd Qt build (fanquake) Pull request description: Alternative to #33562, which was adding a native FreeBSD job; however that had issues with permissions/caching, as well as potential determinism issues. This adds a FreeBSD cross job using Linux and Clang. Would close #33438. The same changes here could also be used to produce FreeBSD binaries out of Guix. ACKs for top commit: hebasto: ACK 65379bb. I've cross-compiled on Ubuntu 25.10 for FreeBSD 14.4 and 15.0. The former binaries (`bitcoind`, `test_bitcoin` and `bitcoin-qt`) were tested on FreeBSD 14.4 locally. Tree-SHA512: 52a3edaa56fe40ca901416cb9e1af04a84505526edfa7309bfa40024baa7d3b1a05303659553d9fbcf1f49d4e3d42b415a1e2523d448b22724d1415a49331259
890a09b fuzz: Use CAmount for storing best_waste (Ava Chow) Pull request description: Waste is a CAmount, which is an int64_t. This will overflow an int, so `best_waste` should also be a `CAmount`. Fixes #34936 ACKs for top commit: murchandamus: ACK 890a09b furszy: ACK 890a09b Tree-SHA512: c6c4f530960f038675d4549c2285c6a4a828099a631486e317ec1215d89688ce109304654a95800978607c360c2ed34803523f5c56ebf7c2324ca095f87825b8
…n disconnects 2478a15 Squashed 'src/ipc/libmultiprocess/' changes from 1868a84451f..70f632bda8f (Ryan Ofsky) Pull request description: Includes: - bitcoin-core/libmultiprocess#246 - bitcoin-core/libmultiprocess#242 - bitcoin-core/libmultiprocess#247 - bitcoin-core/libmultiprocess#251 - bitcoin-core/libmultiprocess#255 - bitcoin-core/libmultiprocess#258 - bitcoin-core/libmultiprocess#262 - bitcoin-core/libmultiprocess#253 - bitcoin-core/libmultiprocess#263 - bitcoin-core/libmultiprocess#256 - bitcoin-core/libmultiprocess#264 - bitcoin-core/libmultiprocess#249 - bitcoin-core/libmultiprocess#265 The main change is bitcoin-core/libmultiprocess#249 which fixes 3 intermittent race conditions detected in bitcoin core CI and antithesis: #34711/#34756, #34777, and #34782. The changes can be verified by running `test/lint/git-subtree-check.sh src/ipc/libmultiprocess` as described in [developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#subtrees) and [lint instructions](https://github.com/bitcoin/bitcoin/tree/master/test/lint#git-subtree-checksh) ACKs for top commit: Sjors: ACK 613a548 ismaelsadeeq: ACK 613a548 Tree-SHA512: d99eebc8b4f45b3c3099298167362cf5e7f3e9e622eef9f17af56388ee5207d77a04b915b2a5a894493e0395aeda70111216f2da0d2a6553f4f6396b3d31a744
3dcdb2b test: wallet: Warning for excessive fallback fee. (David Gumberg) 6664e41 test: wallet: -fallbackfee default is 0 (David Gumberg) d28c989 test: wallet: refactor: fallbackfee extract common send failure checks. (David Gumberg) Pull request description: In an unmerged branch of #32636 (097e00f) I unintentionally broke default `-fallbackfee` behavior, but this was not caught by any tests. See #32636 (comment). Something like the following diff does not cause any test failures on master despite causing a behavior change: ```diff diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index bc27018..079610fba0 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3048,24 +3048,24 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri if (const auto arg{args.GetArg("-fallbackfee")}) { std::optional<CAmount> fallback_fee = ParseMoney(*arg); if (!fallback_fee) { error = strprintf(_("Invalid amount for %s=<amount>: '%s'"), "-fallbackfee", *arg); return nullptr; } else if (fallback_fee.value() > HIGH_TX_FEE_PER_KB) { warnings.push_back(AmountHighWarn("-fallbackfee") + Untranslated(" ") + _("This is the transaction fee you may pay when fee estimates are not available.")); } walletInstance->m_fallback_fee = CFeeRate{fallback_fee.value()}; + // Disable fallback fee in case value was set to 0, enable if non-null value + walletInstance->m_allow_fallback_fee = walletInstance->m_fallback_fee.GetFeePerK() != 0; } - // Disable fallback fee in case value was set to 0, enable if non-null value - walletInstance->m_allow_fallback_fee = walletInstance->m_fallback_fee.GetFeePerK() != 0; ``` This PR adds a functional test check that when no `-fallbackfee` argument is set and fee estimation is not possible, that sending fails because `-fallbackfee` is disabled by default. ACKs for top commit: maflcko: review ACK 3dcdb2b 🐞 w0xlt: reACK 3dcdb2b ismaelsadeeq: reACK 3dcdb2b 👾 Tree-SHA512: 715625673a781ba3ddfed25f0836b01c2197480bd56732fd1ce548e8969573c2a36601de0b8913b3b79a47b8149022aabcf409b62297e7c2c47b68a8843e6570
…n NextSyncBlock() db3c25c index: add explicit early exit in NextSyncBlock() when the input is the chain tip (Hao Xu) Pull request description: When `pindex_prev` is the chain tip, `NextSyncBlock()` should return `nullptr` (there's no next block to sync). Currently this works, but relies on the fork/reorg path to produce the result: `chain.Next(tip)` returns `nullptr`, falling through to `FindFork(tip)` which returns `tip` itself, then `chain.Next(tip)` returns `nullptr` again. Add an explicit early return for this case. This makes `NextSyncBlock()`'s three cases self-documenting: 1. next block exists on the active chain — return it 2. at the chain tip — return `nullptr` 3. on a stale branch — find the fork point and return the block after it It also makes the existing comment ("Since block is not in the chain") accurate — the tip is in the chain, so it shouldn't reach that path. ACKs for top commit: optout21: crACK db3c25c furszy: ACK db3c25c stickies-v: ACK db3c25c Tree-SHA512: c1633bb3d3ffed2643c8e174c2b2283deaeffd0a06eb3fb2cf03eed097bfdc5cf6fbd7ebdcdb44fd56f712be8004ad46c730a33990fb1fc6cfa629c9b14f8cd9
445143b wallet: document structured importdescriptors errors (Renato Britto) Pull request description: Related to #29912 and the machine-readable OpenRPC generated from `RPCHelpMan` metadata work discussed in #34683. Split out from #34764 per review feedback that this change is conceptually separate from the broader elision work there. The `importdescriptors` RPC help currently documents the optional `error` field using an elided `JSONRPC error` placeholder. This PR replaces that with explicit `code` and `message` fields. `importdescriptors` already returns a structured JSON-RPC error object in practice, so this makes the documented result schema match the existing response shape more closely. This PR changes `importdescriptors` help text, and the runtime checks on the returned json to be more strict. ACKs for top commit: nervana21: tACK 445143b maflcko: lgtm ACK 445143b Tree-SHA512: 3bda1cc7dd222c1d2e4dfbb2ee4a3f0201914c8b6bed5efb7fe7866227867c43235a9c5f5ec6f56e7ba286db0a43962c782d7ff29eec9aef4e70dc2c3daa6a0e
The claim that failing mandatory script checks may trigger a DoS ban is incorrect; Bitcoin Core does not automatically ban peers for violating any of these flag checks.
This was overlooked in #34896.
Add missing test coverage for the `getblocktemplate` RPC call "coinbasevalue" field, specifically that it is set to claim the full block reward.
… peering 6b99a3e doc: update cjdns.md for current upstream changes (w0xlt) Pull request description: Update `doc/cjdns.md`. Users following the current doc hit dead links when trying to set up cjdns peering. The upstream cjdns project has reorganized its README and now supports automatic peering, making most of the existing instructions obsolete. Summary * Remove broken links to cjdns README sections ("2. Find a friend", "3. Connect your node to your friend's node") that no longer exist upstream * Remove reference to `hyperboria/peers` repo and `testAvailable.py` (last updated Feb 2024, likely stale) * Add `cjdns.sh` as recommended install method alongside building from source * Document automatic peering via DNS seeding (default since cjdns v22), which makes manual peer discovery unnecessary for most users * Simplify manual peering instructions with a clear `connectTo` example and link to upstream `doc/peering.md` * Add `cjdnstool peers show` as the way to verify network connectivity The Bitcoin Core-specific sections (`-cjdnsreachable`, `-onlynet=cjdns`, admin commands, etc.) are unchanged. ACKs for top commit: achow101: ACK 6b99a3e stratospher: ACK 6b99a3e. worked on nixos. brunoerg: reACK 6b99a3e naiyoma: TestedACK 6b99a3e Tree-SHA512: 00a703a788e96af4fd9456246644c3047b1d5cbed41d97f4f4f64f60b34cd6ffbf052d5e8f32365e65fd09a44fd0e16dd0dd45f6c75563f18075414f9b3eb1e7
4565cff bitcoin-gui: Implement missing Init::makeMining method (Ryan Ofsky) fbea576 test: add interface_ipc_cli.py testing bitcoin-cli -ipcconnect (Ryan Ofsky) 0448a19 ipc: Improve -ipcconnect error checking (Ryan Ofsky) 8d614bf bitcoin-cli: Add -ipcconnect option (Ryan Ofsky) 6a54834 ipc: Expose an RPC interface over the -ipcbind socket (Ryan Ofsky) df76891 refactor: Add ExecuteHTTPRPC function (Ryan Ofsky) 3cd1cd3 ipc: Add MakeBasicInit function (Ryan Ofsky) Pull request description: This implements an idea from sipa in #28722 (comment) to allow `bitcoin-cli` to connect to the node via IPC instead of TCP, if the ENABLE_IPC cmake option is enabled and the node has been started with `-ipcbind`. This feature can be tested with: ``` build/bin/bitcoin-node -regtest -ipcbind=unix -debug=ipc build/bin/bitcoin-cli -regtest -ipcconnect=unix -getinfo ``` The -ipconnect parameter can also be omitted, since this change also makes `bitcoin-cli` prefer IPC over HTTP by default, and falling back to HTTP if an IPC connection can't be established. --- This PR is part of the [process separation project](#28722). ACKs for top commit: achow101: ACK 4565cff pinheadmz: ACK 4565cff enirox001: Tested ACK 4565cff Tree-SHA512: cb0dc521d82591e4eb2723a37ae60949309a206265e0ccfbee1f4d59b426b770426fafa1e842819a2fa27322ecdfcd226f31da70f91c2c31b8095e1380666f1f
The field is not a duration, but a time point. This will add two temporary calls to time_since_epoch(), which are fixed in the next commit.
…tart The two fields represent a time point, not a duration. Also, it is unclear why they use second precision. Fix both issues by using NodeClock::time_point. This refactor should not change any behavior. This resolves the two temporary calls to time_since_epoch() added in the previous commit. However, it adds one new call to time_since_epoch(), which is resolved in the next commit.
Type will be used for reading the cookie in next commit. Also corrects ERR/ERROR mismatch in docstring in request.h, and changes to CamelCase to avoid potential collision with Windows headers (#34965 (comment)).
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
fabab69 refactor: Return std::optional from ParseDouble (MarcoFalke) fa0a094 refactor: Return std::optional from GetWalletNameFromJSONRPCRequest (MarcoFalke) fafb0c4 refactor: Return std::optional from GetLogCategory (MarcoFalke) Pull request description: Using a bool to indicate whether a mutable in-out param was written to is fine in legacy code, but otherwise confusing and brittle: * Sometimes the in-out-param is written to, even when the function returns `false`, like in `ParseDouble`. * Call sites must manually check the return value Fix those issues by returning `std::optional<_>` from `ParseDouble` (and a few other functions). This refactor is a style cleanup and does not change any behavior. ACKs for top commit: stickies-v: re-ACK fabab69 hodlinator: crACK fabab69 rkrux: code review ACK fabab69 Tree-SHA512: e27a27174e9d2200da8b0ca9b6cc056e94d2b25e6332975f1ad660ee85b02680a65ac93b2ed29f10da0ae5f6dc8395bddc9e973a3f925c68a0c8116c9282ea09
…` comment 5fa6898 policy: remove incorrect MANDATORY_SCRIPT_VERIFY_FLAGS comment (ismaelsadeeq) Pull request description: Since #33050 This comment claiming that failing mandatory script checks may trigger a DoS ban/punishment is false. `CheckInputScript` also dispute this comment. https://github.com/bitcoin/bitcoin/blob/b0f68f0a3a361c62cb1e1cb49aacaeac171f3079/src/validation.cpp#L2113-L2118 ACKs for top commit: darosior: utACK 5fa6898 instagibbs: ACK 5fa6898 Tree-SHA512: 84ad68b158e334b0e945f33007de58807b0b3335b1873412b0f359f0fa52d0819e93f42b28ebb6fbe5b355f2710f61e0dd731dc4bce20ccc1b7b2c546a9df550
…sult field 12c3c3f test: mining: add coverage for GBT's "coinbasevalue" result field (Sebastian Falbesoner) Pull request description: This PR adds missing coverage for the "coinbasevalue" result field of the `getblocktemplate` RPC call. Specifically, the introduced test checks that the value is set to claim the full block reward (subsidy plus fees). Can be verified with the following patch, which succeeds on master and fails on the PR branch: ```diff diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index a935810..ba9ac9dadb 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -998,7 +998,7 @@ static RPCHelpMan getblocktemplate() result.pushKV("previousblockhash", block.hashPrevBlock.GetHex()); result.pushKV("transactions", std::move(transactions)); result.pushKV("coinbaseaux", std::move(aux)); - result.pushKV("coinbasevalue", block.vtx[0]->vout[0].nValue); + result.pushKV("coinbasevalue", block.vtx[0]->vout[0].nValue - 1); result.pushKV("longpollid", tip.GetHex() + ToString(nTransactionsUpdatedLast)); result.pushKV("target", hashTarget.GetHex()); result.pushKV("mintime", GetMinimumTime(pindexPrev, consensusParams.DifficultyAdjustmentInterval())); ``` I'm not sure how relevant this field is nowadays in real-world mining scenarios (we use it for the signet miner at least), but it seems useful to have a test for it anyways. Stumbled upon this while looking at bitcoin-inquisition#111. ACKs for top commit: maflcko: lgtm ACK 12c3c3f Sjors: ACK 12c3c3f Tree-SHA512: ae10f63c99b21cf7771feefe3d0e47b2e0d511aceb344cf11efa9182d78f2960f1e079797b8a36db110a3c54acb27a3706413a710a74bf56aed29ea162a6aab2
5a81d73 scripted-diff: rpc: Don't pointlessly capture in RPCMethod lambdas (Anthony Towns) 4e78929 scripted-diff: rpc: Rename RPCHelpMan to RPCMethod (Anthony Towns) Pull request description: When defining `RPCHelpMan` objects, we usually return a lambda, and mostly we define those via `[&](...) { ... }` which explicitly captures any parameters or local variables by reference. If we were to actually use any of those captures (we don't), we would invoke undefined behaviour. So instead, convert all the `[&]` to `[]` to avoid capturing. While we're at it, rename `RPCHelpMan` to `RPCMethod`, reflecting its greater responsibility since #19386. ACKs for top commit: maflcko: review ACK 5a81d73 🏣 stickies-v: ACK 5a81d73 rkrux: code review ACK 5a81d73 Tree-SHA512: 72e9232857ba5bf4c346fb963a2028047f7c7e9b420ef58b3108669204bfbb6952342cfcfd2e8a06f813a21545abf7fc8b0ad422f00d7ec9c1134626cd1650e6
98fcd7a wallet: rpc: Improve error message for low feerates. (David Gumberg) Pull request description: Giving the user a hint about what action to take when they encounter an error related to a feerate that is below the minimum. I encountered this myself not knowing about `-mintxfee` and the manpage was slightly less than clear, ``` -mintxfee=<amt> Fee rates (in BTC/kvB) smaller than this are considered zero fee for transaction creation (default: 0.00001) ``` ACKs for top commit: achow101: ACK 98fcd7a ismaelsadeeq: ACK 98fcd7a w0xlt: ACK 98fcd7a Tree-SHA512: 8471bfd5682ca59137541ab4da670e85947cf7812baa9e07d816e352fccfb5e0e71a06c2baf1a5422d78a7da8c08b9fcbb73d85cc9daba88b7c22e802abe5b2e
2e041b4 help: enrich help text for `-loadblock` (Hao Xu) Pull request description: `-loadblock` doesn't support XOR-ed files, mention it in its help text to avoid troubles for users. ACKs for top commit: maflcko: lgtm ACK 2e041b4 sedited: ACK 2e041b4 Tree-SHA512: 1883824c15687799d60ddabc53d5dc2c274e56c8292da5e7e412adaef65abf70c7f778ef7675c65f738c1e4855e5b0f929e87631b0a52695873e58f8245b3a8d
3aeccb7 depends, qt: Fix build on aarch64 macOS 26.4 (Hennadii Stepanov) Pull request description: Fixes #34951. See an upstream issue: https://qt-project.atlassian.net/browse/QTBUG-145239. ACKs for top commit: fanquake: ACK 3aeccb7 Tree-SHA512: ab8bc42072077d72068eac17ba5d69c98f313dd4e13c5ffcd3a3cc33eb5f8afe2495f470410edc4a8ee60a6c0942c5b117dcf0a46b2194388db2eee82ee69f86
c95968f doc: add missed advisory to 31.0 rel notes (fanquake) Pull request description: Generally we don't post-amend release notes, but we can add the missing EOL security advisory here. See bitcoin-core/bitcoincore.org#1240 and #35132. ACKs for top commit: darosior: ACK c95968f achow101: ACK c95968f sedited: ACK c95968f Tree-SHA512: d7269bc088fa1bc577bc200604f7626eede73d2e920186711ecc5120f9b51e01d02f35fda2d1291289e89132280c6cdb050e4a7e423d4f30b45e3949160b5163
…rsions af0ee28 refactor: use _MiB consistently for Mebibyte conversions (Lőrinc) b3edd30 util: add _GiB for Gibibyte conversions (Lőrinc) Pull request description: ### Problem Byte-size conversions in the codebase currently show up in many equivalent formats (multiplication/division chains, shifts, hex/binary literals), which creates a maintenance burden and makes review error-prone - especially considering the architectural differences of `size_t`. Inspired by #34305 (comment), it seemed appropriate to unify `Mebibyte` usage across the codebase and add `Gibibyte` support with 32/64 bit `size_t` validation. ### Fix This PR refactors those call sites to use `""_MiB` (existing) and `""_GiB` (new), and adds the encountered value/pattern replacements to unit tests to make review straightforward, and to ensure the conversions remain valid. The literals are overflow-checked when converting to `size_t`, and unit tests cover the 32-bit boundary cases. Concretely, it replaces patterns such as: * `1024*1024`, `1<<20`, `0x100000`, `1048576`, `/ 1024 / 1024`, `* (1.0 / 1024 / 1024)` → `1_MiB` or `double(1_MiB)` * `1024*1024*1024`, `1<<30`, `0x40000000`, `1024_MiB`, `>> 30` → `1_GiB` (added unit tests for each replacement category to ease review) Additionally, declarations whose initializer reads a `_MiB`/`_GiB` literal are switched to braced initialization so a future oversized value is rejected at compile time through the narrowing check instead of silently truncating. ### Note In the few places where arithmetic involves signed values, the result is identical to the previous code assuming those quantities never become negative. ACKs for top commit: achow101: ACK af0ee28 janb84: ACK af0ee28 maflcko: review ACK af0ee28 🖍 hodlinator: re-ACK af0ee28 Tree-SHA512: 55286ce3f833f88335394a74e9e0b95c7d023e5cdc9ded40accbbbcd870101e4dcc05926865d6bef4c1be1ebd648aa3fdf947ef9575633ccfe56691f145d7a2d
…tests 7c75244 Change pindexMostWork parameter of ActivateBestChainStep() to reference (optout) c5eb283 Change CChain::FindFork() to take ref (optout) 20b58e2 Change CChain::Next() to take reference (optout) fe2d6e2 Change CChain::Contains() to take reference (optout) db56bcd test: Add CChain::FindFork() tests (optout) 8333abd test: Add CChain basic tests (optout) Pull request description: Refactor `CChain` methods (`Contains()`, `Next()`, `FindFork()`) to use references instead of pointers, to minimize the risk of accidental `nullptr` dereference (memory access violation). Also add missing unit tests to the `CChain` class. The `CChain::Contains()` method (in `src/chain.h`) dereferences its input without checking. The `Next()` method also calls into this with a `nullptr` if invoked with `nullptr`. While most call sites have indirect guarantee that the input is not `nullptr`, it's not easy to establish this to all call sites with high confidence. These methods are publicly available. There is no known high-level use case to trigger this error, but the fix is easy, and makes the code safer. Changes: - Add basic unit tests for `CChain` class methods - Add unit tests for `CChain::FindFork()` - Change `CChain::Contains()` to take reference - Change `CChain::Next()` to take reference - Change `CChain::FindFork()` to take reference - Change `pindexMostWork` parameter of `ActivateBestChainStep()` to reference - Rename changed parameters (`* pindex` --> `& index`) Alternative. A simpler change is to stick with pointers, with extra checks where needed, see #34416 . This change is remotely related to and indirectly triggered by #32875 . Further ideas, not considered in this PR: - Change `InvalidateBlock()` and `PreciousBlock()` to take references. - Change `CChain` internals to store references instead of pointers - Change CChain to always have at least one element (genesis), that way there is always genesis and tip. - Check related methods to return reference (guaranteed non-null) -- `FindFork`, `FindEarliestAtLeast`, `FindForkInGlobalIndex`, `blockman.AddToBlockIndex`, etc. ACKs for top commit: l0rinc: reACK 7c75244 maflcko: re-review ACK 7c75244 🌅 achow101: ACK 7c75244 hodlinator: re-ACK 7c75244 Tree-SHA512: 122f40120058f7e1f0273b3afed9c54966c05f06b6f2fee45bc48430617f24a5e4320a9bb7bb0ac986f2accfa22fabae5cc941b949758ddca2e9fcd472b46c33
…t txs 32325d1 tests: Add test for mempool-invalid wallet tx (Anthony Towns) 25e063d wallet: Add separate balance info for non-mempool wallet txs (Anthony Towns) 81e763f wallet: Have GetBalance report used amount directly without two calls (Anthony Towns) Pull request description: Changes `getbalances` to report the sum of txos spent by transactions that aren't confirmed nor in the mempool (eg due to being part of too long a mempool chain, or spending non-standard outputs, or having a datacarrier output that exceeds `-datacarriersize`, etc). Those values are added to the trusted/untrusted_pending/immature/used fields as appropriate (where previously they were skipped), and subtracted from the new nonmempool field, so that the sum of all fields remains the same. For example: ``` $ bitcoin-cli -regtest getbalances { "mine": { "trusted": 6049.99999220, "untrusted_pending": 0.00000000, "immature": 3200.00000780, "nonmempool": -100.00000000 }, "lastprocessedblock": { "hash": "3ab4582226d5e8ad76438db48d76e822c31bce2cdbc7ba82a5d974a277515d0d", "height": 221 } } ``` Closes #11887 ACKs for top commit: achow101: ACK 32325d1 w0xlt: lgtm ACK 32325d1 musaHaruna: Code Review ACK [32325d1](32325d1) Tree-SHA512: 142581944d1b3213067e219e3b8205f27b89007e545149c01b801bad38fe730c5b2bfdfe6a2064c3649889f66ec48ec7616982564d00e3d83837249e925d8f16
c5ec2d5 logging: replace FormatLogStrInPlace with Format (stickies-v) 3b92ec2 logging: replace BufferedLog with log::Entry (stickies-v) 8115001 logging: pass log::Entry through to logging functions (stickies-v) b414913 util: add timestamp and thread_name to log::Entry (stickies-v) 8a55b17 util: make SourceLocation constructor explicit (stickies-v) Pull request description: Preparatory work to enable struct-based logging for kernel (#34374), but I think it's a mild improvement by itself too. #34465 introduced `util::log::Entry`. This PR updates some internal functions and structs to make better use of that new plumbing. Mostly a refactor, except for very minor timing differences for `timestamp` and `mocktime` which are now uniformly captured at `Entry` generation. ACKs for top commit: maflcko: review ACK c5ec2d5 🚐 ryanofsky: Code review ACK c5ec2d5. Just suggested changes since last review: adding (void) std::move, making SourceLocation constructor explicit, dropping BufferedLog struct. Overall PR is a nice code simplification, and I don't think it has downsides other than buffered logs now containing [ignored](#34865 (comment)) `should_ratelimit` fields, which can be fixed in a followup by separating log entries from log options. sedited: ACK c5ec2d5 Tree-SHA512: 3943f380a426b2c1b405226f231db51548c737467a278936b36a4cc738e01a790258f0886817d7caa1dbba7874f2e7f9ad93c36a137fa35f721f2d988b9863aa
The move-assignment operator for btck::Handle<> unconditionally called DestroyFunc(m_ptr) before reading the source pointer. On a self-move (h = std::move(h)), this destroyed the held resource and then reassigned the now-dangling pointer back to m_ptr via std::exchange, leading to a double-free when the object is later destroyed. Mirror the existing self-check in the copy-assignment operator by guarding the move-assignment with 'if (this != &other)' so a self-move becomes a no-op, leaving the object in a valid state as required by the standard library. Handle<> is the base of 16 public types in the kernel C++ API wrapper (Transaction, Block, BlockHeader, ChainParams, Context, Coin, BlockValidationState, ScriptPubkey, TransactionOutput, Txid, OutPoint, TransactionInput, PrecomputedTransactionData, BlockHash, BlockSpentOutputs, TransactionSpentOutputs), so self-move can arise from generic algorithms operating on containers of these types. Extend CheckHandle in test_kernel to cover self-move-assignment for every Handle-derived type.
Previously, they were using a pattern of defininig a constant megabytes symbol, and then multiplying that by 1_MiB. It is easier to use the _MiB operator directly.
3bf3b6d net: log SOCKS5 auth before sending (takeshikurosawaa) b2debc9 net: cleanup SOCKS5 auth logging (takeshikurosawaa) Pull request description: `Socks5()` logs the supplied `username` and `password` in `BCLog::PROXY` when the server selects `USER_PASS`. Keep the log entry for the auth path, but omit the credentials. Log the message before sending the authentication data. No functional behavior change intended. ACKs for top commit: davidgumberg: crACK 3bf3b6d theStack: ACK 3bf3b6d sedited: ACK 3bf3b6d Tree-SHA512: ef90fca297b954c8659d108259b7f25216c0c0ddb7305539154f14a7bfdad807b70b7ddd0cf6e4ad0127d0a1ccfdfbb3f4785d7662b134409375ff1830b9df9b
5de2f97 dbwrapper: use `SpanReader` for iterator keys (Lőrinc) f0e498a test: cover failed `CDBIterator::GetKey()` deserialization (Lőrinc) Pull request description: ### Problem `CDBIterator::GetKey()` only deserializes the current LevelDB key once and `GetKeyImpl()` already exposes that key as a contiguous borrowed byte span, and `GetKey()` creates a fresh local reader and only performs immediate forward reads before returning. The copied `DataStream` currently insulates the iterator entry from a failed decode, so switching to a borrowed reader is only safe if a deserialization failure still returns false and leaves the same key/value readable afterward. > [!NOTE] > The same simplification does not apply to `GetValue()`, because that path deobfuscates the value bytes in place first and still needs an owning mutable buffer. ### Fix Add a preparatory test with an invalid reads and checks that the failed decode [does not consume](https://github.com/bitcoin/bitcoin/blob/eb85cacd2969caaea682104f498f6b2e6cfb80f8/src/leveldb/include/leveldb/iterator.h#L60-L62) the current iterator entry. Then switch `GetKey()` to `SpanReader` so the key bytes are read in place instead of being copied into a temporary `DataStream`. This keeps the same exception swallowing and `bool` return semantics while avoiding the extra allocation and copy. ### Context Related to #34483 and #35025 ### Reproducer `gettxoutsetinfo` is ~10-12% faster for up-to-date blocks (run on SSD), see: <details><summary>2026-04-20 | gettxoutsetinfo | rpi5-8 | aarch64 | Cortex-A76 | 4 cores | 7.7Gi RAM | ext4 | SSD</summary> ``` COMMITS="64a88c8c1edc7ee5cef623d9aa8179a239e27ce9 57dc020"; \ BASE_DIR="/mnt/my_storage"; DATA_DIR="$BASE_DIR/BitcoinData"; LOG_DIR="$BASE_DIR/logs"; \ mkdir -p "$LOG_DIR" && \ (echo ""; for c in $COMMITS; do git cat-file -e "$c^{commit}" 2>/dev/null || git fetch -q origin "$c" || exit 1; git log -1 --pretty='%h %s' "$c" || exit 1; done) && \ (echo "" && echo "$(date -I) | gettxoutsetinfo | $(hostname) | $(uname -m) | $(lscpu | grep 'Model name' | head -1 | cut -d: -f2 | xargs) | $(nproc) cores | $(free -h | awk '/^Mem:/{print $2}') RAM | $(df -T $BASE_DIR | awk 'NR==2{print $2}') | $(lsblk -no ROTA $(df --output=source $BASE_DIR | tail -1) | grep -q 1 && echo HDD || echo SSD)"; echo "") && \ hyperfine \ --sort command \ --runs 10 \ --export-json "$BASE_DIR/gettxoutsetinfo-$(sed -E 's/([a-f0-9]{8})[a-f0-9]* ?/\1-/g;s/-$//'<<<"$COMMITS")-$(date +%s).json" \ --parameter-list COMMIT ${COMMITS// /,} \ --prepare "killall -9 bitcoind 2>/dev/null || true; rm -f $DATA_DIR/debug.log; git clean -fxd && git reset --hard {COMMIT} && \ cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release && ninja -C build bitcoind bitcoin-cli -j$(nproc) && \ ./build/bin/bitcoind -datadir=$DATA_DIR -connect=0 -listen=0 -dnsseed=0 -coinstatsindex=0 -txindex=0 -blockfilterindex=0 -daemon -printtoconsole=0; \ ./build/bin/bitcoin-cli -datadir=$DATA_DIR -rpcwait getblockcount >/dev/null" \ --conclude "./build/bin/bitcoin-cli -datadir=$DATA_DIR stop 2>/dev/null || true; killall bitcoind 2>/dev/null || true; sleep 10; \ grep -q 'Done loading' $DATA_DIR/debug.log && grep 'Bitcoin Core version' $DATA_DIR/debug.log | grep -q \"\$(git rev-parse --short=12 {COMMIT})\"; \ cp $DATA_DIR/debug.log $LOG_DIR/gettxoutsetinfo-{COMMIT}-$(date +%s).log" \ "./build/bin/bitcoin-cli -datadir=$DATA_DIR -rpcclienttimeout=0 -named gettxoutsetinfo hash_type='none' use_index='false' >/dev/null" 64a88c8 Merge #35096: kernel: align height parameters to int32_t in btck API 57dc020 dbwrapper: use SpanReader for iterator keys Benchmark 1: ./build/bin/bitcoin-cli -datadir=/mnt/my_storage/BitcoinData -rpcclienttimeout=0 -named gettxoutsetinfo hash_type='none' use_index='false' >/dev/null (COMMIT = 64a88c8) Time (mean ± σ): 109.002 s ± 3.091 s [User: 0.003 s, System: 0.004 s] Range (min … max): 106.191 s … 113.608 s 10 runs Benchmark 2: ./build/bin/bitcoin-cli -datadir=/mnt/my_storage/BitcoinData -rpcclienttimeout=0 -named gettxoutsetinfo hash_type='none' use_index='false' >/dev/null (COMMIT = 57dc020) Time (mean ± σ): 97.711 s ± 1.172 s [User: 0.003 s, System: 0.004 s] Range (min … max): 96.651 s … 100.104 s 10 runs Relative speed comparison 1.12 ± 0.03 ./build/bin/bitcoin-cli -datadir=/mnt/my_storage/BitcoinData -rpcclienttimeout=0 -named gettxoutsetinfo hash_type='none' use_index='false' >/dev/null (COMMIT = 64a88c8) 1.00 ./build/bin/bitcoin-cli -datadir=/mnt/my_storage/BitcoinData -rpcclienttimeout=0 -named gettxoutsetinfo hash_type='none' use_index='false' >/dev/null (COMMIT = 57dc020) ``` </details> ACKs for top commit: achow101: ACK 5de2f97 sedited: ACK 5de2f97 andrewtoth: ACK 5de2f97 optout21: ACK 5de2f97 theStack: ACK 5de2f97 Tree-SHA512: 33b62149625b3ce2a378be9b4dffa361f11e324a2768e460c549b9b704efa78bf96ef5e24487d0cec82c18dafff6ba4571c06ad545684cf8738f38b9d21e9b0c
`WalletBalanceMine` duplicated `WalletBalanceClean` exactly. Remove the duplicate registration so the balance benchmark list stays distinct.
`MempoolCheckEphemeralSpends` wrote every prevout to `tx2.vin[0]` instead of `tx2.vin[i]`. That left only one child input pointing at the parent transaction, while the remaining inputs kept default prevouts. Write each prevout to `vin[i]` instead. Add an assertion that the last child input spends the last parent output. Co-authored-by: David Gumberg <davidzgumberg@gmail.com>
`setup()` in nanobench runs once per epoch, not once per timed call. If an epoch executes the benchmark body multiple times, `setup()` can silently leave later iterations with different preconditions. Make `setup()` force `epochIterations(1)` itself: keep rejecting incompatible larger explicit epoch sizes, but allow existing single-iteration callers such as `-sanity-check`. With `setup()` handling this centrally, remove the redundant `epochIterations(1)` calls from the benchmarks that use it. Co-authored-by: David Gumberg <davidzgumberg@gmail.com>
df44afd kernel: expose btck_block_tree_entry_get_ancestor (Peter Zafonte) Pull request description: Callers building a block locator from a stale tip currently have to walk back one entry at a time using btck_block_tree_entry_get_previous. For large step sizes this means hundreds of C API calls per locator entry. This exposes GetAncestor via btck_block_tree_entry_get_ancestor, which uses the existing CBlockIndex skiplist to reach any ancestor height in O(log N) and works on any chain, not just the active chain. Includes a C++ convenience wrapper and tests ~~covering same-height, genesis, and out-of-range cases~~ in btck_block_tree_entry_tests. See previous discussion here #34843 ACKs for top commit: alexanderwiederin: ACK df44afd sedited: ACK df44afd Tree-SHA512: e953b634235129fa972e7f148016e8aee7bad61e16d50191d6724d11c71fe0f7623152ff61aa4c4ec5c8a5b500aaeb36613b5b2ff1e404d01ed7b65aa861f7aa
e6430b2 bench: make `setup()` use single-iteration epochs (Lőrinc) ba0078e bench: fix ephemeral spend inputs (Lőrinc) b8b7f89 bench: drop duplicate balance benchmark (Lőrinc) Pull request description: ### Context Found while reviewing #35018, which led to a few additional benchmark fixes. ### Problem `MempoolCheckEphemeralSpends` populated every child input by writing to `vin[0]`. That left only one child input pointing at the parent transaction, while the remaining inputs kept default prevouts. `WalletBalanceMine` duplicated `WalletBalanceClean` exactly. The same review also showed that nanobench `setup()` can be misused as though it runs once per timed call. It actually runs once per epoch, so using it with multiple epoch iterations can silently leave later iterations with different preconditions. ### Fix Write each ephemeral-spend prevout to the matching child input and assert that the last input spends the last parent output. Remove the duplicate wallet balance benchmark registration. Add a nanobench assertion that `setup()` is only used with `epochIterations(1)`. > [!NOTE] > This PR included a few benchmark adjustments originally which made the tests unstable and were reverted. ACKs for top commit: davidgumberg: crACK e6430b2 sedited: ACK e6430b2 Tree-SHA512: 667c67975d7a3e21333f6c8a4e037ad700bc6dcbd7039e002534e4f2a9328195a618b78accf097e7c43402a1f1b25e32b0c39642fd465729ab4c337abe4f23f1
mempoolfullrbf=1 behaviour has been the default since v28 and the argument has been removed since v29 subsequently. The getmempoolinfo RPC need not return the deprecated `fullrbf` key in the response anymore unless the user requests it via -deprecatedrpc=fullrbf node argument.
…eponses RPCs: getrawmempool, getmempoolancestors, getmempooldescendants, getmempoolentry This key has been marked deprecated since v29, it can be removed from the RPC response now unless the user requests it via the -deprecatedrpc=bip125 node argument.
This commit can be reviewed with the git options: --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
The alias of the size() method is confusing, because: * It claims to be part of the Bitcoin Core stream subset (streams interface), but this is not used by any other stream interface. Mostly the `write(std::span)` and `read(std::span)` define the stream interface. * It casts the size_t to i32, but the only place that calls the function casts that back to size_t. * Providing this alias for size() without a proper reason is confusing. Fix all issues by removing it and using the size() method.
Recently, I ran clang-tidy in my system and had to figure out setting it up correcty on my system (macOS) & understanding its output while tweaking few arguments that made the output easier to parse. This patch documents those in the developer notes.
This follows the approach of the MiB and GiB operators. This allows to remove some `if constexpr (SIZE_MAX == UINT64_MAX)` in the tests.
On some systems the old example of using xargs to pass list of profraw files can exceed xargs's argument limit resulting in multiple executions of llvm-profdata merge command which overwrites it's output file losing coverage information. Switch to using a file with a list of filenames.
fa43da2 refactor: Run ShouldWarnOversizedDbCache calculation in u64 (MarcoFalke) fa58017 util: Return uint64_t from _MiB and _GiB operators (MarcoFalke) fa9ddb0 test: Use MiB operator directly in cuckoocache_tests (MarcoFalke) Pull request description: Currently, the `_MiB` literal operator returns a value of type `size_t`. This is brittle and confusing: * It is inherently impossible to represent larger values, like storage device usage. * Similarly, it is not possible to even type an upper cap on the memory, see the failure in #34692 (comment) * Using `size_t` isn't required here. The function is evaluated at compile time, and even 32-bit compilers can evaluate an `uint64_t` at compile time. * Using `size_t` here encourages it to be used in more places, which will likely lead to more bugs and CVEs, such as #34109, #33724, etc. Fix all issues, by: * Marking the operator `consteval`, to ensure it is really only called at compile-time. * Returning an `uint64_t` value. * Using it in the place where it was previously not possible. Review note: This should have no downside, because the C++11 narrowing checks continue to work as expected. For example, typing `uint8_t{1_MiB};` will continue to fail with the correct error message `error: constant expression evaluates to 1048576 which cannot be narrowed to type 'uint8_t' (aka 'unsigned char') [-Wc++11-narrowing]` (like it does on current master). ACKs for top commit: l0rinc: ACK fa43da2 achow101: ACK fa43da2 hebasto: ACK fa43da2, I have reviewed the code and it looks OK. hodlinator: ACK fa43da2 Tree-SHA512: e19f6bd18c519c2a56d44fbc9e65b02e630bc71170f02a8a8955bf5c9bb2924b55ec2f810de2bbc961162aa668b7383c9fe9b5261fa1de7d9e8413003c32fa87
2a90b61 Add release notes for exportasmap (Fabian Jahr) 8cb2d92 rpc: Add exportasmap RPC (Fabian Jahr) Pull request description: This depends on the embedded data PR itself (#28792), until merge this will remain in draft status. All commit but the last one are from there. There has been interest in exporting the embedded data back into a file. This is implemented here with a simple `exportasmap` RPC which provides this functionality. The exported file can be used to verify the data integrity or statistical analysis using e.g. `contrib/asmap-tool.py`. ACKs for top commit: achow101: ACK 2a90b61 sedited: ACK 2a90b61 seduless: Tested ACK 2a90b61 Tree-SHA512: c8453078efe916ed95bff0695aabd9123d805970e037fad5e9317f4d43e2a4b21970030265bc82b00f3a222ee3db11346eef1fb6fc9393429b9bc6449f1830f1
fa20410 streams: Remove confusing DataStream::in_avail() (MarcoFalke) fa5ab02 move-only: Extract ProcessPong() helper (MarcoFalke) Pull request description: The `in_avail` alias is confusing (see commit message), so remove it. Also extract the `ProcessPong()` helper, as the only place that called `in_avail`. ACKs for top commit: l0rinc: code review ACK fa20410 sedited: ACK fa20410 Tree-SHA512: b5a069abb471c42d39e91a3c55fc7f18fb9ccf8591408b3dd43210d0287785369b6a5d6b59b847d85247a494f208dbbe4a7e88b24caf9b3e6c8fea2157cd0ee0
…-move 14547eb kernel: guard btck::Handle move-assignment against self-move (Thomas) Pull request description: The move-assignment operator for `btck::Handle<>` in `src/kernel/bitcoinkernel_wrapper.h` unconditionally called `DestroyFunc(m_ptr)` before reading the source pointer. On a self-move (`h = std::move(h)`), this destroys the held resource and then restores the now-dangling pointer via `std::exchange(other.m_ptr, nullptr)` (since `&other == this`), which leaves `m_ptr` pointing at freed memory. The destructor then calls `DestroyFunc` again on it, resulting in a double-free. Trace of `h = std::move(h)` with the old code, where `h.m_ptr == P`: 1. `DestroyFunc(m_ptr)` -> `delete P`. `this->m_ptr` still literally stores the now-dangling value `P`. 2. `std::exchange(other.m_ptr, nullptr)` — because `&other == this`, this reads the dangling `P` back, writes `nullptr` to `m_ptr`, and returns `P`. 3. `m_ptr = P` restores the dangling pointer. 4. `~Handle()` later runs `DestroyFunc(P)` -> double free, UB. The copy-assignment operator already guards against self-assignment with `if (this != &other)`; the move variant should be symmetric. The standard library requires moved-from objects to be in a valid (at minimum, safely destructible) state, which the previous implementation violated when source and destination alias. `Handle<>` is the base class of 16 public types in the kernel C++ API wrapper (`Transaction`, `Block`, `BlockHeader`, `ChainParams`, `Context`, `Coin`, `BlockValidationState`, `ScriptPubkey`, `TransactionOutput`, `Txid`, `OutPoint`, `TransactionInput`, `PrecomputedTransactionData`, `BlockHash`, `BlockSpentOutputs`, `TransactionSpentOutputs`), so self-move can arise from generic algorithms operating on containers of these types (`std::sort`, `std::remove`, erase-remove idioms, etc.). Fix: mirror the copy-assignment pattern by guarding the move-assignment body with `if (this != &other)`, making a self-move a no-op. Also extend `CheckHandle` in `src/test/kernel/test_kernel.cpp` to exercise self-move-assignment for every `Handle`-derived type, checking that the stored pointer and the serialized bytes (where applicable) are unchanged. ACKs for top commit: sedited: Thanks, ACK 14547eb alexanderwiederin: ACK 14547eb yuvicc: lgtm ACK 14547eb Tree-SHA512: 334f5ad3045d5f9b06cc0dd096bc911a992773c59cc469765c2082975a9f7a90f2349b9ad94b4b5127290de1fab2f5424a621384c1b4bb9152de99f5da8ed6aa
ef21e29 doc: update llvm based coverage example (Max Edwards) Pull request description: The old example was working on my Mac but when run on Linux I was finding that I had missing coverage information. On some systems the old example of using xargs to pass list of profraw files can exceed xargs's argument limit resulting in multiple executions of llvm-profdata merge command which overwrites it's output file losing coverage information. This PR switches the example to using a file with a list of filenames. There are other ways to solve this like using bash's globbing or a one liner with temporary file but I felt like this was the most simple, robust and easy to understand. ACKs for top commit: vasild: ACK ef21e29 sedited: ACK ef21e29 Tree-SHA512: 2726d9b38088c1c0b6f04d4622bdc4ea28e16b13a962a3afc842798aa5e0eee4682752c73707c5580a43bdb9d0d84d542afd357f166e4dbb7f6beb7afc0522c8
a1e534b doc: clarify clang-tidy in developer notes (rkrux) Pull request description: Recently, I ran clang-tidy in my system and had to figure out setting it up correcty on my system (macOS) & understanding its output while tweaking few arguments that made the output easier to parse. This patch documents those in the developer notes. ACKs for top commit: sedited: ACK a1e534b Tree-SHA512: e95cd44cdad9a9011c18136ec7f1db4e73f78f11c517f94c14cf128beaaea150881868c27e7c5c710c9cab94d285d61dc5e5278f347c229f5df495cd49c532ab
…able from mempool RPCs 8deed0d doc: add release notes for PR 34911 (rkrux) 1a85ca1 rpc, mempool: rpcdeprecate `bip125-replaceable` key in mempool RPCs reponses (rkrux) f89d18c rpc, mempool: rpcdeprecate `fullrbf` key in getmempoolinfo RPC response (rkrux) Pull request description: mempoolfullrbf=1 behaviour has been the default since v28 and the argument has been removed since v29 subsequently. The getmempoolinfo RPC need not return the deprecated `fullrbf` key in the response anymore unless the user requests it via -deprecatedrpc=fullrbf node argument. Also, remove the `bip125-replaceable` key from the mempool RPCs responses (because it, too, has been deprecated since v29) unless the user requests it via -deprecatedrpc=bip125 node argument. ACKs for top commit: optout21: ACK 8deed0d sedited: ACK 8deed0d instagibbs: ACK 8deed0d Tree-SHA512: dfbbdf04ae25dafe81758bd85715af80d736e21bb913ab09ff8fd8225587b7a1bf1dd6b0f7ea05135504e11d43371d98e5f5fc6e3717d581e870e8c2bf97811b
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
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.
build: Drop ZeroMQ patch for glibc < 2.12 (Hennadii Stepanov)
079df96 build: Drop ZeroMQ patch for Mingw-w64 < 4.0 (Hennadii Stepanov)