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.

Sign up for GitHub

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

Jump to bottom

Make merkle proofs optional on multisig ISM #2173

Merged

yorhodes merged 45 commits into main from optional-merkle

May 25, 2023

Member

yorhodes commented May 4, 2023 •

edited

Loading

Description

Validators currently sign (root, index) checkpoints and during verification, a message is passed as calldata, an id() is derived, and a proof of id() at index in root is verified

This provides “all or nothing” censorship resistance guarantees because a validator can only sign roots to allow any contained messages to be processed.

We have considered alternatives where validators sign message directly and we lose censorship resistance in exchange for eliminating merkle proof verification gas costs.

However, if validators sign (root, index, message) tuples, we can skip merkle proof verification on the destination chain while still maintaining censorship resistance by providing two valid metadata formats:

existing validator signatures and merkle proof verification of inclusion
including merkle proof verification for pathway where validators are censoring message

It’s worth noting the validator is required to index event data to produce this new signature format. However, this does not require historical indexing and new validators being spun up can simply begin indexing from tip.

See #2187 for validator changes
See #2248 for relayer and e2e test changes

Drive-by changes

Merkle index also optional

Related issues

Fixes Death to Merkle Tree #2192

Backward compatibility

new ISM deployment is necessary (we could upgrade implementation in theory)
Validator and relayer upgrades

Testing

Unit (fuzz) Tests

asaj reviewed

View reviewed changes

solidity/contracts/libs/isms/MultisigIsmMetadata.sol Show resolved Hide resolved

solidity/contracts/libs/CheckpointLib.sol Show resolved Hide resolved

solidity/contracts/libs/LegacyCheckpointLib.sol Show resolved Hide resolved

solidity/contracts/isms/multisig/AbstractMultisigIsm.sol Outdated Show resolved Hide resolved

solidity/contracts/libs/isms/MultisigIsmMetadata.sol Outdated Show resolved Hide resolved

asaj reviewed

View reviewed changes

solidity/contracts/isms/multisig/AbstractMultisigIsm.sol Outdated Show resolved Hide resolved

solidity/contracts/isms/multisig/AbstractMultisigIsm.sol Outdated Show resolved Hide resolved

solidity/contracts/isms/multisig/AbstractMultisigIsm.sol Outdated Show resolved Hide resolved

solidity/contracts/libs/isms/MultisigIsmMetadata.sol Outdated Show resolved Hide resolved

solidity/contracts/libs/isms/MultisigIsmMetadata.sol Outdated Show resolved Hide resolved

solidity/contracts/isms/multisig/AbstractMultisigIsm.sol Outdated Show resolved Hide resolved

solidity/contracts/isms/multisig/AbstractMultisigIsm.sol Outdated Show resolved Hide resolved

solidity/contracts/isms/multisig/AbstractMultisigIsm.sol Outdated Show resolved Hide resolved

asaj reviewed

View reviewed changes

solidity/contracts/isms/multisig/AbstractMultisigIsm.sol Outdated Show resolved Hide resolved

nambrot mentioned this pull request

Death to Merkle Tree #2192

Closed

1 task

github-advanced-security bot found potential problems

View reviewed changes

solidity/contracts/isms/multisig/AbstractMultisigIsm.sol Fixed Show fixed Hide fixed

yorhodes marked this pull request as ready for review

May 5, 2023 21:26

yorhodes requested a review from tkporter as a code owner

May 5, 2023 21:26

asaj reviewed

View reviewed changes

solidity/contracts/isms/multisig/AbstractMultisigIsm.sol Outdated Show resolved Hide resolved

solidity/contracts/isms/multisig/AbstractMultisigIsm.sol Outdated Show resolved Hide resolved

solidity/contracts/isms/multisig/AbstractMultisigIsm.sol Outdated Show resolved Hide resolved

solidity/contracts/isms/multisig/AbstractMultisigIsm.sol Outdated Show resolved Hide resolved

solidity/contracts/libs/LegacyCheckpointLib.sol Show resolved Hide resolved

solidity/contracts/libs/isms/MultisigIsmMetadata.sol Outdated Show resolved Hide resolved

solidity/contracts/libs/isms/MultisigIsmMetadata.sol Outdated Show resolved Hide resolved

