💬 vasild commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1784640440)
I couldn't find the doc of `bpf_usdt_readarg_p()`. What would it do if the argument is longer than the supplied buffer? I presume the last arg is the length of the buffer, so it is not going to overflow, I guess. But would it add a trailing `'\0'`? That is - could it copy `MAX_PEER_ADDR_LENGTH` from the input to the output and leave it like that? If yes, would that be a problem - a string without a terminating `'\0'`?

💬 vasild commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1784875732)
I am not sure how serious is violating the expectation that this is going to be max 128 chars, but if that could cause troubles, maybe here:

```suggestion
message.substr(0, 127).c_str() // 128 including the terminating '\0'
```

💬 vasild commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1784895477)
I got the impression that `$existing` should be already the number of connections, including this one, why `+ 1`? (same question for the code below)

💬 vasild commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1784877393)
64? Shouldn't that be `MAX_MISBEHAVING_MESSAGE_LENGTH` (which is 128)?

💬 vasild commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1784888459)
What happens if the disconnections do not happen within the timeout? Is that 400 seconds? Would it then go to the assert and fail?

Here we can deterministically wait to have 0 connected peers:

```python
self.wait_until(lambda: len(self.nodes[0].getpeerinfo()) == 0)
```

👍 danielabrozzoni approved a pull request: "doc: update IBD requirements in doc/README.md"
(https://github.com/bitcoin/bitcoin/pull/30992#pullrequestreview-2343540714)
ACK 36a6d4b0078ebb39ed082c866bf49214a2a01241

🤔 jonatack reviewed a pull request: "doc: update IBD requirements in doc/README.md"
(https://github.com/bitcoin/bitcoin/pull/30992#pullrequestreview-2343566063)
ACK 36a6d4b0078ebb39ed082c866bf49214a2a01241

🤔 ryanofsky reviewed a pull request: "refactor: Implement missing error checking for ArgsManager flags"
(https://github.com/bitcoin/bitcoin/pull/16545#pullrequestreview-2343217508)
Updated 5a945600451037693a032e6df94f99a666dd581f -> b5ef85497436c3e9e60c760465d8991592efef07 ([`pr/argcheck.42`](https://github.com/ryanofsky/bitcoin/commits/pr/argcheck.42) -> [`pr/argcheck.43`](https://github.com/ryanofsky/bitcoin/commits/pr/argcheck.43), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/argcheck.42..pr/argcheck.43)) implementing review suggestions

💬 ryanofsky commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1784744305)
re: https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1775897208

(note: This comment is about previous code unaffected by this PR)

> seems to me we're treating the input `value` as an implicit optional, even though originally it's an actual `std::optional<std::string>`.

Absolutely yes. In C and C++ `T*` is semantically equivalent to `std::optional<T&>`.

> Given that we're already returning an `std::optional`, can we consider avoiding pointers and using a `std::optional<std::s
...

💬 ryanofsky commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1784705471)
re: https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1775897382

> nit: alignment

Thanks, updated

💬 ryanofsky commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1784758735)
re: https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1776007657

(note: This comment is about previous code unaffected by this PR)

> I find it quite confusing that have a return value, and error return value and even warning as possible outcomes.

It would probably be better for this to return util::Result, but in fairness this function is only called two places and intended for internal use, so exact form of its API hopefully doesn't matter too much.

💬 ryanofsky commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1784840267)
re: https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1776018821

> we could change this to an `error += ` syntax:

Unsure about this, I feel like this could make strings harder to search for and translate. And since the new code does not explicitly check for bool type it could lead to a bug if support for other types is added. Leaving alone for now. I can see appeal of this approach but it doesn't seem like a clear win.

💬 ryanofsky commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1784831993)
re: https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1776017230

It's interesting to see what that looks like but I see few disadvantages to that approach:

- Superficially, the resulting code is longer and the diff is bigger. Current PR is trying to minimally change this function.
- It seems to be duplicating some code like "Cannot set -%s with no value" error, increasing risk that differences between the two copies of code will be introduced accidentally, and introducing unintend
...

💬 ryanofsky commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1784899977)
re: https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1776028495

Thanks, applied suggestion

💬 ryanofsky commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1784804173)
re: https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1775907489

> nit: this is only used once, maybe we could have a local `TryParseInt64` next to `InterpretBool` - details below

Thanks, adopted suggested approach.

Note: other comment with details is: https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1775907489

💬 ryanofsky commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1784852608)
re: https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1776023938

Again, it's interesting to see what this looks like. so thanks for spelling it out. I don't love the `flags & ALLOW` syntax, so it's nice that helper functions hide it, but I feel the helper functions add a level of indirection that make it harder to plainly see what the code is doing.

Specifically they make this checking code less clear because of instead of checking the flags and then mentioning the flags just check
...

💬 ryanofsky commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1784833826)
re: https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1775907489

> we could group the two bools and use an optional for the in parsing (assuming `std::optional` `value` parameter):
> TryParseInt64

Thanks, switched to this approach.

:lock: fanquake locked an issue: "Bitcoin 💵"
(https://github.com/bitcoin/bitcoin/issues/31020)

🤔 l0rinc reviewed a pull request: "cmake: Avoid hardcoding Qt's major version in Find module / variable names"
(https://github.com/bitcoin/bitcoin/pull/31010#pullrequestreview-2343622187)
Concept ACK

💬 l0rinc commented on pull request "cmake: Avoid hardcoding Qt's major version in Find module / variable names":
(https://github.com/bitcoin/bitcoin/pull/31010#discussion_r1784944693)
Do we need to keep the versions here?