test: Remove message_count #19580

Inspired by this comment on #18638.

message_count is almost completely unused. Additionally, its values are not always consistent with last_message (i.e. we sometimes simultaneously have message_count['foo'] == 1 and 'foo' not in last_message).

This PR removes it and replaces its only use with a new has_verack boolean.

I don't understand why the counts would not be correct, but agree has_verack as boolean is clerer.

I don't understand why the counts would not be correct, but agree has_verack as boolean is clerer.

In the tests in #18638, we do last_message.pop('ping'), after which we simultaneously have message_count['ping'] == 1 and 'ping' not in last_message.

Thanks for the comment, I've edited the PR description to clarify what I mean by "wrong".

I don't think this is an improvement. It's making the P2PConnection class less generic by hard-coding specific message types into the base class.

I also don't think that it's an issue that last_message['foo'] is empty and message_count['foo'] is > 0.

EDIT: This is not a NACK. I don't see the benefit of removing this, but if other people do, I don't feel strongly that it needs to stay.

Code review ACK daad49020427293506d88ae4d0242937232629c7.

message_count was only ever used to track verack so has_verack (or verack_received as it was named in the past) is enough.

It's making the P2PConnection class less generic by hard-coding specific message types into the base class. @jnewbery what do you mean? P2PConnection is not touched.

I don't see the benefit of removing this, but if other people do, I don't feel strongly that it needs to stay.

:+1:

Closing this in favor of #19599

I misunderstood the intended usage of message_count and last_message. Additionally, looking deeper at the code, the intended usage (almost completely) matches the actual usage. The comments just need to be cleaned up, which is done in the above PR.

I now agree with:

I also don't think that it's an issue that last_message['foo'] is empty and message_count['foo'] is > 0.

If people are uncomfortable with unused code, this PR can be reopened but should use 2020-07-remove-message_count-v2 instead. It does away with has_verack.