From e802585558c158de588531ef8357f20cd28e95d9 Mon Sep 17 00:00:00 2001 From: Trevor Porter Date: Tue, 12 Apr 2022 15:37:46 +0100 Subject: [PATCH] self nits --- solidity/core/contracts/Common.sol | 2 +- solidity/core/contracts/Inbox.sol | 2 +- .../contracts/test/TestValidatorManager.sol | 8 +++- .../InboxMultisigValidatorManager.sol | 5 +++ .../MultisigValidatorManager.sol | 19 ++++++---- .../OutboxMultisigValidatorManager.sol | 34 +++++++++++------ .../inboxMultisigValidatorManager.test.ts | 8 ++-- .../multisigValidatorManager.test.ts | 22 +++++------ .../outboxMultisigValidatorManager.test.ts | 8 ++-- solidity/core/test/validator-manager/utils.ts | 2 +- typescript/hardhat/src/TestAbacusDeploy.ts | 34 +++++++++-------- typescript/infra/src/core/deploy.ts | 38 +++++++------------ 12 files changed, 99 insertions(+), 83 deletions(-) diff --git a/solidity/core/contracts/Common.sol b/solidity/core/contracts/Common.sol index e5694d8b0..37c2b87cd 100644 --- a/solidity/core/contracts/Common.sol +++ b/solidity/core/contracts/Common.sol @@ -51,7 +51,7 @@ abstract contract Common is ICommon, OwnableUpgradeable { // ============ Modifiers ============ /** - * @notice Ensures that function is called by the ValidatorManager contract + * @notice Ensures that a function is called by the ValidatorManager contract. */ modifier onlyValidatorManager() { require(msg.sender == validatorManager, "!validatorManager"); diff --git a/solidity/core/contracts/Inbox.sol b/solidity/core/contracts/Inbox.sol index edfc27cde..54e03e1d4 100644 --- a/solidity/core/contracts/Inbox.sol +++ b/solidity/core/contracts/Inbox.sol @@ -82,7 +82,7 @@ contract Inbox is IInbox, Version0, Common { /** * @notice Checkpoints the provided root and index. - * @dev Called by the ValidatorManager, which is responsible for verifying + * @dev Called by the ValidatorManager, which is responsible for verifying a * quorum of validator signatures on the checkpoint. * @dev Reverts if checkpoints's index is not greater than our latest index. * @param _root Checkpoint's merkle root. diff --git a/solidity/core/contracts/test/TestValidatorManager.sol b/solidity/core/contracts/test/TestValidatorManager.sol index 2864c6556..3f2821a68 100644 --- a/solidity/core/contracts/test/TestValidatorManager.sol +++ b/solidity/core/contracts/test/TestValidatorManager.sol @@ -1,11 +1,15 @@ // SPDX-License-Identifier: MIT OR Apache-2.0 pragma solidity >=0.6.11; -import {Inbox} from "../Inbox.sol"; +import {IInbox} from "../../interfaces/IInbox.sol"; +/** + * Intended for testing Inbox.sol, which requires its validator manager + * to be a contract. + */ contract TestValidatorManager { function checkpoint( - Inbox _inbox, + IInbox _inbox, bytes32 _root, uint256 _index ) external { diff --git a/solidity/core/contracts/validator-manager/InboxMultisigValidatorManager.sol b/solidity/core/contracts/validator-manager/InboxMultisigValidatorManager.sol index 36e0b7444..372f331d8 100644 --- a/solidity/core/contracts/validator-manager/InboxMultisigValidatorManager.sol +++ b/solidity/core/contracts/validator-manager/InboxMultisigValidatorManager.sol @@ -6,6 +6,11 @@ pragma abicoder v2; import {IInbox} from "../../interfaces/IInbox.sol"; import {MultisigValidatorManager} from "./MultisigValidatorManager.sol"; +/** + * @title InboxMultisigValidatorManager + * @notice Verifies checkpoints are signed by a quorum of validators and submits + * them to an Inbox. + */ contract InboxMultisigValidatorManager is MultisigValidatorManager { // ============ Constructor ============ diff --git a/solidity/core/contracts/validator-manager/MultisigValidatorManager.sol b/solidity/core/contracts/validator-manager/MultisigValidatorManager.sol index 61830c766..740f1c9d9 100644 --- a/solidity/core/contracts/validator-manager/MultisigValidatorManager.sol +++ b/solidity/core/contracts/validator-manager/MultisigValidatorManager.sol @@ -8,8 +8,9 @@ import {ECDSA} from "@openzeppelin/contracts/cryptography/ECDSA.sol"; import {EnumerableSet} from "@openzeppelin/contracts/utils/EnumerableSet.sol"; /** - * @notice Manages an ownable validator set that sign checkpoints with - * a basic ECDSA multi-signature. + * @title MultisigValidatorManager + * @notice Manages an ownable set of validators that ECDSA sign checkpoints to + * reach a quorum. */ abstract contract MultisigValidatorManager is Ownable { // ============ Libraries ============ @@ -41,22 +42,24 @@ abstract contract MultisigValidatorManager is Ownable { event EnrollValidator(address indexed validator); /** - * @notice Emitted when a validator is unenrolled in the validator set. + * @notice Emitted when a validator is unenrolled from the validator set. * @param validator The address of the validator. */ event UnenrollValidator(address indexed validator); /** * @notice Emitted when the quorum threshold is set. - * @param quorumThreshold The quorum threshold. + * @param quorumThreshold The new quorum threshold. */ event SetQuorumThreshold(uint256 quorumThreshold); // ============ Constructor ============ /** - * @param _outboxDomain The domain of the outbox this validator manager - * tracks the validator set for. + * @param _outboxDomain The domain of the outbox the validator set is for. + * @param _validatorSet The set of validator addresses. + * @param _quorumThreshold The quorum threshold. Must be greater than or equal + * to the length of _validatorSet. */ constructor( uint32 _outboxDomain, @@ -106,7 +109,7 @@ abstract contract MultisigValidatorManager is Ownable { /** * @notice Gets the addresses of the current validator set. - * @dev There is no ordering guarantee. + * @dev There ordering guarantee due to the semantics of EnumerableSet.AddressSet. * @return The addresses of the validator set. */ function validatorSet() external view returns (address[] memory) { @@ -207,6 +210,8 @@ abstract contract MultisigValidatorManager is Ownable { /** * @notice Unenrolls a validator from the validator set. + * @dev Reverts if the resulting validator set length is less than + * the quorum threshold. * @dev Reverts if _validator is not in the validator set. * @param _validator The validator to remove from the validator set. */ diff --git a/solidity/core/contracts/validator-manager/OutboxMultisigValidatorManager.sol b/solidity/core/contracts/validator-manager/OutboxMultisigValidatorManager.sol index 22b22d5b8..46a0af3ad 100644 --- a/solidity/core/contracts/validator-manager/OutboxMultisigValidatorManager.sol +++ b/solidity/core/contracts/validator-manager/OutboxMultisigValidatorManager.sol @@ -6,14 +6,22 @@ pragma abicoder v2; import {IOutbox} from "../../interfaces/IOutbox.sol"; import {MultisigValidatorManager} from "./MultisigValidatorManager.sol"; +/** + * @title OutboxMultisigValidatorManager + * @notice Verifies if an improper checkpoint has been signed by a quorum of + * validators and reports it to an Outbox. + */ contract OutboxMultisigValidatorManager is MultisigValidatorManager { // ============ Events ============ /** * @notice Emitted when proof of an improper checkpoint is submitted. + * @dev Observers of this event should filter by the outbox address. + * @param outbox The outbox. * @param root Root of the improper checkpoint. * @param index Index of the improper checkpoint. * @param signatures A quorum of signatures on the improper checkpoint. + * May include non-validator signatures. */ event ImproperCheckpoint( address indexed outbox, @@ -25,7 +33,10 @@ contract OutboxMultisigValidatorManager is MultisigValidatorManager { // ============ Constructor ============ /** - * @param _localDomain The local domain. + * @param _outboxDomain The local domain. + * @param _validatorSet The set of validator addresses. + * @param _quorumThreshold The quorum threshold. Must be greater than or equal + * to the length of _validatorSet. */ // solhint-disable-next-line no-empty-blocks constructor( @@ -36,16 +47,17 @@ contract OutboxMultisigValidatorManager is MultisigValidatorManager { // ============ External Functions ============ - // Determines if a quorum of signers have signed an improper checkpoint, - // and fails the Outbox if so. - // If staking / slashing existed, we'd want to check this for individual validator - // signatures. Because we don't care about that and we don't want a single byzantine - // validator to be able to fail the outbox, we require a quorum. - // - // Gets the domain from IOutbox(_outbox).localDomain(), then - // requires isQuorum(domain, _root, _index, _signatures), - // requires that the checkpoint is an improper checkpoint, - // and calls IOutbox(_outbox).fail(). (Similar behavior as existing improperCheckpoint) + /** + * @notice Determines if a quorum of validators have signed an improper checkpoint, + * failing the Outbox if so. + * @dev Improper checkpoints signed by individual validators are not handled to prevent + * a single byzantine validator from failing the Outbox. + * @param _outbox The outbox. + * @param _root The merkle root of the checkpoint. + * @param _index The index of the checkpoint. + * @param _signatures Signatures over the checkpoint to be checked for a validator + * quorum. Must be sorted in ascending order by signer address. + */ function improperCheckpoint( IOutbox _outbox, bytes32 _root, diff --git a/solidity/core/test/validator-manager/inboxMultisigValidatorManager.test.ts b/solidity/core/test/validator-manager/inboxMultisigValidatorManager.test.ts index 041df1cda..7abe03341 100644 --- a/solidity/core/test/validator-manager/inboxMultisigValidatorManager.test.ts +++ b/solidity/core/test/validator-manager/inboxMultisigValidatorManager.test.ts @@ -8,14 +8,14 @@ import { InboxMultisigValidatorManager, InboxMultisigValidatorManager__factory, } from '../../types'; -import { getCheckpointSignatures } from './utils'; +import { signCheckpoint } from './utils'; import { expect } from 'chai'; const OUTBOX_DOMAIN = 1234; const INBOX_DOMAIN = 4321; const QUORUM_THRESHOLD = 2; -describe.only('InboxMultisigValidatorManager', () => { +describe('InboxMultisigValidatorManager', () => { let validatorManager: InboxMultisigValidatorManager, inbox: Inbox, signer: SignerWithAddress, @@ -55,7 +55,7 @@ describe.only('InboxMultisigValidatorManager', () => { const index = 1; it('submits a checkpoint to the Inbox if there is a quorum', async () => { - const signatures = await getCheckpointSignatures( + const signatures = await signCheckpoint( root, index, [validator0, validator1], // 2/2 signers, making a quorum @@ -67,7 +67,7 @@ describe.only('InboxMultisigValidatorManager', () => { }); it('reverts if there is not a quorum', async () => { - const signatures = await getCheckpointSignatures( + const signatures = await signCheckpoint( root, index, [validator0], // 1/2 signers is not a quorum diff --git a/solidity/core/test/validator-manager/multisigValidatorManager.test.ts b/solidity/core/test/validator-manager/multisigValidatorManager.test.ts index afb0cc215..8805ed436 100644 --- a/solidity/core/test/validator-manager/multisigValidatorManager.test.ts +++ b/solidity/core/test/validator-manager/multisigValidatorManager.test.ts @@ -7,12 +7,9 @@ import { TestMultisigValidatorManager, TestMultisigValidatorManager__factory, } from '../../types'; -import { getCheckpointSignatures } from './utils'; +import { signCheckpoint } from './utils'; const OUTBOX_DOMAIN = 1234; -const OUTBOX_DOMAIN_HASH = ethers.utils.keccak256( - ethers.utils.solidityPack(['uint32', 'string'], [OUTBOX_DOMAIN, 'ABACUS']), -); const QUORUM_THRESHOLD = 1; const domainHashTestCases = require('../../../../vectors/domainHash.json'); @@ -60,8 +57,9 @@ describe('MultisigValidatorManager', async () => { }); it('sets the outboxDomainHash', async () => { + const outboxDomainHash = await validatorManager.domainHash(OUTBOX_DOMAIN); expect(await validatorManager.outboxDomainHash()).to.equal( - OUTBOX_DOMAIN_HASH, + outboxDomainHash, ); }); @@ -175,7 +173,7 @@ describe('MultisigValidatorManager', async () => { ); }); - it('reverts if the new quorum threshold is > the validator set size', async () => { + it('reverts if the new quorum threshold is greater than the validator set size', async () => { await expect(validatorManager.setQuorumThreshold(3)).to.be.revertedWith( '!range', ); @@ -201,7 +199,7 @@ describe('MultisigValidatorManager', async () => { }); it('returns true when there is a quorum', async () => { - const signatures = await getCheckpointSignatures(root, index, [ + const signatures = await signCheckpoint(root, index, [ validator0, validator1, ]); @@ -210,7 +208,7 @@ describe('MultisigValidatorManager', async () => { }); it('returns true when a quorum exists even if provided with non-validator signatures', async () => { - const signatures = await getCheckpointSignatures( + const signatures = await signCheckpoint( root, index, [validator0, validator1, validator3], // validator 3 is not enrolled @@ -219,8 +217,8 @@ describe('MultisigValidatorManager', async () => { .true; }); - it('returns false when the signature count is < the quorum threshold', async () => { - const signatures = await getCheckpointSignatures(root, index, [ + it('returns false when the signature count is less than the quorum threshold', async () => { + const signatures = await signCheckpoint(root, index, [ validator0, ]); expect(await validatorManager.isQuorum(root, index, signatures)).to.be @@ -228,7 +226,7 @@ describe('MultisigValidatorManager', async () => { }); it('returns false when some signatures are not from enrolled validators', async () => { - const signatures = await getCheckpointSignatures( + const signatures = await signCheckpoint( root, index, [validator0, validator3], // validator 3 is not enrolled @@ -241,7 +239,7 @@ describe('MultisigValidatorManager', async () => { // Reverse the signature order, purposely messing up the // ascending sort const signatures = ( - await getCheckpointSignatures(root, index, [validator0, validator1]) + await signCheckpoint(root, index, [validator0, validator1]) ).reverse(); await expect( diff --git a/solidity/core/test/validator-manager/outboxMultisigValidatorManager.test.ts b/solidity/core/test/validator-manager/outboxMultisigValidatorManager.test.ts index a4d567626..c10d97ef8 100644 --- a/solidity/core/test/validator-manager/outboxMultisigValidatorManager.test.ts +++ b/solidity/core/test/validator-manager/outboxMultisigValidatorManager.test.ts @@ -9,7 +9,7 @@ import { OutboxMultisigValidatorManager, OutboxMultisigValidatorManager__factory, } from '../../types'; -import { getCheckpointSignatures } from './utils'; +import { signCheckpoint } from './utils'; const OUTBOX_DOMAIN = 1234; const INBOX_DOMAIN = 4321; @@ -50,7 +50,7 @@ describe('OutboxMultisigValidatorManager', () => { const index = 1; it('accepts an improper checkpoint if there is a quorum', async () => { - const signatures = await getCheckpointSignatures( + const signatures = await signCheckpoint( root, index, [validator0, validator1], // 2/2 signers, making a quorum @@ -71,7 +71,7 @@ describe('OutboxMultisigValidatorManager', () => { }); it('reverts if there is not a quorum', async () => { - const signatures = await getCheckpointSignatures( + const signatures = await signCheckpoint( root, index, [validator0], // 1/2 signers is not a quorum @@ -97,7 +97,7 @@ describe('OutboxMultisigValidatorManager', () => { await outbox.checkpoint(); const [root, index] = await outbox.latestCheckpoint(); - const signatures = await getCheckpointSignatures( + const signatures = await signCheckpoint( root, index.toNumber(), [validator0, validator1], // 2/2 signers, making a quorum diff --git a/solidity/core/test/validator-manager/utils.ts b/solidity/core/test/validator-manager/utils.ts index 5a06ce4ae..a41fcd049 100644 --- a/solidity/core/test/validator-manager/utils.ts +++ b/solidity/core/test/validator-manager/utils.ts @@ -2,7 +2,7 @@ import { types, Validator } from '@abacus-network/utils'; // Signs a checkpoint with the provided validators and returns // the signatures sorted by validator addresses in ascending order -export async function getCheckpointSignatures( +export async function signCheckpoint( root: types.HexString, index: number, unsortedValidators: Validator[], diff --git a/typescript/hardhat/src/TestAbacusDeploy.ts b/typescript/hardhat/src/TestAbacusDeploy.ts index 175edb3f0..de825c46c 100644 --- a/typescript/hardhat/src/TestAbacusDeploy.ts +++ b/typescript/hardhat/src/TestAbacusDeploy.ts @@ -1,5 +1,5 @@ import { ethers } from "ethers"; -import { types } from "@abacus-network/utils"; +import { types, Validator } from "@abacus-network/utils"; import { Outbox, Outbox__factory, @@ -63,7 +63,7 @@ export class TestAbacusDeploy extends TestDeploy< await outboxMultisigValidatorManagerFactory.deploy( domain, [signerAddress], - 1 + 1, ); const inboxMultisigValidatorManagerFactory = @@ -80,7 +80,7 @@ export class TestAbacusDeploy extends TestDeploy< await inboxMultisigValidatorManagerFactory.deploy( remote, [signerAddress], - 1 + 1, ); inboxMultisigValidatorManagers[remote] = inboxMultisigValidatorManager; }); @@ -112,8 +112,6 @@ export class TestAbacusDeploy extends TestDeploy< const inboxFactory = new TestInbox__factory(signer); const inboxes: Record = {}; - // this.remotes reads this.instances which has not yet been set. - // const remotes = Object.keys(this.config.signer).map((d) => parseInt(d)); const inboxDeploys = remotes.map(async (remote) => { const inboxMultisigValidatorManager = inboxMultisigValidatorManagers[remote]; @@ -217,23 +215,27 @@ export class TestAbacusDeploy extends TestDeploy< ) { return; } - // TODO come back to this - // Update the Outbox and Inboxes to the latest roots. // This is technically not necessary given that we are not proving against // a root in the TestInbox. - // const validator = await Validator.fromSigner( - // this.config.signer[domain], - // domain - // ); - // const { signature } = await validator.signCheckpoint( - // root, - // index.toNumber() - // ); + const validator = await Validator.fromSigner( + this.config.signer[domain], + domain + ); + const { signature } = await validator.signCheckpoint( + root, + index.toNumber() + ); for (const remote of this.remotes(domain)) { const inbox = this.inbox(remote, domain); - await inbox.checkpoint(root, index /*, signature*/); + const inboxMultisigValidatorManager = this.inboxMultisigValidatorManager(remote, domain); + await inboxMultisigValidatorManager.checkpoint( + inbox.address, + root, + index, + [signature], + ); } // Find all messages dispatched on the outbox since the previous checkpoint. diff --git a/typescript/infra/src/core/deploy.ts b/typescript/infra/src/core/deploy.ts index b2ef65944..a88fd58f4 100644 --- a/typescript/infra/src/core/deploy.ts +++ b/typescript/infra/src/core/deploy.ts @@ -1,4 +1,3 @@ -// @ts-nocheck import path from 'path'; import { ethers } from 'ethers'; import { types } from '@abacus-network/utils'; @@ -30,18 +29,6 @@ export class AbacusCoreDeployer extends AbacusAppDeployer< CoreContractAddresses, CoreConfig > { - multisigValidatorManagerConfig( - config: CoreConfig, - domain: types.Domain, - ): MultisigValidatorManagerConfig { - const domainName = this.mustResolveDomainName(domain); - const validatorManagerConfig = config.multisigValidatorManagers[domainName]; - if (!validatorManagerConfig) { - throw new Error(`No validator manager config for ${domainName}`); - } - return validatorManagerConfig; - } - async deployContracts( domain: types.Domain, config: CoreConfig, @@ -67,16 +54,6 @@ export class AbacusCoreDeployer extends AbacusAppDeployer< outboxMultisigValidatorManagerConfig.quorumThreshold, ); - // for (const name of this.domainNames) { - // const validator = config.validators[name]; - // if (!validator) throw new Error(`No validator for ${name}`); - // await validatorManager.enrollValidator( - // this.resolveDomain(name), - // validator, - // overrides, - // ); - // } - const outbox = await this.deployProxiedContract( domain, 'Outbox', @@ -249,15 +226,28 @@ export class AbacusCoreDeployer extends AbacusAppDeployer< ): Promise { const contracts = core.mustGetContracts(domain); const overrides = core.getOverrides(domain); - await contracts.validatorManager.transferOwnership(owner, overrides); + await contracts.outboxMultisigValidatorManager.transferOwnership(owner, overrides); await contracts.xAppConnectionManager.transferOwnership(owner, overrides); await contracts.upgradeBeaconController.transferOwnership(owner, overrides); for (const chain of Object.keys( contracts.addresses.inboxes, ) as ChainName[]) { + await contracts.inboxMultisigValidatorManager(chain).transferOwnership(owner, overrides); await contracts.inbox(chain).transferOwnership(owner, overrides); } const tx = await contracts.outbox.transferOwnership(owner, overrides); return tx.wait(core.getConfirmations(domain)); } + + multisigValidatorManagerConfig( + config: CoreConfig, + domain: types.Domain, + ): MultisigValidatorManagerConfig { + const domainName = this.mustResolveDomainName(domain); + const validatorManagerConfig = config.multisigValidatorManagers[domainName]; + if (!validatorManagerConfig) { + throw new Error(`No validator manager config for ${domainName}`); + } + return validatorManagerConfig; + } }