integrate the event bus, handle protocol update events, make identify emit deltas #659

raulk · 2019-06-19T15:57:43Z

This PR introduces the following:

the event bus in host implementations.
locally emits the EvtLocalProtocolsUpdated events when protocol handlers are added and removed.
identify: introduces deltas to avoid sending huge dumps over the wire, for now only for protocol updates, but can easily extend for addr updates.
identify: emit delta when local protocols change (reacting to EvtLocalProtocolsUpdated).
identify: consume deltas from peers and emit EvtPeerProtocolsUpdated.

TODO:

Release go-libp2p-core after merging eventbus abstraction + initial events go-libp2p-core#17.
Release go-eventbus.
Release go-libp2p-peerstore after merging protobook: support removing protocols. go-libp2p-peerstore#84.
Update go.mod here.

raulk · 2019-06-19T23:25:34Z

Aside from the flaky tests we have, all seems good, including the new TestProtocolHandlerEvents test.

whyrusleeping · 2019-06-20T08:32:56Z

I think changing the protocol for this is overkill, if we want to do that, fine, but it should be separate

raulk · 2019-06-20T09:27:56Z

Unfortunately this is not backwards compatible, old peers would erase all existing data for the peer because everything is empty except for the delta submessage (which they don’t deserialise). We’d need to put this in a separate protocol ID.

raulk · 2019-06-20T09:30:07Z

@whyrusleeping can you think of the benefits of sending full snapshots? Old peers would still need to upgrade to benefit from this logic, so we might as well iterate on the protocol (see my previous comment for keeping backwards compatibility). We’ve been wanting to emit deltas for address changes as well, as these broadcast messages do get expensive.

whyrusleeping

Looks good, agree the Delta is worth doing right now.

p2p/protocol/identify/id.go

whyrusleeping · 2019-06-21T12:40:49Z

@raulk so the idea is that we don't send any delta changes to old non-identify push peers? Or that we just send them the big messages and deal with it?

raulk · 2019-06-21T12:43:35Z

@whyrusleeping right now protocol changes were not being transmitted in identify push anyway. Protocol changes only travel via delta (peers that actually handle them!). In the future, once we support addr changes in delta, we can fallback to full identify push if the peer doesn't support delta. That fallback is just not relevant now.

whyrusleeping

This seems pretty straightforward. And i don't see any glaring problems.

jacobheun · 2019-06-21T14:43:20Z

Per my comment on the spec, libp2p/specs#176 (comment), I think the new protocol is unnecessary as versioning the protocol would allow us to gracefully handle backwards compatibility support.

raulk · 2019-06-21T14:47:56Z

@jacobheun one potential downside is that a id/2.0.0 -> id/1.0.0 interaction would require an extra round trip, and it would block the identification itself.

jacobheun · 2019-06-21T14:52:26Z

one potential downside is that a id/2.0.0 -> id/1.0.0 interaction would require an extra round trip, and it would block the identification itself.

Why? We should know the protocol support before we do the push so we're not unnecessarily pushing to peers that don't support the protocol. Identify push should be separate from identify, which would stay at version 1.0.0, so it shouldn't be blocking. The working implementation of this in JS behaves this way.

raulk · 2019-06-21T17:21:21Z

@jacobheun if we subsume delta into id/2.0.0, when a new peer conducts multistream negotiation with an old peer, it'll first try id/2.0.0, which will result in na, an extra round trip before they settle on 1.0.0. I much rather have delta out-of-band, as identify is on the critical path for several things.

raulk · 2019-06-21T17:55:45Z

Associated spec update: libp2p/specs#176.

jacobheun · 2019-06-21T21:08:23Z

@raulk shouldn't go already know the supported protocol before pushing updates? For example in js, since the delta/push should only occur for connected peers, we'd have already done the identify handshake to get the peers supported protocols. Prior to sending the updates we should be able to check the supported protocol versions. For peers that only support push 1.0.0, we can do the full send and the delta for 2.0.0. The handshake would only occur once we've decided and executed the appropriate push version.

