IDONTWANT message optimisation to cutoff for smaller messages #6456

hopinheimer · 2024-10-02T18:03:24Z

addresses #6437

AgeManning

Nice, thanks for the help.

beacon_node/lighthouse_network/gossipsub/src/behaviour.rs

AgeManning · 2024-10-03T01:06:33Z

beacon_node/lighthouse_network/gossipsub/src/config.rs

+    // traffic and CPU load. This acts as a lower bound cutoff for the message size to which 
+    // IDONTWANT won't be sent to peers. Only works if the peers support Gossipsub1.3 
+    // (see https://github.com/libp2p/specs/blob/master/pubsub/gossipsub/gossipsub-v1.2.md#idontwant-message)
+    // default is 1kb


Might be worth mentioning that the unit for the usize value is bytes.

I always mess this up. resolved!

jxs

Thanks! You need to cargo fmt to pass the check code CI job. The tests also need to be updated, and we should probably add another test for this case, i.e. to not send IDONTWANT when the size is inferior to the limit

jxs · 2024-10-04T14:17:46Z

beacon_node/lighthouse_network/gossipsub/src/behaviour.rs

            return;
        };

+        if message.raw_protobuf_len() < self.config.idontwant_message_size_threshold(){


can we move this check to line 1833? So that it's explicit that we are only calling send_idontwant if the message size is inferior to the threshold instead of having the function decide that.

also the the logic was a little off. I have changed it and written passing test for the same

beacon_node/lighthouse_network/gossipsub/src/config.rs

hopinheimer · 2024-10-08T05:46:03Z

waiting on #6436 to build the cli docs. I use a mac

pawanjay176 · 2024-10-10T19:21:55Z

beacon_node/lighthouse_network/src/config.rs

            outbound_rate_limiter_config: None,
            invalid_block_storage: None,
            inbound_rate_limiter_config: None,
+            idontwant_message_size_threshold: 1000,


It'd be good to extract the default value into a IDONTWANT_MESSAGE_SIZE_THRESHOLD const value.

sounds reasonable. made the requested changes

target_peers few line above also seems to have inline declaration should I extract it too?

jxs

LGTM! cc @AgeManning if you wanna take a final look

AgeManning · 2024-10-14T22:22:29Z

aha, i was trying to fix the CI, but getting merge conflicts. You beat me to it. :). Thanks

hopinheimer · 2024-10-14T23:11:22Z

haha, Imma ss this!

AgeManning · 2024-10-15T00:54:58Z

@Mergifyio queue

mergify · 2024-10-15T00:55:13Z

queue

🛑 The pull request has been removed from the queue `default`

The merge conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

jxs · 2024-10-16T23:35:05Z

@Mergifyio queue

mergify · 2024-10-16T23:35:15Z

queue

🛑 The pull request has been removed from the queue `default`

The merge conditions cannot be satisfied due to checks timeout.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

michaelsproul · 2024-10-17T03:54:18Z

@mergify dequeue

mergify · 2024-10-17T03:54:27Z

dequeue

☑️ The pull request is not queued

michaelsproul · 2024-10-17T03:54:33Z

some stuff got stuck, so just removing this from the queue for now

michaelsproul · 2024-10-17T05:13:10Z

@mergify queue

mergify · 2024-10-17T05:13:53Z

queue

🛑 The pull request has been removed from the queue `default`

The merge conditions cannot be satisfied due to checks timeout.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

michaelsproul · 2024-10-17T07:48:21Z

@mergify requeue

mergify · 2024-10-17T07:48:35Z

requeue

✅ This pull request will be re-embarked automatically

The followup queue command will be automatically executed to re-embark the pull request

mergify · 2024-10-17T07:48:40Z

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 606a113

) * idontwant message opitmising * requested changes and linter appeasing * added the config cli flag * Merge branch 'unstable' into fix/idontwant-optimise * cli docs generated * const declaration * Hide extra technical cli flag * passing ci * Merge branch 'unstable' into fix/idontwant-optimise

idontwant message opitmising

6d28509

hopinheimer changed the title ~~idontwant message optimisation~~ IDONTWANT message optimisation to cutoff for smaller messages Oct 2, 2024

jimmygchen requested review from AgeManning and jxs and removed request for AgeManning October 3, 2024 01:03

jimmygchen added the Networking label Oct 3, 2024

jimmygchen requested a review from AgeManning October 3, 2024 01:03

