peer: Ignore unknown P2P messages #1671

vasild · 2020-12-04T12:48:44Z

This would make it possible for old versions (that contain this change)
to talk with newer versions without requiring a protocol version
increment.

Fixes #1661

Untested and probably a Go expert would devise a more robust way to do that.

This would make it possible for old versions (that contain this change) to talk with newer versions without requiring a protocol version increment. Fixes btcsuite#1661

coveralls · 2020-12-04T12:52:24Z

Pull Request Test Coverage Report for Build 405504879

3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
16 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.01%) to 53.403%

Files with Coverage Reduction	New Missed Lines	%
connmgr/connmanager.go	3	86.07%
database/ffldb/blockio.go	4	92.62%
peer/peer.go	9	75.53%

Totals
Change from base Build 381748156:	-0.01%
Covered Lines:	20830
Relevant Lines:	39005

💛 - Coveralls

jonatack · 2020-12-04T12:54:55Z

Concept ACK

practicalswift · 2020-12-05T00:04:42Z

Concept ACK

jakesylvestre · 2020-12-06T18:31:18Z

Concept ACK.

Would definitely prefer to see some test cases around this. Taking a look at the error handling now.

jakesylvestre

Think this needs a test case, I wrote one here: vasild#1 but feel free to modify as needed.

As for the strings.Contains, I agree it seems ineloquent. While we do this in tests (see: mempool_test and csv_fork_test), It's probably not the best solution here.

One solution might be to add a custom error type in wire/error or reuse the MessageError and check Func

vasild · 2020-12-07T09:29:03Z

@jakesyl, thanks for the test! I added it to this PR.

_{(I think it upset the CI)}

jakesylvestre · 2020-12-07T09:34:09Z

Looks like we need to run go fmt ./... and we should be good to go.

If you could do that and push that’d be awesome otherwise I’ll get to it tomorrow (it’s 4:30 am here 💤)

jakesylvestre

I’m good to go here. Would like to get another review and see if anyone else has concerns over the error check, but it not lgtm

Roasbeef · 2020-12-08T03:23:28Z

Depending on interpretation, this itself is a departure from the "Bitcoin P2P protocol" (w/e that's defined) or we're simply filling in behavior which was previously undefined.

Roasbeef · 2020-12-08T03:28:01Z

peer/peer.go

@@ -1348,6 +1349,13 @@ out:
 				continue
 			}

+			// Ignore unknown messages to ease future extensibility.


Ideally when creating the peer object itself, the caller is able to specify if they wish to permit unknown message or not, as the caller may wish to adhere to a stricter interpretation of what exactly constitutes the Bitcoin P2P protocol.

Also it seems this change should wait on the development of this draft BIP: https://github.com/sdaftuar/bips/blob/2020-08-generalized-feature-negotiation/bip-p2p-feature-negotiation.mediawiki

I think the comment is a reference to https://github.com/benjyz/bitcoinArchive/blob/master/bitcoin0.1/src/main.cpp#L2035-L2039. But I agree for the library this should be extensible

vasild · 2020-12-08T09:43:42Z

Depending on interpretation, this itself is a departure from the "Bitcoin P2P protocol" (w/e that's defined) or we're simply filling in behavior which was previously undefined.

I am not sure if it is defined anywhere whether unknown messages should be ignored or not. My impression is that it is not defined, so I guess this is more "filling in behavior". Bitcoin Core ignores unknown messages.

onyb · 2021-04-13T13:58:20Z

Do we still need this PR, considering #1661 is already resolved by #1670?

Roasbeef · 2021-11-17T03:22:31Z

I think it's still relevant since it sets us up to be able to gracefully handle any other new messages introduced.

Roasbeef

Commented some improvements re concurrency and go error types. I think this can land and have those be follow up changes if the OP isn't still active.

Roasbeef · 2021-11-17T03:24:17Z

peer/peer_test.go

@@ -481,6 +508,11 @@ func TestPeerListeners(t *testing.T) {
 		}
 	}

+	outPeer.QueueMessage(&UnknownMessage{}, nil)


This should pass in a done channel so it can properly synchronize the assertion below. Can address this in a follow up change if the OP has abandoned the PR.

I am not sure how to translate that suggestion to Go code 😭

Roasbeef · 2021-11-17T03:25:16Z

peer/peer.go

@@ -1348,6 +1349,13 @@ out:
 				continue
 			}

