Test sig checker #102
Test sig checker #102
Conversation
| // if the operator was a part of the quorum and the quorum is a part of the provided quorumNumbers | ||
| if (nonSignerQuorumBitmap >> j & (BitmapUtils.bytesArrayToBitmap(quorumNumbers) >> j & 1) == 1) { | ||
| checkSignaturesIndices.nonSignerStakeIndices[i][stakeIndexIndex] = stakeRegistry.getStakeUpdateIndexForOperatorIdForQuorumAtBlockNumber( | ||
| if (nonSignerQuorumBitmap >> uint8(quorumNumbers[quorumNumberIndex]) & 1 == 1) { |
There was a problem hiding this comment.
going to suggest parentheses for clarity here. I realize they're unnecessary but IMO it's easier to verify it's right if we write operators like this (multiple bitwise ops chained together) as:
if ((nonSignerQuorumBitmap >> uint8(quorumNumbers[quorumNumberIndex])) & 1 == 1) {
| // if (i != 0) { | ||
| // require(uint256(nonSignerPubkeyHashes[i]) > uint256(nonSignerPubkeyHashes[i - 1]), "BLSSignatureChecker.checkSignatures: nonSignerPubkeys not sorted"); | ||
| // } |
There was a problem hiding this comment.
is this just a temporary change to deal with ordering being hard?
IIRC this check was key for ensuring no duplicates (we should add a comment to this effect as well)
There was a problem hiding this comment.
I actually think this is fine. if you subtract the same public key twice, then the amount of stake that has confirmed a fact will only be underestimated
There was a problem hiding this comment.
I actually think this is fine. if you subtract the same public key twice, then the amount of stake that has confirmed a fact will only be underestimated
hmm OK I guess. I suppose you wouldn't have a valid signature for the resultant public key anyway, right (in the case where you subtract a signature twice)? Just want to be very sure before deleting anything that was intended as some kind of safety/correctness check.
|
Medium-level design question: what happens when none of the non-signers is part of one of the quorums? is the answer that we just don't optimize for this and can still handle it? because that is my guess based on review so far |
| function recordOperatorQuorumBitmapUpdate(bytes32 operatorId, uint192 quorumBitmap) external { | ||
| uint256 operatorQuorumBitmapHistoryLength = _operatorIdToQuorumBitmapHistory[operatorId].length; | ||
| if (operatorQuorumBitmapHistoryLength != 0) { | ||
| _operatorIdToQuorumBitmapHistory[operatorId][operatorQuorumBitmapHistoryLength - 1].nextUpdateBlockNumber = uint32(block.number); | ||
| } | ||
|
|
||
| _operatorIdToQuorumBitmapHistory[operatorId].push(QuorumBitmapUpdate({ | ||
| updateBlockNumber: uint32(block.number), | ||
| nextUpdateBlockNumber: 0, | ||
| quorumBitmap: quorumBitmap | ||
| })); | ||
| } |
There was a problem hiding this comment.
should there be a corresponding internal function in the BLSRegistryCoordinatorWithIndices? If there is already, why not use it? Am a bit concerned about any possibility for divergence between harnessed + non-harnessed versions
| assertEq(checkSignaturesIndices.nonSignerQuorumBitmapIndices.length, 0); | ||
| assertEq(checkSignaturesIndices.quorumApkIndices.length, allInclusiveQuorumNumbers.length); | ||
| assertEq(checkSignaturesIndices.totalStakeIndices.length, allInclusiveQuorumNumbers.length); | ||
| assertEq(checkSignaturesIndices.nonSignerStakeIndices.length, 0); | ||
| assertEq(checkSignaturesIndices.nonSignerStakeIndices.length, allInclusiveQuorumNumbers.length); |
There was a problem hiding this comment.
could you add some revert messages to these? also helps document exactly what it's checking (although these seem ~fairly clear)
|
|
||
| // 0 nonSigners: 159908 | ||
| // 1 nonSigner: 178683 | ||
| // 2 nonSigners: 197410 |
There was a problem hiding this comment.
can we check anything here other than the call not reverting? it's a 'view' function so no state transition to check, but maybe we can check something about the returned values (looks like we're just tossing those out right now)
There was a problem hiding this comment.
looks like the commit you linked just puts the returned values into memory and then immediately dumps them? this is still short of checking anything about the returned values
| // set the sigma to a different value | ||
| nonSignerStakesAndSignature.sigma.X++; | ||
|
|
||
| cheats.expectRevert(); | ||
| blsSignatureChecker.checkSignatures( | ||
| msgHash, | ||
| quorumNumbers, | ||
| referenceBlockNumber, | ||
| nonSignerStakesAndSignature | ||
| ); | ||
| } |
There was a problem hiding this comment.
should this have a specific revert message or does this end up in some low-level call failure (like a failure in the precompile call?) ?
There was a problem hiding this comment.
precompile call fails rip
There was a problem hiding this comment.
OK plz add a comment like
// expect a non-specific low-level revert, since this call will ultimately fail as part of the precompile call
cheats.expectRevert();
blsSignatureChecker.checkSignatures(
msgHash,
quorumNumbers,
referenceBlockNumber,
nonSignerStakesAndSignature
);
There was a problem hiding this comment.
This is looking really good!
Definitely want more documentation on this, but I don't think I'm seeing any real problems; I'd like to take another look after documentation is added but am primarily expecting to understand better (but it should also help me identify bugs or holes in testing easier)
|
Am approving the PR for now, assuming no further logical changes. Please add some more clarifying comments / documentation prior to merging. |
Added unit tests for the signature checker