It sounds like the go version is attempting to push updates to connected peers without checking their supported protocols, which would be a lot of unnecessary noise for peers that don't support it.

raulk · 2019-06-21T21:44:16Z

@jacobheun we're getting lost in translation here. Indeed, with identify/1.0.0 we discover the protocols the other party speaks. Armed with that info, we only send deltas iff the peer supports the delta protocol.

If I understood correctly, you were proposing to eliminate the extra protocol and subsume deltas into an implicit capability of a hypothetical baseline coarser identify/2.0.0 protocol.

One problem with that approach is the extra multistream round trip in a critical path, as I described.

Also, deltas are relatively decoupled from the baseline protocol, so I don't see any benefit in bundling them together.

jacobheun · 2019-06-22T08:08:55Z

@raulk I'm not talking about changing identify, just push, so this should cause another round trip. I'll use the protocol id's to be explicit.

Instead of:
/ipfs/id/1.0.0
/ipfs/id/push/1.0.0
/p2p/id/delta/1.0.0

Do:
/ipfs/id/1.0.0 (unchanged)
/ipfs/id/push/2.0.0 (increment, and keep /ipfs/id/push/1.0.0 support for while the network upgrades)
~~/p2p/id/delta/1.0.0~~ (removed, this is equivalent to /ipfs/id/push/2.0.0)

This would serve the same purpose as the delta but allow us to fallback to /ipfs/id/push/1.0.0 support. We'd check the list of their most recently supported version of the protocol against our versions and use that.

raulk · 2019-06-22T09:11:42Z

Ah, so it’s a matter of semantics. Delta does not replace the entirety of push’s functionality yet (we don’t send deltas for addr changes yet, we send full dump), so semantically this would get confusing, as push/2.0.0 would send deltas for one thing, and full dumps for another. Once we migrate addr changes to delta, peers can stop handling push and switch entirely to delta.

jacobheun · 2019-06-22T09:39:24Z

If the goal is to deprecate push in favour of delta, then I'm okay with this, I just wanted to avoid having multiple protocols to perform updates long term. I think deprecating push in the spec update PR would be helpful for clarifying the intent.

fwiw, It does feel like an implementation detail/issue is driving the spec change in a specific direction, as the push spec already accounts for partial updates, go just doesn't support that. That may not be the case, but it's something we should be consciences of. Delta will give us finer, diff like, control over the updates we send, so there's a clear improvement there.

raulk · 2019-06-24T13:38:33Z

@jacobheun the baseline protocol supports partial updates, but not diffs as you duly note (added/removed sets)

Stebalien · 2019-07-04T20:31:19Z

The tests aren't flaky, they're failing.

raulk · 2019-07-04T21:35:38Z

@Stebalien succeeded for me in master locally, see logs.

Tests passing on master

~/Workbench/protocol/workspace-go-libp2p/go-libp2p master
❯ go test -v ./...
=== RUN   TestNewHost
--- PASS: TestNewHost (0.12s)
=== RUN   TestBadTransportConstructor
--- PASS: TestBadTransportConstructor (0.00s)
=== RUN   TestNoListenAddrs
--- PASS: TestNoListenAddrs (0.32s)
=== RUN   TestNoTransports
--- PASS: TestNoTransports (0.52s)
=== RUN   TestInsecure
--- PASS: TestInsecure (0.10s)
=== RUN   TestDefaultListenAddrs
--- PASS: TestDefaultListenAddrs (0.50s)
=== RUN   TestChainOptions
--- PASS: TestChainOptions (0.00s)
PASS
ok  	github.com/libp2p/go-libp2p	1.601s
=== RUN   TestNilOption
--- PASS: TestNilOption (0.00s)
=== RUN   TestMuxerSimple
--- PASS: TestMuxerSimple (0.00s)
=== RUN   TestMuxerByValue
--- PASS: TestMuxerByValue (0.00s)
=== RUN   TestMuxerDuplicate
--- PASS: TestMuxerDuplicate (0.00s)
=== RUN   TestMuxerError
--- PASS: TestMuxerError (0.00s)
=== RUN   TestMuxerBadTypes
--- PASS: TestMuxerBadTypes (0.00s)
=== RUN   TestCatchDuplicateTransportsMuxer
=== RUN   TestCatchDuplicateTransportsMuxer/no_duplicate_transports
=== RUN   TestCatchDuplicateTransportsMuxer/duplicate_transports
--- PASS: TestCatchDuplicateTransportsMuxer (0.02s)
    --- PASS: TestCatchDuplicateTransportsMuxer/no_duplicate_transports (0.00s)
    --- PASS: TestCatchDuplicateTransportsMuxer/duplicate_transports (0.00s)