+			// Ignore unknown messages to ease future extensibility.
+			// Check if the error originated from wire/message.go:makeEmptyMessage().
+			if strings.Contains(err.Error(), "unhandled command") {


Should turn this into a concrete error so we can use errors.Is here instead.

According to:

btcd/wire/doc.go

Lines 142 to 146 in 7070d53

Errors

Errors returned by this package are either the raw errors provided by underlying

calls to read/write from streams such as io.EOF, io.ErrUnexpectedEOF, and

io.ErrShortWrite, or of type wire.MessageError. This allows the caller to

and given that the error is returned by makeEmptyMessage() from wire/message.go, I guess it should be wire.MessageError, right? It looks like error.Is is not applicable to it?

Or do you think that ErrUnhandledCommand = errors.New("unhandled command") should be planted somewhere, e.g. in wire/message.go and used, probably in violation of the comment above?

cubancorona · 2022-03-01T07:15:32Z

Doc for wire pkg suggests no position

peer: Ignore unknown P2P messages

015b867

This would make it possible for old versions (that contain this change) to talk with newer versions without requiring a protocol version increment. Fixes btcsuite#1661

jakesylvestre self-requested a review December 6, 2020 16:51

jakesylvestre mentioned this pull request Dec 6, 2020

Add a test case for ignoring unknown message vasild/btcd#1

Merged

jakesylvestre requested changes Dec 6, 2020

View reviewed changes

add unknown message test case

ac83f0d

vasild force-pushed the ignore_unknown_messages branch from 7141bda to ac83f0d Compare December 7, 2020 09:44

jakesylvestre approved these changes Dec 7, 2020

View reviewed changes

jakesylvestre added this to the 0.22.0 milestone Dec 7, 2020

Roasbeef requested changes Dec 8, 2020

View reviewed changes

jcvernaleo modified the milestones: 0.22.0, 0.23.0 Apr 13, 2021

teor2345 mentioned this pull request Sep 23, 2021

ZIP-155: Parse addrv2 in Zebra ZcashFoundation/zebra#2681

Closed

teor2345 mentioned this pull request Oct 14, 2021

ZIP-155: Parse received addrv2 in the Foundation's DNS seeder ZcashFoundation/zebra#2882

Closed

2 tasks

Roasbeef approved these changes Nov 17, 2021

View reviewed changes

Crypt-iQ mentioned this pull request Feb 11, 2022

multi: implement BIP-155 addrv2 support #1812

Merged

Roasbeef closed this in #1812 Apr 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

peer: Ignore unknown P2P messages #1671

peer: Ignore unknown P2P messages #1671

vasild commented Dec 4, 2020

coveralls commented Dec 4, 2020 •

edited

Loading

jonatack commented Dec 4, 2020

practicalswift commented Dec 5, 2020

jakesylvestre commented Dec 6, 2020 •

edited

Loading

jakesylvestre left a comment •

edited

Loading

vasild commented Dec 7, 2020 •

edited

Loading

jakesylvestre commented Dec 7, 2020

jakesylvestre left a comment

Roasbeef commented Dec 8, 2020

Roasbeef Dec 8, 2020

jakesylvestre Dec 13, 2020

vasild commented Dec 8, 2020

onyb commented Apr 13, 2021

Roasbeef commented Nov 17, 2021

Roasbeef left a comment

Roasbeef Nov 17, 2021

vasild Nov 17, 2021

Roasbeef Nov 17, 2021

vasild Nov 17, 2021

cubancorona commented Mar 1, 2022

	Errors

	Errors returned by this package are either the raw errors provided by underlying
	calls to read/write from streams such as io.EOF, io.ErrUnexpectedEOF, and
	io.ErrShortWrite, or of type wire.MessageError. This allows the caller to

peer: Ignore unknown P2P messages #1671

peer: Ignore unknown P2P messages #1671

Conversation

vasild commented Dec 4, 2020

coveralls commented Dec 4, 2020 • edited Loading

Pull Request Test Coverage Report for Build 405504879

💛 - Coveralls

jonatack commented Dec 4, 2020

practicalswift commented Dec 5, 2020

jakesylvestre commented Dec 6, 2020 • edited Loading

jakesylvestre left a comment • edited Loading

Choose a reason for hiding this comment

vasild commented Dec 7, 2020 • edited Loading

jakesylvestre commented Dec 7, 2020

jakesylvestre left a comment

Choose a reason for hiding this comment

Roasbeef commented Dec 8, 2020

Roasbeef Dec 8, 2020

Choose a reason for hiding this comment

jakesylvestre Dec 13, 2020

Choose a reason for hiding this comment

vasild commented Dec 8, 2020

onyb commented Apr 13, 2021

Roasbeef commented Nov 17, 2021

Roasbeef left a comment

Choose a reason for hiding this comment

Roasbeef Nov 17, 2021

Choose a reason for hiding this comment

vasild Nov 17, 2021

Choose a reason for hiding this comment

Roasbeef Nov 17, 2021

Choose a reason for hiding this comment

vasild Nov 17, 2021

Choose a reason for hiding this comment

cubancorona commented Mar 1, 2022

coveralls commented Dec 4, 2020 •

edited

Loading

jakesylvestre commented Dec 6, 2020 •

edited

Loading

jakesylvestre left a comment •

edited

Loading

vasild commented Dec 7, 2020 •

edited

Loading