2370 staketable update validator identification keys #2373
Conversation
…ions that need to be updated to the new spec
…ey. todo: add tests
…ate-validator-identification-keys
…nclusion of the blsKey
alysiahuggins
changed the title
… account the validator registered with
…lsVK comparison to field by field comparison so that it is more efficient and more obvious what is going on
…hat have been commented out for now
…um address, update related implementations and add tests
alysiahuggins
changed the title
alysiahuggins
requested review from
ImJeremyHe,
sveitser,
jbearer,
tbro and
imabdulbasit
as code owners
| /// @dev This function can only be called by the validator itself when it's not in the exit | ||
| /// queue | ||
| /// @dev The validator will need to give up either its old BLS key and/or old Schnorr key | ||
| /// @dev The validator will need to provide a BLS signature over the new BLS key |
There was a problem hiding this comment.
See comment above. The message signed is the address of the validator.
There was a problem hiding this comment.
clarified the comment here 0cc8946
| @@ -24,20 +24,24 @@ abstract contract AbstractStakeTable { | |||
| uint256 public totalVotingStake; | |||
|
|
|||
| /// @notice Signals a registration of a BLS public key. | |||
| /// @param blsVKhash hash of the BLS public key that is registered. | |||
| /// @param account the address of the validator | |||
| /// @param registerEpoch epoch when the registration becomes effective. | |||
There was a problem hiding this comment.
This is HotShot epoch, correct?
There was a problem hiding this comment.
good q. The epoch side of things haven't been fully implemented yet (issue here). Ideally it's the hotshot epoch but yet to determine how the contract would be aware of hotshot's epochs
contracts/src/StakeTable.sol
Outdated
|
|
||
| // Update the node's schnorr key once it's not the same as the old one and it's nonzero | ||
| if ( | ||
| !EdOnBN254.isEqual(newSchnorrVK, node.schnorrVK) |
There was a problem hiding this comment.
For the EdOnBn254 and BN254 types, can we use using ... for ... to make the code a bit more readable?
There was a problem hiding this comment.
e1b7e25 so i didn't add the isEqual function to the BN254 library since it was an external library but i did add it to the EdOnBn254 library so i added the using ... for ... syntax for that.
@philippecamacho shall I add isEqual and a zero point BN254 constant to the BN254 codebase? Also is it a good idea to add a zero point EdOnBN254 constant? Zero point checks have been added to the register and updateConsensusKeys functions.
| @@ -299,28 +319,28 @@ contract StakeTable is AbstractStakeTable { | |||
| // Create an entry for the node. | |||
| node.account = msg.sender; | |||
| node.balance = fixedStakeAmount; | |||
| node.blsVK = blsVK; | |||
There was a problem hiding this comment.
Do I understand correctly that we check that the keys are not zero when if consensus keys are updated but we don't check for that during registration? Is that intentional?
There was a problem hiding this comment.
I guess we don't need to check the blsVk because we verify a signature but what about the schnorrVk?
There was a problem hiding this comment.
yes you're right, there should be checks on the vk(s) here too!
There was a problem hiding this comment.
added checks on zero point keys e1b7e25
contracts/src/StakeTable.sol
Outdated
| && EdOnBN254.isEqual(newSchnorrVK, node.schnorrVK) | ||
| ) | ||
| || ( | ||
| _isEqualBlsKey( |
There was a problem hiding this comment.
Since we verify the BLS sig, is it necessary to check that the key is non-zero?
There was a problem hiding this comment.
Ah I see it's allowed to be zero if only the schnorr key is updated.
There was a problem hiding this comment.
now any zero keys cause a revert 638ed78
contracts/test/StakeTable.t.sol
Outdated
| vm.stopPrank(); | ||
| } | ||
|
|
||
| function test_UpdateConsensusKeysWithInvalidBlsKeyInfoButOnlySchnorrVKChanged_Succeeds() |
There was a problem hiding this comment.
The name of this test function sort of prompted me to think that maybe we should rethink this particular part of the API a bit (previous comment).
Co-authored-by: Philippe Camacho <[email protected]>
…o change occurs if both keys are zero and only one key is changed if the other is a zero key
alysiahuggins
deleted the
2370-staketable-update-validator-identification-keys
branch
Closes #2370 which is a sub issue of #2304
This PR:
This PR does not:
Key places to review:
How to test this PR:
forge test --match-contract StakeTable