Move relayer to subfolder #411

iansuvak · 2024-08-05T16:18:00Z

Why this should be merged

Partially fixes #409. This would also enable us to rename this repo into something more general. icm-offchain-services ?

How this works

Moved

main to relayer/main
relayer specific configs to relayer/config
relayer specific network initialization code from peers.AppRequestNetwork to it's own main.go
Updates Readme and creates a stub top level README that links to individual applications

Kept at top level

E2E testing
scripts folder
build folder (binaries are distinguished by name but output to the same folder)

Not moved

database
messages

The folders above are relayer specific right now but I haven't moved them yet. Should we? They seem auxillary to the core function and could be feasibly generalized in the future.

How this was tested

CI

How is this documented

Updated READMEs

geoff-vball

One nit otherwise LGTM

scripts/build_relayer.sh

michaelkaplan13

👍 I this structure will much better suit us going forward.

README.md

michaelkaplan13 · 2024-08-15T15:32:32Z

README.md

-```bash
-go generate ./...
-```
+1. [AWM Relayer](relayer/README.md)


Should this be ICM relayer now?

May also be helpful to include a 1 sentence description of each service at the top level here.

I'll include descriptions but do we want to hold off on changing the name here until we change the name of the build binary / rest of the documentation / and the whole repo?

Ah yeah good call

michaelkaplan13 · 2024-08-15T15:34:36Z

relayer/main/main.go

@@ -534,3 +535,125 @@ func initializeMetrics() (prometheus.Gatherer, prometheus.Registerer, error) {
 	}
 	return gatherer, registry, nil
 }
+
+func initializeConnectionsAndCheckStake(


initializeConnectionsAndCheckStake, connectToNonPrimaryNetworkPeers, and connectToPrimaryNetworkPeers may make more sense to live in a non-main package specific to the relayer, but not a huge deal either way.

Yeah I thought about it originally, but they are only called as part of initialization at app startup. I am happy to move them but any idea on the naming? I don't think we need a new package for it since it would define no structs, methods or interfaces.

Maybe network_utils.go in the relayer package?

Sounds good to me 👍

Co-authored-by: Michael Kaplan <55204436+michaelkaplan13@users.noreply.github.com> Signed-off-by: Ian Suvak <ian.suvak@avalabs.org>

geoff-vball · 2024-08-15T19:37:26Z

relayer/network_utils.go

+		if ok, quorum, err := checkForSufficientConnectedStake(logger, cfg, connectedValidators, blockchainID); !ok {
+			logger.Error(
+				"Failed to connect to a threshold of stake",
+				zap.String("destinationBlockchainID", blockchainID.String()),
+				zap.Uint64("connectedWeight", connectedValidators.ConnectedWeight),
+				zap.Uint64("totalValidatorWeight", connectedValidators.TotalValidatorWeight),
+				zap.Any("warpQuorum", quorum),
+			)
+			return err
+		}
+	}
+	return nil


In the case that we don't connect to a threshold of stake, we log an error here and then return nil, is that's what is intended?

I think the intended flow here should be to check for an error first, return the error if present, then check ok, which should maybe return a new error if we haven't connected to enough stake.

Thank you for finding this. It indeed seems like a bug in the existing implementation, but this whole initialization step is not really necessary I only kept it for now to maintain existing behavior.

For now I updated the comment to indicate actual behavior, since I do think that the existing bug i.e. more permissive behavior is actually what we want but if we align on another approach I'm happy to implement it.

geoff-vball · 2024-08-15T19:43:59Z

relayer/network_utils.go

+// Fetch the warp quorum from the config and check if the connected stake exceeds the threshold
+func checkForSufficientConnectedStake(
+	logger logging.Logger,
+	cfg *config.Config,
+	connectedValidators *peers.ConnectedCanonicalValidators,
+	destinationBlockchainID ids.ID,
+) (bool, *config.WarpQuorum, error) {
+	quorum, err := cfg.GetWarpQuorum(destinationBlockchainID)
+	if err != nil {
+		logger.Error(
+			"Failed to get warp quorum from config",
+			zap.String("destinationBlockchainID", destinationBlockchainID.String()),
+			zap.Error(err),
+		)
+		return false, nil, err
+	}
+	return utils.CheckStakeWeightExceedsThreshold(
+		big.NewInt(0).SetUint64(connectedValidators.ConnectedWeight),
+		connectedValidators.TotalValidatorWeight,
+		quorum.QuorumNumerator,
+		quorum.QuorumDenominator,
+	), &quorum, nil
+}