jimmygchen added the ready-for-review The code is ready for review label Oct 3, 2024

AgeManning reviewed Oct 3, 2024

View reviewed changes

requested changes and linter appeasing

205078a

hopinheimer requested a review from AgeManning October 3, 2024 05:06

jxs reviewed Oct 4, 2024

View reviewed changes

pawanjay176 reviewed Oct 4, 2024

View reviewed changes

beacon_node/lighthouse_network/gossipsub/src/config.rs Outdated Show resolved Hide resolved

added the config cli flag

b59aee0

hopinheimer added 2 commits October 10, 2024 08:31

Merge branch 'unstable' into fix/idontwant-optimise

a7c7d9d

cli docs generated

98dba97

pawanjay176 reviewed Oct 10, 2024

View reviewed changes

const declaration

9e92d95

hopinheimer requested a review from pawanjay176 October 11, 2024 00:43

jxs approved these changes Oct 12, 2024

View reviewed changes

Hide extra technical cli flag

4d657e6

AgeManning approved these changes Oct 14, 2024

View reviewed changes

passing ci

1aad5ad

mergify bot added a commit that referenced this pull request Oct 15, 2024

Merge of #6456

fa63801

mergify bot mentioned this pull request Oct 15, 2024

merge queue: embarking unstable (2e440df) and #6456 together #6487

Closed

5 tasks

Merge branch 'unstable' into fix/idontwant-optimise

aac93f5

mergify bot added a commit that referenced this pull request Oct 16, 2024

Merge of #6456

b6c33cc

mergify bot mentioned this pull request Oct 16, 2024

merge queue: embarking unstable (772929f) and #6456 together #6499

Closed

5 tasks

michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Oct 17, 2024

mergify bot added a commit that referenced this pull request Oct 17, 2024

Merge of #6456

900359d

mergify bot mentioned this pull request Oct 17, 2024

merge queue: embarking unstable (7091c78) and #6456 together #6505

Closed

5 tasks

mergify bot merged commit 606a113 into sigp:unstable Oct 17, 2024
29 checks passed

jimmygchen mentioned this pull request Oct 22, 2024

Possible upgrade to lighthouse's IDONTWANT GossipSub implementation #6437

Closed

IDONTWANT message optimisation to cutoff for smaller messages #6456

IDONTWANT message optimisation to cutoff for smaller messages #6456

Uh oh!

Conversation

hopinheimer commented Oct 2, 2024

Uh oh!

AgeManning left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AgeManning Oct 3, 2024

Choose a reason for hiding this comment

Uh oh!

hopinheimer Oct 3, 2024

Choose a reason for hiding this comment

Uh oh!

jxs left a comment

Choose a reason for hiding this comment

Uh oh!

jxs Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

hopinheimer Oct 5, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hopinheimer commented Oct 8, 2024

Uh oh!

pawanjay176 Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

hopinheimer Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

hopinheimer Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

jxs left a comment

Choose a reason for hiding this comment

Uh oh!

AgeManning commented Oct 14, 2024

Uh oh!

hopinheimer commented Oct 14, 2024

Uh oh!

AgeManning commented Oct 15, 2024

Uh oh!

mergify bot commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🛑 The pull request has been removed from the queue default

Uh oh!

jxs commented Oct 16, 2024

Uh oh!

mergify bot commented Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🛑 The pull request has been removed from the queue default

Uh oh!

michaelsproul commented Oct 17, 2024

Uh oh!

mergify bot commented Oct 17, 2024

☑️ The pull request is not queued

Uh oh!

michaelsproul commented Oct 17, 2024

Uh oh!

michaelsproul commented Oct 17, 2024

Uh oh!

mergify bot commented Oct 17, 2024

🛑 The pull request has been removed from the queue default

Uh oh!

michaelsproul commented Oct 17, 2024

Uh oh!

mergify bot commented Oct 17, 2024

✅ This pull request will be re-embarked automatically

Uh oh!

mergify bot commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ The pull request has been merged automatically

Uh oh!

Uh oh!

mergify bot commented Oct 15, 2024 •

edited

Loading

🛑 The pull request has been removed from the queue `default`

mergify bot commented Oct 16, 2024 •

edited

Loading

🛑 The pull request has been removed from the queue `default`

🛑 The pull request has been removed from the queue `default`

mergify bot commented Oct 17, 2024 •

edited

Loading