feat: Change sampling algorithm to use feistel permutations#15938
feat: Change sampling algorithm to use feistel permutations#15938spalladino wants to merge 1 commit intopalla/delayed-sig-verificationfrom
Conversation
spalladino
requested review from
LHerskind,
Maddiaa0 and
just-mitch
as code owners
| setOverrideValue(sampledIndices[i], 0); | ||
| } | ||
| // Use a Feistel network to create a permutation of [0, _indexCount) | ||
| uint256 permutedIndex = feistelPermute(_index, _indexCount, _seed); |
There was a problem hiding this comment.
Seems like we should be using the do/while structure here that's done in the batch function instead of calling feistelPermute and doing redundant setup.
| } | ||
|
|
||
| function testConstantTimeNoDuplicates(uint8 _committeeSize, uint16 _indexCount, uint256 _seed) public { | ||
| vm.assume(_committeeSize <= _indexCount); |
There was a problem hiding this comment.
bound would be better here.
| assertEq(sampler.computeSampleIndex(_index, 0, _seed), 0); | ||
| } | ||
|
|
||
| function testConstantTimeCommitteeMember() public { |
There was a problem hiding this comment.
test name is a little misleading I think?
There was a problem hiding this comment.
I like what the test is doing though!
| function testConstantTimeNoDuplicates(uint8 _committeeSize, uint16 _indexCount, uint256 _seed) public { | ||
| vm.assume(_committeeSize <= _indexCount); | ||
| vm.assume(_committeeSize > 0 && _committeeSize <= 100); // Reasonable bounds for testing | ||
| vm.assume(_indexCount > 0 && _indexCount <= 1000); |
There was a problem hiding this comment.
I think the index count should be closer to 20000 if it doesn't make it too slow.
| uint256 validatorSetSize = StakingLib.getAttesterCountAtTime(Timestamp.wrap(ts)); | ||
| uint256 targetCommitteeSize = store.targetCommitteeSize; | ||
|
|
||
| if (targetCommitteeSize == 0 || validatorSetSize < targetCommitteeSize) { |
There was a problem hiding this comment.
Hm. This is actually a breaking change. Currently nodes expect getProposer/getCommittee to revert if the current set size is not large enough.
Unless we have a reason to change it, I think we should keep the current behavior.
just-mitch
left a comment
just-mitch
left a comment
There was a problem hiding this comment.
Nice! Curious how you prompted claude to this? I have some minor nits in the tests, an optimization, and a bigger one around reverting when the validator set is not large enough.
I asked it to look into SampleLib, and figure out a way to compute the sampling index of a single committee member without having to recompute all others. It started by keeping the same algorithm, but exiting early when it got to the requested index, so I prompted it to do it in constant time. Then it suggested feistel, and went all in. I was pleasantly surprised. |
|
Closing in favor of sending signers in calldata as discussed |
2 participants
Feistel permutations remove the need for auxiliary storage for tracking replacements, and most importantly, allow us to compute a given index permutation in constant time, which allows us to get the proposer for a given committee without having to compute all indices.
Props to Claude for the idea and implementation.
Builds on #15813
Median propose cost is
306000.