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

Sim2h WireMessage Receipts #2120

Merged
merged 35 commits into from Feb 24, 2020
Merged

Sim2h WireMessage Receipts #2120

merged 35 commits into from Feb 24, 2020

Conversation

@lucksus
Copy link
Member

lucksus commented Feb 18, 2020

PR summary

This intends to make the channel between nodes and the Sim2h server resilient against data/connection loss on the level of Lib3hMessages. (in the direction: Sim2hWorker -> Sim2h server)

This resilience is actually an assumption that code in core holds, but that wasn't met by the Sim2h/Sim2hWorker implementation yet. A connection dropped just after a message was sent would leave the disconnected node without track of that lost message. It would continue to function after a reconnect, but the message sent before would be lost, if it wasn't received by the server before the disconnect.

With this PR we add a receipt message type WireMessage::Ack(u64) that the Sim2h server sends back, including a hash of the processed message. The Sim2hWorker now only sends one message at a time and waits for the receipt before sending the next one. If a receipt is not received within 10 seconds, it will resend the message.

testing/benchmarking notes

( if any manual testing or benchmarking was/should be done, add notes and/or screenshots here )

followups

  • Apply receipts also for the other direction: Sim2h -> Sim2hWorker

changelog

  • if this is a code change that effects some consumer (e.g. zome developers) of holochain core, then it has been added to our between-release changelog with the format
- summary of change [PR#1234](https://github.com/holochain/holochain-rust/pull/1234)

documentation

@neonphog

This comment has been minimized.

Copy link
Contributor

neonphog commented Feb 18, 2020

@neonphog

This comment has been minimized.

Copy link
Contributor

neonphog commented Feb 18, 2020

Similarly - if you want the hashing to be consistent, I believe the problem lies in the level at which you are hashing.

On the worker side, if you hash the already serialized binary data (optimally changing the outgoing queue to have already serialized data and respective hashes), then on the server side, do the hashing before deserializing the data, then obviously they should match.

Copy link
Contributor

neonphog left a comment

putting a request changes here as a reminder to bump the protocol version - since old clients will be incompatible with new servers & vice-versa

lucksus and others added 8 commits Feb 19, 2020
As we just found out, the serialization of WireMessages is not deterministic because of a hidden HashMap. The code fixed here assumed `message.into::<String/Opaque>()` to always return the same result since it's doing it once for the payload sent, and once for the calculation of the signature. The result would be (and almost certainly was) wrong signatures which make Sim2h server ignore the message.
Copy link
Member Author

lucksus left a comment

notes to self

crates/net/src/sim2h_worker.rs Outdated Show resolved Hide resolved
crates/net/src/sim2h_worker.rs Outdated Show resolved Hide resolved
crates/net/src/sim2h_worker.rs Outdated Show resolved Hide resolved
crates/sim2h/src/wire_message.rs Outdated Show resolved Hide resolved
crates/sim2h/src/wire_message.rs Outdated Show resolved Hide resolved
Copy link
Member

zippy left a comment

This seems to break the sim2h-client command from the cli:
It seems to work but the status value isn't printed.

$ hc sim2h-client -u ws://sim2h-test.holo.host:9001 -m status
url: ws://sim2h-test.holo.host:9001
message: status
looking up: sim2h-test.holo.host
resolved to: 147.75.54.129
connecting to: ws://147.75.54.129:9001/
Generated agent id: HcSCI837pTNb897y9bhY4F479NkxU3frnMrqj358hKkzhpie9I4m5PdTXquzmpr
Await successfull
Sending wire message to sim2h: Status
Ack(15773591541385231263)
lucksus added 5 commits Feb 20, 2020
… for deterministic serialization.
@lucksus

This comment has been minimized.

Copy link
Member Author

lucksus commented Feb 20, 2020

@neonphog, @zippy, implemented both your change requests.

@zippy
zippy approved these changes Feb 20, 2020
Copy link
Member

zippy left a comment

I'm good with merging this, will defer to you about the hashing choice.

@lucksus

This comment has been minimized.

Copy link
Member Author

lucksus commented Feb 24, 2020

@neonphog, implemented those suggestions as well :)

@zippy zippy requested a review from neonphog Feb 24, 2020
@@ -71,6 +74,7 @@ lazy_static! {

/// if we can't acquire a lock in 20 seconds, panic!
const MAX_LOCK_TIMEOUT: u64 = 20000;
pub const RECEIPT_HASH_SEED: u64 = 0;

This comment has been minimized.

Copy link
@neonphog
Copy link
Contributor

neonphog left a comment

sweet, this looks awesome!

@zippy zippy merged commit 54d794c into develop Feb 24, 2020
11 of 15 checks passed
11 of 15 checks passed
ci/circleci: docker-build-circle-build CircleCI is running your tests
Details
ci/circleci: docker-build-circle-cli-tests CircleCI is running your tests
Details
ci/circleci: docker-build-circle-sim2h CircleCI is running your tests
Details
ci/circleci: docker-build-trycp-server CircleCI is running your tests
Details
ci/circleci: app-spec-tests-sim2h Your tests passed on CircleCI!
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: cli-tests Your tests passed on CircleCI!
Details
ci/circleci: docker-build-circle-fmt Your tests passed on CircleCI!
Details
ci/circleci: docker-build-circle-wasm-conductor-tests Your tests passed on CircleCI!
Details
ci/circleci: docker-build-latest Your tests passed on CircleCI!
Details
ci/circleci: docker-build-minimal Your tests passed on CircleCI!
Details
ci/circleci: docker-build-sim2h-server Your tests passed on CircleCI!
Details
ci/circleci: fmt Your tests passed on CircleCI!
Details
ci/circleci: stress-tests-sim2h Your tests passed on CircleCI!
Details
ci/circleci: wasm-conductor-tests Your tests passed on CircleCI!
Details
@zippy zippy deleted the wire-message-receipts branch Feb 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.