💬 TheCharlatan commented on pull request "Add clang-tidy check for thread_local vars":
(https://github.com/bitcoin/bitcoin/pull/30146#discussion_r1609967684)
> I don't have freeBSD to test this, but are you sure it doesn't crash when the dtor is there but empty?

It looks like it should work to me. If a method has a body it should also contain a compound statement, or at least this [book](https://github.com/PacktPublishing/LLVM-Techniques-Tips-and-Best-Practices-Clang-and-Middle-End-Libraries) seems to suggest as much.

> Also, to be clear, that TODO is an upstream comment, not one from me: https://github.com/llvm/llvm-project/blob/main/clang-too
...

👍 AngusP approved a pull request: "contrib/signet/miner: increase miner search space"
(https://github.com/bitcoin/bitcoin/pull/30130#pullrequestreview-2071241565)
utACK 1cf174a2954b434e9623afe86f3643be8eec2d70

👍 TheCharlatan approved a pull request: "Add clang-tidy check for thread_local vars"
(https://github.com/bitcoin/bitcoin/pull/30146#pullrequestreview-2071241175)
ACK ace5b35fb435ca2646bb8738e6578187ae2e8720

💬 TheCharlatan commented on pull request "Add clang-tidy check for thread_local vars":
(https://github.com/bitcoin/bitcoin/pull/30146#discussion_r1609977454)
Nit: Four spaces for indents like in the rest of the code?

🚀 fanquake merged a pull request: "doc: Correct pull request prefix for scripts and tools"
(https://github.com/bitcoin/bitcoin/pull/30150)

💬 theuni commented on pull request "Add clang-tidy check for thread_local vars":
(https://github.com/bitcoin/bitcoin/pull/30146#issuecomment-2124847802)
Updated to resolve @TheCharlatan's nit, and also specified where `hasNonTrivialDestructor` came from.

👍 TheCharlatan approved a pull request: "Add clang-tidy check for thread_local vars"
(https://github.com/bitcoin/bitcoin/pull/30146#pullrequestreview-2071277257)
Re-ACK 34c9cee380e7276cf3f85f2ce56a293e57afd676

💬 furszy commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1610019226)
> nit: As you remove the `version` check, I wonder if this one can be removed as well for the same reason? The final `pong` test should be necessary and sufficient already.

I removed the `version` check because `verack`s are direct responses to `version` messages at the protocol level. I'm not sure we should rely on the `pong` alone because that might change over time?

💬 maflcko commented on pull request "Add clang-tidy check for thread_local vars":
(https://github.com/bitcoin/bitcoin/pull/30146#discussion_r1610026534)
> > I don't have freeBSD to test this, but are you sure it doesn't crash when the dtor is there but empty?
>
> It looks like it should work to me. If a method has a body it should also contain a compound statement, or at least this [book](https://github.com/PacktPublishing/LLVM-Techniques-Tips-and-Best-Practices-Clang-and-Middle-End-Libraries) seems to suggest as much.

Oh, I didn't mean clang-tidy to crash, but the program that uses thread_local in reference to https://bugs.freebsd.org/bug
...

💬 furszy commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1610031890)
> The debug logs could be added when needed. There are already two log lines, so a third (when added) shouldn't matter too much, I'd say.
>
> For example:
>
> ```
> node1 2024-05-01T17:15:16.401946Z (mocktime: 2024-05-01T16:46:25Z) [httpworker.0] [rpc/request.cpp:187] [parse] [rpc] ThreadRPCServer method=getpeerinfo user=__cookie__
> node1 2024-05-01T17:15:16.453092Z (mocktime: 2024-05-01T16:46:25Z) [http] [httpserver.cpp:306] [http_request_cb] [http] Received a POST request for / fro
...

💬 maflcko commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1610033578)
> I'm not sure we should rely on the `pong` alone because that might change over time?

If it changes (for example the ping is sent before the verack), the test will already start to fail intermittently and will need to be adjusted anyway.

See the next line comment:

```
# The message bytes are counted before processing the message, so make
# sure it was fully processed by waiting for a ping.

💬 maflcko commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1610039510)
Yeah, sounds good to close this thread for now.

💬 instagibbs commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1606940558)
nit `TxRequest` isn't a thing?

🤔 instagibbs reviewed a pull request: "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip"
(https://github.com/bitcoin/bitcoin/pull/30111#pullrequestreview-2066488661)
looking pretty straightforward but I'm not an expert in the current locking setup

will give another pass in a bit

💬 instagibbs commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1607161194)
not a locking expert but would `LOCKS_EXCLUDED` be easier to read?

💬 instagibbs commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1607169815)
in commit message mention what it's obsoleted by, e.g., external mutex by caller

💬 instagibbs commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1607177017)
what's the value of having the now bare wrapper `EraseTx`

💬 instagibbs commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1607156264)
seems to be covered at https://github.com/bitcoin/bitcoin/pull/30111/commits/9d413afd127ab1753f316d5d3c54e74830f3c9b6#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R3635 now

💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1610060323)
fixed, thanks

💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1610060814)
took, thanks