Chunking large Identify responses with Signed Records

[aarshkshah1992]

For #954 (comment).

[raulk]

Can't the signed peer record be nil?

[aarshkshah1992]

@raulk We already check for that at

go-libp2p/p2p/protocol/identify/id.go

Line 452 in 3e1b364

if sr == nil || proto.Size(mes) <= legacyIDSize {

.

[vyzo]

I don't think we need to make a new message here, just reuse the existing one and merge in the new fields.

[vyzo]

This should be done just by reading in the existing message.

[Stebalien]

This will also generalize this code to merge protocols, addresses, etc. We can write this as:

func mergeMessages(r ggio.Reader, m proto.Message) error {
	for i := 0; i < maxMessages; i++ {
	  	switch err := r.ReadMsg(mes); err {
	    default:
		case io.EOF:
        	return nil
	    case nil:
			log.Warning("error reading identify message: ", err)
			s.Reset()
			return
		}
	}
    return fmt.Errorf("too many parts")
}

[aarshkshah1992]

@Stebalien

Had to use proto.Merge(dst, src) here after calling the gogo reader because the gogo varint reader uses proto.Unmarshal under the hood which resets(clears) the existing proto message before reading the new one into it.

Other than that, works well.

[vyzo]

What's this nullable thing? I would advise against using gogo specific stuff in the protobuf decls, as there are other implementations in different languages that want to use the same protobuf file.

[Stebalien]

This is just an annotation to tell gogo how to generate the go code for this protobuf. Other protobuf implementations will ignore annotations they don't understand.

[Stebalien]

This will also generalize this code to merge protocols, addresses, etc. We can write this as:

func mergeMessages(r ggio.Reader, m proto.Message) error {
	for i := 0; i < maxMessages; i++ {
	  	switch err := r.ReadMsg(mes); err {
	    default:
		case io.EOF:
        	return nil
	    case nil:
			log.Warning("error reading identify message: ", err)
			s.Reset()
			return
		}
	}
    return fmt.Errorf("too many parts")
}

[Stebalien]

I'm pretty sure we don't need "more". We can just read till the end.

[aarshkshah1992]

This has been removed now since merging the messages works well.

[Stebalien]

We can remove the more field now, correct? I've gone ahead and done that but I'd like to double check with you (we can revert and add it back if you left it there for a reason).

[aarshkshah1992]

@Stebalien Yes, we can remove it. Sorry, I forgot this bit.

[aarshkshah1992]

@Stebalien @vyzo Have made the changes. Please take a look.

One test is failing in the Pull request build:

--- FAIL: TestHostProtoPreference (0.05s)
    basic_host_test.go:339: should have connected on  /testing

But, I'm not being able to reproduce it locally. Could one of you try and see if it fails for you to after merging with master ?

[vyzo]

flaky tests on travis is nothing new unfortunately, I restarted the build to see if it persists.

[vyzo]

we'll need to update to release swarm before merging this.