💬 frankomosh commented on pull request "p2p: add `DifferenceFormatter` fuzz target and invariant check":
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2314870867)
Had only kept it for readability, but yes we can definately do without it

💬 frankomosh commented on pull request "p2p: add `DifferenceFormatter` fuzz target and invariant check":
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2314871367)
yes. thanks

💬 l0rinc commented on pull request "Draft: CCoinMap Experiments":
(https://github.com/bitcoin/bitcoin/pull/32128#discussion_r2314939752)
It looks like this branch isn't needed, in which case we can always just `try_emplace` (and add a dead-code erase for the tests), see [`ae76ec7` (#30673)](https://github.com/bitcoin/bitcoin/pull/30673/commits/ae76ec7bcff0a08a61f294882a71e46d177b009f#diff-f0ed73d62dae6ca28ebd3045e5fc0d5d02eaaacadb4c2a292985a3fbd7e1c77cR184)

I've pushed it to my own fork for now (https://github.com/l0rinc/bitcoin/pull/34), adding you and @andrewtoth as coauthors. Will publish once my benchmarks finish.

💬 maflcko commented on issue "InitError() doesn't always halt node startup when blockchain state exists":
(https://github.com/bitcoin/bitcoin/issues/33276#issuecomment-3243949312)
I can't reproduce this locally, but I guess you are referring to the `initload` thread doing its work? If yes, I presume the only fix would be to validate the args before starting the initload thread?

💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2315050447)
No objection, but I am not sure we should diverge the subtree for a test-only patch. Also, the patch is pre-existing, so it seems a bit unrelated to this pull.

Ideally, Google would fix the UB in some way or another, and then we can pull whatever their fix was, but that shouldn't be a blocker here.

💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2315050556)
fwd decls are slimmer than a full include, and often sufficient, for example when the header only uses the types as pointers or references. IWYU will suggest them sometimes, when possible.

💬 maflcko commented on issue "intermittent timeout in mptest unit test":
(https://github.com/bitcoin/bitcoin/issues/33244#issuecomment-3243976122)
At the same time, it seems to happen frequently in CI, so I prefer my initial suggestion to either rewrite the failing unit test or remove it temporarily.

💬 Sjors commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#issuecomment-3244007472)
> Now that the router has been fully reset without any lingering configs from prior major versions, PCP & NATPMP is actually working.

You would not be the first person running into router bugs while testing NAT punching functionality :-)

💬 Sjors commented on pull request "kernel: chainparams & headersync updates for 30.0":
(https://github.com/bitcoin/bitcoin/pull/33274#discussion_r2315087718)
For a node that's been offline for a year or so and then caught up yesterday:

```
16G testnet3/chainstate/
195G testnet3/blocks/
210G total
```

💬 Sjors commented on pull request "kernel: chainparams & headersync updates for 30.0":
(https://github.com/bitcoin/bitcoin/pull/33274#discussion_r2315089293)
Testnet4 node that's been online a couple of times, but not continuously:

```
938M testnet4/chainstate/
8,1G testnet4/blocks/
9,0G total
```

💬 l0rinc commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2315091137)
The main leveledb repo doesn't even compile for the past 8 months, not sure why we would wait for them. But as I mentioned, I don't mind doing it after the PR, just seems cleaner if we didn't need to change this section again.

💬 Sjors commented on pull request "kernel: chainparams & headersync updates for 30.0":
(https://github.com/bitcoin/bitcoin/pull/33274#discussion_r2315092723)
And one that I spun up in June and his been online since:

```
915M testnet4/chainstate/
20G testnet4/blocks/
21G total
```

💬 l0rinc commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2315093684)
I know, that wasn't my question. But I'll figure it out next, feel free to resolve it.

💬 saikiran57 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#issuecomment-3244074756)
Hi @furszy kindly acknowledge this review its been waiting for very long time.

💬 Sjors commented on pull request "kernel: chainparams & headersync updates for 30.0":
(https://github.com/bitcoin/bitcoin/pull/33274#discussion_r2315145587)
Intermittently online node (though that shouldn't make a difference on this signet):

```
2,8G signet/chainstate/
15G signet/blocks/
18G total
```

💬 maflcko commented on pull request "build: Set AUTHOR_WARNING on warnings":
(https://github.com/bitcoin/bitcoin/pull/33144#issuecomment-3244099875)
Sounds good. Happy to put this in draft for now to see if your removal of `configure_warnings` makes it in, but I haven't looked at 2. and 3., if they make sense.

💬 maflcko commented on pull request "[IBD] specialize block serialization":
(https://github.com/bitcoin/bitcoin/pull/31868#issuecomment-3244110633)
What was the speedup? I can only see your last comment, which says:

> the PR is still in draft mode until I remeasure its effect on IBD

💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2315254202)
https://github.com/bitcoin/bitcoin/blob/7cc9a087069bfcdb79a08ce77eb3a60adf9483af/src/index/base.h#L116

👍 hodlinator approved a pull request: "clang-format: make formatting deterministic for different formatter versions"
(https://github.com/bitcoin/bitcoin/pull/32813#pullrequestreview-3175194035)
re-ACK 13f36c020f0329b5e975282b45292fdf2a495e31

`struct` brace style was made to explicitly differ from `class` in developer-notes.md after some back & forth (https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2303151367).

💬 hodlinator commented on pull request "clang-format: make formatting deterministic for different formatter versions":
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2315150488)
nit:
https://clang.llvm.org/docs/ClangFormatStyleOptions.html#reflowcomments

We might want to set `IndentOnly`, so we don't unnecessarily touch lines. But probably better to follow up on this in another PR, similar to initializer lists (https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2294742258).