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

net/mock: support ConnectionGater in MockNet #2297

Merged
merged 1 commit into from
Jul 7, 2023

Conversation

ajsutton
Copy link
Contributor

Adds support for enforcing ConnectionGater checks to MockNet. This enables integration tests to test connection gating policies such as peer blocking policies.

MockNet doesn't actually use security handshakes or a connection upgrade process, so the whole cycle is simulated with calls to InterceptSecured and InterceptUpgraded being done when the connection is opened.

@ajsutton
Copy link
Contributor Author

I should note as well - the local and remote checks are run on the same thread as part of connecting which I believe matches the real behaviour where if the security handshake or protocol negotiation fails Dial will return an error, but if it should be async they could be moved to peerNet.remoteOpenedConn and just close the connection if the checks fail.

@ajsutton ajsutton force-pushed the aj/mocknet-gater branch from f235c5c to 9370213 Compare May 18, 2023 05:11
@ajsutton
Copy link
Contributor Author

Updated the public interface to support passing in PeerOptions so both the peer store and connection gater can (optionally) be specified when generating or adding a peer. Mocknet is a little more complex but:

  1. This provides a lot more flexibility for callers
  2. The API is now extensible for any other inputs that need to be added without having to add an ever growing range of WithXAndYAndZ type methods.

@marten-seemann
Copy link
Contributor

What's the motivation behind this PR? mocknet is deprecated and will be replaced with an in-memory-transport soon.

@ajsutton
Copy link
Contributor Author

We're using mocknet in our testing but ran into limitations in our ability to test things because the connection gater wasn't in use - in particular we're unable to test that peers that a downscored past a certain point are then disconnected and banned from reconnecting.

A more realistic in-memory-transport sounds excellent but if that's not likely to be ready in the next release it would be nice to get this in if possible to unblock our testing until the in memory transport is available. Long term it sounds like in-memory transport would let our test setup be much closer to the production setup which would be a big win so I'd definitely be keen to switch over to it.

@p-shahi p-shahi requested a review from marten-seemann May 30, 2023 17:53
@marten-seemann
Copy link
Contributor

There's a flaky mocknet test:

  === RUN   TestEventBus
      mock_test.go:672: wrong connectedness type
  --- FAIL: TestEventBus (0.01s)

Can you please debug and fix?

@ajsutton
Copy link
Contributor Author

ajsutton commented Jun 2, 2023

Yep I'll see what I can find.

@Wondertan
Copy link
Contributor

I made that test and can help debugging it

@ajsutton ajsutton force-pushed the aj/mocknet-gater branch from 0341f79 to 6b5ceee Compare June 5, 2023 01:53
@ajsutton
Copy link
Contributor Author

ajsutton commented Jun 5, 2023

@Wondertan some debugging help would be useful. Even on the current master branch I get failures when running the test multiple times. I added:

func TestLoop(t *testing.T) {
	for i := 0; i < 1000; i++ {
		t.Run("", func(t *testing.T) {
			TestEventBus(t)
		})
	}
}

to mock_test.go and get 10-20 failures on each run locally.

It looks like when it fails it's because it gets an additional connected event when it's expecting disconnected events. Throwing a bunch of printlns and a debug.PrintStackTrace() in when it's adding a connection it looks like sometimes the peers are disconnected before the identify process has completed and so it reconnects. Stack trace of the second connection is:

runtime/debug.Stack()
	/opt/homebrew/Cellar/go/1.20.4/libexec/src/runtime/debug/stack.go:24 +0x64
runtime/debug.PrintStack()
	/opt/homebrew/Cellar/go/1.20.4/libexec/src/runtime/debug/stack.go:16 +0x1c
github.com/libp2p/go-libp2p/p2p/net/mock.(*peernet).openConn(0x140002cad00, {0xff?, 0x1031e0b3f?}, 0x19?)
	/Users/aj/Documents/code/go-libp2p/p2p/net/mock/mock_peernet.go:154 +0x1a4
github.com/libp2p/go-libp2p/p2p/net/mock.(*peernet).connect(0x140002cad00, {0x1400019b080, 0x22})
	/Users/aj/Documents/code/go-libp2p/p2p/net/mock/mock_peernet.go:143 +0x3a8
github.com/libp2p/go-libp2p/p2p/net/mock.(*peernet).DialPeer(0x14000080820?, {0xff?, 0x10317e249?}, {0x1400019b080?, 0x1400094fcb8?})
	/Users/aj/Documents/code/go-libp2p/p2p/net/mock/mock_peernet.go:106 +0x28
github.com/libp2p/go-libp2p/p2p/host/basic.(*BasicHost).dialPeer(0x140006be180, {0x103506930, 0x14000526d20}, {0x1400019b080, 0x22})
	/Users/aj/Documents/code/go-libp2p/p2p/host/basic/basic_host.go:739 +0xe8