tkporter reviewed

View reviewed changes

solidity/contracts/isms/multisig/AbstractMultisigIsm.sol Outdated Show resolved Hide resolved

solidity/contracts/libs/isms/MultisigIsmMetadata.sol Outdated Show resolved Hide resolved

solidity/contracts/libs/isms/MultisigIsmMetadata.sol Outdated Show resolved Hide resolved

yorhodes force-pushed the optional-merkle branch from bfede3d to 8f280b9 Compare

May 9, 2023 20:17

yorhodes requested review from asaj and tkporter

May 9, 2023 20:32

tkporter mentioned this pull request

Sealevel MultisigIsm - implement it #2216

Closed

yorhodes requested review from nambrot and jmrossy as code owners

May 10, 2023 21:23

asaj removed request for nambrot, jmrossy and tkporter

May 11, 2023 14:46

asaj reviewed

View reviewed changes

Contributor

asaj left a comment

Very nice. Haven't looked at tests yet

solidity/contracts/isms/multisig/AbstractMerkleRootMultisigIsm.sol Outdated Show resolved Hide resolved

solidity/contracts/isms/multisig/AbstractMerkleRootMultisigIsm.sol Show resolved Hide resolved

solidity/contracts/isms/multisig/AbstractMerkleRootMultisigIsm.sol Outdated Show resolved Hide resolved

solidity/contracts/isms/multisig/AbstractMerkleRootMultisigIsm.sol Show resolved Hide resolved

solidity/contracts/isms/multisig/AbstractMerkleRootMultisigIsm.sol Show resolved Hide resolved

solidity/contracts/libs/isms/MerkleRootMultisigIsmMetadata.sol Outdated Show resolved Hide resolved

solidity/contracts/libs/isms/MerkleRootMultisigIsmMetadata.sol Show resolved Hide resolved

solidity/contracts/libs/isms/MerkleRootMultisigIsmMetadata.sol Outdated Show resolved Hide resolved

solidity/contracts/libs/isms/MessageIdMultisigIsmMetadata.sol Show resolved Hide resolved

typescript/sdk/src/ism/contracts.ts Outdated Show resolved Hide resolved

yorhodes mentioned this pull request

Update validators with new signature scheme #2187

Merged

yorhodes added 10 commits

May 17, 2023 15:05


          Make merkle proofs optional on multisig ISM

f153063


          Fix tests

d338321


          Optimize calldata usage and memory expansion


          Update gas snapshot

cecefc9


          Reorganize metadata suffix

c04cd4e


          Improve test coverage of multisig ism

53019b6


          Fix legacy ism tests

39b7389


          Remove implicit return

a4182fa


          Make metadata property names more descriptive


          Reorganize metadata suffix (again)

9497aca

yorhodes requested a review from asaj

May 23, 2023 17:50

asaj reviewed

View reviewed changes

Contributor

asaj left a comment

Skipped the contracts for expediency

rust/agents/relayer/src/msg/metadata/base.rs Show resolved Hide resolved

rust/agents/relayer/src/msg/metadata/multisig/base.rs Show resolved Hide resolved

rust/agents/relayer/src/msg/metadata/multisig/base.rs Outdated Show resolved Hide resolved

rust/agents/relayer/src/msg/metadata/multisig/base.rs Outdated Show resolved Hide resolved

rust/agents/validator/src/settings.rs Show resolved Hide resolved

typescript/sdk/src/consts/multisigIsm.ts Outdated Show resolved Hide resolved

typescript/sdk/src/ism/HyperlaneIsmFactory.ts Show resolved Hide resolved

typescript/sdk/src/ism/HyperlaneIsmFactory.ts Show resolved Hide resolved

typescript/sdk/src/consts/environments/test.json Show resolved Hide resolved

typescript/sdk/src/ism/HyperlaneIsmFactoryDeployer.ts Outdated Show resolved Hide resolved

yorhodes added 4 commits

May 24, 2023 01:00


          Validator only signs once tree is consistent

eb26ab1


          Address pr comments

2fa9df7


          Cleanup diff

06eab5c


          Address more pr comments

yorhodes requested a review from asaj

May 24, 2023 05:19

