💬 m3dwards commented on pull request "depends: build libnatpmp with CMake":
(https://github.com/bitcoin/bitcoin/pull/29708#issuecomment-2076991934)
I don't believe any of these are a problem but some differences on flags passed to clang on my Mac were as follows:

When using this PR the following extra flags were seen vs master:

-DENABLE_STRNATPMPERR
-MD
-MT CMakeFiles/natpmp.dir/natpmp.c.o
-MF CMakeFiles/natpmp.dir/natpmp.c.o.d
-I/Users/max/source/bitcoin/depends/work/build/aarch64-apple-darwin23.0.0/libnatpmp/f2433bec24ca3d3f22a8a7840728a3ac177f94ba-2550f8d6ff0

On master these flags were seen which were not present when usi
...

💬 stickies-v commented on pull request "rpc: return warnings as an array instead of just a single one":
(https://github.com/bitcoin/bitcoin/pull/29845#issuecomment-2077026714)
> both curl and bitcoin-cli returned an error:

I suspect you're running with `-rpcdoccheck`, you shouldn't get any errors without it? I'm not sure if `-rpcdoccheck` is meant to be an internal-only testing flag (as per #24695), but it [doesn't seem to be documented as such](https://github.com/bitcoin/bitcoin/blob/2a07c4662d7266158d47f79fa2433ab22e22c907/src/init.cpp#L655), so I guess better to be safe - force pushed to make the docs contingent on the `-rpcdeprecated` too.

Thanks for your di
...

💬 TheCharlatan commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#issuecomment-2077029702)
Updated 9b2c9c2fce32fe858d1361e863c72108a384a5c8 -> b0594f6cf157e9bd71b2062cdf0aa65936fd2282 ([noGlobalReindex_1](https://github.com/TheCharlatan/bitcoin/tree/noGlobalReindex_1) -> [noGlobalReindex_2](https://github.com/TheCharlatan/bitcoin/tree/noGlobalReindex_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalReindex_1..noGlobalReindex_2))

* Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1578327024) and [comment](https://github.
...

👍 brunoerg approved a pull request: "Fix typos in description.md and wallet_util.py"
(https://github.com/bitcoin/bitcoin/pull/29938#pullrequestreview-2022321060)
utACK 03e36b3da093e2c23cf51b46f6901cb84ddbf867

💬 maflcko commented on issue "Require thread_local":
(https://github.com/bitcoin/bitcoin/issues/29952#issuecomment-2077068853)
> I couldn't get wine running locally

I hopped into the win64 CI container and ran https://gist.github.com/jamesob/fe9a872051a88b2025b1aa37bfa98605 there. It passed.

But given that the previous test was done on Trusty (14.04?), it will probably be hard to check when it started passing.

💬 brunoerg commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1579393843)
> I agree that could be good! Though I think it would add more complexity to this pull, so I would prefer not to do it here - could be a good follow-up though.

Sgtm!

👍 TheCharlatan approved a pull request: "Disable util::Result copying and assignment"
(https://github.com/bitcoin/bitcoin/pull/29906#pullrequestreview-2022385975)
ACK 66022315641934bda301d61f009dae9cb3b45da0

👍 alfonsoromanz approved a pull request: "doc: Bash is needed in gen_id and is not installed on FreeBSD by default"
(https://github.com/bitcoin/bitcoin/pull/29953#pullrequestreview-2022398345)
ACK 9381052194a78024b3994cc6ad906858c477b88f

💬 m3dwards commented on pull request "depends: build libnatpmp with CMake":
(https://github.com/bitcoin/bitcoin/pull/29708#issuecomment-2077104799)
ACK https://github.com/bitcoin/bitcoin/pull/29708/commits/3c1ae3ee33d4d9dbea046d5ab8ee924a12982759

👍 TheCharlatan approved a pull request: "depends: build libnatpmp with CMake"
(https://github.com/bitcoin/bitcoin/pull/29708#pullrequestreview-2022404068)
ACK 3c1ae3ee33d4d9dbea046d5ab8ee924a12982759

Also checked the natpmp changes since the last update.

💬 vasild commented on pull request "sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es":
(https://github.com/bitcoin/bitcoin/pull/25390#issuecomment-2077119552)
> Are there any examples from the last couple of years where this would have helped to prevent a bug or other issue?

Here is an example where two unrelated variables were guarded by the same mutex (already fixed in the latest code):

https://github.com/bitcoin/bitcoin/blob/c42ded3d9bda8b273780a4a81490bbf1b9e9c261/src/net.cpp#L116-L118

✅ vasild closed a pull request: "sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es"
(https://github.com/bitcoin/bitcoin/pull/25390)

💬 vasild commented on pull request "sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es":
(https://github.com/bitcoin/bitcoin/pull/25390#issuecomment-2077125161)
Closing this due to no interest from reviewers for a long time plus currently my hands are full with more important things. Would be happy to revisit if there is interest.

💬 maflcko commented on pull request "sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es":
(https://github.com/bitcoin/bitcoin/pull/25390#issuecomment-2077134842)
> > Are there any examples from the last couple of years where this would have helped to prevent a bug or other issue?
>
> Here is an example where two unrelated variables were guarded by the same mutex (already fixed in the latest code):

I don't think this is an example of a bug or other issue.

This was added in commit b312cd770701aa806e9b264154646f481d212c1c to fix a tsan warning. I guess the annotation was using the wrong mutex, but I doubt this will happen in newly written code. Als
...

🚀 fanquake merged a pull request: "depends: build libnatpmp with CMake"
(https://github.com/bitcoin/bitcoin/pull/29708)

📝 m3dwards opened a pull request: "depends: pass verbose through to cmake based makefiles"
(https://github.com/bitcoin/bitcoin/pull/29960)
While testing https://github.com/bitcoin/bitcoin/pull/29708 I was not able to enable verbose output to check which flags were being given to the compiler.

With this PR, running depends with V=1 will enable verbose output from makefiles generated by cmake.

How to test:

```shell
# checkout commit where libnatpmp has been changed to being built with cmake
git checkout 3c1ae3ee33d4d9dbea046d5ab8ee924a12982759
make -C depends libnatpmp V=1
```

👍 alfonsoromanz approved a pull request: "Fix typos in description.md and wallet_util.py"
(https://github.com/bitcoin/bitcoin/pull/29938#pullrequestreview-2022456592)
ACK 03e36b3da093e2c23cf51b46f6901cb84ddbf867

🚀 fanquake merged a pull request: "Fix typos in description.md and wallet_util.py"
(https://github.com/bitcoin/bitcoin/pull/29938)

👍 alfonsoromanz approved a pull request: "test: add missing comparison of node1's mempool in MempoolPackagesTest"
(https://github.com/bitcoin/bitcoin/pull/29948#pullrequestreview-2022468934)
Tested ACK e912717ff63f111d8f1cd7ed1fcf054e28f36409. The code looks good to me and the test execution is successful.

💬 fanquake commented on pull request "depends: pass verbose through to cmake based makefiles":
(https://github.com/bitcoin/bitcoin/pull/29960#issuecomment-2077167651)
Concept ACK