-
Notifications
You must be signed in to change notification settings - Fork 511
feat: Change sampling algorithm to use feistel permutations #15938
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -143,10 +143,9 @@ library ValidatorSelectionLib { | |
return; | ||
} | ||
|
||
uint224 sampleSeed = getSampleSeed(_epochNumber); | ||
VerifyStack memory stack = VerifyStack({ | ||
proposerIndex: computeProposerIndex( | ||
_epochNumber, _slot, getSampleSeed(_epochNumber), targetCommitteeSize | ||
), | ||
proposerIndex: computeProposerIndex(_epochNumber, _slot, sampleSeed, targetCommitteeSize), | ||
needed: (targetCommitteeSize << 1) / 3 + 1, // targetCommitteeSize * 2 / 3 + 1, but cheaper | ||
index: 0, | ||
signaturesRecovered: 0, | ||
|
@@ -235,18 +234,30 @@ library ValidatorSelectionLib { | |
} | ||
|
||
Epoch epochNumber = _slot.epochFromSlot(); | ||
ValidatorSelectionStorage storage store = getStorage(); | ||
|
||
uint224 sampleSeed = getSampleSeed(epochNumber); | ||
(uint32 ts, uint256[] memory indices) = sampleValidatorsIndices(epochNumber, sampleSeed); | ||
uint256 committeeSize = indices.length; | ||
if (committeeSize == 0) { | ||
uint32 ts = epochToSampleTime(epochNumber); | ||
uint256 validatorSetSize = StakingLib.getAttesterCountAtTime(Timestamp.wrap(ts)); | ||
uint256 targetCommitteeSize = store.targetCommitteeSize; | ||
|
||
if (targetCommitteeSize == 0 || validatorSetSize < targetCommitteeSize) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
return (address(0), 0); | ||
} | ||
uint256 proposerIndex = computeProposerIndex(epochNumber, _slot, sampleSeed, committeeSize); | ||
return ( | ||
StakingLib.getAttesterFromIndexAtTime(indices[proposerIndex], Timestamp.wrap(ts)), | ||
proposerIndex | ||
|
||
// Compute which committee position is the proposer for this slot | ||
uint256 proposerIndex = | ||
computeProposerIndex(epochNumber, _slot, sampleSeed, targetCommitteeSize); | ||
|
||
// Get the validator index for that committee position in O(1) time | ||
uint256 validatorIndex = SampleLib.computeCommitteeMemberAtIndex( | ||
proposerIndex, targetCommitteeSize, validatorSetSize, sampleSeed | ||
); | ||
|
||
address proposer = StakingLib.getAttesterFromIndexAtTime(validatorIndex, Timestamp.wrap(ts)); | ||
setCachedProposer(_slot, proposer, proposerIndex); | ||
|
||
return (proposer, proposerIndex); | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,15 @@ contract Sampler { | |
{ | ||
return SampleLib.computeSampleIndex(_index, _indexCount, _seed); | ||
} | ||
|
||
function computeCommitteeMemberAtIndex( | ||
uint256 _index, | ||
uint256 _committeeSize, | ||
uint256 _indexCount, | ||
uint256 _seed | ||
) public pure returns (uint256) { | ||
return SampleLib.computeCommitteeMemberAtIndex(_index, _committeeSize, _indexCount, _seed); | ||
} | ||
} | ||
|
||
contract SamplingTest is Test { | ||
|
@@ -99,4 +108,62 @@ contract SamplingTest is Test { | |
// Test modulo 0 case | ||
assertEq(sampler.computeSampleIndex(_index, 0, _seed), 0); | ||
} | ||
|
||
function testConstantTimeCommitteeMember() public { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. test name is a little misleading I think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like what the test is doing though! |
||
uint256 committeeSize = 48; | ||
uint256 indexCount = 1000; | ||
uint256 seed = 12345; | ||
|
||
// Test that we get the same results as the original algorithm | ||
uint256[] memory fullCommittee = sampler.computeCommittee(committeeSize, indexCount, seed); | ||
|
||
for (uint256 i = 0; i < committeeSize; i++) { | ||
uint256 memberAtIndex = sampler.computeCommitteeMemberAtIndex(i, committeeSize, indexCount, seed); | ||
assertEq(memberAtIndex, fullCommittee[i], "Member mismatch at index"); | ||
} | ||
} | ||
|
||
function testConstantTimeNoDuplicates(uint8 _committeeSize, uint16 _indexCount, uint256 _seed) public { | ||
vm.assume(_committeeSize <= _indexCount); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
vm.assume(_committeeSize > 0 && _committeeSize <= 100); // Reasonable bounds for testing | ||
vm.assume(_indexCount > 0 && _indexCount <= 1000); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the index count should be closer to 20000 if it doesn't make it too slow. |
||
vm.assume(_seed != 0); | ||
|
||
// Get all members using constant-time function | ||
uint256[] memory members = new uint256[](_committeeSize); | ||
for (uint256 i = 0; i < _committeeSize; i++) { | ||
members[i] = sampler.computeCommitteeMemberAtIndex(i, _committeeSize, _indexCount, _seed); | ||
} | ||
|
||
// Check no duplicates | ||
for (uint256 i = 0; i < _committeeSize; i++) { | ||
for (uint256 j = i + 1; j < _committeeSize; j++) { | ||
assertNotEq(members[i], members[j], "Duplicate found"); | ||
} | ||
} | ||
} | ||
|
||
function testConstantTimeOutOfBounds() public { | ||
uint256 committeeSize = 10; | ||
uint256 indexCount = 100; | ||
|
||
vm.expectRevert( | ||
abi.encodeWithSelector( | ||
Errors.SampleLib__IndexOutOfBounds.selector, 10, committeeSize | ||
) | ||
); | ||
sampler.computeCommitteeMemberAtIndex(10, committeeSize, indexCount, 1234); | ||
} | ||
|
||
function testConstantTimeCommitteeTooLarge() public { | ||
uint256 committeeSize = 101; | ||
uint256 indexCount = 100; | ||
|
||
vm.expectRevert( | ||
abi.encodeWithSelector( | ||
Errors.SampleLib__SampleLargerThanIndex.selector, committeeSize, indexCount | ||
) | ||
); | ||
sampler.computeCommitteeMemberAtIndex(0, committeeSize, indexCount, 1234); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.