windows: replace deprecated codecvt with fs::u8path - #452
Open
kevkevinpal wants to merge 290 commits into
Open
Conversation
Instead of sending 403 Forbidden, disconnect as soon as possible. To facilitate unit testing, this commit includes a refactor that moves the subnet allow list and relevant methods into the HTTPServer class instead of file-scope static scope.
`ResolveCollisions_()` had a fallback for the case where a pending tried collision no longer collided because the destination tried slot became empty. Under current addrman invariants this cannot happen: once an entry is added to `m_tried_collisions`, the corresponding tried slot remains occupied until the collision is resolved. The only other valid outcomes are that the pending new entry disappears or becomes invalid, both of which are already handled. Remove the dead branch and replace the implicit assumption with assertions in `ResolveCollisions_()` and `SelectTriedCollision_()`.
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
This avoids relying on the bump that occurs through the normal INV process, which is confusing.
Per-peer rate limiting introduces storage and compute costs proportional to the number of peers. This has caused severe bugs in the past, and continues to be a risk in the event of periods of extremely high rates of transaction submission. Avoid these problems by always completely emptying the m_tx_inventory_to_send queue when processing it. Note that this increases the potential size of INV messages we send for normal tx relay from ~1000 (limited by INVENTORY_BROADCAST_MAX) to potentially 50000 (limited by MAX_INV_SZ).
Add a method for (partially) sorting a batch of transactions (specified as a std::vector of wtxids) per mempool order, designed for transaction relay.
Remove CompareInvMempoolOrder, replacing it with the new ExtractBestByMiningScoreWithTopology. The trickle send code is reworked accordingly.
Now unused; replaced by ExtractBestByMiningScoreWithTopology.
This is a simple token bucket parameterized on clock type, used in the following commit.
Without the per-peer rate limiting, nodes can act as an amplifier for transaction spam -- receiving many transactions from one node, but relaying each of them to over 100 other nodes. Limit the impact of this by providing a global rate limit. This is implemented using dual token buckets, one that consumes a token for every transaction, and one that consumes a token for every serialized byte. This rate limits both per-tx resource usage (eg INV messages) and overall relay bandwidth. Main bucket parameters: * Count: 14tx/s rate, 420tx (30s) capacity * Size: 12MB/600s rate (4-6 blocks per target block interval), 50MB capacity The size bucket is expected to be large enough to almost never have an impact in normal usage, even during transaction storms, and is primarily intended to mitigate attack-like scenarios. Outbound connections get a separate pair of buckets, with rates boosted by a 2.5x multiplier. This avoids the excessive memory and CPU usage due to the 100x multiplier from the queues being per-peer. Note that this also reduces the size of INV messages we send for general tx relay back to a more reasonable level of under 600 txs in 99.999% of cases.
Adds a debug-only configuration option to set the target transaction/second rate for relay to inbound connections. This is mostly intended to be set to artificially low values to aid in testing behaviour when a backlog occurs, but is also available in case the default 14tx/s target is somehow too low in practice.
Add `tx_send_rate` and `inv_buckets` fields to getnetworkinfo. The `inv_buckets` field has separate `inbound` and `outbound` entries, reporting backlog count, count tokens, and size tokens. Useful for monitoring relay behavior.
Replace GetLinearization's four per-call heap-allocated vectors (the ready chunk/transaction heaps and both dependency counters) with stack arrays and explicit size counters. This especially benefits linearization of small clusters, where the allocations make up a significant fraction of the total runtime.
Reserve capacity for one entry per transaction in m_suboptimal_chunks at construction time, avoiding repeated reallocations while MakeTopological and StartOptimizing fill the queue.
MergeChunks scans the top chunk's transactions twice: once to count the dependencies on the bottom chunk, and once to locate the randomly picked one. Remember the per-transaction dependency counts from the first pass, so the second pass only computes the intersection of the selected transaction, rather than one per transaction scanned.
Previously ipc_test.cpp contained tests which depended on libmultiprocess and ipc_tests.cpp contained tests which didn't. Separation was needed because libmultiprocess tests need to be built with additional include and link paths, and cmake only has good support for setting these on libraries, not source files. The separation also allowed the add_boost_test custom cmake function to work with no changes, because it could find the boost test registration in ipc_tests.cpp, and then ipc_tests.cpp would run the tests in ipc_test.cpp without them needing to be registered in boost. But with windows support being added, the parse address test can't easily avoid a dependecy on libmultiprocess, because it depends on the ipc/process.h header, and ipc/process.h header will now need platform-specific ProcessId and SocketId types defined by libmultiprocess, rather than plain ints. With all ipc tests depending on libmultiprocess, there is not really a rationale for having separate test files anymore, so this change combines them, and move the cmake add_boost_test function definition so it can be used instead of target_sources to register ipc_tests.cpp with ctest. The change prevents CI errors from including ipc/process.h in ipc_tests.cpp: In file included from /Users/runner/work/bitcoin/bitcoin/repo_archive/src/ipc/test/ipc_tests.cpp:5: In file included from /Users/runner/work/bitcoin/bitcoin/repo_archive/src/ipc/process.h:11: /Users/runner/work/bitcoin/bitcoin/repo_archive/src/ipc/util.h:14:10: fatal error: 'kj/debug.h' file not found 14 | #include <kj/debug.h> https://github.com/bitcoin/bitcoin/actions/runs/24465865499/job/71492617687?pr=35084
Pass exe_name parameter to ipc::Protocol class constructor instead. It never really made sense to have exe parameters as part of the protocol interface and removing them makes adding new features like windows support easier. The exe name values are only used for logging and debuggging purposes to distinguish log messages from different processes.
This just changes Protocol class field order to make sure class members are not destroyed before the event loop thread exits. There is no change in behavior. The change is just being made to clarify intent and avoid potential bugs.
Avoid hardcoded forward slashes is ParseAddress test, use native path separators instead.
Keep standard headers separate from posix headers
Avoid compile error from MSVC: D:\a\bitcoin\bitcoin\src\ipc\interfaces.cpp(24,1): error C1083: Cannot open include file: 'unistd.h': No such file or directory MinGW provides this header but MSVC does not. Header is unneeded on windows because HandleCtrlC code that uses it is not compiled on windows.
Use ProcessId type instead of int to represent process ids to be compatible with an upcoming version of libmultiprocess which adds windows support.
Use SocketId type instead of int to represent socket ids to be compatible with an upcoming version of libmultiprocess which adds windows support.
This is more explicit and comes with compile-time checks to ensure all default types are covered.
This will render all ANY results. However, the one in the help RPC should still be hidden. This is done by applying HelpElisionSkip and also skipping over empty result sections in ToDescriptionString. Also, run clang-format. Can be reviewed via --ignore-all-space
There is no recursion.
This is clearer, because it does not encode the default value in the source code syntax. Also, using named args is clearer than positional args. Also, run clang-format on the new code.
This refactor follow other uses of CLIENT_NAME, such as in RPCMethod stop().
…ster_linearize namespace 21b4b79 test: Move cluster_linearize.h contents into cluster_linearize namespace (Hennadii Stepanov) Pull request description: Clang recently enabled `-Wunused-template` under `-Wall` (see llvm/llvm-project#206123, llvm/llvm-project#207848, llvm/llvm-project#208001). Our codebase [triggers](https://my.cdash.org/builds/3714664/build) some of these warnings. This PR: 1. Avoids Clang's `-Wunused-template` warnings in `src/test/util/cluster_linearize.h` when building the `bench_bitcoin` and `fuzz` targets. 2. Follows the C++ Core Guidelines: "[SF.21: Don't use an unnamed (anonymous) namespace in a header](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#sf21-dont-use-an-unnamed-anonymous-namespace-in-a-header)". 3. Drops `[[maybe_unused]]` annotations, which are no longer needed after changing the linkage from internal to external. Along with bitcoin#35679, this resolves all instances of this warning in the test/bench/fuzz code. Another related change: bitcoin-core/minisketch#102. --- Steps to reproduce on the master branch @ 70d9ec7: ```console $ CXXFLAGS="-Wunused-template" cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DBUILD_BENCH=ON $ cmake --build build -t test_bitcoin $ cmake --build build -t bench_bitcoin [19/62] Building CXX object src/bench/CMakeFiles/bench_bitcoin.dir/cluster_linearize.cpp.o In file included from /home/hebasto/dev/bitcoin/src/bench/cluster_linearize.cpp:9: /home/hebasto/dev/bitcoin/src/test/util/cluster_linearize.h:122:17: warning: unused function template 'Ser' [-Wunused-template] 122 | static void Ser(Stream& s, const DepGraph<SetType>& depgraph) | ^~~ /home/hebasto/dev/bitcoin/src/test/util/cluster_linearize.h:286:6: warning: unused function template 'SanityCheck' [-Wunused-template] 286 | void SanityCheck(const DepGraph<SetType>& depgraph) | ^~~~~~~~~~~ /home/hebasto/dev/bitcoin/src/test/util/cluster_linearize.h:383:6: warning: unused function template 'SanityCheck' [-Wunused-template] 383 | void SanityCheck(const DepGraph<SetType>& depgraph, std::span<const DepGraphIndex> linearization) | ^~~~~~~~~~~ 3 warnings generated. [62/62] Linking CXX executable bin/bench_bitcoin ``` ACKs for top commit: maflcko: lgtm ACK 21b4b79 sedited: ACK 21b4b79 Tree-SHA512: 893082c2dc3295b68738c438094656b3568f85b95e0e2f9b0b9dd178d0613e4cb50cd2300a9595e390b9d0a4c650ca78d2772dd1010c178756cefbeef6e2ed94
The rule is named misc-anonymous-namespace-in-header in clang-tidy-22, but use fuchsia-header-anon-namespaces for compat with clang-tidy-21.
aeca061 rpc: reject null for optional parameters (Ruslan Kasheparov) Pull request description: Treat explicitly passed `null` as missing for optional RPC parameters that are required in certain contexts. ACKs for top commit: achow101: ACK aeca061 maflcko: review ACK aeca061 🥚 sedited: ACK aeca061 Tree-SHA512: 60f146085fd20e532ba3cbefdb76d430938168621706a20b2b62a34318499fd72a8c934b08f690f9b72d19ed26581094517a0986586f95bc4b23fa8743b24d11
c9cedeb coins: group private cache helpers (Lőrinc) Pull request description: **Problem:** `CCoinsViewCache::ReallocateCache()` is public even though only `Flush()` uses it. The private helper declarations are also split between the start and end of the class. **Fix:** Move `ReallocateCache()` and `FetchCoin()` into the existing private section. ACKs for top commit: optout21: reACK c9cedeb sedited: ACK c9cedeb Tree-SHA512: d06062daa9976d767375cd272b3c4ddd838e83fc8b3ac57f6b89a4d8d8837277211786cdd66a794b1a84a39cf6b38f2b9a01699d16e0a648d27b1b93cd0f33b5
3ac8b80 test: test the result order of a multiple import request is correct (Pol Espinasa) e4732bf test: test invalid or missing timestamp throws importdescriptors (Pol Espinasa) 07fb58b test: Test a locked wallet rejects an empty importdescriptors request (Pol Espinasa) Pull request description: In addition to bitcoin#35179 (already merged) this adds more missing test coverage that was detected while rebasing bitcoin#34861. The three tests added checks: - Locked wallet throws because of being locked if giving an empty importdescriptors request. - Invalid or missing timestamp throws as a top level RPC error and not a per-item error. - The order of the requests and the response is the same, even if failing or succeeding. ACKs for top commit: nebula-21: ACK 3ac8b80 Bicaru20: re-ACK 3ac8b80 brunoerg: reACK 3ac8b80 Tree-SHA512: b6ba9e16bbdbefcab2529f49f9aab0ae8885bd2d381c6eec36ae442dea1aa2361e6fb339ab5bc2c51c3bef6216d8d939db53e57ec577f05fe54c07fc46f8f255
…namespace in header fa93132 refactor: Enable misc-definitions-in-headers (MarcoFalke) fa5ca87 refactor: Enable clang-tidy rule to reject anon namespace in header (MarcoFalke) fafe504 refactor: Use C++20 std::identity over IntIdentity (MarcoFalke) Pull request description: This avoids issues like bitcoin#35737 from passing CI in the first place. Also, the rule can help to avoid ODR issues, according to https://clang.llvm.org/extra/clang-tidy/checks/fuchsia/header-anon-namespaces.html This follows https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#sf21-dont-use-an-unnamed-anonymous-namespace-in-a-header ACKs for top commit: sedited: ACK fa93132 Tree-SHA512: c1a9d573aed9f295a9fb1931e96e37a9c39e2339392af79d0a3441ebd29282812c2614eab13a01e0d508802f66b73e8f2387ffcbb7dd28676b4fa43f81f9a680
There is no reason we should process conflicting advertisements for transactions, as they cannot be both accepted into our mempool. Avoid processing these and doing spurious work.
This is unused since cluster-mempool. See the commit that forgot to remove it: ```sh $ git show 216e693 -U99999 | grep newFeeRate CFeeRate newFeeRate(ws.m_modified_fees, ws.m_vsize); - if (const auto err_string{PaysMoreThanConflicts(ws.m_iters_conflicting, newFeeRate, hash)}) { ```
…ementChecks fa9a9a8 refactor: Remove unused newFeeRate var in ReplacementChecks (MarcoFalke) Pull request description: This is unused since cluster-mempool, so it seems confusing to keep around. See the commit that forgot to remove it: ```sh $ git show 216e693 -U99999 | grep newFeeRate CFeeRate newFeeRate(ws.m_modified_fees, ws.m_vsize); - if (const auto err_string{PaysMoreThanConflicts(ws.m_iters_conflicting, newFeeRate, hash)}) { ``` ACKs for top commit: sedited: ACK fa9a9a8 Tree-SHA512: f8804109c59dfe37da754cb1924cc16abbc792fbb3619249a1edbc005f66fd88357c4810926625a375415c5be4ff056d68af3029f8075590252289c1d753a2fd
Like authproxy.py, so that tests can work without having to think whether the cli was used or not.
Also, modify send_cli, so that the test can be run under --usecli
sourceware is blocking/rate-limiting http(s) git requests to combat AI scrapers. Switch to a live mirror hosted at fish.foo Use exported GIT_CONFIG_* to avoid setting `git config --global` which could clobber a user running on bare host, but still propagate to child git processes spawned by `make`.
This reverts commit fa06ea4.
81fcecf Revert "ci: Temporarily remove riscv32 config from GHA matrix" (will) b283e17 ci: use mirror for riscv submodules (will) Pull request description: The https transport is rate-limited to block AI scrapers. Switch to a live mirror on fish.foo to re-enable the riscv job. ACKs for top commit: maflcko: lgtm ACK 81fcecf sedited: ACK 81fcecf Tree-SHA512: 1891046d9847b904a3e4be25b7f313fc2413ad1cc0b2c09b2300f81a2f1f691dad816fa5fcfeeae263f8d5f868d229aca5a7ab3f227ce650d359068488bc1e21
…r load fa7bc26 test: Check that RPCs do not time out, even under load (MarcoFalke) fa2bd96 test: Map cli CalledProcessError on server error to JSONRPCException (MarcoFalke) Pull request description: It turns out there is no test currently to check that the RPC server does not time out under load. With "load" I mean a flood of trivial payloads. That is, the only work needed is JSON encoding and decoding of (let's say) a block of data of 2 MB or so. This may take a few milliseconds, but should never take more than a few seconds. So add a test for this. ACKs for top commit: enirox001: ACK bitcoin@fa7bc26 sedited: ACK fa7bc26 Tree-SHA512: c60646981b7449c757e9fad499e1cd71030376ffb2ae687c8136c6f70accd0a2d76a4cbfc7dd1bc6626ea3a5310a33616a36cd682cc5c4371ec86aa2c8641aeb
e98ffd4 doc: fix stale bitcoin_en.xlf reference (cyb3ralbert) Pull request description: `doc/release-process.md` still instructs users to create the Transifex resource from `src/qt/locale/bitcoin_en.xlf`, even though that file no longer exists. It was removed in bitcoin#34808, which switched the Transifex source to the native Qt `.ts` file. That PR updated the other references to `.xlf` in this document, but this one was missed. The last step in the same list already refers to `bitcoin_en.ts`, as does `.tx/config`. This patch updates the remaining outdated reference. Docs-only change. No tests run. ACKs for top commit: hebasto: ACK e98ffd4. This was overlooked in bitcoin#34808. Tree-SHA512: fc6a295aae8a58e72e3324f52849255fe5c49bc53af71733de44489dd8a5dea86e3dfeaaf01cb47c506e717019a390de97a71505babcb1c98626f5c2fd98274f
1278a59 net_processing: process unique tx INVs only (Gregory Sanders) Pull request description: There is no reason we should process conflicting advertisements for transactions, as they cannot be both accepted into our mempool. Avoid processing these and doing spurious work. Should be no change in observable behavior. ACKs for top commit: ajtowns: ACK 1278a59 fjahr: ACK 1278a59 l0rinc: ACK 1278a59 Tree-SHA512: c62ceed2cc634c8c99509a8495e5f9bb6d4d8d050942f709a6539ae4dfe1ec628ce7ab0ec1392656809e0d827d1fecf2e8fb9bb4600d13a5bf9a34c9e9e3ad6e
7502b9d fuzz: check http_request body matches framing (ameen-alam) Pull request description: The http_request target asserted that ReadBody() returns an empty string. That held for the libevent-based http_libevent::HTTPRequest, where the harness only parsed the request line and headers and never populated a body. Commit 9c20859 (PR bitcoin#35182) replaced libevent with http_bitcoin::HTTPRequest, and the target was switched over in e427c22; its LoadBody() now decodes Content-Length and chunked bodies per RFC 9112, so any fully-parsed request carrying a body trips the stale assertion (e.g. "POST / HTTP/1.1\r\nContent-Length: 3\r\n\r\nabc"). Replace the emptiness check with a framing-consistency check that mirrors LoadBody()'s own branch logic: a chunked body is bounded by MAX_BODY_SIZE, a Content-Length body is exactly that many bytes, and a request with neither framing header has no body. This strengthens the target instead of dropping the assertion. **Steps to reproduce (old assertion):** Build the fuzz binary and pass this input as a file to the `http_request` target: `POST / HTTP/1.1\r\nContent-Length: 3\r\n\r\nabc` → `test/fuzz/http_request.cpp:49: Assertion 'body.empty()' failed` **Testing the fix:** Ran the updated target ~16 min under libFuzzer with ASAN/UBSAN (14.2M execs, no crashes), plus targeted inputs for each branch: Content-Length body, chunked, `Transfer-Encoding: identity` + Content-Length, no framing headers, and `Content-Length: 0`. Happy to contribute the repro input to qa-assets as a follow-up. ACKs for top commit: pinheadmz: ACK 7502b9d marcofleon: tACK 7502b9d Tree-SHA512: 4f2eb6bdb3a4556866a84fe0f1d0d8cf506e2efd1b1c7493a99f67ca452b31a140034d418c4064142b1a66c3a6c34b97df0e2b12c21ea86cd4019ffc7cff3b27
…-only fac4b06 refactor: Use CLIENT_NAME in buildOpenRPCDoc (MarcoFalke) fa3aadb refactor: Use self.Arg<bool> in getopenrpcinfo (MarcoFalke) fa1871a refactor: Remove stale NOLINTNEXTLINE above GetAddressInfoBaseFields (MarcoFalke) fa22647 rpc: Properly make RPCResult::Type::ANY non-test-only (MarcoFalke) fa1242d refactor: Use std::visit in ApplyArgFallback (MarcoFalke) Pull request description: Commit 6a1a66c attempted to properly render RPC results of the type `ANY`. However, the commit is incomplete. Fix it, by properly rendering all `ANY` types. Moreover, a few trivial refactors after bitcoin#34683 are included here. ACKs for top commit: sedited: ACK fac4b06 willcl-ark: ACK fac4b06 Tree-SHA512: b77f1956e4feb9afb93d38245491eec190c4538aec7ac57c327fb2efd86c7da6ee8a1406f81fc18fac06bae77a7d38845a537ca0b8f4ae2d94c38eefd8e88dc1
264555a rpc: avoid descriptor range counter overflow (Lőrinc) 143a13f test: characterize descriptor range endpoint (Lőrinc) Pull request description: **Problem:** The authenticated `scantxoutset`, `scanblocks`, `getdescriptoractivity`, `utxoupdatepsbt`, and `descriptorprocesspsbt` RPCs share a descriptor expansion helper that iterates inclusive `int64_t` ranges with an `int` counter. A ranged descriptor with an explicit `[begin, end]` range ending at `2^31 - 1` expands that valid position, then overflows when advancing the counter to exit the loop. Trap-enabled builds terminate, while other builds invoke undefined behavior. **Fix:** Use `int64_t` for loop control so the one-past-the-end value is representable and every position passed to `Descriptor::Expand()` remains within its existing `int` range. Related: [bitcoin#26275](bitcoin#26275) fixed the same endpoint overflow in `deriveaddresses`. ACKs for top commit: achow101: ACK 264555a polespinasa: ACK 264555a sedited: ACK 264555a Tree-SHA512: 4326182b5897b6f6672e5f7c7296eafdbb6e3b5ed901d61e8fa2cff9b19d372bb8adb88902368dc520ed400e12dd5264ea68677d9ce0feec76fa2ef55fa0d2f4
Drop wstring_convert/codecvt and the related wide process calls (_wsystem, _wexecvp, CreateProcessW) in favor of ::system, _execvp, and CreateProcess.
kevkevinpal
force-pushed
the
remove-deprecated-codecvt
branch
from
August 6, 2026 23:22
00a9ef2 to
6b6d77c
Compare
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
20 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.
Remove std::wstring_convert/codecvt (deprecated in C++17, removed in C++26) in favor of fs::u8path().wstring() for UTF-8 to wide conversions.