github.com/libp2p/go-libp2p/p2p/host/basic.(*BasicHost).Connect(0x140006be180, {0x103506930, 0x14000526d20}, {{0x1400019b080, 0x22}, {0x0, 0x0, 0x0}})
	/Users/aj/Documents/code/go-libp2p/p2p/host/basic/basic_host.go:732 +0x10c
github.com/libp2p/go-libp2p/p2p/host/basic.(*BasicHost).NewStream(0x140006be180, {0x103506930, 0x14000526d20}, {0x1400019b080, 0x22}, {0x14000518290, 0x1, 0x1})
	/Users/aj/Documents/code/go-libp2p/p2p/host/basic/basic_host.go:635 +0xb0
github.com/libp2p/go-libp2p/p2p/protocol/identify.(*idService).sendPushes.func1({0x10350dc40, 0x14000114360})
	/Users/aj/Documents/code/go-libp2p/p2p/protocol/identify/id.go:304 +0x138
created by github.com/libp2p/go-libp2p/p2p/protocol/identify.(*idService).sendPushes
	/Users/aj/Documents/code/go-libp2p/p2p/protocol/identify/id.go:299 +0x6c0

@ajsutton ajsutton force-pushed the aj/mocknet-gater branch from 6b5ceee to 9370213 Compare June 5, 2023 03:34
@Wondertan
Copy link
Contributor

Wondertan commented Jun 5, 2023

@ajsutton, I just tried and cannot reproduce this flake on master, however, I am able to reproduce this on mocknet-gater branch with GOMAXPOCS set to 1,2,3
I tried your snippet(with GOMAXPROCS) and GOMAXPROCS=1 go test ./p2p/net/mock -run TestEventBus -count 100 -v -failfast on both versions 3 times and the results were the same.
I am not sure how gater changes can contribute to flakes

@ajsutton ajsutton force-pushed the aj/mocknet-gater branch from 9370213 to 2691caf Compare June 5, 2023 23:23
@ajsutton
Copy link
Contributor Author

ajsutton commented Jun 5, 2023

ah, looks like it got fixed on master at some point since I branched this off.

$ git checkout 8719fc49446e488621d8bdee7d0fbdaef3f1591a
Previous HEAD position was 9370213e net/mock: support ConnectionGater in MockNet
HEAD is now at 8719fc49 net/mock: mimic Swarm's event and notification behavior in MockNet (#2287)
 aj@Busy  ~/Documents/code/go-libp2p  ➦ 8719fc49  go test ./p2p/net/mock -run TestEventBus -count 10000 -v -failfast
=== RUN   TestEventBus
    mock_test.go:669: wrong connectedness type
--- FAIL: TestEventBus (0.00s)
FAIL
FAIL	github.com/libp2p/go-libp2p/p2p/net/mock	0.397s
FAIL

but on current master it passes consistently for me.

So I've rebased this on master and now it's passing reliably on this branch too.

@ajsutton
Copy link
Contributor Author

ajsutton commented Jun 6, 2023

ok, something different this time:

  === CONT  TestSimultOpen
      simul_test.go:32: error swarm dialing to peer failed to dial 12D3KooWCaRagxPWMBeyU8eHbzvvJrmmc2ympA6bngPuPfBmXNvR:
            * [/ip4/127.0.0.1/udp/59707/quic] INTERNAL_ERROR (local): write udp4 127.0.0.1:48990->127.0.0.1:59707: sendto: operation not permitted
  --- FAIL: TestSimultOpen (0.00s)

If I'm reading that test right its using a real swarm host, not the mock net though.

@p-shahi p-shahi requested a review from MarcoPolo June 27, 2023 21:13
Copy link
Collaborator

@MarcoPolo MarcoPolo left a comment

Choose a reason for hiding this comment

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

Thank you, this looks good. Could you try rerunning CI again? I think you hit a flaky test not related to these changes. (I would, but GH doesn't let me)

Apologies for the delay to review

@ajsutton ajsutton force-pushed the aj/mocknet-gater branch from 2691caf to ad86cc3 Compare July 6, 2023 22:51
@ajsutton
Copy link
Contributor Author

ajsutton commented Jul 6, 2023

I couldn't rerun the test in GH either but have rebased this on top of latest master which got them to run again.

@ajsutton
Copy link
Contributor Author

ajsutton commented Jul 7, 2023

@MarcoPolo Looks happy now.

@MarcoPolo
Copy link
Collaborator

Great! Thanks for contribution!

@MarcoPolo MarcoPolo merged commit cfc50ba into libp2p:master Jul 7, 2023
@ajsutton ajsutton deleted the aj/mocknet-gater branch July 12, 2023 03:16
@MarcoPolo MarcoPolo mentioned this pull request Jul 14, 2023
29 tasks
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.

4 participants