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.

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

identify: filter received addresses based on the node's remote address #2300

Merged
merged 1 commit into from
Jun 2, 2023

Conversation

marten-seemann
Copy link
Contributor

@marten-seemann marten-seemann commented May 18, 2023

Unfortunately, this is impossible to test. I tried using mocknet, but of course that thing is so horribly broken that it can't even reliably set the correct remote address of a connection. It fails at the most easy thing it was designed to do.

@marten-seemann marten-seemann mentioned this pull request May 18, 2023
27 tasks
@marten-seemann marten-seemann changed the base branch from identify-spurious-pushes to master May 18, 2023 12:03
@sukunrt
Copy link
Member

sukunrt commented May 19, 2023

If you want a connection with a remote addr on private IP and a remote addr on loopback ip, it should be possible to do this by listening on "0.0.0.0" address and dialing only the private or the loopback address. Testing public ips does look impossible.

Comment on lines 770 to 769
if _, err := cab.ConsumePeerRecord(signedPeerRecord, ttl); err != nil {
log.Debugf("error adding signed addrs to peerstore: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

for this to be effective, these addresses need to be filtered too, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. I've done some digging and convinced myself that the ConsumePeerRecord API is flawed. Will open a separate issue for that.

@marten-seemann
Copy link
Contributor Author

@sukunrt Should we merge this PR, and do the fixes for signed peer records when we tackle #2304?

@sukunrt
Copy link
Member

sukunrt commented Jun 1, 2023

Yes. We can take signed peer records separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants