💬 theuni commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2627980132)
I avoided lambdas as I've historically observed lots of optims being skipped (with clang, at least) when using them. But since you/@ajtowns/@l0rinc have all made the same comment, I'll play around and see if these indeed compile down to nothing as one would hope.

💬 sipa commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2627980586)
@theuni See my `std::make_index_sequence` based I demonstrated above. It's template based, but doesn't need recursion, and doesn't rely on compiler unrolling. It simply expands to an expression `(a[0] = v, a[1] = v, a[2] = v, ...)` e.g.

💬 sipa commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2627980657)
@theuni See my `std::make_index_sequence` based I demonstrated above. It's template based, but doesn't need recursion, and doesn't rely on compiler unrolling. It simply expands to an expression `(a[0] = v, a[1] = v, a[2] = v, ...)` e.g.

👍 ryanofsky approved a pull request: "qa: Avoid knock-on exception in assert_start_raises_init_error"
(https://github.com/bitcoin/bitcoin/pull/32929#pullrequestreview-3588864587)
Code review ACK 356883f0e48be59bcb154096cef82cbf3f0dd9d8. Thanks for the updates! Just rearranged commits and made minor changes in "missing init errors" test since last review

📝 vasild opened a pull request: "netif: fix compilation warning in QueryDefaultGatewayImpl()"
(https://github.com/bitcoin/bitcoin/pull/34093)
```
src/common/netif.cpp:137:51: error: comparison of integers of different signs: 'int64_t' (aka 'long') and 'unsigned long' [-Werror,-Wsign-compare]
137 | for (nlmsghdr* hdr = (nlmsghdr*)response; NLMSG_OK(hdr, recv_result); hdr = NLMSG_NEXT(hdr, recv_result)) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/netlink/netlink.h:220:31: note: expanded from macro 'NLMSG_OK'
220 | #define NLMSG_OK(_hdr, _len) NL_ITEM_OK(
...

💬 l0rinc commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2627999059)
https://www.agner.org/optimize/optimizing_cpp.pdf contains a few useful examples:
> The automatic vectorization works best if the following conditions are satisfied:
> 1. Use a compiler with good support for automatic vectorization, such as Gnu, Clang,
> or Intel.
> 2. Use the latest version of the compiler. The compilers are becoming better and better
> at vectorization.
>
> 3. If the arrays or structures are accessed through pointers or references then tell the
> compiler explicitly t
...

💬 theuni commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#issuecomment-3666489045)
An additional note about the actual vectorizing algorithm itself...

There's a different (and arguably more obvious) algorithm that's possible when calculating exactly 8 states. Rather than loading each `vec256` with partial info from 2 states, it's possible to use 16 `vec256` where each vector contains 1 element for each of the 8 states. It looks like:

```c++
vec256 x0, x1, x2, x3, x4, x5, x6, x7, x8, x9, x10, x11, x12, x13, x14, x15;

broadcast(x0, 0x61707865);
broadcast(x1, 0x3320646
...

💬 maflcko commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2628013042)
See https://github.com/bitcoin/bitcoin/pull/33436, but it was slow despite having the gui and the fuzz tests disabled. I am running it as part of nightly, so any issues will be caught before a release, but I am not sure if catching everything in pull requests is possible.

Maybe it is possible to split the task into two: One for a cross-compile, which should be faster than a "native" compile via qemu. And another to run the tests, similar to the windows-cross tests.

💬 maflcko commented on pull request "ci: run s390x job":
(https://github.com/bitcoin/bitcoin/pull/33436#issuecomment-3666498761)
Maybe by cross-compilation and only running the tests on s390x, it could be speed up?

💬 sipa commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2628034222)
It can be done with a helper function too, but that's not as concise:

```c++
template <size_t I, size_t... ITER>
ALWAYS_INLINE void arr_set_vec256_inner(std::array<vec256, I>& arr, const vec256& vec, std::index_sequence<ITER...>)
{
((std::get<ITER>(arr) = vec),...);
}

/** Store a vector in all array elements */
template <size_t I>
ALWAYS_INLINE void arr_set_vec256(std::array<vec256, I>& arr, const vec256& vec)
{
arr_set_vec256_inner(arr, vec, std::make_index_sequence<I>())
...

👍 ryanofsky approved a pull request: "miner: drop dummy extraNonce in coinbase scriptSig for templates requested via IPC"
(https://github.com/bitcoin/bitcoin/pull/32420#pullrequestreview-3588945400)
Code review ACK 9832a9aa2bffb5c0e14eb67ce22d9ce900106304. Just rebased since last review and tweaked comment

💬 l0rinc commented on pull request "ci: run s390x job":
(https://github.com/bitcoin/bitcoin/pull/33436#issuecomment-3666537730)
That sounds like a good idea, that's the most painful part.
Is cross compilation stable cross-endianness? I guess it has to be, has anyone tried it?

👍 ryanofsky approved a pull request: "mining: getCoinbase() returns struct instead of raw tx"
(https://github.com/bitcoin/bitcoin/pull/33819#pullrequestreview-3588964247)
Code review ACK 1bd6cae51fedeff62755e03ddcb9346b868f6d83. Just rebasing again since last review to avoid minor conflict

💬 maflcko commented on pull request "ci: Add IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#issuecomment-3666546750)
review ACK 73439910a02c2c07de94ad3a78ac3421ba7218d9 💣

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 73439910a02c
...

💬 maflcko commented on issue "GUI crashes in regtest":
(https://github.com/bitcoin-core/gui/issues/918#issuecomment-3666633855)
Is this really gui-specific? Does it not happen with `bitcoind`?

✅ maflcko closed a pull request: "Defer transaction signing until user clicks Send"
(https://github.com/bitcoin-core/gui/pull/915)

📝 maflcko reopened a pull request: "Defer transaction signing until user clicks Send"
(https://github.com/bitcoin-core/gui/pull/915)
Fixes [#30070](https://github.com/bitcoin/bitcoin/issues/30070)

When creating an unsigned PSBT from the GUI, the transaction was already signed during preparation, causing legacy inputs to have non-empty scriptSig fields. The PSBT parser then rejects them.

This defers signing until the user clicks "Send" instead of signing during preparation. Fee calculation still works since transactions can be created without signing.

Follows the approach suggested by @achow101 in the issue comments.

💬 maflcko commented on pull request "fuzz: doc: remove any mention to `address_deserialize_v2`":
(https://github.com/bitcoin/bitcoin/pull/34091#issuecomment-3666666963)
review ACK caf4843a59a9d2512d69f8fd88a9672112bd80ac 🎾

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK caf4843a59a9
...

💬 fjahr commented on pull request "p2p: Allow block downloads from peers without snapshot block after assumeutxo validation":
(https://github.com/bitcoin/bitcoin/pull/33604#issuecomment-3666699027)
Concept ACK

💬 fjahr commented on pull request "p2p: Allow block downloads from peers without snapshot block after assumeutxo validation":
(https://github.com/bitcoin/bitcoin/pull/33604#discussion_r2628258598)
nit: Could clarify the behavior in the comment here a bit more.

🤔 ryanofsky reviewed a pull request: "util: Add some more Unexpected and Expected methods"
(https://github.com/bitcoin/bitcoin/pull/34032#pullrequestreview-3589136730)
Code review fa874c9f050500dc521ef8d9c81b043687f0dcc7. Looks mostly good and thanks for considering the previous suggestions. One thing that I think may be a problem is use of throwing `value()` method in operator methods declared `noexcept` but I left a more detailed comment below.

Various changes were made since last review: renaming m_err, getting rid of Assume() in value(), testing void value() specialization, adding noexcepts, adding method `&` qualifiers, reformatting and using `std::get_i
...