Kitsune Sharded Gossip #834

freesig · 2021-06-18T08:05:19Z

No description provided.

…nto kitsune-gossip

Connoropolous · 2021-06-18T21:07:04Z

crates/holochain_p2p/src/spawn/actor.rs

+        kitsune_space: Arc<kitsune_p2p::KitsuneSpace>,
+        since_ms: u64,
+        until_ms: u64,
+        arc_set: Arc<kitsune_p2p_types::dht_arc::DhtArcSet>,


I was scanning this code and it occurred to me that there's quite a surprising overlap between names coming up in the code here, given Atomic Reference Counting = Arc, and then the DHT related implementation is dealing in Arc's. The only thing that came to my mind about this is possibly leaving simple and short code comments to distinguish that the Arc<> on the outside is different than the ArcSet on the inside for future code reviewers and Holochain contributors?

Perhaps. I mean DhtArc is hopefully clearly different to Arc. Where would you put the comment though? This type is used in a lot of places. I hope that anyone familiar enough with rust would see Arc and know what that is. The only other thing to do would be to change the name from DhtArc?

Yeah, there's really nothing to be done for it thats useful. I suppose I just thought it was amusing in a way, and unlikely, and pictured some poor reader down the road :p nvm me

We could call it a Darc for short?

The distinction seems clear to me in code, but then when we talk about it more colloquially and in comments, there is this unfortunate name collision.

I still think some variant of "arc" is the best name for it, since it is really helpful to think of it as an arc of a circle, and that is the correct geometrical term.

The only other geometrical term that could make sense is a "sector", which includes the arc points but also includes the area between the two radii. This seems less precise, since really we are only dealing with the one-dimensional wrapping space of DHT locations, and I don't know what the area inside the circle would represent.

…e-gossip

maackle · 2021-07-12T17:39:29Z

crates/holochain/src/conductor/handle.rs

+                    // FIXME: This sucks we have to iterate through the whole vec just to add Arcs.
+                    // Are arcs really saving us that much?


Ha, this is a case where I thought you were talking about DhtArcs here, not sync::Arcs, and was trying to wrap my head around it.

Well, the primitive type that kitsune deals with is basically Arc. So perhaps p2p_gossip_query_agents should be wrapping them in Arcs before returning. Is there any other reason not to? Other than the slight overhead of Arcs?

lol, I take it you saw my comment.

Yeah, the distinction seems clear to me in code, but then when we talk about it more colloquially and in comments, there is this unfortunate name collision

Arc are a pain when you don't need them and you want an owned or mut value because you either need to clone the underlying value or try_unwrap it which is hard to get right so I'd prefer that the db didn't return arcs but I'm not going to object to it if everyone thinks it's the way to go.

I have some comments in the code thinking the same thing, especially for op hashes, I think it's wasteful to put them all in Arcs especially when there could be thousands of them.

crates/holochain/tests/sharded_gossip.rs

crates/kitsune_p2p/kitsune_p2p/src/spawn/actor/space.rs

crates/holochain/src/conductor/handle.rs

… kitsune-gossip

This reverts commit 25e5daa.

crates/kitsune_p2p/kitsune_p2p/src/gossip/sharded_gossip.rs

…e-gossip

maackle · 2021-07-31T01:11:53Z

crates/holochain/src/conductor/cell.rs

