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

Add test case for discv4 bootnodes behaviour #14643

Open
mattsse opened this issue Feb 21, 2025 · 4 comments · May be fixed by #14655
Open

Add test case for discv4 bootnodes behaviour #14643

mattsse opened this issue Feb 21, 2025 · 4 comments · May be fixed by #14655
Assignees
@SoarinSkySagar
Labels
C-enhancement New feature or request C-test A change that impacts how or what we test D-good-first-issue Nice and easy! A great choice to get started

Comments

@mattsse
Copy link
Collaborator

mattsse commented Feb 21, 2025

Describe the feature

this comment states that bootnodes are currently not emitted as an event:

reth/crates/net/discv4/src/lib.rs

Lines 673 to 675 in 86e8a2a

/// This is similar to adding all bootnodes via [`Self::add_node`], but does not fire a
/// [`DiscoveryUpdate::Added`] event for the given bootnodes. So boot nodes don't appear in the
/// update stream, which is usually desirable, since bootnodes should not be connected to.

we don't have a test for this behaviour, and I think this is no longer the case since #1984

TODO

  • add a test case that creates two discv4 instances and launches one with the other as bootnode
  • check if an update event is emitted

Additional context

see existing test cases for reference
@mattsse mattsse added C-enhancement New feature or request S-needs-triage This issue needs to be labelled labels Feb 21, 2025
@mattsse mattsse added D-good-first-issue Nice and easy! A great choice to get started C-test A change that impacts how or what we test and removed S-needs-triage This issue needs to be labelled labels Feb 21, 2025
@frankudoags
Copy link
Contributor

Can I take this? @mattsse

@SoarinSkySagar
Copy link
Contributor

May I have this one?

@mattsse
Copy link
Collaborator Author

mattsse commented Feb 21, 2025

assigning @SoarinSkySagar for this

@frankudoags will find you something else after #14613

@frankudoags
Copy link
Contributor

fairs

@SoarinSkySagar SoarinSkySagar linked a pull request Feb 22, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@SoarinSkySagar SoarinSkySagar

Labels
C-enhancement New feature or request C-test A change that impacts how or what we test D-good-first-issue Nice and easy! A great choice to get started
Projects
Reth Tracker
Status: Todo
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Test case for discv4 bootnodes behaviour SoarinSkySagar/reth
3 participants
@mattsse @frankudoags @SoarinSkySagar