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

Do not merge - protocol change discussion #1333

Closed
wants to merge 2 commits into from
Closed

Conversation

whyrusleeping
Copy link
Member

This branch contains the code from #1065 as well as the change from #1304. without this change, the times for an ipfs ping were at least double that of a unix ping, with these changes, they are quite close.

whyrusleeping@csg-gate:~$ ipfs swarm connect /ip4/192.241.194.199/tcp/4001/ipfs/QmTmyCBwHEu7zHFemLvw18HQBnK3jupWfVPg86VGPZ1jLS
connect QmTmyCBwHEu7zHFemLvw18HQBnK3jupWfVPg86VGPZ1jLS success
whyrusleeping@csg-gate:~$ ipfs ping QmTmyCBwHEu7zHFemLvw18HQBnK3jupWfVPg86VGPZ1jLS
PING QmTmyCBwHEu7zHFemLvw18HQBnK3jupWfVPg86VGPZ1jLS.
Pong received: time=31.90 ms
Pong received: time=31.88 ms
Pong received: time=31.13 ms
Pong received: time=31.77 ms
Pong received: time=31.87 ms
Pong received: time=31.18 ms
Pong received: time=31.31 ms
Pong received: time=31.13 ms
Pong received: time=32.20 ms
Pong received: time=31.93 ms
Average latency: 31.63ms
whyrusleeping@csg-gate:~$ ping 192.241.194.199
PING 192.241.194.199 (192.241.194.199) 56(84) bytes of data.
64 bytes from 192.241.194.199: icmp_req=1 ttl=49 time=28.4 ms
64 bytes from 192.241.194.199: icmp_req=2 ttl=49 time=28.4 ms
64 bytes from 192.241.194.199: icmp_req=3 ttl=49 time=28.3 ms
64 bytes from 192.241.194.199: icmp_req=4 ttl=49 time=28.5 ms
64 bytes from 192.241.194.199: icmp_req=5 ttl=49 time=28.5 ms
64 bytes from 192.241.194.199: icmp_req=6 ttl=49 time=28.3 ms
64 bytes from 192.241.194.199: icmp_req=7 ttl=49 time=28.3 ms
64 bytes from 192.241.194.199: icmp_req=8 ttl=49 time=28.5 ms
64 bytes from 192.241.194.199: icmp_req=9 ttl=49 time=28.3 ms
64 bytes from 192.241.194.199: icmp_req=10 ttl=49 time=28.3 ms
64 bytes from 192.241.194.199: icmp_req=11 ttl=49 time=28.3 ms
64 bytes from 192.241.194.199: icmp_req=12 ttl=49 time=28.5 ms
^C
--- 192.241.194.199 ping statistics ---
12 packets transmitted, 12 received, 0% packet loss, time 11016ms
rtt min/avg/max/mdev = 28.300/28.416/28.577/0.180 ms

jbenet and others added 2 commits June 2, 2015 15:26
There was doublewrapping with an unneeded msgio. given that we
use a stream muxer now, msgio is only needed by secureConn -- to
signal the boundaries of an encrypted / mac-ed ciphertext.

Side note: i think including the varint length in the clear is
actually a bad idea that can be exploited by an attacker. it should
be encrypted, too. (TODO)
@whyrusleeping whyrusleeping added the status/in-progress In progress label Jun 4, 2015
@whyrusleeping
Copy link
Member Author

cc @jbenet

return err
}

return s.buf.Flush()
Copy link
Member

Choose a reason for hiding this comment

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

ok if the flush happens here that's fine (maybe make a note that the flush must be there or else the write wont write-through to the underlying transport?)

@jbenet
Copy link
Member

jbenet commented Jun 4, 2015

i think this is fine as long that msgio flush happens there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants