💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1784580197)
not strictly a superset, right? Uses `NON_GOOD_RESPONSES` if not good which excludes `TX_REAL`

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1784885638)
I'm not sure what this comment means. I know the `honestid` peer needs to be wtxid relaying to ensure that it can properly serve `malleated_tx` when necessary, otherwise `GetCorrectTx` will fetch the wrong response by giving back `correct_tx`.

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1784688753)
This should catch issues marking things as received properly, at least for wtxid version since we're forcing wtxid relay for good peer.
```Suggestion
// Honest peer only tells us about INV_REAL in this scenario, should be empty since REAL_TX received
if (peer.m_nodeid == honestid) {
txdownloadman.CheckIsEmpty(honestid);
}
txdownloadman.DisconnectedPeer(peer.m_nodeid);
```

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1784582239)
Could use `PickCoins` here as well

⚠️ Khalilheyrani6367 opened an issue: "Bitcoin 💵"
(https://github.com/bitcoin/bitcoin/issues/31020)
Bitcoin (BTC) Address: bc1qumf5ap6g85qanrrjhutqrkrvl3phch4fn66frn

💬 Khalilheyrani6367 commented on issue "Bitcoin 💵":
(https://github.com/bitcoin/bitcoin/issues/31020#issuecomment-2389135162)
Deposit bitcoin wallet exodus

✅ pinheadmz closed an issue: "Bitcoin 💵"
(https://github.com/bitcoin/bitcoin/issues/31020)

🤔 vasild reviewed a pull request: "tracing: network connection tracepoints"
(https://github.com/bitcoin/bitcoin/pull/25832#pullrequestreview-2337397321)
Approach ACK 5b59cfc944f9eac73e1e69bcc660280b6809724a

💬 vasild commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1781040991)
That `68` probably comes from here:

https://github.com/bitcoin/bitcoin/blob/a0473442d1c22043f5a288bd9255c006fd85d947/test/functional/interface_usdt_net.py#L22-L23

but I just did this:

Added to `/etc/hosts`:
```
24.116.163.227 aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
```
(that is 70 `a`s)

Then `addnode aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:8333` and `CNode::m_addr_name` gets assigned that 70-chars long string.

Less
...

💬 vasild commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1784658950)
For outbound connections, the peer does not "connect from".

```suggestion
4. Network of the peer as `uint32` (1 = IPv4, 2 = IPv6, 3 = Onion, 4 = I2P, 5 = CJDNS). See `Network` enum in `netaddress.h`.
```

or "Network to which the peer's address belongs".

💬 vasild commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1784806169)
nit: currently we evict only inbounds, but this may change in the future. Maybe drop the "inbound" from here so that this does not get outdated:

```suggestion
Is called when a connection is evicted by us. Passes information about the evicted peer and the time at connection establishment.
```

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