-            PutMetricDatum { .. } | QueryMetrics { .. } => {
-                // Same goes for metrics
+            | QueryGossipAgents { .. }
+            | FetchOpHashesForConstraints { .. }


Can we just rename this QueryOpHashes to match the others?

This query returns the agents and their arcs though?

Or you mean rename the fetch op hashes?

Yeah, rename fetch. "fetch X for constraints" just sounds like a long way to say "query X" to me.

I renamed it, let me know if you object

crates/holochain/src/conductor/handle.rs

maackle

Still reading through, here's my comments so far

maackle · 2021-07-31T01:33:08Z

crates/kitsune_p2p/kitsune_p2p/src/gossip/sharded_gossip.rs

+            // TODO: Check if the node is a successful peer or not and set the timeout accordingly.
+            // TODO: Actually I think this is the wrong time out? This is
+            // how long we wait to timeout a round.
+            round_timeout: self
+                .tuning_params
+                .gossip_peer_on_success_next_gossip_delay_ms,


Should either of these TODOs be addressed now, especially the second one?

Yeh maybe we just have a completely different tuning param for round timeouts? I don't think it has anything to do with if there was an error in the past or not

crates/kitsune_p2p/kitsune_p2p/src/gossip/sharded_gossip.rs

crates/kitsune_p2p/kitsune_p2p/src/gossip/sharded_gossip/bandwidth.rs

crates/kitsune_p2p/kitsune_p2p/src/gossip/sharded_gossip/bloom.rs

maackle · 2021-08-02T16:01:01Z

Hmm, the sanity_run_for_five_seconds is sometimes stalling. It just happened in CI and happened once locally, but now I've run it a dozen times locally and it always passes.

maackle

This is all my comments. I'll approve after this is wrapped up. The sanity_run_for_five_seconds is also sometimes hanging which we should probably look into as part of this.

crates/kitsune_p2p/kitsune_p2p/src/gossip/sharded_gossip/bloom.rs

crates/kitsune_p2p/kitsune_p2p/src/gossip/sharded_gossip/initiate.rs

maackle · 2021-08-02T17:10:33Z

crates/kitsune_p2p/kitsune_p2p/src/gossip/sharded_gossip/store.rs

+//! This module is the ideal interface we would have for the conductor (or other store that kitsune uses).
+//! We should update the conductor to match this interface.


Oh I was wondering what this module is for, I just noticed this comment. What do you mean though by updating the conductor, do you mean updating the whole KitsuneP2pEvent api?

Yeh exactly

Suggested change

//! This module is the ideal interface we would have for the conductor (or other store that kitsune uses).

//! We should update the conductor to match this interface.

//! This module is the ideal interface we would have for the conductor (or other store that kitsune uses).

//! We should update the KitsuneP2pEvent API to match this interface.

crates/kitsune_p2p/kitsune_p2p/src/gossip/sharded_gossip/tests/handler_builder.rs

maackle · 2021-08-02T17:45:57Z

crates/kitsune_p2p/kitsune_p2p/src/gossip/sharded_gossip/tests/test_local_sync.rs

+    // maackle: this kind of sucks. We have to await the handler task in order
+    //          to join any panics from that task into the test to cause a failure.
+    //          However, if there were no panics, then the task will never end,
+    //          so we need to wait "long enough" for any panics to occur
+    match tokio::time::timeout(tokio::time::Duration::from_secs(3), task).await {
+        // Err means timeout, which means no panic, which means the test passed
+        Err(_) => (),
+        // Ok means the task ended, which means there was a panic, which means the test should fail
+        Ok(err) => panic!("handler task panicked: {:?}", err),
+    }


We still need an adequate solution to this mess. @neonphog let's continue the discussion somewhere sometime.

crates/kitsune_p2p/kitsune_p2p/src/gossip/sharded_gossip/tests/test_two_nodes.rs

maackle · 2021-08-02T18:32:22Z

crates/kitsune_p2p/kitsune_p2p/src/gossip/sharded_gossip/store.rs

+    space: &Arc<KitsuneSpace>,
+    agents: impl Iterator<Item = &Arc<KitsuneAgent>>,
+    op_hashes: Vec<Arc<KitsuneOpHash>>,
+) -> KitsuneResult<Vec<(Arc<KitsuneOpHash>, Vec<u8>)>> {


Seems silly to put the op hash in an Arc and not the op data itself, which is larger. Seems to speak to the question of if whether want to always put our kitsune hash types in Arcs, or if we want to do it case-by-case.

We could put the data in an Arc too... but originally the hashes were in various HashSets so they actually got cloned, where the data was not. Once simple_bloom started keeping that sparse data map, perhaps it would have made sense to put an Arc around the data, because it was getting cloned, but the new gossip doesn't have a data map, right? so the data never gets cloned, and the Arc wouldn't help.

Yeh I don't think we should put this in an arc

fetch_op_hashes_for_constraints -> query_op_hashes find_remote_agent_within_arc -> find_remote_agent_within_arcset fetch_op_hash_data -> fetch_op_data

neonphog

I'm comfortable merging this when others are ready.

…e-gossip

maackle

🥳

freesig added 4 commits June 18, 2021 12:39

basic skeleton and test

6339222

Merge branch 'kitsune-arcsets-2' of github.com:/holochain/holochain i…

0be73df

…nto kitsune-gossip

local agents

509863a

stuff

96fa58a

Connoropolous reviewed Jun 18, 2021

View reviewed changes

Base automatically changed from kitsune-arcsets-2 to develop June 25, 2021 20:45

freesig and others added 4 commits June 29, 2021 14:08

Merge branch 'develop' into kitsune-gossip

71762db

Merge branch 'develop' of github.com:/holochain/holochain into kitsun…

478c53f

…e-gossip

working gossip for full sync

87ac37c

Port over fixed bench from develop which somehow got clobbered?

cb77388

maackle reviewed Jul 12, 2021

View reviewed changes

crates/holochain/tests/sharded_gossip.rs Outdated Show resolved Hide resolved

crates/kitsune_p2p/kitsune_p2p/src/spawn/actor/space.rs Outdated Show resolved Hide resolved

maackle added 2 commits July 12, 2021 11:07

Use ArcInterval instead of DhtArc where appropriate

2059b6e

DRY up simple_crud_zome

fbf8bb7

maackle reviewed Jul 12, 2021

View reviewed changes

crates/holochain/src/conductor/handle.rs Outdated Show resolved Hide resolved

maackle and others added 10 commits July 12, 2021 13:20

Small changes

f17f995

don't pull out duplicate ops

54658f3

Merge branch 'kitsune-gossip' of github.com:/holochain/holochain into…

5cadaf4

… kitsune-gossip

Make EventSender generic

25e5daa

Creating test-appropriate ShardedGossip instance

7a8f97f

MockKitsuneP2pEventSender

b09e71a

Separate ep_hnd from the rest, for testing

dc1bae3

Mock handler instead of sender

cd15185

Revert "Make EventSender generic"

5b9005b

This reverts commit 25e5daa.

spawn_handler function for tests

51a0c11

maackle force-pushed the kitsune-gossip branch from 0306773 to 5cadaf4 Compare July 19, 2021 18:53

maackle added 4 commits July 19, 2021 13:19

Remove shrinkwrap

ad39e27

Pull message queues into outer layer

f26f96f

Feasible unit test structure for gossip

3da06b1

Actually push new outgoing msgs onto queue

427bd4b

neonphog reviewed Jul 29, 2021

View reviewed changes

crates/kitsune_p2p/kitsune_p2p/src/gossip/sharded_gossip.rs Outdated Show resolved Hide resolved

neonphog reviewed Jul 29, 2021

View reviewed changes

crates/kitsune_p2p/kitsune_p2p/src/gossip/sharded_gossip.rs Outdated Show resolved Hide resolved

neonphog reviewed Jul 29, 2021

View reviewed changes

crates/kitsune_p2p/kitsune_p2p/src/gossip/sharded_gossip.rs Show resolved Hide resolved

neonphog reviewed Jul 29, 2021

View reviewed changes

crates/kitsune_p2p/kitsune_p2p/src/gossip/sharded_gossip.rs Outdated Show resolved Hide resolved

freesig added 7 commits July 30, 2021 13:15

batch calls, pr fixes

c87d449

Merge branch 'develop' of github.com:/holochain/holochain into kitsun…

14f0630

…e-gossip

fmt

8faa4c9

fix bug

d3b8416

bump mockall

0cb0972

fix flag

d0a9211

fix mock

6597882

maackle reviewed Jul 31, 2021

View reviewed changes

crates/holochain/src/conductor/handle.rs Outdated Show resolved Hide resolved

Fix sanity test by implementing broken KDirect method

280d25c

maackle reviewed Jul 31, 2021

View reviewed changes

maackle reviewed Aug 2, 2021

View reviewed changes

Apply suggestions from code review

65be482

maackle reviewed Aug 2, 2021

View reviewed changes

maackle added 4 commits August 2, 2021 11:45

Comment about OwnershipData constructor

651c2fb

Some function renames:

151cacb

fetch_op_hashes_for_constraints -> query_op_hashes find_remote_agent_within_arc -> find_remote_agent_within_arcset fetch_op_hash_data -> fetch_op_data

Bandwidth -> BandwidthThrottle + comment

7b5128f

fmt

6016106

neonphog approved these changes Aug 3, 2021

View reviewed changes

freesig added 2 commits August 5, 2021 12:53

Merge branch 'develop' of github.com:/holochain/holochain into kitsun…

6706064

…e-gossip

comments to tests

036b9e2

maackle approved these changes Aug 5, 2021

View reviewed changes

Merge branch 'develop' into kitsune-gossip

7086d14

freesig merged commit 9ed2a32 into develop Aug 5, 2021

freesig deleted the kitsune-gossip branch August 5, 2021 23:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kitsune Sharded Gossip #834

Kitsune Sharded Gossip #834

freesig commented Jun 18, 2021

Connoropolous Jun 18, 2021

freesig Jun 29, 2021

Connoropolous Jun 29, 2021

maackle Jul 12, 2021

maackle Jul 12, 2021

maackle Jul 12, 2021

Connoropolous Jul 12, 2021

maackle Jul 12, 2021

freesig Jul 30, 2021

maackle Jul 31, 2021

maackle Jul 31, 2021

freesig Aug 2, 2021

freesig Aug 2, 2021

maackle Aug 2, 2021

maackle Aug 3, 2021

maackle left a comment

maackle Jul 31, 2021

freesig Aug 2, 2021

maackle commented Aug 2, 2021

maackle left a comment

maackle Aug 2, 2021

freesig Aug 5, 2021

maackle Aug 5, 2021

maackle Aug 2, 2021

maackle Aug 2, 2021

neonphog Aug 2, 2021

freesig Aug 5, 2021

neonphog left a comment

maackle left a comment

		// FIXME: This sucks we have to iterate through the whole vec just to add Arcs.
		// Are arcs really saving us that much?

		//! This module is the ideal interface we would have for the conductor (or other store that kitsune uses).
		//! We should update the conductor to match this interface.

Kitsune Sharded Gossip #834

Kitsune Sharded Gossip #834

Conversation

freesig commented Jun 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maackle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maackle commented Aug 2, 2021

maackle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neonphog left a comment

Choose a reason for hiding this comment

maackle left a comment

Choose a reason for hiding this comment