Going along with my above comment - I think this would actually be better served not being a function - the tri-state return is kind of confusing. I think we should just inline this in the calling function.

If we wanted, we could make a helper that takes connectedValidators and a quorum and returns a bool to do the calculation on lines 133-138

I like the suggestion to break up the quorum check and the stake weight check into separate functions.

Questions on checking sufficient stake

cam-schultz

LGTM. I left once comment about the level of one of the logs, but don't feel strongly either way.

relayer/network_utils.go

bernard-avalabs

Generally looks good to me. Just left some minor comments.

bernard-avalabs · 2024-08-16T18:56:17Z

relayer/network_utils.go

+) error {
+	for _, sourceBlockchain := range cfg.SourceBlockchains {
+		if sourceBlockchain.GetSubnetID() == constants.PrimaryNetworkID {
+			if err := connectToPrimaryNetworkPeers(logger, network, cfg, sourceBlockchain); err != nil {


(Probably not for this PR) Not sure if there are any latency requirements for InitializeConnectionsAndCheckStake, but if there are, we could consider parallelizing these connectToPrimaryNetworkPeers calls (up to some threshold concurrency level).

Updated #311 to include addressing followups from you and @geoff-vball in a follow-up

bernard-avalabs · 2024-08-16T19:01:06Z

relayer/network_utils.go

+// Fetch the warp quorum from the config and check if the connected stake exceeds the threshold
+func checkForSufficientConnectedStake(
+	logger logging.Logger,
+	cfg *config.Config,
+	connectedValidators *peers.ConnectedCanonicalValidators,
+	destinationBlockchainID ids.ID,
+) (bool, *config.WarpQuorum, error) {
+	quorum, err := cfg.GetWarpQuorum(destinationBlockchainID)
+	if err != nil {
+		logger.Error(
+			"Failed to get warp quorum from config",
+			zap.String("destinationBlockchainID", destinationBlockchainID.String()),
+			zap.Error(err),
+		)
+		return false, nil, err
+	}
+	return utils.CheckStakeWeightExceedsThreshold(
+		big.NewInt(0).SetUint64(connectedValidators.ConnectedWeight),
+		connectedValidators.TotalValidatorWeight,
+		quorum.QuorumNumerator,
+		quorum.QuorumDenominator,
+	), &quorum, nil
+}


I like the suggestion to break up the quorum check and the stake weight check into separate functions.

Need to update Github workflows to point to build_relayer.sh instead of build.sh

cam-schultz

The entry point path in .goreleaser.yml needs to be updated from main/main.go to relayer/main/main.go

https://github.com/ava-labs/awm-relayer/blob/main/.goreleaser.yml#L4

Move relayer to subfolder along with docs update

dafdb56

iansuvak added the Warp Signature API API service for serving arbitrary Warp signatures from any VM label Aug 5, 2024

iansuvak added this to the Signature Aggregator API - MVP milestone Aug 5, 2024

iansuvak self-assigned this Aug 5, 2024

Base automatically changed from signature-aggregation-api to main August 8, 2024 20:04

iansuvak added 7 commits August 13, 2024 09:07

Merge remote-tracking branch 'origin/main' into demote-relayer

325a064

move relayer configs

53972c7

update all references to configs

03ec5dc

move resources back to top level and update links

82a9bf2

update path to sample-relayer-config.json

1ae2d02

Merge remote-tracking branch 'origin/main' into demote-relayer

03b24a4

move relayer specific network initialization code to its own main

0fb6427

iansuvak force-pushed the demote-relayer branch from c0f394c to 0fb6427 Compare August 13, 2024 14:54

iansuvak changed the title ~~[WIP] Move relayer to subfolder along with docs update~~ Move relayer to subfolder along with docs update Aug 13, 2024

iansuvak marked this pull request as ready for review August 13, 2024 15:07

iansuvak requested a review from a team as a code owner August 13, 2024 15:07

iansuvak requested review from feuGeneA, geoff-vball, minghinmatthewlam, bernard-avalabs, michaelkaplan13 and cam-schultz August 13, 2024 15:07

geoff-vball reviewed Aug 13, 2024

View reviewed changes

scripts/build_relayer.sh Outdated Show resolved Hide resolved

iansuvak added 2 commits August 13, 2024 12:33

standardize on basecfg as import alias

70daa3d

Update scripts

07e43af

iansuvak changed the title ~~Move relayer to subfolder along with docs update~~ Move relayer to subfolder Aug 13, 2024

geoff-vball previously approved these changes Aug 13, 2024

View reviewed changes

michaelkaplan13 reviewed Aug 15, 2024

View reviewed changes

iansuvak and others added 2 commits August 15, 2024 11:56

Update README.md

4742355

Co-authored-by: Michael Kaplan <55204436+michaelkaplan13@users.noreply.github.com> Signed-off-by: Ian Suvak <ian.suvak@avalabs.org>

Merge remote-tracking branch 'origin/main' into demote-relayer

66eba53

iansuvak added 4 commits August 15, 2024 12:14

Add descriptions for top level services

5210e3b

Merge branch 'main' into demote-relayer

776fe94

move network util functions from main.go to their own file in relayer

a4302df

Merge branch 'main' into demote-relayer

99ce1fe

iansuvak requested a review from michaelkaplan13 August 15, 2024 19:11

geoff-vball reviewed Aug 15, 2024

View reviewed changes

Update comment in network initialization

5f631d1

iansuvak mentioned this pull request Aug 16, 2024

E2E testing improvements #436

Merged

cam-schultz previously approved these changes Aug 16, 2024

View reviewed changes

relayer/network_utils.go Outdated Show resolved Hide resolved

iansuvak added 2 commits August 16, 2024 14:53

downgrade error logs to warnings

cb15259

Merge remote-tracking branch 'origin/main' into demote-relayer

573754a

bernard-avalabs reviewed Aug 16, 2024

View reviewed changes

cam-schultz reviewed Aug 16, 2024

View reviewed changes

geoff-vball approved these changes Aug 16, 2024

View reviewed changes

update .goreleaser.yml

aae5c4b

cam-schultz approved these changes Aug 16, 2024

View reviewed changes

bernard-avalabs approved these changes Aug 16, 2024

View reviewed changes

iansuvak mentioned this pull request Aug 16, 2024

Consider simplifying the aggregation logic #388

Open

iansuvak merged commit 46daa55 into main Aug 16, 2024
7 checks passed

iansuvak deleted the demote-relayer branch August 16, 2024 20:25

iansuvak mentioned this pull request Aug 16, 2024

Fix E2E test flake in TriggerProcessMissedBlocks #437

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move relayer to subfolder #411

Move relayer to subfolder #411

iansuvak commented Aug 5, 2024 •

edited

Loading

geoff-vball left a comment

michaelkaplan13 left a comment

michaelkaplan13 Aug 15, 2024

iansuvak Aug 15, 2024

michaelkaplan13 Aug 15, 2024

michaelkaplan13 Aug 15, 2024

iansuvak Aug 15, 2024

michaelkaplan13 Aug 15, 2024

geoff-vball Aug 15, 2024

iansuvak Aug 16, 2024

geoff-vball Aug 15, 2024

bernard-avalabs Aug 16, 2024

cam-schultz left a comment

bernard-avalabs left a comment

bernard-avalabs Aug 16, 2024 •

edited

Loading

iansuvak Aug 16, 2024

bernard-avalabs Aug 16, 2024

cam-schultz left a comment

Move relayer to subfolder #411

Move relayer to subfolder #411

Conversation

iansuvak commented Aug 5, 2024 • edited Loading

Why this should be merged

How this works

How this was tested

How is this documented

geoff-vball left a comment

Choose a reason for hiding this comment

michaelkaplan13 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

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

cam-schultz left a comment

Choose a reason for hiding this comment

bernard-avalabs left a comment

Choose a reason for hiding this comment

bernard-avalabs Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cam-schultz left a comment

Choose a reason for hiding this comment

iansuvak commented Aug 5, 2024 •

edited

Loading

bernard-avalabs Aug 16, 2024 •

edited

Loading