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

💬 theuni commented on pull request "Add clang-tidy check for thread_local vars":
(https://github.com/bitcoin/bitcoin/pull/30146#discussion_r1608615606)
Hah

👍 theuni approved a pull request: "build: Remove `--enable-threadlocal`"
(https://github.com/bitcoin/bitcoin/pull/30137#pullrequestreview-2069121155)
utACK 17fe948cce2eb75f0f3f4b0db9d0d90648c7d4af

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2123033320)
> Am I right that after this change (or alternatives which let BatchWrite just iterate dirty/fresh entries), it would be possible to (perhaps dramatically) reduce the periodic flush time, so that after IBD, we basically always have a mostly non-dirty cache? Such a change could be made today of course, but the cost of iterating the entire cache every time would add up quickly.

I believe you are right, but I'm unsure what the benefit is that you see. From my understanding, the periodic flushes
...

💬 achow101 commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#issuecomment-2123038131)
Rebased following #26606. Now ready for review.

👋 achow101's pull request is ready for review: "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB"
(https://github.com/bitcoin/bitcoin/pull/26596)

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2123045950)
@andrewtoth I was talking about post-IBD. And one advantage to more frequent flushing is reducing the latency spikes you get when a flush is triggered.

👍 theStack approved a pull request: "refactor prep for package rbf"
(https://github.com/bitcoin/bitcoin/pull/30072#pullrequestreview-2068992752)
Code-review ACK 6fcd3cc016eaf07e47c5c448482ff844855a247b

Out of curiosity, I verified that the extended carve-out functional test in 868c3e77e01df8cfdc00a7599f16d0adf019aee7 (checking `testmempoolaccept` and `submitpackage` results) would also fail on master, but in a slightly different way, as the carve out leads to a failure later (in `CheckPackageLimits()` vs the earlier `PreChecks()` for `testmempoolaccept`; for `submitpackage` the first tx succeeds, and only the second fails with `"too-l
...

💬 theStack commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1608552045)
spelling nit:
```suggestion
/** When true, the transaction or package will not be submitted to the mempool. */
```

💬 achow101 commented on pull request "wallet, tests: Avoid stringop-overflow warning in PollutePubKey":
(https://github.com/bitcoin/bitcoin/pull/30131#discussion_r1608661403)
Done

📝 BenWestgate opened a pull request: "script: Fix errors in verify-binaries/verify.py OS platform parsing, update test.py & docs"
(https://github.com/bitcoin/bitcoin/pull/30147)
Closes #30145.

This PR solves two major issues in the `parse_version_string` function of verify-binaries:
1. `-aarch64` binaries cannot be specifically downloaded. The -platform string gets interpreted as a release candidate that doesn't exist due to containing sub-string "rc".
2. Specifying a platform with a "-" in the name causes the parser to ignore both "-platform" AND "-rcN" and download the potentially wrong (non-rc) version for every platform. This also prevented specifying just one
...

💬 theStack commented on pull request "test: improve BDB parser (handle internal/overflow pages, support all page sizes)":
(https://github.com/bitcoin/bitcoin/pull/30125#discussion_r1608681928)
Good idea, done.