Fix test_ignore_too_many_messages_in_ihave test#5
Merged
jxs merged 2 commits intojxs:impl-gossipsub-idontwantfrom May 1, 2024
Merged
Conversation
jxs
approved these changes
May 1, 2024
Owner
|
Thanks for this @ackintosh! |
jxs
added a commit
that referenced
this pull request
Jul 22, 2024
* move gossipsub into a separate crate * Merge branch 'unstable' of github.com:sigp/lighthouse into separate-gossipsub * update rpc.proto and generate rust bindings * gossipsub: implement IDONTWANT messages * address review * move GossipPromises out of PeerScore * impl PeerKind::is_gossipsub that returns true if peer speaks any version of gossipsub * address review 2 * Merge branch 'separate-gossipsub' of github.com:sigp/lighthouse into impl-gossipsub-idontwant * Merge branch 'unstable' of github.com:sigp/lighthouse into impl-gossipsub-idontwant * add metrics * add tests * make 1.2 beta before spec is merged * Merge branch 'unstable' of github.com:sigp/lighthouse into impl-gossipsub-idontwant * cargo clippy * Collect decoded IDONTWANT messages * Use the beta tag in most places to simplify the transition * Fix failed test by using fresh message-ids * Gossipsub v1.2-beta * Merge latest unstable * Cargo update * Merge pull request #5 from ackintosh/impl-gossipsub-idontwant-ackintosh-fix-test Fix `test_ignore_too_many_messages_in_ihave` test * Merge branch 'unstable' of github.com:sigp/lighthouse into impl-gossipsub-idontwant * update CHANGELOG.md * remove beta for 1.2 IDONTWANT spec has been merged * Merge branch 'unstable' of github.com:sigp/lighthouse into impl-gossipsub-idontwant * Merge branch 'impl-gossipsub-idontwant' of github.com:jxs/lighthouse into impl-gossipsub-idontwant * Merge branch 'unstable' of github.com:sigp/lighthouse into impl-gossipsub-idontwant * improve comments wording * Merge branch 'impl-gossipsub-idontwant' of github.com:jxs/lighthouse into impl-gossipsub-idontwant
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
sigp#5422
Issue Addressed
Test
test_ignore_too_many_messages_in_ihaveis failing in this branch:https://github.com/sigp/lighthouse/actions/runs/8796045770/job/24138172575
Here is the assertion that is failing:
lighthouse/beacon_node/lighthouse_network/gossipsub/src/behaviour/tests.rs
Line 4647 in 5946982
Proposed Changes
From my investigation, message-ids that are expected to send iwant are being filtered out here,
!self.gossip_promises.contains(id):lighthouse/beacon_node/lighthouse_network/gossipsub/src/behaviour.rs
Lines 1259 to 1266 in 0cc18cc
I think this was not intended for this test.
To avoid unexpected filtering, I have changed the test to use fresh message-ids (specifically,
message_ids[20..30]).