💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1267187714)
In commit "Fix initialization of setBlockIndexCandidates when working with multiple chainstates" (f41cef2ba1e1f033356691e520e39d9d75d5eb4f)

HAVE_DATA flag doesn't actualy seem to be removed here. Should it be? And since the test is already passing withtout this would the reason just removing HAVE_DATA flag be to satisfy CheckBlockIndex checks?

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1267213310)
In commit "Fix initialization of setBlockIndexCandidates when working with multiple chainstates" (f41cef2ba1e1f033356691e520e39d9d75d5eb4f)

I'm actually confused about how this is working because the HAVE_DATA flag doesn't seem to be removed above.

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1267209493)
In commit "Fix initialization of setBlockIndexCandidates when working with multiple chainstates" (f41cef2ba1e1f033356691e520e39d9d75d5eb4f)

Command at the top of the test saying it checks cs2 "contains all blocks, even those assumed-valid" is out of date. Should be "contains all blocks except those assumed-valid, because they don't have data"

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1267204896)
In commit "Fix initialization of setBlockIndexCandidates when working with multiple chainstates" (f41cef2ba1e1f033356691e520e39d9d75d5eb4f)

This is true, but it seems like it might make the test less confusing and more realistic if we just added a ` cs2.m_chain.SetTip(* assumed_base)` call below the `cs1.m_chain.SetTip(*validated_tip);` call. I think if this was done the cs2.setBlockIndexCandidates would just contain blocks after the snapshot.

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

"below the snapshot height" should be "at the snapshot height and below"

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