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

[Merged by Bors] - Await listening address from libp2p in RPC tests setup #4705

Closed
wants to merge 1 commit into from
Closed

[Merged by Bors] - Await listening address from libp2p in RPC tests setup #4705

wants to merge 1 commit into from

Conversation

jmcph4
Copy link
Member

@jmcph4 jmcph4 commented Sep 6, 2023

Issue Addressed

#4704

Proposed Changes

  • Receive multiaddr from libp2p by awaiting listener setup

Additional Info

See also: #4675

@jmcph4 jmcph4 self-assigned this Sep 6, 2023
@jmcph4
Copy link
Member Author

jmcph4 commented Sep 6, 2023

Logging output seems to suggest that this is working as intended and all of the relevant tests are passing so I'm marking this as ready.

@jmcph4 jmcph4 added the ready-for-review The code is ready for review label Sep 6, 2023
jxs
jxs approved these changes Sep 6, 2023
View reviewed changes
Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

LGTM Jack

beacon_node/lighthouse_network/tests/common.rs
Comment on lines +98 to 99
let port = 0;
let config = build_config(port, boot_nodes);
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
let port = 0;
let config = build_config(port, boot_nodes);
let config = build_config(0, boot_nodes);

Copy link
Collaborator

@divagant-martian divagant-martian Sep 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I actually think it's more readable as he has it right now since it gives the extra context that 0 is a port, not a size, or a retry count etc

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah this is the only reason I didn't roll it into the build_config call directly.

@AgeManning AgeManning added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Sep 7, 2023
@AgeManning
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Sep 7, 2023
@jmcph4
Await listening address from libp2p in RPC tests setup (#4705)
7c04afd 
## Issue Addressed

#4704 

## Proposed Changes

 - Receive multiaddr from libp2p by awaiting listener setup

## Additional Info

See also: #4675
@bors
Copy link

bors bot commented Sep 7, 2023

Build failed:

@AgeManning
Copy link
Member

bors retry

bors bot pushed a commit that referenced this pull request Sep 11, 2023
@jmcph4
Await listening address from libp2p in RPC tests setup (#4705)
35f47f4 
## Issue Addressed

#4704 

## Proposed Changes

 - Receive multiaddr from libp2p by awaiting listener setup

## Additional Info

See also: #4675
@bors
Copy link

bors bot commented Sep 11, 2023

Pull request successfully merged into unstable.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title Await listening address from libp2p in RPC tests setup [Merged by Bors] - Await listening address from libp2p in RPC tests setup Sep 11, 2023
@bors bors bot closed this Sep 11, 2023
@divagant-martian divagant-martian mentioned this pull request Sep 12, 2023
Closed
This was referenced Sep 12, 2023
Closed
Closed
bors bot pushed a commit that referenced this pull request Sep 29, 2023
@jmcph4
Allow libp2p to determine listening addresses (#4700)
65f6772 
## Issue Addressed

#4675 

## Proposed Changes

 - Update local ENR (**only port numbers**) with local addresses received from libp2p (via `SwarmEvent::NewListenAddr`)
 - Only use the zero port for CLI tests

## Additional Info

### See Also ###

 - #4705 
 - #4402 
 - #4745
bors bot pushed a commit that referenced this pull request Sep 29, 2023
@jmcph4
Allow libp2p to determine listening addresses (#4700)
3004217 
## Issue Addressed

#4675 

## Proposed Changes

 - Update local ENR (**only port numbers**) with local addresses received from libp2p (via `SwarmEvent::NewListenAddr`)
 - Only use the zero port for CLI tests

## Additional Info

### See Also ###

 - #4705 
 - #4402 
 - #4745
bors bot pushed a commit that referenced this pull request Sep 29, 2023
@jmcph4
Allow libp2p to determine listening addresses (#4700)
d45e79b 
## Issue Addressed

#4675 

## Proposed Changes

 - Update local ENR (**only port numbers**) with local addresses received from libp2p (via `SwarmEvent::NewListenAddr`)
 - Only use the zero port for CLI tests

## Additional Info

### See Also ###

 - #4705 
 - #4402 
 - #4745
bors bot pushed a commit that referenced this pull request Sep 29, 2023
@jmcph4
Allow libp2p to determine listening addresses (#4700)
c9f48f3 
## Issue Addressed

#4675 

## Proposed Changes

 - Update local ENR (**only port numbers**) with local addresses received from libp2p (via `SwarmEvent::NewListenAddr`)
 - Only use the zero port for CLI tests

## Additional Info

### See Also ###

 - #4705 
 - #4402 
 - #4745
bors bot pushed a commit that referenced this pull request Oct 3, 2023
@jmcph4
Allow libp2p to determine listening addresses (#4700)
1c98806 
## Issue Addressed

#4675 

## Proposed Changes

 - Update local ENR (**only port numbers**) with local addresses received from libp2p (via `SwarmEvent::NewListenAddr`)
 - Only use the zero port for CLI tests

## Additional Info

### See Also ###

 - #4705 
 - #4402 
 - #4745
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
@jmcph4 @Woodpile37
Allow libp2p to determine listening addresses (sigp#4700)
63ed24e 
## Issue Addressed

sigp#4675 

## Proposed Changes

 - Update local ENR (**only port numbers**) with local addresses received from libp2p (via `SwarmEvent::NewListenAddr`)
 - Only use the zero port for CLI tests

## Additional Info

### See Also ###

 - sigp#4705 
 - sigp#4402 
 - sigp#4745
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
@jmcph4 @Woodpile37
Await listening address from libp2p in RPC tests setup (sigp#4705)
d9712f9 
## Issue Addressed

sigp#4704 

## Proposed Changes

 - Receive multiaddr from libp2p by awaiting listener setup

## Additional Info

See also: sigp#4675
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
@jmcph4 @Woodpile37
Allow libp2p to determine listening addresses (sigp#4700)
3b35917 
## Issue Addressed

sigp#4675 

## Proposed Changes

 - Update local ENR (**only port numbers**) with local addresses received from libp2p (via `SwarmEvent::NewListenAddr`)
 - Only use the zero port for CLI tests

## Additional Info

### See Also ###

 - sigp#4705 
 - sigp#4402 
 - sigp#4745
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
@jmcph4 @Woodpile37
Await listening address from libp2p in RPC tests setup (sigp#4705)
6491658 
## Issue Addressed

sigp#4704 

## Proposed Changes

 - Receive multiaddr from libp2p by awaiting listener setup

## Additional Info

See also: sigp#4675
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
@jmcph4 @Woodpile37
Allow libp2p to determine listening addresses (sigp#4700)
bb2090f 
## Issue Addressed

sigp#4675 

## Proposed Changes

 - Update local ENR (**only port numbers**) with local addresses received from libp2p (via `SwarmEvent::NewListenAddr`)
 - Only use the zero port for CLI tests

## Additional Info

### See Also ###

 - sigp#4705 
 - sigp#4402 
 - sigp#4745
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jxs jxs jxs approved these changes

@AgeManning AgeManning AgeManning approved these changes

@divagant-martian divagant-martian Awaiting requested review from divagant-martian

Assignees

@jmcph4 jmcph4

Labels
infra-ci Networking ready-for-merge This PR is ready to merge. test improvement Improve tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jmcph4 @AgeManning @jxs @divagant-martian