Skip to content

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

Closed
jmcph4 wants to merge 1 commit intosigp:unstablefrom
jmcph4:rpc-test-bind-zero
Closed

[Merged by Bors] - Await listening address from libp2p in RPC tests setup#4705
jmcph4 wants to merge 1 commit intosigp:unstablefrom
jmcph4:rpc-test-bind-zero

Conversation

@jmcph4
Copy link
Contributor

@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
@divagant-martian divagant-martian self-requested a review September 6, 2023 03:48
@jmcph4
Copy link
Contributor 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
@jmcph4 jmcph4 requested a review from AgeManning September 6, 2023 05:04
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

@jxs jxs Sep 6, 2023

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
Contributor

@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
Contributor Author

@jmcph4 jmcph4 Sep 7, 2023

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

AgeManning commented Sep 7, 2023

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

AgeManning commented Sep 11, 2023

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