perf: rust backed PTC sampling#9263
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces optimized, Rust-backed implementations for Payload Timeliness Committee (PTC) computations by integrating new functions from the @chainsafe/swap-or-not-shuffle package. The existing JS implementations have been renamed to "naive" versions and are now used for verification in new performance benchmarks. Feedback includes concerns regarding the use of a local tarball dependency which breaks cross-platform support and CI builds, a suggestion to use the assert utility for error handling consistency, and a request to remove debug logging from the test suite.
| "@chainsafe/pubkey-index-map": "^3.0.0", | ||
| "@chainsafe/ssz": "^1.4.0", | ||
| "@chainsafe/swap-or-not-shuffle": "^1.2.1", | ||
| "@chainsafe/swap-or-not-shuffle": "file:../../chainsafe-swap-or-not-shuffle-1.2.1-local.tgz", |
There was a problem hiding this comment.
The use of a local tarball dependency (file:../../...) is problematic for shared repositories as it breaks builds for other developers and CI environments that do not have the file at that specific relative path. Furthermore, the pnpm-lock.yaml changes indicate that several supported platforms (e.g., darwin-x64, linux-x64-gnu) have been removed and versions for native bindings have been downgraded to 0.0.2. This dependency should be properly published to a registry or integrated via workspace references to maintain repository integrity and cross-platform support.
| if (shuffling.length === 0) { | ||
| throw Error("Validator indices must not be empty"); | ||
| } |
There was a problem hiding this comment.
Use the assert utility for consistency with other validation checks in this file and the Lodestar codebase.
assert(shuffling.length > 0, "Validator indices must not be empty");| // eslint-disable-next-line no-console | ||
| console.log(`[vc=${vc}] effectiveBalanceIncrements[0]=${effectiveBalanceIncrements[0]}`); |
There was a problem hiding this comment.
Remove debug console.log and the associated eslint disable comment before merging.
| console.log(`[vc=${vc}] effectiveBalanceIncrements[0]=${effectiveBalanceIncrements[0]}`); | ||
|
|
||
| const naiveResult = naiveComputePayloadTimelinessCommitteesForEpoch( | ||
| cachedState, | ||
| epoch, | ||
| epochCtx.currentShuffling.committees, | ||
| effectiveBalanceIncrements | ||
| ); | ||
| const rustResult = computePayloadTimelinessCommitteesForEpoch( | ||
| cachedState, | ||
| epoch, | ||
| epochCtx.currentShuffling, | ||
| effectiveBalanceIncrements | ||
| ); | ||
| for (let i = 0; i < naiveResult.length; i++) { | ||
| const naive = naiveResult[i]; | ||
| const rust = rustResult[i]; | ||
| if (naive.length !== rust.length) { | ||
| throw new Error(`PTC length mismatch at slot ${i} (vc=${vc}): naive=${naive.length} rust=${rust.length}`); | ||
| } | ||
| for (let j = 0; j < naive.length; j++) { | ||
| if (naive[j] !== rust[j]) { | ||
| throw new Error( | ||
| `PTC index mismatch at slot ${i} position ${j} (vc=${vc}): naive=${naive[j]} rust=${rust[j]}` |
There was a problem hiding this comment.
this should be removed, added this just to check the values from rust and native values
1 participant
Motivation
#9013 and https://github.com/ChainSafe/lodestar/pull/9211/changes/BASE..27a20754340c6fe83a3822f3ba219ac39c3c1efa#r3074070771 highlight that computing ptc sampling is costly. T he sanity tests needed a 30s → 60s timeout bump. This PR moves the resource intensive into Rust
Description
Performance
AI Assistance Disclosure
Claude code was used for perf harness and wiring.