💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1267230477)
In commit "Move CheckBlockIndex() from Chainstate to ChainstateManager" (f921887c1afdcd333cf71ca25e5efbf4991f806e)

I think it would be a little better to use `GetAll` instead of `ActiveChain()` here. All the chains should have the same genesis block.

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1267226100)
In commit "Documentation improvements for assumeutxo" (bef5acee93e128857d20137ea0c83f281a780211)

"it has" should be "it may have"

💬 MarcoFalke commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1640912856)
The `git grep`-based linters obviously assume some kind of formatting, but I think this is fine. Otherwise we'd have to rewrite the locale linter to clang-tidy, because someone could write `std :: to_string` to trick the linter? (Same for the other linters, such as the assertions or includes linter). Maybe for critical stuff we could consider adding both, one using `git grep` and another using clang-tidy, but I don't think we have any "critical" linters yet.

💬 mzumsande commented on pull request "init: return error when block index is non-contiguous, fix feature_init.py file perturbation":
(https://github.com/bitcoin/bitcoin/pull/27823#discussion_r1267265463)
That is true, but the specific case of a missing (or incorrect) genesis block index should get caught in an extra check right after, [see here](https://github.com/bitcoin/bitcoin/blob/c6a338b67e8e7848e6f42329a8b0bf3add16ad51/src/node/chainstate.cpp#L69C1-L74).

There are other cases that wouldn't be caught with the proposed solution: We could have the valid chain and also some extra block indices that don't connect.
A stricter check would therefore be to require each block index of height `x
...

💬 mzumsande commented on pull request "init: return error when block index is non-contiguous, fix feature_init.py file perturbation":
(https://github.com/bitcoin/bitcoin/pull/27823#issuecomment-1640940299)
[9d33681 ](https://github.com/bitcoin/bitcoin/commit/9d3368141b7e782a578efa765899103b29c814f3)to [d27b9a2](https://github.com/bitcoin/bitcoin/commit/d27b9a2248476439ddab7700327f074005a810d5):
Added an explanation why the `tweaked_contents` window of `feature_init.py` was changed: Since the genesis block is not checked by the `-checkblocks` verification, the perturbation window must be chosen such that a higher block in blk*.dat is affected.

👍 luke-jr approved a pull request: "rpc: doc: Added `longpollid` and `data` params to `template_request` #27998"
(https://github.com/bitcoin/bitcoin/pull/28056#pullrequestreview-1535856487)
utACK

💬 luke-jr commented on pull request "rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1640950634)
(Maybe remove # from PR title)

💬 kevkevinpal commented on pull request "init: adding check for : for -torcontrol flag":
(https://github.com/bitcoin/bitcoin/pull/28018#issuecomment-1640951956)
>The way this is currently documented, both host and port should be required. However, it does align with many of our other options to have a default port, and if that makes sense here too then the help for torcontrol arg should be updated to show port as optional as a first step.

I think it makes sense to make the port optional if that is also what is being done for the other options.

> Whilst this is technically true for low-level networking (code), IMO it's extremely un-common to not
...

🤔 pinheadmz reviewed a pull request: "Relay own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/27509#pullrequestreview-1535806985)
code review ACK bde055d28f5d41d9e923f770d130010e72441e91

Read through each commit and checked build at each commit. Left a few questions / comments below. I am going to spend more time examining the test coverage and playing with the feature on signet in the next few days.

Really excellent work! Commit layout makes this branch easy and legible to review. It's a great idea and I am also very excited about an actual working socks5 proxy in the test suite ;-)

💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1267253028)
Would it make any sense here to manually set `peer.m_ping_queued = true` to guarantee (not "maybe") sending ping?

💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1267244372)
IIUC this line schedules a replacement sensitive connection because the one we were passed in has the wrong services and so it gets dumped. I think this is the only line in this function that makes it apply strictly to only sensitive relays. So I wonder if that should be asserted at the top? Or could be renamed to `PushUnbroadcastTxToSensistiveRelay()`? At the very least maybe sensitive-only should be explained in the function description on L933 ?

💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1267277048)
Does this guy need an `else`? Maybe raise error that a connection request was made after all destinations have been used?

💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1267279962)
Why did you need this to support wtxid? Is that how we are verifying our TX made the round trip out and back (without being malleated)?

💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1267248349)
wow, is this really the fastest way to get a random element from `std::set` ? funny.

💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1267276546)
I don't see `.node` used below and I don't think you really use `.requested_to_addr` anywhere after setting it below? Could this just be an array of `CAddress` or something like that?

💬 stickies-v commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1267282655)
nit: not sure the snake case is worth the added LoC (+further down). Would prefer to keep as is.

💬 stickies-v commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1267277566)
nit: no need to specify where it's used for a public method imo
```suggestion
* Whether this object is a privacy network.
```

💬 stickies-v commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1267281474)
nit: any reason the order of these lines got switched? if not necessary, wouldn't do that

💬 stickies-v commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1267279181)
Absolutely, thanks. Just left some small comments.

💬 jonatack commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1267286727)
I think it's important to provide the context here.