ssbc / ssb-friends

Manages the SSB social graph
MIT License
23 stars 6 forks source link

bug: peers i've unblocked are now 0.1 hops #20

Closed ahdinosaur closed 5 years ago

ahdinosaur commented 5 years ago

hi folx, @mmckegg and i discovered something unexpected. i upgraded to the latest Patchwork master using the latest ssb-friends, and then i replicated a heap of people outside my 2 hops.

looking at the output of sbot friends.hops, we noticed that some peers were now at 0.1. in particular, a peer who i had blocked and then unblocked (and was not following) was a 0.1, instead of the expected >= 2.

expected behavior: unblocking a user (and not following them) should cause your hops to be >= 2

actual behavior: unblocking a user (and not following them) means their hops is 0.1 (same-as).

/cc @dominictarr, as i'm not sure how to make sense of the ssb-friends stack.

ahdinosaur commented 5 years ago

have confirmed the bug with a failing test! https://github.com/ssbc/ssb-friends/pull/22 :tada:

i still don't understand where the 0.1 comes from, i was hoping there would be an explicit 0.1 somewhere in the code stack but i don't see such a thing anywhere as far as i can tell.

mmckegg commented 5 years ago

@ahdinosaur I found this tasty tidbit in the dynamic-dijkstra readme:

ssb's options

In ssb we use 1 to represent follow, -1 to represent block, -2 to represent unfollow, and 0.1 to represent "same-as". A feed with path length 2 is a "friend of a friend" (we follow someone +1 who follows them + 1 = 2). If you block someone, that is -1. so -2 can mean blocked by a friend or unfollowed. min defines a positive length to be less than the negative length with the same absolute value, min(-n, n) == n so if a friend follows someone another friend blocks, the friends follow wins, (but if you block them directly, that block wins over the friend's follow)

expand(length, max) return false if length < 0, or length > max.

isAdd(v) returns true if v >= 0

same-as is represented by very low weights (i.e. 0.1) to link two devices a, b together, we have edges a->b and b->a. Low weights can also be used for delegation. Say, a blocklist l can be implemented as a node that only blocks, then someone x subscribes to that blocklist by adding edge x->l with a weight of 0.1.

ahdinosaur commented 5 years ago

oh, i found it! https://github.com/dominictarr/dynamic-dijkstra/blob/master/simple.js#L22

mmckegg commented 5 years ago

lol, looks like we have sameAs after all. All you have to do is block your other device, and then unblock!! 🤣

ahdinosaur commented 5 years ago

random question: why does -2 represent unfollow? i thought unfollowing would return you back to what you were before you followed them, and i thought the default would be your max hops length.

because it seems as though the unblock is being represented as null, which is being defaulted to 0.1.

ahdinosaur commented 5 years ago

yeah, so now i know what needs to change: https://github.com/ssbc/ssb-friends/blob/ff2ac132d142654586bc497b6864d82850ea7c34/contacts.js#L16-L20

but i have no idea what to change it to. :sweat_smile:

ahdinosaur commented 5 years ago

what i'd expect to work:

diff --git a/contacts.js b/contacts.js
index 62a9a2c..80a2ecc 100644
--- a/contacts.js
+++ b/contacts.js
@@ -3,6 +3,7 @@ var isFeed = require('ssb-ref').isFeed
 //track contact messages, follow, unfollow, block

 module.exports = function (sbot, createLayer, config) {
+  var max = config.friends && config.friends.hops || config.replicate && config.replicate.hops || 3

   var layer = createLayer('contacts')
   var initial = false
@@ -13,11 +14,11 @@ module.exports = function (sbot, createLayer, config) {

     var from = data.value.author
     var to = data.value.content.contact
-    var value =
-      data.value.content.following === true ? 1 :
-      data.value.content.following === false ? -2 :
-      data.value.content.blocking || data.value.content.flagged ? -1
-      : null
+    var value = null
+    if (data.value.content.following === true) value = 1
+    else if (data.value.content.following === false) value = max
+    else if (data.value.content.blocking === true || data.value.content.flagged === true) value = -1
+    else if (data.value.content.blocking === false || data.value.content.flagged === false) value = max

     if(isFeed(from) && isFeed(to)) {
       if(initial) 

and the legacy blocking / unblocking test passes, but that's probably because it first follows the person, so that overrides once the block & unblock happens. because without the first follow in the test i added (which is the situation i'm currently in), the block continues to dominate even after the unblock.

mmckegg commented 5 years ago

@ahdinosaur

i thought unfollowing would return you back to what you were before you followed them

Yeah, it should be as if you never followed them. Current behavior is a bit strange.

Also, unblocking someone should return to this state too! I think the fact that there are 2 different bool values (blocking, following) that get reduced to a single state is making this more confusing than it would be.

This is how I am treating it internally in patchwork:

var value = content.blocking
  ? -1
  : content.following
    ? 1
    : null

Where null is neutral: as if I've never followed/blocked, just go with whatever my friends think.

Each contact message constitutes a change to the contact state, where the blocking has precedence. But if you are blocking someone, and then a new contact message with "following: true" is published, this will override the previous block.

mmckegg commented 5 years ago

Here's the current logic written to be formatted the same way (for reference):

var value = content.following 
  ? 1 
  : content.following === false 
    ? -2 
    : content.blocking 
      ? -1
      : null

WUT. Why is ssb-friends allowing following to take precedence over blocking!?

dominictarr commented 5 years ago

wow, this is indeed a serious bug. thanks for finding it @ahdinosaur and @mmckegg

-2 does the same thing as not following because -2 is the same as a friend blocking them. -1 means you block them, -2 is the same as someone one hop away blocks them. it means they probably won't be replicated, but they will if a friend follows them, which makes them 2. Writing this stuff to make updates efficient was really hard, so this was a shortcut to make it easier.

that v || 0.1 should really be v === 0 ? 0.1 : v

following does beat blocking, but that only happens if a single contact message has BOTH following: true and blocking: true (which is weird)

dominictarr commented 5 years ago

everything is fixed in 3.1.9 I also added a test case that checked that if you were hops=2 before blocking, you should go back that after unblocking.

@ahdinosaur the problem with setting it to max hops is that config for max hops can change, if you set a low max then unblocked someone, then set a high max, you'd get weird behaviour and state.

ahdinosaur commented 5 years ago

@dominictarr woo, thanks for the fix! :tada:

mmckegg commented 5 years ago

@dominictarr

following does beat blocking, but that only happens if a single contact message has BOTH following: true and blocking: true (which is weird)

Unless I'm misunderstanding other parts of the code, the problem I can see is following: false, blocking: true resolves to neutral, rather than block! That would be quite a plausible message, and I think everyone would agree that it should result in a block.

staltz commented 5 years ago

Thanks for the new version, Dominic

following does beat blocking, but that only happens if a single contact message has BOTH following: true and blocking: true (which is weird)

But why does following beat blocking?

ahdinosaur commented 5 years ago

But why does following beat blocking?

@staltz: because of how the nested ternary is written: https://github.com/ssbc/ssb-friends/issues/20#issuecomment-448794914

dominictarr commented 5 years ago

@staltz technically, you could do blocking: true, following: true but that doesn't work if you want to express it as a single number (and it doesn't make any sense to be blocking and following anyway). so if a single contact message has following: true, blocking: true it's interpreted as following. doesn't really matter, just had to make a call. I wouldn't consider that a valid message anyway.

staltz commented 5 years ago

I meant: "why should following beat blocking?" :) I understand nested ternaries.

dominictarr commented 5 years ago

@staltz https://www.youtube.com/watch?v=Cghg-QyTP_M