💬 maflcko commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-2407451391)
> Wrt "how useful is this": the first part of this PR (also isolated in #30205) is already used in Sv2Transport tests.

I followed the links but only found a closed pull requests: https://github.com/bitcoin/bitcoin/pull/30332#event-13580183288

I think it is easy to say that something will be used in the future, but I think it would be easier if the framework commits were added with non-redundant tests at the same time.

👍 ryanofsky approved a pull request: "refactor: Check translatable format strings at compile-time"
(https://github.com/bitcoin/bitcoin/pull/31061#pullrequestreview-2362745062)
Code review ACK fae6acacc0216aa07f5a4a0ee75fbade620d7023. This is a nice improvement overall, and definitely better than the status quo due to extra type checking it provides. I do think there is extra complexity here and extra changes that could go away if we adopt #31072 and stop passing untranslated format strings to strprintf.

I also think it probably would be nicer if `_()` would continue to look up translations right away, instead of delaying them until conversion to bilingual str. I t
...

💬 ryanofsky commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1796975768)
In commit "refactor: Delay translation of Untranslated() or _() literals" (fa62737ae398bd56ef9e68b8d0a4d217cfcc532b)

Thinking about this more it seems like if `_` function does not be consteval, and could just a template type that inherits from `bilingual_str`, and included the number of format specifiers included in the string in its type information. This would be nice because runtime semantics of `_` would be unchanged, it would still do the translation at time it is called, and there woul
...

💬 ryanofsky commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1796917617)
re: https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1795269088

I don't see ability to swap `_` and `Untranslated` as a real benefit here, because it is pretty easy to switch between translated and non translated format strings either way, and if you are doing that it probably makes sense to actually look at format string arguments and see if they should be translated or untranslated for consistency. The PR also already loses the ability to swap between `_` and `Untranslated` in othe
...

