Repository navigation
crypto/blake2b: run long F calls in preemptible chunks - #2490
kamuikatsurgi wants to merge 2 commits into
Conversation
Port of ethereum/go-ethereum#35550. F calls with more than 4090 rounds run the assembly rounds in chunks, so the goroutine can be preempted between them. The output is unchanged.
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (85.18%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## upstream-merge-v1.17.4 #2490 +/- ##
==========================================================
- Coverage 56.62% 56.62% -0.01%
==========================================================
Files 944 944
Lines 175181 175206 +25
==========================================================
+ Hits 99194 99203 +9
- Misses 69997 70008 +11
- Partials 5990 5995 +5
... and 18 files with indirect coverage changes
🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
@claude review |
There was a problem hiding this comment.
🔵 Needs a closer look
Consensus-sensitive AVX2 assembly still requires mandatory native validation, and the tests can currently pass without exercising it.
1 open finding
What changed in this PR
Makes long BLAKE2b precompile calls preemptible while preserving consensus output.
Changes:
- Chunks long AVX2 compression calls into 4,090-round segments.
- Falls back to generic Go without AVX2.
- Adds differential and preemption tests.
| File | Description |
|---|---|
crypto/blake2b/blake2bAVX2_amd64.s |
Adds chunkable AVX2 round assembly. |
crypto/blake2b/blake2bAVX2_amd64.go |
Implements chunked long-call processing. |
crypto/blake2b/blake2b.go |
Selects the long-call path above the threshold. |
crypto/blake2b/blake2b_ref.go |
Adds the generic long-call fallback. |
crypto/blake2b/blake2b_generic.go |
Centralizes the schedule-cycle constant. |
crypto/blake2b/blake2b_f_test.go |
Adds correctness and preemption tests. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| func TestFChunkedMatchesGeneric(t *testing.T) { | ||
| r := rand.New(rand.NewSource(1)) |
Summary
Port of ethereum/go-ethereum#35550. On amd64 the blake2F precompile ran all of its rounds inside a single non-preemptible assembly call, with the round count taken from calldata.
Fnow hands calls with more than 4090 rounds tofLong, which keeps the working vector in memory and runs the rounds in chunks through a newfAVX2Roundsentry point, so the goroutine can be preempted between chunks. 4090 is a multiple of the ten-round message schedule. Machines without AVX2 use the generic Go implementation for those long calls. Tracked in POS-3760.Executed tests
go test ./crypto/blake2bon arm64, andGOARCH=amd64 go vet/ cross-build.fGenericacross chunk boundaries (TestFChunkedMatchesGeneric) and a preemption test (TestFLongRoundsIsPreemptible).Rollout notes
Consensus-sensitive code (the output must stay byte-identical), so it needs careful review and native amd64 validation before merge. No behaviour change is intended and no hardfork gate is needed. The upstream PR was still open and unreviewed when this was written.