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.

Sign up for GitHub

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

Jump to bottom

GossipSub v1.2: IDONTWANT control message and priority queue. #553

Merged
merged 31 commits into from
Aug 16, 2024
Merged

GossipSub v1.2: IDONTWANT control message and priority queue. #553

merged 31 commits into from
Aug 16, 2024

Conversation

ppopth
Copy link
Contributor

@ppopth ppopth commented Jan 15, 2024 •
edited
Loading

Sending IDONTWANT

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

Handling IDONTWANT

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

More requested features

  • Add a feature test to not send IDONTWANT if the other side doesnt support it

Spec: libp2p/specs#548

@vyzo vyzo self-requested a review January 15, 2024 18:34
vyzo
vyzo reviewed Feb 10, 2024
View reviewed changes
Copy link
Collaborator

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

So far looking good overall.

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.

I would very much prefer using a dequeue for this.

comm.go Show resolved Hide resolved
rpc_queue.go Outdated Show resolved Hide resolved
rpc_queue.go Outdated Show resolved Hide resolved
@ppopth
Copy link
Contributor Author

ppopth commented Mar 2, 2024

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
Copy link
Contributor Author

ppopth commented Mar 5, 2024

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?

@vyzo
Copy link
Collaborator

vyzo commented Mar 22, 2024

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
Copy link
Contributor Author

ppopth commented Apr 8, 2024

Any progress other than that?

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

@vyzo
Copy link
Collaborator

vyzo commented Apr 8, 2024

yes, looks reasonable so far.

@ppopth
Copy link
Contributor Author

ppopth commented May 1, 2024

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

vyzo
vyzo reviewed May 2, 2024
View reviewed changes
Copy link
Collaborator

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

Generally looks good, but lets make the counter clear less aggressive.

gossipsub.go Outdated Show resolved Hide resolved
gossipsub.go Show resolved Hide resolved
@ppopth
Copy link
Contributor Author

ppopth commented May 3, 2024

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 vacp2p/nim-libp2p#1090

@vyzo
Copy link
Collaborator

vyzo commented May 3, 2024 •
edited
Loading

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
Copy link
Contributor Author

ppopth commented Jun 11, 2024

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

@vyzo
Copy link
Collaborator

vyzo commented Jun 14, 2024

ok, can you rebase on master?

@ppopth ppopth force-pushed the idonthave branch 2 times, most recently from 11fed73 to d44d15a Compare June 20, 2024 05:42
@chaitanyaprem
Copy link
Contributor

@vyzo wondering if this PR ready to be merged or are you waiting for anything to be fixed/addressed?

This seems to improve bandwidth usage and we want to take advantage of that.

vyzo
vyzo reviewed Aug 6, 2024
View reviewed changes
Copy link
Collaborator

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

I think in general it looks good, but i do want to do another pass.

In the meantime:

  • can you bump the version to 1.2
  • can you add a feature test to not send IDONTWANT if the other side doesnt support it?

I will also invoke @Stebalien for review.

@vyzo vyzo requested a review from Stebalien August 6, 2024 15:13
@chaitanyaprem
Copy link
Contributor

I think in general it looks good, but i do want to do another pass.

In the meantime:

* can you bump the version to 1.2

* can you add a feature test to not send IDONTWANT if the other side doesnt support it?

I will also invoke @Stebalien for review.

Haven't looked at the PR in detail, but will give it a try tomorrow.

Stebalien
Stebalien reviewed Aug 6, 2024
View reviewed changes
rpc_queue.go Outdated Show resolved Hide resolved
rpc_queue.go Outdated Show resolved Hide resolved
rpc_queue.go Outdated Show resolved Hide resolved
rpc_queue.go Outdated Show resolved Hide resolved
comm.go Outdated Show resolved Hide resolved
gossipsub.go Show resolved Hide resolved
gossipsub.go Outdated Show resolved Hide resolved
@chaitanyaprem chaitanyaprem mentioned this pull request Aug 7, 2024
Open
3 tasks
@ppopth
Replace sending channel with the smart rpcQueue
6877d7b 
Since we want to implement a priority queue later, we need to replace
the normal sending channels with the new smart structures first.
@ppopth
Implement UrgentPush in the smart rpcQueue
7ddc486 
UrgentPush allows you to push an rpc packet to the front of the queue so
that it will be popped out fast.
@ppopth
Add IDONTWANT to rpc.proto and trace.proto
dc45ebb
@ppopth
Send IDONTWANT right before validation step
f06b3b2 
Most importantly, this commit adds a new method called PreValidation to
the interface PubSubRouter, which will be called right before validating
the gossipsub message.

