💬 2knwhzwkm6-max commented on pull request "contrib: Add zsh completion scripts":
(https://github.com/bitcoin/bitcoin/pull/33402#issuecomment-3663286905)
Tested PR #33402 zsh completions on macOS 26.1 (arm64), zsh 5.9 (arm64-apple-darwin25.0).
Bitcoin Core tools: bitcoin-cli v30.99.0-0f1f02995b71
PR commit: 5f2c0cdf07

Summary:
- Completion functions are discoverable via fpath/compinit (contrib/completions/zsh/).
- Command->completion bindings are set:
- bitcoin-cli -> _bitcoin-cli
- bitcoind -> _bitcoind
- bitcoin-tx -> _bitcoin-tx
- TAB key is bound to completion: "^I" expand-or-complete

🤔 ajtowns reviewed a pull request: "Add initial vectorized chacha20 implementation for 2-3x speedup"
(https://github.com/bitcoin/bitcoin/pull/34083#pullrequestreview-3585574031)
Some nits. Approach looks very nice, and at least going by the bench results, gives a good improvement.

💬 ajtowns commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2625334750)
`static_assert(sizeof(vec256) == 32)` ? (Or `size(vec256) == 32 || ALL_MULTI_STATES_DISABLED`)

💬 ajtowns commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2625248846)
Why separate this into an .ipp file if it's only included in a single .cpp file?

💬 ajtowns commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2625293528)
Using `#if !defined` for these seems old school. Why not static constexpr bools, and `if constexpr (..)`?

Writing:

