💬 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

💬 hebasto commented on pull request "[DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system -- WIP":
(https://github.com/bitcoin/bitcoin/pull/29790#issuecomment-2124935777)
> I've noticed two issues in the "tidy" job so far:
>
> 1. `Cannot open mapping file '/ci_container_base/ci/scratch/build-/contrib/devtools/iwyu/bitcoin.core.imp': No such file or directory.`
>
> 2. Missed disabling `EXPORT_COMPILE_COMMANDS` property for targets in the `crc32c` subtree.

The recent push includes:
- commits from https://github.com/hebasto/bitcoin/pull/205, which fixes the point 1
- commits from https://github.com/hebasto/bitcoin/pull/206, which fixes the point 2

🤔 furszy reviewed a pull request: "refactor: interfaces, make 'createTransaction' less error-prone "
(https://github.com/bitcoin-core/gui/pull/807#pullrequestreview-2071420549)
Thanks for the review ryanofsky!

> I just noticed [bitcoin/bitcoin#25269](https://github.com/bitcoin/bitcoin/pull/25269), which I hadn't seen when I posted my comment above. It looks like that PR was closed for inactivity but it does look like a good approach that solves the issue I didn't like in my comment. (Though maybe that PR is still a slight regression because it doesn't include the amount of the fee in the error message.) I wonder if you'd consider reopening that PR and maybe includin
...

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1610124181)
Added in b2322fc476dc983b91492768d2054c2a3966afdd