yorhodes added 3 commits

May 24, 2023 01:20


          Fix infra build

3c69ff6


          Remove message_id check from merkle root builder

1241afe


          Adjust instrumented logs

254991d

asaj reviewed

View reviewed changes

Contributor

asaj left a comment

Reviewed the diff

rust/agents/relayer/src/msg/metadata/base.rs Outdated Show resolved Hide resolved

rust/agents/relayer/src/msg/metadata/multisig/base.rs Outdated Show resolved Hide resolved

rust/agents/validator/src/submit.rs Outdated Show resolved Hide resolved

asaj reviewed

View reviewed changes

rust/agents/validator/src/validator.rs Show resolved Hide resolved

asaj reviewed

View reviewed changes

Contributor

asaj left a comment

Re-reviewed the contracts, just nits, we're in the home stretch

solidity/contracts/isms/multisig/AbstractMerkleRootMultisigIsm.sol Outdated Show resolved Hide resolved

solidity/contracts/isms/multisig/AbstractMessageIdMultisigIsm.sol Outdated Show resolved Hide resolved

solidity/contracts/isms/multisig/AbstractMessageIdMultisigIsm.sol Outdated Show resolved Hide resolved

solidity/contracts/isms/multisig/AbstractMultisigIsm.sol Show resolved Hide resolved

solidity/contracts/isms/multisig/AbstractMultisigIsm.sol Outdated Show resolved Hide resolved

solidity/contracts/isms/multisig/StaticMultisigIsmFactory.sol Outdated Show resolved Hide resolved

solidity/contracts/libs/LegacyCheckpointLib.sol Outdated Show resolved Hide resolved

solidity/contracts/libs/LegacyCheckpointLib.sol Outdated Show resolved Hide resolved

solidity/test/isms/MultisigIsm.t.sol Outdated Show resolved Hide resolved

solidity/test/isms/MultisigIsm.t.sol Outdated Show resolved Hide resolved

yorhodes added 2 commits

May 24, 2023 13:16


          Fix fragile target logic

08dbc45


          Fix lint

f698c1d

mattiekat approved these changes

View reviewed changes

Contributor

mattiekat left a comment

I just looked at the rust stuff and mostly focused on rust idoms since I think @asaj already looked more at the logic

rust/agents/relayer/Cargo.toml Outdated Show resolved Hide resolved

rust/agents/relayer/src/msg/metadata/multisig/base.rs Outdated Show resolved Hide resolved

rust/agents/relayer/src/msg/metadata/multisig/base.rs Outdated Show resolved Hide resolved

rust/agents/relayer/src/msg/metadata/multisig/base.rs Outdated Show resolved Hide resolved

rust/agents/relayer/src/msg/metadata/multisig/base.rs Outdated Show resolved Hide resolved

rust/hyperlane-base/src/types/multisig.rs Show resolved Hide resolved

rust/hyperlane-base/src/types/multisig.rs Outdated Show resolved Hide resolved

rust/hyperlane-base/src/types/multisig.rs Outdated Show resolved Hide resolved

rust/hyperlane-base/src/types/multisig.rs Show resolved Hide resolved

rust/hyperlane-core/Cargo.toml Outdated Show resolved Hide resolved

mattiekat approved these changes

View reviewed changes

Contributor

mattiekat left a comment

I just looked at the rust stuff and mostly focused on rust idioms since I think @asaj already looked more at the logic

yorhodes added 7 commits

May 24, 2023 13:55


          Address contract pr comments

a5225b2


          Address rust pr comments

4dda113


          Address more rust pr comments

e027a24


          Address more rust pr comments

da1d78a


          Address sdk pr comments

08022e7


          Sleep for max u64 when backfill is done

1b584f7


          Merge branch 'main' into optional-merkle

3e1af6b

yorhodes enabled auto-merge (squash)

May 24, 2023 19:14

yorhodes added 2 commits

May 24, 2023 15:28


          Fix lint again

73b0cb0


          Merge branch 'main' into optional-merkle

ff85708

yorhodes merged commit 50f04db into main

yorhodes deleted the optional-merkle branch

May 25, 2023 14:14

nambrot added this to the Death to Merkle Tree milestone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet