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

Fix 'previous' in the members tangle #88

Merged
merged 19 commits into from
Oct 3, 2023
Merged

Fix 'previous' in the members tangle #88

merged 19 commits into from
Oct 3, 2023

Conversation

Powersource
Copy link
Contributor

@Powersource Powersource commented Sep 23, 2023

Fixes #76

  • tasks in issue
  • actually test
  • what's up with the previous array being 2 long in the exclude test?

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@Powersource Powersource marked this pull request as ready for review September 24, 2023 19:12
@Powersource Powersource mentioned this pull request Sep 24, 2023
9 tasks
lib/get-group-tangle.js Outdated Show resolved Hide resolved
Copy link
Member

@mixmix mixmix left a comment

Choose a reason for hiding this comment

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

Found 1 large gotcha (easy fix, happy to pair on it cos I remember the context)
but looks great otherwise

@socket-security
Copy link

socket-security bot commented Sep 25, 2023

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

@Powersource
Copy link
Contributor Author

@SocketSecurity ignore [email protected]
@SocketSecurity ignore [email protected]

they both have MIT if you check their repos

@Powersource
Copy link
Contributor Author

ugh we're back on stuck tests 😞

@Powersource
Copy link
Contributor Author

seems to have started getting stuck once we merged master into this, so hopefully no big issue in this PR. i'll take a look at master

package.json Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@Powersource
Copy link
Contributor Author

tried tape's different timeout options, but they just fail the test early while it keeps running, it doesn't kill it

@Powersource
Copy link
Contributor Author

I think there are two problems beneath the flakes here (which seem to be because of this branch):

  1. race condition between the initial query in getGroupTangle vs the timeout vs the 2 messages we're posting almost at the same time
  2. that this timeout, which an older GetGroupTangle tests has, is not in this members tangle test.
    // NOTE: Publishing has a queue which means if you publish many things in a row there is a delay before those values are in indexes to be queried.
    const _getGroupTangle = GetGroupTangle(server, keystore)
    const getGroupTangle = (id, cb) => {
    setTimeout(() => _getGroupTangle(id, cb), 300)
    }

I tried a big rewrite where we run the initial query much earlier, to try to fix 1. and get around using the timeout. It got really messy so probably not doing that atm, especially since tests seem to be passing now with just a bit of flake. I might add a timeout to the test like in 2. to avoid flakes.

diff --git a/index.js b/index.js
index 1fcf9bf..06f9193 100644
--- a/index.js
+++ b/index.js
@@ -67,6 +67,8 @@ function init (ssb, config) {
 
   /* secret keys store / helper */
   const keystore = {} // HACK we create an Object so we have a reference to merge into
+  let getGroupTangle = null
+  let getMembersTangle = null
   KeyRing(join(config.path, 'tribes/keystore'), (err, api) => {
     if (err) throw err
 
@@ -74,6 +76,10 @@ function init (ssb, config) {
       if (err) throw err
 
       Object.assign(keystore, api) // merging into existing reference
+
+      getGroupTangle = GetGroupTangle(ssb, keystore, 'group')
+      getMembersTangle = GetGroupTangle(ssb, keystore, 'members')
+
       state.loading.keystore.set(false)
     })
   })
@@ -215,8 +221,6 @@ function init (ssb, config) {
    */
 
   /* Tangle: auto-add tangles.group info to all private-group messages */
-  const getGroupTangle = GetGroupTangle(ssb, keystore, 'group')
-  const getMembersTangle = GetGroupTangle(ssb, keystore, 'members')
   ssb.publish.hook(function (publish, args) {
     const [content, cb] = args
     if (!content.recps) return publish.apply(this, args)
diff --git a/lib/get-group-tangle.js b/lib/get-group-tangle.js
index 2d3442f..a0ff550 100644
--- a/lib/get-group-tangle.js
+++ b/lib/get-group-tangle.js
@@ -10,6 +10,65 @@ module.exports = function GetGroupTangle (server, keystore, tangle = 'group') {
   const cache = new Map([]) // groupId > new Reduce (tangleTips)
 
   // LISTEN
+
+  // for every group we're in, populate the cache as soon as possible
+  pull(
+    keystore.group.list({ live: true }),
+    pull.unique(),
+    pull.drain((groupId) => {
+      console.log('found groupId')
+      const info = keystore.group.get(groupId)
+
+      const query = [
+        {
+          $filter: {
+            dest: info.root,
+            value: {
+              content: {
+                tangles: {
+                  [tangle]: { root: info.root }
+                }
+              }
+            }
+          }
+        },
+        {
+          $map: {
+            key: ['key'],
+            previous: ['value', 'content', 'tangles', tangle, 'previous']
+          }
+        }
+      ]
+      // NOTE: a query which gets all update messages to the root message
+
+      pull(
+        server.backlinks.read({ query }),
+        pull.collect((err, nodes) => {
+          if (err) {
+            console.error('Error getting tangle backlinks for', groupId)
+            return
+          }
+
+          // NOTE: backlink query does not get root node
+          nodes.push({ key: info.root, previous: null })
+
+          // Create a Reduce using the message contents
+          // NOTE - do NOT store the whole msg (node)
+          // we're not doing any reducing of transformations, we care only about
+          // reducing the graph to find the tips
+          // each node should be pruned down to e.g. { key: '%D', previous: ['%B', '%C'] }
+
+          const reduce = new Reduce(strategy, { nodes })
+          // Store Reduce in the cache to use/update it later.
+          console.log('adding to cache', groupId)
+          cache.set(groupId, reduce)
+        })
+      )
+    }, (err) => {
+      console.error("Error listing groups we're in for caching tangles", err)
+    })
+  )
+
   // listen to all new messages that come in
   // if a new message comes in which has msg.value.content.tangles.group.root that is in cache
   // update the value in the cache (this will use our new addNodes functionality)
@@ -20,9 +79,6 @@ module.exports = function GetGroupTangle (server, keystore, tangle = 'group') {
     pull.unique('key'),
     pull.drain(updateCache)
   )
-  // server.post(m => server.get({ id: m.key, private: true, meta: true }, (err, msg) => {
-  //   updateCache(msg)
-  // }))
 
   function updateCache (msg) {
     const { recps, tangles } = msg.value.content
@@ -37,6 +93,8 @@ module.exports = function GetGroupTangle (server, keystore, tangle = 'group') {
           previous: tangles.group.previous
         }]) // Get key and previous from msg
       }
+    } else {
+      //console.error("trying to update tangle cache for group we're not in", msgGroupId)
     }
   }
 
