V3 agents rebase #2746

daniel-savu · 2023-09-21T17:20:37Z

Description

It's your favourite PR coming right back... V3 agents!

Closes https://github.com/hyperlane-xyz/issues/issues/561

Builds on top of #2742

Depends on #2681 for e2e testing

This PR includes:

Merkle tree hook indexer
Merkle tree builder task
Update submitter to trigger retries if no proof is available yet

Slightly more detailed overview of the work here: #2720 (comment)

Drive-by changes

Related issues

Backward compatibility

Testing

daniel-savu · 2023-09-21T17:22:06Z

Doesn't compile yet though

codecov · 2023-09-21T17:28:46Z

Codecov Report

Merging #2746 (863ecfd) into v3 (a60ec18) will increase coverage by 0.56%.
The diff coverage is 0.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##               v3    #2746      +/-   ##
==========================================
+ Coverage   63.92%   64.49%   +0.56%     
==========================================
  Files          97       97              
  Lines         973      966       -7     
  Branches      101      100       -1     
==========================================
+ Hits          622      623       +1     
+ Misses        314      307       -7     
+ Partials       37       36       -1

Components	Coverage Δ
core	`51.61% <ø> (ø)`
hooks	`67.93% <ø> (ø)`
isms	`68.65% <ø> (+0.74%)`	⬆️
token	`84.84% <ø> (ø)`
middlewares	`73.17% <ø> (ø)`

Continues the updates to the rust config shapes by updating deployment and runtime expectations. This PR also attempts to rely on the new single source of truth created by the schema in the SDK as much as reasonably possible which helped delete more code but also give some guarantees of consistency. THIS IS A BREAKING CHANGE! It changes the config shapes the agents want and we should not merge this until we are ready. Fixes #2215 --------- Co-authored-by: Guillaume Bouvignies <[email protected]> Co-authored-by: Yorke Rhodes <[email protected]> Co-authored-by: Guillaume Bouvignies <[email protected]>

This reverts commit 9f999a3.

yorhodes · 2023-10-04T16:12:21Z

.gitignore