🤔 jonatack reviewed a pull request: "rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats'"
(https://github.com/bitcoin/bitcoin/pull/29418#pullrequestreview-2361526335)
Lightly tested, a few initial questions/comments.

💬 jonatack commented on pull request "rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats'":
(https://github.com/bitcoin/bitcoin/pull/29418#discussion_r1796165328)
```suggestion
if (key.empty()) {
```

💬 jonatack commented on pull request "rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats'":
(https://github.com/bitcoin/bitcoin/pull/29418#discussion_r1796983203)
I'm confused by the formatting here.

💬 jonatack commented on pull request "rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats'":
(https://github.com/bitcoin/bitcoin/pull/29418#discussion_r1796982625)
for lines 233-262, it might be more efficient to replace all of these `if self.options.v2transport` conditionals with just one (and if v1 is ever removed, doing so would be a cleaner diff)

💬 jonatack commented on pull request "rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats'":
(https://github.com/bitcoin/bitcoin/pull/29418#discussion_r1796173208)
Suggest making all the added test logging after "Test getnetmsgstats" `debug` level rather than `info`.
```suggestion
self.log.debug("Compare byte count getnetmsgstats vs getnettotals")
```

💬 jonatack commented on pull request "rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats'":
(https://github.com/bitcoin/bitcoin/pull/29418#discussion_r1796992532)
Would it make sense to put these 2 `ConnectionType` utility methods in `node/connection_types.{h,cpp}` where they could be more widely used?

💬 jonatack commented on pull request "rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats'":
(https://github.com/bitcoin/bitcoin/pull/29418#discussion_r1796174636)
nit, this prints as:

`(json array, optional, default=empty (no aggregation)) An array of keywords for aggregating the results.`

Perhaps avoid the double braces with

`(json array, optional, default=empty, no aggregation) An array of keywords for aggregating the results.`

```suggestion
/*fallback=*/RPCArg::DefaultHint{"empty, no aggregation"},
```

💬 jonatack commented on pull request "rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats'":
(https://github.com/bitcoin/bitcoin/pull/29418#discussion_r1796995737)
Note to self, these 2 `Network` methods might be candidates for extraction in https://github.com/bitcoin/bitcoin/pull/27385.

💬 jonatack commented on pull request "rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats'":
(https://github.com/bitcoin/bitcoin/pull/29418#discussion_r1796987675)
Would there be any benefit to moving this class outside of `net.{h,cpp}`? It's quite a lot of code.

(Though, much of its verbosity is the formatting style used that often reads top-to-bottom rather than left-to-right. I prefer the latter.)

🤔 maflcko reviewed a pull request: "scripted-diff: Replace strprintf(Untranslated(...)) with Untranslated(strprintf(...))"
(https://github.com/bitcoin/bitcoin/pull/31072#pullrequestreview-2362888700)
I don't mind the changes here, but I don't think the motivation is correct and it seems misleading.

I don't think it matters much whether this is merged or not, but you could just replace the motivation with:

"This replacement makes the implementation of some compile-time checks in the future easier. Also, it makes the code a bit more consistent in that `Untranslated` isn't allowed as a format-string anymore, only plain `std::string`s, or translated strings. The downside would be that deve
...

💬 maflcko commented on pull request "scripted-diff: Replace strprintf(Untranslated(...)) with Untranslated(strprintf(...))":
(https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1797005420)
in commit " refactor: Use + instead of strformat to concatenate Untranslated strings ":

I still don't think this is correct. You are using `+` to concatenate `Untranslated` and `_()` (translated), possibly mixing English strings with non-English ones. Also, this compiles, so there are no compile-time checks preventing the "mix".

💬 ryanofsky commented on pull request "scripted-diff: Replace strprintf(Untranslated(...)) with Untranslated(strprintf(...))":
(https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1797036507)
> I still don't think this is correct.

What part of this is not correct? Is there a specific sentence or something?

> You are using `+` to concatenate `Untranslated` and `_()` (translated), possibly mixing English strings with non-English ones. Also, this compiles, so there are no compile-time checks preventing the "mix".

There is nothing in this commit message (19f49f138c4756b5219e715d8749d15be07e2c81) which mentions mixing English or non-english strings.

As a whole, this PR is upda
...

💬 fanquake commented on issue "Consider making 27.x Long-Term Support (LTS)":
(https://github.com/bitcoin/bitcoin/issues/31068#issuecomment-2407528118)
> What would be the downside of only having glibc 2.27 release binaries for x86_64 (and/or aarch64), as opposed to having a separate old-glibc build?

If what you're suggesting is :
`x86_64-linux-gnu`: 2.27
`arm-linux-gnueabihf`: 2.modern
`aarch64-linux-gnu`: 2.27
`riscv64-linux-gnu`: 2.modern
`powerpc64-linux-gnu`: 2.modern
`powerpc64le-linux-gnu`: 2.modern (when re-enabled)

Some might be:
* We can't ship modern (hardening) features to users running `x86_64` or `aarch64`. i.e #24123
...

💬 sipa commented on issue "Consider making 27.x Long-Term Support (LTS)":
(https://github.com/bitcoin/bitcoin/issues/31068#issuecomment-2407533799)
@fanquake Fair points, though your second and third point also apply to having a separate old-glibc build (in addition to having modern x86_64/aarch64 builds).

💬 maflcko commented on pull request "scripted-diff: Replace strprintf(Untranslated(...)) with Untranslated(strprintf(...))":
(https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1797046367)
> > I still don't think this is correct.
>
> What part of this is not correct? Is there a specific sentence or something?

Sorry for being unclear. The commit title says

"refactor: Use + instead of strformat to concatenate Untranslated strings".

It should say:

"refactor: Use + instead of strformat to concatenate bilingual strings, which may be Untranslated or translated (_)"

💬 danielabrozzoni commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#discussion_r1797035913)
nit: I don't think `verbosity=2` is needed