=== RUN   TestHandleReturnValue
--- PASS: TestHandleReturnValue (0.00s)
=== RUN   TestCheckReturnType
--- PASS: TestCheckReturnType (0.00s)
=== RUN   TestCallConstructor
--- PASS: TestCallConstructor (0.00s)
PASS
ok  	github.com/libp2p/go-libp2p/config	0.069s
=== RUN   TestMdnsDiscovery
--- SKIP: TestMdnsDiscovery (0.00s)
    mdns_test.go:25: TestMdnsDiscovery fails randomly with current lib
PASS
ok  	github.com/libp2p/go-libp2p/p2p/discovery	0.031s
=== RUN   TestHostSimple
--- PASS: TestHostSimple (0.12s)
=== RUN   TestProtocolHandlerEvents
--- PASS: TestProtocolHandlerEvents (0.10s)
=== RUN   TestHostAddrsFactory
--- PASS: TestHostAddrsFactory (0.09s)
=== RUN   TestHostProtoPreference
--- PASS: TestHostProtoPreference (0.09s)
=== RUN   TestHostProtoMismatch
--- PASS: TestHostProtoMismatch (0.05s)
=== RUN   TestHostProtoPreknowledge
--- PASS: TestHostProtoPreknowledge (0.13s)
=== RUN   TestNewDialOld
22:33:32.835 ERROR     swarm2: swarm listener accept error: accept tcp4 127.0.0.1:58696: use of closed network connection swarm_listen.go:79
--- PASS: TestNewDialOld (0.12s)
=== RUN   TestProtoDowngrade
--- PASS: TestProtoDowngrade (0.14s)
=== RUN   TestAddrResolution
--- PASS: TestAddrResolution (0.13s)
PASS
ok  	github.com/libp2p/go-libp2p/p2p/host/basic	1.007s
=== RUN   TestCleanupAddrs
--- PASS: TestCleanupAddrs (0.00s)
=== RUN   TestAutoRelay
--- PASS: TestAutoRelay (6.47s)
PASS
22:33:38.687 ERROR     swarm2: swarm listener accept error: accept tcp6 [::]:58710: use of closed network connection swarm_listen.go:79
22:33:38.687 ERROR     swarm2: swarm listener accept error: accept tcp4 0.0.0.0:58709: use of closed network connection swarm_listen.go:79
ok  	github.com/libp2p/go-libp2p/p2p/host/relay	6.514s
?   	github.com/libp2p/go-libp2p/p2p/host/routed	[no test files]
=== RUN   TestNotifications
--- FAIL: TestNotifications (0.00s)
    mock_notif_test.go:97: got notif for opened stream
    mock_notif_test.go:107: got notif for closed stream
    mock_notif_test.go:97: got notif for opened stream
    mock_notif_test.go:107: got notif for closed stream
    mock_notif_test.go:97: got notif for opened stream
    mock_notif_test.go:107: got notif for closed stream
    mock_notif_test.go:97: got notif for opened stream
    mock_notif_test.go:107: got notif for closed stream
    mock_notif_test.go:97: got notif for opened stream
    mock_notif_test.go:107: got notif for closed stream
    mock_notif_test.go:97: got notif for opened stream
    mock_notif_test.go:107: got notif for closed stream
    mock_notif_test.go:97: got notif for opened stream
    mock_notif_test.go:107: got notif for closed stream
    mock_notif_test.go:97: got notif for opened stream
    mock_notif_test.go:107: got notif for closed stream
    mock_notif_test.go:97: got notif for opened stream
    mock_notif_test.go:107: got notif for closed stream
    mock_notif_test.go:97: got notif for opened stream
    mock_notif_test.go:107: got notif for closed stream
    mock_notif_test.go:97: got notif for opened stream
    mock_notif_test.go:107: got notif for closed stream
    mock_notif_test.go:97: got notif for opened stream
    mock_notif_test.go:107: got notif for closed stream
    mock_notif_test.go:97: got notif for opened stream
    mock_notif_test.go:107: got notif for closed stream
    mock_notif_test.go:97: got notif for opened stream
    mock_notif_test.go:107: got notif for closed stream
    mock_notif_test.go:97: got notif for opened stream
    mock_notif_test.go:107: got notif for closed stream
    mock_notif_test.go:97: got notif for opened stream
    mock_notif_test.go:107: got notif for closed stream
    mock_notif_test.go:97: got notif for opened stream
    mock_notif_test.go:107: got notif for closed stream
    mock_notif_test.go:97: got notif for opened stream
    mock_notif_test.go:107: got notif for closed stream
    mock_notif_test.go:97: got notif for opened stream
    mock_notif_test.go:107: got notif for closed stream
    mock_notif_test.go:97: got notif for opened stream
    mock_notif_test.go:107: got notif for closed stream
    mock_notif_test.go:148: 13d4Nv46i /ip6/100::543:c6ec:7c2f/tcp/4242 <--0xc00047a000--> 13fSCvvsD /ip6/100::558:d703:3cec/tcp/4242
    mock_notif_test.go:152: 13fSCvvsD /ip6/100::558:d703:3cec/tcp/4242 <--0xc00047a070--> 13d4Nv46i /ip6/100::543:c6ec:7c2f/tcp/4242
    mock_notif_test.go:97: got notif for opened stream
    mock_notif_test.go:102: got incorrect stream 0xc00047a000 0xc00037a230
