💬 sipa commented on pull request "Reduce wasted pseudorandom bytes in ChaCha20 + various improvements":
(https://github.com/bitcoin/bitcoin/pull/26153#discussion_r1267165897)
Done in #28100.

💬 jonatack commented on pull request "crypto: ChaCha20 `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1267196895)
In commit 9e1199c7ce5

```
random.cpp:668:44: error: non-type template argument is not a constant expression
static constexpr std::array<std::byte, rng.KEYLEN> ZERO{};
^~~~~~~~~~
random.cpp:668:44: note: implicit use of 'this' pointer is only allowed within the evaluation of a call to a 'constexpr' member function
1 error generated.
```

```
$ clang --version
Homebrew clang version 16.0.6
Target: arm64-apple-darwin22.5.0
Thread model:
...

💬 sipa commented on pull request "crypto: ChaCha20 `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1267203493)
Strange that GCC doesn't complain about this. Fixed.

👍 ryanofsky approved a pull request: "Rework validation logic for assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/27746#pullrequestreview-1535703245)
Code review ACK 137762f34a845e491b80f9cea07efc4427cb38bf. Left some more suggestions, but this looks good in its current form.

Just so I don't forget, I want to note that that it might make sense to move `IsInitialBlockDownload` and `NotifyHeaderTip` functions from the `Chainstate` class to `ChainstateManager` as a followups to this PR (https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1246868905, https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1237552792)

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

Note: the reason why all entries were added previously was because `pindex->nHeight < first_assumed_valid_height` check was always true, because `first_assumed_valid_height` was `std::numeric_limits<int>::max()`

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

It would be good to remove the reliesOnAssumedValid() function since it is no longer used after this.

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