Conversation
Crypt-iQ
force-pushed
the
06082026/cnodestate
branch
2 times, most recently
from
June 18, 2026 10:46
42031e8 to
765f659
Compare
Crypt-iQ
force-pushed
the
06082026/cnodestate
branch
from
August 26, 2026 00:15
765f659 to
3861ab4
Compare
This clarifies the initial index sync thread is blocked.
Without the drain, a BlockConnected event may execute during shutdown and lead to memory races.
…commands db39de5 doc: add `-walletnotify` security note (Lőrinc) 1f9dfab refactor: use string views in `ReplaceAll` (Lőrinc) 469b0e5 util: make `ReplaceAll` literal (Lőrinc) 604d7e8 test: characterize walletnotify shell injection (Lőrinc) 4efaa67 test: simplify `ReplaceAll` coverage (Lőrinc) Pull request description: **Problem:** On non-Windows builds, operators can configure `-walletnotify` to run a command for wallet transactions, with `%w` replaced by the shell-escaped wallet name. An authenticated RPC caller allowed to create wallets can supply a name containing `$'`, request an address, and send a transaction to it. While replacing `%w`, `ReplaceAll()` passes the escaped wallet name to `std::regex_replace()` as replacement text. There, `$'` copies the command suffix into the escaped name, breaking its quote accounting and allowing shell metacharacters in the wallet name to alter the command. `runCommand()` passes the result to `system()`, so a suitable command template could execute additional shell commands as the node process account. It is not reachable over P2P or by an unauthenticated network peer. bitcoin#25803 introduced this behavior in v24 when it replaced Boost's literal substitution with `std::regex_replace()`. **Fix:** Restore the literal, non-recursive contract `ReplaceAll()` had before bitcoin#25803, matching every current caller's literal search and replacement text, while the wallet notification test covers a wallet name containing `$'`. **Related:** bitcoin#35833 restricts control characters in new wallet names, while this change fixes replacement metacharacters in `ReplaceAll()`. This was found and disclosed responsibly by the Red Team 🟥. ACKs for top commit: maflcko: re-ACK db39de5 💈 jeanpablojp: re-ACK db39de5 stickies-v: re-ACK db39de5 Tree-SHA512: 0be4adecfee50cb4dab90ae3386079767694a6b1fa1d7bd1f10ef73de88707b232f1ba4975a723c465a4d34d12296d501986c657d93bd8ae0bdced16afad1b5e
Without the drain, a BlockConnected event may execute during shutdown and lead to memory races.
Fixes bitcoin#35632 by allowing both outcomes of a race condition. The server behavior is unchanged: in response to a malformed request we send an error code and disconnect. The issue is that sometimes on Windows the RST is caught by the platform and the receive buffer is discarded before the Python client can process it with recv(). We can also be much more polite to misbehaving clients by implementing SO_LINGER as suggested in bitcoin#35780 but that will require more review.
…ash shutdown fab80e8 test: Avoid unsafe memory race in baseindex_no_commit_ahead_of_flush (MarcoFalke) fa0f14e test: Avoid unsafe memory race in index_reorg_crash shutdown (MarcoFalke) faf9c8e test: Clarify index.GetSummary().synced state in index_reorg_crash (MarcoFalke) Pull request description: Currently, the `index_reorg_crash` test may rarely crash due to UB in sanitizers like TSan or ASan. This is perfectly fine, because it is just a rare test-only issue. However, fix it nonetheless by adding a missing drain of the unused in-flight events. Also, add a small check about the synced state while touching this test. ACKs for top commit: arejula27: ACK fab80e8 furszy: ACK fab80e8 Tree-SHA512: 4423e420421aa37d8b59e053f44c455fafb676102866bdf23988cf72f3d3f265b996bd953583ea8208f1534defb0e16b13ef08644be97e61959dc777a2918e5a
cc577de net: align v2 message type validation with v1 range (Bruno Garcia) Pull request description: BIP324 specifies the 13-byte long-form message type encoding as "an ASCII message type (as in the v1 P2P protocol)", but V2Transport::GetMessageType() accepted bytes up to 0x7F, while for V1 it only accepts printable ASCII (0x20-0x7E). This changes V2 to match V1 on it and add test coverage. ACKs for top commit: nervana21: tACK cc577de ajtowns: utACK cc577de w0xlt: ACK cc577de sedited: ACK cc577de Tree-SHA512: 8c97ee20df2311949bbe9655c7e04507c4b47d3b18766aa6ae51691d0870f8a5c25ea54d74c9afb797754572d057b4240533da6bf3c2e0435f3cb32c5fb1c3af
… port allocation 59ebf55 qa: Use IP_PORTRANGE_HIGH on OpenBSD for dynamic port allocation (Hennadii Stepanov) Pull request description: The default ephemeral port range on OpenBSD (1024-49151) overlaps with the test framework's static port range starting at `TEST_RUNNER_PORT_MIN`, the same way FreeBSD's does (see bitcoin#34346). Extend `set_ephemeral_port_range()` to OpenBSD. The socket option and its values are identical to FreeBSD's, so only the platform check changes. ACKs for top commit: maflcko: lgtm ACK 59ebf55 theStack: utACK 59ebf55 Tree-SHA512: 680235cf3e1799361796c0ff36d5f19bf74f79393057dbd7b38b0e92a7df3af669873c66ca1e82f1c78999c20351663b8960990f3f209af89ee18ce0773eb7de
e85e279 rpc: detail x-bitcoin-unit in openrpc help (will) Pull request description: Addresses review comment about clarifying this field: bitcoin#36131 (comment) ACKs for top commit: sedited: ACK e85e279 Tree-SHA512: 7fd0bef8a5d37cd9d2778463b2193c58ec7cced1aa790a0a5807ef093bd51e729b6c8880e1c12af78293eb68abe791f4ae6e96e0457ce85be717e3e776f5406d
This was pointed out as unused in bitcoin#36103, but that seems like a mistake from bitcoin#30214, where it was introduced. Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
a51df9b test: tolerate race condition in interface_http.py (Matthew Zipkin) Pull request description: Fixes bitcoin#35632 by allowing both outcomes of a race condition. The server behavior is unchanged: in response to a malformed request we send an error code and disconnect. The issue is that sometimes on Windows the RST is caught by the platform and the receive buffer is discarded before the Python client can process it with recv(). We can also be much more polite to misbehaving clients by implementing a lingering close using SO_LINGER as suggested in bitcoin#35780 but that will require more review. The exact error in bitcoin#35632 is hard to produce reliably but there are a few close options for reviewers. I tested this on windows native building with MSVC. In both of these cases the patch from this PR caught the error and passed the test. **RemoteDisconnected: Remote end closed connection without response** ```diff diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 9bb8986..62324d3fea 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -1072,7 +1072,7 @@ std::unique_ptr<HTTPRequest> HTTPRemoteClient::TryReadRequest(const std::shared_ e.what()); // We failed to read a complete request from the buffer - WriteNoStoreErrorReply(*client->m_req, HTTP_BAD_REQUEST); + // WriteNoStoreErrorReply(*client->m_req, HTTP_BAD_REQUEST); client->m_disconnect = true; return nullptr; } ``` **ConnectionResetError: [WinError 10054] An existing connection was forcibly closed by the remote host** ```diff diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 9bb8986..be52acb874 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -1154,6 +1154,11 @@ bool HTTPRemoteClient::MaybeDisconnect(std::chrono::time_point<SteadyClock> now, "Disconnecting HTTP client %s (id=%llu)", m_origin, m_id); + auto sock{GetSock()}; + linger opt{}; + opt.l_onoff = 1; // enable SO_LINGER + opt.l_linger = 0; // zero timeout + sock->SetSockOpt(SOL_SOCKET, SO_LINGER, &opt, sizeof(opt)); return true; } ``` ACKs for top commit: jeanpablojp: re-ACK a51df9b winterrdog: tACK a51df9b janb84: re ACK a51df9b hodlinator: re-ACK a51df9b sedited: ACK a51df9b Tree-SHA512: a6244581b2b51af647452e0dc8cd09cdc8d975dee6a0dc8b8064cad136023dad68b4af987303bced91a662bf5fae22871ea718a6a8e68024158a9aef6c5855ef
5ba9af6 ci: pass LIBCXX_INCLUDE_TESTS=OFF to LLVM build (fanquake) feb3bd4 clang-tidy: remove some performance-* options (fanquake) b4bd12d ci: use LLVM 23 in *san, fuzz, *cross jobs (fanquake) Pull request description: LLVM 23.1.0 was recently released, switch to using it across sanitizer, fuzzer and cross-compilation jobs. ACKs for top commit: hebasto: ACK 5ba9af6, I have reviewed the code and it looks OK. willcl-ark: ACK 5ba9af6 Tree-SHA512: 4d203bf1ec6100a21d9a185a37365d358859bbde79f44f93d2e4f5e3c9686f57ca06d6c73da7423eb234dad5b9501d7a09029a430d23b2bcf8aba95f2d88e66d
4550801 validation: use unused SetTargetBlockHash (fanquake) Pull request description: This was pointed out as unused in bitcoin#36103 by jeanpablojp, but that seems like a mistake from bitcoin#30214, where it was introduced. See: bitcoin#36137 (comment). ACKs for top commit: stickies-v: ACK 4550801 ryanofsky: Code review ACK 4550801 Tree-SHA512: 93ccac48855d384f0443b5a25c79c5e6d720b6b77ad7a2bb52989382666e4e5f32c76dd7473428d6bbb503307ada7213021591ad54e463d9f8034fe2da97d10c
The `gcov`-based `CoverageFuzz` script was introduced in 8b6f1c4, as a CMake's replacement for the legacy `cov_fuzz` target. However, neither `cov_fuzz` nor `CoverageFuzz` has a documented usage. Instead, bitcoin#32206 documented compiling for fuzz coverage using the LLVM/Clang toolchain, which does not involve the `CoverageFuzz` script. This change removes the never-documented `CoverageFuzz` script, which is likely unused.
The string-view helpers return views into their input, while `LineReader` stores one. Annotate their inputs so Clang can warn when a returned or stored view outlives a temporary string.
`Split` can return views into its input, but annotating its old reference warns for lvalue strings. Take the span by value so Clang follows the backing storage. `Split<std::string>` copies results but can still warn, while `SplitString` is unaffected.
m_target_blockhash is paired with a mutable m_cached_target_block that must be kept in sync whenever the hash changes.
During the development of the new HTTP server, the headers were initially stored in an unordered_map, but later it was changed to a vector.
The former conforms to developer-notes.md, the latter increases readability.
Expose CTransaction::version, allowing clients to read the transaction version without slicing serialized bytes. Test the accessor against a range of transaction versions.
60bcf13edf Merge bitcoin-core/crc32c-subtree#10: refactor: use inline constexpr in headers b1ce911a5f refactor: use inline constexpr in headers git-subtree-dir: src/crc32c git-subtree-split: 60bcf13edf19ee3886bf0a283d59e7616b4d112c
Correct six argument metadata entries that produce misleading or invalid OpenRPC defaults. The getdeploymentinfo blockhash fallback and four sighashtype fallbacks describe how omitted arguments are resolved. They are not literal values accepted by the RPCs, so mark them as DefaultHint values. The send include_watching option is boolean, but its string default makes the generated schema internally inconsistent. Use a boolean value, matching the analogous sendall option. Runtime behavior is unchanged.
b7f740c rpc: Correct OpenRPC default metadata (will) Pull request description: `getopenrpcinfo` emits two defaults that do not satisfy their schemas. This changes `getdeploymentinfo.blockhash` to a default hint, since its fallback describes the current chain tip, and makes `send.options.include_watching` default to boolean `false`. ACKs for top commit: nervana21: ACK b7f740c sedited: ACK b7f740c Tree-SHA512: fd99c3642ff39ef13116f7f18e2a93b398e347330ea24e5bfd01744528744e5c6fbff12a56213961f8703218a6100f0ea74a10fe3b1d9a221ba8b1fa307a907a
…e and other canonical descriptor string followups 61edcf9 test: Add 31.1 to wallet back compat (Ava Chow) ebf2f69 test: Simplify miniscript descriptor check in wallet back compat (Ava Chow) f6cbcfd wallet: Document WalletDescriptor::UpdateFrom (Ava Chow) f0f6dce descriptor, doc: Clarify miniscript CompatDescriptorHash (Ava Chow) 405b1d6 descriptor: Explicitly handle use_apostrophe cases (Ava Chow) 64abb3e wallet: Compare descriptors by hash of canonical string (Ava Chow) bb5e832 wallet: Make WalletDescriptor's descriptor const (Ava Chow) 4cc00f7 wallet: Remove WalletDescriptor's default constructor (Ava Chow) Pull request description: Instead of re-computing the canonical descriptor string for every call to `HasWalletDescriptor`. `WalletDescriptor` will now compute it once upon construction and cache the hash of that string. The comparison uses a new `WalletDescriptor::Equals` function which compares the canonical string hashes. The hash is used to avoid holding possibly a large amount of memory for a string that is rarely used. This should fix the performance regression described in bitcoin#35445 (comment) Also addresses several review comments related to documentation and code readability: - bitcoin#35445 (comment) - bitcoin#35445 (comment) - bitcoin#35445 (comment) - bitcoin#35445 (comment) - bitcoin#35445 (comment) - bitcoin#35445 (comment) ACKs for top commit: Sjors: ACK 61edcf9 polespinasa: ACK 61edcf9 Tree-SHA512: d13057cdfa89f9831950502f19159a40ca1da693f47d8ae63613fbc74b588994f6748b9124947b29ad1f035cff6650f8ae3d7396025a2363fcf3d32de002adf1
…cmake`" 4f624bb Revert "build: Remove `cmake/script/CoverageFuzz.cmake`" (sedited) Pull request description: This reverts commit 00a5f9b as requested in bitcoin#36161 (comment) . As noted there the script still has users, so should not be removed. ACKs for top commit: kevkevinpal: ACK [4f624bb](bitcoin@4f624bb) marcofleon: ACK 4f624bb Tree-SHA512: 9f79790d5ccc8787f9325fa46a6732ea99b393ac164a55850c4ee0cb6b0042148389876db6dab6b1cc724a8d2732fc8cacf5d1c383e2fbeb027173b0d3389979
0b46fc9 http: Add missing LIFETIMEBOUND annotations (Hodlinator) Pull request description: Helps Clang detect certain dangling reference issues, in a similar vein as bitcoin#36164. ### Known limitations It doesn't catch invalidation nor brace-initialization. <details><summary>Diff illustrating limitations</summary> ```diff --- a/src/test/httpserver_tests.cpp +++ b/src/test/httpserver_tests.cpp @@ -80,6 +80,15 @@ BOOST_AUTO_TEST_CASE(test_query_parameters) BOOST_AUTO_TEST_CASE(http_headers_tests) { + auto foo = HTTPHeaders{}.FindAll("needle"); // Emits warning + (void)foo; + auto bar{HTTPHeaders{}.FindAll("needle")}; // No warning with Clang 22.1.8 :/ + (void)bar; + + HTTPHeaders test; + auto baz = test.FindAll("needle"); + test.Write("needle", "mutation"); // No warning with Clang 22.1.8 :/ + { // Writing response headers HTTPHeaders headers{}; ``` </details> Clang 24 has experimental invalidation detection so maybe that could be used in the far future: https://clang.llvm.org/docs/LifetimeSafety.html#use-after-invalidation-experimental ### Alternative solution A) Return by copy everywhere. Might introduce more heap activity, especially in the case of `HTTPRemoteClient::GetRequest()`. ### Alternative solution B) Refactor the methods to minimize copying while still making things more memory-safe. Replacing `HTTPHeaders::FindAll()` with an `Iterate()`-function taking a lambda which gets to process each header. Gets rid of the heap activity of building a `vector` but introduces copying of `first`. <details><summary>Diff of httpserver.cpp/h</summary> ```diff --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -272,15 +272,11 @@ std::optional<std::string> HTTPHeaders::FindFirst(const std::string_view key) co return std::nullopt; } -std::vector<std::string_view> HTTPHeaders::FindAll(const std::string_view key) const +void HTTPHeaders::Iterate(std::function<void(const std::string& key, const std::string& value)> fn) const { - std::vector<std::string_view> ret; for (const auto& item : m_headers) { - if (CaseInsensitiveEqual(key, item.first)) { - ret.push_back(item.second); - } + fn(item.first, item.second); } - return ret; } void HTTPHeaders::Write(std::string&& key, std::string&& value) @@ -504,18 +500,21 @@ bool HTTPRequest::LoadBody(LineReader& reader) // We read all the chunks but never got the last chunk, wait for client to send more return false; } else { + std::optional<std::string> first; + m_headers.Iterate([&first] (const std::string& key, const std::string& value) { + if (!CaseInsensitiveEqual(key, "Content-Length")) return; + if (!first.has_value()) { + first = value; + } else if (first != value) { + // Duplicate Content-Length headers are allowed only if they all have the same value + // https://www.rfc-editor.org/rfc/rfc7230#section-3.3.3 + throw std::runtime_error("Differing Content-Length values"); + } + }); // No Content-length or Transfer-Encoding header means no body, see libevent evhttp_get_body() - auto content_length_values{m_headers.FindAll("Content-Length")}; - if (content_length_values.empty()) return true; - - // Duplicate Content-Length headers are allowed only if they all have the same value - // https://www.rfc-editor.org/rfc/rfc7230#section-3.3.3 - const auto& first_content_length_value{content_length_values[0]}; - for (size_t i = 1; i < content_length_values.size(); ++i) { - if (content_length_values[i] != first_content_length_value) throw std::runtime_error("Differing Content-Length values"); - } + if (!first.has_value()) return true; - const auto content_length{ToIntegral<uint64_t>(first_content_length_value)}; + const auto content_length{ToIntegral<uint64_t>(first.value())}; if (!content_length) throw std::runtime_error("Cannot parse Content-Length value"); if (*content_length > MAX_BODY_SIZE) throw ContentTooLargeError("Max body size exceeded"); --- a/src/httpserver.h +++ b/src/httpserver.h @@ -97,10 +97,9 @@ public: */ std::optional<std::string> FindFirst(std::string_view key) const; /** - * @PARAM[in] key The field-name of the header to search for - * @returns Views into all values matching the provided key (valid while this object is alive) + * @PARAM[in] fn Receives each header as they are iterated through. */ - std::vector<std::string_view> FindAll(std::string_view key) const LIFETIMEBOUND; + void Iterate(std::function<void(const std::string& key, const std::string& value)> fn) const; void Write(std::string&& key, std::string&& value); /** * @PARAM[in] key The field-name of the header to search for and delete ``` </details> ### Rationale The methods are not called in many places so risk of misuse is low, and we avoid any risk of performance degradation (such as the one found in bitcoin#35182 (comment)). Returning copies without adding mutexes or other thread safety measures does not considerably increase thread-safety. ACKs for top commit: maflcko: lgtm ACK 0b46fc9 l0rinc: code review ACK 0b46fc9 Tree-SHA512: d80670e3a832614f0b33b2c11e7ddd149b11f794e182c3dfb7c5c6045f23a7ea4a179da31c5fed94877a1df061968b65d13d47dcb7f858f853fa31e59497ee03
59f7f3d build: drop use of OBJC_OLD_DISPATCH_PROTOTYPES (fanquake) Pull request description: Now that we target macOS >= `14.0` and use macOS SDK `14.0`, and no-longer use `objc_msgSend`, this can be removed. Was added in bitcoin#16720. ACKs for top commit: willcl-ark: ACK 59f7f3d Tree-SHA512: 020c66f9a1092301f2ae001186f6d3dbf5a05157d5d340d5072db1579c0c614ca1d305a54c3943d7a90df84ca62c665ac53981e550053135f0883943d08b7dfc
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
83434f7 kernel: expose transaction version (nervana21) Pull request description: This PR is a follow-up to bitcoin#36194 and bitcoin#36207. It completes the existing set of transaction accessors allowing clients to read the transaction version without slicing serialized bytes. Tests cover a range of versions. ACKs for top commit: stickies-v: ACK 83434f7 yuvicc: lgtm! ACK 83434f7 sedited: ACK 83434f7 Tree-SHA512: 00b9aeae284aaa5de3992426f7406c593e5cebb3ff3923c04399809caac82465b96a7e6235671574bbfb6c203cca633f9588678f9222e7f698bea8985eabada0
5181c61 Squashed 'src/crc32c/' changes from efb8ea04e4..60bcf13edf (fanquake) Pull request description: Includes: * bitcoin-core/crc32c-subtree#10 Used in bitcoin#36275. ACKs for top commit: hebasto: ACK 2c8372d. Tree-SHA512: 617e74dda248b00e79ed773d32c0d521ff363e9a23c01ae500a8fd25a4330f39a4851d4e2c977f56a2a7a89ad3563c7e57530ed044dba77dc2b17a4ba138df57
fcb2d83 Squashed 'src/minisketch/' changes from 4a179c61e3..a12f5de1c9 (fanquake) Pull request description: Includes: * bitcoin-core/minisketch#105 Used in bitcoin#36275. ACKs for top commit: hebasto: ACK 749bd10. Tree-SHA512: 0637e2af27d6bc84b61296840b4cfd8503162ca8d37226282b10b03f4fa7637bfbfd873146d13d4f76bfd105f2a6cfb5861169730514b6c668a77984ca7b8b39
While using the `getaddrmaninfo` RPC for monitoring the sizes of my address managers, I noticed that it returns the count of unique addresses in the tables. While the addresses in the `tried` table are always unique, the `new` table might contain an address multiple times if we received it from sources in different network groups. I incorrectly assumed the returned values to be the number of address manager slots filled. To make this clearer for the next person, I'm clarifying this in the documentation. The number of filled addrman slots can be derived from the output of the hidden `getrawaddrman` RPC.
e9dc161 correct the linearize-data script (minor) (midnight) Pull request description: For tools using the linearization scripts to continuously create a data dump it is undesirable to truncate the dumped file every time it is written. Doing so makes it harder to be directly consumed by an external tool. To fix this, change the file open mode to not truncate the entire file. After this change dumps are byte-for-byte overwritten. Some trailing data at the end of the file might be left over from a previous run in case of a reorg. To guard against this, clip off any remaining data at the end of the file once the dump is completed. (small patch being passed around which I did not write but thought would be helpful to the community) ACKs for top commit: maflcko: lgtm ACK e9dc161 sedited: ACK e9dc161 Tree-SHA512: 9d67183b386a9dee90c4338b986d8b2121a512192cc615ab297e8023a962a9fb4ad721cc2d360eb8cb8a031b7bc1d8315fb6bdb94eb4c04e624c6feacf2af292
…ests and rename arg -ipcfd to -ipcchild 05e7d57 ipc, test: add checkSpawned() unit tests (ViniciusCestarii) 8bca755 ipc, refactor: rename arg -ipcfd to -ipcchild (ViniciusCestarii) bd3a38f ipc, refactor: return std::optional<mp::SocketId> from checkSpawned() (ViniciusCestarii) Pull request description: Implement proposed follow-ups on dicussion bitcoin#35084 (comment): - Add unit test for checkSpawned(). - Make checkSpawned() return std::optional<mp::SocketId>. - Rename -ipcfd to -ipcchild. ACKs for top commit: jeanpablojp: ACK 05e7d57 enirox001: utACK bitcoin@05e7d57 ryanofsky: Code review ACK 05e7d57. Thanks for the followup! Tree-SHA512: b91d94cbd7c5feb090fd8fac040d731cbc4dfb2b129f79fbb632deca92ae97a08e8b023b3a00f316702bc6e0d94668b1205baacabbc2b39a07b9e397fb4c76f2
21e4ea8 test: cover PSBT unknown field merging (w0xlt) Pull request description: Continuing the work from bitcoin#35310 gere in agreement with w0xlt This PR adds functional coverage for combinepsbt preserving unknown PSBT fields across global, input, and output maps, as suggested [here](bitcoin#34893 (comment)). The test covers both PSBTv0 and PSBTv2 by creating valid base PSBTs with `createpsbt`, injecting unknown key-value pairs into two copies, combining them, and asserting that all unknown fields are retained in the decoded result. ACKs for top commit: polespinasa: ACK 21e4ea8 nebula-21: ACK 21e4ea8 winterrdog: tACK 21e4ea8 w0xlt: ACK 21e4ea8 Tree-SHA512: 0f83ff2b9a519b613c6efaca3707bd8c6fdd00887370e16e927b19f059b751813737adc85ad81366f5c57b91226f56b3b0bdc996451f4d6bc9ec83aa4a189923
… addresses 0d34b6e rpc: clarify that getaddrmaninfo counts unique addresses (0xb10c) Pull request description: While using the `getaddrmaninfo` RPC for monitoring the sizes of my address managers, we noticed that it returns the count of unique addresses in the tables. While the addresses in the `tried` table are always unique, the `new` table might contain an address multiple times if we received it from sources in different network groups. I incorrectly assumed the returned values to be the number of address manager slots filled. To make this clearer for the next person, I'm clarifying this in the documentation. The number of filled addrman slots can be derived from the output of the hidden `getrawaddrman` RPC. ACKs for top commit: fjahr: ACK 0d34b6e stratospher: ACK 0d34b6e. danielabrozzoni: ACK 0d34b6e Tree-SHA512: 44c607ada730d7264cacd1ba8930cc60d84e59bc92e4e08b98f2453c951f2e0251707cd968bf39d544426a674b78aa5f99b8df08acc06201fceae423ffd814e2
716e860 ci: simplify macOS codesign check (fanquake) Pull request description: Followup to bitcoin#34914. See bitcoin#34914 (comment). ACKs for top commit: maflcko: lgtm ACK 716e860 Sjors: ACK 716e860 willcl-ark: code review ACK 716e860 Tree-SHA512: 750c08eef5f5284b2d259ddb73a62a774cfa9154d91a91561a3042897639be9ea649d74ca5212eb07d3e1e24ea4b468d734824a0d91c9cc2544f4d7fedb67706
a34861d test: unify how to avoid the creation of the default wallet in nodes (rkrux) 3458837 test: use ExtendedPrivateKey in wallet_importdescriptors.py (rkrux) Pull request description: Looks like a case was missed in PR bitcoin#35543 that introduced the utility xprv and xpub classes. Also, unify how to avoid the creation of the default wallet in nodes. ACKs for top commit: davidgumberg: reACK bitcoin@a34861d nebula-21: ACK a34861d sedited: ACK a34861d Tree-SHA512: 4ed5d2375d915bfff1898babdd7311ead15169bc1156ab5168b5c7ca2e969c850f752d36caa585b8fe856dca26f3ceb3f7a98ce091d4a8a7c38f532b87250d3f
6d24145 wallet: reuse unlock and passphrase change errors (Ava Chow) 5935546 wallet: return passphrase errors with Expected (Ava Chow) f949b3b wallet: publish descriptor keys after writes (Lőrinc) e3712b7 test: characterize descriptor insertion failure (Lőrinc) bd55810 wallet: abort failed descriptor key erases (Lőrinc) 537e791 test: cover encrypted descriptor key insertion (Lőrinc) d8a6f5e wallet: abort failed descriptor key writes (Lőrinc) 14ce3fd test: characterize descriptor key failures (Lőrinc) 5bc0be1 wallet: reject failed passphrase changes (Lőrinc) ee6d45a test: characterize passphrase write failure (Lőrinc) 2daa2f9 wallet: restore lock state before re-encryption (Ava Chow) 8b23fa4 wallet: abort failed encryption transactions (Lőrinc) e6c374d test: characterize encryption transaction failures (Lőrinc) 7038101 refactor/test: add wallet failure injection (Lőrinc) Pull request description: **Problem:** Wallet encryption and passphrase changes can leave database records and live key state out of sync when a database operation fails. Encryption can report success without persisting the master key, and a passphrase change can activate the new passphrase only in memory. Descriptor key write failures can publish keys that were not persisted, while erase failures can leave plaintext keys on disk. A failed encryption transaction commit aborts the node, and a re-encryption failure during a passphrase change can leave a previously locked wallet unlocked. Boolean results also force callers to duplicate error messages or report database and encryption failures as incorrect passphrases. **Fix:** Publish wallet encryption state only after the database transaction commits, and publish newly inserted descriptor keys only after their database writes succeed. For passphrase changes, restore the original lock state after validating the old passphrase and replace the live master key only after persisting its re-encrypted value. These changes preserve the affected keys on failure and allow retries. Return structured errors from wallet unlock and passphrase changes so callers can share messages and report the specific failure. Fresh descriptor setup after the encryption transaction remains unchanged. ACKs for top commit: achow101: ACK 6d24145 w0xlt: reACK 6d24145 Tree-SHA512: 2d7293d9482ef259732cd0c3dd31eb2c376fc39e23a76141032b6e308b0e71c5d86b3f03995ef3f923cabbc431a139a39a4be02853f352f7103a7f9f52a95299
c8ee319 net: remove m_getaddr_sent (naiyoma) a1ecda2 test: delete redundant addr relay assertion (naiyoma) Pull request description: This PR removes `m_getaddr_sent`, as it no longer behaves as originally intended. Initially, this flag was meant to track when a getaddr message was sent to a peer. Now that the initial self-announcements are sent separately from getaddr responses, the self-announcement sets `m_getaddr_sent `to `false` even though we are still waiting for the getaddr response (1000 addresses). When the getaddr response does arrive, we rely on the size of the addr message, not the flag, to decide whether it should be relayed. This makes the flag redundant. This is the current behavior: - The initial self-announcement is not relayed but it flips m_getaddr_sent to be false - We use addr.size() to avoid relaying (1000) getaddr response.(assuming the other two flags are false) - The first addr message is relayed because the flag is false(this was not the case before bitcoin#34146). Removing this flag ensures that: - The initial self-announcement is relayed. - The getaddr response is still not relayed, using the existing size-based check. I had initially considered an alternative approach naiyoma#14, where the self-announcement would not affect this flag and would therefore retain the initial behaviour, but i decided to reattempt its removal, as this was previously attempted, see bitcoin#19794. Given the changes since then, I believe revisiting it is now more appropriate. ACKs for top commit: w0xlt: ACK c8ee319 danielabrozzoni: tACK c8ee319 mzumsande: Code Review ACK c8ee319 sedited: utACK c8ee319 Tree-SHA512: d4a7e0525efd286597136af7f84b5d3301537a06dc093059102eecd15a2d80d22127e756eaefcb8b069c07987f36ccb997c5eadcc6ffa71b7ec0322777590db5
…d getblockstats cli examples 57a79c6 doc: remove json quoting from gettxoutsetinfo and getblockstats cli examples (csjones) Pull request description: This is a followup change based on bitcoin#33230 (comment) because bitcoin#33230 enabled handling cli arguments with either json or a string but the examples (i.e. `bitcoin-cli help gettxoutsetinfo` and `bitcoin-cli help getblockstats`) left the quotes. I considered adding a new example but felt that change might seem confusing. ACKs for top commit: nervana21: ACK 57a79c6 sedited: ACK 57a79c6 Tree-SHA512: fb1cf210837acb4a62b1adf024ce7b2716178e9f7293d21fee3a058f838603bd38cb46a8405c7b7d33cf2b7b66a4f7012bb0fc1f1cafc94055969f4728dfa2f9
…etpeerinfo result 13ce46b doc: add release note for 27052 (Larry Ruane) 5d5397d test: add functional test for block announcement time tracking (Larry Ruane) 7d3d04e rpc: add last_block_announcement to the getpeerinfo output (Larry Ruane) 26a2516 net: add m_last_block_announcement to CNodeStateStats (Larry Ruane) bb8bca6 change m_last_block_announcement type from int64_t to NodeClock::time_point (Larry Ruane) Pull request description: This PR adds `last_block_announcement` to the per-peer `getpeerinfo` RPC result. This is the most recent time that this peer was the first to notify our node of a new block (one that we didn't already know about), or zero if this peer has never been the first to notify us of a new block. This timestamp already exists internally and is used for stale-tip eviction logic; this PR exposes it at the RPC layer. This PR started out as a suggestion for additional test coverage, see bitcoin#26172 (comment). It turned out that the easiest way to test (already-merged) bitcoin#26172 is to add this field to `getpeerinfo` and have a functional test verify its value. But it may also be useful to have this result in its own right, similar to that RPC's existing `last_block` field -- it indicates something about the quality of our peers. It allows one to predict which peer will be evicted when the stale tip logic activates. (I'm not sure if that would be useful, but it may be.) The functional test added here fails without bitcoin#26172, which is the main goal. This PR does not test the actual stale-tip eviction logic; that's difficult to do with a functional test. But it does test the correctness of the timestamp that the eviction logic depends on. bitcoin#23352 is an attempt to test the eviction logic using a unit test, it's not ready to be merged yet. I think having both kinds of tests would be beneficial. ACKs for top commit: rkrux: lgtm ACK 13ce46b naiyoma: ACK 13ce46b ryanofsky: Code review ACK 13ce46b Tree-SHA512: 64cd7dd688516f694c68d4594583af184d0556230660690b173f0fb0f09f20b526469e77a0f5825103ab6652c34fc6b8d3bce17aeb8e738ea8f5613d2939157c
7c311f5 doc: Add release notes for listrawtransactions (pablomartin4btc) b377786 test: Add functional test for listrawtransactions (pablomartin4btc) 605c4fd wallet, rpc: Add listrawtransactions RPC (pablomartin4btc) ba6c179 wallet, rpc: allow omitting parent_descs from TransactionDescriptionString (pablomartin4btc) dc3e2bc wallet, rpc: extract PushTxAmountAndFee/PushTxDecoded helpers (pablomartin4btc) Pull request description: Add a new `listrawtransactions` RPC that returns wallet transactions without logical interpretation. Unlike `listtransactions`, which only lists transactions with an economic category (sends to external addresses, receives from outside), this RPC returns every transaction the wallet knows about — including consolidations and self-transfers that would otherwise be invisible. Each transaction appears exactly once with its net wallet balance change (`amount`) and, when the wallet funded the transaction, the fee paid (`fee`). Pagination is supported via `count` and `skip` parameters, matching the interface of `listtransactions`. Next possible follow-ups: - A filter parameter (`"all"` / `"received"` / `"sent"` / `"self"`) once per-tx semantics are agreed on - Some fields inherited from `TransactionDescriptionString()` carry references to `category` (e.g. `replaced_by_txid`, `parent_descs`) which don't apply here — could be cleaned up in a separate pass Closes bitcoin#34632. Previous attempt: bitcoin#35009 (by alfonsoromanz), closed in favour of a separate RPC as suggested by achow101. ACKs for top commit: Bicaru20: reACK 7c311f5 achow101: ACK 7c311f5 polespinasa: ACK 7c311f5 Tree-SHA512: c80f98844124d10b16a7f275107e22d4f5983329d93208e95b3e56ac7295c7265eebdb247a4fa7d70da8d38f4b453df17efaee29a4bd6bae8cb1da60b029c489
Both m_requested_hb_cmpctblocks & m_provides_cmpctblocks were protected by cs_main which is unnecessary. Instead convert them to std::atomic<bool> as Mutex is not needed. This way we don't need to worry about potential lock inversion in the future or thread safety annotations. Also, add a negative annotation to NewPoWValidBlock for m_peer_mutex since it now acquires it.
Also make the m_num_preferred_download_peers counter atomic and add an assert in FinalizeNode. The asserts for m_num_preferred_download_peers in FinalizeNode are still safe even without cs_main locking because of the implicit guarantee that ProcessMessage may not run for a peer at the same time as FinalizeNode. This has the nice benefit of being able to remove cs_main usage in some places.
-BEGIN VERIFY SCRIPT- sed -i "s/fPreferredDownload/m_preferred_download/g" $( git grep -l fPreferredDownload ) -END VERIFY SCRIPT-
Crypt-iQ
force-pushed
the
06082026/cnodestate
branch
from
September 23, 2026 13:31
3861ab4 to
9686707
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.
No description provided.