👍 vasild approved a pull request: "net: call `Select` with reachable networks in `ThreadOpenConnections`"
(https://github.com/bitcoin/bitcoin/pull/29436#pullrequestreview-2295868986)
ACK e4e3b44e9cc7227b3ad765397c884999f57bac2e

💬 maflcko commented on pull request "build: Minor build system fixes and amendments":
(https://github.com/bitcoin/bitcoin/pull/30803#issuecomment-2342845494)
re-ACK 1cc93fe7b40f10a7d1d1189058af98a2bce31381

Only change since last review is MSVC fixups as well as dropping dead (and broken) code.

💬 naiyoma commented on pull request "test: Add test for rpcwhitelistdefault":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1753486683)
@stratospher thanks for the suggestion , update [https://github.com/bitcoin/bitcoin/pull/29858/commits/fcf0ead6cd358fd35909e1102bdae2c176b0ace6](https://github.com/bitcoin/bitcoin/pull/29858/commits/fcf0ead6cd358fd35909e1102bdae2c176b0ace6)

👍 naumenkogs approved a pull request: "net: call `Select` with reachable networks in `ThreadOpenConnections`"
(https://github.com/bitcoin/bitcoin/pull/29436#pullrequestreview-2296005895)
ACK e4e3b44e9cc7227b3ad765397c884999f57bac2e

💬 naumenkogs commented on pull request "net, net_processing: additional and consistent disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#issuecomment-2342970431)
ACK 08cd29f5020995294a4f6274e72afab58bf69aa2

The first two commits make logging cleaner and more uniform.
I'm not sure the last commit is useful, but it doesn't hurt either.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1753752267)
> is there a reason we'd want to actively discourage this function from being used?

Mostly just to avoid someone from using the function over `sizeof...(Args)`, which would render everything pointless, because calculating the same value at compile time is obviously going to always pass (modulo parsing errors)

👍 naumenkogs approved a pull request: "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD"
(https://github.com/bitcoin/bitcoin/pull/30807#pullrequestreview-2296198023)
ACK 992f83bb6f4b29b44f4eaace1d1a2c0001d43cac

💬 laanwj commented on issue "build: reproducibility issue with macOS Guix builds":
(https://github.com/bitcoin/bitcoin/issues/30815#issuecomment-2343184608)
> We are already using a tool-level mitigation (for tar). Therefore, using another tool-level mitigation (for CMake) seems consistent.

Sure. No big deal to add another one.

💬 marcofleon commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2343196210)
> Another follow up improvement I'd like is direct assertion that the "chainwork" of the header chain never exceed minimum chainwork, or if that is exceeded, we can drop the assertion that the chain never extends past genesis block.

Agreed, I think something like this would need to be added in order to build a chain on top of the initial set of headers.

💬 fanquake commented on pull request "build: Skip secp256k1 ctime tests when tests are not being built":
(https://github.com/bitcoin/bitcoin/pull/30865#issuecomment-2343234635)
I think I'm missing something here. Isn't it a bug that us already setting `SECP256K1_BUILD_CTIME_TESTS=OFF` is sometimes overridden later on? Otherwise, this change is unclear, because the ctime tests (along with the other secp256k1 tests) are already supposedly controlled by `BUILD_TESTS`, but now the ctime tests are being wrapped in another `BUILD_TESTS` conditional? (adding a comment would be good, otherwise I assume anyone reading this code will assume the second conditional is redundant).

🤔 BrandonOdiwuor reviewed a pull request: "refactor: migrate `bool GetCoin` to return `optional<Coin>`"
(https://github.com/bitcoin/bitcoin/pull/30849#pullrequestreview-2296307198)
Concept ACK

Using an optional return value is a cleaner approach compared to a boolean return with an output parameter.

💬 itornaza commented on pull request "docs: Updated debug build instructions for cmake":
(https://github.com/bitcoin/bitcoin/pull/30840#issuecomment-2343257317)
> Also in this parity list `--enable-debug` is replaced with `-DCMAKE_BUILD_TYPE=Debug` and defaults to `RelWithDebInfo` https://gist.github.com/hebasto/2ef97d3a726bfce08ded9df07f7dab5e

Very helpful link, thanks for sharing!

📝 maflcko opened a pull request: "ci: Print inner env"
(https://github.com/bitcoin/bitcoin/pull/30869)
The outer env is printed when the `/tmp/env-$USER-$CONTAINER_NAME` is created. There is also a separate env printed when building the container image (usually with default values).

To confirm that the inner container env is correctly derived from the outer env, and not from the default build env, print it a third time.

💬 petertodd commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#issuecomment-2343346386)
This could probably be done in a separate pull-req. But we can also remove the `bip125-replaceable` code from the wallet at some point. There's quite a bit of complexity there tracking it; I was just looking at `run_rbf_opt_in_test` in `test/functional/wallet_listtransactions.py` myself for Libre Relay.

🤔 pablomartin4btc reviewed a pull request: "rpc: add `revelant_blocks` to `scanblocks status`"
(https://github.com/bitcoin/bitcoin/pull/30713#pullrequestreview-2296520203)
tACK 39ee30c31f5b33a084f420ed49170e669a8f6440

Release notes has been added since my last review.

<details>
<summary>Tested on mainnet.</summary>

```
/build/src/bitcoin-cli scanblocks start '["addr(bc1p0cp0vyag6snlta2l7c4am3rue7eef9f72l7uhx52m4v27vfydx9s8tfs7t)"]' 850263
{
"from_height": 850263,
"to_height": 860861,
"relevant_blocks": [
"00000000000000000002b3c2a395905df9d12ce30d2237f064cdafacb81eb33d",
"00000000000000000002bc5cc78f5b0913a5230a8f4b0d5060bc9a6090
...

💬 hebasto commented on pull request "build: Use CMake's default permissions in macOS `deploy` target":
(https://github.com/bitcoin/bitcoin/pull/30838#issuecomment-2343387310)
> Concept ACK - looks like switching this to use CMakes default permissions, rather than ours, does fix the issue, but I guess could use more discussion given: [#30815 (comment)](https://github.com/bitcoin/bitcoin/issues/30815#issuecomment-2336699084).

Continuation of the discussion:
- https://github.com/bitcoin/bitcoin/issues/30815#issuecomment-2341060674
- https://github.com/bitcoin/bitcoin/issues/30815#issuecomment-2343184608

💬 fanquake commented on issue "build: reproducibility issue with macOS Guix builds":
(https://github.com/bitcoin/bitcoin/issues/30815#issuecomment-2343430934)
> We are already using a tool-level mitigation (for tar). Therefore, using another tool-level mitigation (for CMake) seems consistent.

Why is it necessary though? Aren't we already setting `umask`:
https://github.com/bitcoin/bitcoin/blob/0725a374941355349bb4bc8a79dad1affb27d3b9/contrib/guix/libexec/build.sh#L9-L17

💬 hebasto commented on issue "build: reproducibility issue with macOS Guix builds":
(https://github.com/bitcoin/bitcoin/issues/30815#issuecomment-2343439211)
> Why is it necessary though? Aren't we already setting `umask`:

Does it work for directories shared via `--share=...` options?

🤔 stickies-v reviewed a pull request: "util: Use consteval checked format string in FatalErrorf"
(https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2296299754)
Approach ACK and code LGTM faca9a821963d629987f6c2a2f7d13b1bf01162c , left some suggestions that would be nice.

PR description also needs a bit of updating for the 2 new commits re forbidden functions and covering `LogConnectFailure`. The 2 new commits don't seem necessary for this PR but they were trivial to review and relevant to the goal so I'm okay keeping them.

💬 stickies-v commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1753929285)
I think this test should have a comment that the current behaviour is because our counting implementation is incorrect, not because it's desired behaviour. Otherwise this could be confusing to developers improving it in the future.

Or even better, add a separate group of tests with behaviour we know would be an improvement, but is not currently implemented:

```cpp
// The current implementation ignores `*` dynamic width/precision
// arguments to minimize code complexity, b
...