In GossipSubRouter, PreValidation will send the IDONTWANT controll
messages to all the mesh peers of the topics of the received messages.
@ppopth
Test GossipSub IDONWANT sending
22d9773
@ppopth
Send IDONWANT only for large messages
62daad4
@ppopth
Handle IDONTWANT control messages
53b1393 
When receiving IDONTWANTs, the host should remember the message ids
contained in IDONTWANTs using a hash map.

When receiving messages with those ids, it shouldn't forward them to the
peers who already sent the IDONTWANTs.

When the maximum number of IDONTWANTs is reached for any particular
peer, the host should ignore any excessive IDONTWANTs from that peer.
@ppopth
Clear expired message IDs from the IDONTWANT cache
4e930a7 
If the messages IDs received from IDONTWANTs are older than 3
heartbeats, they should be removed from the IDONTWANT cache.
@ppopth
Keep the hashes of IDONTWANT message ids instead
1afead5 
Rather than keeping the raw message ids, keep their hashes instead to
save memory and protect again memory DoS attacks.
@ppopth
Copy link
Contributor Author

ppopth commented Aug 15, 2024

@vyzo I didn't get any error with FuzzAppendOrMergeRPC, did you?

$ go test -v -run FuzzAppendOrMergeRPC
=== RUN   FuzzAppendOrMergeRPC
--- PASS: FuzzAppendOrMergeRPC (0.00s)
PASS
ok  	github.com/libp2p/go-libp2p-pubsub	0.005s

@vyzo
Copy link
Collaborator

vyzo commented Aug 15, 2024

thats a flake then.
Ok, disable and open follow up issue!

@ppopth ppopth mentioned this pull request Aug 15, 2024
Open
@ppopth
Copy link
Contributor Author

ppopth commented Aug 15, 2024

done

@vyzo
Copy link
Collaborator

vyzo commented Aug 15, 2024

CI failures - seems like a faulty commit.

@vyzo
Copy link
Collaborator

vyzo commented Aug 15, 2024

@ppopth let's reenable the Fuzz test, it seems it is fine: see #574

Stebalien
Stebalien reviewed Aug 15, 2024
View reviewed changes
gossipsub.go Show resolved Hide resolved
gossipsub.go Outdated Show resolved Hide resolved
rpc_queue.go Outdated Show resolved Hide resolved
rpc_queue.go Outdated Show resolved Hide resolved
@vyzo vyzo mentioned this pull request Aug 15, 2024
Merged
@ppopth
Copy link
Contributor Author

ppopth commented Aug 16, 2024

@Stebalien @vyzo I think it's all done

@vyzo
Copy link
Collaborator

vyzo commented Aug 16, 2024

running the CI, will merge once it greens.

@ppopth
Copy link
Contributor Author

ppopth commented Aug 16, 2024

should I squash the fix-up commits?

vyzo
vyzo approved these changes Aug 16, 2024
View reviewed changes
Copy link
Collaborator

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you!

@vyzo
Copy link
Collaborator

vyzo commented Aug 16, 2024 •
edited
Loading

should I squash the fix-up commits?

no need, I will squash merge anyway

@vyzo vyzo changed the title Go IDONTWANT GossipSub v1.2: IDONTWANT control message and priority queue. Aug 16, 2024
@vyzo vyzo merged commit b421b3a into libp2p:master Aug 16, 2024
7 checks passed
@vyzo vyzo mentioned this pull request Aug 16, 2024
Merged
@ppopth ppopth deleted the idonthave branch August 19, 2024 03:41
@cortze cortze mentioned this pull request Sep 24, 2024
Merged
@nisdas nisdas mentioned this pull request Sep 24, 2024
Merged
@cortze cortze mentioned this pull request Sep 25, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cortze cortze cortze left review comments

@Stebalien Stebalien Stebalien approved these changes

@vyzo vyzo vyzo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ppopth @vyzo @chaitanyaprem @Stebalien @cortze