feat(shuffle): add PTC sampling - #563
Conversation
82ad017 to
929d3b8
Compare
11d1021 to
0ba4381
Compare
b3b1992 to
186ebc1
Compare
## Summary Adds a Zig `swap_or_not_shuffle` module and JS bindings for the shuffle APIs Lodestar uses, so ChainSafe/lodestar can drop `@chainsafe/swap-or-not-shuffle`. - Shares one shuffle implementation between `state_transition` and the JS binding. - Matches the upstream Rust/reference behavior for implemented paths. - Returns explicit errors for degenerate inputs that the reference package would crash or hang on. - Leaves async binding support for a future zapi release, since Lodestar does not use it. ## Testing - Ported upstream shuffle reference tests and existing lodestar-z shuffle vectors. - Added Zig coverage for validation order, error paths, proposer index, and sync committee selection. - Checked beta metrics against unstable; no performance regression found. ## Follow-ups - #563 adds PTC sampling on top of this. - Async shuffle bindings can be added later if a consumer appears. AI assistance was used for drafting and implementation support. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: bing <spiralladder@fastmail.com>
186ebc1 to
44852ad
Compare
44852ad to
2ca9b06
Compare
Performance Report✔️ no performance regression detected Full benchmark results
|
19ad4b2 to
8511dfc
Compare
| ptc_size: js.Number, | ||
| max_effective_balance_electra: js.Number, | ||
| effective_balance_increment: js.Number, | ||
| ) !js.Uint32Array { |
There was a problem hiding this comment.
Is that possible to use a *Into API with out arg, seems a little bit effective for these kinds of invocation
There was a problem hiding this comment.
The module already had an out-based inner function, so this was mostly deleting the allocation wrapper.
There was a problem hiding this comment.
I think grapebaba is requesting an additional function(s) to the binding, *Into variants?
Ports compute_ptc_indices and compute_ptc_indices_for_epoch (head 453b639b) into the swap_or_not_shuffle module with the same graceful error policy as the rest of the port, exposes them on the shuffle binding, and adds the upstream naive lodestar PTC sampler to the TS oracle for differential tests and benches. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
8511dfc to
848d561
Compare
| pub fn computePtcIndicesInto( | ||
| out: []u32, | ||
| seed: []const u8, | ||
| indices: []const u32, | ||
| effective_balance_increments: []const u16, | ||
| max_effective_balance_electra: i64, | ||
| effective_balance_increment: i64, |
There was a problem hiding this comment.
out should be the last param
| pub fn computePtcIndicesInto( | |
| out: []u32, | |
| seed: []const u8, | |
| indices: []const u32, | |
| effective_balance_increments: []const u16, | |
| max_effective_balance_electra: i64, | |
| effective_balance_increment: i64, | |
| pub fn computePtcIndicesInto( | |
| seed: []const u8, | |
| indices: []const u32, | |
| effective_balance_increments: []const u16, | |
| max_effective_balance_electra: i64, | |
| effective_balance_increment: i64, | |
| out: []u32, |
| indices: js.Uint32Array, | ||
| effective_balance_increments: js.Uint16Array, | ||
| ptc_size: js.Number, | ||
| max_effective_balance_electra: js.Number, |
There was a problem hiding this comment.
since this is known at comptime, can't we just not provide this as a param? Or are there reasons to provide this as a param
There was a problem hiding this comment.
Really good point, and points to some pain we will have to figure out.
Current lodestar-ts handles both mainnet and minimal without recompilation, rather relies on load-time selection of the preset.
Lodestar-z handles the preset at compile time. So we will have to figure out how we can bundle code for both presets or else deploy separate versions for each preset.
| ); | ||
| } | ||
|
|
||
| test "computePtcIndicesForEpochInto rejects invalid slot offsets" { |
There was a problem hiding this comment.
i kinda prefer we put all invalid cases into the same test like in test "computePtcIndicesInto rejects invalid inputs gracefully"
| var count: usize = 0; | ||
|
|
||
| var i: usize = 0; | ||
| outer: while (true) : (i += 16) { |
There was a problem hiding this comment.
i don't really like the while true here and think we should avoid this pattern as much as possible, can't we bound with count?
This makes an empty out a valid input without causing non-termination since an empty out would skip this entirely. with that said we should probably still check `out.len > 0
| outer: while (true) : (i += 16) { | |
| outer: while (count < out.len) : (i += 16) { |
Summary
Adds payload timeliness committee (PTC) sampling to the Zig
swap_or_not_shufflemodule and its JS binding, so ChainSafe/lodestar#9263 can stop computing it in JS.compute_ptc_indicesandcompute_ptc_indices_for_epoch, whose original implementation is feat: add computePtcIndices and computePtcIndicesForEpoch swap-or-not-shuffle#24 (453b639b).Testing
Follow-ups
AI assistance was used for drafting and implementation support.