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.

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

Document clock drift parameters in guide #3420

Merged
merged 16 commits into from
Jul 7, 2023
Merged

Document clock drift parameters in guide #3420

merged 16 commits into from
Jul 7, 2023

Conversation

seanchen1991
Copy link
Contributor

Closes: #3139

Description

Adds a new "Handling Clock Drift" section to the Hermes guide. The docs still need some more context on the issues that may arise when the clock drift parameters are mis-configured. @ancazamfir is doing some testing on this, and we'll add those details to this section when she has done this.

PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests: integration (for Hermes) or unit/mock tests (for modules).
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).
  • Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

@seanchen1991 seanchen1991 added the I: documentation Internal: improvements or additions to documentation label Jun 14, 2023
@romac romac marked this pull request as ready for review June 15, 2023 12:58
ancazamfir
ancazamfir requested changes Jun 19, 2023
View reviewed changes
Copy link
Collaborator

@ancazamfir ancazamfir left a comment

Choose a reason for hiding this comment

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

Left some comments, most of them editorial. We need to find a reference to the FLA attacks. @josef-widder anything in the spec that talks about this?

guide/src/advanced/troubleshooting/clock-drift.md Outdated Show resolved Hide resolved
guide/src/advanced/troubleshooting/clock-drift.md Outdated Show resolved Hide resolved
guide/src/advanced/troubleshooting/clock-drift.md Outdated Show resolved Hide resolved
guide/src/advanced/troubleshooting/clock-drift.md Outdated Show resolved Hide resolved
guide/src/advanced/troubleshooting/cross-comp-config.md Outdated Show resolved Hide resolved
seanchen1991 and others added 8 commits June 19, 2023 10:07
@seanchen1991 @ancazamfir
Update guide/src/advanced/troubleshooting/clock-drift.md
1cc5206 
Co-authored-by: Anca Zamfir <[email protected]>
Signed-off-by: Sean Chen <[email protected]>
@seanchen1991 @ancazamfir
Update guide/src/advanced/troubleshooting/clock-drift.md
f8c81f7 
Co-authored-by: Anca Zamfir <[email protected]>
Signed-off-by: Sean Chen <[email protected]>
@seanchen1991
Remove redundant section
19cce87
@seanchen1991 @ancazamfir
Update guide/src/advanced/troubleshooting/clock-drift.md
20163ed 
Co-authored-by: Anca Zamfir <[email protected]>
Signed-off-by: Sean Chen <[email protected]>
@seanchen1991
Merge branch 'master' into sean/clock-drift-docs
1afc3e4
@seanchen1991 @ancazamfir
Update guide/src/advanced/troubleshooting/clock-drift.md
0147acf 
Co-authored-by: Anca Zamfir <[email protected]>
Signed-off-by: Sean Chen <[email protected]>
@seanchen1991
Explain what C constant represents
d6553e3
@seanchen1991
Merge branch 'master' into sean/clock-drift-docs
b4ceff2
@josef-widder
Copy link
Member

Left some comments, most of them editorial. We need to find a reference to the FLA attacks. @josef-widder anything in the spec that talks about this?

What does FLA stand for?

@seanchen1991
Copy link
Contributor Author

What does FLA stand for?

Forward Lunatic Attack I believe.

@josef-widder
Copy link
Member

josef-widder commented Jun 20, 2023 •
edited
Loading

The attack was handled here. There is some documentation in the following ADRs:

I thought that we also updated the specs back then, but back then the specs where in a separate repository so the updates would not have been in the same Tendermint release. I couldn't find anything in the specs however. I can try to dig deeper if the ADRs are not sufficient.

@seanchen1991
Copy link
Contributor Author

Thanks @josef-widder! I think the ADRs are sufficient 🙂

@seanchen1991
Copy link
Contributor Author

@ancazamfir How do these changes look?

ancazamfir
ancazamfir approved these changes Jul 7, 2023
View reviewed changes
Copy link
Collaborator

@ancazamfir ancazamfir left a comment

Choose a reason for hiding this comment

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

Great! Thanks @seanchen1991

@seanchen1991 seanchen1991 merged commit 576aad6 into master Jul 7, 2023
@seanchen1991 seanchen1991 deleted the sean/clock-drift-docs branch July 7, 2023 15:54
DaviRain-Su
DaviRain-Su reviewed Jul 7, 2023
View reviewed changes
guide/src/advanced/troubleshooting/clock-drift.md
@@ -0,0 +1,34 @@
# Handling Clock Drift

IBC light client security model requires that the timestamp of a header included in client updates for some `client` is within `[now - client.trusting_period, now + client.max_clock_drift)`.
Copy link
Contributor

Choose a reason for hiding this comment

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

should ]

Copy link
Contributor Author

@seanchen1991 seanchen1991 Jul 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @DaviRain-Su, I don't believe this is a typo. The ) on the right-hand side of the range indicates that the max value of the range is exclusive such that it doesn't include the value of now + client.max_clock_drift itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh thx your explain, I missunderstand.

