💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1609932428)
I added a fix for https://github.com/bitcoin/bitcoin/pull/28241/files#diff-1b4444ba5939fff2ab9f70d1d0d29fc2653b09a332e462166e01891e261c9fa9 and rebuilding the index now.

💬 theStack commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1609936760)
Isn't there some kind of special handling needed for large values that don't fit into a byte after the shift (i.e. if txout.nValue >4080 sats)?

💬 theStack commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1609948787)
I'd suggest to introduce a fast `CScript::IsPayToTaproot` helper. It can be used both in `MaybeSilentPayment` and directly in the loop for finding the largest output amount (commit 337471e4acff439100488c16876d9f110ef04ef7), without the need to involve `Solver`:

<details>
<summary>patch to add `CScript::IsPayToTaproot()`</summary>

```diff
diff --git a/src/script/script.cpp b/src/script/script.cpp
index 80e8d26bcf..d0e677aaa4 100644
--- a/src/script/script.cpp
+++ b/src/script/script.cp
...

👍 theuni approved a pull request: "doc: Correct pull request prefix for scripts and tools"
(https://github.com/bitcoin/bitcoin/pull/30150#pullrequestreview-2071218768)
ACK fa3e1151a28345edff8f371283745bdd647f9a74

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1609954791)
bebfafcf98d8ed1432407b7603991d54f7cc26c2: it would be useful to have a quick recap here of which strategy is used by `QueryDefaultGateway` for which OS.

I think it will be (slightly) more readable to have a single `QueryDefaultGateway` implementation which then calls `QueryDefaultGatewayWindows`, `QueryDefaultGatewayMac` and `QueryDefaultGatewayLinuxBSD`.

It can start with:

```cpp
Assume(network == NET_IPV4 || network == NET_IPV6)
```

and end with

```cpp
#else return std::nullo
...

💬 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?