🤔 andrewtoth reviewed a pull request: "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests"
(https://github.com/bitcoin/bitcoin/pull/30906#pullrequestreview-2317044728)
I think the commit title `test: Migrate GetCoinsMapEntry to return std::optional<CoinState>` is no longer accurate.

I didn't find splitting the changes in `coins_tests` up into that many commits to be helpful for review. I ended up reviewing the combined diff for that file. Will review in more depth later.

💬 andrewtoth commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1767813420)
I think this should be `Assume`. We should prefer it since it will be compiled out for release builds and this is a hot path.

💬 andrewtoth commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1767815543)
Hmm not sure we should remove the extra context here about what exactly `self` and `sentinel` are and where they come from.

🤔 tdb3 reviewed a pull request: "netinfo: add peer services column and outbound-only peers list"
(https://github.com/bitcoin/bitcoin/pull/30930#pullrequestreview-2317070761)
Approach ACK

Nice improvement.
Performed a quick initial code review and exercised the changes interactively running on signet. I plan to circle back.

✅ maflcko closed a pull request: "blockstorage: Avoid potential Memory Leak"
(https://github.com/bitcoin/bitcoin/pull/30932)

💬 maflcko commented on pull request "blockstorage: Avoid potential Memory Leak":
(https://github.com/bitcoin/bitcoin/pull/30932#issuecomment-2362882553)
Closing as a low-quality (and obviously wrong) LLM generated bot/spam patch, with an (obviously wrong) hallucinated explanation, without any test coverage or otherwise steps to test, and finally test failures in the existing tests.

💬 maflcko commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768048796)
Not sure about adding those. The whole point of the previous pull requests was to remove this linter (https://github.com/bitcoin/bitcoin/issues/30530). Now having follow-ups (https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1767054547) that make it harder, albeit minimally, seems a step in the wrong direction.

💬 maflcko commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768048525)
not sure about removing this. Apart from Wunused, it is also required to be detected in coverage reports (which will obviously omit consteval stuff)

💬 maflcko commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768047784)
style nit (feel free to ignore): I suspect you could reduce verbosity and having to hand-format all of this by using `tuple_cat`, see https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756495304

This should also ensure that all cases are covered and wouldn't require to change the tests vectors.

👍 vasild approved a pull request: "Fix display issues for IPv6 proxy setup in Options Dialog (UI only, no functionality impact)"
(https://github.com/bitcoin-core/gui/pull/836#pullrequestreview-2317404454)
ACK fee4cba48472239f7a426db62f45c4b6af8e5ef2

👍 zaidmstrr approved a pull request: "fix: handle invalid `-rpcbind` port earlier"
(https://github.com/bitcoin/bitcoin/pull/30679#pullrequestreview-2317472339)
Code review ACK [e6994ef](https://github.com/bitcoin/bitcoin/commit/e6994efe08b282dd9e46602bcbad69567fe91dcd)
All the things seem correct as before.

📝 maflcko opened a pull request: "ci: Approximate MAKEJOBS in image build phase"
(https://github.com/bitcoin/bitcoin/pull/30935)
The `MAKEJOBS` env var is the default in image builds, which is fine, because it is only relevant when building msan (or iwyu) and only differs when setting MAKEJOBS to something other than `nproc` (currently used as an approximation).

So the normal workflow of `MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_native_msan.sh" ./ci/test_run_all.sh` already works today.

However, `MAKEJOBS="-j1" FILE_ENV="./ci/test/00_setup_env_native_msan.sh" ./ci/test_run_all.sh` does not.

This is
...

💬 maflcko commented on pull request "doc: Change `nproc` in docs to `getconf _NPROCESSORS_ONLN`":
(https://github.com/bitcoin/bitcoin/pull/30619#issuecomment-2363234138)
Just for reference, the change would be wrong anyway, because the two are used to return different values

```
$ podman run --cpuset-cpus=0-1 --rm ubuntu:noble nproc
2
$ podman run --cpuset-cpus=0-1 --rm ubuntu:noble getconf _NPROCESSORS_ONLN
16

💬 Sjors commented on pull request "Stratum v2 Transport":
(https://github.com/bitcoin/bitcoin/pull/30315#issuecomment-2363248114)
I added a commit (again) that moves `CNetMessage`, `CSerializedNetMsg` and `Transport` to `common/transport.h`. To keep the diff small it doesn't move the implementation; that could be done in a separate PR. This avoids a circular dependency between `bitcoin-node` and `bitcoin-sv2`, which doesn't cause problems here, but leads to headaches in #29432 (e.g. tests spontaneously crashing).

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2363286272)
Rebased to incorporate latest relevant changes to the build system and `AutoFile`. This now also builds on #30901 so please ignore the first commit or leave your review of it in that PR.

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1768296965)
I am now using that function. I thought there might be issue with the missing header guards but the generated files are not listed. There is now also no more comment in the generated file.

💬 hodlinator commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768296972)
Not sure it would reduce verbosity that much, but would definitely consider switching if someone took the time to provide a diff.

Compiler warnings are triggered for missing cases.

💬 hodlinator commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768304651)
Good point about coverage! Error paths at runtime should be covered by `FailFmtWithError`, but happy path might only be run at compile time. Will bring it back.

💬 hodlinator commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768298071)
I'll be happy to remove them once the linter is no longer run on CI.

💬 fjahr commented on pull request "cmake: Revamp handling of data files for `{test,bench}_bitcoin` targets":
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2363304611)
tACK 0c35be69cf2a18a7a9173d0f9f88116b4417c892

I have based #28792 on this now. It works and seems like a nicer way to do what I need there than the previous approach.

Take it with a grain of salt though because I don't think I am experienced enough with CMake to tell if there are any hidden issues with this approach or if there are even better ways to achieve this.