Go IDONTWANT - Githubissues

libp2p / go-libp2p-pubsub

The PubSub implementation for go-libp2p

https://github.com/libp2p/specs/tree/master/pubsub

Other

309 stars 178 forks source link

Go IDONTWANT #553

Open ppopth opened 5 months ago

ppopth commented 5 months ago

Sending IDONTWANT

[x] Implement a smart queue
[x] Add priorities to the smart queue
[x] Put IDONTWANT packets into the smart priority queue as soon as the node gets the packets

Handling IDONTWANT

[x] Use a map to remember the message ids whose IDONTWANT packets have been received
[x] Implement max_idontwant_messages (ignore the IDONWANT packets if the max is reached)
[x] Clear the message IDs from the cache after 3 heartbeats
[x] Hash the message IDs before putting them into the cache.

Spec: https://github.com/libp2p/specs/pull/548

ppopth commented 4 months ago

However, I think using a slice to back the queue is problematic at many levels as it might leak space with a growing array, and also hang on to too much space.

The unused space will be freed eventually https://stackoverflow.com/a/26863706

ppopth commented 4 months ago

I'm not really sure if we need dfd81e80848e5ce4e64fbf30cfdaafb4dc56b9d3 or not. I guess adding a non-standard flag to pb/trace.proto would break some users of the tracers?

vyzo commented 3 months ago

I'm not really sure if we need dfd81e8 or not. I guess adding a non-standard flag to pb/trace.proto would break some users of the tracers?

Yeah, let's not break compatibility with existing code, we don't need it.

Any progress other than that?

ppopth commented 3 months ago

Any progress other than that?

I haven't implemented the second half. Does the first half looks good to you?

vyzo commented 3 months ago

yes, looks reasonable so far.

ppopth commented 2 months ago

@vyzo I finished everything already. Sorry for the delay. I was in vacation and was a bit busy lately. Thank you for your patience.

ppopth commented 2 months ago

The message ids from IDONTWANT can be very large, so hashing it before keeping it in memory quite makes sense. The same thing is already implemented in nim-libp2p https://github.com/vacp2p/nim-libp2p/pull/1090

vyzo commented 2 months ago

sure, but it needs to be cleared, otherwise its memroy leak and dos waiting to happen. Or at best the feature breaks itself for long running nodes.

I suggest for each IDW keep a counter of how many heartbeats it survived, and after 3 (to match the IHAVE ads) it should be cleared.

ppopth commented 3 weeks ago

@vyzo It's all done. Sorry for the delay.

vyzo commented 3 weeks ago

ok, can you rebase on master?