💬 zkfrio commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1656989393)
Concept ACK

There are no answers for questions that do not make sense. Also there are no arguments left against full RBF as it's used regularly.

💬 luke-jr commented on pull request "validation: Flush state after initial sync":
(https://github.com/bitcoin/bitcoin/pull/15218#discussion_r1278476394)
nit: There used to be a blank line before this. IMO it was nicer.

💬 luke-jr commented on pull request "validation: Flush state after initial sync":
(https://github.com/bitcoin/bitcoin/pull/15218#discussion_r1278476346)
Why was the order here swapped? Seems like override should be the very last keyword IMO

💬 luke-jr commented on pull request "validation: Flush state after initial sync":
(https://github.com/bitcoin/bitcoin/pull/15218#discussion_r1278476957)
Do we want to monitor only the active chainstate? Or should we be paying attention to the background one too?

💬 luke-jr commented on pull request "validation: Flush state after initial sync":
(https://github.com/bitcoin/bitcoin/pull/15218#discussion_r1278477065)
`-1` is always going to be `!= current_height` anyway...?

💬 andrewtoth commented on pull request "validation: Flush state after initial sync":
(https://github.com/bitcoin/bitcoin/pull/15218#discussion_r1278481260)
That's a good question.
Please correct me if my assumptions are incorrect, but I believe the other chainstate would be the background syncing while our active is the assumeutxo chainstate which we want to sync quickly and then have active ASAP. In that case, there are only two chainstates, active and background. The background chainstate can take a long time to sync, possibly >24h, which would mean the periodic flush would take effect. So syncing that here would not really be beneficial. For th
...

💬 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