💬 l0rinc commented on pull request "A few followups after introducing `/rest/blockpart/` endpoint":
(https://github.com/bitcoin/bitcoin/pull/34074#issuecomment-3662873909)
ACK 59b93f11e8600d5224359f4d05619c0f56aef1e6

[Since last ack](https://github.com/bitcoin/bitcoin/compare/823d4d8a5b71b7c740a6552f72fbfdb9b2813ca6..59b93f11e8600d5224359f4d05619c0f56aef1e6) the reason was added for status failure and non-json test was added for missing offset

💬 achow101 commented on issue "sqlite legacy descriptor wallet migration fails":
(https://github.com/bitcoin/bitcoin/issues/33468#issuecomment-3663281115)
> it doesn't let me :(

Open the dump file, change the `bdb` at the top to `sqlite` and do `createfromdump`. You'll get a checksum error that gives you the correct checksum. Copy that checksum, open the dump file again, go to the bottom and replace the checksum. `createfromdump` should now work.

💬 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`.