Propagate JSON-RPC errors through the Rust subscription #929
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #929 +/- ##
======================================
Coverage 70.4% 70.5%
======================================
Files 203 203
Lines 16285 16285
======================================
+ Hits 11479 11483 +4
+ Misses 4806 4802 -4
Continue to review full report at Codecov.
|
8 tasks
romac
commented
Jul 16, 2021
romac
commented
Jul 16, 2021
| // We ignore incoming messages whose ID we don't recognize (could be | ||
| // relating to a fire-and-forget unsubscribe request - see the | ||
| // publish_event() method below). | ||
| Ok(()) | ||
| } | ||
|
|
||
| async fn publish_error(&mut self, id: &str, err: Error) { |
There was a problem hiding this comment.
There's a bit of duplicated code between publish_error and publish_event, we should probably clean this up
romac
commented
Jul 16, 2021
romac
commented
Jul 16, 2021
romac
commented
Jul 16, 2021
.changelog/unreleased/improvements/932-propagate-json-rpc-errors.md
Outdated
Show resolved
Hide resolved
…rs.md Co-authored-by: Thane Thomson <thane@informal.systems>
Co-authored-by: Thane Thomson <thane@informal.systems>
* Specify default value for `tendermint::block::Size` * Add .changelog entry * Add test for `time_iota_ms` default (#933) * Bump supported rpc-probe version of Tendermint to v0.34.9 * Regenerate kvstore fixtures using Tendermint v0.34.9 * Update kvstore tests to test #931 * Expose Size::default_time_iota_ms to access from integration test * Implement test for #931 Co-authored-by: Thane Thomson <thane@informal.systems>
There was a problem hiding this comment.
Final minor nit, then I'd say we're good to go here!
I think there should be some follow-up work, which I can look into: to avoid all the allow(dead_code) directives, we need to fix the imports according to the features enabled. We should probably find a way to avoid importing the subscription router unless a subscription-enabled transport is required (e.g. the websocket feature).
thanethomson
approved these changes
Jul 19, 2021
romac
added a commit
to informalsystems/hermes
that referenced
this pull request
Jul 21, 2021
… by Tendermint (#1205) After some investigation, the culprit for #1196 seems to be that Tendermint is closing the WebSocket connection over which we listen for IBC events whenever more than 100 txs are included in a single block [0], as we are not able to pull the events fast enough over the WebSocket connection to avoid completely filling the event buffer in Tendermint (which currently has a hard-coded capacity of 100 events, hence the issue). We never noticed this previously since this problem only appears in practice with a high-enough commit/propose timeout (to allow enough txs to be included in a single block), and we were testing with a lower value for the timeouts. Now that we landed some changes in tendermint-rs [1] which allow us to notice the connection being closed, this PR makes use of this to resubscribe to the events and trigger a packet clear whenever we notice the connection being closed under our feet. [0] tendermint/tendermint#6729 [1] informalsystems/tendermint-rs#929 --- * Propagate JSON-RPC errors through the Rust subscription * Use tendermint-rs branch with both fixes * Fix compilation issue in tests * Clear pending packets when event subscription is cancelled * Temp: Update one-chain script to use 10s commit timeout * Use tendermint-rs master * Update Cargo.lock * Update changelog * Update lockfile * Increase delay before checking for relaying result in e2e tests * Add comment explaining who the RPC error is propagated to * Improve event monitor logs * Reset `timeout_commit` and `timeout_propose` to 1s
hu55a1n1
pushed a commit
to hu55a1n1/hermes
that referenced
this pull request
Sep 13, 2022
… by Tendermint (informalsystems#1205) After some investigation, the culprit for informalsystems#1196 seems to be that Tendermint is closing the WebSocket connection over which we listen for IBC events whenever more than 100 txs are included in a single block [0], as we are not able to pull the events fast enough over the WebSocket connection to avoid completely filling the event buffer in Tendermint (which currently has a hard-coded capacity of 100 events, hence the issue). We never noticed this previously since this problem only appears in practice with a high-enough commit/propose timeout (to allow enough txs to be included in a single block), and we were testing with a lower value for the timeouts. Now that we landed some changes in tendermint-rs [1] which allow us to notice the connection being closed, this PR makes use of this to resubscribe to the events and trigger a packet clear whenever we notice the connection being closed under our feet. [0] tendermint/tendermint#6729 [1] informalsystems/tendermint-rs#929 --- * Propagate JSON-RPC errors through the Rust subscription * Use tendermint-rs branch with both fixes * Fix compilation issue in tests * Clear pending packets when event subscription is cancelled * Temp: Update one-chain script to use 10s commit timeout * Use tendermint-rs master * Update Cargo.lock * Update changelog * Update lockfile * Increase delay before checking for relaying result in e2e tests * Add comment explaining who the RPC error is propagated to * Improve event monitor logs * Reset `timeout_commit` and `timeout_propose` to 1s
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Close: #932
Emit JSON-RPC errors sent through the WS subscription through the
Subscriptionchannel, so that the Hermes event monitor can detect those (eg. when the WS subscription is closed by the Tendermint node) and attempt to resubscribe..changelog/