Negotiate Lower Bandwidth / Higher Density Packets for Peers #428

Merged
erinn merged 5 commits from fastercwtch into master 2022-01-26 20:02:50 +00:00
Owner

Backwards compatible version negotiation for PeerApp which allows newer version to use a much higher density packet format.

Backwards compatible version negotiation for PeerApp which allows newer version to use a much higher density packet format.
dan was assigned by sarah 2022-01-25 20:26:27 +00:00
erinn was assigned by sarah 2022-01-25 20:26:27 +00:00
Member
Drone Build Status: success https://build.openprivacy.ca/cwtch.im/cwtch/690
dan requested changes 2022-01-25 23:06:39 +00:00
@ -42,6 +47,7 @@ func (pa *PeerApp) NewInstance() tapir.Application {
newApp.OnAuth = pa.OnAuth
newApp.OnClose = pa.OnClose
newApp.OnConnecting = pa.OnConnecting
newApp.version.Store(0x01)
Owner

constant?

constant?
sarah marked this conversation as resolved
@ -62,0 +76,4 @@
pa.SendMessage(model2.PeerMessage{
ID: event.ContextVersion,
Context: event.ContextGetVal,
Data: []byte{0x02},
Owner

constant?

constant?
sarah marked this conversation as resolved
@ -81,0 +104,4 @@
err = json.Unmarshal(message, &packet)
} else if pa.version.Load() == 0x02 {
// assume the encoding is invalid
Owner

this is a big chunk, might be best moved to a function?

this is a big chunk, might be best moved to a function?
sarah marked this conversation as resolved
@ -81,0 +108,4 @@
err = errors.New("invalid message")
// find the identifier prefix
idTerminator := bytes.IndexByte(message, '|')
Owner

I'm a little nervous mixing unspecified Ids, event IDs are strings, even tho they should be populated by int -> strings only? its not type specified anywhere so someone could make it binary or strings and either could generate a '|', contexts are strings that right now use '.' as seperators...
we have a lot of string manipulation based on not well documented seperators and we're mixing it with binary data.

could we use Pascal style strings here? neither id nor context should exceed 256 bytes so just have each lead with a byte of length to parse?

I'm a little nervous mixing unspecified Ids, event IDs are strings, even tho they should be populated by int -> strings only? its not type specified anywhere so someone could make it binary or strings and either could generate a '|', contexts are strings that right now use '.' as seperators... we have a lot of string manipulation based on not well documented seperators and we're mixing it with binary data. could we use Pascal style strings here? neither id nor context should exceed 256 bytes so just have each lead with a byte of length to parse?
sarah marked this conversation as resolved
@ -84,0 +148,4 @@
// can remove this check all together)
if packet.ID == event.ContextVersion {
if pa.version.Load() == 0x01 && len(packet.Data) == 1 && packet.Data[0] == 0x02 {
log.Debugf("switching to 2")
Owner

switching what to 2 :P

switching what to 2 :P
sarah marked this conversation as resolved
@ -95,0 +169,4 @@
var err error
if pa.version.Load() == 0x02 {
// treat data as a pre-serialized string, not as a byte array (which will be base64 encoded and bloat the packet size)
Owner

did the json encode to a base64?

did the json encode to a base64?
Author
Owner

yes

yes
dan marked this conversation as resolved
@ -95,0 +170,4 @@
if pa.version.Load() == 0x02 {
// treat data as a pre-serialized string, not as a byte array (which will be base64 encoded and bloat the packet size)
serialized = append(append([]byte(message.ID+"|"), []byte(message.Context+"|")...), message.Data...)
Owner

this is a new message format, we probably want to document it somewhere. as much as this works as a one liner, we might want to make it a function anyways just for clarity

this is a new message format, we probably want to document it somewhere. as much as this works as a one liner, we might want to make it a function anyways just for clarity
sarah marked this conversation as resolved
sarah force-pushed fastercwtch from 267283c3a3 to ff4249e2bc 2022-01-25 23:41:34 +00:00 Compare
sarah force-pushed fastercwtch from 86f0db49a4 to ea9cf5ca87 2022-01-25 23:43:38 +00:00 Compare
sarah added 1 commit 2022-01-25 23:44:49 +00:00
continuous-integration/drone/push Build is pending Details
continuous-integration/drone/pr Build is passing Details
a088e588b1
Comment on Serialization Format
dan approved these changes 2022-01-25 23:53:45 +00:00
@ -42,6 +48,7 @@ func (pa *PeerApp) NewInstance() tapir.Application {
newApp.OnAuth = pa.OnAuth
newApp.OnClose = pa.OnClose
newApp.OnConnecting = pa.OnConnecting
newApp.version.Store(0x01)
Owner

Store(Version1)

Store(Version1)
sarah marked this conversation as resolved
@ -62,0 +72,4 @@
// we are abusing the context here slightly by sending a "malformed" GetVal request.
// as a rule cwtch ignores getval requests that it cannot deserialize so older clients will ignore this
// message.
// version *must* be the first message sent to prevent race conditions for aut events fired on auth
Owner

for 'aut' events?

for 'aut' events?
sarah marked this conversation as resolved
@ -62,0 +77,4 @@
pa.SendMessage(model2.PeerMessage{
ID: event.ContextVersion,
Context: event.ContextGetVal,
Data: []byte{Version1},
Owner

version2?

version2?
sarah marked this conversation as resolved
sarah added 1 commit 2022-01-25 23:55:49 +00:00
continuous-integration/drone/push Build is passing Details
continuous-integration/drone/pr Build is passing Details
ec6e025284
Version Fixups
Member
Drone Build Status: success https://build.openprivacy.ca/cwtch.im/cwtch/698
Member
Drone Build Status: success https://build.openprivacy.ca/cwtch.im/cwtch/700
erinn approved these changes 2022-01-26 20:02:40 +00:00
erinn merged commit 5dc0579075 into master 2022-01-26 20:02:50 +00:00
Sign in to join this conversation.
No description provided.