This repository has been archived by the owner on Nov 6, 2020. It is now read-only.
Problem: AttachedProtocols don't get registered #7610
Merged
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
I was investigating issues I am having with Whisper support. I've enabled Whisper on a custom test network and inserted traces into Whisper handler implementation (Network<T> and NetworkProtocolHandler for Network<T>) and I noticed that the handler was never invoked. After further research on this matter, I found out that AttachedProtocol's register function does nothing: https://github.com/paritytech/parity/blob/master/sync/src/api.rs#L172 but there was an implementation originally: 99075ad#diff-5212acb6bcea60e9804ba7b50f6fe6ec and it did the actual expected logic of registering the protocol in the NetworkService. However, as of 16d84f8#diff-5212acb6bcea60e9804ba7b50f6fe6ec ("finished removing ipc") this implementation is gone and only the no-op function is left. Which leads me to a conclusion that in fact Whisper's handler never gets registered in the service and therefore two nodes won't communicate using it. Solution: Resurrect original non-empty `AttachedProtocols.register` implementation Resolves openethereum#7566
|
It looks like @yrashk signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
5chdn
added
A0-pleasereview 🤓
debris
added
A8-looksgood 🦄
|
thanks! nice catch! |
debris
pushed a commit
that referenced
this pull request
Jan 22, 2018
I was investigating issues I am having with Whisper support. I've enabled Whisper on a custom test network and inserted traces into Whisper handler implementation (Network<T> and NetworkProtocolHandler for Network<T>) and I noticed that the handler was never invoked. After further research on this matter, I found out that AttachedProtocol's register function does nothing: https://github.com/paritytech/parity/blob/master/sync/src/api.rs#L172 but there was an implementation originally: 99075ad#diff-5212acb6bcea60e9804ba7b50f6fe6ec and it did the actual expected logic of registering the protocol in the NetworkService. However, as of 16d84f8#diff-5212acb6bcea60e9804ba7b50f6fe6ec ("finished removing ipc") this implementation is gone and only the no-op function is left. Which leads me to a conclusion that in fact Whisper's handler never gets registered in the service and therefore two nodes won't communicate using it. Solution: Resurrect original non-empty `AttachedProtocols.register` implementation Resolves #7566
Merged
5chdn
pushed a commit
that referenced
this pull request
Jan 23, 2018
* Improve handling of RocksDB corruption (#7630) * kvdb-rocksdb: update rust-rocksdb version * kvdb-rocksdb: mark corruptions and attempt repair on db open * kvdb-rocksdb: better corruption detection on open * kvdb-rocksdb: add corruption_file_name const * kvdb-rocksdb: rename mark_corruption to check_for_corruption * Hardening of CSP (#7621) * Fixed delegatecall's from/to (#7568) * Fixed delegatecall's from/to, closes #7166 * added tests for delegatecall traces, #7167 * Light client RPCs (#7603) * Implement registrar. * Implement eth_getCode * Don't wait for providers. * Don't wait for providers. * Fix linting and wasm tests. * Problem: AttachedProtocols don't get registered (#7610) I was investigating issues I am having with Whisper support. I've enabled Whisper on a custom test network and inserted traces into Whisper handler implementation (Network<T> and NetworkProtocolHandler for Network<T>) and I noticed that the handler was never invoked. After further research on this matter, I found out that AttachedProtocol's register function does nothing: https://github.com/paritytech/parity/blob/master/sync/src/api.rs#L172 but there was an implementation originally: 99075ad#diff-5212acb6bcea60e9804ba7b50f6fe6ec and it did the actual expected logic of registering the protocol in the NetworkService. However, as of 16d84f8#diff-5212acb6bcea60e9804ba7b50f6fe6ec ("finished removing ipc") this implementation is gone and only the no-op function is left. Which leads me to a conclusion that in fact Whisper's handler never gets registered in the service and therefore two nodes won't communicate using it. Solution: Resurrect original non-empty `AttachedProtocols.register` implementation Resolves #7566 * Fix Temporarily Invalid blocks handling (#7613) * Handle temporarily invalid blocks in sync. * Fix tests.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
I was investigating issues I am having with Whisper support. I've
enabled Whisper on a custom test network and inserted traces into
Whisper handler implementation (Network and NetworkProtocolHandler
for Network) and I noticed that the handler was never invoked.
After further research on this matter, I found out that
AttachedProtocol's register function does nothing:
https://github.com/paritytech/parity/blob/master/sync/src/api.rs#L172
but there was an implementation originally:
99075ad#diff-5212acb6bcea60e9804ba7b50f6fe6ec and it did the actual
expected logic of registering the protocol in the NetworkService.
However, as of 16d84f8#diff-5212acb6bcea60e9804ba7b50f6fe6ec ("finished
removing ipc") this implementation is gone and only the no-op function
is left.
Which leads me to a conclusion that in fact Whisper's handler never gets
registered in the service and therefore two nodes won't communicate
using it.
Solution: Resurrect original non-empty
AttachedProtocols.registerimplementation
Resolves #7566