=== RUN   TestNetworkSetup
--- PASS: TestNetworkSetup (0.02s)
=== RUN   TestStreams
--- PASS: TestStreams (0.00s)
=== RUN   TestStreamsStress
--- PASS: TestStreamsStress (1.02s)
=== RUN   TestAdding
--- PASS: TestAdding (0.04s)
=== RUN   TestRateLimiting
--- PASS: TestRateLimiting (0.00s)
=== RUN   TestLimitedStreams
--- FAIL: TestLimitedStreams (2.81s)
    mock_test.go:588: Expected 2ish seconds but got  2.438186423s
=== RUN   TestFuzzManyPeers
--- PASS: TestFuzzManyPeers (18.76s)
=== RUN   TestStreamsWithLatency
--- PASS: TestStreamsWithLatency (1.51s)
FAIL
FAIL	github.com/libp2p/go-libp2p/p2p/net/mock	24.895s
=== RUN   TestObsAddrSet
--- PASS: TestObsAddrSet (0.24s)
=== RUN   TestAddAddrsProfile
--- PASS: TestAddAddrsProfile (14.11s)
=== RUN   TestIDService
--- PASS: TestIDService (6.21s)
    id_test.go:62: test peer1 has peer2 addrs correctly
    id_test.go:79: test peer2 has peer1 addrs correctly
    id_test.go:62: test peer1 has peer2 addrs correctly
    id_test.go:79: test peer2 has peer1 addrs correctly
    id_test.go:62: test peer1 has peer2 addrs correctly
    id_test.go:79: test peer2 has peer1 addrs correctly