@@ -48,60 +106,12 @@ module.exports = function GetGroupTangle (server, keystore, tangle = 'group') {
 
     // if it's in the cache, then get the cached value, then callback
     if (cache.has(groupId)) {
-      // this timeout seems to help for some reason. in some cases messages were posted too fast with tangles 'in parallel', e.g. 2 messages both just having previous: [rootMsgId]
-      return setTimeout(() => {
-        return cb(null, {
-          root: info.root,
-          previous: Object.keys(cache.get(groupId).state)
-        })
-      }, 0)
-    }
-    // if not in cache, compute it and add to the cache
-
-    const query = [
-      {
-        $filter: {
-          dest: info.root,
-          value: {
-            content: {
-              tangles: {
-                [tangle]: { root: info.root }
-              }
-            }
-          }
-        }
-      },
-      {
-        $map: {
-          key: ['key'],
-          previous: ['value', 'content', 'tangles', tangle, 'previous']
-        }
-      }
-    ]
-    // NOTE: a query which gets all update messages to the root message
-
-    pull(
-      server.backlinks.read({ query }),
-      pull.collect((err, nodes) => {
-        if (err) return cb(err)
-
-        // NOTE: backlink query does not get root node
-        nodes.push({ key: info.root, previous: null })
-
-        // Create a Reduce using the message contents
-        // NOTE - do NOT store the whole msg (node)
-        // we're not doing any reducing of transformations, we care only about
-        // reducing the graph to find the tips
-        // each node should be pruned down to e.g. { key: '%D', previous: ['%B', '%C'] }
-
-        const reduce = new Reduce(strategy, { nodes })
-        // Store Reduce in the cache to use/update it later.
-        cache.set(groupId, reduce)
-        cb(null, {
-          root: info.root,
-          previous: Object.keys(reduce.state)
-        })
+      return cb(null, {
+        root: info.root,
+        previous: Object.keys(cache.get(groupId).state)
       })
-    )
+    } else {
+      return cb(Error(`groupId not found in get-group-tangle cache ${tangle} ${groupId}`))
+    }
   }
 }
diff --git a/test/lib/get-group-tangle.test.js b/test/lib/get-group-tangle.test.js
index e9197ba..260229d 100644
--- a/test/lib/get-group-tangle.test.js
+++ b/test/lib/get-group-tangle.test.js
@@ -7,16 +7,20 @@ const { GetGroupTangle } = require('../../lib')
 
 test('get-group-tangle unit test', t => {
   const name = `get-group-tangle-${Date.now()}`
+  console.log('creating server')
   const server = Server({ name, debug: false })
+  console.log('created server')
 
   //    - creating a group and publishing messages (ssb-tribes)
   server.tribes.create(null, (err, data) => {
     if (err) throw err
+    console.log('initing keystore')
     const keystore = {
       group: {
         get (groupId) {
           return { ...data, root: data.groupInitMsg.key } // rootMsgId
-        }
+        },
+        list: () => pull.values([data.groupId])
       }
     }
     // NOTE: Tribes create callback with different data than keystore.group.get :(

Copy link
Member

@mixmix mixmix left a comment

Choose a reason for hiding this comment

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

🦩 😶‍🌫️ 🍰

Looking really good. I only had a couple of style-like questions

}

publish.call(this, content, cb)
})
Copy link
Member

Choose a reason for hiding this comment

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

the maybeMembersTangle is not something I'm fond of. Prefer early return?

getGroupTangle(content.recps[0], (err, groupTangle) => {
  if (err) return cb(Error("Couldn't get group tangle", { cause: err }))

  set(content, 'tangles.group', groupTangle)
  tanglePrune(content, 'group')

  if (!memberType(content.type)) {
    return publish.call(this, content, cb)
  }

  getMembersTangle(content.recps[0], (err, membersTangle) => {
    if (err) return cb(Error("Couldn't get members tangle", { cause: err }))

    set(content, 'tangles.members', membersTangle)
    tanglePrune(content, 'members')
    publish.call(this, content, cb)
  })

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yeah that's smart

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

index.js Outdated
@@ -214,27 +214,40 @@ function init (ssb, config) {
* (because they can't post messages to the group before then)
*/

const memberType = (type) => type === 'group/add-member' || type === 'group/exclude-member'
Copy link
Member

Choose a reason for hiding this comment

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

rename isMemberType to hint it will return Boolean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@Powersource Powersource merged commit 134064b into master Oct 3, 2023
6 checks passed
@Powersource Powersource deleted the members-tangle branch October 3, 2023 08:00
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.

Fix the member tangle
2 participants