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

Hermes retrying mechanism for packet clearing #1146

Merged
merged 85 commits into from
Jul 7, 2021
Merged

Conversation

adizere
Copy link
Member

@adizere adizere commented Jul 2, 2021

Part of the work towards closing #1124

Description

Changes in this PR:

  • enables packet clearing to be retried every X blocks, parametrized in config.toml
  • makes the rpc_timeout from config file reflect in the timeout for "waiting on block commits"
  • adds an explanation to the TxNoConfirmation error message

For contributor use:

  • Updated the Unreleased section of CHANGELOG.md with the issue.
  • If applicable: Unit tests written, added test to CI.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments.
  • Re-reviewed Files changed in the Github PR explorer.

ancazamfir and others added 30 commits May 19, 2021 01:33
* Improve config load message

* Raised log level to error. Log link to config example in the guide.

* Changelog

Co-authored-by: Adi Seredinschi <adi@informal.systems>
Revert commit 054ff2a partly.
Reason: printing of simulation error is handled separately
in parallel PR #1137.
@adizere adizere marked this pull request as ready for review July 5, 2021 12:51
Copy link
Member Author

@adizere adizere left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -412,7 +412,7 @@ impl RelayPath {

/// Should not run more than once per execution.
pub fn clear_packets(&mut self, above_height: Height) -> Result<(), LinkError> {
if self.clear_packets {
if self.clear_packets || above_height.revision_height % 100 == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

100 should be configurable

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what's the best way to make this configurable. Do you suggest we put an option in config.toml? Seems clunky to me.

@adizere adizere merged commit d9eba70 into master Jul 7, 2021
@adizere adizere deleted the anca/no_confirmation branch July 7, 2021 16:39
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
* temp change for ft-transfer to send one msg/Tx

* event monitor: Bulk events from all transactions included in a block

* Update changelog

* temp change for ft-transfer to send one msg/Tx

* Optimize spawning of workers - draft

* Add back check to start workers only if channel is open

* Cleanup

* Check connection state

* temp change for ft-transfer to send one msg/Tx

* Improve config loading message (informalsystems#933)

* Improve config load message

* Raised log level to error. Log link to config example in the guide.

* Changelog

Co-authored-by: Adi Seredinschi <adi@informal.systems>

* Migration to tx sync

* Add Tx simulate

* Add adjustment to the tx simulate result, some cleanup

* Nitpick in CosmosSdkChain::key_and_bytes

* Small cleanup

* Remove duplicate send_msgs method

* Cleanup config file

* Fix typo after refactoring

* Fix `query packet tx` description

* Rework `wait_for_block_commits` to use the `retry` crate

* Compute tx fee based on gas from tx simulate and gas price

* Re-add missing error type

* Combine `fee_denom` and `gas_price` into `gas_price` config option

* Add tests for `mul_ceil`

* Fix config serialization

* Remove `fee_amount` config property

* Update changelog

* Avoid op data regeneration if retries exhausted.

* Increase the number of retries while checking Tx is included in a block.

* Move `query packet tx` to `query tx events`

* better error msgs

* Add Display instance for GasPrice

* Fix default gas price denomination

* Improve some debug messages

* Rename `gas_price.amount` to `gase_price.price`

* Add configurable fee adjustment to make it more likely gas estimate ends up being enough

* Add Tx hash to ChainErr

* Fix config files

* Masked tonic::Code::NotFound result for query_client_connections.

* Modified cfg option from gas to max_gas

* Consistent trust_threshold in default config.toml

* Revert guide updates

* Nit: Imports for query.rs

* Print info message when Hermes starts

* Implement basic filtering based on channel identifiers

* Add per chain filters, only channel based filtering support

* Fix gas adjustement to be percentage on top of computed gas

* Attempt to fix gas_limit

* Fix chain spawn unrwap for QueryUnreceivedPacketsCmd

* Retry on no confirmation

* Print simulation errors. Trim clear packets output.

* Retry timeout parametrized parametrized by rpc_timeout

* Bring back NoConfirmation error and don't retry.

Trigger packet clearing every 100 blocks.

* Don't log during clearing packets unless there is work to do.

* Revert printing of simulation error.

Revert commit 054ff2a partly.
Reason: printing of simulation error is handled separately
in parallel PR informalsystems#1137.

* Cleanup

* Consolidated output for packet clearing methods

* Cleaner code around clear_packets method

* Undid -k option for packet-recv

* More context loaded into TxNoConfirmation

* More context when uni path/link worker encounters error

* Improve tx no confirmation error message

* Make clearing packets interval configurable

* Update changelog

* Update CI config

* Better defaults

* Remove backup file

* Formatting

Co-authored-by: Anca Zamfir <zamfiranca@gmail.com>
Co-authored-by: Romain Ruetschi <romain@informal.systems>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E: gravity External: related to Gravity DEX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants