💬 glozow commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1784873140)
It is a bit funky that a negative verbosity turns into 0! I was kind of expecting a `JSONRPCError`. I don't mind and it looks like it's the same for `getblock`, so not asking for a change, but could be worth a test case?

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