💬 glozow commented on pull request "policy: restrict all TRUC (v3) transactions to 10KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#discussion_r1608401789)
done

💬 glozow commented on pull request "policy: restrict all TRUC (v3) transactions to 10kvB":
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2122725600)
Thanks @murchandamus, addressed nits

💬 pablomartin4btc commented on pull request "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/28670#issuecomment-2122739997)
> Hm, unfortunately, I think this PR was made redundant by #29612 recently. After the latest feedback there which I addressed just 3 weeks ago, we are enhancing the assumeutxo metadata there significantly and this means all of the lines changed here are touched there as well and I don't see much of an overlap either that would let me use the code here.

No prob, thanks for letting me know, I'll review that PR.

✅ pablomartin4btc closed a pull request: "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset"
(https://github.com/bitcoin/bitcoin/pull/28670)

💬 pablomartin4btc commented on pull request "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/28670#issuecomment-2122741411)
> Hm, unfortunately, I think this PR was made redundant by #29612 recently. After the latest feedback there which I addressed just 3 weeks ago, we are enhancing the assumeutxo metadata there significantly and this means all of the lines changed here are touched there as well and I don't see much of an overlap either that would let me use the code here.

No prob, thanks for letting me know, I'll review that PR.

💬 theuni commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1608423170)
That'd be my preference, yes. I'll see about adding a clang-tidy check for that combo.

📝 furszy reopened a pull request: "wallet: re-activate the not triggered "AmountWithFeeExceedsBalance" error"
(https://github.com/bitcoin/bitcoin/pull/25269)
Please review #25806 first.

Came up during a talk with @theStack: the `AmountWithFeeExceedsBalance` error inside `WalletModel:prepareTransaction` is never been thrown.

The explanation for it: `createTransaction` does not retrieve the fee if the process fail for insufficient funds (fee return argument is set only at the bottom of the process, when the transaction creation success). So, if the tx creation fails, it's not available inside `WalletModel::prepareTransaction` to be able to throw
...

👋 furszy's pull request is ready for review: "wallet: re-activate the not triggered "AmountWithFeeExceedsBalance" error"
(https://github.com/bitcoin/bitcoin/pull/25269)

💬 maflcko commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1608448187)
> clang-tidy check

A faster and easier approximation would be to use `git grep -l thread_local`, excluding this file (threadnames.cpp), and the normal lint-exclude list.

💬 stickies-v commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#issuecomment-2122786225)
Approach ACK. First 2 commits (9de8b263dabd6dd2f86f1f0253c6ee3fac7a4407) LGTM but the third one I'm going to need to spend a lot more time wrapping my head around the implications.

💬 adityajatnika commented on pull request "ci: Add mising -Wno-error=maybe-uninitialized to armhf task":
(https://github.com/bitcoin/bitcoin/pull/30144#issuecomment-2122791047)
lgtm

💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1608491222)
Now simplified in d8d47ab395b8fa341e0aac2fe01a4b9488af5347

💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1608491887)
Fixed in 5a20b078c3a418103601ee6eb88e5dba8e6d9863

💬 maflcko commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1608506110)
> extra debug logs noise we will get.

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] Receiv
...

💬 maflcko commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1608509648)
```suggestion
f"-uacomment=testnode{i}", # required for subversion uniqueness across peers
```

style-nit, if you retouch

💬 maflcko commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1608520104)
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.

👍 maflcko approved a pull request: "test: improve robustness of connect_nodes()"
(https://github.com/bitcoin/bitcoin/pull/30118#pullrequestreview-2068926018)
utACK 6629d1d0f8285d1bf2d87341a856abe903f26c13

📝 theuni opened a pull request: "Add clang-tidy check for thread_local vars"
(https://github.com/bitcoin/bitcoin/pull/30146)
Forbid thread_local vars with non-trivial destructors.

This is a follow-up from: https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1608423170

💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-2122988218)
Guix Build (aarch64, x86_64):
```bash
a0f00b836d0843e3e9be0cef471f455d113de49da467424718fafeb981490be5 guix-build-e208d5df7937/output/aarch64-linux-gnu/SHA256SUMS.part
a1ccae9ef1a5f4f1641c864c2aa173c7fde2f63de5519c8a13bf2e13b1a16b94 guix-build-e208d5df7937/output/aarch64-linux-gnu/bitcoin-e208d5df7937-aarch64-linux-gnu-debug.tar.gz
79d2affa00eda5657e322b864fe31c49cde9971c5a1ef303fa781d4740748979 guix-build-e208d5df7937/output/aarch64-linux-gnu/bitcoin-e208d5df7937-aarch64-linux-gnu.tar.gz
...

💬 maflcko commented on pull request "Add clang-tidy check for thread_local vars":
(https://github.com/bitcoin/bitcoin/pull/30146#discussion_r1608612268)
```
init.cpp:723:21: error: unexpected ':' in nested name specifier; did you mean '::'?