#34083 Add initial vectorized chacha20 implementation for 2-3x speedup
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #34083 Add initial vectorized chacha20 implementation for 2-3x speedup | Waiting on author | 0 | Blocked | 434 | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
This pull request adds a vectorized ChaCha20 implementation using compiler vector extensions rather than hand-written assembly or architecture-specific intrinsics. It processes multiple 64-byte blocks in parallel (calculating 1 to 4 states concurrently on x86-64 and up to 15 states on AArch64) when encrypting or decrypting data streams. Scalar ChaCha20 remains in place as a fallback for unsupported architectures and tail bytes.
Problem: ChaCha20 encryption consumes a substantial share of CPU time on the node's network thread when using BIP324 encrypted v2 transport, creating bottlenecks during high-bandwidth peer-to-peer activity such as initial block download.
Evidence of importance:
- PR description: 'Local profiling revealed that chacha20 accounts for a substantial amount of the network thread's time.'
- PR description and commit e3386b9d0d: 'Demonstrates a 2x speedup on x86-64 and 3x for arm+neon.'
Scope: The implementation relies on GCC and Clang __vector_size__ extensions and __builtin_shufflevector rather than explicit SIMD intrinsics or assembly. It uses recursive inline template helpers to force loop unrolling without pragmas.
Risk of a mistake: medium (p2p). A computational error or state desync in ChaCha20 would corrupt encrypted BIP324 network traffic, disconnecting peers and disrupting node connectivity.
Changed since the previous assessment: Includes a squash commit fixing namespaces and circular dependencies, with Lőrinc Pap now credited as co-author.
Reviewability
Paused · Waiting on author
Open objections from l0rinc, sedited with no author reply for 63 days; review would repeat what has been asked.
Waiting on (from the thread read):
- author: addressing CI dependency cycle, 64-byte performance regression, IBD slowdown on Umbrel, test/fuzz coverage, and restructuring review feedback 2026-08-30
- author: address l0rinc's review feedback on CI cycle, 64-byte/IBD regressions, and test/fuzz coverage 2026-08-30
Agreement
Blocked
objection that needs a change, nobody has replied (l0rinc)
Thread notes: Reread under schema 2. Split multi-harm review r:5059109621 into distinct claims (64-byte performance regression, Umbrel IBD slowdown, and test/bench/fuzzing coverage request). Recorded suggestions and questions raised across the thread. / Updated claims from schema 1 re-read: split multiple harms in r:5059109621 into distinct claims (64-byte slowdown, Umbrel IBD slowdown, and test/fuzz suggestions), updated c:5375710529 clears_with to change due to breaking CI linting, and captured suggestions
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| l0rinc (member) | question, correctness | Was this tested on big-endian systems that do not fall back to scalar code? | answered · author clarified big-endian support was best-effort and untested | answered | yes | 2025-12-17: 'quick question before I do: was it tested on any big-endian systems which don't revert to non-vectorized run?' Status from: 2025-12-17: theuni: 'I forgot to mention in the PR description that big-endian was best-effort and untested.' |
| l0rinc (member) | objection, correctness | ChaCha20 produced incorrect output on big-endian platforms | fixed · fix pushed · author pushed big-endian fix suggested by l0rinc | changed | yes | 2025-12-17: 'My biggest objection currently is that it's broken on big-endian systems - left a suggestion how to reproduce and fix it.' Status from: 2026-01-13: theuni: 'Also pushed @l0rinc's fix for big-endian.' |
| ajtowns (member) | question, maintenance | Why separate this into an .ipp file if it is only included in a single .cpp file? | answered · author explained that future runtime-detected implementations require separate compilation flags | answered | yes | 2025-12-17: 'Why separate this into an .ipp file if it's only included in a single .cpp file?' Status from: 2025-12-17: theuni: 'some impls may require different compilation flags (-mavx2/-mavx512vl), which have to be in their own compilation units.' |
| ajtowns (member) | suggestion, approach | Use static constexpr bools and if constexpr instead of preprocessor #if !defined blocks | answered · author agreed if constexpr makes sense where possible | answered | yes | 2025-12-17: 'Using `#if !defined` for these seems old school. Why not static constexpr bools, and `if constexpr (..)`?' Status from: 2025-12-17: theuni: 'if constexpr certainly makes more sense where possible. I'll have a look, thanks!' |
| ajtowns (member) | suggestion, safety | Add a static_assert checking sizeof(vec256) == 32 | contested · author declined explaining accesses use operator[], reviewer clarified it protects against compilers ignoring vector attributes | answered | yes | 2025-12-17: 'static_assert(sizeof(vec256) == 32) ?' Status from: 2025-12-18: ajtowns: 'Sorry, I meant just so you get an error if you're using a compiler ignores the __attribute__ entirely' |
| ajtowns (member) | suggestion, maintenance | Add explanatory comments detailing the inverted quarterround SIMD round logic | answered · author agreed to add context in comments | answered | yes | 2025-12-17: 'Could use a little more explanation in the comment I think? But makes sense to me.' Status from: 2025-12-18: theuni: 'Sure, I can definitely add some more context' |
| l0rinc (member) | suggestion, approach | Use existing internal_bswap_32 helpers rather than __builtin_bswap32 | answered · author agreed to use internal_bswap_32 | answered | yes | 2025-12-17: 'can we assume that all big endian systems have `__builtin_bswap32` available? https://github.com/bitcoin/bitcoin/blob/432b18ca8d0654318a8d882b28b20af2cb2d2e5d/src/compat/byteswap.h#L14-L16 indicates we could use our existing helpers here instead' Status from: 2025-12-17: theuni: 'Thanks, yes, I meant to change that to internal_bswap_32 before pushing. Will do.' |
| l0rinc (member) | suggestion, correctness | Extend test vectors to cover and compare the new vector specializations | open | answered | no | 2025-12-17: 'It seems to me the existing standard test vectors are too short (mostly < 128 bytes) to trigger the vectorized optimization - can we extend the tests to runa and compare the new specializations' |
| l0rinc (member) | suggestion, approach | Use simple fixed-bound runtime loops instead of recursive templates for unrolling | answered · author benchmarked and showed runtime loops and helper abstractions caused severe regressions due to missing inlining and unrolling | answered | yes | 2025-12-17: 'Hmmm, it seems to me we're doing heavy recursive templates here to unroll loops... Can you please try if this also works and results in the same speedup?' Status from: 2025-12-17: theuni: 'Ok, after some experimenting, I am very hesitant to add the helpers as suggested above.' |
| l0rinc (member) | suggestion, maintenance | Use ALWAYS_INLINE defined in attributes.h instead of redefining it locally | contested · author agreed initially but reviewer noted later in rc:3887632159 that it remains locally redefined | answered | yes | 2025-12-17: 'Could we use https://github.com/bitcoin/bitcoin/blob/e16c22fe025f82166c7f3f15a37c96bf4a06e4cf/src/attributes.h#L19-L25 instead?' Status from: 2026-08-29: l0rinc: 'Is there a specific reason for redefining ALWAYS_INLINE here?' |
| sipa (member) | suggestion, approach | Use std::index_sequence compile-time loops instead of recursive templates | answered · author noted they will experiment with index_sequence unrolling | answered | yes | 2025-12-17: 'I believe it's possible to use compile-time loops here instead of recursive templates (if runtime loops don't get optimized sufficiently)' Status from: 2025-12-18: theuni: 'The index_sequence trick is neat. If that ends up looking cleaner/more obvious than the recursive iteration, that works for me. I'll play around with it.' |
| ajtowns (member) | suggestion, maintenance | Add comments documenting compiler versions and architectures that failed to inline lambdas efficiently | answered · author agreed to add more context | answered | yes | 2025-12-18: 'Could we get some comments in the code as to compiler versions (and architecture) that failed to be efficient enough with lambdas' Status from: 2025-12-18: theuni: 'Sure, I can definitely add some more context and clean up the wonky variable names.' |
| sedited (member) | suggestion, maintenance | Add a short description explaining that the crypt function peels off larger blocks first before finishing smaller blocks | open | answered | no | 2025-12-24: 'I think this function should get a short description too. Wasn't immediately clear to me that it attempts to peel off larger blocks first before potentially finishing with a few smaller blocks.' |
| sedited (member) | question, approach | Is there a particular reason for avoiding the existing approach? | open | answered | no | 2025-12-24: 'Is there a particular reason for the aversion to the current approach? It seems easier to read and probably also to debug to me.' |
| sedited (member) | question, approach | Why load multiple words across two blocks into SIMD vectors rather than holding the same word across multiple blocks? | open | answered | no | 2025-12-28: 'Intuitively I would have expected that the vectors hold the same word spread over multiple blocks. But if I understand your approach here, we have multiple words over two blocks. Is it just easier to express the quarter rounds in this way?' |
| l0rinc (member) | objection, approach | The generic vectorized implementation is roughly 2x slower than scalar on ARM64 when compiled with GCC | fixed · fix pushed · author refactored to a 128-bit vector implementation to work around GCC vectorizer limitation | changed | yes | 2026-05-04: 'RPi5 with GCC shows a regression: the generic vectorized implementation is ~2x slower than scalar on ARM64 with GCC, while Clang handles it fine.' Status from: 2026-08-18: theuni: 'The second introduces a pluggable vector interface, and adds a 128bit implementation. This works around gcc's vectorizer' |
| l0rinc (member) | objection, maintenance | A circular include dependency between chacha20_vec and chacha20_vec_128impl fails CI dependency linting | open | changed | no | 2026-08-21: 'Note that the CI indicates a dependency-cycle regression was introduced here.' |
| l0rinc (member) | suggestion, safety | Guard feature macros before using them and restrict targets to x86_64 and aarch64 | open | answered | no | 2026-08-29: 'Now that we're relying on `__builtin_shufflevector` after the GCC codegen issues (and since we only enable this on `x86_64`, `amd64`, and `aarch64`), should we guard the feature macros before using them?' |
| l0rinc (member) | suggestion, approach | Pass state as a fixed-extent span, dispatch vector groups at the start of ChaCha20Aligned::Crypt, and let the existing scalar loop handle remainders | open | answered | no | 2026-08-29: 'Could we pass the state as a fixed-extent span, dispatch vector groups at the start of `ChaCha20Aligned::Crypt`, and let its existing scalar loop process the remainder?' |
| l0rinc (member) | suggestion, maintenance | Extract target loops into named helpers and use a switch statement for exact group counts | open | answered | no | 2026-08-29: 'Could we move each target loop into a named helper, express exact group counts with `switch`, and leave the entry point only to select one constexpr target?' |
| l0rinc (member) | suggestion, maintenance | Add benchmarks for boundary sizes (128, 192, 768-960 bytes) and BIP324 sizes (318-320 bytes) | open | answered | no | 2026-08-29: 'Instead of documenting how each platform behaved... could we cover those four boundary sizes?' |
| l0rinc (member) | suggestion, approach | Expose the backend's minimum block count to skip constructing vector state on short calls that fall back to scalar | open | answered | no | 2026-08-29: 'The AArch64 backend handles 4 or more blocks, yet 1-3-block `Crypt` calls still construct its vector state before returning to scalar code. Could we expose the backend's minimum block count and skip shorter calls consistently' |
| l0rinc (member) | objection, approach | Corecheck measures the 64-byte vector case about 7% slower than scalar code | open | changed | no | 2026-08-30: 'Corecheck currently has no coverage data for the new code and measures the 64-byte case about 7% slower.' |
| l0rinc (member) | objection, approach | IBD with V2 transport was considerably slower with this PR on Umbrel compared to master | open | changed | no | 2026-08-30: 'master V1 and V2 were basically the same, while this PR was considerably slower there' |
| l0rinc (member) | suggestion, correctness | Cover all dispatch block counts with unit tests, isolated benchmarks, and extended differential fuzzing | open | answered | no | 2026-08-30: 'To be more confident in the correctness, I would like to see the new path covered by: tests... focused benchmarks... extension of the existing crypto_diff_fuzz_chacha20 target' |
| l0rinc (member) | suggestion, correctness | Derive input size in crypto_diff_fuzz_chacha20 from blocks [0, 100] plus tail bytes [0, 63] to test every vector group | open | answered | no | 2026-08-30: 'Could we derive the input size from named `blocks` in `[0, 100]` plus `tail_bytes` in `[0, BLOCKLEN - 1]`?' |
| l0rinc (member) | question, approach | Is 15 blocks faster than 8+7 on Apple Silicon? | open | answered | no | 2026-08-30: 'Is 15 also faster than e.g. 8+7? On my M4 Max, processing 14 and 15 blocks as 8+6 and 8+7 is faster than using exact 14- and 15-state groups.' |
| l0rinc (member) | suggestion, approach | Expose target minimums of two blocks on x86-64 and three on AArch64, leaving shorter calls scalar | open | answered | no | 2026-08-30: 'Corecheck measures the one-block vector path slower, while CryptStates<3> improved the 192-byte median from 0.98 to 0.86 ns/byte on this Apple M4 Max. Could we expose target minimums of two blocks on x86-64 and three on AArch64, and leave shorter calls scalar?' |
| l0rinc (member) | suggestion, maintenance | Rename the TU to chacha20_vec_dispatch.cpp to resolve the CI dependency lint cycle | open | answered | no | 2026-08-30: 'Could we rename this TU to `chacha20_vec_dispatch.cpp`, include `chacha20_vec.h` from the implementation header, and keep one definition of `BLOCKLEN` and `STATE_WORDS`?' |
Support:
- ajtowns (member) (2025-12-17) [read: src/crypto/chacha20.cpp, src/crypto/chacha20_vec.ipp, src/crypto/chacha20_vec_base.cpp, src/crypto/chacha_vec_impl.h]: Approach looks very nice, and at least going by the bench results, gives a good improvement.
- jonatack (member) (Concept ACK, 2026-08-20): good work. [not substantive]
Participants:
- ajtowns (member, support, 14 statements 2025-12-17 to 2025-12-24): supported the approach and speedup while suggesting simplifications and template unrolling
- l0rinc (member, objection, 45 statements 2025-12-17 to 2026-08-30): found big-endian bug and GCC ARM64 regression, flagged CI cycle, Corecheck regression, and missing tests
- maflcko (member, neutral, 2 statements 2025-12-17 to 2025-12-18): discussed CI emulation runs on big-endian architectures
- sipa (member, neutral, 4 statements 2025-12-17 to 2025-12-17): suggested std::index_sequence compile-time unrolling
- sedited (member, question, 3 statements 2025-12-24 to 2025-12-28): asked clarifying questions about SIMD design and suggested a function description
- fanquake (member, neutral, 1 statements 2026-08-14 to 2026-08-14): inquired about rebasing the 128-bit branch
- jonatack (member, support, 1 statements 2026-08-20 to 2026-08-20): Concept ACK
Dependencies
Enables:
- Follow-up PRs adding runtime-detected AVX2 and AVX-512 ChaCha20 optimizations
Files
0 lines under test/bench/ci.
- src/crypto/chacha20_vec_128impl.h +281/-0
- src/crypto/chacha20_vec.cpp +90/-0
- src/crypto/chacha20.cpp +33/-5
- src/crypto/chacha20_vec.h +29/-0
- src/crypto/CMakeLists.txt +1/-0
Card
The short text the ranking pass compares across PRs.
This pull request introduces a vectorized ChaCha20 implementation using GCC and Clang vector built-ins to process multiple 64-byte cipher blocks simultaneously on x86-64 and AArch64. The change addresses high CPU consumption on the network thread during BIP324 encrypted v2 P2P transport, which strains nodes during initial block download and block relay. The author reports a 2x speedup on x86-64 and 3x on ARM NEON based on local benchmarks. This work is marked as part 1 of a series intended to later enable runtime-detected AVX2 and AVX-512 optimizations.
Data
dossier JSON · extract JSON · model ledger, generated 2026-10-13T01:25, confidence high, input hash d00808d41db42767
thread read through 2026-08-30 (91 statements), head 8364a78e14
labels: none · draft
thread read through 2026-08-30 (91 statements), head 8364a78e14
labels: none · draft
Categories
Utilities (logging, arguments, libraries)
P2 · speedup
Vectorizes ChaCha20 to address CPU bottlenecks on the network thread during BIP324 transport, targeting a 2-3x speedup across x86-64 and ARM NEON. The description notes that 'chacha20 accounts for a substantial amount of the network thread's time' and provides 'a 2x speedup on x86-64 and 3x for arm+neon', making this an important performance improvement for core cryptographic primitives.
Membership: Modifies and adds cryptographic primitives under src/crypto/ (chacha20).
Factors: security/stability 1, bug 0, performance 2, user value 1, leverage 1