🚀 glozow merged a pull request: "test: Add explicit onion bind to p2p_permissions"
(https://github.com/bitcoin/bitcoin/pull/30805)

📝 romanz opened a pull request: "http: Use 'starts_with' for matching URI prefix"
(https://github.com/bitcoin/bitcoin/pull/30868)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 maflcko commented on pull request "http: Use 'starts_with' for matching URI prefix":
(https://github.com/bitcoin/bitcoin/pull/30868#issuecomment-2342686200)
If there is support for this change, it may be better to use and enable https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-starts-ends-with.html . Otherwise, follow-up pull requests may be created for each instance individually. Also, newly introduced code may re-introduce new instances.

💬 Sjors commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2342749010)
I successfully tested this code as part https://github.com/Sjors/bitcoin/pull/48#issuecomment-2342745012

💬 maflcko commented on pull request "build: Fix `ENABLE_WALLET` option":
(https://github.com/bitcoin/bitcoin/pull/30867#issuecomment-2342790419)
review ACK 0037d53d1a21ec8a5a97a83ab716d68030446021

Just noting that the test config will be a bit confusing, albeit harmless:

```
$ grep -C1 USE_SQLITE ./bld-cmake/test/config.ini
#ENABLE_WALLET=true
USE_SQLITE=true
#USE_BDB=true

💬 maflcko commented on pull request "ci: Post CMake-migration fixes and amendments":
(https://github.com/bitcoin/bitcoin/pull/30841#issuecomment-2342828286)
review ACK c45186ca548362b75a6640393ccf79b11ff727da

💬 maflcko commented on pull request "build: Minimize I/O operations in `GenerateHeaderFrom{Json,Raw}.cmake`":
(https://github.com/bitcoin/bitcoin/pull/30842#issuecomment-2342833531)
review ACK b07fe666f27e2b2515d2cb5a0339512045e64761

Seems fine to always buffer everything in memory twice, because it is already buffered in memory once (as the full `hex_content`) and the data should be small (generally less than 10 MB?)

👍 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.