💬 andrewtoth commented on pull request "validation: Flush state after initial sync":
(https://github.com/bitcoin/bitcoin/pull/15218#issuecomment-1657004059)
@luke-jr thank you for the speedy review! Will address your comments, but will wait for others to review as well.
@Sjors @MarcoFalke rebased, apologies for the delay.

💬 andrewtoth commented on pull request "rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block":
(https://github.com/bitcoin/bitcoin/pull/26415#issuecomment-1657004182)
@TheCharlatan @RandyMcMillan rebased.

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1278482371)
Right! Better to ensure explicitly than rely on the internals of `MaybeSendPing()`, the clock and the fact this is the first call to `MaybeSendPing()` for this peer.

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1278482946)
Both `.node` and `.requested_to_addr` members are used by the test - `.node` is used to check/query what messages `bitcoind` has sent to this peer and `.requested_to_addr` is used to check that the connection was made to an `.onion` address (and not e.g. an IPv4 address).

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1278493071)
I didn't do that because my idea was to fall back to the old behavior of just closing the connection after the first `N` connections have been redirected (all destinations have been used). But you are right, I added a warning message now to ease diagnosing test behavior if that happens. Didn't `raise` because there is no error from the point of view of `socks5.py`.

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1278505682)
Just for convenience to be able to check if this is an unbroadcast transaction from `PeerManagerImpl::AlreadyHaveTx()` which has the id as `GenTxid`.

`CTxMemPool::m_unbroadcast_txids` tracks transactions by `txid` (not `wtxid`). This is relevant: https://github.com/bitcoin/bitcoin/pull/18044#discussion_r460495254.

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1278506090)
Right! I added `-cjdnsreachable`. This changed the test from:

```
TestFramework (DEBUG): Could not add [fc32:17ea:e415:c3bf:9808:149d:b5a2:c9aa] to node0's addrman (collision?)
```

to

```
TestFramework (DEBUG): Added [fc32:17ea:e415:c3bf:9808:149d:b5a2:c9aa] to node0's addrman
```

Thanks!

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1278506979)
I can't find an options struct in `PeerManager` or in `PeerManagerImpl`. Did you mean `struct CConnman::Options`? `gArgs` is used in some other places in `PeerManagerImpl` too, so I assumed it is ok.

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1278507242)
I guess an alternative would be to have a new method `PeerManagerImpl::PushMessage()` which does this check and then calls `CConnman::PushMessage()`. And replace all calls to `m_connman.PushMessage()` (there are 53 of them) with `PushMessage()`. Would that be desirable?

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1657040602)
`bde055d28f...96ea776776`: address suggestions

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1657043863)
`96ea776776...7a3206dc8e`: rebase due to conflicts

💬 amitiuttarwar commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#issuecomment-1657054147)
tested ACK 69abfd3db1

all my feedback is very optional, code looks fine as is. I saw the previous review comments & update around wording suggestions & don't think it's worth much churn just for that, but left my language thoughts just for consideration.

agreed that it makes most sense to return just the totals here, and that generall the `IsTerrible` info is an internal implementation detail that would ideally not be exposed to the users.

I considered the approach of the RPC endpoint e
...

💬 amitiuttarwar commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1278512304)
```suggestion
{RPCResult::Type::NUM, "new", "number of addresses in the new table, which represent potential peers."},
```

the fact that they are periodically tested seems like an implementation detail of addrman and feels a little confusing here.

💬 amitiuttarwar commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1278512805)
could also check that the values for other networks are 0

💬 amitiuttarwar commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1278512102)
```suggestion
{RPCResult::Type::NUM, "total", "total number of addresses in both new/tried tables"},
```

having the clause "from a network" only on total field (vs also on new and tried) is confusing. a bit redundant anyways since these fields are all nested within the networks

💬 amitiuttarwar commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1278512493)
```suggestion
{RPCResult::Type::NUM, "tried", "number of addresses in the tried table, which represent peers with whom the node has successfully connected to in the past."},
```

if you take the previous suggestion, I'd update this too for consistency

💬 amitiuttarwar commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1278516272)
reading the chatGPT suggestion, if you want more detail, I think it'd make more sense to use the part that says they are recently discovered but have not yet been successfully connected to.

💬 amitiuttarwar commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-1657055010)
I think this PR should be in draft right now since it builds on top of #27511 & has some outdated changes / the two commits are represented as a "merge" commit here instead of separately. I would either put it into draft or keep it up-to-date in a form that would be ready for master assuming sufficient review.

💬 MarcoFalke commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1278521260)
It may be good to enumerate why this is needed (if at all).

💬 MarcoFalke commented on pull request "qa, doc: Fix comment":
(https://github.com/bitcoin/bitcoin/pull/28181#discussion_r1278522512)
```suggestion
Check that there all python files in this directory are categorized as a test script or meta script.
```

💬 YellowRoseCx commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1657075910)
Replace by fee makes double spending easier and harm's Bitcoin's ability to be used as a currency in my opinion