Page MenuHomePhabricator

Ignore unknown messages before VERACK
AbandonedPublic

Authored by PiRK on Nov 2 2021, 15:35.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Summary

PR description:

This allows for feature negotiation to take place with messages between VERSION and VERACK in the future, without requiring additional software changes to specifically ignore messages for features that are unimplemented by our software.

Additional rationale from the PR review

My thought is that if other software on the network deploys changes similar to BIP 339 in the future (where a new message is included between version and verack) that we don't want to support in our project, then it's easier to ignore all such messages than it is to add support for specifically ignoring each such message as it is proposed.

If we were to overlook someone's optional p2p protocol change and then bumped our protocol version at a later date in support of some other change, we would risk needlessly disconnecting other peers.

Note that if a peer never sends a verack, we still disconnect them after 60 seconds, so I think the only downside from this change is that we'd tolerate 60 seconds worth of garbage from a peer before they disconnect. Given that a peer can waste our bandwidth anyway by sending the verack and spamming us with (eg) bogus transactions, this bandwidth issue seems like an orthogonal concern. If we want to address the bandwidth issue somehow, I think we could do so for both pre-verack and post-verack messages at a later time.

I think it makes sense to ignore unknown messages before VERACK. It's the obvious place to negotiate optional features, and if there is no problem having them after VERACK, I don't see why there should be punishment for it before that point.

We already tolerate up to 99 unsupported messages prior to a verack, so in almost all cases this is not a behavior change. Explicitly dropping the message and continuing rather than incrementing the misbehaving score seems like an improvement.

Note: Bitcoin ABC increased the misbehavior score by 10 rather than just 1 point, so it tolerated only up to 9 unsupported messages. But it does not fundamentally change the rationale.

This is a backport of core#19723

Test Plan

ninja all check-all

Event Timeline

PiRK requested review of this revision.Nov 2 2021, 15:35
Fabien requested changes to this revision.Nov 17 2021, 08:53
Fabien added a subscriber: Fabien.

Core has a use case for this that doesn't stand for us, at least for now. I'm not a fan of increasing the attack surface, even if it's not a known real world issue, if there is no motivation behind it.

This revision now requires changes to proceed.Nov 17 2021, 08:53