Skip to content

Conversation

@JonathanOppenheimer
Copy link
Member

@JonathanOppenheimer JonathanOppenheimer commented Dec 19, 2025

Why this should be merged

This PR eliminates redundant BLS public key conversions throughout the validator codebase, which is a somewhat significant performance increase.

This is measured in the following benchmark:

go test -bench=. -benchmem ./vms/platformvm/warp | grep "BenchmarkGetCanonicalValidatorSet"

A summary of results:

Size Metric Unoptimized Optimized Improvement
100 validators Time/op 1.75ms 1.04ms 41% faster
100 validators Memory 219KB 159KB 27% less
100 validators Allocs 2,624 1,975 25% fewer
1,000 validators Time/op 17.4ms 12.3ms 29% faster
1,000 validators Memory 2.15MB 1.58MB 27% less
1,000 validators Allocs 26,232 19,749 25% fewer
10,000 validators Time/op 182ms 130ms 29% faster
10,000 validators Memory 21.5MB 15.8MB 27% less
10,000 validators Allocs 262,323 197,489 25% fewer

How this works

For prior context see

Closes #3902

  1. Warp struct

    • Removed redundant PublicKey field from Warp struct, keeping only PublicKeyBytes
    • Updated FlattenValidatorSet to use PublicKeyBytes directly
  2. GetValidatorOutput

    • Added PublicKeyBytes field to store uncompressed bytes (96 bytes)
    • Populated in key locations: ApplyValidatorPublicKeyDiffs, gRPC client, validator set Map() function
    • This avoids the pointer -> bytes conversions

There's no breaking changes in this PR, as the cache fields aren't serialized, so no concerns there either.

How this was tested

CI, the BenchmarkGetCanonicalValidatorSet

Need to be documented in RELEASES.md?

No

@JonathanOppenheimer JonathanOppenheimer self-assigned this Dec 22, 2025
@JonathanOppenheimer JonathanOppenheimer added the enhancement New feature or request label Dec 22, 2025
@JonathanOppenheimer JonathanOppenheimer moved this to Ready 🚦 in avalanchego Dec 22, 2025
@JonathanOppenheimer JonathanOppenheimer marked this pull request as ready for review December 22, 2025 16:16
@JonathanOppenheimer JonathanOppenheimer requested review from a team as code owners December 22, 2025 16:16
@JonathanOppenheimer
Copy link
Member Author

JonathanOppenheimer commented Dec 22, 2025

@yacovm the eager initialization did indeed tank the performance:

BenchmarkFlattenValidatorSet (Validator Set Creation)

Size Master (ns/op) Optimized (ns/op) Diff % Master (B/op) Optimized (B/op) Master (allocs/op) Optimized (allocs/op)
1 304.2 386.9 +27.2% 288 384 5 6
2 459.6 625.0 +36.0% 576 768 9 11
4 783.8 1111 +41.7% 1152 1536 17 21
8 1367 2089 +52.8% 2304 3072 33 41
16 2972 4350 +46.4% 5544 7080 68 84
32 5909 8745 +48.0% 11048 14120 132 164
64 12103 17868 +47.6% 21928 28072 260 324
128 25477 37114 +45.7% 43560 55849 516 644
256 53187 76320 +43.5% 87593 112170 1028 1284
512 116610 155932 +33.7% 175532 224684 2052 2564
1024 241062 333267 +38.2% 350808 449116 4102 5126

BenchmarkGetCanonicalValidatorSet (Full Validator Set Retrieval)

Size Master (ns/op) Optimized (ns/op) Diff % Master (B/op) Optimized (B/op) Master (allocs/op) Optimized (allocs/op)
0 97.27 98.02 +0.8% 0 0 0 0
1 321.0 408.4 +27.2% 288 384 5 6
10 2186 3073 +40.6% 3336 4296 44 54
100 22286 30736 +37.9% 32393 41993 404 504
1000 252871 340886 +34.8% 342809 438812 4006 5006
10000 2939377 3850643 +31.0% 3318845 4278849 40035 50036

