Split P2P Topics And Introduce Middleware (Adapters) #421

prestonvanloon · 2018-08-19T20:29:19Z

Resolves #363

This is still a bit of work in progress as this feature is lacking some critical test paths.
I'm opening this PR for exposure and tracking this work in progress.

New features:

Beacon chain and sharding client subscribe to their specific topics only.
A middleware adapter pattern enables some p2p middleware layer functionality such as streamz metrics counting (via prometheus), instrumentation (via opentracing), request rate limiting, debug logging, etc.

The above middleware examples are not added in this PR but are likely candidates for new features.

…ters

…2p_adapters

codecov · 2018-08-19T22:53:10Z

Codecov Report

Merging #421 into master will decrease coverage by 0.12%.
The diff coverage is 83.78%.

@@            Coverage Diff             @@
##           master     #421      +/-   ##
==========================================
- Coverage   76.58%   76.45%   -0.13%     
==========================================
  Files          35       36       +1     
  Lines        2242     2251       +9     
==========================================
+ Hits         1717     1721       +4     
- Misses        368      372       +4     
- Partials      157      158       +1

Impacted Files	Coverage Δ
beacon-chain/node/node.go	`58.97% <100%> (ø)`	⬆️
shared/p2p/discovery.go	`88.23% <100%> (-4.08%)`	⬇️
validator/node/node.go	`40.21% <100%> (ø)`	⬆️
beacon-chain/node/p2p_config.go	`75% <75%> (ø)`
validator/node/p2p_config.go	`75% <75%> (ø)`
shared/p2p/service.go	`71.27% <85.18%> (+13.95%)`	⬆️
beacon-chain/rpc/service.go	`94.59% <0%> (-5.41%)`	⬇️
beacon-chain/sync/initial-sync/service.go	`68.33% <0%> (-4.17%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5adc94b...e13aa0c. Read the comment docs.

rauljordan · 2018-08-20T03:27:15Z

Gazelle needs rerun but the middleware idea is awesome! Curious to see if perhaps a p2p incentive layer could be attached as something of that sort.

prestonvanloon · 2018-08-24T02:53:30Z

This is ready for review. Please look

terencechain · 2018-08-24T15:17:28Z

beacon-chain/node/p2p_config.go

+ return nil, err
+ }
+
+ // TODO: Define default adapters for logging, monitoring, etc.


Can we open an issue for this?

terencechain · 2018-08-24T15:20:25Z

shared/p2p/register_topic_example_test.go

+
+ s, _ := p2p.NewServer()
+
+ // TODO: Figure out the topic. Is it a protobuf topic, string, or int?


We should create an issue for this too for new comers to pick it up

Oh, this is a leftover TODO. It's a string at the moment.

rawfalafel

Looking good. My main concern is that we've been passing around messages as an interface{} type. Would be nice if we can clean that up in this PR. Otherwise, the actual splitting of topics looks 👌

rawfalafel · 2018-08-24T19:01:15Z

shared/p2p/service.go

+ }
+
+ // Reverse adapter order
+ for i := len(adapters)/2 - 1; i >= 0; i-- {


Shouldn't this list be reversed outside of the loop, or can this list change over time?

Yes, good catch!

rawfalafel · 2018-08-24T19:09:10Z

shared/p2p/service.go

+ return
+ }
+
+ i := feed.Send(Message{Data: d})


Here's the definition for Message:

type Message struct { // Peer represents the sender of the message. Peer Peer // Data can be any type of message found in sharding/p2p/proto package. Data interface{} }

Can type interface{} type for Data be replaced with proto.Message? Allowing any type here seems unnecessarily lax, especially considering that the value was cast down to proto.Message at the beginning of this function.

Sounds good to me

nisdas

Everything looks great! Just had a few comments. Also is it possible to get the number of peers connected ? We could use it for sync

nisdas · 2018-08-26T20:51:42Z

shared/p2p/register_topic_example_test.go

+// limiter or blacklisting condition.
+func reqLogger(next p2p.Handler) p2p.Handler {
+ return func(ctx context.Context, msg p2p.Message) {
+ fmt.Printf("Received message from %s\n", msg.Peer)


msg.Peer should be %v

nisdas · 2018-08-26T20:55:42Z

shared/p2p/register_topic_example_test.go

+
+ s.RegisterTopic(topic, message, adapters...)
+
+ ch := make(chan p2p.Message)


Shouldn't this be buffered ?

rawfalafel

We should definitely avoid using arbitrary timeouts in tests. I poked around libp2p to see if there was a workaround, but nothing obvious popped up.

I'm also curoius if subscribeToTopic is necessary at all, now that we have RegisterTopic.

rawfalafel · 2018-08-26T19:18:45Z

shared/p2p/service.go

+//
+// The topics can originate from multiple sources. In other words, messages on
+// TopicA may come from direct peer communication or a pub/sub channel.
+func (s *Server) RegisterTopic(topic string, message interface{}, adapters ...Adapter) {


message interface{} can be proto.Message as well

I tried this and it's failing tests. I wonder if reflect is assuming that TypeOf(message) is proto.Message rather than the full proto definition.

I didn't investigate thoroughly.

rawfalafel · 2018-08-26T19:29:27Z

shared/p2p/service.go

+}
+
+func (s *Server) emit(feed *event.Feed, msg *floodsub.Message, msgType reflect.Type) {
+ // TODO: reflect.Value.Interface() can panic so we should capture that


You shouldn't have to handle this case explictly, since we're never passing in unexported fields for this value.

OK, I dont recall why the TODO was here so I will remove it. Hopefully it doesn't cause issues.

rawfalafel · 2018-08-26T19:46:23Z

shared/p2p/service.go

+ h = adapter(h)
+ }
+
+ ctx, _ := context.WithTimeout(s.ctx, 10*time.Second)


cancel should be called at the end of this loop to avoid memory leaks

Why? There is a timeout, i'm not sure we even need this timeout. I'll just remove it.

https://stackoverflow.com/questions/44393995/what-happens-if-i-dont-cancel-a-context

Looks like the timeout exists so that the adapter doesn't run continuously for more than 10 seconds.

rawfalafel · 2018-08-26T21:01:32Z

shared/p2p/service.go

- log.WithFields(logrus.Fields{
- "topic": topic,
- }).Debug("Subscribing to topic")
- go s.subscribeToTopic(topic, msgType)


Should we remove this method, subscribeToTopic?

I am surprised this didn't upset the unused linter. It was only used in tests!

rawfalafel · 2018-08-26T21:32:58Z

shared/p2p/service_test.go

+ wait := make(chan struct{})
+ go (func() {
+ defer close(wait)
+ msg := <-ch


You can replace this line and the next with just <-ch

rawfalafel · 2018-08-26T21:34:26Z

shared/p2p/service_test.go

+ }
+
+ // Short timeout to allow libp2p to handle peer connection.
+ time.Sleep(time.Millisecond * 10)


The description for BasicHost.Connect is:

// Connect ensures there is a connection between this host and the peer with // given peer.ID. If there is not an active connection, Connect will issue a // h.Network.Dial, and block until a connection is open, or an error is returned. // Connect will absorb the addresses in pi into its internal peerstore. // It will also resolve any /dns4, /dns6, and /dnsaddr addresses.

which suggests that this timeout shouldn't be necessary. Do you know why it's required? This is a potential source of flakiness, we should definitely try and avoid an arbitrary timeout.

Not really. This pattern is all over the floodsub tests: https://github.com/libp2p/go-floodsub/blob/HEAD/floodsub_test.go

And it fails without it.

rawfalafel · 2018-08-26T21:34:41Z

shared/p2p/service_test.go

+ wait := make(chan struct{})
+ go (func() {
+ defer close(wait)
+ msg := <-ch


rauljordan · 2018-08-28T21:48:50Z

shared/p2p/service.go

+ adapters[i], adapters[opp] = adapters[opp], adapters[i]
+ }
+
+ go (func() {


no need for go (func, do go func() {}() instead

rauljordan · 2018-08-28T21:49:55Z

proto/README.md

Can you add a few sections here about the p2p middleware stuff for visibility?

What does this have to do with protos?

terencechain

LGTM

nisdas

lgtm

rawfalafel

LGTM

prestonvanloon added 14 commits August 5, 2018 12:06

thinking about adapters

45b9df2

more progress on exploring p2p middleware stack

0fc242a

some more progress on the middleware stack idea

e3ee2ca

added a bit more to the example

826afea

working on testing

1978ecf

working a bit more on tests

393c9b4

Merge branch 'master' of github.com:prysmaticlabs/prysm into p2p_adap…

5f4c379

…ters

Merge branch 'master' into p2p_adapters

54c761d

Merge branch 'p2p_adapters' of github.com:prestonvanloon/prysm into p…

daa6014

…2p_adapters

more progress on p2p abstraction with adapters

b02aecc

split topics

6caaef2

split topics

1da3adb

split topics

7bced19

add other config

80fe715

prestonvanloon added the In Progress label Aug 19, 2018

prestonvanloon added this to the Ruby milestone Aug 19, 2018

prestonvanloon self-assigned this Aug 19, 2018

prestonvanloon added 2 commits August 19, 2018 16:29

Merge branch 'master' into p2p_adapters

41b03c9

fix example

e6c32f3

prestonvanloon added 6 commits August 22, 2018 20:44

newline

558139b

merge master

1a0770d

fix merge

fe95160

move more after merge

3d8646d

fix godoc

55cc0c3

a bit more tests

23daf90

merge assignment

aa77a94

prestonvanloon removed the In Progress label Aug 24, 2018

prestonvanloon requested review from nisdas, rauljordan, terencechain and rawfalafel August 24, 2018 02:52

terencechain reviewed Aug 24, 2018

View reviewed changes

rawfalafel suggested changes Aug 24, 2018

View reviewed changes

prestonvanloon added 2 commits August 26, 2018 13:53

update from PR feedback

91f8352

merge and conflicts

fe49351

nisdas reviewed Aug 26, 2018

View reviewed changes

rawfalafel suggested changes Aug 26, 2018

View reviewed changes

rauljordan requested changes Aug 28, 2018

View reviewed changes

prestonvanloon added 6 commits August 28, 2018 20:44

PR feedback

287839f

another go (func

7f83b6f

PR feedback that I missed

7b3e0ff

gofmt

0500af6

gazelle

3ecf3bd

Merge branch 'master' into p2p_adapters

a683da2

terencechain approved these changes Aug 29, 2018

View reviewed changes

nisdas approved these changes Aug 29, 2018

View reviewed changes

rauljordan approved these changes Aug 29, 2018

View reviewed changes

Merge branch 'master' into p2p_adapters

e13aa0c

rawfalafel approved these changes Aug 29, 2018

View reviewed changes

rauljordan merged commit b02042d into prysmaticlabs:master Aug 29, 2018

d1vyank mentioned this pull request Sep 6, 2018

[Instrumentation] Opentracing on P2P Messages #438

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split P2P Topics And Introduce Middleware (Adapters) #421

Split P2P Topics And Introduce Middleware (Adapters) #421

prestonvanloon commented Aug 19, 2018

codecov bot commented Aug 19, 2018 •

edited

Loading

rauljordan commented Aug 20, 2018

prestonvanloon commented Aug 24, 2018

terencechain Aug 24, 2018

terencechain Aug 24, 2018

prestonvanloon Aug 26, 2018

rawfalafel left a comment

rawfalafel Aug 24, 2018

prestonvanloon Aug 26, 2018

rawfalafel Aug 24, 2018

prestonvanloon Aug 26, 2018

nisdas left a comment •

edited

Loading

nisdas Aug 26, 2018

nisdas Aug 26, 2018

rawfalafel left a comment

rawfalafel Aug 26, 2018

prestonvanloon Aug 29, 2018

rawfalafel Aug 26, 2018

prestonvanloon Aug 29, 2018

rawfalafel Aug 26, 2018

prestonvanloon Aug 29, 2018

rawfalafel Aug 29, 2018

rawfalafel Aug 26, 2018

prestonvanloon Aug 29, 2018

rawfalafel Aug 26, 2018

rawfalafel Aug 26, 2018

prestonvanloon Aug 28, 2018

rawfalafel Aug 26, 2018

rauljordan Aug 28, 2018

rauljordan Aug 28, 2018

prestonvanloon Aug 29, 2018

terencechain left a comment

nisdas left a comment

rawfalafel left a comment


		s, _ := p2p.NewServer()

		// TODO: Figure out the topic. Is it a protobuf topic, string, or int?


		s.RegisterTopic(topic, message, adapters...)

		ch := make(chan p2p.Message)

Split P2P Topics And Introduce Middleware (Adapters) #421

Split P2P Topics And Introduce Middleware (Adapters) #421

Conversation

prestonvanloon commented Aug 19, 2018

codecov bot commented Aug 19, 2018 • edited Loading

Codecov Report

rauljordan commented Aug 20, 2018

prestonvanloon commented Aug 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rawfalafel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nisdas left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rawfalafel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencechain left a comment

Choose a reason for hiding this comment

nisdas left a comment

Choose a reason for hiding this comment

rawfalafel left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 19, 2018 •

edited

Loading

nisdas left a comment •

edited

Loading