💬 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).

💬 hodlinator commented on pull request "clang-format: make formatting deterministic for different formatter versions":
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2315284495)
Noting down further arguments for the current version of the PR:
* The fact that *developer-notes.md* already explicitly mentions *functions & methods* means class & struct were probably intentionally included & omitted respectively.
* `AfterStruct` and `AfterClass` were both introduced as part of LLVM 3.8.0 in 2016 (See https://releases.llvm.org/3.8.0/tools/clang/docs/ClangFormatStyleOptions.html vs https://releases.llvm.org/3.7.0/tools/clang/docs/ClangFormatStyleOptions.html), yet only the l
...

💬 maflcko commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3244309188)
re-ACK 3c5da69a232ba1cfb935012aa53e57002efe0d77 🏗

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 3c5da69a232ba1cfb935
...

💬 maflcko commented on pull request "clang-format: make formatting deterministic for different formatter versions":
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2315342371)
> similar to initializer lists ([#32740 (comment)](https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2294742258)).

When formatting initializer lists, one can manually put the `:` on the next line. This should give a stable clang-format output. (With the current line limit, clang-format won't split or merge lines by itself)

💬 maflcko commented on pull request "clang-format: make formatting deterministic for different formatter versions":
(https://github.com/bitcoin/bitcoin/pull/32813#issuecomment-3244360723)
re-ACK 13f36c020f0329b5e975282b45292fdf2a495e31 🖼

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 13f36c020f0329b5e975
...

🤔 optout21 reviewed a pull request: "doc: unify `datacarriersize` warning with release notes"
(https://github.com/bitcoin/bitcoin/pull/33224#pullrequestreview-3175541576)
ACK 2885bd0e1c4fc863a7f28ff0fd353f5cffb03442

Minute details like this in documentation improve the communication of the project.
The change is doc-only, minimal, well isolated.

💬 dergoegge commented on pull request "p2p: add `DifferenceFormatter` fuzz target and invariant check":
(https://github.com/bitcoin/bitcoin/pull/33252#issuecomment-3244443381)
7a6b3b4ebfc04f9fa4520e3caaeeae9dfe0d8c5f still mentions `assert` in the commit message but it's now using `Assume`.

Otherwise this looks good to me.

💬 optout21 commented on pull request "mini miner: enable `Linearize` return package feerates":
(https://github.com/bitcoin/bitcoin/pull/33216#discussion_r2315422039)
nit: Add a comment on the struct?

👍 vasild approved a pull request: "net: Prevent node from binding to the same `CService`"
(https://github.com/bitcoin/bitcoin/pull/33231#pullrequestreview-3175416151)
ACK 28fb7f74c4ae66dcb0f918774d644ac3c393f25b

Some non-blocker comments below, would be happy to re-review if addressed.

💬 vasild commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2315309450)
```suggestion
* @param[in] connOptions Connection options containing the binding vectors to check
```

💬 vasild commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2315349643)
It wasn't clear to me what the return value is before reading the comment, so this is useful. Sometimes comments are more on the side of redundant/noisy/useless, but the line between that and useful comments is blurred and personally I prefer to err on the side of redundancy.

If there are no comments whatsoever, this sets a standard. Then if the function is extended with not-so-obvious parameters or return value changed, the developer doing it will likely not add a comment for the new paramet
...

💬 vasild commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2315387348)
Here and also in the commit message, this lists 3 categories: `-bind`, `-whitebind`, `onion`. There is an `-onion` config option which is not related to binding. This could be confusing. The intention here is to mean "onion" -> "a subset of -bind if suffixed with =onion". That is, just mentioning `-bind` also covers `-bind=...=onion`. So maybe change this to:

`Please check your -bind and -whitebind settings` or to
`Please check your -bind, -bind=...=onion and -whitebind settings`.

💬 vasild commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2315420542)
> since we already know which container the duplicate came from

That is known inside `CheckBindingConflicts()` but the info is not signaled to the caller of the function. Also, even inside the function, only one of the containers is known, the other is not. E.g. if the 3rd for-loop finds a duplicate, then it is unknown whether the entry was added in the first or the second for-loop.

> I think this should at least be translatable for consistency with nearby init errors

Right, +1 for tran
...

💬 vasild commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2315372807)
Since the new function is used only in this file:

```suggestion
static std::optional<CService> CheckBindingConflicts(const CConnman::Options& connOptions)
```