=== RUN   TestProtoMatching
--- PASS: TestProtoMatching (0.00s)
=== RUN   TestIdentifyDeltaOnProtocolChange
--- PASS: TestIdentifyDeltaOnProtocolChange (1.04s)
=== RUN   TestIdentifyDeltaWhileIdentifyingConn
--- PASS: TestIdentifyDeltaWhileIdentifyingConn (1.04s)
PASS
ok  	github.com/libp2p/go-libp2p/p2p/protocol/identify	22.668s
?   	github.com/libp2p/go-libp2p/p2p/protocol/identify/pb	[no test files]
=== RUN   TestPing
--- PASS: TestPing (0.08s)
    ping_test.go:48: ping took:  1.073255ms
    ping_test.go:48: ping took:  134.868µs
    ping_test.go:48: ping took:  155.948µs
    ping_test.go:48: ping took:  130.114µs
    ping_test.go:48: ping took:  130.946µs
    ping_test.go:48: ping took:  205.699µs
    ping_test.go:48: ping took:  110.686µs
    ping_test.go:48: ping took:  119.702µs
    ping_test.go:48: ping took:  123.491µs
    ping_test.go:48: ping took:  114.614µs
PASS
ok  	github.com/libp2p/go-libp2p/p2p/protocol/ping	0.129s
=== RUN   TestBackpressureStreamHandler
--- SKIP: TestBackpressureStreamHandler (0.00s)
    backpressure_test.go:37: Sadly, as cool as this test is, it doesn't work
        Because spdystream doesnt handle stream open backpressure
        well IMO. I'll see about rewriting that part when it becomes
        a problem.
=== RUN   TestStBackpressureStreamWrite

--- PASS: TestStBackpressureStreamWrite (4.49s)

PASS

ok  	github.com/libp2p/go-libp2p/p2p/test/backpressure	4.532s

=== RUN   TestReconnect2

--- PASS: TestReconnect2 (0.38s)

=== RUN   TestReconnect5

--- PASS: TestReconnect5 (0.39s)

PASS

ok  	github.com/libp2p/go-libp2p/p2p/test/reconnects	0.796s

raulk · 2019-07-04T21:41:08Z

^^ EDIT: it's possible what we're seeing is the effect of the race detector limiting goroutines. I also noticed there's a race in the TestIdentifyDeltaOnProtocolChange test.

Stebalien · 2019-07-04T22:51:15Z

I'm seeing several issues:

More goroutines because we're opening more streams for identify push (goroutine race detector limit).
More streams because we're opening more streams for identify push (breaking TestNotifications, look for the very helpful "unsure where these are coming from comment", the answer is "identify").
https://github.com/libp2p/go-libp2p/pull/669/files#diff-d60f565cc4d016b57968fdb9169cacebR224

But yeah, unfortunately these aren't failing reliably.

raulk added 2 commits June 19, 2019 16:56

integrate the event bus into host implementations

6f63be8

emit events on local protocol attach/detach.

dc3f9e7

raulk changed the title ~~integrate the event bus into host implementations~~ integrate the event bus into hosts + emit EvtLocalProtocolsUpdated Jun 19, 2019

raulk added 3 commits June 19, 2019 18:22

remove dead code.

be30c41

wip identify delta + protocols event management.

4e303e6

identify: push deltas for protocol additions/removal.

2bc9bbc

raulk changed the title ~~integrate the event bus into hosts + emit EvtLocalProtocolsUpdated~~ integrate the event bus, handle protocol update events, make identify emit deltas Jun 19, 2019

raulk marked this pull request as ready for review June 19, 2019 22:53

raulk force-pushed the eventbus branch from ab23607 to 84eccec Compare June 19, 2019 23:02

update go.mod.

5348919

raulk force-pushed the eventbus branch from 84eccec to 5348919 Compare June 19, 2019 23:19

raulk requested review from vyzo, Stebalien and whyrusleeping June 20, 2019 08:00

whyrusleeping approved these changes Jun 20, 2019

View reviewed changes

p2p/protocol/identify/id.go Outdated Show resolved Hide resolved

