Tapir Server Refactor #317

Merged
dan merged 12 commits from tapir_server into master 2020-09-28 21:56:20 +00:00
Owner

First cut of the Tapir Service Refactor, presented for discussion purposes.

This needs a few changes in Tapir:

In addition there is an open question around whether we want to force clients to poll for new messaging updates (as in the current sketch), or introduce a broadcast primitive in Tapir like ricochet had (will have a discussion tomorrow).

This change will also delete a ton of code related to the older protobuf models and delegate all of connection management and maintainence to Tapir.

First cut of the Tapir Service Refactor, presented for discussion purposes. This needs a few changes in Tapir: * Ephemeral Connect: https://git.openprivacy.ca/cwtch.im/tapir/issues/20 * Max Connection Length changes https://git.openprivacy.ca/cwtch.im/tapir/issues/22 * Service Metrics: https://git.openprivacy.ca/cwtch.im/tapir/issues/21 In addition there is an open question around whether we want to force clients to poll for new messaging updates (as in the current sketch), or introduce a broadcast primitive in Tapir like ricochet had (will have a discussion tomorrow). This change will also delete a ton of code related to the older protobuf models and delegate all of connection management and maintainence to Tapir.
sarah added the
BLOCKED
label 2020-07-14 00:51:10 +00:00
dan was assigned by sarah 2020-07-14 00:51:10 +00:00
Member
Drone Build Status: failure https://build.openprivacy.ca/cwtch.im/cwtch/781
dan reviewed 2020-07-14 19:00:44 +00:00
dan left a comment
Owner

cool, skimmed the WIP code
if you want to chat

