p2p: Treat handshake misbehavior like unknown message

MarcoFalke commented at 3:24 pm on October 4, 2020: member

Handshake misbehaviour doesn’t cost us more than any other unknown message, so it seems odd to treat it differently

MarcoFalke force-pushed on Oct 4, 2020

DrahtBot added the label P2P on Oct 4, 2020

practicalswift commented at 7:33 pm on October 4, 2020: contributor

Concept ACK

MarcoFalke force-pushed on Oct 5, 2020

MarcoFalke renamed this:
~~p2p: Ignore non-version msgs before version msg~~
p2p: Treat handshake misbehavior like unkown message
on Oct 5, 2020

hebasto commented at 2:01 pm on October 6, 2020: member

Concept ACK.

luke-jr commented at 6:09 pm on October 24, 2020: member

Concept ACK on ignoring non-version messages prior to version, but I think ignoring the Nth version message is wrong.

Since we don’t support actually changing to a new version, disconnecting is the polite reaction. (Probably doesn’t need to punish the node, though.)

jnewbery renamed this:
~~p2p: Treat handshake misbehavior like unkown message~~
p2p: Treat handshake misbehavior like unknown message
on Nov 13, 2020

in src/net_processing.cpp:2368 in fac8e9ac71 outdated

2365@@ -2366,7 +2366,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
2366         // Each connection can only send one version message
2367         if (pfrom.nVersion != 0)
2368         {

jnewbery commented at 12:39 pm on November 17, 2020:

Only if you retouch the branch:

0        if (pfrom.nVersion != 0) {
1            // Each connection can only send one version message

MarcoFalke commented at 6:33 pm on November 18, 2020:

My preference would be to remove the comment, because the logprint is already clear enough

MarcoFalke commented at 8:10 am on November 19, 2020:

Thx, fixed.

in test/functional/p2p_invalid_messages.py:101 in fac8e9ac71 outdated

78@@ -77,6 +79,13 @@ def test_buffer(self):
79         conn.sync_with_ping(timeout=1)
80         self.nodes[0].disconnect_p2ps()
81 
82+    def test_duplicate_version_msg(self):
83+        self.log.info("Test duplicate version message is ignored")
84+        conn = self.nodes[0].add_p2p_connection(P2PDataStore())
85+        with self.nodes[0].assert_debug_log(['redundant version message from peer']):
86+            conn.send_and_ping(msg_version())

jnewbery commented at 12:42 pm on November 17, 2020:

I think it’s much better to test the behaviour rather than the log. Here, that would be sending the message and then verifying that the peer is still connected (you could even check that getpeerinfo.banscore = 0, but that also seems a bit too implementation specific).

MarcoFalke commented at 6:32 pm on November 18, 2020:

The _and_ping part will take care of asserting that the connection is still up, no?

jnewbery commented at 10:08 pm on November 18, 2020:

Yes, I think that would do it, which makes asserting on the log redundant.

MarcoFalke commented at 7:02 am on November 19, 2020:

I like the assert log for two reasons:

I can call git grep -W 'redundant version message' and see the code as well as the test without searching manually
It protects against test mistakes, such as sending a verion msg, which would also be ignored

jnewbery commented at 10:41 am on November 19, 2020:

ok

jnewbery commented at 12:43 pm on November 17, 2020: member

utACK fac8e9ac7177bd2dee6b0532fde917aa4b89f651

Couple of small style suggestions inline.

amitiuttarwar added the label Review club on Nov 17, 2020

amitiuttarwar commented at 6:03 pm on November 18, 2020: contributor

light code review ACK fac8e9ac7177bd2dee6b0532fde917aa4b89f651

MarcoFalke commented at 6:35 pm on November 18, 2020: member

I think ignoring the Nth version message is wrong. Since we don’t support actually changing to a new version, disconnecting is the polite reaction

If a previous released version of Core received two version messages, it would ignore them, so this pull is the least change from previous behaviour.

p2p: Ignore non-version msgs before version msg

Sending a non-version message before the initial version message is peer
misbehavior. Though, it seems arbitrary and confusing to disconnect only
after exactly 100 non-version messages. So remove the Misbehaving and
instead rely on the existing disconnect-due-to-handshake-timeout logic.

fad68afcff

p2p: Ignore version msgs after initial version msg

Sending a version message after the intial version message is peer
misbehavior. Though, it seems arbitrary and confusing to disconnect only
after exactly 100 version messages. Duplicate version messages affect us
no different than any other unknown message. So remove the Misbehaving
and ignore the redundant msgs.

faaad1bbac

MarcoFalke force-pushed on Nov 19, 2020

jnewbery commented at 10:39 am on November 19, 2020: member

utACK faaad1bbac46cfeb22654b4c59f0aac7a680c03a

practicalswift commented at 10:56 am on November 19, 2020: contributor

ACK faaad1bbac46cfeb22654b4c59f0aac7a680c03a: patch looks correct

DrahtBot commented at 4:41 am on November 29, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20524 (test: Move MIN_VERSION_SUPPORTED to p2p.py by jnewbery)
#20411 (test: Fix Version Message Deserialization Discrepancy by troygiorshev)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

MarcoFalke merged this on Dec 12, 2020

MarcoFalke closed this on Dec 12, 2020

MarcoFalke deleted the branch on Dec 12, 2020

sidhujag referenced this in commit 6a9cbeee69 on Dec 13, 2020

DrahtBot locked this on Feb 15, 2022

p2p: Treat handshake misbehavior like unknown message #20079

Conflicts