perf: benchmark infrastructure, O(N²)→O(N) ephemeral key optimisation, and CI regression gate - #3953
Open
piotr-roslaniec wants to merge 13 commits into
Open
perf: benchmark infrastructure, O(N²)→O(N) ephemeral key optimisation, and CI regression gate#3953piotr-roslaniec wants to merge 13 commits into
piotr-roslaniec wants to merge 13 commits into
Conversation
piotr-roslaniec
force-pushed
the
perf/benchmarks
branch
from
May 6, 2026 14:34
b1f9da6 to
4927cac
Compare
Collaborator
Author
|
Re-triggering CI |
lionakhnazarov
left a comment
lionakhnazarov
left a comment
Collaborator
There was a problem hiding this comment.
added some small comments to address. LGTM overall
| // EnablePprof exposes Go runtime profiling endpoints at /debug/pprof/ on | ||
| // the clientinfo port. Requires Port != 0. Never expose to untrusted | ||
| // networks; bind behind a firewall or restrict with an SSH tunnel. | ||
| EnablePprof bool |
Collaborator
There was a problem hiding this comment.
EnablePprof doesn’t turn pprof on or off in the server. It only decides whether you log “pprof enabled”, not whether pprof URLs work.
What actually attaches /debug/pprof/… is the blank import _ "net/http/pprof". That runs when the package loads and registers handlers on http.DefaultServeMux every time—no runtime check.
In Initialize, when cfg.EnablePprof is true we only print a log line (logger.Infof("pprof profiling endpoints enabled…")). When it’s false, you skip that line—but the import already ran, so the routes are still registered on the default mux.
| echo "Total coverage: ${TOTAL}%" | ||
| PASS=$(awk -v t="$TOTAL" 'BEGIN { print (t+0 >= 14) ? "yes" : "no" }') | ||
| if [ "$PASS" != "yes" ]; then | ||
| echo "::error::Coverage ${TOTAL}% is below the 14% minimum threshold" |
Collaborator
There was a problem hiding this comment.
14% total is quite a loose check; lots of untested code can slip in if the average stays barely above it. Was it intentionally?
| cat bench.txt | ||
|
|
||
| - name: Install benchstat | ||
| run: go install golang.org/x/perf/cmd/benchstat@latest |
Collaborator
There was a problem hiding this comment.
go install golang.org/x/perf/cmd/benchstat@latest always pulls whatever is current on that repo’s default branch today. Tomorrow you might get a new release that:
changes how tables are formatted (columns, spacing, wording),
adds/removes summary lines,
tweaks how deltas are expressed.
So CI that passed yesterday might fail tomorrow without any Go code change, or the opposite: a real regression might stop matching your script’s expectations.
Pinning (@v0.0.0-… commit hash or a tagged version if they tag) means everyone gets the same benchstat binary until you bump it on purpose.
…chmarks
Phase 0 -- infrastructure:
- Add `make bench` target (count=10, benchmem, -run='^$')
- Add `client-bench` CI job that runs on main pushes and uploads
bench-*.txt as `go-bench` artifact (no gate yet -- baselines needed first)
Phase 1 -- quick-win benchmarks across six packages:
- pkg/bls: BenchmarkSign, BenchmarkVerify, BenchmarkAggregateBLS (N=10/50/100),
BenchmarkThresholdVerify (51-of-100, production beacon config)
- pkg/altbn128: BenchmarkCompressG1, BenchmarkDecompressG1,
BenchmarkCompressDecompressRoundTripG1/G2
- pkg/tecdsa/signing: BenchmarkMarshalEphemeralPublicKeyMessage,
BenchmarkUnmarshalEphemeralPublicKeyMessage, BenchmarkMarshalSigningShareMessage,
BenchmarkUnmarshalSigningShareMessage, BenchmarkRoundTripEphemeralKey
- pkg/tecdsa/dkg: BenchmarkMarshalEphemeralPublicKeyMessage,
BenchmarkUnmarshalEphemeralPublicKeyMessage, BenchmarkRoundTripDKGMessage
- pkg/net/retransmission: BenchmarkBackoffStrategyTick, BenchmarkStandardStrategyTick;
also add TestBackoffStrategy_TickSequence (200-tick correctness test, pins the
exact fire sequence [1,3,6,11,20,37,70,135] so schedule drift is caught early)
- pkg/tbtc: BenchmarkGetRecentWindows_{100,1000}Windows,
BenchmarkGetSummary_{100,1000}Windows,
BenchmarkCleanupOldWindows_1000Windows (isolates the O(n^2) sort);
also add TestCleanupOldWindows_BoundsMapSize (2000-window insert, asserts cap
enforcement to guard against unbounded memory growth)
…itcoin sighash libp2p (pkg/net/libp2p/channel_test.go): - BenchmarkChannelDeliver_SingleHandler/10Handlers: measures lock+snapshot overhead when all handler channels are full (default branch dominates after first messageHandlerThrottle iterations) - BenchmarkProcessPubsubMessage: raw processPubsubMessage throughput with empty pubsub message, early-returns after proto.Unmarshal on missing unmarshaler Bitcoin (pkg/bitcoin/transaction_builder_test.go): - BenchmarkComputeSignatureHashes_1/5/20Input: measures BIP143 sighash computation scaling across input counts; builder reused across b.N iterations (ComputeSignatureHashes is non-mutating)
- Add EnablePprof bool to clientinfo.Config; when true, registers /debug/pprof/* handlers on http.DefaultServeMux before the HTTP server starts, making profiles available on the existing clientinfo port - Change Initialize(ctx, port int) to Initialize(ctx, cfg Config) so the single call site in cmd/start.go can pass the full config struct; this avoids growing the Initialize parameter list for future Config fields - Add docs/profiling.md covering: security warning (all-interface binding), enable instructions, standard pprof commands, benchmark+profile workflow, and benchstat comparison workflow
net/http/pprof init() registers all /debug/pprof/* routes on DefaultServeMux when the package is imported. The prior explicit http.HandleFunc calls in the EnablePprof branch would have panicked with 'http: multiple registrations for /debug/pprof/'. Switch to blank import (idiomatic Go) so init() handles registration exactly once. The EnablePprof flag now gates the log message only; the handlers are always compiled in when Port != 0 because DefaultServeMux is used. True runtime gating would require a dedicated debug port.
… benchmarks retransmission: BenchmarkStandardStrategyTick was measuring 0 ns/op because the compiler eliminated the noop closure. Add a call counter as a sink -- benchmark now measures 1.7 ns/op (counter increment + comparison), which is a real signal. tecdsa/dkg, tecdsa/signing: existing BenchmarkMarshal/UnmarshalEphemeralPublicKeyMessage used 2 keys; production group size is 100 (99 peers per participant). Add _100Keys variants using a buildEphemeralKeyMap helper. Unmarshal result: 3.9 ms per message (vs 74 µs for 2 keys), revealing btcec.ParsePubKey × 99 as the dominant cost -- ~386 ms per participant per DKG/signing key exchange.
…/gjkr Matches the pattern added to pkg/tecdsa/dkg and pkg/tecdsa/signing. Beacon group size is 64, so the _64Keys variants use 63 peer keys per message. Results: unmarshal with 2 keys=76µs, with 63 keys=2.7ms -- 36x gap confirms btcec.ParsePubKey×N dominates, same as in tECDSA. Baseline now covers all three protocols that use EphemeralPublicKeyMessage.
Store ephemeral public keys as raw bytes in the wire message structs instead of parsed *ephemeral.PublicKey values. EC point decompression (btcec.ParsePubKey, ~37 µs each) is now deferred until generateSymmetricKeys picks the single key addressed to this member, so only 1 parse per message instead of N-1. Benchmark impact at group size N=100 (99 peers): UnmarshalEphemeralPublicKeyMessage_100Keys: 3.9 ms → 396 µs (~10×) Per-round key exchange at N=100: ~386 ms → ~43 ms (~9×) The signing package receives identical treatment; gjkr is excluded because its accusation path (findPublicKey) returns *ephemeral.PublicKey to 6+ call sites and would require a larger cascading refactor.
On each push to main, download the previous go-bench artifact, run benchmarks, then compare with benchstat. Regressions >20% that are statistically significant (no ~) fail the job and print the offending benchmarks. The 20% threshold filters out noise; lower the value once baseline variance is established. Changes: - Add actions/setup-go for benchstat installation on the runner - Use dawidd6/action-download-artifact to fetch the previous run's data - Standardise output file to bench.txt (overwrite: true on upload) - Python one-liner parses benchstat output and gates on delta > 20%
…or path Verify that corrupt (non-parseable) EC point bytes in ephemeralPublicKeys are rejected at generateSymmetricKeys time with a meaningful error. The existing TestGenerateSymmetricKeys_InvalidEphemeralPublicKeyMessage only covers missing keys. The new tests cover the complementary case introduced by the O(N²)→O(N) optimisation: a key is present in the map but contains garbage bytes, so isValidEphemeralPublicKeyMessage passes while the ephemeral.UnmarshalPublicKey call during ECDH returns an error. Only the victim member (whose key in the sender's map was corrupted) sees the error; other members are unaffected.
The 55% threshold was an overestimate; measured total coverage across ./... is 14.4%. Lower the floor to 14% to reflect the real baseline and prevent the gate from blocking the PR.
Replace stale download_artifacts with get_artifacts (the actual target) and add missing mainnet and local phony targets.
piotr-roslaniec
force-pushed
the
perf/benchmarks
branch
from
August 7, 2026 13:35
d7a0d11 to
fdbd264
Compare
📝 WalkthroughWalkthroughThe PR adds benchmark workloads and CI gates, enables optional pprof profiling, documents profiling procedures, and changes DKG and signing ephemeral public keys to use serialized bytes with deferred parsing before ECDH. ChangesBenchmarking and profiling
Serialized ephemeral public keys
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DKGSigningProtocol
participant EphemeralKeyMessage
participant ephemeral.UnmarshalPublicKey
participant ECDH
DKGSigningProtocol->>EphemeralKeyMessage: serialize outgoing public key bytes
DKGSigningProtocol->>EphemeralKeyMessage: select recipient-specific bytes
DKGSigningProtocol->>ephemeral.UnmarshalPublicKey: parse selected bytes
ephemeral.UnmarshalPublicKey->>ECDH: provide parsed public key
ECDH-->>DKGSigningProtocol: derive symmetric key
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/net/libp2p/channel_test.go (1)
620-632: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSeparate successful delivery from full-buffer delivery.
After
messageHandlerThrottlecalls, each handler channel is full. Nearly all later iterations take the non-blocking drop branch. The reported results do not represent successful single-handler delivery or ten-handler fan-out.Drain the handler channels or use active consumers for successful-delivery benchmarks. Keep a separate benchmark for the full-buffer drop path.
Also applies to: 638-655
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/net/libp2p/channel_test.go` around lines 620 - 632, Update the BenchmarkChannelDeliver_SingleHandler and related multi-handler benchmarks so handler channels are drained or actively consumed, measuring successful delivery rather than predominantly full-buffer drops. Preserve the existing full-buffer behavior in a separate benchmark that explicitly measures the non-blocking drop path.pkg/bls/bls_test.go (1)
237-244: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAlign each benchmark name with its timed operation.
Lines 240-241 aggregate inputs during every iteration. This does not isolate aggregate signature verification. Move aggregation before
b.ResetTimer, or rename the benchmark to show that it measures aggregation plus verification.Line 278 recovers a signature but does not call
VerifyG1. Rename this benchmark toBenchmarkRecoverSignature_51Of100, or add verification to the timed loop.Also applies to: 276-279
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/bls/bls_test.go` around lines 237 - 244, Align the BLS benchmark names with their timed work: in the benchmark around AggregateG1Points, AggregateG2Points, and VerifyG1, move aggregation before b.ResetTimer or rename it to indicate aggregation plus verification; in the recovery benchmark around RecoverSignature, either add VerifyG1 to the timed loop or rename it to BenchmarkRecoverSignature_51Of100.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/profiling.md`:
- Around line 98-105: Update the profiling instructions around the Baseline and
Candidate commands to run the baseline from the main revision and the candidate
from the working revision, using separate worktrees or explicit revision
switching. Ensure working-tree changes are restored only when a stash actually
exists, and avoid relying on git stash alone to change revisions.
---
Nitpick comments:
In `@pkg/bls/bls_test.go`:
- Around line 237-244: Align the BLS benchmark names with their timed work: in
the benchmark around AggregateG1Points, AggregateG2Points, and VerifyG1, move
aggregation before b.ResetTimer or rename it to indicate aggregation plus
verification; in the recovery benchmark around RecoverSignature, either add
VerifyG1 to the timed loop or rename it to BenchmarkRecoverSignature_51Of100.
In `@pkg/net/libp2p/channel_test.go`:
- Around line 620-632: Update the BenchmarkChannelDeliver_SingleHandler and
related multi-handler benchmarks so handler channels are drained or actively
consumed, measuring successful delivery rather than predominantly full-buffer
drops. Preserve the existing full-buffer behavior in a separate benchmark that
explicitly measures the non-blocking drop path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f391425-1dd9-4956-a264-18781b74cf82
📒 Files selected for processing (22)
.github/workflows/client.ymlMakefilecmd/start.godocs/profiling.mdpkg/altbn128/altbn128_test.gopkg/beacon/gjkr/marshaling_test.gopkg/bitcoin/transaction_builder_test.gopkg/bls/bls_test.gopkg/clientinfo/clientinfo.gopkg/net/libp2p/channel_test.gopkg/net/retransmission/strategy_test.gopkg/tbtc/coordination_window_metrics_test.gopkg/tecdsa/dkg/marshaling.gopkg/tecdsa/dkg/marshaling_test.gopkg/tecdsa/dkg/message.gopkg/tecdsa/dkg/protocol.gopkg/tecdsa/dkg/protocol_test.gopkg/tecdsa/signing/marshaling.gopkg/tecdsa/signing/marshaling_test.gopkg/tecdsa/signing/message.gopkg/tecdsa/signing/protocol.gopkg/tecdsa/signing/protocol_test.go
Comment on lines
+98
to
+105
| ```sh | ||
| # Baseline (main branch) | ||
| git stash | ||
| go test ./pkg/... -run=^$ -bench=. -count=6 | tee /tmp/baseline.txt | ||
|
|
||
| # Candidate (your branch) | ||
| git stash pop | ||
| go test ./pkg/... -run=^$ -bench=. -count=6 | tee /tmp/candidate.txt |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Run the baseline benchmark on the main revision.
git stash preserves working-tree changes but does not change commits. This sequence can run both benchmarks on the current revision instead of comparing main with the candidate. On a clean worktree, git stash pop also fails because no stash was created. Use separate worktrees or explicitly switch revisions, and restore a stash only when one exists.
Example using an isolated baseline worktree
# Baseline (main branch)
-git stash
-go test ./pkg/... -run=^$ -bench=. -count=6 | tee /tmp/baseline.txt
+git worktree add /tmp/keep-core-main main
+(cd /tmp/keep-core-main && \
+ go test ./pkg/... -run=^$ -bench=. -count=6) | tee /tmp/baseline.txt
+git worktree remove /tmp/keep-core-main
# Candidate (your branch)
-git stash pop
go test ./pkg/... -run=^$ -bench=. -count=6 | tee /tmp/candidate.txt📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ```sh | |
| # Baseline (main branch) | |
| git stash | |
| go test ./pkg/... -run=^$ -bench=. -count=6 | tee /tmp/baseline.txt | |
| # Candidate (your branch) | |
| git stash pop | |
| go test ./pkg/... -run=^$ -bench=. -count=6 | tee /tmp/candidate.txt | |
| # Baseline (main branch) | |
| git worktree add /tmp/keep-core-main main | |
| (cd /tmp/keep-core-main && \ | |
| go test ./pkg/... -run=^$ -bench=. -count=6) | tee /tmp/baseline.txt | |
| git worktree remove /tmp/keep-core-main | |
| # Candidate (your branch) | |
| go test ./pkg/... -run=^$ -bench=. -count=6 | tee /tmp/candidate.txt |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/profiling.md` around lines 98 - 105, Update the profiling instructions
around the Baseline and Candidate commands to run the baseline from the main
revision and the candidate from the working revision, using separate worktrees
or explicit revision switching. Ensure working-tree changes are restored only
when a stash actually exists, and avoid relying on git stash alone to change
revisions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
Notes
Summary by CodeRabbit