cool, skimmed the WIP code if you want to chat
@ -0,0 +47,4 @@
go ta.Listen()
go func() {
for {
ta.Replay()
Owner

haha what is this infinit loop calling .Replay() every 10 seconds forever? with no way to exit? seems like a thread leak?

haha what is this infinit loop calling .Replay() every 10 seconds forever? with no way to exit? seems like a thread leak?
Author
Owner

Temporary because we don't have a broadcast mechanism .

Temporary because we don't have a broadcast mechanism .
@ -0,0 +63,4 @@
data := ta.connection.Expect()
if len(data) == 0 {
log.Debugf("Server closed the connection...")
return // connection is closed
Owner

need to fire a handler for connection shutdown event emit

need to fire a handler for connection shutdown event emit
@ -0,0 +101,4 @@
egm := groups.EncryptedGroupMessage{Ciphertext: ct, Signature: sig};
//token, err := ta.paymentHandler.NextToken(egm.ToBytes(), ta.connection.Hostname())
//if err == nil {
log.Debugf("POSTING A DAMN MESSAGE")
Owner

lol having some fun?

lol having some fun?
dan reviewed 2020-07-14 19:01:23 +00:00
dan left a comment
Owner

cool, skimmed the WIP code
if you want to chat

cool, skimmed the WIP code if you want to chat
Member
Drone Build Status: failure https://build.openprivacy.ca/cwtch.im/cwtch/783
sarah removed the
BLOCKED
label 2020-07-21 18:20:31 +00:00
sarah changed title from WIP: Tapir Server Refactor (DO NOT REVIEW) to Tapir Server Refactor 2020-07-21 18:20:40 +00:00
Member
Drone Build Status: failure https://build.openprivacy.ca/cwtch.im/cwtch/785
Member
Drone Build Status: failure https://build.openprivacy.ca/cwtch.im/cwtch/787
Member
Drone Build Status: success https://build.openprivacy.ca/cwtch.im/cwtch/789
dan approved these changes 2020-07-21 21:05:54 +00:00
dan left a comment
Owner

This still leaves a lot of half wired group functionality.

For example Application (applet).LaunchPeers() calls cwtchPeer.StartGroupConnections()

I'm a little confused what this will do now.

The Group struct seems unchanged but you moved half its functionality into unmarked peer/contacts.

I'll admit it was a bit unweildly having to collect all the groups and suss out their servers and manage connections that way, it wasn't perfectly ideal, but I'm not sure this entierly solves it either, we at least need some easier ways from client apps like UI to differentiate between contacts to peers and servers than the undocumented "check if there is a token attribute". Ideally there's a new profile field then indicating if it's a server and a corresponding profile.isServer or profile.Type and a cwtchPeer filer function so client uis can just get peer contacts or server contacts.

And on the front end we still have some munging to do around group status mapping to a server but maybe it's a bit easier now? as it's mostly just publish messages with the server and let the groups identify themselves :)

This still leaves a lot of half wired group functionality. For example Application (applet).LaunchPeers() calls cwtchPeer.StartGroupConnections() I'm a little confused what this will do now. The Group struct seems unchanged but you moved half its functionality into unmarked peer/contacts. I'll admit it was a bit unweildly having to collect all the groups and suss out their servers and manage connections that way, it wasn't perfectly ideal, but I'm not sure this entierly solves it either, we at least need some easier ways from client apps like UI to differentiate between contacts to peers and servers than the undocumented "check if there is a token attribute". Ideally there's a new profile field then indicating if it's a server and a corresponding profile.isServer or profile.Type and a cwtchPeer filer function so client uis can just get peer contacts or server contacts. And on the front end we still have some munging to do around group status mapping to a server but maybe it's a bit easier now? as it's mostly just publish messages with the server and let the groups identify themselves :)
@ -0,0 +4,4 @@
const (
// KeyTypeOnion - a cwtch address
KeyTypeOnion = "onion" // bulletin board
Owner

These are all string, do we want to make a
type KeyType string
that these can be?

These are all string, do we want to make a ```type KeyType string``` that these can be?
@ -0,0 +28,4 @@
}
// GetKey retrieves a key with a given type from the bundle
func (kb *KeyBundle) GetKey(name string) (Key, error) {
Owner

for clarity arg named 'name' or type string made me think this was gettting a key for an onion, renaming it keytype or making it keytype clarifies the api a bunch :)

for clarity arg named 'name' or type string made me think this was gettting a key for an onion, renaming it keytype or making it keytype clarifies the api a bunch :)
dan marked this conversation as resolved
@ -206,1 +207,4 @@
func (cp *cwtchPeer) AddServer(serverSpecification string) {
keyBundle := new(model.KeyBundle)
json.Unmarshal([]byte(serverSpecification), &keyBundle)
Owner

why is it called serverSpecification if it deserializes to a keybundle?

why is it called serverSpecification if it deserializes to a keybundle?
@ -207,0 +216,4 @@
onion := string(onionKey)
decodedPub, _ := base32.StdEncoding.DecodeString(strings.ToUpper(onion))
ab := keyBundle.AttributeBundle()
ab["nick"] = onion
Owner

ab[attr.GetLocalScope("name")] = onion

is what ui uses

```ab[attr.GetLocalScope("name")] = onion``` is what ui uses
@ -207,0 +219,4 @@
ab["nick"] = onion
pp := &model.PublicProfile{Name: onion, Ed25519PublicKey: decodedPub, Authorization: model.AuthUnknown, Onion: onion, Attributes: ab}
cp.Profile.AddContact(onion, pp)
Owner

this seems to be exposing a bit of a bug (i think ui makes sure we don't double do it but nothing in cwtchPeer or profile do...), no where do we make a check if we already have this contact. with this current behaviour we would be wiping out existing profile along with its message histories and settings and attributes and authentication level.

I assume we need to import a contact for the owner of the group or something? we need something more robust here that can added these keys to an existing contact?

this seems to be exposing a bit of a bug (i think ui makes sure we don't double do it but nothing in cwtchPeer or profile do...), no where do we make a check if we already have this contact. with this current behaviour we would be wiping out existing profile along with its message histories and settings and attributes and authentication level. I assume we need to import a contact for the owner of the group or something? we need something more robust here that can added these keys to an existing contact?
Author
Owner

Will add a check and an error for this case

Will add a check and an error for this case
@ -247,0 +249,4 @@
// peerWithTokenServer is the entry point for cwtchPeer - server relationships
// needs to be run in a goroutine as will block on Open.
func (e *engine) peerWithTokenServer(onion string, tokenServerOnion string, tokenServerY string) {
e.ignoreOnShutdown(e.serverConnecting)(onion)
Owner

we used to have multiple groups per onion, so we had to always check if we were already connected, cus this event might get fired for joining a new group on an existing server? is there now a 1:1 between onions and groups?

we used to have multiple groups per onion, so we had to always check if we were already connected, cus this event might get fired for joining a new group on an existing server? is there now a 1:1 between onions and groups?
Author
Owner

Nope groups are distinct from servers. A server can handle messages from multiple groups - but the peer relationship is handled seperately - Tapir will handle multiple connections now.

Nope groups are distinct from servers. A server can handle messages from multiple groups - but the peer relationship is handled seperately - Tapir will handle multiple connections now.
Owner

but do we want to have multiple tapir connections to a server? or i guess you mean it'll handle closing the old one and replacing it with the new one? that's fine in the case of a new group being added cus it'll go a replay so we get the new group messages. it's just rough on boot up if we have like 10 groups on a server, its a bit messy. so we prolly want something like the existing connecttogroups fn in cwtchpeer that collates the servers and just connects to each one once on start up.

or have i wandered off point?

but do we want to have multiple tapir connections to a server? or i guess you mean it'll handle closing the old one and replacing it with the new one? that's fine in the case of a new group being added cus it'll go a replay so we get the new group messages. it's just rough on boot up if we have like 10 groups on a server, its a bit messy. so we prolly want something like the existing connecttogroups fn in cwtchpeer that collates the servers and just connects to each one once on start up. or have i wandered off point?
Author
Owner

I mean that Tapir will deduplicate multiple connections and so there will only be one connection to the server.

I mean that Tapir will deduplicate multiple connections and so there will only be one connection to the server.
Owner

ah since we're managing connections as peer contacts now anyways there shouldnt be multiple at startup anyways!

ah since we're managing connections as peer contacts now anyways there shouldnt be multiple at startup anyways!
dan marked this conversation as resolved
@ -0,0 +111,4 @@
return
}
}
ta.serverSyncedHandler(ta.connection.Hostname())
Owner

we get only one replayResultMessage from the server? no matter how large the history is? how is this getting around the tapir packet size limit?

we get only one replayResultMessage from the server? no matter how large the history is? how is this getting around the tapir packet size limit?
Author
Owner

The server sends 1 packet per message (like the current protobuf impl)

			for i := 0; i < message.ReplayResult.NumMessages; i++ {
				data := ta.connection.Expect()
                ...
The server sends 1 packet per message (like the current protobuf impl) for i := 0; i < message.ReplayResult.NumMessages; i++ { data := ta.connection.Expect() ...
Owner

aaah ok i see it now, it gets a replayResult with the num of messages to expect and then loops expecting those messages
cool
and since its using same receiveGroupMessageHandler, if a few new messages go to the server and sent to the user, the count will be off by a few, so it would signel "synced" slightly early, but those final messages would be in the pipe, a mater of ms.

would the new messages be processable if we haven't finished syncing?

aaah ok i see it now, it gets a replayResult with the num of messages to expect and then loops expecting those messages cool and since its using same receiveGroupMessageHandler, if a few new messages go to the server and sent to the user, the count will be off by a few, so it would signel "synced" slightly early, but those final messages would be in the pipe, a mater of ms. would the new messages be processable if we haven't finished syncing?
Owner

please run this against a profile that had existing groups, like the cwtch alpha group, I'm wondering what loading that data from storage is gonna do

please run this against a profile that had existing groups, like the cwtch alpha group, I'm wondering what loading that data from storage is gonna do
sarah requested review from dan 2020-07-22 17:15:34 +00:00
Member
Drone Build Status: success https://build.openprivacy.ca/cwtch.im/cwtch/793
dan reviewed 2020-07-22 21:32:07 +00:00
model/profile.go Outdated
@ -80,2 +79,4 @@
}
// IsServer returns true if the onion address is associated with a server.
func (p *PublicProfile) IsServer(onion string) bool {
Owner

I think if you wanted to be fancy you can do:

func (p *PublicProfile) IsServer(onion string) (isServer bool) {
	_, isServer = p.GetAttribute(string(KeyTypeServerOnion));
}
I think if you wanted to be fancy you can do: ``` func (p *PublicProfile) IsServer(onion string) (isServer bool) { _, isServer = p.GetAttribute(string(KeyTypeServerOnion)); } ```
@ -204,6 +205,43 @@ func (cp *cwtchPeer) AddContact(nick, onion string, authorization model.Authoriz
cp.eventBus.Publish(event.NewEventList(event.SetPeerAttribute, event.RemotePeer, onion, event.SaveHistoryKey, event.DeleteHistoryDefault))
}
Owner

Comment (quality.sh misses public methods on structs filling in Interfaces some times wierdly...)

Comment (quality.sh misses public methods on structs filling in Interfaces some times wierdly...)
sarah marked this conversation as resolved
@ -207,0 +216,4 @@
if cp.GetContact(onion) == nil {
decodedPub, _ := base32.StdEncoding.DecodeString(strings.ToUpper(onion))
ab := keyBundle.AttributeBundle()
ab["nick"] = onion
Owner

attr.GetPublicScope("name")

is what ui is using. i dont think any code is using attributes["nick"] anymore. should be safe to just drop this line tho as the ui falls back on onion if no "settings.name" attribute is found

attr.GetPublicScope("name") is what ui is using. i dont think any code is using attributes["nick"] anymore. should be safe to just drop this line tho as the ui falls back on onion if no "settings.name" attribute is found
@ -207,0 +237,4 @@
return nil
}
// Server Already Exists
return errors.New("a contact already exists with the given onion address")
Owner

do we need typed errors here? or should this not be an error? because if a client app is using this is getting an error on AddServer expected if the server exists? they'll have to filter it out from all the other "couldnt add server so we dont have it errors" and ignore this specigic one, cus they can go ahead and still use the server

do we need typed errors here? or should this not be an error? because if a client app is using this is getting an error on AddServer expected if the server exists? they'll have to filter it out from all the other "couldnt add server so we dont have it errors" and ignore this specigic one, cus they can go ahead and still use the server
@ -0,0 +56,4 @@
ta.AuthApp.Init(connection)
if connection.HasCapability(applications.AuthCapability) {
ta.connection = connection
ta.connection.SetCapability(groups.CwtchServerSyncedCapability)
Owner

so TokenBoardClient sets CwtchServerSyncedCapability during init

but then in engine, we check if we have the capability there? is that just to make sure the app behind the connection is the write one and we aren't accidently sending on a peer connection?

so TokenBoardClient sets CwtchServerSyncedCapability during init but then in engine, we check if we have the capability there? is that just to make sure the app behind the connection is the write one and we aren't accidently sending on a peer connection?
sarah marked this conversation as resolved
@ -0,0 +87,4 @@
newMessages := ta.LegacyMessageStore.FetchMessages()
// Set sync and then send any new messages that might have happened while we were syncing
ta.connection.SetCapability(groups.CwtchServerSyncedCapability)
if len(newMessages) > len(messages) {
Owner

so if i'm reading this right, this relays all messages in teh buffer, and then sets CwtchServerSyncedCapability which below blocks/unblocks broadcast of new messages and then relays anything else that came in? seems solid

double messages aren't a problem? maybe swap

newMessages := ta.LegacyMessageStore.FetchMessages()
// Set sync and then send any new messages that might have happened while we were syncing
ta.connection.SetCapability(groups.CwtchServerSyncedCapability)

then? since this isnt atomic code, better to get messages that came in once the broadcast flag is set or there's a micro gap there?

so if i'm reading this right, this relays all messages in teh buffer, and then sets CwtchServerSyncedCapability which below blocks/unblocks broadcast of new messages and then relays anything else that came in? seems solid double messages aren't a problem? maybe swap ``` newMessages := ta.LegacyMessageStore.FetchMessages() // Set sync and then send any new messages that might have happened while we were syncing ta.connection.SetCapability(groups.CwtchServerSyncedCapability) ``` then? since this isnt atomic code, better to get messages that came in once the broadcast flag is set or there's a micro gap there?
sarah marked this conversation as resolved
Member
Drone Build Status: failure https://build.openprivacy.ca/cwtch.im/cwtch/795
Member
Drone Build Status: success https://build.openprivacy.ca/cwtch.im/cwtch/797
Member
Drone Build Status: failure https://build.openprivacy.ca/cwtch.im/cwtch/801
Member
Drone Build Status: failure https://build.openprivacy.ca/cwtch.im/cwtch/803
Member
Drone Build Status: success https://build.openprivacy.ca/cwtch.im/cwtch/805
Member
Drone Build Status: success https://build.openprivacy.ca/cwtch.im/cwtch/809
Owner
  • importing legacy profiles tested and fixed and good

just need:

  • delete the contract retry error logs and dead else { // }
  • still a little sad that the integ test had 3x10s sleeps turned to 3x60s sleeps, that bumps up its run time ~2.7 minutes of sleeping. anything we can do to optimize that. can any be consolidated? especailly the last two send messages between carol and bob, there also is ANOTHER sleep 60 right after that too to account for slow transmission
- importing legacy profiles tested and fixed and good just need: - delete the contract retry error logs and dead else { // } - still a little sad that the integ test had 3x10s sleeps turned to 3x60s sleeps, that bumps up its run time ~2.7 minutes of sleeping. anything we can do to optimize that. can any be consolidated? especailly the last two send messages between carol and bob, there also is ANOTHER sleep 60 right after that too to account for slow transmission
Member
Drone Build Status: success https://build.openprivacy.ca/cwtch.im/cwtch/811
Member
Drone Build Status: failure https://build.openprivacy.ca/cwtch.im/cwtch/813
Member
Drone Build Status: failure https://build.openprivacy.ca/cwtch.im/cwtch/815
Member
Drone Build Status: failure https://build.openprivacy.ca/cwtch.im/cwtch/817
Member
Drone Build Status: success https://build.openprivacy.ca/cwtch.im/cwtch/819
Member
Drone Build Status: success https://build.openprivacy.ca/cwtch.im/cwtch/821
dan merged commit 5473587a3e into master 2020-09-28 21:56:19 +00:00
First-time contributor

This PR removed the CLI app. Is that app's functionality provided by some other package now, or is there no longer any CLI for Cwtch?

This PR removed the CLI app. Is that app's functionality provided by some other package now, or is there no longer any CLI for Cwtch?
Author
Owner

This PR removed the CLI app. Is that app's functionality provided by some other package now, or is there no longer any CLI for Cwtch?

As of right now there is no CLI version of Cwtch.

The CLI app that was removed as part of this PR was already fairly outdated and unmaintained which is why it was removed.

Going into the future I expect there will be 2 main categories of CLI based Cwtch clients:

I'd like to at least support an official Cwtch CLI chat app for basic messaging, and given our current APIs it shouldn't be all that much work, but it's currently less of a priority than getting the Beta version of Desktop/Android apps.

If anyone is interested in pushing forward on this I'm happy to help provide guidance and host the repo.

> This PR removed the CLI app. Is that app's functionality provided by some other package now, or is there no longer any CLI for Cwtch? As of right now there is no CLI version of Cwtch. The CLI app that was removed as part of this PR was already fairly outdated and unmaintained which is why it was removed. Going into the future I expect there will be 2 main categories of CLI based Cwtch clients: * Bots e.g. FuzzBot: https://git.openprivacy.ca/sarah/cwtchbot/src/branch/master/cmd/fuzzbot/fuzzbot.go * Specific terminal interfaces for apps built on Tapir/Cwtch e.g. the Orbscura prototype: https://git.openprivacy.ca/sarah/orbscura I'd like to at least support an official Cwtch CLI chat app for basic messaging, and given our current APIs it shouldn't be all that much work, but it's currently less of a priority than getting the Beta version of Desktop/Android apps. If anyone is interested in pushing forward on this I'm happy to help provide guidance and host the repo.
First-time contributor

I see; thanks @sarah for the answer, and apologies for being 7 months late to thank you. Theoretically I have an interest in a CLI chat app, but I think my skill set is not well suited to working on that, so I guess I'll wait for another hero to step up. Really loving the progress Cwtch has made.

I see; thanks @sarah for the answer, and apologies for being 7 months late to thank you. Theoretically I have an interest in a CLI chat app, but I think my skill set is not well suited to working on that, so I guess I'll wait for another hero to step up. Really loving the progress Cwtch has made.
Sign in to join this conversation.
No description provided.