@@ -28,3 +28,4 @@ yarn-error.log
 **/*.ignore
 .vscode

+tsconfig.editor.json


whats this?

Added by @mattiecnvr afaik

rust/agents/relayer/src/error.rs

yorhodes · 2023-10-04T16:13:10Z

rust/agents/relayer/src/merkle_tree/builder.rs

-    /// Nonce was not found in DB, despite batch providing messages after
-    #[error("Nonce was not found {nonce:?}")]
-    UnavailableNonce {
-        /// Root of prover's local merkle tree
-        nonce: u32,
-    },


we dont need this anymore?

didn't find any uses of it

rust/hyperlane-base/src/db/rocks/hyperlane_db.rs

yorhodes · 2023-10-04T16:24:01Z

rust/hyperlane-base/src/settings/chains.rs

@@ -76,6 +77,8 @@ pub struct CoreContractAddresses {
    pub interchain_gas_paymaster: H256,
    /// Address of the ValidatorAnnounce contract
    pub validator_announce: H256,
+    /// Address of the MerkleTreeHook contract
+    pub merkle_tree_hook: Option<H256>,


what if this is None?

It could be none if there's no deployment. The agents default to querying the mailbox for the merkle tree count in this case (e.g. in sealevel)

rust/chains/hyperlane-ethereum/src/mailbox.rs

rust/chains/hyperlane-ethereum/tests/signer_output.rs

yorhodes · 2023-10-04T16:37:32Z

rust/chains/hyperlane-ethereum/src/merkle_tree_hook.rs

+        // TODO: if `SequenceIndexer` turns out to not depend on `Indexer` at all, then the supertrait
+        // dependency could be removed, even if the builder would still need to return a type that is both
+        // ``SequenceIndexer` and `Indexer`.


should we be tracking this somewhere? I'm not sure I understand all the implications @tkporter

if we do get around to revamping indexing, this will certainly be addressed one way or another. But I think it's low prio for now

+1 this 100% needs refactoring at some point but it is a big undertaking

I'd probably put this in the realm of #2313

rust/agents/relayer/src/error.rs

rust/agents/relayer/src/merkle_tree/builder.rs

rust/agents/relayer/src/merkle_tree/processor.rs

tkporter · 2023-10-04T17:07:18Z

rust/agents/relayer/src/msg/gas_payment/mod.rs

@@ -195,7 +194,7 @@ mod test {
        test_utils::run_test_db(|db| async move {
            let hyperlane_db =
                HyperlaneRocksDB::new(&HyperlaneDomain::new_test_domain("test_no_match"), db);
-            let matching_list = serde_json::from_str(r#"[{"originDomain": 234}]"#).unwrap();
+            let matching_list = serde_json::from_str(r#"[{"origindomain": 234}]"#).unwrap();


is this necessary? assuming this is from Mattie's config change but curious if this implies that originDomain doesn't work anymore

I think what this tests is whether the config is robust against casing inconsistencies and that originDomain will still work

rust/agents/relayer/src/msg/metadata/base.rs

tkporter

I'd definitely say all the lag block number stuff is not critical to change immediately btw, just should do it at some point

rust/agents/relayer/src/msg/metadata/multisig/base.rs

rust/agents/relayer/src/msg/metadata/multisig/merkle_root_multisig.rs

rust/agents/relayer/src/msg/metadata/multisig/message_id_multisig.rs

rust/agents/relayer/src/processor.rs

tkporter · 2023-10-04T17:42:03Z

rust/chains/hyperlane-ethereum/src/merkle_tree_hook.rs

+            .map(|v| v.into())
+            .collect::<Vec<_>>()
+            .try_into()
+            .unwrap();


should we be unwrapping? dunno if this was there before - if it was, worth checking if we can easily remove, but if we can't then nbd

the unwrap is safe here. It's required because although we're iterating over elements of a fixed-size array ([u8; 32]), collect can only produce a collection that implements FromIterator - which isn't the case for fixed-size arrays because an iterator has no knowledge about how many items it's iterating over. So we need the try_into() to convert the Vec<H256> into [H256; 32]. Alternative would be

let mut branch: [H256; 32] = [Default::default(); 32]; raw_tree .branch .iter() .enumerate() .for_each(|(index, v)| branch[index] = v.into());

This code can still panic with an out-of-bounds access if raw_tree.branch exceeds 32, and would probably be less obvious to debug than with the unwrap version. I'd prefer leaving as-is and adding a comment, what's your take?

tkporter · 2023-10-04T17:43:29Z

rust/chains/hyperlane-ethereum/src/merkle_tree_hook.rs

+            .saturating_sub(lag)
+            .into();
+
+        // TODO: implement From<Tree> for IncrementalMerkle


may not wanna do this because the Tree type is ethereum-specific and we should avoid leaking types like that into vm-agnostic code

isnt this the ethereum crate?

I was originally saying this bc IncrementalMerkle is in hyperlane-core and this would require adding hyperlane-ethereum as a dependency to hyperlane-core (which imo we can't do), but maybe we could instead just do Into<IncrementalMerkle> for Tree inside hyperlane-ethereum

implemented inside the merkle tree hook module (this same file) since that's where it's used

btw that comment was already there. in general I kept the merkle tree code intact but it's good we get to revisit these

rust/chains/hyperlane-ethereum/src/merkle_tree_hook.rs

rust/chains/hyperlane-sealevel/src/mailbox.rs

rust/hyperlane-base/src/contract_sync/cursor.rs

rust/hyperlane-base/src/db/rocks/hyperlane_db.rs

tkporter · 2023-10-04T18:02:38Z

rust/hyperlane-base/src/settings/chains.rs

+        let ctx = "Building merkle tree hook";
+        // TODO: if the merkle tree hook is set for sealevel, it's still a mailbox program
+        // that the connection is made to using the pda seeds, which will not be usable.
+        let address = self


I don't like the silently different behavior tbh

I think we should make merkle_tree_hook mandatory for now, and just change Sealevel in the JSON configs to specify the Mailbox address as the merkleTreeHook

Eventually I could see us wanting to make hooks in general optional, but that feels out of the scope of what this PR should do

just flagging that even though we have dummy implementations for the merkle tree indexers on sealevel, we won't be covering sealevel in e2e so panics could still occur

rust/sealevel/programs/ism/multisig-ism-message-id/src/processor.rs

rust/utils/run-locally/src/invariants.rs

rust/agents/relayer/src/processor.rs

### Description Part of the remediations from #2746 --------- Co-authored-by: Trevor Porter <[email protected]>

Remediations for #2746, in addition to #2780

daniel-savu and others added 11 commits September 21, 2023 17:21

contract changes from v3-agents branch

d7dc8af

add mrh builder

c58d664

store in rocksDB

3151149

feat(relayer): merkle insertion indexer

80ab924

wip(relayer): merkle builder task

4555704

first merkle tree task impl

423ad5f

finish merkle tree processor implementation

b3a1f45

fix: clippy

9dc552e

cleanup

6b3a3bb

review fixes

240d6ee

trigger e2e

0bdb5e6

daniel-savu requested review from yorhodes, jmrossy, mattiekat, tkporter and aroralanuk as code owners September 21, 2023 17:20

daniel-savu mentioned this pull request Sep 21, 2023

V3 agent changes #2742

Closed

3 tasks

aroralanuk and others added 3 commits September 21, 2023 14:17

hyperlane interceptor

9fd8f8d

fix compilation

bcf40b6

Hopefully fixed agent config artifact writing for e2e

f8008cb

tkporter requested a review from nambrot as a code owner September 25, 2023 09:17

daniel-savu force-pushed the v3-agents-rebase branch from 5a13f3c to f8008cb Compare September 25, 2023 10:50

tkporter and others added 6 commits September 25, 2023 17:02

Fix yarn kathy maybe

4e16acc

fix: e2e tests don't crash (but don't pass)

2e5f00c

fmt

6f5e404

fix: out of gas v3 txs

852b29b

fix?: yarn clean before e2e

f7911c1

fix: mailbox count

e90d85e

daniel-savu and others added 13 commits October 3, 2023 17:37

fix: validator announce

291eb93

Merge branch 'v3' into v3-agents-rebase

0ba2b9c

Try taking v3 solidity and typescript

2907581

Setup e2e tests with hook deployment

1a1eb88

Fix kathy

d4c587a

TEMP: try disabling sealevel e2e

9f999a3

wip: update metadata format in agents

58e289f

feat: v3 metadata

a76a4c8

Revert "TEMP: try disabling sealevel e2e"

6f639e7

This reverts commit 9f999a3.

temp: disable sealevel e2e invariants

7bfe7cd

Merge branch 'v3' into v3-agents-rebase

044b523

fix clippy

9f88da0

yorhodes approved these changes Oct 4, 2023

View reviewed changes

fix: yorke's review comments

9448b66

tkporter reviewed Oct 4, 2023

View reviewed changes

rust/hyperlane-base/src/db/rocks/hyperlane_db.rs Show resolved Hide resolved

tkporter reviewed Oct 4, 2023

View reviewed changes

fix: trevor's 1st round of comments

863ecfd

daniel-savu commented Oct 4, 2023

View reviewed changes

rust/agents/relayer/src/processor.rs Show resolved Hide resolved

daniel-savu enabled auto-merge (squash) October 4, 2023 18:14

daniel-savu merged commit af5bd88 into v3 Oct 4, 2023

daniel-savu deleted the v3-agents-rebase branch October 4, 2023 18:42

daniel-savu mentioned this pull request Oct 5, 2023

V3 agents feedback #2780

Merged

tkporter added a commit that referenced this pull request Oct 6, 2023

V3 agents feedback (#2780)

c6cea78

### Description Part of the remediations from #2746 --------- Co-authored-by: Trevor Porter <[email protected]>

yorhodes pushed a commit that referenced this pull request Oct 10, 2023

V3 agents feedback (#2780)

59716da

### Description Part of the remediations from #2746 --------- Co-authored-by: Trevor Porter <[email protected]>

daniel-savu mentioned this pull request Oct 10, 2023

last round of PR remediations #2791

Merged

yorhodes mentioned this pull request Nov 16, 2023

cli gas preflight #2927

Merged

daniel-savu added a commit that referenced this pull request Dec 20, 2023

last round of PR remediations (#2791)

f73ee0b

Remediations for #2746, in addition to #2780

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

V3 agents rebase #2746

V3 agents rebase #2746

daniel-savu commented Sep 21, 2023

daniel-savu commented Sep 21, 2023

codecov bot commented Sep 21, 2023 •

edited

Loading

yorhodes Oct 4, 2023

daniel-savu Oct 4, 2023

yorhodes Oct 4, 2023

daniel-savu Oct 4, 2023

yorhodes Oct 4, 2023

daniel-savu Oct 4, 2023

yorhodes Oct 4, 2023

daniel-savu Oct 4, 2023

tkporter Oct 4, 2023

tkporter Oct 4, 2023

daniel-savu Oct 4, 2023 •

edited

Loading

tkporter left a comment

tkporter Oct 4, 2023

daniel-savu Oct 6, 2023

tkporter Oct 4, 2023

yorhodes Oct 4, 2023

tkporter Oct 5, 2023 •

edited

Loading

daniel-savu Oct 6, 2023

daniel-savu Oct 6, 2023

tkporter Oct 4, 2023

daniel-savu Oct 10, 2023

V3 agents rebase #2746

V3 agents rebase #2746

Conversation

daniel-savu commented Sep 21, 2023

Description

Drive-by changes

Related issues

Backward compatibility

Testing

daniel-savu commented Sep 21, 2023

codecov bot commented Sep 21, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-savu Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

tkporter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkporter Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 21, 2023 •

edited

Loading

daniel-savu Oct 4, 2023 •

edited

Loading

tkporter Oct 5, 2023 •

edited

Loading