Implement Topics #126

emhane · 2022-06-03T17:10:02Z

How new connections are injected into topics kbuckets and the local routing table, will probably have to be revised and optimised to avoid having to update the same connection in different kbuckets and to avoid cloning enrs.

A lot of code has been reused purposely to show which parts of the discv5 pre-topics logic tie into the discv5 topics logic. A topic is a 32 byte hash that can be converted into the key of a kbucket entry, just as a node id can. An ADNODES (topics discv5) response body is identical to a NODES (old discv5) response body.

divagant-martian

I think this might be getting close to the finish line, some comments for today

divagant-martian · 2022-09-07T21:58:13Z

src/service.rs

+    /// KBuckets per topic hash.
+    topics_kbuckets: HashMap<TopicHash, KBucketsTable<NodeId, Enr>>,
+
+    /// The peers returned in a NODES response to a TOPICQUERY or REGTOPIC request are inserted in


Suggested change

/// The peers returned in a NODES response to a TOPICQUERY or REGTOPIC request are inserted in

/// The peers received in a NODES response to a TOPICQUERY or REGTOPIC request are inserted in

divagant-martian · 2022-09-09T15:39:10Z

Cargo.toml

@@ -21,6 +21,7 @@ zeroize = { version = "1.4.3", features = ["zeroize_derive"] }
 futures = "0.3.19"
 uint = { version = "0.9.1", default-features = false }
 rlp = "0.5.1"
+sha2 = "0.9.5"


this dep can be accessed directly from the enr crate, as is re-exported and having it here causes problems trying to upgrade

divagant-martian · 2022-10-18T17:32:27Z

src/discv5.rs

@@ -43,15 +45,36 @@ lazy_static! {
        RwLock::new(crate::PermitBanList::default());
 }

+/// Helper function that returns the hash of the given topic string according to the
+/// implemented hashing algorithm.
+pub static HASH: for<'a> fn(topic: &'a str) -> TopicHash = |topic| {


This part is not necessary

Suggested change

pub static HASH: for<'a> fn(topic: &'a str) -> TopicHash = |topic| {

pub static HASH: fn(topic: &str) -> TopicHash = |topic| {

divagant-martian · 2022-10-18T17:40:54Z

src/discv5.rs

            config.incoming_bucket_limit,
            table_filter,
            bucket_filter,
        )));

+        println!("{:?}", local_enr.read().get(ENR_KEY_FEATURES).unwrap());


tracing::info is probably better here with a nice message

divagant-martian · 2022-10-18T20:29:23Z

src/discv5.rs

+            // the service will verify if this node is contactable, we just send it and
+            // await a response.


copy paste error?

divagant-martian · 2022-10-19T20:17:10Z

src/service.rs

