💬 maflcko commented on pull request "ci: add `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES` to TSAN (libc++) job":
(https://github.com/bitcoin/bitcoin/pull/30519#discussion_r1689636199)
nit: If it compiles, while touching, could fix all headers?

```
init.h should add these lines:
#include <atomic> // for atomic

init.h should remove these lines:
- #include <any> // lines 9-9
- #include <memory> // lines 10-10
- #include <string> // lines 11-11

👍 maflcko approved a pull request: "ci: add `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES` to TSAN (libc++) job"
(https://github.com/bitcoin/bitcoin/pull/30519#pullrequestreview-2196512417)
lgtm

This is useful to catch build failures before they hit users that compile from source.

💬 glozow commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1689639934)
Matches my diagrams. Thanks for the additional context! :+1:

👍 tdb3 approved a pull request: "refactor: Add FlatFileSeq member variables in BlockManager"
(https://github.com/bitcoin/bitcoin/pull/30517#pullrequestreview-2196603604)
ACK 7aa8994c6fceae5cf8fb7e661371cdb19d2cb482

👍 TheCharlatan approved a pull request: "Early logging improvements"
(https://github.com/bitcoin/bitcoin/pull/30386#pullrequestreview-2196253338)
Nice, ACK b4dd7ab43e8cfc2c171f67588e4e1ec2705393c2

💬 TheCharlatan commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1689628041)
Nit: Why is this being set again? I'm guessing it is not declared as `const` to future-proof it against introducing an option controlling its size, but it is not clear why disconnecting should clear such a value again.

💬 TheCharlatan commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1689526333)
Nit: Unneeded newline.

💬 hebasto commented on pull request "script/sign: avoid duplicated signature verification after signing (+introduce signing benchmarks)":
(https://github.com/bitcoin/bitcoin/pull/28923#issuecomment-2247822717)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/280.

💬 hebasto commented on pull request "Add fuzz test for FSChaCha20Poly1305, AEADChacha20Poly1305":
(https://github.com/bitcoin/bitcoin/pull/28263#issuecomment-2247823778)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/280.

💬 hebasto commented on pull request "depends: Bump `libmultiprocess` for CMake fixes":
(https://github.com/bitcoin/bitcoin/pull/30513#issuecomment-2247862187)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/277.

💬 hebasto commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2247865660)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/278.

🤔 stratospher reviewed a pull request: "net: Log accepted connection after m_nodes.push_back; Fix intermittent test issue"
(https://github.com/bitcoin/bitcoin/pull/30512#pullrequestreview-2196772985)
tested ACK fa3ea3b.

💬 fanquake commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#discussion_r1689861943)
Added.

💬 fanquake commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2248024050)
> i'd be okay with skipping the check for bitcoin-util: it's the least relevant binary for fortification (no network access, not even file format access). Could reconsider it later if it actually gains some useful functionality 😄

I think I agree, and I've added that skipping now.

👋 fanquake's pull request is ready for review: "security-check: test for `_FORTIFY_SOURCE` usage in release binaries"
(https://github.com/bitcoin/bitcoin/pull/27038)

💬 sipa commented on pull request "cleanse: switch to SecureZeroMemory for Windows cross-compile":
(https://github.com/bitcoin/bitcoin/pull/26950#issuecomment-2248056425)
utACK c399c80a09a393d38368a44ef04753e9f62350f0

👍 ryanofsky approved a pull request: "assumeutxo: Add dumptxoutset height param, remove shell scripts"
(https://github.com/bitcoin/bitcoin/pull/29553#pullrequestreview-2196517706)
Code review ACK 27767184aaa7fa94600c70334cc4122e25872ff6.

Main change since last review is documenting dumptxoutset better and making default behavior be to dump the latest snapshot that can be loaded, instead of the current utxo set. Also improved other documentation and added NetworkDisable class to more reliably restore network state in case of error.

Left some additional suggestions but they aren't very important

💬 ryanofsky commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1689636259)
re: https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1674353019

Oh, I think I just forgot about prune locks added by indexes when I wrote this comment. If indexes are disabled, then the normal `GetPruneRange` function that controls pruning will allow pruning blocks between the snapshot and the tip.

But if indexes are enabled, because indexes prevent any blocks that haven't been indexed yet from being pruned, and because indexes currently just ignore the snapshot and don't take adv
...

💬 ryanofsky commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1689871705)
In commit "RPC: Add height parameter to dumptxoutset" (5df60fb49cfae1e3a56b68e2f43ff0722f39abb7)

It's not safe to dereference `max_height` if snapshot_heights is empty. Would be good to either assert snapshot_heights is nonempty, or just leave target_index null in this case and not roll back if it is null.

💬 ryanofsky commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1689671443)
In commit "RPC: Add height parameter to dumptxoutset" (5df60fb49cfae1e3a56b68e2f43ff0722f39abb7)

Seems like this accepts a height or a hash, so could mention this accepts block hashes and maybe call this height_or_hash or whatever the convention is

💬 ryanofsky commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1689840218)
re: https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1649335464

I don't think this is critical to fix but if you wanted to remove the race condition, the following change should work:

<details><summary>diff</summary>
<p>

```diff
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -74,6 +74,22 @@ static GlobalMutex cs_blockchange;
static std::condition_variable cond_blockchange;
static CUpdatedBlock latestblock GUARDED_BY(cs_blockchange);

+std::tuple<std::un
...