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

auto update clients to prevent expiry #412

Merged
merged 10 commits into from
Feb 12, 2021
Merged

Conversation

akhilkumarpilli
Copy link
Contributor

Fixes: #395

Added a go routine in rly start command, which will run in a loop for every 1m by default. It will check all the clients in provided path and if they are going to expire in next 10mins (default), it will update. Default time values for checking expiry can be modified by flags.

Copy link
Contributor Author

@akhilkumarpilli akhilkumarpilli left a comment

Choose a reason for hiding this comment

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

tested

Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

Great start! Good placement of where to add the go routine. I think we should try to cut down the code and reuse as much as possible. Less code means less maintenance/bugs

cmd/flags.go Outdated Show resolved Hide resolved
cmd/flags.go Outdated Show resolved Hide resolved
cmd/start.go Outdated Show resolved Hide resolved
cmd/start.go Outdated Show resolved Hide resolved
cmd/start.go Outdated Show resolved Hide resolved
cmd/start.go Outdated Show resolved Hide resolved
cmd/start.go Outdated Show resolved Hide resolved
cmd/flags.go Outdated Show resolved Hide resolved
cmd/start.go Outdated Show resolved Hide resolved
cmd/start.go Outdated Show resolved Hide resolved
cmd/flags.go Outdated Show resolved Hide resolved
cmd/start.go Outdated Show resolved Hide resolved
cmd/flags.go Outdated Show resolved Hide resolved
cmd/start.go Outdated Show resolved Hide resolved
relayer/client-tx.go Outdated Show resolved Hide resolved
cmd/start.go Outdated Show resolved Hide resolved
relayer/client-tx.go Outdated Show resolved Hide resolved
relayer/client-tx.go Outdated Show resolved Hide resolved
relayer/client-tx.go Outdated Show resolved Hide resolved
relayer/client-tx.go Outdated Show resolved Hide resolved
relayer/client-tx.go Outdated Show resolved Hide resolved
cmd/start.go Outdated
func UpdateClientsFromChains(src, dst *relayer.Chain) (sleepTime time.Duration, err error) {
var srcTimeExpiry, dstTimeExpiry time.Duration

thresholdTime := viper.GetDuration(flagThresholdTime)
Copy link
Member

Choose a reason for hiding this comment

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

This logic should not be here. We should just return timeToExpiry in this function and subtract thresholdTime from caller.

cmd/start.go Outdated Show resolved Hide resolved
relayer/client-tx.go Outdated Show resolved Hide resolved
relayer/client-tx.go Outdated Show resolved Hide resolved
relayer/client-tx.go Outdated Show resolved Hide resolved
cmd/start.go Outdated
return 0, err
}

if srcTimeExpiry > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

This can be massively simplified by just using math.Min.

If either srcTimeExpiry or dstTimeExpiry is less than 0 just return an error.

cmd/start.go Outdated Show resolved Hide resolved
cmd/start.go Outdated
}

if srcTimeExpiry <= 0 || dstTimeExpiry <= 0 {
return 0, fmt.Errorf("one of the clients (%s,%s) trusting period is 0 or expired",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'd be useful to separate these checks and tell the user which client on which chain is expired. Also I believe it is not possible for the trusting period to be 0

cmd/start.go Outdated Show resolved Hide resolved
return 0, err
}

dstUH, err := sh.GetTrustedHeader(dst, src)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the logic here is switched. Nonetheless this will be replaced with src.GetIBCUpdateHeader(dst) after merging my refactor pr

Copy link
Contributor

Choose a reason for hiding this comment

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

just merged that pr to master, so you can update now

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

I think updating the clients every N blocks makes more sense than using time duration

Comment on lines 353 to 354
// GetClientAndUpdate update clients to prevent expiry
func GetClientAndUpdate(src, dst *Chain, thresholdTime time.Duration) (time.Duration, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this name is misleading in some way, this doesn't return the client but the trusting period.

cmd/start.go Show resolved Hide resolved
Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

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

utACK

Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

utACK, wonderful work!

@colin-axner colin-axner merged commit 81dd43c into master Feb 12, 2021
@colin-axner colin-axner deleted the akhil/395-auto-updates branch February 12, 2021 10:28
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.

Automatic Updates to clients to prevent expiry
4 participants