Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Compatibility with whisper v6 #6179

Merged
merged 6 commits into from
Sep 10, 2017
Merged

Compatibility with whisper v6 #6179

merged 6 commits into from
Sep 10, 2017

Conversation

rphmeier
Copy link
Contributor

With extension for multiple topics when connected to parity peers.
Also immediately dispatches messages upon post from RPC now. Rallying is not required in this version.

@rphmeier rphmeier added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 28, 2017
Cargo.lock Outdated
@@ -119,7 +119,7 @@ name = "bigint"
version = "3.0.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
dependencies = [
"byteorder 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)",
"byteorder 1.1.0 (registry+https://github.com/rust-lang/crates.io-index)",
Copy link
Contributor

Choose a reason for hiding this comment

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

do you need 1.1 version update for this pr?
better keep version updates separately just in case to revert easy

Copy link
Contributor Author

@rphmeier rphmeier Aug 2, 2017

Choose a reason for hiding this comment

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

yes, it is needed because 1.0 does not define behavior of read_f64 to mask out sNaN, while 1.1.0 does. I don't see why it would be reverted, as 1.1.0 is fully backwards compatible with 1.0.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, i was talking about cases where dependencies upgraded for no actual reason, which is obviously not a case here!

@rphmeier rphmeier added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Sep 4, 2017
@rphmeier
Copy link
Contributor Author

rphmeier commented Sep 4, 2017

Ready for review again -- version field has been dropped because it's largely useless. Note that all that geth and parity agree upon here is the network protocol. The RPCs are still up in the air.

@gavofyork
Copy link
Contributor

need resolving & doesn't build.

@NikVolf could you take another look once @rphmeier updates?

@@ -496,6 +496,8 @@ pub trait ManageNetwork : Send + Sync {
fn stop_network(&self);
/// Query the current configuration of the network
fn network_config(&self) -> NetworkConfiguration;
/// Get network context for protocol.
fn with_proto_context(&self, proto: ProtocolId, f: &mut FnMut(&NetworkContext));
Copy link
Contributor

Choose a reason for hiding this comment

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

can you think of better way of extending this api which could be interprocess one day?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main problem it solves is creating a pathway for the whisper RPC to post messages to the whisper network. Extending the API to make that possible would require us to come up with a cross-binary network context, which is a desirable feature in general. Changing the signature to fn proto_context(&self, proto: ProtocolId) -> Option<&NetworkContext> could work, but would require pretty large changes to the inner workings.

return Err(Error::InvalidPowReq);
}

// as of byteorder 1.1.0, this is always defined and
Copy link
Contributor

Choose a reason for hiding this comment

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

... and what ?

@@ -339,7 +329,7 @@ impl<P: PoolHandle + 'static, M: Send + Sync + 'static> Whisper for WhisperClien
payload: encrypted,
topics: req.topics.into_iter().map(|x| abridge_topic(&x.into_inner())).collect(),
work: req.priority,
});
}).map_err(|_| whisper_error("Empty topics"))?;
Copy link
Contributor

@NikVolf NikVolf Sep 6, 2017

Choose a reason for hiding this comment

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

Is it ("Empty topics") the only possible reason for Message::create to fail?

Copy link
Contributor Author

@rphmeier rphmeier Sep 8, 2017

Choose a reason for hiding this comment

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

For now at least, why?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just asking, because it's far from obvious in the context

@NikVolf NikVolf added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 6, 2017
@NikVolf NikVolf added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Sep 8, 2017
@gavofyork gavofyork merged commit 375668b into master Sep 10, 2017
@gavofyork gavofyork deleted the whisper-v6 branch September 10, 2017 16:02
@5chdn 5chdn added the B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. label Sep 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants