💬 glozow commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1784854966)
nit: comment is unnecessary. Also my personal preference is to not put them on the same line

💬 glozow commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1784861016)
an annotation can help here
```suggestion
int verbosity{ParseVerbosity(request.params[1], /*default_verbosity=*/1)};
```

💬 glozow commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1784859575)
I find default arguments to be pretty footgunny and prefer to require explicit. Since you're introducing this function for the first time, I suggest no default arg.

```suggestion
int ParseVerbosity(const UniValue& arg, int default_verbosity);
```

🤔 instagibbs reviewed a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2343039535)
some last nits/suggestions

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