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

Use CometBFT 0.37 compatibility mode for unsupported Tendermint versions (eg. Tendermint 0.35 and 0.36) #3404

Merged
merged 6 commits into from
Jun 8, 2023
Merged

Use CometBFT 0.37 compatibility mode for unsupported Tendermint versions (eg. Tendermint 0.35 and 0.36) #3404

merged 6 commits into from
Jun 8, 2023

Conversation

seanchen1991
Copy link
Contributor

Closes: #XXX

Description

Downgrades the Tendermint version check to a warning instead of an error.

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).

@romac
Fix typo
8458b7b 
Signed-off-by: Romain Ruetschi <romain.ruetschi@gmail.com>
@romac romac changed the title Patch tendermint 0.35 compatibility with 0.37 Use CometBFT 0.37 compatibility mode for unsupported Tendermint versions (eg. Tendermint 0.35 and 0.36) Jun 8, 2023
@romac romac merged commit dddd7fa into master Jun 8, 2023
@romac romac deleted the luca_joss/patch-tendermint-35-compat-with-37 branch June 8, 2023 09:40
@epanchee
Copy link

epanchee commented Jun 12, 2023 •
edited
Loading

Hey.
Wondering does it work with Sei forked tendermint version? They currently name it 0.35.0-unreleased.
Thanks to this PR Hermes new version will be able to relay to/from Sei.

This is their diff: sei-protocol/sei-tendermint@46d0a59...v0.2.21

Not sure which version they forked from.

Update:
Can confirm that Sei's tendermint version is not compatible with CompatMode::V0_37:

2023-06-12T11:45:55.541879Z  WARN ThreadId(28) spawn:chain{chain=...}:client{client=07-tendermint-10}:connection{connection=connection-4}:channel{channel=channel-8}:worker.packet.cmd{src_chain=.. src_port=transfer src_channel=channel-8 dst_chain=pisco-1}:schedule_packet_clearing{height=Some(Height { revision: 1, height: 4441592 })}:relay_pending_packets{height=Some(Height { revision: 1, height: 4441591 })}:build_packet_ack_msgs{query_height=1-4441591}: encountered query failure while pulling packet data: RPC error to endpoint [...]

Caused by:
   0: serde parse error
   1: invalid length 0, expected struct EvidenceList with 1 element at line 1 column 1346

@seanchen1991
Copy link
Contributor Author

Wondering does it work with Sei forked tendermint version?

Just to be clear, your concern is that this branch of Hermes doesn't work with Sei's forked version of Tendermint, right? It's not to do with whether this branch is able to relay to/from Sei chains.

@epanchee
Copy link

Wondering does it work with Sei forked tendermint version?

Just to be clear, your concern is that this branch of Hermes doesn't work with Sei's forked version of Tendermint, right? It's not to do with whether this branch is able to relay to/from Sei chains.

Yes. Built Hermes from master branch and saw the error above