romac pushed a commit that referenced this pull request Jul 14, 2023
@seanchen1991 @ancazamfir @romac
Document clock drift parameters in guide (#3420)
2e6b2d4 
* Add clock-drift.md file to guide

* Add section on mis-configuring clock drift

* Update guide/src/advanced/troubleshooting/clock-drift.md

Co-authored-by: Anca Zamfir <[email protected]>
Signed-off-by: Sean Chen <[email protected]>

* Update guide/src/advanced/troubleshooting/clock-drift.md

Co-authored-by: Anca Zamfir <[email protected]>
Signed-off-by: Sean Chen <[email protected]>

* Remove redundant section

* Update guide/src/advanced/troubleshooting/clock-drift.md

Co-authored-by: Anca Zamfir <[email protected]>
Signed-off-by: Sean Chen <[email protected]>

* Update guide/src/advanced/troubleshooting/clock-drift.md

Co-authored-by: Anca Zamfir <[email protected]>
Signed-off-by: Sean Chen <[email protected]>

* Explain what `C` constant represents

* Add reference to forward lunatic attack

---------

Signed-off-by: Sean Chen <[email protected]>
Co-authored-by: Anca Zamfir <[email protected]>
romac added a commit that referenced this pull request Oct 18, 2023
@romac @ancazamfir
@ljoss17 @dependabot @seanchen1991 @puneet2019 @sainoe
Add evidence command for monitoring and reporting misbehavior evide…
db50666 
…nce to counterparty clients (#3456)

* evidence worker PoC for testing

* Cleanup

* Use ibc-proto branch with new provider message

* Add `MsgSubmitIcsConsumerMisbehaviour` domain type from `anca/ics-misbehaviour-handling` branch

* Report misbehavior evidence to all counterparty clients of the misbehaving chain

* Cleanup

* Submit CCV misbehaviour if needed

* Cleanup

* Check if counterparty is CCV provider

* Cleanup

* Add comment

* Set proposer address in header2

* Prepend client updates - work in progress

* Increase the timeout on CI (#3436)

* Improve some messages in `config auto` (#3438)

* Update Data-Requirements.md

Signed-off-by: Romain Ruetschi <[email protected]>

* Update Data-Requirements.md

Signed-off-by: Romain Ruetschi <[email protected]>

* Add CCV chain bootstrap to CI with Neutron and Gaia (#3451)

* Bump serde from 1.0.164 to 1.0.166 (#3458)

* Bump async-trait from 0.1.68 to 0.1.69 (#3459)

* Bump erased-serde from 0.3.25 to 0.3.26 (#3460)

* Document clock drift parameters in guide (#3420)

* Add clock-drift.md file to guide

* Add section on mis-configuring clock drift

* Update guide/src/advanced/troubleshooting/clock-drift.md

Co-authored-by: Anca Zamfir <[email protected]>
Signed-off-by: Sean Chen <[email protected]>

* Update guide/src/advanced/troubleshooting/clock-drift.md

Co-authored-by: Anca Zamfir <[email protected]>
Signed-off-by: Sean Chen <[email protected]>

* Remove redundant section

* Update guide/src/advanced/troubleshooting/clock-drift.md

Co-authored-by: Anca Zamfir <[email protected]>
Signed-off-by: Sean Chen <[email protected]>

* Update guide/src/advanced/troubleshooting/clock-drift.md

Co-authored-by: Anca Zamfir <[email protected]>
Signed-off-by: Sean Chen <[email protected]>

* Explain what `C` constant represents

* Add reference to forward lunatic attack

---------

Signed-off-by: Sean Chen <[email protected]>
Co-authored-by: Anca Zamfir <[email protected]>

* Bump uuid from 1.3.3 to 1.4.0 (#3461)

Bumps [uuid](https://github.com/uuid-rs/uuid) from 1.3.3 to 1.4.0.
- [Release notes](https://github.com/uuid-rs/uuid/releases)
- [Commits](uuid-rs/uuid@1.3.3...1.4.0)

---
updated-dependencies:
- dependency-name: uuid
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Fix wrong sequence number in `MsgTimeoutOnClose` (#3440)

The MsgTimeoutOnClose requires sequence of dstchannel.NextRecv in ordered channels, rather than packet.Sequence.
The code above resolve the sequence for ordered and unordered, just while creating msg it is ignored.

Signed-off-by: Puneet <[email protected]>

* Include client updates for supporting messages when assembling messages to relay from the operational data (#3468)

* Include client updates for supporting messages when assembling messages to relay from the operational data

* Add changelog entry

* Use `max_expected_time_per_block` value for the `max_block_time` (#3467)

* Use max_block_time queried from /genesis

* Clean solution

* Add unclog entry

* Remove serde-with dependency

* `config auto` now generates a config file even when it encounters an error (#3466)

* Stub out code flow

* Stub out code flow

* Change return type of `hermes_cofig` fn

* Define ConfigAutoError type

* Add some printlns

* Change `get_configs` return type

* Change AutoCmd::run

* Get it to compile

* Fix false reporting of missing chain configs

* Change get_data_from_handles

* Get it working

* Remove some debugging code

* Cargo fmt

* Update `get_configs` doc comment

* Update gas price warning in guide

* Cargo fmt

* Build client update for header at common height

* Add forking script

* Check for misbehavior in the last 100 blocks

* Add ICS misbehaviour test

* Add interchain-security to flake.nix

* Use cosmos.nix branch with proper version of interchain-security

* Remove test script

* Update guide templates

* Post-merge fixes

* Update deps

* Use latest ICS protos

* Adapt to change of `MsgSubmitIcsConsumerMisbehaviour::misbehaviour` to `Any` in upstream protos

* Submit both ICS and standard misbehaviour messages to provider chains

* Fix bug where update client message was dropped

* Revert changes of misbehaviour field to Any

* Submit consumer double voting evidence to the provider

* Formatting

* Fix clippy warnings

* Update guide templates

* fix: send evidences with non-empty infraction block header (#3578)

* try to fill infraction header in double voting msg

* reformat

* fix nit

* fmt

* Formatting

* Make infraction block header required

* Stop after submitting double voting evidence to the provider

* Force refresh of account before sending a tx

* Revert refresh on every call

* Remove hermes binary at root

* Send ICS misbehaviour for CCV consumer chain in misbehaviour worker

* Make the evidence command resilient to error, eg. because a client was already frozen

* Improve logging

* Go back to refreshing the account everytime

* Improve CI test script

* Improve logs

* Add `key-name` and `check-past-blocks` arguments to `evidence` command (#3603)

* Add `key-name` and `check-past-blocks` arguments to `evidence` command

* Update templates

* Better logs

* Update nix flake

* Patch check-guide tool with CCV protos

* Do not refresh account everytime

* Fix for zero height

* Update ICS misbehaviour test to use a different wallet for the `evidence` command

* Remove double sign script

---------

Co-authored-by: Romain Ruetschi <[email protected]>

* Fix post-merge conflict

* Better light client attack misbehaviour test

* Improve logs in fishy error cases

* Better error messages when client state is of unexpected type

* Gracefully handle unsupported client types

* WIP: Add double sign test

* Add test for consumer chain double signing

* Gracefully handle unsupported client types in `query connnections`

* Update flake lockfile

* Better logs in evidence command

* Rename jobs and script

* Fix evidence submission (#3612)

* Fix evidence submission by using fix in custom branch tendermint-rs

* Check that evidence command saw the evidence in the block

* Skip submitting evidence if client is already frozen or expired

* Skip frozen clients

* Add more delay in standard misbehaviour test

* Use latest tendermint-rs

* Properly compute the trusted validator set

* Cleanup

* Remove sleeps in double sign test

* Update ibc-proto

* Update ibc-proto to v0.36.0

* Do not panic when unable to find the chain

* Throttle the requests made to the chain while checking past blocks

* Add changelog entries

* Show logs on failure

* Update ibc-proto to v0.36.1

* Update `ibc-proto` to v0.38.0-pre.1 which includes the required CCV protos

* Improve logs

* Check for successful submission in the integration test

* Fix CI script for the case where the client is already frozen

* Submit the ICS misbehaviour for LCA and double signing even if client is
frozen.

* Fix clippy warning

* Avoid sending client updates without the misbehavior

* Include proposer in validator set

* Only submit ICS evidence when provider has a consensus state at the common height

* Update flake

* WIP: Use Rust light client to report evidence

* WIP: Use Go light client to detect misbehaviour

* Issue error when evidence is emitted at forked height

* Detect and report misbehaviour using the CometBFT light client to avoid freezing the client too early

* Add test for when the client is frozen already by the relayer

* Only send the ICS misbehaviour message when the provider client is already frozen

* Better cache frozen status of client

* Never send IBC message if client is already frozen

Co-authored-by: Anca Zamfir <[email protected]>
Signed-off-by: Romain Ruetschi <[email protected]>

* No need to submit client update if provider chain already has common consensus state

* Abort early if there are no messages to send

* Update comment

* Update double sign test

* Skip UpdateClient message if counterparty has consensus state at common height, whether or not it is a provider chain

* Improve logs a little bit

* Small refactor

* Check that counterparty client id matches the CCV client id on the provider

* Create a dummy connection to exercise the provider detection code

---------

Signed-off-by: Romain Ruetschi <[email protected]>
Signed-off-by: Sean Chen <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]>
Signed-off-by: Puneet <[email protected]>
Signed-off-by: Romain Ruetschi <[email protected]>
Co-authored-by: Anca Zamfir <[email protected]>
Co-authored-by: Luca Joss <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Sean Chen <[email protected]>
Co-authored-by: Anca Zamfir <[email protected]>
Co-authored-by: Puneet <[email protected]>
Co-authored-by: Simon Noetzlin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DaviRain-Su DaviRain-Su DaviRain-Su left review comments

@ancazamfir ancazamfir ancazamfir approved these changes

Assignees
No one assigned
Labels
I: documentation Internal: improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Light client "header from the future" with unsynchronized hermes clock
4 participants
@seanchen1991 @josef-widder @ancazamfir @DaviRain-Su