```c++
#if defined(__x86_64__) || defined(__amd64__)
# define CHACHA20_VEC_DISABLE_STATES_16 true
# define CHACHA20_VEC_DISABLE_STATES_8 true
# define CHACHA20_VEC_DISABLE_STATES_6 true
# define CHACHA20_VEC_DISABLE_STATES_4 false
# define CHACHA20_VEC_DISABLE_STATES_2 false
#elif defined(__ARM_NEON)
...

static constexpr bool CHACHA20_VEC_ALL_MULTI_STATES_D
...

💬 ajtowns commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2625335880)
`if constexpr (ITER + 1 < I)` (we have a space after `if constexpr` elsewhere)

⚠️ BenWestgate opened an issue: "doc: Use multi-path descriptors in descriptors.md and linked tests."
(https://github.com/bitcoin/bitcoin/issues/34086)
### Motivation

These files should use multipath descriptors as per https://github.com/bitcoin/bitcoin/commit/2a46e94a1600a4f28e01db23a89f039acaa2c45e and https://github.com/bitcoin/bitcoin/pull/33286:

**Motivation:**
A single multipath descriptor is the most convenient pattern for multisig; our documentation should use it.

**Locations:**
- [ ] [doc/descriptors.md](https://github.com/bitcoin/bitcoin/tree/master/doc/descriptors.md):
- [ ] "Basic Multisig Example"
- [ ] "Examples" with this
...

💬 ajtowns commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2625986598)
Here's a branch that replaces the #defines and abstracts the recursive template stuff a bit more for your perusal https://github.com/ajtowns/bitcoin/commits/202512-pr34083-templates/

💬 l0rinc commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#issuecomment-3664158140)
Looking forward to reviewing it - quick question before I do: was it tested on big-endian systems?

💬 ajtowns commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2626127018)
My understanding is that you're inverting the logic here: instead of

```c++
#define QUARTERROUND(a,b,c,d) \
X(a,b,d,16) X(c,d,b,12) X(a,d,b,8) X(c,d,b,7)
```

This does the `X(a,b,d,16)` all four times via simd, then `X(c,d,b,12)` all four times, etc. And the simd part relies on the data for those four steps being exactly +4 units apart, which is why the shuffling and unshuffling is necessary for the second round. (Okay, not four times but eight times because each vec256 is two block
...

💬 ajtowns commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2626160716)
Overflow happens every 274GB I guess, which presumably isn't worth putting much effort in to optimising around.

💬 laanwj commented on pull request "test: sock: Enable socket pair tests on Windows":
(https://github.com/bitcoin/bitcoin/pull/33506#issuecomment-3664344101)
> Even though it's probably a bad idea to make changes in the present to suit a future use case that might never come about, I think it is generally useful to be able to create and connect the sockets separately in sock_tests.cpp and to be able to create TCP sockets like the application does.

f it's intentionally TCP then calling it `tcp_socket_pair` might be even better. So that no one decides to make this improvement in the meantime.

👍 hodlinator approved a pull request: "A few followups after introducing `/rest/blockpart/` endpoint"
(https://github.com/bitcoin/bitcoin/pull/34074#pullrequestreview-3586793647)
re-ACK 59b93f11e8600d5224359f4d05619c0f56aef1e6

Tested modifying the expected status codes in the functional test a bit to see how the reason string is printed.

💬 hodlinator commented on pull request "A few followups after introducing `/rest/blockpart/` endpoint":
(https://github.com/bitcoin/bitcoin/pull/34074#discussion_r2626269231)
Thanks for taking this!

Regarding including the `reason`, it seems fairly hard coded: "Bad Request", "OK", etc. So only useful to avoid having to know/look up the HTTP status codes.

📝 maflcko opened a pull request: "log: Use `__func__` for -logsourcelocations"
(https://github.com/bitcoin/bitcoin/pull/34088)
The `-logsourcelocations` option was recently changed to print the full function signature, as a side-effect of moving toward `std::source_location` internally.

This is fine, but at least for me, it makes debugging functional test failures harder, because the log is just so massively verbose, with questionable benefit.

I think the historically used file name, line number, and plain `__func__` name are more than sufficient for `-logsourcelocations`.

So switch back to using that.


For
...

💬 rustaceanrob commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#issuecomment-3664604170)
> found essentially no difference in the bitset encoding and using the run-length

This turned out to not be true. Using bit-packing as shown in the current state of the PR, the encoding was far worse. `0/1` encoding up to 928000 resulted in a hintsfile of 438Mb, whereas the run-lengths for height came in at 173Mb. I think 438Mb is not acceptable, so a hybrid approach is likely required. The construction of both files was around the same duration. I will do a more thorough analysis of chain hi
...

💬 rustaceanrob commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2626401193)
See comment, I will investigate this in the coming weeks.

🤔 hodlinator reviewed a pull request: "qa: Avoid knock-on exception in assert_start_raises_init_error"
(https://github.com/bitcoin/bitcoin/pull/32929#pullrequestreview-3586968227)
Latest push (6b0eac750f3aecb29446d70c039559143e43a011 -> 356883f0e48be59bcb154096cef82cbf3f0dd9d8) was largely motivated by https://github.com/bitcoin/bitcoin/pull/32929#pullrequestreview-3584856747 and contains changes:
* Moves the main fix commit first, and merges in the tiny doc-commit that was updating a comment in the same function.
* Renames `TestInitErrorTimeout` -> `TestMissingInitErrorTimeout` and clarifies log (https://github.com/bitcoin/bitcoin/pull/32929#discussion_r2624666464).
*
...

💬 hodlinator commented on pull request "qa: Avoid knock-on exception in assert_start_raises_init_error":
(https://github.com/bitcoin/bitcoin/pull/32929#discussion_r2626416752)
Good points, worth distinguishing it more from the pre-existing `TestInitErrorStartupFailure`.

💬 frankomosh commented on something "":
(https://github.com/bitcoin/bitcoin/commit/0190ac6622ff017aeead385289918d388a85218a#r173026481)
I think that in the case of `ImmediateTaskRunner`, the validation callbacks run synchronously rather than through the scheduler, which alters the ordering compared to production. Just curious if it would make sense to document that this execution mode is not production equivalent, so that fuzz coverage is interpreted accordingly?(and also for anyone who will use this as a refence in future)