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

Default to 2/3 instead of 1/3 for IBC client security parameters #2876

Closed
6 of 7 tasks
adizere opened this issue Nov 21, 2022 · 9 comments · Fixed by #2988
Closed
6 of 7 tasks

Default to 2/3 instead of 1/3 for IBC client security parameters #2876

adizere opened this issue Nov 21, 2022 · 9 comments · Fixed by #2988
Assignees
Labels
I: configuration Internal: related to Hermes configuration O: security Objective: cause to enhance security and improve safety
Milestone

Comments

@adizere
Copy link
Member

adizere commented Nov 21, 2022

Summary

Replace the 1/3 default with a 2/3 default.

Context: see the recommendation following the work by Tendermint team here: tendermint/tendermint#9420

Acceptance Criteria

  • replace default trust threshold from 1/3 to 2/3 for IBC client security params
  • test relaying in a multi-network testate setup and confirm no impact is observed when validator sets continuously overlap

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate milestone (priority) applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@adizere adizere added O: security Objective: cause to enhance security and improve safety I: configuration Internal: related to Hermes configuration labels Nov 21, 2022
@adizere adizere added this to the v1.3 milestone Nov 21, 2022
@crodriguezvega
Copy link

@adizere Since the trust threshold is configurable, will there be some kind of validation to check that the threshold is >= 2/3? Otherwise an operator might choose to use a lower threshold anyway...

@adizere
Copy link
Member Author

adizere commented Nov 22, 2022

@adizere Since the trust threshold is configurable, will there be some kind of validation to check that the threshold is >= 2/3? Otherwise an operator might choose to use a lower threshold anyway...

The value is already validated to be > 0 and <1, that is, any value chose by an operator with Hermes must be a valid fraction larger than 0 and smaller than 1. But we don't plan to perform validation that it should always be >= 2/3. I think it's reasonable to allow operators to select different other values, eg 1/3 if they wish so. Do you see concerns with this?

@adizere adizere added this to Hermes Dec 13, 2022
@adizere adizere moved this to 📋 Backlog in Hermes Dec 13, 2022
@seanchen1991 seanchen1991 modified the milestones: v1.3, Backlog Dec 13, 2022
@romac romac modified the milestones: Backlog, v1.3 Dec 19, 2022
@romac romac self-assigned this Dec 19, 2022
@romac romac moved this from 📋 Backlog to 📥 Todo in Hermes Dec 19, 2022
@romac romac removed their assignment Dec 19, 2022
@romac
Copy link
Member

romac commented Jan 6, 2023

We currently only relay on clients with a trust threshold >= 1/3 and <= 2/3. Should we relax the upper bound to 1/1?

@adizere
Copy link
Member Author

adizere commented Jan 9, 2023

Should we relax the upper bound to 1/1?

Not sure why we'd need that in the context of this issues. But I don't see problems with this change either!

@romac
Copy link
Member

romac commented Jan 10, 2023

@ancazamfir What do you think?

@romac romac moved this from 📥 Todo to 🏗 In progress in Hermes Jan 10, 2023
@romac romac self-assigned this Jan 10, 2023
@ancazamfir
Copy link
Collaborator

@ancazamfir What do you think?

I think we should keep the >=1/3 for now and change default to 2/3. If someone wants <1/3 they can create the client through other means. So maybe at some point we can allow that with a big warning.

@romac
Copy link
Member

romac commented Jan 11, 2023

Agreed. I was asking about the upper bound, should we relax it to 1/1?

@ancazamfir
Copy link
Collaborator

Agreed. I was asking about the upper bound, should we relax it to 1/1?

sorry i missed the question completely. I don't see a reason to change the upper bound.

@romac
Copy link
Member

romac commented Jan 11, 2023

Alright :)

@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Hermes Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: configuration Internal: related to Hermes configuration O: security Objective: cause to enhance security and improve safety
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

5 participants