romac added a commit that referenced this pull request Jun 13, 2023
@ljoss17 @romac @seanchen1991
Add mechanism to remove idle workers (#3370)
c5a5ce7 
* Add mechanism to remove idle workers

* Add integration test for worker cleanup mechanism

* Add idle channel worker integration test

* Fix CI job

* Improve worker cleanup test logs. Add changelog entry

* Update crates/relayer/src/worker/handle.rs

Co-authored-by: Romain Ruetschi <romain@informal.systems>
Signed-off-by: Luca Joss <43531661+ljoss17@users.noreply.github.com>

* Update crates/relayer/src/worker/map.rs

Co-authored-by: Romain Ruetschi <romain@informal.systems>
Signed-off-by: Luca Joss <43531661+ljoss17@users.noreply.github.com>

* Update crates/relayer/src/worker/map.rs

Co-authored-by: Romain Ruetschi <romain@informal.systems>
Signed-off-by: Luca Joss <43531661+ljoss17@users.noreply.github.com>

* Apply github suggestions and fix error

* Reduce idle worker cleanup frequency

* Increase time between idle worker cleanup checks

* Use CometBFT 0.37 compatibility mode for unsupported Tendermint versions (eg. Tendermint 0.35 and 0.36) (#3404)

* Use CompatMode::V0_34 when unsupported Tendermint version is detected

* Use CompatMode::V0_37 when unsupported Tendermint version is detected

* Fix warning when using fallback v0.37 CompatMode for Tendermint

* Fix typo

Signed-off-by: Romain Ruetschi <romain.ruetschi@gmail.com>

---------

Signed-off-by: Romain Ruetschi <romain.ruetschi@gmail.com>
Co-authored-by: Luca Joss <luca@informal.systems>
Co-authored-by: Romain Ruetschi <romain@informal.systems>

* Small cleanup

* Fix issue which spawned 2 supervisors in clean_workers tests

* Remove assert which can cause 'test_clean_channel_workers' to fail wrongly

* Update .changelog/unreleased/improvements/ibc-relayer/10-cleanup-idle-workers.md

Signed-off-by: Romain Ruetschi <romain.ruetschi@gmail.com>

---------

Signed-off-by: Luca Joss <43531661+ljoss17@users.noreply.github.com>
Signed-off-by: Romain Ruetschi <romain.ruetschi@gmail.com>
Co-authored-by: Romain Ruetschi <romain@informal.systems>
Co-authored-by: Sean Chen <seanchen11235@gmail.com>
romac added a commit that referenced this pull request Jun 14, 2023
@ljoss17 @romac
Disable scanning when client, channel and connection workers are disa…
81317eb 
…bled and `clear_on_start = false` (#3365)

* Only scan chains if 'clear_on_start=true'

* Add supervisor tests when scanning is enabled and disabled

* Add fees to ibc-transfer, required by Osmosis

* Improve supervisor tests with and without chain scan

* Add packet clearing test when scan is disabled

* Fix clearing without scan test

* Only scan when clear_on_start option is enabled, or client refresh or misbehavior workers are enabled, or the --full-scan argument is enabled

* Update no scan tests

* Disable channel and connection workers as well

* Add mechanism to remove idle workers (#3370)

* Add mechanism to remove idle workers

* Add integration test for worker cleanup mechanism

* Add idle channel worker integration test

* Fix CI job

* Improve worker cleanup test logs. Add changelog entry

* Update crates/relayer/src/worker/handle.rs

Co-authored-by: Romain Ruetschi <romain@informal.systems>
Signed-off-by: Luca Joss <43531661+ljoss17@users.noreply.github.com>

* Update crates/relayer/src/worker/map.rs

Co-authored-by: Romain Ruetschi <romain@informal.systems>
Signed-off-by: Luca Joss <43531661+ljoss17@users.noreply.github.com>

* Update crates/relayer/src/worker/map.rs

Co-authored-by: Romain Ruetschi <romain@informal.systems>
Signed-off-by: Luca Joss <43531661+ljoss17@users.noreply.github.com>

* Apply github suggestions and fix error

* Reduce idle worker cleanup frequency

* Increase time between idle worker cleanup checks

* Use CometBFT 0.37 compatibility mode for unsupported Tendermint versions (eg. Tendermint 0.35 and 0.36) (#3404)

* Use CompatMode::V0_34 when unsupported Tendermint version is detected

* Use CompatMode::V0_37 when unsupported Tendermint version is detected

* Fix warning when using fallback v0.37 CompatMode for Tendermint

* Fix typo

* Small cleanup

* Fix issue which spawned 2 supervisors in clean_workers tests

* Remove assert which can cause 'test_clean_channel_workers' to fail wrongly

* Update .changelog/unreleased/improvements/ibc-relayer/10-cleanup-idle-workers.md

* Add documentation on no scan startup

* Add changelog entry

---------

Co-authored-by: Romain Ruetschi <romain@informal.systems>
Co-authored-by: Sean Chen <seasn@informal.systems>
Co-authored-by: Luca Joss <luca@informal.systems>
@romac
Copy link
Member

romac commented Jun 26, 2023

Update: Can confirm that Sei's tendermint version is not compatible with CompatMode::V0_37:

2023-06-12T11:45:55.541879Z  WARN ThreadId(28) spawn:chain{chain=...}:client{client=07-tendermint-10}:connection{connection=connection-4}:channel{channel=channel-8}:worker.packet.cmd{src_chain=.. src_port=transfer src_channel=channel-8 dst_chain=pisco-1}:schedule_packet_clearing{height=Some(Height { revision: 1, height: 4441592 })}:relay_pending_packets{height=Some(Height { revision: 1, height: 4441591 })}:build_packet_ack_msgs{query_height=1-4441591}: encountered query failure while pulling packet data: RPC error to endpoint [...]

Caused by:
   0: serde parse error
   1: invalid length 0, expected struct EvidenceList with 1 element at line 1 column 1346

@epanchee Can you please re-run Hermes with the flag --debug=rpc? (eg. hermes --debug=rpc start)

This will output the JSON-RPC response that failed to parse right before the error. Can you then paste this here in this thread?

@epanchee
Copy link

@romac I ran with --debug=rpc and it makes sense now. I was using public RPC which has some throttle limit.

2023-06-27T11:57:53.646187Z DEBUG ThreadId(30) event_source.rpc{chain.id=atlantic-2}: Incoming response: <html>
<head><title>429 Too Many Requests</title></head>
<body>
<center><h1>429 Too Many Requests</h1></center>
<hr><center>nginx/1.18.0 (Ubuntu)</center>
</body>
</html>

2023-06-27T11:57:53.646305Z ERROR ThreadId(30) event_source.rpc{chain.id=atlantic-2}: event source encountered an error: RPC error: serde parse error: expected value at line 1 column 1

Switching to private Astroport infra solved this issue.
I'm sorry for a false alaram.

@romac
Copy link
Member

romac commented Jun 27, 2023

I'm sorry for a false alaram.

No worries!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romac romac romac approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@seanchen1991 @epanchee @romac @ljoss17