+                    let mut topic_item = topics_to_reg_iter.next();
+                    while let Some((topic, _topic_hash)) = topic_item {
+                        trace!("Publishing topic {} with hash {}", topic, topic.hash());
+                        topic_item = topics_to_reg_iter.next();


I couldn't find a particular reason to do this using an extra variable. This might be a more idiomatic way of doing the same

Suggested change

let mut topic_item = topics_to_reg_iter.next();

while let Some((topic, _topic_hash)) = topic_item {

trace!("Publishing topic {} with hash {}", topic, topic.hash());

topic_item = topics_to_reg_iter.next();

while let Some((topic, _topic_hash)) = topics_to_reg_iter.next() {

trace!("Publishing topic {} with hash {}", topic, topic.hash());

divagant-martian · 2022-10-19T20:20:47Z

src/service.rs

+                        if sent_regtopics >= MAX_REGTOPICS_REGISTER_PER_INTERVAL {
+                            break
+                        }


This part might a bit problematic since what an user would expect is any topic they want to register should be kept alive, I think this could this promise. What do you think?

divagant-martian · 2022-10-19T20:22:09Z

src/service.rs

+                    if topics_to_reg_iter.next().is_none() {
+                        topics_to_reg_iter = self.registration_attempts.keys().map(|topic| (topic.clone(), topic.hash())).collect::<Vec<(Topic, TopicHash)>>().into_iter();
+                    }


So checking this I assume the only reason to do this, having a mutable iterator that needs to be stopped in a poll iteration is the MAX_REGTOPICS_REGISTER_PER_INTERVAL. Do you remember why we have this?

divagant-martian · 2022-10-19T20:32:39Z

src/service.rs

+        }
+    }
+
+    fn get_active_topics(&mut self) -> HashMap<Topic, Vec<NodeId>> {


do you think this kind of maintenance could be done on an interval for example? in order to remove the mutability of a GET service function. We could still do this maintenance step when the call is done but at least it wouldn't be the only moment in which is done

We should also move the maintenance code to it's own function (it's most of the code, yes) but it's repeated at least once more in the send_register_topics logic

divagant-martian · 2022-10-19T20:57:48Z

src/service.rs

+        let encoded_topics_field = topics_field(topic.clone()).encode();
+
+        let enr_size = self.local_enr.read().size() + encoded_topics_field.len();
+        if enr_size >= 300 {


would be nicer as a constant

emhane · 2023-04-03T18:07:38Z

Protocol outdated, closing.

emhane · 2024-01-23T00:46:53Z

this will take a bit longer after all

emhane requested review from AgeManning and divagant-martian June 3, 2022 17:10

emhane linked an issue Jun 3, 2022 that may be closed by this pull request

[Tracking] Implement Topics #125

Open

emhane changed the title ~~Tickets v2~~ Implement Topics Jun 3, 2022

emhane force-pushed the tickets-v2 branch 2 times, most recently from 93cca33 to 7676752 Compare June 16, 2022 20:57

emhane force-pushed the tickets-v2 branch from ee45d46 to f7a3d36 Compare June 17, 2022 20:59

emhane added 23 commits June 21, 2022 10:53

Run cargo fmt

df74c4b

Move hashing of topic up one layer to app

28188d0

Remove unused deps

8c8b1ff

Fix send_register_topics logic

bbe4329

Add warning

622ca2b

Fix regtopics

1746aaf

Fix regtopics

066e2a5

Allow NODES repsonse for RETOPIC

dd12411

Match NODES response for REGTOPIC req

b914ac4

Add hack for topic query double nodes response

ed26c95

Add warning to topic query double nodes response logic

ea80eab

Add debug info

f817aee

Add debug info

741c19a

Add debug info

df6fdb1

Add debug messages

1b9e9c2

Update regtopic active requests for addtional NODES response

003b816

Amplify distances reqeusted for topics

505aff4

Add trace for responses

fe5a5d0

Add trace messages

c3311e6

Add trace message

65cc66c

Fix reinsertion of active req handler

3916017

Ignore wait time zero tickets

b139738

Add trace

fdea8b2

emhane added 5 commits September 8, 2022 00:59

Make use of the fact that nodes can only register themselves as ads

35201e3

Move repeated code to closure

4baeafe

Clean up and comment

ef1d76a

Remove unused methods

ad01569

Correct name

0b09f60

emhane force-pushed the tickets-v2 branch from 39c8342 to 0b09f60 Compare September 8, 2022 11:02

emhane added 6 commits September 8, 2022 16:09

Run cargo fmt

3111592

Merge branch 'master' of github.com:sigp/discv5 into tickets-v2

8386fba

Change name

f2caf4a

Fix merge conflict

2d5954d

Fix bad return bug

66f43ae

fixup! Fix bad return bug

b699290

divagant-martian added work-in-progress Work in progress waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Sep 9, 2022

Set features in ENR

86b7491

emhane force-pushed the tickets-v2 branch from 8f5bbe6 to 86b7491 Compare September 9, 2022 16:49

Return whole topic for human readable name for logs

5b2209a

emhane added ready-for-review and removed work-in-progress Work in progress waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Sep 9, 2022

emhane added 3 commits September 9, 2022 19:47

Only log events for topics to maintain backwards compatibility

a18ae8c

fixup! Only log events for topics to maintain backwards compatibility

b37e1b1

fixup! Only log events for topics to maintain backwards compatibility

69cd507

divagant-martian reviewed Oct 19, 2022

View reviewed changes

AgeManning mentioned this pull request Dec 14, 2022

Support configuring a different protocol id #154

Closed

4 tasks

emhane closed this Apr 3, 2023

emhane reopened this Jan 22, 2024

emhane closed this Jan 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Topics #126

Implement Topics #126

emhane commented Jun 3, 2022 •

edited

Loading

divagant-martian left a comment

divagant-martian Sep 7, 2022

divagant-martian Sep 9, 2022

divagant-martian Oct 18, 2022

divagant-martian Oct 18, 2022

divagant-martian Oct 18, 2022

divagant-martian Oct 19, 2022

divagant-martian Oct 19, 2022

divagant-martian Oct 19, 2022

divagant-martian Oct 19, 2022

divagant-martian Oct 19, 2022

divagant-martian Oct 19, 2022

emhane commented Apr 3, 2023

emhane commented Jan 23, 2024

	/// The peers returned in a NODES response to a TOPICQUERY or REGTOPIC request are inserted in
	/// The peers received in a NODES response to a TOPICQUERY or REGTOPIC request are inserted in

	pub static HASH: for<'a> fn(topic: &'a str) -> TopicHash = \|topic\| {
	pub static HASH: fn(topic: &str) -> TopicHash = \|topic\| {

		// the service will verify if this node is contactable, we just send it and
		// await a response.

Implement Topics #126

Implement Topics #126

Conversation

emhane commented Jun 3, 2022 • edited Loading

divagant-martian 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

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

emhane commented Apr 3, 2023

emhane commented Jan 23, 2024

emhane commented Jun 3, 2022 •

edited

Loading