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

holepunch: add multiaddress filter #1839

Merged
merged 7 commits into from
Nov 17, 2022

Conversation

dennis-tra
Copy link
Contributor

@dennis-tra dennis-tra commented Oct 24, 2022

This PR adds a new option to the holepunch service that allows filtering of multi addresses during the DCUtR protocol.

@dennis-tra dennis-tra changed the title feat: add holepunch address filter option holepunch: add multi address filter Oct 24, 2022
@dennis-tra dennis-tra marked this pull request as ready for review October 24, 2022 17:02
@marten-seemann marten-seemann changed the title holepunch: add multi address filter holepunch: add multiaddress filter Oct 27, 2022
p2p/protocol/holepunch/filter.go Outdated Show resolved Hide resolved
p2p/protocol/holepunch/filter.go Outdated Show resolved Hide resolved
p2p/protocol/holepunch/filter.go Outdated Show resolved Hide resolved
p2p/protocol/holepunch/filter.go Outdated Show resolved Hide resolved
@marten-seemann marten-seemann mentioned this pull request Oct 31, 2022
34 tasks
// It also allows to only consider a subset of received multi addresses
// that remote peers announced to us.
// Theoretically, this API also allows to add multi addresses in both cases.
func WithAddrFilter(maf AddrFilter) Option {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm inconsistently referring to the multiaddress filter as AddrFilter or just filter (see L16) 🤷‍♂️

p2p/protocol/holepunch/filter.go Outdated Show resolved Hide resolved
p2p/protocol/holepunch/filter.go Outdated Show resolved Hide resolved
@marten-seemann
Copy link
Contributor

Code LGTM, but CI still seems unhappy.

@marten-seemann
Copy link
Contributor

@dennis-tra Can you take a look at this?

@dennis-tra
Copy link
Contributor Author

dennis-tra commented Nov 10, 2022

@marten-seemann I don't get why CI is so angry at me #worksonmymachine

There was indeed a failing test that I have fixed. Now, when I run the holepunch package tests in isolation all pass. After I did that I ran the whole test suite locally and I got the below error (same as CI). Then I ran the suite again and now it passes (as the results are cached I believe).

The error is:

      holepunch_test.go:236: 
          	Error Trace:	/home/runner/work/go-libp2p/go-libp2p/p2p/protocol/holepunch/holepunch_test.go:236
          	Error:      	"failed to open hole-punching stream: protocol not supported" does not contain "aborting hole punch initiation as we have no public address"
          	Test:       	TestFailuresOnInitiator/no_addrs_after_filtering

Could it be that there's an other interfering test? Perhaps a mocked host not closed?

@marten-seemann
Copy link
Contributor

Now, when I run the holepunch package tests in isolation all pass. After I did that I ran the whole test suite locally and I got the below error (same as CI). Then I ran the suite again and now it passes (as the results are cached I believe).

That sounds like a flaky test. Go runs the tests from different packages in separate binaries, so it shouldn't be possible that tests from another package influence your holepunching tests.

@marten-seemann
Copy link
Contributor

@dennis-tra Looks like there was a race condition in the test. I added a fix in dennis-tra#1, but since this is in your fork, I can't apply it here. Can you please that PR?

@dennis-tra
Copy link
Contributor Author

dennis-tra commented Nov 17, 2022

@marten-seemann CI seems to be happier - but not quite super happy. Some crypto test is still failing:

=== RUN   TestPreferRelayV2
2022/11/17 10:35:16 failed to sufficiently increase receive buffer size (was: 208 kiB, wanted: 2048 kiB, got: 416 kiB). See https://github.com/lucas-clemente/quic-go/wiki/UDP-Receive-Buffer-Size for details.
    autorelay_test.go:192: 
        	Error Trace:	/home/runner/work/go-libp2p/go-libp2p/p2p/host/autorelay/autorelay_test.go:192
        	Error:      	Condition never satisfied
        	Test:       	TestPreferRelayV2
--- FAIL: TestPreferRelayV2 (3.03s)

Seems to be an unrelated test. Would it suffice to rerun the Go Cryto test and hope that it then works? 🤞

@marten-seemann marten-seemann merged commit f4ddf59 into libp2p:master Nov 17, 2022
@dennis-tra dennis-tra deleted the holepunch-addr-filter branch November 17, 2022 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants