Skip to content
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

Fix support for validator key rotation, add end-to-end test #1643

Merged
merged 219 commits into from
Nov 16, 2019

Conversation

asaj
Copy link
Contributor

@asaj asaj commented Nov 8, 2019

Description

This PR fixes support for validator key rotation, and tests it in the end-to-end governance tests.

Tested

  • Updates the governance end-to-end tests to test that key rotation works by using the account as the validator signer in epoch N, an authorized validator signer in epoch N+1, and a second authorized validator signer in epoch N+2

Other changes

  • Refactor publicKeysData logic to split out the various components, not store the proof-of-possession.
  • Updated end-to-end governance tests to ensure no IBFT round changes by inferring the random proposer order and asserting blocks are proposed in that order
  • Fix small bug in end-to-end tests where private keys written to the same file
  • Update account:authorize CLI command so that we don't assume account and signing keys are managed by the same node, add account:proof-of-possession command
  • Make name optional in account:register
  • Small changes to naming in Accounts.sol
  • Reorder functions in Accounts.sol
  • Move some logic from protocol into utils

Related issues

Backwards compatibility

Not backwards compatible

Copy link
Contributor

@nambrot nambrot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just nits!

) {
const msg = new Buffer('dummy_msg_data')
const data = '0x' + msg.toString('hex')
// Note: Eth.sign typing displays incorrect parameter order
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This note probably should be elsewhere now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, utils/address seems like a better place

// Note: Eth.sign typing displays incorrect parameter order
const sig = await signFn(data, signer)

const rawsig = ethjsutil.fromRpcSig(sig)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional Nit: Didn't know about fromRpcSig!! Could we streamline either that or our implementation (from below =parseSignature) here?

@nambrot nambrot removed their assignment Nov 14, 2019
Copy link
Contributor

@jmrossy jmrossy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small comments

.circleci/config.yml Show resolved Hide resolved
packages/utils/src/address.ts Show resolved Hide resolved
export const privateKeyToPublicKey = (privateKey: string) => {
return '0x' + privateToPublic(Buffer.from(privateKey.slice(2), 'hex')).toString('hex')
export const publicKeyToAddress = (publicKey: string) => {
return toChecksumAddress(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving similar feedback I left for Nam on his PR that touches this:
Would be nice these methods confirmed the leading chars are '0x' before stripping them. So happens @yorhodes has added utils in this file for just this
#1570

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

signatureUtils has this functionality, suggest he use that.

) {
const msg = new Buffer('dummy_msg_data')
const data = '0x' + msg.toString('hex')
// Note: Eth.sign typing displays incorrect parameter order
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, utils/address seems like a better place

return '0x' + pubKey.toString('hex')
}

export function eqAddress(a: Address, b: Address) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an areAddressesEqual below in this file AND @yorhodes adds another one in his PR here: #1570
I'll leave it to you two to coordinate but pretty sure we don't need another util method for this ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must have been a merge conflict? This was supposed to move to address.ts

@asaj asaj requested a review from cmcewen as a code owner November 15, 2019 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

End-to-end tests for validator key rotation
6 participants