raulk mentioned this pull request Jun 20, 2019

identify push needs to only send updated information #630

Closed

raulk added 2 commits June 20, 2019 16:35

make identify delta a separate, more robust protocol.

cfe79c9

deps: upgrade dependencies.

cf82625

raulk force-pushed the eventbus branch from 2014bc3 to cf82625 Compare June 20, 2019 15:35

raulk requested a review from whyrusleeping June 20, 2019 16:09

yusefnapora mentioned this pull request Jun 20, 2019

add identify/delta to identify spec libp2p/specs#176

Closed

Kubuxu self-requested a review June 20, 2019 16:35

fix log; tidy up imports.

091e08c

whyrusleeping approved these changes Jun 21, 2019

View reviewed changes

upgrade to latest eventbus.

d309da0

raulk force-pushed the eventbus branch from 5a068ba to d309da0 Compare June 21, 2019 17:41

raulk added 3 commits June 21, 2019 18:58

adjust log statement.

3ab6b9e

fix test; handle channel closures on subs.

fe58af8

fix tests.

e99b318

raulk merged commit e69d171 into master Jun 24, 2019

raulk deleted the eventbus branch June 24, 2019 13:44

raulk mentioned this pull request Dec 11, 2019

Design blueprint for signed peer records #761

Open

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

integrate the event bus, handle protocol update events, make identify emit deltas #659

integrate the event bus, handle protocol update events, make identify emit deltas #659

raulk commented Jun 19, 2019 •

edited

raulk commented Jun 19, 2019

whyrusleeping commented Jun 20, 2019

raulk commented Jun 20, 2019

raulk commented Jun 20, 2019 •

edited

whyrusleeping left a comment

whyrusleeping commented Jun 21, 2019

raulk commented Jun 21, 2019 •

edited

whyrusleeping left a comment

jacobheun commented Jun 21, 2019

raulk commented Jun 21, 2019

jacobheun commented Jun 21, 2019

raulk commented Jun 21, 2019

raulk commented Jun 21, 2019

jacobheun commented Jun 21, 2019

raulk commented Jun 21, 2019 •

edited

jacobheun commented Jun 22, 2019

raulk commented Jun 22, 2019

jacobheun commented Jun 22, 2019

raulk commented Jun 24, 2019

Stebalien commented Jul 4, 2019

raulk commented Jul 4, 2019 •

edited

raulk commented Jul 4, 2019 •

edited

Stebalien commented Jul 4, 2019

integrate the event bus, handle protocol update events, make identify emit deltas #659

integrate the event bus, handle protocol update events, make identify emit deltas #659

Conversation

raulk commented Jun 19, 2019 • edited

raulk commented Jun 19, 2019

whyrusleeping commented Jun 20, 2019

raulk commented Jun 20, 2019

raulk commented Jun 20, 2019 • edited

whyrusleeping left a comment

Choose a reason for hiding this comment

whyrusleeping commented Jun 21, 2019

raulk commented Jun 21, 2019 • edited

whyrusleeping left a comment

Choose a reason for hiding this comment

jacobheun commented Jun 21, 2019

raulk commented Jun 21, 2019

jacobheun commented Jun 21, 2019

raulk commented Jun 21, 2019

raulk commented Jun 21, 2019

jacobheun commented Jun 21, 2019

raulk commented Jun 21, 2019 • edited

jacobheun commented Jun 22, 2019

raulk commented Jun 22, 2019

jacobheun commented Jun 22, 2019

raulk commented Jun 24, 2019

Stebalien commented Jul 4, 2019

raulk commented Jul 4, 2019 • edited

raulk commented Jul 4, 2019 • edited

Stebalien commented Jul 4, 2019

raulk commented Jun 19, 2019 •

edited

raulk commented Jun 20, 2019 •

edited

raulk commented Jun 21, 2019 •

edited

raulk commented Jun 21, 2019 •

edited

raulk commented Jul 4, 2019 •

edited

raulk commented Jul 4, 2019 •

edited