However maybe we are okay with these tradeoffs, as we do

  • More upfront work and memory during validator set creation
  • For faster, simpler access during signature operations

@JonathanOppenheimer
Copy link
Member Author

I can also revert to 4f55cf3 for the original performance gains. I guess the larger question is does IRL usage mimic the benchmark or nah.

@JonathanOppenheimer
Copy link
Member Author

JonathanOppenheimer commented Dec 22, 2025

I added a new benchmark BenchmarkAggregatePublicKeys to give some visibility into the trade offs with choices in various scenarios. I compared the changes on 3de36f8 (which is my first attempt at optimization which leads to performance increases on the other two benchmarks) to the current tip of this branch, which is post Yacov's suggestions, which more closely resembles master without lazy initialization.

Size Pre-Optimization (3de36f8)ns/op Optimized (Current)ns/op Improvement Pre-OptB/op OptimizedB/op Pre-Optallocs/op Optimizedallocs/op
1 922.6 916.5 -0.7% 576 576 10 10
10 12,855 12,662 -1.5% 3,528 3,528 37 37
100 63,620 63,993 +0.6% 5,626 5,625 49 49
1,000 547,311 536,762 -1.9% 12,967 12,923 49 49
10,000 5,226,422 5,230,765 +0.1% 90,824 86,652 92 49

@JonathanOppenheimer
Copy link
Member Author

Performance Results (ns/op)

Size Baseline (4f55cf3)PublicKeyBytes + Lazy After First Opt (3de36f8)No PublicKeyBytes + Lazy Final (Current)No PublicKeyBytes + Eager Baseline→First First→Final Total Improvement
1 901.2 922.6 916.5 -2.4% +0.7% -1.7%
10 13,063 12,855 12,662 +1.6% +1.5% +3.1%
100 63,658 63,620 63,993 +0.1% -0.6% -0.5%
1,000 545,207 547,311 536,762 -0.4% +1.9% +1.5%
10,000 5,083,804 5,226,422 5,230,765 -2.8% -0.1% -2.9%

@JonathanOppenheimer
Copy link
Member Author

My vote: revert all changes to 4f55cf3, but keep the new benchmark.

@yacovm
Copy link
Contributor

yacovm commented Dec 22, 2025

We should optimize the transaction validation path, so let's revert my idea 👍

@JonathanOppenheimer
Copy link
Member Author

We should optimize the transaction validation path, so let's revert my idea 👍

Reverted!

require.Equal(test.want.TotalWeight, got.TotalWeight)
require.Len(got.Validators, len(test.want.Validators))
for i, wantVdr := range test.want.Validators {
gotVdr := got.Validators[i]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do this too:

				gotVdr.PublicKey = nil
				wantVdr.PublicKey = nil
				require.Equal(wantVdr, gotVdr)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2025-12-31 at 11 25 35 AM

We have to set the bytes directly, which will fail the test.

func (v L1Validator) effectivePublicKey() *bls.PublicKey {
if v.IsActive() {
return bls.PublicKeyFromValidUncompressedBytes(v.PublicKey)
if !v.IsActive() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not that it's important but why the order change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It just matches the early return pattern others have stressed.

@joshua-kim
Copy link
Contributor

Which conventional commits spec are we using? Under some this isn't a feat and this is considered perf

@JonathanOppenheimer JonathanOppenheimer changed the title feat: Optimize BLS key usage perf: Optimize BLS key usage Jan 6, 2026
@JonathanOppenheimer
Copy link
Member Author

Which conventional commits spec are we using? Under some this isn't a feat and this is considered perf

you're right - it should be perf. I just started doing it a couple months ago, and have been using these:

types other than fix: and feat: are allowed, for example @commitlint/config-conventional (based on the Angular convention) recommends build:, chore:, ci:, docs:, style:, refactor:, perf:, test:, and others.

Copy link
Contributor

@alarso16 alarso16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EVM code looks fine, but I would check for more mistakes like the one I noticed

Copy link
Contributor

@alarso16 alarso16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been corrected, seems fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Status: Ready 🚦

Development

Successfully merging this pull request may close these issues.

Optimize BLS key usage

5 participants